git symbolic-ref vs. reflog (vs. rebase)

git symbolic-ref vs. reflog (vs. rebase)

[Csaba Henk]

"git symbolic-ref" is a dangerous command in the sense that it can
change your HEAD position without updating the reflog. Is it
intended behaviour?

I found now a case where this feature causes strange behavior.
It seems to me that either behaviour of git symbolic-ref, or the way
it is used should be changed, but I'm not sure which of these
would be the right solution.

If I'm at a detached HEAD and I do a rebase onto it, from which then
I step back with "git rebase --abort", then I seem to lose references
to the HEAD before rebase, as git reflog won't show it anymore.

This is because of two factors:

- unlike a rebase onto a branch-referred head, we don't get an
  additional reflog entry for moving to the starting position
  of the rebase (ie. we don't actually changes HEAD, nevertheless
  in the branchy case a dummy reflog entry is made for HEAD);

- upon aborting the rebase, a call is done to "git symbolic-ref"
  in the aforementioned unsafe way, ie. it changes HEAD but
  does not update reflog.

The phenomenon is illustrated by the script below. Usage:

- fist arg is the dir where the script will run (should
  not exist at invocation, script creates it).

- optional second arg is a branchname, if you give it, you'll
  see the branchy case, else you get the rebase-onto-detached
  case

Observe the following: 

- in both of branchy and detached cases, one reflog entry for
  the starting HEAD disappears from reflog output, but in the
  branchy case there are two of them before "rebase --abort"
  so you don't lose the reference;

- "rebase --abort" does not change the reflog file, but
  the reflog command's output changes, as it shows the actual HEAD
  as HEAD{0}, regardless of the last log entry.

Csaba

 * * *

bash> cat gtr.sh
#!/bin/bash -e

GIT=${GIT:-git}

