Differential D21686 Diff 51668 src/repository/api/ArcanistMercurialAPI.php

Changeset View

Standalone View

View Options

src/repository/api/ArcanistMercurialAPI.php

Show First 20 Lines • Show All 652 Lines • ▼ Show 20 Lines $this->execxLocal(

'addremove -- %Ls', 'addremove -- %Ls',

$paths); $paths);

$this->reloadWorkingCopy(); $this->reloadWorkingCopy();

} }

public function doCommit($message) { public function doCommit($message) {

$tmp_file = new TempFile(); $tmp_file = new TempFile();

Filesystem::writeFile($tmp_file, $message); Filesystem::writeFile($tmp_file, $message);

$this->execxLocal('commit -l %s', $tmp_file); $this->execxLocal('commit --logfile %s', $tmp_file);

$this->reloadWorkingCopy(); $this->reloadWorkingCopy();

} }

public function amendCommit($message = null) { public function amendCommit($message = null) {

$path_statuses = $this->buildUncommittedStatus();

if ($message === null) { if ($message === null) {

if (empty($path_statuses)) {

// If there are no changes to the working directory and the message is

epriestleyUnsubmitted

Done

if (empty($path_statuses)) {

- // If there are changes to the working directory and the message is not

+ // If there are no changes to the working directory and the message is not

// being changed then there's nothing to amend.

(Missing word?)

epriestley: (Missing word?)

// not being changed then there's nothing to amend. Notably Mercurial

// will return an error code if trying to amend a commit with no change

// to the commit metadata or file changes.

return;

}

$message = $this->getCommitMessage('.'); $message = $this->getCommitMessage('.');

} }

$tmp_file = new TempFile(); $tmp_file = new TempFile();

Filesystem::writeFile($tmp_file, $message); Filesystem::writeFile($tmp_file, $message);

if ($this->getMercurialFeature('evolve')) {

$this->execxLocal('amend --logfile %s --', $tmp_file);

try { try {

$this->execxLocal( $this->execxLocal('evolve --all --');

'commit --amend -l %s',

$tmp_file);

} catch (CommandException $ex) { } catch (CommandException $ex) {

if (preg_match('/nothing changed/', $ex->getStdout())) { $this->execxLocal('evolve --abort --');

// NOTE: Mercurial considers it an error to make a no-op amend. Although

// we generally defer to the underlying VCS to dictate behavior, this

// one seems a little goofy, and we use amend as part of various

// workflows under the assumption that no-op amends are fine. If this

// amend failed because it's a no-op, just continue.

} else {

throw $ex; throw $ex;

} }

cspeckmimAuthorUnsubmitted

Done

If there are uncommitted changes then this will pick them up and amend, though amending the uncommitted changes does introduce the possibility of conflicts occurring during the evolve. Reading ArcanistGitAPI:amendCommit() it looks like if there will be conflicts then an error just throws up.

cspeckmim: If there are uncommitted changes then this will pick them up and amend, though amending the…

$this->reloadWorkingCopy();

return;

}

// Get the child nodes of the current changeset.

list($children) = $this->execxLocal(

cspeckmimAuthorUnsubmitted

Done

I just realized this function is also intended to amend uncommitted changes in the working directory, in which case this will fail unexpectedly. I'll figure out how to handle an unclean working directory here. Might have to stash the changes then unstash them after the rebase with (potentially) new commit message, amend, then continue rebasing the child branches.

cspeckmim: I just realized this function is also intended to amend uncommitted changes in the working…

'log --template %s --rev %s --',

'{node} ',

'children(.)');

$child_nodes = array_filter(explode(' ', $children));

// For a head commit we can simply use `commit --amend` for both new commit

// message and amending changes from the working directory.

epriestleyUnsubmitted

Done

I think this is always safe today since $child can never be a branch name, etc, but should use hgsprintf() for robustness against possible future values of $child.

epriestley: I think this is always safe today since `$child` can never be a branch name, etc, but should…

if (empty($child_nodes)) {

$this->execxLocal('commit --amend --logfile %s --', $tmp_file);

} else {

$this->amendNonHeadCommit($child_nodes, $tmp_file);

} }

$this->reloadWorkingCopy(); $this->reloadWorkingCopy();

} }

/**

* Amends a non-head commit with a new message and file changes. This

* strategy is for Mercurial repositories without the evolve extension.

* 1. Run 'arc-amend' which uses Mercurial internals to amend the current

epriestleyUnsubmitted

Done

This may not be worth dealing with just to get away from looking for English-language text in stderr, but it looks like we can trick Mercurial by passing --date now, provided the commit was made at least one second ago. But, obviously, this changes the commit date, which is not faithful to the amend semantics of Mercurial.

My local Mercurial is smart enough to consider it an error ("nothing changed") if we pass --date <the current commit date>, i.e. passing the flag doesn't skip the no-op check, so we can't cheat that way.

I think this is at best a step sideways (rather than forwards) and not worth pursuing, but I would eventually like to find ways around all stderr-English-text-matching.

epriestley: This may not be worth dealing with just to get away from looking for English-language text in…

cspeckmimAuthorUnsubmitted

Done

Yea this unexpected error when it's not really an error is kinda messy. Though I was wondering if maybe we wouldn't be likely to hit this now that above it's checking that we're amending no file changes and the message hasn't changed. I hadn't looked into whether the amend has the same behavior for different message vs. same message.

cspeckmim: Yea this unexpected error when it's not really an error is kinda messy. Though I was wondering…

cspeckmimAuthorUnsubmitted

Done

From a quick test mercurial does not report this error when only the message changes. With the new check above to return early if neither files nor message changed then this is probably fine to remove, from this site at least.

cspeckmim: From a quick test mercurial does not report this error when only the message changes. With the…

* commit with updated message/file-changes. It results in a new commit

* from the right parent

* 2. For each branch from the original commit, rebase onto the new commit,

* removing the original branch. Note that there is potential for this to

* cause a conflict but this is something the user has to address.

* 3. Strip the original commit.

cspeckmimAuthorUnsubmitted

Done

This is what was blowing up before -- I wasn't specifying a commit message so this was launching my text editor then happily moving along to the next part of the dance.

cspeckmim: This is what was blowing up before -- I wasn't specifying a commit message so this was…

* @param array The list of child changesets off the original commit.

* @param file The file containing the new commit message.

private function amendNonHeadCommit($child_nodes, $tmp_file) {

list($current) = $this->execxLocal(

'log --template %s --rev . --',

'{node}');

$argv = array();

foreach ($this->getMercurialExtensionArguments() as $arg) {

$argv[] = $arg;

}

$argv[] = 'arc-amend';

$argv[] = '--logfile';

$argv[] = $tmp_file;

$this->execxLocal('%Ls', $argv);

list($new_commit) = $this->execxLocal(

'log --rev tip --template %s --',

'{node}');

try {

$rebase_args = array(

'--dest',

$new_commit,

epriestleyUnsubmitted

Done

Can we descendants($current) - $current or similar?

epriestley: Can we `descendants($current) - $current` or similar?

cspeckmimAuthorUnsubmitted

Done

Ooh I forgot we could do something like that. I'll try it out, though as I wrote this comment it was optimistic that Mercurial could rebase multiple branches at once.

cspeckmim: Ooh I forgot we could do something like that. I'll try it out, though as I wrote this comment…

cspeckmimAuthorUnsubmitted

Done

From a quick test (I think I set this up the same) Mercurial seems to complain

abort: source and destination form a cycle

I checked using the same revset with log and it's not including $current, so maybe Mercurial just can't handle this? Seems a little odd though. I'll update the comment to mention this though.

cspeckmim: From a quick test (I think I set this up the same) Mercurial seems to complain ``` abort…

cspeckmimAuthorUnsubmitted

Done

Ah there is a way to do this in a single command utilizing --source multiple times as that argument specifically means the specified changeset and its descendants

hg rebase --dest $new_commit --source $child1 --source $child2 --source $child3 --

cspeckmim: Ah there is a way to do this in a single command utilizing `--source` multiple times as that…

);

foreach ($child_nodes as $child) {

$rebase_args[] = '--source';

$rebase_args[] = $child;

}

$this->execxLocal('rebase %Ls --', $rebase_args);

} catch (CommandException $ex) {

$this->execxLocal('rebase --abort --');

throw $ex;

}

$this->execxLocal('--config extensions.strip= strip --rev %s --',

$current);

}

public function getCommitSummary($commit) { public function getCommitSummary($commit) {

if ($commit == 'null') { if ($commit == 'null') {

return pht('(The Empty Void)'); return pht('(The Empty Void)');

} }

list($summary) = $this->execxLocal( list($summary) = $this->execxLocal(

'log --template {desc} --limit 1 --rev %s', 'log --template {desc} --limit 1 --rev %s',

$commit); $commit);

$summary = head(explode("\n", $summary)); $summary = head(explode("\n", $summary));

return trim($summary); return trim($summary);

cspeckmimAuthorUnsubmitted

Done

There's an issue here and I'm trying to investigate the problem but I got a little lost. The saveStash() call ends up throwing an exception due to a null workflow.

EXCEPTION: (Error) Call to a member function getLogEngine() on null at [<arcanist>/src/workflow/ArcanistWorkflow.php:248]
  #0 ArcanistWorkflow::getLogEngine() called at [<arcanist>/src/repository/state/ArcanistMercurialLocalState.php:157]
  #1 ArcanistMercurialLocalState::saveStash() called at [<arcanist>/src/repository/api/ArcanistMercurialAPI.php:777]
  #2 ArcanistMercurialAPI::amendNonHeadCommit(ArcanistDiffWorkflow, array, array, TempFile) called at [<arcanist>/src/repository/api/ArcanistMercurialAPI.php:724]

However I've confirmed that $workflow isn't null and is an instance of ArcanistWorkflow, so I'm not really sure why this is a problem. Any ideas? I confirmed this rebase dance works back before making more appropriate changes to the local state objects, so this only cropped up after trying to set a proper workflow.

cspeckmim: There's an issue here and I'm trying to investigate the problem but I got a little lost. The…

} }

public function resolveBaseCommitRule($rule, $source) { public function resolveBaseCommitRule($rule, $source) {

list($type, $name) = explode(':', $rule, 2); list($type, $name) = explode(':', $rule, 2);

// NOTE: This function MUST return node hashes or symbolic commits (like // NOTE: This function MUST return node hashes or symbolic commits (like

// branch names or the word "tip"), not revsets. This includes ".^" and // branch names or the word "tip"), not revsets. This includes ".^" and

// similar, which a revset, not a symbolic commit identifier. If you return // similar, which a revset, not a symbolic commit identifier. If you return

▲ Show 20 Lines • Show All 73 Lines • ▼ Show 20 Lines switch ($type) {

$rule, $rule,

$source)); $source));

// NOTE: This should be safe because Mercurial doesn't support // NOTE: This should be safe because Mercurial doesn't support

// amend until 2.2. // amend until 2.2.

return $this->getCanonicalRevisionName('.^'); return $this->getCanonicalRevisionName('.^');

} }

break; break;

case 'bookmark': case 'bookmark':

$revset = $revset =

'limit('. 'limit('.

' sort('. ' sort('.

' (ancestors(.) and bookmark() - .) or'. ' (ancestors(.) and bookmark() - .) or'.

' (ancestors(.) - outgoing()), '. ' (ancestors(.) - outgoing()), '.

' -rev),'. ' -rev),'.

'1)'; '1)';

list($err, $bookmark_base) = $this->execManualLocal( list($err, $bookmark_base) = $this->execManualLocal(

'log --template={node} --rev %s', 'log --template={node} --rev %s',

$revset); $revset);

cspeckmimAuthorUnsubmitted

Done

Just noting that I've been seeing this take ~2.75s on my system and realized it's because of outgoing() which has to exchange info with the server to determine what changesets are outgoing. Changing this to draft() results in this finishing in ~200ms however draft() will not return the same results in some edge cases (pulling public commits from a secondary remote and wanting to include them in a diff?).

I'm doing testing from my home against a server running in the office which is why the delay is more noticeable. Probably the best workaround is using a custom rule

.hgrc

[revsetalias]
arcbase = limit(sort( (ancestors(.) and bookmark() - .) or (ancestors(.) - draft()), -rev), 1)

$ arc set-config base "hg:arcbase"

cspeckmim: Just noting that I've been seeing this take ~2.75s on my system and realized it's because of…

cspeckmimAuthorUnsubmitted

Done

Oof this doesn't actually work. Using HGPLAIN=1 seems to disable configured aliases and the revset is too complex to make it through arc's parsing and mercurial's.

cspeckmim: Oof this doesn't actually work. Using `HGPLAIN=1` seems to disable configured aliases and the…

epriestleyUnsubmitted

Not Done

I imagine "base" rules getting a rewrite in the future. Ideally, I'd like to get rid of them completely, use sensible default (I think draft() is a perfectly sensible default in Mercurial) and expect anyone with a bizarre edge case (like Mercurial with multiple remotes) to just put that logic in a helper script and have the helper script provide the base commit to arc diff using something outside the scope of arc, like shell aliases.

epriestley: I imagine "base" rules getting a rewrite in the future. Ideally, I'd like to get rid of them…

if (!$err) { if (!$err) {

$this->setBaseCommitExplanation( $this->setBaseCommitExplanation(

pht( pht(

"it is the first ancestor of %s that either has a bookmark, ". "it is the first ancestor of %s that either has a bookmark, ".

"or is already in the remote and it matched the rule %s in ". "or is already in the remote and it matched the rule %s in ".

"your %s 'base' configuration", "your %s 'base' configuration",

'.', '.',

$rule, $rule,

▲ Show 20 Lines • Show All 254 Lines • Show Last 20 Lines