Re: Review process improvements

Re: Review process improvements

[Konstantin Ryabitsev]

On Thu, Dec 16, 2021 at 02:46:21PM -0800, Emily Shaffer wrote:
> Some of those discussions resulted in changes - for example,
> GitGitGadget was improved and added to git/git, and we enjoy easy,
> non-noisy CI runs via GitHub Actions. But some things we thought were a
> good idea never went into practice. In the next few months, the Google
> Git team is planning to implement some of the following changes, and
> we'd appreciate your thoughts ahead of time as well as your review later
> on:

I'd like to also mention that I'm hoping to add a few more features to b4 that
will hopefully improve the patch submission process for folks. This feature
set is far from being complete yet, but the gist will be as follows:

1. b4 submit --new: this will create a new tracking branch and define
   some metadata to go with it, such as a cover letter template. The cover
   letter can be edited using `b4 submit --edit-cover` at any time.

2. b4 submit --send: will generate a patch series from any commits created
   from the topical branch fork point and use the cover letter from the
   previous step. It will be able to send the patches using the traditional
   SMTP way, OR it will allow using a web-based submission service I'm setting
   up at kernel.org:

   - anyone will be able to submit valid patches through this service
   - one-off patch submissions will require an email confirmation roundtrip
   - any submitter can also register their ed25519/openssh patatt key with the
     submission service and just cryptographically sign their patches. As long
     as signatures validate, no roundtrip confirmation of patches will be
     required after the initial public key registration.
   - on the receiving end, the patches will be written to a dedicated
     lore.kernel.org feed *as-is*, but also sent to the recipients after doing
     the usual From/Reply-To substitution and moving the original From into
     the in-body git headers (i.e. same as GGG does).

3. b4 submit will properly include base-commit information to all submitted
   patches, as well as a unique change-id trailer (but in the basement of the
   cover letter, not in the commit message trailers as Gerrit does).

4. b4 submit --sync: will retrieve any received code review trailers
   (reviewed-by, acked-by, etc) and amend the corresponding commits in the
   topical branch, assuming we can match patch-id's (I've not tackled this
   yet, so I may be unduly optimistic here).

5. b4 submit --reroll: will prepare a v2 (v3, v4) of the series, reusing the
   same change-id trailer and adding a templated "Changes in v2" entry to the
   cover letter (that must be edited before --send works again).

6. b4 submit --send: will send the new version of the series.

I'm hoping that this will improve the experience of patch submitters *and*
help ensure that CI can be incorporated into the process by streamlining the
submission procedure and providing a public-inbox feed that can be easily
monitored. A service like GGG can fairly easily convert well-formed patch
series (at least those carrying valid base-commit info) into ephemeral
worktrees, or even into simulated pull requests if the goal is to do this via
a generic CI service.

The important goal is to keep development fully decentralized so that even if
the web submission endpoint provided by kernel.org becomes unavailable, plain
old SMTP submission mechanisms can still be used as a fallback.

(Unfortunately, I need to focus my efforts on some kernel.org infrastructure
changes at this time, so I'm not sure when I'll be able to complete these
features.)

> 1. Draft a MAINTAINERS file

So, I *don't* recommend that you go this route. The biggest problem with the
MAINTAINERS file as used by the Linux development community is that making
changes to it is a very high-latency process. It will be less of a problem for
the much smaller git developer community, but it will still result in folks
operating from a stale copy of the file for weeks after it is updated
upstream.

One of the goals behind the "lei" tool by public-inbox is that it allows
search-based subscriptions (including things like "what files are being
modified, what functions are mentioned in the git context", etc). Anyone can
define a set of parameters they are interested in monitoring and receive only
threads matching those pre-filtered messages. We are currently rolling this
out as an end-user maintained setup [1], but the goal is to also offer this to
maintainers as a centrally managed service:

- maintainers will be able to define the search queries they are interested in
  monitoring
- we will set up a feed on our end matching those queries
- we will offer the feed as a separate public-inbox repository, a public IMAP
  folder, a read-only mailing list, and, I'm hoping, a read-only POP box (the
  latter mostly so it can be configured to feed into GMail).

