threads / patch / 21857

patch, 3 partsAdd a "fix" command to "rebase --interactive"

Subject: [PATCH 0/3] Add a "fix" command to "rebase --interactive"

## tl;dr

32 messages between Dec 4, 2009 and Dec 9, 2009. Diffs are folded; open one to read it.

replies: 31people: 11as markdown or json

Michael Haggerty· Dec 4, 2009, 14:36 UTC · lore

This patch series adds "fix" to the commands that can be used within the "rebase --interactive" patch editor. "fix" is like "squash" except that it discards the log message of the corresponding commit.

Why I would like this feature:
One of my favorite aliases is
    fix = commit --amend -C HEAD

which I use in those all-too-frequent head-slapping "I just committed something with a minor typo" moments. It amends the last commit with whatever is staged, reusing the same commit message. It can also be used with the "-a" option, a list of filenames, etc.

But sometimes I don't have my head-slapping moments until a few commits later. In this case, my usual practice is to commit the trivial typo change on top of the current branch, then "rebase --interactive" to move the typo fix on top of the erroneous commit and squash it:

pick 05d3b81 Commit with typo pick c29114a Good commit 1 pick 250b013 Good commit 2 pick 5eb3299 Fix for typo

        |
        V

pick 05d3b81 Commit with typo squash 5eb3299 Fix for typo pick c29114a Good commit 1 pick 250b013 Good commit 2

But then it is necessary to go into the commit message editor, move the cursor down past the first commit message, delete the "Fix for typo" commit message, save, and quit.

This patch implements a "fix" command, similar to "squash", except that the corresponding log message is not included in the log message suggested for the combined commit. (In fact, it includes the log message, but commented out.) It therefore saves the editor chores.

"fix" and "squash" can be used in the same group, in which case the "squash" commit messages are preserved and the "fix" commit messages are skipped.

If the idea of a "fix" command is acceptable, then I would like to implement a further convenience: if a group of commits to be folded together includes *only* "fix" commits, then the first log message should be used without even opening an editor. But I would like to get a reaction to the "fix" command in general before doing so.

Michael Haggerty (3):
  Better document the original repository layout.
  Set a couple more tags in the original repository.
  Add a command "fix" to rebase --interactive.
 Documentation/git-rebase.txt  |   13 ++++++++-----
 git-rebase--interactive.sh    |   39 +++++++++++++++++++++++++++++----------
 t/lib-rebase.sh               |    7 ++++---
 t/t3404-rebase-interactive.sh |   41 +++++++++++++++++++++++++++++++++++++----
 4 files changed, 78 insertions(+), 22 deletions(-)
Michael Haggerty· Dec 4, 2009, 14:36 UTC · re: Michael Haggerty · lore

[PATCH 1/3] Better document the original repository layout.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t3404-rebase-interactive.sh |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)
Show changes to t/t3404-rebase-interactive.sh +5 −4
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 3a37793..073674f 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -16,13 +16,14 @@ set_fake_editor
 
 # set up two branches like this:
 #
-# A - B - C - D - E
+# A - B - C - D - E     (master)
 #   \
-#     F - G - H
+#     F - G - H         (branch1)
 #       \
-#         I
+#         I             (branch2)
 #
-# where B, D and G touch the same file.
+# where B, D and G touch the same file.  In addition, set tags at
+# points A, F, and I.
 
 test_expect_success 'setup' '
 	: > file1 &&

-- 
1.6.5.4
Michael J Gruber· Dec 4, 2009, 14:52 UTC · re: Michael Haggerty · lore

Re: [PATCH 1/3] Better document the original repository layout.

Michael Haggerty venit, vidit, dixit 04.12.2009 15:36:
Show 28 quoted lines
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  t/t3404-rebase-interactive.sh |    9 +++++----
>  1 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 3a37793..073674f 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -16,13 +16,14 @@ set_fake_editor
>  
>  # set up two branches like this:
>  #
> -# A - B - C - D - E
> +# A - B - C - D - E     (master)
>  #   \
> -#     F - G - H
> +#     F - G - H         (branch1)
>  #       \
> -#         I
> +#         I             (branch2)
>  #
> -# where B, D and G touch the same file.
> +# where B, D and G touch the same file.  In addition, set tags at
> +# points A, F, and I.
>  
>  test_expect_success 'setup' '
>  	: > file1 &&

My first reaction to the subject was "Huh? What repository?". So I suggest something like

t3404: Better document the original repository layout
as a more descriptive subject.
Michael
Johannes Schindelin· Dec 4, 2009, 16:51 UTC · re: Michael J Gruber · lore

Re: [PATCH 1/3] Better document the original repository layout.

Hi,
On Fri, 4 Dec 2009, Michael J Gruber wrote:
Show 36 quoted lines
> Michael Haggerty venit, vidit, dixit 04.12.2009 15:36:
> > Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> > ---
> >  t/t3404-rebase-interactive.sh |    9 +++++----
> >  1 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> > index 3a37793..073674f 100755
> > --- a/t/t3404-rebase-interactive.sh
> > +++ b/t/t3404-rebase-interactive.sh
> > @@ -16,13 +16,14 @@ set_fake_editor
> >  
> >  # set up two branches like this:
> >  #
> > -# A - B - C - D - E
> > +# A - B - C - D - E     (master)
> >  #   \
> > -#     F - G - H
> > +#     F - G - H         (branch1)
> >  #       \
> > -#         I
> > +#         I             (branch2)
> >  #
> > -# where B, D and G touch the same file.
> > +# where B, D and G touch the same file.  In addition, set tags at
> > +# points A, F, and I.
> >  
> >  test_expect_success 'setup' '
> >  	: > file1 &&
> 
> My first reaction to the subject was "Huh? What repository?". So I
> suggest something like
> 
> t3404: Better document the original repository layout
> 
> as a more descriptive subject.
That would be good, the patch itself is already good.
ACK
Michael Haggerty· Dec 4, 2009, 14:36 UTC · re: Michael Haggerty · lore

[PATCH 2/3] Set a couple more tags in the original repository.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t3404-rebase-interactive.sh |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
Show changes to t/t3404-rebase-interactive.sh +3 −1
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 073674f..236d698 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -23,7 +23,7 @@ set_fake_editor
 #         I             (branch2)
 #
 # where B, D and G touch the same file.  In addition, set tags at
-# points A, F, and I.
+# points A, E, F, H, and I.
 
 test_expect_success 'setup' '
 	: > file1 &&
@@ -45,6 +45,7 @@ test_expect_success 'setup' '
 	git add file3 &&
 	test_tick &&
 	git commit -m E &&
+	git tag E &&
 	git checkout -b branch1 A &&
 	: > file4 &&
 	git add file4 &&
@@ -58,6 +59,7 @@ test_expect_success 'setup' '
 	git add file5 &&
 	test_tick &&
 	git commit -m H &&
+	git tag H &&
 	git checkout -b branch2 F &&
 	: > file6 &&
 	git add file6 &&

-- 
1.6.5.4
Johannes Schindelin· Dec 4, 2009, 16:52 UTC · re: Michael Haggerty · lore

Re: [PATCH 2/3] Set a couple more tags in the original repository.

Hi,
On Fri, 4 Dec 2009, Michael Haggerty wrote:
> 
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

Alternatively, one could use the test_commit function, I guess. But the patch is good as it is.

