unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* support for git commit --amend/--signoff
@ 2010-06-11  6:19 Dan Nicolaescu
  2010-06-11  8:09 ` Juri Linkov
  0 siblings, 1 reply; 38+ messages in thread
From: Dan Nicolaescu @ 2010-06-11  6:19 UTC (permalink / raw)
  To: emacs-devel


It would be nice if VC commit would support --amend and --signoff for git.

The patch below adds key bindings to toggle some variables in the log-edit buffer.
(for --amend it also inserts the contents of the last commit message)

Now vc-git-checkin needs to know any of these variables has been set,
so that it can pass the right flags to the git commit command.

Other VC backends might want to implement similar things.

What is the best way to pass custom flags to the vc-checkin command?


=== modified file 'lisp/vc-git.el'
--- lisp/vc-git.el      2010-06-09 05:24:01 +0000
+++ lisp/vc-git.el      2010-06-10 00:00:50 +0000
@@ -550,6 +550,48 @@ or an empty string if none."
 
 (declare-function log-edit-extract-headers "log-edit" (headers string))
 
+(defvar vc-git-log-edit-signoff nil)
+
+(defvar vc-git-log-edit-amend nil)
+
+(defun vc-git-log-edit-toggle-signoff ()
+  (interactive)
+  (setq vc-git-log-edit-signoff (not vc-git-log-edit-signoff)))
+
+(defun vc-git-log-edit-toggle-amend ()
+  (interactive)
+  (unless vc-git-log-edit-amend
+    (vc-git-command (current-buffer) 1 nil
+                        "log" "--max-count=1" "--pretty=format:%s" "HEAD"))
+  (setq vc-git-log-edit-amend (not vc-git-log-edit-amend)))
+
+(defvar vc-git-log-edit-map
+  (let ((map (make-sparse-keymap "Git-Log-Edit"))
+  (menu-map (make-sparse-keymap)))
+
+    ;; FIXME: Are these key bindings OK?
+    (define-key map "\C-c\C-s" 'vc-git-log-edit-toggle-signoff)
+    (define-key map "\C-c\C-m" 'vc-git-log-edit-toggle-amend)
+
+    ;; FIXME: This does not work.  And it would be better if we could
+    ;; add items to the Log-Edit menu.
+    (define-key map [menu-bar vc-git-log-edit] (cons "Git-Log-Edit" menu-map))
+    (define-key menu-map [ts]
+      '(menu-item "Signoff" vc-git-log-edit-toggle-signoff
+                    :help "Toggle signoff"
+                            :button (:toggle . vc-git-log-edit-signoff)))
+    (define-key menu-map [ta]
+      '(menu-item "Amend" vc-git-log-edit-toggle-amend
+                    :help "Toggle amend, insert old log when turning it on"
+                            :button (:toggle . vc-git-log-edit-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', it has Git specific extensions."
+  (make-local-variable 'vc-git-log-edit-signoff)
+  (make-local-variable 'vc-git-log-edit-amend))
+
 (defun vc-git-checkin (files rev comment)
   (let ((coding-system-for-write vc-git-commits-coding-system))
     (apply 'vc-git-command nil 0 files




^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-11  6:19 support for git commit --amend/--signoff Dan Nicolaescu
@ 2010-06-11  8:09 ` Juri Linkov
  2010-06-11 13:23   ` Dan Nicolaescu
  0 siblings, 1 reply; 38+ messages in thread
From: Juri Linkov @ 2010-06-11  8:09 UTC (permalink / raw)
  To: Dan Nicolaescu; +Cc: emacs-devel

> It would be nice if VC commit would support --amend and --signoff for git.
>
> The patch below adds key bindings to toggle some variables in the log-edit buffer.
> (for --amend it also inserts the contents of the last commit message)
>
> Now vc-git-checkin needs to know any of these variables has been set,
> so that it can pass the right flags to the git commit command.
>
> Other VC backends might want to implement similar things.
>
> What is the best way to pass custom flags to the vc-checkin command?

The same way as you pass Author:, Summary: and Fixes:?

BTW, Author:, Summary: and Fixes: need key bindings too.
What about using the same key prefix as message-mode - `C-c C-f'?
I.e. like `C-c C-f C-s' (`message-goto-subject')
to add `C-c C-f C-s' (`log-edit-goto-subject'), etc.

-- 
Juri Linkov
http://www.jurta.org/emacs/



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-11  8:09 ` Juri Linkov
@ 2010-06-11 13:23   ` Dan Nicolaescu
  2010-06-11 14:18     ` Stefan Monnier
  0 siblings, 1 reply; 38+ messages in thread
From: Dan Nicolaescu @ 2010-06-11 13:23 UTC (permalink / raw)
  To: Juri Linkov; +Cc: emacs-devel

Juri Linkov <juri@jurta.org> writes:

>> It would be nice if VC commit would support --amend and --signoff for git.
>>
>> The patch below adds key bindings to toggle some variables in the log-edit buffer.
>> (for --amend it also inserts the contents of the last commit message)
>>
>> Now vc-git-checkin needs to know any of these variables has been set,
>> so that it can pass the right flags to the git commit command.
>>
>> Other VC backends might want to implement similar things.
>>
>> What is the best way to pass custom flags to the vc-checkin command?
>
> The same way as you pass Author:, Summary: and Fixes:?

Those are done by parsing the contents of the *VC-log* buffer.
For --amend I think we should have a different mechanism that does not
involve editing the text in the *VC-log* buffer, it's a more
destructive operation and it's not a good idea to allow the user to do
it by mistake by just placing an Amend: line in the buffer.

We need a different mechanism.  I added one such a mechanism at some
point, an optional `extra' argument to the vc-BACKEND-checkin command,
but that got removed...


>
> BTW, Author:, Summary: and Fixes: need key bindings too.
> What about using the same key prefix as message-mode - `C-c C-f'?
> I.e. like `C-c C-f C-s' (`message-goto-subject')
> to add `C-c C-f C-s' (`log-edit-goto-subject'), etc.
>
> -- 
> Juri Linkov
> http://www.jurta.org/emacs/



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-11 13:23   ` Dan Nicolaescu
@ 2010-06-11 14:18     ` Stefan Monnier
  2010-06-11 16:14       ` Štěpán Němec
                         ` (2 more replies)
  0 siblings, 3 replies; 38+ messages in thread
From: Stefan Monnier @ 2010-06-11 14:18 UTC (permalink / raw)
  To: Dan Nicolaescu; +Cc: Juri Linkov, emacs-devel

> For --amend I think we should have a different mechanism that does not
> involve editing the text in the *VC-log* buffer, it's a more
> destructive operation and it's not a good idea to allow the user to do
> it by mistake by just placing an Amend: line in the buffer.

You might be right, but I don't think it would be terribly bad to have it
be editable in the buffer (after all it's also just an "--amend" away when
committing on the command line).

I'm not sure what UI you're considering to use and what users would
want, because I've never used Git's amend feature.  But I think it
should be considered in the light of similar functionality in other VCS,
such as maybe "cvs admin -m" and DaRCS's "amend-record".

For PCL-CVS what I offered was a way to edit a log-message (typically
called from the log-view buffer).  But admittedly, "cvs admin -m" is
a fairly different beast from Git's amend.

If we only consider Git's and DaRCS's forms of amend, I'd say that the
"Amend:" header might be a good approach, and that it should specify the
revision/patch that's amended.  So for Git, you could have a command
that inserts "Amend: <SHA-1>" and then the backend could check that the
SHA-1 is the right one (which would avoid accidental use).


        Stefan



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-11 14:18     ` Stefan Monnier
@ 2010-06-11 16:14       ` Štěpán Němec
  2010-06-11 20:26         ` Stefan Monnier
  2010-06-11 17:34       ` Dan Nicolaescu
  2010-06-11 19:27       ` Juri Linkov
  2 siblings, 1 reply; 38+ messages in thread
From: Štěpán Němec @ 2010-06-11 16:14 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Juri Linkov, Dan Nicolaescu, emacs-devel

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> If we only consider Git's and DaRCS's forms of amend, I'd say that the
> "Amend:" header might be a good approach, and that it should specify the
> revision/patch that's amended.  So for Git, you could have a command
> that inserts "Amend: <SHA-1>" and then the backend could check that the
> SHA-1 is the right one (which would avoid accidental use).

No idea about Darcs, but `git commit --amend' always changes the tip of
the current branch, you can't specify another commit to amend; so the
above would not be useful in this case (same for --signoff).