The hope is that this will allow what you're looking for -- a way to
pre-filter the flow of patches to maintainers by making it topical, without
making the lives of patch submitters unnecessarily difficult by increasing the
chances that they will send their patches to the wrong list.

[1] https://people.kernel.org/monsieuricon/lore-lei-part-1-getting-started

I'm not sure where this all fits into your plans, but I wanted to show what is
happening on our end just so we're working in parallel, as opposed to in our
own separate worlds. :)

Best regards,
-K

Re: Review process improvements

[Christian Couder]

On Fri, Dec 17, 2021 at 11:00 PM Konstantin Ryabitsev
<konstantin@linuxfoundation.org> wrote:
>
> On Thu, Dec 16, 2021 at 02:46:21PM -0800, Emily Shaffer wrote:
> > Some of those discussions resulted in changes - for example,
> > GitGitGadget was improved and added to git/git, and we enjoy easy,
> > non-noisy CI runs via GitHub Actions. But some things we thought were a
> > good idea never went into practice. In the next few months, the Google
> > Git team is planning to implement some of the following changes,

Thanks for trying to make it easier and more efficient to contribute!

> > and
> > we'd appreciate your thoughts ahead of time as well as your review later
> > on:
>
> I'd like to also mention that I'm hoping to add a few more features to b4 that
> will hopefully improve the patch submission process for folks.

Thanks also for implementing and maintaining tools and sites that help
contributing!

I have added a new "Process related tools and sites" section to
https://git.github.io/Hacking-Git/ with links to GitGitGadget,
lore.kernel.org/git, public-inbox, lore+lei, b4 and git-series. I am
willing to add other tools and sites if someone knows others worth
mentioning.

[...]

> > 1. Draft a MAINTAINERS file
>
> So, I *don't* recommend that you go this route. The biggest problem with the
> MAINTAINERS file as used by the Linux development community is that making
> changes to it is a very high-latency process. It will be less of a problem for
> the much smaller git developer community, but it will still result in folks
> operating from a stale copy of the file for weeks after it is updated
> upstream.

My opinion is that a MAINTAINERS file makes more sense when there are
different mailing lists to send patches to, and trusted
lieutenants/maintainers. I am not sure that we are at that point yet.

About documentation, if some good documentation exists elsewhere, it
might be good enough to just point to it from an existing doc and/or
https://git.github.io/Hacking-Git/. Otherwise adding a new separate
doc might be better than growing an existing doc too much, which could
scare new comers.

Re: Review process improvements

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

On Fri, Dec 17, 2021 at 01:39:42PM -0500, Konstantin Ryabitsev wrote:

> 2. b4 submit --send: will generate a patch series from any commits created
>    from the topical branch fork point and use the cover letter from the
>    previous step. It will be able to send the patches using the traditional
>    SMTP way, OR it will allow using a web-based submission service I'm setting
>    up at kernel.org:

One thing my equivalent of this does (I might've used patman but I don't
think I was aware of it at the time) is to tag each revision as it's
published, might be worth considering.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Review process improvements

[Ævar Arnfjörð Bjarmason]

On Fri, Dec 17 2021, Konstantin Ryabitsev wrote:

> [...]
>    - on the receiving end, the patches will be written to a dedicated
>      lore.kernel.org feed *as-is*, but also sent to the recipients after doing
>      the usual From/Reply-To substitution and moving the original From into
>      the in-body git headers (i.e. same as GGG does).

That GGG does this is one reason I haven't considered using it. It
breaks all sorts of E-Mail workflow assumptions from polluting the
address book for every person who uses it, to any "from:<addr>" search
needing special consideration etc.

Of course you'd need authentication etc, but is there a reason for why
such tooling can't work more like an SMTP relay and less like GGG which
(for some reason) insists on taking over the "From" header?

