[PATCH v2 0/3] refs: report old OIDs for batched deletions

[Maciej Ciemborowicz <maciej.ciemborowicz@gmail.com>]

The reference-transaction hook receives zero as both the old and new OID
when branch, tag, fetch, and remote delete refs through refs_delete_refs().
Those callers already know the values that they selected for deletion.

Teach refs_delete_refs() to accept aligned old OIDs and pass them into the
transaction. Besides making the hook records useful, this makes the selected
callers reject concurrent changes instead of deleting values that they did
not inspect. For branch and tag, this restores the compare-and-delete
behavior that existed before 8198907795 converted them to batched deletion.
For pruning, it prevents a stale scan from deleting a ref updated by another
process.

The values are already available at every updated call site, so the series
adds no ref reads and retains batched performance.

Changes since v1:

 * Document the conditional deletion behavior and its race protection.
 * Add tests that update refs from the hook's preparing phase and verify that
   branch deletion and remote pruning preserve the concurrent update.
 * Avoid printing deletion status when a non-atomic prune fails.
 * Use a local string_list_item in refs_delete_refs(), as suggested by
   Karthik.

Based on maint at e9019fcafe (Git 2.55).

Tests:

 * t1416-ref-transaction-hooks.sh (files and reftable)
 * t3200-branch.sh
 * t7004-tag.sh
 * t5510-fetch.sh
 * t5505-remote.sh

Maciej Ciemborowicz (3):
  refs: allow callers to supply old OIDs for batch deletion
  branch, tag: retain old OIDs in batched deletions
  fetch, remote: retain old OIDs when pruning refs

 bisect.c                         |   2 +-
 builtin/branch.c                 |   7 +-
 builtin/fetch.c                  |  13 +++-
 builtin/remote.c                 |  39 +++++++++--
 builtin/tag.c                    |   7 +-
 refs.c                           |  23 ++++---
 refs.h                           |  12 +++-
 t/helper/test-ref-store.c        |   2 +-
 t/t1416-ref-transaction-hooks.sh | 110 +++++++++++++++++++++++++++++++
 9 files changed, 190 insertions(+), 25 deletions(-)

Range-diff against v1:
1:  e1c72cfba ! 1:  5c96a5a1e refs: allow callers to supply old OIDs for batch deletion
    @@ Commit message
         reference-transaction hooks consequently see a null old OID.
     
         Add an optional oid_array whose entries correspond to the refnames. Pass each
    -    non-null OID to ref_transaction_delete(). Existing callers retain the
    -    unconditional behavior for now.
    +    non-null OID to ref_transaction_delete(). Supplying an OID makes the deletion
    +    conditional: if the ref changed after the caller resolved it, the transaction
    +    fails instead of deleting the new value. Existing callers that pass NULL
    +    retain the unconditional behavior.
     
         Signed-off-by: Maciej Ciemborowicz <maciej.ciemborowicz@gmail.com>
     
    @@ refs.c: void ref_transaction_for_each_rejected_update(struct ref_transaction *tr
      	}
      
     -	for_each_string_list_item(item, refnames) {
    --		ret = ref_transaction_delete(transaction, item->string,
    --					     NULL, NULL, flags, msg, &err);
     +	for (i = 0; i < refnames->nr; i++) {
    ++		struct string_list_item *item = &refnames->items[i];
     +		const struct object_id *old_oid = old_oids ? &old_oids->oid[i] : NULL;
     +
     +		if (old_oid && is_null_oid(old_oid))
     +			old_oid = NULL;
    -+		ret = ref_transaction_delete(transaction, refnames->items[i].string,
    + 		ret = ref_transaction_delete(transaction, item->string,
    +-					     NULL, NULL, flags, msg, &err);
     +					     old_oid, NULL, flags, msg, &err);
      		if (ret) {
      			warning(_("could not delete reference %s: %s"),
    --				item->string, err.buf);
    -+				refnames->items[i].string, err.buf);
    - 			strbuf_reset(&err);
    - 			failures = 1;
    - 		}
    + 				item->string, err.buf);
     
      ## refs.h ##
     @@
2:  09e0b8557 ! 2:  d00fdeba2 branch, tag: retain old OIDs in batched deletions
    @@ Metadata
      ## Commit message ##
         branch, tag: retain old OIDs in batched deletions
     
    -    Since 8198907795 (use delete_refs when deleting tags or branches,
    -    2021-01-21), branch and tag deletion pass no old OIDs to the ref transaction.
    -    As a result, reference-transaction hooks report zero as both the old and new
    -    OID.
    +    Before 8198907795 (use delete_refs when deleting tags or branches,
    +    2021-01-21), branch and tag deletion passed each resolved old OID to
    +    delete_ref(). This prevented the command from deleting a ref that another
    +    process had changed after it was inspected.
     
    -    Both commands already resolve the old OIDs before starting the deletion. Pass
    -    those values to refs_delete_refs() so hooks receive useful old values without
    -    adding any ref reads.
    +    The conversion to batched deletion dropped those old OIDs. Besides making the
    +    deletions unconditional, this causes reference-transaction hooks to report
    +    zero as both the old and new OID.
    +
    +    Both commands still resolve the old OIDs before starting the deletion. Pass
    +    those values to refs_delete_refs(). This restores the old race protection and
    +    lets hooks receive useful old values without adding any ref reads. If a ref
    +    changes concurrently, the transaction fails and preserves the new value.
     
         Signed-off-by: Maciej Ciemborowicz <maciej.ciemborowicz@gmail.com>
     
    @@ t/t1416-ref-transaction-hooks.sh: test_expect_success setup '
     +	git tag -d delete-tag &&
     +	test_cmp expect actual
     +'
    ++
    ++test_expect_success 'branch deletion rejects a concurrent update' '
    ++	git branch delete-race PRE &&
    ++	test_hook reference-transaction <<-\EOF &&
    ++		marker=$(git rev-parse --git-path delete-race-once)
    ++		if test "$1" = preparing && test ! -e "$marker"
    ++		then
    ++			>"$marker"
    ++			git update-ref refs/heads/delete-race POST
    ++		fi
    ++		exit 0
    ++	EOF
    ++	test_must_fail git branch -D delete-race 2>err &&
    ++	test_grep "is at $POST_OID but expected $PRE_OID" err &&
    ++	test_cmp_rev POST refs/heads/delete-race
    ++'
     +
      test_expect_success 'hook allows updating ref if successful' '
      	git reset --hard PRE &&
