b4: use new trailers in 'b4 send --resend'

b4: use new trailers in 'b4 send --resend'

[Luca Ceresoli]

Hello,

first of all: thanks for b4, I love it when sending my patches!

I found a little issue when resending a patch series with unmodified
content, but with new trailers received after the initial sending.

This are the events involved:

 1. send a patch series (b4 send)
 2. receive some Reviewed-by, but patch not applied and no changes
    needed
 3. update trailers (b4 trailers -u)
 4. resend (b4 send --resend)

At step 4 I would expect b4 to prepare a [PATCH RESEND] series with
the new trailers added. Instead b4 resends the exact same series,
without any new trailers.

According to my experience in contributing to the Linux kernel, having
the trailers added when resending is the primary goal when resending a
series that needs no other changes, so I think it should be done by b4
automatically.

Best regards,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: b4: use new trailers in 'b4 send --resend'

[Konstantin Ryabitsev]

On Fri, Apr 26, 2024 at 02:01:57PM GMT, Luca Ceresoli wrote:
> first of all: thanks for b4, I love it when sending my patches!
> 
> I found a little issue when resending a patch series with unmodified
> content, but with new trailers received after the initial sending.
> 
> This are the events involved:
> 
>  1. send a patch series (b4 send)
>  2. receive some Reviewed-by, but patch not applied and no changes
>     needed
>  3. update trailers (b4 trailers -u)
>  4. resend (b4 send --resend)
> 
> At step 4 I would expect b4 to prepare a [PATCH RESEND] series with
> the new trailers added. Instead b4 resends the exact same series,
> without any new trailers.

This is the expected behaviour, as far as I know. There should be no 
differences between the series, otherwise it's not really a resend.

It's not really necessary to resend a series just for trailer changes 
anyway. The maintainer will most likely use b4 to retrieve it anyway, 
and that will pull in any new trailers.

-K

Re: b4: use new trailers in 'b4 send --resend'

[Luca Ceresoli]

Hello Konstantin,

thanks for your quick feedback.

On Fri, 26 Apr 2024 11:11:29 -0400
Konstantin Ryabitsev <konstantin@linuxfoundation.org> wrote:

> On Fri, Apr 26, 2024 at 02:01:57PM GMT, Luca Ceresoli wrote:
> > first of all: thanks for b4, I love it when sending my patches!
> > 
> > I found a little issue when resending a patch series with unmodified
> > content, but with new trailers received after the initial sending.
> > 
> > This are the events involved:
> > 
> >  1. send a patch series (b4 send)
> >  2. receive some Reviewed-by, but patch not applied and no changes
> >     needed
> >  3. update trailers (b4 trailers -u)
> >  4. resend (b4 send --resend)
> > 
> > At step 4 I would expect b4 to prepare a [PATCH RESEND] series with
> > the new trailers added. Instead b4 resends the exact same series,
> > without any new trailers.  
> 
> This is the expected behaviour, as far as I know. There should be no 
> differences between the series, otherwise it's not really a resend.
> 
> It's not really necessary to resend a series just for trailer changes 
> anyway. The maintainer will most likely use b4 to retrieve it anyway, 
> and that will pull in any new trailers.

I cannot recall exactly when it did happen and provide a link right now,
but I clearly remember having been asked by some maintainer to do
exactly this, instead of pinging.

The rationale is that sending a new version just to add review tags was
considered bad because one can expect the patches or commit description
to have changed somewhat, which it hasn't.

OTOH resending without new trailers can lead to applying a patch
without those tags (not recognizing reviewers time), or even worse to
discard a patch because it does not show the needed tags.

It all made sense to me. Does it look reasonable to you too, with this
extra explanation?

Best regards,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: b4: use new trailers in 'b4 send --resend'

[Konstantin Ryabitsev]

On Fri, Apr 26, 2024 at 06:44:25PM GMT, Luca Ceresoli wrote:
> > It's not really necessary to resend a series just for trailer 
> > changes anyway. The maintainer will most likely use b4 to retrieve 
> > it anyway, and that will pull in any new trailers.
> 
> I cannot recall exactly when it did happen and provide a link right now,
> but I clearly remember having been asked by some maintainer to do
> exactly this, instead of pinging.
> 
> The rationale is that sending a new version just to add review tags was
> considered bad because one can expect the patches or commit description
> to have changed somewhat, which it hasn't.

