unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
From: Stefan Monnier <monnier@iro.umontreal.ca>
To: Dmitry Gutov <dgutov@yandex.ru>
Cc: 12537@debbugs.gnu.org
Subject: bug#12537: Acknowledgement (support for git commit --amend/--signoff)
Date: Sun, 30 Sep 2012 23:16:05 -0400	[thread overview]
Message-ID: <jwv8vbqoq2j.fsf-monnier+emacs@gnu.org> (raw)
In-Reply-To: <50663D58.4010702@yandex.ru> (Dmitry Gutov's message of "Sat, 29 Sep 2012 04:14:16 +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





  reply	other threads:[~2012-10-01  3:16 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-09-29  0:11 bug#12537: support for git commit --amend/--signoff Dmitry Gutov
     [not found] ` <handler.12537.B.134887753227659.ack@debbugs.gnu.org>
2012-09-29  0:14   ` bug#12537: Acknowledgement (support for git commit --amend/--signoff) Dmitry Gutov
2012-10-01  3:16     ` Stefan Monnier [this message]
2012-10-01  3:59       ` Dmitry Gutov
2012-10-01  4:32         ` Chong Yidong
2012-10-02  0:28           ` Dmitry Gutov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=jwv8vbqoq2j.fsf-monnier+emacs@gnu.org \
    --to=monnier@iro.umontreal.ca \
    --cc=12537@debbugs.gnu.org \
    --cc=dgutov@yandex.ru \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).