threads / rfc / 35806

RFC patch, 13 partsmakefile refactoring

Subject: [PATCH/RFC 0/13] makefile refactoring

## tl;dr

28 messages between Feb 5, 2014 and Feb 10, 2014. Diffs are folded; open one to read it.

replies: 27people: 4as markdown or json

Jeff King· Feb 5, 2014, 17:48 UTC · lore

This started with the desire to move the setting of the LESS and LV environment variables into a Makefile knob. But there's a fair bit of infrastructure involved in that, and this is an attempt to factor out some of that infrastructure to be more easily reusable. And if we like the approach, we can move more build-time config in this direction.

There are a couple of things going on here, but the main ideas are:
  1. Try to get build-time data into files as much as possible, because
     the one thing make understands is dependencies between files. Right
     now we rely on sentinel files like GIT-CFLAGS, which have two
     downsides:
       a. It is easy to miss a dependency that should be in a sentinel
          file, leading to failure to rebuild when we should.
       b. Because they are so cumbersome to use, we tend to put a lot of
          items into a small number of sentinel files, leading to
          unnecessary rebuilds (e.g., turning on XDL_FAST_HASH
          recompiles _everything_, even though only one C file cares
          about it).
  2. Some light meta-programming to avoid repeating ourselves and try to
     make a few things more readable. I've done this here with $(call)
     and $(eval), which are basically the only way to do this in GNU
     make (and here we are relying heavily on GNU make, but as far as I
     know, nobody has had a huge problem with that in the past).
     Frankly, some of it is kind of ugly. And there's some potential for
     portability/version problems, just because we're not using more
     advanced features. If we don't like that approach, an alternative
     is to generate snippets of Makefile in a separate script and
     include them (like we do already for GIT-VERSION-FILE). That would
     let us write in whatever language we want, and avoid portability
     problems. The downside is that it may be less obvious to a reader
     not familiar with the system (e.g., you cannot necessarily grep for
     all the rules in Makefile, though that is already somewhat the case
     with pattern rules).
The two potential criticisms I expect are:
  1. Portability issues with $(call), as mentioned above. We avoided
     this in 2006, but it may be sufficiently available now. See patch 3
     for exact numbers/versions.
  2. Some people may simply find it ugly or too confusing for the
     benefit. Hence the RFC. :)

While I tried to polish these patches enough to be applied, please take this mostly as an RFC on the ideas and direction. There are multiple alternatives to implement some of these things, and I mainly want to see if people think this sort of make meta-programming is a good idea. The patches are:

  [01/13]: Makefile: drop USE_GETTEXT_SCHEME from GIT-CFLAGS
  [02/13]: Makefile: fix git-instaweb dependency on gitweb
    These first two are cleanups I noticed in the area, and can be
    applied regardless of the rest.
  [03/13]: Makefile: introduce make-var helper function
  [04/13]: Makefile: use tempfile/mv strategy for GIT-*
  [05/13]: Makefile: prefer printf to echo for GIT-*
  [06/13]: Makefile: store GIT-* sentinel files in MAKE/
  [07/13]: Makefile: always create files via make-var
    These ones factor out and improve the GIT-* file handling. Even if
    we decide not to go this route, patches 4, 5, and 7 are improvements
    that could apply to the current code. I didn't float them to the top
    of the series because it would involve making the same change in
    several different spots. If we decide not to apply this series, I
    can re-roll them as appropriate.
  [08/13]: Makefile: introduce sq function for shell-quoting
  [09/13]: Makefile: add c-quote helper function
  [10/13]: Makefile: drop *_SQ variables
    If we accept that we can use $(call), these are further readability
    cleanups we can do. They are technically optional, though, with
    respect to the rest of the series.
  [11/13]: Makefile: auto-build C strings from make variables
  [12/13]: Makefile: teach scripts to include make variables
    These ones lay the groundwork for easily getting make variables into
    shell scripts and C programs.
  [13/13]: move LESS/LV pager environment to Makefile
    And this one is the point of the series, which is fairly
    straightforward because of the earlier groundwork.
-Peff
Jeff King· Feb 5, 2014, 17:49 UTC · re: Jeff King · lore

[PATCH 01/13] Makefile: drop USE_GETTEXT_SCHEME from GIT-CFLAGS

This variable is used only by shell scripts, not by C programs. It was originally included in GIT-CFLAGS as part of ad17ea7 (add a Makefile switch to avoid gettext translation in shell scripts, 2012-01-23), in an attempt to trigger rebuilding when the variable changed.

However, shell scripts do not respect GIT-CFLAGS. Later, e4dd89a (Makefile: update scripts when build-time parameters change, 2012-06-20) introduced a separate GIT-SCRIPT-DEFINES to accomplish this, which also included USE_GETTEXT_SCHEME. We can drop the redundant and useless mention in GIT-CFLAGS.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to Makefile +1 −1
diff --git a/Makefile b/Makefile
index dddaf4f..3cd4a92 100644
--- a/Makefile
+++ b/Makefile
@@ -2164,7 +2164,7 @@ GIT-PREFIX: FORCE
 		echo "$$FLAGS" >GIT-PREFIX; \
 	fi
 
-TRACK_CFLAGS = $(CC):$(subst ','\'',$(ALL_CFLAGS)):$(USE_GETTEXT_SCHEME)
+TRACK_CFLAGS = $(CC):$(subst ','\'',$(ALL_CFLAGS))
 
 GIT-CFLAGS: FORCE
 	@FLAGS='$(TRACK_CFLAGS)'; \

-- 
1.8.5.2.500.g8060133
Jeff King· Feb 5, 2014, 17:49 UTC · re: Jeff King · lore

[PATCH 02/13] Makefile: fix git-instaweb dependency on gitweb

We build git-instaweb if NO_PERL is not defined, and it in turns depends on "gitweb", which causes us to descend into the gitweb subdir and recursively invoke make. But because gitweb is marked as .PHONY, this forces a rebuild of git-instaweb every time we run make.

We can drop the dependency of git-instaweb on gitweb. However, that means we do not recurse into "gitweb" at all, so we have to add back in the dependency to "all".

Note that the other subdirectories (like git-gui/ or perl/) are handled directly as build commands of "all", and we could simply do the same thing here. However, this patch keeps "gitweb" as a target so that "make gitweb" continues to work, in case anybody is using that.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to Makefile +2 −1
diff --git a/Makefile b/Makefile
index 3cd4a92..1f3e5d9 100644
--- a/Makefile
+++ b/Makefile
@@ -1818,8 +1818,9 @@ GIT-PERL-DEFINES: FORCE
 .PHONY: gitweb
 gitweb:
 	$(QUIET_SUBDIR0)gitweb $(QUIET_SUBDIR1) all
+all:: gitweb
 
-git-instaweb: git-instaweb.sh gitweb GIT-SCRIPT-DEFINES
+git-instaweb: git-instaweb.sh GIT-SCRIPT-DEFINES
 	$(QUIET_GEN)$(cmd_munge_script) && \
 	chmod +x $@+ && \
 	mv $@+ $@

-- 
1.8.5.2.500.g8060133
Jeff King· Feb 5, 2014, 17:50 UTC · re: Jeff King · lore

[PATCH 03/13] Makefile: introduce make-var helper function

It's a common pattern in our Makefile to echo some make variables into a file, but only if they are different from a previous run. This sentinel file can then be used as a dependency to trigger rebuilds when the make variable changes.

The code to do this is a bit ugly and repetetive; let's factor it out into a reusable function.

Note that this relies on the "call" and "eval" functions of GNU make. We previously avoided using "call", as explained in 39c015c (Fixes for ancient versions of GNU make, 2006-02-18). However, it has been 8 years since then, so perhaps its use is acceptable now.

The "call" function dates back to GNU make 3.77.90 (1997-07-21). The "eval" function dates back to 3.80 (2002-07-08).

If it's still a problem to use these functions, we can do similar meta-programming with something like:

	include magic.mak
	magic.mak:
		./generate-magic-rules >$@+
		mv $@+ $@
where the rules are generated by a shell (or other) script.
Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile | 105 ++++++++++++++++++++++++---------------------------------------
 1 file changed, 40 insertions(+), 65 deletions(-)
Show changes to Makefile +40 −65
diff --git a/Makefile b/Makefile
index 1f3e5d9..50bf252 100644
--- a/Makefile
+++ b/Makefile
@@ -1561,6 +1561,20 @@ ifneq ("$(PROFILE)","")
 endif
 endif
 
+# usage: $(eval $(call make-var,FN,DESC,CONTENTS))
+#
+# Create a rule to write $CONTENTS (which should come from a make variable)
+# to GIT-$FN, but only if not already there. This can be used to create a
+# dependency on a Makefile variable. Prints $DESC to the user.
+define make-var
+GIT-$1: FORCE
+	@VALUE='$$(subst ','\'',$3)'; \
+	if test x"$$$$VALUE" != x"`cat $$@ 2>/dev/null`"; then \
+		echo >&2 "    * new $2"; \
+		echo "$$$$VALUE" >$$@; \
+	fi
+endef
+
 # Shell quote (do not use $(call) to accommodate ancient setups);
 
 SHA1_HEADER_SQ = $(subst ','\'',$(SHA1_HEADER))
@@ -1615,13 +1629,9 @@ SHELL_PATH_CQ_SQ = $(subst ','\'',$(SHELL_PATH_CQ))
 BASIC_CFLAGS += -DSHELL_PATH='$(SHELL_PATH_CQ_SQ)'
 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))
-GIT-USER-AGENT: FORCE
-	@if test x'$(GIT_USER_AGENT_SQ)' != x"`cat GIT-USER-AGENT 2>/dev/null`"; then \
-		echo '$(GIT_USER_AGENT_SQ)' >GIT-USER-AGENT; \
-	fi
+$(eval $(call make-var,USER-AGENT,user agent string,$(GIT_USER_AGENT)))
 
 ifdef DEFAULT_HELP_FORMAT
 BASIC_CFLAGS += -DDEFAULT_HELP_FORMAT='"$(DEFAULT_HELP_FORMAT)"'
@@ -1737,9 +1747,17 @@ common-cmds.h: ./generate-cmdlist.sh command-list.txt
 common-cmds.h: $(wildcard Documentation/git-*.txt)
 	$(QUIET_GEN)./generate-cmdlist.sh > $@+ && mv $@+ $@
 
-SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
-	$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
-	$(gitwebdir_SQ):$(PERL_PATH_SQ)
+$(eval $(call make-var,SCRIPT-DEFINES,script parameters,\
+	:$(SHELL_PATH)\
+	:$(DIFF)\
+	:$(GIT_VERSION)\
+	:$(localedir)\
+	:$(NO_CURL)\
+	:$(USE_GETTEXT_SCHEME)\
+	:$(SANE_TOOL_PATH)\
+	:$(gitwebdir)\
+	:$(PERL_PATH)\
+))
 define cmd_munge_script
 $(RM) $@ $@+ && \
 sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
@@ -1754,14 +1772,6 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
     $@.sh >$@+
 endef
 
-GIT-SCRIPT-DEFINES: FORCE
-	@FLAGS='$(SCRIPT_DEFINES)'; \
-	    if test x"$$FLAGS" != x"`cat $@ 2>/dev/null`" ; then \
-		echo >&2 "    * new script parameters"; \
-		echo "$$FLAGS" >$@; \
-            fi
-
-
 $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh GIT-SCRIPT-DEFINES
 	$(QUIET_GEN)$(cmd_munge_script) && \
 	chmod +x $@+ && \
@@ -1789,7 +1799,10 @@ perl/PM.stamp: FORCE
 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)
 
-PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ)
+$(eval $(call make-var,PERL-DEFINES,perl-specific parameters,\
+	:$(PERL_PATH)\
+	:$(PERLLIB_EXTRA)\
+))
 $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl perl/perl.mak GIT-PERL-DEFINES GIT-VERSION-FILE
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \
@@ -1807,14 +1820,6 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl perl/perl.mak GIT-PERL-DEFINES G
 	chmod +x $@+ && \
 	mv $@+ $@
 
-GIT-PERL-DEFINES: FORCE
-	@FLAGS='$(PERL_DEFINES)'; \
-	    if test x"$$FLAGS" != x"`cat $@ 2>/dev/null`" ; then \
-		echo >&2 "    * new perl-specific parameters"; \
-		echo "$$FLAGS" >$@; \
-	    fi
-
-
 .PHONY: gitweb
 gitweb:
 	$(QUIET_SUBDIR0)gitweb $(QUIET_SUBDIR1) all
@@ -1835,6 +1840,7 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)) git-instaweb: % : unimplemented.sh
 endif # NO_PERL
 
 ifndef NO_PYTHON
+$(eval $(call make-var,PYTHON-VARS,Python interpreter location,$(PYTHON_PATH)))
 $(SCRIPT_PYTHON_GEN): GIT-CFLAGS GIT-PREFIX GIT-PYTHON-VARS
 $(SCRIPT_PYTHON_GEN): % : %.py
 	$(QUIET_GEN)$(RM) $@ $@+ && \
@@ -2154,34 +2160,15 @@ cscope:
 	$(RM) cscope*
 	$(FIND_SOURCE_FILES) | xargs cscope -b
 
-### Detect prefix changes
-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 >&2 "    * new prefix flags"; \
-		echo "$$FLAGS" >GIT-PREFIX; \
-	fi
-
-TRACK_CFLAGS = $(CC):$(subst ','\'',$(ALL_CFLAGS))
-
-GIT-CFLAGS: FORCE
-	@FLAGS='$(TRACK_CFLAGS)'; \
-	    if test x"$$FLAGS" != x"`cat GIT-CFLAGS 2>/dev/null`" ; then \
-		echo >&2 "    * new build flags"; \
-		echo "$$FLAGS" >GIT-CFLAGS; \
-            fi
-
-TRACK_LDFLAGS = $(subst ','\'',$(ALL_LDFLAGS))
-
-GIT-LDFLAGS: FORCE
-	@FLAGS='$(TRACK_LDFLAGS)'; \
-	    if test x"$$FLAGS" != x"`cat GIT-LDFLAGS 2>/dev/null`" ; then \
-		echo >&2 "    * new link flags"; \
-		echo "$$FLAGS" >GIT-LDFLAGS; \
-            fi
+$(eval $(call make-var,PREFIX,prefix flags,\
+	:$(bindir)\
+	:$(gitexecdir)\
+	:$(template_dir)\
+	:$(prefix)\
+	:$(localedir)\
+))
+$(eval $(call make-var,CFLAGS,build flags,$(ALL_CFLAGS)))
+$(eval $(call make-var,LDFLAGS,link flags,$(ALL_LDFLAGS)))
 
 # We need to apply sq twice, once to protect from the shell
 # that runs GIT-BUILD-OPTIONS, and then again to protect it
@@ -2224,18 +2211,6 @@ ifdef GIT_PERF_MAKE_OPTS
 	@echo GIT_PERF_MAKE_OPTS=\''$(subst ','\'',$(subst ','\'',$(GIT_PERF_MAKE_OPTS)))'\' >>$@
 endif
 
