Setup non-pushing gerrit instance for glibc.

Setup non-pushing gerrit instance for glibc.

[Carlos O'Donell]

Community,

With the gracious help of Sergio and Simon I have setup a glibc mirror
on a gerrit instance that *cannot* push to sourceware git.

e.g.

git clone ssh://codonell@gnutoolchain-gerrit.osci.io:29418/glibc
< Commit some work >
# First time only, you need a hook to inject commit IDs for tracking.
gitdir=$(git rev-parse --git-dir); scp -p -P 29418 codonell@gnutoolchain-gerrit.osci.io:hooks/commit-msg ${gitdir}/hooks/
git commit --amend --no-edit
###
git push origin HEAD:refs/for/master

The "refs/for/master" is the way to push commits to be applied to master.
---

The purpose of this is to:

* Try out code reviews / CI etc. in gerrit.
  - Code review of patches pushed to gerrit is possible.

* Try to setup email based code review in gerrit.
  - Currently email is outbound only.
  - We are looking at how to make it entirely email based, but tracked
    in gerrit.

Enjoy! :-)

-- 
Cheers,
Carlos.

Re: Setup non-pushing gerrit instance for glibc.

[Joseph Myers]

On Thu, 24 Oct 2019, Carlos O'Donell wrote:

> * Try to setup email based code review in gerrit.
>   - Currently email is outbound only.

Observations on teething troubles with the initial setup:

