Re: [PATCH v2] Font lock long Git commit summary lines

[Eli Zaretskii <eliz@gnu.org>]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Date: Sun, 04 Sep 2022 16:22:29 -0700
> 
> Here is v2 of my patch.  Thank you for the comments.

Thanks.  May I comment a bit more?

> +(defcustom vc-git-log-edit-summary-target nil
> +  "Target length for Git commit summary lines.

Given the description below, "Target length" is not the best wording
here.  Perhaps "Preferred maximum length" is better?  And
consequently, I think a better name for the variable is

  vc-git-log-edit-summary-warning-len

> +By setting this to an integer around 50, you can improve the
> +compatibility of your commit messages with Git commands that
> +print the summary line in width-constrained contexts.  However,
> +many commit summaries will need to exceed this length.

I'd lose the last sentence here: it confuses the whole issue.  When
you say "Preferred" or "Target", and describe what will happen beyond
that, you already in effect tell the reader that the limitation is not
a hard one, but just a guideline.

> +(defface vc-git-log-edit-summary-warning
> +  '((t :inherit warning))
> +  "Face for Git commit summary lines beyond the target length.")

This should mention vc-git-log-edit-summary-target.  Imagine a user
who looks at the face's doc string and wonders which "target length"
does the above allude to.

> +(defcustom vc-git-log-edit-summary-max 68

I'd call this vc-git-log-edit-summary-max-len

> +  "Maximum length for Git commit summary lines.
> +If non-nil, characters in summary lines beyond this length are
   ^^^^^^^^^^
"If a number, ..."  Saying "if non-nil, ... beyond this length" is
problematic, because not all non-nil values are numbers.

> +(defface vc-git-log-edit-summary-error
> +  '((t :inherit error))
> +  "Face for Git commit summary lines beyond the maximum length.")

Reference the variable in the doc string.

> +(defun vc-git--log-edit-summary-check (limit)
> +  (and (re-search-forward "^Summary: " limit t)
> +       (when-let ((regex
> +                   (cond ((and vc-git-log-edit-summary-max
> +                               vc-git-log-edit-summary-target)

Should this test using numberp?

> +                         (vc-git-log-edit-summary-max
> +                          (format ".\\{,%d\\}\\(?2:.*\\)"
> +                                  vc-git-log-edit-summary-max))
> +                         (vc-git-log-edit-summary-target
> +                          (format ".\\{,%d\\}\\(.*\\)"
> +                                  vc-git-log-edit-summary-target)))))

Likewise here.