rebase --autosquash does not handle fixup! of fixup!

rebase --autosquash does not handle fixup! of fixup!

[Andrew Pimlott]

git rebase -i --autosquash does not handle a fixup! of a fixup!, such as
the history:

    aaaaaaa fix nasty bug
    ...
    bbbbbbb fixup! fix nasty bug
    ...
    ccccccc fixup! fixup! fix nasty bug

--autosquash produces:

    pick aaaaaaa fix nasty bug
    fixup bbbbbbb fixup! fix nasty bug
    ...
    pick ccccccc fixup! fixup! fix nasty bug

This defeats the workflow I was hoping to use:

    git commit -m 'fix nasty bug'
    ...
    git commit --fixup :/nasty
    ...
    git commit --fixup :/nasty

The second :/nasty resolves to the previous fixup, not the initial
commit.  I could have made the regular expression more precise, but this
would be a hassle.

Would a change to support fixup! fixup! be considered?

Andrew

(Please Cc: me on replies.)

Re: rebase --autosquash does not handle fixup! of fixup!

[Thomas Rast]

Andrew Pimlott <andrew@pimlott.net> writes:

> git rebase -i --autosquash does not handle a fixup! of a fixup!, such as
> the history:
>
>     aaaaaaa fix nasty bug
>     ...
>     bbbbbbb fixup! fix nasty bug
>     ...
>     ccccccc fixup! fixup! fix nasty bug
>
> --autosquash produces:
>
>     pick aaaaaaa fix nasty bug
>     fixup bbbbbbb fixup! fix nasty bug
>     ...
>     pick ccccccc fixup! fixup! fix nasty bug
>
> This defeats the workflow I was hoping to use:
>
>     git commit -m 'fix nasty bug'
>     ...
>     git commit --fixup :/nasty
>     ...
>     git commit --fixup :/nasty
>
> The second :/nasty resolves to the previous fixup, not the initial
> commit.  I could have made the regular expression more precise, but this
> would be a hassle.
>
> Would a change to support fixup! fixup! be considered?

Sure, why not.  You could start with something like the patch below
(untested).  If that happens to work, just add a test and a good commit
message.


diff --git i/git-rebase--interactive.sh w/git-rebase--interactive.sh
index f953d8d..798ae81 100644
--- i/git-rebase--interactive.sh
+++ w/git-rebase--interactive.sh
@@ -689,7 +689,17 @@ rearrange_squash () {
 		case "$message" in
 		"squash! "*|"fixup! "*)
 			action="${message%%!*}"
-			rest="${message#*! }"
+			rest=$message
+			while : ; do
+				case "$rest" in
+				"squash! "*|"fixup! "*)
+					rest="${rest#*! }"
+					;;
+				*)
+					break
+					;;
+				esac
+			done
 			echo "$sha1 $action $rest"
 			# if it's a single word, try to resolve to a full sha1 and
 			# emit a second copy. This allows us to match on both message


-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[PATCH] rebase -i: fixup fixup! fixup!

[Andrew Pimlott]

Excerpts from Thomas Rast's message of Tue Jun 11 11:50:07 -0700 2013:
> Andrew Pimlott <andrew@pimlott.net> writes:
> >     git commit -m 'fix nasty bug'
> >     ...
> >     git commit --fixup :/nasty
> >     ...
> >     git commit --fixup :/nasty
> >
> > The second :/nasty resolves to the previous fixup, not the initial
> > commit.  I could have made the regular expression more precise, but this
> > would be a hassle.
> >
> > Would a change to support fixup! fixup! be considered?
> 
> Sure, why not.  You could start with something like the patch below
> (untested).  If that happens to work, just add a test and a good commit
> message.

It happened to work and I added a test.  But then it occurred to me that
it might have been better to fix commit --fixup/--squash to strip the
fixup! or squash! from the referenced commit in the first place.
Anyhow, below is my patch for --autosquash, but unles someone has an
objection to doing it in commit, I'll work on that.

Andrew

Ignore subsequent "fixup! " or "squash! " after the first.  Handy in case a
git commit --fixup/--squash referred to a previous fixup/squash instead of
the original commit, for example with :/msg.

Signed-off-by: Andrew Pimlott <andrew@pimlott.net>
---
 Documentation/git-rebase.txt |    4 +++-
 git-rebase--interactive.sh   |   13 ++++++++++-
 t/t3415-rebase-autosquash.sh |   49 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index c84854a..725cf27 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -389,7 +389,9 @@ squash/fixup series.
 	the same ..., automatically modify the todo list of rebase -i
 	so that the commit marked for squashing comes right after the
 	commit to be modified, and change the action of the moved
-	commit from `pick` to `squash` (or `fixup`).
+	commit from `pick` to `squash` (or `fixup`).  Ignores subsequent
+	"fixup! " or "squash! " after the first, in case you referred to a
+	previous fixup/squash with `git commit --fixup/--squash`.
 +
 This option is only valid when the '--interactive' option is used.
 +
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f953d8d..54ed4c3 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -689,7 +689,18 @@ rearrange_squash () {
 		case "$message" in
 		"squash! "*|"fixup! "*)
 			action="${message%%!*}"
-			rest="${message#*! }"
+			rest=$message
+			# ignore any squash! or fixup! after the first
+			while : ; do
+				case "$rest" in
+				"squash! "*|"fixup! "*)
+					rest="${rest#*! }"
+					;;
+				*)
+					break
+					;;
+				esac
+			done
 			echo "$sha1 $action $rest"
 			# if it's a single word, try to resolve to a full sha1 and
 			# emit a second copy. This allows us to match on both message
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index a1e86c4..1a3f40a 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -193,4 +193,53 @@ test_expect_success 'use commit --squash' '
 	test_auto_commit_flags squash 2
 '
 
+test_auto_fixup_fixup () {
+	git reset --hard base &&
+	echo 1 >file1 &&
+	git add -u &&
+	test_tick &&
+	git commit -m "$1! first" &&
+	echo 2 >file1 &&
+	git add -u &&
+	test_tick &&
+	git commit -m "$1! $2! first" &&
+	git tag "final-$1-$2" &&
+	test_tick &&
+	git rebase --autosquash -i HEAD^^^^ &&
+	git log --oneline >actual &&
+	test_pause &&
+	if [ "$1" = "fixup" ]; then
+		test_line_count = 3 actual
+	elif [ "$1" = "squash" ]; then
+		test_line_count = 4 actual
+	else
+		false
+	fi &&
+	git diff --exit-code "final-$1-$2" &&
+	test 2 = "$(git cat-file blob HEAD^:file1)" &&
+	if [ "$1" = "fixup" ]; then
+		test 1 = $(git cat-file commit HEAD^ | grep first | wc -l)
+	elif [ "$1" = "squash" ]; then
+		test 3 = $(git cat-file commit HEAD^ | grep first | wc -l)
+	else
+		false
+	fi
+}
+
+test_expect_success 'fixup! fixup!' '
+	test_auto_fixup_fixup fixup fixup
+'
+
+test_expect_success 'fixup! squash!' '
+	test_auto_fixup_fixup fixup squash
+'
+
+test_expect_success 'squash! squash!' '
+	test_auto_fixup_fixup squash squash
+'
+
+test_expect_success 'squash! fixup!' '
+	test_auto_fixup_fixup squash fixup
+'
+
 test_done
