RFC: using supersedes: trailer to indicate patch/series revision flow

RFC: using supersedes: trailer to indicate patch/series revision flow

[Konstantin Ryabitsev]

Hi, all:

The only mechanism we currently have for patch/series versioning is 
subject suffixes. I think it would be useful to have a way to more 
explicitly mark that a series obsoletes a previous version, and I 
propose this is done with a `supersedes:` trailer at the end of the 
cover letter or in the first patch of the series:

E.g.:

Initial patch:

---8<---
From: Dev Eloper <dev.eloper@example.com>
Message-Id: <1572991351-86061-1-git-send-email-dev.eloper@example.com>
Subject: [PATCH] Change foo

Foo is no good. Use Bar.

Signed-off-by: Dev Eloper <dev.eloper@example.com>
---
  foo | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/foo b/foo
...

base-commit: 23fdb198ae81f47a574296dab5167c5e136a02ba
-- 
2.24.0
---8<---

Follow-up patch:

---8<---
From: Dev Eloper <dev.eloper@example.com>
Message-Id: <1572991352-86062-2-git-send-email-dev.eloper@example.com>
Subject: [PATCH,v2] Change foo

Foo is no good. Use Bar. Also use baz.

Signed-off-by: Dev Eloper <dev.eloper@example.com>
---
  foo | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/foo b/foo
...

supersedes: <1572991351-86061-1-git-send-email-dev.eloper@example.com>
base-commit: 23fdb198ae81f47a574296dab5167c5e136a02ba
-- 
2.24.0
---8<---

Questions:

1. Should this be exposed via git-format-patch flags, or just used by 
    specialized tooling?
2. Should supersedes: link to the previous version of the patch, or the 
    first ever version of the patch? I am leaning towards the latter, 
    even though in this case the message-id largely becomes identical in 
    usage to Gerrit's Change-Id.
3. Should the supersedes trailer have:
    a. message-id without brackets
    b. message-id with brackets
    c. https://lore.kernel.org/r/message-id
    My preference is b, to match with The Message-Id header usage.


-K

Re: RFC: using supersedes: trailer to indicate patch/series revision flow

[Rafael J. Wysocki]

On Thursday, November 7, 2019 9:43:49 PM CET Konstantin Ryabitsev wrote:
> Hi, all:
> 
> The only mechanism we currently have for patch/series versioning is 
> subject suffixes. I think it would be useful to have a way to more 
> explicitly mark that a series obsoletes a previous version, and I 
> propose this is done with a `supersedes:` trailer at the end of the 
> cover letter or in the first patch of the series:

All such things are a pain for patches generated by quilt, say.

> E.g.:
> 
> Initial patch:
> 
> ---8<---
> From: Dev Eloper <dev.eloper@example.com>
> Message-Id: <1572991351-86061-1-git-send-email-dev.eloper@example.com>
> Subject: [PATCH] Change foo
> 
> Foo is no good. Use Bar.
> 
> Signed-off-by: Dev Eloper <dev.eloper@example.com>
> ---
> 
>   foo | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/foo b/foo
> ...
> 
> base-commit: 23fdb198ae81f47a574296dab5167c5e136a02ba
> --
> 2.24.0
> ---8<---
> 
> Follow-up patch:
> 
> ---8<---
> From: Dev Eloper <dev.eloper@example.com>
> Message-Id: <1572991352-86062-2-git-send-email-dev.eloper@example.com>
> Subject: [PATCH,v2] Change foo
> 
> Foo is no good. Use Bar. Also use baz.
> 
> Signed-off-by: Dev Eloper <dev.eloper@example.com>
> ---
> 
>   foo | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/foo b/foo
> ...
> 
> supersedes: <1572991351-86061-1-git-send-email-dev.eloper@example.com>
> base-commit: 23fdb198ae81f47a574296dab5167c5e136a02ba
> --
> 2.24.0
> ---8<---
> 
> Questions:
> 
> 1. Should this be exposed via git-format-patch flags, or just used by
>     specialized tooling?

I'd vote for specialized tooling (if anything).

> 2. Should supersedes: link to the previous version of the patch, or the
>     first ever version of the patch? I am leaning towards the latter,

And then how do you know that version 2 was superseded by version 3?

>     even though in this case the message-id largely becomes identical in
>     usage to Gerrit's Change-Id.
> 
> 3. Should the supersedes trailer have:
>     a. message-id without brackets
>     b. message-id with brackets
>     c. https://lore.kernel.org/r/message-id
>     My preference is b, to match with The Message-Id header usage.

