Re: [PATCH v2 1/5] GIT-VERSION-GEN: fix overriding version via environment

[Patrick Steinhardt <ps@pks.im>]

On Fri, Dec 20, 2024 at 10:52:23AM -0500, Jeff King wrote:
> On Fri, Dec 20, 2024 at 01:22:45PM +0100, Patrick Steinhardt wrote:
> 
> > diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
> > index de0e63bdfbac263884e2ea328cc2ef11ace7a238..27f9d6a81f77248c652649ae21d0ec51b8f2d247 100755
> > --- a/GIT-VERSION-GEN
> > +++ b/GIT-VERSION-GEN
> > @@ -29,7 +29,10 @@ export GIT_CEILING_DIRECTORIES
> >  
> >  # First see if there is a version file (included in release tarballs),
> >  # then try git-describe, then default.
> > -if test -f "$SOURCE_DIR"/version
> > +if test -n "$GIT_VERSION"
> > +then
> > +    VN="$GIT_VERSION"
> > +elif test -f "$SOURCE_DIR"/version
> 
> Hmm. If $GIT_VERSION is set, then we set $VN here...
> 
> > -GIT_VERSION=$(expr "$VN" : v*'\(.*\)')
> > +# Only strip leading `v` in case we have derived VN manually. Otherwise we
> > +# retain whatever the user has set in their environment.
> > +if test -z "$GIT_VERSION"
> > +then
> > +    GIT_VERSION=$(expr "$VN" : v*'\(.*\)')
> > +fi
> 
> ...but later we ignore $VN completely.
> 
> So it would work equally well with the first hunk dropped completely.
> However, having an entry in the cascading if/else does mean that we
> short-circuit the effort to run git-describe, etc.
> 
> I don't think the old code ever did that (we'd generate the Makefile
> snippet in GIT-VERSION-FILE, read it back, and then make would still
> override the value from the snippet).
> 
> So I dunno. I like keeping things simple, but I also like skipping
> unnecessary code, too. Maybe if the top hunk were:
> 
>   if test -n "$GIT_VERSION"
>   then
>     : do nothing, we will use this value verbatim
>   elif ...
> 
> that would make the intended flow more obvious.
> 
> There are probably other ways to structure it, too. The whole $VN thing
> could be inside the:
> 
>   if test -z "$GIT_VERSION"
> 
> block. Or alternatively, if each block of the if/else just ran expr and
> set $GIT_VERSION itself (perhaps with a one-liner helper function) then
> we wouldn't need $VN at all.
> 
> I don't know how much trouble it's worth to refactor all this. Mostly I
> was just surprised to see the first hunk at all in this version.

I think wrapping it all in `if test -z "$GIT_VERSION"` would be best. I
was thinking about whether to do that refactoring, but I shied away from
it due to the required reindentation. But I agree that the end result is
a bit on the awkward side.

You know, let me send another iteration where I just do it, now that
we're two having the same thought.

Patrick