[PATCH] Teach 'rebase -i' the command "amend"

[PATCH] Teach 'rebase -i' the command "amend"

[Björn Gustavsson]

Make it easier to edit just the commit message for a commit
using 'git rebase -i' by introducing the "amend" command.

Signed-off-by: Björn Gustavsson <bgustavsson@gmail.com>
---
Here is my first ever patch of git.

I find that I often use 'git rebase -i' just to correct typos
in my commit messages or to fill in more details, so I thought
it would be a good idea to add to add an "amend" command to
'git rebase -i'.

/Björn

 Documentation/git-rebase.txt  |    3 +++
 git-rebase--interactive.sh    |    9 +++++++++
 t/lib-rebase.sh               |    6 +++---
 t/t3404-rebase-interactive.sh |   14 ++++++++++++++
 4 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 0aefc34..28b90ec 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -368,6 +368,9 @@ By replacing the command "pick" with the command "edit", you can tell
 the files and/or the commit message, amend the commit, and continue
 rebasing.
 
+If you just want to edit the commit message for a commit, you can replace
+the command "pick" with the command "amend".
+
 If you want to fold two or more commits into one, replace the command
 "pick" with "squash" for the second and subsequent commit.  If the
 commits had different authors, it will attribute the squashed commit to
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 23ded48..3f6bcc0 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -340,6 +340,14 @@ do_next () {
 		pick_one $sha1 ||
 			die_with_patch $sha1 "Could not apply $sha1... $rest"
 		;;
+	amend|a)
+		comment_for_reflog amend
+
+		mark_action_done
+		pick_one $sha1 ||
+			die_with_patch $sha1 "Could not apply $sha1... $rest"
+		output git commit --amend
+		;;
 	edit|e)
 		comment_for_reflog edit
 
@@ -752,6 +760,7 @@ first and then run 'git rebase --continue' again."
 #
 # Commands:
 #  p, pick = use commit
+#  a, amend = use commit, but allow editing of the commit message
 #  e, edit = use commit, but stop for amending
 #  s, squash = use commit, but meld into previous commit
 #
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 260a231..2ca0481 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -9,8 +9,8 @@
 #
 #	"[<lineno1>] [<lineno2>]..."
 #
-#   If a line number is prefixed with "squash" or "edit", the respective line's
-#   command will be replaced with the specified one.
+#   If a line number is prefixed with "squash", "edit", or "amend", the
+#   respective line's command will be replaced with the specified one.
 
 set_fake_editor () {
 	echo "#!$SHELL_PATH" >fake-editor.sh
@@ -32,7 +32,7 @@ cat "$1".tmp
 action=pick
 for line in $FAKE_LINES; do
 	case $line in
-	squash|edit)
+	squash|edit|amend)
 		action="$line";;
 	*)
 		echo sed -n "${line}s/^pick/$action/p"
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 4cae019..3520480 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -470,4 +470,18 @@ test_expect_success 'avoid unnecessary reset' '
 	test 123456789 = $MTIME
 '
 
+test_expect_success 'amend' '
+	git checkout -b amend-branch master &&
+	FAKE_LINES="1 2 3 amend 4" FAKE_COMMIT_MESSAGE="E changed" git rebase -i A &&
+	git show HEAD | grep "E changed" &&
+	test $(git rev-parse master) != $(git rev-parse HEAD) &&
+	test $(git rev-parse master^) = $(git rev-parse HEAD^) &&
+	FAKE_LINES="1 2 amend 3 4" FAKE_COMMIT_MESSAGE="D changed" git rebase -i A &&
+	git show HEAD^ | grep "D changed" &&
+	FAKE_LINES="amend 1 2 3 4" FAKE_COMMIT_MESSAGE="B changed" git rebase -i A &&
+	git show HEAD~3 | grep "B changed" &&
+	FAKE_LINES="1 amend 2 3 4" FAKE_COMMIT_MESSAGE="C changed" git rebase -i A &&
+	git show HEAD~2 | grep "C changed"
+'
+
 test_done
-- 
1.6.5.rc2.18.g117a

Re: [PATCH] Teach 'rebase -i' the command "amend"

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 337 bytes --]

Hi,

On Sun, 4 Oct 2009, Björn Gustavsson wrote:

> Make it easier to edit just the commit message for a commit
> using 'git rebase -i' by introducing the "amend" command.

I thought that we had a discussion about this and that the consensus was 
that "amend" would be misleading.  Maybe you can find that thread in 
GMane?

Ciao,
Dscho

Re: [PATCH] Teach 'rebase -i' the command "amend"

[Björn Gustavsson]

2009/10/4 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> I thought that we had a discussion about this and that the consensus was
> that "amend" would be misleading.  Maybe you can find that thread in
> GMane?

I found this thread from January 2009:

http://thread.gmane.org/gmane.comp.version-control.git/105738

Having read the thread, I agree that "amend" would be misleading.

There were several suggestions for alternate command names
in that thread, for example:

"msg", "msgedit", "message", "reword", "rephrase"

It think that "msgedit" was suggested by several people. ("editmsg"
was also suggested, but it is not possible as the abbreviation "e" would
become ambiguous.)

