[PATCH] * vc-git.el (vc-git-log-edit-mode): Font lock long summary lines.

[PATCH] * vc-git.el (vc-git-log-edit-mode): Font lock long summary lines.

[Sean Whitton]

---
 lisp/vc/vc-git.el | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

I'd like to install this patch to highlight long summary lines when composing
git commit messages.  Comments welcome.

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 7395253745..8338bfcb26 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -913,7 +913,14 @@ vc-git-log-edit-mode-map
 
 (define-derived-mode vc-git-log-edit-mode log-edit-mode "Log-Edit/git"
   "Major mode for editing Git log messages.
-It is based on `log-edit-mode', and has Git-specific extensions.")
+It is based on `log-edit-mode', and has Git-specific extensions."
+  (setq-local
+   log-edit-font-lock-keywords
+   (append log-edit-font-lock-keywords
+           ;; Warn about going over 50 chars, warn harder about going
+           ;; over 68, per established conventions.
+           '(("\\`Summary: .\\{,50\\}\\(.\\{,18\\}\\)\\(.*\\)"
+	      (1 'warning prepend t) (2 'error prepend t))))))
 
 (defvar vc-git-patch-string nil)
 
-- 
2.30.2

Re: [PATCH] * vc-git.el (vc-git-log-edit-mode): Font lock long summary lines.

[Lars Ingebrigtsen]

Sean Whitton <spwhitton@spwhitton.name> writes:

> I'd like to install this patch to highlight long summary lines when composing
> git commit messages.  Comments welcome.

What constitutes a too-long summary line varies by project, so I don't
think doing this by default would be appreciated.  But it would be a
user option.

Re: [PATCH] * vc-git.el (vc-git-log-edit-mode): Font lock long summary lines.

[Sean Whitton]

Hello,

On Sun 04 Sep 2022 at 12:50PM +02, Lars Ingebrigtsen wrote:

> Sean Whitton <spwhitton@spwhitton.name> writes:
>
>> I'd like to install this patch to highlight long summary lines when composing
>> git commit messages.  Comments welcome.
>
> What constitutes a too-long summary line varies by project, so I don't
> think doing this by default would be appreciated.  But it would be a
> user option.

Ah okay, I don't recall having myself seen anything written down other
than 50/68, though I know plenty of projects don't care about it at all.

I'm thinking two defcustoms, then:

    vc-git-summary-target-length defaults to nil,
         I'll make emacs.git's .dir-locals.el set it to 50

    vc-git-summary-max-length defaults to 68.

(The name of the latter is based on `git-commit-summary-max-length' from
git-commit.el on my system, which I believe is pulled in by Magit.)

-- 
Sean Whitton

Re: [PATCH] * vc-git.el (vc-git-log-edit-mode): Font lock long summary lines.

[Protesilaos Stavrou]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Date: Sun, 04 Sep 2022 10:27:09 -0700
>
> Hello,

Hello Sean,

> On Sun 04 Sep 2022 at 12:50PM +02, Lars Ingebrigtsen wrote:
>
>> Sean Whitton <spwhitton@spwhitton.name> writes:
>>
>>> I'd like to install this patch to highlight long summary lines when composing
>>> git commit messages.  Comments welcome.
>>
>> What constitutes a too-long summary line varies by project, so I don't
>> think doing this by default would be appreciated.  But it would be a
>> user option.
>
> Ah okay, I don't recall having myself seen anything written down other
> than 50/68, though I know plenty of projects don't care about it at all.
>
> I'm thinking two defcustoms, then:
>
>     vc-git-summary-target-length defaults to nil,
>          I'll make emacs.git's .dir-locals.el set it to 50
>
>     vc-git-summary-max-length defaults to 68.
>
> (The name of the latter is based on `git-commit-summary-max-length' from
> git-commit.el on my system, which I believe is pulled in by Magit.)

Please consider defining the relevant faces as well.  The 'warning' and
'error' are not always appropriate here and by hardcoding them we no
longer have the chance to modify them easily (thinking about themes).