I think in its case it's because it wanted to mirror all the discussion
to GitHub. But presumably a ML-native tool won't have that problem (and
presumably GGG could do the same mirroring by following the ML after
submission).

Re: Review process improvements

[Fabian Stelzer]

On 22.12.2021 04:26, Ævar Arnfjörð Bjarmason wrote:
>
>On Fri, Dec 17 2021, Konstantin Ryabitsev wrote:
>
>> [...]
>>    - on the receiving end, the patches will be written to a dedicated
>>      lore.kernel.org feed *as-is*, but also sent to the recipients after doing
>>      the usual From/Reply-To substitution and moving the original From into
>>      the in-body git headers (i.e. same as GGG does).
>
>That GGG does this is one reason I haven't considered using it. It
>breaks all sorts of E-Mail workflow assumptions from polluting the
>address book for every person who uses it, to any "from:<addr>" search
>needing special consideration etc.

I agree that this is a very annoying side-effect of GGG.

>
>Of course you'd need authentication etc, but is there a reason for why
>such tooling can't work more like an SMTP relay and less like GGG which
>(for some reason) insists on taking over the "From" header?
>

I think SPF/DMARC spam filtering will prevent tools like GGG to do this any 
other way. For GGG to be able to send with your From: you'd either need to 
give it access to your smtp credentials or configure your whole domain to 
allow GGG mail servers (basically github) to send email for it. I think both 
options are not really something you'd want.

>I think in its case it's because it wanted to mirror all the discussion
>to GitHub. But presumably a ML-native tool won't have that problem (and
>presumably GGG could do the same mirroring by following the ML after
>submission).
>

Re: Review process improvements

[Konstantin Ryabitsev]

On Wed, Dec 22, 2021 at 04:26:39AM +0100, Ævar Arnfjörð Bjarmason wrote:
> That GGG does this is one reason I haven't considered using it. It
> breaks all sorts of E-Mail workflow assumptions from polluting the
> address book for every person who uses it, to any "from:<addr>" search
> needing special consideration etc.
> 
> Of course you'd need authentication etc, but is there a reason for why
> such tooling can't work more like an SMTP relay and less like GGG which
> (for some reason) insists on taking over the "From" header?

This would require pretending that we're authorized to send mail from the
domain name of the commit author, so this unfortunately won't work (and hence
the reason why GGG does it this way). E.g. say you have:

From: foo@redhat.com
Subject: [PATCH] Fix foo

For DMARC policies to properly work, this message would need to have a DKIM
signature from redhat.com, so we'd need to have access to Red Hat's private
keys. If we don't use DKIM signatures, then the recipient SMTP gateways may
mark the message as spam (and they would be right, since we are pretending to
be foo@redhat.com, i.e. acting just like phishers).

The only way for this to work is to do the From / X-Original-From /
Follow-up-to / Reply-To header dance. Git does support this very well, and
most email clients do the right thing when encountering this situation, but
it's not going to have the exact same visual flow as patches sent directly via
SMTP.

However, we will also write these messages to a public-inbox repository before
making the From: substitutions, so if someone retrieves these patches with b4
or lei, as opposed to receiving them via their SMTP mailbox, they should be
able to get proper From: in the headers.

-K

Re: Review process improvements

[Junio C Hamano]

Konstantin Ryabitsev <konstantin@linuxfoundation.org> writes:

> This would require pretending that we're authorized to send mail from the
> domain name of the commit author, so this unfortunately won't work (and hence
> the reason why GGG does it this way). E.g. say you have:
>
> From: foo@redhat.com
> Subject: [PATCH] Fix foo

Would it help to use "Sender:"?  When GGG or any other automation
are trying to send e-mail on behalf of the person shown on "From:",
I thought that it is the mechanism for them to use to identify
themselves.

Re: Review process improvements

[Konstantin Ryabitsev]

