Re: [PATCH v2 2/3] Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 26, 2022, 16:47 UTC
Message-ID: <xmqqsfjailak.fsf@gitster.g>
In-Reply-To: <patch-v2-2.3-6dcb49f25c4-20221026T143534Z-avarab@gmail.com>

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

Show 5 quoted lines

> Define the variables that make up TEST_OBJS earlier, and don't go back
> & forth in their definition. Before we'd first append $X to
> $(TEST_PROGRAMS), and then substitute $X back out of it to define
> $(TEST_OBJS). Let's instead add a new $(TEST_PROGRAM_OBJS) variable,
> which avoids this needless back & forth substitution.

Makes sense, I guess.  So TEST_OBJS is no longer used?

>  TEST_PROGRAMS = $(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X))
> +all:: $(TEST_PROGRAMS)

This change is not necessary to achieve the stated goal of this step, though. It is one of those "while at it" distraction that consumes our already constrained reviewer bandwidth, no?

Having said that, "all::" being able to be built up with independent pieces shine here in this split from the original. It probably is easier to reason about while seeing this isolated area of Makefile what is being done to TEST_PROGRAMS.

The rest of the patch is quite straight-forward renaming of TEST_OBJS to TEST_PROGRAM_OBJS and an improvement of how the elements on the list are computed from the source-of-truth list that is TEST_PROGRAMS_NEED_X that looks correct.

> +TEST_PROGRAM_OBJS += $(patsubst %,t/helper/%.o,$(TEST_PROGRAMS_NEED_X))
> +.PRECIOUS: $(TEST_PROGRAM_OBJS)

Show 41 quoted lines

>  # List built-in command $C whose implementation cmd_$C() is not in
>  # builtin/$C.o but is linked in as part of some other command.
> @@ -2543,10 +2548,8 @@ REFTABLE_TEST_OBJS += reftable/stack_test.o
>  REFTABLE_TEST_OBJS += reftable/test_framework.o
>  REFTABLE_TEST_OBJS += reftable/tree_test.o
>  
> -TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
> -
>  .PHONY: test-objs
> -test-objs: $(TEST_OBJS)
> +test-objs: $(TEST_PROGRAM_OBJS)
>  
>  GIT_OBJS += $(LIB_OBJS)
>  GIT_OBJS += $(BUILTIN_OBJS)
> @@ -2562,7 +2565,7 @@ scalar-objs: $(SCALAR_OBJS)
>  OBJECTS += $(GIT_OBJS)
>  OBJECTS += $(SCALAR_OBJS)
>  OBJECTS += $(PROGRAM_OBJS)
> -OBJECTS += $(TEST_OBJS)
> +OBJECTS += $(TEST_PROGRAM_OBJS)
>  OBJECTS += $(XDIFF_OBJS)
>  OBJECTS += $(FUZZ_OBJS)
>  OBJECTS += $(REFTABLE_OBJS) $(REFTABLE_TEST_OBJS)
> @@ -3061,7 +3064,7 @@ 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)
> +all:: $(test_bindir_programs)
>  
>  bin-wrappers/%: wrap-for-bin.sh
>  	$(call mkdir_p_parent_template)
> @@ -3087,8 +3090,6 @@ perf: all
>  
>  .PHONY: test perf
>  
> -.PRECIOUS: $(TEST_OBJS)
> -
>  t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
>  
>  t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS) $(REFTABLE_TEST_LIB)