ACK.
Michael Haggerty· Dec 4, 2009, 14:36 UTC · re: Michael Haggerty · lore

[PATCH 3/3] Add a command "fix" to rebase --interactive.

The command is like "squash", except that it discards the commit message of the corresponding commit.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Documentation/git-rebase.txt  |   13 ++++++++-----
 git-rebase--interactive.sh    |   39 +++++++++++++++++++++++++++++----------
 t/lib-rebase.sh               |    7 ++++---
 t/t3404-rebase-interactive.sh |   30 ++++++++++++++++++++++++++++++
 4 files changed, 71 insertions(+), 18 deletions(-)
Show changes to 4 files +71 −18

Documentation/git-rebase.txt, git-rebase--interactive.sh, t/lib-rebase.sh, t/t3404-rebase-interactive.sh

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index ca5e1e8..eafab57 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -382,9 +382,12 @@ If you just want to edit the commit message for a commit, replace the
 command "pick" with the command "reword".
 
 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
-the author of the first commit.
+"pick" for the second and subsequent commits with "squash" or "fix".
+If the commits had different authors, the folded commit will be
+attributed to the author of the first commit.  The suggested commit
+message for the folded commit is the concatenation of the commit
+messages of the first commit and of those with the "squash" command,
+but omits the commit messages of commits with the "fix" command.
 
 'git-rebase' will stop when "pick" has been replaced with "edit" or
 when a command fails due to merge errors. When you are done editing
@@ -512,8 +515,8 @@ Easy case: The changes are literally the same.::
 Hard case: The changes are not the same.::
 
 	This happens if the 'subsystem' rebase had conflicts, or used
-	`\--interactive` to omit, edit, or squash commits; or if the
-	upstream used one of `commit \--amend`, `reset`, or
+	`\--interactive` to omit, edit, squash, or fix commits; or if
+	the upstream used one of `commit \--amend`, `reset`, or
 	`filter-branch`.
 
 
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0bd3bf7..539413d 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -302,7 +302,7 @@ nth_string () {
 
 make_squash_message () {
 	if test -f "$SQUASH_MSG"; then
-		COUNT=$(($(sed -n "s/^# This is [^0-9]*\([1-9][0-9]*\).*/\1/p" \
+		COUNT=$(($(sed -n "s/^# Th[^0-9]*\([1-9][0-9]*\)\(th\|st\|nd\|rd\) commit message.*:/\1/p" \
 			< "$SQUASH_MSG" | sed -ne '$p')+1))
 		echo "# This is a combination of $COUNT commits."
 		sed -e 1d -e '2,/^./{
@@ -315,10 +315,20 @@ make_squash_message () {
 		echo
 		git cat-file commit HEAD | sed -e '1,/^$/d'
 	fi
-	echo
-	echo "# This is the $(nth_string $COUNT) commit message:"
-	echo
-	git cat-file commit $1 | sed -e '1,/^$/d'
+	case $1 in
+	squash)
+		echo
+		echo "# This is the $(nth_string $COUNT) commit message:"
+		echo
+		git cat-file commit $2 | sed -e '1,/^$/d'
+		;;
+	fix)
+		echo
+		echo "# The $(nth_string $COUNT) commit message will be skipped:"
+		echo
+		git cat-file commit $2 | sed -e '1,/^$/d' -e 's/^/#/'
+		;;
+	esac
 }
 
 peek_next_command () {
@@ -367,20 +377,28 @@ do_next () {
 		warn
 		exit 0
 		;;
-	squash|s)
-		comment_for_reflog squash
+	squash|s|fix|f)
+		case "$command" in
+		squash|s)
+			squash_style=squash
+			;;
+		fix|f)
+			squash_style=fix
+			;;
+		esac
+		comment_for_reflog $squash_style
 
 		test -f "$DONE" && has_action "$DONE" ||
-			die "Cannot 'squash' without a previous commit"
+			die "Cannot '$squash_style' without a previous commit"
 
 		mark_action_done
-		make_squash_message $sha1 > "$MSG"
+		make_squash_message $squash_style $sha1 > "$MSG"
 		failed=f
 		author_script=$(get_author_ident_from_commit HEAD)
 		output git reset --soft HEAD^
 		pick_one -n $sha1 || failed=t
 		case "$(peek_next_command)" in
-		squash|s)
+		squash|s|fix|f)
 			USE_OUTPUT=output
 			MSG_OPT=-F
 			EDIT_OR_FILE="$MSG"
@@ -768,6 +786,7 @@ first and then run 'git rebase --continue' again."
 #  r, reword = use commit, but edit the commit message
 #  e, edit = use commit, but stop for amending
 #  s, squash = use commit, but meld into previous commit
+#  f, fix = like "squash", but discard this commit's log message
 #
 # If you remove a line here THAT COMMIT WILL BE LOST.
 # However, if you remove everything, the rebase will be aborted.
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 62f452c..8b42f23 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -9,8 +9,9 @@
 #
 #	"[<lineno1>] [<lineno2>]..."
 #
-#   If a line number is prefixed with "squash", "edit", or "reword", the
-#   respective line's command will be replaced with the specified one.
+#   If a line number is prefixed with "squash", "fix", "edit", or
+#   "reword", the respective line's command will be replaced with the
+#   specified one.
 
 set_fake_editor () {
 	echo "#!$SHELL_PATH" >fake-editor.sh
@@ -32,7 +33,7 @@ cat "$1".tmp
 action=pick
 for line in $FAKE_LINES; do
 	case $line in
-	squash|edit|reword)
+	squash|fix|edit|reword)
 		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 236d698..27645c4 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -264,6 +264,36 @@ test_expect_success 'multi-squash only fires up editor once' '
 	test 1 = $(git show | grep ONCE | wc -l)
 '
 
+test_expect_success 'multi-fix only fires up editor once' '
+	git checkout -b multi-fix E &&
+	base=$(git rev-parse HEAD~4) &&
+	FAKE_COMMIT_AMEND="ONCE" FAKE_LINES="1 fix 2 fix 3 fix 4" \
+		git rebase -i $base &&
+	test $base = $(git rev-parse HEAD^) &&
+	test 1 = $(git show | grep ONCE | wc -l) &&
+	git checkout to-be-rebased &&
+	git branch -D multi-fix
+'
+
+cat > expect-squash-fix << EOF
+B
+
+D
+
+ONCE
+EOF
+
+test_expect_success 'squash and fix generate correct log messages' '
+	git checkout -b squash-fix E &&
+	base=$(git rev-parse HEAD~4) &&
+	FAKE_COMMIT_AMEND="ONCE" FAKE_LINES="1 fix 2 squash 3 fix 4" \
+		git rebase -i $base &&
+	git cat-file commit HEAD | sed -e 1,/^\$/d > actual-squash-fix &&
+	test_cmp expect-squash-fix actual-squash-fix &&
+	git checkout to-be-rebased &&
+	git branch -D squash-fix
+'
+
 test_expect_success 'squash works as expected' '
 	for n in one two three four
 	do

-- 
1.6.5.4
Johannes Schindelin· Dec 4, 2009, 16:57 UTC · re: Michael Haggerty · lore

Re: [PATCH 3/3] Add a command "fix" to rebase --interactive.

