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
F15540461: D17951.id.diff
Fri, Apr 25, 7:43 AM
F15538156: D17951.diff
Thu, Apr 24, 5:40 PM
F15518042: D17951.diff
Sat, Apr 19, 10:24 AM
F15498556: D17951.id43229.diff
Sun, Apr 13, 1:15 PM
F15475950: D17951.diff
Mon, Apr 7, 1:28 AM
F15344455: D17951.id43350.diff
Mar 10 2025, 4:43 AM
F15292787: D17951.id43350.diff
Mar 5 2025, 2:56 AM
F15292786: D17951.id43229.diff
Mar 5 2025, 2:56 AM
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 17086
Build 22850: Run Core Tests
Build 22849: 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.