bug#12537: Acknowledgement (support for git commit --amend/--signoff)

[Dmitry Gutov]

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?