Changeset View
Standalone View
src/land/engine/ArcanistMercurialLandEngine.php
| <?php | <?php | ||||
| final class ArcanistMercurialLandEngine | final class ArcanistMercurialLandEngine | ||||
| extends ArcanistLandEngine { | extends ArcanistLandEngine { | ||||
| private $ontoBranchMarker; | private $ontoBranchMarker; | ||||
| private $ontoMarkers; | private $ontoMarkers; | ||||
| private $rebasedActiveCommit; | |||||
| protected function getDefaultSymbols() { | protected function getDefaultSymbols() { | ||||
| $api = $this->getRepositoryAPI(); | $api = $this->getRepositoryAPI(); | ||||
| $log = $this->getLogEngine(); | $log = $this->getLogEngine(); | ||||
| // TODO: In Mercurial, you normally can not create a branch and a bookmark | // TODO: In Mercurial, you normally can not create a branch and a bookmark | ||||
| // with the same name. However, you can fetch a branch or bookmark from | // with the same name. However, you can fetch a branch or bookmark from | ||||
| // a remote that has the same name as a local branch or bookmark of the | // a remote that has the same name as a local branch or bookmark of the | ||||
| // other type, and end up with a local branch and bookmark with the same | // other type, and end up with a local branch and bookmark with the same | ||||
| ▲ Show 20 Lines • Show All 639 Lines • ▼ Show 20 Lines | private function fetchTarget(ArcanistLandTarget $target) { | ||||
| if ($target_marker->isBranch()) { | if ($target_marker->isBranch()) { | ||||
| $err = $this->newPassthru( | $err = $this->newPassthru( | ||||
| 'pull --branch %s -- %s', | 'pull --branch %s -- %s', | ||||
| $target->getRef(), | $target->getRef(), | ||||
| $target->getRemote()); | $target->getRemote()); | ||||
| } else { | } else { | ||||
| // NOTE: This may have side effects: | // NOTE: This may have side effects: | ||||
| // | // | ||||
| // - It can create a "bookmark@remote" bookmark if there is a local | // - It can create a "bookmark@remote" bookmark if there is a local | ||||
| // bookmark with the same name that is not an ancestor. | // bookmark with the same name that is not an ancestor. | ||||
| // - It can create an arbitrary number of other bookmarks. | // - It can create an arbitrary number of other bookmarks. | ||||
| // | // | ||||
| // Since these seem to generally be intentional behaviors in Mercurial, | // Since these seem to generally be intentional behaviors in Mercurial, | ||||
| // and should theoretically be familiar to Mercurial users, just accept | // and should theoretically be familiar to Mercurial users, just accept | ||||
| // them as the cost of doing business. | // them as the cost of doing business. | ||||
cspeckmim: Aha | |||||
| $err = $this->newPassthru( | $err = $this->newPassthru( | ||||
| 'pull --bookmark %s -- %s', | 'pull --bookmark %s -- %s', | ||||
| $target->getRef(), | $target->getRef(), | ||||
| $target->getRemote()); | $target->getRemote()); | ||||
| } | } | ||||
| // NOTE: It's possible that between the time we ran "ls-markers" and the | // NOTE: It's possible that between the time we ran "ls-markers" and the | ||||
| Show All 12 Lines | final class ArcanistMercurialLandEngine | ||||
| protected function selectCommits($into_commit, array $symbols) { | protected function selectCommits($into_commit, array $symbols) { | ||||
| assert_instances_of($symbols, 'ArcanistLandSymbol'); | assert_instances_of($symbols, 'ArcanistLandSymbol'); | ||||
| $api = $this->getRepositoryAPI(); | $api = $this->getRepositoryAPI(); | ||||
| $commit_map = array(); | $commit_map = array(); | ||||
| foreach ($symbols as $symbol) { | foreach ($symbols as $symbol) { | ||||
| $symbol_commit = $symbol->getCommit(); | $symbol_commit = $symbol->getCommit(); | ||||
| $template = '{node}-{parents}-'; | $template = '{node}-{parents % \'{node} \'}-{desc|firstline}\\n'; | ||||
With this change it seems to now select the proper commits when there are multiple local commits as part of a diff. I got to use that fancy template format you taught me! Here is what arcanist displays this to the user as INTO COMMIT Preparing merge into "master" from remote "default", at commit "e84517035151".
LANDING These changes will land:
* D22361 test commit
8c1f1f2610ee test commit
fba13f95233f test commit
>>> Land these changes? [y/N/?] y(this sample output would be better if I had any more creativity for my test commit messages and diff titles) This is what it previously displayed INTO COMMIT Preparing merge into "master" from remote "default", at commit "e84517035151".
LANDING These changes will land:
* D22361 test commit
fba13f95233f 8c1f1f2610ee4d90d871df01e5383387f86efa58-23:999d8d1f780f -
>>> Land these changes? [y/N/?] yMy workflow I use most of the time involves locally amending commits so I only ever have a single commit per diff which is why I probably never ran into this. Arcanist's display when selecting a single commit was also a little less suspect INTO COMMIT Preparing merge into "master" from remote "default", at commit "74de4ab3c972".
LANDING These changes will land:
* D22359 test commit
a33f5df2c48a
>>> Land these changes? [y/N/?] ycspeckmim: With this change it seems to now select the proper commits when there are multiple local… | |||||
| // The returned array of commits is expected to be ordered by newest | |||||
| // to oldest commits. Use 'reverse()' in the template so the output | |||||
| // is ordered with newest commit as the first line. | |||||
| if ($into_commit === null) { | if ($into_commit === null) { | ||||
| list($commits) = $api->execxLocal( | list($commits) = $api->execxLocal( | ||||
| 'log --rev %s --template %s --', | 'log --rev %s --template %s --', | ||||
| hgsprintf('reverse(ancestors(%s))', $into_commit), | hgsprintf('reverse(ancestors(%s))', $into_commit), | ||||
| $template); | $template); | ||||
| } else { | } else { | ||||
| list($commits) = $api->execxLocal( | list($commits) = $api->execxLocal( | ||||
| 'log --rev %s --template %s --', | 'log --rev %s --template %s --', | ||||
| ▲ Show 20 Lines • Show All 73 Lines • ▼ Show 20 Lines | protected function executeMerge(ArcanistLandCommitSet $set, $into_commit) { | ||||
| // TODO: Add a Mercurial version check requiring 2.1.1 or newer. | // TODO: Add a Mercurial version check requiring 2.1.1 or newer. | ||||
| $api->execxLocal( | $api->execxLocal( | ||||
| 'update --rev %s', | 'update --rev %s', | ||||
| hgsprintf('%s', $into_commit)); | hgsprintf('%s', $into_commit)); | ||||
| $commits = $set->getCommits(); | $commits = $set->getCommits(); | ||||
| $min_commit = last($commits)->getHash(); | // confirmCommits() reverses the order of the commits as they're ordered | ||||
| $max_commit = head($commits)->getHash(); | // above in selectCommits(), now the head of the list is the oldest and | ||||
| // last is the newest. | |||||
| $newest_commit = last($commits)->getHash(); | |||||
| $oldest_commit = head($commits)->getHash(); | |||||
I think this is totally fine, but I was using min and max in a topological sense (i.e., the max commit has no descendants in the range, and the min commit has no ancestors in the range). In Mercurial this is a bit ambiguous (the local changeset IDs could be in whatever order, I think, and is probably the thing you'd think of first as far as max and min go). In both version control systems, the "max" commit may not be the "newest" commit because all commit dates are user-controlled and a commit's commit/author/etc dates may be before those of its ancestors. This is rare (and always indicates some kind of anomalous user behavior) but comes up often enough (e.g., see T11537) that I've tried to put myself in the frame of mind that none of the dates on commits are guaranteed to tell you anything about commit ordering. If you ever use commit dates to impose ordering, you can end up with some nonsense buts like commits someone accidentally "authored" in 2099 being pinned to the top of a feed forever. I don't know of any concise, well-understood term for "topological maximum". "Newest" is probably less opaque than "max". epriestley: I think this is totally fine, but I was using `min` and `max` in a topological sense (i.e., the… | |||||
I wasn't entirely sure about the renaming I was doing for the exact reasons you mentioned. I've been looking at commenting other parts of ArcanistLandEngine with some of my learnings here and clarifying in what order things are processed and describing things concisely is difficult. Yesterday I was thinking about with using the term ancestor and descendant instead.
I think this description is very useful. I think I'm going to make some updates to naming and/or comments, possibly changing back to min and max since I am also hesitant about including chronological terms which might be more confusing in the future. cspeckmim: I wasn't entirely sure about the renaming I was doing for the exact reasons you mentioned. I've… | |||||
| $revision_ref = $set->getRevisionRef(); | $revision_ref = $set->getRevisionRef(); | ||||
| $commit_message = $revision_ref->getCommitMessage(); | $commit_message = $revision_ref->getCommitMessage(); | ||||
| // If we're landing "--onto" a branch, set that as the branch marker | // If we're landing "--onto" a branch, set that as the branch marker | ||||
| // before creating the new commit. | // before creating the new commit. | ||||
| // TODO: We could skip this if we know that the "$into_commit" already | // TODO: We could skip this if we know that the "$into_commit" already | ||||
| // has the right branch, which it will if we created it. | // has the right branch, which it will if we created it. | ||||
| $branch_marker = $this->ontoBranchMarker; | $branch_marker = $this->ontoBranchMarker; | ||||
| if ($branch_marker) { | if ($branch_marker) { | ||||
| $api->execxLocal('branch -- %s', $branch_marker->getName()); | $api->execxLocal('branch -- %s', $branch_marker->getName()); | ||||
| } | } | ||||
| try { | try { | ||||
| $argv = array(); | $argv = array(); | ||||
| $argv[] = '--dest'; | $argv[] = '--dest'; | ||||
| $argv[] = hgsprintf('%s', $into_commit); | $argv[] = hgsprintf('%s', $into_commit); | ||||
| $argv[] = '--rev'; | $argv[] = '--rev'; | ||||
| $argv[] = hgsprintf('%s..%s', $min_commit, $max_commit); | $argv[] = hgsprintf('%s..%s', $oldest_commit, $newest_commit); | ||||
I'm not totally positive this is correct, but I tested landing when there's a single commit for the diff and two commits for the diff and the resulting behavior was correct. cspeckmim: I'm not totally positive this is correct, but I tested landing when there's a single commit for… | |||||
| $argv[] = '--logfile'; | $argv[] = '--logfile'; | ||||
| $argv[] = '-'; | $argv[] = '-'; | ||||
| $argv[] = '--keep'; | $argv[] = '--keep'; | ||||
| $argv[] = '--collapse'; | $argv[] = '--collapse'; | ||||
| $future = $api->execFutureLocal('rebase %Ls', $argv); | $future = $api->execFutureLocal('rebase %Ls', $argv); | ||||
| $future->write($commit_message); | $future->write($commit_message); | ||||
| $future->resolvex(); | $future->resolvex(); | ||||
| } catch (CommandException $ex) { | } catch (CommandException $ex) { | ||||
| // TODO | // Aborting the rebase should restore the same state prior to running the | ||||
| // $api->execManualLocal('rebase --abort'); | // rebase command. | ||||
| $api->execManualLocal('rebase --abort'); | |||||
I originally uncommented this, largely because of the issue my coworker was running into and the problem manifests as the rebase operation being in a partial state, so aborting it seems appropriate. However when I ran into the min..max vs. max..min issue the result was that the rebase just exited because there was nothing to do so this --abort resulted in a failure~ At the moment I'm leaning towards still running rebase --abort here (and the other location) as when these uncommon scenarios hit the working directory is in a slightly cleaner state. If there was a way to run the rebase --abort and discard the result of that it would probably be best. cspeckmim: I originally uncommented this, largely because of the issue my coworker was running into and… | |||||
(I expect this should do that, since it's using the "manual" call. Maybe there's another callsite not using the "manual" flavor.) epriestley: (I expect this should do that, since it's using the "manual" call. Maybe there's another… | |||||
Ah I probably saw the mercurial error output and assumed an exception was thrown when one actually wasn't. My comment about "discard the result" was probably focused around hiding that output. It looks like ExecFuture might be able to do something like that with setStderrSizeLimit(0) though I'm not too worried about it. cspeckmim: >>the result was that the rebase just exited because there was nothing to do so this `--abort`… | |||||
| throw $ex; | throw $ex; | ||||
| } | } | ||||
| // Find all the bookmarks which pointed at commits we just rebased, and | // Find all the bookmarks which pointed at commits we just rebased, and | ||||
| // put them back the way they were before rebasing moved them. We aren't | // put them back the way they were before rebasing moved them. We aren't | ||||
| // deleting the old commits yet and don't want to move the bookmarks. | // deleting the old commits yet and don't want to move the bookmarks. | ||||
| $obsolete_map = array(); | $obsolete_map = array(); | ||||
| Show All 12 Lines | foreach ($bookmark_refs as $bookmark_ref) { | ||||
| 'bookmark --force --rev %s -- %s', | 'bookmark --force --rev %s -- %s', | ||||
| $bookmark_hash, | $bookmark_hash, | ||||
| $bookmark_ref->getName()); | $bookmark_ref->getName()); | ||||
| } | } | ||||
| list($stdout) = $api->execxLocal('log --rev tip --template %s', '{node}'); | list($stdout) = $api->execxLocal('log --rev tip --template %s', '{node}'); | ||||
| $new_cursor = trim($stdout); | $new_cursor = trim($stdout); | ||||
| // If any of the commits that were rebased was the active commit before the | |||||
| // workflow started, track the new commit so it can be used as the working | |||||
| // directory after the land has succeeded. | |||||
| if (isset($obsolete_map[$this->getLocalState()->getLocalCommit()])) { | |||||
| $this->rebasedActiveCommit = $new_cursor; | |||||
| } | |||||
| return $new_cursor; | return $new_cursor; | ||||
| } | } | ||||
| protected function pushChange($into_commit) { | protected function pushChange($into_commit) { | ||||
| $api = $this->getRepositoryAPI(); | $api = $this->getRepositoryAPI(); | ||||
| list($head, $body, $tail) = $this->newPushCommands($into_commit); | list($head, $body, $tail_pass, $tail_fail) = $this->newPushCommands( | ||||
| $into_commit); | |||||
| foreach ($head as $command) { | foreach ($head as $command) { | ||||
| $api->execxLocal('%Ls', $command); | $api->execxLocal('%Ls', $command); | ||||
| } | } | ||||
| try { | try { | ||||
| foreach ($body as $command) { | foreach ($body as $command) { | ||||
| $err = $this->newPassthru('%Ls', $command); | $err = $this->newPassthru('%Ls', $command); | ||||
| if ($err) { | if ($err) { | ||||
| throw new ArcanistLandPushFailureException( | throw new ArcanistLandPushFailureException( | ||||
| pht( | pht( | ||||
| 'Push failed! Fix the error and run "arc land" again.')); | 'Push failed! Fix the error and run "arc land" again.')); | ||||
| } | } | ||||
| } | } | ||||
| } finally { | |||||
| foreach ($tail as $command) { | foreach ($tail_pass as $command) { | ||||
| $api->execxLocal('%Ls', $command); | |||||
| } | |||||
| } catch (Exception $ex) { | |||||
| foreach ($tail_fail as $command) { | |||||
| $api->execxLocal('%Ls', $command); | |||||
| } | |||||
| throw $ex; | |||||
| } catch (Throwable $ex) { | |||||
| foreach ($tail_fail as $command) { | |||||
| $api->execxLocal('%Ls', $command); | $api->execxLocal('%Ls', $command); | ||||
| } | } | ||||
| throw $ex; | |||||
| } | } | ||||
| } | } | ||||
| private function newPushCommands($into_commit) { | private function newPushCommands($into_commit) { | ||||
| $api = $this->getRepositoryAPI(); | $api = $this->getRepositoryAPI(); | ||||
| $head_commands = array(); | $head_commands = array(); | ||||
| $body_commands = array(); | $body_commands = array(); | ||||
| $tail_commands = array(); | $tail_pass_commands = array(); | ||||
| $tail_fail_commands = array(); | |||||
| $bookmarks = array(); | $bookmarks = array(); | ||||
| foreach ($this->ontoMarkers as $onto_marker) { | foreach ($this->ontoMarkers as $onto_marker) { | ||||
| if (!$onto_marker->isBookmark()) { | if (!$onto_marker->isBookmark()) { | ||||
| continue; | continue; | ||||
| } | } | ||||
| $bookmarks[] = $onto_marker; | $bookmarks[] = $onto_marker; | ||||
| } | } | ||||
| // If we're pushing to bookmarks, move all the bookmarks we want to push | // If we're pushing to bookmarks, move all the bookmarks we want to push | ||||
| // to the merge commit. (There doesn't seem to be any way to specify | // to the merge commit. (There doesn't seem to be any way to specify | ||||
| // "push commit X as bookmark Y" in Mercurial.) | // "push commit X as bookmark Y" in Mercurial.) | ||||
| $restore = array(); | $delete_bookmarks = array(); | ||||
| $restore_bookmarks = array(); | |||||
| if ($bookmarks) { | if ($bookmarks) { | ||||
| $markers = $api->newMarkerRefQuery() | $markers = $api->newMarkerRefQuery() | ||||
| ->withNames(mpull($bookmarks, 'getName')) | ->withNames(mpull($bookmarks, 'getName')) | ||||
| ->withMarkerTypes(array(ArcanistMarkerRef::TYPE_BOOKMARK)) | ->withMarkerTypes(array(ArcanistMarkerRef::TYPE_BOOKMARK)) | ||||
| ->execute(); | ->execute(); | ||||
| $markers = mpull($markers, 'getCommitHash', 'getName'); | $markers = mpull($markers, 'getCommitHash', 'getName'); | ||||
| foreach ($bookmarks as $bookmark) { | foreach ($bookmarks as $bookmark) { | ||||
| $bookmark_name = $bookmark->getName(); | $bookmark_name = $bookmark->getName(); | ||||
| $old_position = idx($markers, $bookmark_name); | $old_position = idx($markers, $bookmark_name); | ||||
| $new_position = $into_commit; | $new_position = $into_commit; | ||||
| if ($old_position === $new_position) { | if ($old_position === $new_position) { | ||||
| $delete_bookmarks[] = $bookmark_name; | |||||
| continue; | continue; | ||||
| } | } | ||||
| $head_commands[] = array( | $head_commands[] = array( | ||||
| 'bookmark', | 'bookmark', | ||||
| '--force', | '--force', | ||||
| '--rev', | '--rev', | ||||
| hgsprintf('%s', $api->getDisplayHash($new_position)), | hgsprintf('%s', $api->getDisplayHash($new_position)), | ||||
| '--', | '--', | ||||
| $bookmark_name, | $bookmark_name, | ||||
| ); | ); | ||||
| $api->execxLocal( | $api->execxLocal( | ||||
| 'bookmark --force --rev %s -- %s', | 'bookmark --force --rev %s -- %s', | ||||
| hgsprintf('%s', $new_position), | hgsprintf('%s', $new_position), | ||||
| $bookmark_name); | $bookmark_name); | ||||
| $restore[$bookmark_name] = $old_position; | $restore_bookmarks[$bookmark_name] = $old_position; | ||||
| } | } | ||||
| } | } | ||||
| // Now, prepare the actual push. | // Now, prepare the actual push. | ||||
| $argv = array(); | $argv = array(); | ||||
| $argv[] = 'push'; | $argv[] = 'push'; | ||||
| Show All 10 Lines | if ($bookmarks) { | ||||
| $argv[] = hgsprintf('%s', $into_commit); | $argv[] = hgsprintf('%s', $into_commit); | ||||
| } | } | ||||
| $argv[] = '--'; | $argv[] = '--'; | ||||
| $argv[] = $this->getOntoRemote(); | $argv[] = $this->getOntoRemote(); | ||||
| $body_commands[] = $argv; | $body_commands[] = $argv; | ||||
| // Finally, restore the bookmarks. | // Finally, clean up the bookmarks. | ||||
| foreach ($restore as $bookmark_name => $old_position) { | // Delete all bookmarks in a single command rather than individually. | ||||
| $tail = array(); | if ($delete_bookmarks) { | ||||
| $tail[] = 'bookmark'; | $tail = array( | ||||
| 'bookmark', | |||||
| '--delete', | |||||
| '--', | |||||
| ); | |||||
| foreach ($delete_bookmarks as $bookmark_name) { | |||||
| $tail[] = $bookmark_name; | |||||
| } | |||||
I tested out this case of landing onto master while not having that bookmark locally. This doesn't end up deleting the markers and I'm pretty sure it's because of other newMarkerRefQuery() or possibly as part of fetchTarget(). Assuming I'm correctly interpreting what the intent of this then it's probably okay to just remove this code which will simplify things a bit. cspeckmim: I tested out this case of landing onto `master` while not having that bookmark locally. This… | |||||
I'm possibly a little bit lost here, but assuming I'm getting this right: I think it's reasonable to expect that landing onto a bookmark which you don't have locally will create that bookmark locally. The equivalent behavior doesn't happen under git (you can land onto a remote ref without creating a corresponding local ref) but we can't really do the thing Git does in Mercurial, and "give you the bookmark" seems reasonable/expected/consistent with what hg pull does by default. Although we could perhaps imagine cases where this matters, I think these workflows are all silly (in Git, too) and they're only worth "getting right" (i.e., executing with the smallest possible set of side effects) when we don't have to work unreasonably hard to minimize sensible, VCS-default side effects. It seems like way more work than it's worth to try to keep the local working copy clean of bookmarks you land onto. epriestley: I'm possibly a little bit lost here, but assuming I'm getting this right:
I think it's… | |||||
Yea that makes sense, thank you for adding clarifying details. I'll remove this extra code for deleting the new bookmarks to simplify this code some. cspeckmim: Yea that makes sense, thank you for adding clarifying details. I'll remove this extra code for… | |||||
| if ($old_position === null) { | // Do this regardless of success or failure. | ||||
| $tail[] = '--delete'; | $tail_pass_commands[] = $tail; | ||||
| } else { | $tail_fail_commands[] = $tail; | ||||
| $tail[] = '--force'; | |||||
| $tail[] = '--rev'; | |||||
| $tail[] = hgsprintf('%s', $api->getDisplayHash($old_position)); | |||||
| } | } | ||||
| $tail[] = '--'; | // Mercurial allows pulling multiple bookmarks in a single command which | ||||
| // will be faster if we're landing onto multiple bookmarks. | |||||
| if ($restore_bookmarks) { | |||||
| $tail = array( | |||||
| 'pull', | |||||
| ); | |||||
| foreach ($restore_bookmarks as $bookmark_name => $old_position) { | |||||
| // Instead of restoring the previous state, assume landing onto | |||||
| // bookmarks is supposed to be synced with the remote. Pull the latest | |||||
| // state of these bookmarks. | |||||
| $tail[] = '--bookmark'; | |||||
| $tail[] = $bookmark_name; | $tail[] = $bookmark_name; | ||||
| $tail_commands[] = $tail; | // Unable to move multiple bookmarks in a single command, so add | ||||
| // separate commands for each individual bookmark. | |||||
| $tail_fail_commands[] = array( | |||||
| 'bookmark', | |||||
| '--force', | |||||
| '--rev', | |||||
| hgsprintf('%s', $api->getDisplayHash($old_position)), | |||||
| ); | |||||
| } | |||||
| if ($tail) { | |||||
| $tail_pass_commands[] = $tail; | |||||
| } | |||||
| } | } | ||||
| return array( | return array( | ||||
| $head_commands, | $head_commands, | ||||
| $body_commands, | $body_commands, | ||||
| $tail_commands, | $tail_pass_commands, | ||||
| $tail_fail_commands, | |||||
| ); | ); | ||||
| } | } | ||||
| protected function cascadeState(ArcanistLandCommitSet $set, $into_commit) { | protected function cascadeState(ArcanistLandCommitSet $set, $into_commit) { | ||||
| $api = $this->getRepositoryAPI(); | $api = $this->getRepositoryAPI(); | ||||
| $log = $this->getLogEngine(); | $log = $this->getLogEngine(); | ||||
| // This has no effect when we're executing a merge strategy. | // This has no effect when we're executing a merge strategy. | ||||
| if (!$this->isSquashStrategy()) { | if (!$this->isSquashStrategy()) { | ||||
| return; | return; | ||||
| } | } | ||||
| $old_commit = last($set->getCommits())->getHash(); | $old_commit = last($set->getCommits())->getHash(); | ||||
| $new_commit = $into_commit; | $new_commit = $into_commit; | ||||
| list($output) = $api->execxLocal( | list($output) = $api->execxLocal( | ||||
| 'log --rev %s --template %s', | 'log --rev %s --template %s', | ||||
| hgsprintf('children(%s)', $old_commit), | hgsprintf('children(%s)', $old_commit), | ||||
| '{node}\n'); | '{node}\n'); | ||||
I was just reviewing this in passing, should the backslash here be double-escaped? For a test case I think this would involve setting up multiple dependent revisions and landing the top most one, seeing them all get landed? cspeckmim: I was just reviewing this in passing, should the backslash here be double-escaped? For a test… | |||||
I only have a minute between baby-events, but \n means "literal backslash, literal n" when the string uses single quotes. The syntax highlighter is arguably misleading about this. It would perhaps be more correct in some purist sense to write this as \\n, which has the same value, but the argument would look something like "there are two totally equivalent ways to specify this; from a purity standpoint we'd prefer to use only one way for consistency; \\n is the less-ambiguous specification (and has the same behavior in both single and double quotes) and thus is reasonable to prefer". epriestley@orbital ~/dev/phabricator $ cat quotes.php <?php echo 'node\n'; echo "\n"; echo 'node\\n'; echo "\n"; epriestley@orbital ~/dev/phabricator $ php -f quotes.php node\n node\n epriestley: > I was just reviewing this in passing, should the backslash here be double-escaped?
I only… | |||||
I think I was starting to catch on to the single vs. double quotes though I wasn't aware \\n is the same as \n in single quotes. I think I'm also getting tripped up with what value we need passed to the template - a literal newline or the individual \ and n characters, which when I say it aloud it's pretty clear we don't want to be passing a literal newline. Using the appropriate syntax when testing with bash also adds some fun to interpreting everything.
 cspeckmim: >`\n` means "literal backslash, literal n" **when the string uses single quotes**
I think I was… | |||||
| $child_hashes = phutil_split_lines($output, false); | $child_hashes = phutil_split_lines($output, false); | ||||
| foreach ($child_hashes as $child_hash) { | foreach ($child_hashes as $child_hash) { | ||||
| if (!strlen($child_hash)) { | if (!strlen($child_hash)) { | ||||
| continue; | continue; | ||||
| } | } | ||||
| // TODO: If the only heads which are descendants of this child will | // TODO: If the only heads which are descendants of this child will | ||||
| // be deleted, we can skip this rebase? | // be deleted, we can skip this rebase? | ||||
| try { | try { | ||||
| $api->execxLocal( | $api->execxLocal( | ||||
| 'rebase --source %s --dest %s --keep --keepbranches', | 'rebase --source %s --dest %s --keep --keepbranches', | ||||
| $child_hash, | $child_hash, | ||||
| $new_commit); | $new_commit); | ||||
| } catch (CommandException $ex) { | } catch (CommandException $ex) { | ||||
| // TODO: Recover state. | // Aborting the rebase should restore the same state prior to running | ||||
| // the rebase command. | |||||
| $api->execManualLocal('rebase --abort'); | |||||
| throw $ex; | throw $ex; | ||||
| } | } | ||||
| } | } | ||||
| } | } | ||||
| protected function pruneBranches(array $sets) { | protected function pruneBranches(array $sets) { | ||||
| assert_instances_of($sets, 'ArcanistLandCommitSet'); | assert_instances_of($sets, 'ArcanistLandCommitSet'); | ||||
| ▲ Show 20 Lines • Show All 71 Lines • ▼ Show 20 Lines | if ($api->getMercurialFeature('evolve')) { | ||||
| $rev_set); | $rev_set); | ||||
| } | } | ||||
| } | } | ||||
| protected function reconcileLocalState( | protected function reconcileLocalState( | ||||
| $into_commit, | $into_commit, | ||||
| ArcanistRepositoryLocalState $state) { | ArcanistRepositoryLocalState $state) { | ||||
| // TODO: For now, just leave users wherever they ended up. | $api = $this->getRepositoryAPI(); | ||||
| // If the starting working state was not part of land process just update | |||||
| // to that original working state. | |||||
| if (!$this->rebasedActiveCommit) { | |||||
Actually I could probably add a hg update $this->getLocalState()->getLocalCommit() here so that landing from an unrelated commit would leave you back at that commit cspeckmim: Actually I could probably add a `hg update $this->getLocalState()->getLocalCommit()` here so… | |||||
Incredibly minor quibble, but this test should probably be an explicit if ($this->rebasedActiveCommit === null). This particular test can't really get the wrong result, but string logic operators in PHP have some "gotchas" where they're uncomfortably permissive (see also T2312) and it's better to never let your guard down. In particular, if you have the obviously-a-real-value commit hash "0e482192" and perform this comparison: if ($commit == 0) ... ...PHP will cast the string "0e482192" to an integer, interpret it as "0x10^482192" (i.e., "e" means exponentiation in engineering notation), which has value 0, and it will pass the conditional. epriestley: Incredibly minor quibble, but this test should probably be an explicit `if ($this… | |||||
I will update this.
o_o This definitely sounds like something I would only uncover through a terrible bug hunt cspeckmim: I will update this.
>...PHP will cast the string "0e482192" to an integer, interpret it as… | |||||
| $update_marker = $this->getLocalState()->getLocalCommit(); | |||||
| if ($this->getLocalState()->getLocalBookmark() !== null) { | |||||
| $update_marker = $this->getLocalState()->getLocalBookmark(); | |||||
| } | |||||
| $api->execxLocal( | |||||
| 'update -- %s', | |||||
| $update_marker); | |||||
| $state->discardLocalState(); | |||||
| return; | |||||
| } | |||||
| // If the working state was landed into multiple destinations then the | |||||
| // resulting working state is ambiguous. | |||||
| if (count($this->ontoMarkers) != 1) { | |||||
| $state->discardLocalState(); | |||||
| return; | |||||
| } | |||||
| // Get the current state of bookmarks | |||||
| $bookmark_refs = $api->newMarkerRefQuery() | |||||
| ->withMarkerTypes( | |||||
| array( | |||||
| ArcanistMarkerRef::TYPE_BOOKMARK, | |||||
| )) | |||||
| ->execute(); | |||||
| $onto_marker = $this->ontoMarkers[0]->getName(); | |||||
| $update_marker = $this->rebasedActiveCommit; | |||||
| // Find any bookmarks which exist on the commit which is the result of the | |||||
| // starting working directory's rebase. If any of those bookmarks are also | |||||
| // the destination marker then we use that bookmark as the update in order | |||||
| // for it to become active. | |||||
| $onto_markers = mpull($this->ontoMarkers, null, 'getName'); | |||||
| foreach ($bookmark_refs as $bookmark_ref) { | |||||
| if ($bookmark_ref->getCommitHash() == $this->rebasedActiveCommit && | |||||
| $bookmark_ref->getName() == $onto_marker) { | |||||
| $update_marker = $onto_marker; | |||||
| break; | |||||
| } | |||||
| } | |||||
| $api->execxLocal( | |||||
| 'update -- %s', | |||||
| $update_marker); | |||||
| $state->discardLocalState(); | $state->discardLocalState(); | ||||
| } | } | ||||
| protected function didHoldChanges($into_commit) { | protected function didHoldChanges($into_commit) { | ||||
| $log = $this->getLogEngine(); | $log = $this->getLogEngine(); | ||||
| $local_state = $this->getLocalState(); | $local_state = $this->getLocalState(); | ||||
| $message = pht( | $message = pht( | ||||
| 'Holding changes locally, they have not been pushed.'); | 'Holding changes locally, they have not been pushed.'); | ||||
| list($head, $body, $tail) = $this->newPushCommands($into_commit); | list($head, $body, $tail_pass, $tail_fail) = $this->newPushCommands( | ||||
| $commands = array_merge($head, $body, $tail); | $into_commit); | ||||
| $commands = array_merge($head, $body, $tail_pass); | |||||
| echo tsprintf( | echo tsprintf( | ||||
| "\n%!\n%s\n\n", | "\n%!\n%s\n\n", | ||||
| pht('HOLD CHANGES'), | pht('HOLD CHANGES'), | ||||
| $message); | $message); | ||||
| echo tsprintf( | echo tsprintf( | ||||
| "%s\n\n", | "%s\n\n", | ||||
| Show All 33 Lines | |||||
Aha