| 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 |
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
Nov 19, 2015
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
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
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 |