Re: [PATCH v4 14/23] Makefile: re-add and use the "shellquote" macros

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Re-add and use, and expand on "shellquote" macros added in
> 4769948afe7 (Deal with $(bindir) and friends with whitespaces.,
> 2005-10-10).
>
> We avoided using them due to the "$(call)" feature of GNU make being
> relatively new at the time, but it isn't anymore. We hard depend on
> GNU make versions that have it.
>
> The use of "$(call)" was removed in 39c015c556f (Fixes for ancient
> versions of GNU make, 2006-02-18) and 7ffe7098dca (Fix installation of
> templates on ancient systems., 2006-07-29) due to those
> incompatibilities with older GNU make versions, and we've used the
> more verbose *_SQ pattern ever since.
>
> The "$(call)" feature was introduced in GNU make version 3.78,
> released on the 22nd of September, 1999. That release also introduced
> "$(error)" and "$(warning)", which we've been making use of since
> f2fabbf76e4 (Teach Makefile to check header dependencies, 2010-01-26).
>
> This extends upon the macros added in 4769948afe7: We now have macros
> for quoting a ' inside '', and a ' with no surrounding '' as before.
>
> Additionally provide and use a "shelldquote" macro along with
> "shellquote" for the common case of wanting to quote a C string we
> pass to the compiler with a -D flag.
>
> This doesn't get rid of all of our shell quoting. We've still got some
> in the main Makefile, let's leave most of it to avoid in-flight
> conflicts. I've fully converted "templates/Makefile" and "t/Makefile"
> though.

All of the above may very well explain why we decided not to use
$(call shellquote) in the past, and it may also explain why it is
safe to start using it again today, but it does not explain why we
want to do so in the first place.

Having to write

	$(call shellquote,$(VAR))

in longhand is much more cumbersome to read, write and merge than
defining a prepackaged VAR_SQ just once and refer it as

	'$(VAR_SQ)'

everywhere.

In fact, having to resolve merge conflicts in t/Makefile today,
which wouldn't have been necessary if we didn't have this change,
felt like a makework to me, and my time would have been better spent
elsewhere [*].

Is there a justification other than "this uses a feature supported
by newer GNUMake" we can give to future readers of the log message?
Why do we want to make this change?

It may be that this patch was particularly unlucky to collide with
something else to irritate me, and the above comment may apply to
other patches we saw recently on the list (and they are lucky that
they did not collide with anything not yet in 'master').  

In any case, I am wondering if we see too much churn without much
real benefit X-<.

Here is a sample of the damage.  Look how long the lines that use
_SQ twice have become.

 clean-chainlint:
-	$(RM) -r '$(CHAINLINTTMP_SQ)'
+	$(RM) -r $(call shellquote,$(CHAINLINTTMP))
 
 check-chainlint:
-	@mkdir -p '$(CHAINLINTTMP_SQ)' && \
-	sed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/tests && \
-	sed -e '/^[ 	]*$$/d' $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \
-	$(CHAINLINT) '$(CHAINLINTTMP_SQ)'/tests | grep -v '^[	]*$$' >'$(CHAINLINTTMP_SQ)'/actual && \
-	diff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual
+	@mkdir -p $(call shellquote,$(CHAINLINTTMP)) && \
+	sed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >$(call shellquote,$(CHAINLINTTMP))/tests && \
+	sed -e '/^[ 	]*$$/d' $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >$(call shellquote,$(CHAINLINTTMP))/expect && \
+	$(CHAINLINT) $(call shellquote,$(CHAINLINTTMP))/tests | grep -v '^[	]*$$' >$(call shellquote,$(CHAINLINTTMP))/actual && \
+	diff -u $(call shellquote,$(CHAINLINTTMP))/expect $(call shellquote,$(CHAINLINTTMP))/actual