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

Previous Next

Full log

View this message in rfc822 format

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 12537 <at> debbugs.gnu.org
Subject: 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?

Previous Next

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