Re: [PATCH 2/4] version: add git_user_agent function

I'm terribly late to the party, seeing as this is already in next, but:

Jeff King [off-list ref] writes:

This is basically a fancy way of saying "git/$GIT_VERSION",
except that it is overridable at build-time and through the
environment. Which means that people who don't want to
advertise their git version (for privacy or security
reasons) can tweak it.

...

+GIT_USER_AGENT = git/$(GIT_VERSION)

...

+GIT_USER_AGENT_SQ = $(subst ','\'',$(GIT_USER_AGENT))
+GIT_USER_AGENT_CQ = "$(subst ",\",$(subst \,\\,$(GIT_USER_AGENT)))"
+GIT_USER_AGENT_CQ_SQ = $(subst ','\'',$(GIT_USER_AGENT_CQ))
+BASIC_CFLAGS += -DGIT_USER_AGENT='$(GIT_USER_AGENT_CQ_SQ)'

Unless the user manually sets GIT_USER_AGENT, This forces a full rebuild
due to changed CFLAGS whenever the version changes.  Can you make it so
that only version.o needs to be rebuilt, as with the normal git version
string?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

On Tue, Jun 19, 2012 at 08:40:04PM +0200, Thomas Rast wrote:

I'm terribly late to the party, seeing as this is already in next, but:

It's never too late.

quoted

+GIT_USER_AGENT = git/$(GIT_VERSION)

...

quoted

+GIT_USER_AGENT_SQ = $(subst ','\'',$(GIT_USER_AGENT))
+GIT_USER_AGENT_CQ = "$(subst ",\",$(subst \,\\,$(GIT_USER_AGENT)))"
+GIT_USER_AGENT_CQ_SQ = $(subst ','\'',$(GIT_USER_AGENT_CQ))
+BASIC_CFLAGS += -DGIT_USER_AGENT='$(GIT_USER_AGENT_CQ_SQ)'

Unless the user manually sets GIT_USER_AGENT, This forces a full rebuild
due to changed CFLAGS whenever the version changes.  Can you make it so
that only version.o needs to be rebuilt, as with the normal git version
string?

Ick, yeah. I had though this was already the case, but it turns out that
it was a peculiarity of my personal config.mak (I set a custom $prefix
based on the branch, and it has a similar problem). I'll see if I can
fix both.

-Peff

On Tue, Jun 19, 2012 at 02:59:16PM -0400, Jeff King wrote:

quoted

Unless the user manually sets GIT_USER_AGENT, This forces a full rebuild
due to changed CFLAGS whenever the version changes.  Can you make it so
that only version.o needs to be rebuilt, as with the normal git version
string?

Ick, yeah. I had though this was already the case, but it turns out that
it was a peculiarity of my personal config.mak (I set a custom $prefix
based on the branch, and it has a similar problem). I'll see if I can
fix both.

Here it is.

  [1/3]: Makefile: apply dependencies consistently to sparse/asm targets
  [2/3]: Makefile: split GIT_USER_AGENT from GIT-CFLAGS
  [3/3]: Makefile: split prefix flags from GIT-CFLAGS

Patches go on top of jk/version-string.

-Peff

When a C file includes a header file or depends on a
command-line "-D" macro, we note it in the Makefile like:

  git.o: common-cmds.h

However, other targets built from the C file should also
know about this dependency (in particular, .sp and .s files
that are not part of the usual build process). We sometimes
noted these and sometimes not; let's make sure they are
always included.

Signed-off-by: Jeff King <redacted>
---
 Makefile | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 62de0b4..537d2ea 100644
--- a/Makefile
+++ b/Makefile

@@ -1972,7 +1972,7 @@ shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
 strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $(PROGRAMS) git$X
 
-git.o: common-cmds.h
+git.sp git.s git.o: common-cmds.h
 git.sp git.s git.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \

@@ -1982,9 +1982,9 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ git.o \
 		$(BUILTIN_OBJS) $(ALL_LDFLAGS) $(LIBS)
 
-help.sp help.o: common-cmds.h
+help.sp help.s help.o: common-cmds.h
 
-builtin/help.sp builtin/help.o: common-cmds.h
+builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \

-- 
1.7.11.rc3.5.g201460b

