[PATCH v1 1/1] docs: process: submitting-patches: Clarify the Reported-by usage

[PATCH v1 1/1] docs: process: submitting-patches: Clarify the Reported-by usage

[Andy Shevchenko]

It's unclear from "Submitting Patches" documentation that Reported-by
is not supposed to be used against new features. (It's more clear
in the section 5.4 "Patch formatting and changelogs" of the "A guide
to the Kernel Development Process", where it suggests that change
should fix something existing in the kernel. Clarify the Reported-by
usage in the "Submitting Patches".

Reported-by: Florian Eckert <fe@dev.tdt.de>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 Documentation/process/submitting-patches.rst | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
index 31ea120ce531..24c1a5565385 100644
--- a/Documentation/process/submitting-patches.rst
+++ b/Documentation/process/submitting-patches.rst
@@ -495,7 +495,8 @@ Using Reported-by:, Tested-by:, Reviewed-by:, Suggested-by: and Fixes:
 The Reported-by tag gives credit to people who find bugs and report them and it
 hopefully inspires them to help us again in the future.  Please note that if
 the bug was reported in private, then ask for permission first before using the
-Reported-by tag.
+Reported-by tag. A new feature can't be reported since there is no code in the
+kernel to fix.
 
 A Tested-by: tag indicates that the patch has been successfully tested (in
 some environment) by the person named.  This tag informs maintainers that
-- 
2.34.1

Re: [PATCH v1 1/1] docs: process: submitting-patches: Clarify the Reported-by usage

[Jonathan Corbet]

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> It's unclear from "Submitting Patches" documentation that Reported-by
> is not supposed to be used against new features. (It's more clear
> in the section 5.4 "Patch formatting and changelogs" of the "A guide
> to the Kernel Development Process", where it suggests that change
> should fix something existing in the kernel. Clarify the Reported-by
> usage in the "Submitting Patches".
>
> Reported-by: Florian Eckert <fe@dev.tdt.de>

You're sure this added documentation isn't a new feature that shouldn't
have a Reported-by? :)

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  Documentation/process/submitting-patches.rst | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
> index 31ea120ce531..24c1a5565385 100644
> --- a/Documentation/process/submitting-patches.rst
> +++ b/Documentation/process/submitting-patches.rst
> @@ -495,7 +495,8 @@ Using Reported-by:, Tested-by:, Reviewed-by:, Suggested-by: and Fixes:
>  The Reported-by tag gives credit to people who find bugs and report them and it
>  hopefully inspires them to help us again in the future.  Please note that if
>  the bug was reported in private, then ask for permission first before using the
> -Reported-by tag.
> +Reported-by tag. A new feature can't be reported since there is no code in the
> +kernel to fix.

How about instead something like "Reported-by is intended for bugs;
please do not use it to credit feature requests"?

(i.e. I want the shed in green :)

Thanks,

jon

Re: [PATCH v1 1/1] docs: process: submitting-patches: Clarify the Reported-by usage

[Andy Shevchenko]

On Thu, Jan 27, 2022 at 09:08:06AM -0700, Jonathan Corbet wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> 
> > It's unclear from "Submitting Patches" documentation that Reported-by
> > is not supposed to be used against new features. (It's more clear
> > in the section 5.4 "Patch formatting and changelogs" of the "A guide
> > to the Kernel Development Process", where it suggests that change
> > should fix something existing in the kernel. Clarify the Reported-by
> > usage in the "Submitting Patches".
> >
> > Reported-by: Florian Eckert <fe@dev.tdt.de>
> 
> You're sure this added documentation isn't a new feature that shouldn't
> have a Reported-by? :)
> 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  Documentation/process/submitting-patches.rst | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
> > index 31ea120ce531..24c1a5565385 100644
> > --- a/Documentation/process/submitting-patches.rst
> > +++ b/Documentation/process/submitting-patches.rst
> > @@ -495,7 +495,8 @@ Using Reported-by:, Tested-by:, Reviewed-by:, Suggested-by: and Fixes:
> >  The Reported-by tag gives credit to people who find bugs and report them and it
> >  hopefully inspires them to help us again in the future.  Please note that if
> >  the bug was reported in private, then ask for permission first before using the
> > -Reported-by tag.
> > +Reported-by tag. A new feature can't be reported since there is no code in the
> > +kernel to fix.
> 
> How about instead something like "Reported-by is intended for bugs;
> please do not use it to credit feature requests"?
> 
> (i.e. I want the shed in green :)

Sure, you are native speaker at the end of the day :-)


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] docs: process: submitting-patches: Clarify the Reported-by usage

