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

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

[Carlos O'Donell]

Florian,

If we ever switch to supporting Gerrit, then we will automatically
gain the Reviewed-by: lines depending on the configuration of Gerrit.
Reviewers would understand this as part of doing their reviews and
granting the review +1/+2.

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.

-- 
Cheers,
Carlos.

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

[Joseph Myers]

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.

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Carlos O'Donell]

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.

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

[Joseph Myers]

On Tue, 12 Nov 2019, Carlos O'Donell wrote:

>     - Gerrit does not close the review but adds a new patchset version
>       because the commit message changed.

I don't see this as a useful way for it to behave.

By including a Change-Id (that matches the Change-Id of something tracked 
in gerrit) in a commit pushed to master, the committer is making an 
assertion that what they are pushing is the latest version of the tracked 
change, and (by pushing) that it needs no further review.  On that basis, 
the review should automatically be closed, without needing to add any new 
patchset version.

This shouldn't just be about Reviewed-by.  If someone says a patch is OK 
with a specific change made to it (whether to the code or to the commit 
message), it should be enough to make that change and retest and push to 
master, without needing to go through extra administration in a review 
system just because of that change.

*Reducing* the amount of administration required is a good thing (for 
example, if we can develop a way for a commit message to say that the 
commit fixes a given bug, and for that to result in the bug being marked 
RESOLVED / FIXED with target milestone set automatically).  Increasing the 
amount of administration needed for a patch that's OK with changes doesn't 
seem a good idea to me; patch tracking systems should be reducing the work 
humans need to do rather than adding extra steps.

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Florian Weimer]

* Joseph Myers:

> On Tue, 12 Nov 2019, Carlos O'Donell wrote:
>
>>     - Gerrit does not close the review but adds a new patchset version
>>       because the commit message changed.
>
> I don't see this as a useful way for it to behave.
>
> By including a Change-Id (that matches the Change-Id of something tracked 
> in gerrit) in a commit pushed to master, the committer is making an 
> assertion that what they are pushing is the latest version of the tracked 
> change, and (by pushing) that it needs no further review.  On that basis, 
> the review should automatically be closed, without needing to add any new 
> patchset version.

I'm not yet decided here.

> This shouldn't just be about Reviewed-by.  If someone says a patch is OK 
> with a specific change made to it (whether to the code or to the commit 
> message), it should be enough to make that change and retest and push to 
> master, without needing to go through extra administration in a review 
> system just because of that change.

If you want to use Gerrit as some sort of audit trail (and we will
eventually face attacks on the GNU toolchain sources; maybe it has
already happened and we just haven't noticed yet), then of course it's
necessary that any tiny change gets re-reviewed.

If on the other hand it is just an optional tool to help a contributor
to produce their commit in a collaborative fashion, then it's indeed
silly to ask for a re-review once the contributor is satisfied with
what they've got.

But as discussed, for Gerrit in push mote and as far as Reviewed-By:
lines are concerned, this should be a non-issue.  It's a consequence
of our own limited use of Gerrit.

> *Reducing* the amount of administration required is a good thing (for 
> example, if we can develop a way for a commit message to say that the 
> commit fixes a given bug, and for that to result in the bug being marked 
> RESOLVED / FIXED with target milestone set automatically).  Increasing the 
> amount of administration needed for a patch that's OK with changes doesn't 
> seem a good idea to me; patch tracking systems should be reducing the work 
> humans need to do rather than adding extra steps.

Fully agreed.

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

[Joseph Myers]

On Tue, 12 Nov 2019, Florian Weimer wrote:

> > This shouldn't just be about Reviewed-by.  If someone says a patch is OK 
> > with a specific change made to it (whether to the code or to the commit 
> > message), it should be enough to make that change and retest and push to 
> > master, without needing to go through extra administration in a review 
> > system just because of that change.
> 
> If you want to use Gerrit as some sort of audit trail (and we will
> eventually face attacks on the GNU toolchain sources; maybe it has
> already happened and we just haven't noticed yet), then of course it's
> necessary that any tiny change gets re-reviewed.

We say maintainers can commit changes in their areas without needing 
review, and that certain kinds of changes are considered obvious for 
anyone with commit access.  So I don't think "OK with change X" (without 
needing re-review) is fundamentally different from that.

git provides the audit trail of exactly what changes went into the 
repository.  (And the glibc-cvs list shows which user account did the 
push, which is key information if it turns out a malicious change was 
pushed.  If we enable pushing from gerrit (with some set of gerrit users 
who also have write access being able to cause gerrit to push a change), 
we should consider how to track which gerrit account it was that caused a 
change to be pushed, in a similarly distributed way.)

> If on the other hand it is just an optional tool to help a contributor
> to produce their commit in a collaborative fashion, then it's indeed
> silly to ask for a re-review once the contributor is satisfied with
> what they've got.

I'm thinking of it as an optional tool to help the community review 
changes and track the state of changes being proposed (by providing the 
database of those that have yet to be pushed to master or abandoned, and 
possibly in future by providing other features such as automatic CI to 
help reviewers).

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Florian Weimer]