The default user-agent depends on the GIT_VERSION, which
means that anytime you switch versions, it causes a full
rebuild. Instead, let's split it out into its own file and
restrict the dependency to version.o.

Signed-off-by: Jeff King <redacted>
---
This can't just depend on GIT-VERSION-FILE, since the user may have set
GIT_USER_AGENT independently, and we would want to trigger a rebuild in
that case.

I'm not happy about adding an extra built file and 4 lines of ugly shell
script per variable, but I don't see a good way around it. It would be
slightly nicer if we actually just built auto/user-agent.h with the
value, #included it from version.c, and then let the automatic
dependency checker pick it up. But we'd still have to do the "only
update it if the value is actually changed" dance.

I've worked on systems in the past which use a file per variable, which
lets make handle the dependencies normally. Something like:

  $ echo my-value >config/foo
  $ cat Makefile
  auto/foo.o: config/foo
          ./build-cstr $<

but that is a radical departure from the current config procedure. It
also doesn't play all that well with version control (are config/* files
versioned? If not, where do the defaults come from? If so, how do you
override without the vcs wanting to commit changes?).

We are pretty well tied to GNU make. So there might be some GNU-specific
way of templating GIT-CFLAGS, GIT-LDFLAGS, GIT-USER-AGENT etc.

 .gitignore |  1 +
 Makefile   | 10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/.gitignore b/.gitignore
index bf66648..7329cfe 100644
--- a/.gitignore
+++ b/.gitignore

@@ -2,6 +2,7 @@
 /GIT-CFLAGS
 /GIT-LDFLAGS
 /GIT-GUI-VARS
+/GIT-USER-AGENT
 /GIT-VERSION-FILE
 /bin-wrappers/
 /git

diff --git a/Makefile b/Makefile
index 537d2ea..42ce2dd 100644
--- a/Makefile
+++ b/Makefile

@@ -1924,7 +1924,11 @@ endif
 GIT_USER_AGENT_SQ = $(subst ','\'',$(GIT_USER_AGENT))
 GIT_USER_AGENT_CQ = "$(subst ",\",$(subst \,\\,$(GIT_USER_AGENT)))"
 GIT_USER_AGENT_CQ_SQ = $(subst ','\'',$(GIT_USER_AGENT_CQ))
-BASIC_CFLAGS += -DGIT_USER_AGENT='$(GIT_USER_AGENT_CQ_SQ)'
+GIT-USER-AGENT: FORCE
+	@if test x'$(GIT_USER_AGENT_SQ)' != x"`cat GIT-USER-AGENT 2>/dev/null`"; then \
+		echo >&2 "    * new user-agent flag"; \
+		echo '$(GIT_USER_AGENT_SQ)' >GIT-USER-AGENT; \
+	fi
 
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)

@@ -1990,8 +1994,10 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_SQ)"'
 
+version.sp version.s version.o: GIT-USER-AGENT
 version.sp version.s version.o: EXTRA_CPPFLAGS = \
-	'-DGIT_VERSION="$(GIT_VERSION)"'
+	'-DGIT_VERSION="$(GIT_VERSION)"' \
+	'-DGIT_USER_AGENT=$(GIT_USER_AGENT_CQ_SQ)'
 
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \

-- 
1.7.11.rc3.5.g201460b

Most of the build targets do not care about the setting of
$prefix (or its derivative variables), but will be rebuilt
if the prefix changes. For most setups this doesn't matter
(they set prefix once and never change it), but for a setup
which puts each branch or version in its own prefix, this
unnecessarily causes a full rebuild whenever the branc is
changed.

Signed-off-by: Jeff King <redacted>
---
We could break this down even further and save a few compilations (e.g.,
config.o does not care about bindir, only sysconfdir). But given that
most people just set prefix anyway, I don't think it's worth the extra
lines of Makefile (and this patch will never generate a _wrong_ answer;
it is too conservative in doing extra compiles).

 .gitignore |  1 +
 Makefile   | 29 +++++++++++++++++++++--------
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/.gitignore b/.gitignore
index 7329cfe..c60c5a3 100644
--- a/.gitignore
+++ b/.gitignore

@@ -2,6 +2,7 @@
 /GIT-CFLAGS
 /GIT-LDFLAGS
 /GIT-GUI-VARS
+/GIT-PREFIX
 /GIT-USER-AGENT
 /GIT-VERSION-FILE
 /bin-wrappers/

