My favorites | Sign in
Project Home Downloads Wiki Issues Source
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 3594: Pushing an empty commit with cherry-pick submit strategy has erroneous behavior
2 people starred this issue and may be notified of changes. Back to list
Status:  New
Owner:  ----


Sign in to add a comment
 
Project Member Reported by dougk....@gmail.com, Oct 6, 2015
Affected Version: 2.10, 2.11, master

What steps will reproduce the problem?
1. Create a new project with the "cherry-pick" submit strategy
2. Push some code to the project
3. Create a new commit with "git commit --allow-empty" and push it for review
4. Approve and submit the change through the Gerrit UI

What is the expected output? What do you see instead?
I expect the change to merge with the empty commit successfully.  Instead, in 2.10, the user pressing "submit" sees a path conflict error, and in 2.11 and master, the change is marked "Merged" without updating the underlying repository at all.  The fact no changes are made to the repository despite the "merged" message is perhaps most troubling for me.  The "Merge if necessary" strategy does not have this issue, either.

Please provide any additional information below.

Nov 19, 2015
#1 quentin....@gmail.com
I have observed that version 2.8 also will produce the "path conflict error" problem with an empty patch.

I suspect the underlying cherry-pick command needs to use "--allow-empty", I would suggest based on a new project variable.

I also suspect there might be a need to disambiguate between a patch with content that applies as empty (something else has already applied that patch) vs. an empty patch.
Nov 19, 2015
Project Member #2 dougk....@gmail.com
Thinking about this more, I'm not so sure how possible a fix is for just that reason -- is it a cherry-pick which has no result (i.e. change already made by other commits) or is it intentionally empty?  I'm open to ideas, but I'm not aware of anything obvious...
Nov 19, 2015
#3 quentin....@gmail.com
I forgot that git now disambiguates between the cases I mentioned now, so just adding "--allow-empty" would only handle truly empty commits.  For the record, git-cherry-pick now has "--allow-empty" and "--keep-redundant-commits", see https://git-scm.com/docs/git-cherry-pick

As to "why is this needed", consider a downstream environment with a continuous integration system using an automatic builder.  The builder should appear as just another Gerrit user to ensure complete and annotated behavior.  I've seen three use cases that result in empty commits:

1. upstream commits are already merged in developer patches, instead of cherry-picked individually, and apply as empty, this would require "--keep-redundant-commits".

2. upstream commits really are empty - mode changes, directory removals (see http://comments.gmane.org/gmane.comp.compilers.llvm.cvs/265602), would require "--allow-empty".

3. an upstream commit needs to be intentionally skipped (e.g. for a business reason); while leaving them out works, recording the skip with n empty commit improves the audit trail; this would required "--allow-empty".


I can see a simple solution with a new project option "Allow-Empty-Cherry-Picks" that would add both options to every cherry pick.  Or I can see a more fine-grained solution that involves project options or submit options or (what else? API options?) that would result in --allow-empty or --keep-redundant-commits in certain cases.  Detecting "non-null -> empty" vs. "empty -> empty" just means looking to see if the contents of the patch are non-null before deciding.
Nov 19, 2015
Project Member #4 dougk....@gmail.com
Remember that Gerrit uses JGit, which, while compatible with the Git protocols and on-disk format, doesn't necessarily have the same options.  Also, the "cherry-pick merge strategy" in Gerrit is a whole different beast, too.  Instead of creating a merge or trivial rebase while merging the commit, it logically performs a cherry-pick, and attaches some extra review metadata to the commit message before merging the change into the repository.

As you suggested, a new project option would be a potential way to address this, but it'd be interesting since it'd really only impact the cherry-pick strategy, I think (I'll admit, I've not tested this scenario against "Merge if Necessary" or "Rebase if Necessary").
Sign in to add a comment

Powered by Google Project Hosting