if !([ $# -eq 1 ] || [ $# -eq 2 ]) || [ "$1" = -h ]; then
    echo usage: `basename $0` scratchdir [branchname]
fi

if ! [ -z $2 ]; then
    branch=$2
    bropt=-b
fi

mkdir "$1"
cd "$1"

{
$GIT init

echo master > file
$GIT add file
$GIT commit -m Master.

$GIT checkout -b topic
echo topic > file
$GIT commit -am Topic.

$GIT checkout $bropt $branch master~0
echo detached > file
$GIT commit -am 'Rebase base'.
} &>/dev/null

echo
$GIT log --all --graph --decorate --pretty=oneline
echo

set -x

$GIT hash-object .git/logs/HEAD
$GIT rebase HEAD topic || :
$GIT hash-object .git/logs/HEAD
$GIT reflog
# ruby is used to compactify the logfile,
# if you don't have ruby, comment out
# the filtering part.
cat .git/logs/HEAD | ruby -ane 'puts $F.map{|x| x[0..(x =~ /:$/ ? -1 : 7)] }.join(" ")'
$GIT rebase --abort
$GIT hash-object .git/logs/HEAD
$GIT reflog

bash> ./gtr.sh testdir

* 27a1300e35857be6357adaf6759eb8ce0818d37b (topic) Topic.
| * 5d4be82c08bf6effc906a37419220ca428834844 (HEAD) Rebase base.
|/
* 6fa006d134be4d986e1a680cac4eddd61494164a (master) Master.

+ git hash-object .git/logs/HEAD
cd15f363cf16ea3e03f5177b502de8ce3543ff21
+ git rebase HEAD topic
First, rewinding head to replay your work on top of it...
Applying: Topic.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging file
CONFLICT (content): Merge conflict in file
Failed to merge in the changes.
Patch failed at 0001 Topic.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

+ :
+ git hash-object .git/logs/HEAD
cd15f363cf16ea3e03f5177b502de8ce3543ff21
+ git reflog
5d4be82 HEAD@{0}: commit: Rebase base.
6fa006d HEAD@{1}: checkout: moving from topic to master~0
27a1300 HEAD@{2}: commit: Topic.
6fa006d HEAD@{3}: checkout: moving from master to topic
6fa006d HEAD@{4}: commit (initial): Master.
+ cat .git/logs/HEAD
+ ruby -ane 'puts $F.map{|x| x[0..(x =~ /:$/ ? -1 : 7)] }.join(" ")'
00000000 6fa006d1 Csaba Henk <csaba@l 13040883 +0530 commit (initial): Master.
6fa006d1 6fa006d1 Csaba Henk <csaba@l 13040883 +0530 checkout: moving from master to topic
6fa006d1 27a1300e Csaba Henk <csaba@l 13040883 +0530 commit: Topic.
27a1300e 6fa006d1 Csaba Henk <csaba@l 13040883 +0530 checkout: moving from topic to master~0
6fa006d1 5d4be82c Csaba Henk <csaba@l 13040883 +0530 commit: Rebase base.
+ git rebase --abort
HEAD is now at 27a1300 Topic.
+ git hash-object .git/logs/HEAD
cd15f363cf16ea3e03f5177b502de8ce3543ff21
+ git reflog
27a1300 HEAD@{0}: commit: Rebase base.
6fa006d HEAD@{1}: checkout: moving from topic to master~0
27a1300 HEAD@{2}: commit: Topic.
6fa006d HEAD@{3}: checkout: moving from master to topic
6fa006d HEAD@{4}: commit (initial): Master.
bash> rm -rf testdir
bash> ./gtr.sh testdir foobr

* fa0c2b339d1f048281ed7098d8d7d55a12b35154 (HEAD, foobr) Rebase base.
| * a87b990fcd5c7a109611694dcc9523d4f00ae8fe (topic) Topic.
|/
* 1e2d1c12f46105c9329479535780ec230afe7fa0 (master) Master.

+ git hash-object .git/logs/HEAD
734ae3b60eee21ee2b8b51450040389004d56e5d
+ git rebase HEAD topic
First, rewinding head to replay your work on top of it...
Applying: Topic.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging file
CONFLICT (content): Merge conflict in file
Failed to merge in the changes.
Patch failed at 0001 Topic.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

+ :
+ git hash-object .git/logs/HEAD
5acf3d5f543333f549b3f9d295ed2ba91074b43d
+ git reflog
fa0c2b3 HEAD@{0}: checkout: moving from foobr to fa0c2b339d1f048281ed7098d8d7d55a12b35154^0
fa0c2b3 HEAD@{1}: commit: Rebase base.
1e2d1c1 HEAD@{2}: checkout: moving from topic to foobr
a87b990 HEAD@{3}: commit: Topic.
1e2d1c1 HEAD@{4}: checkout: moving from master to topic
1e2d1c1 HEAD@{5}: commit (initial): Master.
+ cat .git/logs/HEAD
+ ruby -ane 'puts $F.map{|x| x[0..(x =~ /:$/ ? -1 : 7)] }.join(" ")'
00000000 1e2d1c12 Csaba Henk <csaba@l 13040887 +0530 commit (initial): Master.
1e2d1c12 1e2d1c12 Csaba Henk <csaba@l 13040887 +0530 checkout: moving from master to topic
1e2d1c12 a87b990f Csaba Henk <csaba@l 13040887 +0530 commit: Topic.
a87b990f 1e2d1c12 Csaba Henk <csaba@l 13040887 +0530 checkout: moving from topic to foobr
1e2d1c12 fa0c2b33 Csaba Henk <csaba@l 13040887 +0530 commit: Rebase base.
fa0c2b33 fa0c2b33 Csaba Henk <csaba@l 13040887 +0530 checkout: moving from foobr to fa0c2b33
+ git rebase --abort
HEAD is now at a87b990 Topic.
+ git hash-object .git/logs/HEAD
5acf3d5f543333f549b3f9d295ed2ba91074b43d
+ git reflog
a87b990 HEAD@{0}: checkout: moving from foobr to fa0c2b339d1f048281ed7098d8d7d55a12b35154^0
fa0c2b3 HEAD@{1}: commit: Rebase base.
1e2d1c1 HEAD@{2}: checkout: moving from topic to foobr
a87b990 HEAD@{3}: commit: Topic.
1e2d1c1 HEAD@{4}: checkout: moving from master to topic
1e2d1c1 HEAD@{5}: commit (initial): Master.

Re: git symbolic-ref vs. reflog (vs. rebase)

[Junio C Hamano]

Csaba Henk <csaba@lowlife.hu> writes:

> "git symbolic-ref" is a dangerous command in the sense that it can
> change your HEAD position without updating the reflog. Is it
> intended behaviour?

Yes, it is.  But you can choose to do:

	$ git branch side
	$ git symoblic-ref -m "move to side" HEAD refs/heads/side
        $ git log --oneline -g HEAD@{0}
        05ddb9b HEAD@{0}: move to side
	e69de29 HEAD@{1}: commit (initial): first commit

if you wanted to.

Re: git symbolic-ref vs. reflog (vs. rebase)

[Csaba Henk]

On Fri, Apr 29, 2011 at 9:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Csaba Henk <csaba@lowlife.hu> writes:
>
>> "git symbolic-ref" is a dangerous command in the sense that it can
>> change your HEAD position without updating the reflog. Is it
>> intended behaviour?
>
> Yes, it is.  But you can choose to do:
>
>        $ git branch side
>        $ git symoblic-ref -m "move to side" HEAD refs/heads/side
>        $ git log --oneline -g HEAD@{0}
>        05ddb9b HEAD@{0}: move to side
>        e69de29 HEAD@{1}: commit (initial): first commit
>
> if you wanted to.

So do you think the following patch would be the correct fix for the
rebase issue:

diff --git a/git-rebase.sh b/git-rebase.sh
index cbb0ea9..10c1727 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -284,7 +284,7 @@ do
                head_name="$(cat "$dotest"/head-name)" &&
                case "$head_name" in
                refs/*)
-                       git symbolic-ref HEAD $head_name ||
+                       git symbolic-ref -m "rebase: aborting" HEAD
$head_name ||
                        die "Could not move back to $head_name"
                        ;;
                esac


?

Csaba

Re: git symbolic-ref vs. reflog (vs. rebase)

[Jeff King]

On Fri, Apr 29, 2011 at 09:19:44AM -0700, Junio C Hamano wrote:

> Csaba Henk <csaba@lowlife.hu> writes:
> 
> > "git symbolic-ref" is a dangerous command in the sense that it can
> > change your HEAD position without updating the reflog. Is it
> > intended behaviour?
> 
> Yes, it is.  But you can choose to do:
> 
> 	$ git branch side
> 	$ git symoblic-ref -m "move to side" HEAD refs/heads/side
>         $ git log --oneline -g HEAD@{0}
>         05ddb9b HEAD@{0}: move to side
> 	e69de29 HEAD@{1}: commit (initial): first commit
> 
> if you wanted to.

I think every caller should be using "-m" these days.  I know we can't
_require_ it for historical reasons. But shouldn't symbolic-ref always
write a reflog entry? Even something like "we changed and I can't tell
you why" to cover older scripts that call symbolic-ref?

-Peff

Re: git symbolic-ref vs. reflog (vs. rebase)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think every caller should be using "-m" these days.  I know we can't
> _require_ it for historical reasons. But shouldn't symbolic-ref always
> write a reflog entry? Even something like "we changed and I can't tell
> you why" to cover older scripts that call symbolic-ref?

I think the particular instance Csaba saw in rebase may want to pass the
reason why it flipped the HEAD.

Flipping HEAD temporarily to another ref to do something, only to flip it
back before giving the control back to the user, might be something a
script may want to have a choice of not logging, so I am mildly negative
on changing the command to unconditionally log empty entry without being
told.

"update-ref" seems to write an empty entry even when not given an "-m"
option, and we can view it as robbing a similar choice from the scripts.
We might want to fix it.  I dunno.

Re: git symbolic-ref vs. reflog (vs. rebase)

[Jeff King]

On Fri, Apr 29, 2011 at 04:00:11PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I think every caller should be using "-m" these days.  I know we can't
> > _require_ it for historical reasons. But shouldn't symbolic-ref always
> > write a reflog entry? Even something like "we changed and I can't tell
> > you why" to cover older scripts that call symbolic-ref?
> 
> I think the particular instance Csaba saw in rebase may want to pass the
> reason why it flipped the HEAD.

Oh, definitely. I was thinking more for external scripts that we can't
fix.

> Flipping HEAD temporarily to another ref to do something, only to flip it
> back before giving the control back to the user, might be something a
> script may want to have a choice of not logging, so I am mildly negative
> on changing the command to unconditionally log empty entry without being
> told.

Yeah, that is a legitimate use case. But I suspect many scripts were
simply never updated after reflogs were introduced (or their authors)
were lazy, so they flip it once without a reflog, and then the next
well-behaved writer who comes along ends up writing a reflog entry that
shows a hole.

So it is a question of whether helping probably-broken users is worth
shutting off a legitimate use case. I also wonder how valuable that use
case is. As a user, I think I'd rather see _everything_ in the reflog,
even if the script-writer doesn't think it's important.

I don't know that it matters much in any case. We haven't had any bug
reports either way; and given the current behavior, one can always yell
at the script author to properly reflog.

> "update-ref" seems to write an empty entry even when not given an "-m"
> option, and we can view it as robbing a similar choice from the scripts.
> We might want to fix it.  I dunno.

I'm inclined to wait until somebody actually wants it. In the meantime
it helps users of old and/or broken scripts by providing an additional
checkpoint of state that might otherwise be missing.

-Peff

Re: git symbolic-ref vs. reflog (vs. rebase)

[Jeff King]

On Fri, Apr 29, 2011 at 10:51:31PM +0530, Csaba Henk wrote:

> > Yes, it is.  But you can choose to do:
> >
> >        $ git branch side
> >        $ git symoblic-ref -m "move to side" HEAD refs/heads/side
> >        $ git log --oneline -g HEAD@{0}
> >        05ddb9b HEAD@{0}: move to side
> >        e69de29 HEAD@{1}: commit (initial): first commit
> >
> > if you wanted to.
> 
> So do you think the following patch would be the correct fix for the
> rebase issue:
> 
> diff --git a/git-rebase.sh b/git-rebase.sh
> index cbb0ea9..10c1727 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -284,7 +284,7 @@ do
>                 head_name="$(cat "$dotest"/head-name)" &&
>                 case "$head_name" in
>                 refs/*)
> -                       git symbolic-ref HEAD $head_name ||
> +                       git symbolic-ref -m "rebase: aborting" HEAD
> $head_name ||
>                         die "Could not move back to $head_name"
>                         ;;
>                 esac

I count 2 uses of symbolic-ref without reflogs in git-rebase, one more
in git-rebase--interactive, and one in git-cvsexportcommit. Should all
be fixed to write a reflog entry?

The one in git-rebase.sh:move_to_original_branch even takes care to
write a reflog message when updating the ref. So the branch reflog has
an entry for the rebase being finished, but the HEAD reflog gets
nothing. Presumably it's OK because we're moving from the detached
version of some commit to seeing it on the branch.

What does a user expect? Personally I think it would be more readable to
see the extra entry. For example, this is from my current reflog:

  $ git log -g --oneline
  ...
  50d3062 HEAD@{23}: checkout: moving from jk/merge-one-file to 50d3062ab2cea4e999b8f3bafd211ff348bca600
  660fe6b HEAD@{24}: commit (amend): merge-one-file: fix broken merges with GIT_WORK_TREE
  ebad5e6 HEAD@{25}: commit (amend): merge-one-file: fix broken merges with GIT_WORK_TREE
  306f37e HEAD@{26}: rebase -i (pick): merge-one-file: fix broken merges with GIT_WORK_TREE
  0681894 HEAD@{27}: commit (amend): add tests for merge-index / merge-one-file
  77d4cba HEAD@{28}: cherry-pick
  50d3062 HEAD@{29}: checkout: moving from jk/merge-one-file to 50d3062ab2cea4e999b8f3bafd211ff348bca600
  ...

So reading in reverse order, I see that I started a rebase, which moved
us to a detached HEAD, then picked and amended some commits. And then
nothing, and suddenly I'm moving back from a branch to another detached
HEAD (I rebased again). It would be easier to follow if HEAD{23} were
actually:

  660fe6b HEAD{23}: rebase finished: returning to jk/merge-one-file

This feels a little nit-picky. I have to admit I don't try to follow
these sorts of reflogs all the time, so it's not like this is a pressing
issue. But when it comes to a user deciphering a broken state in their
repository, or trying to figure out which actions they took in a rebase
last week, the more entries we can give them, the better.

-Peff

Re: git symbolic-ref vs. reflog (vs. rebase)

[Csaba Henk]

On Sat, Apr 30, 2011 at 4:30 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> I think every caller should be using "-m" these days.  I know we can't
>> _require_ it for historical reasons. But shouldn't symbolic-ref always
>> write a reflog entry? Even something like "we changed and I can't tell
>> you why" to cover older scripts that call symbolic-ref?
>
> I think the particular instance Csaba saw in rebase may want to pass the
> reason why it flipped the HEAD.
>
> Flipping HEAD temporarily to another ref to do something, only to flip it
> back before giving the control back to the user, might be something a
> script may want to have a choice of not logging, so I am mildly negative
> on changing the command to unconditionally log empty entry without being
> told.
>
> "update-ref" seems to write an empty entry even when not given an "-m"
> option, and we can view it as robbing a similar choice from the scripts.
> We might want to fix it.  I dunno.

What if symbolic-ref and/or update-ref were changed so that the default
invocation would add a reflog entry with a default (empty?) note, however,
also provide an option with which suppressing the reflog can be requested?

Csaba

Re: git symbolic-ref vs. reflog (vs. rebase)

[Csaba Henk]

On Sat, Apr 30, 2011 at 9:43 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Apr 29, 2011 at 10:51:31PM +0530, Csaba Henk wrote:
>
>> > Yes, it is.  But you can choose to do:
>> >
>> >        $ git branch side
>> >        $ git symoblic-ref -m "move to side" HEAD refs/heads/side
>> >        $ git log --oneline -g HEAD@{0}
>> >        05ddb9b HEAD@{0}: move to side
>> >        e69de29 HEAD@{1}: commit (initial): first commit
>> >
>> > if you wanted to.
>>
>> So do you think the following patch would be the correct fix for the
>> rebase issue:
>>
>> diff --git a/git-rebase.sh b/git-rebase.sh
>> index cbb0ea9..10c1727 100755
>> --- a/git-rebase.sh
>> +++ b/git-rebase.sh
>> @@ -284,7 +284,7 @@ do
>>                 head_name="$(cat "$dotest"/head-name)" &&
>>                 case "$head_name" in
>>                 refs/*)
>> -                       git symbolic-ref HEAD $head_name ||
>> +                       git symbolic-ref -m "rebase: aborting" HEAD
>> $head_name ||
>>                         die "Could not move back to $head_name"
>>                         ;;
>>                 esac
>
> I count 2 uses of symbolic-ref without reflogs in git-rebase, one more
> in git-rebase--interactive, and one in git-cvsexportcommit. Should all
> be fixed to write a reflog entry?

Yeah, that's why I didn't set the above patch as PATCH -- I lack the insight
to rebase / plumbing in general to be able to propose a change with the right
impact.

Csaba

[PATCH 1/2] rebase: create HEAD reflog entry when aborting

[Jeff King]

From: Csaba Henk <csaba@lowlife.hu>

When we abort a rebase, we return to the original value of
HEAD. Failing to write a reflog entry means we create a
gap in the reflog (which can cause "git show
HEAD@{5.minutes.ago}" to issue a warning). Plus having the
extra entry makes the reflog easier to follow for a human.

Signed-off-by: Jeff King <peff@peff.net>
---
This is the one that actually gave you trouble. It's different than
other cases (dealt with in 2/2) in that it actually creates a gap in the
reflog. So I think fixing it is the right thing to do, and your original
patch is fine.

 git-rebase.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 7a54bfc..57cbe49 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -332,7 +332,7 @@ abort)
 	read_basic_state
 	case "$head_name" in
 	refs/*)
-		git symbolic-ref HEAD $head_name ||
+		git symbolic-ref -m "rebase: aborting" HEAD $head_name ||
 		die "Could not move back to $head_name"
 		;;
 	esac
-- 
1.7.5.3.12.g99e25

[PATCH 2/2] rebase: write a reflog entry when finishing

[Jeff King]

When we finish a rebase, our detached HEAD is at the final
result. We update the original branch ref with this result,
and then point the HEAD symbolic ref at the updated branch.
We write a reflog for the branch update, but not for the
update of HEAD.

Because we're already at the final result on the detached
HEAD, moving to the branch actually doesn't change our
commit sha1 at all. So in that sense, a reflog entry would
be pointless.

However, humans do read reflogs, and an entry saying "rebase
finished: returning to refs/heads/master" can be helpful in
understanding what is going on.

Signed-off-by: Jeff King <peff@peff.net>
---
See the minor test update for an example of how this could break people
or scripts who assume they know what's at which point in the reflog. In
general I think all bets are off with HEAD, because commands do their
work on a detached HEAD, and then provide a nice single-entry result in
the branch reflog (which is why I switched the test to use the more
stable master@{1}).

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

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 41ba96a..65690af 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -510,7 +510,9 @@ do_next () {
 	refs/*)
 		message="$GIT_REFLOG_ACTION: $head_name onto $shortonto" &&
 		git update-ref -m "$message" $head_name $newhead $orig_head &&
-		git symbolic-ref HEAD $head_name
+		git symbolic-ref \
+		  -m "$GIT_REFLOG_ACTION: returning to $head_name" \
+		  HEAD $head_name
 		;;
 	esac && {
 		test ! -f "$state_dir"/verbose ||
diff --git a/git-rebase.sh b/git-rebase.sh
index 57cbe49..d7855ea 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -153,7 +153,9 @@ move_to_original_branch () {
 		message="rebase finished: $head_name onto $onto"
 		git update-ref -m "$message" \
 			$head_name $(git rev-parse HEAD) $orig_head &&
-		git symbolic-ref HEAD $head_name ||
+		git symbolic-ref \
+			-m "rebase finished: returning to $head_name" \
+			HEAD $head_name ||
 		die "Could not move back to $head_name"
 		;;
 	esac
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 7d8147b..47c8371 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -317,7 +317,7 @@ test_expect_success '--continue tries to commit' '
 '
 
 test_expect_success 'verbose flag is heeded, even after --continue' '
-	git reset --hard HEAD@{1} &&
+	git reset --hard master@{1} &&
 	test_tick &&
 	test_must_fail git rebase -v -i --onto new-branch1 HEAD^ &&
 	echo resolved > file1 &&
-- 
1.7.5.3.12.g99e25