Re: How to keep Reviewed-by lines in git commits with gerrit.

[Carlos O'Donell <carlos at redhat dot com>]

On 11/12/19 12:40 PM, Joseph Myers wrote:
> On Tue, 12 Nov 2019, Carlos O'Donell wrote:
> 
>> In the meantime I can do two things as a reviewer to help you keep
>> the Reviewed-by lines.
>>
>> 1. If I am about to grant +2 review I edit the commit and generate
>>    a new patchset version, and add my Reviewed-by line.
>> 2. I submit my review of +2.
>>
>> Then when you push, you just need to make sure your new commit message
>> matches and it should close the review.
> 
> That seems overly complicated. I thought the Change-Id was how gerrit 
> told whether something was the same change, so would have expected that 
> simply adding the Reviewed-by to the commit message when doing the final 
> commit, but keeping the same Change-Id, would suffice, without extra patch 
> set versions being needed unless there is an actual need for a new version 
> to be reviewed.

The Change-Id allows tracking.

The determination of a patchset being the "same" involves making
sure the commit message is unchanged (SHA1 hash doesn't change).

So if you add a Reviewed-by line, then it generates a new patchset that
needs review again because the commit message changed. No analysis of the
semantics of the change are carried out, and I can agree that this analysis
is not always trivial.

All of this is a consequence of the "experimental" nature of our gerrit
instance. In a scenario where Gerrit did the final push, we would not have
this problem. Gerrit itself adds the Reviewed-by: based on the actual reviewers
granting review, and so you never have this problem.

The problem arises in a mixed tool usage:

* Submit patch to review in gerrit, but push directly with altered commit message.
    - Patch review is accepted.
    - Reviewed-by lines are given.
    - Final commit message SHA1 changes because Reviewed-by lines
      change the commit message.
    - Gerrit does not close the review but adds a new patchset version
      because the commit message changed.

The options are:

(a) Do not modify the commit message after review is granted.
    - I don't like this because you loose the Reviewed-by: additions.

(b) Have the reviewer add their Reviewed-by: line to gerrit commit,
    hit save, *then* grant the +2.
    - Requires committer alter the commit message in the same way.
    - More work for "experimental" uses of gerrit.

I dislike (a) because it causes us to loose tracking about reviewers.
It is hard to get reviewers, and this tracking is incentive for them and
allows me to point out to their employers the good work they are doing.

I'm happy to use (b) while we experiment.

I would much rather see this scenario:

(1) If you submit to gerrit, then gerrit does the final push.
- Gerrit adds the Reviewed-by lines automatically. No extra work.

(2) If you submit to the mailing list, then push directly.
- Adding Reviewed-by lines from your reviewers (git commit --amend).

That is you pick a path and stay on it without mixing the workflows
for the *final commit*. We can mix reviewing to some degree by email
followup.

We have a mixed problem today because we haven't enabled gerrit to
push to glibc git's repo.

Does that clarify my position?

-- 
Cheers,
Carlos.