[PATCH 0/2] GIT-VERSION-GEN: fix overriding values

[PATCH 0/2] GIT-VERSION-GEN: fix overriding values

[Patrick Steinhardt]

Hi,

Peff reported that overriding GIT_VERSION and GIT_DATE broke recently
due to the refactoring of GIT-VERSION-GEN. This small commit series
fixes those cases, but also fixes the equivalent issue with
GIT_BUILT_FROM_COMMIT.

Thanks!

Patrick

---
Patrick Steinhardt (2):
      GIT-VERSION-GEN: fix overriding version via environment
      GIT-VERSION-GEN: fix overriding GIT_BUILT_FROM_COMMIT and GIT_DATE

 GIT-VERSION-GEN | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)


---
base-commit: d882f382b3d939d90cfa58d17b17802338f05d66
change-id: 20241219-b4-pks-git-version-via-environment-035490abec26

[PATCH 1/2] GIT-VERSION-GEN: fix overriding version via environment

[Patrick Steinhardt]

GIT-VERSION-GEN tries to derive the version that Git is being built from
via multiple different sources in the following order:

  1. A file called "version" in the source tree's root directory, if it
     exists.

  2. The current commit in case Git is built from a Git repository.

  3. Otherwise, we use a fallback version stored in a variable which is
     bumped whenever a new Git version is getting tagged.

It used to be possible to override the version by overriding the
`GIT_VERSION` Makefile variable (e.g. `make GIT_VERSION=foo`). This
worked somewhat by chance, only: `GIT-VERSION-GEN` would write the
actual Git version into `GIT-VERSION-FILE`, not the overridden value,
but when including the file into our Makefile we would not override the
`GIT_VERSION` variable because it has already been set by the user. And
because our Makefile used the variable to propagate the version to our
build tools instead of using `GIT-VERSION-FILE` the resulting build
artifacts used the overridden version.

But that subtle mechanism broke with 4838deab65 (Makefile: refactor
GIT-VERSION-GEN to be reusable, 2024-12-06) and subsequent commits
because the version information is not propagated via the Makefile
variable anymore, but instead via the files that `GIT-VERSION-GEN`
started to write. And as the script never knew about the `GIT_VERSION`
environment variable in the first place it uses one of the values listed
above instead of the overridden value.

Fix this issue by making `GIT-VERSION-GEN` handle the case where
`GIT_VERSION` has been set via the environment.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 GIT-VERSION-GEN | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index de0e63bdfbac263884e2ea328cc2ef11ace7a238..787c6cfd04f0a43d0c1c8a6690185d26ccf2fc2f 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
 then
 	VN=$(cat "$SOURCE_DIR"/version) || VN="$DEF_VER"
 elif {

-- 
2.47.0

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> worked somewhat by chance, only: ...
>
> But that subtle mechanism broke with 4838deab65 (Makefile: refactor
> GIT-VERSION-GEN to be reusable, 2024-12-06) and subsequent commits

With such a nice analysis, it does not look like it was "by chance"
working, though ;-)  And ...

> because the version information is not propagated via the Makefile
> variable anymore, but instead via the files that `GIT-VERSION-GEN`
> started to write. And as the script never knew about the `GIT_VERSION`
> environment variable in the first place it uses one of the values listed
> above instead of the overridden value.
>
> Fix this issue by making `GIT-VERSION-GEN` handle the case where
> `GIT_VERSION` has been set via the environment.

... the "fix" sounds very much the logical and only correct
solution.

Thanks, queued.

