New issue
Advanced search Search tips

Issue 723787 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Gerrit: Unable to upload dependent patches on another dev's CL

Project Member Reported by danakj@chromium.org, May 17 2017

Issue description

1. git checkout -b BASELINE origin/master
2. git cl patch --gerrit XYZ
3. git checkout -tb DEPENDENT BASELINE
4. git cl upload --gerrit

Get this error instead of upload succeeding.

Upload upstream branch BASELINE first.
It is likely that this branch has been rebased since its last upload, so you just need to upload it again.
(If you uploaded it with --no-squash, then branch dependencies are not supported, and you should reupload with --squash.)

 

Comment 1 by danakj@chromium.org, May 17 2017

Description: Show this description

Comment 2 by danakj@chromium.org, May 17 2017

I was in a situation where I already have DEPENDENT branch existing. I did set 1 and 2, and rebased DEPENDENT onto BASELINE, and ran git branch -u BASELINE. So just in case the error looks different it would be...

1. git checkout -b BASELINE origin/master
2. git cl patch --gerrit XYZ
3. git checkout -b DEPENDENT origin/master
4. git commit -a -m stuff
5. git cl upload --gerrit
6. git rebase --onto BASELINE HEAD^
7. git branch -u BASELINE
8. git cl upload --gerrit

Comment 3 by danakj@chromium.org, May 17 2017

Cc: enne@chromium.org

Comment 4 by danakj@chromium.org, May 17 2017

Some rather nasty side effects come out of "git cl patch" from someone else's CL apparently.

I tried to work around the above problem by doing

git branch -u origin/master
git cl upload --gerrit HEAD^

This patch is still on top of a commit from BASELINE, but I wanted to upload just my delta. The resulting upload also uploaded my local copy of BASELINE to the code review for it, which I do not own. It also gave me an error about doing so.

remote: Processing changes: updated: 2, done            
remote: (W) e8e87b9: commit subject >50 characters; use shorter first paragraph        
remote: (I) e8e87b9: no files changed, was rebased        
remote: 
remote: Updated Changes:        
remote:   https://chromium-review.googlesource.com/507047 Add PaintOpBuffer::PlaybackRanges() to play a set of ranges of ops.        
remote:   https://chromium-review.googlesource.com/508028 cc: refactor PaintOpBuffer::Iterator::peek        
remote: 
To https://chromium.googlesource.com/chromium/src.git
 * [new branch]                e8e87b9c305b17a64dc0a8fe1b09819a9146d3c4 -> refs/for/refs/heads/master%m=playbackrange_nodep,notify=NONE

Error after CL description prompt -- saving description to /usr/local/google/home/danakj/.git_cl_description_backup

Created|Updated 2 issues on Gerrit, but only 1 expected.
Change-Id: I7ec67e796a3072074a5d910fd800c84df83e6d4c

Comment 5 by aga...@chromium.org, May 18 2017

Cc: tandrii@chromium.org
Labels: Milestone-Launch Proj-Gerrit-Migration
Status: Assigned (was: Untriaged)
I see what's happening in the first two comments. It is, unfortunately, working as intended, so I'd like to ask you what behavior you'd prefer.

"git cl upload" requires that the upstream branch have been uploaded. This is to prevent confusing gerrit and other reviewers by introducing commits that aren't actually meant to be reviewed. In particular, it can result in this scenario:
$ git checkout baseline && git cl upload
$ git checkout dependent && git cl upload
$ git rebase-update  # fetches origin/master and rebases the whole tree of branches
$ git checkout dependent && git cl upload
# this fails because the rebased version of baseline hasn't been uploaded
This is basically what's happening here: "git cl patch" implicitly rebases (it actually cherry-picks, but whatever) the commit you download on top of whatever is currently checked out on the branch you're on. Unless you happen to have checked out the exact commit that it was based on when it was uploaded, this means you've rebased it, and that rebased version hasn't been uploaded.

So there are two main options here:
* Have git-cl-patch continue to act like the Rietveld version, rebasing the patch on top of whatever you have checked out, but then requiring that it be uploaded before dependent branches can be uploaded
* Have git-cl-patch change to directly check out exactly the commit that has been uploaded to gerrit, ignoring whatever state was on the branch when you ran it. This allows easier creation of dependent branches, but kinda violates the semantics of "patch" (it's more like "git cl download").
* Or I guess a third option: put the second option behind a flag.

Which of these feels most natural to you? Which of these do you think would be the most intuitive to the most Chromium developers?

Comment 6 by danakj@chromium.org, May 18 2017

> * Have git-cl-patch continue to act like the Rietveld version, rebasing the
> patch on top of whatever you have checked out, but then requiring that it be
> uploaded before dependent branches can be uploaded

Can you explain this a bit?

My understanding of rietveld and what I would like is that they are the same but I would describe it as:
Create a diff from the diff to the branch's upstream (which is the baseline branch which was not rebased which is a huge diff and then you notice something is wrong - noticing this in a more coherent way could be nice)
Whatever the diff from HEAD to the branch's upstream is uploaded to codereview.

IOW uploading dependent patch is completely independent from the state of baseline *in gerrit*. I can upload that later, and bots will fail on the dependent one until I do.

Comment 7 by danakj@chromium.org, May 18 2017

Oh, and in this scenario, if I rebase-update, I could still do "git cl upload HEAD^" and it would just upload the diff to HEAD^ instead of to the non-rebased upstream branch.

Comment 8 by aga...@chromium.org, May 18 2017

Gerrit has a very strict model: every *commit* pushed to refs/for/foo results in a codereview being created. Not every branch. This means that if you upload a dependent CL which is based on commits that haven't already been pushed to gerrit one of two things will happen:
1) You'll actually create many reviews, one for each commit that hasn't been uploaded previously; or
2) git-cl-upload will squash all those commits into a single commit for you, but then the resulting review will contain a bunch of diff you didn't want it to.

