On Thu, Dec 3, 2020 at 10:35 AM Leon Romanovsky <[email protected]> wrote:
> On Wed, Dec 02, 2020 at 08:02:27PM -0800, Dan Williams wrote:
> > On Wed, Dec 2, 2020 at 3:44 PM Vlastimil Babka <[email protected]> wrote:
> > > there was a bit of debate on Twitter about this, so I thought I would bring it
> > > here. Imagine a scenario where patch sits as a commit in -next and there's a bug
> > > report or fix, possibly by a bot or with some static analysis. The maintainer
> > > decides to fold it into the original patch, which makes sense for e.g.
> > > bisectability. But there seem to be no clear rules about attribution in this
> > > case, which looks like there should be, probably in
> > > Documentation/maintainer/modifying-patches.rst
> > >
> > > The original bug fix might include a From: $author, a Reported-by: (e.g.
> > > syzbot), Fixes: $next-commit, some tag such as Addresses-Coverity: to credit the
> > > static analysis tool, and an SoB. After folding, all that's left might be a line
> > > as "include fix from $author" in the SoB area. This is a loss of
> > > metadata/attribution just due to folding, and might make contributors unhappy.
> > > Had they sent the fix after the original commit was mainline and immutable, all
> > > the info above would "survive" in the form of new commit.
> > >
> > > So I think we could decide what the proper format would be, and document it
> > > properly. I personally wouldn't mind just copy/pasting the whole commit message
> > > of the fix (with just a short issue description, no need to include stacktraces
> > > etc if the fix is folded), we could just standardize where, and how to delimit
> > > it from the main commit message. If it's a report (person or bot) of a bug that
> > > the main author then fixed, preserve the Reported-by in the same way (making
> > > clear it's not a Reported-By for the "main thing" addressed by the commit).
> > >
> > > In the debate one less verbose alternatve proposed was a SoB with comment
> > > describing it's for a fix and not whole patch, as some see SoB as the main mark
> > > of contribution, that can be easily found and counted etc. I'm not so sure about
> > > it myself, as AFAIK SoB is mainly a DCO thing, and for a maintainer it means
> > > something else ("passed through my tree") than for a patch author. And this
> > > approach would still lose the other tags.
> > >
> > > Thoughts?
> >
> > How about a convention to add a Reported-by: and a Link: to the
> > incremental fixup discussion? It's just polite to credit helpful
> > feedback, not sure it needs a more formal process.
>
> Maybe "Fixup-Reported-by:" and "Fixup-Link:"?
And "Earlier-Review-Comments-Provided-by:"?
How far do we want to go?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On Thu, Dec 03, 2020 at 10:36:56AM +0100, Geert Uytterhoeven wrote:
> On Thu, Dec 3, 2020 at 10:35 AM Leon Romanovsky <[email protected]> wrote:
> > On Wed, Dec 02, 2020 at 08:02:27PM -0800, Dan Williams wrote:
> > > On Wed, Dec 2, 2020 at 3:44 PM Vlastimil Babka <[email protected]> wrote:
> > > > there was a bit of debate on Twitter about this, so I thought I would bring it
> > > > here. Imagine a scenario where patch sits as a commit in -next and there's a bug
> > > > report or fix, possibly by a bot or with some static analysis. The maintainer
> > > > decides to fold it into the original patch, which makes sense for e.g.
> > > > bisectability. But there seem to be no clear rules about attribution in this
> > > > case, which looks like there should be, probably in
> > > > Documentation/maintainer/modifying-patches.rst
> > > >
> > > > The original bug fix might include a From: $author, a Reported-by: (e.g.
> > > > syzbot), Fixes: $next-commit, some tag such as Addresses-Coverity: to credit the
> > > > static analysis tool, and an SoB. After folding, all that's left might be a line
> > > > as "include fix from $author" in the SoB area. This is a loss of
> > > > metadata/attribution just due to folding, and might make contributors unhappy.
> > > > Had they sent the fix after the original commit was mainline and immutable, all
> > > > the info above would "survive" in the form of new commit.
> > > >
> > > > So I think we could decide what the proper format would be, and document it
> > > > properly. I personally wouldn't mind just copy/pasting the whole commit message
> > > > of the fix (with just a short issue description, no need to include stacktraces
> > > > etc if the fix is folded), we could just standardize where, and how to delimit
> > > > it from the main commit message. If it's a report (person or bot) of a bug that
> > > > the main author then fixed, preserve the Reported-by in the same way (making
> > > > clear it's not a Reported-By for the "main thing" addressed by the commit).
> > > >
> > > > In the debate one less verbose alternatve proposed was a SoB with comment
> > > > describing it's for a fix and not whole patch, as some see SoB as the main mark
> > > > of contribution, that can be easily found and counted etc. I'm not so sure about
> > > > it myself, as AFAIK SoB is mainly a DCO thing, and for a maintainer it means
> > > > something else ("passed through my tree") than for a patch author. And this
> > > > approach would still lose the other tags.
> > > >
> > > > Thoughts?
> > >
> > > How about a convention to add a Reported-by: and a Link: to the
> > > incremental fixup discussion? It's just polite to credit helpful
> > > feedback, not sure it needs a more formal process.
> >
> > Maybe "Fixup-Reported-by:" and "Fixup-Link:"?
>
> And "Earlier-Review-Comments-Provided-by:"?
>
> How far do we want to go?
I don't want to overload existing meaning of "Reported-by:" and "Link:",
so anything else is fine by me.
I imagine that all those who puts their own Reviewed-by, Signed-off-by
and Tested-by in the same patch will be happy to use something like you
are proposing - "Co-developed-Signed-Reviewed-Tested-by:" tag.
Thanks
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> 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
On Thu, Dec 03, 2020 at 12:40:47PM +0200, Leon Romanovsky wrote:
> On Thu, Dec 03, 2020 at 10:36:56AM +0100, Geert Uytterhoeven wrote:
> > On Thu, Dec 3, 2020 at 10:35 AM Leon Romanovsky <[email protected]> wrote:
> > > On Wed, Dec 02, 2020 at 08:02:27PM -0800, Dan Williams wrote:
> > > > On Wed, Dec 2, 2020 at 3:44 PM Vlastimil Babka <[email protected]> wrote:
> > > > > there was a bit of debate on Twitter about this, so I thought I would bring it
> > > > > here. Imagine a scenario where patch sits as a commit in -next and there's a bug
> > > > > report or fix, possibly by a bot or with some static analysis. The maintainer
> > > > > decides to fold it into the original patch, which makes sense for e.g.
> > > > > bisectability. But there seem to be no clear rules about attribution in this
> > > > > case, which looks like there should be, probably in
> > > > > Documentation/maintainer/modifying-patches.rst
> > > > >
> > > > > The original bug fix might include a From: $author, a Reported-by: (e.g.
> > > > > syzbot), Fixes: $next-commit, some tag such as Addresses-Coverity: to credit the
> > > > > static analysis tool, and an SoB. After folding, all that's left might be a line
> > > > > as "include fix from $author" in the SoB area. This is a loss of
> > > > > metadata/attribution just due to folding, and might make contributors unhappy.
> > > > > Had they sent the fix after the original commit was mainline and immutable, all
> > > > > the info above would "survive" in the form of new commit.
> > > > >
> > > > > So I think we could decide what the proper format would be, and document it
> > > > > properly. I personally wouldn't mind just copy/pasting the whole commit message
> > > > > of the fix (with just a short issue description, no need to include stacktraces
> > > > > etc if the fix is folded), we could just standardize where, and how to delimit
> > > > > it from the main commit message. If it's a report (person or bot) of a bug that
> > > > > the main author then fixed, preserve the Reported-by in the same way (making
> > > > > clear it's not a Reported-By for the "main thing" addressed by the commit).
> > > > >
> > > > > In the debate one less verbose alternatve proposed was a SoB with comment
> > > > > describing it's for a fix and not whole patch, as some see SoB as the main mark
> > > > > of contribution, that can be easily found and counted etc. I'm not so sure about
> > > > > it myself, as AFAIK SoB is mainly a DCO thing, and for a maintainer it means
> > > > > something else ("passed through my tree") than for a patch author. And this
> > > > > approach would still lose the other tags.
> > > > >
> > > > > Thoughts?
> > > >
> > > > How about a convention to add a Reported-by: and a Link: to the
> > > > incremental fixup discussion? It's just polite to credit helpful
> > > > feedback, not sure it needs a more formal process.
> > >
> > > Maybe "Fixup-Reported-by:" and "Fixup-Link:"?
> >
> > And "Earlier-Review-Comments-Provided-by:"?
> >
> > How far do we want to go?
>
> I don't want to overload existing meaning of "Reported-by:" and "Link:",
> so anything else is fine by me.
>
> I imagine that all those who puts their own Reviewed-by, Signed-off-by
> and Tested-by in the same patch will be happy to use something like you
> are proposing - "Co-developed-Signed-Reviewed-Tested-by:" tag.
We already have "Co-developerd-by:" as a valid tag, no need to merge
more into this :)
On Thu, Dec 03, 2020 at 07:30:44PM +0100, Greg KH wrote:
> On Thu, Dec 03, 2020 at 12:40:47PM +0200, Leon Romanovsky wrote:
> > On Thu, Dec 03, 2020 at 10:36:56AM +0100, Geert Uytterhoeven wrote:
> > > On Thu, Dec 3, 2020 at 10:35 AM Leon Romanovsky <[email protected]> wrote:
> > > > On Wed, Dec 02, 2020 at 08:02:27PM -0800, Dan Williams wrote:
> > > > > On Wed, Dec 2, 2020 at 3:44 PM Vlastimil Babka <[email protected]> wrote:
> > > > > > there was a bit of debate on Twitter about this, so I thought I would bring it
> > > > > > here. Imagine a scenario where patch sits as a commit in -next and there's a bug
> > > > > > report or fix, possibly by a bot or with some static analysis. The maintainer
> > > > > > decides to fold it into the original patch, which makes sense for e.g.
> > > > > > bisectability. But there seem to be no clear rules about attribution in this
> > > > > > case, which looks like there should be, probably in
> > > > > > Documentation/maintainer/modifying-patches.rst
> > > > > >
> > > > > > The original bug fix might include a From: $author, a Reported-by: (e.g.
> > > > > > syzbot), Fixes: $next-commit, some tag such as Addresses-Coverity: to credit the
> > > > > > static analysis tool, and an SoB. After folding, all that's left might be a line
> > > > > > as "include fix from $author" in the SoB area. This is a loss of
> > > > > > metadata/attribution just due to folding, and might make contributors unhappy.
> > > > > > Had they sent the fix after the original commit was mainline and immutable, all
> > > > > > the info above would "survive" in the form of new commit.
> > > > > >
> > > > > > So I think we could decide what the proper format would be, and document it
> > > > > > properly. I personally wouldn't mind just copy/pasting the whole commit message
> > > > > > of the fix (with just a short issue description, no need to include stacktraces
> > > > > > etc if the fix is folded), we could just standardize where, and how to delimit
> > > > > > it from the main commit message. If it's a report (person or bot) of a bug that
> > > > > > the main author then fixed, preserve the Reported-by in the same way (making
> > > > > > clear it's not a Reported-By for the "main thing" addressed by the commit).
> > > > > >
> > > > > > In the debate one less verbose alternatve proposed was a SoB with comment
> > > > > > describing it's for a fix and not whole patch, as some see SoB as the main mark
> > > > > > of contribution, that can be easily found and counted etc. I'm not so sure about
> > > > > > it myself, as AFAIK SoB is mainly a DCO thing, and for a maintainer it means
> > > > > > something else ("passed through my tree") than for a patch author. And this
> > > > > > approach would still lose the other tags.
> > > > > >
> > > > > > Thoughts?
> > > > >
> > > > > How about a convention to add a Reported-by: and a Link: to the
> > > > > incremental fixup discussion? It's just polite to credit helpful
> > > > > feedback, not sure it needs a more formal process.
> > > >
> > > > Maybe "Fixup-Reported-by:" and "Fixup-Link:"?
> > >
> > > And "Earlier-Review-Comments-Provided-by:"?
> > >
> > > How far do we want to go?
> >
> > I don't want to overload existing meaning of "Reported-by:" and "Link:",
> > so anything else is fine by me.
> >
> > I imagine that all those who puts their own Reviewed-by, Signed-off-by
> > and Tested-by in the same patch will be happy to use something like you
> > are proposing - "Co-developed-Signed-Reviewed-Tested-by:" tag.
>
> We already have "Co-developerd-by:" as a valid tag, no need to merge
> more into this :)
It was joke, but the reality is even more exciting.
See commit 71cc849b7093 ("KVM: x86: Fix split-irqchip vs interrupt injection window request")
for the need of "Reported-Analyzed-Reviewed-Tested-by:" tag.
And endless amount of commits with "Reviewed-Signed-by:" from maintainers that gives wrong
impression that other maintainers merge code without reviewing it.
Thanks
On Thu, Dec 03, 2020 at 07:30:44PM +0100, Greg KH wrote:
> On Thu, Dec 03, 2020 at 12:40:47PM +0200, Leon Romanovsky wrote:
> > On Thu, Dec 03, 2020 at 10:36:56AM +0100, Geert Uytterhoeven wrote:
> > > On Thu, Dec 3, 2020 at 10:35 AM Leon Romanovsky <[email protected]> wrote:
> > > > On Wed, Dec 02, 2020 at 08:02:27PM -0800, Dan Williams wrote:
> > > > > On Wed, Dec 2, 2020 at 3:44 PM Vlastimil Babka <[email protected]> wrote:
> > > > > > there was a bit of debate on Twitter about this, so I thought I would bring it
> > > > > > here. Imagine a scenario where patch sits as a commit in -next and there's a bug
> > > > > > report or fix, possibly by a bot or with some static analysis. The maintainer
> > > > > > decides to fold it into the original patch, which makes sense for e.g.
> > > > > > bisectability. But there seem to be no clear rules about attribution in this
> > > > > > case, which looks like there should be, probably in
> > > > > > Documentation/maintainer/modifying-patches.rst
> > > > > >
> > > > > > The original bug fix might include a From: $author, a Reported-by: (e.g.
> > > > > > syzbot), Fixes: $next-commit, some tag such as Addresses-Coverity: to credit the
> > > > > > static analysis tool, and an SoB. After folding, all that's left might be a line
> > > > > > as "include fix from $author" in the SoB area. This is a loss of
> > > > > > metadata/attribution just due to folding, and might make contributors unhappy.
> > > > > > Had they sent the fix after the original commit was mainline and immutable, all
> > > > > > the info above would "survive" in the form of new commit.
> > > > > >
> > > > > > So I think we could decide what the proper format would be, and document it
> > > > > > properly. I personally wouldn't mind just copy/pasting the whole commit message
> > > > > > of the fix (with just a short issue description, no need to include stacktraces
> > > > > > etc if the fix is folded), we could just standardize where, and how to delimit
> > > > > > it from the main commit message. If it's a report (person or bot) of a bug that
> > > > > > the main author then fixed, preserve the Reported-by in the same way (making
> > > > > > clear it's not a Reported-By for the "main thing" addressed by the commit).
> > > > > >
> > > > > > In the debate one less verbose alternatve proposed was a SoB with comment
> > > > > > describing it's for a fix and not whole patch, as some see SoB as the main mark
> > > > > > of contribution, that can be easily found and counted etc. I'm not so sure about
> > > > > > it myself, as AFAIK SoB is mainly a DCO thing, and for a maintainer it means
> > > > > > something else ("passed through my tree") than for a patch author. And this
> > > > > > approach would still lose the other tags.
> > > > > >
> > > > > > Thoughts?
> > > > >
> > > > > How about a convention to add a Reported-by: and a Link: to the
> > > > > incremental fixup discussion? It's just polite to credit helpful
> > > > > feedback, not sure it needs a more formal process.
To me, "Reported-by:" is associated with "this person reported the
problem being fixed by this commit". For these kinds of larger commits,
that may not be sensible. I.e. it's some larger feature that the "I
found a problem with this commit" author wasn't even involved in.
I think it's important to capture those in some way, even if they're
not considered "copyrightable" or whatever -- that's not the bar I'm
interested in; I want to make sure people are acknowledged for the time
and effort they spent. And whether we like it or not, these kinds of
tags do that. Besides, such fix authors have expressly asked for this,
which should be sufficient reason enough to find a solution.
> > > > Maybe "Fixup-Reported-by:" and "Fixup-Link:"?
> > >
> > > And "Earlier-Review-Comments-Provided-by:"?
> > >
> > > How far do we want to go?
> >
> > I don't want to overload existing meaning of "Reported-by:" and "Link:",
> > so anything else is fine by me.
Agreed.
> > I imagine that all those who puts their own Reviewed-by, Signed-off-by
> > and Tested-by in the same patch will be happy to use something like you
> > are proposing - "Co-developed-Signed-Reviewed-Tested-by:" tag.
>
> We already have "Co-developerd-by:" as a valid tag, no need to merge
> more into this :)
"Co-developed-by", to me, has a connotation of significant authorship.
For the "weaker" cases, I tend to use "Suggested-by" or put something
like "Based on a patch by $person[link]" in the body.
For the kinds of fixes mentioned here, and more specifically for the
kinds of fixes that I have received from both Colin Ian King and Dan
Carpenter that fall into this "tiny fix"[1] category, I think something
simply like "Adjusted-by" could be used. I've already tried to include
"Link" tags to things that got folded in, but without the Adjusted-by tag,
it lacks the right kind of searchability and recognition.
"Fixes-by" is too close to "Fixes" (and implies more than one
fix). "Fixup-by" implies singular. "Debugged-by" is like the other
existing high-level tags, in that they speak to the ENTIRE patch.
If not "Adjusted-by", what about "Tweaked-by", "Helped-by",
"Corrected-by"?
Colin, Dan, any thoughts on how you'd like to see stuff?
-Kees
[1] "tiny" in the sense of characters changed, usually. There was very
much NOT a "tiny" amount of time spent on it, nor do they have "tiny"
impact -- which is the whole point of calling this out in the
commit.
--
Kees Cook
On Tue, 2020-12-08 at 16:34 -0800, Kees Cook wrote:
> If not "Adjusted-by", what about "Tweaked-by", "Helped-by",
> "Corrected-by"?
Improved-by: / Enhanced-by: / Revisions-by:
Or simply don't use anything but a link to the conversion thread
like Konstantin suggested.
I still want to know what actual value these things have and to whom.
On Tue, Dec 08, 2020 at 09:01:49PM -0800, Joe Perches wrote:
> On Tue, 2020-12-08 at 16:34 -0800, Kees Cook wrote:
>
> > If not "Adjusted-by", what about "Tweaked-by", "Helped-by",
> > "Corrected-by"?
>
> Improved-by: / Enhanced-by: / Revisions-by:
>
I don't think we should give any credit for improvements or enhancements,
only for fixes. Complaining about style is its own reward.
Having to redo a patch is already a huge headache. Normally, I already
considered the reviewer's prefered style and decided I didn't like it.
Then to make me redo the patch in an ugly style and say thankyou on
top of that??? Forget about it. Plus, as a reviewer I hate reviewing
patches over and over.
I've argued for years that we should have a Fixes-from: tag. The zero
day bot is already encouraging people to add Reported-by tags for this
and a lot of people do.
regards,
dan carpenter
On 12/9/20 8:58 AM, Dan Carpenter wrote:
> On Tue, Dec 08, 2020 at 09:01:49PM -0800, Joe Perches wrote:
>> On Tue, 2020-12-08 at 16:34 -0800, Kees Cook wrote:
>>
>> > If not "Adjusted-by", what about "Tweaked-by", "Helped-by",
>> > "Corrected-by"?
>>
>> Improved-by: / Enhanced-by: / Revisions-by:
>>
>
> I don't think we should give any credit for improvements or enhancements,
Well, some are actually useful and not about reviewer's preferred style :) But
if an author redoes the patch as a result, it's their choice to mention useful
improvements in the next version's change log.
> only for fixes. Complaining about style is its own reward.
Right, let's focus on fixes and reports of bugs, that would have resulted in a
standalone commit, but don't.
> Having to redo a patch is already a huge headache. Normally, I already
> considered the reviewer's prefered style and decided I didn't like it.
> Then to make me redo the patch in an ugly style and say thankyou on
> top of that??? Forget about it. Plus, as a reviewer I hate reviewing
> patches over and over.
>
> I've argued for years that we should have a Fixes-from: tag. The zero
Standardizing the Fixes-from: tag (or any better name) would be a forward
progress, yes.
> day bot is already encouraging people to add Reported-by tags for this
> and a lot of people do.
"Reported-by:" becomes ambiguous once the bugfix for the reported issue in the
patch is folded, as it's no longer clear whether the bot reported the original
issue the patch is fixing, or a bug in the fix. So we should have a different
variant. "Fixes-reported-by:" so it has the same prefix?
> regards,
> dan carpenter
>
On Wed, 2020-12-09 at 10:58 +0300, Dan Carpenter wrote:
> On Tue, Dec 08, 2020 at 09:01:49PM -0800, Joe Perches wrote:
> > On Tue, 2020-12-08 at 16:34 -0800, Kees Cook wrote:
> >
> > > If not "Adjusted-by", what about "Tweaked-by", "Helped-by",
> > > "Corrected-by"?
> >
> > Improved-by: / Enhanced-by: / Revisions-by:
> >
>
> I don't think we should give any credit for improvements or enhancements,
> only for fixes.
Hey Dan.
I do. If a patch isn't comprehensive and a reviewer notices some
missing coverage or algorithmic performance enhancement, I think that
should be noted.
> Complaining about style is its own reward.
<chuckle, maybe so. I view it more like coaching...>
I believe I've said multiple times that style changes shouldn't require
additional commentary added to a patch.
I'm not making any suggestion to comment for style, only logic or defect
reduction/improvements as described above.
> Having to redo a patch is already a huge headache. Normally, I already
> considered the reviewer's prefered style and decided I didn't like it.
Example please. We both seem to prefer consistent style.
> Then to make me redo the patch in an ugly style and say thank you on
> top of that??? Forget about it.
Not a thing I've asked for.
> Plus, as a reviewer I hate reviewing patches over and over.
interdiff could be improved.
> I've argued for years that we should have a Fixes-from: tag. The zero
> day bot is already encouraging people to add Reported-by tags for this
> and a lot of people do.
It's still a question of what fixes means in any context.
https://www.google.com/search?q=%27fixes-from%3A%27%20carpenter%20site%3Alore.kernel.org
gives:
It looks like there aren't many great matches for your search
And I'm rather in favor of letting people make up their own
<whatever>-by: uses and not being too concerned about the specific
whatever word or phrase used. Postel's law and such.
On Wed, Dec 9, 2020 at 9:45 AM Vlastimil Babka <[email protected]> wrote:
> On 12/9/20 8:58 AM, Dan Carpenter wrote:
> > On Tue, Dec 08, 2020 at 09:01:49PM -0800, Joe Perches wrote:
> >> On Tue, 2020-12-08 at 16:34 -0800, Kees Cook wrote:
> >>
> >> > If not "Adjusted-by", what about "Tweaked-by", "Helped-by",
> >> > "Corrected-by"?
> >>
> >> Improved-by: / Enhanced-by: / Revisions-by:
> >>
> >
> > I don't think we should give any credit for improvements or enhancements,
>
> Well, some are actually useful and not about reviewer's preferred style :) But
> if an author redoes the patch as a result, it's their choice to mention useful
> improvements in the next version's change log.
>
> > only for fixes. Complaining about style is its own reward.
>
> Right, let's focus on fixes and reports of bugs, that would have resulted in a
> standalone commit, but don't.
>
> > Having to redo a patch is already a huge headache. Normally, I already
> > considered the reviewer's prefered style and decided I didn't like it.
> > Then to make me redo the patch in an ugly style and say thankyou on
> > top of that??? Forget about it. Plus, as a reviewer I hate reviewing
> > patches over and over.
> >
> > I've argued for years that we should have a Fixes-from: tag. The zero
>
> Standardizing the Fixes-from: tag (or any better name) would be a forward
> progress, yes.
>
> > day bot is already encouraging people to add Reported-by tags for this
> > and a lot of people do.
>
> "Reported-by:" becomes ambiguous once the bugfix for the reported issue in the
> patch is folded, as it's no longer clear whether the bot reported the original
> issue the patch is fixing, or a bug in the fix. So we should have a different
> variant. "Fixes-reported-by:" so it has the same prefix?
Taken-into-account-comments-from:
Any terse English word for that?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On Wed, Dec 09, 2020 at 12:54:30AM -0800, Joe Perches wrote:
> On Wed, 2020-12-09 at 10:58 +0300, Dan Carpenter wrote:
> > On Tue, Dec 08, 2020 at 09:01:49PM -0800, Joe Perches wrote:
> > > On Tue, 2020-12-08 at 16:34 -0800, Kees Cook wrote:
> > >
> > > > If not "Adjusted-by", what about "Tweaked-by", "Helped-by",
> > > > "Corrected-by"?
> > >
> > > Improved-by: / Enhanced-by: / Revisions-by:
> > >
> >
> > I don't think we should give any credit for improvements or enhancements,
> > only for fixes.
>
> Hey Dan.
>
> I do. If a patch isn't comprehensive and a reviewer notices some
> missing coverage or algorithmic performance enhancement, I think that
> should be noted.
>
> > Complaining about style is its own reward.
>
> <chuckle, maybe so. I view it more like coaching...>
>
> I believe I've said multiple times that style changes shouldn't require
> additional commentary added to a patch.
>
> I'm not making any suggestion to comment for style, only logic or defect
> reduction/improvements as described above.
How about we make the standard, "Would this fix be backported to stable?"
>
> > Having to redo a patch is already a huge headache. Normally, I already
> > considered the reviewer's prefered style and decided I didn't like it.
>
> Example please. We both seem to prefer consistent style.
>
For example, if you have a signedness bugs:
ret = frob(unsigned_long_size);
- if (ret < unsigned_long_size)
+ if (ret < 0 || ret < unsigned_long_size)
vs:
+ if (ret < (int)unsigned_long_size)
goto whatever;
To me, whoever fixes the bug gets to choose their prefered style but
maybe some reviewers have strong feelings one way or the other.
> > Then to make me redo the patch in an ugly style and say thank you on
> > top of that??? Forget about it.
>
> Not a thing I've asked for.
>
> > Plus, as a reviewer I hate reviewing patches over and over.
>
> interdiff could be improved.
>
> > I've argued for years that we should have a Fixes-from: tag. The zero
> > day bot is already encouraging people to add Reported-by tags for this
> > and a lot of people do.
>
> It's still a question of what fixes means in any context.
>
> https://www.google.com/search?q=%27fixes-from%3A%27%20carpenter%20site%3Alore.kernel.org
> gives:
> It looks like there aren't many great matches for your search
>
No, I mean people add Reported-by tags for fixes to the original commit
like in commit f026d8ca2904 ("igc: add support to eeprom, registers and
link self-tests").
regards,
dan carpenter
On Wed, Dec 9, 2020 at 2:31 AM Dan Carpenter <[email protected]> wrote:
>
> On Wed, Dec 09, 2020 at 12:54:30AM -0800, Joe Perches wrote:
> > On Wed, 2020-12-09 at 10:58 +0300, Dan Carpenter wrote:
> > > On Tue, Dec 08, 2020 at 09:01:49PM -0800, Joe Perches wrote:
> > > > On Tue, 2020-12-08 at 16:34 -0800, Kees Cook wrote:
> > > >
> > > > > If not "Adjusted-by", what about "Tweaked-by", "Helped-by",
> > > > > "Corrected-by"?
> > > >
> > > > Improved-by: / Enhanced-by: / Revisions-by:
> > > >
> > >
> > > I don't think we should give any credit for improvements or enhancements,
> > > only for fixes.
> >
> > Hey Dan.
> >
> > I do. If a patch isn't comprehensive and a reviewer notices some
> > missing coverage or algorithmic performance enhancement, I think that
> > should be noted.
> >
> > > Complaining about style is its own reward.
> >
> > <chuckle, maybe so. I view it more like coaching...>
> >
> > I believe I've said multiple times that style changes shouldn't require
> > additional commentary added to a patch.
> >
> > I'm not making any suggestion to comment for style, only logic or defect
> > reduction/improvements as described above.
>
> How about we make the standard, "Would this fix be backported to stable?"
>
> >
> > > Having to redo a patch is already a huge headache. Normally, I already
> > > considered the reviewer's prefered style and decided I didn't like it.
> >
> > Example please. We both seem to prefer consistent style.
> >
>
> For example, if you have a signedness bugs:
>
> ret = frob(unsigned_long_size);
> - if (ret < unsigned_long_size)
> + if (ret < 0 || ret < unsigned_long_size)
> vs:
> + if (ret < (int)unsigned_long_size)
> goto whatever;
>
> To me, whoever fixes the bug gets to choose their prefered style but
> maybe some reviewers have strong feelings one way or the other.
>
> > > Then to make me redo the patch in an ugly style and say thank you on
> > > top of that??? Forget about it.
> >
> > Not a thing I've asked for.
> >
> > > Plus, as a reviewer I hate reviewing patches over and over.
> >
> > interdiff could be improved.
> >
> > > I've argued for years that we should have a Fixes-from: tag. The zero
> > > day bot is already encouraging people to add Reported-by tags for this
> > > and a lot of people do.
> >
> > It's still a question of what fixes means in any context.
> >
> > https://www.google.com/search?q=%27fixes-from%3A%27%20carpenter%20site%3Alore.kernel.org
> > gives:
> > It looks like there aren't many great matches for your search
> >
>
> No, I mean people add Reported-by tags for fixes to the original commit
> like in commit f026d8ca2904 ("igc: add support to eeprom, registers and
> link self-tests").
For after the fact post-processing of tags to generate summary
reports, is there a significant difference between Reported-by for
"original commit motivation" and Reported-by for "follow-on fixups"?
Especially since this practice of Reported-by for fixups is already
deployed in the tree (at least kbuild-robot credit reports and my
subsystems operate this way).
If the fix is a slightly late and needs to come as a follow-on patch
the tag will be Reported-by on that fix. I fail to perceive a benefit
in augmenting the tag to indicate the resolution of the race condition
of the commit making it upstream.