tl;dr git 2.10 is not yet something git cl can rely on.

see https://bugs.chromium.org/p/gerrit/issues/detail?id=4544#c21. For now, marking as blocked on that bug.

Sounds good. Bumping milestone in lieu of this.

I'm not completely agreeing with the new title. If REST api is implemented before we everyone gets git 2.10, there is good sense to make use of it, although at a cost of extra post-push latency.

I’m not a fan of extra latency :)

Neither am I [1]. And I think we should set cc as a push option instead of post push call.
That said, even just push is considerably slower for Gerrit than uploading to Rietveld on small CLs :(

[1] don't tell anyone, so instead of running presubmit locally, I trigger CQ dry run instead: $ git cl upload --bypass-hooks -d   . Saves me countless[2] seconds daily :)
[2] but obvious bounded by 24*60*60 :)

 Issue 673993  has been merged into this issue.

I think we now can rely on git version being at least 2.10. Macs ship it now, googlers linux workstations have it, and Windows has been at 2.11  shipped by us for a while now.

I'll do it next Monday. Doing this on weekend is scary, because some weird tooling might be broken in the process.

Sounds good. Thanks, Andrii.

From https://git-scm.com/docs/git-push#git-push---push-option

-o
--push-option
Transmit the given string to the server, which passes them to the pre-receive as well as the post-receive hook. The given string must not contain a NUL or LF character.

OK, turns out Gerrit doesn't yet support this fully. Blocked on Gerrit work b/34810198

b/34810198 is now marked as fixed.

https://chromium-review.googlesource.com/c/468926/

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

commit 0267fd2c290a4ca2300d58abf53af32f8324e285
Author: Andrii Shyshkalov <tandrii@chromium.org>
Date: Wed Apr 05 12:39:00 2017

git cl upload for Gerrit: use push options instead of refspec.

This removes limitation of no special chars in patchset titles.

BUG= chromium:663787 , chromium:707963 , gerrit:5184 
R=sergiyb@chromium.org,agable@chromium.org
TEST=uploaded this CL using depot_tools with this patch :)

Change-Id: I5d684d0a0aa286a45ff99cca6d57aefa8436cd0f
Reviewed-on: https://chromium-review.googlesource.com/468926
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>
Reviewed-by: Sergiy Byelozyorov <sergiyb@google.com>

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

 Issue gerrit:5184  has been merged into this issue.

Something went wrong, investigating..

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

commit febbae9d2edf9b7861dcee602ae6eaecb25d3e09
Author: Andrii Shyshkalov <tandrii@chromium.org>
Date: Wed Apr 05 15:05:33 2017

Revert "git cl upload for Gerrit: use push options instead of refspec."

This reverts commit 0267fd2c290a4ca2300d58abf53af32f8324e285.

Reason for revert: seems to have changed some behavior as reported by SKIA.

Original change's description:
> git cl upload for Gerrit: use push options instead of refspec.
> 
> This removes limitation of no special chars in patchset titles.
> 
> BUG= chromium:663787 , chromium:707963 , gerrit:5184 
> R=​sergiyb@chromium.org,agable@chromium.org
> TEST=uploaded this CL using depot_tools with this patch :)
> 
> Change-Id: I5d684d0a0aa286a45ff99cca6d57aefa8436cd0f
> Reviewed-on: https://chromium-review.googlesource.com/468926
> Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>
> Reviewed-by: Sergiy Byelozyorov <sergiyb@google.com>
> 

TBR=agable@chromium.org,sergiyb@google.com,tandrii@chromium.org,sergiyb@chromium.org,borenet@chromium.org,chromium-reviews@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= chromium:663787 , chromium:707963 , gerrit:5184 

Change-Id: I3306091b14b97a200150389d0480b69120af8c61
Reviewed-on: https://chromium-review.googlesource.com/469006
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>

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

FTR here's the command used by Skia's autorollers which stopped working. I think the problem was that the bot dit not self +1 the CL. This is copy/paste from logs, quotes missing:

git cl upload --bypass-hooks -f --use-commit-queue --gerrit --send-mail --cc jvanverth@chromium.org -m Roll src/third_party/skia/ 8653e9738..33aa2c7b5 (7 commits)

https://skia.googlesource.com/skia.git/+log/8653e973893b..33aa2c7b5c35

