threads / patch / 62489

patchrefs: don't invoke reference-transaction hook for reflogs

Subject: [PATCH] refs: don't invoke reference-transaction hook for reflogs

## tl;dr

3 messages between Nov 14, 2024 and Nov 15, 2024. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Karthik Nayak· Nov 14, 2024, 09:58 UTC · lore

The reference-transaction hook is invoked whenever there is a reference update being performed. For each state of the transaction, we iterate over the updates present and pass this information to the hook.

The `ref_update` structure is used to hold these updates within a `transaction`. We use the same structure for holding reflog updates too. Which means that the reference transaction hook is also obtaining information about a reflog update. This is a bug, since:

  - The hook is designed to work with reference updates and reflogs
  updates are different.
  - The hook doesn't have the required information to distinguish
  reference updates from reflog updates.

This is particularly evident when the default branch (pointed by HEAD) is updated, we see that the hook also receives information about HEAD being changed. In reality, we only add a reflog update for HEAD, while HEAD's values remains the same.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs.c                           | 3 +++
 t/t1416-ref-transaction-hooks.sh | 2 --
 2 files changed, 3 insertions(+), 2 deletions(-)

Show changes to 2 files +3 −2

refs.c, t/t1416-ref-transaction-hooks.sh

diff --git a/refs.c b/refs.c
index 5f729ed4124f7fe8fa9df7fd1f1ce951abefc585..7866cd7378da95b3cdd508500633958802d166d0 100644
--- a/refs.c
+++ b/refs.c
@@ -2185,6 +2185,9 @@ static int run_transaction_hook(struct ref_transaction *transaction,
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = transaction->updates[i];
 
+		if (update->flags & REF_LOG_ONLY)
+			continue;
+
 		strbuf_reset(&buf);
 
 		if (!(update->flags & REF_HAVE_OLD))
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index 5a812ca3c0b5d3f7de60dc999de6aafaf1f384a6..262efec60e85b7e00def18cce15d5ce15897836d 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -53,7 +53,6 @@ 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/main
 	EOF
 	git update-ref HEAD POST <<-EOF &&
@@ -76,7 +75,6 @@ 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/main
 	EOF
 	git update-ref HEAD POST &&

---
base-commit: b31fb630c0fc6869a33ed717163e8a1210460d94
change-id: 20241113-348-do-not-call-the-reference-transaction-hooks-for-reflogs-bb471a0b192f

Best regards,

-- 
Karthik Nayak <karthik.188@gmail.com>

Junio C Hamano· Nov 14, 2024, 23:48 UTC · re: Karthik Nayak · lore

Re: [PATCH] refs: don't invoke reference-transaction hook for reflogs

Karthik Nayak <karthik.188@gmail.com> writes:

Show 8 quoted lines

> The reference-transaction hook is invoked whenever there is a reference
> update being performed. For each state of the transaction, we iterate
> over the updates present and pass this information to the hook.
>
> The `ref_update` structure is used to hold these updates within a
> `transaction`. We use the same structure for holding reflog updates too.
> Which means that the reference transaction hook is also obtaining
> information about a reflog update. This is a bug, since:

Yeah, the transaction hook is deciding how the values of refs should (or should not) change, and its decisions should be sufficient to determine what should happen to corresponding reflog updates. If an update to the 'main' branch is let through, that update should result in a new reflog record for that branch. If such an update is blocked, there is no update to the branch, and a reflog record would not be created for such an update that did not happen.

One thing that the above argument does not capture is "stash", especially "stash drop". The way the subsystem abuses reflog disconnects ref updates from reflog updates, so there _is_ a use case for hooks to interfere with reflog updates.

However, the existing ref update transaction hook does not have to be the mechanism to vet "git stash" operation. If we really needed to, we could add reflog transaction hook for that later, outside the scope of this fix.

Show 40 quoted lines

> diff --git a/refs.c b/refs.c
> index 5f729ed4124f7fe8fa9df7fd1f1ce951abefc585..7866cd7378da95b3cdd508500633958802d166d0 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2185,6 +2185,9 @@ static int run_transaction_hook(struct ref_transaction *transaction,
>  	for (i = 0; i < transaction->nr; i++) {
>  		struct ref_update *update = transaction->updates[i];
>  
> +		if (update->flags & REF_LOG_ONLY)
> +			continue;
> +
>  		strbuf_reset(&buf);
>  
>  		if (!(update->flags & REF_HAVE_OLD))
> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
> index 5a812ca3c0b5d3f7de60dc999de6aafaf1f384a6..262efec60e85b7e00def18cce15d5ce15897836d 100755
> --- a/t/t1416-ref-transaction-hooks.sh
> +++ b/t/t1416-ref-transaction-hooks.sh
> @@ -53,7 +53,6 @@ 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/main
>  	EOF
>  	git update-ref HEAD POST <<-EOF &&
> @@ -76,7 +75,6 @@ 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/main
>  	EOF
>  	git update-ref HEAD POST &&
>
> ---
> base-commit: b31fb630c0fc6869a33ed717163e8a1210460d94
> change-id: 20241113-348-do-not-call-the-reference-transaction-hooks-for-reflogs-bb471a0b192f
>
> Best regards,

Patrick Steinhardt· Nov 15, 2024, 07:01 UTC · re: Junio C Hamano · lore

Re: [PATCH] refs: don't invoke reference-transaction hook for reflogs

On Fri, Nov 15, 2024 at 08:48:12AM +0900, Junio C Hamano wrote:

Show 28 quoted lines

> Karthik Nayak <karthik.188@gmail.com> writes:
> 
> > The reference-transaction hook is invoked whenever there is a reference
> > update being performed. For each state of the transaction, we iterate
> > over the updates present and pass this information to the hook.
> >
> > The `ref_update` structure is used to hold these updates within a
> > `transaction`. We use the same structure for holding reflog updates too.
> > Which means that the reference transaction hook is also obtaining
> > information about a reflog update. This is a bug, since:
> 
> Yeah, the transaction hook is deciding how the values of refs should
> (or should not) change, and its decisions should be sufficient to
> determine what should happen to corresponding reflog updates.  If an
> update to the 'main' branch is let through, that update should result
> in a new reflog record for that branch.  If such an update is blocked,
> there is no update to the branch, and a reflog record would not be
> created for such an update that did not happen.
> 
> One thing that the above argument does not capture is "stash",
> especially "stash drop".  The way the subsystem abuses reflog
> disconnects ref updates from reflog updates, so there _is_ a use
> case for hooks to interfere with reflog updates.
> 
> However, the existing ref update transaction hook does not have to
> be the mechanism to vet "git stash" operation.  If we really needed
> to, we could add reflog transaction hook for that later, outside the
> scope of this fix.

Indeed, I'm also happy to declare this a bug and change the behaviour retroactively to skip over reflogs. I highly doubt that anybody uses this in a sensible way to handle reflog updates: they have no way to distinguish a ref update from a reflog update, so they would have to essentially guess what is what.

So this patch looks good to me, thanks!

Patrick