From: Jeff King <peff@peff.net>
Date: Fri, 14 Apr 2023 21:35:37 GMT
Subject: Re: [PATCH] doc: set actual revdate for manpages
Message-ID: <20230414213537.GA640081@coredump.intra.peff.net>
In-Reply-To: <643949a57396c_5b7e62948d@chronos.notmuch>

On Fri, Apr 14, 2023 at 06:40:05AM -0600, Felipe Contreras wrote:

> > It would be nicer to populate the date variable in that case, like we do
> > for GIT_VERSION. I think that could look something like this:
> 
> Indeed, that would be better, but that totally can be done in a separate patch,
> or a separate series even.
> 
> The perfect doesn't have to be the enemy of the good, and that can be done
> later.
> 
> In the meantime something is better than nothing.

Sometimes, as long as it is not making the status quo worse in the
meantime. In this case, if the tarball build continues to use the
current date (which it appears to), then it's not changing. It does
become inconsistent with the output you get from building in-repo, which
is unfortunate. But that might be acceptable.

If so, it would make sense to document that decision in the commit
message.  We'd also want to suppress stderr from the Git invocation so
that tarball builders don't see a confusing error message. And probably
we should avoid using the "git show" porcelain in a script. I think:

  git rev-list -1 --no-commit-header --format=%as HEAD

would do what you want (or %cs; I don't have a strong opinion there).

> > --- a/GIT-VERSION-GEN
> > +++ b/GIT-VERSION-GEN
> > @@ -10,7 +10,8 @@ LF='
> >  # then try git-describe, then default.
> >  if test -f version
> >  then
> > -	VN=$(cat version) || VN="$DEF_VER"
> > +	VN=$(cut -d" " -f1 version) || VN="$DEF_VER"
> > +	DN=$(cut -d" " -f2 version) || DN=""
> 
> Although this works, I have an issue with doing multiple unnecessary forks.
> 
> This does the same, no?
> 
>   read VN DN <version

Yeah, that should be fine. The original could probably be using "read"
in the first place. In general it's worth micro-optimizing this script
because it runs on every single "make" invocation, though this code path
in particular is only run for tarball builders (who presumably build a
lot less than developers).

It might also be worth just putting the two fields in two separate
files, if there's any question of spaces in the version field.

-Peff