* Carlos O'Donell:

> If we ever switch to supporting Gerrit, then we will automatically
> gain the Reviewed-by: lines depending on the configuration of Gerrit.
> Reviewers would understand this as part of doing their reviews and
> granting the review +1/+2.
>
> 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.

We should not edit the commit message manually after it has been
reviewed.

Either it is automatically added by the review tool, or we need to
gather patch review statistics from the review tool.

The latter has the advantage that post-commit review also counts,
which is not possible if we only consider Reviewed-By: lines in
commits.

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

[Carlos O'Donell]

On 11/12/19 2:07 PM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> If we ever switch to supporting Gerrit, then we will automatically
>> gain the Reviewed-by: lines depending on the configuration of Gerrit.
>> Reviewers would understand this as part of doing their reviews and
>> granting the review +1/+2.
>>
>> 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.
> 
> We should not edit the commit message manually after it has been
> reviewed.

I agree. Should we setup gerrit to push to glibc git?

> Either it is automatically added by the review tool, or we need to
> gather patch review statistics from the review tool.

I would prefer it to be automatically added by the review tool, as
gerrit is designed to do.

> The latter has the advantage that post-commit review also counts,
> which is not possible if we only consider Reviewed-By: lines in
> commits.

Post-commit review is indeed lost in a system that uses commit
messages to track review, but I'd argue that this is a such a small
minority of the work that it's not worth accurately modeling in
the framework.

The reason I'm strongly in favor of Reviwed-by: in commit messages
is also that the review data goes with the commit, and clones of the
repo for analysis by any other 3rd party with normal git tooling.

-- 
Cheers,
Carlos.

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

[Florian Weimer]

* Carlos O'Donell:

>> We should not edit the commit message manually after it has been
>> reviewed.
>
> I agree. Should we setup gerrit to push to glibc git?

I think it's to early for that.  I'm not sure that Gerrit is the right
tool for us.  Too many of us have yet to use it as patch submitters
and reviewers.

I also think that improving the email interface is a dead end and not
worth the effort.  Unless we are willing to abandon email-based patch
review, we should not switch to Gerrit (or any other web-based tool).

Gerrit doesn't have to be in push mode in order to edited commit
messages.  You can press the DOWNLOAD button, copy the cherry-pick
command line into a shell, and use that to push a commit to master.
I've been doing that a couple of times and it works well.

>> Either it is automatically added by the review tool, or we need to
>> gather patch review statistics from the review tool.
>
> I would prefer it to be automatically added by the review tool, as
> gerrit is designed to do.

Is it?  If we need too many customizations, we'll be stuck with our
own special pet, which I don't want.

>> The latter has the advantage that post-commit review also counts,
>> which is not possible if we only consider Reviewed-By: lines in
>> commits.
>
> Post-commit review is indeed lost in a system that uses commit
> messages to track review, but I'd argue that this is a such a small
> minority of the work that it's not worth accurately modeling in
> the framework.

I'm not sure about that.  In many cases, I've missed Reviewed-By:
lines due to lack of tool support.  But there have been a few where I
simply had not received the review email message when producing the
actual commit.

> The reason I'm strongly in favor of Reviwed-by: in commit messages
> is also that the review data goes with the commit, and clones of the
> repo for analysis by any other 3rd party with normal git tooling.

I would argue this is a fringe use case.  How is this relevant to the
glibc project, anyway?  It's not that we're going to promote
contributors due to new roles based on the number of reviews they
conducted.

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

[Jonathan Nieder]

Florian Weimer wrote:

>>> We should not edit the commit message manually after it has been
>>> reviewed.
>>
>> I agree. Should we setup gerrit to push to glibc git?
>
> I think it's to early for that.

Makes sense.  Keep in mind that as Carlos has mentioned, Gerrit can
be used to manage ACLs for a Git repository without requiring review
(which means that people with a workflow based on pushing after an
on-mailing-list review wouldn't be broken by this).

[...]
>> I would prefer it to be automatically added by the review tool, as
>> gerrit is designed to do.
>
> Is it?

The Cherry Pick and Rebase Always submit strategies[1] automatically
add Reviewed-by footers.

Other submit strategies (like Merge If Necessary) are designed to not
touch the change uploader's commit (e.g. they may have signed it) so
they don't add the footer.

[...]
Carlos O'Donell wrote:

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

Also keep in mind that reviews can be made "sticky" on commit message
changes.  See [2] for more details.

There's a Gerrit hackathon[3] ongoing as we speak.  If you have any
questions you'd like me to relay to Gerrit devs or if you'd like to
meet up on IRC or video chat, let me know.

Thanks,
Jonathan

[1] https://gerrit-review.googlesource.com/Documentation/concept-changes.html#submit-strategies
[2] https://gerrit-review.googlesource.com/Documentation/config-labels.html#label_copyAllScoresIfNoCodeChange
[3] https://gerrit.googlesource.com/summit/2019/+/master/schedule-usa.md

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

[Florian Weimer]

* Jonathan Nieder:

>>> I would prefer it to be automatically added by the review tool, as
>>> gerrit is designed to do.
>>
>> Is it?
>
> The Cherry Pick and Rebase Always submit strategies[1] automatically
> add Reviewed-by footers.
>
> Other submit strategies (like Merge If Necessary) are designed to not
> touch the change uploader's commit (e.g. they may have signed it) so
> they don't add the footer.

> [1]
> https://gerrit-review.googlesource.com/Documentation/concept-changes.html#submit-strategies

I'm confused.  Is it expected that these changes are not visible in
the commits I can download (under the DOWNLOAD button) or in the web
UI?  Are they added only during the push?

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

[Jonathan Nieder]

Florian Weimer wrote:
> * Jonathan Nieder:

>>>> I would prefer it to be automatically added by the review tool, as
>>>> gerrit is designed to do.
>>>
>>> Is it?
>>
>> The Cherry Pick and Rebase Always submit strategies[1] automatically
>> add Reviewed-by footers.
>>
>> Other submit strategies (like Merge If Necessary) are designed to not
>> touch the change uploader's commit (e.g. they may have signed it) so
>> they don't add the footer.
>
>> [1]
>> https://gerrit-review.googlesource.com/Documentation/concept-changes.html#submit-strategies
>
> I'm confused.  Is it expected that these changes are not visible in
> the commits I can download (under the DOWNLOAD button) or in the web
> UI?  Are they added only during the push?

They are added at submit time, after the review.  At that point, they
show up as one final patch set.  See
https://chromium-review.googlesource.com/c/chromium/src/+/1900995 for
an example.  (The Cr-Commit-Position comes from a chromium-specific
plugin.)

You can use
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#submit-preview
to preview it, though that's a bit fussy.

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

[Florian Weimer]

* Jonathan Nieder:

> Florian Weimer wrote:
>> * Jonathan Nieder:
>
>>>>> I would prefer it to be automatically added by the review tool, as
>>>>> gerrit is designed to do.
>>>>
>>>> Is it?
>>>
>>> The Cherry Pick and Rebase Always submit strategies[1] automatically
>>> add Reviewed-by footers.
>>>
>>> Other submit strategies (like Merge If Necessary) are designed to not
>>> touch the change uploader's commit (e.g. they may have signed it) so
>>> they don't add the footer.
>>
>>> [1]
>>> https://gerrit-review.googlesource.com/Documentation/concept-changes.html#submit-strategies
>>
>> I'm confused.  Is it expected that these changes are not visible in
>> the commits I can download (under the DOWNLOAD button) or in the web
>> UI?  Are they added only during the push?
>
> They are added at submit time, after the review.  At that point, they
> show up as one final patch set.  See
> https://chromium-review.googlesource.com/c/chromium/src/+/1900995 for
> an example.  (The Cr-Commit-Position comes from a chromium-specific
> plugin.)
>
> You can use
> https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#submit-preview
> to preview it, though that's a bit fussy.

Ah, it wasn't clear to me if that was auto-generated.  Then it would
indeed help to enable push-to-sourceware for further tests.

I would like to see how Adhemerval's hppa pthread.h series moves
through review.  I will have further comments then.

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

[Carlos O'Donell]

On 11/12/19 3:14 PM, Florian Weimer wrote:
> Ah, it wasn't clear to me if that was auto-generated.  Then it would
> indeed help to enable push-to-sourceware for further tests.

Right, we could turn this on, but then we'd have to carefully tune the
set of reviewers, and split them into two groups.

> I would like to see how Adhemerval's hppa pthread.h series moves
> through review.  I will have further comments then.

Sounds good.

-- 
Cheers,
Carlos.

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

[Florian Weimer]

* Carlos O'Donell:

> On 11/12/19 3:14 PM, Florian Weimer wrote:
>> Ah, it wasn't clear to me if that was auto-generated.  Then it would
>> indeed help to enable push-to-sourceware for further tests.
>
> Right, we could turn this on, but then we'd have to carefully tune the
> set of reviewers, and split them into two groups.

The set of +2 reviewers is already restricted.  At least people
complained about this.

Can we use groups to manage CLA status?  We'll need two, one for
personal CLAs (can change email address) and one for employer CLAs
(may have to be revoked on email address change; we don't need to
start out with an automated process for that).

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

[Florian Weimer]

* Jonathan Nieder:

> There's a Gerrit hackathon[3] ongoing as we speak.  If you have any
> questions you'd like me to relay to Gerrit devs or if you'd like to
> meet up on IRC or video chat, let me know.

I've got a question: Is there a way to produce a direct link to the
signup page?

Our instance generates a signup link like this:

<https://gnutoolchain-gerrit.osci.io:8877/auth/realms/gerrit/login-actions/registration?client_id=gerrit&tab_id=h8iVt3Ny_2s>

But as you can see, it won't work in another browser session.

The reason I'm asking is that it would be nice to put a link to the
signup page into the documentation, so that would-be contributors can
just click on the link and create their account.

Maybe that's specific to the Keycloak integration we have, though.

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

[Carlos O'Donell]

On 11/12/19 3:48 PM, Florian Weimer wrote:
> * Jonathan Nieder:
> 
>> There's a Gerrit hackathon[3] ongoing as we speak.  If you have any
>> questions you'd like me to relay to Gerrit devs or if you'd like to
>> meet up on IRC or video chat, let me know.
> 
> I've got a question: Is there a way to produce a direct link to the
> signup page?
> 
> Our instance generates a signup link like this:
> 
> <https://gnutoolchain-gerrit.osci.io:8877/auth/realms/gerrit/login-actions/registration?client_id=gerrit&tab_id=h8iVt3Ny_2s>
> 
> But as you can see, it won't work in another browser session.
> 
> The reason I'm asking is that it would be nice to put a link to the
> signup page into the documentation, so that would-be contributors can
> just click on the link and create their account.

Agreed.

My long-term vision here is that we'd have a central GNU Toolchain
website where you can register, and then have direct OAuth2 from
there so it just works. That way your single account gets you access
to various GNU Toolchain services using OAuth2.

-- 
Cheers,
Carlos.

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

[Florian Weimer]

* Florian Weimer:

> * Jonathan Nieder:
>
>> There's a Gerrit hackathon[3] ongoing as we speak.  If you have any
>> questions you'd like me to relay to Gerrit devs or if you'd like to
>> meet up on IRC or video chat, let me know.
>
> I've got a question: Is there a way to produce a direct link to the
> signup page?
>
> Our instance generates a signup link like this:
>
> <https://gnutoolchain-gerrit.osci.io:8877/auth/realms/gerrit/login-actions/registration?client_id=gerrit&tab_id=h8iVt3Ny_2s>
>
> But as you can see, it won't work in another browser session.
>
> The reason I'm asking is that it would be nice to put a link to the
> signup page into the documentation, so that would-be contributors can
> just click on the link and create their account.
>
> Maybe that's specific to the Keycloak integration we have, though.

It seems to be a Keycloak thing.  This should work:

<https://gnutoolchain-gerrit.osci.io:8877/auth/realms/gerrit/protocol/openid-connect/registrations?client_id=gerrit&response_type=code&redirect_uri=https%3A%2F%2Fgnutoolchain-gerrit.osci.io%2Fr%2Foauth&scope=openid>

Does anyone who hasn't got an account yet want to try it? 8-)

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

[Carlos O'Donell]

On 11/12/19 4:10 PM, Florian Weimer wrote:
> * Florian Weimer:
> 
>> * Jonathan Nieder:
>>
>>> There's a Gerrit hackathon[3] ongoing as we speak.  If you have any
>>> questions you'd like me to relay to Gerrit devs or if you'd like to
>>> meet up on IRC or video chat, let me know.
>>
>> I've got a question: Is there a way to produce a direct link to the
>> signup page?
>>
>> Our instance generates a signup link like this:
>>
>> <https://gnutoolchain-gerrit.osci.io:8877/auth/realms/gerrit/login-actions/registration?client_id=gerrit&tab_id=h8iVt3Ny_2s>
>>
>> But as you can see, it won't work in another browser session.
>>
>> The reason I'm asking is that it would be nice to put a link to the
>> signup page into the documentation, so that would-be contributors can
>> just click on the link and create their account.
>>
>> Maybe that's specific to the Keycloak integration we have, though.
> 
> It seems to be a Keycloak thing.  This should work:
> 
> <https://gnutoolchain-gerrit.osci.io:8877/auth/realms/gerrit/protocol/openid-connect/registrations?client_id=gerrit&response_type=code&redirect_uri=https%3A%2F%2Fgnutoolchain-gerrit.osci.io%2Fr%2Foauth&scope=openid>
> 
> Does anyone who hasn't got an account yet want to try it? 8-)
> 

Just picking a name out of the hat randomly :-)

Siddhesh, Tulio,

Could you please try making an account to test the URL? :-)

-- 
Cheers,
Carlos.

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

[Siddhesh Poyarekar]

On 13/11/19 3:41 am, Carlos O'Donell wrote:
> Just picking a name out of the hat randomly :-)
> 
> Siddhesh, Tulio,
> 
> Could you please try making an account to test the URL? :-)
> 

Ah, I signed up a few days ago.

Siddhesh

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

[Tulio Magno Quites Machado Filho]

Carlos O'Donell <carlos@redhat.com> writes:

> On 11/12/19 4:10 PM, Florian Weimer wrote:
>> It seems to be a Keycloak thing.  This should work:
>> 
>> <https://gnutoolchain-gerrit.osci.io:8877/auth/realms/gerrit/protocol/openid-connect/registrations?client_id=gerrit&response_type=code&redirect_uri=https%3A%2F%2Fgnutoolchain-gerrit.osci.io%2Fr%2Foauth&scope=openid>
>> 
>> Does anyone who hasn't got an account yet want to try it? 8-)
>
> Just picking a name out of the hat randomly :-)
>
> Siddhesh, Tulio,
>
> Could you please try making an account to test the URL? :-)