-- 
1.7.10.4

Re: [PATCH] rebase -i: fixup fixup! fixup!

[Andrew Pimlott]

Excerpts from Andrew Pimlott's message of Fri Jun 14 12:31:57 -0700 2013:
> It happened to work and I added a test.  But then it occurred to me that
> it might have been better to fix commit --fixup/--squash to strip the
> fixup! or squash! from the referenced commit in the first place.
> Anyhow, below is my patch for --autosquash, but unles someone has an
> objection to doing it in commit, I'll work on that.

Here is a patch for commit.c that does this.

Andrew

When building the commit message for --fixup/--squash, ignore a leading
fixup! or squash! on the referenced commit.  Handy in case the user referred
to an earlier squash/fixup commit instead of the original commit, for
example with :/msg.

Signed-off-by: Andrew Pimlott <andrew@pimlott.net>
---
 builtin/commit.c  |   18 ++++++++++++++----
 t/t7500-commit.sh |   36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 1621dfc..370df88 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -601,8 +601,13 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 			if (!c)
 				die(_("could not lookup commit %s"), squash_message);
 			ctx.output_encoding = get_commit_output_encoding();
-			format_commit_message(c, "squash! %s\n\n", &sb,
-					      &ctx);
+			format_commit_message(c, "%s\n\n", &sb, &ctx);
+			if (!prefixcmp(sb.buf, "fixup! ")) {
+				strbuf_remove(&sb, 0, strlen("fixup! "));
+			} else if (!prefixcmp(sb.buf, "squash! ")) {
+				strbuf_remove(&sb, 0, strlen("squash! "));
+			}
+			strbuf_insert(&sb, 0, "squash! ", strlen("squash! "));
 		}
 	}
 
@@ -634,8 +639,13 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		if (!commit)
 			die(_("could not lookup commit %s"), fixup_message);
 		ctx.output_encoding = get_commit_output_encoding();
-		format_commit_message(commit, "fixup! %s\n\n",
-				      &sb, &ctx);
+		format_commit_message(commit, "%s\n\n", &sb, &ctx);
+		if (!prefixcmp(sb.buf, "fixup! ")) {
+			strbuf_remove(&sb, 0, strlen("fixup! "));
+		} else if (!prefixcmp(sb.buf, "squash! ")) {
+			strbuf_remove(&sb, 0, strlen("squash! "));
+		}
+		strbuf_insert(&sb, 0, "fixup! ", strlen("fixup! "));
 		hook_arg1 = "message";
 	} else if (!stat(git_path("MERGE_MSG"), &statbuf)) {
 		if (strbuf_read_file(&sb, git_path("MERGE_MSG"), 0) < 0)
diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index 436b7b6..ecdf74a 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -320,4 +320,40 @@ test_expect_success 'invalid message options when using --fixup' '
 	test_must_fail git commit --fixup HEAD~1 -F log
 '
 
+test_expect_success 'commit --fixup of existing fixup' '
+	commit_for_rebase_autosquash_setup &&
+	git commit --fixup HEAD~1 &&
+	echo "fourth content line" >>foo &&
+	git add foo
+	git commit --fixup HEAD &&
+	commit_msg_is "fixup! target message subject line"
+'
+
+test_expect_success 'commit --fixup of existing squash' '
+	commit_for_rebase_autosquash_setup &&
+	git commit --squash HEAD~1 &&
+	echo "fourth content line" >>foo &&
+	git add foo
+	git commit --fixup HEAD &&
+	commit_msg_is "fixup! target message subject line"
+'
+
+test_expect_success 'commit --squash of existing squash' '
+	commit_for_rebase_autosquash_setup &&
+	git commit --squash HEAD~1 &&
+	echo "fourth content line" >>foo &&
+	git add foo
+	git commit --squash HEAD &&
+	commit_msg_is "squash! target message subject linecommit message"
+'
+
+test_expect_success 'commit --squash of existing fixup' '
+	commit_for_rebase_autosquash_setup &&
+	git commit --fixup HEAD~1 &&
+	echo "fourth content line" >>foo &&
+	git add foo
+	git commit --squash HEAD &&
+	commit_msg_is "squash! target message subject linecommit message"
+'
+
 test_done
-- 
1.7.10.4

Re: [PATCH] rebase -i: fixup fixup! fixup!

[Junio C Hamano]

Andrew Pimlott <andrew@pimlott.net> writes:

> Excerpts from Andrew Pimlott's message of Fri Jun 14 12:31:57 -0700 2013:
>> It happened to work and I added a test.  But then it occurred to me that
>> it might have been better to fix commit --fixup/--squash to strip the
>> fixup! or squash! from the referenced commit in the first place.
>> Anyhow, below is my patch for --autosquash, but unles someone has an
>> objection to doing it in commit, I'll work on that.

Is it always true that you would squash and fixup in the same order
as these follow-up commits happened?

That is, if you did this (time flows from top to bottom):

	1 A
        2 B
        3 fixup A
        4 squash B
        5 fixup fixup A
        6 fixup A

I am wondering if applying 6 on top of 5 is always what you want, or
you would want to apply it to 3 instead.  Otherwise you would have
written

	6 fixup fixup fixup A

instead.

The two reordering possibilities are:

        1 A                        1 A             
        3 fixup A                  3 fixup A       
        5 fixup fixup A            6 fixup A       
        6 fixup A                  5 fixup fixup A
        2 B                        2 B             
        4 squash B                 4 squash B      

If you strip out the prefix when you make commits, you may lose the
information if you want to use in order to express these different
orders.  I am not sure if it matters in practice, but I am not yet
convinced it is a good idea.

By the way, the message I am responding to is not something we can
apply. I am assuming these paches are for discussion-only; before
sending the final one, please check Documentation/SubmittingPatches.

Thanks.

Re: [PATCH] rebase -i: fixup fixup! fixup!

[Junio C Hamano]

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

> Andrew Pimlott <andrew@pimlott.net> writes:
>
>> Excerpts from Andrew Pimlott's message of Fri Jun 14 12:31:57 -0700 2013:
>>> It happened to work and I added a test.  But then it occurred to me that
>>> it might have been better to fix commit --fixup/--squash to strip the
>>> fixup! or squash! from the referenced commit in the first place.
>>> Anyhow, below is my patch for --autosquash, but unles someone has an
>>> objection to doing it in commit, I'll work on that.
> ...
> If you strip out the prefix when you make commits, you may lose the
> information if you want to use in order to express these different
> orders.

One design principle I would use as a yardstick is to see any code
that deliberately lose information to achieve something as highly
suspicious.  You can discard extra information when you read and
use, if you do not need it, but if you do not record it in the first
place, you cannot later enhance the reader to take advantage of it.

In general, whenever you see yourself _discarding_ information to
solve an issue, you should carefully ask yourself if that is the
right solution.

I wish we can make sure contributors can learn various design
principles we have benefited from over the course of this project
much better.

But it is a bit difficult to _teach_ others.

Writing them down is difficult, not because the rules are vague, but
because they are like air.  I am sure regular contributors with good
design taste share this sentiment.

You will know a violation of them when you see one, you naturally
stick to them yourself without even having to think about them, but
enumerating them without seeing concrete issues takes effort.

And this "lets squash multiple --fixup/--squash" happened to realize
that "we try not to deliberately lose information" is one of them.

Thanks.

Re: [PATCH] rebase -i: fixup fixup! fixup!

[Thomas Rast]

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

> Andrew Pimlott <andrew@pimlott.net> writes:
>
>> Excerpts from Andrew Pimlott's message of Fri Jun 14 12:31:57 -0700 2013:
>>> It happened to work and I added a test.  But then it occurred to me that
>>> it might have been better to fix commit --fixup/--squash to strip the
>>> fixup! or squash! from the referenced commit in the first place.
>>> Anyhow, below is my patch for --autosquash, but unles someone has an
>>> objection to doing it in commit, I'll work on that.
>
> Is it always true that you would squash and fixup in the same order
> as these follow-up commits happened?
>
> That is, if you did this (time flows from top to bottom):
>
> 	1 A
>         2 B
>         3 fixup A
>         4 squash B
>         5 fixup fixup A
>         6 fixup A
>
> I am wondering if applying 6 on top of 5 is always what you want, or
> you would want to apply it to 3 instead.  Otherwise you would have
> written
>
> 	6 fixup fixup fixup A
>
> instead.
>
> The two reordering possibilities are:
>
>         1 A                        1 A             
>         3 fixup A                  3 fixup A       
>         5 fixup fixup A            6 fixup A       
>         6 fixup A                  5 fixup fixup A
>         2 B                        2 B             
>         4 squash B                 4 squash B      
>
> If you strip out the prefix when you make commits, you may lose the
> information if you want to use in order to express these different
> orders.  I am not sure if it matters in practice, but I am not yet
> convinced it is a good idea.

Isn't it a bit of an academic question?

All 'fixup* A' are clearly intended to be squashed into A eventually.
You could reorder them, but unless you arranged your fixups as nonlinear
history (does anyone do that?) they have been built sequentially.  So at
best the extra reordering does not buy you anything, because you're
going to fix up A anyways.  You may even get extra conflicts during the
reordering, which make the process less automatic and more error-prone.

  [If you did actually arrange things nonlinearly, so that you have

    * A
    |\
    | * fixup A
    | |
    * | fixup A
    |/
    * M  (you need M so that you can rebase both fixups simultaneously)

  then you might actually use the number of 'fixup' prefixes to determine
  their order.  However, if you actually generate such history, you have
  to go out of your way to look at the other branches too, and make sure
  that the number of prefixes is sufficiently unique to disambiguate the
  order as far as you want it to do that, etc.  It sounds like too much of
  a headache to be worth using like that.]

And once you have that, it seems a nicer and cleaner idea to generate
'fixup! A' each time, instead of a successive sequence of

  fixup! A
  fixup! fixup! A
  fixup! fixup! fixup! A
  ...

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] rebase -i: fixup fixup! fixup!

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> Isn't it a bit of an academic question?
> ...
> And once you have that, it seems a nicer and cleaner idea to generate
> 'fixup! A' each time, instead of a successive sequence of
>
>   fixup! A
>   fixup! fixup! A
>   fixup! fixup! fixup! A
>   ...