-### Detect Python interpreter path changes
-ifndef NO_PYTHON
-TRACK_PYTHON = $(subst ','\'',-DPYTHON_PATH='$(PYTHON_PATH_SQ)')
-
-GIT-PYTHON-VARS: FORCE
-	@VARS='$(TRACK_PYTHON)'; \
-	    if test x"$$VARS" != x"`cat $@ 2>/dev/null`" ; then \
-		echo >&2 "    * new Python interpreter location"; \
-		echo "$$VARS" >$@; \
-            fi
-endif
-
 test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
 
 all:: $(TEST_PROGRAMS) $(test_bindir_programs)

-- 
1.8.5.2.500.g8060133
Eric Sunshine· Feb 6, 2014, 08:55 UTC · re: Jeff King · lore

Re: [PATCH 03/13] Makefile: introduce make-var helper function

On Wed, Feb 5, 2014 at 12:50 PM, Jeff King <peff@peff.net> wrote:
Show 7 quoted lines
> It's a common pattern in our Makefile to echo some make
> variables into a file, but only if they are different from a
> previous run. This sentinel file can then be used as a
> dependency to trigger rebuilds when the make variable
> changes.
>
> The code to do this is a bit ugly and repetetive; let's
s/repetetive/repetitive/
Show 23 quoted lines
> factor it out into a reusable function.
>
> Note that this relies on the "call" and "eval" functions of
> GNU make. We previously avoided using "call", as explained
> in 39c015c (Fixes for ancient versions of GNU make,
> 2006-02-18). However, it has been 8 years since then, so
> perhaps its use is acceptable now.
>
> The "call" function dates back to GNU make 3.77.90
> (1997-07-21). The "eval" function dates back to 3.80
> (2002-07-08).
>
> If it's still a problem to use these functions, we can
> do similar meta-programming with something like:
>
>         include magic.mak
>         magic.mak:
>                 ./generate-magic-rules >$@+
>                 mv $@+ $@
>
> where the rules are generated by a shell (or other) script.
>
> Signed-off-by: Jeff King <peff@peff.net>
Jeff King· Feb 5, 2014, 17:50 UTC · re: Jeff King · lore

[PATCH 04/13] Makefile: use tempfile/mv strategy for GIT-*

We create GIT-CFLAGS and related files by echoing into a shell redirection. It's possible for an error to cause the file to end up empty or truncated. In theory, this could cause us to later make an incorrect comparison of the file contents to a Makefile variable, and avoid rebuilding some dependencies.

In practice, this is very unlikely to happen, as the followup run would have to have changed the variable to match the truncation exactly. However, it is good hygiene to use our usual tempfile + mv trick to make sure that the file is created atomically.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to Makefile +2 −1
diff --git a/Makefile b/Makefile
index 50bf252..b06d5ec 100644
--- a/Makefile
+++ b/Makefile
@@ -1571,7 +1571,8 @@ GIT-$1: FORCE
 	@VALUE='$$(subst ','\'',$3)'; \
 	if test x"$$$$VALUE" != x"`cat $$@ 2>/dev/null`"; then \
 		echo >&2 "    * new $2"; \
-		echo "$$$$VALUE" >$$@; \
+		echo "$$$$VALUE" >$$@+ && \
+		mv $$@+ $$@; \
 	fi
 endef
 

-- 
1.8.5.2.500.g8060133
Jeff King· Feb 5, 2014, 17:50 UTC · re: Jeff King · lore

[PATCH 05/13] Makefile: prefer printf to echo for GIT-*

When we write Makefile variables to a sentinel file, we use "echo" to do so. Since we are writing arbitrary data which may contain backslash escapes (particularly with file paths on Windows), echo may or may not expand those escapes, depending on which shell is in use.

During the next run of "make", we check the sentinel file to see if it is different than the Makefile variable. If escapes were expanded, then we will erroneously think it changed and trigger a rebuild. You can see this easily by running:

  make prefix='foo\bar'
multiple times; it will re-build some files repeatedly.
Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to Makefile +1 −1
diff --git a/Makefile b/Makefile
index b06d5ec..60dc53b 100644
--- a/Makefile
+++ b/Makefile
@@ -1571,7 +1571,7 @@ GIT-$1: FORCE
 	@VALUE='$$(subst ','\'',$3)'; \
 	if test x"$$$$VALUE" != x"`cat $$@ 2>/dev/null`"; then \
 		echo >&2 "    * new $2"; \
-		echo "$$$$VALUE" >$$@+ && \
+		printf '%s\n' "$$$$VALUE" >$$@+ && \
 		mv $$@+ $$@; \
 	fi
 endef

-- 
1.8.5.2.500.g8060133
Jeff King· Feb 5, 2014, 17:52 UTC · re: Jeff King · lore

[PATCH 06/13] Makefile: store GIT-* sentinel files in MAKE/

The Makefile creates files like GIT-CFLAGS to keep track of the make variables used in the last build. We have a number of these files now, all starting with GIT-*. Let's move them into their own subdirectory, which has two advantages:

  1. It's less clutter in the main directory.
  2. Each file needs to be marked separately in .gitignore.
     With a subdirectory, we can easily mark them all with a
     wildcard (we cannot do a wildcard for GIT-* because
     of precious files like GIT-VERSION-GEN).

