GNU bug report logs - #12537
support for git commit --amend/--signoff

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 12537 in the body.
You can then email your comments to 12537 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: Dmitry Gutov <dgutov <at> yandex.ru>
To: bug-gnu-emacs <at> gnu.org
Subject: support for git commit --amend/--signoff
Date: Sat, 29 Sep 2012 04:11:44 +0400

Tags: patch

This is based on Dan Nicolaescu's patch from here: 
http://lists.gnu.org/archive/html/emacs-devel/2010-06/msg00784.html

I modified it according to Stefan's request, and made some other tweaks.

Notes:
1) Magit handles the Amend action in a similar way: it also inserts a 
header at the top of the message edit buffer. I haven't seen any 
complaints from users.
2) I haven't been able to make menu-bar keymap work as intended.
I copied log-edit-menu to the local menu-map variable, and it shows, but 
if I don't set the parent keymap of vc-git-log-edit-mode-map to 
log-edit-mode-map, the menu popup doesn't show the latter's keybindings 
(and they likely don't work, haven't tried). If I do set it as parent, 
then the "*VC-log*" mode line element menu only contains two elements, 
but submenus, one for each keymap. I don't think that's optimal, so I 
discarded the menu-map part altogether.
3) Toggling Amend on/off repeatedly may lead to slightly different 
behavior if the commit message subject looks like a "header: value" 
string, and especially if that's the only line in the message. The 
difference would be in the added newlines, and the commit subject will 
become highlighted as a header line.
To counteract this, Magit inserts a "-- magit header ends here --" line 
after the headers. Not sure if we should do the same.
4) The new first argument format of log-edit-extract-headers is kinda 
awkward, but it's the only way I could think of to make it 
backwards-compatible, and I do think that this is the function that 
should handle the yes/no headers logic. The third element in the new 
form ("yes") is more or less superfluous (we could just hardcode it 
everywhere as the only possible value for "true"), but without it, the 
new form would look even more awkward. Suggestions welcome.

--Dmitry

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: 12537 <at> debbugs.gnu.org
Subject: Re: bug#12537: Acknowledgement (support for git commit
	--amend/--signoff)
Date: Sat, 29 Sep 2012 04:14:16 +0400

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

Sorry, here's the patch.

[amend.diff (text/plain, attachment)]

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 12537 <at> debbugs.gnu.org
Subject: Re: bug#12537: Acknowledgement (support for git commit
	--amend/--signoff)
Date: Sun, 30 Sep 2012 23:16:05 -0400

