<div dir="ltr"><div><br></div><div><br><div class="gmail_quote"><div dir="auto">On Di., 10. Nov. 2020 at 19:41, Dwaipayan Ray <<a href="mailto:dwaipayanray1@gmail.com" target="_blank">dwaipayanray1@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
I was looking into GIT_COMMIT_ID warnings for checkpatch<br>
and certainly there are false positives which needs to be fixed<br>
as also said by a previous discussion.<br>
<br>
But before fixing it, I would like your comments on what should<br>
be fixed and what shouldn't considering the added code can<br>
be complex.<br>
<br>
The data below is an evaluation of about 50k commits from v5.4.<br>
<br>
$ cat checkpatch_out_50k.txt | grep -A3 "GIT_COMMIT_ID" \<br>
   | grep "References" | wc -l<br>
45</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
First of all there are 45 warnings of type:<br>
References: 2248a28384fe ("drm/i915/gen8+: Add RC6 CTX corruption WA")<br>
References: 860afa086841 ("drm/i915/gt: Flush gen7 even harder"), etc.<br>
<br>
Should references be simply exempt from this check? I don't<br>
know much about it. But seems checkpatch is incorrect here.<br>
</blockquote><div dir="auto"><br></div><div dir="auto">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.</div><div dir="auto"><br></div><div dir="auto">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.</div><div dir="auto"><br></div><div dir="auto">Do you want to do that? Expect some heavy discussion and some bikeshedding.</div><div dir="auto"><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
For other messages,<br>
$ cat checkpatch_out_50k.txt | grep -A3 "GIT_COMMIT_ID"  | \<br>
  grep -v "GIT_COMMIT_ID" |     grep -P "^\s*[a-z0-9]{12,}" | wc -l<br>
468<br>
<br>
There were 468 missing "commit" word before the GIT SHA. Some<br>
of these were due to split lines. I think that could be fixed with some<br>
effort.<br>
<br>
And finally the other fixable warning is for those with split commit<br>
titles itself.<br>
</blockquote><div dir="auto"><br></div><div dir="auto">Yes, you saw that another mentee started this work but could not really finish that (yet?), right?</div><div dir="auto"><br></div><div>This sounds like a good case.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
Other than that, there was also this particular case with non escaped<br>
quotes within the commit title.<br>
Like:<br>
commit 97a32539b956 ("proc: convert everything to "struct proc_ops"")<br>
Should this be handled as well. Or is this a human error? I am not quite<br>
sure.<br>
</blockquote><div dir="auto"><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>Lukas</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
Thanks,<br>
Dwaipayan.<br>
</blockquote></div></div>
</div>