Hi Ævar
On 21/01/2022 12:01, Ævar Arnfjörð Bjarmason wrote:
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.
Random cruft breaking the build was the reason I objected to this
change, just because the cruft was being tracked by git in this case
does not change that.
Best Wishes
Phillip
