git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: Using Gerrit to review Git patches (was: Re: Transaction patch series overview)

From
Shawn Pearce <spearce@spearce.org>
Date
Aug 28, 2014, 05:07 UTC
Message-ID
<CAJo=hJv7+iMHC7rO1w2Hmnq4Ho3W0hwh77Z_gRvY4hzE=VnxGQ@mail.gmail.com>
In-Reply-To
<53FE5359.4030403@alum.mit.edu>
On Wed, Aug 27, 2014 at 2:53 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
Show 25 quoted lines
> On 08/26/2014 02:03 AM, Jonathan Nieder wrote:
>> Jonathan Nieder wrote:
>> [...]
>>> I've having trouble keeping track of how patches change, which patches
>>> have been reviewed and which haven't, unaddressed comments, and so on,
>>> so as an experiment I've pushed this part of the series to the Gerrit
>>> server at
>>>
>>>  https://code-review.googlesource.com/#/q/topic:ref-transaction-1
>>
>> Outcome of the experiment: patches gained some minor changes and then
>> [...]
>>
>> I found the web UI helpful in seeing how each patch evolved since I
>> last looked at it.  Interdiff relative to the version in pu is below.
>> I'm still hoping for a tweak in response to a minor comment and then I
>> can put up a copy of the updated series to pull.
>
> Thanks for organizing this "experiment". I was one of the guinea pigs
> :-) I have wanted to review more of Ronnie's patches (actually, all of
> them!) but have been overwhelmed by the number of iterations and the
> number of patch series flying around in parallel. I was also interested
> to try out Gerrit, which I haven't used before. So I took up Jonathan's
> invitation and reviewed the first patch series in Gerrit. Here are some
> of my first impressions.

Thank you for making this writeup. It contains many valuable nuggets that we should follow up on.

Show 12 quoted lines
> * Overall, I found it easier to review commits in Gerrit than on the
> mailing list, especially a long patch series like this one that has seen
> so much flux. It was easier to see the comments from all reviewers that
> apply to a patch, which is difficult on the mailing list when comments
> are scattered over the many iterations of the patch series. It was
> easier to incrementally increase the context around a patch. It was easy
> to use the copy-paste commands provided in the "download" menu to fetch
> the commit that I was reviewing into my local Git repository, and from
> there to build it or investigate it using other tools.
>
> * The Gerrit interface is very busy. It was somewhat overwhelming to me
> as a beginner.

Very astute observation. I have been using it for 5+ years so its hard for me to see things like this.

Unfortunately many parts of the UI have grown over the years. Contributors come along and want to add X, months later another adds field Y. Suddenly we have the entire alphabet and we don't know how it got that way. Sounds like git reset --{hard|soft|mixed|wtf}. :)

Its not an excuse. The UI should be less overwhelming. Its going to be a challenge.

> On the other hand, the help menus ("?" key) are good and
> the keyboard shortcuts are convenient. I didn't have to read much
> documentation to get started doing review in Gerrit, at least at a basic
> level.
Yay!
Show 5 quoted lines
> * During two of my big Gerrit sessions the website was very responsive
> and pleasant to use. During the third, it was terribly slow, like 5 - 15
> seconds per page update. If I had only experienced the slow behavior, I
> would have rejected Gerrit immediately. I hope that the slow behavior
> was a rare anomaly.

This third session was a rare anomaly unique to code-review.googlesource.com. It isn't typical for Gerrit Code Review.

We have a server set in Europe that was mostly answering you in the first two sessions. During the third session this server set was temporarily offline. Requests degraded to a server set in the US, which pushed us into a worst-case scenario due to a misconfiguration of our load balancers. The lag you experienced was mostly due to this misconfiguration, not EU-US networking.

Earlier today I brought back up the European server set, so latency should be reduced again.

Unfortunately I haven't figured out how to fix the load balancer misconfiguration. :(

> * Gerrit sends out an endless flood of emails that mostly seem pretty
> useless to me. I wish it weren't so chatty and that its emails were
> better organized.
Absolutely agree.
> * At one point a back-and-forth in a line comment grew into a more
> general issue that was more appropriate for the mailing list. The
> transition from Gerrit to mailing list was a bit awkward.

We find this a bit awkward in JGit too. Our solution has been to (mostly) keep discussion within the code review context. This has to some extent forced more discussion to be around specific code, and avoid wandering off into the philosophical weeds. Without concrete code to talk about, there is nothing to say. :)