Would the patch have a chance to be accepted if I renamed
the new command to "msgedit"?

/Björn

-- 
Björn Gustavsson, Erlang/OTP, Ericsson AB

Re: [PATCH] Teach 'rebase -i' the command "amend"

[Junio C Hamano]

Björn Gustavsson <bgustavsson@gmail.com> writes:

> I found this thread from January 2009:
>
> http://thread.gmane.org/gmane.comp.version-control.git/105738
>
> Having read the thread, I agree that "amend" would be misleading.

Yeah, I thought "reword" might be a good command name back then.

Was the naming the only issue in the discussion there?

Re: [PATCH] Teach 'rebase -i' the command "amend"

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1153 bytes --]

Hi,

On Mon, 5 Oct 2009, Björn Gustavsson wrote:

> 2009/10/4 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> > I thought that we had a discussion about this and that the consensus was
> > that "amend" would be misleading.  Maybe you can find that thread in
> > GMane?
> 
> I found this thread from January 2009:
> 
> http://thread.gmane.org/gmane.comp.version-control.git/105738
> 
> Having read the thread, I agree that "amend" would be misleading.
> 
> There were several suggestions for alternate command names
> in that thread, for example:
> 
> "msg", "msgedit", "message", "reword", "rephrase"
> 
> It think that "msgedit" was suggested by several people. ("editmsg"
> was also suggested, but it is not possible as the abbreviation "e" would
> become ambiguous.)
> 
> Would the patch have a chance to be accepted if I renamed
> the new command to "msgedit"?

My objection was that the "upcoming" (yeah, Sverre, I am imitating Duke 
Nukem Forever here) "merge" command would clash with "msgedit", which was 
why I suggested "rephrase" (but would be okay with "reword" Junio 
mentions).

AFAIR the naming was the only objection.

Ciao,
Dscho

Re: [PATCH] Teach 'rebase -i' the command "amend"

[Sverre Rabbelier]

Heya,

2009/10/5 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> My objection was that the "upcoming" (yeah, Sverre, I am imitating Duke
> Nukem Forever here) "merge" command would clash with "msgedit", which was
> why I suggested "rephrase" (but would be okay with "reword" Junio
> mentions).

I also dislike 'msgedit' because it's abbrev-ed, I would prefer
"reword" for that reason.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] Teach 'rebase -i' the command "amend"

[Björn Gustavsson]

On Mon, Oct 5, 2009 at 11:39 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> 2009/10/5 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>> My objection was that the "upcoming" (yeah, Sverre, I am imitating Duke
>> Nukem Forever here) "merge" command would clash with "msgedit", which was
>> why I suggested "rephrase" (but would be okay with "reword" Junio
>> mentions).
>
> I also dislike 'msgedit' because it's abbrev-ed, I would prefer
> "reword" for that reason.
>

Thanks for the comments. "reword" it will be then. I'll send a new patch soon.

-- 
Björn Gustavsson, Erlang/OTP, Ericsson AB

Re: [PATCH] Teach 'rebase -i' the command "amend"

[Anders Melchiorsen]

Björn Gustavsson <bgustavsson@gmail.com> writes:

> Thanks for the comments. "reword" it will be then. I'll send a new patch soon.

If you could also make it possible to edit the commit summary line
right in the git-rebase-todo buffer, that would be great.

Being in an editor but still not able to fix typos is a nuisance.


Cheers,
Anders.

Re: [PATCH] Teach 'rebase -i' the command "amend"

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 624 bytes --]

Hi,

On Mon, 5 Oct 2009, Anders Melchiorsen wrote:

> Björn Gustavsson <bgustavsson@gmail.com> writes:
> 
> > Thanks for the comments. "reword" it will be then. I'll send a new patch soon.
> 
> If you could also make it possible to edit the commit summary line
> right in the git-rebase-todo buffer, that would be great.
> 
> Being in an editor but still not able to fix typos is a nuisance.

NAK.

Supporting that would be totally out of line with the way rebase -i is 
supposed to work.

Besides, if you already have typos in the commit subject, you _better_ 
check the whole commit message, so: double NAK.

Ciao,
Dscho

Re: [PATCH] Teach 'rebase -i' the command "amend"

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Being in an editor but still not able to fix typos is a nuisance.
>
> NAK.
>
> Supporting that would be totally out of line with the way rebase -i is 
> supposed to work.

If the rebase insn sheet were richer, and had a way to show the full
message, like this:

pick 4973aa2 git-pull: dead code removal
    Back when a74b170 (git-pull: disallow implicit merging to detached HEAD,
    2007-01-15) added this check, $? referred to the error status of reading
    HEAD as a symbolic-ref; but cd67e4d (Teach 'git pull' about --rebase,
    2007-11-28) moved the command away from where the check is, and nobody
    noticed the breakage.  Ever since, $? has always been 0 (tr at the end of
    the pipe to find merge_head never fails) and other case arms were never
    reached.
    
    These days, error_on_no_merge_candidates function is prepared to handle a
    detached HEAD case, which was what the code this patch removes used to
    handle.
    
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

