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@xxxxxxxx>
Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx>
---
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
