git reflog delete HEAD@{1} HEAD@{2} caught me by surprise...

git reflog delete HEAD@{1} HEAD@{2} caught me by surprise...

[George Spelvin]

Try the following commands in an empty directory:
(I'm using the bash extention for {1..5}.)

git init
for i in {1..5}; do git commit -m "Commit $i" --allow-empty; done
git reflog
	bc93e06 HEAD@{0}: commit: Commit 5
	e14f92d HEAD@{1}: commit: Commit 4
	ac64d8e HEAD@{2}: commit: Commit 3
	287602b HEAD@{3}: commit: Commit 2
	183a378 HEAD@{4}: commit (initial): Commit 1
git reflog delete HEAD@{{1,2}}
git reflog
	bc93e06 HEAD@{0}: commit: Commit 5
	e14f92d HEAD@{1}: commit: Commit 3
	287602b HEAD@{2}: commit (initial): Commit 1

Er...I meant to delete Commit 3 from the reflog, not Commit 2.

In hindsight, it's obvious how this could happen, but it definitely
took me by surprise when I was trying to delete the reference to
a large and messy merge (that I didn't want and bloated my repository
size) from the reflog.

If this is, in fact, Working As Designed, could I request a paragraph
in the man page clarifying it?  something like:


To delete single entries from the reflog, use the subcommand "delete"
and specify the _exact_ entry (e.g. "`git reflog delete master@{2}`").

You may delete multiple reflog entries with one delete command,
_however_ they are processed one at a time, so earlier deletes will cause
renumbering that will affect later ones.  To delete reflog entries @{2}
and @{3}, the command would be either "`git reflog delete @{3} @{2}`",
or "`git reflog delete @{2} @{2}`".

Re: git reflog delete HEAD@{1} HEAD@{2} caught me by surprise...

[Junio C Hamano]

"George Spelvin" <linux@horizon.com> writes:

> Try the following commands in an empty directory:
> (I'm using the bash extention for {1..5}.)
>
> git init
> for i in {1..5}; do git commit -m "Commit $i" --allow-empty; done
> git reflog
> 	bc93e06 HEAD@{0}: commit: Commit 5
> 	e14f92d HEAD@{1}: commit: Commit 4
> 	ac64d8e HEAD@{2}: commit: Commit 3
> 	287602b HEAD@{3}: commit: Commit 2
> 	183a378 HEAD@{4}: commit (initial): Commit 1
> git reflog delete HEAD@{{1,2}}
> git reflog
> 	bc93e06 HEAD@{0}: commit: Commit 5
> 	e14f92d HEAD@{1}: commit: Commit 3
> 	287602b HEAD@{2}: commit (initial): Commit 1
>
> Er...I meant to delete Commit 3 from the reflog, not Commit 2.

Yeah, I've never tried to delete more than one with one command,
but I am not surprised a naaïve implementation removes the first,
and then removes the second in the resulting list.

> To delete single entries from the reflog, use the subcommand "delete"
> and specify the _exact_ entry (e.g. "`git reflog delete master@{2}`").
>
> You may delete multiple reflog entries with one delete command,
> _however_ they are processed one at a time, so earlier deletes will cause
> renumbering that will affect later ones.  To delete reflog entries @{2}
> and @{3}, the command would be either "`git reflog delete @{3} @{2}`",
> or "`git reflog delete @{2} @{2}`".

I would actually call that behaviour a bug.  Perhaps it should grab
all the command line arguments first, group them per the ref the
reflog entries are based on, and expire _all_ reflog entries from
the same reflog in one go?

Until that happens, it may make sense to error it out when more than
one entries are given from the command line, at least for the same
ref.

Re: git reflog delete HEAD@{1} HEAD@{2} caught me by surprise...

[George Spelvin]

> I would actually call that behaviour a bug.

Well, yes, that was my inclination, too.  But writing documentation was
easier than writing a code patch.  :-)

Even when it is fixed, a comment about when it was fixed and what the
buggy version did should live in the BUGS section for a while, to warn
people writing portable scripts.

> Perhaps it should grab
> all the command line arguments first, group them per the ref the
> reflog entries are based on, and expire _all_ reflog entries from
> the same reflog in one go?

Two other options are to sort them in decreasing entry order (which you
could do either per-reflog, or simply globally), or to remember previous
deletions so you can adjust the numbers of later ones.

One tricky point is whether it's possible for a reflog to have two names,
via a symlink or something.  That definitely complicates collision
detection.

> Until that happens, it may make sense to error it out when more than
> one entries are given from the command line, at least for the same
> ref.

Detecting this seems like half the implementation work of fixing it,
so I'm not sure it's worth bothering.


Looking at the code (builtin/reflog.c), I notice that expire_reflog()
takes a lock on the ref, but the previous count_reflog_ent code doesn't,
so things aren't necessarily race-proof.  I haven't figured out if the
race is a problem (i.e. does expire_reflog do something nasty if the
struct cmd_reflog_expire_cb holds stale data?), but I noticed...