Re: [PATCH 0/5] Fixing problem with deleting symrefs

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 16, 2012, 16:54 UTC
Message-ID: <7v4nlutl1l.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1350395094-11404-1-git-send-email-johan@herland.net>

Johan Herland <johan@herland.net> writes:

Show 16 quoted lines

> I see that Rene Scharfe has also worked on the same issue, while I was
> preparing these patches...
>
> On Mon, Oct 15, 2012 at 11:29 AM, Junio C Hamano <jch2355@gmail.com> wrote:
>> Even though update-ref deferences a symref when it updates one to point at a
>> new object, I personally don't think update-ref -d that derefs makes any
>> sense. I'd rather see it error out when given a symref, with or without
>> --no-deref option.
>
> I'm not sure. We have multiple testcases that directly test deleting a ref
> through a symref (e.g. t1400), so supporting this seems like a concious
> decision. Erroring out when given a symref will break the following
> testcases:
>  - t1400 (git update-ref -d)
>  - t3310 & t3311 (git notes merge --abort is broken)
>  - t5505 (git remote set-head --delete and renaming a remote is broken)

"Concious" does not necessarily mean "Sane", though. But this is water under the bridge. Too many people must have started relying on this crazy "feature" since mid 2008, and removing it would break them.

 - "update-ref -d --no-deref SYMREF", even though it is a synonym
   for "symbolic-ref -d SYMREF", makes sense.  Removing it would
   only buy us breakage.

 - "update-ref -d --no-deref SYMREF OLDSHA1" does not make *ANY*
   sense as a request, but again it would not hurt to keep it.

 - "update-ref -d --deref SYMREF [OLDSHA1]" is questionable.  What
   is the use case?  What is the next step after doing such an
   operation, now you have SYMREF that is dangling?

Show 8 quoted lines

>> But even if it did, removing a ref pointed by a symref should really remove
>> it, and forgetting to remove a leftover entry in packed-ref has no excuse
>> bug.
>>
>> I'd say what you observed is a double bug.
>
> Patch #1 - #2 fixes the 2nd bug (removing through a symref should remove
> both loose and packed versions of the ref).

OK.  That is surely needed.