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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 26, 2022, 18:47 UTC
Message-ID: <221026.86y1t2beoj.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqsfjailak.fsf@gitster.g>

On Wed, Oct 26 2022, Junio C Hamano wrote:

Show 9 quoted lines

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> 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?

Yes, sorry I'll clarify that in a re-roll.

Show 6 quoted lines

>>  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?

I figured this would be better use of that bandwith, since the reviewer doesn't need to wonder why these are still spread befo/after the main body of the change.

Not everyone is keenly aware of the at first odd way a Makefile is read (per "3.7 How 'make' Reads a Makefile" in the GNU make manual).

But I'm happy to eject this part if that helps, but...

> 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.

...here I'm not quite sure if you want to keep it after all or nat...

> 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.

Thanks for the quick review!