An id of the exact patch (or series) superseded by this one IMO.

Cheers!

Re: RFC: using supersedes: trailer to indicate patch/series revision flow

[Andrew Donnellan]

On 8/11/19 7:43 am, Konstantin Ryabitsev wrote:
> Questions:
> 
> 1. Should this be exposed via git-format-patch flags, or just used by    
> specialized tooling?

I guess we might want to expose it through git format-patch for the sake 
of completeness? Though if people actually use it with git format-patch, 
then the answer to Q2 is irrelevant because people will use it to link 
to whatever the heck they feel like at the time...

Though what do you envision the user workflow actually looking like with 
a specialised tool?

> 2. Should supersedes: link to the previous version of the patch, or the 
>     first ever version of the patch? I am leaning towards the latter,    
> even though in this case the message-id largely becomes identical in    
> usage to Gerrit's Change-Id.

If we link to the first version of the patch, we can't tell that V3 
supersedes V2. Though if we link to only the previous version, we can't 
tell that V3 supersedes V1 without looking at V2.

 From a Patchwork perspective, if we were to use this to track patch 
versions I think we could work with either, except for some corner cases 
where mail is not received or received in the wrong order and there's 
not enough subject metadata to work things out.

You could also include multiple trailers e.g. the first version and last 
version, or all previous versions that are superseded, though that seems 
a bit verbose.

> 3. Should the supersedes trailer have:
>     a. message-id without brackets
>     b. message-id with brackets
>     c. https://lore.kernel.org/r/message-id
>     My preference is b, to match with The Message-Id header usage.

A or B makes the most sense.

-- 
Andrew Donnellan              OzLabs, ADL Canberra
ajd@linux.ibm.com             IBM Australia Limited

Re: RFC: using supersedes: trailer to indicate patch/series revision flow

[Han-Wen Nienhuys]

On Fri, Nov 8, 2019 at 12:45 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > 2. Should supersedes: link to the previous version of the patch, or the
> >     first ever version of the patch? I am leaning towards the latter,
>
> And then how do you know that version 2 was superseded by version 3?

You throw the message ID into a search engine, and see what it returns.

The advantage of keeping the patch series ID stable is that you can
consider a patchseries as a document and then easily index it inside a
service (say, patchwork) using Lucene, ElasticSearch or some other
common technology.

If you make the "supersedes" refer to specific versions, a workflow
service will be more susceptible to errors if messages were lost, and
the service has to work harder to aggregate the different versions of
a patchseries together.

Is it common for different authors to superseed each other's patch
series? If yes, "superseeds: precise version" is more precise, if not,
you get the same information from the timestamp of the cover letter.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Re: RFC: using supersedes: trailer to indicate patch/series revision flow

[Vegard Nossum]

On 11/7/19 9:43 PM, Konstantin Ryabitsev wrote:
> Hi, all:
> 
> The only mechanism we currently have for patch/series versioning is 
> subject suffixes. I think it would be useful to have a way to more 
> explicitly mark that a series obsoletes a previous version, and I 
> propose this is done with a `supersedes:` trailer at the end of the 
> cover letter or in the first patch of the series:

[...]

I think it's better to put it in the changelog itself (similar to S-O-B,
R-B, A-B, etc. lines) because it allows you to look the information up
straight from a commit once it has been merged.

This is what I suggested under "Step 3", second point here:

https://lore.kernel.org/workflows/b9fb52b8-8168-6bf0-9a72-1e6c44a281a5@oracle.com/

In the scheme I proposed, you would additionally refer to the previous
version by SHA1, which means diffing the patchsets would be as easy as
typing 'git diff' and then pasting the two SHA1s (which would both be
right there in front of you -- no need to follow lore links and applying
patches by hand to compare them).

This is just one data point, but in the team I work in this would be
incredibly useful to have -- we frequently look at upstream/merged
commits and sometimes want to compare them to mailing list submissions
and running git diff/log/range-diff is FAR easier than searching
mailing lists and applying series by hand or trying to figure out by
eyeballing email patches.

> Questions:
> 
[...]
> 2. Should supersedes: link to the previous version of the patch, or the 
>     first ever version of the patch? I am leaning towards the latter,    
> even though in this case the message-id largely becomes identical in    
> usage to Gerrit's Change-Id.

If you add the info to each patch, then it makes sense to link to the
previous version of that patch, since you can always find the first/last
patch from that.

