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

From: Jens Lindström <jl@opera.com>
Date: May 24, 2014, 07:54 UTC
Message-ID: <CAEef6Wxg2QkWDzjuogh5sjjE=HZ9NRUmiY9AhExSXK7H3pAcNA@mail.gmail.com>
In-Reply-To: <xmqq38g0l8we.fsf@gitster.dls.corp.google.com>

On Fri, May 23, 2014 at 7:09 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 25 quoted lines

> Jens Lindström <jl@opera.com> writes:
>> 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....

There is one reason why one would want to call delete_ref() even if the ref itself was already fully deleted by repack_without_refs() (because it was only packed) and that is that delete_ref() also removes the ref log, if there is one. We could refactor the deletion to

  1) repack_without_refs() on all refs
  2) delete_ref() on still existing (loose) refs
  3) delete_ref_log() on all refs

to let us only call delete_ref() on existing refs, and then keep the current value check.

Personally I don't feel that we're solving an important problem here. Making sure not to overwrite a ref that has been updated since we read its value is of course of great value for 'receive-pack' and 'commit' and such, but in this case we're removing a remote and all its remote-tracking branches and configuration. If a remote-tracking branch is updated concurrently, the current value check would fail, and the remote configuration and that one branch would remain. But if the update had happened just an instant earlier, just before we started removing the remote, we would have happily deleted that remote-tracking branch as well, throwing away whatever information the update stored in it.

/ Jens