Page MenuHomePhabricator

Added autopatch script for patching and rebasing phabricator diffs
Needs RevisionPublic

Authored by jasonbcox on Tue, Oct 22, 17:46.

Details

Reviewers
deadalnix
Fabien
Group Reviewers
Restricted Project
Summary

This is the first step in a pipeline like the following: autopatch (+rebase) -> build/test -> land. This was inspired after feedback on the land-bot script in D4195.

Reviewer note: I would prefer to split this script into two parts that patch and rebase separately, but this process is intertwined due to the way arcanist handles patches that build on a stack of changes.

Example:
You have two changes A and B, where B builds on A which builds on top of master (currently commit M):

M (master) <- A <- B

After succesful review, you land A. Phabricator has the state stored as such:

M <- A' (master)
  \-- A <- B

The landed A is shown as A' because the hash has changed, but the contents have not.

With a clean source tree on latest master, you arc patch Dxxx and arcanist will fail to find
hash A despite A' being present. You add --skip-dependencies because you know A' is effectively A
and the patch succeeds, but it builds on latest master without prompting. This means that if someone
landed changes before you like:

M <- A' <- C <- B'

The B' will be the result of arc patch, as it will not attempt to search for the dependency A
which doesn't exist on your machine (and because you specified --skip-dependencies to get around
not finding it in the first place).

For this reason, it doesn't make sense to split the script apart, as the preparation steps for a clean arc patch
effectively trigger a rebase under circumstances similar to the above. The final git rebase at the end handles
the remaining cases where the dependency of the patch being applied is available (usually part of master directly).

Test Plan
test-rebase.sh
ninja check-arcanist
ninja check
ninja check-all

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D4279
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8030
Build 14056: Bitcoin ABC Buildbot
Build 14055: arc lint + arc unit

Event Timeline

jasonbcox created this revision.Tue, Oct 22, 17:46
jasonbcox planned changes to this revision.Tue, Oct 22, 17:46
jasonbcox edited the summary of this revision. (Show Details)Tue, Oct 22, 19:03
jasonbcox edited the test plan for this revision. (Show Details)
jasonbcox updated this revision to Diff 13649.Wed, Oct 23, 01:28

Cleaned up tests and lint issues

Is it a rebase script, or an arc patch script?

Is it a rebase script, or an arc patch script?

I guess this is a good point. Take off the git rebase at the end and it's clearly the later.

jasonbcox planned changes to this revision.Thu, Nov 7, 22:06
jasonbcox updated this revision to Diff 13983.Fri, Nov 8, 00:49

Rename to autopatch in order to not confuse rebasing being the primary goal of the script.

jasonbcox retitled this revision from Added a rebase script for automation purposes to Added autopatch script for patching and rebasing phabricator diffs.Fri, Nov 8, 00:56
jasonbcox edited the summary of this revision. (Show Details)

I've also updated the summary to provide rationale for why this script should be one piece, as it may not be obvious at first glance.

Fabien requested changes to this revision.Fri, Nov 8, 08:25
Fabien added a subscriber: Fabien.
Fabien added inline comments.
contrib/arcanist/autopatch.sh
72

Nit: incase => in case

73

Could it fail if another stash exists with the same default name ? Maybe just warn about dirty tree and bail out is a better solution here, as this is what arcanist does in such a circumstance.

78

Why is this necessary ?

contrib/arcanist/test/test-autopatch.sh
17

Nit: ${PATCH_FILE}

24

! -z == -n

37

Dito

77

Dito

This revision now requires changes to proceed.Fri, Nov 8, 08:25
jasonbcox added inline comments.Fri, Nov 8, 17:17
contrib/arcanist/autopatch.sh
73

I suppose that's theoretically possible, but my understanding is stash always pushes down on a stack. ie. if you have:

stash@{0} - some message

existing and call git stash then you end up with:

stash@{0} - some new message
stash@{1} - some message

Regardless, I agree that mucking about with the source tree less is better than more.

78

For the case where you're on master and may have modified it with changes that do not belong in the diff that you're patching. Without this step, it's possible to do the following:
M (origin/master) <- A (master) <- P (arc patch Dxxxx)
and then arc land on master and A + P could be landed.

contrib/arcanist/test/test-autopatch.sh
24

Thank you! I knew something like this existed but I couldn't find it digging around. Searching for one-letter bash commands doesn't work so well with Google.