Gerrit status update

Gerrit status update

[Simon Marchi]

Hi all,

We had a discussion on IRC yesterday about the shortcomings of Gerrit
for our workflow.  I'd like to share what was discussed, but also give
a chance to those who are not on IRC to have a voice.

The two main problems of Gerrit, when applied to our current workflow,
are related to patch series.  While it is possible to upload a sequence
of changes, it is not possible to treat a sequence of changes as a
logical patch series.  And therefore:

- It is not possible to attach a cover letter to a series.
- The email notifications are not threaded like a series would.  It's
  basically impossible to follow a series by email, as each patch will
  have its own little thread.

Some people (including me) may not mind these problems too much, but
they are important enough to some people that I don't think it would be
wise to continue using Gerrit for now.  Otherwise it will just increase
the friction for contributing and reviewing.  If Gerrit ever evolves
to handle patch series like we want it to, I would be happy to give it
another try.

I'd like to know what people think about this.

If we agree that we want to stop using Gerrit, I would disable the
creation of new changes, but still allow commenting and uploading new
change versions, so we can at least finish with the changes that are
there already.

---

If we abandon Gerrit, the question is, what now?  We have started
looking into a patch tracker after Tom Tromey mentioned at the Cauldron
that patches getting lost on the list was a problem (which I think most
people agree with).  Gerrit helped by giving us a list of open changes,
that would could filter and search however we liked.  It also brought
other advantages (and disadvantages), but the initial problem was
tracking pending patches.

The other tool that gets mentioned often, which could help with this, is
Patchwork:

  http://jk.ozlabs.org/projects/patchwork/

There is already a Patchwork instance that tracks the GDB mailing list
here:

  https://patchwork.sourceware.org/project/gdb/list/

that we have tried to use in the past (now it's filled with Gerrit
notifications, don't mind those, it's not representative of how it would
look).  The problem was that manual intervention was needed to update
the status of patches, which was never done, so it didn't help.

A newer version of Patchwork includes a git hook that can supposedly
automatically close patches when they are pushed to the git repository:

  https://patchwork.readthedocs.io/en/latest/deployment/installation/#optional-configure-your-vcs-to-automatically-update-patches

If this works reasonably well enough, it could automate a good part of
the grunt work and make Patchwork usable.

