[v5,3/7] refs: support symrefs in 'reference-transaction' hook

Commit Message

karthik nayak May 1, 2024, 8:22 p.m. UTC

From: Karthik Nayak <[e-mail]>

The 'reference-transaction' hook runs whenever a reference update is
made to the system. In a previous commit, we added the `old_target` and
`new_target` fields to the `reference_transaction_update()`. In
following commits we'll also add the code to handle symref's in the
reference backends.

Support symrefs also in the 'reference-transaction' hook, by modifying
the current format:
    <old-oid> SP <new-oid> SP <ref-name> LF
to be be:
    <old-value> SP <new-value> SP <ref-name> LF
where for regular refs the output would not change and remain the same.
But when either 'old-value' or 'new-value' is a symref, we print the ref
as 'ref:<ref-target>'.

This does break backward compatibility, but the 'reference-transaction'
hook's documentation always stated that support for symbolic references
may be added in the future.

We do not add any tests in this commit since there is no git command
which activates this flow, in an upcoming commit, we'll start using
transaction based symref updates as the default, we'll add tests there
for the hook too.

Signed-off-by: Karthik Nayak <[e-mail]>
---
 Documentation/githooks.txt | 14 +++++++++-----
 refs.c                     | 16 ++++++++++++----
 2 files changed, 21 insertions(+), 9 deletions(-)

Comments

Junio C Hamano May 1, 2024, 11:05 p.m. UTC | #1

Karthik Nayak <[e-mail]> writes:

> +		if (update->flags & REF_HAVE_OLD && update->old_target)

Although the precedence rule does not require it,

		if ((update->flags & REF_HAVE_OLD) && update_old_target)

is probably easier to read.

> +			strbuf_addf(&buf, "ref:%s ", update->old_target);
> +		else
> +			strbuf_addf(&buf, "%s ", oid_to_hex(&update->old_oid));

So the promise this code assumes is that .old_target member is
non-NULL if and only if the ref originally is a symbolic ref?

And if the "we do not care what the original value is, whether it is
a normal ref or a symbolic one" case, .old_oid would be all '\0' and
REF_HAVE_OLD bit is not set?

If we can write it like so:

	if (!(update->flags & REF_HAVE_OLD))
		strbuf_addf(&buf, "%s ", oid_to_hex(null_oid()));
	else if (update->old_target)
		strbuf_addf(&buf, "ref:%s ", update->old_target);
	else
		strbuf_addf(&buf, "ref:%s ", oid_to_hex(update->old_oid));

it may make the intent of the code a lot more clear.  If we are
operating in "!HAVE_OLD" mode, we show 0{40}.  Otherwise, old_target
is non-NULL when the thing is symbolic, and if old_target is NULL,
it is not symbolic and has its own value.

The same comment applies to the other side.

> +		if (update->flags & REF_HAVE_NEW && update->new_target)
> +			strbuf_addf(&buf, "ref:%s ", update->new_target);
> +		else
> +			strbuf_addf(&buf, "%s ", oid_to_hex(&update->new_oid));


> +		strbuf_addf(&buf, "%s\n", update->refname);
>  
>  		if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
>  			if (errno != EPIPE) {

karthik nayak May 2, 2024, 5:32 a.m. UTC | #2

Junio C Hamano <[e-mail]> writes:

> Karthik Nayak <[e-mail]> writes:
>
>> +		if (update->flags & REF_HAVE_OLD && update->old_target)
>
> Although the precedence rule does not require it,
>
> 		if ((update->flags & REF_HAVE_OLD) && update_old_target)
>
> is probably easier to read.
>

Will add.

>> +			strbuf_addf(&buf, "ref:%s ", update->old_target);
>> +		else
>> +			strbuf_addf(&buf, "%s ", oid_to_hex(&update->old_oid));
>
> So the promise this code assumes is that .old_target member is
> non-NULL if and only if the ref originally is a symbolic ref?
>

Yes, for old_target this is correct. new_target could be set for a ref
to convert it to a symbolic ref.

> And if the "we do not care what the original value is, whether it is
> a normal ref or a symbolic one" case, .old_oid would be all '\0' and
> REF_HAVE_OLD bit is not set?
>

Yup that's accurate.

> If we can write it like so:
>
> 	if (!(update->flags & REF_HAVE_OLD))
> 		strbuf_addf(&buf, "%s ", oid_to_hex(null_oid()));
> 	else if (update->old_target)
> 		strbuf_addf(&buf, "ref:%s ", update->old_target);
> 	else
> 		strbuf_addf(&buf, "ref:%s ", oid_to_hex(update->old_oid));
>
> it may make the intent of the code a lot more clear.  If we are
> operating in "!HAVE_OLD" mode, we show 0{40}.  Otherwise, old_target
> is non-NULL when the thing is symbolic, and if old_target is NULL,
> it is not symbolic and has its own value.
>
> The same comment applies to the other side.
>

I see how it makes it clearer, but I think the intent with the existing
code was clear too. I'll add this change to my local for the next
version.

>> +		if (update->flags & REF_HAVE_NEW && update->new_target)
>> +			strbuf_addf(&buf, "ref:%s ", update->new_target);
>> +		else
>> +			strbuf_addf(&buf, "%s ", oid_to_hex(&update->new_oid));
>
>
>> +		strbuf_addf(&buf, "%s\n", update->refname);
>>
>>  		if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
>>  			if (errno != EPIPE) {

Patch

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index ee9b92c90d..06e997131b 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -486,7 +486,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).
+also supports symbolic reference updates.
 
 The hook takes exactly one argument, which is the current state the
 given reference transaction is in:
@@ -503,16 +503,20 @@  given reference transaction is in:
 For each reference update that was added to the transaction, the hook
 receives on standard input a line of the format:
 
-  <old-oid> SP <new-oid> SP <ref-name> LF
+  <old-value> SP <new-value> SP <ref-name> LF
 
-where `<old-oid>` is the old object name passed into the reference
-transaction, `<new-oid>` is the new object name to be stored in the
+where `<old-value>` is the old object name passed into the reference
+transaction, `<new-value>` is the new object name to be stored in the
 ref and `<ref-name>` is the full name of the ref. When force updating
 the reference regardless of its current value or when the reference is
-to be created anew, `<old-oid>` is the all-zeroes object name. To
+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 symbolic reference updates the `<old_value>` and `<new-value>`
+fields could denote references instead of objects. A reference will be
+denoted with a 'ref:' prefix, like `ref:<ref-target>`.
+
 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 47bc9dd103..5dfe93060a 100644
--- a/refs.c
+++ b/refs.c
@@ -2350,10 +2350,18 @@  static int run_transaction_hook(struct ref_transaction *transaction,
 		struct ref_update *update = transaction->updates[i];
 
 		strbuf_reset(&buf);
-		strbuf_addf(&buf, "%s %s %s\n",
-			    oid_to_hex(&update->old_oid),
-			    oid_to_hex(&update->new_oid),
-			    update->refname);
+
+		if (update->flags & REF_HAVE_OLD && update->old_target)
+			strbuf_addf(&buf, "ref:%s ", update->old_target);
+		else
+			strbuf_addf(&buf, "%s ", oid_to_hex(&update->old_oid));
+
+		if (update->flags & REF_HAVE_NEW && update->new_target)
+			strbuf_addf(&buf, "ref:%s ", update->new_target);
+		else
+			strbuf_addf(&buf, "%s ", oid_to_hex(&update->new_oid));
+
+		strbuf_addf(&buf, "%s\n", update->refname);
 
 		if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
 			if (errno != EPIPE) {