I've just tested and it worked!

-- 
Tulio Magno

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

[Florian Weimer]

* Tulio Magno Quites Machado Filho:

> Carlos O'Donell <carlos@redhat.com> writes:
>
>> On 11/12/19 4:10 PM, Florian Weimer wrote:
>>> It seems to be a Keycloak thing.  This should work:
>>> 
>>> <https://gnutoolchain-gerrit.osci.io:8877/auth/realms/gerrit/protocol/openid-connect/registrations?client_id=gerrit&response_type=code&redirect_uri=https%3A%2F%2Fgnutoolchain-gerrit.osci.io%2Fr%2Foauth&scope=openid>
>>> 
>>> Does anyone who hasn't got an account yet want to try it? 8-)
>>
>> Just picking a name out of the hat randomly :-)
>>
>> Siddhesh, Tulio,
>>
>> Could you please try making an account to test the URL? :-)
>
> I've just tested and it worked!

Nice!  Arjun tried as well, but I think we are running into Red Hat's
inbound mail filters.  It's probably the same issue that stopped this
working for Arm.

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

[Carlos O'Donell]

On 11/12/19 2:59 PM, Jonathan Nieder wrote:
> There's a Gerrit hackathon[3] ongoing as we speak.  If you have any
> questions you'd like me to relay to Gerrit devs or if you'd like to
> meet up on IRC or video chat, let me know.

Jonathan,

One question I have, which Simon and I are discussing is:

How do you configure gerrit to support using a remote repository?

It would seem that it should be possible to run gerrit on box X, while
box Y hosts the git instance, but if this is not possible, then it isn't
entirely clear from the documentation.

Can replication be used for this feature?

-- 
Cheers,
Carlos.

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

[Joseph Myers]

On Tue, 12 Nov 2019, Carlos O'Donell wrote:

> How do you configure gerrit to support using a remote repository?
> 
> It would seem that it should be possible to run gerrit on box X, while
> box Y hosts the git instance, but if this is not possible, then it isn't
> entirely clear from the documentation.

Does this actually matter for our purposes?  That is, once the new 
sourceware system with a newer OS installation is in use, if we wish to 
use gerrit in production would there be problems with hosting it on 
sourceware?

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Florian Weimer]

