Re: [PATCH 1/2] remote: defer repacking packed-refs when deleting refs

From: Junio C Hamano <gitster@pobox.com>
Date: May 23, 2014, 17:09 UTC
Message-ID: <xmqq38g0l8we.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAEef6WyXGXAdm_DkzNsuLgeFFpQsxvTiuJbK3ypc8-Cz2qD_KQ@mail.gmail.com>

Jens Lindström <jl@opera.com> writes:

> Removing the remote configuration (I assume you mean the
> "remote.<name>" section from .git/config) last in 'remote rm' would be
> a bit better I think.  Especially ...

Yes, that is exactly why I suggested it ;-)

> Doing the repacking first and then run through and delete loose refs
> and ref logs leads to a smaller and nicer patch as well ...

Hmph, that would be a bonus, then. I suggested it primarily as a correctness thing against an interrupted operation.

> One additional change was required in
> builtin/remote.c:remove_branches().  It used to pass in the expected
> SHA-1 of the ref to delete_ref(), which only works if the ref exists.
> If repack_without_refs() is called first, most refs won't exist,...

Why? A ref, before you start pruning or removing a remote, could be in one of these three states.

 (a) only loose refs/remotes/them/frotz exists
 (b) both loose and packed refs/remotes/them/nitfol exist
 (c) only packed refs/remotes/them/xyzzy exists

And then you repack packed-refs file without these three refs. When you do so, you know that you would need to remove frotz and nitfol, and also you know you do not want to call delete_ref for xyzzy, no?

In other words, the problem you are describing in remove_branches(), that it wants to make sure that the ref-to-be-removed still points at the expected object, does not sound like it is doing anything inherently wrong---rather, it sounds like you didn't make necessary changes to the caller to make sure that you do not call delete_ref() on something you know you removed already.

Puzzled....