[PATCH 00/10] Implement transactions for the packed ref store

This should be the second-to-last patch series in the quest for
mmapped packed-refs.

Before this patch series, there is still more coupling than necessary
between `files_ref_store` and `packed_ref_store`:

* `files_ref_store` adds packed references (e.g., during `git clone`
  and `git pack-refs`) by inserting them into the `packed_ref_store`'s
  internal `ref_cache`, then calling `commit_packed_refs()`. But
  `files_ref_store` shouldn't even *know* that `packed_ref_store` uses
  a `ref_cache`, let alone muck about in it.

* `files_ref_store` deletes packed references by calling
  `repack_without_refs()`.

Instead, implement reference transactions and `delete_refs()` for
`packed_ref_store`, and change `files_ref_store` to use these standard
methods rather than internal `packed_ref_store` methods.

This patch series finishes up the encapsulation of `packed_ref_store`.
At the end of the series, the outside world only interacts with it via
the refs API plus a couple of locking-related functions. That will
make it easy for the next patch series to change the internal workings
of `packed_ref_store` without affecting `files_ref_store`.

Along the way, we fix some longstanding problems with reference
updates. Quoting from patch [08/10]:

    First, the old code didn't obtain the `packed-refs` lock until
    `files_transaction_finish()`. This means that a failure to acquire the
    `packed-refs` lock (e.g., due to contention with another process)
    wasn't detected until it was too late (problems like this are supposed
    to be detected in the "prepare" phase). The new code acquires the
    `packed-refs` lock in `files_transaction_prepare()`, the same stage of
    the processing when the loose reference locks are being acquired,
    removing another reason why the "prepare" phase might succeed and the
    "finish" phase might nevertheless fail.

    Second, the old code deleted the loose version of a reference before
    deleting any packed version of the same reference. This left a moment
    when another process might think that the packed version of the
    reference is current, which is incorrect. (Even worse, the packed
    version of the reference can be arbitrarily old, and might even point
    at an object that has since been garbage-collected.)

    Third, if a reference deletion fails to acquire the `packed-refs` lock
    altogether, then the old code might leave the repository in the
    incorrect state (possibly corrupt) described in the previous
    paragraph.

    Now we activate the new "packed-refs" file (sans any references that
    are being deleted) *before* deleting the corresponding loose
    references. But we hold the "packed-refs" lock until after the loose
    references have been finalized, thus preventing a simultaneous
    "pack-refs" process from packing the loose version of the reference in
    the time gap, which would otherwise defeat our attempt to delete it.

This patch series is also available as branch
`packed-ref-transactions` in my fork on GitHub [1]. A draft of the
patch series to change `packed_ref_store` to use mmap and get rid of
its `ref_cache` is available as branch `mmap-packed-refs` from the
same source.

Michael

[1] https://github.com/mhagger/git

Michael Haggerty (10):
  packed-backend: don't adjust the reference count on lock/unlock
  struct ref_transaction: add a place for backends to store data
  packed_ref_store: implement reference transactions
  packed_delete_refs(): implement method
  files_pack_refs(): use a reference transaction to write packed refs
  files_initial_transaction_commit(): use a transaction for packed refs
  t1404: demonstrate two problems with reference transactions
  files_ref_store: use a transaction to update packed refs
  packed-backend: rip out some now-unused code
  files_transaction_finish(): delete reflogs before references

 refs/files-backend.c         | 200 ++++++++++++++-----
 refs/packed-backend.c        | 457 +++++++++++++++++++++++++++++--------------
 refs/packed-backend.h        |  17 +-
 refs/refs-internal.h         |   1 +
 t/t1404-update-ref-errors.sh |  71 +++++++
 5 files changed, 534 insertions(+), 212 deletions(-)

-- 
2.14.1

Now that the packed reference store supports transactions, we can use
a transaction to write the packed versions of references that we want
to pack. This decreases the coupling between `files_ref_store` and
`packed_ref_store`.

Signed-off-by: Michael Haggerty <redacted>
---
 refs/files-backend.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2c78f63494..0fc44eaca5 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c

@@ -1100,9 +1100,14 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 	int ok;
 	struct ref_to_prune *refs_to_prune = NULL;
 	struct strbuf err = STRBUF_INIT;
+	struct ref_transaction *transaction;
 
 	packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR, &err);
 
+	transaction = ref_store_transaction_begin(refs->packed_ref_store, &err);
+	if (!transaction)
+		return -1;
+
 	iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, 0);
 	while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
 		/*

@@ -1115,12 +1120,14 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 			continue;
 
 		/*
-		 * Create an entry in the packed-refs cache equivalent
-		 * to the one from the loose ref cache, except that
-		 * we don't copy the peeled status, because we want it
-		 * to be re-peeled.
+		 * Add a reference creation for this reference to the
+		 * packed-refs transaction:
 		 */
