Page MenuHomePhabricator

`arc patch` attempts to cherry-pick already landed dependencies
Closed, DuplicatePublic

Description

In the example below, arcpatch-D319 is a diff that has already landed and is marked "closed":

Created and checked out branch arcpatch-D23.
Created and checked out branch arcpatch-D319.
Checking patch <path>...
Checking patch <path>...
Applied patch <path> cleanly.
Applied patch <path> cleanly.

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

STDOUT
On branch arcpatch-D23
You are currently cherry-picking commit 230bfd3.

nothing to commit, working directory 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'

(Run with `--trace` for a full exception trace.)

Versions:

Event Timeline

staticshock raised the priority of this task from to Needs Triage.
staticshock updated the task description. (Show Details)
staticshock added a project: Arcanist.
staticshock added a subscriber: staticshock.

My colleagues and I have run into what seems to be the same problem. A diff D1259 was created to be dependent on D1258 (and D1261 was later added as dependent on D1259, though I do not think that is important).

The problem seems to have been caused because D1258 was created on the master branch (a mistake, but one that we see people make occasionally). Since arc land doesn't work very well when this happens, the commit was pushed to origin/master using git push. Phabricator linked the commit with the diff, so all's well and good, or so it seems.

D1259 was created correctly on a branch. But when people try to arc patch D1259, things break down as above, with arcanist trying to pull in D1258.

git log on master, shows that 1258 has gone through just fine. This is the same for everyone.

cfc15d7d84e26f9086981f2042af77a0fb8a7130 avoid some copying in sql
28a7446c71f1672e8067c1d90d73cf38a363ec29 Implemented getKvStationParam   <=== This is D1258
531618c36c93c1ef63a76efbb5270368efa4a6c5 Removed getKvReferenceStations

arc patch D1259 with the originator of the two diffs works fine, however - resulting in a different log for branch arcpatch-D1259. This is the same as the T1259 branch where the work is done.

326c56ec240029ed0af1ee020135ef496139782d Cleaned up SqlKvApp tests   <=== This is D1259
55b68f1d2da86a515c3d4bc45bab073ea9ba2bec Implemented getKvStationParam   <=== This is D1258
531618c36c93c1ef63a76efbb5270368efa4a6c5 Removed getKvReferenceStations

We worked aorund the difficulties patching by having reviewers run arc patch D1259 --skip-dependencies. THat works - it complains about missing 55b68f... but patches the master up anyway. This allowed reviewers to review the diff.

This does bring us back to the question of how to land D1259. arc landon that of course results in:

These commits will be landed:

      - b00b453 Cleaned up SqlKvApp tests
      - 55b68f1 Implemented getKvStationParam

Usage Exception: There are multiple revisions on feature branch 'tests' which are not present on 'master':

     - D1259: Cleaned up SqlKvApp tests
     - D1258: Implemented getKvStationParam

Separate these revisions onto different branches, or use --revision <id> to use the commit message from <id> and land them all.

Which is actually not very helpful, because it doesn't really explain what is meant by separating the revisions onto different branches or how to go about it. And landing both revisions is - from what I've seen previously - a good way to mess up the source tree completely.

Hope the above helps to identify the problem. Also, if you can explain how it is intended that one should split up two diffs on the same branch, that would surely be helpful.

Also, based on our experiences, I feel it would be useful if the arcanist tool was more restrictive; e.g., not allowing diffs on the master; perhaps even an arc commit that people can use in the workflow, that disallows commits directly to master. People don't make the mistake all the time, but it is frequent enough to be annoying - and pretty much every issue I've seen people have with arcanist (including my own), have come about because people messed up on the master branch.

Ah - never mind about the landing it part. Doing a git rebase origin/master followed by arc land solved that issue. Though the error message/suggestion remains confusing (and does not at all appear related to the actual solution).

eadler added a project: Restricted Project.Aug 5 2016, 5:24 PM