Re: Please return trailer-order option

Re: Please return trailer-order option

[Konstantin Ryabitsev]

On Thu, Jul 28, 2022 at 09:42:10PM +0300, Leon Romanovsky wrote:
> >> Do you think that it is acceptable to return that option?
> >
> > Yes, it will probably come back as "trailer-priority" and preserve the
> > chain-of-custody ordering as discussed on the list. It should make it back in
> > by 0.10 release.
> 
> Sounds awesome.

FYI, this option is back as of today. It's still called "trailer-order", so
you shouldn't need to change anything. There are some important changes in how
it works:

1. we only reorder trailers that are in your own chain-of-custody section
   (below the last Signed-off-by that isn't yours)
2. we always move your own Signed-off-by to the bottom of your own
   chain-of-custody section

Please let me know if this new behaviour is working as you would expect it.

-K

Re: Please return trailer-order option

[Leon Romanovsky]

On Thu, Sep 01, 2022 at 05:40:47PM -0400, Konstantin Ryabitsev wrote:
> On Thu, Jul 28, 2022 at 09:42:10PM +0300, Leon Romanovsky wrote:
> > >> Do you think that it is acceptable to return that option?
> > >
> > > Yes, it will probably come back as "trailer-priority" and preserve the
> > > chain-of-custody ordering as discussed on the list. It should make it back in
> > > by 0.10 release.
> > 
> > Sounds awesome.
> 
> FYI, this option is back as of today. It's still called "trailer-order", so
> you shouldn't need to change anything. There are some important changes in how
> it works:
> 
> 1. we only reorder trailers that are in your own chain-of-custody section
>    (below the last Signed-off-by that isn't yours)
> 2. we always move your own Signed-off-by to the bottom of your own
>    chain-of-custody section
> 
> Please let me know if this new behaviour is working as you would expect it.

Thank you for doing it.

I tried latest b4 with the following config:
[b4]
         trailer-order = "Cc,Fixes*,Link*,Suggested*,Reviewed*,Tested*,*"

➜  kernel git:(tmp) ~/src/b4/b4.sh shazam -l -s https://lore.kernel.org/all/20220901202645.1463552-1-irogers@google.com
Grabbing thread from lore.kernel.org/all/20220901202645.1463552-1-irogers%40google.com/t.mbox.gz
Checking for newer revisions on https://lore.kernel.org/all/
Analyzing 2 messages in the thread
Checking attestation on all messages, may take a moment...
---
  [PATCH v1] selftests/xsk: Avoid use-after-free on ctx
    + Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
    + Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
  ---
  NOTE: install dkimpy for DKIM signature verification
---
Total patches: 1
---
Applying: selftests/xsk: Avoid use-after-free on ctx

The end result as an outcome of 1 is:
    Fixes: 39e940d4abfa ("selftests/xsk: Destroy BPF resources only when ctx refcount drops to 0")
    Signed-off-by: Ian Rogers <irogers@google.com>
    Link: https://lore.kernel.org/r/20220901202645.1463552-1-irogers@google.com
    Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
    Signed-off-by: Leon Romanovsky <leonro@nvidia.com>

While I would like to see Link being below Fixes line:
    Fixes: 39e940d4abfa ("selftests/xsk: Destroy BPF resources only when ctx refcount drops to 0")
    Link: https://lore.kernel.org/r/20220901202645.1463552-1-irogers@google.com
    Signed-off-by: Ian Rogers <irogers@google.com>
    Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
    Signed-off-by: Leon Romanovsky <leonro@nvidia.com>

Thanks

> 
> -K

Re: Please return trailer-order option

[Konstantin Ryabitsev]

On Fri, Sep 02, 2022 at 01:25:36PM +0300, Leon Romanovsky wrote:
> I tried latest b4 with the following config:
> [b4]
>          trailer-order = "Cc,Fixes*,Link*,Suggested*,Reviewed*,Tested*,*"
> 
> ➜  kernel git:(tmp) ~/src/b4/b4.sh shazam -l -s https://lore.kernel.org/all/20220901202645.1463552-1-irogers@google.com
> Grabbing thread from lore.kernel.org/all/20220901202645.1463552-1-irogers%40google.com/t.mbox.gz
> Checking for newer revisions on https://lore.kernel.org/all/
> Analyzing 2 messages in the thread
> Checking attestation on all messages, may take a moment...
> ---
>   [PATCH v1] selftests/xsk: Avoid use-after-free on ctx
>     + Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
>     + Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>   ---
>   NOTE: install dkimpy for DKIM signature verification
> ---
> Total patches: 1
> ---
> Applying: selftests/xsk: Avoid use-after-free on ctx
> 
> The end result as an outcome of 1 is:
>     Fixes: 39e940d4abfa ("selftests/xsk: Destroy BPF resources only when ctx refcount drops to 0")
>     Signed-off-by: Ian Rogers <irogers@google.com>
>     Link: https://lore.kernel.org/r/20220901202645.1463552-1-irogers@google.com
>     Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
>     Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> 
> While I would like to see Link being below Fixes line:
>     Fixes: 39e940d4abfa ("selftests/xsk: Destroy BPF resources only when ctx refcount drops to 0")
>     Link: https://lore.kernel.org/r/20220901202645.1463552-1-irogers@google.com
>     Signed-off-by: Ian Rogers <irogers@google.com>
>     Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
>     Signed-off-by: Leon Romanovsky <leonro@nvidia.com>

Right, this is the intended change in behaviour -- we don't want to touch
anything that's above the Signed-off-by that doesn't belong to you. I see why
you want to move the Link: line above Ian's SoB (to indicate that it's his
message), but I think from the point of view of chain-of-custody it really
belongs under it:

    Fixes: 39e940d4abfa ("selftests/xsk: Destroy BPF resources only when ctx refcount drops to 0")
    Signed-off-by: Ian Rogers <irogers@google.com>

This is Ian saying that he's signing off on anything above this trailer. If we
move Link: above his signoff, this would indicate that he's also signing off
on the contents of that URL, but that would be putting words in his mouth. If
we put the Link under it, though, this indicates where *you* got the message, so
it really belongs in your own custody section:

    Link: https://lore.kernel.org/r/20220901202645.1463552-1-irogers@google.com

"I got this message at this URL"

    Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>

"I also collected this Acked-by, which can also be found at that URL"

    Signed-off-by: Leon Romanovsky <leonro@nvidia.com>

"I am signing off on anything above this line"

Hope this helps explain the reasoning.

-Konstantin

Re: Please return trailer-order option

[Leon Romanovsky]

On Fri, Sep 02, 2022 at 10:40:33AM -0400, Konstantin Ryabitsev wrote:
> On Fri, Sep 02, 2022 at 01:25:36PM +0300, Leon Romanovsky wrote:
> > I tried latest b4 with the following config:
> > [b4]
> >          trailer-order = "Cc,Fixes*,Link*,Suggested*,Reviewed*,Tested*,*"
> > 
> > ➜  kernel git:(tmp) ~/src/b4/b4.sh shazam -l -s https://lore.kernel.org/all/20220901202645.1463552-1-irogers@google.com
> > Grabbing thread from lore.kernel.org/all/20220901202645.1463552-1-irogers%40google.com/t.mbox.gz
> > Checking for newer revisions on https://lore.kernel.org/all/
> > Analyzing 2 messages in the thread
> > Checking attestation on all messages, may take a moment...
> > ---
> >   [PATCH v1] selftests/xsk: Avoid use-after-free on ctx
> >     + Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
> >     + Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> >   ---
> >   NOTE: install dkimpy for DKIM signature verification
> > ---
> > Total patches: 1
> > ---
> > Applying: selftests/xsk: Avoid use-after-free on ctx
> > 
> > The end result as an outcome of 1 is:
> >     Fixes: 39e940d4abfa ("selftests/xsk: Destroy BPF resources only when ctx refcount drops to 0")
> >     Signed-off-by: Ian Rogers <irogers@google.com>
> >     Link: https://lore.kernel.org/r/20220901202645.1463552-1-irogers@google.com
> >     Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
> >     Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > 
> > While I would like to see Link being below Fixes line:
> >     Fixes: 39e940d4abfa ("selftests/xsk: Destroy BPF resources only when ctx refcount drops to 0")
> >     Link: https://lore.kernel.org/r/20220901202645.1463552-1-irogers@google.com
> >     Signed-off-by: Ian Rogers <irogers@google.com>
> >     Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
> >     Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> 
> Right, this is the intended change in behaviour -- we don't want to touch
> anything that's above the Signed-off-by that doesn't belong to you. I see why
> you want to move the Link: line above Ian's SoB (to indicate that it's his
> message), but I think from the point of view of chain-of-custody it really
> belongs under it:
> 
>     Fixes: 39e940d4abfa ("selftests/xsk: Destroy BPF resources only when ctx refcount drops to 0")
>     Signed-off-by: Ian Rogers <irogers@google.com>
> 
> This is Ian saying that he's signing off on anything above this trailer. If we
> move Link: above his signoff, this would indicate that he's also signing off
> on the contents of that URL, but that would be putting words in his mouth. If
> we put the Link under it, though, this indicates where *you* got the message, so
> it really belongs in your own custody section:
> 
>     Link: https://lore.kernel.org/r/20220901202645.1463552-1-irogers@google.com
> 
> "I got this message at this URL"
> 
>     Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
> 
> "I also collected this Acked-by, which can also be found at that URL"
> 
>     Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> 
> "I am signing off on anything above this line"
> 
> Hope this helps explain the reasoning.

Yeah, it explains, the thing is that pure chain-of-custody exists only in
flows that mostly based on pull request.

In my subsystem case, we fix many of submitted patches (spelling,
grammar, coding style and sometimes code itself), because for us the end
goal (having fix/patch merged) is more important than process. We don't want
to scare casual developers with strict rules, of course, the more experienced
submitter, the less tolerant toward his mistakes.

So in my mind, commit message structure is: "Subject, message, Cc, Fixes,
Link, various *-off-by".

Thanks

> 
> -Konstantin