Re: [PATCH] githooks.txt: Clarify documentation on reference-transaction hook

From: Patrick Steinhardt <ps@pks.im>
Date: Mar 1, 2021, 09:33 UTC
Message-ID: <YDy05NMxhfSKbGtS@ncase>
In-Reply-To: <YDiPP3Ml73igpT7Q@coredump.intra.peff.net>

On Fri, Feb 26, 2021 at 01:03:43AM -0500, Jeff King wrote:

Show 17 quoted lines

> On Thu, Feb 25, 2021 at 06:50:12AM +0100, Patrick Steinhardt wrote:
> 
> > The reference-transaction hook doesn't clearly document its scope and
> > what values it receives as input. Document it to make it less surprising
> > and clearly delimit its (current) scope.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > 
> > I've been postponing doing this simple doc update for far too long, but
> > here it finally is. It simply clarifies its current workings and
> > limitations without changing anything. This is not supposed to be a "We
> > don't want it to ever cover symrefs", but rather to avoid confusion.
> 
> I think that's a good step forward. We might want to say "does not cover
> symbolic references (but that may change in the future)" to make it
> clear that nothing is definite.

Fair, I'll add that.

> OTOH, I suspect adding them would require a change to the hook's stdin
> format, so it is not like a hook could be written in a way to magically
> handle them if things change in the future.

Yeah. Hindsight is 20/20, but this should've used some kind of prefix with the explicitly stated hint that additional prefixed can be added in the future. I've got myself to blame for that, I should've known better to make this more readily extensible.

Show 15 quoted lines

> > @@ -492,6 +493,13 @@ 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 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-value>` is 40 `0`. To distinguish these cases,
> > +you can inspect the current value of `<ref-name>` via `git rev-parse`.
> 
> We should probably avoid saying "40" here. Maybe "all zeroes" or
> something.
> 
> -Peff

I wasn't completely sure how to word this and thus opted to do the same as the other hooks. Most notably, both pre-push and pre-receive hook explicitly call out SHA-1 and the 40-character thingy.

Maybe I'll just add a patch on top to also change those to instead say "the all-zero object ID" or something similar.

Patrick