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

From: George Spelvin <linux@horizon.com>
Date: Oct 14, 2012, 07:02 UTC
Message-ID: <20121014070218.16887.qmail@science.horizon.com>
In-Reply-To: <7vlif91wv6.fsf@alter.siamese.dyndns.org>

> 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...