As to reordering, you are absolutely correct.

If you are going to apply all three anyway, then the end result
either does not change at all (when none of them overlap textually),
or you will end up with unnecessary conflicts (when they do).

But if you were to pick (and drop some), all three labeled with
"fixup A" vs later ones having more "fixup" in front will make a
difference in identification and usability.  When you want to drop
the second fixup, "fixup fixup A" can be chosen unambiguously in
your editor among "fixup A", "fixup fixup A" and "fixup fixup fixup
A".

It also somewhat feels wrong when the user sees this:

    $ git log --oneline -2
    xxxx A
    yyyy fixup! A

and asks to do this:

    $ git commit --fixup yyyy

and if you end up with "fixup! A", not "fixup! fixup! A".  The user
is asking to follow-up on the "fixup! A", not on the original "A".

Does dropping these leading "fixup!" (or "squash!") at commit time
make the application in "rebase -i --autosquash" significantly
easier to do?

Re: [PATCH] rebase -i: fixup fixup! fixup!

[Thomas Rast]

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

> Thomas Rast <trast@inf.ethz.ch> writes:
>
>> Isn't it a bit of an academic question?
>> ...
>> And once you have that, it seems a nicer and cleaner idea to generate
>> 'fixup! A' each time, instead of a successive sequence of
>>
>>   fixup! A
>>   fixup! fixup! A
>>   fixup! fixup! fixup! A
>>   ...
>
> As to reordering, you are absolutely correct.
[...]
> Does dropping these leading "fixup!" (or "squash!") at commit time
> make the application in "rebase -i --autosquash" significantly
> easier to do?

Conveniently enough we have seen both already ;-)  Andrew's version for
commit.c could use a bit of refactorization, since it inserts the same
code in two places, but then it's about the same complexity as the
change for rebase.

I'm not sure it's worth arguing about whether the "fixup! fixup!"  is a
symptom of some underlying problem, and changing rebase is only tapering
over the symptom; or whether it's actually a useful distinction.  Either
one works fine as a fix for an annoyance that Andrew had, and that bit
me in the past too.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] rebase -i: fixup fixup! fixup!

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> Conveniently enough we have seen both already ;-)  Andrew's version for
> commit.c could use a bit of refactorization, since it inserts the same
> code in two places, but then it's about the same complexity as the
> change for rebase.
>
> I'm not sure it's worth arguing about whether the "fixup! fixup!"  is a
> symptom of some underlying problem, and changing rebase is only tapering
> over the symptom; or whether it's actually a useful distinction.

If they are about the same complexity, then my instict tells me that
it is a better design not to strip on the writing side.

Thanks.

Re: [PATCH] rebase -i: fixup fixup! fixup!

[Andrew Pimlott]

Excerpts from Junio C Hamano's message of Mon Jun 17 07:27:20 -0700 2013:
> Thomas Rast <trast@inf.ethz.ch> writes:
> > I'm not sure it's worth arguing about whether the "fixup! fixup!"  is a
> > symptom of some underlying problem, and changing rebase is only tapering
> > over the symptom; or whether it's actually a useful distinction.
> 
> If they are about the same complexity, then my instict tells me that
> it is a better design not to strip on the writing side.

Thank you for the discussion.  Sorry I have joined recently.

I agree that it is better to preserve information as long as feasible.
If we are going to strip it, it may as well be later.  That is Thomas's
rearrange_squash patch, which I will send again.

The next question is, do we go all the way and respect the nested
fixup!s in rearrange_squash?  I understand the case for it, though it's
hardly compelling to me in practice. :-)  That would be more complicated
than Thomas's patch.  But I'm happy to try it if someone gives me a
nudge.  If not, at least the information is preserved in case someone
wants to do this later.

