Re: [PATCH 8/8] refs: support symrefs in 'reference-transaction' hook

[Patrick Steinhardt <ps@pks.im>]

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

On Sat, Mar 30, 2024 at 11:46:23PM +0100, Karthik Nayak wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
> 
> The 'reference-transaction' hook runs whenever a reference update is
> made to the system. In the previous commit, we added support for the
> `update-symref` command in `git-update-ref`. While it allowed us to now
> create symbolic refs via `git-update-ref`, it didn't activate the
> 'reference-transaction' hook.
> 
> Let's activate the hook for symbolic reference updates too. This takes
> the form `<symref-target> SP <ref-name> LF`, which deviates from the
> form for regular updates since this only contains two fields.
> 
> While this seems to be backward incompatible, it is okay, since the only
> way the `reference-transaction` hook outputs this syntax is when
> `git-update-ref` is used with `update-symref` command. The command was
> only introduced in the previous commit and hence only users of this
> command will face this incompatibility.
> 
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  Documentation/githooks.txt       | 13 +++++++++++--
>  refs.c                           | 17 +++++++++--------
>  t/t1416-ref-transaction-hooks.sh | 27 +++++++++++++++++++++++++++
>  3 files changed, 47 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index 37f91d5b50..ae9f02974d 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -485,8 +485,7 @@ reference-transaction
>  
>  This hook is invoked by any Git command that performs reference
>  updates. It executes whenever a reference transaction is prepared,
> -committed or aborted and may thus get called multiple times. The hook
> -does not cover symbolic references (but that may change in the future).
> +committed or aborted and may thus get called multiple times.
>  
>  The hook takes exactly one argument, which is the current state the
>  given reference transaction is in:
> @@ -513,6 +512,16 @@ to be created anew, `<old-value>` is the all-zeroes object name. To
>  distinguish these cases, you can inspect the current value of
>  `<ref-name>` via `git rev-parse`.
>  
> +For each symbolic reference update that was added to the transaction,
> +the hook receives on standard input a line of the format:
> +
> +  <symref-target> SP <ref-name> LF

I was wondering whether we want the format to be a bit more explicit.
The proposed format works alright because refnames must not contain any
spaces, and thus it can be disambiguated from normal ref updates. But
it's rather easy to miss for authors of this hook.

I don't really know of a better format though. We could of course prefix
things with "symref:" or something like this, but that might not be a
good idea either. In hindsight it would've been clever to have a
specific prefix for every single ref update in the reference-transaction
hook. But well, here we are.

Patrick

> +where `<symref-target>` is the target of the symbolic reference update
> +passed into the reference transaction, `<ref-name>` is the full name of
> +the ref being updated. To distinguish from the regular updates, we can
> +note that there are only two fields.
> +
>  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.c b/refs.c
> index 706dcd6deb..d0929c5684 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2342,16 +2342,17 @@ static int run_transaction_hook(struct ref_transaction *transaction,
>  
>  	for (i = 0; i < transaction->nr; i++) {
>  		struct ref_update *update = transaction->updates[i];
> +		strbuf_reset(&buf);
>  
> -		// Reference transaction does not support symbolic updates.
>  		if (update->flags & REF_UPDATE_SYMREF)
> -			continue;
> -
> -		strbuf_reset(&buf);
> -		strbuf_addf(&buf, "%s %s %s\n",
> -			    oid_to_hex(&update->old_oid),
> -			    oid_to_hex(&update->new_oid),
> -			    update->refname);
> +			strbuf_addf(&buf, "%s %s\n",
> +				    update->symref_target,
> +				    update->refname);
> +		else
> +			strbuf_addf(&buf, "%s %s %s\n",
> +				    oid_to_hex(&update->old_oid),
> +				    oid_to_hex(&update->new_oid),
> +				    update->refname);
>  
>  		if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
>  			if (errno != EPIPE) {
> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
> index 2092488090..fac5d5fc6d 100755
> --- a/t/t1416-ref-transaction-hooks.sh
> +++ b/t/t1416-ref-transaction-hooks.sh
> @@ -83,6 +83,33 @@ test_expect_success 'hook gets all queued updates in committed state' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'hook gets all queued symref updates' '
> +	test_when_finished "rm actual" &&
> +	test_hook reference-transaction <<-\EOF &&
> +		echo "$*" >>actual
> +		while read -r line
> +		do
> +			printf "%s\n" "$line"
> +		done >>actual
> +	EOF
> +	cat >expect <<-EOF &&
> +		prepared
> +		refs/heads/test TESTSYMREF
> +		refs/heads/test refs/heads/symref
> +		committed
> +		refs/heads/test TESTSYMREF
> +		refs/heads/test refs/heads/symref
> +	EOF
> +	git update-ref --no-deref --stdin <<-EOF &&
> +		start
> +		update-symref TESTSYMREF refs/heads/test
> +		update-symref refs/heads/symref refs/heads/test
> +		prepare
> +		commit
> +	EOF
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'hook gets all queued updates in aborted state' '
>  	test_when_finished "rm actual" &&
>  	git reset --hard PRE &&
> -- 
> 2.43.GIT
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]