[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eli Zaretskii
Subject: Re: [PATCH v2] Font lock long Git commit summary lines
Date: Mon, 05 Sep 2022 15:07:49 +0300

> 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


> +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.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]