>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  GIT-VERSION-GEN | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
> index de0e63bdfbac263884e2ea328cc2ef11ace7a238..787c6cfd04f0a43d0c1c8a6690185d26ccf2fc2f 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
>  then
>  	VN=$(cat "$SOURCE_DIR"/version) || VN="$DEF_VER"
>  elif {

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

[Jeff King]

On Thu, Dec 19, 2024 at 04:53:36PM +0100, Patrick Steinhardt wrote:

> But that subtle mechanism broke with 4838deab65 (Makefile: refactor
> GIT-VERSION-GEN to be reusable, 2024-12-06) and subsequent commits
> because the version information is not propagated via the Makefile
> variable anymore, but instead via the files that `GIT-VERSION-GEN`
> started to write. And as the script never knew about the `GIT_VERSION`
> environment variable in the first place it uses one of the values listed
> above instead of the overridden value.
> 
> Fix this issue by making `GIT-VERSION-GEN` handle the case where
> `GIT_VERSION` has been set via the environment.

I think this is the right direction, but there are two subtleties we
might want to address. The first is that you are adjusting $VN and not
$GIT_VERSION itself:

> diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
> index de0e63bdfbac263884e2ea328cc2ef11ace7a238..787c6cfd04f0a43d0c1c8a6690185d26ccf2fc2f 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
>  then
>  	VN=$(cat "$SOURCE_DIR"/version) || VN="$DEF_VER"
>  elif {

Later we process $VN into $GIT_VERSION, but after removing the leading
"v" (which would usually be there from the tag name):

  GIT_VERSION=$(expr "$VN" : v*'\(.*\)')

So if I do:

  make GIT_VERSION=very-special

with v2.47 I'd end up with the version "very-special". But after your
patch, it is "ery-special".

I'd guess it's unlikely to come up in practice, but if we are trying to
restore the old behavior, that's one difference.


The second is that the value is read from the environment, but make will
not always put its variables into the environment. Ones given on the
command line are, so:

  make GIT_VERSION=foo

works as before. But:

  echo 'GIT_VERSION = foo' >config.mak
  make

will not, as the variable isn't set in the environment. The invocation
of GIT-VERSION-GEN already passes along GIT_USER_AGENT explicitly:

  version-def.h: version-def.h.in GIT-VERSION-GEN GIT-VERSION-FILE GIT-USER-AGENT
          $(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" $< $@+
          @if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi

Should we do the same thing for GIT_VERSION? And GIT_DATE, etc? If we're
going to do many of these, it might also be easier to just add "export
GIT_VERSION", etc, in the Makefile.

-Peff

PS I don't know if meson.build would need something similar. It does not
   even pass through GIT_USER_AGENT now.

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

[Patrick Steinhardt]

On Fri, Dec 20, 2024 at 02:34:37AM -0500, Jeff King wrote:
> On Thu, Dec 19, 2024 at 04:53:36PM +0100, Patrick Steinhardt wrote:
> > diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
> > index de0e63bdfbac263884e2ea328cc2ef11ace7a238..787c6cfd04f0a43d0c1c8a6690185d26ccf2fc2f 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
> >  then
> >  	VN=$(cat "$SOURCE_DIR"/version) || VN="$DEF_VER"
> >  elif {
> 
> Later we process $VN into $GIT_VERSION, but after removing the leading
> "v" (which would usually be there from the tag name):
> 
>   GIT_VERSION=$(expr "$VN" : v*'\(.*\)')
> 
> So if I do:
> 
>   make GIT_VERSION=very-special
> 
> with v2.47 I'd end up with the version "very-special". But after your
> patch, it is "ery-special".
> 
> I'd guess it's unlikely to come up in practice, but if we are trying to
> restore the old behavior, that's one difference.

Ah, indeed, will fix.

> The second is that the value is read from the environment, but make will
> not always put its variables into the environment. Ones given on the
> command line are, so:
> 
>   make GIT_VERSION=foo
> 
> works as before. But:
> 
>   echo 'GIT_VERSION = foo' >config.mak
>   make
> 
> will not, as the variable isn't set in the environment. The invocation
> of GIT-VERSION-GEN already passes along GIT_USER_AGENT explicitly:
> 
>   version-def.h: version-def.h.in GIT-VERSION-GEN GIT-VERSION-FILE GIT-USER-AGENT
>           $(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" $< $@+
>           @if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
> 
> Should we do the same thing for GIT_VERSION? And GIT_DATE, etc? If we're
> going to do many of these, it might also be easier to just add "export
> GIT_VERSION", etc, in the Makefile.

I guess. It'll become quite painful to do this at every callsite, so
I'll add another commit on top to introduce a call template that does
all of this for us.

> PS I don't know if meson.build would need something similar. It does not
>    even pass through GIT_USER_AGENT now.

It's easy enough to do, so why not?

Patrick

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

[Jeff King]

On Fri, Dec 20, 2024 at 09:45:36AM +0100, Patrick Steinhardt wrote:

> >   version-def.h: version-def.h.in GIT-VERSION-GEN GIT-VERSION-FILE GIT-USER-AGENT
> >           $(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" $< $@+
> >           @if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
> > 
> > Should we do the same thing for GIT_VERSION? And GIT_DATE, etc? If we're
> > going to do many of these, it might also be easier to just add "export
> > GIT_VERSION", etc, in the Makefile.
> 
> I guess. It'll become quite painful to do this at every callsite, so
> I'll add another commit on top to introduce a call template that does
> all of this for us.

Is there any reason not to just do:

  export GIT_VERSION
  export GIT_DATE
  export GIT_BUILT_FROM_COMMIT
  export GIT_USER_AGENT

in shared.mak? Then you only have to do it once, and no need for
templates.

-Peff

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

[Patrick Steinhardt]

On Fri, Dec 20, 2024 at 03:56:26AM -0500, Jeff King wrote:
> On Fri, Dec 20, 2024 at 09:45:36AM +0100, Patrick Steinhardt wrote:
> 
> > >   version-def.h: version-def.h.in GIT-VERSION-GEN GIT-VERSION-FILE GIT-USER-AGENT
> > >           $(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" $< $@+
> > >           @if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
> > > 
> > > Should we do the same thing for GIT_VERSION? And GIT_DATE, etc? If we're
> > > going to do many of these, it might also be easier to just add "export
> > > GIT_VERSION", etc, in the Makefile.
> > 
> > I guess. It'll become quite painful to do this at every callsite, so
> > I'll add another commit on top to introduce a call template that does
> > all of this for us.
> 
> Is there any reason not to just do:
> 
>   export GIT_VERSION
>   export GIT_DATE
>   export GIT_BUILT_FROM_COMMIT
>   export GIT_USER_AGENT
> 
> in shared.mak? Then you only have to do it once, and no need for
> templates.

You could do that, yeah, but the user needs to be aware that they can.
I'm happy to not go down that path and live with the above solution.
Alternatively, this would be what the call template would look like.

Patrick

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 3392e1ce7e..a7cb885b67 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -211,12 +211,10 @@ XMLTO_EXTRA += --skip-validation
 XMLTO_EXTRA += -x manpage.xsl
 
 asciidoctor-extensions.rb: asciidoctor-extensions.rb.in FORCE
-	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@+
-	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
+	$(QUIET_GEN)$(call version_gen,$(shell pwd)/..,$<,$@)
 else
 asciidoc.conf: asciidoc.conf.in FORCE
-	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@+
-	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
+	$(QUIET_GEN)$(call version_gen,$(shell pwd)/..,$<,$@)
 endif
 
 ASCIIDOC_DEPS += docinfo.html
diff --git a/Makefile b/Makefile
index 79739a13d2..9cfe3d0aa9 100644
--- a/Makefile
+++ b/Makefile
@@ -593,7 +593,7 @@ include shared.mak
 
 GIT-VERSION-FILE: FORCE
 	@OLD=$$(cat $@ 2>/dev/null || :) && \
-	$(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" GIT-VERSION-FILE.in $@ && \
+	$(call version_gen,"$(shell pwd)",GIT-VERSION-FILE.in,$@) && \
 	NEW=$$(cat $@ 2>/dev/null || :) && \
 	if test "$$OLD" != "$$NEW"; then echo "$$NEW" >&2; fi
 -include GIT-VERSION-FILE
@@ -2512,8 +2512,7 @@ pager.sp pager.s pager.o: EXTRA_CPPFLAGS = \
 	-DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'
 
 version-def.h: version-def.h.in GIT-VERSION-GEN GIT-VERSION-FILE GIT-USER-AGENT
-	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" $< $@+
-	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
+	$(QUIET_GEN)$(call version_gen,"$(shell pwd)",$<,$@)
 
 version.sp version.s version.o: version-def.h
 
@@ -2554,8 +2553,7 @@ $(SCRIPT_SH_GEN) $(SCRIPT_LIB) : % : %.sh generate-script.sh GIT-BUILD-OPTIONS G
 	mv $@+ $@
 
 git.rc: git.rc.in GIT-VERSION-GEN GIT-VERSION-FILE
-	$(QUIET_GEN)$(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" $< $@+
-	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
+	$(QUIET_GEN)$(call version_gen,"$(shell pwd)",$<,$@)
 
 git.res: git.rc GIT-PREFIX
 	$(QUIET_RC)$(RC) -i $< -o $@
diff --git a/shared.mak b/shared.mak
index 29bebd30d8..8e0a19691f 100644
--- a/shared.mak
+++ b/shared.mak
@@ -116,3 +116,14 @@ endef
 define libpath_template
 -L$(1) $(if $(filter-out -L,$(CC_LD_DYNPATH)),$(CC_LD_DYNPATH)$(1))
 endef
+
+# Populate build information into a file via GIT-VERSION-GEN. Requires the
+# absolute path to the root source directory as well as input and output files
+# as arguments, in that order.
+define version_gen
+GIT_BUILT_FROM_COMMIT="$(GIT_BUILT_FROM_COMMIT)" \
+GIT_DATE="$(GIT_DATE)" \
+GIT_USER_AGENT="$(GIT_USER_AGENT)" \
+GIT_VERSION="$(GIT_VERSION)" \
+$(SHELL_PATH) "$(1)/GIT-VERSION-GEN" "$(1)" "$(2)" "$(3)"
+endef

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

[Jeff King]

On Fri, Dec 20, 2024 at 10:31:30AM +0100, Patrick Steinhardt wrote:

> > > I guess. It'll become quite painful to do this at every callsite, so
> > > I'll add another commit on top to introduce a call template that does
> > > all of this for us.
> > 
> > Is there any reason not to just do:
> > 
> >   export GIT_VERSION
> >   export GIT_DATE
> >   export GIT_BUILT_FROM_COMMIT
> >   export GIT_USER_AGENT
> > 
> > in shared.mak? Then you only have to do it once, and no need for
> > templates.
> 
> You could do that, yeah, but the user needs to be aware that they can.
> I'm happy to not go down that path and live with the above solution.
> Alternatively, this would be what the call template would look like.

I meant that _we_ would mark those variables for export ourselves (in
shared.mak, which is used by all of our Makefiles, not the user's
config.mak).

I.e., this:

diff --git a/shared.mak b/shared.mak
index 29bebd30d8..4aa7dbf5e0 100644
--- a/shared.mak
+++ b/shared.mak
@@ -116,3 +116,5 @@ endef
 define libpath_template
 -L$(1) $(if $(filter-out -L,$(CC_LD_DYNPATH)),$(CC_LD_DYNPATH)$(1))
 endef
+
+export GIT_VERSION

which makes:

  echo 'GIT_VERSION = foo' >config.mak
  make

behave as it used to (when coupled with the fix you already sent to
respect the variable within the version-gen script).

-Peff

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

[Patrick Steinhardt]

On Fri, Dec 20, 2024 at 06:17:16AM -0500, Jeff King wrote:
> On Fri, Dec 20, 2024 at 10:31:30AM +0100, Patrick Steinhardt wrote:
> 
> > > > I guess. It'll become quite painful to do this at every callsite, so
> > > > I'll add another commit on top to introduce a call template that does
> > > > all of this for us.
> > > 
> > > Is there any reason not to just do:
> > > 
> > >   export GIT_VERSION
> > >   export GIT_DATE
> > >   export GIT_BUILT_FROM_COMMIT
> > >   export GIT_USER_AGENT
> > > 
> > > in shared.mak? Then you only have to do it once, and no need for
> > > templates.
> > 
> > You could do that, yeah, but the user needs to be aware that they can.
> > I'm happy to not go down that path and live with the above solution.
> > Alternatively, this would be what the call template would look like.
> 
> I meant that _we_ would mark those variables for export ourselves (in
> shared.mak, which is used by all of our Makefiles, not the user's
> config.mak).
> 
> I.e., this:
> 
> diff --git a/shared.mak b/shared.mak
> index 29bebd30d8..4aa7dbf5e0 100644
> --- a/shared.mak
> +++ b/shared.mak
> @@ -116,3 +116,5 @@ endef
>  define libpath_template
>  -L$(1) $(if $(filter-out -L,$(CC_LD_DYNPATH)),$(CC_LD_DYNPATH)$(1))
>  endef
> +
> +export GIT_VERSION
> 
> which makes:
> 
>   echo 'GIT_VERSION = foo' >config.mak
>   make
> 
> behave as it used to (when coupled with the fix you already sent to
> respect the variable within the version-gen script).

Ah, I misread "shared.mak" and thought you meant "config.mak". That
makes more sense then, thanks!

Patrick

[PATCH 2/2] GIT-VERSION-GEN: fix overriding GIT_BUILT_FROM_COMMIT and GIT_DATE

[Patrick Steinhardt]

Same as with the preceding commit, neither GIT_BUILT_FROM_COMMIT nor
GIT_DATE can be overridden via the environment. Especially the latter is
of importance given that we set it in our own "Documentation/doc-diff"
script.

Make the values of both variables overridable.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 GIT-VERSION-GEN | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index 787c6cfd04f0a43d0c1c8a6690185d26ccf2fc2f..f8367f6d09ff2ada8868e575d6ec8f1f9b27534d 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN
@@ -53,10 +53,18 @@ then
 else
 	VN="$DEF_VER"
 fi
-
 GIT_VERSION=$(expr "$VN" : v*'\(.*\)')
-GIT_BUILT_FROM_COMMIT=$(git -C "$SOURCE_DIR" rev-parse -q --verify HEAD 2>/dev/null)
-GIT_DATE=$(git -C "$SOURCE_DIR" show --quiet --format='%as' 2>/dev/null)
+
+if test -z "$GIT_BUILT_FROM_COMMIT"
+then
+    GIT_BUILT_FROM_COMMIT=$(git -C "$SOURCE_DIR" rev-parse -q --verify HEAD 2>/dev/null)
+fi
+
+if test -z "$GIT_DATE"
+then
+    GIT_DATE=$(git -C "$SOURCE_DIR" show --quiet --format='%as' 2>/dev/null)
+fi
+
 if test -z "$GIT_USER_AGENT"
 then
 	GIT_USER_AGENT="git/$GIT_VERSION"

-- 
2.47.0

Re: [PATCH 2/2] GIT-VERSION-GEN: fix overriding GIT_BUILT_FROM_COMMIT and GIT_DATE

[Kyle Lippincott]

On Thu, Dec 19, 2024 at 7:55 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> Same as with the preceding commit, neither GIT_BUILT_FROM_COMMIT nor
> GIT_DATE can be overridden via the environment. Especially the latter is
> of importance given that we set it in our own "Documentation/doc-diff"
> script.
>
> Make the values of both variables overridable.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  GIT-VERSION-GEN | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)

Looks good, thanks for fixing this and for all the work done on the
cleanups for the build system changes.

>
> diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
> index 787c6cfd04f0a43d0c1c8a6690185d26ccf2fc2f..f8367f6d09ff2ada8868e575d6ec8f1f9b27534d 100755
> --- a/GIT-VERSION-GEN
> +++ b/GIT-VERSION-GEN
> @@ -53,10 +53,18 @@ then
>  else
>         VN="$DEF_VER"
>  fi
> -
>  GIT_VERSION=$(expr "$VN" : v*'\(.*\)')
> -GIT_BUILT_FROM_COMMIT=$(git -C "$SOURCE_DIR" rev-parse -q --verify HEAD 2>/dev/null)
> -GIT_DATE=$(git -C "$SOURCE_DIR" show --quiet --format='%as' 2>/dev/null)
> +
> +if test -z "$GIT_BUILT_FROM_COMMIT"
> +then
> +    GIT_BUILT_FROM_COMMIT=$(git -C "$SOURCE_DIR" rev-parse -q --verify HEAD 2>/dev/null)
> +fi
> +
> +if test -z "$GIT_DATE"
> +then
> +    GIT_DATE=$(git -C "$SOURCE_DIR" show --quiet --format='%as' 2>/dev/null)
> +fi
> +
>  if test -z "$GIT_USER_AGENT"
>  then
>         GIT_USER_AGENT="git/$GIT_VERSION"
>
> --
> 2.47.0
>

Re: [PATCH 2/2] GIT-VERSION-GEN: fix overriding GIT_BUILT_FROM_COMMIT and GIT_DATE

[Junio C Hamano]

Kyle Lippincott <spectral@google.com> writes:

> On Thu, Dec 19, 2024 at 7:55 AM Patrick Steinhardt <ps@pks.im> wrote:
>>
>> Same as with the preceding commit, neither GIT_BUILT_FROM_COMMIT nor
>> GIT_DATE can be overridden via the environment. Especially the latter is
>> of importance given that we set it in our own "Documentation/doc-diff"
>> script.
>>
>> Make the values of both variables overridable.
>>
>> Signed-off-by: Patrick Steinhardt <ps@pks.im>
>> ---
>>  GIT-VERSION-GEN | 14 +++++++++++---
>>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> Looks good, thanks for fixing this and for all the work done on the
> cleanups for the build system changes.

Thanks, all.

Re: [PATCH 2/2] GIT-VERSION-GEN: fix overriding GIT_BUILT_FROM_COMMIT and GIT_DATE

[Jeff King]

On Thu, Dec 19, 2024 at 04:53:37PM +0100, Patrick Steinhardt wrote:

>  GIT_VERSION=$(expr "$VN" : v*'\(.*\)')
> -GIT_BUILT_FROM_COMMIT=$(git -C "$SOURCE_DIR" rev-parse -q --verify HEAD 2>/dev/null)
> -GIT_DATE=$(git -C "$SOURCE_DIR" show --quiet --format='%as' 2>/dev/null)
> +
> +if test -z "$GIT_BUILT_FROM_COMMIT"
> +then
> +    GIT_BUILT_FROM_COMMIT=$(git -C "$SOURCE_DIR" rev-parse -q --verify HEAD 2>/dev/null)
> +fi
> +
> +if test -z "$GIT_DATE"
> +then
> +    GIT_DATE=$(git -C "$SOURCE_DIR" show --quiet --format='%as' 2>/dev/null)
> +fi

Looks good. I doubt anybody would want to override BUILT_FROM_COMMIT
(and it was never possible to do so, even before your recent patches),
but it's reasonable to include it as well.

-Peff

Re: [PATCH 2/2] GIT-VERSION-GEN: fix overriding GIT_BUILT_FROM_COMMIT and GIT_DATE

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Dec 19, 2024 at 04:53:37PM +0100, Patrick Steinhardt wrote:
>
>>  GIT_VERSION=$(expr "$VN" : v*'\(.*\)')
>> -GIT_BUILT_FROM_COMMIT=$(git -C "$SOURCE_DIR" rev-parse -q --verify HEAD 2>/dev/null)
>> -GIT_DATE=$(git -C "$SOURCE_DIR" show --quiet --format='%as' 2>/dev/null)
>> +
>> +if test -z "$GIT_BUILT_FROM_COMMIT"
>> +then
>> +    GIT_BUILT_FROM_COMMIT=$(git -C "$SOURCE_DIR" rev-parse -q --verify HEAD 2>/dev/null)
>> +fi
>> +
>> +if test -z "$GIT_DATE"
>> +then
>> +    GIT_DATE=$(git -C "$SOURCE_DIR" show --quiet --format='%as' 2>/dev/null)
>> +fi
>
> Looks good. I doubt anybody would want to override BUILT_FROM_COMMIT
> (and it was never possible to do so, even before your recent patches),
> but it's reasonable to include it as well.

Thanks, both.

[PATCH v2 0/5] GIT-VERSION-GEN: fix overriding values

[Patrick Steinhardt]

Hi,

Peff reported that overriding GIT_VERSION and GIT_DATE broke recently
due to the refactoring of GIT-VERSION-GEN. This small commit series
fixes those cases, but also fixes the equivalent issue with
GIT_BUILT_FROM_COMMIT.

Changes in v2:

  - Don't strip leading `v`s when `GIT_VERSION` was set explicitly.
  - Allow setting build info via "config.mak" again.
  - Wire up build info options for Meson.
  - Link to v1: https://lore.kernel.org/r/20241219-b4-pks-git-version-via-environment-v1-0-9393af058240@pks.im

Thanks!

Patrick

---
Patrick Steinhardt (5):
      GIT-VERSION-GEN: fix overriding version via environment
      GIT-VERSION-GEN: fix overriding GIT_BUILT_FROM_COMMIT and GIT_DATE
      Makefile: drop unneeded indirection for GIT-VERSION-GEN outputs
      Makefile: respect build info declared in "config.mak"
      meson: add options to override build information

 Documentation/Makefile    |  6 ++----
 Documentation/meson.build |  1 +
 GIT-VERSION-GEN           | 25 +++++++++++++++++++++----
 Makefile                  |  6 ++----
 meson.build               | 13 +++++++++++++
 meson_options.txt         | 10 ++++++++++
 shared.mak                |  7 +++++++
 7 files changed, 56 insertions(+), 12 deletions(-)

Range-diff versus v1:

1:  3560d0934f ! 1:  f9aabaa9b7 GIT-VERSION-GEN: fix overriding version via environment
    @@ GIT-VERSION-GEN: export GIT_CEILING_DIRECTORIES
      then
      	VN=$(cat "$SOURCE_DIR"/version) || VN="$DEF_VER"
      elif {
    +@@ GIT-VERSION-GEN: else
    + 	VN="$DEF_VER"
    + fi
    + 
    +-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
    ++
    + GIT_BUILT_FROM_COMMIT=$(git -C "$SOURCE_DIR" rev-parse -q --verify HEAD 2>/dev/null)
    + GIT_DATE=$(git -C "$SOURCE_DIR" show --quiet --format='%as' 2>/dev/null)
    + if test -z "$GIT_USER_AGENT"
2:  ec7477d14e ! 2:  2db637757e GIT-VERSION-GEN: fix overriding GIT_BUILT_FROM_COMMIT and GIT_DATE
    @@ Commit message
     
      ## GIT-VERSION-GEN ##
     @@ GIT-VERSION-GEN: then
    - else
    - 	VN="$DEF_VER"
    +     GIT_VERSION=$(expr "$VN" : v*'\(.*\)')
      fi
    --
    - GIT_VERSION=$(expr "$VN" : v*'\(.*\)')
    + 
     -GIT_BUILT_FROM_COMMIT=$(git -C "$SOURCE_DIR" rev-parse -q --verify HEAD 2>/dev/null)
     -GIT_DATE=$(git -C "$SOURCE_DIR" show --quiet --format='%as' 2>/dev/null)
    -+
     +if test -z "$GIT_BUILT_FROM_COMMIT"
     +then
     +    GIT_BUILT_FROM_COMMIT=$(git -C "$SOURCE_DIR" rev-parse -q --verify HEAD 2>/dev/null)
-:  ---------- > 3:  1024d10b46 Makefile: drop unneeded indirection for GIT-VERSION-GEN outputs
-:  ---------- > 4:  14a2e5d7bb Makefile: respect build info declared in "config.mak"
-:  ---------- > 5:  974ab29b35 meson: add options to override build information

---
base-commit: d882f382b3d939d90cfa58d17b17802338f05d66
change-id: 20241219-b4-pks-git-version-via-environment-035490abec26

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

[Patrick Steinhardt]

GIT-VERSION-GEN tries to derive the version that Git is being built from
via multiple different sources in the following order:

  1. A file called "version" in the source tree's root directory, if it
     exists.

  2. The current commit in case Git is built from a Git repository.

  3. Otherwise, we use a fallback version stored in a variable which is
     bumped whenever a new Git version is getting tagged.

It used to be possible to override the version by overriding the
`GIT_VERSION` Makefile variable (e.g. `make GIT_VERSION=foo`). This
worked somewhat by chance, only: `GIT-VERSION-GEN` would write the
actual Git version into `GIT-VERSION-FILE`, not the overridden value,
but when including the file into our Makefile we would not override the
`GIT_VERSION` variable because it has already been set by the user. And
because our Makefile used the variable to propagate the version to our
build tools instead of using `GIT-VERSION-FILE` the resulting build
artifacts used the overridden version.

But that subtle mechanism broke with 4838deab65 (Makefile: refactor
GIT-VERSION-GEN to be reusable, 2024-12-06) and subsequent commits
because the version information is not propagated via the Makefile
variable anymore, but instead via the files that `GIT-VERSION-GEN`
started to write. And as the script never knew about the `GIT_VERSION`
environment variable in the first place it uses one of the values listed
above instead of the overridden value.

Fix this issue by making `GIT-VERSION-GEN` handle the case where
`GIT_VERSION` has been set via the environment.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 GIT-VERSION-GEN | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

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
 then
 	VN=$(cat "$SOURCE_DIR"/version) || VN="$DEF_VER"
 elif {
@@ -51,7 +54,13 @@ else
 	VN="$DEF_VER"
 fi
 
-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
+
 GIT_BUILT_FROM_COMMIT=$(git -C "$SOURCE_DIR" rev-parse -q --verify HEAD 2>/dev/null)
 GIT_DATE=$(git -C "$SOURCE_DIR" show --quiet --format='%as' 2>/dev/null)
 if test -z "$GIT_USER_AGENT"

-- 
2.48.0.rc0.184.g0fc57dec57.dirty

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

[Jeff King]

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.

-Peff

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

[Patrick Steinhardt]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

True.

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

True again.  It has been quite a while since I wrote the original
before the meson topic came up and the script hasn't changed for a
long time (other than DEF_VER line for obvious reasons), but I think
in that ancient version, $VN _was_ the variable to be looked at and
GIT_VERSION did not even exist as a shell variable at all.

If $GIT_VERSION is serving the same role as old $VN in the
mesonified version, perhaps we should get rid of the $VN variable to
clarify the new world order.

> 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

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

[Patrick Steinhardt]

On Fri, Dec 20, 2024 at 08:17:14AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> True.
> 
> > 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.
> 
> True again.  It has been quite a while since I wrote the original
> before the meson topic came up and the script hasn't changed for a
> long time (other than DEF_VER line for obvious reasons), but I think
> in that ancient version, $VN _was_ the variable to be looked at and
> GIT_VERSION did not even exist as a shell variable at all.
> 
> If $GIT_VERSION is serving the same role as old $VN in the
> mesonified version, perhaps we should get rid of the $VN variable to
> clarify the new world order.

I think for now I'll keep $VN, if even just to discern the intermediate
value without the leading "v" stripped from the final version that we
have in "$GIT_VERSION". But I'll send a revised version where I make the
control flow a bit more obvious.

Thanks!

Patrick

[PATCH v2 2/5] GIT-VERSION-GEN: fix overriding GIT_BUILT_FROM_COMMIT and GIT_DATE

[Patrick Steinhardt]

Same as with the preceding commit, neither GIT_BUILT_FROM_COMMIT nor
GIT_DATE can be overridden via the environment. Especially the latter is
of importance given that we set it in our own "Documentation/doc-diff"
script.

Make the values of both variables overridable.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 GIT-VERSION-GEN | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index 27f9d6a81f77248c652649ae21d0ec51b8f2d247..4273d5284008e2787854928163e31802bf8a3d27 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN
@@ -61,8 +61,16 @@ then
     GIT_VERSION=$(expr "$VN" : v*'\(.*\)')
 fi
 
-GIT_BUILT_FROM_COMMIT=$(git -C "$SOURCE_DIR" rev-parse -q --verify HEAD 2>/dev/null)
-GIT_DATE=$(git -C "$SOURCE_DIR" show --quiet --format='%as' 2>/dev/null)
+if test -z "$GIT_BUILT_FROM_COMMIT"
+then
+    GIT_BUILT_FROM_COMMIT=$(git -C "$SOURCE_DIR" rev-parse -q --verify HEAD 2>/dev/null)
+fi
+
+if test -z "$GIT_DATE"
+then
+    GIT_DATE=$(git -C "$SOURCE_DIR" show --quiet --format='%as' 2>/dev/null)
+fi
+
 if test -z "$GIT_USER_AGENT"
 then
 	GIT_USER_AGENT="git/$GIT_VERSION"

-- 
2.48.0.rc0.184.g0fc57dec57.dirty

[PATCH v2 3/5] Makefile: drop unneeded indirection for GIT-VERSION-GEN outputs

[Patrick Steinhardt]

Some of the callsites of GIT-VERSION-GEN generate the target file with a
"+" suffix first and then move the file into place when the new contents
are different compared to the old contents. This allows us to avoid a
needless rebuild by not updating timestamps of the target file when its
contents will remain unchanged anyway.

In fact though, this exact logic is already handled in GIT-VERSION-GEN,
so doing this manually is pointless. This is a leftover from an earlier
version of 4838deab65 (Makefile: refactor GIT-VERSION-GEN to be
reusable, 2024-12-06), where the script didn't handle that logic for us.

Drop the needless indirection.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/Makefile | 6 ++----
 Makefile               | 6 ++----
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 3392e1ce7ebc540784912476847380d9c1775ac8..cee88dbda66265141b87d5e5c16bf86df22fa4ef 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -211,12 +211,10 @@ XMLTO_EXTRA += --skip-validation
 XMLTO_EXTRA += -x manpage.xsl
 
 asciidoctor-extensions.rb: asciidoctor-extensions.rb.in FORCE
-	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@+
-	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
+	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@
 else
 asciidoc.conf: asciidoc.conf.in FORCE
-	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@+
-	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
+	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@
 endif
 
 ASCIIDOC_DEPS += docinfo.html
diff --git a/Makefile b/Makefile
index 79739a13d2132204f56b1ef4ca879bd51c5164b4..695a9d9765daf864605002d572129bae7a8c4e40 100644
--- a/Makefile
+++ b/Makefile
@@ -2512,8 +2512,7 @@ pager.sp pager.s pager.o: EXTRA_CPPFLAGS = \
 	-DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'
 
 version-def.h: version-def.h.in GIT-VERSION-GEN GIT-VERSION-FILE GIT-USER-AGENT
-	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" $< $@+
-	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
+	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" $< $@
 
 version.sp version.s version.o: version-def.h
 
@@ -2554,8 +2553,7 @@ $(SCRIPT_SH_GEN) $(SCRIPT_LIB) : % : %.sh generate-script.sh GIT-BUILD-OPTIONS G
 	mv $@+ $@
 
 git.rc: git.rc.in GIT-VERSION-GEN GIT-VERSION-FILE
-	$(QUIET_GEN)$(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" $< $@+
-	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
+	$(QUIET_GEN)$(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" $< $@
 
 git.res: git.rc GIT-PREFIX
 	$(QUIET_RC)$(RC) -i $< -o $@

-- 
2.48.0.rc0.184.g0fc57dec57.dirty

Re: [PATCH v2 3/5] Makefile: drop unneeded indirection for GIT-VERSION-GEN outputs

[Jeff King]

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

> Some of the callsites of GIT-VERSION-GEN generate the target file with a
> "+" suffix first and then move the file into place when the new contents
> are different compared to the old contents. This allows us to avoid a
> needless rebuild by not updating timestamps of the target file when its
> contents will remain unchanged anyway.
> 
> In fact though, this exact logic is already handled in GIT-VERSION-GEN,
> so doing this manually is pointless. This is a leftover from an earlier
> version of 4838deab65 (Makefile: refactor GIT-VERSION-GEN to be
> reusable, 2024-12-06), where the script didn't handle that logic for us.
> 
> Drop the needless indirection.

Nice. I think when we can do stuff like this in an actual script instead
of in a Makefile, the result is more readable.

-Peff

[PATCH v2 4/5] Makefile: respect build info declared in "config.mak"

[Patrick Steinhardt]

In preceding commits we fixed that build info set via e.g. `make
GIT_VERSION=foo` didn't get propagated to GIT-VERSION-GEN. Similarly
though, setting build info via "config.mak" does not work anymore either
because the variables are only declared as Makefile variables and thus
aren't accessible by the script.

Fix the issue by exporting those variables via "shared.mak". This also
allows us to deduplicate the export of GIT_USER_AGENT.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/Makefile | 4 ++--
 Makefile               | 2 +-
 shared.mak             | 7 +++++++
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index cee88dbda66265141b87d5e5c16bf86df22fa4ef..4c652dfa14f6af2c1374e2f83d3311b36dd297c4 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -211,10 +211,10 @@ XMLTO_EXTRA += --skip-validation
 XMLTO_EXTRA += -x manpage.xsl
 
 asciidoctor-extensions.rb: asciidoctor-extensions.rb.in FORCE
-	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@
+	$(QUIET_GEN)$(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@
 else
 asciidoc.conf: asciidoc.conf.in FORCE
-	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@
+	$(QUIET_GEN)$(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@
 endif
 
 ASCIIDOC_DEPS += docinfo.html
diff --git a/Makefile b/Makefile
index 695a9d9765daf864605002d572129bae7a8c4e40..788f6ee1721850e75f19fe2181f578ad2e783043 100644
--- a/Makefile
+++ b/Makefile
@@ -2512,7 +2512,7 @@ pager.sp pager.s pager.o: EXTRA_CPPFLAGS = \
 	-DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'
 
 version-def.h: version-def.h.in GIT-VERSION-GEN GIT-VERSION-FILE GIT-USER-AGENT
-	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" $< $@
+	$(QUIET_GEN)$(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" $< $@
 
 version.sp version.s version.o: version-def.h
 
diff --git a/shared.mak b/shared.mak
index 29bebd30d8acbce9f50661cef48ecdbae1e41f5a..8cd132b5a03b85d4eabc3819eb3d1f64d39afa47 100644
--- a/shared.mak
+++ b/shared.mak
@@ -116,3 +116,10 @@ endef
 define libpath_template
 -L$(1) $(if $(filter-out -L,$(CC_LD_DYNPATH)),$(CC_LD_DYNPATH)$(1))
 endef
+
+# Export build information so that variables defined in config.mak can be read
+# by GIT-VERSION-GEN.
+export GIT_BUILT_FROM_COMMIT
+export GIT_DATE
+export GIT_USER_AGENT
+export GIT_VERSION

-- 
2.48.0.rc0.184.g0fc57dec57.dirty

Re: [PATCH v2 4/5] Makefile: respect build info declared in "config.mak"

[Jeff King]

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

> In preceding commits we fixed that build info set via e.g. `make
> GIT_VERSION=foo` didn't get propagated to GIT-VERSION-GEN. Similarly
> though, setting build info via "config.mak" does not work anymore either
> because the variables are only declared as Makefile variables and thus
> aren't accessible by the script.
> 
> Fix the issue by exporting those variables via "shared.mak". This also
> allows us to deduplicate the export of GIT_USER_AGENT.

This looks good. It fixes the issue, and I am happy that:

>  asciidoctor-extensions.rb: asciidoctor-extensions.rb.in FORCE
> -	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@
> +	$(QUIET_GEN)$(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@

...these spots get even simpler.

-Peff

Re: [PATCH v2 4/5] Makefile: respect build info declared in "config.mak"

[Patrick Steinhardt]

On Fri, Dec 20, 2024 at 10:54:33AM -0500, Jeff King wrote:
> On Fri, Dec 20, 2024 at 01:22:48PM +0100, Patrick Steinhardt wrote:
> 
> > In preceding commits we fixed that build info set via e.g. `make
> > GIT_VERSION=foo` didn't get propagated to GIT-VERSION-GEN. Similarly
> > though, setting build info via "config.mak" does not work anymore either
> > because the variables are only declared as Makefile variables and thus
> > aren't accessible by the script.
> > 
> > Fix the issue by exporting those variables via "shared.mak". This also
> > allows us to deduplicate the export of GIT_USER_AGENT.
> 
> This looks good. It fixes the issue, and I am happy that:
> 
> >  asciidoctor-extensions.rb: asciidoctor-extensions.rb.in FORCE
> > -	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@
> > +	$(QUIET_GEN)$(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@
> 
> ...these spots get even simpler.

Meh. I just noticed that this doesn't work: we include GIT-VERSION-FILE
and export its value, and consequently any subsequent invocation of
GIT-VERSION-GEN will continue to use the value that we have in
GIT-VERSION-FILE. So it's effectively only computed the first time.

This would all be much simpler if we didn't include the file in the
first place. In Documentation/Makefile we don't indeed use it anymore.
But in the top-level Makefile we do use it to generate the name of a
couple of archives. I'll have a look there.

Patrick

Re: [PATCH v2 4/5] Makefile: respect build info declared in "config.mak"

[Jeff King]

On Fri, Dec 20, 2024 at 05:47:14PM +0100, Patrick Steinhardt wrote:

> > This looks good. It fixes the issue, and I am happy that:
> > 
> > >  asciidoctor-extensions.rb: asciidoctor-extensions.rb.in FORCE
> > > -	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@
> > > +	$(QUIET_GEN)$(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@
> > 
> > ...these spots get even simpler.
> 
> Meh. I just noticed that this doesn't work: we include GIT-VERSION-FILE
> and export its value, and consequently any subsequent invocation of
> GIT-VERSION-GEN will continue to use the value that we have in
> GIT-VERSION-FILE. So it's effectively only computed the first time.

I'm not sure what you mean.

I wondered earlier if we might into a chicken-and-egg problem like that,
but I tested and it seemed to work fine. The rule for GIT-VERSION-FILE
means we'll build it before make reads it, so that first run of it will
get the updated value. And:

  make GIT_VERSION=foo && bin-wrappers/git version
  make GIT_VERSION=bar && bin-wrappers/git version

does what you'd expect. And the docs work the same way:

  cd Documentation
  make GIT_VERSION=foo git.1 && man -l git.1
  make GIT_VERSION=bar git.1 && man -l git.1

Is there a case you found that doesn't work?

-Peff

Re: [PATCH v2 4/5] Makefile: respect build info declared in "config.mak"

[Patrick Steinhardt]

On Fri, Dec 20, 2024 at 12:51:36PM -0500, Jeff King wrote:
> On Fri, Dec 20, 2024 at 05:47:14PM +0100, Patrick Steinhardt wrote:
> 
> > > This looks good. It fixes the issue, and I am happy that:
> > > 
> > > >  asciidoctor-extensions.rb: asciidoctor-extensions.rb.in FORCE
> > > > -	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@
> > > > +	$(QUIET_GEN)$(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@
> > > 
> > > ...these spots get even simpler.
> > 
> > Meh. I just noticed that this doesn't work: we include GIT-VERSION-FILE
> > and export its value, and consequently any subsequent invocation of
> > GIT-VERSION-GEN will continue to use the value that we have in
> > GIT-VERSION-FILE. So it's effectively only computed the first time.
> 
> I'm not sure what you mean.
> 
> I wondered earlier if we might into a chicken-and-egg problem like that,
> but I tested and it seemed to work fine. The rule for GIT-VERSION-FILE
> means we'll build it before make reads it, so that first run of it will
> get the updated value. And:
> 
>   make GIT_VERSION=foo && bin-wrappers/git version
>   make GIT_VERSION=bar && bin-wrappers/git version
> 
> does what you'd expect. And the docs work the same way:
> 
>   cd Documentation
>   make GIT_VERSION=foo git.1 && man -l git.1
>   make GIT_VERSION=bar git.1 && man -l git.1
> 
> Is there a case you found that doesn't work?

Yes:

    $ make GIT-VERSION-FILE GIT_VERSION=foo
    GIT_VERSION=foo
    make: 'GIT-VERSION-FILE' is up to date.
    $ cat GIT-VERSION-FILE
    GIT_VERSION=foo

    # And now run without GIT_VERSION set.
    make: 'GIT-VERSION-FILE' is up to date.
    GIT_VERSION=foo

So the value remains "sticky" in this case. And that is true whenever
you don't set GIT_VERSION at all, we always stick with what is currently
in that file.

I've got a version now that works for all cases, but I had to use
Makefile templates and some shuffling to make it work.

Patrick

Re: [PATCH v2 4/5] Makefile: respect build info declared in "config.mak"

[Patrick Steinhardt]

On Fri, Dec 20, 2024 at 07:02:10PM +0100, Patrick Steinhardt wrote:
> On Fri, Dec 20, 2024 at 12:51:36PM -0500, Jeff King wrote:
> > On Fri, Dec 20, 2024 at 05:47:14PM +0100, Patrick Steinhardt wrote:
> > 
> > > > This looks good. It fixes the issue, and I am happy that:
> > > > 
> > > > >  asciidoctor-extensions.rb: asciidoctor-extensions.rb.in FORCE
> > > > > -	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@
> > > > > +	$(QUIET_GEN)$(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@
> > > > 
> > > > ...these spots get even simpler.
> > > 
> > > Meh. I just noticed that this doesn't work: we include GIT-VERSION-FILE
> > > and export its value, and consequently any subsequent invocation of
> > > GIT-VERSION-GEN will continue to use the value that we have in
> > > GIT-VERSION-FILE. So it's effectively only computed the first time.
> > 
> > I'm not sure what you mean.
> > 
> > I wondered earlier if we might into a chicken-and-egg problem like that,
> > but I tested and it seemed to work fine. The rule for GIT-VERSION-FILE
> > means we'll build it before make reads it, so that first run of it will
> > get the updated value. And:
> > 
> >   make GIT_VERSION=foo && bin-wrappers/git version
> >   make GIT_VERSION=bar && bin-wrappers/git version
> > 
> > does what you'd expect. And the docs work the same way:
> > 
> >   cd Documentation
> >   make GIT_VERSION=foo git.1 && man -l git.1
> >   make GIT_VERSION=bar git.1 && man -l git.1
> > 
> > Is there a case you found that doesn't work?
> 
> Yes:
> 
>     $ make GIT-VERSION-FILE GIT_VERSION=foo
>     GIT_VERSION=foo
>     make: 'GIT-VERSION-FILE' is up to date.
>     $ cat GIT-VERSION-FILE
>     GIT_VERSION=foo
> 
>     # And now run without GIT_VERSION set.
>     make: 'GIT-VERSION-FILE' is up to date.
>     GIT_VERSION=foo
> 
> So the value remains "sticky" in this case. And that is true whenever
> you don't set GIT_VERSION at all, we always stick with what is currently
> in that file.
> 
> I've got a version now that works for all cases, but I had to use
> Makefile templates and some shuffling to make it work.

The root of the problem is this [1]:

    To this end, after reading in all makefiles make will consider each
    as a goal target, in the order in which they were processed, and
    attempt to update it. If parallel builds (see Parallel Execution)
    are enabled then makefiles will be rebuilt in parallel as well.

So the Makefile will _first_ read in all includes before deciding
whether or not it needs to regenerate any of them. So we already export
the current GIT_VERSION in case "GIT-VERSION-FILE" exists at the time
where we run "GIT-VERSION-GEN".

That behaviour is quite surprising to me, but it seems to work as
designed.

Patrick

[1]: https://www.gnu.org/software/make/manual/html_node/Remaking-Makefiles.html

Re: [PATCH v2 4/5] Makefile: respect build info declared in "config.mak"

[Jeff King]

On Fri, Dec 20, 2024 at 07:02:09PM +0100, Patrick Steinhardt wrote:

> > Is there a case you found that doesn't work?
> 
> Yes:
> 
>     $ make GIT-VERSION-FILE GIT_VERSION=foo
>     GIT_VERSION=foo
>     make: 'GIT-VERSION-FILE' is up to date.
>     $ cat GIT-VERSION-FILE
>     GIT_VERSION=foo
> 
>     # And now run without GIT_VERSION set.
>     make: 'GIT-VERSION-FILE' is up to date.
>     GIT_VERSION=foo
> 
> So the value remains "sticky" in this case. And that is true whenever
> you don't set GIT_VERSION at all, we always stick with what is currently
> in that file.

Ah, right. Even though we have a recipe to build it, and make knows it
must be built (because it depends on FORCE), make will read it (and all
includes) first before executing any rules.

Something like this seems to work:

diff --git a/Makefile b/Makefile
index 788f6ee172..0eb08d98f4 100644
--- a/Makefile
+++ b/Makefile
@@ -596,7 +596,12 @@ GIT-VERSION-FILE: FORCE
 	$(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" GIT-VERSION-FILE.in $@ && \
 	NEW=$$(cat $@ 2>/dev/null || :) && \
 	if test "$$OLD" != "$$NEW"; then echo "$$NEW" >&2; fi
+# Never include it on the first read-through, only after make has tried to
+# refresh includes. We do not want the old values to pollute our new run of the
+# rule above.
+ifdef MAKE_RESTARTS
 -include GIT-VERSION-FILE
+endif
 
 # Set our default configuration.
 #


But I don't know if there are any gotchas (I did not even know about
MAKE_RESTARTS until digging in the docs looking for a solution here).
If we can stop including it as a Makefile snippet entirely, I think that
is easier to reason about.

-Peff

Re: [PATCH v2 4/5] Makefile: respect build info declared in "config.mak"

[Patrick Steinhardt]

On Fri, Dec 20, 2024 at 01:24:27PM -0500, Jeff King wrote:
> On Fri, Dec 20, 2024 at 07:02:09PM +0100, Patrick Steinhardt wrote:
> 
> > > Is there a case you found that doesn't work?
> > 
> > Yes:
> > 
> >     $ make GIT-VERSION-FILE GIT_VERSION=foo
> >     GIT_VERSION=foo
> >     make: 'GIT-VERSION-FILE' is up to date.
> >     $ cat GIT-VERSION-FILE
> >     GIT_VERSION=foo
> > 
> >     # And now run without GIT_VERSION set.
> >     make: 'GIT-VERSION-FILE' is up to date.
> >     GIT_VERSION=foo
> > 
> > So the value remains "sticky" in this case. And that is true whenever
> > you don't set GIT_VERSION at all, we always stick with what is currently
> > in that file.
> 
> Ah, right. Even though we have a recipe to build it, and make knows it
> must be built (because it depends on FORCE), make will read it (and all
> includes) first before executing any rules.
> 
> Something like this seems to work:
> 
> diff --git a/Makefile b/Makefile
> index 788f6ee172..0eb08d98f4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -596,7 +596,12 @@ GIT-VERSION-FILE: FORCE
>  	$(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" GIT-VERSION-FILE.in $@ && \
>  	NEW=$$(cat $@ 2>/dev/null || :) && \
>  	if test "$$OLD" != "$$NEW"; then echo "$$NEW" >&2; fi
> +# Never include it on the first read-through, only after make has tried to
> +# refresh includes. We do not want the old values to pollute our new run of the
> +# rule above.
> +ifdef MAKE_RESTARTS
>  -include GIT-VERSION-FILE
> +endif
>  
>  # Set our default configuration.
>  #

Oh, nifty! Playing around with it indeed seems to make things work, and
it's simpler than what I have.

> But I don't know if there are any gotchas (I did not even know about
> MAKE_RESTARTS until digging in the docs looking for a solution here).

Good question indeed. I was wondering whether Make restarts at all in
case where none of the included Makefiles change. But it very much seems
like it does.

The next question is since when the option has been available, as it's
quite, and the answer is that it has been introduced via 978819e1 (Add a
new variable: MAKE_RESTARTS, to count how many times make has re-exec'd.
When rebuilding makefiles, unset -B if MAKE_RESTARTS is >0.,
2005-06-25), which is Make v3.81. Even macOS has that to the best of my
knowledge.

It still does feel somewhat hacky in the end.

> If we can stop including it as a Makefile snippet entirely, I think that
> is easier to reason about.

I very much agree, but it's a non-trivial change. I'll leave that for a
future iteration.

I'm a bit torn now. I have a solution locally that feels less hacky, but
it requires a bit more shuffling. If the eventual goal would be to get
rid of the include in the first place it feels somewhat pointless to do
these changes.

Patrick

Re: [PATCH v2 4/5] Makefile: respect build info declared in "config.mak"

[Patrick Steinhardt]

On Fri, Dec 20, 2024 at 07:39:38PM +0100, Patrick Steinhardt wrote:
> On Fri, Dec 20, 2024 at 01:24:27PM -0500, Jeff King wrote:
> > On Fri, Dec 20, 2024 at 07:02:09PM +0100, Patrick Steinhardt wrote:
> > 
> > > > Is there a case you found that doesn't work?
> > > 
> > > Yes:
> > > 
> > >     $ make GIT-VERSION-FILE GIT_VERSION=foo
> > >     GIT_VERSION=foo
> > >     make: 'GIT-VERSION-FILE' is up to date.
> > >     $ cat GIT-VERSION-FILE
> > >     GIT_VERSION=foo
> > > 
> > >     # And now run without GIT_VERSION set.
> > >     make: 'GIT-VERSION-FILE' is up to date.
> > >     GIT_VERSION=foo
> > > 
> > > So the value remains "sticky" in this case. And that is true whenever
> > > you don't set GIT_VERSION at all, we always stick with what is currently
> > > in that file.
> > 
> > Ah, right. Even though we have a recipe to build it, and make knows it
> > must be built (because it depends on FORCE), make will read it (and all
> > includes) first before executing any rules.
> > 
> > Something like this seems to work:
> > 
> > diff --git a/Makefile b/Makefile
> > index 788f6ee172..0eb08d98f4 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -596,7 +596,12 @@ GIT-VERSION-FILE: FORCE
> >  	$(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" GIT-VERSION-FILE.in $@ && \
> >  	NEW=$$(cat $@ 2>/dev/null || :) && \
> >  	if test "$$OLD" != "$$NEW"; then echo "$$NEW" >&2; fi
> > +# Never include it on the first read-through, only after make has tried to
> > +# refresh includes. We do not want the old values to pollute our new run of the
> > +# rule above.
> > +ifdef MAKE_RESTARTS
> >  -include GIT-VERSION-FILE
> > +endif
> >  
> >  # Set our default configuration.
> >  #
> 
> Oh, nifty! Playing around with it indeed seems to make things work, and
> it's simpler than what I have.
> 
> > But I don't know if there are any gotchas (I did not even know about
> > MAKE_RESTARTS until digging in the docs looking for a solution here).
> 
> Good question indeed. I was wondering whether Make restarts at all in
> case where none of the included Makefiles change. But it very much seems
> like it does.
> 
> The next question is since when the option has been available, as it's
> quite, and the answer is that it has been introduced via 978819e1 (Add a
> new variable: MAKE_RESTARTS, to count how many times make has re-exec'd.
> When rebuilding makefiles, unset -B if MAKE_RESTARTS is >0.,
> 2005-06-25), which is Make v3.81. Even macOS has that to the best of my
> knowledge.
> 
> It still does feel somewhat hacky in the end.
> 
> > If we can stop including it as a Makefile snippet entirely, I think that
> > is easier to reason about.
> 
> I very much agree, but it's a non-trivial change. I'll leave that for a
> future iteration.
> 
> I'm a bit torn now. I have a solution locally that feels less hacky, but
> it requires a bit more shuffling. If the eventual goal would be to get
> rid of the include in the first place it feels somewhat pointless to do
> these changes.

Okay, I did find an issue where it does not work:

    $ git clean -dfx
    $ make GIT-USER-AGENT
    $ cat GIT-USER-AGENT
    git/
    $ cat GIT-VERSION-FILE
    cat: GIT-VERSION-FILE: No such file or directory

It does not generate the version file at all anymore when it's not an
explicit dependency. While I could of course add the missing dependency
I don't know whether there are any other implicit dependencies that
would be broken, as well. My gut feeling says "probably".

I'll go with my version instead.

Patrick

Re: [PATCH v2 4/5] Makefile: respect build info declared in "config.mak"

[Jeff King]

On Fri, Dec 20, 2024 at 08:07:52PM +0100, Patrick Steinhardt wrote:

> > > +# Never include it on the first read-through, only after make has tried to
> > > +# refresh includes. We do not want the old values to pollute our new run of the
> > > +# rule above.
> > > +ifdef MAKE_RESTARTS
> > >  -include GIT-VERSION-FILE
> > > +endif
> [...]
> Okay, I did find an issue where it does not work:
> 
>     $ git clean -dfx
>     $ make GIT-USER-AGENT
>     $ cat GIT-USER-AGENT
>     git/
>     $ cat GIT-VERSION-FILE
>     cat: GIT-VERSION-FILE: No such file or directory
> 
> It does not generate the version file at all anymore when it's not an
> explicit dependency. While I could of course add the missing dependency
> I don't know whether there are any other implicit dependencies that
> would be broken, as well. My gut feeling says "probably".

Doh, of course. We really want to say "do include this and consider it a
dependency, but don't read it yet". But I don't think there's a way to
tell make to do that.

I looked over your alternative approach with the OVERRIDE variable. I
can't think of any downsides, aside from the general head-spinning
complexity. ;) So that seems like a good approach for now (and I see
it's already in master. Yay).

-Peff

[PATCH v2 5/5] meson: add options to override build information

[Patrick Steinhardt]

We inject various different kinds of build information into build
artifacts, like the version string or the commit from which Git was
built. Add options to let users explicitly override this information
with Meson.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/meson.build |  1 +
 meson.build               | 13 +++++++++++++
 meson_options.txt         | 10 ++++++++++
 3 files changed, 24 insertions(+)

diff --git a/Documentation/meson.build b/Documentation/meson.build
index f2426ccaa30c29bd60b850eb0a9a4ab77c66a629..fca3eab1f1360a5fdeda89c1766ab8cdb3267b89 100644
--- a/Documentation/meson.build
+++ b/Documentation/meson.build
@@ -219,6 +219,7 @@ asciidoc_conf = custom_target(
   input: meson.current_source_dir() / 'asciidoc.conf.in',
   output: 'asciidoc.conf',
   depends: [git_version_file],
+  env: version_gen_environment,
 )
 
 asciidoc_common_options = [
diff --git a/meson.build b/meson.build
index 0dccebcdf16b07650d943e53643f0e09e2975cc9..be32d60e841a3055ed2bdf6fd449a48b66d94cd0 100644
--- a/meson.build
+++ b/meson.build
@@ -201,6 +201,16 @@ if get_option('sane_tool_path') != ''
   script_environment.prepend('PATH', get_option('sane_tool_path'))
 endif
 
+# The environment used by GIT-VERSION-GEN. Note that we explicitly override
+# environment variables that might be set by the user. This is by design so
+# that we always use whatever Meson has configured instead of what is present
+# in the environment.
+version_gen_environment = script_environment
+version_gen_environment.set('GIT_BUILT_FROM_COMMIT', get_option('built_from_commit'))
+version_gen_environment.set('GIT_DATE', get_option('build_date'))
+version_gen_environment.set('GIT_USER_AGENT', get_option('user_agent'))
+version_gen_environment.set('GIT_VERSION', get_option('version'))
+
 compiler = meson.get_compiler('c')
 
 libgit_sources = [
@@ -1485,6 +1495,7 @@ git_version_file = custom_target(
   ],
   input: meson.current_source_dir() / 'GIT-VERSION-FILE.in',
   output: 'GIT-VERSION-FILE',
+  env: version_gen_environment,
   build_always_stale: true,
 )
 
@@ -1501,6 +1512,7 @@ version_def_h = custom_target(
   # Depend on GIT-VERSION-FILE so that we don't always try to rebuild this
   # target for the same commit.
   depends: [git_version_file],
+  env: version_gen_environment,
 )
 
 # Build a separate library for "version.c" so that we do not have to rebuild
@@ -1544,6 +1556,7 @@ if host_machine.system() == 'windows'
     input: meson.current_source_dir() / 'git.rc.in',
     output: 'git.rc',
     depends: [git_version_file],
+    env: version_gen_environment,
   )
 
   common_main_sources += import('windows').compile_resources(git_rc,
diff --git a/meson_options.txt b/meson_options.txt
index 32a72139bae870745d9131cc9086a4594826be91..8ead1349550807420bdb95e55298ea4f3f2ea9d0 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -16,6 +16,16 @@ option('runtime_prefix', type: 'boolean', value: false,
 option('sane_tool_path', type: 'string', value: '',
   description: 'A colon-separated list of paths to prepend to PATH if your tools in /usr/bin are broken.')
 
+# Build information compiled into Git and other parts like documentation.
+option('build_date', type: 'string', value: '',
+  description: 'Build date reported by our documentation.')
+option('built_from_commit', type: 'string', value: '',
+  description: 'Commit that Git was built from reported by git-version(1).')
+option('user_agent', type: 'string', value: '',
+  description: 'User agent reported to remote servers.')
+option('version', type: 'string', value: '',
+  description: 'Version string reported by git-version(1) and other tools.')
+
 # Features supported by Git.
 option('curl', type: 'feature', value: 'enabled',
   description: 'Build helpers used to access remotes with the HTTP transport.')

-- 
2.48.0.rc0.184.g0fc57dec57.dirty

Re: [PATCH v2 0/5] GIT-VERSION-GEN: fix overriding values

[Jeff King]

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

> Changes in v2:
> 
>   - Don't strip leading `v`s when `GIT_VERSION` was set explicitly.
>   - Allow setting build info via "config.mak" again.
>   - Wire up build info options for Meson.
>   - Link to v1: https://lore.kernel.org/r/20241219-b4-pks-git-version-via-environment-v1-0-9393af058240@pks.im

Thanks, I confirmed that this fixes the doc-diff issue, and setting
values in config.mak works.

I left some small comments on patch 1. Patches 2-4 look good to me, and
I'm not qualified to comment on meson patches. ;)

-Peff

[PATCH v3 0/6] GIT-VERSION-GEN: fix overriding values

[Patrick Steinhardt]

Hi,

Peff reported that overriding GIT_VERSION and GIT_DATE broke recently
due to the refactoring of GIT-VERSION-GEN. This small commit series
fixes those cases, but also fixes the equivalent issue with
GIT_BUILT_FROM_COMMIT.

Changes in v2:

  - Don't strip leading `v`s when `GIT_VERSION` was set explicitly.
  - Allow setting build info via "config.mak" again.
  - Wire up build info options for Meson.
  - Link to v1: https://lore.kernel.org/r/20241219-b4-pks-git-version-via-environment-v1-0-9393af058240@pks.im

Changes in v3:
  - Redo things such that we use Makefile templates instead and a
    separate GIT_VERSION_OVERRIDE variable. This requires a bit of
    shuffling, but fixes a problem where the version becomes "stuck" due
    to how Makefiles handle includes.
  - Add another patch to stop including GIT-VERSION-FILE in
    Documentation/Makefile.
  - Improve code flow in GIT-VERSION-GEN.
  - Link to v2: https://lore.kernel.org/r/20241220-b4-pks-git-version-via-environment-v2-0-f1457a5e8c38@pks.im

Thanks!

Patrick

---
Patrick Steinhardt (6):
      Makefile: stop including "GIT-VERSION-FILE" in docs
      Makefile: drop unneeded indirection for GIT-VERSION-GEN outputs
      Makefile: introduce template for GIT-VERSION-GEN
      GIT-VERSION-GEN: fix overriding GIT_VERSION
      GIT-VERSION-GEN: fix overriding GIT_BUILT_FROM_COMMIT and GIT_DATE
      meson: add options to override build information

 Documentation/Makefile    | 17 +++++---------
 Documentation/meson.build |  1 +
 GIT-VERSION-GEN           | 58 ++++++++++++++++++++++++++++-------------------
 Makefile                  | 25 +++++++++++---------
 meson.build               | 13 +++++++++++
 meson_options.txt         | 10 ++++++++
 shared.mak                | 11 +++++++++
 7 files changed, 90 insertions(+), 45 deletions(-)

Range-diff versus v2:

1:  16c647d30b < -:  ---------- GIT-VERSION-GEN: fix overriding version via environment
2:  819154dc77 < -:  ---------- GIT-VERSION-GEN: fix overriding GIT_BUILT_FROM_COMMIT and GIT_DATE
-:  ---------- > 1:  dadeded025 Makefile: stop including "GIT-VERSION-FILE" in docs
3:  b658a5a42f = 2:  e5a4fff080 Makefile: drop unneeded indirection for GIT-VERSION-GEN outputs
4:  79b407d6a2 < -:  ---------- Makefile: respect build info declared in "config.mak"
-:  ---------- > 3:  c271f33aeb Makefile: introduce template for GIT-VERSION-GEN
-:  ---------- > 4:  c4f2c8aa17 GIT-VERSION-GEN: fix overriding GIT_VERSION
-:  ---------- > 5:  5d231080e0 GIT-VERSION-GEN: fix overriding GIT_BUILT_FROM_COMMIT and GIT_DATE
5:  26feb8bc81 = 6:  53bcb815c4 meson: add options to override build information

---
base-commit: d882f382b3d939d90cfa58d17b17802338f05d66
change-id: 20241219-b4-pks-git-version-via-environment-035490abec26

[PATCH v3 1/6] Makefile: stop including "GIT-VERSION-FILE" in docs

[Patrick Steinhardt]

We include "GIT-VERSION-FILE" in our docs Makefile, but don't actually
use the "GIT_VERSION" variable that it provides. This is a leftover from
the conversion to make "GIT-VERSION-GEN" generate version information
in-place by substituting placeholders in 4838deab65 (Makefile: refactor
GIT-VERSION-GEN to be reusable, 2024-12-06) and subsequent commits,
where all usages of the variable were removed.

Stop including the file.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/Makefile | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 3392e1ce7ebc540784912476847380d9c1775ac8..44c9e9369a11a6a5091079b7221a085b2f08e6cd 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -276,13 +276,6 @@ install-pdf: pdf
 install-html: html
 	'$(SHELL_PATH_SQ)' ./install-webdoc.sh $(DESTDIR)$(htmldir)
 
-../GIT-VERSION-FILE: FORCE
-	$(QUIET_SUBDIR0)../ $(QUIET_SUBDIR1) GIT-VERSION-FILE
-
-ifneq ($(filter-out lint-docs clean,$(MAKECMDGOALS)),)
--include ../GIT-VERSION-FILE
-endif
-
 mergetools_txt = mergetools-diff.txt mergetools-merge.txt
 
 #

-- 
2.48.0.rc0.184.g0fc57dec57.dirty

[PATCH v3 2/6] Makefile: drop unneeded indirection for GIT-VERSION-GEN outputs

[Patrick Steinhardt]

Some of the callsites of GIT-VERSION-GEN generate the target file with a
"+" suffix first and then move the file into place when the new contents
are different compared to the old contents. This allows us to avoid a
needless rebuild by not updating timestamps of the target file when its
contents will remain unchanged anyway.

In fact though, this exact logic is already handled in GIT-VERSION-GEN,
so doing this manually is pointless. This is a leftover from an earlier
version of 4838deab65 (Makefile: refactor GIT-VERSION-GEN to be
reusable, 2024-12-06), where the script didn't handle that logic for us.

Drop the needless indirection.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/Makefile | 6 ++----
 Makefile               | 6 ++----
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 44c9e9369a11a6a5091079b7221a085b2f08e6cd..1a398d0dc359671d461fceb7a1636268a51411da 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -211,12 +211,10 @@ XMLTO_EXTRA += --skip-validation
 XMLTO_EXTRA += -x manpage.xsl
 
 asciidoctor-extensions.rb: asciidoctor-extensions.rb.in FORCE
-	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@+
-	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
+	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@
 else
 asciidoc.conf: asciidoc.conf.in FORCE
-	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@+
-	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
+	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@
 endif
 
 ASCIIDOC_DEPS += docinfo.html
diff --git a/Makefile b/Makefile
index 79739a13d2132204f56b1ef4ca879bd51c5164b4..695a9d9765daf864605002d572129bae7a8c4e40 100644
--- a/Makefile
+++ b/Makefile
@@ -2512,8 +2512,7 @@ pager.sp pager.s pager.o: EXTRA_CPPFLAGS = \
 	-DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'
 
 version-def.h: version-def.h.in GIT-VERSION-GEN GIT-VERSION-FILE GIT-USER-AGENT
-	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" $< $@+
-	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
+	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" $< $@
 
 version.sp version.s version.o: version-def.h
 
@@ -2554,8 +2553,7 @@ $(SCRIPT_SH_GEN) $(SCRIPT_LIB) : % : %.sh generate-script.sh GIT-BUILD-OPTIONS G
 	mv $@+ $@
 
 git.rc: git.rc.in GIT-VERSION-GEN GIT-VERSION-FILE
-	$(QUIET_GEN)$(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" $< $@+
-	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
+	$(QUIET_GEN)$(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" $< $@
 
 git.res: git.rc GIT-PREFIX
 	$(QUIET_RC)$(RC) -i $< -o $@

-- 
2.48.0.rc0.184.g0fc57dec57.dirty

[PATCH v3 3/6] Makefile: introduce template for GIT-VERSION-GEN

[Patrick Steinhardt]

Introduce a new template to call GIT-VERSION-GEN. This will allow us to
iterate on how exactly the script is called in subsequent commits
without having to adapt all call sites every time.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/Makefile | 4 ++--
 Makefile               | 6 +++---
 shared.mak             | 8 ++++++++
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 1a398d0dc359671d461fceb7a1636268a51411da..ff30ab6c4295525757f6a150ec4ff0c72487f440 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -211,10 +211,10 @@ XMLTO_EXTRA += --skip-validation
 XMLTO_EXTRA += -x manpage.xsl
 
 asciidoctor-extensions.rb: asciidoctor-extensions.rb.in FORCE
-	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@
+	$(QUIET_GEN)$(call version_gen,"$(shell pwd)/..",$<,$@)
 else
 asciidoc.conf: asciidoc.conf.in FORCE
-	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@
+	$(QUIET_GEN)$(call version_gen,"$(shell pwd)/..",$<,$@)
 endif
 
 ASCIIDOC_DEPS += docinfo.html
diff --git a/Makefile b/Makefile
index 695a9d9765daf864605002d572129bae7a8c4e40..9cfe3d0aa968eff10379d22edff6cc6f4518c2ff 100644
--- a/Makefile
+++ b/Makefile
@@ -593,7 +593,7 @@ include shared.mak
 
 GIT-VERSION-FILE: FORCE
 	@OLD=$$(cat $@ 2>/dev/null || :) && \
-	$(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" GIT-VERSION-FILE.in $@ && \
+	$(call version_gen,"$(shell pwd)",GIT-VERSION-FILE.in,$@) && \
 	NEW=$$(cat $@ 2>/dev/null || :) && \
 	if test "$$OLD" != "$$NEW"; then echo "$$NEW" >&2; fi
 -include GIT-VERSION-FILE
@@ -2512,7 +2512,7 @@ pager.sp pager.s pager.o: EXTRA_CPPFLAGS = \
 	-DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'
 
 version-def.h: version-def.h.in GIT-VERSION-GEN GIT-VERSION-FILE GIT-USER-AGENT
-	$(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" $< $@
+	$(QUIET_GEN)$(call version_gen,"$(shell pwd)",$<,$@)
 
 version.sp version.s version.o: version-def.h
 
@@ -2553,7 +2553,7 @@ $(SCRIPT_SH_GEN) $(SCRIPT_LIB) : % : %.sh generate-script.sh GIT-BUILD-OPTIONS G
 	mv $@+ $@
 
 git.rc: git.rc.in GIT-VERSION-GEN GIT-VERSION-FILE
-	$(QUIET_GEN)$(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" $< $@
+	$(QUIET_GEN)$(call version_gen,"$(shell pwd)",$<,$@)
 
 git.res: git.rc GIT-PREFIX
 	$(QUIET_RC)$(RC) -i $< -o $@
diff --git a/shared.mak b/shared.mak
index 29bebd30d8acbce9f50661cef48ecdbae1e41f5a..b23c5505c9692b032cd0b18d3e4ede288614d937 100644
--- a/shared.mak
+++ b/shared.mak
@@ -116,3 +116,11 @@ endef
 define libpath_template
 -L$(1) $(if $(filter-out -L,$(CC_LD_DYNPATH)),$(CC_LD_DYNPATH)$(1))
 endef
+
+# Populate build information into a file via GIT-VERSION-GEN. Requires the
+# absolute path to the root source directory as well as input and output files
+# as arguments, in that order.
+define version_gen
+GIT_USER_AGENT="$(GIT_USER_AGENT)" \
+$(SHELL_PATH) "$(1)/GIT-VERSION-GEN" "$(1)" "$(2)" "$(3)"
+endef

-- 
2.48.0.rc0.184.g0fc57dec57.dirty

[PATCH v3 4/6] GIT-VERSION-GEN: fix overriding GIT_VERSION

[Patrick Steinhardt]

GIT-VERSION-GEN tries to derive the version that Git is being built from
via multiple different sources in the following order:

  1. A file called "version" in the source tree's root directory, if it
     exists.

  2. The current commit in case Git is built from a Git repository.

  3. Otherwise, we use a fallback version stored in a variable which is
     bumped whenever a new Git version is getting tagged.

It used to be possible to override the version by overriding the
`GIT_VERSION` Makefile variable (e.g. `make GIT_VERSION=foo`). This
worked somewhat by chance, only: `GIT-VERSION-GEN` would write the
actual Git version into `GIT-VERSION-FILE`, not the overridden value,
but when including the file into our Makefile we would not override the
`GIT_VERSION` variable because it has already been set by the user. And
because our Makefile used the variable to propagate the version to our
build tools instead of using `GIT-VERSION-FILE` the resulting build
artifacts used the overridden version.

But that subtle mechanism broke with 4838deab65 (Makefile: refactor
GIT-VERSION-GEN to be reusable, 2024-12-06) and subsequent commits
because the version information is not propagated via the Makefile
variable anymore, but instead via the files that `GIT-VERSION-GEN`
started to write. And as the script never knew about the `GIT_VERSION`
environment variable in the first place it uses one of the values listed
above instead of the overridden value.

Fix this issue by making `GIT-VERSION-GEN` handle the case where
`GIT_VERSION` has been set via the environment.

Note that this requires us to introduce a new GIT_VERSION_OVERRIDE
variable that stores a potential user-provided value, either via the
environment or via "config.mak". Ideally we wouldn't need it and could
just continue to use GIT_VERSION for this. But unfortunately, Makefiles
will first include all sub-Makefiles before figuring out whether it
needs to re-make any of them [1]. Consequently, if there already is a
GIT-VERSION-FILE, we would have slurped in its value of GIT_VERSION
before we call GIT-VERSION-GEN, and because GIT-VERSION-GEN now uses
that value as an override it would mean that the first generated value
for GIT_VERSION will remain unchanged.

Furthermore we have to move the include for "GIT-VERSION-FILE" after the
includes for "config.mak" and related so that GIT_VERSION_OVERRIDE can
be set to the value provided by "config.mak".

[1]: https://www.gnu.org/software/make/manual/html_node/Remaking-Makefiles.html

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/Makefile |  4 ++++
 GIT-VERSION-GEN        | 48 ++++++++++++++++++++++++++----------------------
 Makefile               | 19 ++++++++++++-------
 shared.mak             |  1 +
 4 files changed, 43 insertions(+), 29 deletions(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index ff30ab6c4295525757f6a150ec4ff0c72487f440..a89823e1d1ee5042367bdcca6ed426196d49ce89 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -181,6 +181,10 @@ endif
 -include ../config.mak.autogen
 -include ../config.mak
 
+# Set GIT_VERSION_OVERRIDE such that version_gen knows to substitute
+# GIT_VERSION in case it was set by the user.
+GIT_VERSION_OVERRIDE := $(GIT_VERSION)
+
 ifndef NO_MAN_BOLD_LITERAL
 XMLTO_EXTRA += -m manpage-bold-literal.xsl
 endif
diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index de0e63bdfbac263884e2ea328cc2ef11ace7a238..9b785da226eff2d7952d3306f45fd2933fdafaca 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN
@@ -27,31 +27,35 @@ fi
 GIT_CEILING_DIRECTORIES="$SOURCE_DIR/.."
 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 -z "$GIT_VERSION"
 then
-	VN=$(cat "$SOURCE_DIR"/version) || VN="$DEF_VER"
-elif {
-		test -d "$SOURCE_DIR/.git" ||
-		test -d "${GIT_DIR:-.git}" ||
-		test -f "$SOURCE_DIR"/.git;
-	} &&
-	VN=$(git -C "$SOURCE_DIR" describe --match "v[0-9]*" HEAD 2>/dev/null) &&
-	case "$VN" in
-	*$LF*) (exit 1) ;;
-	v[0-9]*)
-		git -C "$SOURCE_DIR" update-index -q --refresh
-		test -z "$(git -C "$SOURCE_DIR" diff-index --name-only HEAD --)" ||
-		VN="$VN-dirty" ;;
-	esac
-then
-	VN=$(echo "$VN" | sed -e 's/-/./g');
-else
-	VN="$DEF_VER"
+	# First see if there is a version file (included in release tarballs),
+	# then try git-describe, then default.
+	if test -f "$SOURCE_DIR"/version
+	then
+		VN=$(cat "$SOURCE_DIR"/version) || VN="$DEF_VER"
+	elif {
+			test -d "$SOURCE_DIR/.git" ||
+			test -d "${GIT_DIR:-.git}" ||
+			test -f "$SOURCE_DIR"/.git;
+		} &&
+		VN=$(git -C "$SOURCE_DIR" describe --match "v[0-9]*" HEAD 2>/dev/null) &&
+		case "$VN" in
+		*$LF*) (exit 1) ;;
+		v[0-9]*)
+			git -C "$SOURCE_DIR" update-index -q --refresh
+			test -z "$(git -C "$SOURCE_DIR" diff-index --name-only HEAD --)" ||
+			VN="$VN-dirty" ;;
+		esac
+	then
+		VN=$(echo "$VN" | sed -e 's/-/./g');
+	else
+		VN="$DEF_VER"
+	fi
+
+	GIT_VERSION=$(expr "$VN" : v*'\(.*\)')
 fi
 
-GIT_VERSION=$(expr "$VN" : v*'\(.*\)')
 GIT_BUILT_FROM_COMMIT=$(git -C "$SOURCE_DIR" rev-parse -q --verify HEAD 2>/dev/null)
 GIT_DATE=$(git -C "$SOURCE_DIR" show --quiet --format='%as' 2>/dev/null)
 if test -z "$GIT_USER_AGENT"
diff --git a/Makefile b/Makefile
index 9cfe3d0aa968eff10379d22edff6cc6f4518c2ff..3cdd2278fdaa40feb6139992aa275ad26aaae4a6 100644
--- a/Makefile
+++ b/Makefile
@@ -591,13 +591,6 @@ include shared.mak
 #
 #        Disable -pedantic compilation.
 
-GIT-VERSION-FILE: FORCE
-	@OLD=$$(cat $@ 2>/dev/null || :) && \
-	$(call version_gen,"$(shell pwd)",GIT-VERSION-FILE.in,$@) && \
-	NEW=$$(cat $@ 2>/dev/null || :) && \
-	if test "$$OLD" != "$$NEW"; then echo "$$NEW" >&2; fi
--include GIT-VERSION-FILE
-
 # Set our default configuration.
 #
 # Among the variables below, these:
@@ -1465,6 +1458,18 @@ ifdef DEVELOPER
 include config.mak.dev
 endif
 
+GIT-VERSION-FILE: FORCE
+	@OLD=$$(cat $@ 2>/dev/null || :) && \
+	$(call version_gen,"$(shell pwd)",GIT-VERSION-FILE.in,$@) && \
+	NEW=$$(cat $@ 2>/dev/null || :) && \
+	if test "$$OLD" != "$$NEW"; then echo "$$NEW" >&2; fi
+
+# We need to set GIT_VERSION_OVERRIDE before including the version file as
+# otherwise any user-provided value for GIT_VERSION would have been overridden
+# already.
+GIT_VERSION_OVERRIDE := $(GIT_VERSION)
+-include GIT-VERSION-FILE
+
 # what 'all' will build and 'install' will install in gitexecdir,
 # excluding programs for built-in commands
 ALL_PROGRAMS = $(PROGRAMS) $(SCRIPTS)
diff --git a/shared.mak b/shared.mak
index b23c5505c9692b032cd0b18d3e4ede288614d937..a66f46969e301b35d88650f2c6abc6c0ba1b0f3d 100644
--- a/shared.mak
+++ b/shared.mak
@@ -122,5 +122,6 @@ endef
 # as arguments, in that order.
 define version_gen
 GIT_USER_AGENT="$(GIT_USER_AGENT)" \
+GIT_VERSION="$(GIT_VERSION_OVERRIDE)" \
 $(SHELL_PATH) "$(1)/GIT-VERSION-GEN" "$(1)" "$(2)" "$(3)"
 endef

-- 
2.48.0.rc0.184.g0fc57dec57.dirty

Re: [PATCH v3 4/6] GIT-VERSION-GEN: fix overriding GIT_VERSION

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index ff30ab6c4295525757f6a150ec4ff0c72487f440..a89823e1d1ee5042367bdcca6ed426196d49ce89 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -181,6 +181,10 @@ endif
>  -include ../config.mak.autogen
>  -include ../config.mak
>  
> +# Set GIT_VERSION_OVERRIDE such that version_gen knows to substitute
> +# GIT_VERSION in case it was set by the user.
> +GIT_VERSION_OVERRIDE := $(GIT_VERSION)
> +
>  ifndef NO_MAN_BOLD_LITERAL
>  XMLTO_EXTRA += -m manpage-bold-literal.xsl
>  endif

So the idea is that those targets and scripts may have their own
GIT_VERION value when they run GIT-VERSION-GEN to cause GIT_VERSION
to computed, and in such a case, they should pass the GIT_VERSION
they have in GIT_VERSION_OVERRIDE, and thanks to the version_gen
thing, this value in GIT_VERSION_OVERRIDE is passed in the
environment as GIT_VERSION when GIT-VERSION-GEN is run, and the
value in turn is passed intact.  Somehow this makes my head spin, as
it looks quite convoluted, but the overall flow should yield the
desired value.

Queued.  Thanks.

Re: [PATCH v3 4/6] GIT-VERSION-GEN: fix overriding GIT_VERSION

[Patrick Steinhardt]

On Fri, Dec 20, 2024 at 01:50:25PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > diff --git a/Documentation/Makefile b/Documentation/Makefile
> > index ff30ab6c4295525757f6a150ec4ff0c72487f440..a89823e1d1ee5042367bdcca6ed426196d49ce89 100644
> > --- a/Documentation/Makefile
> > +++ b/Documentation/Makefile
> > @@ -181,6 +181,10 @@ endif
> >  -include ../config.mak.autogen
> >  -include ../config.mak
> >  
> > +# Set GIT_VERSION_OVERRIDE such that version_gen knows to substitute
> > +# GIT_VERSION in case it was set by the user.
> > +GIT_VERSION_OVERRIDE := $(GIT_VERSION)
> > +
> >  ifndef NO_MAN_BOLD_LITERAL
> >  XMLTO_EXTRA += -m manpage-bold-literal.xsl
> >  endif
> 
> So the idea is that those targets and scripts may have their own
> GIT_VERION value when they run GIT-VERSION-GEN to cause GIT_VERSION
> to computed, and in such a case, they should pass the GIT_VERSION
> they have in GIT_VERSION_OVERRIDE, and thanks to the version_gen
> thing, this value in GIT_VERSION_OVERRIDE is passed in the
> environment as GIT_VERSION when GIT-VERSION-GEN is run, and the
> value in turn is passed intact.  Somehow this makes my head spin, as
> it looks quite convoluted, but the overall flow should yield the
> desired value.

Agreed, it makes mine spin, as well. Next release cycle I'll have a look
at whether I can get rid of the include of "GIT-VERSION-FILE" altogether
to make the logic simpler.

Patrick

[PATCH v3 5/6] GIT-VERSION-GEN: fix overriding GIT_BUILT_FROM_COMMIT and GIT_DATE

[Patrick Steinhardt]

Same as with the preceding commit, neither GIT_BUILT_FROM_COMMIT nor
GIT_DATE can be overridden via the environment. Especially the latter is
of importance given that we set it in our own "Documentation/doc-diff"
script.

Make the values of both variables overridable. Luckily we don't pull in
these values via any included Makefiles, so the fix is trivial compared
to the fix for GIT_VERSON.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 GIT-VERSION-GEN | 12 ++++++++++--
 shared.mak      |  2 ++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index 9b785da226eff2d7952d3306f45fd2933fdafaca..497b4e48d20f9d1d20f2db2a3aae2a92316b7ca9 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN
@@ -56,8 +56,16 @@ then
 	GIT_VERSION=$(expr "$VN" : v*'\(.*\)')
 fi
 
-GIT_BUILT_FROM_COMMIT=$(git -C "$SOURCE_DIR" rev-parse -q --verify HEAD 2>/dev/null)
-GIT_DATE=$(git -C "$SOURCE_DIR" show --quiet --format='%as' 2>/dev/null)
+if test -z "$GIT_BUILT_FROM_COMMIT"
+then
+	GIT_BUILT_FROM_COMMIT=$(git -C "$SOURCE_DIR" rev-parse -q --verify HEAD 2>/dev/null)
+fi
+
+if test -z "$GIT_DATE"
+then
+	GIT_DATE=$(git -C "$SOURCE_DIR" show --quiet --format='%as' 2>/dev/null)
+fi
+
 if test -z "$GIT_USER_AGENT"
 then
 	GIT_USER_AGENT="git/$GIT_VERSION"
diff --git a/shared.mak b/shared.mak
index a66f46969e301b35d88650f2c6abc6c0ba1b0f3d..1a99848a95174c5e386c59321655937e7b7d8a28 100644
--- a/shared.mak
+++ b/shared.mak
@@ -121,6 +121,8 @@ endef
 # absolute path to the root source directory as well as input and output files
 # as arguments, in that order.
 define version_gen
+GIT_BUILT_FROM_COMMIT="$(GIT_BUILT_FROM_COMMIT)" \
+GIT_DATE="$(GIT_DATE)" \
 GIT_USER_AGENT="$(GIT_USER_AGENT)" \
 GIT_VERSION="$(GIT_VERSION_OVERRIDE)" \
 $(SHELL_PATH) "$(1)/GIT-VERSION-GEN" "$(1)" "$(2)" "$(3)"

-- 
2.48.0.rc0.184.g0fc57dec57.dirty

[PATCH v3 6/6] meson: add options to override build information

[Patrick Steinhardt]

We inject various different kinds of build information into build
artifacts, like the version string or the commit from which Git was
built. Add options to let users explicitly override this information
with Meson.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/meson.build |  1 +
 meson.build               | 13 +++++++++++++
 meson_options.txt         | 10 ++++++++++
 3 files changed, 24 insertions(+)

diff --git a/Documentation/meson.build b/Documentation/meson.build
index f2426ccaa30c29bd60b850eb0a9a4ab77c66a629..fca3eab1f1360a5fdeda89c1766ab8cdb3267b89 100644
--- a/Documentation/meson.build
+++ b/Documentation/meson.build
@@ -219,6 +219,7 @@ asciidoc_conf = custom_target(
   input: meson.current_source_dir() / 'asciidoc.conf.in',
   output: 'asciidoc.conf',
   depends: [git_version_file],
+  env: version_gen_environment,
 )
 
 asciidoc_common_options = [
diff --git a/meson.build b/meson.build
index 0dccebcdf16b07650d943e53643f0e09e2975cc9..be32d60e841a3055ed2bdf6fd449a48b66d94cd0 100644
--- a/meson.build
+++ b/meson.build
@@ -201,6 +201,16 @@ if get_option('sane_tool_path') != ''
   script_environment.prepend('PATH', get_option('sane_tool_path'))
 endif
 
+# The environment used by GIT-VERSION-GEN. Note that we explicitly override
+# environment variables that might be set by the user. This is by design so
+# that we always use whatever Meson has configured instead of what is present
+# in the environment.
+version_gen_environment = script_environment
+version_gen_environment.set('GIT_BUILT_FROM_COMMIT', get_option('built_from_commit'))
+version_gen_environment.set('GIT_DATE', get_option('build_date'))
+version_gen_environment.set('GIT_USER_AGENT', get_option('user_agent'))
+version_gen_environment.set('GIT_VERSION', get_option('version'))
+
 compiler = meson.get_compiler('c')
 
 libgit_sources = [
@@ -1485,6 +1495,7 @@ git_version_file = custom_target(
   ],
   input: meson.current_source_dir() / 'GIT-VERSION-FILE.in',
   output: 'GIT-VERSION-FILE',
+  env: version_gen_environment,
   build_always_stale: true,
 )
 
@@ -1501,6 +1512,7 @@ version_def_h = custom_target(
   # Depend on GIT-VERSION-FILE so that we don't always try to rebuild this
   # target for the same commit.
   depends: [git_version_file],
+  env: version_gen_environment,
 )
 
 # Build a separate library for "version.c" so that we do not have to rebuild
@@ -1544,6 +1556,7 @@ if host_machine.system() == 'windows'
     input: meson.current_source_dir() / 'git.rc.in',
     output: 'git.rc',
     depends: [git_version_file],
+    env: version_gen_environment,
   )
 
   common_main_sources += import('windows').compile_resources(git_rc,
diff --git a/meson_options.txt b/meson_options.txt
index 32a72139bae870745d9131cc9086a4594826be91..8ead1349550807420bdb95e55298ea4f3f2ea9d0 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -16,6 +16,16 @@ option('runtime_prefix', type: 'boolean', value: false,
 option('sane_tool_path', type: 'string', value: '',
   description: 'A colon-separated list of paths to prepend to PATH if your tools in /usr/bin are broken.')
 
+# Build information compiled into Git and other parts like documentation.
+option('build_date', type: 'string', value: '',
+  description: 'Build date reported by our documentation.')
+option('built_from_commit', type: 'string', value: '',
+  description: 'Commit that Git was built from reported by git-version(1).')
+option('user_agent', type: 'string', value: '',
+  description: 'User agent reported to remote servers.')
+option('version', type: 'string', value: '',
+  description: 'Version string reported by git-version(1) and other tools.')
+
 # Features supported by Git.
 option('curl', type: 'feature', value: 'enabled',
   description: 'Build helpers used to access remotes with the HTTP transport.')

-- 
2.48.0.rc0.184.g0fc57dec57.dirty