Thomas Rast [off-list ref] 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.
Excerpts from Junio C Hamano's message of Mon Jun 17 07:27:20 -0700 2013:
Thomas Rast [off-list ref] writes:
quoted
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 <redacted>
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 <redacted>
---
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 messagediff --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
Andrew Pimlott [off-list ref] 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 <redacted>
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
Excerpts from Junio C Hamano's message of Tue Jun 25 14:33:18 -0700 2013:
Andrew Pimlott [off-list ref] writes:
Just reponding for the "procedual" part for now.
quoted
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