-		add_packed_ref(refs->packed_ref_store, iter->refname, iter->oid);
+		if (ref_transaction_update(transaction, iter->refname,
+					   iter->oid->hash, NULL,
+					   REF_NODEREF, NULL, &err))
+			die("failure preparing to create packed reference %s: %s",
+			    iter->refname, err.buf);
 
 		/* Schedule the loose reference for pruning if requested. */
 		if ((flags & PACK_REFS_PRUNE)) {

@@ -1134,8 +1141,11 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 	if (ok != ITER_DONE)
 		die("error while iterating over references");
 
-	if (commit_packed_refs(refs->packed_ref_store, &err))
-		die("unable to overwrite old ref-pack file: %s", err.buf);
+	if (ref_transaction_commit(transaction, &err))
+		die("unable to write new packed-refs: %s", err.buf);
+
+	ref_transaction_free(transaction);
+
 	packed_refs_unlock(refs->packed_ref_store);
 
 	prune_refs(refs, refs_to_prune);

-- 
2.14.1

Implement `packed_delete_refs()` using a reference transaction. This
means that `files_delete_refs()` can use `refs_delete_refs()` instead
of `repack_without_refs()` to delete any packed references, decreasing
the coupling between the classes.

Signed-off-by: Michael Haggerty <redacted>
---
 refs/files-backend.c  |  2 +-
 refs/packed-backend.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index fccbc24ac4..2c78f63494 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c

@@ -1157,7 +1157,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,
 	if (packed_refs_lock(refs->packed_ref_store, 0, &err))
 		goto error;
 
-	if (repack_without_refs(refs->packed_ref_store, refnames, &err)) {
+	if (refs_delete_refs(refs->packed_ref_store, msg, refnames, flags)) {
 		packed_refs_unlock(refs->packed_ref_store);
 		goto error;
 	}

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index d19b3bfba5..83a088118f 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c

@@ -1082,7 +1082,50 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store,
 static int packed_delete_refs(struct ref_store *ref_store, const char *msg,
 			     struct string_list *refnames, unsigned int flags)
 {
-	die("BUG: not implemented yet");
+	struct packed_ref_store *refs =
+		packed_downcast(ref_store, REF_STORE_WRITE, "delete_refs");
+	struct strbuf err = STRBUF_INIT;
+	struct ref_transaction *transaction;
+	struct string_list_item *item;
+	int ret;
+
+	(void)refs; /* We need the check above, but don't use the variable */
+
+	if (!refnames->nr)
+		return 0;
+
+	/*
+	 * Since we don't check the references' old_oids, the
+	 * individual updates can't fail, so we can pack all of the
+	 * updates into a single transaction.
+	 */
+
+	transaction = ref_store_transaction_begin(ref_store, &err);
+	if (!transaction)
+		return -1;
+
+	for_each_string_list_item(item, refnames) {
+		if (ref_transaction_delete(transaction, item->string, NULL,
+					   flags, msg, &err)) {
+			warning(_("could not delete reference %s: %s"),
+				item->string, err.buf);
+			strbuf_reset(&err);
+		}
+	}
+
+	ret = ref_transaction_commit(transaction, &err);
+
+	if (ret) {
+		if (refnames->nr == 1)
+			error(_("could not delete reference %s: %s"),
+			      refnames->items[0].string, err.buf);
+		else
+			error(_("could not delete references: %s"), err.buf);
+	}
+
+	ref_transaction_free(transaction);
+	strbuf_release(&err);
+	return ret;
 }
 
 static int packed_pack_refs(struct ref_store *ref_store, unsigned int flags)

-- 
2.14.1

On 08/29, Michael Haggerty wrote:

Can't say I've seen a line like this before, Is the intent to just mark
that this variable won't be used like you mention in the comment?

+
+	if (!refnames->nr)
+		return 0;
+
+	/*
+	 * Since we don't check the references' old_oids, the
+	 * individual updates can't fail, so we can pack all of the
+	 * updates into a single transaction.
+	 */
+
+	transaction = ref_store_transaction_begin(ref_store, &err);
+	if (!transaction)
+		return -1;
+
+	for_each_string_list_item(item, refnames) {
+		if (ref_transaction_delete(transaction, item->string, NULL,
+					   flags, msg, &err)) {
+			warning(_("could not delete reference %s: %s"),
+				item->string, err.buf);
+			strbuf_reset(&err);
+		}
+	}
+
+	ret = ref_transaction_commit(transaction, &err);
+
+	if (ret) {
+		if (refnames->nr == 1)
+			error(_("could not delete reference %s: %s"),
+			      refnames->items[0].string, err.buf);
+		else
+			error(_("could not delete references: %s"), err.buf);
+	}
+
+	ref_transaction_free(transaction);
+	strbuf_release(&err);
+	return ret;
 }
 
 static int packed_pack_refs(struct ref_store *ref_store, unsigned int flags)
-- 
2.14.1

-- 
Brandon Williams

On 08/29/2017 08:07 PM, Brandon Williams wrote:

On 08/29, Michael Haggerty wrote:

quoted

[...]

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index d19b3bfba5..83a088118f 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c

@@ -1082,7 +1082,50 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store,
 static int packed_delete_refs(struct ref_store *ref_store, const char *msg,
 			     struct string_list *refnames, unsigned int flags)
 {
-	die("BUG: not implemented yet");
+	struct packed_ref_store *refs =
+		packed_downcast(ref_store, REF_STORE_WRITE, "delete_refs");
+	struct strbuf err = STRBUF_INIT;
+	struct ref_transaction *transaction;
+	struct string_list_item *item;
+	int ret;
+
+	(void)refs; /* We need the check above, but don't use the variable */

Can't say I've seen a line like this before, Is the intent to just mark
that this variable won't be used like you mention in the comment?

The `(void)refs;` part tells the compiler not to complain about a
variable that is defined but never used.

So why define the variable in the first place? I could instead do

    (void)packed_downcast(ref_store, REF_STORE_WRITE, "delete_refs");

My reason (which people might disagree with) is that I like to keep this
boilerplate consistent from method to method. To switch to the second
variant, not only would the line look different, but it would also have
to be moved down several lines, past the other variables' declarations,
to avoid the dreaded "ISO C90 forbids mixed declarations and code".

I'd prefer to be able to mix declarations and code, but the project's
style prohibits it.

Michael

Used a `packed_ref_store` transaction in the implementation of
`files_initial_transaction_commit()` rather than using internal
features of the packed ref store. This further decouples
`files_ref_store` from `packed_ref_store`.

Signed-off-by: Michael Haggerty <redacted>
---
 refs/files-backend.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0fc44eaca5..29c7c78602 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c

@@ -2663,6 +2663,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
 	size_t i;
 	int ret = 0;
 	struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
+	struct ref_transaction *packed_transaction = NULL;
 
 	assert(err);
 

@@ -2695,6 +2696,12 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
 				 &affected_refnames))
 		die("BUG: initial ref transaction called with existing refs");
 
