Page MenuHomePhabricator

Build and check `arc patch` dependency chain before creating branches
Needs ReviewPublic

Authored by alexmv on May 18 2017, 5:27 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 25 2024, 12:40 AM
Unknown Object (File)
Feb 20 2024, 4:43 PM
Unknown Object (File)
Feb 16 2024, 2:37 AM
Unknown Object (File)
Feb 3 2024, 3:53 PM
Unknown Object (File)
Jan 29 2024, 1:45 PM
Unknown Object (File)
Jan 20 2024, 7:20 AM
Unknown Object (File)
Jan 17 2024, 5:11 PM
Unknown Object (File)
Jan 17 2024, 5:07 PM
Subscribers

Details

Reviewers
None
Group Reviewers
Blessed Reviewers
Summary

Validate that we can construct the dependency chain first,
before any branches are created. This means that aborts in
sanityCheck, or dependency cycles, do not leave the developer on an
empty branch.

It also removes misleading messages of the form "Created and checked
out branch ..." for every dependency in the chain -- those branches
were merely created as intermediate steps, and were silently removed
before the command completed. Finally, this removes a hack where
--nobranch would actually often create branches, due to interactions
with the intermediate branches mentioned above.

Test Plan

arc patch with a dependency chain. arc patch --nocommit and arc patch --nobranch on both git and hg.

Diff Detail

Repository
rARC Arcanist
Branch
up-branch-dep-creation (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 17261
Build 23117: Run Core Tests
Build 23116: arc lint + arc unit

Event Timeline

One piece I'm not sure about, see inline. The rest of this seems like a reasonable step forward.

src/workflow/ArcanistPatchWorkflow.php
430–443

I think don't understand where this came from or what problem it's solving, can you walk me through it?

src/workflow/ArcanistPatchWorkflow.php
430–443

If one had A <- B <- C, we previously:

  • Observe the "base" commit for C as the last commit of B, which we don't have locally
  • Create "arcpatch-C" at the current commit (because we don't have the "base" commit in our local repo)
  • Apply A and B with --nobranch --skip-dependencies
    • Create "arcpatch-A" at the parent of A
    • Apply A
    • Checkout "arcpatch-C"
    • Cherry-pick "arcpatch-A" onto "arcpatch-C"
    • Delete "arcpatch-A"
    • Look for "base" commit of B, which is A' -- a different commit hash of A than we have due to differing committers and commit times
    • Create "arcpatch-B" at the current commit ("arcpatch-C"), since we don't have the local "base" commit
    • Apply B
    • Checkout "arcpatch-C"
    • Cherry-pick "arcpatch-B" onto "arcpatch-C"
    • Delete "arcpatch-B"
  • Apply C onto "arcpatch-C"

This hunk resolves the "base" commit as the parent of "A", which means we can make an educated decision about if we have that commit locally. So:

  • Resolve patch dependencies as A <- B
  • Set "base" commit to parent of the first one, A
  • Create "arcpatch-C" at the parent of A, since that's the "base" commit (which we do have, though we may need to fetch it)
  • Apply A and B with --nobranch --skip-dependencies
    • Apply A
    • Apply B
  • Apply C onto "arcpatch-C"

Add some hopefully edifying comments

Remove the equivalent codepath for Mercurial, which removed the
bookmark which we now never create at all. The entire original_branch
code is now unused, and this removed.