git-commit.el does this:

    (defface git-commit-overlong-summary
      '((t :inherit font-lock-warning-face))
      "Face used for the tail of overlong commit message summaries."
      :group 'git-commit-faces)

    (defface git-commit-nonempty-second-line
      '((t :inherit font-lock-warning-face))
      "Face used for non-whitespace on the second line of commit messages."
      :group 'git-commit-faces)

The new faces can inherit from 'warning' and 'error', of course.

All the best,
Protesilaos (or simply "Prot")

-- 
Protesilaos Stavrou
https://protesilaos.com

Re: [PATCH] * vc-git.el (vc-git-log-edit-mode): Font lock long summary lines.

[Matthias Meulien]

Sean Whitton <spwhitton@spwhitton.name> writes:

> (...)  I'd like to install this patch to highlight long summary lines
> when composing git commit messages.  Comments welcome.

Isn't this feature already implemented through whitespace-mode with
custom value for whitespace-line-column?
-- 
Matthias

Re: [PATCH] * vc-git.el (vc-git-log-edit-mode): Font lock long summary lines.

[Philip Kaludercic]

Matthias Meulien <orontee@gmail.com> writes:

> Sean Whitton <spwhitton@spwhitton.name> writes:
>
>> (...)  I'd like to install this patch to highlight long summary lines
>> when composing git commit messages.  Comments welcome.
>
> Isn't this feature already implemented through whitespace-mode with
> custom value for whitespace-line-column?

I don't believe the whitespace mode distinguishes between warnings at 50
characters and "errors" at 68 characters.

Re: [PATCH] * vc-git.el (vc-git-log-edit-mode): Font lock long summary lines.

[Sean Whitton]

Hello,

On Sun 04 Sep 2022 at 06:41PM GMT, Philip Kaludercic wrote:

> Matthias Meulien <orontee@gmail.com> writes:
>
>> Sean Whitton <spwhitton@spwhitton.name> writes:
>>
>>> (...)  I'd like to install this patch to highlight long summary lines
>>> when composing git commit messages.  Comments welcome.
>>
>> Isn't this feature already implemented through whitespace-mode with
>> custom value for whitespace-line-column?
>
> I don't believe the whitespace mode distinguishes between warnings at 50
> characters and "errors" at 68 characters.

Additionally, we only want it on the Summary: line, not elsewhere.

-- 
Sean Whitton

[PATCH v2] Font lock long Git commit summary lines

[Sean Whitton]

[-- Attachment #1: Type: text/plain, Size: 71 bytes --]

Here is v2 of my patch.  Thank you for the comments.

-- 
Sean Whitton

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Font-lock-long-Git-commit-summary-lines.patch --]
[-- Type: text/x-patch, Size: 4871 bytes --]

From 142261030f7759428701afa6bc4e1e5ac72dc7b4 Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Sun, 4 Sep 2022 16:20:15 -0700
Subject: [PATCH] Font lock long Git commit summary lines

* lisp/vc/vc-git.el (vc-git-log-edit-summary-target)
(vc-git-log-edit-summary-max): New defcustoms.
(vc-git-log-edit-summary-warning, vc-git-log-edit-summary-error): New faces.
(vc-git--log-edit-summary-check): New function.
(vc-git-log-edit-mode): Add vc-git--log-edit-summary-check to
log-edit-font-lock-keywords to font lock long Git commit summary
lines.
* .dir-locals.el: Set vc-git-log-edit-summary-target.
---
 .dir-locals.el    |  3 ++-
 lisp/vc/vc-git.el | 62 ++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/.dir-locals.el b/.dir-locals.el
index 7812beb001..666d8019cc 100644
--- a/.dir-locals.el
+++ b/.dir-locals.el
@@ -5,7 +5,8 @@
          (sentence-end-double-space . t)
          (fill-column . 70)
 	 (emacs-lisp-docstring-fill-column . 65)
-         (bug-reference-url-format . "https://debbugs.gnu.org/%s")))
+         (bug-reference-url-format . "https://debbugs.gnu.org/%s")
+	 (vc-git-log-edit-summary-target . 50)))
  (c-mode . ((c-file-style . "GNU")
             (c-noise-macro-names . ("INLINE" "ATTRIBUTE_NO_SANITIZE_UNDEFINED" "UNINIT" "CALLBACK" "ALIGN_STACK"))
             (electric-quote-comment . nil)
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 7395253745..e7c04393de 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -858,6 +858,43 @@ vc-git-branches
 
 ;;; STATE-CHANGING FUNCTIONS
 
+(defcustom vc-git-log-edit-summary-target nil
+  "Target length for Git commit summary lines.
+If non-nil, characters in Summary: lines beyond this length are
+displayed in the `vc-git-log-edit-summary-warning' face.
+
+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.
+
+See also `vc-git-log-edit-summary-max'."
+  :type '(choice (const :tag "No target" nil)
+                 (natnum :tag "Target length"))
+  :safe (lambda (x) (or (not x) (integerp x))))
+
+(defface vc-git-log-edit-summary-warning
+  '((t :inherit warning))
+  "Face for Git commit summary lines beyond the target length.")
+
+(defcustom vc-git-log-edit-summary-max 68
+  "Maximum length for Git commit summary lines.
+If non-nil, characters in summary lines beyond this length are
+displayed in the `vc-git-log-edit-summary-error' face.
+
+It is good practice to avoid writing summary lines longer than
+this because otherwise the summary line will be truncated in many
+contexts in which Git commands display summary lines.
+
+See also `vc-git-log-edit-summary-target'."
+  :type '(choice (const :tag "No target" nil)
+                 (natnum :tag "Target length"))
+  :safe (lambda (x) (or (not x) (integerp x))))
+
+(defface vc-git-log-edit-summary-error
+  '((t :inherit error))
+  "Face for Git commit summary lines beyond the maximum length.")
+
 (defun vc-git-create-repo ()
   "Create a new Git repository."
   (vc-git-command nil 0 nil "init"))