+	packed_transaction = ref_store_transaction_begin(refs->packed_ref_store, err);
+	if (!packed_transaction) {
+		ret = TRANSACTION_GENERIC_ERROR;
+		goto cleanup;
+	}
+
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = transaction->updates[i];
 

@@ -2707,6 +2714,15 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
 			ret = TRANSACTION_NAME_CONFLICT;
 			goto cleanup;
 		}
+
+		/*
+		 * Add a reference creation for this reference to the
+		 * packed-refs transaction:
+		 */
+		ref_transaction_add_update(packed_transaction, update->refname,
+					   update->flags & ~REF_HAVE_OLD,
+					   update->new_oid.hash, update->old_oid.hash,
+					   NULL);
 	}
 
 	if (packed_refs_lock(refs->packed_ref_store, 0, err)) {

@@ -2714,21 +2730,14 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
 		goto cleanup;
 	}
 
-	for (i = 0; i < transaction->nr; i++) {
-		struct ref_update *update = transaction->updates[i];
-
-		if ((update->flags & REF_HAVE_NEW) &&
-		    !is_null_oid(&update->new_oid))
-			add_packed_ref(refs->packed_ref_store, update->refname,
-				       &update->new_oid);
-	}
-
-	if (commit_packed_refs(refs->packed_ref_store, err)) {
+	if (initial_ref_transaction_commit(packed_transaction, err)) {
 		ret = TRANSACTION_GENERIC_ERROR;
 		goto cleanup;
 	}
 
 cleanup:
+	if (packed_transaction)
+		ref_transaction_free(packed_transaction);
 	packed_refs_unlock(refs->packed_ref_store);
 	transaction->state = REF_TRANSACTION_CLOSED;
 	string_list_clear(&affected_refnames, 0);

-- 
2.14.1

On Tue, Aug 29, 2017 at 10:20:30AM +0200, Michael Haggerty wrote:

Used a `packed_ref_store` transaction in the implementation of
`files_initial_transaction_commit()` rather than using internal
features of the packed ref store. This further decouples
`files_ref_store` from `packed_ref_store`.

Very nice to see these couplings going away.

Minor nit: s/Used/Use/ in the commit message.

-Peff

On 09/08/2017 09:27 AM, Jeff King wrote:

On Tue, Aug 29, 2017 at 10:20:30AM +0200, Michael Haggerty wrote:

quoted

Used a `packed_ref_store` transaction in the implementation of
`files_initial_transaction_commit()` rather than using internal
features of the packed ref store. This further decouples
`files_ref_store` from `packed_ref_store`.

Very nice to see these couplings going away.

Minor nit: s/Used/Use/ in the commit message.

Thanks; will fix.

Michael

Currently, a loose reference is deleted even before locking the
`packed-refs` file, let alone deleting any packed version of the
reference. This leads to two problems, demonstrated by two new tests:

* While a reference is being deleted, other processes might see the
  old, packed value of the reference for a moment before the packed
  version is deleted. Normally this would be hard to observe, but we
  can prolong the window by locking the `packed-refs` file externally
  before running `update-ref`, then unlocking it before `update-ref`'s
  attempt to acquire the lock times out.

* If the `packed-refs` file is locked so long that `update-ref` fails
  to lock it, then the reference can be left permanently in the
  incorrect state described in the previous point.

In a moment, both problems will be fixed.

Signed-off-by: Michael Haggerty <redacted>
---
The first of the new tests is rather involved; it uses two background
processes plus a foreground process that polls the value of a
reference. But despite sensitivity to timing, I think it should be
robust even in this broken state. Once the functionality being tested
is fixed, this test should never produce false positives, though
really bad timing (e.g., if it takes more than a second for
`update-ref` to get going) could still lead to false negatives.

Each of the new tests takes about a second to run because they
simulate lock contention.

If anybody has suggestions for better ways to test these things,
please speak up :-)

 t/t1404-update-ref-errors.sh | 71 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
