New issue
Advanced search Search tips

Issue 741648 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: ----



Sign in to add a comment

Gerrit should not allow uploading patchset to CL created by another user

Project Member Reported by sdefresne@chromium.org, Jul 12 2017

Issue description

I 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.
 
Here are the steps I performed (copy-n-paste from my terminal session). You can see that I did not forget the "git cl issue 0" step:

$ git new-branch cronet
$ git cl patch 552763
remote: Counting objects: 483693, done
remote: Finding sources: 100% (43/43)
remote: Total 43 (delta 0), reused 23 (delta 0)
Unpacking objects: 100% (43/43), done.
From https://chromium.googlesource.com/chromium/src
 * branch                      refs/changes/63/552763/21 -> FETCH_HEAD
Committed patch for change 552763 patchset 21 locally.
Note: this created a local commit which does not have the same hash as the one uploaded for review. This will make uploading changes based on top of this branch difficult.
If you want to do that, use "git cl patch --force" instead.
$ git cl issue
Issue number: 552763 (https://chromium-review.googlesource.com/552763)
$ git cl issue 0
Issue number: None (None)
$ git cl upload 
Running presubmit upload checks ...

Presubmit checks took 27.2s to calculate.

Presubmit checks passed.
 components/cronet/ios/BUILD.gn                           |  1 +
 components/cronet/ios/DEPS                               |  2 +-
 components/cronet/ios/cronet_environment.mm              |  5 +++--
 ios/chrome/app/DEPS                                      |  1 +
 ios/chrome/app/startup/BUILD.gn                          |  1 +
 ios/chrome/app/startup/ios_chrome_main.mm                | 17 +++++++++++------
 ios/chrome/browser/web/chrome_web_client.h               |  2 --
 ios/chrome/browser/web/chrome_web_client.mm              |  6 ------
 ios/web/app/BUILD.gn                                     |  1 +
 ios/web/app/web_main.mm                                  | 13 +++++++++++--
 ios/web/app/web_main_loop.h                              | 13 +++++++++----
 ios/web/app/web_main_loop.mm                             | 53 +++++++++++------------------------------------------
 ios/web/app/web_main_runner.mm                           | 11 ++++++-----
 ios/web/public/app/BUILD.gn                              |  1 +
 ios/web/public/app/task_scheduler_init_params_callback.h | 19 +++++++++++++++++++
 ios/web/public/app/web_main.h                            | 14 ++++++++------
 ios/web/public/app/web_main_runner.h                     |  2 +-
 ios/web/public/global_state/BUILD.gn                     | 16 ++++++++++++++++
 ios/web/public/global_state/ios_global_state.h           | 25 +++++++++++++++++++++++++
 ios/web/public/global_state/ios_global_state.mm          | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ios/web/public/web_client.h                              |  5 -----
 ios/web/public/web_client.mm                             |  5 -----
 ios/web/shell/app_delegate.mm                            |  6 ++++--
 ios/web_view/internal/web_view_global_state_util.mm      |  4 ++--
 24 files changed, 191 insertions(+), 91 deletions(-)
remote: Processing changes: updated: 1, done            
remote: (W) 405d6d8: commit subject >50 characters; use shorter first paragraph        
remote: 
remote: Updated Changes:        
remote:   https://chromium-review.googlesource.com/552763 Share task scheduler initialization between ios/web_view and cronet.        
remote: 
To https://chromium.googlesource.com/chromium/src.git
 * [new branch]                405d6d85dd1a05a3e5da7e2d62ffa5a310e17e82 -> refs/for/refs/heads/master%wip,m=Initial_upload

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/).
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.

Comment 4 by vapier@chromium.org, Jul 12 2017

Cc: vapier@chromium.org
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.

Comment 5 by ajwong@google.com, 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.

Comment 6 by aga...@chromium.org, Jul 12 2017

Cc: aga...@chromium.org

Comment 7 by ajwong@google.com, 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.
It does sound like "git cl issue" should be changed to remove Change-Id: lines which should solve the problem.

Comment 9 by aga...@chromium.org, Jul 12 2017

Owner: aga...@chromium.org
Status: Started (was: Unconfirmed)
https://chromium-review.googlesource.com/c/568267/
Project Member

Comment 10 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
Updating your depot_tools should prevent "git cl issue 0" from leaving a Change-Id behind now.
Project Member

Comment 12 by bugdroid1@chromium.org, 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