[Alexander Dahl]

Hello,

Am Thu, Jan 27, 2022 at 09:08:06AM -0700 schrieb Jonathan Corbet:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> 
> > It's unclear from "Submitting Patches" documentation that Reported-by
> > is not supposed to be used against new features. (It's more clear
> > in the section 5.4 "Patch formatting and changelogs" of the "A guide
> > to the Kernel Development Process", where it suggests that change
> > should fix something existing in the kernel. Clarify the Reported-by
> > usage in the "Submitting Patches".
> >
> > Reported-by: Florian Eckert <fe@dev.tdt.de>
> 
> You're sure this added documentation isn't a new feature that shouldn't
> have a Reported-by? :)
> 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  Documentation/process/submitting-patches.rst | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
> > index 31ea120ce531..24c1a5565385 100644
> > --- a/Documentation/process/submitting-patches.rst
> > +++ b/Documentation/process/submitting-patches.rst
> > @@ -495,7 +495,8 @@ Using Reported-by:, Tested-by:, Reviewed-by:, Suggested-by: and Fixes:
> >  The Reported-by tag gives credit to people who find bugs and report them and it
> >  hopefully inspires them to help us again in the future.  Please note that if
> >  the bug was reported in private, then ask for permission first before using the
> > -Reported-by tag.
> > +Reported-by tag. A new feature can't be reported since there is no code in the
> > +kernel to fix.
> 
> How about instead something like "Reported-by is intended for bugs;
> please do not use it to credit feature requests"?

What should be used for feature requests then? Suggested-by? Would it
help to mention it here?

Greets
Alex

Re: [PATCH v1 1/1] docs: process: submitting-patches: Clarify the Reported-by usage

[Matthew Wilcox]

On Thu, Jan 27, 2022 at 09:08:06AM -0700, Jonathan Corbet wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> 
> > It's unclear from "Submitting Patches" documentation that Reported-by
> > is not supposed to be used against new features. (It's more clear
> > in the section 5.4 "Patch formatting and changelogs" of the "A guide
> > to the Kernel Development Process", where it suggests that change
> > should fix something existing in the kernel. Clarify the Reported-by
> > usage in the "Submitting Patches".
> >
> > Reported-by: Florian Eckert <fe@dev.tdt.de>
> 
> You're sure this added documentation isn't a new feature that shouldn't
> have a Reported-by? :)
> 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  Documentation/process/submitting-patches.rst | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
> > index 31ea120ce531..24c1a5565385 100644
> > --- a/Documentation/process/submitting-patches.rst
> > +++ b/Documentation/process/submitting-patches.rst
> > @@ -495,7 +495,8 @@ Using Reported-by:, Tested-by:, Reviewed-by:, Suggested-by: and Fixes:
> >  The Reported-by tag gives credit to people who find bugs and report them and it
> >  hopefully inspires them to help us again in the future.  Please note that if
> >  the bug was reported in private, then ask for permission first before using the
> > -Reported-by tag.
> > +Reported-by tag. A new feature can't be reported since there is no code in the
> > +kernel to fix.
> 
> How about instead something like "Reported-by is intended for bugs;
> please do not use it to credit feature requests"?

I think this misunderstands the problem that Andy is trying to fix.

The situation: I write a patch.  I post it for review.  A bot does
something and finds a bug (could be compile-error, could be boot
problem).  That bot sends a bug report with a suggestion to add
Reported-by:.  That suggestion is inappropriate because the bug never
made it upstream, so it looks like the bot reported the "problem"
that the patch "fixes".

It's not unique to "new feature" patches.  If I'm fixing a bug and
my fix also contains a bug spotted by a bot, adding Reported-by
makes it look like the bot spotted the original bug, rather than
spotting a bug in the fix.