Yes, to be clear -- I wasn't suggesting that you send a new revision 
just to add trailers either. We're both aligned here.

> OTOH resending without new trailers can lead to applying a patch
> without those tags (not recognizing reviewers time), or even worse to
> discard a patch because it does not show the needed tags.

I expect this is one of those "every subsystem does things slightly 
differently" situation. If the maintainer uses b4, this entire step is 
unnecessary, because any missing trailers will be applied anyway when 
they run "b4 am". If they don't use b4 -- or if they use a very old 
version of b4 which didn't match by patch-id to collect trailers sent to 
other identical submissions -- then they would have a reason to 
complain.

I may have to run a poll among maintainers to see which is the behaviour 
they want to see:

- resend should be identical to what was sent the first time
- resend should include any additional trailers received since the 
  initial send-out

-K

Re: b4: use new trailers in 'b4 send --resend'

[Luca Ceresoli]

Hello Konstantin,

On Fri, 26 Apr 2024 14:04:49 -0400
Konstantin Ryabitsev <konstantin@linuxfoundation.org> wrote:

> On Fri, Apr 26, 2024 at 06:44:25PM GMT, Luca Ceresoli wrote:
> > > It's not really necessary to resend a series just for trailer 
> > > changes anyway. The maintainer will most likely use b4 to retrieve 
> > > it anyway, and that will pull in any new trailers.  
> > 
> > I cannot recall exactly when it did happen and provide a link right now,
> > but I clearly remember having been asked by some maintainer to do
> > exactly this, instead of pinging.
> > 
> > The rationale is that sending a new version just to add review tags was
> > considered bad because one can expect the patches or commit description
> > to have changed somewhat, which it hasn't.  
> 
> Yes, to be clear -- I wasn't suggesting that you send a new revision 
> just to add trailers either. We're both aligned here.
> 
> > OTOH resending without new trailers can lead to applying a patch
> > without those tags (not recognizing reviewers time), or even worse to
> > discard a patch because it does not show the needed tags.  
> 
> I expect this is one of those "every subsystem does things slightly 
> differently" situation. If the maintainer uses b4, this entire step is 
> unnecessary, because any missing trailers will be applied anyway when 
> they run "b4 am".

Ah, interesting feature. I had no idea about it as I'm using b4 for
sending patches only. Thanks for the clarification!

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: b4: use new trailers in 'b4 send --resend'

[Mark Brown]

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

On Fri, Apr 26, 2024 at 02:04:49PM -0400, Konstantin Ryabitsev wrote:
> On Fri, Apr 26, 2024 at 06:44:25PM GMT, Luca Ceresoli wrote:

> > > It's not really necessary to resend a series just for trailer 
> > > changes anyway. The maintainer will most likely use b4 to retrieve 
> > > it anyway, and that will pull in any new trailers.

> > I cannot recall exactly when it did happen and provide a link right now,
> > but I clearly remember having been asked by some maintainer to do
> > exactly this, instead of pinging.

> Yes, to be clear -- I wasn't suggesting that you send a new revision 
> just to add trailers either. We're both aligned here.

This sounds like something I routinely say, though not in order to
collect trailers but rather because if something went AWOL the basic
answer to a ping is going to be a "I have no idea what you're talking
about" so it's just easier to send the thing again combined with a
general desire to discourage people from sending content free pings.

> I may have to run a poll among maintainers to see which is the behaviour 
> they want to see:

> - resend should be identical to what was sent the first time
> - resend should include any additional trailers received since the 
>   initial send-out

One possible issue with not having the trailers in a new thread would be
that you might not get as far as fetching the patches with b4 if it
looked like they'd not got enough review.  There are also people who get
really annoyed when they see something that looks like a submitter is
dropping their acks or reviews needlessly, among other things it does
mean people can end up repeating review they've already done.

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