* bug#12537: support for git commit --amend/--signoff
@ 2012-09-29 0:11 Dmitry Gutov
[not found] ` <handler.12537.B.134887753227659.ack@debbugs.gnu.org>
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Gutov @ 2012-09-29 0:11 UTC (permalink / raw)
To: 12537
Tags: patch
This is based on Dan Nicolaescu's patch from here:
http://lists.gnu.org/archive/html/emacs-devel/2010-06/msg00784.html
I modified it according to Stefan's request, and made some other tweaks.
Notes:
1) Magit handles the Amend action in a similar way: it also inserts a
header at the top of the message edit buffer. I haven't seen any
complaints from users.
2) I haven't been able to make menu-bar keymap work as intended.
I copied log-edit-menu to the local menu-map variable, and it shows, but
if I don't set the parent keymap of vc-git-log-edit-mode-map to
log-edit-mode-map, the menu popup doesn't show the latter's keybindings
(and they likely don't work, haven't tried). If I do set it as parent,
then the "*VC-log*" mode line element menu only contains two elements,
but submenus, one for each keymap. I don't think that's optimal, so I
discarded the menu-map part altogether.
3) Toggling Amend on/off repeatedly may lead to slightly different
behavior if the commit message subject looks like a "header: value"
string, and especially if that's the only line in the message. The
difference would be in the added newlines, and the commit subject will
become highlighted as a header line.
To counteract this, Magit inserts a "-- magit header ends here --" line
after the headers. Not sure if we should do the same.
4) The new first argument format of log-edit-extract-headers is kinda
awkward, but it's the only way I could think of to make it
backwards-compatible, and I do think that this is the function that
should handle the yes/no headers logic. The third element in the new
form ("yes") is more or less superfluous (we could just hardcode it
everywhere as the only possible value for "true"), but without it, the
new form would look even more awkward. Suggestions welcome.
--Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#12537: Acknowledgement (support for git commit --amend/--signoff)
[not found] ` <handler.12537.B.134887753227659.ack@debbugs.gnu.org>
@ 2012-09-29 0:14 ` Dmitry Gutov
2012-10-01 3:16 ` Stefan Monnier
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Gutov @ 2012-09-29 0:14 UTC (permalink / raw)
To: 12537
[-- Attachment #1: Type: text/plain, Size: 25 bytes --]
Sorry, here's the patch.
[-- Attachment #2: amend.diff --]
[-- Type: text/plain, Size: 6170 bytes --]
=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog 2012-09-22 15:24:26 +0000
+++ lisp/ChangeLog 2012-09-28 23:47:18 +0000
@@ -1,3 +1,18 @@
+2012-09-28 Dmitry Gutov <dgutov@yandex.ru>
+
+ * vc/vc-git.el (vc-git-log-edit-toggle-signoff): New function.
+ (vc-git-log-edit-toggle-amend): New function.
+ (vc-git-log-edit-toggle-signoff): New function.
+ (vc-git-log-edit-mode): New major mode.
+ (vc-git-log-edit-mode-map): Keymap for it.
+ (vc-git-checkin): Handle "Amend" and "Sign-Off" headers.
+
+ * vc/log-edit.el (log-edit-font-lock-keywords): Allow non-letter
+ characters in header names, like hyphens.
+ (log-edit-toggle-header): New function.
+ (log-edit-extract-headers): Accept an alternative form of the
+ first argument's elements, allowing to handle yes/no headers.
+
2012-09-22 Chong Yidong <cyd@gnu.org>
* repeat.el (repeat): Doc fix (Bug#12348).
=== modified file 'lisp/vc/log-edit.el'
--- lisp/vc/log-edit.el 2012-08-28 16:01:59 +0000
+++ lisp/vc/log-edit.el 2012-09-28 23:17:10 +0000
@@ -352,7 +352,8 @@
(defvar log-edit-font-lock-keywords
;; Copied/inspired by message-font-lock-keywords.
`((log-edit-match-to-eoh
- (,(concat "^\\(\\([[:alpha:]]+\\):\\)" log-edit-header-contents-regexp)
+ (,(concat "^\\(\\([[:alpha:]][^: \n\t]+\\):\\)"
+ log-edit-header-contents-regexp)
(progn (goto-char (match-beginning 0)) (match-end 0)) nil
(1 (if (assoc (match-string 2) log-edit-headers-alist)
'log-edit-header
@@ -908,12 +909,47 @@
(insert "\n"))
log-edit-author))
+(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."
+ (let ((start (point))
+ (val t)
+ (line (concat name ": " value "\n")))
+ (save-restriction
+ (rfc822-goto-eoh)
+ (narrow-to-region (point-min) (point))
+ (goto-char (point-min))
+ (if (re-search-forward (concat "^" name ":"
+ log-edit-header-contents-regexp)
+ nil t)
+ (if (setq val (not (string= (match-string 1) value)))
+ (replace-match line t t)
+ (replace-match "" t t))
+ (insert line)))
+ (rfc822-goto-eoh)
+ (if (bobp)
+ (when (looking-at "\\([ \t]*\n\\)+")
+ (delete-region (match-beginning 0) (match-end 0)))
+ (delete-horizontal-space)
+ (unless (looking-at "\n")
+ (insert "\n")))
+ (when (> start (point))
+ (goto-char start))
+ val))
+
(defun log-edit-extract-headers (headers comment)
"Extract headers from COMMENT to form command line arguments.
HEADERS should be an alist with elements of the form (HEADER . CMDARG)
-associating header names to the corresponding cmdline option name and the
-result is then a list of the form (MSG CMDARG1 HDRTEXT1 CMDARG2 HDRTEXT2...).
-where MSG is the remaining text from STRING.
+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.
If \"Summary\" is not in HEADERS, then the \"Summary\" header is extracted
anyway and put back as the first line of MSG."
(with-temp-buffer
@@ -931,8 +967,11 @@
nil t)
(if (eq t (cdr header))
(setq summary (match-string 1))
- (push (match-string 1) res)
- (push (or (cdr header) (car header)) res))
+ (if (consp (cdr header))
+ (when (string= (match-string 1) (nth 2 header))
+ (push (nth 1 header) res))
+ (push (match-string 1) res)
+ (push (or (cdr header) (car header)) res)))
(replace-match "" t t)))
;; Remove header separator if the header is empty.
(widen)
=== modified file 'lisp/vc/vc-git.el'
--- lisp/vc/vc-git.el 2012-09-17 05:41:04 +0000
+++ lisp/vc/vc-git.el 2012-09-28 23:44:39 +0000
@@ -608,14 +608,39 @@
(defun vc-git-unregister (file)
(vc-git-command nil 0 file "rm" "-f" "--cached" "--"))
-(declare-function log-edit-extract-headers "log-edit" (headers string))
+(defun vc-git-log-edit-toggle-signoff ()
+ (interactive)
+ (log-edit-toggle-header "Sign-Off" "yes"))
+
+(defun vc-git-log-edit-toggle-amend ()
+ (interactive)
+ (when (log-edit-toggle-header "Amend" "yes")
+ (goto-char (point-max))
+ (unless (bolp) (insert "\n"))
+ (insert (with-output-to-string
+ (vc-git-command
+ standard-output 1 nil
+ "log" "--max-count=1" "--pretty=format:%B" "HEAD")))))
+
+(defvar vc-git-log-edit-mode-map
+ (let ((map (make-sparse-keymap "Git-Log-Edit")))
+ (define-key map "\C-c\C-s" 'vc-git-log-edit-toggle-signoff)
+ (define-key map "\C-c\C-e" 'vc-git-log-edit-toggle-amend)
+ map))
+
+(define-derived-mode vc-git-log-edit-mode log-edit-mode "*VC-log*"
+ "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}")
(defun vc-git-checkin (files _rev comment)
(let ((coding-system-for-write vc-git-commits-coding-system))
(apply 'vc-git-command nil 0 files
(nconc (list "commit" "-m")
(log-edit-extract-headers '(("Author" . "--author")
- ("Date" . "--date"))
+ ("Date" . "--date")
+ ("Amend" "--amend" "yes")
+ ("Sign-Off" "--signoff" "yes"))
comment)
(list "--only" "--")))))
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#12537: Acknowledgement (support for git commit --amend/--signoff)
2012-09-29 0:14 ` bug#12537: Acknowledgement (support for git commit --amend/--signoff) Dmitry Gutov
@ 2012-10-01 3:16 ` Stefan Monnier
2012-10-01 3:59 ` Dmitry Gutov
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Monnier @ 2012-10-01 3:16 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 12537
> - (,(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
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#12537: Acknowledgement (support for git commit --amend/--signoff)
2012-10-01 3:16 ` Stefan Monnier
@ 2012-10-01 3:59 ` Dmitry Gutov
2012-10-01 4:32 ` Chong Yidong
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Gutov @ 2012-10-01 3:59 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 12537
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#12537: Acknowledgement (support for git commit --amend/--signoff)
2012-10-01 3:59 ` Dmitry Gutov
@ 2012-10-01 4:32 ` Chong Yidong
2012-10-02 0:28 ` Dmitry Gutov
0 siblings, 1 reply; 6+ messages in thread
From: Chong Yidong @ 2012-10-01 4:32 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 12537
Dmitry Gutov <dgutov@yandex.ru> writes:
> 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?
No problem.
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#12537: Acknowledgement (support for git commit --amend/--signoff)
2012-10-01 4:32 ` Chong Yidong
@ 2012-10-02 0:28 ` Dmitry Gutov
0 siblings, 0 replies; 6+ messages in thread
From: Dmitry Gutov @ 2012-10-02 0:28 UTC (permalink / raw)
To: Stefan Monnier; +Cc: Chong Yidong, 12537-done
Installed, closing.
By the way, the separator line added in 110266 is a nice touch.
--Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-10-02 0:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
2012-10-01 3:59 ` Dmitry Gutov
2012-10-01 4:32 ` Chong Yidong
2012-10-02 0:28 ` Dmitry Gutov
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).