* Joseph Myers:

> On Tue, 12 Nov 2019, Carlos O'Donell wrote:
>
>> How do you configure gerrit to support using a remote repository?
>> 
>> It would seem that it should be possible to run gerrit on box X, while
>> box Y hosts the git instance, but if this is not possible, then it isn't
>> entirely clear from the documentation.
>
> Does this actually matter for our purposes?  That is, once the new 
> sourceware system with a newer OS installation is in use, if we wish to 
> use gerrit in production would there be problems with hosting it on 
> sourceware?

I think it is prudent to avoid a dependency on the sourceware upgrade
if we can.

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

[Jonathan Nieder]

Hi Carlos,

Carlos O'Donell wrote:
> On 11/12/19 2:59 PM, Jonathan Nieder wrote:

>> There's a Gerrit hackathon[3] ongoing as we speak.  If you have any
>> questions you'd like me to relay to Gerrit devs or if you'd like to
>> meet up on IRC or video chat, let me know.
>
> Jonathan,
>
> One question I have, which Simon and I are discussing is:
>
> How do you configure gerrit to support using a remote repository?
>
> It would seem that it should be possible to run gerrit on box X, while
> box Y hosts the git instance, but if this is not possible, then it isn't
> entirely clear from the documentation.
>
> Can replication be used for this feature?