Regarding patches, I tried to follow the SubmittingPatches guidelines,
but I was confused about how to include a commit in an existing thread.
I think I was mislead by git-format-patch(1), "When a patch is part of
an ongoing discussion...", which says to remove most header fields.

So if I don't want to break the discussion, should I append the unedited
format-patch output to my message after "scissors", or should I send it
as a whole new message with --in-reply-to?  Or something else?  I'll try
the first.

Andrew

---8<---
From 99023bff23f18a341441d6b7c447d9630a11b489 Mon Sep 17 00:00:00 2001
From: Andrew Pimlott <andrew@pimlott.net>
Date: Fri, 14 Jun 2013 10:33:16 -0700
Subject: [PATCH 1/4] rebase -i: handle fixup! fixup! in --autosquash

In rebase -i --autosquash, ignore all "fixup! " or "squash! " after the
first.  Handy in case a git commit --fixup/--squash referred to an earlier
fixup/squash instead of the original commit, for example with :/msg.

Signed-off-by: Andrew Pimlott <andrew@pimlott.net>
---
 Documentation/git-rebase.txt |    4 +++-
 git-rebase--interactive.sh   |   13 ++++++++++-
 t/t3415-rebase-autosquash.sh |   49 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index c84854a..6b2e1c8 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -389,7 +389,9 @@ squash/fixup series.
 	the same ..., automatically modify the todo list of rebase -i
 	so that the commit marked for squashing comes right after the
 	commit to be modified, and change the action of the moved
-	commit from `pick` to `squash` (or `fixup`).
+	commit from `pick` to `squash` (or `fixup`).  Ignores subsequent
+	"fixup! " or "squash! " after the first, in case you referred to an
+	earlier fixup/squash with `git commit --fixup/--squash`.
 +
 This option is only valid when the '--interactive' option is used.
 +
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f953d8d..54ed4c3 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -689,7 +689,18 @@ rearrange_squash () {
 		case "$message" in
 		"squash! "*|"fixup! "*)
 			action="${message%%!*}"
-			rest="${message#*! }"
+			rest=$message
+			# ignore any squash! or fixup! after the first
+			while : ; do
+				case "$rest" in
+				"squash! "*|"fixup! "*)
+					rest="${rest#*! }"
+					;;
+				*)
+					break
+					;;
+				esac
+			done
 			echo "$sha1 $action $rest"
 			# if it's a single word, try to resolve to a full sha1 and
 			# emit a second copy. This allows us to match on both message
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index a1e86c4..1a3f40a 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -193,4 +193,53 @@ test_expect_success 'use commit --squash' '
 	test_auto_commit_flags squash 2
 '
 
+test_auto_fixup_fixup () {
+	git reset --hard base &&
+	echo 1 >file1 &&
+	git add -u &&
+	test_tick &&
+	git commit -m "$1! first" &&
+	echo 2 >file1 &&
+	git add -u &&
+	test_tick &&
+	git commit -m "$1! $2! first" &&
+	git tag "final-$1-$2" &&
+	test_tick &&
+	git rebase --autosquash -i HEAD^^^^ &&
+	git log --oneline >actual &&
+	test_pause &&
+	if [ "$1" = "fixup" ]; then
+		test_line_count = 3 actual
+	elif [ "$1" = "squash" ]; then
+		test_line_count = 4 actual
+	else
+		false
+	fi &&
+	git diff --exit-code "final-$1-$2" &&
+	test 2 = "$(git cat-file blob HEAD^:file1)" &&
+	if [ "$1" = "fixup" ]; then
+		test 1 = $(git cat-file commit HEAD^ | grep first | wc -l)
+	elif [ "$1" = "squash" ]; then
+		test 3 = $(git cat-file commit HEAD^ | grep first | wc -l)
+	else
+		false
+	fi
+}
+
+test_expect_success 'fixup! fixup!' '
+	test_auto_fixup_fixup fixup fixup
+'
+
+test_expect_success 'fixup! squash!' '
+	test_auto_fixup_fixup fixup squash
+'
+
+test_expect_success 'squash! squash!' '
+	test_auto_fixup_fixup squash squash
+'
+
+test_expect_success 'squash! fixup!' '
+	test_auto_fixup_fixup squash fixup
+'
+
 test_done
-- 
1.7.10.4

Re: [PATCH] rebase -i: fixup fixup! fixup!

[Junio C Hamano]

Andrew Pimlott <andrew@pimlott.net> writes:

Just reponding for the "procedual" part for now.

> So if I don't want to break the discussion, should I append the unedited
> format-patch output to my message after "scissors", or should I send it
> as a whole new message with --in-reply-to?  Or something else?  I'll try
> the first.

Which is fine, and you are almost there, but you do not want

 (1) "From 99023b..." that is not part of the message (it is a
     delimiter between multiple patches when/in case a file contains
     more than one);

 (2) "From: Andrew..." that is the same as the e-mail header in the
     message I am responding to;

 (3) "Date: ..." which is older than the e-mail header in the
     message I am responding to---the latter is the date people
     actually saw this patch on the mailing list, so it is
     preferrable to use it than the timestamp in your repository.

So in this case, I'd expect to see, after the "-- >8 --" line, only
"Subject: " line, a blank and the log message.

>
> ---8<---
> From 99023bff23f18a341441d6b7c447d9630a11b489 Mon Sep 17 00:00:00 2001
> From: Andrew Pimlott <andrew@pimlott.net>
> Date: Fri, 14 Jun 2013 10:33:16 -0700
> Subject: [PATCH 1/4] rebase -i: handle fixup! fixup! in --autosquash
>
> In rebase -i --autosquash, ignore all "fixup! " or "squash! " after the

Re: [PATCH] rebase -i: fixup fixup! fixup!

[Andrew Pimlott]

Excerpts from Junio C Hamano's message of Tue Jun 25 14:33:18 -0700 2013:
> Andrew Pimlott <andrew@pimlott.net> writes:
> 
> Just reponding for the "procedual" part for now.
> 
> > So if I don't want to break the discussion, should I append the unedited
> > format-patch output to my message after "scissors", or should I send it
> > as a whole new message with --in-reply-to?  Or something else?  I'll try
> > the first.
> 
> Which is fine, and you are almost there, but you do not want
> 
>  (1) "From 99023b..." that is not part of the message (it is a
>      delimiter between multiple patches when/in case a file contains
>      more than one);
> 
>  (2) "From: Andrew..." that is the same as the e-mail header in the
>      message I am responding to;
> 
>  (3) "Date: ..." which is older than the e-mail header in the
>      message I am responding to---the latter is the date people
>      actually saw this patch on the mailing list, so it is
>      preferrable to use it than the timestamp in your repository.
> 
> So in this case, I'd expect to see, after the "-- >8 --" line, only
> "Subject: " line, a blank and the log message.

Thank you.  It was not clear to me even after several doc readings what
git-mailinfo would look for where.  I think I assumed that the idea was
to transmit the original commit perfectly, and I stubbornly failed to
give up that assumption even when it clearly didn't fit.  Everything
makes more sense with the understanding that the receiver will pull
together non-patch metadata in the way that makes sense from his point
of view (and that a different commit will come back via fetch).  I will
take a whack at clarifying the docs if I have time.

