Re: [PATCH v2 1/3] Makefile: factor sed-powered '#!/bin/sh' munging into a variable

From: Junio C Hamano <gitster@pobox.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Victoria Dye <vdye@github.com>,
	Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH v2 1/3] Makefile: factor sed-powered '#!/bin/sh' munging into a variable
Date: Wed, 26 Oct 2022 10:51:48 -0700	[thread overview]
Message-ID: <xmqq1qquiibv.fsf@gitster.g> (raw)
In-Reply-To: patch-v2-1.3-fc6c5a6a8df-20221026T143534Z-avarab@gmail.com

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

> Reduce the amount of magical copy/pasting in the Makefile by factoring
> the munging of "#!/bin/sh" on the first line of a shellscript into a
> variable we can re-use in the various rules that need to do so.

At least when taken standalone, this looks more like replacing one
magical copy pasting with another magical one, the difference being
that the latter is not immediately obvious without referring back to
the definition of the variable.

If we need to change the replacement wholesale in a later step, then
it might give us a good trade off, but otherwise I am not sure why
this is a good idea that is worth the churn.  

When adding or updating these actions, in the original, you can typo
SHELL_PATH_SQ, but you can typo cmd_munge_script_sed_shell_path_arg
in the updated to break it the same way.  So...?

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Makefile | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 85f03c6aed1..45b22d33513 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2344,8 +2344,12 @@ GIT-SCRIPT-DEFINES: FORCE
>  		echo "$$FLAGS" >$@; \
>              fi
>  
> +define cmd_munge_script_sed_shell_path_arg
> +'1s|#!.*/sh|#!$(SHELL_PATH_SQ)|'
> +endef
> +
>  define cmd_munge_script
> -sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
> +sed -e $(call cmd_munge_script_sed_shell_path_arg) \
>      -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
>      -e 's|@@DIFF@@|$(DIFF_SQ)|' \
>      -e 's|@@LOCALEDIR@@|$(localedir_SQ)|g' \
> @@ -2447,7 +2451,7 @@ git-instaweb: git-instaweb.sh GIT-SCRIPT-DEFINES
>  else # NO_PERL
>  $(SCRIPT_PERL_GEN) git-instaweb: % : unimplemented.sh
>  	$(QUIET_GEN) \
> -	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
> +	sed -e $(call cmd_munge_script_sed_shell_path_arg) \
>  	    -e 's|@@REASON@@|NO_PERL=$(NO_PERL)|g' \
>  	    unimplemented.sh >$@+ && \
>  	chmod +x $@+ && \
> @@ -2468,7 +2472,7 @@ $(SCRIPT_PYTHON_GEN): % : %.py
>  else # NO_PYTHON
>  $(SCRIPT_PYTHON_GEN): % : unimplemented.sh
>  	$(QUIET_GEN) \
> -	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
> +	sed -e $(call cmd_munge_script_sed_shell_path_arg) \
>  	    -e 's|@@REASON@@|NO_PYTHON=$(NO_PYTHON)|g' \
>  	    unimplemented.sh >$@+ && \
>  	chmod +x $@+ && \
> @@ -3061,7 +3065,7 @@ all:: $(TEST_PROGRAMS) $(test_bindir_programs)
>  
>  bin-wrappers/%: wrap-for-bin.sh
>  	$(call mkdir_p_parent_template)
> -	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
> +	$(QUIET_GEN)sed -e $(call cmd_munge_script_sed_shell_path_arg) \
>  	     -e 's|@@BUILD_DIR@@|$(shell pwd)|' \
>  	     -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%,$(@F))$(if $(filter-out $(BINDIR_PROGRAMS_NO_X),$(@F)),$(X),)|' < $< > $@ && \
>  	chmod +x $@