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

From: Jens Lindström <jl@opera.com>
Date: May 23, 2014, 10:03 UTC
Message-ID: <CAEef6WyXGXAdm_DkzNsuLgeFFpQsxvTiuJbK3ypc8-Cz2qD_KQ@mail.gmail.com>
In-Reply-To: <xmqqlhtwxkg9.fsf@gitster.dls.corp.google.com>

On Tue, May 20, 2014 at 10:29 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 28 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> A bit safer way to organize might be to first create a list of the
>> refs to be removed in-core, update packed-refs without these refs to
>> be removed, and then finally remove the loose ones, but I haven't
>> thought things through.
>
> Perhaps a removal of remote can go in this order to be resistant
> against an abort-in-the-middle.
>
>  * update packed-refs without the refs that came from the remote.
>
>    - when interrupted before the new temporary file is renamed to
>      the final, it would be a no-op.
>
>    - when interrupted after the rename, only some refs that came
>      from the remote may disappear.
>
>  * remove the loose refs that came from the remote.
>
>  * finally, remove the configuration related to the remote.
>
> This order would let you interrupt "remote rm" without leaving the
> repository in a broken state.  Before the final state, it may appear
> that you have some but not all remote-tracking refs from the remote
> in your repository, but you would not have any ref that point at an
> obsolete object.  Running "remote rm" again, once it finishes, will
> give you the desired result.

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 with the very slow removal of branches an impatient user would likely abort the command if there were many remote-tracking branches, after which rerunning would fail since the remote configuration was already gone, and then there would be no obvious way to get rid of the remaining remote-tracking branches.

Doing the repacking first and then run through and delete loose refs and ref logs leads to a smaller and nicer patch as well; no need to tell delete_ref() to not repack then, since repack_without_refs() will just find that the ref isn't in packed-refs already and do nothing.

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, and delete_ref() would fail. Branch removal from 'remote prune' didn't have this problem, since it called delete_ref() with a NULL SHA-1.

Show 5 quoted lines

> A longer-term goal might be to have ref-transaction infrastructure
> clever enough to coalesce the "repack-without-these-refs" requests
> into one automatically without forcing the callers you are fixing
> care about these things, though.  And such a transaction semantics
> may have to also cover the updating of configuration files.

Yes, some kind of more advanced transaction mechanism would be great, and would likely solve this type of performance issue by design.

/ Jens