@@ -911,9 +948,32 @@ vc-git-log-edit-mode-map
   "C-c C-n" #'vc-git-log-edit-toggle-no-verify
   "C-c C-e" #'vc-git-log-edit-toggle-amend)
 
+(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)
+                          (format ".\\{,%d\\}\\(.\\{,%d\\}\\)\\(.*\\)"
+                                  vc-git-log-edit-summary-target
+                                  (- vc-git-log-edit-summary-max
+                                     vc-git-log-edit-summary-target)))
+                         (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)))))
+         (re-search-forward regex limit t))))
+
 (define-derived-mode vc-git-log-edit-mode log-edit-mode "Log-Edit/git"
   "Major mode for editing Git log messages.
-It is based on `log-edit-mode', and has Git-specific extensions.")
+It is based on `log-edit-mode', and has Git-specific extensions."
+  (setq-local
+   log-edit-font-lock-keywords
+   (append log-edit-font-lock-keywords
+           '((vc-git--log-edit-summary-check
+	      (1 'vc-git-log-edit-summary-warning prepend t)
+              (2 'vc-git-log-edit-summary-error prepend t))))))
 
 (defvar vc-git-patch-string nil)
 
-- 
2.30.2

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

[Lars Ingebrigtsen]

Sean Whitton <spwhitton@spwhitton.name> writes:

> Here is v2 of my patch.  Thank you for the comments.

Looks good to me; please go ahead and push.

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

[Eli Zaretskii]

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

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

[Sean Whitton]

Hello,

I've applied most of this, thank you.  Rather than replace 'target',
which I think is right, I renamed the two faces.  What we have are two
warnings, no errors.

On Mon 05 Sep 2022 at 03:07PM +03, Eli Zaretskii wrote:

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

I put calls to natnump in, but I'm not sure about it.  Is it really most
useful to silently fail to highlight the characters if somehow something
other than a number has ended up in the variables?  I'm new to working
with font lock so would be grateful for more input.

-- 
Sean Whitton

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

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Date: Mon, 05 Sep 2022 11:14:27 -0700
> 
> >> +(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.
> 
> I put calls to natnump in, but I'm not sure about it.  Is it really most
> useful to silently fail to highlight the characters if somehow something
> other than a number has ended up in the variables?  I'm new to working
> with font lock so would be grateful for more input.

In similar cases we document this, something like

  ... any other value is treated as nil.

This is IMO less fragile in the face of users doing silly things with
the value.

Thanks.

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

[Sean Whitton]

Hello,

On Mon 05 Sep 2022 at 09:23PM +03, Eli Zaretskii wrote:

> In similar cases we document this, something like
>
>   ... any other value is treated as nil.
>
> This is IMO less fragile in the face of users doing silly things with
> the value.

Thanks, pushed something like this.

-- 
Sean Whitton