This patch moves all of the generated GIT-* files into MAKE/*, with one exception: GIT-VERSION-FILE. This could be moved along with the rest, but there is a reasonable chance that there are some out-of-tree scripts that may peek at it (whereas things like GIT-CFLAGS are purely internal, and we update all internal references).

Signed-off-by: Jeff King <peff@peff.net>
---
I'm not married to the name "MAKE", but I do think the separate
directory is a win for simplicity and avoiding repetition (as you can
see in the diffstat). Other suggestions welcome.
 .gitignore            |  8 -------
 MAKE/.gitignore       |  1 +
 Makefile              | 66 +++++++++++++++++++++++++--------------------------
 perl/Makefile         | 10 ++++----
 t/perf/run            |  2 +-
 t/test-lib.sh         |  2 +-
 t/valgrind/analyze.sh |  4 ++--
 7 files changed, 42 insertions(+), 51 deletions(-)
 create mode 100644 MAKE/.gitignore
Show changes to 7 files +42 −51

.gitignore, MAKE/.gitignore, Makefile, perl/Makefile, t/perf/run, t/test-lib.sh, t/valgrind/analyze.sh

diff --git a/.gitignore b/.gitignore
index b5f9def..586a067 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,11 +1,3 @@
-/GIT-BUILD-OPTIONS
-/GIT-CFLAGS
-/GIT-LDFLAGS
-/GIT-PREFIX
-/GIT-PERL-DEFINES
-/GIT-PYTHON-VARS
-/GIT-SCRIPT-DEFINES
-/GIT-USER-AGENT
 /GIT-VERSION-FILE
 /bin-wrappers/
 /git
diff --git a/MAKE/.gitignore b/MAKE/.gitignore
new file mode 100644
index 0000000..72e8ffc
--- /dev/null
+++ b/MAKE/.gitignore
@@ -0,0 +1 @@
+*
diff --git a/Makefile b/Makefile
index 60dc53b..7fecdf1 100644
--- a/Makefile
+++ b/Makefile
@@ -1564,10 +1564,10 @@ endif
 # usage: $(eval $(call make-var,FN,DESC,CONTENTS))
 #
 # Create a rule to write $CONTENTS (which should come from a make variable)
-# to GIT-$FN, but only if not already there. This can be used to create a
+# to MAKE/$FN, but only if not already there. This can be used to create a
 # dependency on a Makefile variable. Prints $DESC to the user.
 define make-var
-GIT-$1: FORCE
+MAKE/$1: FORCE
 	@VALUE='$$(subst ','\'',$3)'; \
 	if test x"$$$$VALUE" != x"`cat $$@ 2>/dev/null`"; then \
 		echo >&2 "    * new $2"; \
@@ -1658,7 +1658,7 @@ all:: profile-clean
 endif
 endif
 
-all:: $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
+all:: $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) MAKE/BUILD-OPTIONS
 ifneq (,$X)
 	$(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';)
 endif
@@ -1714,25 +1714,25 @@ strip: $(PROGRAMS) git$X
 #   dependencies here will not need to change if the force-build
 #   details change some day.
 
-git.sp git.s git.o: GIT-PREFIX
+git.sp git.s git.o: MAKE/PREFIX
 git.sp git.s git.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
 
-git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
+git$X: git.o MAKE/LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ git.o \
 		$(BUILTIN_OBJS) $(ALL_LDFLAGS) $(LIBS)
 
 help.sp help.s 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: common-cmds.h MAKE/PREFIX
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
 
-version.sp version.s version.o: GIT-VERSION-FILE GIT-USER-AGENT
+version.sp version.s version.o: GIT-VERSION-FILE MAKE/USER-AGENT
 version.sp version.s version.o: EXTRA_CPPFLAGS = \
 	'-DGIT_VERSION="$(GIT_VERSION)"' \
 	'-DGIT_USER_AGENT=$(GIT_USER_AGENT_CQ_SQ)'
@@ -1773,12 +1773,12 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
     $@.sh >$@+
 endef
 
-$(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh GIT-SCRIPT-DEFINES
+$(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh MAKE/SCRIPT-DEFINES
 	$(QUIET_GEN)$(cmd_munge_script) && \
 	chmod +x $@+ && \
 	mv $@+ $@
 
-$(SCRIPT_LIB) : % : %.sh GIT-SCRIPT-DEFINES
+$(SCRIPT_LIB) : % : %.sh MAKE/SCRIPT-DEFINES
 	$(QUIET_GEN)$(cmd_munge_script) && \
 	mv $@+ $@
 
@@ -1797,14 +1797,14 @@ perl/PM.stamp: FORCE
 	{ cmp $@+ $@ >/dev/null 2>/dev/null || mv $@+ $@; } && \
 	$(RM) $@+
 
-perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL
+perl/perl.mak: MAKE/CFLAGS MAKE/PREFIX perl/Makefile perl/Makefile.PL
 	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F)
 
 $(eval $(call make-var,PERL-DEFINES,perl-specific parameters,\
 	:$(PERL_PATH)\
 	:$(PERLLIB_EXTRA)\
 ))
-$(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl perl/perl.mak GIT-PERL-DEFINES GIT-VERSION-FILE
+$(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl perl/perl.mak MAKE/PERL-DEFINES GIT-VERSION-FILE
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \
 	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
@@ -1826,7 +1826,7 @@ gitweb:
 	$(QUIET_SUBDIR0)gitweb $(QUIET_SUBDIR1) all
 all:: gitweb
 
-git-instaweb: git-instaweb.sh GIT-SCRIPT-DEFINES
+git-instaweb: git-instaweb.sh MAKE/SCRIPT-DEFINES
 	$(QUIET_GEN)$(cmd_munge_script) && \
 	chmod +x $@+ && \
 	mv $@+ $@
@@ -1842,7 +1842,7 @@ endif # NO_PERL
 
 ifndef NO_PYTHON
 $(eval $(call make-var,PYTHON-VARS,Python interpreter location,$(PYTHON_PATH)))
-$(SCRIPT_PYTHON_GEN): GIT-CFLAGS GIT-PREFIX GIT-PYTHON-VARS
+$(SCRIPT_PYTHON_GEN): MAKE/CFLAGS MAKE/PREFIX MAKE/PYTHON-VARS
 $(SCRIPT_PYTHON_GEN): % : %.py
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	sed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
@@ -1984,13 +1984,13 @@ endif
 endif
 
 ifndef CHECK_HEADER_DEPENDENCIES
-$(C_OBJ): %.o: %.c GIT-CFLAGS $(missing_dep_dirs)
+$(C_OBJ): %.o: %.c MAKE/CFLAGS $(missing_dep_dirs)
 	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
-$(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs)
+$(ASM_OBJ): %.o: %.S MAKE/CFLAGS $(missing_dep_dirs)
 	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
 endif
 
-%.s: %.c GIT-CFLAGS FORCE
+%.s: %.c MAKE/CFLAGS FORCE
 	$(QUIET_CC)$(CC) -o $@ -S $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
 
 ifdef USE_COMPUTED_HEADER_DEPENDENCIES
@@ -2011,25 +2011,25 @@ else
 $(OBJECTS): $(LIB_H)
 endif
 
-exec_cmd.sp exec_cmd.s exec_cmd.o: GIT-PREFIX
+exec_cmd.sp exec_cmd.s exec_cmd.o: MAKE/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: MAKE/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: MAKE/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: MAKE/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: MAKE/PREFIX
 gettext.sp gettext.s gettext.o: EXTRA_CPPFLAGS = \
 	-DGIT_LOCALE_PATH='"$(localedir_SQ)"'
 
@@ -2051,21 +2051,21 @@ compat/nedmalloc/nedmalloc.sp compat/nedmalloc/nedmalloc.o: EXTRA_CPPFLAGS = \
 compat/nedmalloc/nedmalloc.sp: SPARSE_FLAGS += -Wno-non-pointer-null
 endif
 
-git-%$X: %.o GIT-LDFLAGS $(GITLIBS)
+git-%$X: %.o MAKE/LDFLAGS $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
 
-git-imap-send$X: imap-send.o GIT-LDFLAGS $(GITLIBS)
+git-imap-send$X: imap-send.o MAKE/LDFLAGS $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)
 
-git-http-fetch$X: http.o http-walker.o http-fetch.o GIT-LDFLAGS $(GITLIBS)
+git-http-fetch$X: http.o http-walker.o http-fetch.o MAKE/LDFLAGS $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(LIBS) $(CURL_LIBCURL)
-git-http-push$X: http.o http-push.o GIT-LDFLAGS $(GITLIBS)
+git-http-push$X: http.o http-push.o MAKE/LDFLAGS $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
 
-git-remote-testsvn$X: remote-testsvn.o GIT-LDFLAGS $(GITLIBS) $(VCSSVN_LIB)
+git-remote-testsvn$X: remote-testsvn.o MAKE/LDFLAGS $(GITLIBS) $(VCSSVN_LIB)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS) \
 	$(VCSSVN_LIB)
 
@@ -2075,7 +2075,7 @@ $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY)
 	ln -s $< $@ 2>/dev/null || \
 	cp $< $@
 
-$(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS)
+$(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o MAKE/LDFLAGS $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
 
@@ -2172,9 +2172,9 @@ $(eval $(call make-var,CFLAGS,build flags,$(ALL_CFLAGS)))
 $(eval $(call make-var,LDFLAGS,link flags,$(ALL_LDFLAGS)))
 
 # We need to apply sq twice, once to protect from the shell
-# that runs GIT-BUILD-OPTIONS, and then again to protect it
+# that runs MAKE/BUILD-OPTIONS, and then again to protect it
 # and the first level quoting from the shell that runs "echo".
-GIT-BUILD-OPTIONS: FORCE
+MAKE/BUILD-OPTIONS: FORCE
 	@echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@
 	@echo PERL_PATH=\''$(subst ','\'',$(PERL_PATH_SQ))'\' >>$@
 	@echo DIFF=\''$(subst ','\'',$(subst ','\'',$(DIFF)))'\' >>$@
@@ -2255,7 +2255,7 @@ test-svn-fe$X: vcs-svn/lib.a
 
 .PRECIOUS: $(TEST_OBJS)
 
-test-%$X: test-%.o GIT-LDFLAGS $(GITLIBS)
+test-%$X: test-%.o MAKE/LDFLAGS $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
 
 check-sha1:: test-sha1$X
@@ -2263,7 +2263,7 @@ check-sha1:: test-sha1$X
 
 SP_OBJ = $(patsubst %.o,%.sp,$(C_OBJ))
 
-$(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
+$(SP_OBJ): %.sp: %.c MAKE/CFLAGS FORCE
 	$(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \
 		$(SPARSE_FLAGS) $<
 
@@ -2477,9 +2477,7 @@ ifndef NO_TCLTK
 	$(MAKE) -C gitk-git clean
 	$(MAKE) -C git-gui clean
 endif
-	$(RM) GIT-VERSION-FILE GIT-CFLAGS GIT-LDFLAGS GIT-BUILD-OPTIONS
-	$(RM) GIT-USER-AGENT GIT-PREFIX
-	$(RM) GIT-SCRIPT-DEFINES GIT-PERL-DEFINES GIT-PYTHON-VARS
+	$(RM) GIT-VERSION-FILE MAKE/*
 
 .PHONY: all install profile-clean clean strip
 .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
diff --git a/perl/Makefile b/perl/Makefile
index 15d96fc..b27420e 100644
--- a/perl/Makefile
+++ b/perl/Makefile
@@ -41,7 +41,7 @@ modules += Git/SVN/Prompt
 modules += Git/SVN/Ra
 modules += Git/SVN/Utils
 
-$(makfile): ../GIT-CFLAGS Makefile
+$(makfile): ../MAKE/CFLAGS Makefile
 	echo all: private-Error.pm Git.pm Git/I18N.pm > $@
 	set -e; \
 	for i in $(modules); \
@@ -79,11 +79,11 @@ $(makfile): ../GIT-CFLAGS Makefile
 	echo instlibdir: >> $@
 	echo '	echo $(instdir_SQ)' >> $@
 else
-$(makfile): Makefile.PL ../GIT-CFLAGS
+$(makfile): Makefile.PL ../MAKE/CFLAGS
 	$(PERL_PATH) $< PREFIX='$(prefix_SQ)' INSTALL_BASE='' --localedir='$(localedir_SQ)'
 endif
 
 # this is just added comfort for calling make directly in perl dir
-# (even though GIT-CFLAGS aren't used yet. If ever)
-../GIT-CFLAGS:
-	$(MAKE) -C .. GIT-CFLAGS
+# (even though MAKE/CFLAGS aren't used yet. If ever)
+../MAKE/CFLAGS:
+	$(MAKE) -C .. MAKE/CFLAGS
diff --git a/t/perf/run b/t/perf/run
index cfd7012..489d6cb 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -73,7 +73,7 @@ GIT_PERF_AGGREGATING_LATER=t
 export GIT_PERF_AGGREGATING_LATER
 
 cd "$(dirname $0)"
-. ../../GIT-BUILD-OPTIONS
+. ../../MAKE/BUILD-OPTIONS
 
 if test $# = 0 -o "$1" = -- -o -f "$1"; then
 	set -- . "$@"
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1531c24..2c1ce73 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -48,7 +48,7 @@ then
 	exit 1
 fi
 
-. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
+. "$GIT_BUILD_DIR"/MAKE/BUILD-OPTIONS
 export PERL_PATH SHELL_PATH
 
 # if --tee was passed, write the output not only to the terminal, but
diff --git a/t/valgrind/analyze.sh b/t/valgrind/analyze.sh
index 2ffc80f..346e0ff 100755
--- a/t/valgrind/analyze.sh
+++ b/t/valgrind/analyze.sh
@@ -1,7 +1,7 @@
 #!/bin/sh
 
-# Get TEST_OUTPUT_DIRECTORY from GIT-BUILD-OPTIONS if it's there...
-. "$(dirname "$0")/../../GIT-BUILD-OPTIONS"
+# Get TEST_OUTPUT_DIRECTORY from MAKE/BUILD-OPTIONS if it's there...
+. "$(dirname "$0")/../../MAKE/BUILD-OPTIONS"
 # ... otherwise set it to the default value.
 : ${TEST_OUTPUT_DIRECTORY=$(dirname "$0")/..}
 

-- 
1.8.5.2.500.g8060133
Junio C Hamano· Feb 5, 2014, 19:05 UTC · re: Jeff King · lore

Re: [PATCH 06/13] Makefile: store GIT-* sentinel files in MAKE/

Jeff King <peff@peff.net> writes:
Show 6 quoted lines
> This patch moves all of the generated GIT-* files into
> MAKE/*, with one exception: GIT-VERSION-FILE. This could be
> moved along with the rest, but there is a reasonable chance
> that there are some out-of-tree scripts that may peek at it
> (whereas things like GIT-CFLAGS are purely internal, and we
> update all internal references).

OK. We do have internal references to include the version file to Makefiles in subdirectories, which we could adjust if we wanted to. I tend to agree that leaving it there would be a safer thing to do, but at the same time I think it is OK to move it if we wanted to. The third parties need to adjust and they are capable of adjusting.

Thanks. The changes look good. I do not have a strong opinion on the name, but I do agree a separate directory unclutters things in a very good way.

Show 338 quoted lines
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I'm not married to the name "MAKE", but I do think the separate
> directory is a win for simplicity and avoiding repetition (as you can
> see in the diffstat). Other suggestions welcome.
>
>  .gitignore            |  8 -------
>  MAKE/.gitignore       |  1 +
>  Makefile              | 66 +++++++++++++++++++++++++--------------------------
>  perl/Makefile         | 10 ++++----
>  t/perf/run            |  2 +-
>  t/test-lib.sh         |  2 +-
>  t/valgrind/analyze.sh |  4 ++--
>  7 files changed, 42 insertions(+), 51 deletions(-)
>  create mode 100644 MAKE/.gitignore
>
> diff --git a/.gitignore b/.gitignore
> index b5f9def..586a067 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -1,11 +1,3 @@
> -/GIT-BUILD-OPTIONS
> -/GIT-CFLAGS
> -/GIT-LDFLAGS
> -/GIT-PREFIX
> -/GIT-PERL-DEFINES
> -/GIT-PYTHON-VARS
> -/GIT-SCRIPT-DEFINES
> -/GIT-USER-AGENT
>  /GIT-VERSION-FILE
>  /bin-wrappers/
>  /git
> diff --git a/MAKE/.gitignore b/MAKE/.gitignore
> new file mode 100644
> index 0000000..72e8ffc
> --- /dev/null
> +++ b/MAKE/.gitignore
> @@ -0,0 +1 @@
> +*
> diff --git a/Makefile b/Makefile
> index 60dc53b..7fecdf1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1564,10 +1564,10 @@ endif
>  # usage: $(eval $(call make-var,FN,DESC,CONTENTS))
>  #
>  # Create a rule to write $CONTENTS (which should come from a make variable)
> -# to GIT-$FN, but only if not already there. This can be used to create a
> +# to MAKE/$FN, but only if not already there. This can be used to create a
>  # dependency on a Makefile variable. Prints $DESC to the user.
>  define make-var
> -GIT-$1: FORCE
> +MAKE/$1: FORCE
>  	@VALUE='$$(subst ','\'',$3)'; \
>  	if test x"$$$$VALUE" != x"`cat $$@ 2>/dev/null`"; then \
>  		echo >&2 "    * new $2"; \
> @@ -1658,7 +1658,7 @@ all:: profile-clean
>  endif
>  endif
>  
> -all:: $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
> +all:: $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) MAKE/BUILD-OPTIONS
>  ifneq (,$X)
>  	$(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';)
>  endif
> @@ -1714,25 +1714,25 @@ strip: $(PROGRAMS) git$X
>  #   dependencies here will not need to change if the force-build
>  #   details change some day.
>  
> -git.sp git.s git.o: GIT-PREFIX
> +git.sp git.s git.o: MAKE/PREFIX
>  git.sp git.s git.o: EXTRA_CPPFLAGS = \
>  	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
>  	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
>  	'-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
>  
> -git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
> +git$X: git.o MAKE/LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ git.o \
>  		$(BUILTIN_OBJS) $(ALL_LDFLAGS) $(LIBS)
>  
>  help.sp help.s 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: common-cmds.h MAKE/PREFIX
>  builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
>  	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
>  	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
>  	'-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
>  
> -version.sp version.s version.o: GIT-VERSION-FILE GIT-USER-AGENT
> +version.sp version.s version.o: GIT-VERSION-FILE MAKE/USER-AGENT
>  version.sp version.s version.o: EXTRA_CPPFLAGS = \
>  	'-DGIT_VERSION="$(GIT_VERSION)"' \
>  	'-DGIT_USER_AGENT=$(GIT_USER_AGENT_CQ_SQ)'
> @@ -1773,12 +1773,12 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
>      $@.sh >$@+
>  endef
>  
> -$(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh GIT-SCRIPT-DEFINES
> +$(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh MAKE/SCRIPT-DEFINES
>  	$(QUIET_GEN)$(cmd_munge_script) && \
>  	chmod +x $@+ && \
>  	mv $@+ $@
>  
> -$(SCRIPT_LIB) : % : %.sh GIT-SCRIPT-DEFINES
> +$(SCRIPT_LIB) : % : %.sh MAKE/SCRIPT-DEFINES
>  	$(QUIET_GEN)$(cmd_munge_script) && \
>  	mv $@+ $@
>  
> @@ -1797,14 +1797,14 @@ perl/PM.stamp: FORCE
>  	{ cmp $@+ $@ >/dev/null 2>/dev/null || mv $@+ $@; } && \
>  	$(RM) $@+
>  
> -perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL
> +perl/perl.mak: MAKE/CFLAGS MAKE/PREFIX perl/Makefile perl/Makefile.PL
>  	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F)
>  
>  $(eval $(call make-var,PERL-DEFINES,perl-specific parameters,\
>  	:$(PERL_PATH)\
>  	:$(PERLLIB_EXTRA)\
>  ))
> -$(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl perl/perl.mak GIT-PERL-DEFINES GIT-VERSION-FILE
> +$(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl perl/perl.mak MAKE/PERL-DEFINES GIT-VERSION-FILE
>  	$(QUIET_GEN)$(RM) $@ $@+ && \
>  	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \
>  	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
> @@ -1826,7 +1826,7 @@ gitweb:
>  	$(QUIET_SUBDIR0)gitweb $(QUIET_SUBDIR1) all
>  all:: gitweb
>  
> -git-instaweb: git-instaweb.sh GIT-SCRIPT-DEFINES
> +git-instaweb: git-instaweb.sh MAKE/SCRIPT-DEFINES
>  	$(QUIET_GEN)$(cmd_munge_script) && \
>  	chmod +x $@+ && \
>  	mv $@+ $@
> @@ -1842,7 +1842,7 @@ endif # NO_PERL
>  
>  ifndef NO_PYTHON
>  $(eval $(call make-var,PYTHON-VARS,Python interpreter location,$(PYTHON_PATH)))
> -$(SCRIPT_PYTHON_GEN): GIT-CFLAGS GIT-PREFIX GIT-PYTHON-VARS
> +$(SCRIPT_PYTHON_GEN): MAKE/CFLAGS MAKE/PREFIX MAKE/PYTHON-VARS
>  $(SCRIPT_PYTHON_GEN): % : %.py
>  	$(QUIET_GEN)$(RM) $@ $@+ && \
>  	sed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
> @@ -1984,13 +1984,13 @@ endif
>  endif
>  
>  ifndef CHECK_HEADER_DEPENDENCIES
> -$(C_OBJ): %.o: %.c GIT-CFLAGS $(missing_dep_dirs)
> +$(C_OBJ): %.o: %.c MAKE/CFLAGS $(missing_dep_dirs)
>  	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
> -$(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs)
> +$(ASM_OBJ): %.o: %.S MAKE/CFLAGS $(missing_dep_dirs)
>  	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
>  endif
>  
> -%.s: %.c GIT-CFLAGS FORCE
> +%.s: %.c MAKE/CFLAGS FORCE
>  	$(QUIET_CC)$(CC) -o $@ -S $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
>  
>  ifdef USE_COMPUTED_HEADER_DEPENDENCIES
> @@ -2011,25 +2011,25 @@ else
>  $(OBJECTS): $(LIB_H)
>  endif
>  
> -exec_cmd.sp exec_cmd.s exec_cmd.o: GIT-PREFIX
> +exec_cmd.sp exec_cmd.s exec_cmd.o: MAKE/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: MAKE/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: MAKE/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: MAKE/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: MAKE/PREFIX
>  gettext.sp gettext.s gettext.o: EXTRA_CPPFLAGS = \
>  	-DGIT_LOCALE_PATH='"$(localedir_SQ)"'
>  
> @@ -2051,21 +2051,21 @@ compat/nedmalloc/nedmalloc.sp compat/nedmalloc/nedmalloc.o: EXTRA_CPPFLAGS = \
>  compat/nedmalloc/nedmalloc.sp: SPARSE_FLAGS += -Wno-non-pointer-null
>  endif
>  
> -git-%$X: %.o GIT-LDFLAGS $(GITLIBS)
> +git-%$X: %.o MAKE/LDFLAGS $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
>  
> -git-imap-send$X: imap-send.o GIT-LDFLAGS $(GITLIBS)
> +git-imap-send$X: imap-send.o MAKE/LDFLAGS $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
>  		$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)
>  
> -git-http-fetch$X: http.o http-walker.o http-fetch.o GIT-LDFLAGS $(GITLIBS)
> +git-http-fetch$X: http.o http-walker.o http-fetch.o MAKE/LDFLAGS $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
>  		$(LIBS) $(CURL_LIBCURL)
> -git-http-push$X: http.o http-push.o GIT-LDFLAGS $(GITLIBS)
> +git-http-push$X: http.o http-push.o MAKE/LDFLAGS $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
>  		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
>  
> -git-remote-testsvn$X: remote-testsvn.o GIT-LDFLAGS $(GITLIBS) $(VCSSVN_LIB)
> +git-remote-testsvn$X: remote-testsvn.o MAKE/LDFLAGS $(GITLIBS) $(VCSSVN_LIB)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS) \
>  	$(VCSSVN_LIB)
>  
> @@ -2075,7 +2075,7 @@ $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY)
>  	ln -s $< $@ 2>/dev/null || \
>  	cp $< $@
>  
> -$(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS)
> +$(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o MAKE/LDFLAGS $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
>  		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
>  
> @@ -2172,9 +2172,9 @@ $(eval $(call make-var,CFLAGS,build flags,$(ALL_CFLAGS)))
>  $(eval $(call make-var,LDFLAGS,link flags,$(ALL_LDFLAGS)))
>  
>  # We need to apply sq twice, once to protect from the shell
> -# that runs GIT-BUILD-OPTIONS, and then again to protect it
> +# that runs MAKE/BUILD-OPTIONS, and then again to protect it
>  # and the first level quoting from the shell that runs "echo".
> -GIT-BUILD-OPTIONS: FORCE
> +MAKE/BUILD-OPTIONS: FORCE
>  	@echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@
>  	@echo PERL_PATH=\''$(subst ','\'',$(PERL_PATH_SQ))'\' >>$@
>  	@echo DIFF=\''$(subst ','\'',$(subst ','\'',$(DIFF)))'\' >>$@
> @@ -2255,7 +2255,7 @@ test-svn-fe$X: vcs-svn/lib.a
>  
>  .PRECIOUS: $(TEST_OBJS)
>  
> -test-%$X: test-%.o GIT-LDFLAGS $(GITLIBS)
> +test-%$X: test-%.o MAKE/LDFLAGS $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
>  
>  check-sha1:: test-sha1$X
> @@ -2263,7 +2263,7 @@ check-sha1:: test-sha1$X
>  
>  SP_OBJ = $(patsubst %.o,%.sp,$(C_OBJ))
>  
> -$(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
> +$(SP_OBJ): %.sp: %.c MAKE/CFLAGS FORCE
>  	$(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \
>  		$(SPARSE_FLAGS) $<
>  
> @@ -2477,9 +2477,7 @@ ifndef NO_TCLTK
>  	$(MAKE) -C gitk-git clean
>  	$(MAKE) -C git-gui clean
>  endif
> -	$(RM) GIT-VERSION-FILE GIT-CFLAGS GIT-LDFLAGS GIT-BUILD-OPTIONS
> -	$(RM) GIT-USER-AGENT GIT-PREFIX
> -	$(RM) GIT-SCRIPT-DEFINES GIT-PERL-DEFINES GIT-PYTHON-VARS
> +	$(RM) GIT-VERSION-FILE MAKE/*
>  
>  .PHONY: all install profile-clean clean strip
>  .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
> diff --git a/perl/Makefile b/perl/Makefile
> index 15d96fc..b27420e 100644
> --- a/perl/Makefile
> +++ b/perl/Makefile
> @@ -41,7 +41,7 @@ modules += Git/SVN/Prompt
>  modules += Git/SVN/Ra
>  modules += Git/SVN/Utils
>  
> -$(makfile): ../GIT-CFLAGS Makefile
> +$(makfile): ../MAKE/CFLAGS Makefile
>  	echo all: private-Error.pm Git.pm Git/I18N.pm > $@
>  	set -e; \
>  	for i in $(modules); \
> @@ -79,11 +79,11 @@ $(makfile): ../GIT-CFLAGS Makefile
>  	echo instlibdir: >> $@
>  	echo '	echo $(instdir_SQ)' >> $@
>  else
> -$(makfile): Makefile.PL ../GIT-CFLAGS
> +$(makfile): Makefile.PL ../MAKE/CFLAGS
>  	$(PERL_PATH) $< PREFIX='$(prefix_SQ)' INSTALL_BASE='' --localedir='$(localedir_SQ)'
>  endif
>  
>  # this is just added comfort for calling make directly in perl dir
> -# (even though GIT-CFLAGS aren't used yet. If ever)
> -../GIT-CFLAGS:
> -	$(MAKE) -C .. GIT-CFLAGS
> +# (even though MAKE/CFLAGS aren't used yet. If ever)
> +../MAKE/CFLAGS:
> +	$(MAKE) -C .. MAKE/CFLAGS
> diff --git a/t/perf/run b/t/perf/run
> index cfd7012..489d6cb 100755
> --- a/t/perf/run
> +++ b/t/perf/run
> @@ -73,7 +73,7 @@ GIT_PERF_AGGREGATING_LATER=t
>  export GIT_PERF_AGGREGATING_LATER
>  
>  cd "$(dirname $0)"
> -. ../../GIT-BUILD-OPTIONS
> +. ../../MAKE/BUILD-OPTIONS
>  
>  if test $# = 0 -o "$1" = -- -o -f "$1"; then
>  	set -- . "$@"
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 1531c24..2c1ce73 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -48,7 +48,7 @@ then
>  	exit 1
>  fi
>  
> -. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
> +. "$GIT_BUILD_DIR"/MAKE/BUILD-OPTIONS
>  export PERL_PATH SHELL_PATH
>  
>  # if --tee was passed, write the output not only to the terminal, but
> diff --git a/t/valgrind/analyze.sh b/t/valgrind/analyze.sh
> index 2ffc80f..346e0ff 100755
> --- a/t/valgrind/analyze.sh
> +++ b/t/valgrind/analyze.sh
> @@ -1,7 +1,7 @@
>  #!/bin/sh
>  
> -# Get TEST_OUTPUT_DIRECTORY from GIT-BUILD-OPTIONS if it's there...
> -. "$(dirname "$0")/../../GIT-BUILD-OPTIONS"
> +# Get TEST_OUTPUT_DIRECTORY from MAKE/BUILD-OPTIONS if it's there...
> +. "$(dirname "$0")/../../MAKE/BUILD-OPTIONS"
>  # ... otherwise set it to the default value.
>  : ${TEST_OUTPUT_DIRECTORY=$(dirname "$0")/..}
Jeff King· Feb 5, 2014, 17:53 UTC · re: Jeff King · lore

[PATCH 07/13] Makefile: always create files via make-var

If we are trying to store an empty sentinel value for a make-var, we would consider a missing file the same as an empty variable. E.g., repeatedly running:

  make GIT_USER_AGENT=

will not create MAKE/USER-AGENT at all if it does not exist, and subsequent make invocations will force a rebuild. This is not generally a problem in practice, since most of the files always have some boilerplate (even LDFLAGS, because it is formed with "+=", will have a stray space in it). But this does fix the rare case, and future-proofs us as we add more similar variables.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to Makefile +1 −1
diff --git a/Makefile b/Makefile
index 7fecdf1..e12039f 100644
--- a/Makefile
+++ b/Makefile
@@ -1569,7 +1569,7 @@ endif
 define make-var
 MAKE/$1: FORCE
 	@VALUE='$$(subst ','\'',$3)'; \
-	if test x"$$$$VALUE" != x"`cat $$@ 2>/dev/null`"; then \
+	if ! test -e $$@ || test x"$$$$VALUE" != x"`cat $$@`"; then \
 		echo >&2 "    * new $2"; \
 		printf '%s\n' "$$$$VALUE" >$$@+ && \
 		mv $$@+ $$@; \

-- 
1.8.5.2.500.g8060133
Jeff King· Feb 5, 2014, 17:57 UTC · re: Jeff King · lore

[PATCH 08/13] Makefile: introduce sq function for shell-quoting

Since we have recently abolished the prohibition on $(call) in our Makefile, we can use it to factor out the repeated shell-quoting we do. There are two upsides:

  1. It is much more readable than inline calls to
     $(subst ','\'').
  2. It is short enough that we can do away with the _SQ
     variants of many variables (note that we do not do this
     just yet, as there is a little more cleanup needed
     first).
Signed-off-by: Jeff King <peff@peff.net>
---
This is the one I am most on the fence about. I think (2) is nice. And
for (1), we do end up more readable in some instances (e.g., the
horrible inline subst calls in generating BUILD-OPTIONS go away). But
many instances are not really any more readable (e.g., see the first
hunk below).
Two things come to mind:
  1. If we really prefer reading $(FOO_SQ) but don't want to manually
     write each one, we could have a simple script that reads the
     Makefile and produces an _SQ variant of every variable, whether we
     need it or not.
  2. The later parts of the series introduce a new way of getting
     values into programs that does not involve the command-line. So in
     theory many of these quoted uses of variables would just go away in
     the long run (if we choose to pursue that direction).
 Makefile | 120 ++++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 64 insertions(+), 56 deletions(-)
Show changes to Makefile +64 −56
diff --git a/Makefile b/Makefile
index e12039f..868872f 100644
--- a/Makefile
+++ b/Makefile
@@ -506,11 +506,11 @@ build-python-script: $(SCRIPT_PYTHON_GEN)
 
 .PHONY: install-perl-script install-sh-script install-python-script
 install-sh-script: $(SCRIPT_SH_INS)
-	$(INSTALL) $^ '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
+	$(INSTALL) $^ $(call sq,$(DESTDIR)$(gitexec_instdir))
 install-perl-script: $(SCRIPT_PERL_INS)
-	$(INSTALL) $^ '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
+	$(INSTALL) $^ $(call sq,$(DESTDIR)$(gitexec_instdir))
 install-python-script: $(SCRIPT_PYTHON_INS)
-	$(INSTALL) $^ '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
+	$(INSTALL) $^ $(call sq,$(DESTDIR)$(gitexec_instdir))
 
 .PHONY: clean-perl-script clean-sh-script clean-python-script
 clean-sh-script:
@@ -1040,7 +1040,7 @@ endif
 
 ifdef SANE_TOOL_PATH
 SANE_TOOL_PATH_SQ = $(subst ','\'',$(SANE_TOOL_PATH))
-BROKEN_PATH_FIX = 's|^\# @@BROKEN_PATH_FIX@@$$|git_broken_path_fix $(SANE_TOOL_PATH_SQ)|'
+BROKEN_PATH_FIX = 's|^\# @@BROKEN_PATH_FIX@@$$|git_broken_path_fix $(call sqi,$(SANE_TOOL_PATH)|'
 PATH := $(SANE_TOOL_PATH):${PATH}
 else
 BROKEN_PATH_FIX = '/^\# @@BROKEN_PATH_FIX@@$$/d'
@@ -1561,6 +1561,13 @@ ifneq ("$(PROFILE)","")
 endif
 endif
 
+# usage: $(call sq,CONTENTS)
+#
+# Quote the value as appropriate for the shell. Use "sqi" if you are
+# already "i"nside single-quotes.
+sqi = $(subst ','\'',$1)
+sq = '$(call sqi,$1)'
+
 # usage: $(eval $(call make-var,FN,DESC,CONTENTS))
 #
 # Create a rule to write $CONTENTS (which should come from a make variable)
@@ -1568,7 +1575,7 @@ endif
 # dependency on a Makefile variable. Prints $DESC to the user.
 define make-var
 MAKE/$1: FORCE
-	@VALUE='$$(subst ','\'',$3)'; \
+	@VALUE=$$(call sq,$3); \
 	if ! test -e $$@ || test x"$$$$VALUE" != x"`cat $$@`"; then \
 		echo >&2 "    * new $2"; \
 		printf '%s\n' "$$$$VALUE" >$$@+ && \
@@ -1603,7 +1610,7 @@ PERLLIB_EXTRA_SQ = $(subst ','\'',$(PERLLIB_EXTRA))
 
 LIBS = $(GITLIBS) $(EXTLIBS)
 
-BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \
+BASIC_CFLAGS += -DSHA1_HEADER=$(call sq,$(SHA1_HEADER)) \
 	$(COMPAT_CFLAGS)
 LIB_OBJS += $(COMPAT_OBJS)
 
@@ -1665,13 +1672,13 @@ endif
 
 all::
 ifndef NO_TCLTK
-	$(QUIET_SUBDIR0)git-gui $(QUIET_SUBDIR1) gitexecdir='$(gitexec_instdir_SQ)' all
+	$(QUIET_SUBDIR0)git-gui $(QUIET_SUBDIR1) gitexecdir=$(call sq,$(gitexec_instdir)) all
 	$(QUIET_SUBDIR0)gitk-git $(QUIET_SUBDIR1) all
 endif
 ifndef NO_PERL
-	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' localedir='$(localedir_SQ)' all
+	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH=$(call sq,$(PERL_PATH)) prefix=$(call sq,$(prefix)) localedir=$(call sq,$(localedir)) all
 endif
-	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'
+	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) SHELL_PATH=$(call sq,$(SHELL_PATH)) PERL_PATH=$(call sq,$(PERL_PATH))
 
 please_set_SHELL_PATH_to_a_more_modern_shell:
 	@$$(:)
@@ -1761,15 +1768,15 @@ $(eval $(call make-var,SCRIPT-DEFINES,script parameters,\
 ))
 define cmd_munge_script
 $(RM) $@ $@+ && \
-sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
-    -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
-    -e 's|@@DIFF@@|$(DIFF_SQ)|' \
-    -e 's|@@LOCALEDIR@@|$(localedir_SQ)|g' \
+sed -e '1s|#!.*/sh|#!$(call sqi,$(SHELL_PATH))|' \
+    -e 's|@SHELL_PATH@|$(call sqi,$(SHELL_PATH))|' \
+    -e 's|@@DIFF@@|$(call sqi,$(DIFF))|' \
+    -e 's|@@LOCALEDIR@@|$(call sqi,$(localedir))|g' \
     -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
     -e 's/@@USE_GETTEXT_SCHEME@@/$(USE_GETTEXT_SCHEME)/g' \
     -e $(BROKEN_PATH_FIX) \
