Address from trailer on individual patch is sent entire series?

Address from trailer on individual patch is sent entire series?

[Nathan Chancellor]

Hi Konstantin,

I was going over my recent "sent with b4" series and I noticed that the
kernel test robot, a Reported-by: on a specific patch in the series, was
Cc'd on the entire series.

Patch with trailer:

https://lore.kernel.org/linux-kbuild/20221228-drop-qunused-arguments-v1-2-658cbc8fc592@kernel.org/

Patch without trailer still has Cc:

https://lore.kernel.org/linux-kbuild/20221228-drop-qunused-arguments-v1-5-658cbc8fc592@kernel.org/

I would have expected this to behave like explicitly provided patch Cc's
so that the kernel test robot would have just received that patch and
the cover letter? You can see the raw series from tree on kernel.org, as
I pushed the v1 tag that 'b4 send' generated:

https://git.kernel.org/pub/scm/linux/kernel/git/nathan/linux.git/tag/?h=sent/drop-qunused-arguments-v1

in case you need to see anything there. If I have missed some
configuration option or other problem, please let me know!

Cheers,
Nathan

Re: Address from trailer on individual patch is sent entire series?

[Konstantin Ryabitsev]

On Thu, Jan 05, 2023 at 06:27:09PM -0700, Nathan Chancellor wrote:
> Hi Konstantin,
> 
> I was going over my recent "sent with b4" series and I noticed that the
> kernel test robot, a Reported-by: on a specific patch in the series, was
> Cc'd on the entire series.

This is one of those time when people's expectations diverge and either way
you do it, someone will find that it's not how they want it to work.

Originally, my polling suggested that everyone always wanted to receive the
entire patch series, even if they were only responsible for a single patch.
This was later changed for in-patch "Cc:" trailers, because there were actual
CI workflows based on that and sending them the entire series was causing more
harm than good.

