Re: [PATCH] refs: Always pass old object name to reftx hook

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 5363 bytes --]

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@pks.im>

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]