Gerrit should not allow uploading patchset to CL created by another user |
|||||
Issue descriptionI sometimes use the following pattern when I want to suggest an alternate implementation to a CL that I'm asked to review: 1. create a new branch locally, 2. git cl patch xxx 3. git cl issue 0 4. git cl upload 5. make necessary changes to CL 6. git cl upload 7. point to diff between my two patchsets I tried to to this today with gerrit and ended up uploading a new patchset to a CL created by another user (https://chromium-review.googlesource.com/c/552763/ see ). I think I did not forget step 3. and gerrit used the Change-Id: in the commit message or other information from the commit (I forgot to "git commit --amend --reset-author") but even if I did this should not have happened. It should not be possible to upload a patchset to another user CL.
,
Jul 12 2017
I tried repeating the steps, but adding a step 3.1. where I did "git commit --amend --reset-author" and removed the "Change-Id:" from the description and this correctly created a new CL (https://chromium-review.googlesource.com/c/568143/).
,
Jul 12 2017
Note that when doing steps described in #2, I did not receive any warning. I only received a warning when I tried to upload a second patchset: $ git cl upload -t 'Make WebMainParams moveable' WARNING: Change 552763 is owned by michaeldo@chromium.org, but you authenticate to Gerrit as sdefresne@chromium.org. Uploading may fail due to lack of permissions. Press Enter to upload, or Ctrl+C to abortinterrupted But at that point is was already too late and I had already uploaded a patchset to a CL authored by another user.
,
Jul 12 2017
changing `git cl` to warn is fine, but changing Gerrit is not. we use this feature in CrOS from time to time. it's not a bug imo.
,
Jul 12 2017
I think this hit me as well yesterday here https://chromium-review.googlesource.com/c/564157/ leading to patch set 10 getting unexpected uploaded to the wrong issue. I'm going to guess that "git cl issue 0" somehow puts the metadata in an inconsistent state and that the guard for the warning isn't grabbing ownership/issue information from the same place that "git cl upload" is.
,
Jul 12 2017
,
Jul 12 2017
I think I figured out what's happening. The "git cl patch" leaves a Change-Id: field in the commit message. When you do "git cl upload" it pushes this to the server. Gerrit then helpfully decides that you must have clearly meant to have associated this with the previous change. I mean, what else could you have wanted to do. ;) Since gerrit is doing the conflating of patchsets server side, it bypasses the git-cl stuff. Fun fun. I wonder if "git cl issue 0" needs to go through and remove the Change-Id: fields.
,
Jul 12 2017
It does sound like "git cl issue" should be changed to remove Change-Id: lines which should solve the problem.
,
Jul 12 2017
https://chromium-review.googlesource.com/c/568267/
,
Jul 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/tools/depot_tools/+/400e989b1b91f6dc195266a40edb6a89bc9ee721 commit 400e989b1b91f6dc195266a40edb6a89bc9ee721 Author: Aaron Gable <agable@chromium.org> Date: Wed Jul 12 23:36:35 2017 'git cl issue 0': Remove Change-Id Although git-cl-upload warns when uploading a new patchset to a change owned by someone else, if the uploader has run 'git cl issue 0', then git-cl believes they'll be uploading a new change, so it doesn't bother checking. However, once the upload begins, Gerrit notices the Change-Id in the commit message, and instead adds a new patchset to someone else's review (if the uploader is a committer). This change introduces some logic to git-cl-issue to also remove any Change-Id from the commit message when a user tries to clear the metadata about their branch. Bug: 741648 Change-Id: I6c7c3b24a7fc09c68220c8200b732fbdf9cf1fd3 Reviewed-on: https://chromium-review.googlesource.com/568267 Reviewed-by: Robbie Iannucci <iannucci@chromium.org> Commit-Queue: Aaron Gable <agable@chromium.org> [modify] https://crrev.com/400e989b1b91f6dc195266a40edb6a89bc9ee721/tests/git_cl_test.py [modify] https://crrev.com/400e989b1b91f6dc195266a40edb6a89bc9ee721/git_cl.py
,
Jul 12 2017
Updating your depot_tools should prevent "git cl issue 0" from leaving a Change-Id behind now.
,
Jul 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/tools/depot_tools/+/4e5207d6f0ce1edeb4d73a0dc3fbae2a7ff032d5 commit 4e5207d6f0ce1edeb4d73a0dc3fbae2a7ff032d5 Author: Aaron Gable <agable@chromium.org> Date: Thu Jul 13 17:13:10 2017 git-cl issue 0: Don't operate on empty description TBR=tandrii Bug: 741648 Change-Id: If9bcab1892e30ea5fae127302da12f0d9a201cb8 Reviewed-on: https://chromium-review.googlesource.com/570181 Reviewed-by: Aaron Gable <agable@chromium.org> Commit-Queue: Aaron Gable <agable@chromium.org> [modify] https://crrev.com/4e5207d6f0ce1edeb4d73a0dc3fbae2a7ff032d5/git_cl.py |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by sdefresne@chromium.org
, Jul 12 2017