diff --git a/Makefile b/Makefile
index 42ce2dd..9761ae8 100644
--- a/Makefile
+++ b/Makefile

@@ -1976,7 +1976,7 @@ shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
 strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $(PROGRAMS) git$X
 
-git.sp git.s git.o: common-cmds.h
+git.sp git.s git.o: common-cmds.h GIT-PREFIX
 git.sp git.s git.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \

@@ -1988,7 +1988,7 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
 
 help.sp help.s help.o: common-cmds.h
 
-builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h
+builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h GIT-PREFIX
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \

@@ -2036,7 +2036,7 @@ $(SCRIPT_LIB) : % : %.sh
 ifndef NO_PERL
 $(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
 
-perl/perl.mak: GIT-CFLAGS perl/Makefile perl/Makefile.PL
+perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL
 	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F)
 
 $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl

@@ -2080,7 +2080,7 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)) git-instaweb: % : unimplemented.sh
 endif # NO_PERL
 
 ifndef NO_PYTHON
-$(patsubst %.py,%,$(SCRIPT_PYTHON)): GIT-CFLAGS
+$(patsubst %.py,%,$(SCRIPT_PYTHON)): GIT-CFLAGS GIT-PREFIX
 $(patsubst %.py,%,$(SCRIPT_PYTHON)): % : %.py
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C git_remote_helpers -s \

@@ -2263,20 +2263,25 @@ xdiff-interface.o $(XDIFF_OBJS): $(XDIFF_H)
 $(VCSSVN_OBJS) $(VCSSVN_TEST_OBJS): $(LIB_H) $(VCSSVN_H)
 endif
 
+exec_cmd.sp exec_cmd.s exec_cmd.o: GIT-PREFIX
 exec_cmd.sp exec_cmd.s exec_cmd.o: EXTRA_CPPFLAGS = \
 	'-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
 	'-DBINDIR="$(bindir_relative_SQ)"' \
 	'-DPREFIX="$(prefix_SQ)"'
 
+builtin/init-db.sp builtin/init-db.s builtin/init-db.o: GIT-PREFIX
 builtin/init-db.sp builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \
 	-DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
 
+config.sp config.s config.o: GIT-PREFIX
 config.sp config.s config.o: EXTRA_CPPFLAGS = \
 	-DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"'
 
+attr.sp attr.s attr.o: GIT-PREFIX
 attr.sp attr.s attr.o: EXTRA_CPPFLAGS = \
 	-DETC_GITATTRIBUTES='"$(ETC_GITATTRIBUTES_SQ)"'
 
+gettext.sp gettext.s gettext.o: GIT-PREFIX
 gettext.sp gettext.s gettext.o: EXTRA_CPPFLAGS = \
 	-DGIT_LOCALE_PATH='"$(localedir_SQ)"'
 

@@ -2400,14 +2405,22 @@ cscope:
 	$(FIND_SOURCE_FILES) | xargs cscope -b
 
 ### Detect prefix changes
-TRACK_CFLAGS = $(CC):$(subst ','\'',$(ALL_CFLAGS)):\
-             $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\
-             $(localedir_SQ):$(USE_GETTEXT_SCHEME)
+TRACK_PREFIX = $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\
+		$(localedir_SQ)
+
+GIT-PREFIX: FORCE
+	@FLAGS='$(TRACK_PREFIX)'; \
+	if test x"$$FLAGS" != x"`cat GIT-PREFIX 2>/dev/null`" ; then \
+		echo 1>&2 "    * new prefix flags"; \
+		echo "$$FLAGS" >GIT-PREFIX; \
+	fi
+
+TRACK_CFLAGS = $(CC):$(subst ','\'',$(ALL_CFLAGS)):$(USE_GETTEXT_SCHEME)
 
 GIT-CFLAGS: FORCE
 	@FLAGS='$(TRACK_CFLAGS)'; \
 	    if test x"$$FLAGS" != x"`cat GIT-CFLAGS 2>/dev/null`" ; then \
-		echo 1>&2 "    * new build flags or prefix"; \
+		echo 1>&2 "    * new build flags"; \
 		echo "$$FLAGS" >GIT-CFLAGS; \
             fi
 

-- 
1.7.11.rc3.5.g201460b