index c34ece48f5..752f83c377 100755
--- a/t/t1404-update-ref-errors.sh
+++ b/t/t1404-update-ref-errors.sh

@@ -404,4 +404,75 @@ test_expect_success 'broken reference blocks indirect create' '
 	test_cmp expected output.err
 '
 
+test_expect_failure 'no bogus intermediate values during delete' '
+	prefix=refs/slow-transaction &&
+	# Set up a reference with differing loose and packed versions:
+	git update-ref $prefix/foo $C &&
+	git pack-refs --all &&
+	git update-ref $prefix/foo $D &&
+	git for-each-ref $prefix >unchanged &&
+	# Now try to update the reference, but hold the `packed-refs` lock
+	# for a while to see what happens while the process is blocked:
+	: >.git/packed-refs.lock &&
+	test_when_finished "rm -f .git/packed-refs.lock" &&
+	{
+		sleep 1 &&
+		rm -f .git/packed-refs.lock &
+	} &&
+	pid1=$! &&
+	{
+		# Note: the following command is intentionally run in the
+		# background. We extend the timeout so that `update-ref`
+		# tries to acquire the `packed-refs` lock longer than it
+		# takes the background process above to delete it:
+		git -c core.packedrefstimeout=2000 update-ref -d $prefix/foo &
+	} &&
+	pid2=$! &&
+	ok=true &&
+	while kill -0 $pid2 2>/dev/null
+	do
+		sha1=$(git rev-parse --verify --quiet $prefix/foo || echo undefined) &&
+		case "$sha1" in
+		$D)
+			# This is OK; it just means that nothing has happened yet.
+			: ;;
+		undefined)
+			# This is OK; it means the deletion was successful.
+			: ;;
+		$C)
+			# This value should never be seen. Probably the loose
+			# reference has been deleted but the packed reference
+			# is still there:
+			echo "$prefix/foo incorrectly observed to be C" &&
+			break
+			;;
+		*)
+			# WTF?
+			echo "$prefix/foo unexpected value observed: $sha1" &&
+			break
+			;;
+		esac
+	done >out &&
+	wait $pid1 &&
+	wait $pid2 &&
+	test_must_be_empty out &&
+	test_must_fail git rev-parse --verify --quiet $prefix/foo
+'
+
+test_expect_failure 'delete fails cleanly if packed-refs file is locked' '
+	prefix=refs/locked-packed-refs &&
+	# Set up a reference with differing loose and packed versions:
+	git update-ref $prefix/foo $C &&
+	git pack-refs --all &&
+	git update-ref $prefix/foo $D &&
+	git for-each-ref $prefix >unchanged &&
+	# Now try to delete it while the `packed-refs` lock is held:
+	: >.git/packed-refs.lock &&
+	test_when_finished "rm -f .git/packed-refs.lock" &&
+	test_must_fail git update-ref -d $prefix/foo >out 2>err &&
+	git for-each-ref $prefix >actual &&
+	grep "Unable to create $Q.*packed-refs.lock$Q: File exists" err &&
+	test_cmp unchanged actual
+'
+
 test_done

-- 
2.14.1

On Tue, Aug 29, 2017 at 1:20 AM, Michael Haggerty [off-list ref] wrote:

    If sig is 0, then no signal is sent, but error checking is still
    performed; this can be used to check for the existence of a
    process ID or process group ID.

So the kill -0 is the idiomatic form of "while $pid2 is still alive"?
ignoring errors due to the dev/null redirection?