Andrew

Re: [PATCH] rebase -i: fixup fixup! fixup!

[Junio C Hamano]

Andrew Pimlott <andrew@pimlott.net> writes:

> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index c84854a..6b2e1c8 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -389,7 +389,9 @@ squash/fixup series.
>  	the same ..., automatically modify the todo list of rebase -i
>  	so that the commit marked for squashing comes right after the
>  	commit to be modified, and change the action of the moved
> -	commit from `pick` to `squash` (or `fixup`).
> +	commit from `pick` to `squash` (or `fixup`).  Ignores subsequent
> +	"fixup! " or "squash! " after the first, in case you referred to an
> +	earlier fixup/squash with `git commit --fixup/--squash`.
>  +
>  This option is only valid when the '--interactive' option is used.
>  +
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index f953d8d..54ed4c3 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -689,7 +689,18 @@ rearrange_squash () {
>  		case "$message" in
>  		"squash! "*|"fixup! "*)
>  			action="${message%%!*}"
> -			rest="${message#*! }"
> +			rest=$message
> +			# ignore any squash! or fixup! after the first
> +			while : ; do

Style:

	while :
        do

> +				case "$rest" in
> +				"squash! "*|"fixup! "*)
> +					rest="${rest#*! }"
> +					;;
> +				*)
> +					break
> +					;;
> +				esac
> +			done
>  			echo "$sha1 $action $rest"
>  			# if it's a single word, try to resolve to a full sha1 and
>  			# emit a second copy. This allows us to match on both message
> diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
> index a1e86c4..1a3f40a 100755
> --- a/t/t3415-rebase-autosquash.sh
> +++ b/t/t3415-rebase-autosquash.sh
> @@ -193,4 +193,53 @@ test_expect_success 'use commit --squash' '
>  	test_auto_commit_flags squash 2
>  '
>  
> +test_auto_fixup_fixup () {
> +	git reset --hard base &&
> +	echo 1 >file1 &&
> +	git add -u &&
> +	test_tick &&
> +	git commit -m "$1! first" &&
> +	echo 2 >file1 &&
> +	git add -u &&
> +	test_tick &&
> +	git commit -m "$1! $2! first" &&
> +	git tag "final-$1-$2" &&
> +	test_tick &&
> +	git rebase --autosquash -i HEAD^^^^ &&
> +	git log --oneline >actual &&
> +	test_pause &&

This patch obviously hasn't been tested.  It breaks without -v.

> +	if [ "$1" = "fixup" ]; then
> +		test_line_count = 3 actual
> +	elif [ "$1" = "squash" ]; then
> +		test_line_count = 4 actual
> +	else
> +		false
> +	fi &&

Style

	if test "$1" = "fixup"
	then
        	...
	elif test "$1" = "squash"
	then
		...

(you got the idea).

> +	git diff --exit-code "final-$1-$2" &&
> +	test 2 = "$(git cat-file blob HEAD^:file1)" &&
> +	if [ "$1" = "fixup" ]; then
> +		test 1 = $(git cat-file commit HEAD^ | grep first | wc -l)
> +	elif [ "$1" = "squash" ]; then
> +		test 3 = $(git cat-file commit HEAD^ | grep first | wc -l)
> +	else
> +		false
> +	fi
> +}
> +
> +test_expect_success 'fixup! fixup!' '
> +	test_auto_fixup_fixup fixup fixup
> +'
> +
> +test_expect_success 'fixup! squash!' '
> +	test_auto_fixup_fixup fixup squash
> +'
> +
> +test_expect_success 'squash! squash!' '
> +	test_auto_fixup_fixup squash squash
> +'

This does not seem to pass for me.

> +test_expect_success 'squash! fixup!' '
> +	test_auto_fixup_fixup squash fixup
> +'
> +
>  test_done

Re: [PATCH] rebase -i: fixup fixup! fixup!

[Junio C Hamano]

Andrew Pimlott <andrew@pimlott.net> writes:

> I agree that it is better to preserve information as long as feasible.
> If we are going to strip it, it may as well be later.  That is Thomas's
> rearrange_squash patch, which I will send again.

Thanks.

> The next question is, do we go all the way and respect the nested
> fixup!s in rearrange_squash?  I understand the case for it, though it's
> hardly compelling to me in practice. :-)  That would be more complicated
> than Thomas's patch.  But I'm happy to try it if someone gives me a
> nudge.  If not, at least the information is preserved in case someone
> wants to do this later.

I think it is fine not to be too smart, as long as we do not lose
information that would help the user to compensate.

After all, autosquash will give the user an opportunity to eyeball
the result of automatic rearrangement.  If the user did this:

	git commit -m original
        git commit --fixup original ;# obviously fixing the first one
        git commit --fixup '!fixup original' ;# explicitly fixing the second
	git commit --fixup original ;# may want to fix the first one

and then "git rebase --autosquash" gave him this:

	pick d78c915 original
        fixup 0c6388e original
        fixup d15b556 !fixup original
        fixup 1e39bcd original

it may not be what the user originally intended, but I think it is
OK.

As long as "!fixup original" message is kept in the buffer, the user
can notice and rearrange, e.g.

	pick d78c915 original
        fixup 0c6388e original
        fixup 1e39bcd original
        fixup d15b556 !fixup original

if the user really wants to.

Re: [PATCH] rebase -i: fixup fixup! fixup!

[Junio C Hamano]

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

I guess I made typoes in the examples that made then unusable...

> I think it is fine not to be too smart, as long as we do not lose
> information that would help the user to compensate.
>
> After all, autosquash will give the user an opportunity to eyeball
> the result of automatic rearrangement.  If the user did this:
>
> 	git commit -m original
>       git commit --fixup original ;# obviously fixing the first one
>       git commit --fixup '!fixup original' ;# explicitly fixing the second
> 	git commit --fixup original ;# may want to fix the first one
>
> and then "git rebase --autosquash" gave him this:
>
(the result of automatic rearrangement should read like this)

        pick d78c915 original
        fixup 0c6388e !fixup original
        fixup d15b556 !fixup !fixup original
        fixup 1e39bcd !fixup original

> it may not be what the user originally intended, but I think it is
> OK.
>
> As long as "!fixup original" message is kept in the buffer, the user
> can notice and rearrange, e.g.

(and the manual rearrangement should read like this)

        pick d78c915 original
        fixup 0c6388e !fixup original
        fixup 1e39bcd !fixup original
        fixup d15b556 !fixup !fixup original

> if the user really wants to.

Re: [PATCH] rebase -i: fixup fixup! fixup!

[Andrew Pimlott]

Excerpts from Junio C Hamano's message of Tue Jun 25 14:45:07 -0700 2013:
> After all, autosquash will give the user an opportunity to eyeball
> the result of automatic rearrangement.  If the user did this:
> 
>     git commit -m original
>         git commit --fixup original ;# obviously fixing the first one
>         git commit --fixup '!fixup original' ;# explicitly fixing the second
>     git commit --fixup original ;# may want to fix the first one
> 
> and then "git rebase --autosquash" gave him this:
> 
>     pick d78c915 original
>         fixup 0c6388e original
>         fixup d15b556 !fixup original
>         fixup 1e39bcd original