However, every other trailer will still receive the entire series, because I
believe it's still more correct than just sending them a single patch. If
that's not the actual consensus and we decide that all other in-patch trailers
should be handled the same as Cc: (they get the cover letter plus only the
individual patches where they are cc'd), I can certainly implement that as
well.

Regards,
Konstantin

Re: Address from trailer on individual patch is sent entire series?

[Nathan Chancellor]

On Fri, Jan 06, 2023 at 11:34:42AM -0500, Konstantin Ryabitsev wrote:
> On Thu, Jan 05, 2023 at 06:27:09PM -0700, Nathan Chancellor wrote:
> > Hi Konstantin,
> > 
> > I was going over my recent "sent with b4" series and I noticed that the
> > kernel test robot, a Reported-by: on a specific patch in the series, was
> > Cc'd on the entire series.
> 
> This is one of those time when people's expectations diverge and either way
> you do it, someone will find that it's not how they want it to work.
> 
> Originally, my polling suggested that everyone always wanted to receive the
> entire patch series, even if they were only responsible for a single patch.
> This was later changed for in-patch "Cc:" trailers, because there were actual
> CI workflows based on that and sending them the entire series was causing more
> harm than good.
> 
> However, every other trailer will still receive the entire series, because I
> believe it's still more correct than just sending them a single patch.

Understood, thanks for taking the time to explain your rationale!

I am sure you are aware this contradicts what git send-email does, which
is obviously fine in and of itself (I understand b4 is not a git
send-email clone), but I am not sure this difference will be for the
better.

For example, take the series I just mentioned in my previous mail. There
are only three patches that are relevant to the s390 folks, so
previously I Cc'd them on just those three patches along with the cover
letter so that is what clear what I was expecting (tags for those three
patches so they could be carried via another tree). I promptly got a
review and ack from Sven and Heiko respectively, which I added to the
relevant patches in my local tree.

If I have to send a v2, Heiko and Sven are going to get Cc'd on the
entire series, which is now entirely irrelevant to them, versus getting
just those few patches, which they can recognized as ones that they have
already dealt with. I suppose I am going to get bit by this more often
than others will because I work all over the tree so perhaps it is not
that big of a concern in practice.

> If that's not the actual consensus and we decide that all other
> in-patch trailers should be handled the same as Cc: (they get the
> cover letter plus only the individual patches where they are cc'd), I
> can certainly implement that as well.

It would be good to poll others to see what they think, as I could be
way off base here.

Cheers,
Nathan

Re: Address from trailer on individual patch is sent entire series?

[Konstantin Ryabitsev]

On Fri, Jan 06, 2023 at 09:54:31AM -0700, Nathan Chancellor wrote:
> I am sure you are aware this contradicts what git send-email does, which
> is obviously fine in and of itself (I understand b4 is not a git
> send-email clone), but I am not sure this difference will be for the
> better.

Yes, thinking about this some more, it's probably best if we emulate the
most-expected behaviour of "what would git-send-email do". This still does
both, in a sense:

"b4 prep --auto-to-cc" collects addresses as returned by get_maintainers.pl
and will send these people/addresses the entire series. However, it will no
longer add addresses from trailers found in individual commits.

"b4 send" will send patches to any to/cc addresses in the cover letter, plus
any addresses mentioned in individual commits. The cover letter will still go
out to everyone, so an address from Reported-by in [PATCH 4/15] will get two
messages: [PATCH 0/15], [PATCH 4/15].

Will that be closer to your expectations?

-K

Re: Address from trailer on individual patch is sent entire series?

[Nathan Chancellor]

On Fri, Jan 06, 2023 at 02:20:18PM -0500, Konstantin Ryabitsev wrote:
> On Fri, Jan 06, 2023 at 09:54:31AM -0700, Nathan Chancellor wrote:
> > I am sure you are aware this contradicts what git send-email does, which
> > is obviously fine in and of itself (I understand b4 is not a git
> > send-email clone), but I am not sure this difference will be for the
> > better.
> 
> Yes, thinking about this some more, it's probably best if we emulate the
> most-expected behaviour of "what would git-send-email do". This still does
> both, in a sense:
> 
> "b4 prep --auto-to-cc" collects addresses as returned by get_maintainers.pl
> and will send these people/addresses the entire series. However, it will no
> longer add addresses from trailers found in individual commits.
> 
> "b4 send" will send patches to any to/cc addresses in the cover letter, plus
> any addresses mentioned in individual commits. The cover letter will still go
> out to everyone, so an address from Reported-by in [PATCH 4/15] will get two
> messages: [PATCH 0/15], [PATCH 4/15].
> 
> Will that be closer to your expectations?

Yes, I think the 'send' change sounds completely reasonable!

I do not use 'b4 prep --auto-to-cc' since I already have my own
'get_maintainer.pl' workflow so I cannot really comment on how much of a
change that suggestion would be. I could imagine a Suggested-by: from a
maintainer on one patch in a series is not uncommon, which would mean
their Cc: would not be added to the cover letter with your suggested
change, meaning they might miss the whole series. That would not be good
but I am not sure if that is just paranoia or a real problem.

Cheers,
Nathan

Re: Address from trailer on individual patch is sent entire series?

[Konstantin Ryabitsev]

On Fri, Jan 06, 2023 at 12:43:59PM -0700, Nathan Chancellor wrote:
> Yes, I think the 'send' change sounds completely reasonable!
> 
> I do not use 'b4 prep --auto-to-cc' since I already have my own
> 'get_maintainer.pl' workflow so I cannot really comment on how much of a
> change that suggestion would be. I could imagine a Suggested-by: from a
> maintainer on one patch in a series is not uncommon, which would mean
> their Cc: would not be added to the cover letter with your suggested
> change, meaning they might miss the whole series. That would not be good
> but I am not sure if that is just paranoia or a real problem.

Okay, this change is in master. I'll probably backport it to 0.11.y based on
testing feedback. Please give it a whirl.

-K

Re: Address from trailer on individual patch is sent entire series?

[Nathan Chancellor]

On Fri, Jan 06, 2023 at 07:12:54PM -0500, Konstantin Ryabitsev wrote:
> On Fri, Jan 06, 2023 at 12:43:59PM -0700, Nathan Chancellor wrote:
> > Yes, I think the 'send' change sounds completely reasonable!
> > 
> > I do not use 'b4 prep --auto-to-cc' since I already have my own
> > 'get_maintainer.pl' workflow so I cannot really comment on how much of a
> > change that suggestion would be. I could imagine a Suggested-by: from a
> > maintainer on one patch in a series is not uncommon, which would mean
> > their Cc: would not be added to the cover letter with your suggested
> > change, meaning they might miss the whole series. That would not be good
> > but I am not sure if that is just paranoia or a real problem.
> 
> Okay, this change is in master. I'll probably backport it to 0.11.y based on
> testing feedback. Please give it a whirl.

Thank you so much for the quick turnaround! This appears to work how I
would expect it to, based on both 'b4 send -d' and 'b4 send --reflect'.

Cheers,
Nathan