Re: [PATCH 1/6] BaseTools/PatchCheck: Stop parsing commit message after --- separator

["Laszlo Ersek" <lersek@redhat.com>]

On 12/05/19 17:12, Philippe Mathieu-Daude wrote:
> git-format-patch uses the '---' separator to mark the end of the
> commit message. Anything after the separator is stripped by git-am,
> it is pointless to check this part.
> 
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
> ---
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  BaseTools/Scripts/PatchCheck.py | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
> index 2a4e6f603e79..8bcf857a7d15 100755
> --- a/BaseTools/Scripts/PatchCheck.py
> +++ b/BaseTools/Scripts/PatchCheck.py
> @@ -209,6 +209,10 @@ class CommitMessageCheck:
>                         'empty.')
>  
>          for i in range(2, count):
> +            if lines[i].strip() == '---':
> +                # '---' marks the end of the commit message.
> +                count = i
> +                break
>              if (len(lines[i]) >= 76 and
>                  len(lines[i].split()) > 1 and
>                  not lines[i].startswith('git-svn-id:')):
> 

This shortcut seems to prevent two checks (on the affected area):
- line length check,
- separation of the signature block by an empty line.

While preventing the second check in the affected area certainly makes
sense, I think preserving the line length check would be nice.

For example, "git-notes" places notes in that area, and if someone uses
a text editor configuration in which hard line breaks are not inserted,
we could end up with a humongous note there. Even though it would not be
applied by git-am, I think we should not get such notes posted to the list.

(This text editor configuration is not theoretical: earlier we used to
get actual commit messages with no hard line breaks.)

So... I don't want to complicate this for you needlessly, but you
mention in the series blurb that this patch is a bugfix. What is the bug?

Hmmm... does the check (mistakenly) fire if the diffstat is very wide?
(It can easily be very wide, because our pathnames are long.) Hmmm.

Okay, I agree that's a much more practical problem than what I describe
above. So:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo