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

On Mon, Oct 26, 2020 at 08:43:03AM +0100, Patrick Steinhardt wrote:

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(-)