reconcileLocalState() is another possible place to put the post-push update logic -- that runs exactly once, and a little later.

"Cascading" is updating all local refs which depend on refs that were selected-and-modified by the land operation. So if you have dependent features in several dependent branches named change1 -> change2 -> change3 locally, and arc land change2, we want to (in the general case) rebase your local change3 onto the landed version of change2 so that it no longer has out-of-date ancestors.

This is called "cascading" because if you have change1 -> change2 -> ... -> changeN and arc land change1, we update change2 on the new change1, then update change3 on the new change2, etc., all the way down the tree, "cascading" the change down commit-by-commit.

"Reconciling" is updating local refs for other reasons than because they depend on landed changes, generally because they are "equivalent" in some way to the "onto" or "into" branches. I think the operation you're doing is the best conceptual match for the "reconcile" step, but there might be technical reasons that it doesn't fit there.

Doing a basic test this behavior looks good

$ hg wip
@   19:823c7457b7ef cspeck (30 seconds ago) tip test             <- draft commit to be landed, working state when running `arc land`
|     test commit
| o   17:33c3ae4d1277 cspeck (8 minutes ago) master
|/      test commit
o   14:82892d507c28 cspeck (24 minutes ago)
|     test commit
~

$ arc land test
...

$ hg wip
@   20:0b01052fc32e cspeck (80 seconds ago) tip master           <- master is active
|     test commit
~

I'll test out a couple more scenarios but I think this is right. It felt correct while writing it anyways~

Not-working state test

• I created a diff with an inactive bookmark test but working directory on an older commit not related to the one being landed
• I landed that diff
• I verified the ending resulting working state was on the older commit I was on prior to landing

In this scenario the resulting working set ends up being something else -> the previous master commit (but bookmark inactive). I switch arcanist back to master branch to check without these changes and the behavior is the same, so even if this state seems unexpected it's not a regression introduced with these changes

$ hg wip
o   28:a33f5df2c48a cspeck (32 seconds ago) tip test
|     test commit
| o   26:74de4ab3c972 cspeck (2 minutes ago) master
|/      test commit
o   23:999d8d1f780f cspeck (4 minutes ago)
|     test commit
@   20:0b01052fc32e cspeck (25 minutes ago)            <- this commit is active working state
|     test commit
~

$ arc land test
...

$ hg wip
o   29:8b15ac1bbdcd cspeck (61 seconds ago) tip
|     test commit
@   26:74de4ab3c972 cspeck (3 minutes ago) master       <- ended up on this commit, with master not active
|     test commit
~

I'm going to do a few more tests -- I'm guessing the --base argument would affect what commits get snagged up during landing (or is that captured during diff phase only?). I normally use arc:bookmark however I recently switched it to arc:this so it's also compatible with git. All of my tests I've run currently were using arc:this.

I'm guessing the --base argument would affect what commits get snagged up during landing (or is that captured during diff phase only?)

Ah it looks like the --base flag isn't part of diff workflow

$ arc land test --base arc:bookmark
Usage Exception: Argument "--base" is unrecognized. Use "--" to indicate the end of flags.

Just in case I arc set-config base arc:bookmark and re-ran the multi-commit test and things worked fine.

I normally use arc:bookmark however I recently switched it to arc:this so it's also compatible with git. All of my tests I've run currently were using arc:this.

Went back through https://secure.phabricator.com/book/phabricator/article/arcanist_commit_ranges/ and realized I can specify a ruleset of arc:bookmark, arc:this to get the behavior I want in both mercurial and git. Huzzah!

Sorry for the churn on this, it sorta became a whackamole for an assortment of issues I ran into. I think most of the test cases I've tried out now indicate that the behavior is pretty solid at this point. For now I don't have any remaining test cases to cover and I think this is in a state for review.

I'll make these last few minor updates and do a quick few tests to make sure everything is still in working order

If I want to go spelunking later to see about improving/adding this behavior is this the right place to do this or is there another place this TODO should go?

Oh, I think I expect ArcanistMercurialWorkingCopyRevisionHardpointQuery to have no idea which revisions the commits should go with in the "rebase" or "local commit on top of other commits" cases. The "there's only one logical thing we can actually do" logic, where arc land decides they must either go with the contiguous ancestor revision or the workflow must abort, happens later on. The major parts of related control flow are:

• ArcanistLandEngine->loadRevisionRefs(), where arc land processes the --revision flag and commits which have obvious (or obviously-ambiguous) revision mappings. Commits that we aren't sure about (usually, including touch-up commits and rebased commits) fall through:

// NOTE: We may exit this method with commits that are still unassociated.
// These will be handled later by the "implicit commits" mechanism.

