LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Alex Elder <elder@linaro.org>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Alex Elder <elder@kernel.org>, kernel test robot <lkp@intel.com>,
	linux-staging@lists.linux.dev, Johan Hovold <johan@kernel.org>,
	linux-kernel@vger.kernel.org, greybus-dev@lists.linaro.org,
	"Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Subject: Re: [greybus-dev] [PATCH v2] staging: greybus: Convert uart.c from IDR to XArray
Date: Mon, 16 Aug 2021 14:17:07 -0500	[thread overview]
Message-ID: <e37b5a51-29ed-2a91-6285-aaa8885e0b9c@linaro.org> (raw)
In-Reply-To: <20210816183639.GF7722@kadam>

On 8/16/21 1:36 PM, Dan Carpenter wrote:
>>> There should be a Fixes-from: tag for bugs found in review (not style
>>> issues) but when I suggest it then people just say to use the
>>> Reported-by tag.
>> I think things caught during review aren't normally worthy
>> of specific mention in the commit message (though maybe in
>> the non-committed part under "---").  I mean, that's what
>> review is for.  And in the case of what<lkp@intel.com>
>> does, that's effectively a technical aspect of "review."
> I'm not talking about stuff like intending or naming schemes, I'm
> talking about real bugs like missing error codes or NULL dereferences.
> People do count tags so we might as well add them for worthwhile
> behavior.

So you're saying that things caught during review *should* be
given credit, as opposed to acknowledging the credit for catching
it only when the bug slips by the reviewers, caught after commit.

I understand that, and I get your point about the incentives
(which take the form of tags with acknowledgement).

As I indicated earlier, I'm all for showering credit on everyone
that helps.  But I still think doing so for input taken during
the review phase is too much, and full of fuzzy cases (how do you
judge whether a suggestion is worth acknowledging?).

I think what you do with Smatch is outstanding, and you deserve
a lot of credit for it.  But like checkpatch.pl, it would be even
better if people used it to catch things *before* they ever went
out for review.  That option would give *no* credit to Smatch for
catching problems early.  Yet catching issues as early as possible
is a good thing.  Should we acknowledge checkpatch.pl when it
tells us to fix something it finds; if so, which of them?

>> So I don't think "Fixes-from" (whatever that means) or
>> "Reported-by" make sense for this type of update.
>>
> Earlier today I forwarded a kbuild Smatch warning where someone had
> used "sizeof(0)" instead of "0" but because the patch was already
> applied, that means I got Reported-by credit.  If the kbuild-bot could
> have reported the bug before the networking people applied it that's
> more valuable but I get less credit.  It's a perverse incentive.

It's a perverse incentive for you as Smatch developer.  But I think
the better place to put an incentive is on getting people to avoid
sending patches at all until they have used tools available to
automatically find issues before they get out for review.

> Also I sort of don't like the Reviewed-by tag.  I see a lot of people
> adding Reviewed-by but I've never seen them point out a bug during the
> review process so that seems pretty worthless.  But Fixes-from means
> that person knows what they're talking about.

That's not a problem with Reviewed-by, it's a problem with people
misusing it.  Are you suggesting that "Fixes-from" would be applied
by the developer, not reviewer?  Regardless, Reviewed-by is *supposed*
to carry meaning.  "Documentation/process/submitting-patches.rst" has
a section that describes what the "Reviewer's statement of oversight"
represents.

I think it would be nice to recognize review feedback.  It's
actually more valuable than the summary statement "I have
reviewed this and find it acceptable."  But I don't believe
adding new acknowledgement tags is a good way to do it.

					-Alex

> 
> regards,
> dan carpenter


  parent reply	other threads:[~2021-08-16 19:17 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-14 18:11 Fabio M. De Francesco
2021-08-16 14:46 ` Alex Elder
2021-08-16 15:01   ` Greg Kroah-Hartman
2021-08-16 15:06     ` Dan Carpenter
2021-08-16 15:10       ` [greybus-dev] " Alex Elder
2021-08-16 18:36         ` Dan Carpenter
2021-08-16 18:38           ` Dan Carpenter
2021-08-16 19:17           ` Alex Elder [this message]
2021-08-17  4:58             ` Dan Carpenter
2021-08-16 16:55   ` Fabio M. De Francesco
2021-08-17 10:17   ` Fabio M. De Francesco
2021-08-25  5:20   ` Fabio M. De Francesco
2021-08-25 13:45     ` [greybus-dev] " Alex Elder
2021-08-26 15:57       ` Fabio M. De Francesco

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e37b5a51-29ed-2a91-6285-aaa8885e0b9c@linaro.org \
    --to=elder@linaro.org \
    --cc=dan.carpenter@oracle.com \
    --cc=elder@kernel.org \
    --cc=fmdefrancesco@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=greybus-dev@lists.linaro.org \
    --cc=johan@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-staging@lists.linux.dev \
    --cc=lkp@intel.com \
    --subject='Re: [greybus-dev] [PATCH v2] staging: greybus: Convert uart.c from IDR to XArray' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).