If we also used the merge trick I described to refer to a whole patchset
with a single SHA1 then you can additionally link to the previous
version of that patchset from the merge commit, meaning you effectively
get both (one link in each patch to the previous version of _that_
patch, plus one link in the final merge commit to the previous version
of the full patchset).


Vegard

Re: RFC: using supersedes: trailer to indicate patch/series revision flow

[Geert Uytterhoeven]

Hi Konstantin,

On Thu, Nov 7, 2019 at 9:44 PM Konstantin Ryabitsev
<konstantin@linuxfoundation.org> wrote:
> The only mechanism we currently have for patch/series versioning is
> subject suffixes. I think it would be useful to have a way to more
> explicitly mark that a series obsoletes a previous version, and I
> propose this is done with a `supersedes:` trailer at the end of the
> cover letter or in the first patch of the series:

In theory, this sounds like a good idea to me.
Whether it's practical to implement it, is to be seen...

> Questions:
>
> 1. Should this be exposed via git-format-patch flags, or just used by
>     specialized tooling?

While I switched to always applying bundles from patchwork to have the
Link: added automatically, I'm not aware of a simple way to add a
superseded header automatically.

> 2. Should supersedes: link to the previous version of the patch, or the
>     first ever version of the patch? I am leaning towards the latter,
>     even though in this case the message-id largely becomes identical in
>     usage to Gerrit's Change-Id.

The previous version, as that's the one being superseded, and doing it
this way allows to iterate backwards through history.
The first ever version has been superseded already by the previous
version (if any).

> 3. Should the supersedes trailer have:
>     a. message-id without brackets
>     b. message-id with brackets
>     c. https://lore.kernel.org/r/message-id
>     My preference is b, to match with The Message-Id header usage.

I think c makes most sense, as it allows to follow the Link: in a commit, and
iterate backwards using the same method.
Else we have to convert manually from message-id to lore link.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: RFC: using supersedes: trailer to indicate patch/series revision flow

[Geert Uytterhoeven]

Hi Han-Wen,

On Fri, Nov 8, 2019 at 9:31 AM Han-Wen Nienhuys <hanwen@google.com> wrote:
> On Fri, Nov 8, 2019 at 12:45 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > 2. Should supersedes: link to the previous version of the patch, or the
> > >     first ever version of the patch? I am leaning towards the latter,
> >
> > And then how do you know that version 2 was superseded by version 3?
>
> You throw the message ID into a search engine, and see what it returns.
>
> The advantage of keeping the patch series ID stable is that you can
> consider a patchseries as a document and then easily index it inside a
> service (say, patchwork) using Lucene, ElasticSearch or some other
> common technology.
>
> If you make the "supersedes" refer to specific versions, a workflow
> service will be more susceptible to errors if messages were lost, and
> the service has to work harder to aggregate the different versions of
> a patchseries together.
>
> Is it common for different authors to superseed each other's patch
> series? If yes, "superseeds: precise version" is more precise, if not,
> you get the same information from the timestamp of the cover letter.

All/most of the above applies only to versioning of patch series that
implement a fixed feature, with a fixed scope.
So Vn+1 is really an improved version of Vn, and nothing more or less.

But this does not apply to many patch series in Linux kernel development.
Patch series may
  - grow (more patches added),
  - shrink (some patches rejected, or already applied independently),
  - be split in multiple series,
  - dropped but some parts reused in other series (possibly by someone
    else),
  - ...
That's the hard work in patch tracking.

So series IDs do not cope well with this, and "superseded" really should
apply to individual patches, not series, too.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: RFC: using supersedes: trailer to indicate patch/series revision flow

[Laurent Pinchart]

Hi Geert,

On Fri, Nov 08, 2019 at 10:59:55AM +0100, Geert Uytterhoeven wrote:
> On Fri, Nov 8, 2019 at 9:31 AM Han-Wen Nienhuys <hanwen@google.com> wrote:
> > On Fri, Nov 8, 2019 at 12:45 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > 2. Should supersedes: link to the previous version of the patch, or the
> > > >     first ever version of the patch? I am leaning towards the latter,
> > >
> > > And then how do you know that version 2 was superseded by version 3?
> >
> > You throw the message ID into a search engine, and see what it returns.
> >
> > The advantage of keeping the patch series ID stable is that you can
> > consider a patchseries as a document and then easily index it inside a
> > service (say, patchwork) using Lucene, ElasticSearch or some other
> > common technology.
> >
> > If you make the "supersedes" refer to specific versions, a workflow
> > service will be more susceptible to errors if messages were lost, and
> > the service has to work harder to aggregate the different versions of
> > a patchseries together.
> >
> > Is it common for different authors to superseed each other's patch
> > series? If yes, "superseeds: precise version" is more precise, if not,
> > you get the same information from the timestamp of the cover letter.
> 
> All/most of the above applies only to versioning of patch series that
> implement a fixed feature, with a fixed scope.
> So Vn+1 is really an improved version of Vn, and nothing more or less.
> 
> But this does not apply to many patch series in Linux kernel development.
> Patch series may
>   - grow (more patches added),