And due to the nature of this test we have to have a busy
loop, we cannot rate limit the cpu usage inside the loop
via some shorter sleeps, as ideally we want to observe
the ref at any time.

    In an ideal world this test would instruct the kernel to interrupt
    the executing program (update-ref) at certain events such as
    touching/writing/deleting files and in each interrupt we could
    inspect the file system in a read only fashion.

+       do
+               sha1=$(git rev-parse --verify --quiet $prefix/foo || echo undefined) &&
+               case "$sha1" in
+               $D)
+                       # This is OK; it just means that nothing has happened yet.
+                       : ;;
+               undefined)
+                       # This is OK; it means the deletion was successful.
+                       : ;;
+               $C)
+                       # This value should never be seen. Probably the loose
+                       # reference has been deleted but the packed reference
+                       # is still there:
+                       echo "$prefix/foo incorrectly observed to be C" &&
+                       break
+                       ;;
+               *)
+                       # WTF?
+                       echo "$prefix/foo unexpected value observed: $sha1" &&
+                       break
+                       ;;
+               esac
+       done >out &&
+       wait $pid1 &&
+       wait $pid2 &&

oh, you use explicit pids here to check each exit code.

If anybody has suggestions for better ways to test these things,
please speak up :-)

I don't think I'd have a satisfactory answer to that, as the timing is inherent
to the things we test. In other software projects that are less low level, I
would have suggested to use a time/clock mock, which can be stopped
and then inspection can be performed at defined states.

On 08/30/2017 07:21 PM, Stefan Beller wrote:

On Tue, Aug 29, 2017 at 1:20 AM, Michael Haggerty [off-list ref] wrote:

quoted

[...]
+test_expect_failure 'no bogus intermediate values during delete' '
+       prefix=refs/slow-transaction &&
+       # Set up a reference with differing loose and packed versions:
+       git update-ref $prefix/foo $C &&
+       git pack-refs --all &&
+       git update-ref $prefix/foo $D &&
+       git for-each-ref $prefix >unchanged &&
+       # Now try to update the reference, but hold the `packed-refs` lock
+       # for a while to see what happens while the process is blocked:
+       : >.git/packed-refs.lock &&
+       test_when_finished "rm -f .git/packed-refs.lock" &&
+       {
+               sleep 1 &&
+               rm -f .git/packed-refs.lock &
+       } &&
+       pid1=$! &&
+       {
+               # Note: the following command is intentionally run in the
+               # background. We extend the timeout so that `update-ref`
+               # tries to acquire the `packed-refs` lock longer than it
+               # takes the background process above to delete it:
+               git -c core.packedrefstimeout=2000 update-ref -d $prefix/foo &
+       } &&
+       pid2=$! &&
+       ok=true &&
+       while kill -0 $pid2 2>/dev/null

    If sig is 0, then no signal is sent, but error checking is still
    performed; this can be used to check for the existence of a
    process ID or process group ID.

So the kill -0 is the idiomatic form of "while $pid2 is still alive"?
ignoring errors due to the dev/null redirection?

And due to the nature of this test we have to have a busy
loop, we cannot rate limit the cpu usage inside the loop
via some shorter sleeps, as ideally we want to observe
the ref at any time.

Correct on both counts.

I just noticed that there is a stray line `ok=true &&` from an earlier
draft. I'll remove that in v2.

    In an ideal world this test would instruct the kernel to interrupt
    the executing program (update-ref) at certain events such as
    touching/writing/deleting files and in each interrupt we could
    inspect the file system in a read only fashion.

A tool like `strace` could be used for tests like this, but it would be
terribly non-portable. (But I often use strace manually to check that
the ordering of filesystem events is correct.)

quoted

+       do
+               sha1=$(git rev-parse --verify --quiet $prefix/foo || echo undefined) &&
+               case "$sha1" in
+               $D)
+                       # This is OK; it just means that nothing has happened yet.
+                       : ;;
+               undefined)
+                       # This is OK; it means the deletion was successful.
+                       : ;;
+               $C)
+                       # This value should never be seen. Probably the loose
+                       # reference has been deleted but the packed reference
+                       # is still there:
+                       echo "$prefix/foo incorrectly observed to be C" &&
+                       break
+                       ;;
+               *)
+                       # WTF?
+                       echo "$prefix/foo unexpected value observed: $sha1" &&
+                       break
+                       ;;
+               esac
+       done >out &&
+       wait $pid1 &&
+       wait $pid2 &&

oh, you use explicit pids here to check each exit code.

quoted

If anybody has suggestions for better ways to test these things,
please speak up :-)

I don't think I'd have a satisfactory answer to that, as the timing is inherent
to the things we test. In other software projects that are less low level, I
would have suggested to use a time/clock mock, which can be stopped
and then inspection can be performed at defined states.

