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

From: Stefan Monnier <[email-masked]>
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