I assume you mean:

    pick d78c915 original
    fixup 0c6388e fixup! original
    fixup d15b556 fixup! fixup! original
    fixup 1e39bcd !fixup! original

The current master code tries to keep the original commit message
intact.  I assume you would preserve that behavior, so you would want to
see "fixup! fixup!"

> it may not be what the user originally intended, but I think it is
> OK.
> 
> As long as "!fixup original" message is kept in the buffer, the user
> can notice and rearrange, e.g.

Thomas's patch didn't do this: fixup! or squash! after the first is
simply discarded, so you see:

    pick d78c915 original
    fixup 0c6388e fixup! original
    fixup d15b556 fixup! original
    fixup 1e39bcd !fixup! original

But it will be a simple change to keep all the fixup!s and squash!s.  I
will do this (and try to make up for the carelessness of my previous
patch).

Andrew

Re: [PATCH] rebase -i: fixup fixup! fixup!

[Andrew Pimlott]

Excerpts from Andrew Pimlott's message of Tue Jun 25 16:03:52 -0700 2013:
> Thomas's patch didn't do this: fixup! or squash! after the first is
> simply discarded, so you see:
> 
>     pick d78c915 original
>     fixup 0c6388e fixup! original
>     fixup d15b556 fixup! original
>     fixup 1e39bcd fixup! original
> 
> But it will be a simple change to keep all the fixup!s and squash!s.  I
> will do this (and try to make up for the carelessness of my previous
> patch).

In order to test this, I wrote a helper function to dump the rebase -i
todo list.  Would you like this introduced in its own patch, or
combined?  See below.

Andrew

---8<---
Subject: [PATCH] lib-rebase: set_cat_todo_editor

Add a helper for testing rebase -i todo lists.  This can be used to verify
the expected user experience, even for todo list changes that do not affect
the outcome of rebase.

Signed-off-by: Andrew Pimlott <andrew@pimlott.net>
---
 t/lib-rebase.sh |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 4b74ae4..d118dd6 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -66,6 +66,19 @@ EOF
 	chmod a+x fake-editor.sh
 }
 
+# After set_cat_todo_editor, rebase -i will write the todo list (ignoring
+# blank lines and comments) to stdout, and exit failure.
+
+set_cat_todo_editor () {
+	echo "#!$SHELL_PATH" >fake-editor.sh
+	cat >> fake-editor.sh <<\EOF
+grep "^[^#]" "$1"
+exit 1
+EOF
+	chmod a+x fake-editor.sh
+	test_set_editor "$(pwd)/fake-editor.sh"
+}
+
 # checks that the revisions in "$2" represent a linear range with the
 # subjects in "$1"
 test_linear_range () {
-- 
1.7.10.4

Re: [PATCH] rebase -i: fixup fixup! fixup!

[Junio C Hamano]

Andrew Pimlott <andrew@pimlott.net> writes:

> In order to test this, I wrote a helper function to dump the rebase -i
> todo list.  Would you like this introduced in its own patch, or
> combined?  See below.

Depends on how involved the addition of the tests that actually use
the helper, but in general it would be a good idea to add it in the
first patch that actually uses it.  Unused code added in a separate
patch will not point at that patch when bisecting, if that unused
code was broken from the beginning (not that I see anything
immediately broken in the code the following adds).


> ---8<---
> Subject: [PATCH] lib-rebase: set_cat_todo_editor
>
> Add a helper for testing rebase -i todo lists.  This can be used to verify
> the expected user experience, even for todo list changes that do not affect
> the outcome of rebase.
>
> Signed-off-by: Andrew Pimlott <andrew@pimlott.net>
> ---
>  t/lib-rebase.sh |   13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
> index 4b74ae4..d118dd6 100644
> --- a/t/lib-rebase.sh
> +++ b/t/lib-rebase.sh
> @@ -66,6 +66,19 @@ EOF
>  	chmod a+x fake-editor.sh
>  }
>  
> +# After set_cat_todo_editor, rebase -i will write the todo list (ignoring
> +# blank lines and comments) to stdout, and exit failure.
> +
> +set_cat_todo_editor () {
> +	echo "#!$SHELL_PATH" >fake-editor.sh
> +	cat >> fake-editor.sh <<\EOF
> +grep "^[^#]" "$1"
> +exit 1
> +EOF
> +	chmod a+x fake-editor.sh

These days we should use write_script to do this kind of thing, I
think.

> +	test_set_editor "$(pwd)/fake-editor.sh"
> +}
> +
>  # checks that the revisions in "$2" represent a linear range with the
>  # subjects in "$1"
>  test_linear_range () {

Re: [PATCH] rebase -i: fixup fixup! fixup!

[Andrew Pimlott]

Excerpts from Junio C Hamano's message of Wed Jun 26 16:48:57 -0700 2013:
> Andrew Pimlott <andrew@pimlott.net> writes:
> > In order to test this, I wrote a helper function to dump the rebase -i
> > todo list.  Would you like this introduced in its own patch, or
> > combined?  See below.
> 
> Depends on how involved the addition of the tests that actually use
> the helper, but in general it would be a good idea to add it in the
> first patch that actually uses it.  Unused code added in a separate
> patch will not point at that patch when bisecting, if that unused
> code was broken from the beginning (not that I see anything
> immediately broken in the code the following adds).

Ok, here is the complete commit, incorporating all feedback.

Andrew

---8<---
Subject: [PATCH 1/3] rebase -i: handle fixup! fixup! in --autosquash

In rebase -i --autosquash, ignore all "fixup! " or "squash! " after the
first.  This supports the case when a git commit --fixup/--squash referred
to an earlier fixup/squash instead of the original commit (whether
intentionally, as when the user expressly meant to note that the commit
fixes an earlier fixup; or inadvertently, as when the user meant to refer to
the original commit with :/msg; or out of laziness, as when the user could
remember how to refer to the fixup but not the original).

In the todo list, the full commit message is preserved, in case it provides
useful cues to the user.  A test helper set_cat_todo_editor is introduced to
check this.

Helped-by: Thomas Rast <trast@inf.ethz.ch>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Andrew Pimlott <andrew@pimlott.net>
---
 Documentation/git-rebase.txt |    4 ++-
 git-rebase--interactive.sh   |   25 ++++++++++++++----
 t/lib-rebase.sh              |   14 +++++++++++
 t/t3415-rebase-autosquash.sh |   57 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 94 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index c84854a..6b2e1c8 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -389,7 +389,9 @@ squash/fixup series.
 	the same ..., automatically modify the todo list of rebase -i
 	so that the commit marked for squashing comes right after the
 	commit to be modified, and change the action of the moved
-	commit from `pick` to `squash` (or `fixup`).
+	commit from `pick` to `squash` (or `fixup`).  Ignores subsequent
+	"fixup! " or "squash! " after the first, in case you referred to an
+	earlier fixup/squash with `git commit --fixup/--squash`.
 +
 This option is only valid when the '--interactive' option is used.
 +
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f953d8d..169e876 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -689,8 +689,22 @@ rearrange_squash () {
 		case "$message" in
 		"squash! "*|"fixup! "*)
 			action="${message%%!*}"
-			rest="${message#*! }"
-			echo "$sha1 $action $rest"
+			rest=$message
+			prefix=
+			# skip all squash! or fixup! (but save for later)
+			while :
+			do
+				case "$rest" in
+				"squash! "*|"fixup! "*)
+					prefix="$prefix${rest%%!*},"
+					rest="${rest#*! }"
+					;;
+				*)
+					break
+					;;
+				esac
+			done
+			echo "$sha1 $action $prefix $rest"
 			# if it's a single word, try to resolve to a full sha1 and
 			# emit a second copy. This allows us to match on both message
 			# and on sha1 prefix