I just realized that, given that the main goal here is to check the
value of the reference while `update-ref` is waiting on the
`packed-refs` lock, we can do the test without a busy loop. Instead, we
roughly

	: >.git/packed-refs.lock &&
	{
		git -c core.packedrefstimeout=2000 update-ref -d $prefix/foo &
	} &&
	pid2=$! &&
	sleep 1 &&
	# Verify that update-ref is still running:
	kill -0 $pid2 &&
	# ...verify that the reference still has its old value...
	rm -f .git/packed-refs.lock &&
	wait $pid2 &&
	# ...verify that the reference is now gone...

It's true that this version wouldn't discover incorrect transitional
values of the reference that happen at other times, but that was very
unlikely anyway given the speed disparity between C and shell. I'll make
this change in v2.

Michael

Now the outside world interacts with the packed ref store only via the
generic refs API plus a few lock-related functions. This allows us to
delete some functions that are no longer used, thereby completing the
encapsulation of the packed ref store.

Signed-off-by: Michael Haggerty <redacted>
---
 refs/packed-backend.c | 193 --------------------------------------------------
 refs/packed-backend.h |   8 ---
 2 files changed, 201 deletions(-)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 83a088118f..90f44c1fbb 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c

@@ -91,19 +91,6 @@ struct ref_store *packed_ref_store_create(const char *path,
 	return ref_store;
 }
 