A nice feature of Gerrit is another contributor can add their own iterations to the same commit in the series, offering up alternative proposals. Like on the mailing list, this allows someone to just say "what if you did it this way...", and give everyone a concrete chunk of code to consider and discuss. Again this avoids wandering off into the weeds.

> So, overall I found it pleasant and efficient to review patches in
> Gerrit. I would welcome more such "experiments". It would have been even
> better if Gerrit would generate more useful notification emails.

One idea has been to just stop sending automatic emails, and instead require the author to send a message saying "please take another look at my series". Just disable the #@!(@(! automatic email delivery that stems out of `git push ...` creating or updating pending reviews.

Unfortunately there are factions within the Gerrit contributor base that find value in those automated emails. :(

Previous: Michael HaggertyNext: Jonathan Nieder
Message 57 of 129 in “Transaction patch series overview”
  1. Ronnie SahlbergJul 30, 2014
  2. Ronnie SahlbergJul 31, 2014
  3. Ronnie SahlbergAug 8, 2014
  4. Ronnie SahlbergAug 19, 2014
  5. Junio C HamanoAug 19, 2014
  6. Jonathan NiederAug 20, 2014
  7. Jonathan NiederAug 26, 2014
  8. Junio C HamanoAug 26, 2014
  9. Jonathan NiederAug 26, 2014
  10. 0/20 rs/ref-transaction-1 (Re: Transaction patch series overview)Jonathan Nieder, Aug 27, 2014
  11. 01/20 refs.c: change ref_transaction_create to do error checking and return statusJonathan Nieder, Aug 27, 2014
  12. 02/20 refs.c: update ref_transaction_delete to check for error and return statusJonathan Nieder, Aug 27, 2014
  13. 03/20 refs.c: make ref_transaction_begin take an err argumentJonathan Nieder, Aug 27, 2014
  14. 04/20 refs.c: add transaction.status and track OPEN/CLOSEDJonathan Nieder, Aug 27, 2014
  15. 05/20 tag.c: use ref transactions when doing updatesJonathan Nieder, Aug 27, 2014
  16. 06/20 replace.c: use the ref transaction functions for updatesJonathan Nieder, Aug 27, 2014
  17. 07/20 commit.c: use ref transactions for updatesJonathan Nieder, Aug 27, 2014
  18. 08/20 sequencer.c: use ref transactions for all ref updatesJonathan Nieder, Aug 27, 2014
  19. 09/20 fast-import.c: change update_branch to use ref transactionsJonathan Nieder, Aug 27, 2014
  20. 10/20 branch.c: use ref transaction for all ref updatesJonathan Nieder, Aug 27, 2014
  21. 11/20 refs.c: change update_ref to use a transactionJonathan Nieder, Aug 27, 2014
  22. 12/20 receive-pack.c: use a reference transaction for updating the refsJonathan Nieder, Aug 27, 2014
  23. 13/20 fast-import.c: use a ref transaction when dumping tagsJonathan Nieder, Aug 27, 2014
  24. 14/20 walker.c: use ref transaction for ref updatesJonathan Nieder, Aug 27, 2014
  25. 15/20 refs.c: make lock_ref_sha1 staticJonathan Nieder, Aug 27, 2014
  26. 16/20 refs.c: remove the update_ref_lock functionJonathan Nieder, Aug 27, 2014
  27. 17/20 refs.c: remove the update_ref_write functionJonathan Nieder, Aug 27, 2014
  28. 18/20 refs.c: remove lock_ref_sha1Jonathan Nieder, Aug 27, 2014
  29. 19/20 refs.c: make prune_ref use a transaction to delete the refJonathan Nieder, Aug 27, 2014
  30. 20/20 refs.c: make delete_ref use a transactionJonathan Nieder, Aug 27, 2014
  31. Junio C HamanoAug 27, 2014
  32. Junio C HamanoAug 27, 2014
  33. 0/22 rs/ref-transaction-1 (Re: Transaction patch series overview)Jonathan Nieder, Sep 2, 2014
  34. 01/22 refs.c: change ref_transaction_create to do error checking and return statusJonathan Nieder, Sep 2, 2014
  35. 02/22 refs.c: update ref_transaction_delete to check for error and return statusJonathan Nieder, Sep 2, 2014
  36. 03/22 refs.c: make ref_transaction_begin take an err argumentJonathan Nieder, Sep 2, 2014
  37. 04/22 refs.c: add transaction.status and track OPEN/CLOSEDJonathan Nieder, Sep 2, 2014
  38. 05/22 tag.c: use ref transactions when doing updatesJonathan Nieder, Sep 2, 2014
  39. 06/22 replace.c: use the ref transaction functions for updatesJonathan Nieder, Sep 2, 2014
  40. 07/22 commit.c: use ref transactions for updatesJonathan Nieder, Sep 2, 2014
  41. 08/22 sequencer.c: use ref transactions for all ref updatesJonathan Nieder, Sep 2, 2014
  42. 09/22 fast-import.c: change update_branch to use ref transactionsJonathan Nieder, Sep 2, 2014
  43. 10/22 branch.c: use ref transaction for all ref updatesJonathan Nieder, Sep 2, 2014
  44. 11/22 refs.c: change update_ref to use a transactionJonathan Nieder, Sep 2, 2014
  45. 12/22 receive-pack.c: use a reference transaction for updating the refsJonathan Nieder, Sep 2, 2014
  46. 13/22 fast-import.c: use a ref transaction when dumping tagsJonathan Nieder, Sep 2, 2014
  47. 14/22 walker.c: use ref transaction for ref updatesJonathan Nieder, Sep 2, 2014
  48. 15/22 refs.c: make lock_ref_sha1 staticJonathan Nieder, Sep 2, 2014
  49. 16/22 refs.c: remove the update_ref_lock functionJonathan Nieder, Sep 2, 2014
  50. 17/22 refs.c: remove the update_ref_write functionJonathan Nieder, Sep 2, 2014
  51. 18/22 refs.c: remove lock_ref_sha1Jonathan Nieder, Sep 2, 2014
  52. 19/22 refs.c: make prune_ref use a transaction to delete the refJonathan Nieder, Sep 2, 2014
  53. 20/22 refs.c: make delete_ref use a transactionJonathan Nieder, Sep 2, 2014
  54. 21/22 update-ref --stdin: narrow scope of err strbufJonathan Nieder, Sep 2, 2014
  55. 22/22 update-ref --stdin: pass transaction around explicitlyJonathan Nieder, Sep 2, 2014
  56. Using Gerrit to review Git patches (was: Re: Transaction patch series overview)Michael Haggerty, Aug 27, 2014
  57. Shawn PearceAug 28, 2014
  58. 0/19 rs/ref-transaction (Re: Transaction patch series overview)Jonathan Nieder, Sep 11, 2014
  59. 01/19 mv test: recreate mod/ directory instead of relying on stale copyJonathan Nieder, Sep 11, 2014
  60. 02/19 wrapper.c: remove/unlink_or_warn: simplify, treat ENOENT as successJonathan Nieder, Sep 11, 2014
  61. 03/19 wrapper.c: add a new function unlink_or_msgJonathan Nieder, Sep 11, 2014
  62. 04/19 refs.c: add an err argument to delete_ref_looseJonathan Nieder, Sep 11, 2014
  63. 05/19 refs.c: pass the ref log message to _create/delete/update instead of _commitJonathan Nieder, Sep 11, 2014
  64. 06/19 rename_ref: don't ask read_ref_full where the ref came fromJonathan Nieder, Sep 11, 2014
  65. 07/19 refs.c: move the check for valid refname to lock_ref_sha1_basicJonathan Nieder, Sep 11, 2014
  66. 08/19 refs.c: call lock_ref_sha1_basic directly from commitJonathan Nieder, Sep 11, 2014
  67. 09/19 refs.c: pass a skip list to name_conflict_fnJonathan Nieder, Sep 11, 2014
  68. 10/19 refs.c: ref_transaction_commit: distinguish name conflicts from other errorsJonathan Nieder, Sep 11, 2014
  69. 11/19 fetch.c: change s_update_ref to use a ref transactionJonathan Nieder, Sep 11, 2014
  70. 12/19 refs.c: make write_ref_sha1 staticJonathan Nieder, Sep 11, 2014
  71. 13/19 refs.c: change resolve_ref_unsafe reading argument to be a flags fieldJonathan Nieder, Sep 11, 2014
  72. 14/19 branch -d: avoid repeated symref resolutionJonathan Nieder, Sep 11, 2014
  73. 15/19 refs.c: fix handling of badly named refsJonathan Nieder, Sep 11, 2014
  74. 16/19 for-each-ref.c: improve message before aborting on broken refJonathan Nieder, Sep 11, 2014
  75. 17/19 refs.c: do not permit err == NULLJonathan Nieder, Sep 11, 2014
  76. 18/19 lockfile: remove unable_to_lock_errorJonathan Nieder, Sep 11, 2014
  77. 19/19 ref_transaction_commit: bail out on failure to remove a refJonathan Nieder, Sep 11, 2014
  78. Junio C HamanoSep 11, 2014
  79. Junio C HamanoSep 11, 2014
  80. Jonathan NiederSep 12, 2014
  81. Junio C HamanoSep 12, 2014
  82. Jonathan NiederSep 12, 2014
  83. Junio C HamanoSep 12, 2014
  84. Junio C HamanoSep 12, 2014
  85. Junio C HamanoSep 13, 2014
  86. Michael HaggertySep 12, 2014
  87. Jonathan NiederSep 12, 2014
  88. Michael HaggertySep 17, 2014
  89. Junio C HamanoSep 18, 2014
  90. Jonathan NiederSep 18, 2014
  91. Junio C HamanoSep 18, 2014
  92. Jonathan NiederSep 18, 2014
  93. Junio C HamanoSep 25, 2014
  94. Jonathan NiederSep 25, 2014
  95. Junio C HamanoSep 25, 2014
  96. 0/24 rs/ref-transactionJonathan Nieder, Oct 2, 2014
  97. 01/24 mv test: recreate mod/ directory instead of relying on stale copyJonathan Nieder, Oct 2, 2014
  98. 02/24 wrapper.c: remove/unlink_or_warn: simplify, treat ENOENT as successJonathan Nieder, Oct 2, 2014
  99. 03/24 wrapper.c: add a new function unlink_or_msgJonathan Nieder, Oct 2, 2014
  100. 04/24 refs.c: add an err argument to delete_ref_looseJonathan Nieder, Oct 2, 2014
  101. 05/24 refs.c: pass the ref log message to _create/delete/update instead of _commitJonathan Nieder, Oct 2, 2014
  102. 06/24 rename_ref: don't ask read_ref_full where the ref came fromJonathan Nieder, Oct 2, 2014
  103. 07/24 refs.c: refuse to lock badly named refs in lock_ref_sha1_basicJonathan Nieder, Oct 2, 2014
  104. 08/24 refs.c: call lock_ref_sha1_basic directly from commitJonathan Nieder, Oct 2, 2014
  105. 09/24 refs.c: pass a list of names to skip to is_refname_availableJonathan Nieder, Oct 2, 2014
  106. Junio C HamanoOct 2, 2014
  107. Jonathan NiederOct 3, 2014
  108. Junio C HamanoOct 3, 2014
  109. 09/24 refs.c: pass a list of names to skip to is_refname_availableJonathan Nieder, Oct 3, 2014
  110. Junio C HamanoOct 7, 2014
  111. 10/24 refs.c: ref_transaction_commit: distinguish name conflicts from other errorsJonathan Nieder, Oct 2, 2014
  112. 11/24 fetch.c: change s_update_ref to use a ref transactionJonathan Nieder, Oct 2, 2014
  113. 12/24 refs.c: make write_ref_sha1 staticJonathan Nieder, Oct 2, 2014
  114. 13/24 refs.c: change resolve_ref_unsafe reading argument to be a flags fieldJonathan Nieder, Oct 2, 2014
  115. 14/24 reflog test: test interaction with detached HEADJonathan Nieder, Oct 2, 2014
  116. 15/24 branch -d: avoid repeated symref resolutionJonathan Nieder, Oct 2, 2014
  117. 16/24 branch -d: simplify by using RESOLVE_REF_READING flagJonathan Nieder, Oct 2, 2014
  118. 17/24 packed-ref cache: forbid dot-components in refnamesJonathan Nieder, Oct 2, 2014
  119. 18/24 test: put tests for handling of bad ref names in one placeJonathan Nieder, Oct 2, 2014
  120. 19/24 refs.c: allow listing and deleting badly named refsJonathan Nieder, Oct 2, 2014
  121. Junio C HamanoOct 2, 2014
  122. Ronnie SahlbergOct 3, 2014
  123. Ronnie SahlbergOct 3, 2014
  124. Junio C HamanoOct 3, 2014
  125. 20/24 for-each-ref.c: improve message before aborting on broken refJonathan Nieder, Oct 2, 2014
  126. 21/24 remote rm/prune: print a message when writing packed-refs failsJonathan Nieder, Oct 2, 2014
  127. 22/24 refs.c: do not permit err == NULLJonathan Nieder, Oct 2, 2014
  128. 23/24 lockfile: remove unable_to_lock_errorJonathan Nieder, Oct 2, 2014
  129. 24/24 ref_transaction_commit: bail out on failure to remove a refJonathan Nieder, Oct 2, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.