Štěpán



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-11 14:18     ` Stefan Monnier
  2010-06-11 16:14       ` Štěpán Němec
@ 2010-06-11 17:34       ` Dan Nicolaescu
  2010-06-11 19:27       ` Juri Linkov
  2 siblings, 0 replies; 38+ messages in thread
From: Dan Nicolaescu @ 2010-06-11 17:34 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Juri Linkov, emacs-devel

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> For --amend I think we should have a different mechanism that does not
>> involve editing the text in the *VC-log* buffer, it's a more
>> destructive operation and it's not a good idea to allow the user to do
>> it by mistake by just placing an Amend: line in the buffer.
>
> You might be right, but I don't think it would be terribly bad to have it
> be editable in the buffer (after all it's also just an "--amend" away when
> committing on the command line).
>
> I'm not sure what UI you're considering to use and what users would
> want, because I've never used Git's amend feature.  But I think it
> should be considered in the light of similar functionality in other VCS,
> such as maybe "cvs admin -m" and DaRCS's "amend-record".
>
> For PCL-CVS what I offered was a way to edit a log-message (typically
> called from the log-view buffer).  But admittedly, "cvs admin -m" is
> a fairly different beast from Git's amend.

We already have a UI for that in VC: from the log buffer you can use
"e" to edit the commit log.

git commit --amend is different, it can be used to add more files to
the same commit, and it can only affect the last commit.

> If we only consider Git's and DaRCS's forms of amend, I'd say that the
> "Amend:" header might be a good approach, and that it should specify the
> revision/patch that's amended.  So for Git, you could have a command
> that inserts "Amend: <SHA-1>" and then the backend could check that the
> SHA-1 is the right one (which would avoid accidental use).

That looks like a bad UI: the user sees information that is
unnecessary, and might feel compelled to verify if the sha1 is indeed
to correct one (i.e. time wasted for no good reason).

Other VCs have commands that we might want to support in a similar manner, for example:
hg has --close-branch

So besides the markup in the VC-log buffer, we need another way to
pass arguments to the commit command.



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-11 14:18     ` Stefan Monnier
  2010-06-11 16:14       ` Štěpán Němec
  2010-06-11 17:34       ` Dan Nicolaescu
@ 2010-06-11 19:27       ` Juri Linkov
  2010-06-11 20:16         ` Dan Nicolaescu
  2010-06-11 20:35         ` Stefan Monnier
  2 siblings, 2 replies; 38+ messages in thread
From: Juri Linkov @ 2010-06-11 19:27 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Dan Nicolaescu, emacs-devel

> If we only consider Git's and DaRCS's forms of amend, I'd say that the
> "Amend:" header might be a good approach, and that it should specify the
> revision/patch that's amended.  So for Git, you could have a command
> that inserts "Amend: <SHA-1>" and then the backend could check that the
> SHA-1 is the right one (which would avoid accidental use).

A more general variant would be a header that allows to specify
command line arguments like

Arguments: --amend --signoff

This will provide interchangeability: the user will be able to copy
arguments from the *VC-log* buffer to the external command line and
back to the *VC-log* buffer to construct the necessary command line.

-- 
Juri Linkov
http://www.jurta.org/emacs/



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-11 19:27       ` Juri Linkov
@ 2010-06-11 20:16         ` Dan Nicolaescu
  2010-06-11 20:38           ` Juri Linkov
                             ` (2 more replies)
  2010-06-11 20:35         ` Stefan Monnier
  1 sibling, 3 replies; 38+ messages in thread
From: Dan Nicolaescu @ 2010-06-11 20:16 UTC (permalink / raw)
  To: Juri Linkov; +Cc: Stefan Monnier, emacs-devel

Juri Linkov <juri@jurta.org> writes:

>> If we only consider Git's and DaRCS's forms of amend, I'd say that the
>> "Amend:" header might be a good approach, and that it should specify the
>> revision/patch that's amended.  So for Git, you could have a command
>> that inserts "Amend: <SHA-1>" and then the backend could check that the
>> SHA-1 is the right one (which would avoid accidental use).
>
> A more general variant would be a header that allows to specify
> command line arguments like
>
> Arguments: --amend --signoff
>
> This will provide interchangeability: the user will be able to copy
> arguments from the *VC-log* buffer to the external command line and
> back to the *VC-log* buffer to construct the necessary command line.


That's very ugly from the UI point of view, it's not better than doing
the same thing from the command line directly.

More, for --amend it's desirable to copy the contents of the original
log in the *VC-log* buffer, so that the user can edit it, and check in
the modified version.



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-11 16:14       ` Štěpán Němec
@ 2010-06-11 20:26         ` Stefan Monnier
  2010-06-12  2:19           ` Dan Nicolaescu
  0 siblings, 1 reply; 38+ messages in thread
From: Stefan Monnier @ 2010-06-11 20:26 UTC (permalink / raw)
  To: Štěpán Němec; +Cc: Juri Linkov, Dan Nicolaescu, emacs-devel

>> If we only consider Git's and DaRCS's forms of amend, I'd say that the
>> "Amend:" header might be a good approach, and that it should specify the
>> revision/patch that's amended.  So for Git, you could have a command
>> that inserts "Amend: <SHA-1>" and then the backend could check that the
>> SHA-1 is the right one (which would avoid accidental use).

> No idea about Darcs, but `git commit --amend' always changes the tip of
> the current branch, you can't specify another commit to amend;

I know that (and DaRCS doesn't have such a limitation).

> so the above would not be useful in this case (same for --signoff).

It is not useful but it is needed because an empty header is normally
the same as no header, so just "Amend:" can't be enough, we'd have to
put something there.  I suggested a SHA-1 just because Dan though there
was a risk of people writing the header by mistake.


        Stefan



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-11 19:27       ` Juri Linkov
  2010-06-11 20:16         ` Dan Nicolaescu
@ 2010-06-11 20:35         ` Stefan Monnier
  1 sibling, 0 replies; 38+ messages in thread
From: Stefan Monnier @ 2010-06-11 20:35 UTC (permalink / raw)
  To: Juri Linkov; +Cc: Dan Nicolaescu, emacs-devel

> A more general variant would be a header that allows to specify
> command line arguments like

> Arguments: --amend --signoff

> This will provide interchangeability: the user will be able to copy
> arguments from the *VC-log* buffer to the external command line and
> back to the *VC-log* buffer to construct the necessary command line.

I like that.  I'm not sure it is the ultimate answer to the --amend
functionality, but it's a good feature to have, so the user can always
get what she wants by providing the args manually.


        Stefan



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-11 20:16         ` Dan Nicolaescu
@ 2010-06-11 20:38           ` Juri Linkov
  2010-06-11 23:48             ` W Dan Meyer
  2010-06-12  2:21             ` Dan Nicolaescu
  2010-06-11 23:44           ` Thien-Thi Nguyen
  2010-06-12 20:15           ` Stefan Monnier
  2 siblings, 2 replies; 38+ messages in thread
From: Juri Linkov @ 2010-06-11 20:38 UTC (permalink / raw)
  To: Dan Nicolaescu; +Cc: Stefan Monnier, emacs-devel

>> Arguments: --amend --signoff
>>
>> This will provide interchangeability: the user will be able to copy
>> arguments from the *VC-log* buffer to the external command line and
>> back to the *VC-log* buffer to construct the necessary command line.
>
> That's very ugly from the UI point of view, it's not better than doing
> the same thing from the command line directly.

In Emacs it's easier to select files and to write a log message
than doing the same in the command line.

But currently vc mode is limited to a small set of supported arguments.
Allowing to specify the command line arguments will give the user the
full power of the command line when doing vc operations from Emacs.

> More, for --amend it's desirable to copy the contents of the original
> log in the *VC-log* buffer, so that the user can edit it, and check in
> the modified version.

Maybe, --amend is a special case that requires special processing.
But generally it's also desirable to be able to specify the command line
arguments from the *VC-log* buffer.

-- 
Juri Linkov
http://www.jurta.org/emacs/



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-11 20:16         ` Dan Nicolaescu
  2010-06-11 20:38           ` Juri Linkov
