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