Sorry for the slow reply.  Gerrit really wants to "own" the branches
it writes to, since its role is that of an automated maintainer (that
is, it controls what can be pushed to each branch, vets whether there
has been adequate code review, and so on).  It copes fine with the
repository being updated behind its back but does not know how to do
any fancy two-way sync with a remote repository.

That means you could do one of the following things:

 1. Treat the gerrit-hosted repository as "source of truth", push all
    changes through there, and use the replication plugin to mirror to
    sourceware.org.

    1a. Install a pre-receive hook at sourceware.org to remind people
        to push to Gerrit instead.

    1b. Even better would be a hook at sourceware.org that forwards
	the push to Gerrit transparently.  Alas, Git's update hook
	doesn't provide a way to respond with "I performed the update
	on my own; please report success and skip performing the ref
	update", so this would require improving that part of Git's
	hook interface.

 2. Treat the sourceware.org repository as "source of truth" and use
    Gerrit for reviews only until you've installed Gerrit there (as
    you're doing now)

 3. Try to do something fancy with two-way sync.

At first glance, either (1a) or (2) looks sensible to me.  I'm happy
to help in any way I can (or get one of the more experienced Gerrit
admins here involved ;-)).

Thanks,
Jonathan

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

[Jonathan Nieder]

Jonathan Nieder wrote:

