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

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

On 10/24/2017 01:05 PM, Michael Haggerty wrote:

On 10/24/2017 10:24 AM, Jeff King wrote:

quoted

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, &affected_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.

On Tue, Oct 24, 2017 at 01:05:00PM +0200, Michael Haggerty wrote:

quoted

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