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

From: Jeff King <peff@peff.net>
Date: Dec 20, 2024, 15:52 UTC
Message-ID: <20241220155223.GA152570@coredump.intra.peff.net>
In-Reply-To: <20241220-b4-pks-git-version-via-environment-v2-1-f1457a5e8c38@pks.im>

On Fri, Dec 20, 2024 at 01:22:45PM +0100, Patrick Steinhardt wrote:

Show 13 quoted lines

> 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...

Show 7 quoted lines

> -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.

-Peff