Re: [PATCH v4] refs: implement reference transaction hook

[Taylor Blau <me@ttaylorr.com>]

On Mon, Oct 26, 2020 at 08:43:03AM +0100, Patrick Steinhardt wrote:
> @Taylor, given that you've already dug into the code: do you already
> have plans to post a patch for this?

You are likely in a better position to do that than I am. I am
unfamiliar enough with the refs.c code to feel confident that my change
is correct, let alone working. The combination of REF_HAVE_OLD, the lock
OID, the update OID, and so on is very puzzling.

Ordinarily, I'd be happy to post a patch after familiarizing myself, but
right now I don't have the time. So, I may come back to this in six
months, but I certainly won't feel bad if you beat me to it ;-).

In the meantime, I'd be fine to apply Peff's patch with some fix-ups,
maybe something like what's below the scissors line.

Taylor

--- >8 ---

Subject: [PATCH] t1416: specify pre-state for ref-updates

The ref-transaction hook documentation says that the expected format for
each line is:

  <old-value> SP <new-value> SP <ref-name> LF

without defining what <old-value> is. It could either be the current
state of the refs (after locking, but before committing the
transaction), or the optional caller-provided pre-state.

If <old-value> is to mean the caller-provided pre-state, than $ZERO_OID
could be taken to mean that the update is allowed to take place without
requiring the ref to be at some state. On the other hand, if <old-value>
is taken to mean "the current value of the reference", then that
requires a behavior change.

But that may only be semi-realistic, since any careful callers are
likely to pass a pre-state around anyway, and failing to meet that
pre-state means the hook will not even be invoked.

So, tweak the tests to more closely match how callers will actually
invoke this hook by providing a pre-state explicitly and then asserting
that it made its way down to the ref-transaction hook.

If we do decide to go further and implement a behavior change, it would
make sense to modify the tests to instead look something like:

  for before in "$PRE" ""
  do
    cat >expect <<-EOF &&
      $ZERO_OID $POST_OID HEAD
      $ZERO_OID $POST_OID refs/heads/master
      $PRE_OID $POST_OID HEAD
      $PRE_OID $POST_OID refs/heads/master
    EOF
    git reset --hard $PRE &&
    git update-ref HEAD POST $before &&
    test_cmp expect actual
  done

Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t1416-ref-transaction-hooks.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index f6e741c6c0..74f94e293c 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -52,10 +52,10 @@ test_expect_success 'hook gets all queued updates in prepared state' '
 		fi
 	EOF
 	cat >expect <<-EOF &&
-		$ZERO_OID $POST_OID HEAD
-		$ZERO_OID $POST_OID refs/heads/master
+		$PRE_OID $POST_OID HEAD
+		$PRE_OID $POST_OID refs/heads/master
 	EOF
-	git update-ref HEAD POST <<-EOF &&
+	git update-ref HEAD POST PRE <<-EOF &&
 		update HEAD $ZERO_OID $POST_OID
 		update refs/heads/master $ZERO_OID $POST_OID
 	EOF
@@ -75,10 +75,10 @@ test_expect_success 'hook gets all queued updates in committed state' '
 		fi
 	EOF
 	cat >expect <<-EOF &&
-		$ZERO_OID $POST_OID HEAD
-		$ZERO_OID $POST_OID refs/heads/master
+		$PRE_OID $POST_OID HEAD
+		$PRE_OID $POST_OID refs/heads/master
 	EOF
-	git update-ref HEAD POST &&
+	git update-ref HEAD POST PRE &&
 	test_cmp expect actual
 '

--
2.29.1.9.g605042ee00