Changeset View
Standalone View
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) { | ||||||||
| // TODO: If the message isn't being changed and there are no unsaved | |||||||||
Lint: TODO Comment: This comment has a TODO. | |||||||||
| // changes then just return without making any modifications? | |||||||||
| if ($message === null) { | if ($message === null) { | ||||||||
| $message = $this->getCommitMessage('.'); | $message = $this->getCommitMessage('.'); | ||||||||
| } | } | ||||||||
Done Inline Actions
(Missing word?) epriestley: (Missing word?) | |||||||||
| $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); | |||||||||
| $this->execxLocal('evolve --all --'); | |||||||||
| $this->reloadWorkingCopy(); | |||||||||
| return; | |||||||||
| } | |||||||||
| // Handling the amending of a non-head changeset without evolve means some | |||||||||
| // extra rebasing needs to happen. Rebase the current changeset onto the | |||||||||
| // same parent but with a new commit message, then rebase each child branch | |||||||||
| // onto the resulting changeset, and strip the original. | |||||||||
| $hg_analyzer = PhutilBinaryAnalyzer::getForBinary('hg'); | |||||||||
| if ($hg_analyzer->isMercurialTemplatePnodeAvailable()) { | |||||||||
| $template = '{p1.node} {node}'; | |||||||||
| } else { | |||||||||
| $template = '{p1node} {node}'; | |||||||||
| } | |||||||||
| list($current_and_parent) = $this->execxLocal( | |||||||||
| 'log --template %s --rev . --', | |||||||||
| $template); | |||||||||
| $nodes = explode(' ', $current_and_parent); | |||||||||
| $parent = $nodes[0]; | |||||||||
| $current = $nodes[1]; | |||||||||
| // For some reason including "{children % '{node}'}" in the above template | |||||||||
| // unexpectedly prints the current node and not child nodes. If it worked | |||||||||
| // this could be a single log invocation. | |||||||||
| list($children) = $this->execxLocal( | |||||||||
| 'log --template %s --rev %s', | |||||||||
| '{node} ', | |||||||||
| 'children(.)'); | |||||||||
| $child_nodes = array_filter(explode(' ', $children)); | |||||||||
| if (empty($child_nodes)) { | |||||||||
| try { | try { | ||||||||
| $this->execxLocal( | $this->execxLocal('commit --amend --logfile %s --', $tmp_file); | ||||||||
| 'commit --amend -l %s', | |||||||||
| $tmp_file); | |||||||||
| } catch (CommandException $ex) { | } catch (CommandException $ex) { | ||||||||
| if (preg_match('/nothing changed/', $ex->getStdout())) { | if (preg_match('/nothing changed/', $ex->getStdout())) { | ||||||||
| // NOTE: Mercurial considers it an error to make a no-op amend. Although | // NOTE: Mercurial considers it an error to make a no-op amend. | ||||||||
| // we generally defer to the underlying VCS to dictate behavior, this | // Although we generally defer to the underlying VCS to dictate | ||||||||
| // one seems a little goofy, and we use amend as part of various | // behavior, this one seems a little goofy, and we use amend as part | ||||||||
| // workflows under the assumption that no-op amends are fine. If this | // of various workflows under the assumption that no-op amends are | ||||||||
| // amend failed because it's a no-op, just continue. | // fine. If this amend failed because it's a no-op, just continue. | ||||||||
| } else { | } else { | ||||||||
| throw $ex; | throw $ex; | ||||||||
| } | } | ||||||||
| } | } | ||||||||
| $this->reloadWorkingCopy(); | |||||||||
| return; | |||||||||
| } | |||||||||
| // TODO: Stash unsaved changes. | |||||||||
Lint: TODO Comment This comment has a TODO. Lint: TODO Comment: This comment has a TODO. | |||||||||
| try { | |||||||||
| // Mercurial doesn't allow rebasing with a new commit message unless | |||||||||
| // the `--collapse` flag is also specified... It shouldn't matter | |||||||||
| // since we're only rebasing a single commit here. | |||||||||
| $this->execxLocal( | |||||||||
| 'rebase --dest %s --rev . --keep --collapse --logfile %s --', | |||||||||
| $parent, | |||||||||
| $tmp_file); | |||||||||
| } catch (CommandException $ex) { | |||||||||
| $this->execxLocal('rebase --abort'); | |||||||||
| throw $ex; | |||||||||
| } | |||||||||
| list($new_commit) = $this->execxLocal( | |||||||||
| 'log --rev tip --template %s --', | |||||||||
| '{node}'); | |||||||||
Done Inline ActionsI 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… | |||||||||
Done Inline ActionsIf 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… | |||||||||
| // TODO: Un-stash unsaved changes, amend to the new commit, get new commit. | |||||||||
Lint: TODO Comment This comment has a TODO. Lint: TODO Comment: This comment has a TODO. | |||||||||
| try { | |||||||||
| foreach ($child_nodes as $child) { | |||||||||
| // descendants(rev) will also include rev itself which is why this | |||||||||
| // can't just use a single rebase of descendants($current). | |||||||||
| $revset = 'descendants('.$child.')'; | |||||||||
epriestleyUnsubmitted Done Inline ActionsI 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… | |||||||||
| $this->execxLocal( | |||||||||
| 'rebase --dest %s --rev %s --', | |||||||||
| $new_commit, | |||||||||
| $revset); | |||||||||
| } | |||||||||
| } catch (CommandException $ex) { | |||||||||
| $this->execxLocal('rebase --abort'); | |||||||||
| throw $ex; | |||||||||
| } | |||||||||
| $this->execxLocal('--config extensions.strip= strip %s', $current); | |||||||||
| $this->reloadWorkingCopy(); | $this->reloadWorkingCopy(); | ||||||||
| } | } | ||||||||
| public function getCommitSummary($commit) { | public function getCommitSummary($commit) { | ||||||||
Done Inline ActionsThere'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… | |||||||||
| 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)); | ||||||||
Done Inline ActionsThis 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… | |||||||||
| return trim($summary); | return trim($summary); | ||||||||
| } | } | ||||||||
| public function resolveBaseCommitRule($rule, $source) { | public function resolveBaseCommitRule($rule, $source) { | ||||||||
| list($type, $name) = explode(':', $rule, 2); | list($type, $name) = explode(':', $rule, 2); | ||||||||
Done Inline ActionsThis 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… | |||||||||
Done Inline ActionsYea 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… | |||||||||
Done Inline ActionsFrom 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… | |||||||||
| // 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 | ||||||||
| // a revset it will be escaped later and looked up literally. | // a revset it will be escaped later and looked up literally. | ||||||||
| switch ($type) { | switch ($type) { | ||||||||
| case 'hg': | case 'hg': | ||||||||
| $matches = null; | $matches = null; | ||||||||
| if (preg_match('/^gca\((.+)\)$/', $name, $matches)) { | if (preg_match('/^gca\((.+)\)$/', $name, $matches)) { | ||||||||
| list($err, $merge_base) = $this->execManualLocal( | list($err, $merge_base) = $this->execManualLocal( | ||||||||
| 'log --template={node} --rev %s', | 'log --template={node} --rev %s', | ||||||||
| sprintf('ancestor(., %s)', $matches[1])); | sprintf('ancestor(., %s)', $matches[1])); | ||||||||
| if (!$err) { | if (!$err) { | ||||||||
| $this->setBaseCommitExplanation( | $this->setBaseCommitExplanation( | ||||||||
| pht( | pht( | ||||||||
| "it is the greatest common ancestor of '%s' and %s, as ". | "it is the greatest common ancestor of '%s' and %s, as ". | ||||||||
| "specified by '%s' in your %s 'base' configuration.", | "specified by '%s' in your %s 'base' configuration.", | ||||||||
| $matches[1], | $matches[1], | ||||||||
| '.', | '.', | ||||||||
| $rule, | $rule, | ||||||||
Done Inline ActionsCan we descendants($current) - $current or similar? epriestley: Can we `descendants($current) - $current` or similar? | |||||||||
Done Inline ActionsOoh 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… | |||||||||
Done Inline ActionsFrom 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… | |||||||||
Done Inline ActionsAh 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… | |||||||||
| $source)); | $source)); | ||||||||
| return trim($merge_base); | return trim($merge_base); | ||||||||
| } | } | ||||||||
| } else { | } else { | ||||||||
| list($err, $commit) = $this->execManualLocal( | list($err, $commit) = $this->execManualLocal( | ||||||||
| 'log --template {node} --rev %s', | 'log --template {node} --rev %s', | ||||||||
| hgsprintf('%s', $name)); | hgsprintf('%s', $name)); | ||||||||
| ▲ Show 20 Lines • Show All 48 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); | ||||||||
Done Inline ActionsJust 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… | |||||||||
Done Inline ActionsOof 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… | |||||||||
Not Done Inline ActionsI 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 | |||||||||
This comment has a TODO.