I am just a bit skeptical about how well (if it does it at all)
Patchwork handles patches or series versions.  If I send a v1 series
with 15 patches, then a v2 with 17 patches, including a few renamed
subjects, how does it cope with that?  I am a bit afraid that it won't
be that good (though I'd like to be proven wrong) and that it will
require a lot of manual work to close patches.

The good news is that it should be possible to try this with a local
Patchwork test instance:

1. Feed it a bunch of messages from the gdb-patches archive, see how
   it handles patches/series versions.
2. Then, feed it the git commits from binutils-gdb, see how well it
   auto closes patches.

I might do that in the following days, when I have some free time and
a sudden impulse of motivation.

Gerrit is able to track the multiple versions of a change because we
insert a unique Change-Id in the commit log of each patch.  This is very
reliable, and it would make handling of new versions/merged patches very
accurate.  But it goes against the Patchwork design:

  https://patchwork.readthedocs.io/en/latest/usage/design/

  "Don’t pollute the project’s changelogs with Patchwork poop

   A project’s changelogs are valuable - we don’t want to add Patchwork-specific metadata."

So unfortunately, we won't get that from upstream.  But it would always
be possible to modify Patchwork to add this feature for our own
enjoyment.  Nothing would force people who send patches to have this
Id, but if the majority of the frequent contributors use it, it could
still be useful.

Simon

Re: Gerrit status update

[Tom Tromey]

Simon> I'd like to share what was discussed, but also give a chance to
Simon> those who are not on IRC to have a voice.

Thanks for doing this.

Simon> If we agree that we want to stop using Gerrit, I would disable the
Simon> creation of new changes, but still allow commenting and uploading new
Simon> change versions, so we can at least finish with the changes that are
Simon> there already.

If this happens, it would also be fine by me to just have everybody
resubmit their patches.


I switched most of my patch submissions to gerrit.  I find it has some
nice benefits -- the tracking, but also I think the review interface is
quite nice.  In some ways, it's even nicer than email, because I find
that side-by-side diffs are often easier to read.

However, I do find the lack of a cover letter and the lack of threading
to be minuses.  It's harder to follow gdb-patches now.


Simon> A newer version of Patchwork includes a git hook that can supposedly
Simon> automatically close patches when they are pushed to the git repository:
Simon>   https://patchwork.readthedocs.io/en/latest/deployment/installation/#optional-configure-your-vcs-to-automatically-update-patches
...
Simon> I am just a bit skeptical about how well (if it does it at all)
Simon> Patchwork handles patches or series versions.  If I send a v1 series
Simon> with 15 patches, then a v2 with 17 patches, including a few renamed
Simon> subjects, how does it cope with that?

I'm not too worried about this scenario (see below) but I think one to
test is if a patch is abandoned from the middle of a series.  Making
this work requires some notion of a series in the first place.  I think
the new Patchwork has this, but it's been a while since I looked.

Simon> Gerrit is able to track the multiple versions of a change because we
Simon> insert a unique Change-Id in the commit log of each patch.  This is very
Simon> reliable, and it would make handling of new versions/merged patches very
Simon> accurate.  But it goes against the Patchwork design:

Simon>   https://patchwork.readthedocs.io/en/latest/usage/design/

Simon>   "Don’t pollute the project’s changelogs with Patchwork poop

Simon>    A project’s changelogs are valuable - we don’t want to add Patchwork-specific metadata."

Simon> So unfortunately, we won't get that from upstream.  But it would always
Simon> be possible to modify Patchwork to add this feature for our own
Simon> enjoyment.

Yeah... I think this bit of Patchwork dogma is simply mistaken.
Reliability requires a patch ID that does not vary; and since git
doesn't provide this, it has to be provided externally.

Furthermore, we already accepted these IDs for gerrit.  I find them
readily ignorable.

I think modifying Patchwork to do this is very simple.  First, we could
either re-use the gerrit commit hook, or write our own (I wrote one
once, it's in my github).

Second, in the Patchwork tree is a file, tools/post-receive.hook.  This
is a shell script that has a function to get the patch hash:

    get_patchwork_hash() {
        local hash
        hash=$(git diff "$1~..$1" | python $PW_DIR/hasher.py)
        echo "$hash"
        test -n "$hash"
    }

I think all that's needed is to replace this with something to extract
our ID.  This can be done with "git interpret-trailers --parse".


One other thing gerrit provides is the ability to fetch patches from
gerrit.  This can be done with patchworks as well, using its CLI.
However, I imagine it is less reliable, since it isn't git on the other
end... like, with git you can do 3-way merges, but with patchworks you
couldn't.


I've looked at patchworks a little, as you can tell; but I haven't
looked at gerrit at all.  So, I have no idea of the relative difficulty
of adding the features we want to gerrit.

Tom

Re: Gerrit status update

[Luis Machado]

On 11/22/19 6:58 PM, Simon Marchi wrote:
> Hi all,
> 
> We had a discussion on IRC yesterday about the shortcomings of Gerrit
> for our workflow.  I'd like to share what was discussed, but also give
> a chance to those who are not on IRC to have a voice.

Thanks.

> 
> The two main problems of Gerrit, when applied to our current workflow,
> are related to patch series.  While it is possible to upload a sequence
> of changes, it is not possible to treat a sequence of changes as a
> logical patch series.  And therefore:
> 
> - It is not possible to attach a cover letter to a series.
> - The email notifications are not threaded like a series would.  It's
>    basically impossible to follow a series by email, as each patch will
>    have its own little thread.
> 
I went through yours and Tom's e-mail, but also found glibc's take on it 
(https://sourceware.org/ml/libc-alpha/2019-11/msg00774.html).

Carlos pointed out a possible way to have a cover letter, and apparently 
that is more or less the recommended way of doing it.

It seems to me the biggest issue is having gerrit translated properly to 
e-mail format, both in terms of interaction and organization of replies 
(threaded series etc).

On the positive side i think gerrit provides quite a few useful features 
to speed up the process of submission and reviewing of patches, which 
people have already pointed out. So i won't list those here.

Considering the drawbacks of gerrit seem to be mostly concentrated 
around series of patches, maybe bigger series in particular, should we 
try and do those through e-mail instead. Maybe as a recommendation?

For other smaller contributions, gerrit still seems to be fairly useful.

As for patchwork, my experience using it in the past wasn't that great. 
It may have improved since then, but i remember it simply being 
forgotten over time, and patches piling up anyway.

Re: Gerrit status update

[Simon Marchi]

On 2019-12-05 7:14 p.m., Luis Machado wrote:
> On 11/22/19 6:58 PM, Simon Marchi wrote:
>> Hi all,
>>
>> We had a discussion on IRC yesterday about the shortcomings of Gerrit
>> for our workflow.  I'd like to share what was discussed, but also give
>> a chance to those who are not on IRC to have a voice.
> 
> Thanks.
> 
>>
>> The two main problems of Gerrit, when applied to our current workflow,
>> are related to patch series.  While it is possible to upload a sequence
>> of changes, it is not possible to treat a sequence of changes as a
>> logical patch series.  And therefore:
>>
>> - It is not possible to attach a cover letter to a series.
>> - The email notifications are not threaded like a series would.  It's
>>    basically impossible to follow a series by email, as each patch will
>>    have its own little thread.
>>
> I went through yours and Tom's e-mail, but also found glibc's take on it (https://sourceware.org/ml/libc-alpha/2019-11/msg00774.html).

Oh, thanks for pointing our that thread on the libc list, I didn't know about
it.  I also went through it, I think overall it summarizes the situation pretty
well.  You could s/glibc/gdb/ in the discussion and it would still apply.

> Carlos pointed out a possible way to have a cover letter, and apparently that is more or less the recommended way of doing it.

Yes, I agree that having an empty commit at the end of the chain would
probably work well enough.

> 
> It seems to me the biggest issue is having gerrit translated properly to e-mail format, both in terms of interaction and organization of replies (threaded series etc).
> 
> On the positive side i think gerrit provides quite a few useful features to speed up the process of submission and reviewing of patches, which people have already pointed out. So i won't list those here.

Agreed.

> Considering the drawbacks of gerrit seem to be mostly concentrated around series of patches, maybe bigger series in particular, should we try and do those through e-mail instead. Maybe as a recommendation?
> 
> For other smaller contributions, gerrit still seems to be fairly useful.

Hmm, I'm not sure I'd like to keep two parallel systems, with a vague rule
to tell which one to use.  It will be confusing for us and newcomers.

And don't forget that the initial goal of this effort is to improve the
tracking of patches, to avoid patches falling between the cracks.  If some
patches are sent by email and are not tracked, we are partially missing our
goal.

Also, I think the benefits of Gerrit shine particularly when dealing with
patch series, especially the diff-between-versions.  For a single patch,
I can easily apply the two patches locally and do a simple diff.

I just learned about git range-diff, reading the glibc thread.  I'll try to
use it in the future to compare patch series versions.  Although it doesn't
seem to be able to show the diffs using one's favorite diff viewer, and I'm
not particularly fond of reading raw diffs.

> As for patchwork, my experience using it in the past wasn't that great. It may have improved since then, but i remember it simply being forgotten over time, and patches piling up anyway.

Yes, that's what the experience has shown.  The amount of manual work required
to maintain it made it not worth it.  So I would not consider using Patchwork
without some accurate patch tracking based on some kind Change-Id, like we
have with Gerrit, that would allow us to do some automation.

We can start to do this outside Patchwork, using some script that use the
Patchwork API.  If that works well enough, we could even attempt to
contribute it upstream.  One of the Patchwork developer is open to include
this kind of feature:

  https://github.com/getpatchwork/patchwork/issues/327#issuecomment-561573729

Simon

Re: Gerrit status update

[Simon Marchi]

On 2019-11-22 7:25 p.m., Tom Tromey wrote:
> Simon> A newer version of Patchwork includes a git hook that can supposedly
> Simon> automatically close patches when they are pushed to the git repository:
> Simon>   https://patchwork.readthedocs.io/en/latest/deployment/installation/#optional-configure-your-vcs-to-automatically-update-patches
> ...
> Simon> I am just a bit skeptical about how well (if it does it at all)
> Simon> Patchwork handles patches or series versions.  If I send a v1 series
> Simon> with 15 patches, then a v2 with 17 patches, including a few renamed
> Simon> subjects, how does it cope with that?
> 
> I'm not too worried about this scenario (see below) but I think one to
> test is if a patch is abandoned from the middle of a series.  Making
> this work requires some notion of a series in the first place.  I think
> the new Patchwork has this, but it's been a while since I looked.

Yes, it does.  We could give series an id of their own (Series-Id?) and
mark all the patches from v1 as obsolete when v2 is sent.  I'm not sure if
and how that Series-Id could be automatically generated and included in
the cover letter of subsequent versions, but that's just a technical detail.

I'm not too worried about this scenario in the first place.  If you send a
lone patch and decide to abandon it, you would need to go to Patchwork to
mark it as abandonned.  If you drop a patch from a series, it's the same.

> 
> Simon> Gerrit is able to track the multiple versions of a change because we
> Simon> insert a unique Change-Id in the commit log of each patch.  This is very
> Simon> reliable, and it would make handling of new versions/merged patches very
> Simon> accurate.  But it goes against the Patchwork design:
> 
> Simon>   https://patchwork.readthedocs.io/en/latest/usage/design/
> 
> Simon>   "Don’t pollute the project’s changelogs with Patchwork poop
> 
> Simon>    A project’s changelogs are valuable - we don’t want to add Patchwork-specific metadata."
> 
> Simon> So unfortunately, we won't get that from upstream.  But it would always
> Simon> be possible to modify Patchwork to add this feature for our own
> Simon> enjoyment.
> 
> Yeah... I think this bit of Patchwork dogma is simply mistaken.
> Reliability requires a patch ID that does not vary; and since git
> doesn't provide this, it has to be provided externally.

I reached out to Patchwork devs, and it seems like they would be open to
the idea of supporting tracking patches using a Change-Id:

  https://github.com/getpatchwork/patchwork/issues/327#issuecomment-561573729

I think the idea is simply that it should not be mandatory to have them
to use Patchwork.

> Furthermore, we already accepted these IDs for gerrit.  I find them
> readily ignorable.
> 
> I think modifying Patchwork to do this is very simple.  First, we could
> either re-use the gerrit commit hook, or write our own (I wrote one
> once, it's in my github).
> 
> Second, in the Patchwork tree is a file, tools/post-receive.hook.  This
> is a shell script that has a function to get the patch hash:
> 
>     get_patchwork_hash() {
>         local hash
>         hash=$(git diff "$1~..$1" | python $PW_DIR/hasher.py)
>         echo "$hash"
>         test -n "$hash"
>     }
> 
> I think all that's needed is to replace this with something to extract
> our ID.  This can be done with "git interpret-trailers --parse".

Yep.

> One other thing gerrit provides is the ability to fetch patches from
> gerrit.  This can be done with patchworks as well, using its CLI.
> However, I imagine it is less reliable, since it isn't git on the other
> end... like, with git you can do 3-way merges, but with patchworks you
> couldn't.

When the author has used send-email, "git am -3" usually works well. I hope
that git-pw uses it.

> I've looked at patchworks a little, as you can tell; but I haven't
> looked at gerrit at all.  So, I have no idea of the relative difficulty
> of adding the features we want to gerrit.

From what I saw, it's way more than the time I have to put on it without
becoming a full-time Gerrit developer.

Simon