On Tue, Nov 09 2021, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@xxxxxxxxx> writes: > >>> We can also have some DEVOPTS knob so we'll prune out files found if >>> a >>> $(shell)-out to "git status" tells us they're untracked. I wouldn't mind >>> that (and could implement it) if it was optional. >>> Also note that you've got some of this already, e.g. we'll pick up >>> *.h >>> files via a glob for "make TAGS", the dependency graph etc. >> >> I'd be happier using 'git ls-files' with a glob if we need to move >> away from listing the files explicitly rather than having to pass some >> exclude list when running make. Having seen your comments below about >> ls-files/find I had a look at the Makefile and they always seem to be >> used together as "git ls-files ... || find ...". Doing that would mean >> we wouldn't try to build any untracked files but still find everything >> in a tarball. > > I've been quiet on this topic because honestly I do not find the > pros-and-cons favourable for more use of wildcards [*]. Tools like > git (especially .gitignore) and Makefile are to help users to be > safely sloppy by ensuring that random crufts the users may create in > the working tree for their convenience are not picked up by default > unless the project to consciously expresses the desire to use them. > > Allowing to be sloppy while maintaining Makefile feels like a false > economy, and having to paper it over by adding exceptions and > forcing developers to learn such ad-hoc rules even more so. > > Side note: TAGS generation and some other minor things may use > $(wildcard) and can throw tokens in cruft files in the output, > which is not ideal, but the damage is local. We cannot treat > that the same as building binaries and tarballs. > > If we could use "git ls-files" consistently, that may make it > somewhat safer; you'd at least need to "git add" a new file before > it gets into the picture. But it would be impossible, because we > need to be able to bootstrap Git from a tarball extract. > >>>>> We could make this simpler still for the Makefile by moving >>>>> "unix-socket.c" etc. to e.g. a "conditional-src/" directory, likewise >>>>> for $(PROGRAM_OBJS) to e.g. "programs/". If we did that we would not >>>>> need the "$(filter-out)" for LIB_OBJS. I don't think that's worth it, >>>>> e.g. due to "git log -- <path>" on the files now needing a "--follow". > > And it is not quite clear to me why we want to even more pile > workaround like this on top. This also is to paper over the mistake > of being sloppy and using $(wildcard), which makes it unable to > distinguish, among the ones that match a pattern, between FOO_OBJS > and BAR_OBJS, no? Moving files around in the working tree to group > related things together is a good thing, and it has been a good move > to separate built-ins and library-ish parts into different > directories. But the above does not sound like it. > > Other than "these source files may or may not be compiled > depending", what trait do files in conditional-src/ share, other > than "dividing them into a separate category makes it simpler to > write Makefile using $(wildcard)"? I do not think of a good one. > > The only time I found that the large list of files in Makefile was > problematic was *NOT* when multiple topics added, renamed or removed > the files (it is pretty much bog standard merge conflicts that do > not happen very often to begin with). It is when this kind of > "large scale refactoring" for the sake of refactoring happens. > >> I'm not so worried about those other targets, but being able to >> reliably build and test git with some cruft lying around is useful >> though. I'm still not entirely sure what the motivation for this >> 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. 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/ 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_OBJS += reftable/basics.o -REFTABLE_OBJS += reftable/error.o -REFTABLE_OBJS += reftable/block.o -REFTABLE_OBJS += reftable/blocksource.o -REFTABLE_OBJS += reftable/iter.o -REFTABLE_OBJS += reftable/publicbasics.o -REFTABLE_OBJS += reftable/merged.o -REFTABLE_OBJS += reftable/pq.o -REFTABLE_OBJS += reftable/reader.o -REFTABLE_OBJS += reftable/record.o -REFTABLE_OBJS += reftable/refname.o -REFTABLE_OBJS += reftable/generic.o -REFTABLE_OBJS += reftable/stack.o -REFTABLE_OBJS += reftable/tree.o -REFTABLE_OBJS += reftable/writer.o - -REFTABLE_TEST_OBJS += reftable/basics_test.o -REFTABLE_TEST_OBJS += reftable/block_test.o -REFTABLE_TEST_OBJS += reftable/dump.o -REFTABLE_TEST_OBJS += reftable/merged_test.o -REFTABLE_TEST_OBJS += reftable/pq_test.o -REFTABLE_TEST_OBJS += reftable/record_test.o -REFTABLE_TEST_OBJS += reftable/readwrite_test.o -REFTABLE_TEST_OBJS += reftable/refname_test.o -REFTABLE_TEST_OBJS += reftable/stack_test.o -REFTABLE_TEST_OBJS += reftable/test_framework.o -REFTABLE_TEST_OBJS += reftable/tree_test.o +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, and we'd have been getting linker errors all along on the dead code (just showing one of many here): $ 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 [...] clang: error: linker command failed with exit code 1 (use -v to see invocation) make: *** [Makefile:2925: t/helper/test-tool] Error 1 make: Target 'all' not remade because of errors.
