1. Patchew
  2. QEMU
  3. View series

[PATCH v2] scripts/checkpatch.pl: check for placeholders in cover letter patches

Manos Pitsidianakis posted 1 patch 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240130101107.214872-1-manos.pitsidianakis@linaro.org
scripts/checkpatch.pl | 14 ++++++++++++++
1 file changed, 14 insertions(+)
[PATCH v2] scripts/checkpatch.pl: check for placeholders in cover letter patches
Posted by Manos Pitsidianakis 10 months ago 
Check if a file argument is a cover letter patch produced by
git-format-patch --cover-letter; It is initialized with subject suffix "
*** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***". If they
exist, warn the user.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
Range-diff against v1:
1:  64b7ec2287 ! 1:  9bf816eb4c scripts/checkpatch.pl: check for placeholders in cover letter patches
    @@ scripts/checkpatch.pl: sub process {
     +# --cover-letter; It is initialized with subject suffix
     +# " *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***"
     +		if ($in_header_lines &&
    -+		    $rawline =~ /^Subject:.+[*]{3} SUBJECT HERE [*]{3}\s*$/) {
    -+        WARN("Patch appears to be a cover letter with uninitialized subject" .
    -+             " '*** SUBJECT HERE ***'\n$hereline\n");
    ++				$rawline =~ /^Subject:.+[*]{3} SUBJECT HERE [*]{3}\s*$/) {
    ++			WARN("Patch appears to be a cover letter with " .
    ++						"uninitialized subject '*** SUBJECT HERE ***'\n$hereline\n");
     +		}
     +
     +		if ($rawline =~ /^[*]{3} BLURB HERE [*]{3}\s*$/) {
    -+        WARN("Patch appears to be a cover letter with leftover placeholder " .
    -+             "text '*** BLURB HERE ***'\n$hereline\n");
    ++			WARN("Patch appears to be a cover letter with " .
    ++						"leftover placeholder text '*** BLURB HERE ***'\n$hereline\n");
     +		}
     +
      		if ($in_commit_log && $non_utf8_charset && $realfile =~ /^$/ &&

 scripts/checkpatch.pl | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7026895074..9a8d49f1d8 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1650,6 +1650,20 @@ sub process {
 			$non_utf8_charset = 1;
 		}
 
+# Check if this is a cover letter patch produced by git-format-patch
+# --cover-letter; It is initialized with subject suffix
+# " *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***"
+		if ($in_header_lines &&
+				$rawline =~ /^Subject:.+[*]{3} SUBJECT HERE [*]{3}\s*$/) {
+			WARN("Patch appears to be a cover letter with " .
+						"uninitialized subject '*** SUBJECT HERE ***'\n$hereline\n");
+		}
+
+		if ($rawline =~ /^[*]{3} BLURB HERE [*]{3}\s*$/) {
+			WARN("Patch appears to be a cover letter with " .
+						"leftover placeholder text '*** BLURB HERE ***'\n$hereline\n");
+		}
+
 		if ($in_commit_log && $non_utf8_charset && $realfile =~ /^$/ &&
 		    $rawline =~ /$NON_ASCII_UTF8/) {
 			WARN("8-bit UTF-8 used in possible commit log\n" . $herecurr);

base-commit: 11be70677c70fdccd452a3233653949b79e97908
-- 
γαῖα πυρί μιχθήτω
Re: [PATCH v2] scripts/checkpatch.pl: check for placeholders in cover letter patches
Posted by Peter Maydell 10 months ago 
On Tue, 30 Jan 2024 at 10:11, Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
>
> Check if a file argument is a cover letter patch produced by
> git-format-patch --cover-letter; It is initialized with subject suffix "
> *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***". If they
> exist, warn the user.

FWIW, as far as I can see from my email archive, this particular
mistake has been made by contributors to qemu-devel perhaps
half a dozen times at most in the last decade...

thanks
-- PMM
Re: [PATCH v2] scripts/checkpatch.pl: check for placeholders in cover letter patches
Posted by Manos Pitsidianakis 10 months ago 
On Tue, 30 Jan 2024 at 12:34, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 30 Jan 2024 at 10:11, Manos Pitsidianakis
> <manos.pitsidianakis@linaro.org> wrote:
> >
> > Check if a file argument is a cover letter patch produced by
> > git-format-patch --cover-letter; It is initialized with subject suffix "
> > *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***". If they
> > exist, warn the user.
>
> FWIW, as far as I can see from my email archive, this particular
> mistake has been made by contributors to qemu-devel perhaps
> half a dozen times at most in the last decade...
>
> thanks
> -- PMM

Peter, searching for `b:"BLURB HERE"` in lore.kernel.org yields about
170 results including these patches.

https://lore.kernel.org/qemu-devel/?q=b%3A%22BLURB+HERE%22
Re: [PATCH v2] scripts/checkpatch.pl: check for placeholders in cover letter patches
Posted by Peter Maydell 10 months ago 
On Tue, 30 Jan 2024 at 10:39, Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
>
> On Tue, 30 Jan 2024 at 12:34, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Tue, 30 Jan 2024 at 10:11, Manos Pitsidianakis
> > <manos.pitsidianakis@linaro.org> wrote:
> > >
> > > Check if a file argument is a cover letter patch produced by
> > > git-format-patch --cover-letter; It is initialized with subject suffix "
> > > *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***". If they
> > > exist, warn the user.
> >
> > FWIW, as far as I can see from my email archive, this particular
> > mistake has been made by contributors to qemu-devel perhaps
> > half a dozen times at most in the last decade...
> >
> > thanks
> > -- PMM
>
> Peter, searching for `b:"BLURB HERE"` in lore.kernel.org yields about
> 170 results including these patches.
>
> https://lore.kernel.org/qemu-devel/?q=b%3A%22BLURB+HERE%22

Yes, there's a few more 'blurb here' results than 'subject here'
results, but they're almost always just where the submitter did
provide a proper blurb but then forgot to delete the 'BLURB HERE'
line, rather than where there's no blurb at all.

thanks
-- PMM
Re: [PATCH v2] scripts/checkpatch.pl: check for placeholders in cover letter patches
Posted by Manos Pitsidianakis 10 months ago 
On Tue, 30 Jan 2024 at 12:42, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 30 Jan 2024 at 10:39, Manos Pitsidianakis
> <manos.pitsidianakis@linaro.org> wrote:
> >
> > On Tue, 30 Jan 2024 at 12:34, Peter Maydell <peter.maydell@linaro.org> wrote:
> > >
> > > On Tue, 30 Jan 2024 at 10:11, Manos Pitsidianakis
> > > <manos.pitsidianakis@linaro.org> wrote:
> > > >
> > > > Check if a file argument is a cover letter patch produced by
> > > > git-format-patch --cover-letter; It is initialized with subject suffix "
> > > > *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***". If they
> > > > exist, warn the user.
> > >
> > > FWIW, as far as I can see from my email archive, this particular
> > > mistake has been made by contributors to qemu-devel perhaps
> > > half a dozen times at most in the last decade...
> > >
> > > thanks
> > > -- PMM
> >
> > Peter, searching for `b:"BLURB HERE"` in lore.kernel.org yields about
> > 170 results including these patches.
> >
> > https://lore.kernel.org/qemu-devel/?q=b%3A%22BLURB+HERE%22
>
> Yes, there's a few more 'blurb here' results than 'subject here'
> results, but they're almost always just where the submitter did
> provide a proper blurb but then forgot to delete the 'BLURB HERE'
> line, rather than where there's no blurb at all.

Though you said half a dozen times at most.

In general the only comments so far are examples of "moving the
goalposts" fallacy, where the argument changes each time and the
discussion changes topic every time.

https://en.wikipedia.org/wiki/Moving_the_goalposts

I know it's not anyone's intention in this case, but I'd like to
remind everyone that this can be perceived negatively by contributors
and demotivate them from contributing to QEMU at all. Let's keep the
discussion constructive instead of dismissive. I say this in a
completely friendly manner, no negativity intended.

Manos
Re: [PATCH v2] scripts/checkpatch.pl: check for placeholders in cover letter patches
Posted by Peter Maydell 10 months ago 
On Tue, 30 Jan 2024 at 10:51, Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
>
> On Tue, 30 Jan 2024 at 12:42, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Tue, 30 Jan 2024 at 10:39, Manos Pitsidianakis
> > <manos.pitsidianakis@linaro.org> wrote:
> > >
> > > On Tue, 30 Jan 2024 at 12:34, Peter Maydell <peter.maydell@linaro.org> wrote:
> > > >
> > > > On Tue, 30 Jan 2024 at 10:11, Manos Pitsidianakis
> > > > <manos.pitsidianakis@linaro.org> wrote:
> > > > >
> > > > > Check if a file argument is a cover letter patch produced by
> > > > > git-format-patch --cover-letter; It is initialized with subject suffix "
> > > > > *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***". If they
> > > > > exist, warn the user.
> > > >
> > > > FWIW, as far as I can see from my email archive, this particular
> > > > mistake has been made by contributors to qemu-devel perhaps
> > > > half a dozen times at most in the last decade...
> > > >
> > > > thanks
> > > > -- PMM
> > >
> > > Peter, searching for `b:"BLURB HERE"` in lore.kernel.org yields about
> > > 170 results including these patches.
> > >
> > > https://lore.kernel.org/qemu-devel/?q=b%3A%22BLURB+HERE%22
> >
> > Yes, there's a few more 'blurb here' results than 'subject here'
> > results, but they're almost always just where the submitter did
> > provide a proper blurb but then forgot to delete the 'BLURB HERE'
> > line, rather than where there's no blurb at all.
>
> Though you said half a dozen times at most.

Yes, because I was counting 'subject here'.

My question here is really "how much do we care about having
checkpatch point out this error?".

thanks
-- PMM
Re: [PATCH v2] scripts/checkpatch.pl: check for placeholders in cover letter patches
Posted by Manos Pitsidianakis 10 months ago 
On Tue, 30 Jan 2024 at 12:57, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 30 Jan 2024 at 10:51, Manos Pitsidianakis
> <manos.pitsidianakis@linaro.org> wrote:
> >
> > On Tue, 30 Jan 2024 at 12:42, Peter Maydell <peter.maydell@linaro.org> wrote:
> > >
> > > On Tue, 30 Jan 2024 at 10:39, Manos Pitsidianakis
> > > <manos.pitsidianakis@linaro.org> wrote:
> > > >
> > > > On Tue, 30 Jan 2024 at 12:34, Peter Maydell <peter.maydell@linaro.org> wrote:
> > > > >
> > > > > On Tue, 30 Jan 2024 at 10:11, Manos Pitsidianakis
> > > > > <manos.pitsidianakis@linaro.org> wrote:
> > > > > >
> > > > > > Check if a file argument is a cover letter patch produced by
> > > > > > git-format-patch --cover-letter; It is initialized with subject suffix "
> > > > > > *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***". If they
> > > > > > exist, warn the user.
> > > > >
> > > > > FWIW, as far as I can see from my email archive, this particular
> > > > > mistake has been made by contributors to qemu-devel perhaps
> > > > > half a dozen times at most in the last decade...
> > > > >
> > > > > thanks
> > > > > -- PMM
> > > >
> > > > Peter, searching for `b:"BLURB HERE"` in lore.kernel.org yields about
> > > > 170 results including these patches.
> > > >
> > > > https://lore.kernel.org/qemu-devel/?q=b%3A%22BLURB+HERE%22
> > >
> > > Yes, there's a few more 'blurb here' results than 'subject here'
> > > results, but they're almost always just where the submitter did
> > > provide a proper blurb but then forgot to delete the 'BLURB HERE'
> > > line, rather than where there's no blurb at all.
> >
> > Though you said half a dozen times at most.
>
> Yes, because I was counting 'subject here'.
>
> My question here is really "how much do we care about having
> checkpatch point out this error?".
>
> thanks
> -- PMM

I do, because it gives some peace of mind. But I do not care so much
that I'd want to continue this conversation further.

-- 
Manos Pitsidianakis
Emulation and Virtualization Engineer at Linaro Ltd
Re: [PATCH v2] scripts/checkpatch.pl: check for placeholders in cover letter patches
Posted by Philippe Mathieu-Daudé 10 months ago 
Hi Manos,

On 30/1/24 12:02, Manos Pitsidianakis wrote:
> On Tue, 30 Jan 2024 at 12:57, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Tue, 30 Jan 2024 at 10:51, Manos Pitsidianakis
>> <manos.pitsidianakis@linaro.org> wrote:
>>>
>>> On Tue, 30 Jan 2024 at 12:42, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>
>>>> On Tue, 30 Jan 2024 at 10:39, Manos Pitsidianakis
>>>> <manos.pitsidianakis@linaro.org> wrote:
>>>>>
>>>>> On Tue, 30 Jan 2024 at 12:34, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>>>
>>>>>> On Tue, 30 Jan 2024 at 10:11, Manos Pitsidianakis
>>>>>> <manos.pitsidianakis@linaro.org> wrote:
>>>>>>>
>>>>>>> Check if a file argument is a cover letter patch produced by
>>>>>>> git-format-patch --cover-letter; It is initialized with subject suffix "
>>>>>>> *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***". If they
>>>>>>> exist, warn the user.
>>>>>>
>>>>>> FWIW, as far as I can see from my email archive, this particular
>>>>>> mistake has been made by contributors to qemu-devel perhaps
>>>>>> half a dozen times at most in the last decade...
>>>>>>
>>>>>> thanks
>>>>>> -- PMM
>>>>>
>>>>> Peter, searching for `b:"BLURB HERE"` in lore.kernel.org yields about
>>>>> 170 results including these patches.
>>>>>
>>>>> https://lore.kernel.org/qemu-devel/?q=b%3A%22BLURB+HERE%22

This comment is the default --blurb-template from git-publish:
https://github.com/stefanha/git-publish/blob/master/git-publish#L742
As the tool is also used to post patches to other projects, I'd
recommend fixing it there at the source.

Somewhere between the pre-publish-send-email and git_send_email()
calls you could check cover_letter_path doesn't contain the default
blurb.

>>>> Yes, there's a few more 'blurb here' results than 'subject here'
>>>> results, but they're almost always just where the submitter did
>>>> provide a proper blurb but then forgot to delete the 'BLURB HERE'
>>>> line, rather than where there's no blurb at all.
>>>
>>> Though you said half a dozen times at most.
>>
>> Yes, because I was counting 'subject here'.
>>
>> My question here is really "how much do we care about having
>> checkpatch point out this error?".
>>
>> thanks
>> -- PMM
> 
> I do, because it gives some peace of mind. But I do not care so much
> that I'd want to continue this conversation further.
>
Re: [PATCH v2] scripts/checkpatch.pl: check for placeholders in cover letter patches
Posted by Peter Maydell 10 months ago 
On Tue, 30 Jan 2024 at 11:24, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Manos,
>
> On 30/1/24 12:02, Manos Pitsidianakis wrote:
> > On Tue, 30 Jan 2024 at 12:57, Peter Maydell <peter.maydell@linaro.org> wrote:
> >>
> >> On Tue, 30 Jan 2024 at 10:51, Manos Pitsidianakis
> >> <manos.pitsidianakis@linaro.org> wrote:
> >>>
> >>> On Tue, 30 Jan 2024 at 12:42, Peter Maydell <peter.maydell@linaro.org> wrote:
> >>>>
> >>>> On Tue, 30 Jan 2024 at 10:39, Manos Pitsidianakis
> >>>> <manos.pitsidianakis@linaro.org> wrote:
> >>>>>
> >>>>> On Tue, 30 Jan 2024 at 12:34, Peter Maydell <peter.maydell@linaro.org> wrote:
> >>>>>>
> >>>>>> On Tue, 30 Jan 2024 at 10:11, Manos Pitsidianakis
> >>>>>> <manos.pitsidianakis@linaro.org> wrote:
> >>>>>>>
> >>>>>>> Check if a file argument is a cover letter patch produced by
> >>>>>>> git-format-patch --cover-letter; It is initialized with subject suffix "
> >>>>>>> *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***". If they
> >>>>>>> exist, warn the user.
> >>>>>>
> >>>>>> FWIW, as far as I can see from my email archive, this particular
> >>>>>> mistake has been made by contributors to qemu-devel perhaps
> >>>>>> half a dozen times at most in the last decade...
> >>>>>>
> >>>>>> thanks
> >>>>>> -- PMM
> >>>>>
> >>>>> Peter, searching for `b:"BLURB HERE"` in lore.kernel.org yields about
> >>>>> 170 results including these patches.
> >>>>>
> >>>>> https://lore.kernel.org/qemu-devel/?q=b%3A%22BLURB+HERE%22
>
> This comment is the default --blurb-template from git-publish:
> https://github.com/stefanha/git-publish/blob/master/git-publish#L742
> As the tool is also used to post patches to other projects, I'd
> recommend fixing it there at the source.

It's also in the general 'git format-patch' cover letter template,
where the workflow is supposed to be "produce cover letter template,
manually edit it, send it". Stray template markers generally are
the result of (a) a new contributor not knowing about the 'edit'
step or (b) remembering to add the subject and blurb but forgetting
to delete the 'blurb' template line so it gets left in at the
bottom of the cover letter. So I think it is a check that is within
checkpatch.pl's remit.

thanks
-- PMM
Re: [PATCH v2] scripts/checkpatch.pl: check for placeholders in cover letter patches
Posted by Philippe Mathieu-Daudé 10 months ago 
On 30/1/24 12:30, Peter Maydell wrote:
> On Tue, 30 Jan 2024 at 11:24, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Hi Manos,
>>
>> On 30/1/24 12:02, Manos Pitsidianakis wrote:
>>> On Tue, 30 Jan 2024 at 12:57, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>
>>>> On Tue, 30 Jan 2024 at 10:51, Manos Pitsidianakis
>>>> <manos.pitsidianakis@linaro.org> wrote:
>>>>>
>>>>> On Tue, 30 Jan 2024 at 12:42, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>>>
>>>>>> On Tue, 30 Jan 2024 at 10:39, Manos Pitsidianakis
>>>>>> <manos.pitsidianakis@linaro.org> wrote:
>>>>>>>
>>>>>>> On Tue, 30 Jan 2024 at 12:34, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>>>>>
>>>>>>>> On Tue, 30 Jan 2024 at 10:11, Manos Pitsidianakis
>>>>>>>> <manos.pitsidianakis@linaro.org> wrote:
>>>>>>>>>
>>>>>>>>> Check if a file argument is a cover letter patch produced by
>>>>>>>>> git-format-patch --cover-letter; It is initialized with subject suffix "
>>>>>>>>> *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***". If they
>>>>>>>>> exist, warn the user.
>>>>>>>>
>>>>>>>> FWIW, as far as I can see from my email archive, this particular
>>>>>>>> mistake has been made by contributors to qemu-devel perhaps
>>>>>>>> half a dozen times at most in the last decade...
>>>>>>>>
>>>>>>>> thanks
>>>>>>>> -- PMM
>>>>>>>
>>>>>>> Peter, searching for `b:"BLURB HERE"` in lore.kernel.org yields about
>>>>>>> 170 results including these patches.
>>>>>>>
>>>>>>> https://lore.kernel.org/qemu-devel/?q=b%3A%22BLURB+HERE%22
>>
>> This comment is the default --blurb-template from git-publish:
>> https://github.com/stefanha/git-publish/blob/master/git-publish#L742
>> As the tool is also used to post patches to other projects, I'd
>> recommend fixing it there at the source.
> 
> It's also in the general 'git format-patch' cover letter template,
> where the workflow is supposed to be "produce cover letter template,
> manually edit it, send it". Stray template markers generally are
> the result of (a) a new contributor not knowing about the 'edit'
> step or (b) remembering to add the subject and blurb but forgetting
> to delete the 'blurb' template line so it gets left in at the
> bottom of the cover letter. So I think it is a check that is within
> checkpatch.pl's remit.

Oh, it is so long since the last time I used git-format-patch
manually that I thought this template was a git-publish feature :)
Re: [PATCH v2] scripts/checkpatch.pl: check for placeholders in cover letter patches
Posted by Daniel P. Berrangé 10 months ago 
On Tue, Jan 30, 2024 at 12:11:07PM +0200, Manos Pitsidianakis wrote:
> Check if a file argument is a cover letter patch produced by
> git-format-patch --cover-letter; It is initialized with subject suffix "
> *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***". If they
> exist, warn the user.
> 
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
> Range-diff against v1:
> 1:  64b7ec2287 ! 1:  9bf816eb4c scripts/checkpatch.pl: check for placeholders in cover letter patches
>     @@ scripts/checkpatch.pl: sub process {
>      +# --cover-letter; It is initialized with subject suffix
>      +# " *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***"
>      +		if ($in_header_lines &&
>     -+		    $rawline =~ /^Subject:.+[*]{3} SUBJECT HERE [*]{3}\s*$/) {
>     -+        WARN("Patch appears to be a cover letter with uninitialized subject" .
>     -+             " '*** SUBJECT HERE ***'\n$hereline\n");
>     ++				$rawline =~ /^Subject:.+[*]{3} SUBJECT HERE [*]{3}\s*$/) {
>     ++			WARN("Patch appears to be a cover letter with " .
>     ++						"uninitialized subject '*** SUBJECT HERE ***'\n$hereline\n");
>      +		}
>      +
>      +		if ($rawline =~ /^[*]{3} BLURB HERE [*]{3}\s*$/) {
>     -+        WARN("Patch appears to be a cover letter with leftover placeholder " .
>     -+             "text '*** BLURB HERE ***'\n$hereline\n");
>     ++			WARN("Patch appears to be a cover letter with " .
>     ++						"leftover placeholder text '*** BLURB HERE ***'\n$hereline\n");
>      +		}
>      +
>       		if ($in_commit_log && $non_utf8_charset && $realfile =~ /^$/ &&
> 
>  scripts/checkpatch.pl | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 7026895074..9a8d49f1d8 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1650,6 +1650,20 @@ sub process {
>  			$non_utf8_charset = 1;
>  		}
>  
> +# Check if this is a cover letter patch produced by git-format-patch
> +# --cover-letter; It is initialized with subject suffix
> +# " *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***"
> +		if ($in_header_lines &&
> +				$rawline =~ /^Subject:.+[*]{3} SUBJECT HERE [*]{3}\s*$/) {

This continuation line is now hugely over-indented - it should
be aligned just after the '('

> +			WARN("Patch appears to be a cover letter with " .
> +						"uninitialized subject '*** SUBJECT HERE ***'\n$hereline\n");

As is this

> +		}
> +
> +		if ($rawline =~ /^[*]{3} BLURB HERE [*]{3}\s*$/) {
> +			WARN("Patch appears to be a cover letter with " .
> +						"leftover placeholder text '*** BLURB HERE ***'\n$hereline\n");

And this.

> +		}
> +
>  		if ($in_commit_log && $non_utf8_charset && $realfile =~ /^$/ &&
>  		    $rawline =~ /$NON_ASCII_UTF8/) {
>  			WARN("8-bit UTF-8 used in possible commit log\n" . $herecurr);
> 
> base-commit: 11be70677c70fdccd452a3233653949b79e97908
> -- 
> γαῖα πυρί μιχθήτω
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH v2] scripts/checkpatch.pl: check for placeholders in cover letter patches
Posted by Manos Pitsidianakis 10 months ago 
On Tue, 30 Jan 2024 12:15, "Daniel P. Berrangé" <berrange@redhat.com> wrote:
>On Tue, Jan 30, 2024 at 12:11:07PM +0200, Manos Pitsidianakis wrote:
>> Check if a file argument is a cover letter patch produced by
>> git-format-patch --cover-letter; It is initialized with subject suffix "
>> *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***". If they
>> exist, warn the user.
>> 
>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>> ---
>> Range-diff against v1:
>> 1:  64b7ec2287 ! 1:  9bf816eb4c scripts/checkpatch.pl: check for placeholders in cover letter patches
>>     @@ scripts/checkpatch.pl: sub process {
>>      +# --cover-letter; It is initialized with subject suffix
>>      +# " *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***"
>>      +		if ($in_header_lines &&
>>     -+		    $rawline =~ /^Subject:.+[*]{3} SUBJECT HERE [*]{3}\s*$/) {
>>     -+        WARN("Patch appears to be a cover letter with uninitialized subject" .
>>     -+             " '*** SUBJECT HERE ***'\n$hereline\n");
>>     ++				$rawline =~ /^Subject:.+[*]{3} SUBJECT HERE [*]{3}\s*$/) {
>>     ++			WARN("Patch appears to be a cover letter with " .
>>     ++						"uninitialized subject '*** SUBJECT HERE ***'\n$hereline\n");
>>      +		}
>>      +
>>      +		if ($rawline =~ /^[*]{3} BLURB HERE [*]{3}\s*$/) {
>>     -+        WARN("Patch appears to be a cover letter with leftover placeholder " .
>>     -+             "text '*** BLURB HERE ***'\n$hereline\n");
>>     ++			WARN("Patch appears to be a cover letter with " .
>>     ++						"leftover placeholder text '*** BLURB HERE ***'\n$hereline\n");
>>      +		}
>>      +
>>       		if ($in_commit_log && $non_utf8_charset && $realfile =~ /^$/ &&
>> 
>>  scripts/checkpatch.pl | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>> 
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index 7026895074..9a8d49f1d8 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -1650,6 +1650,20 @@ sub process {
>>  			$non_utf8_charset = 1;
>>  		}
>>  
>> +# Check if this is a cover letter patch produced by git-format-patch
>> +# --cover-letter; It is initialized with subject suffix
>> +# " *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***"
>> +		if ($in_header_lines &&
>> +				$rawline =~ /^Subject:.+[*]{3} SUBJECT HERE [*]{3}\s*$/) {
>
>This continuation line is now hugely over-indented - it should
>be aligned just after the '('

It is not, it just uses tabs. Like line 2693 in current master:

https://gitlab.com/qemu-project/qemu/-/blob/11be70677c70fdccd452a3233653949b79e97908/scripts/checkpatch.pl#L2693

I will quote the **QEMU Coding Style** again on whitespace:

> Whitespace
> 
> Of course, the most important aspect in any coding style is whitespace. 
> Crusty old coders who have trouble spotting the glasses on their noses 
> can tell the difference between a tab and eight spaces from a distance 
> of approximately fifteen parsecs. Many a flamewar has been fought and 
> lost on this issue.

> QEMU indents are four spaces. Tabs are never used, except in Makefiles 
> where they have been irreversibly coded into the syntax. Spaces of 
> course are superior to tabs because:
> 
>     You have just one way to specify whitespace, not two. Ambiguity breeds mistakes.
> 
>     The confusion surrounding ‘use tabs to indent, spaces to justify’ is gone.
> 
>     Tab indents push your code to the right, making your screen seriously unbalanced.
> 
>     Tabs will be rendered incorrectly on editors who are misconfigured not to use tab stops of eight positions.
> 
>     Tabs are rendered badly in patches, causing off-by-one errors in almost every line.
> 
>    It is the QEMU coding style.

I think it's better if we leave this discussion here, and accept v1 
which is consistent with the coding style, or this one which is 
consistent with the inconsistency of the tabs and spaces mix of the 
checkpatch.pl source code as a compromise, if it is deemed important.

Thanks,
Manos



>
>> +			WARN("Patch appears to be a cover letter with " .
>> +						"uninitialized subject '*** SUBJECT HERE ***'\n$hereline\n");
>
>As is this
>
>> +		}
>> +
>> +		if ($rawline =~ /^[*]{3} BLURB HERE [*]{3}\s*$/) {
>> +			WARN("Patch appears to be a cover letter with " .
>> +						"leftover placeholder text '*** BLURB HERE ***'\n$hereline\n");
>
>And this.
>
>> +		}
>> +
>>  		if ($in_commit_log && $non_utf8_charset && $realfile =~ /^$/ &&
>>  		    $rawline =~ /$NON_ASCII_UTF8/) {
>>  			WARN("8-bit UTF-8 used in possible commit log\n" . $herecurr);
>> 
>> base-commit: 11be70677c70fdccd452a3233653949b79e97908
>> -- 
>> γαῖα πυρί μιχθήτω
>> 
>
>With regards,
>Daniel
>-- 
>|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>
Re: [PATCH v2] scripts/checkpatch.pl: check for placeholders in cover letter patches
Posted by Alex Bennée 10 months ago 
Manos Pitsidianakis <manos.pitsidianakis@linaro.org> writes:

> On Tue, 30 Jan 2024 12:15, "Daniel P. Berrangé" <berrange@redhat.com> wrote:
>>On Tue, Jan 30, 2024 at 12:11:07PM +0200, Manos Pitsidianakis wrote:
>>> Check if a file argument is a cover letter patch produced by
>>> git-format-patch --cover-letter; It is initialized with subject suffix "
>>> *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***". If they
>>> exist, warn the user.
>>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>>> ---
>>> Range-diff against v1:
>>> 1:  64b7ec2287 ! 1:  9bf816eb4c scripts/checkpatch.pl: check for placeholders in cover letter patches
>>>     @@ scripts/checkpatch.pl: sub process {
>>>      +# --cover-letter; It is initialized with subject suffix
>>>      +# " *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***"
>>>      +		if ($in_header_lines &&
>>>     -+		    $rawline =~ /^Subject:.+[*]{3} SUBJECT HERE [*]{3}\s*$/) {
>>>     -+        WARN("Patch appears to be a cover letter with uninitialized subject" .
>>>     -+             " '*** SUBJECT HERE ***'\n$hereline\n");
>>>     ++				$rawline =~ /^Subject:.+[*]{3} SUBJECT HERE [*]{3}\s*$/) {
>>>     ++			WARN("Patch appears to be a cover letter with " .
>>>     ++						"uninitialized subject '*** SUBJECT HERE ***'\n$hereline\n");
>>>      +		}
>>>      +
>>>      +		if ($rawline =~ /^[*]{3} BLURB HERE [*]{3}\s*$/) {
>>>     -+        WARN("Patch appears to be a cover letter with leftover placeholder " .
>>>     -+             "text '*** BLURB HERE ***'\n$hereline\n");
>>>     ++			WARN("Patch appears to be a cover letter with " .
>>>     ++						"leftover placeholder text '*** BLURB HERE ***'\n$hereline\n");
>>>      +		}
>>>      +
>>>       		if ($in_commit_log && $non_utf8_charset && $realfile =~ /^$/ &&
>>>  scripts/checkpatch.pl | 14 ++++++++++++++
>>>  1 file changed, 14 insertions(+)
>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>> index 7026895074..9a8d49f1d8 100755
>>> --- a/scripts/checkpatch.pl
>>> +++ b/scripts/checkpatch.pl
>>> @@ -1650,6 +1650,20 @@ sub process {
>>>  			$non_utf8_charset = 1;
>>>  		}
>>>  +# Check if this is a cover letter patch produced by
>>> git-format-patch
>>> +# --cover-letter; It is initialized with subject suffix
>>> +# " *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***"
>>> +		if ($in_header_lines &&
>>> +				$rawline =~ /^Subject:.+[*]{3} SUBJECT HERE [*]{3}\s*$/) {
>>
>>This continuation line is now hugely over-indented - it should
>>be aligned just after the '('
>
> It is not, it just uses tabs. Like line 2693 in current master:
>
> https://gitlab.com/qemu-project/qemu/-/blob/11be70677c70fdccd452a3233653949b79e97908/scripts/checkpatch.pl#L2693
>
> I will quote the **QEMU Coding Style** again on whitespace:
>
>> Whitespace
>> Of course, the most important aspect in any coding style is
>> whitespace. Crusty old coders who have trouble spotting the glasses
>> on their noses can tell the difference between a tab and eight
>> spaces from a distance of approximately fifteen parsecs. Many a
>> flamewar has been fought and lost on this issue.
>
>> QEMU indents are four spaces. Tabs are never used, except in
>> Makefiles where they have been irreversibly coded into the syntax.
>> Spaces of course are superior to tabs because:
>>     You have just one way to specify whitespace, not two. Ambiguity
>> breeds mistakes.
>>     The confusion surrounding ‘use tabs to indent, spaces to
>> justify’ is gone.
>>     Tab indents push your code to the right, making your screen
>> seriously unbalanced.
>>     Tabs will be rendered incorrectly on editors who are
>> misconfigured not to use tab stops of eight positions.
>>     Tabs are rendered badly in patches, causing off-by-one errors in
>> almost every line.
>>    It is the QEMU coding style.
>
> I think it's better if we leave this discussion here, and accept v1
> which is consistent with the coding style, or this one which is
> consistent with the inconsistency of the tabs and spaces mix of the
> checkpatch.pl source code as a compromise, if it is deemed important.

I suspect the problem is that checkpatch.pl is an import from the Linux
source tree which has since had syncs with its upstream as well as a
slew of QEMU specific patches. If we don't care about tracking upstream
anymore we could bite the bullet and fix indentation going forward.

Of course arguably we should replace it with a python script and reduce
our dependence on perl. I'm sure someone had a go at that once but it
might have only been a partial undertaking.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro
Re: [PATCH v2] scripts/checkpatch.pl: check for placeholders in cover letter patches
Posted by Philippe Mathieu-Daudé 10 months ago 
On 30/1/24 16:11, Alex Bennée wrote:
> Manos Pitsidianakis <manos.pitsidianakis@linaro.org> writes:
> 
>> On Tue, 30 Jan 2024 12:15, "Daniel P. Berrangé" <berrange@redhat.com> wrote:
>>> On Tue, Jan 30, 2024 at 12:11:07PM +0200, Manos Pitsidianakis wrote:
>>>> Check if a file argument is a cover letter patch produced by
>>>> git-format-patch --cover-letter; It is initialized with subject suffix "
>>>> *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***". If they
>>>> exist, warn the user.
>>>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>>>> ---
>>>> Range-diff against v1:
>>>> 1:  64b7ec2287 ! 1:  9bf816eb4c scripts/checkpatch.pl: check for placeholders in cover letter patches
>>>>      @@ scripts/checkpatch.pl: sub process {
>>>>       +# --cover-letter; It is initialized with subject suffix
>>>>       +# " *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***"
>>>>       +		if ($in_header_lines &&
>>>>      -+		    $rawline =~ /^Subject:.+[*]{3} SUBJECT HERE [*]{3}\s*$/) {
>>>>      -+        WARN("Patch appears to be a cover letter with uninitialized subject" .
>>>>      -+             " '*** SUBJECT HERE ***'\n$hereline\n");
>>>>      ++				$rawline =~ /^Subject:.+[*]{3} SUBJECT HERE [*]{3}\s*$/) {
>>>>      ++			WARN("Patch appears to be a cover letter with " .
>>>>      ++						"uninitialized subject '*** SUBJECT HERE ***'\n$hereline\n");
>>>>       +		}
>>>>       +
>>>>       +		if ($rawline =~ /^[*]{3} BLURB HERE [*]{3}\s*$/) {
>>>>      -+        WARN("Patch appears to be a cover letter with leftover placeholder " .
>>>>      -+             "text '*** BLURB HERE ***'\n$hereline\n");
>>>>      ++			WARN("Patch appears to be a cover letter with " .
>>>>      ++						"leftover placeholder text '*** BLURB HERE ***'\n$hereline\n");
>>>>       +		}
>>>>       +
>>>>        		if ($in_commit_log && $non_utf8_charset && $realfile =~ /^$/ &&
>>>>   scripts/checkpatch.pl | 14 ++++++++++++++
>>>>   1 file changed, 14 insertions(+)
>>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>>> index 7026895074..9a8d49f1d8 100755
>>>> --- a/scripts/checkpatch.pl
>>>> +++ b/scripts/checkpatch.pl
>>>> @@ -1650,6 +1650,20 @@ sub process {
>>>>   			$non_utf8_charset = 1;
>>>>   		}
>>>>   +# Check if this is a cover letter patch produced by
>>>> git-format-patch
>>>> +# --cover-letter; It is initialized with subject suffix
>>>> +# " *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***"
>>>> +		if ($in_header_lines &&
>>>> +				$rawline =~ /^Subject:.+[*]{3} SUBJECT HERE [*]{3}\s*$/) {
>>>
>>> This continuation line is now hugely over-indented - it should
>>> be aligned just after the '('
>>
>> It is not, it just uses tabs. Like line 2693 in current master:
>>
>> https://gitlab.com/qemu-project/qemu/-/blob/11be70677c70fdccd452a3233653949b79e97908/scripts/checkpatch.pl#L2693
>>
>> I will quote the **QEMU Coding Style** again on whitespace:
>>
>>> Whitespace
>>> Of course, the most important aspect in any coding style is
>>> whitespace. Crusty old coders who have trouble spotting the glasses
>>> on their noses can tell the difference between a tab and eight
>>> spaces from a distance of approximately fifteen parsecs. Many a
>>> flamewar has been fought and lost on this issue.
>>
>>> QEMU indents are four spaces. Tabs are never used, except in
>>> Makefiles where they have been irreversibly coded into the syntax.
>>> Spaces of course are superior to tabs because:
>>>      You have just one way to specify whitespace, not two. Ambiguity
>>> breeds mistakes.
>>>      The confusion surrounding ‘use tabs to indent, spaces to
>>> justify’ is gone.
>>>      Tab indents push your code to the right, making your screen
>>> seriously unbalanced.
>>>      Tabs will be rendered incorrectly on editors who are
>>> misconfigured not to use tab stops of eight positions.
>>>      Tabs are rendered badly in patches, causing off-by-one errors in
>>> almost every line.
>>>     It is the QEMU coding style.
>>
>> I think it's better if we leave this discussion here, and accept v1
>> which is consistent with the coding style, or this one which is
>> consistent with the inconsistency of the tabs and spaces mix of the
>> checkpatch.pl source code as a compromise, if it is deemed important.
> 
> I suspect the problem is that checkpatch.pl is an import from the Linux
> source tree which has since had syncs with its upstream as well as a
> slew of QEMU specific patches. If we don't care about tracking upstream
> anymore we could bite the bullet and fix indentation going forward.

We diverged quite some time ago and don't track it anymore AFAICT.
Regardless, git tools are clever enough to deal with space changes
and a tab/space commit can be added to .git-blame-ignore-revs.

> Of course arguably we should replace it with a python script and reduce
> our dependence on perl. I'm sure someone had a go at that once but it
> might have only been a partial undertaking.
>
Re: [PATCH v2] scripts/checkpatch.pl: check for placeholders in cover letter patches
Posted by Peter Maydell 10 months ago 
On Tue, 30 Jan 2024 at 15:11, Alex Bennée <alex.bennee@linaro.org> wrote:
> I suspect the problem is that checkpatch.pl is an import from the Linux
> source tree which has since had syncs with its upstream as well as a
> slew of QEMU specific patches. If we don't care about tracking upstream
> anymore we could bite the bullet and fix indentation going forward.

I had a look at trying to update to be based on a newer
version of the Linux checkpatch some years ago, but it turned
out to not be a trivial thing to do, and we've only diverged
further since then. (Which is unfortunate, because I imagine
there have been bugfixes to the perl script's handling of
parsing fragments of C code which we'd like.)

-- PMM

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.