@@ -699,7 +713,7 @@ rearrange_squash () {
 				if test -n "$fullsha"; then
 					# prefix the action to uniquely identify this line as
 					# intended for full sha1 match
-					echo "$sha1 +$action $fullsha"
+					echo "$sha1 +$action $prefix $fullsha"
 				fi
 			fi
 		esac
@@ -714,7 +728,7 @@ rearrange_squash () {
 		esac
 		printf '%s\n' "$pick $sha1 $message"
 		used="$used$sha1 "
-		while read -r squash action msg_content
+		while read -r squash action msg_prefix msg_content
 		do
 			case " $used" in
 			*" $squash "*) continue ;;
@@ -730,7 +744,8 @@ rearrange_squash () {
 				case "$message" in "$msg_content"*) emit=1;; esac ;;
 			esac
 			if test $emit = 1; then
-				printf '%s\n' "$action $squash $action! $msg_content"
+				real_prefix=$(echo "$msg_prefix" | sed "s/,/! /g")
+				printf '%s\n' "$action $squash ${real_prefix}$msg_content"
 				used="$used$squash "
 			fi
 		done <"$1.sq"
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 4b74ae4..be5a4c7 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -66,6 +66,20 @@ EOF
 	chmod a+x fake-editor.sh
 }
 
+# After set_cat_todo_editor, rebase -i will write the todo list (ignoring
+# blank lines and comments) to stdout, and exit failure (so you should run
+# it with test_must_fail).  This can be used to verify the expected user
+# experience, for todo list changes that do not affect the outcome of
+# rebase; or as an extra check in addition to checking the outcome.
+
+set_cat_todo_editor () {
+	write_script fake-editor.sh <<\EOF
+grep "^[^#]" "$1"
+exit 1
+EOF
+	test_set_editor "$(pwd)/fake-editor.sh"
+}
+
 # checks that the revisions in "$2" represent a linear range with the
 # subjects in "$1"
 test_linear_range () {
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index a1e86c4..7c989f9 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -4,6 +4,8 @@ test_description='auto squash'
 
 . ./test-lib.sh
 
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
 test_expect_success setup '
 	echo 0 >file0 &&
 	git add . &&
@@ -193,4 +195,59 @@ test_expect_success 'use commit --squash' '
 	test_auto_commit_flags squash 2
 '
 
+test_auto_fixup_fixup () {
+	git reset --hard base &&
+	echo 1 >file1 &&
+	git add -u &&
+	test_tick &&
+	git commit -m "$1! first" &&
+	echo 2 >file1 &&
+	git add -u &&
+	test_tick &&
+	git commit -m "$1! $2! first" &&
+	git tag "final-$1-$2" &&
+	test_tick &&
+	(
+		set_cat_todo_editor &&
+		test_must_fail git rebase --autosquash -i HEAD^^^^ >actual &&
+		cat >expected <<EOF
+pick $(git rev-parse --short HEAD^^^) first commit
+$1 $(git rev-parse --short HEAD^) $1! first
+$1 $(git rev-parse --short HEAD) $1! $2! first
+pick $(git rev-parse --short HEAD^^) second commit
+EOF
+		test_cmp expected actual
+	) &&
+	git rebase --autosquash -i HEAD^^^^ &&
+	git log --oneline >actual &&
+	test_line_count = 3 actual
+	git diff --exit-code "final-$1-$2" &&
+	test 2 = "$(git cat-file blob HEAD^:file1)" &&
+	if test "$1" = "fixup"
+	then
+		test 1 = $(git cat-file commit HEAD^ | grep first | wc -l)
+	elif test "$1" = "squash"
+	then
+		test 3 = $(git cat-file commit HEAD^ | grep first | wc -l)
+	else
+		false
+	fi
+}
+
+test_expect_success 'fixup! fixup!' '
+	test_auto_fixup_fixup fixup fixup
+'
+
+test_expect_success 'fixup! squash!' '
+	test_auto_fixup_fixup fixup squash
+'
+
+test_expect_success 'squash! squash!' '
+	test_auto_fixup_fixup squash squash
+'
+
+test_expect_success 'squash! fixup!' '
+	test_auto_fixup_fixup squash fixup
+'
+
 test_done
-- 
1.7.10.4

Re: [PATCH] rebase -i: fixup fixup! fixup!

[Andrew Pimlott]

Excerpts from Andrew Pimlott's message of Wed Jun 26 17:20:32 -0700 2013:
> Excerpts from Junio C Hamano's message of Wed Jun 26 16:48:57 -0700 2013:
> > Andrew Pimlott <andrew@pimlott.net> writes:
> > > In order to test this, I wrote a helper function to dump the rebase -i
> > > todo list.  Would you like this introduced in its own patch, or
> > > combined?  See below.
> > 
> > Depends on how involved the addition of the tests that actually use
> > the helper, but in general it would be a good idea to add it in the
> > first patch that actually uses it.  Unused code added in a separate
> > patch will not point at that patch when bisecting, if that unused
> > code was broken from the beginning (not that I see anything
> > immediately broken in the code the following adds).
> 
> Ok, here is the complete commit, incorporating all feedback.

Updated for recommended here-doc style.

Andrew

---8<---
Subject: [PATCH] rebase -i: handle fixup! fixup! in --autosquash

In rebase -i --autosquash, ignore all "fixup! " or "squash! " after the
first.  This supports the case when a git commit --fixup/--squash referred
to an earlier fixup/squash instead of the original commit (whether
intentionally, as when the user expressly meant to note that the commit
fixes an earlier fixup; or inadvertently, as when the user meant to refer to
the original commit with :/msg; or out of laziness, as when the user could
remember how to refer to the fixup but not the original).

In the todo list, the full commit message is preserved, in case it provides
useful cues to the user.  A test helper set_cat_todo_editor is introduced to
check this.

Helped-by: Thomas Rast <trast@inf.ethz.ch>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Andrew Pimlott <andrew@pimlott.net>
---
 Documentation/git-rebase.txt |    4 ++-
 git-rebase--interactive.sh   |   25 ++++++++++++++----
 t/lib-rebase.sh              |   14 +++++++++++
 t/t3415-rebase-autosquash.sh |   57 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 94 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index c84854a..6b2e1c8 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -389,7 +389,9 @@ squash/fixup series.
 	the same ..., automatically modify the todo list of rebase -i
 	so that the commit marked for squashing comes right after the
 	commit to be modified, and change the action of the moved
-	commit from `pick` to `squash` (or `fixup`).
+	commit from `pick` to `squash` (or `fixup`).  Ignores subsequent
+	"fixup! " or "squash! " after the first, in case you referred to an
+	earlier fixup/squash with `git commit --fixup/--squash`.
 +
 This option is only valid when the '--interactive' option is used.
 +
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f953d8d..169e876 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -689,8 +689,22 @@ rearrange_squash () {
 		case "$message" in
 		"squash! "*|"fixup! "*)
 			action="${message%%!*}"
-			rest="${message#*! }"
-			echo "$sha1 $action $rest"
+			rest=$message
+			prefix=
+			# skip all squash! or fixup! (but save for later)
+			while :
+			do
+				case "$rest" in
+				"squash! "*|"fixup! "*)
+					prefix="$prefix${rest%%!*},"
+					rest="${rest#*! }"
+					;;
+				*)
+					break
+					;;
+				esac
+			done
+			echo "$sha1 $action $prefix $rest"
 			# if it's a single word, try to resolve to a full sha1 and
 			# emit a second copy. This allows us to match on both message
 			# and on sha1 prefix
