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

From: Jeff King <peff@peff.net>
Date: Oct 27, 2020, 05:37 UTC
Message-ID: <20201027053753.GA3005508@coredump.intra.peff.net>
In-Reply-To: <X5dhN+dsLXlKfukF@nand.local>

On Mon, Oct 26, 2020 at 07:52:23PM -0400, Taylor Blau wrote:

Show 15 quoted lines

> 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.

Thanks for moving this forward. I'm definitely OK with leaving the code for now and just tightening the test. But there is one curiosity, still. Your patch tweaks two tests:

> @@ -52,10 +52,10 @@ test_expect_success 'hook gets all queued updates in prepared state' '
> [...]
> @@ -75,10 +75,10 @@ test_expect_success 'hook gets all queued updates in committed state' '

but there's a third one, which checks the hook's view of the "aborted" state. And there we _do_ pass in an expected state which is not $PRE_OID. But it's $ZERO_OID, so we can't tell if if the hook is getting what we passed in, or some sensible value.

If we instead do this:

diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index 74f94e293c..a7ed983d3c 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -94,14 +94,15 @@ test_expect_success 'hook gets all queued updates in aborted state' '
 			done >actual
 		fi
 	EOF
+	nonsense_oid=$(echo $ZERO_OID | tr 0 a) &&
 	cat >expect <<-EOF &&
-		$ZERO_OID $POST_OID HEAD
-		$ZERO_OID $POST_OID refs/heads/master
+		$nonsense_oid $POST_OID HEAD
+		$nonsense_oid $POST_OID refs/heads/master
 	EOF
 	git update-ref --stdin <<-EOF &&
 		start
-		update HEAD POST $ZERO_OID
-		update refs/heads/master POST $ZERO_OID
+		update HEAD POST $nonsense_oid
+		update refs/heads/master POST $nonsense_oid
 		abort
 	EOF
 	test_cmp expect actual

Then test still passes, because it's passing along the value we
provided. And I think that's the only sensible thing we can do: show the
hook the proposed update. There's little point in telling it what the
actual ref values were, I would think.

So I think it's worth adding in to the test, but:

  - probably that meaning of old-value needs to be discussed in more
    detail in the hook documentation, because it feels like it differs a
    bit in the "aborted" case versus the "committed" case

  - we'd want to make sure this keeps passing if further changes to the
    code are made to support the case without an expected state
    specified (and not, say, accidentally passing $PRE_OID to the hook)

  - we'd likewise want eventually a test for the case without an
    expected state (which I guess would actually pass $ZERO_OID to the
    hook). Of course we're using a mismatch of the expected value as the
    reason to abort, so we'd have to find another reliable way to make
    the transaction abort. :) Perhaps a refname with illegal characters,
    or trying to create "foo/bar" when "foo" exists (or vice versa).

Most of that can be bumped to later, but I think it's worth squashing
something like the hunk above into the patch you posted.

-Peff