I do not see why we shouldn't allow people to edit any part of the above
to reword.

I would even understand (but not necessarily agree) if somebody wants to
give the patch text and let users edit to reapply.

So I do not agree with your "totally out of line" at all.

> Besides, if you already have typos in the commit subject, you _better_ 
> check the whole commit message, so: double NAK.

That sounds a bit too dogmatic.

But I tend to agree with you that we would be better off not accepting
such a "retitle" patch, as it strongly encourages single-liner commit log
messages.

Oh, there was no patch?  Then nevermind...

Re: [PATCH] Teach 'rebase -i' the command "amend"

[Sverre Rabbelier]

Heya,

2009/10/5 Junio C Hamano <gitster@pobox.com>:
> If the rebase insn sheet were richer, and had a way to show the full
> message, like this:

But that's not what rebase -i's insn sheet is about is it?  It's not
"rewrite my commits so that they are the way I want them", it's about
"change the order of my commits"/squash some/drop some. The polishing
of the commits itself is done after finishing the insn sheet.

> I do not see why we shouldn't allow people to edit any part of the above
> to reword.

Because it then becomes very hard to do any actual reordering. I'm not
saying it's a bad idea, just that it's a bad idea to do it in 'git
rebase -i', conceptually. Although what you suggest would be useful to
me, I just think it should be a different command, git rewrite perhaps
:P. Definitely not "git rebase -i --rewrite-message" though, becaue it
is not at all about rebasing anymore.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] Teach 'rebase -i' the command "amend"

[Johannes Schindelin]

Hi,

On Mon, 5 Oct 2009, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> Being in an editor but still not able to fix typos is a nuisance.
> >
> > NAK.
> >
> > Supporting that would be totally out of line with the way rebase -i is 
> > supposed to work.
> 
> If the rebase insn sheet were richer, and had a way to show the full
> message, like this:
> 
> pick 4973aa2 git-pull: dead code removal
>     Back when a74b170 (git-pull: disallow implicit merging to detached HEAD,
>     2007-01-15) added this check, $? referred to the error status of reading
>     HEAD as a symbolic-ref; but cd67e4d (Teach 'git pull' about --rebase,
>     2007-11-28) moved the command away from where the check is, and nobody
>     noticed the breakage.  Ever since, $? has always been 0 (tr at the end of
>     the pipe to find merge_head never fails) and other case arms were never
>     reached.
>     
>     These days, error_on_no_merge_candidates function is prepared to handle a
>     detached HEAD case, which was what the code this patch removes used to
>     handle.
>     
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
> 
> I do not see why we shouldn't allow people to edit any part of the above
> to reword.
> 
> I would even understand (but not necessarily agree) if somebody wants to
> give the patch text and let users edit to reapply.
> 
> So I do not agree with your "totally out of line" at all.

Are you serious?  "rebase -i" was _always_ about showing an edit script, 
i.e. to tell Git _what_ you want to do with _which_ commits, identified by 
short commit names.

The oneline was _always_ meant as a pure convenience for the user.

This paradigm has been true even to back when "rebase -i" was still called 
"edit-patch-series", and that is the reason I claimed that it is totally 
out of line.  Because it is.

> > Besides, if you already have typos in the commit subject, you _better_ 
> > check the whole commit message, so: double NAK.
> 
> That sounds a bit too dogmatic.
> 
> But I tend to agree with you that we would be better off not accepting
> such a "retitle" patch, as it strongly encourages single-liner commit log
> messages.

Now, that is at least as dogmatic as what I proposed.

> Oh, there was no patch?  Then nevermind...

Sorry, but I changed my mind on this attitude.  Earlier, you would find me 
valuing arguments only when backed up by code.  But since the GitTogether 
in Berlin I know of at least one discussion where somebody simply had not 
enough time to back up a (submodule-related) argument, the validity of the 
argument notwithstanding.

So not always is a "no patch? Nevermind" the correct thing to do.

In this particular case, however, I agree.  Just touching up the first 
line of commit messages is such a special case, and so removed from what a 
"rebase" is about that I would claim that this case would be better 
handled by a really trivial shell script that launches an editor and then 
drives filter-branch based on what the user said.

Ciao,
Dscho

Re: [PATCH] Teach 'rebase -i' the command "amend"

[Johannes Sixt]

Johannes Schindelin schrieb:
> Are you serious?  "rebase -i" was _always_ about showing an edit script, 
> i.e. to tell Git _what_ you want to do with _which_ commits, identified by 
> short commit names.
> 
> The oneline was _always_ meant as a pure convenience for the user.

Good that you mention it: The one-liner is really convenient. I tend to
replace it by reminders like "====== fix unused var", "===== edit msg",
etc. (after marking the commit "edit") and I'm glad that rebase-i later
prints the one-liner from the insn file rather than the original subject
line before it stops.

(I do it this way because on Windows I can't afford to call rebase-i on a
long patch series for every single task. Rather, I plan the tasks while
the insn editor is open, and this way I keep reminders about what the plan
was.)

No, I definitely don't want the one-liners to end up in the commit
message. ;-)

-- Hannes