$ git log 8653e9738..33aa2c7b5 --date=short --no-merges --format='%ad %ae %s'
2017-04-05 brianosman Allow FPs to elevate default precision for the entire fragment program
2017-04-03 borenet Change PRESUBMIT.py to use [Get|Update]DescriptionLines
2017-04-05 robertphillips Revert "Rm readPixels from GrSurface & move read/writeSurfacePixels to GrContextPriv"
2017-04-05 reed remove SK_SUPPORT_LEGACY_PAINT_TEXTDECORATION
2017-04-04 robertphillips Rm readPixels from GrSurface & move read/writeSurfacePixels to GrContextPriv
2017-04-05 kjlubick Remove Tab3 from pool
2017-04-05 kjlubick Add recipe to build for Asus Flip Chromebook

Created with:
  roll-dep src/third_party/skia


Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls


CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=jvanverth@chromium.org

borenet@ I can't repro your report: I created https://chrome-internal-review.googlesource.com/348164 the same way you wrote, but CR+1 was correctly set. 


Here is what I did in details: 

$ cd depot_tools && git co 0267fd2c290a4ca2300d58abf53af32f8324e285

$ cd <another repor> && create fake commit.

0:tandrii@andrii:/icq test$ git show -s --format=%B
This is test. Please, ignore.

TBR=sergiyb@google.com





0:tandrii@andrii:/icq test$ git cl issue 0
Issue number: None (None)



0:tandrii@andrii:/icq test$ git cl upload --bypass-hooks -f -s -m "`git show -s --format=%B`" --cc machenbach@google.com -v -v 
....
[D2017-04-06 13:36:11,613 682 139818590197568 subprocess2.py] git rev-parse HEAD:
[D2017-04-06 13:36:11,618 682 139818590197568 subprocess2.py] git commit-tree af617f939cc88b9aaf73c387ef949e73aa4851c6 -p f38a7b067d4ad76d774ecb6d4e329ec5030af74b -m This is test. Please, ignore.

TBR=sergiyb@google.com

Change-Id: Ibcd9c063ca42772fc28fc238fc6daa42c1fe969c
[D2017-04-06 13:36:11,622 682 139818590197568 subprocess2.py] git rev-list f38a7b067d4ad76d774ecb6d4e329ec5030af74b..424da731b8989a47691e7088ce6b716b05c8fcee
Adding self-LGTM (Code-Review +1) because of TBRs
[D2017-04-06 13:36:11,633 682 139818590197568 subprocess2.py] git config remote.origin.url
[D2017-04-06 13:36:11,639 682 139818590197568 subprocess2.py] git push -o l=Code-Review+1 -o m=Initial upload -o notify=ALL -o r=sergiyb@google.com https://chrome-internal.googlesource.com/infra/infra_internal.git 424da731b8989a47691e7088
ce6b716b05c8fcee:refs/for/refs/heads/master
remote: Processing changes: new: 1, done            
remote: 
remote: New Changes:        
remote:   https://chrome-internal-review.googlesource.com/348164 This is test. Please, ignore.        
remote: 
To https://chrome-internal.googlesource.com/infra/infra_internal.git
 * [new branch]        424da731b8989a47691e7088ce6b716b05c8fcee -> refs/for/refs/heads/master