@ 2010-06-11 23:44           ` Thien-Thi Nguyen
  2010-06-12 20:15           ` Stefan Monnier
  2 siblings, 0 replies; 38+ messages in thread
From: Thien-Thi Nguyen @ 2010-06-11 23:44 UTC (permalink / raw)
  To: emacs-devel

() Dan Nicolaescu <dann@gnu.org>
() Fri, 11 Jun 2010 16:16:18 -0400

   Juri Linkov <juri@jurta.org> writes:

   > This will provide interchangeability: the user will be able to copy
   > arguments from the *VC-log* buffer to the external command line and
   > back to the *VC-log* buffer to construct the necessary command line.

   That's very ugly from the UI point of view, it's not better than doing
   the same thing from the command line directly.

I disagree on the aesthetics.  Straightforward editable text is what
Emacs is good for.  Such an interface invites user-defined UI.  For
example, keyboard macros are compatible with it, as are dedicated
commands (what you seem to be pushing), as are advised functions, etc.
It is also extensible in the other direction (for people who have
extended, or ~/bin-wrapped programs).

Perlis sez: It is better to have 100 functions operate on one data
structure than 10 functions on 10 data structures.

thi



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-11 20:38           ` Juri Linkov
@ 2010-06-11 23:48             ` W Dan Meyer
  2010-06-12 20:23               ` Juri Linkov
  2010-06-12  2:21             ` Dan Nicolaescu
  1 sibling, 1 reply; 38+ messages in thread
From: W Dan Meyer @ 2010-06-11 23:48 UTC (permalink / raw)
  To: Juri Linkov; +Cc: Dan Nicolaescu, Stefan Monnier, emacs-devel

Juri Linkov <juri@jurta.org> writes:

>>> Arguments: --amend --signoff
>>>
>>> This will provide interchangeability: the user will be able to copy
>>> arguments from the *VC-log* buffer to the external command line and
>>> back to the *VC-log* buffer to construct the necessary command line.
>>
>> That's very ugly from the UI point of view, it's not better than doing
>> the same thing from the command line directly.
>
> In Emacs it's easier to select files and to write a log message
> than doing the same in the command line.
>
> But currently vc mode is limited to a small set of supported arguments.
> Allowing to specify the command line arguments will give the user the
> full power of the command line when doing vc operations from Emacs.

Before I started using VC for real, I used to issue (and I still do it
sometimes), M-&, and just type the vcs command - this way I've had a
some completion, asynchronous command and output in a separate
buffer. Maybe something like C-x g . would be good but:

- with completion of possible commands (the interface is not really
  different, it is always a driver name and then token representing
  command)

- adjusting the output (choosing output buffer mode, being quiet, etc.)

- being quiet and synchronous for some class of commands

This would be based on the assumption that almost every time. `log'
command produces log, diff produces `diff' and `rm' could be silent and
not show the buffer (plus it could be possibly synchronous). So
adjusting those two things (completion and processing output) would give
certain advantages but allowed user to type any flavour of command he
wished.

BTW: There could be in Emacs sort of way of specifying grammar for
parsing command-line commands apart from usual user interfaces. It could
be generated from <command> -h. That could allow completion with online
help, and if the format is -h standarised enough even completion of
certain types of entries. (Like <file>).

>
>> More, for --amend it's desirable to copy the contents of the original
>> log in the *VC-log* buffer, so that the user can edit it, and check in
>> the modified version.
>
> Maybe, --amend is a special case that requires special processing.
> But generally it's also desirable to be able to specify the command line
> arguments from the *VC-log* buffer.

Wojciech



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-11 20:26         ` Stefan Monnier
@ 2010-06-12  2:19           ` Dan Nicolaescu
  2010-06-12 19:59             ` Juri Linkov
  2010-06-12 20:19             ` Stefan Monnier
  0 siblings, 2 replies; 38+ messages in thread
From: Dan Nicolaescu @ 2010-06-12  2:19 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Juri Linkov, Štěpán Němec, emacs-devel

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>>> If we only consider Git's and DaRCS's forms of amend, I'd say that the
>>> "Amend:" header might be a good approach, and that it should specify the
>>> revision/patch that's amended.  So for Git, you could have a command
>>> that inserts "Amend: <SHA-1>" and then the backend could check that the
>>> SHA-1 is the right one (which would avoid accidental use).
>
>> No idea about Darcs, but `git commit --amend' always changes the tip of
>> the current branch, you can't specify another commit to amend;
>
> I know that (and DaRCS doesn't have such a limitation).
>> so the above would not be useful in this case (same for --signoff).
>
> It is not useful but it is needed because an empty header is normally
> the same as no header, so just "Amend:" can't be enough, we'd have to
> put something there.  I suggested a SHA-1 just because Dan though there
> was a risk of people writing the header by mistake.

That would be a very strong argument agains doing it that way for
amend then.  Coupled with the fact that for amend we actually want to
insert the previous comit log, that calls for a different solution.

We could have a log-edit-extra-flags function that computes a set of
extra flags, and pass those flags to vc-git-checkin.



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-11 20:38           ` Juri Linkov
  2010-06-11 23:48             ` W Dan Meyer
@ 2010-06-12  2:21             ` Dan Nicolaescu
  1 sibling, 0 replies; 38+ messages in thread
From: Dan Nicolaescu @ 2010-06-12  2:21 UTC (permalink / raw)
  To: Juri Linkov; +Cc: Stefan Monnier, emacs-devel

Juri Linkov <juri@jurta.org> writes:

>>> Arguments: --amend --signoff
>>>
>>> This will provide interchangeability: the user will be able to copy
>>> arguments from the *VC-log* buffer to the external command line and
>>> back to the *VC-log* buffer to construct the necessary command line.
>>
>> That's very ugly from the UI point of view, it's not better than doing
>> the same thing from the command line directly.
>
> In Emacs it's easier to select files and to write a log message
> than doing the same in the command line.
>
> But currently vc mode is limited to a small set of supported arguments.
> Allowing to specify the command line arguments will give the user the
> full power of the command line when doing vc operations from Emacs.
>
>> More, for --amend it's desirable to copy the contents of the original
>> log in the *VC-log* buffer, so that the user can edit it, and check in
>> the modified version.
>
> Maybe, --amend is a special case that requires special processing.
> But generally it's also desirable to be able to specify the command line
> arguments from the *VC-log* buffer.

I'd like to support the special case well, it's a more frequent operation.



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-12  2:19           ` Dan Nicolaescu
@ 2010-06-12 19:59             ` Juri Linkov
  2010-06-12 20:19             ` Stefan Monnier
  1 sibling, 0 replies; 38+ messages in thread
From: Juri Linkov @ 2010-06-12 19:59 UTC (permalink / raw)
  To: Dan Nicolaescu; +Cc: Stefan Monnier, emacs-devel

> We could have a log-edit-extra-flags function that computes a set of
> extra flags, and pass those flags to vc-git-checkin.

Is it like log-edit-extract-headers?  Instead of extracting headers
it could extract variables.

-- 
Juri Linkov
http://www.jurta.org/emacs/



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-11 20:16         ` Dan Nicolaescu
  2010-06-11 20:38           ` Juri Linkov
  2010-06-11 23:44           ` Thien-Thi Nguyen
