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