This shouldn't be a problem, as the new patches get posted for the first
time, and thus don't supersede anything else than what the previous
version of the series contained.

>   - shrink (some patches rejected, or already applied independently),

This should also not be an issue, as the patches that are dropped or
that are already applied will not be resubmitted separately.

>   - be split in multiple series,
>   - dropped but some parts reused in other series (possibly by someone
>     else),
>   - ...

These are the difficult cases :-(

> That's the hard work in patch tracking.
> 
> So series IDs do not cope well with this, and "superseded" really should
> apply to individual patches, not series, too.

-- 
Regards,

Laurent Pinchart

Re: RFC: using supersedes: trailer to indicate patch/series revision flow

[Rafael J. Wysocki]

On Friday, November 8, 2019 10:59:55 AM CET Geert Uytterhoeven wrote:
> Hi Han-Wen,
> 
> On Fri, Nov 8, 2019 at 9:31 AM Han-Wen Nienhuys <hanwen@google.com> wrote:
> > On Fri, Nov 8, 2019 at 12:45 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > 2. Should supersedes: link to the previous version of the patch, or the
> > > >     first ever version of the patch? I am leaning towards the latter,
> > >
> > > And then how do you know that version 2 was superseded by version 3?
> >
> > You throw the message ID into a search engine, and see what it returns.
> >
> > The advantage of keeping the patch series ID stable is that you can
> > consider a patchseries as a document and then easily index it inside a
> > service (say, patchwork) using Lucene, ElasticSearch or some other
> > common technology.
> >
> > If you make the "supersedes" refer to specific versions, a workflow
> > service will be more susceptible to errors if messages were lost, and
> > the service has to work harder to aggregate the different versions of
> > a patchseries together.
> >
> > Is it common for different authors to superseed each other's patch
> > series? If yes, "superseeds: precise version" is more precise, if not,
> > you get the same information from the timestamp of the cover letter.
> 
> All/most of the above applies only to versioning of patch series that
> implement a fixed feature, with a fixed scope.
> So Vn+1 is really an improved version of Vn, and nothing more or less.
> 
> But this does not apply to many patch series in Linux kernel development.
> Patch series may
>   - grow (more patches added),
>   - shrink (some patches rejected, or already applied independently),
>   - be split in multiple series,
>   - dropped but some parts reused in other series (possibly by someone
>     else),
>   - ...
> That's the hard work in patch tracking.
> 
> So series IDs do not cope well with this, and "superseded" really should
> apply to individual patches, not series, too.

Right.

Re: RFC: using supersedes: trailer to indicate patch/series revision flow

[Eric Wong]

Konstantin Ryabitsev <konstantin@linuxfoundation.org> wrote:
> Hi, all:
> 
> The only mechanism we currently have for patch/series versioning is
> subject suffixes. I think it would be useful to have a way to more
> explicitly mark that a series obsoletes a previous version, and I
> propose this is done with a `supersedes:` trailer at the end of the
> cover letter or in the first patch of the series:

<snip>

> Subject: [PATCH,v2] Change foo
> 
> Foo is no good. Use Bar. Also use baz.
> 
> Signed-off-by: Dev Eloper <dev.eloper@example.com>
> ---
>   foo | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/foo b/foo
> ...
> 
> supersedes: <1572991351-86061-1-git-send-email-dev.eloper@example.com>

This would works badly with limited bandwidth and storage
situations.

--interdiff and --range-diff options already give plenty of
useful info inline; and --range-diff can be enhanced to work
better in scenarios where there's enough storage/bandwidth for a
powerful search engine (--interdiff already does):

  Here's a quick PoC which allows --range-diff to be more useful
  to searcn engines (but the current regression is it's too
  noisy for human eyes, and making it less so requires more work):
  https://public-inbox.org/git/20191017121045.GA15364@dcvr/
  ("range-diff: show old/new blob OIDs in comments")

And search engine tooling still needs to be built around search
for better interdiff and range-diff blob expansion (since it
already exists for regular patches).