@ 2010-06-12 20:15           ` Stefan Monnier
  2 siblings, 0 replies; 38+ messages in thread
From: Stefan Monnier @ 2010-06-12 20:15 UTC (permalink / raw)
  To: Dan Nicolaescu; +Cc: Juri Linkov, emacs-devel

> More, for --amend it's desirable to copy the contents of the original
> log in the *VC-log* buffer, so that the user can edit it, and check in
> the modified version.

I'd tend to agree.  In this case, we'd probably want to provide this via
a separate command (we could call it vc-amend).


        Stefan



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-12  2:19           ` Dan Nicolaescu
  2010-06-12 19:59             ` Juri Linkov
@ 2010-06-12 20:19             ` Stefan Monnier
  2010-06-19  6:38               ` Dan Nicolaescu
  1 sibling, 1 reply; 38+ messages in thread
From: Stefan Monnier @ 2010-06-12 20:19 UTC (permalink / raw)
  To: Dan Nicolaescu; +Cc: Juri Linkov, Štěpán Němec, emacs-devel

> We could have a log-edit-extra-flags function that computes a set of
> extra flags, and pass those flags to vc-git-checkin.

If amend is triggered from a new command (like vc-amend), then there's no
need for any special support in log-edit.  We could easily store the
relevant info in log-edit-callback or in vc-specific
buffer-local variables.


        Stefan



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-11 23:48             ` W Dan Meyer
@ 2010-06-12 20:23               ` Juri Linkov
  0 siblings, 0 replies; 38+ messages in thread
From: Juri Linkov @ 2010-06-12 20:23 UTC (permalink / raw)
  To: W Dan Meyer; +Cc: Dan Nicolaescu, Stefan Monnier, emacs-devel

> BTW: There could be in Emacs sort of way of specifying grammar for
> parsing command-line commands apart from usual user interfaces. It could
> be generated from <command> -h. That could allow completion with online
> help, and if the format is -h standarised enough even completion of
> certain types of entries. (Like <file>).

Isn't it what pcomplete.el, pcmpl-cvs.el and other pcmpl-*.el try to do?

-- 
Juri Linkov
http://www.jurta.org/emacs/



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-12 20:19             ` Stefan Monnier
@ 2010-06-19  6:38               ` Dan Nicolaescu
  2010-06-23  7:17                 ` Stefan Monnier
  0 siblings, 1 reply; 38+ messages in thread
From: Dan Nicolaescu @ 2010-06-19  6:38 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Juri Linkov, Štěpán Němec, emacs-devel

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> We could have a log-edit-extra-flags function that computes a set of
>> extra flags, and pass those flags to vc-git-checkin.
>
> If amend is triggered from a new command (like vc-amend), then there's no
> need for any special support in log-edit.  We could easily store the
> relevant info in log-edit-callback or in vc-specific
> buffer-local variables.

The problem with that is that vc-amend does not integrate very well
with C-x v v and it is very Git specific, and given the reluctance of
some other VCs to change history, it does not seem that it will be
generalized.
More, it does not solve the problem of --signoff (which might be adopted by other VCs).

Here's a solution that takes care of all problems and it is extensible.
I adds one line to log-edit.el and two to vc.el.
vc-git-log-edit-get-extra-flags-function does the work in vc-git.el



=== modified file 'lisp/vc/log-edit.el'
--- lisp/vc/log-edit.el	2010-06-12 17:14:43 +0000
+++ lisp/vc/log-edit.el	2010-06-16 17:37:47 +0000
@@ -189,6 +189,7 @@ when this variable is set to nil.")
 (defvar log-edit-callback nil)
 (defvar log-edit-diff-function nil)
 (defvar log-edit-listfun nil)
+(defvar log-edit-get-extra-flags-function nil)
 
 (defvar log-edit-parent-buffer nil)
 