> That means you could do one of the following things:
>
>  1. Treat the gerrit-hosted repository as "source of truth", push all
>     changes through there, and use the replication plugin to mirror to
>     sourceware.org.
>
>     1a. Install a pre-receive hook at sourceware.org to remind people
>         to push to Gerrit instead.
>
>     1b. Even better would be a hook at sourceware.org that forwards
>         the push to Gerrit transparently.  Alas, Git's update hook
>         doesn't provide a way to respond with "I performed the update
>         on my own; please report success and skip performing the ref
>         update", so this would require improving that part of Git's
>         hook interface.
>
>  2. Treat the sourceware.org repository as "source of truth" and use
>     Gerrit for reviews only until you've installed Gerrit there (as
>     you're doing now)
>
>  3. Try to do something fancy with two-way sync.

Nassar (cc-ed) suggests two more options:

   4. Use NFS to share the repository between sourceware.org and
      gnutoolchain-gerrit.osci.io

   5. Redirect Git traffic from sourceware.org to
      gnutoolchain-gerrit.osci.io (using an HTTP 302 in apache config
      for https traffic, netcat for git://, ...)

If it's straightforward to set up NFS in this environment, then (4) is
a good option.

Option (5) might be fussy for people pushing over ssh.  (1a) seems
like a simpler approach in that direction.

Jonathan

> At first glance, either (1a) or (2) looks sensible to me.  I'm happy
> to help in any way I can (or get one of the more experienced Gerrit
> admins here involved ;-)).

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

