On Wed, Nov 04, 2020 at 03:58:40PM +0100, Patrick Steinhardt wrote:
> Inputs of the reference-transaction hook currently depends on the
> command which is being run. For example if the command `git update-ref
> $REF $A $B` is executed, it will receive "$B $A $REF" as input, but if
> the command `git update-ref $REF $A` is executed without providing the
> old value, then it will receive "0*40 $A $REF" as input. This is due to
> the fact that we directly write queued transaction updates into the
> hook's standard input, which will not contain the old object value in
> case it wasn't provided.
>
> While this behaviour reflects what is happening as part of the
> repository, it doesn't feel like it is useful. The main intent of the
> reference-transaction hook is to be able to completely audit all
> reference updates, no matter where they come from. As such, it makes a
> lot more sense to always provide actual values instead of what the user
> wanted. Furthermore, it's impossible for the hook to distinguish whether
> this is intended to be a branch creation or a branch update without
> doing additional digging with the current format.
>
> Fix the issue by storing the old object value into the queued
> transaction update operation if it wasn't provided by the caller.
>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
Quick ping on this patch. Is there any interest or shall I just drop it?
Patrick
> Documentation/githooks.txt | 6 ++++++
> refs/files-backend.c | 8 ++++++++
> refs/packed-backend.c | 2 ++
> t/t1416-ref-transaction-hooks.sh | 12 ++++++------
> 4 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index 4e097dc4e9..8f3540e2f6 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -492,6 +492,12 @@ receives on standard input a line of the format:
>
> <old-value> SP <new-value> SP <ref-name> LF
>
> +where `<old-value>` is the old object name stored in the ref,
> +`<new-value>` is the new object name to be stored in the ref and
> +`<ref-name>` is the full name of the ref.
> +When creating a new ref, `<old-value>` is 40 `0`.
> +When deleting an old ref, `<new-value>` is 40 `0`.
> +
> The exit status of the hook is ignored for any state except for the
> "prepared" state. In the "prepared" state, a non-zero exit status will
> cause the transaction to be aborted. The hook will not be called with
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 04e85e7002..5b10d3822b 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2452,6 +2452,9 @@ static int lock_ref_for_update(struct files_ref_store *refs,
> ret = TRANSACTION_GENERIC_ERROR;
> goto out;
> }
> +
> + if (!(update->flags & REF_HAVE_OLD))
> + oidcpy(&update->old_oid, &lock->old_oid);
> } else {
> /*
> * Create a new update for the reference this
> @@ -2474,6 +2477,9 @@ static int lock_ref_for_update(struct files_ref_store *refs,
> goto out;
> }
>
> + if (!(update->flags & REF_HAVE_OLD))
> + oidcpy(&update->old_oid, &lock->old_oid);
> +
> /*
> * If this update is happening indirectly because of a
> * symref update, record the old OID in the parent
> @@ -2484,6 +2490,8 @@ static int lock_ref_for_update(struct files_ref_store *refs,
> parent_update = parent_update->parent_update) {
> struct ref_lock *parent_lock = parent_update->backend_data;
> oidcpy(&parent_lock->old_oid, &lock->old_oid);
> + if (!(parent_update->flags & REF_HAVE_OLD))
> + oidcpy(&parent_update->old_oid, &lock->old_oid);
> }
> }
>
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index b912f2505f..08f0feee3d 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -1178,6 +1178,8 @@ static int write_with_updates(struct packed_ref_store *refs,
> oid_to_hex(&update->old_oid));
> goto error;
> }
> + } else {
> + oidcpy(&update->old_oid, iter->oid);
> }
>
> /* Now figure out what to use for the new value: */
> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
> index f6e741c6c0..111533682a 100755
> --- a/t/t1416-ref-transaction-hooks.sh
> +++ b/t/t1416-ref-transaction-hooks.sh
> @@ -52,12 +52,12 @@ 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 &&
> - update HEAD $ZERO_OID $POST_OID
> - update refs/heads/master $ZERO_OID $POST_OID
> + update HEAD $PRE_OID $POST_OID
> + update refs/heads/master $PRE_OID $POST_OID
> EOF
> test_cmp expect actual
> '
> @@ -75,8 +75,8 @@ 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 &&
> test_cmp expect actual
> --
> 2.29.2
>
Attachment:
signature.asc
Description: PGP signature