-/*
- * Die if refs is not the main ref store. caller is used in any
- * necessary error messages.
- */
-static void packed_assert_main_repository(struct packed_ref_store *refs,
-					  const char *caller)
-{
-	if (refs->store_flags & REF_STORE_MAIN)
-		return;
-
-	die("BUG: operation %s only allowed for main ref store", caller);
-}
-
 /*
  * Downcast `ref_store` to `packed_ref_store`. Die if `ref_store` is
  * not a `packed_ref_store`. Also die if `packed_ref_store` doesn't

@@ -321,40 +308,6 @@ static struct ref_dir *get_packed_refs(struct packed_ref_store *refs)
 	return get_packed_ref_dir(get_packed_ref_cache(refs));
 }
 
-/*
- * Add or overwrite a reference in the in-memory packed reference
- * cache. This may only be called while the packed-refs file is locked
- * (see packed_refs_lock()). To actually write the packed-refs file,
- * call commit_packed_refs().
- */
-void add_packed_ref(struct ref_store *ref_store,
-		    const char *refname, const struct object_id *oid)
-{
-	struct packed_ref_store *refs =
-		packed_downcast(ref_store, REF_STORE_WRITE,
-				"add_packed_ref");
-	struct ref_dir *packed_refs;
-	struct ref_entry *packed_entry;
-
-	if (!is_lock_file_locked(&refs->lock))
-		die("BUG: packed refs not locked");
-
-	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
-		die("Reference has invalid format: '%s'", refname);
-
-	packed_refs = get_packed_refs(refs);
-	packed_entry = find_ref_entry(packed_refs, refname);
-	if (packed_entry) {
-		/* Overwrite the existing entry: */
-		oidcpy(&packed_entry->u.value.oid, oid);
-		packed_entry->flag = REF_ISPACKED;
-		oidclr(&packed_entry->u.value.peeled);
-	} else {
-		packed_entry = create_ref_entry(refname, oid, REF_ISPACKED);
-		add_ref_entry(packed_refs, packed_entry);
-	}
-}
-
 /*
  * Return the ref_entry for the given refname from the packed
  * references.  If it does not exist, return NULL.

@@ -592,152 +545,6 @@ int packed_refs_is_locked(struct ref_store *ref_store)
 static const char PACKED_REFS_HEADER[] =
 	"# pack-refs with: peeled fully-peeled \n";
 
-/*
- * Write the current version of the packed refs cache from memory to
- * disk. The packed-refs file must already be locked for writing (see
- * packed_refs_lock()). Return zero on success. On errors, rollback
- * the lockfile, write an error message to `err`, and return a nonzero
- * value.
- */
-int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
-{
-	struct packed_ref_store *refs =
-		packed_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,
-				"commit_packed_refs");
-	struct packed_ref_cache *packed_ref_cache =
-		get_packed_ref_cache(refs);
-	int ok;
-	int ret = -1;
-	struct strbuf sb = STRBUF_INIT;
-	FILE *out;
-	struct ref_iterator *iter;
-	char *packed_refs_path;
-
-	if (!is_lock_file_locked(&refs->lock))
-		die("BUG: commit_packed_refs() called when unlocked");
-
-	/*
-	 * If packed-refs is a symlink, we want to overwrite the
-	 * symlinked-to file, not the symlink itself. Also, put the
-	 * staging file next to it:
-	 */
-	packed_refs_path = get_locked_file_path(&refs->lock);
-	strbuf_addf(&sb, "%s.new", packed_refs_path);
-	if (create_tempfile(&refs->tempfile, sb.buf) < 0) {
-		strbuf_addf(err, "unable to create file %s: %s",
-			    sb.buf, strerror(errno));
-		strbuf_release(&sb);
-		goto out;
-	}
-	strbuf_release(&sb);
-
-	out = fdopen_tempfile(&refs->tempfile, "w");
-	if (!out) {
-		strbuf_addf(err, "unable to fdopen packed-refs tempfile: %s",
-			    strerror(errno));
-		goto error;
-	}
-
-	if (fprintf(out, "%s", PACKED_REFS_HEADER) < 0) {
-		strbuf_addf(err, "error writing to %s: %s",
-			    get_tempfile_path(&refs->tempfile), strerror(errno));
-		goto error;
-	}
-
-	iter = cache_ref_iterator_begin(packed_ref_cache->cache, NULL, 0);
-	while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
-		struct object_id peeled;
-		int peel_error = ref_iterator_peel(iter, &peeled);
-
-		if (write_packed_entry(out, iter->refname, iter->oid->hash,
-				       peel_error ? NULL : peeled.hash)) {
-			strbuf_addf(err, "error writing to %s: %s",
-				    get_tempfile_path(&refs->tempfile),
-				    strerror(errno));
-			ref_iterator_abort(iter);
-			goto error;
-		}
-	}
-
-	if (ok != ITER_DONE) {
-		strbuf_addf(err, "unable to rewrite packed-refs file: "
-			    "error iterating over old contents");
-		goto error;
-	}
-
-	if (rename_tempfile(&refs->tempfile, packed_refs_path)) {
-		strbuf_addf(err, "error replacing %s: %s",
-			    refs->path, strerror(errno));
-		goto out;
-	}
-
-	ret = 0;
-	goto out;
-
-error:
-	delete_tempfile(&refs->tempfile);
-
-out:
-	free(packed_refs_path);
-	return ret;
-}
-
-/*
- * Rewrite the packed-refs file, omitting any refs listed in
- * 'refnames'. On error, leave packed-refs unchanged, write an error
- * message to 'err', and return a nonzero value. The packed refs lock
- * must be held when calling this function; it will still be held when
- * the function returns.
- *
- * The refs in 'refnames' needn't be sorted. `err` must not be NULL.
- */
-int repack_without_refs(struct ref_store *ref_store,
-			struct string_list *refnames, struct strbuf *err)
-{
-	struct packed_ref_store *refs =
-		packed_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,
-				"repack_without_refs");
-	struct ref_dir *packed;
-	struct string_list_item *refname;
-	int needs_repacking = 0, removed = 0;
-
-	packed_assert_main_repository(refs, "repack_without_refs");
-	assert(err);
-
-	if (!is_lock_file_locked(&refs->lock))
-		die("BUG: repack_without_refs called without holding lock");
-
-	/* Look for a packed ref */
-	for_each_string_list_item(refname, refnames) {
-		if (get_packed_ref(refs, refname->string)) {
-			needs_repacking = 1;
-			break;
-		}
-	}
-
-	/* Avoid locking if we have nothing to do */
-	if (!needs_repacking)
-		return 0; /* no refname exists in packed refs */
-
-	packed = get_packed_refs(refs);
-
-	/* Remove refnames from the cache */
-	for_each_string_list_item(refname, refnames)
-		if (remove_entry_from_dir(packed, refname->string) != -1)
-			removed = 1;
-	if (!removed) {
-		/*
-		 * All packed entries disappeared while we were
-		 * acquiring the lock.
-		 */
-		clear_packed_ref_cache(refs);
-		return 0;
-	}
-
-	/* Write what remains */
-	return commit_packed_refs(&refs->base, err);
-}
-
 static int packed_init_db(struct ref_store *ref_store, struct strbuf *err)
 {
 	/* Nothing to do. */

diff --git a/refs/packed-backend.h b/refs/packed-backend.h
index 7af2897757..61687e408a 100644
--- a/refs/packed-backend.h
+++ b/refs/packed-backend.h

@@ -23,12 +23,4 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
 void packed_refs_unlock(struct ref_store *ref_store);
 int packed_refs_is_locked(struct ref_store *ref_store);
 
-void add_packed_ref(struct ref_store *ref_store,
-		    const char *refname, const struct object_id *oid);
-
-int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err);
-
-int repack_without_refs(struct ref_store *ref_store,
-			struct string_list *refnames, struct strbuf *err);
-
 #endif /* REFS_PACKED_BACKEND_H */

-- 
2.14.1

On 08/29, Michael Haggerty wrote:

Now the outside world interacts with the packed ref store only via the
generic refs API plus a few lock-related functions. This allows us to
delete some functions that are no longer used, thereby completing the
encapsulation of the packed ref store.

Love seeing patches which only remove old code :)

-- 
Brandon Williams

When processing a `files_ref_store` transaction, it is sometimes
necessary to delete some references from the "packed-refs" file. Do
that using a reference transaction conducted against the
`packed_ref_store`.