[Carlos O'Donell]

On 11/12/19 2:39 PM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>>> We should not edit the commit message manually after it has been
>>> reviewed.
>>
>> I agree. Should we setup gerrit to push to glibc git?
> 
> I think it's to early for that.  I'm not sure that Gerrit is the right
> tool for us.  Too many of us have yet to use it as patch submitters
> and reviewers.

Is it too early though?

Enabling gerrit to push would not stop others from pushing to master.

It would just enable the tool to push to glibc git itself after reaching
a +2 review rating. And we could limit who can give the +2 reviews by
setting up groups accordingly.

I agree that more people should try it out as patch submitters and
reviewers.

I agree it's too early to tell if this is the tool we want to add to
our review toolbox.

> I also think that improving the email interface is a dead end and not
> worth the effort.  Unless we are willing to abandon email-based patch
> review, we should not switch to Gerrit (or any other web-based tool).

I agree that making gerrit suit *all* of our email needs is a dead end.

I do not think we will ever abandon email-based patch review.

I do not think that having email-based patch review blocks our use of
another tool for patch review.

Yes, it creates 2 tools, email and another tool, as valid ways to submit
patches, but I don't see that as a problem.

I think we should also be looking at Patchwork2.

> Gerrit doesn't have to be in push mode in order to edited commit
> messages.  You can press the DOWNLOAD button, copy the cherry-pick
> command line into a shell, and use that to push a commit to master.
> I've been doing that a couple of times and it works well.

Yes, but in push mode it can automatically add Reviewed-by: lines if
the reviewers sign off with +1 or +2.

>>> Either it is automatically added by the review tool, or we need to
>>> gather patch review statistics from the review tool.
>>
>> I would prefer it to be automatically added by the review tool, as
>> gerrit is designed to do.
> 
> Is it?  If we need too many customizations, we'll be stuck with our
> own special pet, which I don't want.

It is.

I agree about not wanting to own our own special version of gerrit.

It is specifically designed to be able to do this.

https://twitter.com/CarlosODonell/status/1192563350191902723

I see Jonathan mentioned this already.

>>> The latter has the advantage that post-commit review also counts,
>>> which is not possible if we only consider Reviewed-By: lines in
>>> commits.
>>
>> Post-commit review is indeed lost in a system that uses commit
>> messages to track review, but I'd argue that this is a such a small
>> minority of the work that it's not worth accurately modeling in
>> the framework.
> 
> I'm not sure about that.  In many cases, I've missed Reviewed-By:
> lines due to lack of tool support.  But there have been a few where I
> simply had not received the review email message when producing the
> actual commit.

It's hard to objectively quantify because the data is hard to get.

The only thing we can do is use a system that avoids it by adding the
reviewed-by lines automatically, like gerrit can do?

Staying on email-only patch reviews we will simply continue to have
the same issues.

>> The reason I'm strongly in favor of Reviwed-by: in commit messages
>> is also that the review data goes with the commit, and clones of the
>> repo for analysis by any other 3rd party with normal git tooling.
> 
> I would argue this is a fringe use case.  How is this relevant to the
> glibc project, anyway?  It's not that we're going to promote
> contributors due to new roles based on the number of reviews they
> conducted.

I'm not quite sure what you argue is a fringe use case? The point I made
about 3rd parties?

In general I see 3rd parties as my manager doing a review of what work
I'm doing upstream. People not involved in the project directly.

Why I want "Reviewed-by:" :

(a) Preserving the review attributions.

This history is something we have worked hard to preserve either through
ChangeLogs, CVS, or git. Having it in a distinct place like gerrit's
metadata may cause it to be lost (which is why I want records on the
mailing lists of the the reviews). The only real things which stand
the test of time so far are the mailing list archives, and the commit
messages (and not even that sometimes). I might argue the reviewed-by
should also go into the ChangeLog messages we generate (maybe it will).

How is this relevant to glibc? To the project itself?

(b) The relevance of recording reviewers.

Firstly, for me, it allows me to show my employer that I'm doing
something upstream. Likewise for other employees doing reviews.
These are objective ways to measure your reviews are helping the
project. It is a *measurable* thing, that can be measured
independently by a manager in a very quick way.

Secondly, it is an objective metric to look at project health and
to see how many reviewers there are working in the project (without
needing to parse emails manually). I look at Reviewed-by at every
release to see who is reviewing and if I've been increasing my own
numbers of reviews to help patch backlogs.

(c) The relevance of a Reviewed-by: line.

I'm not particularly tied to using Reviewed-by: lines in commit messages,
but I'd like some way to get the data and have it go with the commit so
we don't loose the data (see (a)).

(d) Taking action on what we see.

I was initially worried that we had few reviewers, but this year alone
we have 25 unique recorded reviewers. That's much better than I expected.
Next I want to see about encouraging those that reviewed when their reviews
were fewer than 1 a month, and I thought it might be a good idea. Perahaps
these reviewers might be interested in mentoring. Likewise contributors with
zero recorded reviews might be good candidates for mentoring also.

-- 
Cheers,
Carlos.

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

[Florian Weimer]

* Carlos O'Donell:

> On 11/12/19 2:39 PM, Florian Weimer wrote:
>> * Carlos O'Donell:
>> 
>>>> We should not edit the commit message manually after it has been
>>>> reviewed.
>>>
>>> I agree. Should we setup gerrit to push to glibc git?
>> 
>> I think it's to early for that.  I'm not sure that Gerrit is the right
>> tool for us.  Too many of us have yet to use it as patch submitters
>> and reviewers.
>
> Is it too early though?
>
> Enabling gerrit to push would not stop others from pushing to master.

I see.  If we agree that it's still provisional, I think we could
activate it.  Presumably we don't have to give it a full shell account
on sourceware.org.

>> I also think that improving the email interface is a dead end and not
>> worth the effort.  Unless we are willing to abandon email-based patch
>> review, we should not switch to Gerrit (or any other web-based tool).
>
> I agree that making gerrit suit *all* of our email needs is a dead end.
>
> I do not think we will ever abandon email-based patch review.
>
> I do not think that having email-based patch review blocks our use of
> another tool for patch review.
>
> Yes, it creates 2 tools, email and another tool, as valid ways to submit
> patches, but I don't see that as a problem.

If there are problems (like approvals or objections posted to the
mailing list) I think we can make adjustments once the need arises.
I agree it's hard to predict what will happen.

>>>> The latter has the advantage that post-commit review also counts,
>>>> which is not possible if we only consider Reviewed-By: lines in
>>>> commits.
>>>
>>> Post-commit review is indeed lost in a system that uses commit
>>> messages to track review, but I'd argue that this is a such a small
>>> minority of the work that it's not worth accurately modeling in
>>> the framework.
>> 
>> I'm not sure about that.  In many cases, I've missed Reviewed-By:
>> lines due to lack of tool support.  But there have been a few where I
>> simply had not received the review email message when producing the
>> actual commit.
>
> It's hard to objectively quantify because the data is hard to get.
>
> The only thing we can do is use a system that avoids it by adding the
> reviewed-by lines automatically, like gerrit can do?
>
> Staying on email-only patch reviews we will simply continue to have
> the same issues.

Sure, if it's a stock Gerrit feature, we should enable it.

>>> The reason I'm strongly in favor of Reviwed-by: in commit messages
>>> is also that the review data goes with the commit, and clones of the
>>> repo for analysis by any other 3rd party with normal git tooling.
>> 
>> I would argue this is a fringe use case.  How is this relevant to the
>> glibc project, anyway?  It's not that we're going to promote
>> contributors due to new roles based on the number of reviews they
>> conducted.
>
> I'm not quite sure what you argue is a fringe use case? The point I made
> about 3rd parties?
>
> In general I see 3rd parties as my manager doing a review of what work
> I'm doing upstream. People not involved in the project directly.

Well yes, but that's internal Red Hat politics. 8-p

> I was initially worried that we had few reviewers, but this year
> alone we have 25 unique recorded reviewers. That's much better than
> I expected.  Next I want to see about encouraging those that
> reviewed when their reviews were fewer than 1 a month, and I thought
> it might be a good idea. Perahaps these reviewers might be
> interested in mentoring. Likewise contributors with zero recorded
> reviews might be good candidates for mentoring also.

I don't doubt the usefulness of these statistics. What I was trying to
say is that maybe the review tool is the better place to gather review
statistics.  But if Reviewed-By: gets promoted automatically and
that's what you need, then by all means use it.

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

[Carlos O'Donell]

On 11/12/19 3:57 PM, Florian Weimer wrote:
> * Carlos O'Donell:
>> In general I see 3rd parties as my manager doing a review of what work
>> I'm doing upstream. People not involved in the project directly.
> 
> Well yes, but that's internal Red Hat politics. 8-p

If we see these politics, I expect others see them also, but worse.

My goal as a steward would be to look for win-win situations where
I can both satisfy a corporate demand to review work being done,
and at the project level thank those reviewers.

I think we are in rough agreement here that if gerrit can add the
reviewed-by lines then that's what we want.

We can turn it on experimentally.

I think we *will* have to give the instances a way to push without
a shell account, and I'm not sure how to do that.

-- 
Cheers,
Carlos.

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

[Florian Weimer]

* Carlos O'Donell:

> I think we *will* have to give the instances a way to push without
> a shell account, and I'm not sure how to do that.

I think that's the default sourceware.org setup.  The login shell
seems to be /sourceware/infra/bin/rash.  I think there are scripts to
set this up.  We just need to bypass any email challenges.  I expect
Frank knows all the details.

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

[Carlos O'Donell]

On 11/12/19 4:19 PM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> I think we *will* have to give the instances a way to push without
>> a shell account, and I'm not sure how to do that.
> 
> I think that's the default sourceware.org setup.  The login shell
> seems to be /sourceware/infra/bin/rash.  I think there are scripts to
> set this up.  We just need to bypass any email challenges.  I expect
> Frank knows all the details.

I'll follow up with Frank on the sourceware side, and Simon on the gerrit side.

-- 
Cheers,
Carlos.

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

[Florian Weimer]

* Carlos O'Donell:

> On 11/12/19 4:19 PM, Florian Weimer wrote:
>> * Carlos O'Donell:
>> 
>>> I think we *will* have to give the instances a way to push without
>>> a shell account, and I'm not sure how to do that.
>> 
>> I think that's the default sourceware.org setup.  The login shell
>> seems to be /sourceware/infra/bin/rash.  I think there are scripts to
>> set this up.  We just need to bypass any email challenges.  I expect
>> Frank knows all the details.
>
> I'll follow up with Frank on the sourceware side, and Simon on the
> gerrit side.

Thanks.  This is really exciting.

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

[Carlos O'Donell]

On 11/12/19 4:21 PM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> On 11/12/19 4:19 PM, Florian Weimer wrote:
>>> * Carlos O'Donell:
>>>
>>>> I think we *will* have to give the instances a way to push without
>>>> a shell account, and I'm not sure how to do that.
>>>
>>> I think that's the default sourceware.org setup.  The login shell
>>> seems to be /sourceware/infra/bin/rash.  I think there are scripts to
>>> set this up.  We just need to bypass any email challenges.  I expect
>>> Frank knows all the details.
>>
>> I'll follow up with Frank on the sourceware side, and Simon on the
>> gerrit side.
> 
> Thanks.  This is really exciting.
> 

For clarity I won't get to this until next week though.

-- 
Cheers,
Carlos.

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

[Florian Weimer]

* Carlos O'Donell:

> On 11/12/19 4:21 PM, Florian Weimer wrote:
>> * Carlos O'Donell:
>> 
>>> On 11/12/19 4:19 PM, Florian Weimer wrote:
>>>> * Carlos O'Donell:
>>>>
>>>>> I think we *will* have to give the instances a way to push without
>>>>> a shell account, and I'm not sure how to do that.
>>>>
>>>> I think that's the default sourceware.org setup.  The login shell
>>>> seems to be /sourceware/infra/bin/rash.  I think there are scripts to
>>>> set this up.  We just need to bypass any email challenges.  I expect
>>>> Frank knows all the details.
>>>
>>> I'll follow up with Frank on the sourceware side, and Simon on the
>>> gerrit side.
>> 
>> Thanks.  This is really exciting.
>
> For clarity I won't get to this until next week though.

Understood.  We also need a conversation if we want to share account
names between this Gerrit instance and sourceware.org.