> -     (,(concat "^\\(\\([[:alpha:]]+\\):\\)" log-edit-header-contents-regexp)
> +     (,(concat "^\\(\\([[:alpha:]][^: \n\t]+\\):\\)"
> +               log-edit-header-contents-regexp)

I'd prefer to only add hyphens, as in [[:alpha:]-].

> +(defun log-edit-toggle-header (name value)
> +  "Toggle a boolean-type header in the current buffer.
> +If the value of NAME is VALUE, remove it.  Otherwise, add it if
> +it's not present and set it to VALUE.  Afterward, if there are headers,
> +make sure there is an empty line after them.  If there are no headers,
> +remove all empty lines at the beginning of the buffer.
> +Return t if toggled on, otherwise nil."

How 'bout leaving the header, just with an empty content, so you never
have to deal with "remove a sole empty line if there's no header left"?

> +or (HEADER CMDARG VALUE) associating header names to the corresponding
> +cmdlineoption name and the result is then a list of the form
> +\(MSG CMDARG1 HDRTEXT1 CMDARG2 HDRTEXT2...\) where MSG is the remaining text
> +from STRING.  For HEADERS elements of the second type, the header value is
> +not added to the list.  And CMDARG is added to the result list only if
> +the header value is the same as VALUE.

I think I'd rather provide something a bit more general.  E.g. accept
entries of the form (HEADER . FUNCTION) where function takes the
header's value and returns a list of arguments where vc-git can provide
as FUNCTION something like
(lambda (val) (if (equal val "yes") '("--amend")))

> +(defun vc-git-log-edit-toggle-signoff ()
> +  (interactive)
> +  (log-edit-toggle-header "Sign-Off" "yes"))

please provide a docstring for interactive functions.

> +(defun vc-git-log-edit-toggle-amend ()
> +  (interactive)

Same here.

> +(define-derived-mode vc-git-log-edit-mode log-edit-mode "*VC-log*"

"*VC-log*"?  Really?  Shouldn't that be "Log-Edit" or "Log-Edit/git"
or something?

> +  "Major mode for editing Git log messages.
> +It is based on `log-edit-mode', and has Git-specific extensions.
> +\\{vc-git-log-edit-mode-map}")

The \\{vc-git-log-edit-mode-map} shouldn't be needed since
define-derived-mode will add it for you anyway.

Other than that, it looks OK, so feel free to install it after you fixed
the above details.


        Stefan

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 12537 <at> debbugs.gnu.org
Subject: Re: bug#12537: Acknowledgement (support for git commit
	--amend/--signoff)
Date: Mon, 01 Oct 2012 07:59:15 +0400

On 01.10.2012 7:16, Stefan Monnier wrote:
>> -     (,(concat "^\\(\\([[:alpha:]]+\\):\\)" log-edit-header-contents-regexp)
>> +     (,(concat "^\\(\\([[:alpha:]][^: \n\t]+\\):\\)"
>> +               log-edit-header-contents-regexp)
>
> I'd prefer to only add hyphens, as in [[:alpha:]-].

Ok. How about I also add the limitation that the first character must be 
a capital letter? message-font-lock-keywords has that.

>> +(defun log-edit-toggle-header (name value)
>> +  "Toggle a boolean-type header in the current buffer.
>> +If the value of NAME is VALUE, remove it.  Otherwise, add it if
>> +it's not present and set it to VALUE.  Afterward, if there are headers,
>> +make sure there is an empty line after them.  If there are no headers,
>> +remove all empty lines at the beginning of the buffer.
>> +Return t if toggled on, otherwise nil."
>
> How 'bout leaving the header, just with an empty content, so you never
> have to deal with "remove a sole empty line if there's no header left"?

Works for me.

>> +or (HEADER CMDARG VALUE) associating header names to the corresponding
>> +cmdlineoption name and the result is then a list of the form
>> +\(MSG CMDARG1 HDRTEXT1 CMDARG2 HDRTEXT2...\) where MSG is the remaining text
>> +from STRING.  For HEADERS elements of the second type, the header value is
>> +not added to the list.  And CMDARG is added to the result list only if
>> +the header value is the same as VALUE.
>
> I think I'd rather provide something a bit more general.  E.g. accept
> entries of the form (HEADER . FUNCTION) where function takes the
> header's value and returns a list of arguments where vc-git can provide
> as FUNCTION something like
> (lambda (val) (if (equal val "yes") '("--amend")))

Okay. That's definitely less awkward than my proposed change.

>> +(defun vc-git-log-edit-toggle-signoff ()
>> +  (interactive)
>> +  (log-edit-toggle-header "Sign-Off" "yes"))
>
> please provide a docstring for interactive functions.
>
>> +(defun vc-git-log-edit-toggle-amend ()
>> +  (interactive)
>
> Same here.
>
>> +(define-derived-mode vc-git-log-edit-mode log-edit-mode "*VC-log*"
>
> "*VC-log*"?  Really?  Shouldn't that be "Log-Edit" or "Log-Edit/git"
> or something?

Sure, will do. I like the "Log-Edit/git" option better.

>> +  "Major mode for editing Git log messages.
>> +It is based on `log-edit-mode', and has Git-specific extensions.
>> +\\{vc-git-log-edit-mode-map}")
>
> The \\{vc-git-log-edit-mode-map} shouldn't be needed since
> define-derived-mode will add it for you anyway.
>
> Other than that, it looks OK, so feel free to install it after you fixed
> the above details.

I think we're entering the feature freeze period right about now. Is it 
okay if I install the updated patch 16 hours or so later?

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

From: Chong Yidong <cyd <at> gnu.org>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 12537 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#12537: Acknowledgement (support for git commit
	--amend/--signoff)
Date: Mon, 01 Oct 2012 12:32:10 +0800

Dmitry Gutov <dgutov <at> yandex.ru> writes:

> I think we're entering the feature freeze period right about now. Is
> it okay if I install the updated patch 16 hours or so later?

No problem.

Message #22 received at 12537-done <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Chong Yidong <cyd <at> gnu.org>, 12537-done <at> debbugs.gnu.org
Subject: Re: bug#12537: Acknowledgement (support for git commit
	--amend/--signoff)
Date: Tue, 02 Oct 2012 04:28:44 +0400

Installed, closing.

By the way, the separator line added in 110266 is a nice touch.

--Dmitry

Previous Next

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