[Linux-kernel-mentees] git_commit_id false positives

[Lukas Bulwahn]

On Di., 10. Nov. 2020 at 19:41, Dwaipayan Ray <dwaipayanray1 at gmail.com>
wrote:

> Hi,
> I was looking into GIT_COMMIT_ID warnings for checkpatch
> and certainly there are false positives which needs to be fixed
> as also said by a previous discussion.
>
> But before fixing it, I would like your comments on what should
> be fixed and what shouldn't considering the added code can
> be complex.
>
> The data below is an evaluation of about 50k commits from v5.4.
>
> $ cat checkpatch_out_50k.txt | grep -A3 "GIT_COMMIT_ID" \
>    | grep "References" | wc -l
> 45


> First of all there are 45 warnings of type:
> References: 2248a28384fe ("drm/i915/gen8+: Add RC6 CTX corruption WA")
> References: 860afa086841 ("drm/i915/gt: Flush gen7 even harder"), etc.
>
> Should references be simply exempt from this check? I don't
> know much about it. But seems checkpatch is incorrect here.
>

I understand what the authors intend to do. They want to refer to a commit
as a tag, such as Link: for URLs or Fixes: for commits that fixed another
commit. However, this format of using References: is not an agreed and
common convention.

We could ask on the mailing list if there is some preference for such a
common agreement. Then, we can extend the kernel process documentation and
add a rule for checking this convention in checkpatch.

Do you want to do that? Expect some heavy discussion and some bikeshedding.


> For other messages,
> $ cat checkpatch_out_50k.txt | grep -A3 "GIT_COMMIT_ID"  | \
>   grep -v "GIT_COMMIT_ID" |     grep -P "^\s*[a-z0-9]{12,}" | wc -l
> 468
>
> There were 468 missing "commit" word before the GIT SHA. Some
> of these were due to split lines. I think that could be fixed with some
> effort.
>
> And finally the other fixable warning is for those with split commit
> titles itself.
>

Yes, you saw that another mentee started this work but could not really
finish that (yet?), right?

This sounds like a good case.


> Other than that, there was also this particular case with non escaped
> quotes within the commit title.
> Like:
> commit 97a32539b956 ("proc: convert everything to "struct proc_ops"")
> Should this be handled as well. Or is this a human error? I am not quite
> sure.
>

Of course, a commit title can contain quotes itself, but checkpatch does
not handle that case properly; this case can be quite tricky to handle. Let
us fix the other cases first and then see with an evaluation how often this
remaining case really happens.

Another case of another non-standard commit reference is that when I refer
to a commit with a broken title that still contains "[PATCH] xyz: do
something" in the git history, I simply refer to it only with commit
abcdabcdabcd ("xyz: do something") and I drop the [PATCH] prefix.

That is a non-standard way but I think it should be accepted as standard or
at least acceptable to checkpatch. Again, that also needs some discussion
with more kernel developers.

Lukas


> Thanks,
> Dwaipayan.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linuxfoundation.org/pipermail/linux-kernel-mentees/attachments/20201110/0918b905/attachment.html>