Re: [PATCH v2 0/6] Fix checkout-dash to work with rebase

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 17, 2013, 17:56 UTC
Message-ID: <7vk3lsei4i.fsf@alter.siamese.dyndns.org>
In-Reply-To: <7v1u81ibjp.fsf@alter.siamese.dyndns.org>

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

Show 26 quoted lines

> In other words, the order I was anticipating to see after the
> discussion (this is different from saying "A series that is not
> ordered like this is unacceptable") was:
>
>>   wt-status: remove unused field in grab_1st_switch_cbdata
>
> This is an unrelated clean-up, and can be done before anything else.
>
>>   t/checkout-last: checkout - doesn't work after rebase
>
> This spells out what we want to happen at the end and marks the
> current breakage.
>
>>   status: do not depend on rebase reflog messages
>
> This compensates for fallouts from the next change.
>
>>   checkout: respect GIT_REFLOG_ACTION
>
> And this is the fix, the most important step.
>
>>   rebase: prepare to write reflog message for checkout
>>   rebase -i: prepare to write reflog message for checkout
>
> And these are icing on the cake, but that cannot be done before
> status is fixed.

I actually tried to reorder the patches and they seem to work OK as expected. And I think it makes sense to order them the way I've been suggesting, so I'll tentatively queue the result of reordering on 'rr/rebase-checkout-reflog' and push it out as a part of 'pu'.

Please check to see if I introduced a new bug while doing so.

Regardless of the ordering, however, I suspect two patches that change the message recorded in reflog in "rebase" need further fixing.

For example, the one in "git reabse" does this:

    GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
    git checkout -q "$onto^0" || die "could not detach HEAD"
    git update-ref ORIG_HEAD $orig_head
    ...
    run_specific_rebase

But the specific rebase, e.g. git-rebase--interactive, does this:

	case $head_name in
	refs/*)
		message="$GIT_REFLOG_ACTION: $head_name onto $onto" &&
		git update-ref -m "$message" $head_name $newhead $orig_head &&
		git symbolic-ref \
		  -m "$GIT_REFLOG_ACTION: returning to $head_name" \
		  HEAD $head_name
		;;
	esac && {

I think the message you added to "git reabse" is only meant for that specific "checkout $onto", but it is set globally. Wouldn't it affect later use, which expected it to be "rebase" and nothing else?

Perhaps something like this on top of the entire series may be sufficient (which will be queued as "SQUASH???" at the tip).

Hint for anybody (not limited to Ram):

There also are other 'git checkout' invocations in git-rebase\*.sh that are not yet covered by these "nicer reflog message" patches, which may want to be fixed up before these two icing-on-cake patches become ready to be merged; see output of

	git grep -C2 'git checkout' -- git-rebase\*.sh

Thanks.

 git-rebase--interactive.sh    | 15 ++++++++++-----
 git-rebase.sh                 |  4 ++--
 t/t3404-rebase-interactive.sh | 15 +++++++++++++++
 3 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a05a6e4..f21ff7c 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -837,9 +837,11 @@ comment_for_reflog start
 
 if test ! -z "$switch_to"
 then
-	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
-	output git checkout "$switch_to" -- ||
-		die "Could not checkout $switch_to"
+	(
+		GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
+		export GIT_REFLOG_ACTION
+		output git checkout "$switch_to" --
+	) || die "Could not checkout $switch_to"
 fi
 
 orig_head=$(git rev-parse --verify HEAD) || die "No HEAD?"
@@ -981,7 +983,10 @@ has_action "$todo" ||
 
 test -d "$rewritten" || test -n "$force_rebase" || skip_unnecessary_picks
 
-GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
-output git checkout $onto || die_abort "could not detach HEAD"
+(
+	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
+	export GIT_REFLOG_ACTION
+	output git checkout $onto
+) || die_abort "could not detach HEAD"
 git update-ref ORIG_HEAD $orig_head
 do_rest
diff --git a/git-rebase.sh b/git-rebase.sh
index 7d55372..2344eef 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -523,8 +523,8 @@ test "$type" = interactive && run_specific_rebase
 # Detach HEAD and reset the tree
 say "$(gettext "First, rewinding head to replay your work on top of it...")"
 
-GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
-git checkout -q "$onto^0" || die "could not detach HEAD"
+GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" \
+	git checkout -q "$onto^0" || die "could not detach HEAD"
 git update-ref ORIG_HEAD $orig_head
 
 # If the $onto is a proper descendant of the tip of the branch, then
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index a58406d..c175ef1 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -934,6 +934,21 @@ test_expect_success 'rebase --edit-todo can be used to modify todo' '
 	test L = $(git cat-file commit HEAD | sed -ne \$p)
 '
 
+test_expect_success 'rebase -i produces readable reflog' '
+	git reset --hard &&
+	git branch -f branch1 H &&
+	git rebase -i --onto I F branch1 &&
+	cat >expect <<-\EOF &&
+	rebase -i (start): checkout I
+	rebase -i (pick): G
+	rebase -i (pick): H
+	rebase -i (finish): returning to refs/heads/branch1
+	EOF
+	tail -n 4 .git/logs/HEAD |
+	sed -e "s/.*	//" >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'rebase -i respects core.commentchar' '
 	git reset --hard &&
 	git checkout E^0 &&