@@ -699,7 +713,7 @@ rearrange_squash () {
 				if test -n "$fullsha"; then
 					# prefix the action to uniquely identify this line as
 					# intended for full sha1 match
-					echo "$sha1 +$action $fullsha"
+					echo "$sha1 +$action $prefix $fullsha"
 				fi
 			fi
 		esac
@@ -714,7 +728,7 @@ rearrange_squash () {
 		esac
 		printf '%s\n' "$pick $sha1 $message"
 		used="$used$sha1 "
-		while read -r squash action msg_content
+		while read -r squash action msg_prefix msg_content
 		do
 			case " $used" in
 			*" $squash "*) continue ;;
@@ -730,7 +744,8 @@ rearrange_squash () {
 				case "$message" in "$msg_content"*) emit=1;; esac ;;
 			esac
 			if test $emit = 1; then
-				printf '%s\n' "$action $squash $action! $msg_content"
+				real_prefix=$(echo "$msg_prefix" | sed "s/,/! /g")
+				printf '%s\n' "$action $squash ${real_prefix}$msg_content"
 				used="$used$squash "
 			fi
 		done <"$1.sq"
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 4b74ae4..cfd3409 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -66,6 +66,20 @@ EOF
 	chmod a+x fake-editor.sh
 }
 
+# After set_cat_todo_editor, rebase -i will write the todo list (ignoring
+# blank lines and comments) to stdout, and exit failure (so you should run
+# it with test_must_fail).  This can be used to verify the expected user
+# experience, for todo list changes that do not affect the outcome of
+# rebase; or as an extra check in addition to checking the outcome.
+
+set_cat_todo_editor () {
+	write_script fake-editor.sh <<-\EOF
+	grep "^[^#]" "$1"
+	exit 1
+	EOF
+	test_set_editor "$(pwd)/fake-editor.sh"
+}
+
 # checks that the revisions in "$2" represent a linear range with the
 # subjects in "$1"
 test_linear_range () {
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index a1e86c4..7c989f9 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -4,6 +4,8 @@ test_description='auto squash'
 
 . ./test-lib.sh
 
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
 test_expect_success setup '
 	echo 0 >file0 &&
 	git add . &&
@@ -193,4 +195,59 @@ test_expect_success 'use commit --squash' '
 	test_auto_commit_flags squash 2
 '
 
+test_auto_fixup_fixup () {
+	git reset --hard base &&
+	echo 1 >file1 &&
+	git add -u &&
+	test_tick &&
+	git commit -m "$1! first" &&
+	echo 2 >file1 &&
+	git add -u &&
+	test_tick &&
+	git commit -m "$1! $2! first" &&
+	git tag "final-$1-$2" &&
+	test_tick &&
+	(
+		set_cat_todo_editor &&
+		test_must_fail git rebase --autosquash -i HEAD^^^^ >actual &&
+		cat >expected <<EOF
+pick $(git rev-parse --short HEAD^^^) first commit
+$1 $(git rev-parse --short HEAD^) $1! first
+$1 $(git rev-parse --short HEAD) $1! $2! first
+pick $(git rev-parse --short HEAD^^) second commit
+EOF
+		test_cmp expected actual
+	) &&
+	git rebase --autosquash -i HEAD^^^^ &&
+	git log --oneline >actual &&
+	test_line_count = 3 actual
+	git diff --exit-code "final-$1-$2" &&
+	test 2 = "$(git cat-file blob HEAD^:file1)" &&
+	if test "$1" = "fixup"
+	then
+		test 1 = $(git cat-file commit HEAD^ | grep first | wc -l)
+	elif test "$1" = "squash"
+	then
+		test 3 = $(git cat-file commit HEAD^ | grep first | wc -l)
+	else
+		false
+	fi
+}
+
+test_expect_success 'fixup! fixup!' '
+	test_auto_fixup_fixup fixup fixup
+'
+
+test_expect_success 'fixup! squash!' '
+	test_auto_fixup_fixup fixup squash
+'
+
+test_expect_success 'squash! squash!' '
+	test_auto_fixup_fixup squash squash
+'
+
+test_expect_success 'squash! fixup!' '
+	test_auto_fixup_fixup squash fixup
+'
+
 test_done
-- 
1.7.10.4

Re: [PATCH] rebase -i: fixup fixup! fixup!

[Junio C Hamano]

Andrew Pimlott <andrew@pimlott.net> writes:

> Updated for recommended here-doc style.

Thanks.

> +test_auto_fixup_fixup () {
> +	git reset --hard base &&
> +	echo 1 >file1 &&
> +	git add -u &&
> +	test_tick &&
> +	git commit -m "$1! first" &&
> +	echo 2 >file1 &&
> +	git add -u &&
> +	test_tick &&
> +	git commit -m "$1! $2! first" &&
> +	git tag "final-$1-$2" &&
> +	test_tick &&
> +	(
> +		set_cat_todo_editor &&
> +		test_must_fail git rebase --autosquash -i HEAD^^^^ >actual &&
> +		cat >expected <<EOF
> +pick $(git rev-parse --short HEAD^^^) first commit
> +$1 $(git rev-parse --short HEAD^) $1! first
> +$1 $(git rev-parse --short HEAD) $1! $2! first
> +pick $(git rev-parse --short HEAD^^) second commit
> +EOF
> +		test_cmp expected actual

Two issues here, which I'll locally amend (no need to resend):

		cat >expected <<-EOF &&
		pick ...
		...
                EOF
		test_cmp expected actual

Re: [PATCH] rebase -i: fixup fixup! fixup!

[Andrew Pimlott]

Excerpts from Junio C Hamano's message of Thu Jun 27 13:52:33 -0700 2013:
> Two issues here, which I'll locally amend (no need to resend):

Great!  Thank you for your help and patience.

>         cat >expected <<-EOF &&
>         pick ...
>         ...
>                 EOF
>         test_cmp expected actual

Is that two issues?

Andrew