1. What's the status of fixing the problem with insufficient diff context 
in emails when comments relate to particular parts of the diff?  They need 
to quote the relevant amount of context (typically at least the whole diff 
hunk, including the hunk header showing the changed function name) rather 
than just one line and a reference to an external URL.  It's important for 
messages with reviews to be meaningful in themselves without depending on 
external links.  This is a longstanding problem (it was obvious in some 
experiments some people did with proposing GCC patches with Rietveld, 
gerrit's predecessor, several years ago).  Someone in the GDB discussions 
mentioned a prototype patch to add some context to the emails, so maybe we 
could use that patch.

2. Could text comments in emails from gerrit be properly wrapped?  
Messages such as 
<https://sourceware.org/ml/libc-alpha/2019-10/msg00715.html> are hard to 
read in the list archives because of very long lines.  (Of course, diff 
context / quoted source lines should not be wrapped.)

3. Could we document (on the wiki, I suppose) the process for setting up 
git remotes if you want a git repository to get local copies of all 
versions of all changes proposed this way?  My understanding is gerrit 
makes those available as refs named in some particular way, so adding a 
remote with appropriate fetch config should work, but the particular 
recipe ought to be documented.

4. Could we document how to get and keep up to date a complete local copy 
of all the glibc review data in gerrit (comments etc.) using whatever APIs 
are available?  Again, I think the relevant APIs already exist, but how to 
use them for glibc ought to be documented (this is especially the case for 
a service like this that's experimental, and thus might go away in 
future).

5. It would also be useful to have documentation for how someone should 
make a patch series appear appropriately in gerrit if they want to propose 
a series that way.  That means the emails for a patch series should 
include 1/N, 2/N etc. in their subjects (with a 0/N cover letter as 
appropriate).

6. Lower priority, but note there are certain kinds of changes involving 
huge diffs (e.g. to generated files) that thus *would* need a message size 
limit and pointing to a URL for the diffs in that case, for it to be 
possible to handle such changes through gerrit.  (When sending email 
manually for such a change one can always omit the 50 MB of diffs to 
generated files, but as I understand it part of the point of such a patch 
review system is that the system seems the exact change proposed to be 
committed, complete with generated files, so enabling automated testing of 
patch proposals if desired.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Setup non-pushing gerrit instance for glibc.

[Ian Kent]

On Thu, 2019-10-24 at 21:41 +0000, Joseph Myers wrote:
> On Thu, 24 Oct 2019, Carlos O'Donell wrote:
> 
> > * Try to setup email based code review in gerrit.
> >   - Currently email is outbound only.
> 
> Observations on teething troubles with the initial setup:
> 
> 1. What's the status of fixing the problem with insufficient diff
> context 
> in emails when comments relate to particular parts of the diff?  They
> need 
> to quote the relevant amount of context (typically at least the whole
> diff 
> hunk, including the hunk header showing the changed function name)
> rather 
> than just one line and a reference to an external URL.  It's
> important for 
> messages with reviews to be meaningful in themselves without
> depending on 
> external links.  This is a longstanding problem (it was obvious in
> some 
> experiments some people did with proposing GCC patches with
> Rietveld, 
> gerrit's predecessor, several years ago).  Someone in the GDB
> discussions 
> mentioned a prototype patch to add some context to the emails, so
> maybe we 
> could use that patch.
> 
> 2. Could text comments in emails from gerrit be properly wrapped?  
> Messages such as 
> <https://sourceware.org/ml/libc-alpha/2019-10/msg00715.html> are hard
> to 
> read in the list archives because of very long lines.  (Of course,
> diff 
> context / quoted source lines should not be wrapped.)
> 
> 3. Could we document (on the wiki, I suppose) the process for setting
> up 
> git remotes if you want a git repository to get local copies of all 
> versions of all changes proposed this way?  My understanding is
> gerrit 
> makes those available as refs named in some particular way, so adding
> a 
> remote with appropriate fetch config should work, but the particular 
> recipe ought to be documented.
> 
> 4. Could we document how to get and keep up to date a complete local
> copy 
> of all the glibc review data in gerrit (comments etc.) using whatever
> APIs 
> are available?  Again, I think the relevant APIs already exist, but
> how to 
> use them for glibc ought to be documented (this is especially the
> case for 
> a service like this that's experimental, and thus might go away in 
> future).
> 
> 5. It would also be useful to have documentation for how someone
> should 
> make a patch series appear appropriately in gerrit if they want to
> propose 
> a series that way.  That means the emails for a patch series should 
> include 1/N, 2/N etc. in their subjects (with a 0/N cover letter as 
> appropriate).

I haven't spent much time with Gerrit but the time I have spent
I've found myself thinking Gerrit is designed with the assumption
that submissions are "one" patch.

I found handling a patch series (which I almost always have when
doing changes) rather painful!

> 
> 6. Lower priority, but note there are certain kinds of changes
> involving 
> huge diffs (e.g. to generated files) that thus *would* need a message
> size 
> limit and pointing to a URL for the diffs in that case, for it to be 
> possible to handle such changes through gerrit.  (When sending email 
> manually for such a change one can always omit the 50 MB of diffs to 
> generated files, but as I understand it part of the point of such a
> patch 
> review system is that the system seems the exact change proposed to
> be 
> committed, complete with generated files, so enabling automated
> testing of 
> patch proposals if desired.)
> 

Re: Setup non-pushing gerrit instance for glibc.

[Carlos O'Donell]

On 10/24/19 5:41 PM, Joseph Myers wrote:
> On Thu, 24 Oct 2019, Carlos O'Donell wrote:
> 
>> * Try to setup email based code review in gerrit.
>>   - Currently email is outbound only.
> 
> Observations on teething troubles with the initial setup:
> 
> 1. What's the status of fixing the problem with insufficient diff context 
> in emails when comments relate to particular parts of the diff?  They need 
> to quote the relevant amount of context (typically at least the whole diff 
> hunk, including the hunk header showing the changed function name) rather 
> than just one line and a reference to an external URL.  It's important for 
> messages with reviews to be meaningful in themselves without depending on 
> external links.  This is a longstanding problem (it was obvious in some 
> experiments some people did with proposing GCC patches with Rietveld, 
> gerrit's predecessor, several years ago).  Someone in the GDB discussions 
> mentioned a prototype patch to add some context to the emails, so maybe we 
> could use that patch.

I don't have any status on this change.

Sergio, Were you working on a patch like this to expand the diff context?

If I understand the problem correctly it's that:
~~~
https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/303/1//COMMIT_MSG@19 
PS1, Line 19: Signed-off-by: Carlos O'Donell <carlos@redhat.com>
We do not use DCO and Signed-off-by.
~~~
Is insufficient context. It shouldn't just be line 19, it should be the
whole hunk with the comment on the line it belongs to.

> 2. Could text comments in emails from gerrit be properly wrapped?  
> Messages such as 
> <https://sourceware.org/ml/libc-alpha/2019-10/msg00715.html> are hard to 
> read in the list archives because of very long lines.  (Of course, diff 
> context / quoted source lines should not be wrapped.)

I agree that should be fixed.

I also find it hard to read.

> 3. Could we document (on the wiki, I suppose) the process for setting up 
> git remotes if you want a git repository to get local copies of all 
> versions of all changes proposed this way?  My understanding is gerrit 
> makes those available as refs named in some particular way, so adding a 
> remote with appropriate fetch config should work, but the particular 
> recipe ought to be documented.

Yes, each gerrit review is a ref you can pull and review.

For example my localedef change can be fetched like this:

git fetch "https://gnutoolchain-gerrit.osci.io/r/glibc" refs/changes/03/303/1 && git checkout FETCH_HEAD

I expect you can pull all of refs/changes/* to get all the reviews locally.

We can try to work this out.

> 4. Could we document how to get and keep up to date a complete local copy 
> of all the glibc review data in gerrit (comments etc.) using whatever APIs 
> are available?  Again, I think the relevant APIs already exist, but how to 
> use them for glibc ought to be documented (this is especially the case for 
> a service like this that's experimental, and thus might go away in 
> future).

Agreed, we can try.

> 5. It would also be useful to have documentation for how someone should 
> make a patch series appear appropriately in gerrit if they want to propose 
> a series that way.  That means the emails for a patch series should 
> include 1/N, 2/N etc. in their subjects (with a 0/N cover letter as 
> appropriate).

Agreed, that will be a next step. I'm trying to get through just one patch
review right now to see how that works.

> 6. Lower priority, but note there are certain kinds of changes involving 
> huge diffs (e.g. to generated files) that thus *would* need a message size 
> limit and pointing to a URL for the diffs in that case, for it to be 
> possible to handle such changes through gerrit.  (When sending email 
> manually for such a change one can always omit the 50 MB of diffs to 
> generated files, but as I understand it part of the point of such a patch 
> review system is that the system seems the exact change proposed to be 
> committed, complete with generated files, so enabling automated testing of 
> patch proposals if desired.)

Correct, you'd need the full commit of 50MiB, but I don't know what happens
in that case with gerrit. I could try something that is MiB's in size, like
removing all the locale data.

Thanks for your feedback and list of things you'd like to see, it will take
time to sort through them.

-- 
Cheers,
Carlos.

Re: Setup non-pushing gerrit instance for glibc.

[Jonathan Nieder]

Hi,

Ian Kent wrote:

> I haven't spent much time with Gerrit but the time I have spent
> I've found myself thinking Gerrit is designed with the assumption
> that submissions are "one" patch.
>
> I found handling a patch series (which I almost always have when
> doing changes) rather painful!

Interesting.  The Gerrit project itself[1] uses Gerrit and very often
uses patch series.  See [2] for an example.

The section in its documentation on "developing multiple changes in
parallel"[3] (admittedly poorly named) says a little more about this.

Thanks and hope that helps,
Jonathan

[1] https://gerrit-review.googlesource.com/
[2] https://gerrit-review.googlesource.com/c/gerrit/+/208641
[3] https://gerrit-review.googlesource.com/Documentation/intro-user.html#multiple-features

Re: Setup non-pushing gerrit instance for glibc.

[Jonathan Nieder]

Hi,

A few quick notes from an ex Gerrit developer (nowadays almost all my
time is devoted to Git instead).

Joseph Myers wrote:

> Observations on teething troubles with the initial setup:
>
> 1. What's the status of fixing the problem with insufficient diff context
> in emails when comments relate to particular parts of the diff?

A quick search doesn't find a bug open about this at
https://crbug.com/gerrit/new.

[...]
> 2. Could text comments in emails from gerrit be properly wrapped?
> Messages such as
> <https://sourceware.org/ml/libc-alpha/2019-10/msg00715.html> are hard to
> read in the list archives because of very long lines.  (Of course, diff
> context / quoted source lines should not be wrapped.)

Makes sense.  Likewise, I don't see an existing bug open for this.

> 4. Could we document how to get and keep up to date a complete local copy
> of all the glibc review data in gerrit (comments etc.) using whatever APIs
> are available?

"git clone --mirror" would do this --- the code for the 5th iteration
of change #1234 is at refs/changes/34/1234/5, and the metadata (review
comments, etc) is at refs/changes/34/1234/meta.

See https://gerrit-review.googlesource.com/Documentation/concept-refs-for-namespace.html
and https://gerrit-review.googlesource.com/Documentation/user-upload.html#_gritty_details
for more on this subject.

Alternatively you can do something with
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html,
but that sounds fussier.

[...]
> 5. It would also be useful to have documentation for how someone should
> make a patch series appear appropriately in gerrit if they want to propose
> a series that way.  That means the emails for a patch series should
> include 1/N, 2/N etc. in their subjects (with a 0/N cover letter as
> appropriate).

I really like this idea.  Feel free to file a feature request for it
at https://crbug.com/gerrit/new.

> 6. Lower priority, but note there are certain kinds of changes involving
> huge diffs (e.g. to generated files) that thus *would* need a message size
> limit and pointing to a URL for the diffs in that case, for it to be
> possible to handle such changes through gerrit.

The default behavior is to limit diffs to 256k:
https://gerrit-review.googlesource.com/Documentation/config-gerrit.html#sendemail.maximumDiffSize

Thanks,
Jonathan

Re: Setup non-pushing gerrit instance for glibc.

[Sergio Durigan Junior]

On Thursday, October 24 2019, Carlos O'Donell wrote:

> On 10/24/19 5:41 PM, Joseph Myers wrote:
>> On Thu, 24 Oct 2019, Carlos O'Donell wrote:
>> 
>>> * Try to setup email based code review in gerrit.
>>>   - Currently email is outbound only.
>> 
>> Observations on teething troubles with the initial setup:
>> 
>> 1. What's the status of fixing the problem with insufficient diff context 
>> in emails when comments relate to particular parts of the diff?  They need 
>> to quote the relevant amount of context (typically at least the whole diff 
>> hunk, including the hunk header showing the changed function name) rather 
>> than just one line and a reference to an external URL.  It's important for 
>> messages with reviews to be meaningful in themselves without depending on 
>> external links.  This is a longstanding problem (it was obvious in some 
>> experiments some people did with proposing GCC patches with Rietveld, 
>> gerrit's predecessor, several years ago).  Someone in the GDB discussions 
>> mentioned a prototype patch to add some context to the emails, so maybe we 
>> could use that patch.
>
> I don't have any status on this change.
>
> Sergio, Were you working on a patch like this to expand the diff context?
>
> If I understand the problem correctly it's that:
> ~~~
> https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/303/1//COMMIT_MSG@19 
> PS1, Line 19: Signed-off-by: Carlos O'Donell <carlos@redhat.com>
> We do not use DCO and Signed-off-by.
> ~~~
> Is insufficient context. It shouldn't just be line 19, it should be the
> whole hunk with the comment on the line it belongs to.

Simon was working on this, and he told me he has a prototype fix that he
was planning to deploy on our instance.  I don't know the current
status, though.  I added him to the Cc list.

BTW, I agree this is a problem that should be fixed; quoting just one
line from the change is really not sufficient.

>> 2. Could text comments in emails from gerrit be properly wrapped?  
>> Messages such as 
>> <https://sourceware.org/ml/libc-alpha/2019-10/msg00715.html> are hard to 
>> read in the list archives because of very long lines.  (Of course, diff 
>> context / quoted source lines should not be wrapped.)
>
> I agree that should be fixed.
>
> I also find it hard to read.

The users can configure line wrapping under Settings -> Edit
Preferences.  This is a per-user configuration; gerrit doesn't have a
global setting that forces line wrapping.  However, I think it's
possible to edit the All-Users repository and make this setting
automatically enabled for everybody.

/me does some tests...

Hm, it seems the line wrapping setting doesn't really apply to messages?
I tried writing a comment without line wrapping here:

  https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/161/3#message-9f0b67bbd0ef0765d07f417a15f50aa79a5414d5

And it generated this message:

  https://sourceware.org/ml/gdb-patches/2019-10/msg00905.html

I mean, gnus wraps the line for me, but I understand this is my MUA's
setting, and not something we can really expect.

>> 5. It would also be useful to have documentation for how someone should 
>> make a patch series appear appropriately in gerrit if they want to propose 
>> a series that way.  That means the emails for a patch series should 
>> include 1/N, 2/N etc. in their subjects (with a 0/N cover letter as 
>> appropriate).
>
> Agreed, that will be a next step. I'm trying to get through just one patch
> review right now to see how that works.

Bear in mind that gerrit still doesn't support cover letters.  We've had
a discussion about this on gdb-patches, and the outcome is:

1) The gerrit bug requesting support for cover letters has been reopened
(it had been closed due to inactivity).  You can see the bug here:

  https://bugs.chromium.org/p/gerrit/issues/detail?id=924

There is a discussion happening there, but unfortunately I am unable to
participate because their bug tracker demands a Google account, which I
don't have.

2) It is possible, although very hacky, to create a cover letter using a
git empty commit as the first commit.

3) It is possible to continue using the mailing list for that: you can
send the change via gerrit, which will send the email notifications to
the ml.  Then, you can send an email in reply to one of gerrit's
messages explaning what the series is about.  This is also not very
nice.

You can see the gdb-patches discussion here:

  https://sourceware.org/ml/gdb-patches/2019-10/msg00694.html

Even though gerrit has the concept of a patch series, when it sends the
email notifications it doesn't thread them, and it also doesn't include
the "X/N" numbering scheme, so it's not trivial to identify series in
the mailing list (which can also be seen as a drawback).

>> 6. Lower priority, but note there are certain kinds of changes involving 
>> huge diffs (e.g. to generated files) that thus *would* need a message size 
>> limit and pointing to a URL for the diffs in that case, for it to be 
>> possible to handle such changes through gerrit.  (When sending email 
>> manually for such a change one can always omit the 50 MB of diffs to 
>> generated files, but as I understand it part of the point of such a patch 
>> review system is that the system seems the exact change proposed to be 
>> committed, complete with generated files, so enabling automated testing of 
>> patch proposals if desired.)
>
> Correct, you'd need the full commit of 50MiB, but I don't know what happens
> in that case with gerrit. I could try something that is MiB's in size, like
> removing all the locale data.

That'd be interesting to see.

> Thanks for your feedback and list of things you'd like to see, it will take
> time to sort through them.

Indeed: thank you for the feedback, and it will take some time to sort
through them, for sure :-).

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: Setup non-pushing gerrit instance for glibc.

[Simon Marchi]

Hi all,

Thanks a lot for you comments, it gives a good idea of what the main
pain points are.

On 2019-10-24 10:04 p.m., Sergio Durigan Junior wrote:
> Simon was working on this, and he told me he has a prototype fix that he
> was planning to deploy on our instance.  I don't know the current
> status, though.  I added him to the Cc list.
> 
> BTW, I agree this is a problem that should be fixed; quoting just one
> line from the change is really not sufficient.

This is a prototype I did after spending maybe 15 minutes with the source of
Gerrit, so don't expect too much :).  I was able to tell Gerrit to include
a fixed amount of lines above the line the comment is attached to.

We'll maybe manage to do something a bit smarter, but I don't think we'll
easily be able to quote the _hunk_.  This is where the mail is generated:

https://gerrit.googlesource.com/gerrit/+/refs/heads/master/java/com/google/gerrit/server/mail/send/CommentSender.java#313

That code has access to the lines of the file where the comment was applied.
So if you put your comment on the left pane, in the web UI (e.g. on the base),
then we'll quote the lines of that version.  If you put your comment on right
(e.g. on v1), then we'll quote the lines of that version.  But I don't think
we'll be able to make up a hunk with +'s and -'s.

Also, remember that you can go put a comment on an unchanged section on the
file (e.g. to say "you forgot to update this call"), so there would be no
diff hunk to show for that comment anyway.

I filed this, if you want to subscribe: https://bugs.chromium.org/p/gerrit/issues/detail?id=11804

> The users can configure line wrapping under Settings -> Edit
> Preferences.  This is a per-user configuration; gerrit doesn't have a
> global setting that forces line wrapping.  However, I think it's
> possible to edit the All-Users repository and make this setting
> automatically enabled for everybody.
> 
> /me does some tests...
> 
> Hm, it seems the line wrapping setting doesn't really apply to messages?
> I tried writing a comment without line wrapping here:
> 
>   https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/161/3#message-9f0b67bbd0ef0765d07f417a15f50aa79a5414d5
> 
> And it generated this message:
> 
>   https://sourceware.org/ml/gdb-patches/2019-10/msg00905.html
> 
> I mean, gnus wraps the line for me, but I understand this is my MUA's
> setting, and not something we can really expect.

Yeah, I don't think a user setting can affect that, since there's only one
notification email sent to the list for everybody.

I presume that's something we can control in the generation of the email.

I filed: https://bugs.chromium.org/p/gerrit/issues/detail?id=11805

>>> 5. It would also be useful to have documentation for how someone should 
>>> make a patch series appear appropriately in gerrit if they want to propose 
>>> a series that way.  That means the emails for a patch series should 
>>> include 1/N, 2/N etc. in their subjects (with a 0/N cover letter as 
>>> appropriate).

Putting the cover letter problem aside for now, just addressing the 1/N to N/N
numbering problem.  I think the most honest answer here is: don't expect that so
soon, because it just not in Gerrit's model.

When sending patches by emails, we need to be able to send many patches in a way
that enforces a certain order and shows dependencies between patches.  If you just
sent 10 individual patches instead of a series of 10 patches it would be
impossible to know in what order I need to apply them, or even that they
are related.  So that's why we patch series make sense.

With Gerrit, you push a chain of git commit:

  A <- B <- C

They are naturally related due to them being git commits and having a parent-child
relationship.  If I check out C, I'll automatically check out its parent B and C.
Gerrit doesn't see that as a series, just as individual changes that are dependent
on one another.

Let's say I push a new version of C.  A and B are still at their v1, while C is at
its v2.  Then, if I push D on top of that, D will be at its v1.

Then, I (or even you!) could make a new change E on top of A, where E would be
unrelated to B, C and D (other than sharing A in their ancestry).

I can then decide to rebase A by itself (because master has moved on), which will
create a v2 of A.  All the other changes will still be based on the v1 of A.  I
will be able to rebase the other changes later on the latest version of A, if I
want to.

Since Gerrit allows this kind of non-linear work, where it does not treat chains
of commits as rigid/atomic sequences, I have a hard time coming up with a way how
Gerrit should group that into series as we know them from emails.

Also, I know not everybody here is a fan of web UIs, but browsing the changes
on the web UI lets you see the relation chain between changes (when looking
at a particular change):

  https://nova.polymtl.ca/~simark/ss/ad123r4v.png

So that's how I would see the big picture of how those changes are linked together.

Simon

Re: Setup non-pushing gerrit instance for glibc.

[Simon Marchi]

On 2019-10-24 11:14 p.m., Simon Marchi wrote:
> This is a prototype I did after spending maybe 15 minutes with the source of
> Gerrit, so don't expect too much :).  I was able to tell Gerrit to include
> a fixed amount of lines above the line the comment is attached to.

So this is what I have so far:

  https://gnutoolchain-gerrit.osci.io/r/c/gerrit/+/304

Simon

Re: Setup non-pushing gerrit instance for glibc.

[Ian Kent]

On Thu, 2019-10-24 at 18:16 -0700, Jonathan Nieder wrote:
> Hi,
> 
> Ian Kent wrote:
> 
> > I haven't spent much time with Gerrit but the time I have spent
> > I've found myself thinking Gerrit is designed with the assumption
> > that submissions are "one" patch.
> > 
> > I found handling a patch series (which I almost always have when
> > doing changes) rather painful!
> 
> Interesting.  The Gerrit project itself[1] uses Gerrit and very often
> uses patch series.  See [2] for an example.
> 
> The section in its documentation on "developing multiple changes in
> parallel"[3] (admittedly poorly named) says a little more about this.

Right, and I have managed to get a couple of multi-patch changes
through the Gerrit review process in the past but I found it very
difficult to find out how to achieve it and I expect the same thing
will happen again next time I need do it.

This likely isn't a problem for people that use Gerrit on a day
to day basis (once familiar with the processes).

So I guess my point is that something specific for new adopters is
needed in the documentation, but of course I could be just plain
thick so maybe it's not worth worrying about, ;)

I also haven't looked at the links you provided so maybe I making
noise about nothing at all, OTOH those likes look a bit familiar
...

> 
> Thanks and hope that helps,
> Jonathan
> 
> [1] https://gerrit-review.googlesource.com/
> [2] https://gerrit-review.googlesource.com/c/gerrit/+/208641
> [3] 
> https://gerrit-review.googlesource.com/Documentation/intro-user.html#multiple-features

Re: Setup non-pushing gerrit instance for glibc.

[Florian Weimer]

* Carlos O'Donell:

> With the gracious help of Sergio and Simon I have setup a glibc mirror
> on a gerrit instance that *cannot* push to sourceware git.

I find it a bit non-obvious to see that there pending unresolved
comments from previous patch iterations.  If I go to

  <https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/303>

I end up with the last version, and there are no comments for it yet.
In order to see them, I scroll down, clock on “Comment Threads”, and
then perhaps filter on “Only unresolved threads”, and then you see the
leftovers.  Is there a more convenient way to get this information?  I
mean, you really should address all these old comments in a new
submission, but it's currently quite hard to see if you don't.

(This is obviously not a blocker for the tool as a whole.)

Thanks,
Florian

Re: Setup non-pushing gerrit instance for glibc.

[Joseph Myers]

On Thu, 24 Oct 2019, Simon Marchi wrote:

> We'll maybe manage to do something a bit smarter, but I don't think we'll
> easily be able to quote the _hunk_.  This is where the mail is generated:

I think the hunk - not just the new version of the code on its own - is 
critical information if someone is commenting on a particular part of a 
change, needed for such comments to be properly readable in context.

> Also, remember that you can go put a comment on an unchanged section on the
> file (e.g. to say "you forgot to update this call"), so there would be no
> diff hunk to show for that comment anyway.

In that case it should at least show a reasonable amount of context, with 
some indication of what function it is in, like in the diff hunk header.

> They are naturally related due to them being git commits and having a 
> parent-child relationship.  If I check out C, I'll automatically check 
> out its parent B and C. Gerrit doesn't see that as a series, just as 
> individual changes that are dependent on one another.

There is a difference of *intent* between changes that depend on one 
another and a patch series.  A patch series is saying:

* there is a common purpose motivating the patches; and

* some patches may best be understood by reference to ones later in the 
series (if e.g. one patch adds an interface that a later one adds users 
for), so the link to those later patches is important for review purposes.

And so a patch series should be sent out to the list for review as such.  
There may be comments on the series as a whole, or on individual patches 
in it.

There's also the variant of patch series that are explained in the cover 
letter (or in other text not intended for the final commit) as being split 
only for review purposes and intended to be squashed for commit, if the 
pieces most convenient for review don't form bisectable commits.  I'm not 
sure how much any review system needs to know about the distinction 
between the two kinds of patch series if it's not pushing the commits 
itself.

> Let's say I push a new version of C.  A and B are still at their v1, 
> while C is at its v2.  Then, if I push D on top of that, D will be at 
> its v1.
> 
> Then, I (or even you!) could make a new change E on top of A, where E 
> would be unrelated to B, C and D (other than sharing A in their 
> ancestry).
> 
> I can then decide to rebase A by itself (because master has moved on), 
> which will create a v2 of A.  All the other changes will still be based 
> on the v1 of A.  I will be able to rebase the other changes later on the 
> latest version of A, if I want to.

All of this is perfectly meaningful for patch series - you can do [PATCHv2 
3/3] and then potentially [PATCH 4/3] for D if it's intended as part of 
the series (or just a separate submission for D if it's not intended as 
part of the series).  You can revise patch 1/3 with or without refreshing 
the whole series.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Setup non-pushing gerrit instance for glibc.

[Joseph Myers]

On Thu, 24 Oct 2019, Carlos O'Donell wrote:

> * Try to setup email based code review in gerrit.
>   - Currently email is outbound only.
>   - We are looking at how to make it entirely email based, but tracked
>     in gerrit.

Right now gerrit is sending bounces for any email replies I send, despite 
having set up a gerrit account to be able to reply, which 
<https://sourceware.org/ml/gdb-patches/2019-10/msg00867.html> suggests 
ought to suffice.  Both before and after setting up an account it sent the 
same bounce:

  Gerrit Code Review was unable to parse your email.

  This might be because your email did not quote Gerrit's email, because 
  you are using an unsupported email client, or because of a bug.

  Thus, no actions were taken by Gerrit in response to this email, and you 
  should use the Gerrit website to continue.

  This email was sent in response to an email coming from this address. In 
  case you did not send Gerrit an email, feel free to ignore this.

  This is a send-only email address.  Replies to this message will not be 
  read or answered.--

Note: of course I didn't quote the junk from gerrit's email footer, I 
quoted only the relevant context I wanted to comment on.  Gerrit needs to 
identify what issue is being commented on either from References / 
In-Reply-To headers, or from e.g. an issue number in the subject line (I 
think the latter is what Bugzilla uses, at least it doesn't expect 
anything in particular to be quoted from its outgoing messages).  It ought 
to work with nothing quoted from the email being commented on, or with 
selected text being quoted and commented on (but I suppose it's also a 
good idea for it to have enough smarts to discard a full email quoted in a 
top-post, I think Bugzilla automatically discards everything starting 
after a "-- " signature start line).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Setup non-pushing gerrit instance for glibc.

[Simon Marchi]

On 2019-10-25 10:48 a.m., Joseph Myers wrote:
> On Thu, 24 Oct 2019, Simon Marchi wrote:
> 
>> We'll maybe manage to do something a bit smarter, but I don't think we'll
>> easily be able to quote the _hunk_.  This is where the mail is generated:
> 
> I think the hunk - not just the new version of the code on its own - is 
> critical information if someone is commenting on a particular part of a 
> change, needed for such comments to be properly readable in context.

I understand that, I'm just saying I don't think I'll be able to do this change
(maybe an experienced Gerrit developer could).

>> Also, remember that you can go put a comment on an unchanged section on the
>> file (e.g. to say "you forgot to update this call"), so there would be no
>> diff hunk to show for that comment anyway.
> 
> In that case it should at least show a reasonable amount of context, with 
> some indication of what function it is in, like in the diff hunk header.

From my experience, git diff doesn't really get the function name, it just
gives you the first preceding line that starts with a non-whitespace.  It
often "breaks" when you have labels or preprocessor directives at column 0,
for example:

diff --git a/src/plugins/ctf/common/metadata/decoder.c b/src/plugins/ctf/common/metadata/decoder.c
index a62beebfcd99..5e4facb4ff27 100644
--- a/src/plugins/ctf/common/metadata/decoder.c
+++ b/src/plugins/ctf/common/metadata/decoder.c
@@ -351,6 +351,7 @@ end:
 				"mdec-addr=%p", mdec);
 		}
 	}
+	hello

 	free(buf);

Since it's pretty basic, I could probably replicate that method to include a
few lines before the line of interest, as well as the first preceding line that
starts with a non-whitespace.


>> They are naturally related due to them being git commits and having a 
>> parent-child relationship.  If I check out C, I'll automatically check 
>> out its parent B and C. Gerrit doesn't see that as a series, just as 
>> individual changes that are dependent on one another.
> 
> There is a difference of *intent* between changes that depend on one 
> another and a patch series.  A patch series is saying:
> 
> * there is a common purpose motivating the patches; and

In Gerrit, you'd probably use topics to indicate that many changes have
a common purpose (whether they are interdependent or not).

https://gerrit-review.googlesource.com/Documentation/intro-user.html#topics

> * some patches may best be understood by reference to ones later in the 
> series (if e.g. one patch adds an interface that a later one adds users 
> for), so the link to those later patches is important for review purposes.

For that second point, I believe that a preparatory patch should mention
in its commit message that it's adding something not used yet, with the
intent of using it in a later patch.  This way, somebody browsing the git
(without access to the cover letter) tree will understand why this particular
commit adds a function that's not used.  So that is clear to the reviewer
anyway.

As long as Gerrit lets you find those later patches by showing you the
relation chain, I think it's clear.

> And so a patch series should be sent out to the list for review as such.  
> There may be comments on the series as a whole, or on individual patches 
> in it.
> 
> There's also the variant of patch series that are explained in the cover 
> letter (or in other text not intended for the final commit) as being split 
> only for review purposes and intended to be squashed for commit, if the 
> pieces most convenient for review don't form bisectable commits.  I'm not 
> sure how much any review system needs to know about the distinction 
> between the two kinds of patch series if it's not pushing the commits 
> itself.

I don't think there would be a problem with that, except that the two emails
on the mailing would not be threaded together.  More thoughts about that
below.

>> Let's say I push a new version of C.  A and B are still at their v1, 
>> while C is at its v2.  Then, if I push D on top of that, D will be at 
>> its v1.
>>
>> Then, I (or even you!) could make a new change E on top of A, where E 
>> would be unrelated to B, C and D (other than sharing A in their 
>> ancestry).
>>
>> I can then decide to rebase A by itself (because master has moved on), 
>> which will create a v2 of A.  All the other changes will still be based 
>> on the v1 of A.  I will be able to rebase the other changes later on the 
>> latest version of A, if I want to.
> 
> All of this is perfectly meaningful for patch series - you can do [PATCHv2 
> 3/3] and then potentially [PATCH 4/3] for D if it's intended as part of 
> the series (or just a separate submission for D if it's not intended as 
> part of the series).  You can revise patch 1/3 with or without refreshing 
> the whole series.

I understand that we can do that with patch series.  My point is that I don't
see an easy way of translating changes from Gerrit's (current) model - i.e. git
branches - into a patch series model.

About cover letters: Sergio mentioned that a hacky way of doing a cover letter
would be to add an empty commit, whose commit message is the cover letter.  At
first I thought this commit would come before the actual commits.  But if we put
it at the end of the chain, maybe it works.  Let's say you have

  A: Preparatory patch to add function foo
  B: Preparatory patch to add function bar
  C: Big feature that uses foo and bar
  D: Cover letter that explains my series

With this relationship:

  A <- B <- C <- D

Then when you checkout out the cover letter, you check out the whole series.
If you modify C and push again, both C and D will be at v2.  So essentially,
the version of D kind of becomes the version of the series.

And when you actually push the series, you don't push D.

I understand that there is still the downside that if you are following only
from the mailing list, these mails are not threaded together.  Perhaps that
can be improved, like if I open a new change that depends on another change
that is still open (not merged yet, still up for review), then we send the
mail in reply to the mail of that other change.

Simon

Re: Setup non-pushing gerrit instance for glibc.

[Joseph Myers]

On Fri, 25 Oct 2019, Simon Marchi wrote:

> > In that case it should at least show a reasonable amount of context, with 
> > some indication of what function it is in, like in the diff hunk header.
> 
> From my experience, git diff doesn't really get the function name, it just
> gives you the first preceding line that starts with a non-whitespace.  It
> often "breaks" when you have labels or preprocessor directives at column 0,
> for example:

Yes.  It's a reasonable heuristic that works in many common cases (and can 
be configured with a diff attribute in .gitattributes and a corresponding 
diff.<language>.xfuncname setting in .git/config, if desired).  Sometimes 
the default amount of context in a diff isn't enough either, and sometimes 
code is rearranged in a way that makes diff output unhelpful.

I'd like reasonable heuristics to be used in the gerrit messages so that 
what they show is sufficient in most cases for it to be possible to fully 
follow and contribute to the discussion via email without needing to go to 
the website.  Showing the diff hunk, with default settings (so the same 
logic as used by default by git diff to identify a function name) seems 
like a reasonable heuristic for that purpose.

> > There is a difference of *intent* between changes that depend on one 
> > another and a patch series.  A patch series is saying:
> > 
> > * there is a common purpose motivating the patches; and
> 
> In Gerrit, you'd probably use topics to indicate that many changes have
> a common purpose (whether they are interdependent or not).
> 
> https://gerrit-review.googlesource.com/Documentation/intro-user.html#topics

If using a topic were to mark the patches as a patch series (including 
marking email subjects and threading them accordingly), that would be 
fine.  I don't think it's necessary for simply pushing multiple changes at 
once to be the way you cause something to be handled as a patch series, if 
topics are the idiomatic concept in gerrit that corresponds to a patch 
series as generated by git-format-patch.

> > There's also the variant of patch series that are explained in the cover 
> > letter (or in other text not intended for the final commit) as being split 
> > only for review purposes and intended to be squashed for commit, if the 
> > pieces most convenient for review don't form bisectable commits.  I'm not 
> > sure how much any review system needs to know about the distinction 
> > between the two kinds of patch series if it's not pushing the commits 
> > itself.
> 
> I don't think there would be a problem with that, except that the two emails
> on the mailing would not be threaded together.  More thoughts about that
> below.

Well, there would be issues if you wanted gerrit to push changes itself.  
And how does the "identify this as being the same change" system (for 
knowing when something has been committed) work if multiple separately 
reviewed patches are squashed into one commit?

(Is the valid syntax for Change-Ids documented anywhere?  In particular, 
is it OK to write a human-readable Change-Id oneself?)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Setup non-pushing gerrit instance for glibc.

[Simon Marchi]

On 2019-10-25 12:10 p.m., Joseph Myers wrote:
>>> There is a difference of *intent* between changes that depend on one 
>>> another and a patch series.  A patch series is saying:
>>>
>>> * there is a common purpose motivating the patches; and
>>
>> In Gerrit, you'd probably use topics to indicate that many changes have
>> a common purpose (whether they are interdependent or not).
>>
>> https://gerrit-review.googlesource.com/Documentation/intro-user.html#topics
> 
> If using a topic were to mark the patches as a patch series (including 
> marking email subjects and threading them accordingly), that would be 
> fine.  I don't think it's necessary for simply pushing multiple changes at 
> once to be the way you cause something to be handled as a patch series, if 
> topics are the idiomatic concept in gerrit that corresponds to a patch 
> series as generated by git-format-patch.

I don't believe the topic is included in the email (at least not the subject),
but that is likely possible to do.  For example, including it in the prefix tag:

  [review my-topic-name]

I think that would help, but we would still be missing the information about
the order of the patches.  Any ideas for that?

>>> There's also the variant of patch series that are explained in the cover 
>>> letter (or in other text not intended for the final commit) as being split 
>>> only for review purposes and intended to be squashed for commit, if the 
>>> pieces most convenient for review don't form bisectable commits.  I'm not 
>>> sure how much any review system needs to know about the distinction 
>>> between the two kinds of patch series if it's not pushing the commits 
>>> itself.
>>
>> I don't think there would be a problem with that, except that the two emails
>> on the mailing would not be threaded together.  More thoughts about that
>> below.
> 
> Well, there would be issues if you wanted gerrit to push changes itself.  
> And how does the "identify this as being the same change" system (for 
> knowing when something has been committed) work if multiple separately 
> reviewed patches are squashed into one commit?

In that case, I would keep in the squashed commit the Change-Id of the first
commit (i.e. the commit that contains the logical change, not the generated
stuff).  When pushing that commit, it will auto-close the first change.  The
last/final version of that change would therefore be the full thing, including
the generated stuff.

I would then need to abandon the second commit.  In the reason field (when you
click Abandon) I would say that since the content has been squashed in the
parent change, which is now merged, this one is no longer needed.

> (Is the valid syntax for Change-Ids documented anywhere?  In particular, 
> is it OK to write a human-readable Change-Id oneself?)

It's not mentioned here [1] so I suppose it's not documented.  We would have
to look in the source code (or just try it) to see what's supported.

[1] https://gerrit-review.googlesource.com/Documentation/user-changeid.html

Simon

Re: Setup non-pushing gerrit instance for glibc.

[Sergio Durigan Junior]

On Friday, October 25 2019, Joseph Myers wrote:

> On Thu, 24 Oct 2019, Carlos O'Donell wrote:
>
>> * Try to setup email based code review in gerrit.
>>   - Currently email is outbound only.
>>   - We are looking at how to make it entirely email based, but tracked
>>     in gerrit.
>
> Right now gerrit is sending bounces for any email replies I send, despite 
> having set up a gerrit account to be able to reply, which 
> <https://sourceware.org/ml/gdb-patches/2019-10/msg00867.html> suggests 
> ought to suffice.  Both before and after setting up an account it sent the 
> same bounce:
>
>   Gerrit Code Review was unable to parse your email.
>
>   This might be because your email did not quote Gerrit's email, because 
>   you are using an unsupported email client, or because of a bug.
>
>   Thus, no actions were taken by Gerrit in response to this email, and you 
>   should use the Gerrit website to continue.
>
>   This email was sent in response to an email coming from this address. In 
>   case you did not send Gerrit an email, feel free to ignore this.
>
>   This is a send-only email address.  Replies to this message will not be 
>   read or answered.--
>
> Note: of course I didn't quote the junk from gerrit's email footer, I 
> quoted only the relevant context I wanted to comment on.  Gerrit needs to 
> identify what issue is being commented on either from References / 
> In-Reply-To headers, or from e.g. an issue number in the subject line (I 
> think the latter is what Bugzilla uses, at least it doesn't expect 
> anything in particular to be quoted from its outgoing messages).  It ought 
> to work with nothing quoted from the email being commented on, or with 
> selected text being quoted and commented on (but I suppose it's also a 
> good idea for it to have enough smarts to discard a full email quoted in a 
> top-post, I think Bugzilla automatically discards everything starting 
> after a "-- " signature start line).

Unfortunately, you do need to quote the content from the email footer.
I agree that gerrit should be able to keep track of Message-Id's and
match the replies using In-Reply-To, but unfortunately it doesn't do
that.  It relies on the information present at the quoted footer to
determine which change the email refers to.

I was also surprised by this when I was configuring the email support,
because we had disabled the inclusion of the footer in all changes, and
I started getting bounces as well.  I had to re-enable the footer in
order to get everything working.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: Setup non-pushing gerrit instance for glibc.

[Joseph Myers]

On Fri, 25 Oct 2019, Sergio Durigan Junior wrote:

> Unfortunately, you do need to quote the content from the email footer.
> I agree that gerrit should be able to keep track of Message-Id's and
> match the replies using In-Reply-To, but unfortunately it doesn't do
> that.  It relies on the information present at the quoted footer to
> determine which change the email refers to.

If it doesn't work as well as Bugzilla at handling incoming email, it's 
probably not ready for serious use in glibc development (i.e. anyone 
submitting patches that way will need to watch for replies on the mailing 
list that don't get into gerrit's data, until gerrit's email handling is 
fixed).  Is there an issue for fixing gerrit to handle emails quoting 
relevant context without the footer?

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Setup non-pushing gerrit instance for glibc.

[Sergio Durigan Junior]

On Friday, October 25 2019, Joseph Myers wrote:

> On Fri, 25 Oct 2019, Sergio Durigan Junior wrote:
>
>> Unfortunately, you do need to quote the content from the email footer.
>> I agree that gerrit should be able to keep track of Message-Id's and
>> match the replies using In-Reply-To, but unfortunately it doesn't do
>> that.  It relies on the information present at the quoted footer to
>> determine which change the email refers to.
>
> If it doesn't work as well as Bugzilla at handling incoming email, it's 
> probably not ready for serious use in glibc development (i.e. anyone 
> submitting patches that way will need to watch for replies on the mailing 
> list that don't get into gerrit's data, until gerrit's email handling is 
> fixed).  Is there an issue for fixing gerrit to handle emails quoting 
> relevant context without the footer?

TBH, I wouldn't know right now.  I haven't looked at gerrit's source
code to see how hard/feasible it would be to fix this.

What we can do is insert a message in the emails explaining that the
footer needs to be preserved when replying to it.  We will also have to
warn the user that replying directly to a new change message will not
work; gerrit can only understand email replies to comments.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: Setup non-pushing gerrit instance for glibc.

[Joseph Myers]

On Fri, 25 Oct 2019, Sergio Durigan Junior wrote:

> footer needs to be preserved when replying to it.  We will also have to
> warn the user that replying directly to a new change message will not
> work; gerrit can only understand email replies to comments.

That seems like an even more serious issue for usability of email replies, 
in that a reply to a new patch submission could well be the most common 
case.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Setup non-pushing gerrit instance for glibc.

[Jonathan Nieder]

Joseph Myers wrote:
> On Fri, 25 Oct 2019, Sergio Durigan Junior wrote:

>> Unfortunately, you do need to quote the content from the email footer.
>> I agree that gerrit should be able to keep track of Message-Id's and
>> match the replies using In-Reply-To, but unfortunately it doesn't do
>> that.  It relies on the information present at the quoted footer to
>> determine which change the email refers to.
>
> If it doesn't work as well as Bugzilla at handling incoming email, it's
> probably not ready for serious use in glibc development

I think this is https://crbug.com/gerrit/8904. Please star that bug
and coordinate fixes there.

Thanks,
Jonathan

Re: Setup non-pushing gerrit instance for glibc.

[Carlos O'Donell]

On 10/25/19 6:03 PM, Jonathan Nieder wrote:
> Joseph Myers wrote:
>> On Fri, 25 Oct 2019, Sergio Durigan Junior wrote:
> 
>>> Unfortunately, you do need to quote the content from the email footer.
>>> I agree that gerrit should be able to keep track of Message-Id's and
>>> match the replies using In-Reply-To, but unfortunately it doesn't do
>>> that.  It relies on the information present at the quoted footer to
>>> determine which change the email refers to.
>>
>> If it doesn't work as well as Bugzilla at handling incoming email, it's
>> probably not ready for serious use in glibc development
> 
> I think this is https://crbug.com/gerrit/8904. Please star that bug
> and coordinate fixes there.

I've starred it and commented on it.

-- 
Cheers,
Carlos.

Re: Setup non-pushing gerrit instance for glibc.

[Sergio Durigan Junior]

On Friday, October 25 2019, Joseph Myers wrote:

> On Fri, 25 Oct 2019, Sergio Durigan Junior wrote:
>
>> footer needs to be preserved when replying to it.  We will also have to
>> warn the user that replying directly to a new change message will not
>> work; gerrit can only understand email replies to comments.
>
> That seems like an even more serious issue for usability of email replies, 
> in that a reply to a new patch submission could well be the most common 
> case.

I understand (and share) the frustration (really do!), but I think we
should take a step back and consider what the community wants, and what
gerrit offers.

Gerrit will probably never be able to be fully functional with email,
because that is not how it is designed.  We can and will file bugs and
RFEs against the upstream project, but, as far as I understand it, it
just is not designed to be 100% compatible with mailing list-based
reviews.  Its main goal is to provide a web-based patch reviewing
system, and this is how its developers expect people to use it.

If you look at other projects that use gerrit, you will notice that, for
most of them, their entire flow of communication happens in the web
interface.  They can even have a mailing list which receives email
notifications from gerrit, but their use of email is pretty basic (i.e.,
a lot of times their gerrit instance doesn't attach the patches to the
emails, nor allows email replies).  It's only a way of letting other
people know: "Hey, there's a new change to be reviewed, please go to
this URL and check it out".

When the GDB community decided to give gerrit a try, the main problem it
was looking to solve was the "patches go missing" issue.  Basically, we
have a lot of patches being sent to the project and not so many
reviewers, so it's not uncommon for a patch to get "buried" waiting for
a review, and if the author doesn't ping it, it gets lost.  With gerrit,
you can get a pretty good view of all the patches that were submitted so
far, and you can always check if a patch is too old or if it's been too
long since the last interaction on it.

What the GDB (and now the glibc) community is now having to decide is
whether it is beneficial to abandon our mailing list-based workflow
(with all its pros and cons) and start using a web-based workflow (with
all its pros and cons).  Simon and I are doing our best to accomodate
the needs of those who would like to stay in the email camp as much as
we can, but it just won't be possible to say "you can use only email if
you want, no need to go to the web interface".

There are of course other tools that we can consider.  For example,
recently we started looking into re-activating the patchwork
installation on sourceware.org, upgrading it, and implementing hooks
which will make it possible for the patchwork software to automatically
"close"/"archive" patches which have been pushed to the repo.  If we
choose to go this route, we will still be able to use emails to review
patches, but we will lose the features gerrit offers.

Anyway, I'm sorry about writing so much, but I wanted to give you folks
a broader perspective of why we're trying gerrit, and what we can
reasonably expect from it.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: Setup non-pushing gerrit instance for glibc.

[Joseph Myers]

On Sun, 27 Oct 2019, Sergio Durigan Junior wrote:

> When the GDB community decided to give gerrit a try, the main problem it
> was looking to solve was the "patches go missing" issue.

We have that problem in glibc, but we also have the problem of how to make 
review as efficient for the reviewer as possible - enabling reviewing 100 
patches a day, as Carlos said at the glibc BoF.

If reviewing a patch, or understanding the context for comments, requires 
opening a browser tab and cutting and pasting a URL in there and clicking 
around to find things on that page, that *reduces* my efficiency and 
breaks up the flow of going through patches, bug reports and comments (on 
various projects) that have come in over the time interval since I last 
went through them, compared to simply having all the needed information in 
an email in at least 90% of cases.  Maybe there are 10% of cases where a 
patch needs to break the flow anyway (e.g. if I suspect some issue but 
need to do a build with the patch applied to test for it before replying).  
But there are 90% of common cases that it should be possible to handle 
properly by email if a few issues are fixed:

* Properly support emails with inline replies and the metadata at the 
bottom of the email not quoted (only the relevant content replied to being 
quoted).  This means (a) attaching them to the right issue, based on 
message-ids found in email headers and (b) not losing the quoted text 
being replied to which is important to understanding the replies.

* Handle email replies to notifications of new patches, not just to 
comments on them.

* Include diff hunks in emails with comments on changed code (we now have 
more context in the code quoted, which is an improvement, but seeing the 
actual *changes* being commented on, rather than just one version of the 
code, is important to provide sufficient information in many cases).


Most of the time, an initial response to a bug report in Bugzilla does not 
require opening a browser (the main pain point is dealing with attached 
testcases, since Bugzilla doesn't attach those to emails).  Reviewing and 
cleaning up state for all open bugs in a particular area, via the web 
interface, is a more occasional activity.  The first two points above are 
things that work just fine with email replies in Bugzilla (replying inline 
to any Bugzilla message works fine without any special rules about what to 
quote where); the third point isn't applicable there.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Setup non-pushing gerrit instance for glibc.

[Jonathan Nieder]

Hi,

Joseph Myers wrote:

> We have that problem in glibc, but we also have the problem of how to make
> review as efficient for the reviewer as possible - enabling reviewing 100
> patches a day, as Carlos said at the glibc BoF.
>
> If reviewing a patch, or understanding the context for comments, requires
> opening a browser tab and cutting and pasting a URL in there and clicking
> around to find things on that page, that *reduces* my efficiency
[...]
> But there are 90% of common cases that it should be possible to handle
> properly by email if a few issues are fixed:

Thanks for writing this up.  I agree with this goal.

For that 10% that you can't handle by email, let me also recommend
https://opendev.org/ttygroup/gertty.

> * Properly support emails with inline replies and the metadata at the
> bottom of the email not quoted (only the relevant content replied to being
> quoted).  This means (a) attaching them to the right issue, based on
> message-ids found in email headers and

This is https://crbug.com/gerrit/8904.

>                                        (b) not losing the quoted text
> being replied to which is important to understanding the replies.

Can you say more about this (e.g. do you have an example)?

> * Handle email replies to notifications of new patches, not just to
> comments on them.

I would expect this to already work as well.

> * Include diff hunks in emails with comments on changed code (we now have
> more context in the code quoted, which is an improvement, but seeing the
> actual *changes* being commented on, rather than just one version of the
> code, is important to provide sufficient information in many cases).

This is related to https://crbug.com/gerrit/11804, but it's not quite
the same.  It sounds like you'd like the snippets to be in unified
diff format (which makes sense to me).

> Most of the time, an initial response to a bug report in Bugzilla does not
> require opening a browser
[etc]

Thanks for this context as well.  It makes the workflow a bit more
concrete.

Sincerely,
Jonathan

Re: Setup non-pushing gerrit instance for glibc.

[Joseph Myers]

On Mon, 28 Oct 2019, Jonathan Nieder wrote:

> >                                        (b) not losing the quoted text
> > being replied to which is important to understanding the replies.
> 
> Can you say more about this (e.g. do you have an example)?

My example is from GDB.

https://sourceware.org/ml/gdb-patches/2019-10/msg00942.html was a message 
sent to gerrit, in reply to a comments message.

Now look at that one in gerrit - 
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/126 - or as it came 
back to the list from gerrit - 
https://sourceware.org/ml/gdb-patches/2019-10/msg00943.html - and note 
that in gerrit's version, the first paragraph "On 2019-10-26 7:10 p.m., 
Tom Tromey (Code Review) wrote:" is followed by the next paragraph *after* 
the quoted text - the quoted text in between has been lost (Simon's 
comment in gerrit two comments later clarifies for gerrit readers, "I 
replied by email, so that's the header my email client added when replying 
to Tom's comment, and Gerrit interpreted it as part of my comment.").

The problem of course isn't that gerrit kept that heading text, it's that 
it lost the quoted text after it.  Removing quoted text at the end of the 
message makes sense, but removing quoted text that's followed by comments 
doesn't.

(The examples in the gerrit documentation of email handling suggest it's 
already *supposed* to handle inline replies interspersed with quoted text, 
not just pure top-posting, but it evidently mishandled this particular 
email.)

> > * Handle email replies to notifications of new patches, not just to
> > comments on them.
> 
> I would expect this to already work as well.

So would I; only handling replies to comments seems an odd limitation.  I 
haven't verified if it does or not; I'm just going on what 
https://sourceware.org/ml/libc-alpha/2019-10/msg00812.html says about "We 
will also have to warn the user that replying directly to a new change 
message will not work; gerrit can only understand email replies to 
comments.".

> > * Include diff hunks in emails with comments on changed code (we now have
> > more context in the code quoted, which is an improvement, but seeing the
> > actual *changes* being commented on, rather than just one version of the
> > code, is important to provide sufficient information in many cases).
> 
> This is related to https://crbug.com/gerrit/11804, but it's not quite
> the same.  It sounds like you'd like the snippets to be in unified
> diff format (which makes sense to me).

Yes.  Giving snippets in diff format (assuming there actually are changes 
around the code in question rather than someone commenting on unmodified 
code) is a reasonable heuristic to give the relevant information in 90% of 
cases.  (The remaining 10% includes cases that already exist where e.g. 
reordering code in the file means the diff output isn't very helpful 
anyway, and whatever gerrit does then would be no worse than quoting an 
unhelpful diff in a manually sent email.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Setup non-pushing gerrit instance for glibc.

[Sergio Durigan Junior]

On Monday, October 28 2019, Joseph Myers wrote:

> On Mon, 28 Oct 2019, Jonathan Nieder wrote:
>> > * Handle email replies to notifications of new patches, not just to
>> > comments on them.
>> 
>> I would expect this to already work as well.
>
> So would I; only handling replies to comments seems an odd limitation.  I 
> haven't verified if it does or not; I'm just going on what 
> https://sourceware.org/ml/libc-alpha/2019-10/msg00812.html says about "We 
> will also have to warn the user that replying directly to a new change 
> message will not work; gerrit can only understand email replies to 
> comments.".

That's correct: replying directly to a new change notification does not
work.  Gerrit cannot parse the email due.  The only thing that works is
replying to a comment.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: Setup non-pushing gerrit instance for glibc.

[Szabolcs Nagy]

On 24/10/2019 20:39, Carlos O'Donell wrote:
> * Try out code reviews / CI etc. in gerrit.
>   - Code review of patches pushed to gerrit is possible.

'[BZ #..]' in subject line could be turned into bugzilla
links on the gerrit webui.

(i find the gerrit webui confusing and slow, but with a few
little tweaks it may become useful)

Re: Setup non-pushing gerrit instance for glibc.

[Gabriel F. T. Gomes]

Hi, community,

I finally used gerrit today.  Afterwards, I re-read this thread and here
are my impressions and questions...

First of all, thanks for doing this.  :)

(CC'ing Sergio, because I don't know if he follows the list and I have a
question for him)

1. On Reviewed-by statements,

On Tue, 12 Nov 2019, Florian Weimer (Code Review) wrote:
>
>(I'm not sure if we can keep up the practice of adding Reviewed-By: lines
>with Gerrit, because doing so creates a new patch version that needs
>review.)

Florian noticed [1] that Reviewed-by statements might break our use of
gerrit, because changing the commit message creates a new version of the
patch.  Carlos mentioned that having the statements is useful for his
metrics (I also like these fields, even though I don't use them as
professionally).

If we are going to make gerrit push automatically one day, could we make
it add the Reviewed-by statement automatically (based on the Reviewers
field) when it pushes the change?  Maybe it doesn't even need to create a
new version of the patch with the new commit message (although it would be
nice if it could create some sort of link between the patch that actually
gets commit and the patch displayed in the web interface (the same that is
sent to the mailing list)).

If that is possible, then the patch author (or even the reviewers) won't
need to change the commit message *during patch review*, which will avoid
the creation of the new patch version.

[1] https://sourceware.org/ml/libc-alpha/2019-11/msg00404.html

2. On in-reply-to fields,

Also in the same review [1], I noticed that the emails sent to the mailing
list are threaded in a weird way.  Everything seems to be sent in-reply-to
the first email of the thread, similar to bugzilla's behavior.

That is only a bit weird when it comes to patch versions, because the
subject lines contain a [review vN] tag, making it somewhat easier
(although not perfect) to distinguish which messages belong to each
version.  It will maybe become a bit more weird if a long discussion
happens in a sub-thread.

3. On quote selection freedom,

When writing reviews by email, I like to select the precise amount of
quotes (stitching different quotes together, and snipping unimportant
parts) that I judge relevant for the conversation.  This is, obviously,
a subjective judgment and I don't expect any tool to be able to do the
same ever (famous last words?).  My point here is, I didn't find a way to
do that (adjust the amount of quote and snipping) on the web interface.

On Fri, 08 Nov 2019, Simon Marchi wrote:
>
>So please try to use "Quote" instead of "Reply" and quote
>the relevant portion of what you are replying to (just like you would by
>email), so that it appears in the notification email.

Simon mentioned [2] a "Quote" (button?).  Maybe that is what I wanted and
couldn't find.

Like others in the community, I think I prefer using email, which gives me
freedom to select the amount of quoting and snipping I want, so maybe this
will never be a problem to anyone.

[2] https://sourceware.org/ml/libc-alpha/2019-11/msg00306.html

4. On the "patches go missing" problem,

On Sun, 27 Oct 2019, Sergio Durigan Junior wrote:
>
>When the GDB community decided to give gerrit a try, the main problem it
>was looking to solve was the "patches go missing" issue.  Basically, we
>have a lot of patches being sent to the project and not so many
>reviewers, so it's not uncommon for a patch to get "buried" waiting for
>a review, and if the author doesn't ping it, it gets lost.  With gerrit,
>you can get a pretty good view of all the patches that were submitted so
>far, and you can always check if a patch is too old or if it's been too
>long since the last interaction on it.

Can you (GDB community) already tell if gerrit actually helped with those
patches that went missing, or is it to soon?  While I understand that
gerrit has the potential to help with that, I also get the impression that
patches will still pile up, but on the web interface, instead of on the
mailing list.  I get this impression because our patchwork instance has
piles of patches (btw, this is not an inquisitorial kind of question - my
heart is still open for the new tool - I just want to know if you think
gerrit is helping).

Re: Setup non-pushing gerrit instance for glibc.

[Carlos O'Donell]

On 11/12/19 1:53 PM, Gabriel F. T. Gomes wrote:
> Hi, community,
> 
> I finally used gerrit today.  Afterwards, I re-read this thread and here
> are my impressions and questions...
> 
> First of all, thanks for doing this.  :)
> 
> (CC'ing Sergio, because I don't know if he follows the list and I have a
> question for him)
> 
> 1. On Reviewed-by statements,
> 
> On Tue, 12 Nov 2019, Florian Weimer (Code Review) wrote:
>>
>> (I'm not sure if we can keep up the practice of adding Reviewed-By: lines
>> with Gerrit, because doing so creates a new patch version that needs
>> review.)
> 
> Florian noticed [1] that Reviewed-by statements might break our use of
> gerrit, because changing the commit message creates a new version of the
> patch.  Carlos mentioned that having the statements is useful for his
> metrics (I also like these fields, even though I don't use them as
> professionally).
> 
> If we are going to make gerrit push automatically one day, could we make
> it add the Reviewed-by statement automatically (based on the Reviewers
> field) when it pushes the change?  Maybe it doesn't even need to create a
> new version of the patch with the new commit message (although it would be
> nice if it could create some sort of link between the patch that actually
> gets commit and the patch displayed in the web interface (the same that is
> sent to the mailing list)).
> 
> If that is possible, then the patch author (or even the reviewers) won't
> need to change the commit message *during patch review*, which will avoid
> the creation of the new patch version.
> 
> [1] https://sourceware.org/ml/libc-alpha/2019-11/msg00404.html

I try to explain the consequences and tradeoffs here:

https://www.sourceware.org/ml/libc-alpha/2019-11/msg00439.html

Yes, gerrit can automatically add Reviewed-by.

Yes it will work if we stick to one workflow for patch submission.


> 3. On quote selection freedom,
> 
> When writing reviews by email, I like to select the precise amount of
> quotes (stitching different quotes together, and snipping unimportant
> parts) that I judge relevant for the conversation.  This is, obviously,
> a subjective judgment and I don't expect any tool to be able to do the
> same ever (famous last words?).  My point here is, I didn't find a way to
> do that (adjust the amount of quote and snipping) on the web interface.

Highlight with your cursor the text you want, and hit 'c' to comment.

> On Fri, 08 Nov 2019, Simon Marchi wrote:
>>
>> So please try to use "Quote" instead of "Reply" and quote
>> the relevant portion of what you are replying to (just like you would by
>> email), so that it appears in the notification email.
> 
> Simon mentioned [2] a "Quote" (button?).  Maybe that is what I wanted and
> couldn't find.

Highlight, and hit 'c'?

> Like others in the community, I think I prefer using email, which gives me
> freedom to select the amount of quoting and snipping I want, so maybe this
> will never be a problem to anyone.

Does highlighting and pressing 'c' resolve this for you?

> [2] https://sourceware.org/ml/libc-alpha/2019-11/msg00306.html
> 
> 4. On the "patches go missing" problem,
> 
> On Sun, 27 Oct 2019, Sergio Durigan Junior wrote:
>>
>> When the GDB community decided to give gerrit a try, the main problem it
>> was looking to solve was the "patches go missing" issue.  Basically, we
>> have a lot of patches being sent to the project and not so many
>> reviewers, so it's not uncommon for a patch to get "buried" waiting for
>> a review, and if the author doesn't ping it, it gets lost.  With gerrit,
>> you can get a pretty good view of all the patches that were submitted so
>> far, and you can always check if a patch is too old or if it's been too
>> long since the last interaction on it.
> 
> Can you (GDB community) already tell if gerrit actually helped with those
> patches that went missing, or is it to soon?  While I understand that
> gerrit has the potential to help with that, I also get the impression that
> patches will still pile up, but on the web interface, instead of on the
> mailing list.  I get this impression because our patchwork instance has
> piles of patches (btw, this is not an inquisitorial kind of question - my
> heart is still open for the new tool - I just want to know if you think
> gerrit is helping).

The problem with Patchwork is that it never helped with patch review.

Patchwork was simply another way to view the mailing list.

Gerrit today lets me actually complete reviews from start-to-finish and
can act as an assitance to me.

For example Gerrit already tells me if patches have merge conflicts, so
I can ignore those or tell people to rebase.

Eventually if we get some ci/cd tied to gerrit it should be really really
helpful for reviewers.

-- 
Cheers,
Carlos.

Re: Setup non-pushing gerrit instance for glibc.

[Florian Weimer]

* Carlos O'Donell:

> For example Gerrit already tells me if patches have merge conflicts, so
> I can ignore those or tell people to rebase.

Gerrit's notion of a rebases and conflicts is a bit unclear to me.  I
think it doesn't run the real Git behind the scenes, so it can't know
what kind of conflicts can be resolved by automated merging.  For my
patches, I have seen both valid and spurious conflict indicators.  In
one case, I could hit Rebase in the web UI and it would automatically
rebase the patch, but it wasn't clear to me why it was needed in that
particular case.

I don't know if any of the other web-based tools handle this better.

Re: Setup non-pushing gerrit instance for glibc.

[Jonathan Nieder]

Florian Weimer wrote:

> Gerrit's notion of a rebases and conflicts is a bit unclear to me.  I
> think it doesn't run the real Git behind the scenes, so it can't know
> what kind of conflicts can be resolved by automated merging.

Gerrit uses JGit, which does perform automated merging.  It is an
independent implementation from Git --- there's no guarantee that
their behavior always matches each other, but if one is worse in a
particular case then the relevant project would consider it a bug.

Thanks,
Jonathan

Re: Setup non-pushing gerrit instance for glibc.

[Gabriel F. T. Gomes]

Hi, Carlos,

On Tue, 12 Nov 2019, Carlos O'Donell wrote:
>
>On 11/12/19 1:53 PM, Gabriel F. T. Gomes wrote:
>> 
>> Simon mentioned [2] a "Quote" (button?).  Maybe that is what I wanted and
>> couldn't find.  
>
>Highlight, and hit 'c'?

A-ha!  So that's how you do it.  Thanks!

>> Like others in the community, I think I prefer using email, which gives me
>> freedom to select the amount of quoting and snipping I want, so maybe this
>> will never be a problem to anyone.  
>
>Does highlighting and pressing 'c' resolve this for you?

It does.

And replying to comments in the web interface works like email, because I
can edit the quote, so yes, I think this cover the vast majority (if not
all) of the cases where I want to quote and snip.

>The problem with Patchwork is that it never helped with patch review.
>
>Patchwork was simply another way to view the mailing list.

I guess you're right.  Patches were piling up, probably because we weren't
using patchwork that much.

>Gerrit today lets me actually complete reviews from start-to-finish and
>can act as an assitance to me.
>
>For example Gerrit already tells me if patches have merge conflicts, so
>I can ignore those or tell people to rebase.
>
>Eventually if we get some ci/cd tied to gerrit it should be really really
>helpful for reviewers.

Right, right.  I hope you didn't get me wrong, I like the idea of having
gerrit for glibc and I certainly want to enable more workflows.  I am not
against gerrit, and I'm sorry if my comments sounded that way.

Thanks for your comments.

Re: Setup non-pushing gerrit instance for glibc.

[Carlos O'Donell]

On 11/12/19 4:10 PM, Gabriel F. T. Gomes wrote:
> Hi, Carlos,
> 
> On Tue, 12 Nov 2019, Carlos O'Donell wrote:
>>
>> On 11/12/19 1:53 PM, Gabriel F. T. Gomes wrote:
>>>
>>> Simon mentioned [2] a "Quote" (button?).  Maybe that is what I wanted and
>>> couldn't find.  
>>
>> Highlight, and hit 'c'?
> 
> A-ha!  So that's how you do it.  Thanks!
> 
>>> Like others in the community, I think I prefer using email, which gives me
>>> freedom to select the amount of quoting and snipping I want, so maybe this
>>> will never be a problem to anyone.  
>>
>> Does highlighting and pressing 'c' resolve this for you?
> 
> It does.
> 
> And replying to comments in the web interface works like email, because I
> can edit the quote, so yes, I think this cover the vast majority (if not
> all) of the cases where I want to quote and snip.
> 
>> The problem with Patchwork is that it never helped with patch review.
>>
>> Patchwork was simply another way to view the mailing list.
> 
> I guess you're right.  Patches were piling up, probably because we weren't
> using patchwork that much.
> 
>> Gerrit today lets me actually complete reviews from start-to-finish and
>> can act as an assitance to me.
>>
>> For example Gerrit already tells me if patches have merge conflicts, so
>> I can ignore those or tell people to rebase.
>>
>> Eventually if we get some ci/cd tied to gerrit it should be really really
>> helpful for reviewers.
> 
> Right, right.  I hope you didn't get me wrong, I like the idea of having
> gerrit for glibc and I certainly want to enable more workflows.  I am not
> against gerrit, and I'm sorry if my comments sounded that way.
> 
> Thanks for your comments.

Please accept my apologies if I sounded terse or defensive, that was not
my intent.

I would like to see us *really* experiment well with Gerrit, and I'm
absolutely over the moon at the number of patches Florian is feeding gerrit.

It has allowed me to easily track his patches, and they auto-close if we can
get the matching Changeset-ID in there and avoid the Reviewed-by: change
pitfall (by enabling pushes).

You can't believe how many times I have to ask people "Give me a list of URLs
to patches you want reviewed and keep it up to date please." which is just dumb
in this day and age.

-- 
Cheers,
Carlos.