Page MenuHomePhabricator

Added autopatch script for patching and rebasing phabricator diffs
ClosedPublic

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

Details

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-autopatch.sh
ninja check-arcanist
ninja check
ninja check-all

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jasonbcox edited the test plan for this revision. (Show Details)

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.

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.Nov 8 2019, 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.Nov 8 2019, 08:25
Fabien added a subscriber: Fabien.
Fabien added inline comments.
contrib/arcanist/autopatch.sh
72 ↗(On Diff #13983)

Nit: incase => in case

73 ↗(On Diff #13983)

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 ↗(On Diff #13983)

Why is this necessary ?

contrib/arcanist/test/test-autopatch.sh
17 ↗(On Diff #13983)

Nit: ${PATCH_FILE}

24 ↗(On Diff #13983)

! -z == -n

37 ↗(On Diff #13983)

Dito

77 ↗(On Diff #13983)

Dito

This revision now requires changes to proceed.Nov 8 2019, 08:25
contrib/arcanist/autopatch.sh
73 ↗(On Diff #13983)

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 ↗(On Diff #13983)

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 ↗(On Diff #13983)

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.

Fixed according to feedback

Fabien requested changes to this revision.Nov 20 2019, 07:33
Fabien added inline comments.
contrib/arcanist/autopatch.sh
76 ↗(On Diff #13983)

origin/${BRANCH} should be ${REMOTE}/${BRANCH} ?

78 ↗(On Diff #13983)

I don't think this is a good solution:

  • It will eventually lose some of the host commits, as stated in the below echo
  • It only fix the case where master if ahead of origin/master. If master is behind you may want to pull instead, checking that the path is fast-forward first.

There are multiple way to check if master has some change ahead of origin/master, and I'm in favor of bailing out in this case.
Thought ?

94 ↗(On Diff #13983)

arc patch --skip-dependencies can succeed if there are dependencies that are not landed yet (intended that there is no conflict), I'm not sure that's the intended behavior.
If a user want to land a diff declared with dependencies, it's probably a better idea to request him to clean up it's dependencies prior to landing.

This revision now requires changes to proceed.Nov 20 2019, 07:33
jasonbcox added inline comments.
contrib/arcanist/autopatch.sh
78 ↗(On Diff #13983)

I think bailing out is also an option, though I didn't initially intend for users to call this script directly, so that wasn't the primary use case. I don't have a good reason to prevent this behavior though.

If origin/master is ahead, the git reset --hard takes care of that. But if there's a better way to test for fast-forwardability, I would prefer that. I'll ping you offline for details.

94 ↗(On Diff #13983)

The really unfortunate consequence of this is that stacks will be discouraged on phab. This is because any diff with dependencies will need to be rebased, tests run again (on phab), and then landed. That requires more manual work on part of the user, which is something this script aims to solve so it's kind of self-defeating to require this.

An alternative solution would be to check all dependencies and see that they're simply closed on phab. This isn't error-proof, but an improvement on the script as it is now without negatively impacting UX.

Fix origin/${BRANCH} -> ${REMOTE}/${BRANCH} so that any remote and branch name combination can work.

contrib/arcanist/autopatch.sh
76 ↗(On Diff #13983)

you're right. I made this assumption because ${REMOTE} wasn't working for local file paths. As it turns out, the simple fix for this is to add it as a remote (thereby giving it a name) before using it.

Bail instead of resetting local commits.
Also fixed some bugs.

Fabien added inline comments.
contrib/arcanist/autopatch.sh
81 ↗(On Diff #14277)

This probably deserves an echo to display why it can't be performed

This revision is now accepted and ready to land.Nov 21 2019, 20:18

Added error message + minor refactor