Re: [PATCH 2/8] scalar: include in standard Git build & installation

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sep 1, 2022, 09:11 UTC
Message-ID: <sso99so6-n28s-rq86-8q20-4456r3pn869r@tzk.qr>
In-Reply-To: <4d69e5eaccb8873eece666a3d2bb2b22abdde7ea.1661961746.git.gitgitgadget@gmail.com>

Hi Victoria,

On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:

Show 25 quoted lines

> From: Victoria Dye <vdye@github.com>
>
> Move 'scalar' out of 'contrib/' and into the root of the Git tree. The goal
> of this change is to build 'scalar' as part of the standard Git build &
> install processes.
>
> This patch includes both the physical move of Scalar's files out of
> 'contrib/' ('scalar.c', 'scalar.txt', and 't9xxx-scalar.sh'), and the
> changes to the build definitions in 'Makefile' and 'CMakelists.txt' to
> accommodate the new program.
>
> At a high level, Scalar is built so that:
> - there is a 'scalar-objs' target (similar to those created in 029bac01a8
>   (Makefile: add {program,xdiff,test,git,fuzz}-objs & objects targets,
>   2021-02-23)) for debugging purposes.
> - it appears in the root of the install directory (rather than the
>   gitexecdir).
> - it is included in the 'bin-wrappers/' directory for use in tests.
> - it receives a platform-specific executable suffix (e.g., '.exe'), if
>   applicable.
> - 'scalar.txt' is installed as 'man1' documentation.
> - the 'clean' target removes the 'scalar' executable.
>
> Additionally, update the root level '.gitignore' file to ignore the Scalar
> executable.

A great commit message, and even though the diff is large, it is eminently reviewable, with one exception:

Show 6 quoted lines

> @@ -3062,7 +3067,7 @@ bin-wrappers/%: wrap-for-bin.sh
>  	$(call mkdir_p_parent_template)
>  	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
>  	     -e 's|@@BUILD_DIR@@|$(shell pwd)|' \
> -	     -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%$(X),$(@F))$(patsubst git%,$(X),$(filter $(@F),$(BINDIR_PROGRAMS_NEED_X)))|' < $< > $@ && \
> +	     -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%,$(@F))$(if $(filter-out $(BINDIR_PROGRAMS_NO_X),$(@F)),$(X),)|' < $< > $@ && \

It took me a good while to wrap my head around this (and let me be clear: I consider none of this your fault, it's the fault of the design of the Makefile syntax).

Let me untangle this, for posterity's benefit. We substitute the placeholder `@@PROG@@` with a concatenation of two strings that are both derived from `@F`, i.e. the basename of the to-be-wrapped command.

Before, the first string would be the dashed command, verbatim, unless its name starts with a `test-` prefix, in which case we would add another prefix (`t/helper/`) _and_ append the `.exe` suffix on Windows.

And the second string would be a `.exe` for all remaining cases that need it: the commands starting with `git` and listed as needing the suffix. Otherwise, the second string would be empty.

Convoluted, sure, but it worked.

In the new version, the first string would be the basename of the to-be-wrapped command, with `t/helper/` prefixed for the `test-` helpers.

The second string would be a `.exe` for _all_ commands, except for those specifically listed as not wanting that suffix as per `BINDIR_PROGRAMS_NO_X`.

The new logic is so much simpler to understand!

Feel free to add a `Reviewed-by:` footer for me if you send another iteration of this patch series.

Thank you, Dscho