On Wed, Dec 22, 2021 at 11:42:02AM -0800, Junio C Hamano wrote:
> > This would require pretending that we're authorized to send mail from the
> > domain name of the commit author, so this unfortunately won't work (and hence
> > the reason why GGG does it this way). E.g. say you have:
> >
> > From: foo@redhat.com
> > Subject: [PATCH] Fix foo
> 
> Would it help to use "Sender:"?  When GGG or any other automation
> are trying to send e-mail on behalf of the person shown on "From:",
> I thought that it is the mechanism for them to use to identify
> themselves.

Indeed, that's how the DKIM standard wanted to deal with this problem, however
when the DMARC RFC was being drafted, this approach was deemed insufficient.
They have a good explanation for it -- there is no standard among UI clients
to handle the Sender/From discrepancy. Most MUAs will happily ignore the
Sender: field and will only show what is in From:, so this approach was
considered ineffective against phishing attacks. An attacker could easily
register a domain, set DKIM records, and then use any From: they wanted as
long as they used a valid Sender: header, knowing that it would be ignored by
most mail clients.

So, DMARC deliberately ignores the Sender: header and *only* pays attention to
the From: field for its purpose.

-K

Why GitGitGadget does not use Sender:, was Re: Review process improvements

[Johannes Schindelin]

Hi Junio,

On Wed, 22 Dec 2021, Junio C Hamano wrote:

> Konstantin Ryabitsev <konstantin@linuxfoundation.org> writes:
>
> > This would require pretending that we're authorized to send mail from the
> > domain name of the commit author, so this unfortunately won't work (and hence
> > the reason why GGG does it this way). E.g. say you have:
> >
> > From: foo@redhat.com
> > Subject: [PATCH] Fix foo
>
> Would it help to use "Sender:"?

You had suggested that before, and I explained why it does not work in
https://lore.kernel.org/git/nycvar.QRO.7.76.6.2008241445200.56@tvgsbejvaqbjf.bet/.
The gist is: GMail interferes with your cunning plan.

> When GGG or any other automation are trying to send e-mail on behalf of
> the person shown on "From:", I thought that it is the mechanism for them
> to use to identify themselves.

Ciao,
Dscho

Re: Why GitGitGadget does not use Sender:, was Re: Review process improvements

[Junio C Hamano]

Konstantin gave a more generally applicable answer to think about
this issue in <20211222213247.5dnj3zlj53lh6l32@meerkat.local> in a
separate thread last year.  I would not be surprised if GMail pays
attention to the reasoning he gave in that message and that is the
reason why it does not use the "Sender" as I hoped.  I expect other
e-mail providers should do the same.

Thanks.  

Re: Review process improvements

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1487 bytes --]

> I'd like to also mention that I'm hoping to add a few more features to b4 that
> will hopefully improve the patch submission process for folks.

This looks excellent! I wanted to mention a few features that have been
popular in the git-publish patch series management tool
(https://github.com/stefanha/git-publish/) in case you want to include
something similar in b4:

- "Profiles" (.gitpublishprofile or stored in .git/config) with To and
  CC addresses, --subject-prefix, cover letter templates, etc. The
  default profile is automatically loaded when you run "git publish". If
  the git repo you're working on includes a default profile then you
  don't need to specify any command-line options to send a correctly
  formatted patch series to the maintainers! A great help for one-off
  contributors but also a time-saver for regular contributors.

- An interactive mode that lets you inspect the final patch series and
  edit the CC list. Avoids mistakes and embarassment :).

- Saving the CC list between revisions (v2, v3, etc).

- Sending pull requests (for sub-maintainers) because there are times
  when pull requests also need a v2 or even a v3 ;). The workflow is
  basically the same as sending patch series.

- githooks(5) before sending patches series and pull requests. Useful
  for coding style and patch series linters.

The git-publish man page is here if you want to read more:
https://github.com/stefanha/git-publish/blob/master/git-publish.pod#quickstart

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]