GNU bug report logs - #58092
29.0.50; [PATCH] Add log-edit-summary-separator face

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 58092 in the body.
You can then email your comments to 58092 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Protesilaos Stavrou <info <at> protesilaos.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; [PATCH] Add log-edit-summary-separator face
Date: Mon, 26 Sep 2022 15:44:50 +0300

[Message part 1 (text/plain, inline)]

Dear maintainers,

The attached patch replaces some hardcoded face properties in log-edit
buffers with a named face.

The named face allows users/themes to customise how this line is styled.

What do you think?

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

-- 
Protesilaos Stavrou
https://protesilaos.com

[0001-Add-log-edit-summary-separator-face.patch (text/x-patch, attachment)]

Message #8 received at 58092 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Protesilaos Stavrou <info <at> protesilaos.com>
Cc: 58092 <at> debbugs.gnu.org
Subject: Re: bug#58092: 29.0.50; [PATCH] Add log-edit-summary-separator face
Date: Mon, 26 Sep 2022 15:20:56 +0200

Protesilaos Stavrou <info <at> protesilaos.com> writes:

> The named face allows users/themes to customise how this line is styled.
>
> What do you think?

Sounds good to me.

> +(defface log-edit-summary-separator '((t :height 0.1 :inverse-video t :extend t))

Minor nit pick: That's a too-long line.

Message #11 received at 58092 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Protesilaos Stavrou <info <at> protesilaos.com>, 58092 <at> debbugs.gnu.org
Subject: Re: bug#58092: 29.0.50; [PATCH] Add log-edit-summary-separator face
Date: Mon, 26 Sep 2022 09:22:09 -0400

Protesilaos Stavrou <info <at> protesilaos.com> writes:

> From ea287b3d918c6a2cf6181c5671187143b3774bd5 Mon Sep 17 00:00:00 2001
> Message-Id: <ea287b3d918c6a2cf6181c5671187143b3774bd5.1664196100.git.info <at> protesilaos.com>
> From: Protesilaos Stavrou <info <at> protesilaos.com>
> Date: Mon, 26 Sep 2022 15:41:18 +0300
> Subject: [PATCH] Add log-edit-summary-separator face
>
> * etc/NEWS: Report on it.

This line is optional, but I'd put it below the main change and say
something like "Announce the new face."

> * lisp/vc/log-edit.el (log-edit-summary-separator): Add new face.
> (log-edit-font-lock-keywords): Replace hardcoded face attributes with
> named face.
> ---
>  etc/NEWS            | 5 +++++
>  lisp/vc/log-edit.el | 6 +++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/etc/NEWS b/etc/NEWS
> index 0a5b7bc29c..ced4ece35d 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -1766,6 +1766,11 @@ Writing shorter summary lines avoids truncation in contexts in which
>  Git commands display summary lines.  See the two new user options
>  'vc-git-log-edit-summary-target-len' and 'vc-git-log-edit-summary-max-len'.
>
> +---
> +*** New 'log-edit-summary-separator' face
> +Styles the line that separates the log-edit headers from the log-edit
> +summary.

I'd say:

    "It is used for the 'log-edit' headers ..."

> +
>  ** Message
>
>  ---
> diff --git a/lisp/vc/log-edit.el b/lisp/vc/log-edit.el
> index 5290616302..25b47a75af 100644
> --- a/lisp/vc/log-edit.el
> +++ b/lisp/vc/log-edit.el
> @@ -325,6 +325,10 @@ ;;; Actual code
>  (defface log-edit-summary '((t :inherit font-lock-function-name-face))
>    "Face for the summary in `log-edit-mode' buffers.")
>
> +(defface log-edit-summary-separator '((t :height 0.1 :inverse-video t :extend t))

Maybe put the value on a separate line (in case we want to extend it
later)?

> +  "Face for the summary separator line in `log-edit-mode' buffers."
> +  :version "29.1")
> +
>  (defface log-edit-header '((t :inherit font-lock-keyword-face))
>    "Face for the headers in `log-edit-mode' buffers.")
>
> @@ -393,7 +397,7 @@ (defvar log-edit-font-lock-keywords
>           nil lax))
>       ("^\n"
>        (progn (goto-char (match-end 0)) (1+ (match-end 0))) nil
> -      (0 '(face (:height 0.1 :inverse-video t :extend t)
> +      (0 '(face log-edit-summary-separator
>             display-line-numbers-disable t rear-nonsticky t))))
>      (log-edit--match-first-line (0 'log-edit-summary))))

Other than those nits, LGTM (but I didn't test it).

Message #14 received at 58092 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Protesilaos Stavrou <info <at> protesilaos.com>, 58092 <at> debbugs.gnu.org
Subject: Re: bug#58092: 29.0.50; [PATCH] Add log-edit-summary-separator face
Date: Mon, 26 Sep 2022 16:28:56 +0300

Hi!

On 26.09.2022 15:44, Protesilaos Stavrou wrote:
> Dear maintainers,
> 
> The attached patch replaces some hardcoded face properties in log-edit
> buffers with a named face.
> 
> The named face allows users/themes to customise how this line is styled.
> 
> What do you think?

LGTM, except perhaps call it headers-separator? It's used to separate 
all the headers, not just summary.

Message #17 received at 58092 <at> debbugs.gnu.org (full text, mbox):

From: Protesilaos Stavrou <info <at> protesilaos.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>, 58092 <at> debbugs.gnu.org
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, Stefan Kangas <stefankangas <at> gmail.com>
Subject: Re: bug#58092: 29.0.50; [PATCH] Add log-edit-summary-separator face
Date: Mon, 26 Sep 2022 17:06:22 +0300

> From: Dmitry Gutov <dgutov <at> yandex.ru>
> Date: Mon, 26 Sep 2022 16:28:56 +0300
>
> Hi!
>
> On 26.09.2022 15:44, Protesilaos Stavrou wrote:
>> Dear maintainers,
>> 
>> The attached patch replaces some hardcoded face properties in log-edit
>> buffers with a named face.
>> 
>> The named face allows users/themes to customise how this line is styled.
>> 
>> What do you think?
>
> LGTM, except perhaps call it headers-separator? It's used to separate 
> all the headers, not just summary.

Hello everyone,

I made the changes as you suggested and pushed them as commit
a386833503.  However, I forgot to update the commit message of the
patch: it still mentions the old name of the face
'log-edit-summary-separator' instead of 'log-edit-headers-separator'.

Sorry for this mistake!  Can I revert or amend it?

-- 
Protesilaos Stavrou
https://protesilaos.com

Message #20 received at 58092 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Protesilaos Stavrou <info <at> protesilaos.com>, 58092 <at> debbugs.gnu.org
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, Stefan Kangas <stefankangas <at> gmail.com>
Subject: Re: bug#58092: 29.0.50; [PATCH] Add log-edit-summary-separator face
Date: Mon, 26 Sep 2022 17:32:32 +0300

On 26.09.2022 17:06, Protesilaos Stavrou wrote:
>> From: Dmitry Gutov<dgutov <at> yandex.ru>
>> Date: Mon, 26 Sep 2022 16:28:56 +0300
>>
>> Hi!
>>
>> On 26.09.2022 15:44, Protesilaos Stavrou wrote:
>>> Dear maintainers,
>>>
>>> The attached patch replaces some hardcoded face properties in log-edit
>>> buffers with a named face.
>>>
>>> The named face allows users/themes to customise how this line is styled.
>>>
>>> What do you think?
>> LGTM, except perhaps call it headers-separator? It's used to separate
>> all the headers, not just summary.
> Hello everyone,
> 
> I made the changes as you suggested and pushed them as commit
> a386833503.  However, I forgot to update the commit message of the
> patch: it still mentions the old name of the face
> 'log-edit-summary-separator' instead of 'log-edit-headers-separator'.
> 
> Sorry for this mistake!  Can I revert or amend it?

If it's just in the commit message, then a revert won't help.

And I don't think we have any tools for amending the change log so far.

In any case, NEWS is more important than the commit message. So it's 
probably fine.

Message #23 received at 58092 <at> debbugs.gnu.org (full text, mbox):

From: Robert Pluim <rpluim <at> gmail.com>
To: Protesilaos Stavrou <info <at> protesilaos.com>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 58092 <at> debbugs.gnu.org,
 Stefan Kangas <stefankangas <at> gmail.com>, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#58092: 29.0.50; [PATCH] Add log-edit-summary-separator face
Date: Mon, 26 Sep 2022 16:40:36 +0200

>>>>> On Mon, 26 Sep 2022 17:06:22 +0300, Protesilaos Stavrou <info <at> protesilaos.com> said:
    Protesilaos> I made the changes as you suggested and pushed them as commit
    Protesilaos> a386833503.  However, I forgot to update the commit message of the
    Protesilaos> patch: it still mentions the old name of the face
    Protesilaos> 'log-edit-summary-separator' instead of 'log-edit-headers-separator'.

    Protesilaos> Sorry for this mistake!  Can I revert or amend it?

amend no. revert yes, but itʼs overkill for just a commit message issue. You
can always run 'make change-history' and fix it in the resulting
ChangeLog file (see admin/notes/repo for the details)

Robert
-- 

Message #26 received at 58092 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: Protesilaos Stavrou <info <at> protesilaos.com>, 58092 <at> debbugs.gnu.org,
 Lars Ingebrigtsen <larsi <at> gnus.org>, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#58092: 29.0.50; [PATCH] Add log-edit-summary-separator face
Date: Mon, 26 Sep 2022 16:49:39 +0200

Robert Pluim <rpluim <at> gmail.com> writes:

> amend no. revert yes, but itʼs overkill for just a commit message issue. You
> can always run 'make change-history' and fix it in the resulting
> ChangeLog file (see admin/notes/repo for the details)

Please don't do that, it will make merging more tricky later.

Message #29 received at 58092 <at> debbugs.gnu.org (full text, mbox):

From: Protesilaos Stavrou <info <at> protesilaos.com>
To: Stefan Kangas <stefankangas <at> gmail.com>, Robert Pluim <rpluim <at> gmail.com>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 58092 <at> debbugs.gnu.org,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#58092: 29.0.50; [PATCH] Add log-edit-summary-separator face
Date: Mon, 26 Sep 2022 17:55:18 +0300

> From: Stefan Kangas <stefankangas <at> gmail.com>
> Date: Mon, 26 Sep 2022 16:49:39 +0200
>
> Robert Pluim <rpluim <at> gmail.com> writes:
>
>> amend no. revert yes, but itʼs overkill for just a commit message issue. You
>> can always run 'make change-history' and fix it in the resulting
>> ChangeLog file (see admin/notes/repo for the details)
>
> Please don't do that, it will make merging more tricky later.

Okay, I will just leave it as-is.  Will be more careful next time.

-- 
Protesilaos Stavrou
https://protesilaos.com

Message #34 received at 58092 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, Robert Pluim <rpluim <at> gmail.com>,
 Dmitry Gutov <dgutov <at> yandex.ru>, Protesilaos Stavrou <info <at> protesilaos.com>,
 58092 <at> debbugs.gnu.org
Subject: Re: bug#58092: 29.0.50; [PATCH] Add log-edit-summary-separator face
Date: Tue, 27 Sep 2022 08:50:31 -0700

Hello,

On Mon 26 Sep 2022 at 04:49PM +02, Stefan Kangas wrote:

> Robert Pluim <rpluim <at> gmail.com> writes:
>
>> amend no. revert yes, but itʼs overkill for just a commit message issue. You
>> can always run 'make change-history' and fix it in the resulting
>> ChangeLog file (see admin/notes/repo for the details)
>
> Please don't do that, it will make merging more tricky later.

Hmm, so it's never okay to do that?  Or is it to be saved for the more
egregrious errors?

-- 
Sean Whitton

Message #37 received at 58092 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, Robert Pluim <rpluim <at> gmail.com>,
 Dmitry Gutov <dgutov <at> yandex.ru>, Protesilaos Stavrou <info <at> protesilaos.com>,
 58092 <at> debbugs.gnu.org
Subject: Re: bug#58092: 29.0.50; [PATCH] Add log-edit-summary-separator face
Date: Tue, 27 Sep 2022 12:15:29 -0400

Sean Whitton <spwhitton <at> spwhitton.name> writes:

>>> amend no. revert yes, but itʼs overkill for just a commit message issue. You
>>> can always run 'make change-history' and fix it in the resulting
>>> ChangeLog file (see admin/notes/repo for the details)
>>
>> Please don't do that, it will make merging more tricky later.
>
> Hmm, so it's never okay to do that?  Or is it to be saved for the more
> egregrious errors?

It's easier for me (or whoever is charge of merging and/or releases) if
we don't do that, so I'd prefer it if we could reserve it for
particularly nasty mistakes.

However, if it has to be done, please do it in two separate commits: one
that generates the ChangeLog and one that fixes the mistake.

Message #40 received at 58092 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, Robert Pluim <rpluim <at> gmail.com>,
 Dmitry Gutov <dgutov <at> yandex.ru>, Protesilaos Stavrou <info <at> protesilaos.com>,
 58092 <at> debbugs.gnu.org
Subject: Re: bug#58092: 29.0.50; [PATCH] Add log-edit-summary-separator face
Date: Tue, 27 Sep 2022 17:49:42 -0700

Hello,

On Tue 27 Sep 2022 at 12:15PM -04, Stefan Kangas wrote:

> Sean Whitton <spwhitton <at> spwhitton.name> writes:
>
>>>> amend no. revert yes, but itʼs overkill for just a commit message issue. You
>>>> can always run 'make change-history' and fix it in the resulting
>>>> ChangeLog file (see admin/notes/repo for the details)
>>>
>>> Please don't do that, it will make merging more tricky later.
>>
>> Hmm, so it's never okay to do that?  Or is it to be saved for the more
>> egregrious errors?
>
> It's easier for me (or whoever is charge of merging and/or releases) if
> we don't do that, so I'd prefer it if we could reserve it for
> particularly nasty mistakes.
>
> However, if it has to be done, please do it in two separate commits: one
> that generates the ChangeLog and one that fixes the mistake.

Thanks.  I've updated admin/notes/repo with this information.

-- 
Sean Whitton

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.