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

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

[Michael Haggerty]

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

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

[Michael Haggerty]

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 &&
-- 
1.6.5.4

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

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t3404-rebase-interactive.sh |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

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

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

[Michael Haggerty]

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

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

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

[Michael J Gruber]

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.

Michael

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

[Michael J Gruber]

Michael Haggerty venit, vidit, dixit 04.12.2009 15:36:
> 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.

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

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

[Shawn O. Pearce]

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

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

[Johannes Schindelin]

Hi,

On Fri, 4 Dec 2009, Michael J Gruber wrote:

> 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

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

[Johannes Schindelin]

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.

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

[Johannes Schindelin]

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:

> 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

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

[Junio C Hamano]

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.

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

[Matthieu Moy]

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.

> 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/

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

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> 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/

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

[Junio C Hamano]

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.

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

[Johannes Schindelin]

Hi,

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" \
> >  			< "$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

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

[Johannes Schindelin]

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.

Ciao,
Dscho

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

[Nanako Shiraishi]

Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>

> 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/

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

[Björn Gustavsson]

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.

> 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

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

[Junio C Hamano]

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

> 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

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

[Junio C Hamano]

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.

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

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

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

[Junio C Hamano]

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

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

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

Re: [PATCH 0/3] Add a "fix" command to "rebase --interactive", [PATCH] rebase -i --autosquash: auto-squash commits

[Nanako Shiraishi]

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

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/

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

[Junio C Hamano]

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:"?

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

[Nanako Shiraishi]

Quoting Junio C Hamano <gitster@pobox.com>

> 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/

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

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

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

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

[Junio C Hamano]

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.

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

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

[Jeff King]

On Tue, Dec 08, 2009 at 01:24:50AM -0800, Junio C Hamano wrote:

> 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

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

[Sverre Rabbelier]

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

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

[Matthieu Moy]

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/

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

[Nanako Shiraishi]

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.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

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

[Aaron Cohen]

On Tue, Dec 8, 2009 at 10:55 PM, Nanako Shiraishi <nanako3@lavabit.com> wrote:
> 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.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

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