[D2017-04-06 13:36:14,373 682 139818590197568 subprocess2.py] git config branch.test.gerritissue 348164
[D2017-04-06 13:36:14,381 682 139818590197568 subprocess2.py] git config branch.test.gerritserver https://chrome-internal-review.googlesource.com
[D2017-04-06 13:36:14,388 682 139818590197568 subprocess2.py] git config branch.test.gerritsquashhash 424da731b8989a47691e7088ce6b716b05c8fcee
[D2017-04-06 13:36:14,395 682 139818590197568 subprocess2.py] git config rietveld.cc
[D2017-04-06 13:36:14,408 682 139818590197568 gerrit_util.py] POST https://chrome-internal-review.googlesource.com/a/changes/348164/reviewers
[D2017-04-06 13:36:14,408 682 139818590197568 gerrit_util.py] Content-Type: application/json
[D2017-04-06 13:36:14,408 682 139818590197568 gerrit_util.py] Authorization: HIDDEN
[D2017-04-06 13:36:14,408 682 139818590197568 gerrit_util.py] {"reviewer": "chrome-reviews@google.com", "state": "CC", "notify": "ALL"}
[D2017-04-06 13:36:14,744 682 139818590197568 gerrit_util.py] got response 422 for POST https://chrome-internal-review.googlesource.com/a/changes/348164/reviewers
[W2017-04-06 13:36:14,744 682 139818590197568 gerrit_util.py] Note: "chrome-reviews@google.com" not added as a cc
[D2017-04-06 13:36:14,749 682 139818590197568 gerrit_util.py] POST https://chrome-internal-review.googlesource.com/a/changes/348164/reviewers
[D2017-04-06 13:36:14,749 682 139818590197568 gerrit_util.py] Content-Type: application/json
[D2017-04-06 13:36:14,750 682 139818590197568 gerrit_util.py] Authorization: HIDDEN
[D2017-04-06 13:36:14,750 682 139818590197568 gerrit_util.py] {"reviewer": "chrome-infra-reviews+infra_internal@google.com", "state": "CC", "notify": "ALL"}
[D2017-04-06 13:36:15,072 682 139818590197568 gerrit_util.py] got response 422 for POST https://chrome-internal-review.googlesource.com/a/changes/348164/reviewers
[W2017-04-06 13:36:15,073 682 139818590197568 gerrit_util.py] Note: "chrome-infra-reviews+infra_internal@google.com" not added as a cc
[D2017-04-06 13:36:15,078 682 139818590197568 gerrit_util.py] POST https://chrome-internal-review.googlesource.com/a/changes/348164/reviewers
[D2017-04-06 13:36:15,078 682 139818590197568 gerrit_util.py] Content-Type: application/json
[D2017-04-06 13:36:15,078 682 139818590197568 gerrit_util.py] Authorization: HIDDEN
[D2017-04-06 13:36:15,078 682 139818590197568 gerrit_util.py] {"reviewer": "machenbach@google.com", "state": "CC", "notify": "ALL"}
[D2017-04-06 13:36:16,510 682 139818590197568 gerrit_util.py] got response 200 for POST https://chrome-internal-review.googlesource.com/a/changes/348164/reviewers
[D2017-04-06 13:36:16,513 682 139818590197568 subprocess2.py] git rev-parse HEAD
[D2017-04-06 13:36:16,520 682 139818590197568 subprocess2.py] git symbolic-ref HEAD
[D2017-04-06 13:36:16,526 682 139818590197568 subprocess2.py] git config branch.test.last-upload-hash 3e59db1017bad163c2d867a2f24ac7e97f5cf78e
...

As quite a few devs are still not yet on 2.10, and even ubuntu 16.04 is shipping just 2.9, (see https://groups.google.com/a/chromium.org/d/msg/chromium-dev/GN2mSweoOPk/aVdCA21_AwAJ), I'm marking this as available. My CL, which has been reverted, can be relanded any time.

Primiano@ also suggested to consider having a fallback to current state (ie using refspec suffix) for old git clients, but I think it'll make code, particularly tests ever more complex and less maintainable.

Moving everyone to git 2.11+ is infeasible before launch. We'd still like to do this -- or forego refspecs and push options entirely in favor of a single big api call -- but it doesn't seem to be important enough to block launch.

 Issue 721428  has been merged into this issue.

> forego refspecs and push options entirely in favor of a single big api call

What did you have in mind? I don't think Gerrit will ever support uploading arbitrary amounts of git repo data in a way that is not the git wire protocol.

Not git repo data, just metadata: patchset title, reviewers, ccs, notify mode. We can do all of that except patchset title in a single call right now.

Ah, well, if it were me, I'd prefer that all be an atomic operation along with creating the patch set, but suit yourself.

So would we. But we simply can't use push options until we can roll out a new version of git to everyone (hard), and we can't use the refspec options because we regularly exceed the length limit (and because that doesn't resolve the problem of special characters in patchset titles).

As a regular Gerrit user (I work in V8), this bites me several times a day. As a user, I don't know anything about the technical requirements, but from a UI perspective this seems like a noticeable regression from Rietveld. I suspect that as Chromium folks start picking up Gerrit this may become a hotter topic.

Ok, I just started a deep dive with iannucci@ into what it takes to roll out git>=2.10 to all our devs and bots. Have our work cut out for us but a clear path forward. Hopefully it won't take more than a week or two.

No promises though D:

 Issue 707963  has been merged into this issue.

Current state:
experiment to use cipd to deploy git to windows folks: https://chromium-review.googlesource.com/c/495632
experiment to use the same for linux: https://chromium-review.googlesource.com/c/506388
bugs blocking adoption of similar on mac: https://bugs.chromium.org/p/chromium/issues/detail?id=722476 and https://bugs.chromium.org/p/chromium/issues/detail?id=718868

 Issue 723706  has been merged into this issue.

 Issue gerrit:6251  has been merged into this issue.

I'm not sure what version we are aiming for now (can't tell from https://chromium-review.googlesource.com/c/495632 ) but hopefully it's not still 2.10 for Windows :).

I get a nasty segfault in git 2.10 on Windows trying to use gerrit features -- https://bugs.chromium.org/p/gerrit/issues/detail?id=6654

Upgrading to upgrading to 2.13.2.windows.1 got me past it.

 Issue gerrit:6815  has been merged into this issue.

Current bundled Git version is: version:2.13.3

I've starred the bug because I regularly need to put domain names in CL descriptions – those can be hard to decipher if you don't know what happened.

I'll try to remember to update the description in the web UI, but this is currently in the rare sweet spot of happening often enough to be annoying – yet infrequently enough for me to instinctively remember every time that it's broken. A permanent fix would certainly be appreciated (even if it relies on a local config). :-)

Can't 'git cl upload' just encode the string on the client and then decode it later when it gets out of refspec?

No, it's gerrit that does the decoding, not git-cl. The only support Gerrit has  is "underscores become spaces". I suppose we could ask Gerrit to add support for decoding base64 patchset names, but there would also have to be a flag to indicate that it should do so, and we'd have to use a non-standard base64 encoding since git refspecs can't contain slash, plus, or equals.

 Issue gerrit:7028  has been merged into this issue.

It may be worth just doing a post-push API call and then converting this bug to a performance-improvement bug :)