-    -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \
-    -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
+    -e 's|@@GITWEBDIR@@|$(call sqi,$(gitwebdir))|g' \
+    -e 's|@@PERL@@|$(call sqi,$(PERL_PATH))|g' \
     $@.sh >$@+
 endef
 
@@ -1798,7 +1805,7 @@ perl/PM.stamp: FORCE
 	$(RM) $@+
 
 perl/perl.mak: MAKE/CFLAGS MAKE/PREFIX perl/Makefile perl/Makefile.PL
-	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F)
+	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH=$(call sq,$(PERL_PATH)) prefix=$(call sq,$(prefix)) $(@F)
 
 $(eval $(call make-var,PERL-DEFINES,perl-specific parameters,\
 	:$(PERL_PATH)\
@@ -1807,10 +1814,10 @@ $(eval $(call make-var,PERL-DEFINES,perl-specific parameters,\
 $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl perl/perl.mak MAKE/PERL-DEFINES GIT-VERSION-FILE
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \
-	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
+	INSTLIBDIR_EXTRA=$(call sq,$(PERLLIB_EXTRA)) && \
 	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
 	sed -e '1{' \
-	    -e '	s|#!.*perl|#!$(PERL_PATH_SQ)|' \
+	    -e '	s|#!.*perl|#!$(call sqi,$(PERL_PATH))|' \
 	    -e '	h' \
 	    -e '	s=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} || "'"$$INSTLIBDIR"'"));=' \
 	    -e '	H' \
@@ -1833,7 +1840,7 @@ git-instaweb: git-instaweb.sh MAKE/SCRIPT-DEFINES
 else # NO_PERL
 $(patsubst %.perl,%,$(SCRIPT_PERL)) git-instaweb: % : unimplemented.sh
 	$(QUIET_GEN)$(RM) $@ $@+ && \
-	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
+	sed -e '1s|#!.*/sh|#!$(call sqi,$(SHELL_PATH))|' \
 	    -e 's|@@REASON@@|NO_PERL=$(NO_PERL)|g' \
 	    unimplemented.sh >$@+ && \
 	chmod +x $@+ && \
@@ -1845,14 +1852,14 @@ $(eval $(call make-var,PYTHON-VARS,Python interpreter location,$(PYTHON_PATH)))
 $(SCRIPT_PYTHON_GEN): MAKE/CFLAGS MAKE/PREFIX MAKE/PYTHON-VARS
 $(SCRIPT_PYTHON_GEN): % : %.py
 	$(QUIET_GEN)$(RM) $@ $@+ && \
-	sed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
+	sed -e '1s|#!.*python|#!$(call sqi,$(PYTHON_PATH))|' \
 	    $< >$@+ && \
 	chmod +x $@+ && \
 	mv $@+ $@
 else # NO_PYTHON
 $(SCRIPT_PYTHON_GEN): % : unimplemented.sh
 	$(QUIET_GEN)$(RM) $@ $@+ && \
-	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
+	sed -e '1s|#!.*/sh|#!$(call sqi,$(SHELL_PATH))|' \
 	    -e 's|@@REASON@@|NO_PYTHON=$(NO_PYTHON)|g' \
 	    unimplemented.sh >$@+ && \
 	chmod +x $@+ && \
@@ -2174,42 +2181,43 @@ $(eval $(call make-var,LDFLAGS,link flags,$(ALL_LDFLAGS)))
 # We need to apply sq twice, once to protect from the shell
 # that runs MAKE/BUILD-OPTIONS, and then again to protect it
 # and the first level quoting from the shell that runs "echo".
+ssq = $(call sq,$(call sq,$1))
 MAKE/BUILD-OPTIONS: FORCE
-	@echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@
-	@echo PERL_PATH=\''$(subst ','\'',$(PERL_PATH_SQ))'\' >>$@
-	@echo DIFF=\''$(subst ','\'',$(subst ','\'',$(DIFF)))'\' >>$@
-	@echo PYTHON_PATH=\''$(subst ','\'',$(PYTHON_PATH_SQ))'\' >>$@
-	@echo TAR=\''$(subst ','\'',$(subst ','\'',$(TAR)))'\' >>$@
-	@echo NO_CURL=\''$(subst ','\'',$(subst ','\'',$(NO_CURL)))'\' >>$@
-	@echo USE_LIBPCRE=\''$(subst ','\'',$(subst ','\'',$(USE_LIBPCRE)))'\' >>$@
-	@echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@
-	@echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@
-	@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@
+	@echo SHELL_PATH=$(call ssq,$(SHELL_PATH)) >$@
+	@echo PERL_PATH=$(call ssq,$(PERL_PATH)) >>$@
+	@echo DIFF=$(call ssq,$(DIFF)) >>$@
+	@echo PYTHON_PATH=$(call ssq,$(PYTHON_PATH)) >>$@
+	@echo TAR=$(call ssq,$(TAR)) >>$@
+	@echo NO_CURL=$(call ssq,$(NO_CURL)) >>$@
+	@echo USE_LIBPCRE=$(call ssq,$(USE_LIBPCRE)) >>$@
+	@echo NO_PERL=$(call ssq,$(NO_PERL)) >>$@
+	@echo NO_PYTHON=$(call ssq,$(NO_PYTHON)) >>$@
+	@echo NO_UNIX_SOCKETS=$(call ssq,$(NO_UNIX_SOCKETS)) >>$@
 ifdef TEST_OUTPUT_DIRECTORY
-	@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@
+	@echo TEST_OUTPUT_DIRECTORY=$(call ssq,$(TEST_OUTPUT_DIRECTORY)) >>$@
 endif
 ifdef GIT_TEST_OPTS
-	@echo GIT_TEST_OPTS=\''$(subst ','\'',$(subst ','\'',$(GIT_TEST_OPTS)))'\' >>$@
+	@echo GIT_TEST_OPTS=$(call ssq,$(GIT_TEST_OPTS)) >>$@
 endif
 ifdef GIT_TEST_CMP
-	@echo GIT_TEST_CMP=\''$(subst ','\'',$(subst ','\'',$(GIT_TEST_CMP)))'\' >>$@
+	@echo GIT_TEST_CMP=$(call ssq,$(GIT_TEST_CMP)) >>$@
 endif
 ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT
 	@echo GIT_TEST_CMP_USE_COPIED_CONTEXT=YesPlease >>$@
 endif
-	@echo NO_GETTEXT=\''$(subst ','\'',$(subst ','\'',$(NO_GETTEXT)))'\' >>$@
-	@echo GETTEXT_POISON=\''$(subst ','\'',$(subst ','\'',$(GETTEXT_POISON)))'\' >>$@
+	@echo NO_GETTEXT=$(call ssq,$(NO_GETTEXT)) >>$@
+	@echo GETTEXT_POISON=$(call ssq,$(GETTEXT_POISON)) >>$@
 ifdef GIT_PERF_REPEAT_COUNT
-	@echo GIT_PERF_REPEAT_COUNT=\''$(subst ','\'',$(subst ','\'',$(GIT_PERF_REPEAT_COUNT)))'\' >>$@
+	@echo GIT_PERF_REPEAT_COUNT=$(call ssq,$(GIT_PERF_REPEAT_COUNT)) >>$@
 endif
 ifdef GIT_PERF_REPO
-	@echo GIT_PERF_REPO=\''$(subst ','\'',$(subst ','\'',$(GIT_PERF_REPO)))'\' >>$@
+	@echo GIT_PERF_REPO=$(call ssq,$(GIT_PERF_REPO)) >>$@
 endif
 ifdef GIT_PERF_LARGE_REPO
-	@echo GIT_PERF_LARGE_REPO=\''$(subst ','\'',$(subst ','\'',$(GIT_PERF_LARGE_REPO)))'\' >>$@
+	@echo GIT_PERF_LARGE_REPO=$(call ssq,$(GIT_PERF_LARGE_REPO)) >>$@
 endif
 ifdef GIT_PERF_MAKE_OPTS
-	@echo GIT_PERF_MAKE_OPTS=\''$(subst ','\'',$(subst ','\'',$(GIT_PERF_MAKE_OPTS)))'\' >>$@
+	@echo GIT_PERF_MAKE_OPTS=$(call ssq,$(GIT_PERF_MAKE_OPTS)) >>$@
 endif
 
 test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
@@ -2219,7 +2227,7 @@ all:: $(NO_INSTALL)
 
 bin-wrappers/%: wrap-for-bin.sh
 	@mkdir -p bin-wrappers
-	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
+	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(call sqi,$(SHELL_PATH))|' \
 	     -e 's|@@BUILD_DIR@@|$(shell pwd)|' \
 	     -e 's|@@PROG@@|$(@F)|' < $< > $@ && \
 	chmod +x $@
@@ -2307,33 +2315,33 @@ mergetools_instdir_SQ = $(subst ','\'',$(mergetools_instdir))
 install_bindir_programs := $(patsubst %,%$X,$(BINDIR_PROGRAMS_NEED_X)) $(BINDIR_PROGRAMS_NO_X)
 
 install: all
-	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(bindir_SQ)'
-	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) $(ALL_PROGRAMS) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'
-	$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
-	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
-	$(INSTALL) -m 644 mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
+	$(INSTALL) -d -m 755 $(call sq,$(DESTDIR)$(bindir))
+	$(INSTALL) -d -m 755 $(call sq,$(DESTDIR)$(gitexec_instdir))
+	$(INSTALL) $(ALL_PROGRAMS) $(call sq,$(DESTDIR)$(gitexec_instdir))
+	$(INSTALL) -m 644 $(SCRIPT_LIB) $(call sq,$(DESTDIR)$(gitexec_instdir))
+	$(INSTALL) $(install_bindir_programs) $(call sq,$(DESTDIR)$(bindir))
+	$(MAKE) -C templates DESTDIR=$(call sq,$(DESTDIR)) install
+	$(INSTALL) -d -m 755 $(call sq,$(DESTDIR)$(mergetools_instdir))
+	$(INSTALL) -m 644 mergetools/* $(call sq,$(DESTDIR)$(mergetools_instdir))
 ifndef NO_GETTEXT
-	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)'
+	$(INSTALL) -d -m 755 $(call sq,$(DESTDIR)$(localedir))
 	(cd po/build/locale && $(TAR) cf - .) | \
-	(cd '$(DESTDIR_SQ)$(localedir_SQ)' && umask 022 && $(TAR) xof -)
+	(cd $(call sq,$(DESTDIR)$(localedir)) && umask 022 && $(TAR) xof -)
 endif
 ifndef NO_PERL
-	$(MAKE) -C perl prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' install
+	$(MAKE) -C perl prefix=$(call sq,$(prefix)) DESTDIR=$(call sq,$(DESTDIR)) install
 	$(MAKE) -C gitweb install
 endif
 ifndef NO_TCLTK
 	$(MAKE) -C gitk-git install
-	$(MAKE) -C git-gui gitexecdir='$(gitexec_instdir_SQ)' install
+	$(MAKE) -C git-gui gitexecdir=$(call sq,$(gitexec_instdir)) install
 endif
 ifneq (,$X)
-	$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), test '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p' -ef '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p$X' || $(RM) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p';)
+	$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), test $(call sq,$(DESTDIR)$(gitexec_instdir)/$p) -ef $(call sq,$(DESTDIR)$(gitexec_instdir)/$p$X) || $(RM) $(call sq,$(DESTDIR)$(gitexec_instdir)/$p);)
 endif
 
-	bindir=$$(cd '$(DESTDIR_SQ)$(bindir_SQ)' && pwd) && \
-	execdir=$$(cd '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' && pwd) && \
+	bindir=$$(cd $(call sq,$(DESTDIR)$(bindir)) && pwd) && \
+	execdir=$$(cd $(call sq,$(DESTDIR)$(gitexec_instdir)) && pwd) && \
 	{ test "$$bindir/" = "$$execdir/" || \
 	  for p in git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); do \
 		$(RM) "$$execdir/$$p" && \

-- 
1.8.5.2.500.g8060133
Junio C Hamano· Feb 5, 2014, 19:12 UTC · re: Jeff King · lore

Re: [PATCH 08/13] Makefile: introduce sq function for shell-quoting

Jeff King <peff@peff.net> writes:
Show 11 quoted lines
> Since we have recently abolished the prohibition on $(call)
> in our Makefile, we can use it to factor out the repeated
> shell-quoting we do. There are two upsides:
>
>   1. It is much more readable than inline calls to
>      $(subst ','\'').
>
>   2. It is short enough that we can do away with the _SQ
>      variants of many variables (note that we do not do this
>      just yet, as there is a little more cleanup needed
>      first).
Yay.
Show 8 quoted lines
> .... But
> many instances are not really any more readable (e.g., see the first
> hunk below).
> ...
>  .PHONY: install-perl-script install-sh-script install-python-script
>  install-sh-script: $(SCRIPT_SH_INS)
> -	$(INSTALL) $^ '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
> +	$(INSTALL) $^ $(call sq,$(DESTDIR)$(gitexec_instdir))

Hmph, I do not see it as bad as the "make-var", which forces you to say $(eval $(call ...)); this $(call sq, ...) is fairly readable.

Jeff King· Feb 5, 2014, 17:58 UTC · re: Jeff King · lore

[PATCH 09/13] Makefile: add c-quote helper function

We have to c-quote strings coming from Makefile variables when we pass them to the compiler via "-D". Now that we can use $(call) in our Makefile, we can factor out the quoting to make things easier to read. We can also apply it more consistently, as there were many spots that should have been C-quoting but did not. For example:

  make prefix='foo\bar'
would produce an exec_cmd.o with a broken prefix.
Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile | 58 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 29 insertions(+), 29 deletions(-)
Show changes to Makefile +29 −29
diff --git a/Makefile b/Makefile
index 868872f..b1c3fb4 100644
--- a/Makefile
+++ b/Makefile
@@ -1568,6 +1568,17 @@ endif
 sqi = $(subst ','\'',$1)
 sq = '$(call sqi,$1)'
 
+# usage: $(call cq,CONTENTS)
+#
+# Quote the value as appropriate for a C string literal.
+cq = "$(subst ",\",$(subst \,\\,$1))"
+
+# usage: $(call scq,CONTENTS)
+#
+# Quote the value as C string inside a shell string. Good for passing strings
+# on the command line via "-DFOO=$(call # scq,$(FOO))".
+scq = $(call sq,$(call cq,$1))
+
 # usage: $(eval $(call make-var,FN,DESC,CONTENTS))
 #
 # Create a rule to write $CONTENTS (which should come from a make variable)
@@ -1617,28 +1628,17 @@ LIB_OBJS += $(COMPAT_OBJS)
 # Quote for C
 
 ifdef DEFAULT_EDITOR
-DEFAULT_EDITOR_CQ = "$(subst ",\",$(subst \,\\,$(DEFAULT_EDITOR)))"
-DEFAULT_EDITOR_CQ_SQ = $(subst ','\'',$(DEFAULT_EDITOR_CQ))
-
-BASIC_CFLAGS += -DDEFAULT_EDITOR='$(DEFAULT_EDITOR_CQ_SQ)'
+BASIC_CFLAGS += -DDEFAULT_EDITOR=$(call scq,$(DEFAULT_EDITOR))
 endif
 
 ifdef DEFAULT_PAGER
-DEFAULT_PAGER_CQ = "$(subst ",\",$(subst \,\\,$(DEFAULT_PAGER)))"
-DEFAULT_PAGER_CQ_SQ = $(subst ','\'',$(DEFAULT_PAGER_CQ))
-
-BASIC_CFLAGS += -DDEFAULT_PAGER='$(DEFAULT_PAGER_CQ_SQ)'
+BASIC_CFLAGS += -DDEFAULT_PAGER=$(call scq,$(DEFAULT_PAGER))
 endif
 
 ifdef SHELL_PATH
-SHELL_PATH_CQ = "$(subst ",\",$(subst \,\\,$(SHELL_PATH)))"
-SHELL_PATH_CQ_SQ = $(subst ','\'',$(SHELL_PATH_CQ))
-
-BASIC_CFLAGS += -DSHELL_PATH='$(SHELL_PATH_CQ_SQ)'
+BASIC_CFLAGS += -DSHELL_PATH=$(call scq,$(SHELL_PATH))
 endif
 
-GIT_USER_AGENT_CQ = "$(subst ",\",$(subst \,\\,$(GIT_USER_AGENT)))"
-GIT_USER_AGENT_CQ_SQ = $(subst ','\'',$(GIT_USER_AGENT_CQ))
 $(eval $(call make-var,USER-AGENT,user agent string,$(GIT_USER_AGENT)))
 
 ifdef DEFAULT_HELP_FORMAT
@@ -1723,9 +1723,9 @@ strip: $(PROGRAMS) git$X
 
 git.sp git.s git.o: MAKE/PREFIX
 git.sp git.s git.o: EXTRA_CPPFLAGS = \
-	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
-	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
-	'-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
+	-DGIT_HTML_PATH=$(call scq,$(htmldir_relative)) \
+	-DGIT_MAN_PATH=$(call scq,$(mandir_relative)) \
+	-DGIT_INFO_PATH=$(call scq,$(infodir_relative))
 
 git$X: git.o MAKE/LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ git.o \
@@ -1735,14 +1735,14 @@ help.sp help.s help.o: common-cmds.h
 
 builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h MAKE/PREFIX
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
-	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
-	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
-	'-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
+	-DGIT_HTML_PATH=$(call scq,$(htmldir_relative)) \
+	-DGIT_MAN_PATH=$(call scq,$(mandir_relative)) \
+	-DGIT_INFO_PATH=$(call scq,$(infodir_relative))
 
 version.sp version.s version.o: GIT-VERSION-FILE MAKE/USER-AGENT
 version.sp version.s version.o: EXTRA_CPPFLAGS = \
-	'-DGIT_VERSION="$(GIT_VERSION)"' \
-	'-DGIT_USER_AGENT=$(GIT_USER_AGENT_CQ_SQ)'
+	-DGIT_VERSION=$(call scq,$(GIT_VERSION)) \
+	-DGIT_USER_AGENT=$(call scq,$(GIT_USER_AGENT))
 
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \
@@ -2020,25 +2020,25 @@ endif
 
 exec_cmd.sp exec_cmd.s exec_cmd.o: MAKE/PREFIX
 exec_cmd.sp exec_cmd.s exec_cmd.o: EXTRA_CPPFLAGS = \
-	'-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
-	'-DBINDIR="$(bindir_relative_SQ)"' \
-	'-DPREFIX="$(prefix_SQ)"'
+	-DGIT_EXEC_PATH=$(call scq,$(gitexecdir)) \
+	-DBINDIR=$(call scq,$(bindir_relative)) \
+	-DPREFIX=$(call scq,$(prefix))
 
 builtin/init-db.sp builtin/init-db.s builtin/init-db.o: MAKE/PREFIX
 builtin/init-db.sp builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \
-	-DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
+	-DDEFAULT_GIT_TEMPLATE_DIR=$(call scq,$(template_dir))
 
 config.sp config.s config.o: MAKE/PREFIX
 config.sp config.s config.o: EXTRA_CPPFLAGS = \
-	-DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"'
+	-DETC_GITCONFIG=$(call scq,$(ETC_GITCONFIG))
 
 attr.sp attr.s attr.o: MAKE/PREFIX
 attr.sp attr.s attr.o: EXTRA_CPPFLAGS = \
-	-DETC_GITATTRIBUTES='"$(ETC_GITATTRIBUTES_SQ)"'
+	-DETC_GITATTRIBUTES=$(call scq,$(ETC_GITATTRIBUTES))
 
 gettext.sp gettext.s gettext.o: MAKE/PREFIX
 gettext.sp gettext.s gettext.o: EXTRA_CPPFLAGS = \
-	-DGIT_LOCALE_PATH='"$(localedir_SQ)"'
+	-DGIT_LOCALE_PATH=$(call scq,$(localedir))
 
 http-push.sp http.sp http-walker.sp remote-curl.sp: SPARSE_FLAGS += \
 	-DCURL_DISABLE_TYPECHECK

-- 
1.8.5.2.500.g8060133
Junio C Hamano· Feb 5, 2014, 19:13 UTC · re: Jeff King · lore

Re: [PATCH 09/13] Makefile: add c-quote helper function

Jeff King <peff@peff.net> writes:
Show 33 quoted lines
> We have to c-quote strings coming from Makefile variables
> when we pass them to the compiler via "-D". Now that we can
> use $(call) in our Makefile, we can factor out the quoting
> to make things easier to read. We can also apply it more
> consistently, as there were many spots that should have been
> C-quoting but did not. For example:
>
>   make prefix='foo\bar'
>
> would produce an exec_cmd.o with a broken prefix.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Makefile | 58 +++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 29 insertions(+), 29 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 868872f..b1c3fb4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1568,6 +1568,17 @@ endif
>  sqi = $(subst ','\'',$1)
>  sq = '$(call sqi,$1)'
>  
> +# usage: $(call cq,CONTENTS)
> +#
> +# Quote the value as appropriate for a C string literal.
> +cq = "$(subst ",\",$(subst \,\\,$1))"
> +
> +# usage: $(call scq,CONTENTS)
> +#
> +# Quote the value as C string inside a shell string. Good for passing strings
> +# on the command line via "-DFOO=$(call # scq,$(FOO))".
"call # scq"???
Show 103 quoted lines
> +scq = $(call sq,$(call cq,$1))
> +
>  # usage: $(eval $(call make-var,FN,DESC,CONTENTS))
>  #
>  # Create a rule to write $CONTENTS (which should come from a make variable)
> @@ -1617,28 +1628,17 @@ LIB_OBJS += $(COMPAT_OBJS)
>  # Quote for C
>  
>  ifdef DEFAULT_EDITOR
> -DEFAULT_EDITOR_CQ = "$(subst ",\",$(subst \,\\,$(DEFAULT_EDITOR)))"
> -DEFAULT_EDITOR_CQ_SQ = $(subst ','\'',$(DEFAULT_EDITOR_CQ))
> -
> -BASIC_CFLAGS += -DDEFAULT_EDITOR='$(DEFAULT_EDITOR_CQ_SQ)'
> +BASIC_CFLAGS += -DDEFAULT_EDITOR=$(call scq,$(DEFAULT_EDITOR))
>  endif
>  
>  ifdef DEFAULT_PAGER
> -DEFAULT_PAGER_CQ = "$(subst ",\",$(subst \,\\,$(DEFAULT_PAGER)))"
> -DEFAULT_PAGER_CQ_SQ = $(subst ','\'',$(DEFAULT_PAGER_CQ))
> -
> -BASIC_CFLAGS += -DDEFAULT_PAGER='$(DEFAULT_PAGER_CQ_SQ)'
> +BASIC_CFLAGS += -DDEFAULT_PAGER=$(call scq,$(DEFAULT_PAGER))
>  endif
>  
>  ifdef SHELL_PATH
> -SHELL_PATH_CQ = "$(subst ",\",$(subst \,\\,$(SHELL_PATH)))"
> -SHELL_PATH_CQ_SQ = $(subst ','\'',$(SHELL_PATH_CQ))
> -
> -BASIC_CFLAGS += -DSHELL_PATH='$(SHELL_PATH_CQ_SQ)'
> +BASIC_CFLAGS += -DSHELL_PATH=$(call scq,$(SHELL_PATH))
>  endif
>  
> -GIT_USER_AGENT_CQ = "$(subst ",\",$(subst \,\\,$(GIT_USER_AGENT)))"
> -GIT_USER_AGENT_CQ_SQ = $(subst ','\'',$(GIT_USER_AGENT_CQ))
>  $(eval $(call make-var,USER-AGENT,user agent string,$(GIT_USER_AGENT)))
>  
>  ifdef DEFAULT_HELP_FORMAT
> @@ -1723,9 +1723,9 @@ strip: $(PROGRAMS) git$X
>  
>  git.sp git.s git.o: MAKE/PREFIX
>  git.sp git.s git.o: EXTRA_CPPFLAGS = \
> -	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
> -	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
> -	'-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
> +	-DGIT_HTML_PATH=$(call scq,$(htmldir_relative)) \
> +	-DGIT_MAN_PATH=$(call scq,$(mandir_relative)) \
> +	-DGIT_INFO_PATH=$(call scq,$(infodir_relative))
>  
>  git$X: git.o MAKE/LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ git.o \
> @@ -1735,14 +1735,14 @@ help.sp help.s help.o: common-cmds.h
>  
>  builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h MAKE/PREFIX
>  builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
> -	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
> -	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
> -	'-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
> +	-DGIT_HTML_PATH=$(call scq,$(htmldir_relative)) \
> +	-DGIT_MAN_PATH=$(call scq,$(mandir_relative)) \
> +	-DGIT_INFO_PATH=$(call scq,$(infodir_relative))
>  
>  version.sp version.s version.o: GIT-VERSION-FILE MAKE/USER-AGENT
>  version.sp version.s version.o: EXTRA_CPPFLAGS = \
> -	'-DGIT_VERSION="$(GIT_VERSION)"' \
> -	'-DGIT_USER_AGENT=$(GIT_USER_AGENT_CQ_SQ)'
> +	-DGIT_VERSION=$(call scq,$(GIT_VERSION)) \
> +	-DGIT_USER_AGENT=$(call scq,$(GIT_USER_AGENT))
>  
>  $(BUILT_INS): git$X
>  	$(QUIET_BUILT_IN)$(RM) $@ && \
> @@ -2020,25 +2020,25 @@ endif
>  
>  exec_cmd.sp exec_cmd.s exec_cmd.o: MAKE/PREFIX
>  exec_cmd.sp exec_cmd.s exec_cmd.o: EXTRA_CPPFLAGS = \
> -	'-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
> -	'-DBINDIR="$(bindir_relative_SQ)"' \
> -	'-DPREFIX="$(prefix_SQ)"'
> +	-DGIT_EXEC_PATH=$(call scq,$(gitexecdir)) \
> +	-DBINDIR=$(call scq,$(bindir_relative)) \
> +	-DPREFIX=$(call scq,$(prefix))
>  
>  builtin/init-db.sp builtin/init-db.s builtin/init-db.o: MAKE/PREFIX
>  builtin/init-db.sp builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \
> -	-DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
> +	-DDEFAULT_GIT_TEMPLATE_DIR=$(call scq,$(template_dir))
>  
>  config.sp config.s config.o: MAKE/PREFIX
>  config.sp config.s config.o: EXTRA_CPPFLAGS = \
> -	-DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"'
> +	-DETC_GITCONFIG=$(call scq,$(ETC_GITCONFIG))
>  
>  attr.sp attr.s attr.o: MAKE/PREFIX
>  attr.sp attr.s attr.o: EXTRA_CPPFLAGS = \
> -	-DETC_GITATTRIBUTES='"$(ETC_GITATTRIBUTES_SQ)"'
> +	-DETC_GITATTRIBUTES=$(call scq,$(ETC_GITATTRIBUTES))
>  
>  gettext.sp gettext.s gettext.o: MAKE/PREFIX
>  gettext.sp gettext.s gettext.o: EXTRA_CPPFLAGS = \
> -	-DGIT_LOCALE_PATH='"$(localedir_SQ)"'
> +	-DGIT_LOCALE_PATH=$(call scq,$(localedir))
>  
>  http-push.sp http.sp http-walker.sp remote-curl.sp: SPARSE_FLAGS += \
>  	-DCURL_DISABLE_TYPECHECK
Jeff King· Feb 5, 2014, 19:17 UTC · re: Junio C Hamano · lore

Re: [PATCH 09/13] Makefile: add c-quote helper function

On Wed, Feb 05, 2014 at 11:13:24AM -0800, Junio C Hamano wrote:
> > +# Quote the value as C string inside a shell string. Good for passing strings
> > +# on the command line via "-DFOO=$(call # scq,$(FOO))".
> 
> "call # scq"???
Whoops. Bad rewrapping of the comment. It should obviously just be
  -DFOO=$(call scq,$(FOO))
-Peff
Jeff King· Feb 5, 2014, 17:58 UTC · re: Jeff King · lore

[PATCH 10/13] Makefile: drop *_SQ variables

Now that all uses of the SQ variables have been dropped, we can stop setting the variables.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile | 28 ----------------------------
 1 file changed, 28 deletions(-)
Show changes to Makefile +0 −28
diff --git a/Makefile b/Makefile
index b1c3fb4..cd07a70 100644
--- a/Makefile
+++ b/Makefile
@@ -1039,7 +1039,6 @@ endif
 endif
 
 ifdef SANE_TOOL_PATH
-SANE_TOOL_PATH_SQ = $(subst ','\'',$(SANE_TOOL_PATH))
 BROKEN_PATH_FIX = 's|^\# @@BROKEN_PATH_FIX@@$$|git_broken_path_fix $(call sqi,$(SANE_TOOL_PATH)|'
 PATH := $(SANE_TOOL_PATH):${PATH}
 else
@@ -1594,31 +1593,6 @@ MAKE/$1: FORCE
 	fi
 endef
 
-# Shell quote (do not use $(call) to accommodate ancient setups);
-
-SHA1_HEADER_SQ = $(subst ','\'',$(SHA1_HEADER))
-ETC_GITCONFIG_SQ = $(subst ','\'',$(ETC_GITCONFIG))
-ETC_GITATTRIBUTES_SQ = $(subst ','\'',$(ETC_GITATTRIBUTES))
-
-DESTDIR_SQ = $(subst ','\'',$(DESTDIR))
-bindir_SQ = $(subst ','\'',$(bindir))
-bindir_relative_SQ = $(subst ','\'',$(bindir_relative))
-mandir_relative_SQ = $(subst ','\'',$(mandir_relative))
-infodir_relative_SQ = $(subst ','\'',$(infodir_relative))
-localedir_SQ = $(subst ','\'',$(localedir))
-gitexecdir_SQ = $(subst ','\'',$(gitexecdir))
-template_dir_SQ = $(subst ','\'',$(template_dir))
-htmldir_relative_SQ = $(subst ','\'',$(htmldir_relative))
-prefix_SQ = $(subst ','\'',$(prefix))
-gitwebdir_SQ = $(subst ','\'',$(gitwebdir))
-
-SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
-PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
-PYTHON_PATH_SQ = $(subst ','\'',$(PYTHON_PATH))
-TCLTK_PATH_SQ = $(subst ','\'',$(TCLTK_PATH))
-DIFF_SQ = $(subst ','\'',$(DIFF))
-PERLLIB_EXTRA_SQ = $(subst ','\'',$(PERLLIB_EXTRA))
-
 LIBS = $(GITLIBS) $(EXTLIBS)
 
 BASIC_CFLAGS += -DSHA1_HEADER=$(call sq,$(SHA1_HEADER)) \
@@ -2302,7 +2276,6 @@ gitexec_instdir = $(gitexecdir)
 else
 gitexec_instdir = $(prefix)/$(gitexecdir)
 endif
-gitexec_instdir_SQ = $(subst ','\'',$(gitexec_instdir))
 export gitexec_instdir
 
 ifneq ($(filter /%,$(firstword $(mergetoolsdir))),)
@@ -2310,7 +2283,6 @@ mergetools_instdir = $(mergetoolsdir)
 else
 mergetools_instdir = $(prefix)/$(mergetoolsdir)
 endif
-mergetools_instdir_SQ = $(subst ','\'',$(mergetools_instdir))
 
 install_bindir_programs := $(patsubst %,%$X,$(BINDIR_PROGRAMS_NEED_X)) $(BINDIR_PROGRAMS_NO_X)
 

-- 
1.8.5.2.500.g8060133
Jeff King· Feb 5, 2014, 18:02 UTC · re: Jeff King · lore

[PATCH 11/13] Makefile: auto-build C strings from make variables

We already insert the values of some make variables into files in MAKE/*. We can therefore build a simple pattern rule for converting such a value into a C string in a header file, which can then be included and used as normal.

The new system is demonstrated on version.c, where it can replace the use of "-D" on the command-line. Note that we still have to manually specify a dependency in the Makefile. In an ideal world, we would auto-detect the header dependency; however, there are two holdups:

  1. We cannot rely on having automatic header dependency
     generation on all platforms.
  2. Even when we do generate the dependencies, we rely on
     being able to compile the file once, which means our
     system cannot handle generated headers without a manual
     dependency to bootstrap it.
Signed-off-by: Jeff King <peff@peff.net>
---
This is a technique that I have used in other projects with great
success, as the make variable dependencies are represented as file
dependencies (which is the only type of dependency make knows about).

_But_ it ends up a lot less nice here because of the way we do auto-dependencies. I'm totally open to revamping the way we handle our header dependencies, but I didn't do that in this series.

 Makefile         | 11 +++++++----
 script/mkcstring | 18 ++++++++++++++++++
 version.c        |  6 ++++--
 3 files changed, 29 insertions(+), 6 deletions(-)
 create mode 100644 script/mkcstring
Show changes to 3 files +29 −6

Makefile, script/mkcstring, version.c

diff --git a/Makefile b/Makefile
index cd07a70..203171d 100644
--- a/Makefile
+++ b/Makefile
@@ -1593,6 +1593,11 @@ MAKE/$1: FORCE
 	fi
 endef
 
+MAKE/%-string.h: MAKE/% script/mkcstring
+	$(QUIET_GEN)$(SHELL_PATH) script/mkcstring \
+		$(subst -,_,$*) <$< >$@+ && \
+		mv $@+ $@
+
 LIBS = $(GITLIBS) $(EXTLIBS)
 
 BASIC_CFLAGS += -DSHA1_HEADER=$(call sq,$(SHA1_HEADER)) \
@@ -1614,6 +1619,7 @@ BASIC_CFLAGS += -DSHELL_PATH=$(call scq,$(SHELL_PATH))
 endif
 
 $(eval $(call make-var,USER-AGENT,user agent string,$(GIT_USER_AGENT)))
+$(eval $(call make-var,VERSION,version,$(GIT_VERSION)))
 
 ifdef DEFAULT_HELP_FORMAT
 BASIC_CFLAGS += -DDEFAULT_HELP_FORMAT='"$(DEFAULT_HELP_FORMAT)"'
@@ -1713,10 +1719,7 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	-DGIT_MAN_PATH=$(call scq,$(mandir_relative)) \
 	-DGIT_INFO_PATH=$(call scq,$(infodir_relative))
 
-version.sp version.s version.o: GIT-VERSION-FILE MAKE/USER-AGENT
-version.sp version.s version.o: EXTRA_CPPFLAGS = \
-	-DGIT_VERSION=$(call scq,$(GIT_VERSION)) \
-	-DGIT_USER_AGENT=$(call scq,$(GIT_USER_AGENT))
+version.sp version.s version.o: MAKE/VERSION-string.h MAKE/USER-AGENT-string.h
 
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \
diff --git a/script/mkcstring b/script/mkcstring
new file mode 100644
index 0000000..c01f430
--- /dev/null
+++ b/script/mkcstring
@@ -0,0 +1,18 @@
+#!/bin/sh
+
+name=$1; shift
+
+c_quote() {
+	sed 's/\\/\\\\/g; s/"/\\"/'
+}
+
+cat <<-EOF
+#ifndef MAKE_${name}_H
+#define MAKE_${name}_H
+
+/* Auto-generated by mkcstring */
+
+#define MAKE_${name} "$(c_quote)"
+
+#endif /* MAKE_${name}_H */
+EOF
diff --git a/version.c b/version.c
index 6106a80..f68a93b 100644
--- a/version.c
+++ b/version.c
@@ -1,8 +1,10 @@
 #include "git-compat-util.h"
 #include "version.h"
 #include "strbuf.h"
+#include "MAKE/USER-AGENT-string.h"
+#include "MAKE/VERSION-string.h"
 
-const char git_version_string[] = GIT_VERSION;
+const char git_version_string[] = MAKE_VERSION;
 
 const char *git_user_agent(void)
 {
@@ -11,7 +13,7 @@ const char *git_user_agent(void)
 	if (!agent) {
 		agent = getenv("GIT_USER_AGENT");
 		if (!agent)
-			agent = GIT_USER_AGENT;
+			agent = MAKE_USER_AGENT;
 	}
 
 	return agent;

-- 
1.8.5.2.500.g8060133
Junio C Hamano· Feb 5, 2014, 19:17 UTC · re: Jeff King · lore

Re: [PATCH 11/13] Makefile: auto-build C strings from make variables

Jeff King <peff@peff.net> writes:
Show 12 quoted lines
> diff --git a/script/mkcstring b/script/mkcstring
> new file mode 100644
> index 0000000..c01f430
> --- /dev/null
> +++ b/script/mkcstring
> @@ -0,0 +1,18 @@
> +#!/bin/sh
> +
> +name=$1; shift
> +
> +c_quote() {
> +	sed 's/\\/\\\\/g; s/"/\\"/'
No 'g' for the second one?
Show 37 quoted lines
> +}
> +
> +cat <<-EOF
> +#ifndef MAKE_${name}_H
> +#define MAKE_${name}_H
> +
> +/* Auto-generated by mkcstring */
> +
> +#define MAKE_${name} "$(c_quote)"
> +
> +#endif /* MAKE_${name}_H */
> +EOF
> diff --git a/version.c b/version.c
> index 6106a80..f68a93b 100644
> --- a/version.c
> +++ b/version.c
> @@ -1,8 +1,10 @@
>  #include "git-compat-util.h"
>  #include "version.h"
>  #include "strbuf.h"
> +#include "MAKE/USER-AGENT-string.h"
> +#include "MAKE/VERSION-string.h"
>  
> -const char git_version_string[] = GIT_VERSION;
> +const char git_version_string[] = MAKE_VERSION;
>  
>  const char *git_user_agent(void)
>  {
> @@ -11,7 +13,7 @@ const char *git_user_agent(void)
>  	if (!agent) {
>  		agent = getenv("GIT_USER_AGENT");
>  		if (!agent)
> -			agent = GIT_USER_AGENT;
> +			agent = MAKE_USER_AGENT;
>  	}
>  
>  	return agent;
Jeff King· Feb 5, 2014, 19:20 UTC · re: Junio C Hamano · lore

Re: [PATCH 11/13] Makefile: auto-build C strings from make variables

On Wed, Feb 05, 2014 at 11:17:13AM -0800, Junio C Hamano wrote:
Show 16 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > diff --git a/script/mkcstring b/script/mkcstring
> > new file mode 100644
> > index 0000000..c01f430
> > --- /dev/null
> > +++ b/script/mkcstring
> > @@ -0,0 +1,18 @@
> > +#!/bin/sh
> > +
> > +name=$1; shift
> > +
> > +c_quote() {
> > +	sed 's/\\/\\\\/g; s/"/\\"/'
> 
> No 'g' for the second one?
That's a bug. Thanks for catching.

I tested most of these changes manually, but I missed this one by only testing a value with a single quote. At one point I had introduced:

  $(eval $(call make-var,FOO,debug variable,$(FOO)))

so you could do "make MAKE/FOO" and "make MAKE/foo-string.c", but I did not include it in the series. Adding a test suite to our Makefile kind of seems like overkill, but as it gets complex, maybe some simple sanity checks are worthwhile (not part of the regular test suite, but maybe just a "./test-make" script to make sure it behaves). I dunno.

-Peff
Jeff King· Feb 5, 2014, 18:05 UTC · re: Jeff King · lore

[PATCH 12/13] Makefile: teach scripts to include make variables

The current scheme for getting build-time variables into a shell script is to munge the script with sed, and stick the munged variable into a special sentinel file so that "make" knows about the dependency.

Instead, we can combine both functions by generating a shell snippet with our value, and then "building" shell scripts by concatenating their snippets. "make" then handles the dependency automatically, and it's easy to generate tighter dependencies.

We demonstrate here by moving the "DIFF" substitution into its own snippet, which lets us rebuild only the single affected file when it changes.

Signed-off-by: Jeff King <peff@peff.net>
---
The astute reader will notice that:
  MAKE_DIFF='diff'
  $MAKE_DIFF ...

that we end up with is not _quite_ the same as replacing "$MAKE_DIFF" in the actual script text with "diff" at build-time. In particular, the way that whitespace and quotes are treated is different. I'm not sure what we would want to do here. Calling "eval" would work. Or we could have the snippet produce a function, rather than a variable, like:

  DIFF() {
      diff "$@"
  }
 Makefile        | 18 +++++++++++++++---
 git-sh-setup.sh |  2 +-
 script/mksh     |  4 ++++
 3 files changed, 20 insertions(+), 4 deletions(-)
 create mode 100644 script/mksh
Show changes to 3 files +20 −4

Makefile, git-sh-setup.sh, script/mksh

diff --git a/Makefile b/Makefile
index 203171d..ad3100d 100644
--- a/Makefile
+++ b/Makefile
@@ -1598,6 +1598,11 @@ MAKE/%-string.h: MAKE/% script/mkcstring
 		$(subst -,_,$*) <$< >$@+ && \
 		mv $@+ $@
 
+MAKE/%.sh: MAKE/% script/mksh
+	$(QUIET_GEN)$(SHELL_PATH) script/mksh \
+		$(subst -,_,$*) <$< >$@+ && \
+		mv $@+ $@
+
 LIBS = $(GITLIBS) $(EXTLIBS)
 
 BASIC_CFLAGS += -DSHA1_HEADER=$(call sq,$(SHA1_HEADER)) \
@@ -1734,7 +1739,6 @@ common-cmds.h: $(wildcard Documentation/git-*.txt)
 
 $(eval $(call make-var,SCRIPT-DEFINES,script parameters,\
 	:$(SHELL_PATH)\
-	:$(DIFF)\
 	:$(GIT_VERSION)\
 	:$(localedir)\
 	:$(NO_CURL)\
@@ -1743,18 +1747,24 @@ $(eval $(call make-var,SCRIPT-DEFINES,script parameters,\
 	:$(gitwebdir)\
 	:$(PERL_PATH)\
 ))
+$(eval $(call make-var,DIFF,diff command,$(DIFF)))
 define cmd_munge_script
 $(RM) $@ $@+ && \
+{ \
+includes="$(filter MAKE/%.sh,$^)"; \
+if ! test -z "$$includes"; then \
+	cat $$includes; \
+fi && \
 sed -e '1s|#!.*/sh|#!$(call sqi,$(SHELL_PATH))|' \
     -e 's|@SHELL_PATH@|$(call sqi,$(SHELL_PATH))|' \
-    -e 's|@@DIFF@@|$(call sqi,$(DIFF))|' \
     -e 's|@@LOCALEDIR@@|$(call sqi,$(localedir))|g' \
     -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
     -e 's/@@USE_GETTEXT_SCHEME@@/$(USE_GETTEXT_SCHEME)/g' \
     -e $(BROKEN_PATH_FIX) \
     -e 's|@@GITWEBDIR@@|$(call sqi,$(gitwebdir))|g' \
     -e 's|@@PERL@@|$(call sqi,$(PERL_PATH))|g' \
-    $@.sh >$@+
+    $@.sh; \
+} >$@+
 endef
 
 $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh MAKE/SCRIPT-DEFINES
@@ -1766,6 +1776,8 @@ $(SCRIPT_LIB) : % : %.sh MAKE/SCRIPT-DEFINES
 	$(QUIET_GEN)$(cmd_munge_script) && \
 	mv $@+ $@
 
+git-sh-setup: MAKE/DIFF.sh
+
 git.res: git.rc GIT-VERSION-FILE
 	$(QUIET_RC)$(RC) \
 	  $(join -DMAJOR= -DMINOR=, $(wordlist 1,2,$(subst -, ,$(subst ., ,$(GIT_VERSION))))) \
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index fffa3c7..627d289 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -285,7 +285,7 @@ clear_local_git_env() {
 # remove lines from $1 that are not in $2, leaving only common lines.
 create_virtual_base() {
 	sz0=$(wc -c <"$1")
-	@@DIFF@@ -u -La/"$1" -Lb/"$1" "$1" "$2" | git apply --no-add
+	$MAKE_DIFF -u -La/"$1" -Lb/"$1" "$1" "$2" | git apply --no-add
 	sz1=$(wc -c <"$1")
 
 	# If we do not have enough common material, it is not
diff --git a/script/mksh b/script/mksh
new file mode 100644
index 0000000..d41e77a
--- /dev/null
+++ b/script/mksh
@@ -0,0 +1,4 @@
+#!/bin/sh
+
+name=$1; shift
+printf "MAKE_%s='%s'\n" "$name" "$(sed "s/'/'\\''/g")"

-- 
1.8.5.2.500.g8060133
Junio C Hamano· Feb 5, 2014, 19:26 UTC · re: Jeff King · lore

Re: [PATCH 12/13] Makefile: teach scripts to include make variables

Jeff King <peff@peff.net> writes:
Show 20 quoted lines
>  define cmd_munge_script
>  $(RM) $@ $@+ && \
> +{ \
> +includes="$(filter MAKE/%.sh,$^)"; \
> +if ! test -z "$$includes"; then \
> +	cat $$includes; \
> +fi && \
>  sed -e '1s|#!.*/sh|#!$(call sqi,$(SHELL_PATH))|' \
>      -e 's|@SHELL_PATH@|$(call sqi,$(SHELL_PATH))|' \
> -    -e 's|@@DIFF@@|$(call sqi,$(DIFF))|' \
>      -e 's|@@LOCALEDIR@@|$(call sqi,$(localedir))|g' \
>      -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
>      -e 's/@@USE_GETTEXT_SCHEME@@/$(USE_GETTEXT_SCHEME)/g' \
>      -e $(BROKEN_PATH_FIX) \
>      -e 's|@@GITWEBDIR@@|$(call sqi,$(gitwebdir))|g' \
>      -e 's|@@PERL@@|$(call sqi,$(PERL_PATH))|g' \
> -    $@.sh >$@+
> +    $@.sh; \
> +} >$@+
>  endef
Sorry, but I am not quite sure what is going on here.
 - if $includes does not exist, cat $includes will barf but that is
   OK;
 - if $includes is empty, there is no point running cat so that is
   OK;
 - if $includes is not empty, we want to cat.

And then after emitting that piece, we start processing the *.sh source file, replacing she-bang line?

Show 11 quoted lines
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index fffa3c7..627d289 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -285,7 +285,7 @@ clear_local_git_env() {
>  # remove lines from $1 that are not in $2, leaving only common lines.
>  create_virtual_base() {
>  	sz0=$(wc -c <"$1")
> -	@@DIFF@@ -u -La/"$1" -Lb/"$1" "$1" "$2" | git apply --no-add
> +	$MAKE_DIFF -u -La/"$1" -Lb/"$1" "$1" "$2" | git apply --no-add
>  	sz1=$(wc -c <"$1")

This would mean that after this mechanism is extensively employed throughout our codebase, any random environment variable the user has whose name happens to begin with "MAKE_" will interfere with us (rather, we will override such a variable while we run). Having to carve out our own namespace in such a way is OK, but we would want to see that namespace somewhat related to the name of our project, not to the name of somebody else's like "make", no?

Show 12 quoted lines
>  
>  	# If we do not have enough common material, it is not
> diff --git a/script/mksh b/script/mksh
> new file mode 100644
> index 0000000..d41e77a
> --- /dev/null
> +++ b/script/mksh
> @@ -0,0 +1,4 @@
> +#!/bin/sh
> +
> +name=$1; shift
> +printf "MAKE_%s='%s'\n" "$name" "$(sed "s/'/'\\''/g")"
Jeff King· Feb 5, 2014, 19:50 UTC · re: Junio C Hamano · lore

Re: [PATCH 12/13] Makefile: teach scripts to include make variables

On Wed, Feb 05, 2014 at 11:26:58AM -0800, Junio C Hamano wrote:
Show 27 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> >  define cmd_munge_script
> >  $(RM) $@ $@+ && \
> > +{ \
> > +includes="$(filter MAKE/%.sh,$^)"; \
> > +if ! test -z "$$includes"; then \
> > +	cat $$includes; \
> > +fi && \
> >  sed -e '1s|#!.*/sh|#!$(call sqi,$(SHELL_PATH))|' \
> >      -e 's|@SHELL_PATH@|$(call sqi,$(SHELL_PATH))|' \
> > -    -e 's|@@DIFF@@|$(call sqi,$(DIFF))|' \
> >      -e 's|@@LOCALEDIR@@|$(call sqi,$(localedir))|g' \
> >      -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
> >      -e 's/@@USE_GETTEXT_SCHEME@@/$(USE_GETTEXT_SCHEME)/g' \
> >      -e $(BROKEN_PATH_FIX) \
> >      -e 's|@@GITWEBDIR@@|$(call sqi,$(gitwebdir))|g' \
> >      -e 's|@@PERL@@|$(call sqi,$(PERL_PATH))|g' \
> > -    $@.sh >$@+
> > +    $@.sh; \
> > +} >$@+
> >  endef
> 
> Sorry, but I am not quite sure what is going on here.
> [...]
> And then after emitting that piece, we start processing the *.sh
> source file, replacing she-bang line?
Yes, there is a bug here. The intent was to end up with:
  #!/bin/sh
  [include snippets]
  [the actual script]

but of course I bungled that, because #!/bin/sh is part of the file already, and our snippets should not come before. If we take this technique to its logical conclusion, the sed replacement goes away entirely, and we end up with something like:

  %: %.sh MAKE/SHELL_SHEBANG.sh
          { cat $(filter MAKE/%.sh,$^) && sed 1d $<; } >$@+
          chmod +x $@+
          mv $@+ $@
  MAKE/SHELL_SHEBANG.sh: MAKE/SHELL_SHEBANG
          echo "#!$(head -1 $<)" >$@+ &&
          mv $@+ $@
I.e., the shebang line simply becomes the first snippet.
Show 13 quoted lines
> >  create_virtual_base() {
> >  	sz0=$(wc -c <"$1")
> > -	@@DIFF@@ -u -La/"$1" -Lb/"$1" "$1" "$2" | git apply --no-add
> > +	$MAKE_DIFF -u -La/"$1" -Lb/"$1" "$1" "$2" | git apply --no-add
> >  	sz1=$(wc -c <"$1")
> 
> This would mean that after this mechanism is extensively employed
> throughout our codebase, any random environment variable the user
> has whose name happens to begin with "MAKE_" will interfere with us
> (rather, we will override such a variable while we run).  Having to
> carve out our own namespace in such a way is OK, but we would want
> to see that namespace somewhat related to the name of our project,
> not to the name of somebody else's like "make", no?

Yes, it probably makes sense to carve out our own namespace. I kind of dislike the use of environment variables at all, though, just because it really bleeds through into how you write the script (as opposed to just replacing like we do now, or in C, where a snippet defining a preprocessor macro is just fine).

An alternative is that we could do something like:
  %: %.sh
          script/mkshellscript $^ >$@+ &&
          chmod +x $@+ &&
          mv $@+ $@

and then have mkshellscript look like (and this could obviously be inline in the Makefile, but I think it's worth pulling it out to avoid the quoting hell):

  #!/bin/sh
  main=$1; shift
  sed_quote() {
          sed 's/\\/\\\\/g; s/|/\\|/g'
  }
  # generate a sed expression that will replace tokens; if we are given
  # the file MAKE/FOO.sh, the expression will replace  @@FOO@@ with the
  # contents of that file
  sed_replace() {
          for var in "$@"; do
                  key=${i#MAKE/}
                  key=${key%.sh}
                  printf 's|@@%s@@|%s|g' "$key" "$(sed_quote <$var)"
          done
  }
  sed "$(sed_replace "$@")" <"$main"
-Peff
Thomas Rast· Feb 8, 2014, 21:47 UTC · re: Jeff King · lore

Re: [PATCH 12/13] Makefile: teach scripts to include make variables

Jeff King <peff@peff.net> writes:
Show 14 quoted lines
> The current scheme for getting build-time variables into a
> shell script is to munge the script with sed, and stick the
> munged variable into a special sentinel file so that "make"
> knows about the dependency.
>
> Instead, we can combine both functions by generating a shell
> snippet with our value, and then "building" shell scripts by
> concatenating their snippets. "make" then handles the
> dependency automatically, and it's easy to generate tighter
> dependencies.
>
> We demonstrate here by moving the "DIFF" substitution into
> its own snippet, which lets us rebuild only the single
> affected file when it changes.

I can't look right now *why* this happens, but this breaks ./t2300-cd-to-toplevel.sh --valgrind with messages like

  expecting success: 
                  (
                          cd 'repo' &&
                          . "$(git --exec-path)"/git-sh-setup &&
                          cd_to_toplevel &&
                          [ "$(pwd -P)" = "$TOPLEVEL" ]
                  )
  ./test-lib.sh: line 414: /home/thomas/g/t/valgrind/bin/git-sh-setup: No such file or directory
  not ok 1 - at physical root
  #
  #                       (
  #                               cd 'repo' &&
  #                               . "$(git --exec-path)"/git-sh-setup &&
  #                               cd_to_toplevel &&
  #                               [ "$(pwd -P)" = "$TOPLEVEL" ]
  #                       )
  #

I don't know why it only affects this test, or why it doesn't break when within 'git bisect run' -- probably there's something funky going on in the environment, quite possibly in my own configs.

-- 
Thomas Rast
tr@thomasrast.ch
Jeff King· Feb 10, 2014, 01:15 UTC · re: Thomas Rast · lore

Re: [PATCH 12/13] Makefile: teach scripts to include make variables

On Sat, Feb 08, 2014 at 10:47:16PM +0100, Thomas Rast wrote:
Show 19 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > The current scheme for getting build-time variables into a
> > shell script is to munge the script with sed, and stick the
> > munged variable into a special sentinel file so that "make"
> > knows about the dependency.
> >
> > Instead, we can combine both functions by generating a shell
> > snippet with our value, and then "building" shell scripts by
> > concatenating their snippets. "make" then handles the
> > dependency automatically, and it's easy to generate tighter
> > dependencies.
> >
> > We demonstrate here by moving the "DIFF" substitution into
> > its own snippet, which lets us rebuild only the single
> > affected file when it changes.
> 
> I can't look right now *why* this happens, but this breaks
> ./t2300-cd-to-toplevel.sh --valgrind with messages like

I think it's the bug that Junio already pointed out; git-sh-setup gets the DIFF=... snippet instead of the initial #!-line. I didn't look at the details, but that probably screws up valgrind's symlinking, since we no longer realize it's a shell script.

Once that bug is fixed, I'll double-check that the problem goes away.
-Peff
Jeff King· Feb 5, 2014, 18:08 UTC · re: Jeff King · lore

[PATCH 13/13] move LESS/LV pager environment to Makefile

We set the LESS and LV variables to sensible defaults if they are not already set. However, the code is brittle. There is no easy way to change the defaults at compile time, and we have to duplicate the code in git-sh-setup and in pager.c.

Let's turn it into a normal Makefile knob instead.
Signed-off-by: Jeff King <peff@peff.net>
---
Bits of this patch were liberally stolen from Junio's weather-balloon
patch. All bugs are mine. :)

Note that setup_pager_env in the C code still does do a little parsing, since we need to have the name of each variable in a separate buffer to pass to getenv(). I chose to do it this way so that we could introduce a standard "mkcarray" helper that could be used in other places. But it would also be easy to do all of that parsing at compile time and produce an array of structs like:

  { "LESS", "LESS=-FRSX" },
  { "LV", "LV=-c" }
 Makefile        | 22 +++++++++++++++++++++-
 git-sh-setup.sh |  9 +++++----
 pager.c         | 34 +++++++++++++++++++++++-----------
 script/mkcarray | 25 +++++++++++++++++++++++++
 4 files changed, 74 insertions(+), 16 deletions(-)
 create mode 100644 script/mkcarray
Show changes to 4 files +74 −16

Makefile, git-sh-setup.sh, pager.c, script/mkcarray

diff --git a/Makefile b/Makefile
index ad3100d..fff8e72 100644
--- a/Makefile
+++ b/Makefile
@@ -342,6 +342,14 @@ all::
 # Define DEFAULT_HELP_FORMAT to "man", "info" or "html"
 # (defaults to "man") if you want to have a different default when
 # "git help" is called without a parameter specifying the format.
+#
+# Define PAGER_ENV to a SP separated VAR=VAL pairs to define
+# default environment variables to be passed when a pager is spawned, e.g.
+#
+#    PAGER_ENV = LESS=-FRSX LV=-c
+#
+# to say "export LESS=-FRSX (and LV=-c) if the environment variable
+# LESS (and LV) is not set, respectively".
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1505,6 +1513,10 @@ ifeq ($(PYTHON_PATH),)
 NO_PYTHON = NoThanks
 endif
 
+ifndef PAGER_ENV
+PAGER_ENV = LESS=-FRSX LV=-c
+endif
+
 QUIET_SUBDIR0  = +$(MAKE) -C # space to separate -C and subdir
 QUIET_SUBDIR1  =
 
@@ -1598,6 +1610,11 @@ MAKE/%-string.h: MAKE/% script/mkcstring
 		$(subst -,_,$*) <$< >$@+ && \
 		mv $@+ $@
 
+MAKE/%-array.h: MAKE/% script/mkcarray
+	$(QUIET_GEN)$(SHELL_PATH) script/mkcarray \
+		$(subst -,_,$*) <$< >$@+ && \
+		mv $@+ $@
+
 MAKE/%.sh: MAKE/% script/mksh
 	$(QUIET_GEN)$(SHELL_PATH) script/mksh \
 		$(subst -,_,$*) <$< >$@+ && \
@@ -1726,6 +1743,9 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 
 version.sp version.s version.o: MAKE/VERSION-string.h MAKE/USER-AGENT-string.h
 
+$(eval $(call make-var,PAGER-ENV,pager environment,$(PAGER_ENV)))
+pager.sp pager.s pager.o: MAKE/PAGER-ENV-array.h
+
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \
 	ln $< $@ 2>/dev/null || \
@@ -1776,7 +1796,7 @@ $(SCRIPT_LIB) : % : %.sh MAKE/SCRIPT-DEFINES
 	$(QUIET_GEN)$(cmd_munge_script) && \
 	mv $@+ $@
 
-git-sh-setup: MAKE/DIFF.sh
+git-sh-setup: MAKE/DIFF.sh MAKE/PAGER-ENV.sh
 
 git.res: git.rc GIT-VERSION-FILE
 	$(QUIET_RC)$(RC) \
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 627d289..be4a58a 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -158,10 +158,11 @@ git_pager() {
 	else
 		GIT_PAGER=cat
 	fi
-	: ${LESS=-FRSX}
-	: ${LV=-c}
-	export LESS LV
-
+	for vardef in $MAKE_PAGER_ENV
+	do
+		var=${vardef%%=*}
+		eval ": \${$vardef} && export $var"
+	done
 	eval "$GIT_PAGER" '"$@"'
 }
 
diff --git a/pager.c b/pager.c
index 0cc75a8..6db84c6 100644
--- a/pager.c
+++ b/pager.c
@@ -1,6 +1,8 @@
 #include "cache.h"
 #include "run-command.h"
 #include "sigchain.h"
+#include "argv-array.h"
+#include "MAKE/PAGER-ENV-array.h"
 
 #ifndef DEFAULT_PAGER
 #define DEFAULT_PAGER "less"
@@ -60,9 +62,26 @@ const char *git_pager(int stdout_is_tty)
 	return pager;
 }
 
+static void setup_pager_env(struct argv_array *env)
+{
+	int i;
+
+	for (i = 0; MAKE_PAGER_ENV[i]; i++) {
+		struct strbuf buf = STRBUF_INIT;
+		const char *p = MAKE_PAGER_ENV[i];
+		const char *eq = strchrnul(p, '=');
+
+		strbuf_add(&buf, p, eq - p);
+		if (!getenv(buf.buf))
+			argv_array_push(env, p);
+		strbuf_release(&buf);
+	}
+}
+
 void setup_pager(void)
 {
 	const char *pager = git_pager(isatty(1));
+	struct argv_array env = ARGV_ARRAY_INIT;
 
 	if (!pager || pager_in_use())
 		return;
@@ -80,17 +99,10 @@ void setup_pager(void)
 	pager_process.use_shell = 1;
 	pager_process.argv = pager_argv;
 	pager_process.in = -1;
-	if (!getenv("LESS") || !getenv("LV")) {
-		static const char *env[3];
-		int i = 0;
-
-		if (!getenv("LESS"))
-			env[i++] = "LESS=FRSX";
-		if (!getenv("LV"))
-			env[i++] = "LV=-c";
-		env[i] = NULL;
-		pager_process.env = env;
-	}
+
+	setup_pager_env(&env);
+	pager_process.env = argv_array_detach(&env, NULL);
+
 	if (start_command(&pager_process))
 		return;
 
diff --git a/script/mkcarray b/script/mkcarray
new file mode 100644
index 0000000..ed980ab
--- /dev/null
+++ b/script/mkcarray
@@ -0,0 +1,25 @@
+#!/bin/sh
+
+name=$1; shift
+
+quote() {
+	echo "$1" | sed 's/\\/\\\\/g; s/"/\\"/'
+}
+
+cat <<-EOF
+#ifndef ${name}_H
+#define ${name}_H
+
+static const char *MAKE_${name}[] = {
+EOF
+
+for i in $(cat); do
+	printf '\t"%s",\n' "$(quote "$i")"
+done
+
+cat <<EOF
+	NULL
+};
+
+#endif /* ${name}_H */
+EOF

-- 
1.8.5.2.500.g8060133
Jeff King· Feb 5, 2014, 19:23 UTC · re: Jeff King · lore

Re: [PATCH 13/13] move LESS/LV pager environment to Makefile

On Wed, Feb 05, 2014 at 01:08:57PM -0500, Jeff King wrote:
> +quote() {
> +	echo "$1" | sed 's/\\/\\\\/g; s/"/\\"/'
> +}
This of course has the same "/g" bug as the earlier patch in the series.

I was tempted to pull the function out into script/lib-c.sh, so that both can source it, but perhaps that is overkill.

-Peff
Jeff King· Feb 5, 2014, 19:52 UTC · re: Jeff King · lore

Re: [PATCH 13/13] move LESS/LV pager environment to Makefile

On Wed, Feb 05, 2014 at 02:23:50PM -0500, Jeff King wrote:
Show 10 quoted lines
> On Wed, Feb 05, 2014 at 01:08:57PM -0500, Jeff King wrote:
> 
> > +quote() {
> > +	echo "$1" | sed 's/\\/\\\\/g; s/"/\\"/'
> > +}
> 
> This of course has the same "/g" bug as the earlier patch in the series.
> 
> I was tempted to pull the function out into script/lib-c.sh, so that
> both can source it, but perhaps that is overkill.

Also, this (and the other spot) should probably be using "printf" instead of echo for portability (i.e., the config variables might have backslashes in them).

-Peff

← back to recent threads