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

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 23, 2020, 20:00 UTC
Message-ID: <X5M2Yd7cxVRN1nlA@nand.local>
In-Reply-To: <20201023010315.GA1542721@coredump.intra.peff.net>

On Thu, Oct 22, 2020 at 09:03:15PM -0400, Jeff King wrote:

Show 5 quoted lines

> We are expecting to see $ZERO_OID in that slot, even though the current
> value of the ref is "PRE" due to our reset above.
>
> Should this be $PRE_OID (we don't have that variable, but it would be
> the result of "git rev-parse PRE")?

I also skimmed past it, but we do already have $PRE_OID, which is convenient.

Show 8 quoted lines

> I could alternatively see an argument that <old-value> is the old-value
> that the caller asked for. So seeing $ZERO_OID is saying that the caller
> wants to move from _anything_ to $POST_OID, and we're not willing to
> tell the hook what the current value actually is.
>
> We could actually fill in the current value for zero cost. The reason we
> found this is that we have a custom patch at GitHub that fills in these
> values when we open the ref after locking.

Yup, modulo being easy for symrefs (which I talk about in [1]), but it shouldn't be impossible.

[1]: https://lore.kernel.org/git/X5M1oe4lfkUy9lAh@nand.local

Show 32 quoted lines

> In real usage, I'm not sure how much the distinction would matter,
> because any careful caller would provide a non-zero "old" value. And if
> that doesn't match the current value, we'd reject the transaction before
> we even hit the hook, I think. It's only the fact that the update-ref
> calls are sloppy and do not provide an expected old value that it even
> matters.
>
> So I wonder if:
>
> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
> index f6e741c6c0..8155522a1a 100755
> --- a/t/t1416-ref-transaction-hooks.sh
> +++ b/t/t1416-ref-transaction-hooks.sh
> @@ -9,6 +9,7 @@ test_expect_success setup '
>  	test_commit PRE &&
>  	PRE_OID=$(git rev-parse PRE) &&
>  	test_commit POST &&
> +	PRE_OID=$(git rev-parse PRE) &&
>  	POST_OID=$(git rev-parse POST)
>  '
>
> @@ -52,10 +53,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 POST <<-EOF &&

This should be "git update-ref HEAD POST PRE", since PRE is the before state.

> would be a step forward. This isn't changing the actual behavior,
> obviously. It's just tweaking the test so that it tests the more likely
> real-world case.  But we'd possibly want to actually change the behavior
> to always send the actual ref value to the hook.

I have to look at the issue in [1] a little bit more to determine whether or not it requires major surgery. If it does, then I'd be fine going forward with just your patch to the tests. If it doesn't, then updating the refs machinery to invoke the hook with the before OIDs correctly even for symrefs seems sensible to me.

> -Peff

Thanks, Taylor