The best thing to do in this case is nothing.  Do not credit the bot.
Maybe add a Checked-by:, but that would be a new trailer and I really
don't think we need a new kind of trailer to get wrong.

Re: [PATCH v1 1/1] docs: process: submitting-patches: Clarify the Reported-by usage

[Johan Hovold]

On Fri, Jan 28, 2022 at 01:44:20PM +0000, Matthew Wilcox wrote:
> On Thu, Jan 27, 2022 at 09:08:06AM -0700, Jonathan Corbet wrote:
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> > 
> > > It's unclear from "Submitting Patches" documentation that Reported-by
> > > is not supposed to be used against new features. (It's more clear
> > > in the section 5.4 "Patch formatting and changelogs" of the "A guide
> > > to the Kernel Development Process", where it suggests that change
> > > should fix something existing in the kernel. Clarify the Reported-by
> > > usage in the "Submitting Patches".

> > How about instead something like "Reported-by is intended for bugs;
> > please do not use it to credit feature requests"?
> 
> I think this misunderstands the problem that Andy is trying to fix.
> 
> The situation: I write a patch.  I post it for review.  A bot does
> something and finds a bug (could be compile-error, could be boot
> problem).  That bot sends a bug report with a suggestion to add
> Reported-by:.  That suggestion is inappropriate because the bug never
> made it upstream, so it looks like the bot reported the "problem"
> that the patch "fixes".
> 
> It's not unique to "new feature" patches.  If I'm fixing a bug and
> my fix also contains a bug spotted by a bot, adding Reported-by
> makes it look like the bot spotted the original bug, rather than
> spotting a bug in the fix.
> 
> The best thing to do in this case is nothing.  Do not credit the bot.
> Maybe add a Checked-by:, but that would be a new trailer and I really
> don't think we need a new kind of trailer to get wrong.

It seems like the only way to fix this is to fix the bots. Adding more
documentation is unlikely to help in this case.

Can't we file a bug to whoever is running the bots (Intel?) and ask them
to remove the suggestion to add a Reported-by when the bot is testing a
patch (as opposed to mainline or even -next)?

Johan

Re: [PATCH v1 1/1] docs: process: submitting-patches: Clarify the Reported-by usage

[Andy Shevchenko]

On Mon, Jan 31, 2022 at 04:18:30PM +0100, Johan Hovold wrote:
> On Fri, Jan 28, 2022 at 01:44:20PM +0000, Matthew Wilcox wrote:
> > On Thu, Jan 27, 2022 at 09:08:06AM -0700, Jonathan Corbet wrote:
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> > > 
> > > > It's unclear from "Submitting Patches" documentation that Reported-by
> > > > is not supposed to be used against new features. (It's more clear
> > > > in the section 5.4 "Patch formatting and changelogs" of the "A guide
> > > > to the Kernel Development Process", where it suggests that change
> > > > should fix something existing in the kernel. Clarify the Reported-by
> > > > usage in the "Submitting Patches".
> 
> > > How about instead something like "Reported-by is intended for bugs;
> > > please do not use it to credit feature requests"?
> > 
> > I think this misunderstands the problem that Andy is trying to fix.
> > 
> > The situation: I write a patch.  I post it for review.  A bot does
> > something and finds a bug (could be compile-error, could be boot
> > problem).  That bot sends a bug report with a suggestion to add
> > Reported-by:.  That suggestion is inappropriate because the bug never
> > made it upstream, so it looks like the bot reported the "problem"
> > that the patch "fixes".
> > 
> > It's not unique to "new feature" patches.  If I'm fixing a bug and
> > my fix also contains a bug spotted by a bot, adding Reported-by
> > makes it look like the bot spotted the original bug, rather than
> > spotting a bug in the fix.
> > 
> > The best thing to do in this case is nothing.  Do not credit the bot.
> > Maybe add a Checked-by:, but that would be a new trailer and I really
> > don't think we need a new kind of trailer to get wrong.
> 
> It seems like the only way to fix this is to fix the bots. Adding more
> documentation is unlikely to help in this case.

Links to the documentation at least may clarify the point in case of a review.

> Can't we file a bug to whoever is running the bots (Intel?) and ask them
> to remove the suggestion to add a Reported-by when the bot is testing a
> patch (as opposed to mainline or even -next)?

The granularity here is not a repo. It's a code itself and in some cases
it might be easy to distinguish new feature from the code modifications,
but when code is already there and feature is just an extension of the
existing file(s), it's hard to tell. And it might be true or not.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] docs: process: submitting-patches: Clarify the Reported-by usage

[Johan Hovold]

On Mon, Jan 31, 2022 at 05:34:35PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 31, 2022 at 04:18:30PM +0100, Johan Hovold wrote:
> > On Fri, Jan 28, 2022 at 01:44:20PM +0000, Matthew Wilcox wrote:

> > > I think this misunderstands the problem that Andy is trying to fix.
> > > 
> > > The situation: I write a patch.  I post it for review.  A bot does
> > > something and finds a bug (could be compile-error, could be boot
> > > problem).  That bot sends a bug report with a suggestion to add
> > > Reported-by:.  That suggestion is inappropriate because the bug never
> > > made it upstream, so it looks like the bot reported the "problem"
> > > that the patch "fixes".
> > > 
> > > It's not unique to "new feature" patches.  If I'm fixing a bug and
> > > my fix also contains a bug spotted by a bot, adding Reported-by
> > > makes it look like the bot spotted the original bug, rather than
> > > spotting a bug in the fix.
> > > 
> > > The best thing to do in this case is nothing.  Do not credit the bot.
> > > Maybe add a Checked-by:, but that would be a new trailer and I really
> > > don't think we need a new kind of trailer to get wrong.
> > 
> > It seems like the only way to fix this is to fix the bots. Adding more
> > documentation is unlikely to help in this case.
> 
> Links to the documentation at least may clarify the point in case of a
> review.

Sure.

> > Can't we file a bug to whoever is running the bots (Intel?) and ask them
> > to remove the suggestion to add a Reported-by when the bot is testing a
> > patch (as opposed to mainline or even -next)?
> 
> The granularity here is not a repo. It's a code itself and in some cases
> it might be easy to distinguish new feature from the code modifications,
> but when code is already there and feature is just an extension of the
> existing file(s), it's hard to tell. And it might be true or not.

Not sure I understand what you're saying here. Perhaps you and Matthew
are talking about different things after all.

But for Matthew's issue, the case where the bots are testing posted
patches ("Thank you for the patch! Yet something to improve:) should be
easy to fix by simply dropping or rephrasing the "kindly add following
tag as appropriate" suggestion.

When testing merged code, it may be harder to tell whether the branch in
question can be rebased or not (and an incremental fix with a
reported-by tag is warranted).

Johan

Re: [PATCH v1 1/1] docs: process: submitting-patches: Clarify the Reported-by usage

[Andy Shevchenko]

On Mon, Jan 31, 2022 at 05:47:32PM +0100, Johan Hovold wrote:
> On Mon, Jan 31, 2022 at 05:34:35PM +0200, Andy Shevchenko wrote:
> > On Mon, Jan 31, 2022 at 04:18:30PM +0100, Johan Hovold wrote:
> > > On Fri, Jan 28, 2022 at 01:44:20PM +0000, Matthew Wilcox wrote:
> 
> > > > I think this misunderstands the problem that Andy is trying to fix.
> > > > 
> > > > The situation: I write a patch.  I post it for review.  A bot does
> > > > something and finds a bug (could be compile-error, could be boot
> > > > problem).  That bot sends a bug report with a suggestion to add
> > > > Reported-by:.  That suggestion is inappropriate because the bug never
> > > > made it upstream, so it looks like the bot reported the "problem"
> > > > that the patch "fixes".
> > > > 
> > > > It's not unique to "new feature" patches.  If I'm fixing a bug and
> > > > my fix also contains a bug spotted by a bot, adding Reported-by
> > > > makes it look like the bot spotted the original bug, rather than
> > > > spotting a bug in the fix.
> > > > 
> > > > The best thing to do in this case is nothing.  Do not credit the bot.
> > > > Maybe add a Checked-by:, but that would be a new trailer and I really
> > > > don't think we need a new kind of trailer to get wrong.
> > > 
> > > It seems like the only way to fix this is to fix the bots. Adding more
> > > documentation is unlikely to help in this case.
> > 
> > Links to the documentation at least may clarify the point in case of a
> > review.
> 
> Sure.
> 
> > > Can't we file a bug to whoever is running the bots (Intel?) and ask them
> > > to remove the suggestion to add a Reported-by when the bot is testing a
> > > patch (as opposed to mainline or even -next)?
> > 
> > The granularity here is not a repo. It's a code itself and in some cases
> > it might be easy to distinguish new feature from the code modifications,
> > but when code is already there and feature is just an extension of the
> > existing file(s), it's hard to tell. And it might be true or not.
> 
> Not sure I understand what you're saying here. Perhaps you and Matthew
> are talking about different things after all.

I'm talking about your suggestion to fix the bots. It's not easy.
The problem is the same as Matthew explained.

> But for Matthew's issue, the case where the bots are testing posted
> patches ("Thank you for the patch! Yet something to improve:) should be
> easy to fix by simply dropping or rephrasing the "kindly add following
> tag as appropriate" suggestion.

Yes, but this is not "fixing the bots", it falls into category "working around"
them, because even for a clear bug report the suggestion can be stronger.
And doing that properly without kinda AI not easy.

> When testing merged code, it may be harder to tell whether the branch in
> question can be rebased or not (and an incremental fix with a
> reported-by tag is warranted).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] docs: process: submitting-patches: Clarify the Reported-by usage

[Johan Hovold]

On Mon, Jan 31, 2022 at 08:16:42PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 31, 2022 at 05:47:32PM +0100, Johan Hovold wrote:
> > On Mon, Jan 31, 2022 at 05:34:35PM +0200, Andy Shevchenko wrote:
> > > On Mon, Jan 31, 2022 at 04:18:30PM +0100, Johan Hovold wrote:
> > > > On Fri, Jan 28, 2022 at 01:44:20PM +0000, Matthew Wilcox wrote:
> > 
> > > > > I think this misunderstands the problem that Andy is trying to fix.
> > > > > 
> > > > > The situation: I write a patch.  I post it for review.  A bot does
> > > > > something and finds a bug (could be compile-error, could be boot
> > > > > problem).  That bot sends a bug report with a suggestion to add
> > > > > Reported-by:.  That suggestion is inappropriate because the bug never
> > > > > made it upstream, so it looks like the bot reported the "problem"
> > > > > that the patch "fixes".
> > > > > 
> > > > > It's not unique to "new feature" patches.  If I'm fixing a bug and
> > > > > my fix also contains a bug spotted by a bot, adding Reported-by
> > > > > makes it look like the bot spotted the original bug, rather than
> > > > > spotting a bug in the fix.
> > > > > 
> > > > > The best thing to do in this case is nothing.  Do not credit the bot.
> > > > > Maybe add a Checked-by:, but that would be a new trailer and I really
> > > > > don't think we need a new kind of trailer to get wrong.
> > > > 
> > > > It seems like the only way to fix this is to fix the bots. Adding more
> > > > documentation is unlikely to help in this case.
> > > 
> > > Links to the documentation at least may clarify the point in case of a
> > > review.
> > 
> > Sure.
> > 
> > > > Can't we file a bug to whoever is running the bots (Intel?) and ask them
> > > > to remove the suggestion to add a Reported-by when the bot is testing a
> > > > patch (as opposed to mainline or even -next)?
> > > 
> > > The granularity here is not a repo. It's a code itself and in some cases
> > > it might be easy to distinguish new feature from the code modifications,
> > > but when code is already there and feature is just an extension of the
> > > existing file(s), it's hard to tell. And it might be true or not.
> > 
> > Not sure I understand what you're saying here. Perhaps you and Matthew
> > are talking about different things after all.
> 
> I'm talking about your suggestion to fix the bots. It's not easy.
> The problem is the same as Matthew explained.

Perhaps I'm missing something, but if you re-read Mathews description
above, it still seems to me like the issue is that the bots are trying
to claim credit for finding things that haven't been merged yet.

Your suggestion is to document that the bots should be ignored. My
suggestion is to fix the bots.
 
> > But for Matthew's issue, the case where the bots are testing posted
> > patches ("Thank you for the patch! Yet something to improve:) should be
> > easy to fix by simply dropping or rephrasing the "kindly add following
> > tag as appropriate" suggestion.
> 
> Yes, but this is not "fixing the bots", it falls into category "working around"
> them, because even for a clear bug report the suggestion can be stronger.
> And doing that properly without kinda AI not easy.

A patch has (typically) not been merged yet when the bots find problems,
but when testing a branch it's harder to tell (e.g. unless it's
mainline).

The bot already knows it is testing a patch, so the "please give me
credit" suggestion can be dropped or rephrased in that case. That's
fixing the bot. And no need for AI.

> > When testing merged code, it may be harder to tell whether the branch in
> > question can be rebased or not (and an incremental fix with a
> > reported-by tag is warranted).

Johan

Re: [PATCH v1 1/1] docs: process: submitting-patches: Clarify the Reported-by usage

[Dan Carpenter]

On Thu, Jan 27, 2022 at 05:53:34PM +0200, Andy Shevchenko wrote:
> It's unclear from "Submitting Patches" documentation that Reported-by
> is not supposed to be used against new features. (It's more clear
> in the section 5.4 "Patch formatting and changelogs" of the "A guide
> to the Kernel Development Process", where it suggests that change
> should fix something existing in the kernel. Clarify the Reported-by
> usage in the "Submitting Patches".
> 
> Reported-by: Florian Eckert <fe@dev.tdt.de>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---

I've always lobbied for a Fixes-from for bug fixes that get folded into
the original patch but I also kind of agree with Dan Williams that
Reported-by tags are fine for new code.

https://lore.kernel.org/all/CAPcyv4gbgDMGBQ3t4q1EckELbMG5JXi10YuzLsMjFns67od=sw@mail.gmail.com/

Everyone can figure it out from the context that kbuild bot didn't
really tell anyone to write a new driver.  It's just the reported-by
count that's used to justify funding a team of devs.

regards,
dan carpenter

Re: [PATCH v1 1/1] docs: process: submitting-patches: Clarify the Reported-by usage

[Dan Carpenter]

On Tue, Feb 01, 2022 at 09:51:33AM +0100, Johan Hovold wrote:
> On Mon, Jan 31, 2022 at 08:16:42PM +0200, Andy Shevchenko wrote:
> > On Mon, Jan 31, 2022 at 05:47:32PM +0100, Johan Hovold wrote:
> > > On Mon, Jan 31, 2022 at 05:34:35PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Jan 31, 2022 at 04:18:30PM +0100, Johan Hovold wrote:
> > > > > On Fri, Jan 28, 2022 at 01:44:20PM +0000, Matthew Wilcox wrote:
> > > 
> > > > > > I think this misunderstands the problem that Andy is trying to fix.
> > > > > > 
> > > > > > The situation: I write a patch.  I post it for review.  A bot does
> > > > > > something and finds a bug (could be compile-error, could be boot
> > > > > > problem).  That bot sends a bug report with a suggestion to add
> > > > > > Reported-by:.  That suggestion is inappropriate because the bug never
> > > > > > made it upstream, so it looks like the bot reported the "problem"
> > > > > > that the patch "fixes".
> > > > > > 
> > > > > > It's not unique to "new feature" patches.  If I'm fixing a bug and
> > > > > > my fix also contains a bug spotted by a bot, adding Reported-by
> > > > > > makes it look like the bot spotted the original bug, rather than
> > > > > > spotting a bug in the fix.
> > > > > > 
> > > > > > The best thing to do in this case is nothing.  Do not credit the bot.
> > > > > > Maybe add a Checked-by:, but that would be a new trailer and I really
> > > > > > don't think we need a new kind of trailer to get wrong.
> > > > > 
> > > > > It seems like the only way to fix this is to fix the bots. Adding more
> > > > > documentation is unlikely to help in this case.
> > > > 
> > > > Links to the documentation at least may clarify the point in case of a
> > > > review.
> > > 
> > > Sure.
> > > 
> > > > > Can't we file a bug to whoever is running the bots (Intel?) and ask them
> > > > > to remove the suggestion to add a Reported-by when the bot is testing a
> > > > > patch (as opposed to mainline or even -next)?
> > > > 
> > > > The granularity here is not a repo. It's a code itself and in some cases
> > > > it might be easy to distinguish new feature from the code modifications,
> > > > but when code is already there and feature is just an extension of the
> > > > existing file(s), it's hard to tell. And it might be true or not.
> > > 
> > > Not sure I understand what you're saying here. Perhaps you and Matthew
> > > are talking about different things after all.
> > 
> > I'm talking about your suggestion to fix the bots. It's not easy.
> > The problem is the same as Matthew explained.
> 
> Perhaps I'm missing something, but if you re-read Mathews description
> above, it still seems to me like the issue is that the bots are trying
> to claim credit for finding things that haven't been merged yet.
> 
> Your suggestion is to document that the bots should be ignored. My
> suggestion is to fix the bots.

Originally the kbuild bot used to not have that notice but adding it
meant that kbuild bot got a lot more visibility.  The truth is that
managers love metrics and it helps people get paid.

The whole point of kbuild-bot was to search the lists and test code
before it gets merged.  If they just waited and tested linux-next they
would get their reported by tags because most trees don't rebase.  But
we're punishing them for being better at their job.  It's a perverse
incentive.

We should create a new tag for finding bugs during review.

regards,
dan carpenter

Re: [PATCH v1 1/1] docs: process: submitting-patches: Clarify the Reported-by usage

[Johan Hovold]

On Thu, Mar 03, 2022 at 12:54:32PM +0300, Dan Carpenter wrote:
> On Tue, Feb 01, 2022 at 09:51:33AM +0100, Johan Hovold wrote:
> > On Mon, Jan 31, 2022 at 08:16:42PM +0200, Andy Shevchenko wrote:
> > > On Mon, Jan 31, 2022 at 05:47:32PM +0100, Johan Hovold wrote:
> > > > On Mon, Jan 31, 2022 at 05:34:35PM +0200, Andy Shevchenko wrote:
> > > > > On Mon, Jan 31, 2022 at 04:18:30PM +0100, Johan Hovold wrote:
> > > > > > On Fri, Jan 28, 2022 at 01:44:20PM +0000, Matthew Wilcox wrote:
> > > > 
> > > > > > > I think this misunderstands the problem that Andy is trying to fix.
> > > > > > > 
> > > > > > > The situation: I write a patch.  I post it for review.  A bot does
> > > > > > > something and finds a bug (could be compile-error, could be boot
> > > > > > > problem).  That bot sends a bug report with a suggestion to add
> > > > > > > Reported-by:.  That suggestion is inappropriate because the bug never
> > > > > > > made it upstream, so it looks like the bot reported the "problem"
> > > > > > > that the patch "fixes".
> > > > > > > 
> > > > > > > It's not unique to "new feature" patches.  If I'm fixing a bug and
> > > > > > > my fix also contains a bug spotted by a bot, adding Reported-by
> > > > > > > makes it look like the bot spotted the original bug, rather than
> > > > > > > spotting a bug in the fix.
> > > > > > > 
> > > > > > > The best thing to do in this case is nothing.  Do not credit the bot.
> > > > > > > Maybe add a Checked-by:, but that would be a new trailer and I really
> > > > > > > don't think we need a new kind of trailer to get wrong.
> > > > > > 
> > > > > > It seems like the only way to fix this is to fix the bots. Adding more
> > > > > > documentation is unlikely to help in this case.

> > Perhaps I'm missing something, but if you re-read Mathews description
> > above, it still seems to me like the issue is that the bots are trying
> > to claim credit for finding things that haven't been merged yet.
> > 
> > Your suggestion is to document that the bots should be ignored. My
> > suggestion is to fix the bots.
> 
> Originally the kbuild bot used to not have that notice but adding it
> meant that kbuild bot got a lot more visibility.  The truth is that
> managers love metrics and it helps people get paid.
> 
> The whole point of kbuild-bot was to search the lists and test code
> before it gets merged.  If they just waited and tested linux-next they
> would get their reported by tags because most trees don't rebase.  But
> we're punishing them for being better at their job.  It's a perverse
> incentive.

I hear you. But I also get Matthew's and Andy's point about it not being
quite right to give an automatic test program Reported-by credit for
finding, say, an unused variable in a not yet merged patch. And perhaps
even more so since real reviewers often get no credit at all (but
perhaps a mention in a cover letter).

> We should create a new tag for finding bugs during review.

Yeah, I guess your perverse incentive argument applies equally to human
reviewers even if I'm also reluctant to going down this path.

Johan

Re: [PATCH v1 1/1] docs: process: submitting-patches: Clarify the Reported-by usage

[Dan Carpenter]

On Thu, Mar 03, 2022 at 02:27:42PM +0100, Johan Hovold wrote:
> On Thu, Mar 03, 2022 at 12:54:32PM +0300, Dan Carpenter wrote:
> > On Tue, Feb 01, 2022 at 09:51:33AM +0100, Johan Hovold wrote:
> > > On Mon, Jan 31, 2022 at 08:16:42PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Jan 31, 2022 at 05:47:32PM +0100, Johan Hovold wrote:
> > > > > On Mon, Jan 31, 2022 at 05:34:35PM +0200, Andy Shevchenko wrote:
> > > > > > On Mon, Jan 31, 2022 at 04:18:30PM +0100, Johan Hovold wrote:
> > > > > > > On Fri, Jan 28, 2022 at 01:44:20PM +0000, Matthew Wilcox wrote:
> > > > > 
> > > > > > > > I think this misunderstands the problem that Andy is trying to fix.
> > > > > > > > 
> > > > > > > > The situation: I write a patch.  I post it for review.  A bot does
> > > > > > > > something and finds a bug (could be compile-error, could be boot
> > > > > > > > problem).  That bot sends a bug report with a suggestion to add
> > > > > > > > Reported-by:.  That suggestion is inappropriate because the bug never
> > > > > > > > made it upstream, so it looks like the bot reported the "problem"
> > > > > > > > that the patch "fixes".
> > > > > > > > 
> > > > > > > > It's not unique to "new feature" patches.  If I'm fixing a bug and
> > > > > > > > my fix also contains a bug spotted by a bot, adding Reported-by
> > > > > > > > makes it look like the bot spotted the original bug, rather than
> > > > > > > > spotting a bug in the fix.
> > > > > > > > 
> > > > > > > > The best thing to do in this case is nothing.  Do not credit the bot.
> > > > > > > > Maybe add a Checked-by:, but that would be a new trailer and I really
> > > > > > > > don't think we need a new kind of trailer to get wrong.
> > > > > > > 
> > > > > > > It seems like the only way to fix this is to fix the bots. Adding more
> > > > > > > documentation is unlikely to help in this case.
> 
> > > Perhaps I'm missing something, but if you re-read Mathews description
> > > above, it still seems to me like the issue is that the bots are trying
> > > to claim credit for finding things that haven't been merged yet.
> > > 
> > > Your suggestion is to document that the bots should be ignored. My
> > > suggestion is to fix the bots.
> > 
> > Originally the kbuild bot used to not have that notice but adding it
> > meant that kbuild bot got a lot more visibility.  The truth is that
> > managers love metrics and it helps people get paid.
> > 
> > The whole point of kbuild-bot was to search the lists and test code
> > before it gets merged.  If they just waited and tested linux-next they
> > would get their reported by tags because most trees don't rebase.  But
> > we're punishing them for being better at their job.  It's a perverse
> > incentive.
> 
> I hear you. But I also get Matthew's and Andy's point about it not being
> quite right to give an automatic test program Reported-by credit for
> finding, say, an unused variable in a not yet merged patch. And perhaps
> even more so since real reviewers often get no credit at all (but
> perhaps a mention in a cover letter).
> 
> > We should create a new tag for finding bugs during review.
> 
> Yeah, I guess your perverse incentive argument applies equally to human
> reviewers even if I'm also reluctant to going down this path.

We could make the tag more explicit:  Bug-fixes-from:

You would only get credit for crashing bugs.  Even manual reviewers
should get credit.  You would not get credit for style complaints or
noticing typos in the commit message.  If the bug is not severe enough
to merit a backport or a Fixes tag then no credit.

People always complain that this rule is ambiguous but we make those
choices all the time about what to include in the -stable trees.

regards,
dan carpenter