3:  95c8abce3 ! 3:  461c36ccd fetch, remote: retain old OIDs when pruning refs
    @@ Commit message
         new_oid member. The pruning paths discard that value and request unconditional
         deletion, so reference-transaction hooks receive a null old OID.
     
    -    Carry the recorded values into the deletion transactions. This reuses data
    -    collected while finding stale refs and therefore requires no additional ref
    -    reads.
    +    Carry the recorded values into the deletion transactions. Besides giving the
    +    hooks useful values, this stops a stale scan from deleting a ref that another
    +    process updated before the transaction acquired its locks. A concurrent
    +    change now makes the prune fail and preserves the new value.
    +
    +    This reuses data collected while finding stale refs and therefore requires no
    +    additional ref reads. Do not print deletion status when a non-atomic prune
    +    fails its old-OID check.
     
         Signed-off-by: Maciej Ciemborowicz <maciej.ciemborowicz@gmail.com>
     
    @@ builtin/fetch.c: static int prune_refs(struct display_state *display_state,
     -						  NULL, 0);
     +						  &old_oids, 0);
      		}
    ++		if (result)
    ++			goto cleanup;
      	}
      
    + 	if (verbosity >= 0) {
     @@ builtin/fetch.c: static int prune_refs(struct display_state *display_state,
      
      cleanup:
    @@ builtin/remote.c: static int prune_remote(const char *remote, int dry_run)
     +	for_each_string_list_item(item, &refs_to_prune)
     +		oid_array_append(&old_oids, item->util);
      
    - 	if (!dry_run)
    +-	if (!dry_run)
    ++	if (!dry_run) {
      		result |= refs_delete_refs(get_main_ref_store(the_repository),
      					   "remote: prune", &refs_to_prune,
     -					   NULL, 0);
     +					   &old_oids, 0);
    ++		if (result)
    ++			goto cleanup;
    ++	}
      
      	for_each_string_list_item(item, &states.stale) {
     -		const char *refname = item->util;
    @@ builtin/remote.c: static int prune_remote(const char *remote, int dry_run)
      		if (dry_run)
      			printf_ln(_(" * [would prune] %s"),
     @@ builtin/remote.c: static int prune_remote(const char *remote, int dry_run)
    + 	refs_warn_dangling_symrefs(get_main_ref_store(the_repository),
      				   stdout, " ", dry_run, &refs_to_prune);
      
    ++cleanup:
      	string_list_clear(&refs_to_prune, 0);
     +	oid_array_clear(&old_oids);
      	free_remote_ref_states(&states);
    @@ builtin/remote.c: static int prune_remote(const char *remote, int dry_run)
      }
     
      ## t/t1416-ref-transaction-hooks.sh ##
    -@@ t/t1416-ref-transaction-hooks.sh: test_expect_success 'hook gets old values for batched branch/tag deletion' '
    - 	test_cmp expect actual
    +@@ t/t1416-ref-transaction-hooks.sh: test_expect_success 'branch deletion rejects a concurrent update' '
    + 	test_cmp_rev POST refs/heads/delete-race
      '
      
     +test_expect_success 'hook gets old values when pruning remote refs' '
    @@ t/t1416-ref-transaction-hooks.sh: test_expect_success 'hook gets old values for
     +		test_cmp expect actual
     +	)
     +'
    ++
    ++test_expect_success 'remote prune rejects a concurrent update' '
    ++	test_when_finished "rm -rf race-empty.git race-prune" &&
    ++	test_create_repo race-empty.git --bare &&
    ++	test_create_repo race-prune &&
    ++	test_commit -C race-prune one &&
    ++	one=$(git -C race-prune rev-parse HEAD) &&
    ++	test_commit -C race-prune two &&
    ++	two=$(git -C race-prune rev-parse HEAD) &&
    ++	git -C race-prune remote add origin ../race-empty.git &&
    ++	git -C race-prune update-ref refs/remotes/origin/race "$one" &&
    ++	test_hook -C race-prune reference-transaction <<-\EOF &&
    ++		marker=$(git rev-parse --git-path prune-race-once)
    ++		if test "$1" = preparing && test ! -e "$marker"
    ++		then
    ++			>"$marker"
    ++			git update-ref refs/remotes/origin/race HEAD
    ++		fi
    ++		exit 0
    ++	EOF
    ++	test_must_fail git -C race-prune remote prune origin >out 2>err &&
    ++	test "$two" = "$(git -C race-prune rev-parse refs/remotes/origin/race)" &&
    ++	! grep "\[pruned\]" out
    ++'
     +
      test_expect_success 'hook allows updating ref if successful' '
      	git reset --hard PRE &&
-- 
2.39.3 (Apple Git-146)