Re: [PATCH] Remove branch by putting a null sha1 into the ref file.

From: Christian Couder <chriscool@tuxfamily.org>
Date: Sep 24, 2006, 04:45 UTC
Message-ID: <200609240645.54467.chriscool@tuxfamily.org>
In-Reply-To: <7veju2nthl.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano wrote:

Show 8 quoted lines

> Christian Couder <chriscool@tuxfamily.org> writes:
> > You are right, so what about moving ".git/refs/heads/frotz"
> > to ".git/deleted-refs/heads/frotz.ref"
> > or ".git/deleted-refs/heads/frotz~ref" (because "~" is forbidden in ref
> > names).
>
> But wouldn't it bring up the issue of locking among deleters,
> updaters/creators, and traversers?

Yes, it will get worse.

Show 7 quoted lines

> If we choose to use packed-refs.lock as the "set of all refs"
> lock, the whole sequence would become something like this.  Note
> that this tries to make readers lockless but I am sure there are
> nasty race condition issues.  I am not sure what we would want
> to do about them.
>
> = Looking up a ref $frotz.

We could acquire .git/$frotz.lock here if we want to be sure that nothing will happen to it while we read it, but anyway something could happen to it just after we read it and before we use it. So the right thing to do is perhaps to let the caller acquire the lock if it needs to. We should just check in the other cases below that nothing can happen to the ref if someone acquired .git/$frotz.lock.

Show 8 quoted lines

>  - check if .git/$frotz exists, and use it if it does.
>
>  - check if .git/deleted-refs/$frotz~ref exists, and return "no
>    such ref" if it does.
>
>  - find $frotz in .git/packed-refs.
>
> = Looping over refs.

Same as above. Depending on what the caller wants to do, it could acquire .git/packed-refs.lock or some .git/$frotz.lock to make sure nothing happens to all or only some refs.

Show 24 quoted lines

>  - grab all .git/refs/ and subtract all .git/deleted-refs/
>
>  - walk .git/packed-refs and the result from the above in
>    parallel as in the current code.
>
> = Storing a new value in ref $frotz.
>
>  - acquire .git/packed-refs.lock
>
>  - lock .git/$frotz.lock.
>
>  - write into .git/$frotz.lock.
>
>  - create or update .git/logs/$frotz as needed.
>
>  - if .git/deleted-refs/$frotz~ref exists, unlink it.
>
>  - rename .git/$frotz.lock to .git/$frotz to unlock it.
>
>  - unlink .git/packed-refs.lock
>
> = Deleting a ref $frotz.
>
>  - acquire .git/packed-refs.lock

- acquire .git/$frotz.lock seems needed too

>  - look up $frotz; if it does not exist either barf or return
>    silent (haven't thought it through yet).
>
>  - create .git/deleted-refs/$frotz~ref

or move .git/$frotz to .git/deleted-refs/$frotz~ref if .git/$frotz exists

>  - remove .git/logs/$frotz

- unlink .git/$frotz.lock

Show 13 quoted lines

>  - unlink .git/packed-refs.lock
>
> = Packing refs, with optional pruning.
>
>  - lock .git/packed-refs.lock
>
>  - loop over refs:
>    - write it out to .git/packed-refs.lock unless a symref.
>    - if it is a loose one (not a symref), remember it for pruning.
>
>  - if pruning:
>    - remove the entire .git/deleted-refs/ hierarchy
>    - remove the remembered ones

Perhaps we should acquire .git/$frotz.lock for each $frotz that we remove when pruning.

>  - rename .git/packed-refs.lock to .git/packed-refs

Thanks, Christian.