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 <p...@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(-)

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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to