Hi,
On Fri, 4 Dec 2009, Michael Haggerty wrote:
> The command is like "squash", except that it discards the commit message
> of the corresponding commit.
> 
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
The only part which made me a bit uneasy was this one:
Show 13 quoted lines
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 0bd3bf7..539413d 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -302,7 +302,7 @@ nth_string () {
>  
>  make_squash_message () {
>  	if test -f "$SQUASH_MSG"; then
> -		COUNT=$(($(sed -n "s/^# This is [^0-9]*\([1-9][0-9]*\).*/\1/p" \
> +		COUNT=$(($(sed -n "s/^# Th[^0-9]*\([1-9][0-9]*\)\(th\|st\|nd\|rd\) commit message.*:/\1/p" \
>  			< "$SQUASH_MSG" | sed -ne '$p')+1))
>  		echo "# This is a combination of $COUNT commits."
>  		sed -e 1d -e '2,/^./{

But it is fine. (Took this idiot a couple of seconds to figure out why; I'd have put "th" last, being a mathematician instead of a computer scientist, so my natural numbers start with 1, not 0 ;-)

To be honest, I never thought of "fix" as something I want, but patiently deleted the respective commit messages of my fixup commits (usually called "amend" or "amend.<something helpful>").

This is brilliant.
ACK
Junio C Hamano· Dec 4, 2009, 17:40 UTC · re: Michael Haggerty · lore

Re: [PATCH 3/3] Add a command "fix" to rebase --interactive.

Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 11 quoted lines
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 0bd3bf7..539413d 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -302,7 +302,7 @@ nth_string () {
>  
>  make_squash_message () {
>  	if test -f "$SQUASH_MSG"; then
> -		COUNT=$(($(sed -n "s/^# This is [^0-9]*\([1-9][0-9]*\).*/\1/p" \
> +		COUNT=$(($(sed -n "s/^# Th[^0-9]*\([1-9][0-9]*\)\(th\|st\|nd\|rd\) commit message.*:/\1/p" \
>  			< "$SQUASH_MSG" | sed -ne '$p')+1))

This sed replacement worries me. I don't have a time to check myself today but do we use \(this\|or\|that\) alternates with our sed script already elsewhere in the codebase (test scripts do not count)?

Otherwise this may suddenly be breaking a platform that has an implementation of sed that may be substandard but so far has been sufficient to work with git.

Matthieu Moy· Dec 4, 2009, 17:44 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] Add a command "fix" to rebase --interactive.

Junio C Hamano <gitster@pobox.com> writes:
Show 17 quoted lines
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index 0bd3bf7..539413d 100755
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -302,7 +302,7 @@ nth_string () {
>>  
>>  make_squash_message () {
>>  	if test -f "$SQUASH_MSG"; then
>> -		COUNT=$(($(sed -n "s/^# This is [^0-9]*\([1-9][0-9]*\).*/\1/p" \
>> +		COUNT=$(($(sed -n "s/^# Th[^0-9]*\([1-9][0-9]*\)\(th\|st\|nd\|rd\) commit message.*:/\1/p" \
>>  			< "$SQUASH_MSG" | sed -ne '$p')+1))
>
> This sed replacement worries me.  I don't have a time to check myself
> today but do we use \(this\|or\|that\) alternates with our sed script
> already elsewhere in the codebase (test scripts do not count)?
It seems we don't:

git$ git grep '\\|' *.sh git-rebase--interactive.sh: COUNT=$(($(sed -n "s/^# Th[^0-9]*\([1-9][0-9]*\)\(th\|st\|nd\|rd\) commit message.*:/\ git$ 

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Johannes Schindelin· Dec 4, 2009, 18:44 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] Add a command "fix" to rebase --interactive.

Hi,
On Fri, 4 Dec 2009, Junio C Hamano wrote:
Show 21 quoted lines
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
> > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > index 0bd3bf7..539413d 100755
> > --- a/git-rebase--interactive.sh
> > +++ b/git-rebase--interactive.sh
> > @@ -302,7 +302,7 @@ nth_string () {
> >  
> >  make_squash_message () {
> >  	if test -f "$SQUASH_MSG"; then
> > -		COUNT=$(($(sed -n "s/^# This is [^0-9]*\([1-9][0-9]*\).*/\1/p" \
> > +		COUNT=$(($(sed -n "s/^# Th[^0-9]*\([1-9][0-9]*\)\(th\|st\|nd\|rd\) commit message.*:/\1/p" \
> >  			< "$SQUASH_MSG" | sed -ne '$p')+1))
> 
> This sed replacement worries me.  I don't have a time to check myself
> today but do we use \(this\|or\|that\) alternates with our sed script
> already elsewhere in the codebase (test scripts do not count)?
> 
> Otherwise this may suddenly be breaking a platform that has an
> implementation of sed that may be substandard but so far has been
> sufficient to work with git.
IIRC "|" was not correctly handled by BSD sed (used e.g. in MacOSX).

So maybe it would be best to just look for "commit message"? I agree with Michael that the regex should not be too loose.

Ciao, Dscho

Junio C Hamano· Dec 5, 2009, 18:53 UTC · re: Johannes Schindelin · lore

Re: [PATCH 3/3] Add a command "fix" to rebase --interactive.

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 16 quoted lines
> On Fri, 4 Dec 2009, Junio C Hamano wrote:
>
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> 
>> > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> > index 0bd3bf7..539413d 100755
>> > --- a/git-rebase--interactive.sh
>> > +++ b/git-rebase--interactive.sh
>> > @@ -302,7 +302,7 @@ nth_string () {
>> >  
>> >  make_squash_message () {
>> >  	if test -f "$SQUASH_MSG"; then
>> > -		COUNT=$(($(sed -n "s/^# This is [^0-9]*\([1-9][0-9]*\).*/\1/p" \
>> > +		COUNT=$(($(sed -n "s/^# Th[^0-9]*\([1-9][0-9]*\)\(th\|st\|nd\|rd\) commit message.*:/\1/p" \
> ...
> IIRC "|" was not correctly handled by BSD sed (used e.g. in MacOSX).

You are right; we actually have seen and fixed a similar breakage. For example, "git log --all-match --grep=BRE --grep=ERE" finds:

    commit 1883a0d3b7ad7c9de1ac790bda6f1a6181237439
    Author: Junio C Hamano <gitster@pobox.com>
    Date:   Fri Sep 19 23:52:49 2008 -0700
        diff: use extended regexp to find hunk headers
        Using ERE elements such as "|" (alternation) by backquoting in BRE
        is a GNU extension and should not be done in portable programs.
and "sed" is defined to take BRE.
Tentatively I'd queue this on top of 3/3 for eventual squashing.
Thanks
-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Sat, 5 Dec 2009 10:42:44 -0800
Subject: [PATCH] [squash to Add a command "fix"] Avoid "\(alternatives\|choices\)" in sed script
---
 git-rebase--interactive.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to git-rebase--interactive.sh +1 −1
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 539413d..c30209e 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -302,7 +302,7 @@ nth_string () {
 
 make_squash_message () {
 	if test -f "$SQUASH_MSG"; then
-		COUNT=$(($(sed -n "s/^# Th[^0-9]*\([1-9][0-9]*\)\(th\|st\|nd\|rd\) commit message.*:/\1/p" \
+		COUNT=$(($(sed -n "s/^# Th[^0-9]*\([1-9][0-9]*\)[tsnr][htd] commit message.*:$/\1/p" \
 			< "$SQUASH_MSG" | sed -ne '$p')+1))
 		echo "# This is a combination of $COUNT commits."
 		sed -e 1d -e '2,/^./{

-- 
1.6.6.rc1.31.g1a56b
Michael J Gruber· Dec 4, 2009, 15:13 UTC · re: Michael Haggerty · lore

Re: [PATCH 0/3] Add a "fix" command to "rebase --interactive"

Michael Haggerty venit, vidit, dixit 04.12.2009 15:36:
Show 52 quoted lines
> This patch series adds "fix" to the commands that can be used within
> the "rebase --interactive" patch editor.  "fix" is like "squash"
> except that it discards the log message of the corresponding commit.
> 
> Why I would like this feature:
> 
> One of my favorite aliases is
> 
>     fix = commit --amend -C HEAD
> 
> which I use in those all-too-frequent head-slapping "I just committed
> something with a minor typo" moments.  It amends the last commit with
> whatever is staged, reusing the same commit message.  It can also be
> used with the "-a" option, a list of filenames, etc.
> 
> But sometimes I don't have my head-slapping moments until a few
> commits later.  In this case, my usual practice is to commit the
> trivial typo change on top of the current branch, then "rebase
> --interactive" to move the typo fix on top of the erroneous commit and
> squash it:
> 
> pick 05d3b81 Commit with typo
> pick c29114a Good commit 1
> pick 250b013 Good commit 2
> pick 5eb3299 Fix for typo
> 
>         |
>         V
> 
> pick 05d3b81 Commit with typo
> squash 5eb3299 Fix for typo
> pick c29114a Good commit 1
> pick 250b013 Good commit 2
> 
> But then it is necessary to go into the commit message editor, move
> the cursor down past the first commit message, delete the "Fix for
> typo" commit message, save, and quit.
> 
> This patch implements a "fix" command, similar to "squash", except
> that the corresponding log message is not included in the log message
> suggested for the combined commit.  (In fact, it includes the log
> message, but commented out.)  It therefore saves the editor chores.
> 
> "fix" and "squash" can be used in the same group, in which case the
> "squash" commit messages are preserved and the "fix" commit messages
> are skipped.
> 
> If the idea of a "fix" command is acceptable, then I would like to
> implement a further convenience: if a group of commits to be folded
> together includes *only* "fix" commits, then the first log message
> should be used without even opening an editor.  But I would like to
> get a reaction to the "fix" command in general before doing so.

I'd say that would make a useful command ("fix") even more useful, being just the right counterpart to "reword" for trivial commit message fixes.

OTOH, it would not be possible any more to squash in a few fixes and then edit the message. Maybe having to quit the editor is not that much work after all.

As a bike-shedding side note: So far, all commands are verbs which describe actions to take on that commit. In that sense a "fix deadbeef" would be confusing: You don't fix deadbeef, you fix the predecessor using deadbeef.

A bit of brainstorming (suck/use/smash/apply/join) does not convince me of any of my alternatives, but maybe they convince someone else :)

Michael

P.S.: I thought there's some heavy rb-i rewrite in progress (sequencer based or not?), but you cc'ed Dscho anyway who knows best.

Show 11 quoted lines
> Michael Haggerty (3):
>   Better document the original repository layout.
>   Set a couple more tags in the original repository.
>   Add a command "fix" to rebase --interactive.
> 
>  Documentation/git-rebase.txt  |   13 ++++++++-----
>  git-rebase--interactive.sh    |   39 +++++++++++++++++++++++++++++----------
>  t/lib-rebase.sh               |    7 ++++---
>  t/t3404-rebase-interactive.sh |   41 +++++++++++++++++++++++++++++++++++++----
>  4 files changed, 78 insertions(+), 22 deletions(-)
>
Matthieu Moy· Dec 4, 2009, 17:40 UTC · re: Michael J Gruber · lore

Re: [PATCH 0/3] Add a "fix" command to "rebase --interactive"

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 8 quoted lines
>> If the idea of a "fix" command is acceptable, then I would like to
>> implement a further convenience: if a group of commits to be folded
>> together includes *only* "fix" commits, then the first log message
>> should be used without even opening an editor.  But I would like to
>> get a reaction to the "fix" command in general before doing so.
>
> I'd say that would make a useful command ("fix") even more useful, being
> just the right counterpart to "reword" for trivial commit message fixes.
+1 for fix, and +1 for the "don't even launch the editor" too.
> OTOH, it would not be possible any more to squash in a few fixes and
> then edit the message. Maybe having to quit the editor is not that much
> work after all.

Well, it's still possible, by using "squash" and deleting the extra part. In both cases, there's one scenario where one has a little more to do than strictly required (either delete a bit of texte or close the editor), but I think the senario "just want to fix the content with another patch, but the commit message was good" is so common that it deserves being optimized.

In particular, that would make a rebase -i with only reordering and fixes non-interactive. With 3 fixes, you just wait for Git around a second and it's done, smoothly, while it requires launching/closing the editor 3 times ...

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· Dec 4, 2009, 17:44 UTC · re: Matthieu Moy · lore

Re: [PATCH 0/3] Add a "fix" command to "rebase --interactive"

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 12 quoted lines
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>>> If the idea of a "fix" command is acceptable, then I would like to
>>> implement a further convenience: if a group of commits to be folded
>>> together includes *only* "fix" commits, then the first log message
>>> should be used without even opening an editor.  But I would like to
>>> get a reaction to the "fix" command in general before doing so.
>>
>> I'd say that would make a useful command ("fix") even more useful, being
>> just the right counterpart to "reword" for trivial commit message fixes.
>
> +1 for fix, and +1 for the "don't even launch the editor" too.

I like it, too. Also I vaguely recall that there was a series that died that would have allowed you to give hints to help this behaviour at the time you make "fix-up" commits; we may want to resurrect it on top of this feature.

Johannes Schindelin· Dec 4, 2009, 18:47 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/3] Add a "fix" command to "rebase --interactive"

Hi,
On Fri, 4 Dec 2009, Junio C Hamano wrote:
Show 19 quoted lines
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> 
> > Michael J Gruber <git@drmicha.warpmail.net> writes:
> >
> >>> If the idea of a "fix" command is acceptable, then I would like to
> >>> implement a further convenience: if a group of commits to be folded
> >>> together includes *only* "fix" commits, then the first log message
> >>> should be used without even opening an editor.  But I would like to
> >>> get a reaction to the "fix" command in general before doing so.
> >>
> >> I'd say that would make a useful command ("fix") even more useful, being
> >> just the right counterpart to "reword" for trivial commit message fixes.
> >
> > +1 for fix, and +1 for the "don't even launch the editor" too.
> 
> I like it, too.  Also I vaguely recall that there was a series that died
> that would have allowed you to give hints to help this behaviour at the
> time you make "fix-up" commits; we may want to resurrect it on top of this
> feature.

I'll just repeat this exactly one more time: it is not always possible to know whether you make a fix-up commit, and it is not always possible to be sure that you want to amend the next time you do a rebase.

So: Commit time is definitely a bad time to decide on the action in some 
future rebase event.

Ciao, Dscho

Nanako Shiraishi· Dec 4, 2009, 21:27 UTC · re: Johannes Schindelin · lore

Re: [PATCH 0/3] Add a "fix" command to "rebase --interactive"

Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>
Show 30 quoted lines
> Hi,
>
> On Fri, 4 Dec 2009, Junio C Hamano wrote:
>
>> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>> 
>> > Michael J Gruber <git@drmicha.warpmail.net> writes:
>> >
>> >>> If the idea of a "fix" command is acceptable, then I would like to
>> >>> implement a further convenience: if a group of commits to be folded
>> >>> together includes *only* "fix" commits, then the first log message
>> >>> should be used without even opening an editor.  But I would like to
>> >>> get a reaction to the "fix" command in general before doing so.
>> >>
>> >> I'd say that would make a useful command ("fix") even more useful, being
>> >> just the right counterpart to "reword" for trivial commit message fixes.
>> >
>> > +1 for fix, and +1 for the "don't even launch the editor" too.
>> 
>> I like it, too.  Also I vaguely recall that there was a series that died
>> that would have allowed you to give hints to help this behaviour at the
>> time you make "fix-up" commits; we may want to resurrect it on top of this
>> feature.
>
> I'll just repeat this exactly one more time: it is not always possible to 
> know whether you make a fix-up commit, and it is not always possible to be 
> sure that you want to amend the next time you do a rebase.
>
> So: Commit time is definitely a bad time to decide on the action in some 
> future rebase event.
I think Junio is referring to this thread:
  http://thread.gmane.org/gmane.comp.version-control.git/127923/focus=121874

The old patch added a convention to mark a fix-up commit with a special string "!fixup" and refer to which commit in the series it is fixing. It added --autosquash option to rebase--interactive that tells it to move such a commit to an appropriate place in the series and change its 'pick' to 'squash'. I think with Michael's patches, it can change 'pick' to 'fix' instead.

I too think Michael's "fix" is a good feature, and in the workflow by Shawn, he knows he is fixing up an earlier commit, and he knows he doesn't want to add anything to the message by the fix-up commit when he makes that commit (how else would he have messages like "a", "s", or "foo").

I don't think your objection should block *others* (like Shawn and Junio) who can decide when they make commits from using the feature from my old patch to make it even easier to clean up their topics. If *you* can't decide if you want to amend or not when you make a fix-up commit, you can leave your fix-up commits unmarked, run interactive rebase without the --autosquash option, and use Michael's 'fix' manually. People who can sometimes but not always decide when they make commits can do the same when they can't.

Isn't it what Junio suggested by his "on top of this feature", and wouldn't that make everybody happy?

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/
Junio C Hamano· Dec 5, 2009, 07:39 UTC · re: Nanako Shiraishi · lore

Re: [PATCH 0/3] Add a "fix" command to "rebase --interactive"

Nanako Shiraishi <nanako3@lavabit.com> writes:
> I think Junio is referring to this thread:
>
>   http://thread.gmane.org/gmane.comp.version-control.git/127923/focus=121874

Yes, that was the one I had in mind, and it is a shame that we somehow ended up not taking it, perhaps in an improved form, back then. Even today alone, I missed the "mark for later fixing/squashing and then rebase at the end" feature twice at dayjob.

Show 5 quoted lines
> I too think Michael's "fix" is a good feature, and in the 
> workflow by Shawn, he knows he is fixing up an earlier 
> commit, and he knows he doesn't want to add anything to 
> the message by the fix-up commit when he makes that commit 
> (how else would he have messages like "a", "s", or "foo").

There is a slight distinction between "Shawn's fix-up commits have garbage message and he does not want any part of them in the final commit message" and "Shawn is happy with the message of the original commit whose tree these fix-up commits are meant to correct." He may still not be entirely happy with the original message. Wanting to edit the commit log message, and not wanting to use the messages from follow-up commits, are two different things.

I would agree that it is a good idea for "rebase -i" with only "fix" and not "squash" to skip the editing of the final message. You manually move, or tell your "rebase --autosquash" option to automatically move, the follow-up commits next to the ones they are meant to correct while editing the rebase-i insn, and you change their "pick" to "fix" (or "fixup" as Dscho and others suggested in the earlier round you quoted) only when you know you want to keep the message of the original commit. Otherwise you can change them to "squash" not to "fix", and you can edit the final log message that way.

If Michael rolls his second round with your "--autosquash", or you do so yourself on top of his patch, I think it _might_ be safer to mark the ones automatically moved as "squash", and not as "fix", and have the users explicitly change the "squash" they want to "fix" themselves. Alternatively, you can also use two magic tokens (i.e. instead of one "fixup!", allow people to use "squash!" and "fixup!") and change the action chosen for the moved commits to "squash" and "fixup" respectively.

Show 13 quoted lines
> I don't think your objection should block *others* (like
> Shawn and Junio) who can decide when they make commits 
> from using the feature from my old patch to make it even 
> easier to clean up their topics. If *you* can't decide if 
> you want to amend or not when you make a fix-up commit, you 
> can leave your fix-up commits unmarked, run interactive 
> rebase without the --autosquash option, and use Michael's 
> 'fix' manually. People who can sometimes but not always 
> decide when they make commits can do the same when they 
> can't.
>
> Isn't it what Junio suggested by his "on top of this feature", 
> and wouldn't that make everybody happy?

The answer to the first question is "yes"---I did't understand why Dscho objected to a feature he can choose not to use if he doesn't want to, and I still don't (unless I was misreading your earlier patch back then and it somehow forced all rebase-i users to decide upfront at the commit time, but I highly doubt it).

I don't know about the second one but I am guessing it would also be "yes".

Nanako Shiraishi· Dec 8, 2009, 03:13 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/3] Add a "fix" command to "rebase --interactive"

Teach a new option, --autosquash, to the interactive rebase. When the commit log message begins with "!fixup ...", and there is a commit whose title begins with the same ..., automatically modify the todo list of rebase -i so that the commit marked for squashing come right after the commit to be modified, and change the action of the moved commit from pick to squash.

Signed-off-by: Nanako Shiraishi <nanako3@lavabit.com>
---
 Junio C Hamano <gitster@pobox.com> writes:
 > If Michael rolls his second round with your "--autosquash", or you do so
 > yourself on top of his patch, I think it _might_ be safer to mark the ones
 > automatically moved as "squash", and not as "fix", and have the users
 > explicitly change the "squash" they want to "fix" themselves.
 > Alternatively, you can also use two magic tokens (i.e. instead of one
 > "fixup!", allow people to use "squash!" and "fixup!")  and change the
 > action chosen for the moved commits to "squash" and "fixup" respectively.
 Here is a rebased and updated version of my patch from June 
 2009. It should apply cleanly on top of Michael's patch.
 Documentation/git-rebase.txt |   10 +++++++
 git-rebase--interactive.sh   |   43 +++++++++++++++++++++++++++++++
 t/t3415-rebase-autosquash.sh |   58 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 111 insertions(+), 0 deletions(-)
 create mode 100755 t/t3415-rebase-autosquash.sh
Show changes to 3 files +111 −0

Documentation/git-rebase.txt, git-rebase--interactive.sh, t/t3415-rebase-autosquash.sh

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 9b648ec..87cb62d 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -308,6 +308,16 @@ which makes little sense.
 	root commits will be rewritten to have <newbase> as parent
 	instead.
 
+--autosquash::
+	When the commit log message begins with "!squash ..." (or
+	"!fixup ..."), and there is a commit whose title begins with
+	the same ..., automatically modify the todo list of rebase -i
+	so that the commit marked for quashing come right after the 
+	commit to be modified, and change the action of the moved 
+	commit from `pick` to `squash` (or `fixup`).
++
+This option is only valid when '--interactive' option is used.
+
 include::merge-strategies.txt[]
 
 NOTES
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 30de96e..b014231 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -28,6 +28,7 @@ abort              abort rebasing process and restore original branch
 skip               skip current patch and continue rebasing process
 no-verify          override pre-rebase hook from stopping the operation
 root               rebase all reachable commmits up to the root(s)
+autosquash         move commits that begin with !squash/!fixup
 "
 
 . git-sh-setup
@@ -46,6 +47,7 @@ ONTO=
 VERBOSE=
 OK_TO_SKIP_PRE_REBASE=
 REBASE_ROOT=
+AUTOSQUASH=
 
 GIT_CHERRY_PICK_HELP="  After resolving the conflicts,
 mark the corrected paths with 'git add <paths>', and
@@ -519,6 +521,43 @@ get_saved_options () {
 	test -f "$DOTEST"/rebase-root && REBASE_ROOT=t
 }
 
+# Rearrange the todo list that has both "pick sha1 msg" and
+# "pick sha1 !fixup/!squash msg" appears in it so that the latter
+# comes immediately after the former, and change "pick" to
+# "fixup"/"squash".
+rearrange_squash () {
+	sed -n -e 's/^pick \([0-9a-f]*\) !\(squash\) /\1 \2 /p' \
+		-e 's/^pick \([0-9a-f]*\) !\(fixup\) /\1 \2 /p' \
+		"$1" >"$1.sq"
+	test -s "$1.sq" || return
+
+	sed -e '/^pick [0-9a-f]* !squash /d' \
+		-e '/^pick [0-9a-f]* !fixup /d' \
+		"$1" |
+	(
+		used=
+		while read pick sha1 message
+		do
+			echo "$pick $sha1 $message"
+			while read squash action msg
+			do
+				case " $used" in
+				*" $squash "*)
+					continue ;;
+				esac
+				case "$message" in
+				"$msg"*)
+					echo "$action $squash !$action $msg"
+					used="$used$squash "
+					;;
+				esac
+			done <"$1.sq"
+		done >"$1.rearranged"
+	)
+	cat "$1.rearranged" >"$1"
+	rm -f "$1.sq"
+}
+
 while test $# != 0
 do
 	case "$1" in
@@ -624,6 +663,9 @@ first and then run 'git rebase --continue' again."
 	--root)
 		REBASE_ROOT=t
 		;;
+	--autosquash)
+		AUTOSQUASH=t
+		;;
 	--onto)
 		shift
 		ONTO=$(git rev-parse --verify "$1") ||
@@ -783,6 +825,7 @@ first and then run 'git rebase --continue' again."
 		fi
 
 		test -s "$TODO" || echo noop >> "$TODO"
+		test -n "$AUTOSQUASH" && rearrange_squash "$TODO"
 		cat >> "$TODO" << EOF
 
 # Rebase $SHORTREVISIONS onto $SHORTONTO
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
new file mode 100755
index 0000000..5ea2073
--- /dev/null
+++ b/t/t3415-rebase-autosquash.sh
@@ -0,0 +1,58 @@
+#!/bin/sh
+
+test_description='auto squash'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	echo 0 > file0 &&
+	git add . &&
+	test_tick &&
+	git commit -m "initial commit" &&
+	echo 0 > file1 &&
+	echo 2 > file2 &&
+	git add . &&
+	test_tick &&
+	git commit -m "first commit" &&
+	echo 3 > file3 &&
+	git add . &&
+	test_tick &&
+	git commit -m "second commit" &&
+	git tag base
+'
+
+test_expect_success 'auto fixup' '
+	git reset --hard base &&
+	echo 1 > file1 &&
+	git add -u &&
+	test_tick &&
+	git commit -m "!fixup first"
+
+	git tag final-fixup &&
+	test_tick &&
+	git rebase --autosquash -i HEAD^^^ &&
+	git log --oneline >actual &&
+	test 3 = $(wc -l <actual) &&
+	git diff --exit-code final-fixup &&
+	test 1 = "$(git cat-file blob HEAD^:file1)" &&
+	test 1 = $(git cat-file commit HEAD^ | grep first | wc -l)
+'
+
+test_expect_success 'auto squash' '
+	git reset --hard base &&
+	echo 1 > file1 &&
+	git add -u &&
+	test_tick &&
+	git commit -m "!squash first"
+
+	git tag final-squash &&
+	test_tick &&
+	git rebase --autosquash -i HEAD^^^ &&
+	git log --oneline >actual &&
+	test 3 = $(wc -l <actual) &&
+	git diff --exit-code final-squash &&
+	test 1 = "$(git cat-file blob HEAD^:file1)" &&
+	test 2 = $(git cat-file commit HEAD^ | grep first | wc -l)
+'
+
+test_done

-- 
1.6.6.rc0.60.g4926




-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/
Junio C Hamano· Dec 8, 2009, 03:28 UTC · re: Nanako Shiraishi · lore

Re: [PATCH 0/3] Add a "fix" command to "rebase --interactive"

Nanako Shiraishi <nanako3@lavabit.com> writes:
Show 8 quoted lines
> Teach a new option, --autosquash, to the interactive rebase.
> When the commit log message begins with "!fixup ...", and there
> is a commit whose title begins with the same ..., automatically
> modify the todo list of rebase -i so that the commit marked for
> squashing come right after the commit to be modified, and change
> the action of the moved commit from pick to squash.
>
> Signed-off-by: Nanako Shiraishi <nanako3@lavabit.com>
Hmph, did you forget to retitle the message, or keep in-body "Subject:"?
Nanako Shiraishi· Dec 8, 2009, 06:01 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/3] Add a "fix" command to "rebase --interactive"

Quoting Junio C Hamano <gitster@pobox.com>
Show 12 quoted lines
> Nanako Shiraishi <nanako3@lavabit.com> writes:
>
>> Teach a new option, --autosquash, to the interactive rebase.
>> When the commit log message begins with "!fixup ...", and there
>> is a commit whose title begins with the same ..., automatically
>> modify the todo list of rebase -i so that the commit marked for
>> squashing come right after the commit to be modified, and change
>> the action of the moved commit from pick to squash.
>>
>> Signed-off-by: Nanako Shiraishi <nanako3@lavabit.com>
>
> Hmph, did you forget to retitle the message, or keep in-body "Subject:"?
Sorry. Yes I did. Please amend it to -
 Subject: rebase -i --autosquash: auto-squash commits
-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/
Junio C Hamano· Dec 8, 2009, 07:43 UTC · re: Nanako Shiraishi · lore

Re: [PATCH 0/3] Add a "fix" command to "rebase --interactive"

Nanako Shiraishi <nanako3@lavabit.com> writes:
Show 5 quoted lines
>> Hmph, did you forget to retitle the message, or keep in-body "Subject:"?
>
> Sorry. Yes I did. Please amend it to -
>
>  Subject: rebase -i --autosquash: auto-squash commits
Ok.
Junio C Hamano· Dec 8, 2009, 09:24 UTC · re: Nanako Shiraishi · lore

Re: [PATCH 0/3] Add a "fix" command to "rebase --interactive"

Nanako Shiraishi <nanako3@lavabit.com> writes:
Show 27 quoted lines
> @@ -519,6 +521,43 @@ get_saved_options () {
>  	test -f "$DOTEST"/rebase-root && REBASE_ROOT=t
>  }
>  
> +# Rearrange the todo list that has both "pick sha1 msg" and
> +# "pick sha1 !fixup/!squash msg" appears in it so that the latter
> +# comes immediately after the former, and change "pick" to
> +# "fixup"/"squash".
> +rearrange_squash () {
> +	sed -n -e 's/^pick \([0-9a-f]*\) !\(squash\) /\1 \2 /p' \
> +		-e 's/^pick \([0-9a-f]*\) !\(fixup\) /\1 \2 /p' \
> +		"$1" >"$1.sq"
> +	test -s "$1.sq" || return
> +
> +	sed -e '/^pick [0-9a-f]* !squash /d' \
> +		-e '/^pick [0-9a-f]* !fixup /d' \
> +		"$1" |
> +	(
> +		used=
> +		while read pick sha1 message
> +		do
> +	...
> +		done >"$1.rearranged"
> +	)
> +	cat "$1.rearranged" >"$1"
> +	rm -f "$1.sq"
> +}

The logic to move the lines seem to have been improved since the last round, which is good. I've amended this to remove "$1.rearranged" as well.

Unlike the very initial round, but like the second round, this feature is controlled by an explicit command line option, so it should be reasonably safe.

I hate bikeshedding but somehow
    git commit -m "fixup! commit with this message"
feels much more natural than having to write
    git commit -m "!fixup commit with this message".
Jeff King· Dec 8, 2009, 09:35 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/3] Add a "fix" command to "rebase --interactive"

On Tue, Dec 08, 2009 at 01:24:50AM -0800, Junio C Hamano wrote:
Show 7 quoted lines
> I hate bikeshedding but somehow
> 
>     git commit -m "fixup! commit with this message"
> 
> feels much more natural than having to write
> 
>     git commit -m "!fixup commit with this message".
Also:
  $ bash
  $ echo "!fixup commit"
  bash: !fixup: event not found
  $ echo "fixup! commit"
  fixup! commit
-Peff
Sverre Rabbelier· Dec 8, 2009, 13:51 UTC · re: Jeff King · lore

Re: [PATCH 0/3] Add a "fix" command to "rebase --interactive"

Heya,
On Tue, Dec 8, 2009 at 10:35, Jeff King <peff@peff.net> wrote:
Show 5 quoted lines
>  $ bash
>  $ echo "!fixup commit"
>  bash: !fixup: event not found
>  $ echo "fixup! commit"
>  fixup! commit

Speaking of which, must we use that annoying bang? I hate how bash gets in my way when I try to write a commit message with a a bang in it, I'd much rather use a different character that is not in risk of being mistreated by my shell. (Although it seems that bash does do TRT in the 'fixup!' case.)

-- 
Cheers,

Sverre Rabbelier
Nanako Shiraishi· Dec 9, 2009, 03:55 UTC · re: Sverre Rabbelier · lore

Re: [PATCH 0/3] Add a "fix" command to "rebase --interactive"

Quoting Sverre Rabbelier <srabbelier@gmail.com>
Show 19 quoted lines
> Heya,
>
> On Tue, Dec 8, 2009 at 10:35, Jeff King <peff@peff.net> wrote:
>> $ bash
>> $ echo "!fixup commit"
>> bash: !fixup: event not found
>> $ echo "fixup! commit"
>> fixup! commit
>
> Speaking of which, must we use that annoying bang? I hate how bash
> gets in my way when I try to write a commit message with a a bang in
> it, I'd much rather use a different character that is not in risk of
> being mistreated by my shell. (Although it seems that bash does do TRT
> in the 'fixup!' case.)
>
> -- 
> Cheers,
>
> Sverre Rabbelier
There was a strong objection (I think from Johanes) against not 
using 'unusual' letters during the initial round back in June 
2009. Even when explicitly giving '--auto-squash' from the 
command line, there can be commits with confusing titles 
like "fixup the ancient bug in cat-file" in addition to 
the ones you wanted to mark with the "fixup!" marker.
-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/
Aaron Cohen· Dec 9, 2009, 04:41 UTC · re: Nanako Shiraishi · lore

Re: [PATCH 0/3] Add a "fix" command to "rebase --interactive"

On Tue, Dec 8, 2009 at 10:55 PM, Nanako Shiraishi <nanako3@lavabit.com> wrote:
Show 30 quoted lines
> Quoting Sverre Rabbelier <srabbelier@gmail.com>
>
>> Heya,
>>
>> On Tue, Dec 8, 2009 at 10:35, Jeff King <peff@peff.net> wrote:
>>> $ bash
>>> $ echo "!fixup commit"
>>> bash: !fixup: event not found
>>> $ echo "fixup! commit"
>>> fixup! commit
>>
>> Speaking of which, must we use that annoying bang? I hate how bash
>> gets in my way when I try to write a commit message with a a bang in
>> it, I'd much rather use a different character that is not in risk of
>> being mistreated by my shell. (Although it seems that bash does do TRT
>> in the 'fixup!' case.)
>>
>> --
>> Cheers,
>>
>> Sverre Rabbelier
>
> There was a strong objection (I think from Johanes) against not
> using 'unusual' letters during the initial round back in June
> 2009. Even when explicitly giving '--auto-squash' from the
> command line, there can be commits with confusing titles
> like "fixup the ancient bug in cat-file" in addition to
> the ones you wanted to mark with the "fixup!" marker.
>
> --

I'm sorry to pipe in with my perhaps half-baked idea from lurkerdom, but would autosquash make more sense designed as a hook rather than as a special case behavior of git-rebase?

I think this feature could be implemented by having git-rebase call a hook if supplied the --autosquash command. The hook script would be supplied with the id of the blobs being fixed up on standard input, and it could do whatever it wanted internally (including looking for magic !commands in the commit messages of the blobs).

The result of the script should be the list of blobs to be commited as a result of the script's munging, on standard output.

Junio C Hamano· Dec 9, 2009, 06:16 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/3] Add a "fix" command to "rebase --interactive"

Junio C Hamano <gitster@pobox.com> writes:
Show 32 quoted lines
> Nanako Shiraishi <nanako3@lavabit.com> writes:
>
>> @@ -519,6 +521,43 @@ get_saved_options () {
>>  	test -f "$DOTEST"/rebase-root && REBASE_ROOT=t
>>  }
>>  
>> +# Rearrange the todo list that has both "pick sha1 msg" and
>> +# "pick sha1 !fixup/!squash msg" appears in it so that the latter
>> +# comes immediately after the former, and change "pick" to
>> +# "fixup"/"squash".
>> +rearrange_squash () {
>> +	sed -n -e 's/^pick \([0-9a-f]*\) !\(squash\) /\1 \2 /p' \
>> +		-e 's/^pick \([0-9a-f]*\) !\(fixup\) /\1 \2 /p' \
>> +		"$1" >"$1.sq"
>> +	test -s "$1.sq" || return
>> +
>> +	sed -e '/^pick [0-9a-f]* !squash /d' \
>> +		-e '/^pick [0-9a-f]* !fixup /d' \
>> +		"$1" |
>> +	(
>> +		used=
>> +		while read pick sha1 message
>> +		do
>> +	...
>> +		done >"$1.rearranged"
>> +	)
>> +	cat "$1.rearranged" >"$1"
>> +	rm -f "$1.sq"
>> +}
>
> The logic to move the lines seem to have been improved since the last
> round, which is good.  I've amended this to remove "$1.rearranged" as well.

Actually I think the logic in the version from the June is more correct; doesn't this version drop commits that are marked as "squash! <message>" but with a misspelled <message> part? I think your old version, when it didn't find a matching one in early part, left such an unmatched "fixup" in place.

Here is a fix-up patch I think should be squashed in. I added a test to make sure that a mismatching "squash!" is kept in place. Please double check for sanity.

Thanks.
 git-rebase--interactive.sh   |   38 ++++++++++++++++----------------------
 t/t3415-rebase-autosquash.sh |   15 +++++++++++++++
 2 files changed, 31 insertions(+), 22 deletions(-)
Show changes to 2 files +31 −22

git-rebase--interactive.sh, t/t3415-rebase-autosquash.sh

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c68cc5b..935e9e1 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -531,29 +531,23 @@ rearrange_squash () {
 		"$1" >"$1.sq"
 	test -s "$1.sq" || return
 
-	sed -e '/^pick [0-9a-f]* squash! /d' \
-		-e '/^pick [0-9a-f]* fixup! /d' \
-		"$1" |
-	(
-		used=
-		while read pick sha1 message
+	used=
+	while read pick sha1 message
+	do
+		case " $used" in
+		*" $sha1 "*) continue ;;
+		esac
+		echo "$pick $sha1 $message"
+		while read squash action msg
 		do
-			echo "$pick $sha1 $message"
-			while read squash action msg
-			do
-				case " $used" in
-				*" $squash "*)
-					continue ;;
-				esac
-				case "$message" in
-				"$msg"*)
-					echo "$action $squash $action! $msg"
-					used="$used$squash "
-					;;
-				esac
-			done <"$1.sq"
-		done >"$1.rearranged"
-	)
+			case "$message" in
+			"$msg"*)
+				echo "$action $squash $action! $msg"
+				used="$used$squash "
+				;;
+			esac
+		done <"$1.sq"
+	done >"$1.rearranged" <"$1"
 	cat "$1.rearranged" >"$1"
 	rm -f "$1.sq" "$1.rearranged"
 }
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index f7a0f7a..b63f4e2 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -55,4 +55,19 @@ test_expect_success 'auto squash' '
 	test 2 = $(git cat-file commit HEAD^ | grep first | wc -l)
 '
 
+test_expect_success 'misspelled auto squash' '
+	git reset --hard base &&
+	echo 1 >file1 &&
+	git add -u &&
+	test_tick &&
+	git commit -m "squash! forst"
+	git tag final-missquash &&
+	test_tick &&
+	git rebase --autosquash -i HEAD^^^ &&
+	git log --oneline >actual &&
+	test 4 = $(wc -l <actual) &&
+	git diff --exit-code final-missquash &&
+	test 0 = $(git rev-list final-missquash...HEAD | wc -l)
+'
+
 test_done
Matthieu Moy· Dec 8, 2009, 14:39 UTC · re: Nanako Shiraishi · lore

Re: [PATCH 0/3] Add a "fix" command to "rebase --interactive"

Nanako Shiraishi <nanako3@lavabit.com> writes:
> Teach a new option, --autosquash, to the interactive rebase.
> When the commit log message begins with "!fixup ...", and there
> is a commit whose title begins with the same ...

You should be clearer than "title" here. My understanding is that this is the _message_ (man git-commit talks about log message or commit message).

It's a detail, but I could make sense to allow putting something other than the commit message here, like an object name:

git commit -m "fixup! 66eb61bd" git commit -m "fixup! HEAD^^"

The last one is a bit tricky, since it should mean "HEAD^^" right before I did the commit.

All that said, I probably won't be a user of that particular feature (although I love the new "fixup" command for rebase -i), so don't see any complaint here, just food for thoughts ;-).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Shawn O. Pearce· Dec 4, 2009, 15:50 UTC · re: Michael Haggerty · lore

Re: [PATCH 0/3] Add a "fix" command to "rebase --interactive"

Michael Haggerty <mhagger@alum.mit.edu> wrote:
> This patch series adds "fix" to the commands that can be used within
> the "rebase --interactive" patch editor.  "fix" is like "squash"
> except that it discards the log message of the corresponding commit.
...
Show 5 quoted lines
> But sometimes I don't have my head-slapping moments until a few
> commits later.  In this case, my usual practice is to commit the
> trivial typo change on top of the current branch, then "rebase
> --interactive" to move the typo fix on top of the erroneous commit and
> squash it:
...
> pick 05d3b81 Commit with typo
> squash 5eb3299 Fix for typo
> pick c29114a Good commit 1
> pick 250b013 Good commit 2
+1 for fix.  Totally.

When developing I *often* have not just 1 to squash, but 10 or so in a single rebase -i session, all with useless commit messages like "a", "s", or "foo". Given that I also have a commit-msg hook which injects additional lines into each of those messages, squash can be annoying to use...

fix would be really cool. To be honest, its my most common use of squash... to use the first commit's message, but combine in the misc. idiot fixes I've collected since I wrote that commit.

-- 
Shawn.
Björn Gustavsson· Dec 4, 2009, 22:19 UTC · re: Michael Haggerty · lore

Re: [PATCH 0/3] Add a "fix" command to "rebase --interactive"

On Fri, Dec 4, 2009 at 3:36 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
Show 5 quoted lines
> If the idea of a "fix" command is acceptable, then I would like to
> implement a further convenience: if a group of commits to be folded
> together includes *only* "fix" commits, then the first log message
> should be used without even opening an editor.  But I would like to
> get a reaction to the "fix" command in general before doing so.

I would really prefer that the editor is not entered at all if there are only "fix" commands in a group of commits to be folded.

> Michael Haggerty (3):
>  Better document the original repository layout.
>  Set a couple more tags in the original repository.
>  Add a command "fix" to rebase --interactive.
Nitpick: the recommended style is to leave out the full stop
at the end of the first line of the commit message.
-- 
Björn Gustavsson, Erlang/OTP, Ericsson AB
Junio C Hamano· Dec 4, 2009, 22:29 UTC · re: Björn Gustavsson · lore

Re: [PATCH 0/3] Add a "fix" command to "rebase --interactive"

Björn Gustavsson <bgustavsson@gmail.com> writes:
Show 10 quoted lines
> On Fri, Dec 4, 2009 at 3:36 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>
>> If the idea of a "fix" command is acceptable, then I would like to
>> implement a further convenience: if a group of commits to be folded
>> together includes *only* "fix" commits, then the first log message
>> should be used without even opening an editor. But I would like to
>> get a reaction to the "fix" command in general before doing so.
>
> I would really prefer that the editor is not entered at all if there
> are only "fix" commands in a group of commits to be folded.

I think all of these ideas were already discussed in the earlier thread from mid June this year, so I do not think there is no need for any more "me too like it" comments to show the support for this feature, unless it adds any new ideas of value. I think "fixup and nothing else shouldn't open editor" and "an option to pay attention to specially marked commit" are good ideas but they both appear in the old thread already.

IIRC, the end result of the bikeshedding for the name of the command was won by Dscho's "fixup":

  http://thread.gmane.org/gmane.comp.version-control.git/127923/focus=121820

← back to recent threads