• ...to ArcanistLandEngine->execute(). Here, commits are sorted into sets of contiguous commits (ArcanistLandCommitSet), and this implicitly attaches the "touch-up" commits to the revisions of their ancestors (see also ArcanistLandCommit->newRevisionRef(), which looks at a commit's ancestors to find a related revision if it doesn't have an explicit revision from the HardpointQuery).
• Finally, if we did locate implicit commits, we ask the user to confirm that they're expected touch-up / rebase commits a little later:

final protected function confirmImplicitCommits(array $sets, array $symbols) {
  assert_instances_of($sets, 'ArcanistLandCommitSet');
  assert_instances_of($symbols, 'ArcanistLandSymbol');

  $implicit = array();
  foreach ($sets as $set) {
    if ($set->hasImplicitCommits()) {
      $implicit[] = $set;
    }
  }

  if (!$implicit) {
    return;
  }

  echo tsprintf(
    "\n%!\n%W\n",
    pht('IMPLICIT COMMITS'),
    pht(
      'Some commits reachable from the specified sources (%s) are not '.
      'associated with revisions, and may not have been reviewed. These '.
      'commits will be landed as though they belong to the nearest '.
      'ancestor revision:',
      $this->getDisplaySymbols($symbols)));

  foreach ($implicit as $set) {
    $this->printCommitSet($set);
  }

  $query = pht(
    'Continue with this mapping between commits and revisions?');

  $this->getWorkflow()
    ->getPrompt('arc.land.implicit')
    ->setQuery($query)
    ->execute();
}

Generally, the flow for arc land A B C --onto master is to: take all ancestors of A, B, and C that are not ancestors of master; group them by revision; then try to land each group. The "touch-up commits implicitly go with the ancestor revision" stuff is inside the "group the commits by revision" part.

From https://secure.phabricator.com/D21680?id=51633#inline-60703

Still, I'd expect arc land <head2> to look at the ancestors between head2 and master, find the "Differential Revision" commit, and then figure out that "head2" can only be part of that revision, possibly prompting you ("Local range includes commits which don't match the revision, are you sure you want to land them?").

Ah I think the behavior I'm seeing where arc land fails to identify the proper commit is due to the order of commits and revisions that are being created.

If you create commit A then use arc diff, then create commit B the result is

B (bookmark)
|
A - "Differential Revision: https://..."
|
o master

Running arc land bookmark does appear to look at commit B, then A, figure out that A is associated to a revision, then implicitly ties B into that same revision. However if you first create both commits then create the diff from`B` the result is

B (bookmark) - "Differential Revision: https://..."
|
A
|
o master

When running arc land bookmark it identifies B with the revision but then fails to group A with a commit set and gives an error that it can't identify which revision A belongs to, even though arc diff lumped the commit in with the revision due to base configuration. Looking at the revision in phab it only shows in the "Commits" tab that the changeset for B is included but not for A. In the first scenario, after running arc diff to update and include changes for B the phab revision includes that commit. This makes me think that in this second scenario where running arc diff initially includes multiple commits that the revision should be updated to include the commit hashes for each, and then might result in having better detect this scenario if it's searching for revisions with associated commit hashes.

This makes me think that in this second scenario where running arc diff initially includes multiple commits that the revision should be updated to include the commit hashes for each, and then might result in having better detect this scenario if it's searching for revisions with associated commit hashes.

I believe this is the expected behavior. Specifically:

However if you first create both commits then create the diff from B the result is...

If you created a diff from only B explicitly (arc diff B^, i.e. force arc diff to omit A by selecting only B), I think arc land is correct to abort and force you to sort things out: you've put an unpublished, unreviewed commit in history and it definitely isn't part of the revision you're landing because you explicitly excluded it. arc could continue faithfully only by discarding it, and this seems sort of dangerous/arbitrary: it seems more likely your expected state and actual state differ than that that you've intentionally put a junk local commit in history which isn't related to any revision and which you intend to discard.

If you created a diff from B normally (with something like arc diff master in Git, or most sensible base rules), arc diff will select both A and B and create a revision with the changes in both commits, and both commits associated with the revision.

For example, here's arc diff --base arc:outgoing against the state you describe, which includes both commits:

arc land then correctly associates both commits with the revision:

$ arc land --hold --conduit-uri=http://local.phacility.com/

...

 INTO COMMIT  Preparing merge into "default" from remote "default", at commit "162a9c1e8e44".
 LANDING  These changes will land:

  *   D18 A
          409349be71c5  A
          ade642d0c045  B

 >>>  Land these changes? <y>

Oh, maybe I'm mixing up running arc diff B against running plain arc diff while being on B

I’ve used this a few times in the past week and the behavior is overall better for my own case. I had one other developer upgrade to master branch and also reported better behavior, though his setup is very similar to my own.