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

[Patrick Steinhardt <ps@pks.im>]

On Fri, Nov 15, 2024 at 08:48:12AM +0900, Junio C Hamano wrote:
> 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