This is why git-cl-upload refuses to upload when you're based on top of a rebased version of someone else's CL. It can either update their CL (rude), upload a carbon copy of their CL (pointless and confusing), or include the diff of their CL in yours (ugly and unwanted).

So basically, uploading a dependent patch is *not* completely independent from the state of the baseline in gerrit.

Comment 9 by enne@chromium.org, May 18 2017

If the dependent patch is just a rebase, I'd mostly like git cl upload to just silently do whatever it needs to do to update the dependent review to say the patch also applies cleanly to that commit too.  Maybe that means a new noop patchset, although it'd be nice if it didn't interrupt cq stuff in progress too.
Status: Started (was: Assigned)
This is a CL to change the behavior of git-cl-patch so things should work how people expect most of the time: https://chromium-review.googlesource.com/c/527345/
Project Member

Comment 11 by bugdroid1@chromium.org, Jun 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/9387b4f0be621a3027848e0624a3ca3a70ee5195

commit 9387b4f0be621a3027848e0624a3ca3a70ee5195
Author: Aaron Gable <agable@chromium.org>
Date: Thu Jun 08 17:54:22 2017

Make gerrit 'git cl patch' use hard reset by default

This CL changes the way that "git cl patch" behaves for Gerrit changes.

Previously, git-cl-patch behaved just like it did for Rietveld:
make sure you're on a branch, download the diff, apply it on top
of your branch. However, this causes problems with Gerrit. Namely,
when you upload a change to Gerrit, git-cl has to make sure that all
parents of your local change have previously been uploaded as well,
either as other changes or as commits already landed on the target
branch. But the method for "applying a patch" from Gerrit was to
cherry-pick it, and that changes the commit hash. So the resulting
commit would *not* have been uploaded to Gerrit. Thus, the
following routine didn't work with Gerrit:
$ git checkout -t origin/master -b your-work
$ git cl patch 123456
$ git checkout -tb my-work
$ #hack and commit
$ git cl upload
This would fail during the upload with a message saying that the
contents of 'your-work' hadn't been uploaded.

This CL fixes the situation by replacing the cherry-pick with
a hard reset. This means that the contents of the 'your-work'
branch will be *exactly* what was downloaded from Gerrit. Uploads
based on top of that commit will work just fine.

Finally, in a concession to some people who want 'git cl patch'
to actually apply a patch instead of performing a hard reset, if
the current branch contains local work, then rather than leaving
that work behind with a hard reset, we fall back to the old
cherry-pick behavior with a confirmation dialog and warning that
uploading will be hard.

Bug:  723787 
Change-Id: I3ad164f6d3078bff00139d446bb8ce97738a1344
Reviewed-on: https://chromium-review.googlesource.com/527345
Commit-Queue: Aaron Gable <agable@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>

[modify] https://crrev.com/9387b4f0be621a3027848e0624a3ca3a70ee5195/tests/git_cl_test.py
[modify] https://crrev.com/9387b4f0be621a3027848e0624a3ca3a70ee5195/git_cl.py

Status: Fixed (was: Started)
git-cl-patch has been updated to play much nicer with Gerrit. I'm going to mark this fixed. There's also some discussion in this bug about uploading new patchsets to parent/child CLs during a rebase -- for that, please follow along in issue 728390
Project Member

Comment 13 by bugdroid1@chromium.org, Jun 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/bacbdb9a559c58d0fe462422c913710fd2f38677

commit bacbdb9a559c58d0fe462422c913710fd2f38677
Author: Aaron Gable <agable@chromium.org>
Date: Fri Jun 09 17:39:08 2017

git-cl-patch: fix --is-ancestor flag

TBR=tandrii@chromium.org

Bug:  723787 
Change-Id: Ie5017747f2070116774cd12ba0dd2ea531b2d0aa
Reviewed-on: https://chromium-review.googlesource.com/529547
Reviewed-by: Aaron Gable <agable@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Aaron Gable <agable@chromium.org>

[modify] https://crrev.com/bacbdb9a559c58d0fe462422c913710fd2f38677/tests/git_cl_test.py
[modify] https://crrev.com/bacbdb9a559c58d0fe462422c913710fd2f38677/git_cl.py

Project Member

Comment 14 by bugdroid1@chromium.org, Jun 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/62619a38110f1a49f7ed40128f843cf4bf803dc1

commit 62619a38110f1a49f7ed40128f843cf4bf803dc1
Author: Aaron Gable <agable@chromium.org>
Date: Fri Jun 16 15:26:20 2017

git-cl-patch: Return to cherry-pick, but hard reset with --force

This is a partial revert of https://chromium-review.googlesource.com/c/527345/
Turns out more people were confused by the new behavior than
expected, so we're returning to "cherry-pick" being the default,
but supporting the collaboration workflow is important, so we're
adding a warning message and support for "reset --hard" behind a
pre-existing flag.

Bug:  723787 
Change-Id: Ib6038a42e3bdcc0db93c1f32d759e9ff0e91a065
Reviewed-on: https://chromium-review.googlesource.com/538137
Commit-Queue: Aaron Gable <agable@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>

[modify] https://crrev.com/62619a38110f1a49f7ed40128f843cf4bf803dc1/tests/git_cl_test.py
[modify] https://crrev.com/62619a38110f1a49f7ed40128f843cf4bf803dc1/git_cl.py

Sign in to add a comment