Re: [PATCH v2 3/3] Makefile: replace most hardcoded object lists with $(wildcard)

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 22, 2022, 06:36 UTC
Message-ID: <xmqq35lgb7l5.fsf@gitster.g>
In-Reply-To: <220121.86o845jnvv.gmgdl@evledraar.gmail.com>

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

Show 6 quoted lines

>>> change is (adding new files is not that common) but I think using the
>>> established "git ls-files || find" pattern would be a good way of
>>> globbing without picking up rubbish if there is a compelling reason to
>>> drop the lists.
>>
>> Yes.

To avoid any misunderstandings, the above "Yes" was given to the statement, including the "if there is a compelling reason" part (and there isn't a compelling reason).

> Reviewing the reftable coverity topic I was reminded of this
> patch. I.e. in it we have this fix:
> https://lore.kernel.org/git/xmqqtugl102l.fsf@gitster.g/

I didn't give any "fix" in that message, though.

Show 18 quoted lines

> Which shows another advantage of using this sort of $(wildcard) pattern,
> i.e. if we had this:
> 	
> 	diff --git a/Makefile b/Makefile
> 	index 5580859afdb..48ea18afa53 100644
> 	--- a/Makefile
> 	+++ b/Makefile
> 	@@ -2443,33 +2443,9 @@ XDIFF_OBJS += xdiff/xutils.o
> 	 .PHONY: xdiff-objs
> 	 xdiff-objs: $(XDIFF_OBJS)
> 	 
> 	+REFTABLE_SOURCES = $(wildcard reftable/*.c)
> 	+REFTABLE_OBJS += $(filter-out test,$(REFTABLE_SOURCES:%.c=%.o))
> 	+REFTABLE_TEST_OBJS += $(filter test,$(REFTABLE_SOURCES:%.c=%.o))
> 	 
> 	 TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
>
> We'd have a shorter Makefile, not need to manually maintain the list,

Both are not all that important.

> and we'd have been getting linker errors all along on the dead code
> (just showing one of many here):

I am not sure if I follow.  You are forgetting to tell us something.

Are you talking about an error you would see when you do what?

Perhaps after you remove reftable/generic.c and have the definition of reftable_table_seek_ref() that used to be there in reftable/reftable.c?

Assuming that is the scenario you have in mind, ...

> 	$ make
> 	[...]
> 	/usr/bin/ld: reftable/libreftable.a(generic.o): in function `reftable_table_seek_ref':
> 	/home/avar/g/git/reftable/generic.c:17: multiple definition of `reftable_table_seek_ref'; reftable/libreftable.a(reftable.o):/home/avar/g/git/reftable/reftable.c:17: first defined here

... I do not think concrete list of filenames vs list of filenames created by $(wildcard) has any effect on that the fact that lib.a that is incrementally updated by the "ar r lib.a" command does not lose a stale object file from it.

If we have a concrete filename list and removed generic.c, if we forget to remove it from the list, it will be noticed way before "ld" has the chance to complain. We fail to produce generic.o, which may be a plus. If we did not forget to also remove it from the list when we removed the file, then $(wildcard) will give us the same list of filenames, so you'd see the same error from your ld, no?