Page MenuHomePhabricator

`arc patch` blows up when the revision's parent has already landed
Open, Needs TriagePublic

Description

Repro

Steps:

  1. In a Git repo, set up revision A, then revision B depending on A; then land A.
  2. Make a fresh clone of the repo (that doesn't already have the commit that B was based on), at a recent master that contains A.
  3. Run arc patch on B. (For example: arc patch D17607 in rGITTEST.)

Expected:
arc patch produces a commit corresponding to B.

Actual:
arc patch spews a mysterious error message from git cherry-pick:

$ time arc patch D17607
Created and checked out branch arcpatch-D17607.
Created and checked out branch arcpatch-D17606.
Checking patch thing...
Applied patch thing cleanly.

 Cherry Pick Failed!
Exception
Command failed with error #1!
COMMAND
git cherry-pick 'arcpatch-D17606'

STDOUT
On branch arcpatch-D17607_1
Your branch is up-to-date with 'master'.
You are currently cherry-picking commit fac7969.

nothing to commit, working tree clean


STDERR
The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:

    git commit --allow-empty

Otherwise, please use 'git reset'

Analysis

From a --trace, it looks like arc patch is patching in A first. It does the usual "make a commit at the original base" thing, tries to cherry-pick it, and then gets confused when Git warns that the cherry-pick is a no-op. If arc patch were clever here, it should probably just realize it doesn't need to do any of that, because A has already landed and the commit it landed as is an ancestor of HEAD, and skip straight to patching in B.

Event Timeline

gregprice created this task.Apr 4 2017, 1:01 AM

Also T6482 seems like it's fixed. I've been quite appreciating how Arcanist is increasingly aware of dependencies between revisions.

I'm also slightly confused because, as someone just reminded me, the usual behavior of arc patch is to apply the patch to the appropriate base commit for the revision, not to try to do it on the current HEAD. In this context, that seems like it'd mean applying it directly to the arcpatch-A branch, rather than try to cherry-pick that commit up on top of where the user ran arc patch from.

epriestley moved this task from Backlog to arc patch on the Arcanist board.Apr 10 2017, 10:14 PM