=== modified file 'lisp/vc/vc.el'
--- lisp/vc/vc.el	2010-06-11 19:09:57 +0000
+++ lisp/vc/vc.el	2010-06-16 19:09:37 +0000
@@ -1398,7 +1406,9 @@ Runs the normal hooks `vc-before-checkin
           ;; vc-checkin-switches, but 'the' local buffer is
           ;; not a well-defined concept for filesets.
           (progn
-            (vc-call-backend backend 'checkin files rev comment)
+            (vc-call-backend backend 'checkin files rev comment
+			     (when log-edit-get-extra-flags-function
+			       (funcall log-edit-get-extra-flags-function)))
             (mapc 'vc-delete-automatic-version-backups files))
           `((vc-state . up-to-date)
             (vc-checkout-time . ,(nth 5 (file-attributes file)))

=== modified file 'lisp/vc/vc-annotate.el'
=== modified file 'lisp/vc/vc-bzr.el'
=== modified file 'lisp/vc/vc-git.el'
--- lisp/vc/vc-git.el	2010-06-11 19:09:57 +0000
+++ lisp/vc/vc-git.el	2010-06-16 18:10:00 +0000
@@ -550,13 +550,65 @@ or an empty string if none."
 
 (declare-function log-edit-extract-headers "log-edit" (headers string))
 
-(defun vc-git-checkin (files rev comment)
+(defvar vc-git-log-edit-signoff nil)
+
+(defvar vc-git-log-edit-amend nil)
+
+(defun vc-git-log-edit-toggle-signoff ()
+  (interactive)
+  (setq vc-git-log-edit-signoff (not vc-git-log-edit-signoff)))
+
+(defun vc-git-log-edit-toggle-amend ()
+  (interactive)
+  (unless vc-git-log-edit-amend
+    (vc-git-command (current-buffer) 1 nil
+		    "log" "--max-count=1" "--pretty=format:%s" "HEAD"))
+  (setq vc-git-log-edit-amend (not vc-git-log-edit-amend)))
+
+(defvar vc-git-log-edit-map
+  (let ((map (make-sparse-keymap "Git-Log-Edit"))
+	(menu-map (make-sparse-keymap)))
+
+    ;; FIXME: Are these key bindings OK?
+    (define-key map "\C-c\C-s" 'vc-git-log-edit-toggle-signoff)
+    (define-key map "\C-c\C-m" 'vc-git-log-edit-toggle-amend)
+
+    ;; FIXME: This does not work.  And it would be better if we could
+    ;; add items to the Log-Edit menu.
+    (define-key map [menu-bar vc-git-log-edit] (cons "Git-Log-Edit" menu-map))
+    (define-key menu-map [ts]
+      '(menu-item "Signoff" vc-git-log-edit-toggle-signoff
+		  :help "Toggle signoff"
+		  :button (:toggle . vc-git-log-edit-signoff)))
+    (define-key menu-map [ta]
+      '(menu-item "Amend" vc-git-log-edit-toggle-amend
+		  :help "Toggle amend, insert old log when turning it on"
+		  :button (:toggle . vc-git-log-edit-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', it has Git specific extensions."
+  (set (make-local-variable 'log-edit-get-extra-flags-function)
+       'vc-git-log-edit-get-extra-flags-function)
+  (make-local-variable 'vc-git-log-edit-signoff)
+  (make-local-variable 'vc-git-log-edit-amend))
+
+(defun vc-git-log-edit-get-extra-flags-function ()
+  (let (result)
+    (when vc-git-log-edit-amend
+      (push "--amend" result))
+    (when vc-git-log-edit-signoff
+      (push "--signoff" result))))
+
+(defun vc-git-checkin (files rev comment &optional extra-flags)
   (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"))
                                             comment)
+		  extra-flags
                   (list "--only" "--")))))
 
 (defun vc-git-find-revision (file rev buffer)




^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-19  6:38               ` Dan Nicolaescu
@ 2010-06-23  7:17                 ` Stefan Monnier
  2010-06-23  7:45                   ` David Kastrup
                                     ` (2 more replies)
  0 siblings, 3 replies; 38+ messages in thread
From: Stefan Monnier @ 2010-06-23  7:17 UTC (permalink / raw)
  To: Dan Nicolaescu; +Cc: Juri Linkov, Štěpán Němec, emacs-devel

>>> We could have a log-edit-extra-flags function that computes a set of
>>> extra flags, and pass those flags to vc-git-checkin.
>> If amend is triggered from a new command (like vc-amend), then there's no
>> need for any special support in log-edit.  We could easily store the
>> relevant info in log-edit-callback or in vc-specific
>> buffer-local variables.
> The problem with that is that vc-amend does not integrate very well
> with C-x v v

That doesn't bother me too much.  I wish we could move further away from
C-x v v since I find it doesn't really work for modern VCSes.

> and it is very Git specific,

Not sure about the "very", but in any case I don't see why that would
argue for integration inside "commit" rather than for the addition of
a separate command.

> and given the reluctance of some other VCs to change history, it does
> not seem that it will be generalized.

DaRCS already supports a more general form of Git's amend.  Bazaar is
unlikely to go there, tho, indeed (although they are not opposed to
adding a more limited command to modify commit-comments).  Don't know
about others.

> More, it does not solve the problem of --signoff (which might be
> adopted by other VCs).

What's the problem with sign-off?  It seems this one fits perfectly well
inside a "Signed-Off-By:" header.

> Here's a solution that takes care of all problems and it is extensible.
> I adds one line to log-edit.el and two to vc.el.
> vc-git-log-edit-get-extra-flags-function does the work in vc-git.el

Why add a var "log-edit-get-extra-flags-function" that's never used by
log-edit?

BTW, I think you can get what you want already without touching any of
vc.el and log-edit.el by making vc-git-log-edit-toggle-amend tweak the
log-edit-callback variable.

Maybe adding an `extra-flags' arg to `checkin' is the better approach,
but adding it everywhere just for the sake of Git doesn't sound too
good, which is why I'm resisting it.

Also as a user I'd *really* like to have a clear visual feedback about
the fact that I'm amending something, which is another reason why having
it inside the headers is an attractive direction.


        Stefan



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-23  7:17                 ` Stefan Monnier
@ 2010-06-23  7:45                   ` David Kastrup
  2010-06-23  9:00                   ` Miles Bader
  2010-06-23 18:45                   ` Dan Nicolaescu
  2 siblings, 0 replies; 38+ messages in thread
From: David Kastrup @ 2010-06-23  7:45 UTC (permalink / raw)
  To: emacs-devel

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>>> We could have a log-edit-extra-flags function that computes a set of
>>>> extra flags, and pass those flags to vc-git-checkin.
>>> If amend is triggered from a new command (like vc-amend), then there's no
>>> need for any special support in log-edit.  We could easily store the
>>> relevant info in log-edit-callback or in vc-specific
>>> buffer-local variables.
>> The problem with that is that vc-amend does not integrate very well
>> with C-x v v
>
> That doesn't bother me too much.  I wish we could move further away from
> C-x v v since I find it doesn't really work for modern VCSes.
>
>> and it is very Git specific,
>
> Not sure about the "very", but in any case I don't see why that would
> argue for integration inside "commit" rather than for the addition of
> a separate command.

I thing that a zero prefix argument to C-x v v would feel quite natural
for "amend".

-- 
David Kastrup




^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-23  7:17                 ` Stefan Monnier
  2010-06-23  7:45                   ` David Kastrup
@ 2010-06-23  9:00                   ` Miles Bader
  2010-06-23 18:55                     ` Dan Nicolaescu
  2010-06-23 18:45                   ` Dan Nicolaescu
  2 siblings, 1 reply; 38+ messages in thread
From: Miles Bader @ 2010-06-23  9:00 UTC (permalink / raw)
  To: Stefan Monnier
  Cc: Juri Linkov, Dan Nicolaescu, Štěpán Němec,
	emacs-devel

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> The problem with that is that vc-amend does not integrate very well
>> with C-x v v
>
> That doesn't bother me too much.  I wish we could move further away from
> C-x v v since I find it doesn't really work for modern VCSes.

It doesn't...?  I mean, for single-file checkins[*], it works fine, right?

[*] Maybe rare with Emacs, because it uses changelogs, but fairly
common, IME, in projects that don't, especially if you use a "commit
really often" style

I do admit, I'd like some better frobs to do multiple-file commits
without bringing up a vc-dir...

-Miles

-- 
`...the Soviet Union was sliding in to an economic collapse so comprehensive
 that in the end its factories produced not goods but bads: finished products
 less valuable than the raw materials they were made from.'  [The Economist]



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-23  7:17                 ` Stefan Monnier
  2010-06-23  7:45                   ` David Kastrup
  2010-06-23  9:00                   ` Miles Bader
@ 2010-06-23 18:45                   ` Dan Nicolaescu
  2010-06-23 22:04                     ` Stefan Monnier
  2 siblings, 1 reply; 38+ messages in thread
From: Dan Nicolaescu @ 2010-06-23 18:45 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Juri Linkov, Štěpán Němec, emacs-devel

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>>> We could have a log-edit-extra-flags function that computes a set of
>>>> extra flags, and pass those flags to vc-git-checkin.
>>> If amend is triggered from a new command (like vc-amend), then there's no
>>> need for any special support in log-edit.  We could easily store the
>>> relevant info in log-edit-callback or in vc-specific
>>> buffer-local variables.
>> The problem with that is that vc-amend does not integrate very well
>> with C-x v v
>
> That doesn't bother me too much.  I wish we could move further away from
> C-x v v since I find it doesn't really work for modern VCSes.

Not sure why you think so, but please don't try to take it away, it
has been in use for so long, and it's still very useful.

>> and it is very Git specific,
>
> Not sure about the "very", but in any case I don't see why that would
> argue for integration inside "commit" rather than for the addition of
> a separate command.

You can do C-x v v to commit one file, then C-x v v and switch ammend
on to add another file to the same commit.

Also --amend it's still a property of commit, it's not a different
command, why make it a different command in emacs?


>> and given the reluctance of some other VCs to change history, it does
>> not seem that it will be generalized.
>
> DaRCS already supports a more general form of Git's amend.  Bazaar is

No idea, we don't support DaRCS at all...

> unlikely to go there, tho, indeed (although they are not opposed to
> adding a more limited command to modify commit-comments).  Don't know
> about others.


>
>> More, it does not solve the problem of --signoff (which might be
>> adopted by other VCs).
>
> What's the problem with sign-off?  It seems this one fits perfectly well
> inside a "Signed-Off-By:" header.

Signed-Off-By: as an empty header does not work.  --signoff does not have any arguments.


>> Here's a solution that takes care of all problems and it is extensible.
>> I adds one line to log-edit.el and two to vc.el.
>> vc-git-log-edit-get-extra-flags-function does the work in vc-git.el
>
> Why add a var "log-edit-get-extra-flags-function" that's never used by
> log-edit?

It's used by vc-checkin, the main user of log-edit and by the modes derived from log-edit.
Do you have a better proposal?

> BTW, I think you can get what you want already without touching any of
> vc.el and log-edit.el by making vc-git-log-edit-toggle-amend tweak the
> log-edit-callback variable.

That sounds fragile and much harder to understand for someone trying
to develop vc.el 5 years from now ...


> Maybe adding an `extra-flags' arg to `checkin' is the better approach,
> but adding it everywhere just for the sake of Git doesn't sound too
> good, which is why I'm resisting it.

Why do you think that adding a top level vc-amend command that will
only be used by git better than adding an unused argument (for now, it
can become useful in the future) to other backends?

> Also as a user I'd *really* like to have a clear visual feedback about
> the fact that I'm amending something, which is another reason why having
> it inside the headers is an attractive direction.

For that in my local tree I have a minor mode instead of
vc-git-log-edit-toggle-amend, it shows "amend" in the modeline.




^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-23  9:00                   ` Miles Bader
@ 2010-06-23 18:55                     ` Dan Nicolaescu
  0 siblings, 0 replies; 38+ messages in thread
From: Dan Nicolaescu @ 2010-06-23 18:55 UTC (permalink / raw)
  To: Miles Bader
  Cc: Juri Linkov, Štěpán Němec, Stefan Monnier,
	emacs-devel

Miles Bader <miles@gnu.org> writes:

> I do admit, I'd like some better frobs to do multiple-file commits
> without bringing up a vc-dir...

What would you like to do?



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-23 18:45                   ` Dan Nicolaescu
@ 2010-06-23 22:04                     ` Stefan Monnier
  2010-06-23 23:23                       ` Dan Nicolaescu
  0 siblings, 1 reply; 38+ messages in thread
From: Stefan Monnier @ 2010-06-23 22:04 UTC (permalink / raw)
  To: Dan Nicolaescu; +Cc: Juri Linkov, Štěpán Němec, emacs-devel

>> That doesn't bother me too much.  I wish we could move further away from
>> C-x v v since I find it doesn't really work for modern VCSes.
> Not sure why you think so, but please don't try to take it away, it
> has been in use for so long, and it's still very useful.

I guess "C-x v v" in itself is not a bad key and it does something
useful, so in this sense I don't intend to get rid of it, but its
binding to vc-next-action is not very useful, since in modern VCSes, the
only thing it can ever do (or close enough) is "commit", so the name
is misleading.

> Also --amend it's still a property of commit, it's not a different
> command, why make it a different command in emacs?

Yes, that's the main argument in favor of keeping it within the
"vc-commit" command (currently misnamed vc-next-action).

>>> and given the reluctance of some other VCs to change history, it does
>>> not seem that it will be generalized.
>> DaRCS already supports a more general form of Git's amend.  Bazaar is
> No idea, we don't support DaRCS at all...

We do want to support DaRCS, tho.

>>> More, it does not solve the problem of --signoff (which might be
>>> adopted by other VCs).
>> What's the problem with sign-off?  It seems this one fits perfectly well
>> inside a "Signed-Off-By:" header.
> Signed-Off-By: as an empty header does not work.  --signoff does not
> have any arguments.

Oh, that's why you tend to treat it like --amend.  I see it's a problem,
indeed, but I don't think that storing it into a buffer-local variable
is a good answer.  I'd rather have a "Sign-Off: yes" (you can still
have a vc-git-log-edit-toggle-signoff command that adds/removes such
a header).

I was thinking of (re)using the Signed-Off-By thingy used by Linux kernel
developers, but I'm not exactly sure of how it's intended to be used, so
I'm not completely clear if and how it could be cleanly integrated with
VC's need to support --signoff.

>> Why add a var "log-edit-get-extra-flags-function" that's never used by
>> log-edit?
> It's used by vc-checkin, the main user of log-edit and by the modes
> derived from log-edit.  Do you have a better proposal?

Yes: use a name that starts with "vc-" since it's a variable added for
VC, and used exclusively by VC.

>> Also as a user I'd *really* like to have a clear visual feedback about
>> the fact that I'm amending something, which is another reason why having
>> it inside the headers is an attractive direction.
> For that in my local tree I have a minor mode instead of
> vc-git-log-edit-toggle-amend, it shows "amend" in the modeline.

Why not show it inside the buffer (e.g. in the header ;-) instead?


        Stefan



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-23 22:04                     ` Stefan Monnier
@ 2010-06-23 23:23                       ` Dan Nicolaescu
  2010-06-24 21:03                         ` Stefan Monnier
  0 siblings, 1 reply; 38+ messages in thread
From: Dan Nicolaescu @ 2010-06-23 23:23 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Juri Linkov, Štěpán Němec, emacs-devel

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Also --amend it's still a property of commit, it's not a different
>> command, why make it a different command in emacs?
>
> Yes, that's the main argument in favor of keeping it within the
> "vc-commit" command (currently misnamed vc-next-action).
>
>>>> and given the reluctance of some other VCs to change history, it does
>>>> not seem that it will be generalized.
>>> DaRCS already supports a more general form of Git's amend.  Bazaar is
>> No idea, we don't support DaRCS at all...
>
> We do want to support DaRCS, tho.
>
>>>> More, it does not solve the problem of --signoff (which might be
>>>> adopted by other VCs).
>>> What's the problem with sign-off?  It seems this one fits perfectly well
>>> inside a "Signed-Off-By:" header.
>> Signed-Off-By: as an empty header does not work.  --signoff does not
>> have any arguments.
>
> Oh, that's why you tend to treat it like --amend.  I see it's a problem,
> indeed, but I don't think that storing it into a buffer-local variable
> is a good answer.  I'd rather have a "Sign-Off: yes" (you can still

But that means that 
Sign-off: no | off
will still add --signoff.  Which is confusing and wrong.
More
Sign-off: foo@bar.com
gives the impression that it will use foo@bar.com to sign off, but that's not the case.
--signoff does not have an argument, adding one artificially just
creates unneeded confusion.

> I was thinking of (re)using the Signed-Off-By thingy used by Linux kernel
> developers, but I'm not exactly sure of how it's intended to be used, so
> I'm not completely clear if and how it could be cleanly integrated with
> VC's need to support --signoff.

I've already presented a solution that works, and it does not create
any confusion.

>>> Why add a var "log-edit-get-extra-flags-function" that's never used by
>>> log-edit?
>> It's used by vc-checkin, the main user of log-edit and by the modes
>> derived from log-edit.  Do you have a better proposal?
>
> Yes: use a name that starts with "vc-" since it's a variable added for
> VC, and used exclusively by VC.

OK.

>>> Also as a user I'd *really* like to have a clear visual feedback about
>>> the fact that I'm amending something, which is another reason why having
>>> it inside the headers is an attractive direction.
>> For that in my local tree I have a minor mode instead of
>> vc-git-log-edit-toggle-amend, it shows "amend" in the modeline.
>
> Why not show it inside the buffer (e.g. in the header ;-) instead?

Because we want to insert the previous log when using --amend, so it's
better to use a command instead of a header.



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-23 23:23                       ` Dan Nicolaescu
@ 2010-06-24 21:03                         ` Stefan Monnier
  2010-06-24 21:18                           ` Dan Nicolaescu
  0 siblings, 1 reply; 38+ messages in thread
From: Stefan Monnier @ 2010-06-24 21:03 UTC (permalink / raw)
  To: Dan Nicolaescu; +Cc: Juri Linkov, Štěpán Němec, emacs-devel

>> Oh, that's why you tend to treat it like --amend.  I see it's a problem,
>> indeed, but I don't think that storing it into a buffer-local variable
>> is a good answer.  I'd rather have a "Sign-Off: yes" (you can still

> But that means that 
> Sign-off: no | off
> will still add --signoff.

Not at all.  The code gets to decide which values mean "add a --signoff
argument".  I'd start with only accepting "yes".

> Which is confusing and wrong.

It would be indeed, but that's not what I'm suggesting we do.

> More
> Sign-off: foo@bar.com
> gives the impression that it will use foo@bar.com to sign off, but that's not the case.

For the values other than "yes" we can highlight them with
font-lock-warning-face and the backend can prompt the user when faced
with such unclear values.  We should also do something similar for
unknown values of the "Fixed:" field, BTW.

>> Why not show it inside the buffer (e.g. in the header ;-) instead?
> Because we want to insert the previous log when using --amend, so it's
> better to use a command instead of a header.

The question is not "a command vs a header" but "a variable vs
a header".  In both cases we will want to provide a command (which will
fetch the previous log, etc...).


        Stefan



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-24 21:03                         ` Stefan Monnier
@ 2010-06-24 21:18                           ` Dan Nicolaescu
  2010-06-24 22:25                             ` Stefan Monnier
  0 siblings, 1 reply; 38+ messages in thread
From: Dan Nicolaescu @ 2010-06-24 21:18 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Juri Linkov, Štěpán Němec, emacs-devel

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> Oh, that's why you tend to treat it like --amend.  I see it's a problem,
>>> indeed, but I don't think that storing it into a buffer-local variable
>>> is a good answer.  I'd rather have a "Sign-Off: yes" (you can still
>
>> But that means that 
>> Sign-off: no | off
>> will still add --signoff.
>
> Not at all.  The code gets to decide which values mean "add a --signoff
> argument".  I'd start with only accepting "yes".
>
>> Which is confusing and wrong.
>
> It would be indeed, but that's not what I'm suggesting we do.
>
>> More
>> Sign-off: foo@bar.com
>> gives the impression that it will use foo@bar.com to sign off, but that's not the case.
>
> For the values other than "yes" we can highlight them with
> font-lock-warning-face and the backend can prompt the user when faced
> with such unclear values.  We should also do something similar for
> unknown values of the "Fixed:" field, BTW.

So now let's go back to the origin of this: there's a lot of code that
needs to be added just to deal with the various possibilities for just
the Signoff header, and even more for Ammend.

Can you please show how this is better than adding a single argument
to the vc-checkin method?  (For which the code already exists).

>>> Why not show it inside the buffer (e.g. in the header ;-) instead?
>> Because we want to insert the previous log when using --amend, so it's
>> better to use a command instead of a header.
>
> The question is not "a command vs a header" but "a variable vs
> a header".  In both cases we will want to provide a command (which will
> fetch the previous log, etc...).

So what happens if one deletes the Ammend: header?   Accidentally or not?

--amend and --signoff simply do not fit the header paradigm.  
Can we please admit that and move on?



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-24 21:18                           ` Dan Nicolaescu
@ 2010-06-24 22:25                             ` Stefan Monnier
  2010-06-24 23:14                               ` Dan Nicolaescu
  0 siblings, 1 reply; 38+ messages in thread
From: Stefan Monnier @ 2010-06-24 22:25 UTC (permalink / raw)
  To: Dan Nicolaescu; +Cc: Juri Linkov, Štěpán Němec, emacs-devel

> Can you please show how this is better than adding a single argument
> to the vc-checkin method?  (For which the code already exists).

It makes more of the state plainly visible to the user, editable with
Emacs's usual commands, rather than hidden in variables that are much
more difficult for the user to control.
I.e. it's more Emacsy by keeping things shallow.

>>>> Why not show it inside the buffer (e.g. in the header ;-) instead?
>>> Because we want to insert the previous log when using --amend, so it's
>>> better to use a command instead of a header.
>> The question is not "a command vs a header" but "a variable vs
>> a header".  In both cases we will want to provide a command (which will
>> fetch the previous log, etc...).
> So what happens if one deletes the Ammend: header?   Accidentally or not?

It means the commit doesn't amend.  Same thing if you set your vars
accidentally or if you call the toggle command accidentally, ...

I really don't see it as a problem.  Even if it's done accidentally,
it's obvious for the user what the behavior will be, since it's written
in plain text.

> --amend and --signoff simply do not fit the header paradigm.  
> Can we please admit that and move on?

I really don't see it.


        Stefan



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-24 22:25                             ` Stefan Monnier
@ 2010-06-24 23:14                               ` Dan Nicolaescu
  2010-06-25  1:16                                 ` Stefan Monnier
  0 siblings, 1 reply; 38+ messages in thread
From: Dan Nicolaescu @ 2010-06-24 23:14 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Juri Linkov, Štěpán Němec, emacs-devel

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Can you please show how this is better than adding a single argument
>> to the vc-checkin method?  (For which the code already exists).
>
> It makes more of the state plainly visible to the user, editable with
> Emacs's usual commands, rather than hidden in variables that are much
> more difficult for the user to control.

How is it more difficult to control something that has an explicit
command attached to it?  (that you've stated that we need to have
anyway)

> I.e. it's more Emacsy by keeping things shallow.

That's not quite true for a lot of things we do in Emacs.

>>>>> Why not show it inside the buffer (e.g. in the header ;-) instead?
>>>> Because we want to insert the previous log when using --amend, so it's
>>>> better to use a command instead of a header.
>>> The question is not "a command vs a header" but "a variable vs
>>> a header".  In both cases we will want to provide a command (which will
>>> fetch the previous log, etc...).
>> So what happens if one deletes the Ammend: header?   Accidentally or not?
>
> It means the commit doesn't amend.  Same thing if you set your vars
> accidentally or if you call the toggle command accidentally, ...
>
> I really don't see it as a problem.  Even if it's done accidentally,
> it's obvious for the user what the behavior will be, since it's written
> in plain text.

Why deal with any potential complications that are not needed at all?

>> --amend and --signoff simply do not fit the header paradigm.  
>> Can we please admit that and move on?
>
> I really don't see it.

Are willing to implement your version and ask users if they prefer it?

I've implemented my version and have been using it for > 6 months,
it's much better to not have to deal with headers.





^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-24 23:14                               ` Dan Nicolaescu
@ 2010-06-25  1:16                                 ` Stefan Monnier
  2010-06-25  2:27                                   ` Dan Nicolaescu
  0 siblings, 1 reply; 38+ messages in thread
From: Stefan Monnier @ 2010-06-25  1:16 UTC (permalink / raw)
  To: Dan Nicolaescu; +Cc: Juri Linkov, Štěpán Němec, emacs-devel

>>> Can you please show how this is better than adding a single argument
>>> to the vc-checkin method?  (For which the code already exists).
>> It makes more of the state plainly visible to the user, editable with
>> Emacs's usual commands, rather than hidden in variables that are much
>> more difficult for the user to control.
> How is it more difficult to control something that has an explicit
> command attached to it?  (that you've stated that we need to have
> anyway)

It's more difficult because you're limited to what the command lets
you do, whereas with plain text, you can do anything you like
(including running the provided command).

>> I.e. it's more Emacsy by keeping things shallow.
> That's not quite true for a lot of things we do in Emacs.

And in most cases, that is a problem.  Sometimes it's justified, and
sometimes it's just because noone has gone through the trouble to make
it better.

>> It means the commit doesn't amend.  Same thing if you set your vars
>> accidentally or if you call the toggle command accidentally, ...
>> 
>> I really don't see it as a problem.  Even if it's done accidentally,
>> it's obvious for the user what the behavior will be, since it's written
>> in plain text.

> Why deal with any potential complications that are not needed at all?

What complications are you referring to?

>>> --amend and --signoff simply do not fit the header paradigm.  
>>> Can we please admit that and move on?
>> I really don't see it.
> Are willing to implement your version and ask users if they prefer it?

Don't know.  But I don't want the implementation you propose.  I want
the information to be shallow and readily available for the user to
manipulate as she sees fit.


        Stefan



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-25  1:16                                 ` Stefan Monnier
@ 2010-06-25  2:27                                   ` Dan Nicolaescu
  2010-06-25 11:44                                     ` Miles Bader
  0 siblings, 1 reply; 38+ messages in thread
From: Dan Nicolaescu @ 2010-06-25  2:27 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Juri Linkov, Štěpán Němec, emacs-devel

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>>> Can you please show how this is better than adding a single argument
>>>> to the vc-checkin method?  (For which the code already exists).
>>> It makes more of the state plainly visible to the user, editable with
>>> Emacs's usual commands, rather than hidden in variables that are much
>>> more difficult for the user to control.
>> How is it more difficult to control something that has an explicit
>> command attached to it?  (that you've stated that we need to have
>> anyway)
>
> It's more difficult because you're limited to what the command lets
> you do, whereas with plain text, you can do anything you like
> (including running the provided command).

As a general principle this sounds fine, but in this particular case
it does not hold too much water.  Your general case includes the
particular case, but it adds unneeded convulutions due to having to
deal with random editing.

Also, a lot of times specific solutions are much better than generic ones.

BTW, what you propose is the perforce model, you might want to talk to
some perforce users about how good it is to have to edit for various
commands instead of providing an imperative interface.

>>> It means the commit doesn't amend.  Same thing if you set your vars
>>> accidentally or if you call the toggle command accidentally, ...
>>> 
>>> I really don't see it as a problem.  Even if it's done accidentally,
>>> it's obvious for the user what the behavior will be, since it's written
>>> in plain text.
>
>> Why deal with any potential complications that are not needed at all?
>
> What complications are you referring to?

Having to educate the users what they can and cannot do with the
markup and the code to deal with it that has ZERO end-user value.

>>>> --amend and --signoff simply do not fit the header paradigm.  
>>>> Can we please admit that and move on?
>>> I really don't see it.
>> Are willing to implement your version and ask users if they prefer it?
>
> Don't know.  But I don't want the implementation you propose.  I want
> the information to be shallow and readily available for the user to
> manipulate as she sees fit.

It seems that this is your personal preference and it's not actually
technically justified.

Given that you don't really plan to do anything about it, and I am
convinced that it's the wrong solution for the problem based on my
personal experience both with this particular feature and from using
perforce. 
How about you yield here to the person that has done the vast majority
of work in the VC area in the last few years, both in terms of new
features and in terms of bug fixing?
If users come back and ask for markup for amend and commit we can add
them later, but at this particular point this is just delaying giving
end users access to some very useful features.




^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-25  2:27                                   ` Dan Nicolaescu
@ 2010-06-25 11:44                                     ` Miles Bader
  2010-06-26  5:09                                       ` Dan Nicolaescu
  2010-06-26 10:11                                       ` David Kastrup
  0 siblings, 2 replies; 38+ messages in thread
From: Miles Bader @ 2010-06-25 11:44 UTC (permalink / raw)
  To: Dan Nicolaescu
  Cc: Juri Linkov, Štěpán Němec, Stefan Monnier,
	emacs-devel

Dan Nicolaescu <dann@gnu.org> writes:
> Given that you don't really plan to do anything about it, and I am
> convinced that it's the wrong solution for the problem based on my
> personal experience both with this particular feature and from using
> perforce. 

I kinda like Stefan's method better too.

It ("Stefan's method") seems much more flexible, albeit maybe a little
more open to risk from the user fiddles where he shouldn't have.

I also agree with Stefan that it's somewhat more "emacsy", as it
leverages ordinary text in a buffer instead of hidden magic.  Like other
such cases I think it will thus tend to appeal more to experienced users
and maybe scare off noobs a bit; but since I am an experienced user, I'd
prefer flexibility and transparency.  As for the noobs, I think they win
in the long run, even if they're a bit put off in the short run, because
the relative transparency of such a mechanism makes it easier for them
to do more advanced things without getting tripped up by some hidden
behind-the-scenes mechanism.

A dedicated "vc-append" (or whatever) command which sets up the log
buffer appropriately with a pre-stuffed message and appropriate
meta-data seems a reasonable thing to me, but I really like the use of
the log buffer itself to hold all meta-data.

-Miles

p.s. Actually I don't know if it was Stefan that actually came up with
that method originally, but anyway, you get the idea.......

-- 
Suburbia: where they tear out the trees and then name streets after them.



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-25 11:44                                     ` Miles Bader
@ 2010-06-26  5:09                                       ` Dan Nicolaescu
  2010-07-01  0:01                                         ` Stefan Monnier
  2010-06-26 10:11                                       ` David Kastrup
  1 sibling, 1 reply; 38+ messages in thread
From: Dan Nicolaescu @ 2010-06-26  5:09 UTC (permalink / raw)
  To: Miles Bader
  Cc: Juri Linkov, Štěpán Němec, Stefan Monnier,
	emacs-devel

Miles Bader <miles@gnu.org> writes:

> Dan Nicolaescu <dann@gnu.org> writes:
>> Given that you don't really plan to do anything about it, and I am
>> convinced that it's the wrong solution for the problem based on my
>> personal experience both with this particular feature and from using
>> perforce. 
>
> I kinda like Stefan's method better too.
>
> It ("Stefan's method") seems much more flexible, albeit maybe a little
> more open to risk from the user fiddles where he shouldn't have.
>
> I also agree with Stefan that it's somewhat more "emacsy", as it
> leverages ordinary text in a buffer instead of hidden magic.  Like other
> such cases I think it will thus tend to appeal more to experienced users
> and maybe scare off noobs a bit; but since I am an experienced user, I'd
> prefer flexibility and transparency.  As for the noobs, I think they win
> in the long run, even if they're a bit put off in the short run, because
> the relative transparency of such a mechanism makes it easier for them
> to do more advanced things without getting tripped up by some hidden
> behind-the-scenes mechanism.
>
> A dedicated "vc-append" (or whatever) command which sets up the log
> buffer appropriately with a pre-stuffed message and appropriate
> meta-data seems a reasonable thing to me, but I really like the use of
> the log buffer itself to hold all meta-data.

Are you interested in implementing that version and asking users which
one they prefer?
My version exists, it's functional, it's probably less than 25 lines
of code (hard to count exactly, I have many other indepenedent
changes).

This discussion does not seem to go anywhere, all the arguments have
been made already.   And it's quite a trivial issue to start with...

So I'd like to request a resolution.

I have provided a working implementation and motivation why it's the
right thing to do, and why the other proposal is not a good idea.

It seems that there are 3 possible courses of action.

1. check in my code
2. reject my code and do nothing
3. someone else volunteers to provide an alternative and check that in

[To be fair, it would be nice if 3 was chosen to ask at least a few
users if the prefer 1 or 3]



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-25 11:44                                     ` Miles Bader
  2010-06-26  5:09                                       ` Dan Nicolaescu
@ 2010-06-26 10:11                                       ` David Kastrup
  2010-06-28 21:04                                         ` Juri Linkov
  1 sibling, 1 reply; 38+ messages in thread
From: David Kastrup @ 2010-06-26 10:11 UTC (permalink / raw)
  To: emacs-devel

Miles Bader <miles@gnu.org> writes:

> Dan Nicolaescu <dann@gnu.org> writes:
>> Given that you don't really plan to do anything about it, and I am
>> convinced that it's the wrong solution for the problem based on my
>> personal experience both with this particular feature and from using
>> perforce. 
>
> I kinda like Stefan's method better too.

It seems like it would be quite more problematic to script/macro/batch.
Perhaps one needs to think about an argument for non-interactive use
that makes it easy to substitute for the editing of the commit message?
So if one pre-specifies command line options in some manner, the
respective pseudo-lines in the commit message are not generated in the
first place?

That way, one can use Stefan's method by default, but rather
transparently move to as much of Dan's method as one likes.

Anyway, the principal problem appears similar to web forums as opposed
to Usenet.  They seem user-friendly on the surface, but I can't keep up
with more than a few web forums because of time restraints.  Handling
dozens of Usenet groups via gnus, in contrast, is easy.

-- 
David Kastrup




^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-26 10:11                                       ` David Kastrup
@ 2010-06-28 21:04                                         ` Juri Linkov
  0 siblings, 0 replies; 38+ messages in thread
From: Juri Linkov @ 2010-06-28 21:04 UTC (permalink / raw)
  To: David Kastrup; +Cc: emacs-devel

> That way, one can use Stefan's method by default, but rather
> transparently move to as much of Dan's method as one likes.

I wonder if, for instance, I'd like to bind `C-c C-c' to a command
that asks for confirmation of command line arguments in the minibuffer,
would it be possible to implement such a command with Stefan's method
or Dan's method?

-- 
Juri Linkov
http://www.jurta.org/emacs/



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: support for git commit --amend/--signoff
  2010-06-26  5:09                                       ` Dan Nicolaescu
@ 2010-07-01  0:01                                         ` Stefan Monnier
  0 siblings, 0 replies; 38+ messages in thread
From: Stefan Monnier @ 2010-07-01  0:01 UTC (permalink / raw)
  To: Dan Nicolaescu
  Cc: Juri Linkov, emacs-devel, Štěpán Němec,
	Miles Bader

> It seems that there are 3 possible courses of action.
> 1. check in my code

I already rejected this option.

> 2. reject my code and do nothing
> 3. someone else volunteers to provide an alternative and check that in

As you know, I already re-implemented your original "commit args for
authors and subject lines", so who know, I may re-implement your amend
patch if noone else gets there before I do.


        Stefan "travelling"



^ permalink raw reply	[flat|nested] 38+ messages in thread

end of thread, other threads:[~2010-07-01  0:01 UTC | newest]

Thread overview: 38+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-06-11  6:19 support for git commit --amend/--signoff Dan Nicolaescu
2010-06-11  8:09 ` Juri Linkov
2010-06-11 13:23   ` Dan Nicolaescu
2010-06-11 14:18     ` Stefan Monnier
2010-06-11 16:14       ` Štěpán Němec
2010-06-11 20:26         ` Stefan Monnier
2010-06-12  2:19           ` Dan Nicolaescu
2010-06-12 19:59             ` Juri Linkov
2010-06-12 20:19             ` Stefan Monnier
2010-06-19  6:38               ` Dan Nicolaescu
2010-06-23  7:17                 ` Stefan Monnier
2010-06-23  7:45                   ` David Kastrup
2010-06-23  9:00                   ` Miles Bader
2010-06-23 18:55                     ` Dan Nicolaescu
2010-06-23 18:45                   ` Dan Nicolaescu
2010-06-23 22:04                     ` Stefan Monnier
2010-06-23 23:23                       ` Dan Nicolaescu
2010-06-24 21:03                         ` Stefan Monnier
2010-06-24 21:18                           ` Dan Nicolaescu
2010-06-24 22:25                             ` Stefan Monnier
2010-06-24 23:14                               ` Dan Nicolaescu
2010-06-25  1:16                                 ` Stefan Monnier
2010-06-25  2:27                                   ` Dan Nicolaescu
2010-06-25 11:44                                     ` Miles Bader
2010-06-26  5:09                                       ` Dan Nicolaescu
2010-07-01  0:01                                         ` Stefan Monnier
2010-06-26 10:11                                       ` David Kastrup
2010-06-28 21:04                                         ` Juri Linkov
2010-06-11 17:34       ` Dan Nicolaescu
2010-06-11 19:27       ` Juri Linkov
2010-06-11 20:16         ` Dan Nicolaescu
2010-06-11 20:38           ` Juri Linkov
2010-06-11 23:48             ` W Dan Meyer
2010-06-12 20:23               ` Juri Linkov
2010-06-12  2:21             ` Dan Nicolaescu
2010-06-11 23:44           ` Thien-Thi Nguyen
2010-06-12 20:15           ` Stefan Monnier
2010-06-11 20:35         ` Stefan Monnier

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).