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

Previous Next

Full log

View this message in rfc822 format

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

Previous Next

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