Re: v2.15.0-rc2 ref deletion bug
On Tue, Oct 24, 2017 at 01:05:00PM +0200, Michael Haggerty wrote: > > I'd expect one of: > > > > 1. We delete "foo" before updating "foo/bar", and we end up with a > > single ref. > > I don't think that this is possible in the general case in a single > transaction. The problem is that the code would need to take locks > > refs/tags/foo.lock > refs/tags/foo/bar.lock > > But the latter couldn't coexist with the loose reference file > > refs/tags/foo > > , which might already exist. Yeah, you're right. I was thinking of the opposite case (where you could create "foo.lock" even though "foo/bar" exists), but this one is impossible with filesystem semantics. > It is only imaginable to do this in a single transaction if you pack and > prune `refs/tags/foo` first, to get the loose reference out of the way, > before executing the transaction. Even then, you would have to beware of > a race where another process writes a loose version of `refs/tags/foo` > between the time that `pack-refs` prunes it and the time that the > transaction obtains the lock again. Yeah, it's probably better to avoid playing games here. Moving to a non-filesystem storage backend would just make the problem go away. -Peff
Re: v2.15.0-rc2 ref deletion bug
On 10/24/2017 01:05 PM, Michael Haggerty wrote: > On 10/24/2017 10:24 AM, Jeff King wrote: >> I found a potentially serious bug in v2.15.0-rc2 (and earlier release >> candidates, too) that we may want to deal with before the release. >> >> If I do: >> [...] >> then at the end we have no refs at all! > > That's a serious bug. I'm looking into it right now. The fix is trivial (see below). But let me add some tests and make sure that there are no similar breakages in the area, then submit a full patch. Michael - refs/files-backend.c - index 29eb5e826f..fc3f2abcc6 100644 @@ -2523,15 +2523,15 @@ static int files_transaction_prepare(struct ref_store *ref_store, */ for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; ret = lock_ref_for_update(refs, update, transaction, head_ref, _refnames, err); if (ret) - break; + goto cleanup; if (update->flags & REF_DELETING && !(update->flags & REF_LOG_ONLY) && !(update->flags & REF_ISPRUNING)) { /* * This reference has to be deleted from * packed-refs if it exists there.
Re: v2.15.0-rc2 ref deletion bug
On 10/24/2017 10:24 AM, Jeff King wrote: > I found a potentially serious bug in v2.15.0-rc2 (and earlier release > candidates, too) that we may want to deal with before the release. > > If I do: > > git init -q repo > cd repo > obj=$(git hash-object -w /dev/null) > git update-ref refs/tags/foo $obj > git update-ref --stdin <<-EOF > delete refs/tags/foo > update refs/tags/foo/bar $obj > EOF > git for-each-ref > > then at the end we have no refs at all! That's a serious bug. I'm looking into it right now. > I'd expect one of: > > 1. We delete "foo" before updating "foo/bar", and we end up with a > single ref. I don't think that this is possible in the general case in a single transaction. The problem is that the code would need to take locks refs/tags/foo.lock refs/tags/foo/bar.lock But the latter couldn't coexist with the loose reference file refs/tags/foo , which might already exist. It is only imaginable to do this in a single transaction if you pack and prune `refs/tags/foo` first, to get the loose reference out of the way, before executing the transaction. Even then, you would have to beware of a race where another process writes a loose version of `refs/tags/foo` between the time that `pack-refs` prunes it and the time that the transaction obtains the lock again. > [...] Michael
v2.15.0-rc2 ref deletion bug
I found a potentially serious bug in v2.15.0-rc2 (and earlier release candidates, too) that we may want to deal with before the release. If I do: git init -q repo cd repo obj=$(git hash-object -w /dev/null) git update-ref refs/tags/foo $obj git update-ref --stdin <<-EOF delete refs/tags/foo update refs/tags/foo/bar $obj EOF git for-each-ref then at the end we have no refs at all! I'd expect one of: 1. We delete "foo" before updating "foo/bar", and we end up with a single ref. 2. We complain that we cannot update "foo/bar" because "foo" still exists. I was hoping for (1). But in earlier releases we did (2). That makes sense because it's safer to do all updates in a transaction before doing any deletes (since if there's a simultaneous prune we'd rather see both refs present for a moment rather than neither). But the v2.15 behavior is just buggy, and may lead to data loss (we silently lose the refs, and then a subsequent prune may lose the objects). This bisects to Michael's dc39e09942 (files_ref_store: use a transaction to update packed refs, 2017-09-08). Curiously, it doesn't happen if you reverse the order of the entries in the transaction (which _shouldn't_ matter, since we try to process it atomically, but obviously it just tickles this bug in a funny way). I haven't figured out if the deletion has to be a prefix of the update to trigger the bug, or if the problem is more widespread. -Peff