Or, a hybrid approach; detect if git is >= 2.10 and do that, otherwise print a warning "Your git is < 2.10, setting patchset with extra API call" or something.

 Issue gerrit:7109  has been merged into this issue.

Why not use percent encoding? git-check-ref-format doesn't seem to indicate that % is forbidden, and if a percent-encoded value happens to wind up in stdout or some UI surface, it's less of a catastrophe than base64.

It seems to work: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/676086

The corresponding server side change would be something like: modify com.google.gerrit.server.git.receive.ReceiveCommits.MagicBranchInput.addMessage as follows:

+ import static java.nio.charset.StandardCharsets.UTF_8;
+ import java.net.URLDecoder;

    @Option(
      name = "--message",
      aliases = {"-m"},
      metaVar = "MESSAGE",
      usage = "Comment message to apply to the review"
    )
    void addMessage(String token) {
      // git push does not allow spaces in refs.
-      message = token.replace("_", " ");
+      message = URLDecoder.decode(token.replace("_", " "), UTF_8.name());
    }

Can we just do this?

agable: ping -- what do you think about using percent encoding here?

I can't make that call -- that change would need to be made inside Gerrit, and they might have concerns about backwards compatibility. Also, % is the character that Gerrit uses to separate the "real" ref from all of the trailers (including the message token), so there may be some other parsing that would be broken by the presence of %s in the message.

I'd advise you to file a bug at crbug.com/gerrit or to upload your patch in Comment 53 for their direct review on gerrit-review.googlesource.com.

I've created https://gerrit-review.googlesource.com/c/gerrit/+/131331

https://gerrit-review.googlesource.com/c/gerrit/+/131331 has landed in gerrit.

Next step is to land https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/676086 in depot tools once our gerrit instance picks up the new functionality.  agable@, is there a scheduled next push for our gerrit instance?

I can clean up https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/676086 and send it for proper review.

Please send that CL for review; I'm happy to take a look.

That change won't be imported internally until tomorrow most likely, and it usually takes about 4 days for a full deployment cycle after an import. You can follow along from the links provided in go/life-of-a-gerrit-change.

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

commit 8692b18870fe10e274763b511e002e3e882bf30b
Author: Nick Carter <nick@chromium.org>
Date: Tue Nov 07 18:09:26 2017

Allow punctuation in patch set titles by means of percent encoding.

Bug:  663787 
Change-Id: Ie7e2fca42db0c840d713dc6edba5dc0c2b65223d
Reviewed-on: https://chromium-review.googlesource.com/676086
Reviewed-by: Aaron Gable <agable@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Nick Carter <nick@chromium.org>

[modify] https://crrev.com/8692b18870fe10e274763b511e002e3e882bf30b/tests/git_cl_test.py
[modify] https://crrev.com/8692b18870fe10e274763b511e002e3e882bf30b/git_cl.py
[modify] https://crrev.com/8692b18870fe10e274763b511e002e3e882bf30b/gerrit_util.py

PSA: This issue is fixed. If you're still encountering the bug / warning message, it should go away the next time you update your depot_tools (e.g. run 'gclient')