This change further decouples `files_ref_store` from
`packed_ref_store`. It also fixes multiple problems, including the two
revealed by test cases added in the previous commit.

First, the old code didn't obtain the `packed-refs` lock until
`files_transaction_finish()`. This means that a failure to acquire the
`packed-refs` lock (e.g., due to contention with another process)
wasn't detected until it was too late (problems like this are supposed
to be detected in the "prepare" phase). The new code acquires the
`packed-refs` lock in `files_transaction_prepare()`, the same stage of
the processing when the loose reference locks are being acquired,
removing another reason why the "prepare" phase might succeed and the
"finish" phase might nevertheless fail.

Second, the old code deleted the loose version of a reference before
deleting any packed version of the same reference. This left a moment
when another process might think that the packed version of the
reference is current, which is incorrect. (Even worse, the packed
version of the reference can be arbitrarily old, and might even point
at an object that has since been garbage-collected.)

Third, if a reference deletion fails to acquire the `packed-refs` lock
altogether, then the old code might leave the repository in the
incorrect state (possibly corrupt) described in the previous
paragraph.

Now we activate the new "packed-refs" file (sans any references that
are being deleted) *before* deleting the corresponding loose
references. But we hold the "packed-refs" lock until after the loose
references have been finalized, thus preventing a simultaneous
"pack-refs" process from packing the loose version of the reference in
the time gap, which would otherwise defeat our attempt to delete it.

Signed-off-by: Michael Haggerty <redacted>
---
 refs/files-backend.c         | 132 +++++++++++++++++++++++++++++++++----------
 t/t1404-update-ref-errors.sh |   4 +-
 2 files changed, 103 insertions(+), 33 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 29c7c78602..4f4c47b9db 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c

@@ -2391,13 +2391,22 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 	return 0;
 }
 
+struct files_transaction_backend_data {
+	struct ref_transaction *packed_transaction;
+	int packed_refs_locked;
+};
+
 /*
  * Unlock any references in `transaction` that are still locked, and
  * mark the transaction closed.
  */
-static void files_transaction_cleanup(struct ref_transaction *transaction)
+static void files_transaction_cleanup(struct files_ref_store *refs,
+				      struct ref_transaction *transaction)
 {
 	size_t i;
+	struct files_transaction_backend_data *backend_data =
+		transaction->backend_data;
+	struct strbuf err = STRBUF_INIT;
 
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = transaction->updates[i];

@@ -2409,6 +2418,17 @@ static void files_transaction_cleanup(struct ref_transaction *transaction)
 		}
 	}
 
+	if (backend_data->packed_transaction &&
+	    ref_transaction_abort(backend_data->packed_transaction, &err)) {
+		error("error aborting transaction: %s", err.buf);
+		strbuf_release(&err);
+	}
+
+	if (backend_data->packed_refs_locked)
+		packed_refs_unlock(refs->packed_ref_store);
+
+	free(backend_data);
+
 	transaction->state = REF_TRANSACTION_CLOSED;
 }
 

@@ -2425,12 +2445,17 @@ static int files_transaction_prepare(struct ref_store *ref_store,
 	char *head_ref = NULL;
 	int head_type;
 	struct object_id head_oid;
+	struct files_transaction_backend_data *backend_data;
+	struct ref_transaction *packed_transaction = NULL;
 
 	assert(err);
 
 	if (!transaction->nr)
 		goto cleanup;
 
+	backend_data = xcalloc(1, sizeof(*backend_data));
+	transaction->backend_data = backend_data;
+
 	/*
 	 * Fail if a refname appears more than once in the
 	 * transaction. (If we end up splitting up any updates using

@@ -2497,6 +2522,41 @@ static int files_transaction_prepare(struct ref_store *ref_store,
 					  head_ref, &affected_refnames, err);
 		if (ret)
 			break;
+
+		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.
+			 */
+			if (!packed_transaction) {
+				packed_transaction = ref_store_transaction_begin(
+						refs->packed_ref_store, err);
+				if (!packed_transaction) {
+					ret = TRANSACTION_GENERIC_ERROR;
+					goto cleanup;
+				}
+
+				backend_data->packed_transaction =
+					packed_transaction;
+			}
+
+			ref_transaction_add_update(
+					packed_transaction, update->refname,
+					update->flags & ~REF_HAVE_OLD,
+					update->new_oid.hash, update->old_oid.hash,
+					NULL);
+		}
+	}
+
+	if (packed_transaction) {
+		if (packed_refs_lock(refs->packed_ref_store, 0, err)) {
+			ret = TRANSACTION_GENERIC_ERROR;
+			goto cleanup;
+		}
+		backend_data->packed_refs_locked = 1;
+		ret = ref_transaction_prepare(packed_transaction, err);
 	}
 
 cleanup: