[PATCH] Makefile: replace most hardcoded object lists with $(wildcard)

[PATCH] Makefile: replace most hardcoded object lists with $(wildcard)

[Ævar Arnfjörð Bjarmason]

Remove the hardcoded lists of objects in favor of using
$(wildcard). This means that every time a built-in, test tool etc. is
added we won't need to patch the top-level Makefile, except for the
few remaining cases where the asset in question would make it onto one
of our list of exceptions.

Ever since 81b50f3ce40 (Move 'builtin-*' into a 'builtin/'
subdirectory, 2010-02-22) this has been relatively easy to do (and
even before that we could glob builtin-*.c). This pattern of
exhaustively enumerating files was then carried forward for
e.g. TEST_BUILTINS_OBJS in efd71f8913a (t/helper: add an empty
test-tool program, 2018-03-24).

One reason not to do this is that now a new *.c file at the top-level
will be immediately picked up, so if a new *.c file is being worked on
"make" will error if it doesn't compile, whereas before that file
would need to be explicitly listed in the Makefile. I think than small
trade-off is worth it.

There's a few small "while we're at it" changes here, since I'm
touching the code in question:

 - Start splitting up the the "Guard against the environment" section
   at the top, but don't move anything that exists there out to avoid
   merge conflicts

 - The $(TEST_BUILTINS_OBJS) variable was needlessly complex, because
   it didn't have the full paths we'd pathsubst it back & forth.

 - Introduce *_SRC in addition to *_OBJ for the variable I'm
   touching. Eventually we'll want to do this for all the *.o files,
   i.e. make the *.c list a source of truth for *.o, which means we can
   e.g. use that exhaustive list for "make TAGS".

 - Add a missing "curl-objs" target. See 029bac01a87 (Makefile: add
   {program,xdiff,test,git,fuzz}-objs & objects targets, 2021-02-23)
   for the commit that added the rest.

 - De-indent an "ifndef" block, we don't usually indent their
   contents.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

There's probably never a good time to submit a change like this,
i.e. it'll likely always conflict with something, perhaps the
around-release period is paradoxically better than most.

This conflicts with some existing topics (including one of my own to
add the "hook" built in), but those merge conflicts are resolvable by
keeping this side of the conflict. I.e. we'll no longer need to
manually maintain these lists in the Makefile for the common cases.

 Makefile | 484 +++++++------------------------------------------------
 1 file changed, 54 insertions(+), 430 deletions(-)

diff --git a/Makefile b/Makefile
index 12be39ac497..2f20fa54940 100644
--- a/Makefile
+++ b/Makefile
@@ -590,6 +590,19 @@ TEST_OBJS =
 TEST_PROGRAMS_NEED_X =
 THIRD_PARTY_SOURCES =
 
+## Guard against env: programs
+TEST_PROGRAMS =
+
+## Guard against env: sources
+CURL_SRC =
+TEST_PROGRAMS_NEED_X_SRC =
+XDIFF_SRC =
+
+## Guard against env: objects
+ALL_COMPAT_OBJS =
+CURL_OBJS =
+LIB_OBJS_DIRS =
+
 # Having this variable in your environment would break pipelines because
 # you cause "cd" to echo its destination to stdout.  It can also take
 # scripts to unexpected places.  If you like CDPATH, define it for your
@@ -688,87 +701,23 @@ X =
 
 PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
 
-TEST_BUILTINS_OBJS += test-advise.o
-TEST_BUILTINS_OBJS += test-bitmap.o
-TEST_BUILTINS_OBJS += test-bloom.o
-TEST_BUILTINS_OBJS += test-chmtime.o
-TEST_BUILTINS_OBJS += test-config.o
-TEST_BUILTINS_OBJS += test-crontab.o
-TEST_BUILTINS_OBJS += test-ctype.o
-TEST_BUILTINS_OBJS += test-date.o
-TEST_BUILTINS_OBJS += test-delta.o
-TEST_BUILTINS_OBJS += test-dir-iterator.o
-TEST_BUILTINS_OBJS += test-drop-caches.o
-TEST_BUILTINS_OBJS += test-dump-cache-tree.o
-TEST_BUILTINS_OBJS += test-dump-fsmonitor.o
-TEST_BUILTINS_OBJS += test-dump-split-index.o
-TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
-TEST_BUILTINS_OBJS += test-example-decorate.o
-TEST_BUILTINS_OBJS += test-fast-rebase.o
-TEST_BUILTINS_OBJS += test-genrandom.o
-TEST_BUILTINS_OBJS += test-genzeros.o
-TEST_BUILTINS_OBJS += test-getcwd.o
-TEST_BUILTINS_OBJS += test-hash-speed.o
-TEST_BUILTINS_OBJS += test-hash.o
-TEST_BUILTINS_OBJS += test-hashmap.o
-TEST_BUILTINS_OBJS += test-index-version.o
-TEST_BUILTINS_OBJS += test-json-writer.o
-TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o
-TEST_BUILTINS_OBJS += test-match-trees.o
-TEST_BUILTINS_OBJS += test-mergesort.o
-TEST_BUILTINS_OBJS += test-mktemp.o
-TEST_BUILTINS_OBJS += test-oid-array.o
-TEST_BUILTINS_OBJS += test-oidmap.o
-TEST_BUILTINS_OBJS += test-oidtree.o
-TEST_BUILTINS_OBJS += test-online-cpus.o
-TEST_BUILTINS_OBJS += test-parse-options.o
-TEST_BUILTINS_OBJS += test-parse-pathspec-file.o
-TEST_BUILTINS_OBJS += test-partial-clone.o
-TEST_BUILTINS_OBJS += test-path-utils.o
-TEST_BUILTINS_OBJS += test-pcre2-config.o
-TEST_BUILTINS_OBJS += test-pkt-line.o
-TEST_BUILTINS_OBJS += test-prio-queue.o
-TEST_BUILTINS_OBJS += test-proc-receive.o
-TEST_BUILTINS_OBJS += test-progress.o
-TEST_BUILTINS_OBJS += test-reach.o
-TEST_BUILTINS_OBJS += test-read-cache.o
-TEST_BUILTINS_OBJS += test-read-graph.o
-TEST_BUILTINS_OBJS += test-read-midx.o
-TEST_BUILTINS_OBJS += test-ref-store.o
-TEST_BUILTINS_OBJS += test-regex.o
-TEST_BUILTINS_OBJS += test-repository.o
-TEST_BUILTINS_OBJS += test-revision-walking.o
-TEST_BUILTINS_OBJS += test-run-command.o
-TEST_BUILTINS_OBJS += test-scrap-cache-tree.o
-TEST_BUILTINS_OBJS += test-serve-v2.o
-TEST_BUILTINS_OBJS += test-sha1.o
-TEST_BUILTINS_OBJS += test-sha256.o
-TEST_BUILTINS_OBJS += test-sigchain.o
-TEST_BUILTINS_OBJS += test-simple-ipc.o
-TEST_BUILTINS_OBJS += test-strcmp-offset.o
-TEST_BUILTINS_OBJS += test-string-list.o
-TEST_BUILTINS_OBJS += test-submodule-config.o
-TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o
-TEST_BUILTINS_OBJS += test-subprocess.o
-TEST_BUILTINS_OBJS += test-trace2.o
-TEST_BUILTINS_OBJS += test-urlmatch-normalization.o
-TEST_BUILTINS_OBJS += test-userdiff.o
-TEST_BUILTINS_OBJS += test-wildmatch.o
-TEST_BUILTINS_OBJS += test-windows-named-pipe.o
-TEST_BUILTINS_OBJS += test-write-cache.o
-TEST_BUILTINS_OBJS += test-xml-encode.o
-
 # Do not add more tests here unless they have extra dependencies. Add
 # them in TEST_BUILTINS_OBJS above.
 TEST_PROGRAMS_NEED_X += test-fake-ssh
 TEST_PROGRAMS_NEED_X += test-tool
 
-TEST_PROGRAMS = $(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X))
+TEST_PROGRAMS_NEED_X_SRC += $(TEST_PROGRAMS_NEED_X:%=t/helper/%.c)
+TEST_PROGRAMS += $(TEST_PROGRAMS_NEED_X_SRC:%.c=%$X)
+TEST_BUILTINS_SRC += $(filter-out $(TEST_PROGRAMS_NEED_X_SRC),$(wildcard t/helper/*.c))
+TEST_BUILTINS_OBJS += $(TEST_BUILTINS_SRC:%.c=%.o)
 
-# 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.
+# List built-in command $C whose implementation cmd_$C() is in
+# builtin/$C.o
+BUILTIN_OBJS = $(patsubst %.c,%.o,$(wildcard builtin/*.c))
 BUILT_INS += $(patsubst builtin/%.o,git-%$X,$(BUILTIN_OBJS))
 
+# 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.
 BUILT_INS += git-cherry$X
 BUILT_INS += git-cherry-pick$X
 BUILT_INS += git-format-patch$X
@@ -828,355 +777,28 @@ LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentat
 	-name Documentation -prune -o \
 	-name '*.h' -print)))
 
-LIB_OBJS += abspath.o
-LIB_OBJS += add-interactive.o
-LIB_OBJS += add-patch.o
-LIB_OBJS += advice.o
-LIB_OBJS += alias.o
-LIB_OBJS += alloc.o
-LIB_OBJS += apply.o
-LIB_OBJS += archive-tar.o
-LIB_OBJS += archive-zip.o
-LIB_OBJS += archive.o
-LIB_OBJS += attr.o
-LIB_OBJS += base85.o
-LIB_OBJS += bisect.o
-LIB_OBJS += blame.o
-LIB_OBJS += blob.o
-LIB_OBJS += bloom.o
-LIB_OBJS += branch.o
-LIB_OBJS += bulk-checkin.o
-LIB_OBJS += bundle.o
-LIB_OBJS += cache-tree.o
-LIB_OBJS += cbtree.o
-LIB_OBJS += chdir-notify.o
-LIB_OBJS += checkout.o
-LIB_OBJS += chunk-format.o
-LIB_OBJS += color.o
-LIB_OBJS += column.o
-LIB_OBJS += combine-diff.o
-LIB_OBJS += commit-graph.o
-LIB_OBJS += commit-reach.o
-LIB_OBJS += commit.o
+# LIB_OBJS: compat/* objects that live at the top-level
+ALL_COMPAT_OBJS += unix-socket.o
+ALL_COMPAT_OBJS += unix-stream-server.o
+ALL_COMPAT_OBJS += sha1dc_git.o
+
+# LIB_OBJS: Mostly glob *.c at the top-level, with some exlusions
+LIB_OBJS += $(filter-out \
+	$(ALL_COMPAT_OBJS) \
+	git.o common-main.o $(PROGRAM_OBJS) \
+	$(FUZZ_OBJS) $(CURL_OBJS),\
+	$(patsubst %.c,%.o,$(wildcard *.c)))
+
+# LIB_OBJS: Directories that contain only LIB_OBJS
+LIB_OBJS_DIRS += ewah
+LIB_OBJS_DIRS += negotiator
+LIB_OBJS_DIRS += refs
+LIB_OBJS_DIRS += trace2
+LIB_OBJS += $(patsubst %.c,%.o,$(foreach dir,$(LIB_OBJS_DIRS),$(wildcard $(dir)/*.c)))
+
+# LIB_OBJS: unconditional compat/* objects
 LIB_OBJS += compat/obstack.o
 LIB_OBJS += compat/terminal.o
-LIB_OBJS += config.o
-LIB_OBJS += connect.o
-LIB_OBJS += connected.o
-LIB_OBJS += convert.o
-LIB_OBJS += copy.o
-LIB_OBJS += credential.o
-LIB_OBJS += csum-file.o
-LIB_OBJS += ctype.o
-LIB_OBJS += date.o
-LIB_OBJS += decorate.o
-LIB_OBJS += delta-islands.o
-LIB_OBJS += diff-delta.o
-LIB_OBJS += diff-merges.o
-LIB_OBJS += diff-lib.o
-LIB_OBJS += diff-no-index.o
-LIB_OBJS += diff.o
-LIB_OBJS += diffcore-break.o
-LIB_OBJS += diffcore-delta.o
-LIB_OBJS += diffcore-order.o
-LIB_OBJS += diffcore-pickaxe.o
-LIB_OBJS += diffcore-rename.o
-LIB_OBJS += diffcore-rotate.o
-LIB_OBJS += dir-iterator.o
-LIB_OBJS += dir.o
-LIB_OBJS += editor.o
-LIB_OBJS += entry.o
-LIB_OBJS += environment.o
-LIB_OBJS += ewah/bitmap.o
-LIB_OBJS += ewah/ewah_bitmap.o
-LIB_OBJS += ewah/ewah_io.o
-LIB_OBJS += ewah/ewah_rlw.o
-LIB_OBJS += exec-cmd.o
-LIB_OBJS += fetch-negotiator.o
-LIB_OBJS += fetch-pack.o
-LIB_OBJS += fmt-merge-msg.o
-LIB_OBJS += fsck.o
-LIB_OBJS += fsmonitor.o
-LIB_OBJS += gettext.o
-LIB_OBJS += gpg-interface.o
-LIB_OBJS += graph.o
-LIB_OBJS += grep.o
-LIB_OBJS += hash-lookup.o
-LIB_OBJS += hashmap.o
-LIB_OBJS += help.o
-LIB_OBJS += hex.o
-LIB_OBJS += hook.o
-LIB_OBJS += ident.o
-LIB_OBJS += json-writer.o
-LIB_OBJS += kwset.o
-LIB_OBJS += levenshtein.o
-LIB_OBJS += line-log.o
-LIB_OBJS += line-range.o
-LIB_OBJS += linear-assignment.o
-LIB_OBJS += list-objects-filter-options.o
-LIB_OBJS += list-objects-filter.o
-LIB_OBJS += list-objects.o
-LIB_OBJS += ll-merge.o
-LIB_OBJS += lockfile.o
-LIB_OBJS += log-tree.o
-LIB_OBJS += ls-refs.o
-LIB_OBJS += mailinfo.o
-LIB_OBJS += mailmap.o
-LIB_OBJS += match-trees.o
-LIB_OBJS += mem-pool.o
-LIB_OBJS += merge-blobs.o
-LIB_OBJS += merge-ort.o
-LIB_OBJS += merge-ort-wrappers.o
-LIB_OBJS += merge-recursive.o
-LIB_OBJS += merge.o
-LIB_OBJS += mergesort.o
-LIB_OBJS += midx.o
-LIB_OBJS += name-hash.o
-LIB_OBJS += negotiator/default.o
-LIB_OBJS += negotiator/noop.o
-LIB_OBJS += negotiator/skipping.o
-LIB_OBJS += notes-cache.o
-LIB_OBJS += notes-merge.o
-LIB_OBJS += notes-utils.o
-LIB_OBJS += notes.o
-LIB_OBJS += object-file.o
-LIB_OBJS += object-name.o
-LIB_OBJS += object.o
-LIB_OBJS += oid-array.o
-LIB_OBJS += oidmap.o
-LIB_OBJS += oidset.o
-LIB_OBJS += oidtree.o
-LIB_OBJS += pack-bitmap-write.o
-LIB_OBJS += pack-bitmap.o
-LIB_OBJS += pack-check.o
-LIB_OBJS += pack-objects.o
-LIB_OBJS += pack-revindex.o
-LIB_OBJS += pack-write.o
-LIB_OBJS += packfile.o
-LIB_OBJS += pager.o
-LIB_OBJS += parallel-checkout.o
-LIB_OBJS += parse-options-cb.o
-LIB_OBJS += parse-options.o
-LIB_OBJS += patch-delta.o
-LIB_OBJS += patch-ids.o
-LIB_OBJS += path.o
-LIB_OBJS += pathspec.o
-LIB_OBJS += pkt-line.o
-LIB_OBJS += preload-index.o
-LIB_OBJS += pretty.o
-LIB_OBJS += prio-queue.o
-LIB_OBJS += progress.o
-LIB_OBJS += promisor-remote.o
-LIB_OBJS += prompt.o
-LIB_OBJS += protocol.o
-LIB_OBJS += protocol-caps.o
-LIB_OBJS += prune-packed.o
-LIB_OBJS += quote.o
-LIB_OBJS += range-diff.o
-LIB_OBJS += reachable.o
-LIB_OBJS += read-cache.o
-LIB_OBJS += rebase-interactive.o
-LIB_OBJS += rebase.o
-LIB_OBJS += ref-filter.o
-LIB_OBJS += reflog-walk.o
-LIB_OBJS += refs.o
-LIB_OBJS += refs/debug.o
-LIB_OBJS += refs/files-backend.o
-LIB_OBJS += refs/iterator.o
-LIB_OBJS += refs/packed-backend.o
-LIB_OBJS += refs/ref-cache.o
-LIB_OBJS += refspec.o
-LIB_OBJS += remote.o
-LIB_OBJS += replace-object.o
-LIB_OBJS += repo-settings.o
-LIB_OBJS += repository.o
-LIB_OBJS += rerere.o
-LIB_OBJS += reset.o
-LIB_OBJS += resolve-undo.o
-LIB_OBJS += revision.o
-LIB_OBJS += run-command.o
-LIB_OBJS += send-pack.o
-LIB_OBJS += sequencer.o
-LIB_OBJS += serve.o
-LIB_OBJS += server-info.o
-LIB_OBJS += setup.o
-LIB_OBJS += shallow.o
-LIB_OBJS += sideband.o
-LIB_OBJS += sigchain.o
-LIB_OBJS += sparse-index.o
-LIB_OBJS += split-index.o
-LIB_OBJS += stable-qsort.o
-LIB_OBJS += strbuf.o
-LIB_OBJS += streaming.o
-LIB_OBJS += string-list.o
-LIB_OBJS += strmap.o
-LIB_OBJS += strvec.o
-LIB_OBJS += sub-process.o
-LIB_OBJS += submodule-config.o
-LIB_OBJS += submodule.o
-LIB_OBJS += symlinks.o
-LIB_OBJS += tag.o
-LIB_OBJS += tempfile.o
-LIB_OBJS += thread-utils.o
-LIB_OBJS += tmp-objdir.o
-LIB_OBJS += trace.o
-LIB_OBJS += trace2.o
-LIB_OBJS += trace2/tr2_cfg.o
-LIB_OBJS += trace2/tr2_cmd_name.o
-LIB_OBJS += trace2/tr2_dst.o
-LIB_OBJS += trace2/tr2_sid.o
-LIB_OBJS += trace2/tr2_sysenv.o
-LIB_OBJS += trace2/tr2_tbuf.o
-LIB_OBJS += trace2/tr2_tgt_event.o
-LIB_OBJS += trace2/tr2_tgt_normal.o
-LIB_OBJS += trace2/tr2_tgt_perf.o
-LIB_OBJS += trace2/tr2_tls.o
-LIB_OBJS += trailer.o
-LIB_OBJS += transport-helper.o
-LIB_OBJS += transport.o
-LIB_OBJS += tree-diff.o
-LIB_OBJS += tree-walk.o
-LIB_OBJS += tree.o
-LIB_OBJS += unpack-trees.o
-LIB_OBJS += upload-pack.o
-LIB_OBJS += url.o
-LIB_OBJS += urlmatch.o
-LIB_OBJS += usage.o
-LIB_OBJS += userdiff.o
-LIB_OBJS += utf8.o
-LIB_OBJS += varint.o
-LIB_OBJS += version.o
-LIB_OBJS += versioncmp.o
-LIB_OBJS += walker.o
-LIB_OBJS += wildmatch.o
-LIB_OBJS += worktree.o
-LIB_OBJS += wrapper.o
-LIB_OBJS += write-or-die.o
-LIB_OBJS += ws.o
-LIB_OBJS += wt-status.o
-LIB_OBJS += xdiff-interface.o
-LIB_OBJS += zlib.o
-
-BUILTIN_OBJS += builtin/add.o
-BUILTIN_OBJS += builtin/am.o
-BUILTIN_OBJS += builtin/annotate.o
-BUILTIN_OBJS += builtin/apply.o
-BUILTIN_OBJS += builtin/archive.o
-BUILTIN_OBJS += builtin/bisect--helper.o
-BUILTIN_OBJS += builtin/blame.o
-BUILTIN_OBJS += builtin/branch.o
-BUILTIN_OBJS += builtin/bugreport.o
-BUILTIN_OBJS += builtin/bundle.o
-BUILTIN_OBJS += builtin/cat-file.o
-BUILTIN_OBJS += builtin/check-attr.o
-BUILTIN_OBJS += builtin/check-ignore.o
-BUILTIN_OBJS += builtin/check-mailmap.o
-BUILTIN_OBJS += builtin/check-ref-format.o
-BUILTIN_OBJS += builtin/checkout--worker.o
-BUILTIN_OBJS += builtin/checkout-index.o
-BUILTIN_OBJS += builtin/checkout.o
-BUILTIN_OBJS += builtin/clean.o
-BUILTIN_OBJS += builtin/clone.o
-BUILTIN_OBJS += builtin/column.o
-BUILTIN_OBJS += builtin/commit-graph.o
-BUILTIN_OBJS += builtin/commit-tree.o
-BUILTIN_OBJS += builtin/commit.o
-BUILTIN_OBJS += builtin/config.o
-BUILTIN_OBJS += builtin/count-objects.o
-BUILTIN_OBJS += builtin/credential-cache--daemon.o
-BUILTIN_OBJS += builtin/credential-cache.o
-BUILTIN_OBJS += builtin/credential-store.o
-BUILTIN_OBJS += builtin/credential.o
-BUILTIN_OBJS += builtin/describe.o
-BUILTIN_OBJS += builtin/diff-files.o
-BUILTIN_OBJS += builtin/diff-index.o
-BUILTIN_OBJS += builtin/diff-tree.o
-BUILTIN_OBJS += builtin/diff.o
-BUILTIN_OBJS += builtin/difftool.o
-BUILTIN_OBJS += builtin/env--helper.o
-BUILTIN_OBJS += builtin/fast-export.o
-BUILTIN_OBJS += builtin/fast-import.o
-BUILTIN_OBJS += builtin/fetch-pack.o
-BUILTIN_OBJS += builtin/fetch.o
-BUILTIN_OBJS += builtin/fmt-merge-msg.o
-BUILTIN_OBJS += builtin/for-each-ref.o
-BUILTIN_OBJS += builtin/for-each-repo.o
-BUILTIN_OBJS += builtin/fsck.o
-BUILTIN_OBJS += builtin/gc.o
-BUILTIN_OBJS += builtin/get-tar-commit-id.o
-BUILTIN_OBJS += builtin/grep.o
-BUILTIN_OBJS += builtin/hash-object.o
-BUILTIN_OBJS += builtin/help.o
-BUILTIN_OBJS += builtin/index-pack.o
-BUILTIN_OBJS += builtin/init-db.o
-BUILTIN_OBJS += builtin/interpret-trailers.o
-BUILTIN_OBJS += builtin/log.o
-BUILTIN_OBJS += builtin/ls-files.o
-BUILTIN_OBJS += builtin/ls-remote.o
-BUILTIN_OBJS += builtin/ls-tree.o
-BUILTIN_OBJS += builtin/mailinfo.o
-BUILTIN_OBJS += builtin/mailsplit.o
-BUILTIN_OBJS += builtin/merge-base.o
-BUILTIN_OBJS += builtin/merge-file.o
-BUILTIN_OBJS += builtin/merge-index.o
-BUILTIN_OBJS += builtin/merge-ours.o
-BUILTIN_OBJS += builtin/merge-recursive.o
-BUILTIN_OBJS += builtin/merge-tree.o
-BUILTIN_OBJS += builtin/merge.o
-BUILTIN_OBJS += builtin/mktag.o
-BUILTIN_OBJS += builtin/mktree.o
-BUILTIN_OBJS += builtin/multi-pack-index.o
-BUILTIN_OBJS += builtin/mv.o
-BUILTIN_OBJS += builtin/name-rev.o
-BUILTIN_OBJS += builtin/notes.o
-BUILTIN_OBJS += builtin/pack-objects.o
-BUILTIN_OBJS += builtin/pack-redundant.o
-BUILTIN_OBJS += builtin/pack-refs.o
-BUILTIN_OBJS += builtin/patch-id.o
-BUILTIN_OBJS += builtin/prune-packed.o
-BUILTIN_OBJS += builtin/prune.o
-BUILTIN_OBJS += builtin/pull.o
-BUILTIN_OBJS += builtin/push.o
-BUILTIN_OBJS += builtin/range-diff.o
-BUILTIN_OBJS += builtin/read-tree.o
-BUILTIN_OBJS += builtin/rebase.o
-BUILTIN_OBJS += builtin/receive-pack.o
-BUILTIN_OBJS += builtin/reflog.o
-BUILTIN_OBJS += builtin/remote-ext.o
-BUILTIN_OBJS += builtin/remote-fd.o
-BUILTIN_OBJS += builtin/remote.o
-BUILTIN_OBJS += builtin/repack.o
-BUILTIN_OBJS += builtin/replace.o
-BUILTIN_OBJS += builtin/rerere.o
-BUILTIN_OBJS += builtin/reset.o
-BUILTIN_OBJS += builtin/rev-list.o
-BUILTIN_OBJS += builtin/rev-parse.o
-BUILTIN_OBJS += builtin/revert.o
-BUILTIN_OBJS += builtin/rm.o
-BUILTIN_OBJS += builtin/send-pack.o
-BUILTIN_OBJS += builtin/shortlog.o
-BUILTIN_OBJS += builtin/show-branch.o
-BUILTIN_OBJS += builtin/show-index.o
-BUILTIN_OBJS += builtin/show-ref.o
-BUILTIN_OBJS += builtin/sparse-checkout.o
-BUILTIN_OBJS += builtin/stash.o
-BUILTIN_OBJS += builtin/stripspace.o
-BUILTIN_OBJS += builtin/submodule--helper.o
-BUILTIN_OBJS += builtin/symbolic-ref.o
-BUILTIN_OBJS += builtin/tag.o
-BUILTIN_OBJS += builtin/unpack-file.o
-BUILTIN_OBJS += builtin/unpack-objects.o
-BUILTIN_OBJS += builtin/update-index.o
-BUILTIN_OBJS += builtin/update-ref.o
-BUILTIN_OBJS += builtin/update-server-info.o
-BUILTIN_OBJS += builtin/upload-archive.o
-BUILTIN_OBJS += builtin/upload-pack.o
-BUILTIN_OBJS += builtin/var.o
-BUILTIN_OBJS += builtin/verify-commit.o
-BUILTIN_OBJS += builtin/verify-pack.o
-BUILTIN_OBJS += builtin/verify-tag.o
-BUILTIN_OBJS += builtin/worktree.o
-BUILTIN_OBJS += builtin/write-tree.o
 
 # THIRD_PARTY_SOURCES is a list of patterns compatible with the
 # $(filter) and $(filter-out) family of functions. They specify source
@@ -2427,17 +2049,12 @@ reconfigure config.mak.autogen: config.status
 .PHONY: reconfigure # This is a convenience target.
 endif
 
-XDIFF_OBJS += xdiff/xdiffi.o
-XDIFF_OBJS += xdiff/xemit.o
-XDIFF_OBJS += xdiff/xhistogram.o
-XDIFF_OBJS += xdiff/xmerge.o
-XDIFF_OBJS += xdiff/xpatience.o
-XDIFF_OBJS += xdiff/xprepare.o
-XDIFF_OBJS += xdiff/xutils.o
+XDIFF_SRC += $(wildcard xdiff/*.c)
+XDIFF_OBJS += $(XDIFF_SRC:.c=.o)
 .PHONY: xdiff-objs
 xdiff-objs: $(XDIFF_OBJS)
 
-TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
+TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(TEST_BUILTINS_OBJS)
 .PHONY: test-objs
 test-objs: $(TEST_OBJS)
 
@@ -2448,13 +2065,20 @@ GIT_OBJS += git.o
 .PHONY: git-objs
 git-objs: $(GIT_OBJS)
 
+CURL_SRC += http.c
+CURL_SRC += http-walker.c
+CURL_SRC += remote-curl.c
+CURL_OBJS += $(CURL_SRC:.c=.o)
+.PHONY: curl-objs
+curl-objs: $(CURL_OBJS)
+
 OBJECTS += $(GIT_OBJS)
 OBJECTS += $(PROGRAM_OBJS)
 OBJECTS += $(TEST_OBJS)
 OBJECTS += $(XDIFF_OBJS)
 OBJECTS += $(FUZZ_OBJS)
 ifndef NO_CURL
-	OBJECTS += http.o http-walker.o remote-curl.o
+OBJECTS += $(CURL_OBJS)
 endif
 .PHONY: objects
 objects: $(OBJECTS)
@@ -2891,7 +2515,7 @@ perf: all
 
 .PRECIOUS: $(TEST_OBJS)
 
-t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
+t/helper/test-tool$X: $(TEST_BUILTINS_OBJS)
 
 t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
-- 
2.33.1.1570.g069344fdd45

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

[Paul Smith]

On Sun, 2021-10-31 at 00:32 +0200, Ævar Arnfjörð Bjarmason wrote:
> +LIB_OBJS += $(patsubst %.c,%.o,$(foreach dir,$(LIB_OBJS_DIRS),$(wildcard $(dir)/*.c)))

Another way to write this would be:

   LIB_OBJS += $(patsubst %.c,%.o,$(wildcard $(addsuffix /*.c,$(LIB_OBJS_DIRS)))

I don't know that there's any reason to choose one over the other.  I
don't think there's any real performance difference although one could
imagine this version to be VERY SLIGHTLY faster.  Also this one is a
little more "Lisp-ish"... that might be a pro or a con depending :).

Just kibitzing while waiting for dinner to arrive...

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

[Ævar Arnfjörð Bjarmason]

On Sat, Oct 30 2021, Paul Smith wrote:

> On Sun, 2021-10-31 at 00:32 +0200, Ævar Arnfjörð Bjarmason wrote:
>> +LIB_OBJS += $(patsubst %.c,%.o,$(foreach dir,$(LIB_OBJS_DIRS),$(wildcard $(dir)/*.c)))
>
> Another way to write this would be:
>
>    LIB_OBJS += $(patsubst %.c,%.o,$(wildcard $(addsuffix /*.c,$(LIB_OBJS_DIRS)))
>
> I don't know that there's any reason to choose one over the other.  I
> don't think there's any real performance difference although one could
> imagine this version to be VERY SLIGHTLY faster.  Also this one is a
> little more "Lisp-ish"... that might be a pro or a con depending :).
>
> Just kibitzing while waiting for dinner to arrive...

Thanks. I changed it to use that in the v2.

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

[Jeff King]

On Sun, Oct 31, 2021 at 12:32:26AM +0200, Ævar Arnfjörð Bjarmason wrote:

> Remove the hardcoded lists of objects in favor of using
> $(wildcard). This means that every time a built-in, test tool etc. is
> added we won't need to patch the top-level Makefile, except for the
> few remaining cases where the asset in question would make it onto one
> of our list of exceptions.
> 
> Ever since 81b50f3ce40 (Move 'builtin-*' into a 'builtin/'
> subdirectory, 2010-02-22) this has been relatively easy to do (and
> even before that we could glob builtin-*.c). This pattern of
> exhaustively enumerating files was then carried forward for
> e.g. TEST_BUILTINS_OBJS in efd71f8913a (t/helper: add an empty
> test-tool program, 2018-03-24).
> 
> One reason not to do this is that now a new *.c file at the top-level
> will be immediately picked up, so if a new *.c file is being worked on
> "make" will error if it doesn't compile, whereas before that file
> would need to be explicitly listed in the Makefile. I think than small
> trade-off is worth it.

A more general way of thinking about this is that we are switching from
"ignore source files by default" to "stick source files into LIB_OBJS by
default". So it's helpful if you were going to stick that file into
LIB_OBJS, but harmful otherwise.

Your "new *.c file" example is one case, because it wouldn't have been
added _yet_. And I agree it's probably not that big a deal in practice.

The other cases are ones similar to what you had to exclude from
LIB_OBJS manually here:

> +LIB_OBJS += $(filter-out \
> +	$(ALL_COMPAT_OBJS) \
> +	git.o common-main.o $(PROGRAM_OBJS) \
> +	$(FUZZ_OBJS) $(CURL_OBJS),\
> +	$(patsubst %.c,%.o,$(wildcard *.c)))

So if I wanted to add a new external program source but forgot to put it
into PROGRAM_OBJS, the default would now be to pick it up in LIB_OBJS.
That's weird and definitely not what you'd want, but presumably you'd
figure it out pretty quickly because we wouldn't have built the command
you expected to exist.

Likewise, there's an interesting tradeoff here for non-program object
files. The current Makefile does not need to mention unix-socket.o
outside of the NO_UNIX_SOCKETS ifdef block, because that's where we
stick it in LIB_OBJS. After your patch, it gets mentioned twice: in that
same spot, but also as an exception to the LIB_OBJS rule (via the
ALL_COMPAT_OBJS variable above).

So we're trading off having to remember to do one thing (add stuff to
LIB_OBJS) for another (add stuff to the exception list). Now one of
those happens a lot more than the other, which is why you get such a
nice diffstat. So it might be worth the tradeoff.

I don't have a very strong opinion either way on this. I felt like we'd
discussed this direction before, and came up with this thread from the
archive:

  https://lore.kernel.org/git/20110222155637.GC27178@sigill.intra.peff.net/

There it was coupled with suggestions to actually change the file
layout. That could make some of those exceptions go away (say, if all of
LIB_OBJS was in "lib/"), but it's a bigger change overall. So I offer it
here mostly for historical context / interest.

I didn't see anything obviously wrong in the patch, but two comments:

>  - De-indent an "ifndef" block, we don't usually indent their
>    contents.

Quite a lot of existing conditional blocks are indented, but I think for
conditional inclusions of one entry in a larger list (where the rest of
the list isn't indented), this makes sense. And that's what you changed
here.

> +# LIB_OBJS: compat/* objects that live at the top-level
> +ALL_COMPAT_OBJS += unix-socket.o
> +ALL_COMPAT_OBJS += unix-stream-server.o
> +ALL_COMPAT_OBJS += sha1dc_git.o

I think "compat" is a misnomer here. For one thing, they're by
definition not "compat/*" objects, because they're not in that
directory. ;) But more importantly, the interesting thing about them is
not that they're compatibility layers, but that they're part of a
conditional compilation. I.e., we might or might not want them, which
will be determined elsewhere in the Makefile, so they must not be part
of the base LIB_OBJS set.

Probably CONDITIONAL_OBJS or something might be more descriptive. That
_could_ be used to include things like CURL_OBJS, but there's probably
value in keeping those in their own list anyway.

Likewise, they could go into a conditional-src/ directory (or some
less-horrible name) to keep them distinct without needing an explicit
list in the Makefile. That's sort of the flip-side of putting all the
other LIB_OBJS ones into lib/.

-Peff

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

[Ævar Arnfjörð Bjarmason]

On Sun, Oct 31 2021, Jeff King wrote:

> On Sun, Oct 31, 2021 at 12:32:26AM +0200, Ævar Arnfjörð Bjarmason wrote:
> [...]
>> +# LIB_OBJS: compat/* objects that live at the top-level
>> +ALL_COMPAT_OBJS += unix-socket.o
>> +ALL_COMPAT_OBJS += unix-stream-server.o
>> +ALL_COMPAT_OBJS += sha1dc_git.o
>
> I think "compat" is a misnomer here. For one thing, they're by
> definition not "compat/*" objects, because they're not in that
> directory. ;) But more importantly, the interesting thing about them is
> not that they're compatibility layers, but that they're part of a
> conditional compilation. I.e., we might or might not want them, which
> will be determined elsewhere in the Makefile, so they must not be part
> of the base LIB_OBJS set.
>
> Probably CONDITIONAL_OBJS or something might be more descriptive. That
> _could_ be used to include things like CURL_OBJS, but there's probably
> value in keeping those in their own list anyway.

Good point, will rename them.

> Likewise, they could go into a conditional-src/ directory (or some
> less-horrible name) to keep them distinct without needing an explicit
> list in the Makefile. That's sort of the flip-side of putting all the
> other LIB_OBJS ones into lib/.

The goal here was just to get us rid of tiresome merge conflicts when
two things are added to adjacent part of these lists going forward,
rather than some source-tree reorganization. I didn't search around and
didn't find that 2011-era thread.

I think overall just maintaining the list of the few exceptions is
better than any sort of general mass-move of these files.

Even if we carefully trickle those in at a rate that doesn't conflict
with anything in-flight, the end result will be that e.g.:

    git log -- lib/grep.c

Will stop at that rename commit, similar to builtin/log.c, unless you
specify --follow etc. Just that doesn't make it worth it to me. Likewise
sha1_file.c to sha1-file.c to object-file.c, which is a case I run into
every time I get a "git log" pathspec glob wrong.

Also.

I didn't notice before submitting this but this patch breaks the
vs-build job, because the cmake build in "contrib" is screen-scraping
the Makefile[1].

What's the status of that code? It's rather tiresome to need to patch
two independent and incompatible build systems every time there's some
structural change in the Makefile.

I hadn't looked in any detail at that recipe before, but it the vs-build
job has a hard dependency on GNU make anyway, since we use it for "make
artifacts-tar".

So whatever cmake special-sauce is happening there I don't see why
vs-build couldn't call out "make" for most of the work it's doing, isn't
it just some replacement for what the "vcxproj" target in
config.mak.uname used to do?

1. https://github.com/avar/git/runs/4057171803?check_suite_focus=true

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

[Jeff King]

On Sun, Oct 31, 2021 at 02:00:42PM +0100, Ævar Arnfjörð Bjarmason wrote:

> > Likewise, they could go into a conditional-src/ directory (or some
> > less-horrible name) to keep them distinct without needing an explicit
> > list in the Makefile. That's sort of the flip-side of putting all the
> > other LIB_OBJS ones into lib/.
> 
> The goal here was just to get us rid of tiresome merge conflicts when
> two things are added to adjacent part of these lists going forward,
> rather than some source-tree reorganization. I didn't search around and
> didn't find that 2011-era thread.

Right, sorry, I didn't mean to sidetrack. That was just the only
discussion I could find on the topic. I agree that it's worth
considering the two topics (moving files around vs Makefile changes)
separately.

> Even if we carefully trickle those in at a rate that doesn't conflict
> with anything in-flight, the end result will be that e.g.:
> 
>     git log -- lib/grep.c
> 
> Will stop at that rename commit, similar to builtin/log.c, unless you
> specify --follow etc. Just that doesn't make it worth it to me. Likewise
> sha1_file.c to sha1-file.c to object-file.c, which is a case I run into
> every time I get a "git log" pathspec glob wrong.

Agreed. One of the arguments back when we started to move around a few
files is that this dog-fooding would encourage us to make --follow mode
better. That hasn't really happened, though.

> I didn't notice before submitting this but this patch breaks the
> vs-build job, because the cmake build in "contrib" is screen-scraping
> the Makefile[1].
> 
> What's the status of that code? It's rather tiresome to need to patch
> two independent and incompatible build systems every time there's some
> structural change in the Makefile.

My opinion when we took in the cmake topic was that it would be OK for
people working on the main Makefile to break cmake. It's an add-on and
the people who care about cmake are the ones who will do the work to
track the Makefile.

But since there's a CI job that will nag you if it fails, that kind of
makes it everybody's problem in practice. That doesn't change my opinion
on how things _should_ work, but I have done small fixups as necessary
to stop the nagging.

> I hadn't looked in any detail at that recipe before, but it the vs-build
> job has a hard dependency on GNU make anyway, since we use it for "make
> artifacts-tar".
> 
> So whatever cmake special-sauce is happening there I don't see why
> vs-build couldn't call out "make" for most of the work it's doing, isn't
> it just some replacement for what the "vcxproj" target in
> config.mak.uname used to do?

The big question for me is whether that really is a hard dependency.
Obviously "make artifacts-tar" is for the CI job, but is the cmake stuff
supposed to work for regular users without relying on having GNU make at
all? I have no clue.

-Peff

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

[Ævar Arnfjörð Bjarmason]

On Wed, Nov 03 2021, Jeff King wrote:

> On Sun, Oct 31, 2021 at 02:00:42PM +0100, Ævar Arnfjörð Bjarmason wrote:
> [...]
>> I didn't notice before submitting this but this patch breaks the
>> vs-build job, because the cmake build in "contrib" is screen-scraping
>> the Makefile[1].
>> 
>> What's the status of that code? It's rather tiresome to need to patch
>> two independent and incompatible build systems every time there's some
>> structural change in the Makefile.
>
> My opinion when we took in the cmake topic was that it would be OK for
> people working on the main Makefile to break cmake. It's an add-on and
> the people who care about cmake are the ones who will do the work to
> track the Makefile.
>
> But since there's a CI job that will nag you if it fails, that kind of
> makes it everybody's problem in practice. That doesn't change my opinion
> on how things _should_ work, but I have done small fixups as necessary
> to stop the nagging.

Yes, that was clearly the intent from reading the original discussion,
but we've crept towards it being an actual hard dependency. I'd also be
fine with some direction that just removed that vs-build/vs-test job to
something optional...

>> I hadn't looked in any detail at that recipe before, but it the vs-build
>> job has a hard dependency on GNU make anyway, since we use it for "make
>> artifacts-tar".
>> 
>> So whatever cmake special-sauce is happening there I don't see why
>> vs-build couldn't call out "make" for most of the work it's doing, isn't
>> it just some replacement for what the "vcxproj" target in
>> config.mak.uname used to do?
>
> The big question for me is whether that really is a hard dependency.
> Obviously "make artifacts-tar" is for the CI job, but is the cmake stuff
> supposed to work for regular users without relying on having GNU make at
> all? I have no clue.

It's a hard dependency for the job, since it tars up its built assets in
the first step, and those are then unpacked and used in subsequent
steps. It's being used to ferry the built binaries between CI phases.

But yes, the intent was clearly to not have a dependency on GNU make,
but as I argue in
<patch-v2-3.3-cd62d8f92d1-20211101T191231Z-avarab@gmail.com> I think
having those developers simply install it is better than forcing us to
maintain two distinct and incompatible build systems.

The selling point was that it was going to be really easy to maintain
them in parallel, i.e. you'd just add a thing to a list here and a list
there, but that assumes that nothing will ever structurally change in
the Makefile.

I think the other X-Y problem being solved there was that cmake has some
better integration for Visual Studio somehow. I.e. it does what the
"vcxproj" target in config.mak.uname does/did.

I think that would be a fine use for cmake, and we can clearly
accomplish that by having our cmake file effectively be a mostly thin
wrapper for logic that lives in the Makefile.

I.e. it would ask the Makefile what's in this list or other, what the
CFLAGS are etc. etc., and feed that into relevant cmake variables.

My patch starts us moving in that direction (but doesn't get anywhere
close to that end-goal). I think if we did that the ~1k line
CMakeLists.txt would be maybe 100-300 lines.

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2555 bytes --]

Hi Peff,

On Wed, 3 Nov 2021, Jeff King wrote:

> On Sun, Oct 31, 2021 at 02:00:42PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
> > I didn't notice before submitting this but this patch breaks the
> > vs-build job, because the cmake build in "contrib" is screen-scraping
> > the Makefile[1].
> >
> > What's the status of that code? It's rather tiresome to need to patch
> > two independent and incompatible build systems every time there's some
> > structural change in the Makefile.
>
> My opinion when we took in the cmake topic was that it would be OK for
> people working on the main Makefile to break cmake. It's an add-on and
> the people who care about cmake are the ones who will do the work to
> track the Makefile.

I do try to have a look at breakages in `seen` when I have the time, but
lately I didn't. That's why you may have felt more of these CMake
headaches.

> But since there's a CI job that will nag you if it fails, that kind of
> makes it everybody's problem in practice. That doesn't change my opinion
> on how things _should_ work, but I have done small fixups as necessary
> to stop the nagging.

One very simple solution is to leave the Makefile alone unless it really,
really needs to be changed. There are costs to refactoring, and quite
honestly, it might be a good thing that something like a failing vs-build
job discourages refactoring for refactoring's sake.

> > I hadn't looked in any detail at that recipe before, but it the
> > vs-build job has a hard dependency on GNU make anyway, since we use it
> > for "make artifacts-tar".
> >
> > So whatever cmake special-sauce is happening there I don't see why
> > vs-build couldn't call out "make" for most of the work it's doing,
> > isn't it just some replacement for what the "vcxproj" target in
> > config.mak.uname used to do?
>
> The big question for me is whether that really is a hard dependency.
> Obviously "make artifacts-tar" is for the CI job, but is the cmake stuff
> supposed to work for regular users without relying on having GNU make at
> all? I have no clue.

The entire point of the CMake configuration is to allow developers on
Windows to use the tools they are used to, to build Git. And believe it or
not, GNU make is not one of those tools! I know. Very hard to believe. :-)

So yeah, the vs-build/vs-test combo tries to pay attention to this
intended scenario, and avoids using GNU make or any other of those Unix
tools that much less ubiquitous than some might want to believe.

Ciao,
Dscho

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

[Ævar Arnfjörð Bjarmason]

On Thu, Nov 04 2021, Johannes Schindelin wrote:

> Hi Peff,
>
> On Wed, 3 Nov 2021, Jeff King wrote:
>
>> On Sun, Oct 31, 2021 at 02:00:42PM +0100, Ævar Arnfjörð Bjarmason wrote:
>>
>> > I didn't notice before submitting this but this patch breaks the
>> > vs-build job, because the cmake build in "contrib" is screen-scraping
>> > the Makefile[1].
>> >
>> > What's the status of that code? It's rather tiresome to need to patch
>> > two independent and incompatible build systems every time there's some
>> > structural change in the Makefile.
>>
>> My opinion when we took in the cmake topic was that it would be OK for
>> people working on the main Makefile to break cmake. It's an add-on and
>> the people who care about cmake are the ones who will do the work to
>> track the Makefile.
>
> I do try to have a look at breakages in `seen` when I have the time, but
> lately I didn't. That's why you may have felt more of these CMake
> headaches.

It's not only things that make it into "seen", as most will test their
topic in GitHub CI before submission in their own repos.

>> But since there's a CI job that will nag you if it fails, that kind of
>> makes it everybody's problem in practice. That doesn't change my opinion
>> on how things _should_ work, but I have done small fixups as necessary
>> to stop the nagging.
>
> One very simple solution is to leave the Makefile alone unless it really,
> really needs to be changed. There are costs to refactoring, and quite
> honestly, it might be a good thing that something like a failing vs-build
> job discourages refactoring for refactoring's sake.

Sure, but that's the case with any critical component we're using. A
question of "is it worth leaving it alone" is distinct from "is it
painful to touch it because you need to implement a fix twice in two
incompatible languages?".

In this case I do think the change is justified. I've personally got a
few local topics that I keep having to (even with rerere) solve
conflicts for due to this list of files, and Junio deals with the same.

Ditto for some of the changes I've made recently to make things
non-.PHONY. That's resulted in major workflow improvements for me,

But in any case, the selling point of the original cmake integration was
not something to the effect of:

    "nobody should have to change this in anything but ever so this
    re-implementation is a one-off"

But rather something like:

    "This re-implementation is a one-off, but any updates to both should
    be trivial."

As someone who's had a couple of recent run-ins with cmake I can tell
you it's really not trivial at all.

So given that the selling point of the original change didn't turn out
as was expected I think it's fair to re-visit whether we'd like to take
this path going forward, or to choose another trade-off.

>> > I hadn't looked in any detail at that recipe before, but it the
>> > vs-build job has a hard dependency on GNU make anyway, since we use it
>> > for "make artifacts-tar".
>> >
>> > So whatever cmake special-sauce is happening there I don't see why
>> > vs-build couldn't call out "make" for most of the work it's doing,
>> > isn't it just some replacement for what the "vcxproj" target in
>> > config.mak.uname used to do?
>>
>> The big question for me is whether that really is a hard dependency.
>> Obviously "make artifacts-tar" is for the CI job, but is the cmake stuff
>> supposed to work for regular users without relying on having GNU make at
>> all? I have no clue.
>
> The entire point of the CMake configuration is to allow developers on
> Windows to use the tools they are used to, to build Git. And believe it or
> not, GNU make is not one of those tools! I know. Very hard to believe. :-)

I believe that, the question is why it isn't a better trade-off to just
ask those users to install that software. Our Windows CI is doing it
on-the-fly, so clearly it's not that hard to do it.

Note that I'm not saying that whatever integration those users get in VS
from the special-cause CMake integration should change. We're only
talking about it invoking "make" under the hood in a way that'll be
invisible to the user.

POSIX "sh" isn't native to Windows either, and that CMake file invokes
shellscripts we ship to e.g. build the generated headers, so this
workflow is clearly something that's OK for an end-user once the one-off
hassle of installing a package is over with.

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

[Philip Oakley]

On 04/11/2021 09:46, Ævar Arnfjörð Bjarmason wrote:
>> he entire point of the CMake configuration is to allow developers on
>> Windows to use the tools they are used to, to build Git. And believe it or
>> not, GNU make is not one of those tools! I know. Very hard to believe. :-)
> I believe that, the question is why it isn't a better trade-off to just
> ask those users to install that software. Our Windows CI is doing it
> on-the-fly, so clearly it's not that hard to do it.
Just to say that, while it is real easy to download and install the
Git-for-Windows SDK (https://gitforwindows.org/#download-sdk), for most
(Windows) users it's a foreign land, with few friends who understand
what things like `gdb` are all about. It's all doable, but the learning
curve can be hard. The CI doesn't need a learning curve ;-)

Being able to fire up a well 'trusted' tool like Visual Studio to
investigate the code does help contributors understand the code.

--
Philip

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

[Junio C Hamano]

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

> In this case I do think the change is justified. I've personally got a
> few local topics that I keep having to (even with rerere) solve
> conflicts for due to this list of files, and Junio deals with the same.
>
> Ditto for some of the changes I've made recently to make things
> non-.PHONY. That's resulted in major workflow improvements for me,

To me, I haven't noticed any workflow improvements for me, so I do
not see how my name got into the sentence.

> But in any case, the selling point of the original cmake integration was
> not something to the effect of:
>
>     "nobody should have to change this in anything but ever so this
>     re-implementation is a one-off"

I agree that this wasn't how it was sold, but ...

> But rather something like:
>
>     "This re-implementation is a one-off, but any updates to both should
>     be trivial."

... I do not think this was how it was sold, either.  As far as I
recall, it was rather: this may double the maintenance burden, but
the reward to help casual Windows builders is large enough that
those who want to add the CMake support are willing to bear their
share of the burden.

> As someone who's had a couple of recent run-ins with cmake I can tell
> you it's really not trivial at all.

Surely.

> So given that the selling point of the original change didn't turn out
> as was expected I think it's fair to re-visit whether we'd like to take
> this path going forward, or to choose another trade-off.

OK.  The rest of the message I am responding to is your revisiting,
I guess.

>> The entire point of the CMake configuration is to allow developers on
>> Windows to use the tools they are used to, to build Git. And believe it or
>> not, GNU make is not one of those tools! I know. Very hard to believe. :-)
>
> I believe that, the question is why it isn't a better trade-off to just
> ask those users to install that software. Our Windows CI is doing it
> on-the-fly, so clearly it's not that hard to do it.
>
> Note that I'm not saying that whatever integration those users get in VS
> from the special-cause CMake integration should change. We're only
> talking about it invoking "make" under the hood in a way that'll be
> invisible to the user.
>
> POSIX "sh" isn't native to Windows either, and that CMake file invokes
> shellscripts we ship to e.g. build the generated headers, so this
> workflow is clearly something that's OK for an end-user once the one-off
> hassle of installing a package is over with.

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

[Ævar Arnfjörð Bjarmason]

This series replaces the hardcoded lists *.o files in the top-level
Makefile with a reliance on globbing, e.g. built-ins are $(wildcard
builtin/*.c now, instead of an exhausitive list of every built-in in
the tree.

This makes future development easier as we won't be running into merge
conflicts with these lists, see 3/3 for some more disucssion.

As noted in the v1 discussion the v1 had changes that broke the
"cmake" integration. In going back and reading the discussions around
it it seems we've ended up with a state for that "contrib" component
that was never the intent, and which we tried to explicitly avoid when
integrating it.

There are more details in the updated commit message, but this v2 gets
it working with passing CI, and in such a way as to reduce future
maintenance burden related to that component.

Ævar Arnfjörð Bjarmason (3):
  Makefile: rename $(SCRIPT_LIB) to $(SCRIPT_LIB_GEN)
  Makefile: add a utility to dump variables
  Makefile: replace most hardcoded object lists with $(wildcard)

 Makefile                            | 510 ++++------------------------
 contrib/buildsystems/CMakeLists.txt |  53 ++-
 2 files changed, 91 insertions(+), 472 deletions(-)

Range-diff against v1:
-:  ----------- > 1:  0b23b8395ec Makefile: rename $(SCRIPT_LIB) to $(SCRIPT_LIB_GEN)
-:  ----------- > 2:  97738b056cf Makefile: add a utility to dump variables
1:  bbacbed5c95 ! 3:  cd62d8f92d1 Makefile: replace most hardcoded object lists with $(wildcard)
    @@ Commit message
         would need to be explicitly listed in the Makefile. I think than small
         trade-off is worth it.
     
    +    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".
    +
         There's a few small "while we're at it" changes here, since I'm
         touching the code in question:
     
    @@ Commit message
          - De-indent an "ifndef" block, we don't usually indent their
            contents.
     
    +    On the CMake changes here:
    +
    +     - When CMake support was introduced in was introduced
    +       061c2240b1b (Introduce CMake support for configuring Git, 2020-06-12)
    +       there was a discussion about the maintenance burden of maintaining the
    +       top-level Makefile in parallel with CMakeLists.txt[1] where reviewers
    +       were assured that doing so would simply be a matter of adding something
    +       to a list in the CMake recipe.
    +
    +       Between change and some recent changes of mine where the "vs-build"
    +       job failed to a divergence between the Makefile and CMakeList.txt I
    +       can confidently say that that doesn't at all match reality. Even
    +       seemingly trivial changes to the Makefile like this one are forcing
    +       us to do a deep-dive into CMake internals to make forward progress
    +       with our main build system.
    +
    +     - The promised "We can add a (continue-on-error) to vs-build job to
    +       make this process less of a hindrance." in [2] never materialized.
    +       Since 4c2c38e800f (ci: modification of main.yml to use cmake for
    +       vs-build job, 2020-06-26) got a hard dependency on CMake as far as
    +       getting the CI to pass goes.
    +
    +     - The "vs-build" CI doesn't actually require that there be no GNU make
    +       usage in the job, as it itself has a hard dependency on running a
    +       "make -n artifacts-tar" command. So as far as any vs-specific special-sauce
    +       goes we don't need a GNU-make free build system for vs-build.
    +
    +     - The stated goal in 061c2240b1b of avoiding a GNU make dependency
    +       for developer because it requires an SDK that "occupies around two
    +       gigabytes" and "three quarters of a gigabyte worth of Git objects"
    +       hardly seems worthwhile trade-off given the above. Disk space is cheap,
    +       developer time required to maintain two parallel build systems isn't.
    +
    +    My attempt to amend/revert 4c2c38e800f to have it use the
    +    pre-4c2c38e800f "make" invocation as a fallback failed, partially
    +    because I don't have a Windows development environment, so any attempt
    +    to change it is a painfully slow round-trip to GitHub CI.
    +
    +    Let's instead have CMake call out to the Makefile asking it what the
    +    definition of various variables lists is, rather than being forced to
    +    maintain those lists in a way that CMake can parse with regexes (which
    +    precludes anything but a giant hardcoded list).
    +
    +    I could familiarize myself enough with CMake to do this in some
    +    CMake-native way, but that would take "just as long as adding it to
    +    the Makefile"[2] (I think that took me <5 minutes, but I'm several
    +    hours into fighting with CMake)
    +
    +    So I consider this both a bugfix to the stated aims of this CMake
    +    integration, and a better way forward for having an alternate build
    +    system. I.e. If someone really does care about a having a
    +    GNU-make-less dependency for the "vs-build" I think this change offers
    +    a much better way forward for that.
    +
    +    Once we invoke the Makefile to spew out e.g. its idea of "LIB_OBJS",
    +    it's going to be trivial to do that via some wrapper script that lives
    +    in "contrib/buildsystems". Such a script would either invoke "make
    +    print-{var,list}-%", or alternatively use an in-tree committed text
    +    file with the last known result of such a "make print-{var,list}-%"
    +    run.
    +
    +    1. https://lore.kernel.org/git/xmqq8sikblv2.fsf@gitster.c.googlers.com
    +    2. https://lore.kernel.org/git/CAKiG+9Xtof8Hj3npsS-M0SnT_dcjtHjP_+avWB4oOHkaMdnSbw@mail.gmail.com/
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Makefile ##
    @@ Makefile: TEST_OBJS =
     +XDIFF_SRC =
     +
     +## Guard against env: objects
    -+ALL_COMPAT_OBJS =
    ++CONDITIONAL_OBJS =
     +CURL_OBJS =
     +LIB_OBJS_DIRS =
     +
    - # Having this variable in your environment would break pipelines because
    - # you cause "cd" to echo its destination to stdout.  It can also take
    - # scripts to unexpected places.  If you like CDPATH, define it for your
    + # Utility to dump whatever variables are defined here
    + print-var-%:
    + 	@echo $($*)
     @@ Makefile: X =
      
      PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
    @@ Makefile: LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!
     -LIB_OBJS += commit-reach.o
     -LIB_OBJS += commit.o
     +# LIB_OBJS: compat/* objects that live at the top-level
    -+ALL_COMPAT_OBJS += unix-socket.o
    -+ALL_COMPAT_OBJS += unix-stream-server.o
    -+ALL_COMPAT_OBJS += sha1dc_git.o
    ++CONDITIONAL_OBJS += unix-socket.o
    ++CONDITIONAL_OBJS += unix-stream-server.o
    ++CONDITIONAL_OBJS += sha1dc_git.o
     +
     +# LIB_OBJS: Mostly glob *.c at the top-level, with some exlusions
     +LIB_OBJS += $(filter-out \
    -+	$(ALL_COMPAT_OBJS) \
    ++	$(CONDITIONAL_OBJS) \
     +	git.o common-main.o $(PROGRAM_OBJS) \
     +	$(FUZZ_OBJS) $(CURL_OBJS),\
     +	$(patsubst %.c,%.o,$(wildcard *.c)))
    @@ Makefile: LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!
     +LIB_OBJS_DIRS += negotiator
     +LIB_OBJS_DIRS += refs
     +LIB_OBJS_DIRS += trace2
    -+LIB_OBJS += $(patsubst %.c,%.o,$(foreach dir,$(LIB_OBJS_DIRS),$(wildcard $(dir)/*.c)))
    ++LIB_OBJS += $(patsubst %.c,%.o,$(wildcard $(addsuffix /*.c,$(LIB_OBJS_DIRS))))
     +
     +# LIB_OBJS: unconditional compat/* objects
      LIB_OBJS += compat/obstack.o
    @@ Makefile: LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!
      
      # THIRD_PARTY_SOURCES is a list of patterns compatible with the
      # $(filter) and $(filter-out) family of functions. They specify source
    +@@ Makefile: endif
    + LIBS = $(filter-out %.o, $(GITLIBS)) $(EXTLIBS)
    + 
    + BASIC_CFLAGS += $(COMPAT_CFLAGS)
    ++LIB_OBJS_NO_COMPAT := $(LIB_OBJS)
    + LIB_OBJS += $(COMPAT_OBJS)
    + 
    + # Quote for C
     @@ Makefile: reconfigure config.mak.autogen: config.status
      .PHONY: reconfigure # This is a convenience target.
      endif
    @@ Makefile: perf: all
      
      t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS)
      	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
    +
    + ## contrib/buildsystems/CMakeLists.txt ##
    +@@ contrib/buildsystems/CMakeLists.txt: project(git
    + #TODO Add pcre support
    + 
    + #macros for parsing the Makefile for sources and scripts
    +-macro(parse_makefile_for_sources list_var regex)
    +-	file(STRINGS ${CMAKE_SOURCE_DIR}/Makefile ${list_var} REGEX "^${regex} \\+=(.*)")
    +-	string(REPLACE "${regex} +=" "" ${list_var} ${${list_var}})
    +-	string(REPLACE "$(COMPAT_OBJS)" "" ${list_var} ${${list_var}}) #remove "$(COMPAT_OBJS)" This is only for libgit.
    +-	string(STRIP ${${list_var}} ${list_var}) #remove trailing/leading whitespaces
    +-	string(REPLACE ".o" ".c;" ${list_var} ${${list_var}}) #change .o to .c, ; is for converting the string into a list
    +-	list(TRANSFORM ${list_var} STRIP) #remove trailing/leading whitespaces for each element in list
    +-	list(REMOVE_ITEM ${list_var} "") #remove empty list elements
    +-endmacro()
    +-
    +-macro(parse_makefile_for_scripts list_var regex lang)
    +-	file(STRINGS ${CMAKE_SOURCE_DIR}/Makefile ${list_var} REGEX "^${regex} \\+=(.*)")
    +-	string(REPLACE "${regex} +=" "" ${list_var} ${${list_var}})
    +-	string(STRIP ${${list_var}} ${list_var}) #remove trailing/leading whitespaces
    +-	string(REPLACE " " ";" ${list_var} ${${list_var}}) #convert string to a list
    +-	if(NOT ${lang}) #exclude for SCRIPT_LIB
    +-		list(TRANSFORM ${list_var} REPLACE "${lang}" "") #do the replacement
    +-	endif()
    +-endmacro()
    +-
    +-macro(parse_makefile_for_executables list_var regex)
    +-	file(STRINGS ${CMAKE_SOURCE_DIR}/Makefile ${list_var} REGEX "^${regex} \\+= git-(.*)")
    +-	string(REPLACE "${regex} +=" "" ${list_var} ${${list_var}})
    ++macro(ask_makefile_for_list list_var var_name)
    ++	execute_process(COMMAND make print-list-${var_name}
    ++			WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
    ++			OUTPUT_VARIABLE ${list_var})
    ++	string(REGEX REPLACE "\\.o\n" ".c\n" ${list_var} ${${list_var}}) #change .o to .c
    + 	string(STRIP ${${list_var}} ${list_var}) #remove trailing/leading whitespaces
    +-	string(REPLACE "git-" "" ${list_var} ${${list_var}}) #strip `git-` prefix
    +-	string(REPLACE "\$X" ";" ${list_var} ${${list_var}}) #strip $X, ; is for converting the string into a list
    ++	## Parse the Makefile print-list-% format
    ++	string(REGEX REPLACE "${var_name} =\n" "" ${list_var} ${${list_var}})
    ++	string(REGEX REPLACE "${var_name} \\+?= ([^\n]+)" "\\1;" ${list_var} ${${list_var}})
    + 	list(TRANSFORM ${list_var} STRIP) #remove trailing/leading whitespaces for each element in list
    + 	list(REMOVE_ITEM ${list_var} "") #remove empty list elements
    + endmacro()
    +@@ contrib/buildsystems/CMakeLists.txt: include_directories(${CMAKE_BINARY_DIR})
    + 
    + #build
    + #libgit
    +-parse_makefile_for_sources(libgit_SOURCES "LIB_OBJS")
    ++ask_makefile_for_list(libgit_SOURCES "LIB_OBJS_NO_COMPAT")
    + 
    + list(TRANSFORM libgit_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
    + list(TRANSFORM compat_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
    + add_library(libgit ${libgit_SOURCES} ${compat_SOURCES})
    + 
    + #libxdiff
    +-parse_makefile_for_sources(libxdiff_SOURCES "XDIFF_OBJS")
    ++ask_makefile_for_list(libxdiff_SOURCES "XDIFF_OBJS")
    + 
    + list(TRANSFORM libxdiff_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
    + add_library(xdiff STATIC ${libxdiff_SOURCES})
    +@@ contrib/buildsystems/CMakeLists.txt: elseif(UNIX)
    + endif()
    + 
    + #git
    +-parse_makefile_for_sources(git_SOURCES "BUILTIN_OBJS")
    ++ask_makefile_for_list(git_SOURCES "BUILTIN_OBJS")
    + 
    + list(TRANSFORM git_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
    + add_executable(git ${CMAKE_SOURCE_DIR}/git.c ${git_SOURCES})
    +@@ contrib/buildsystems/CMakeLists.txt: if(CURL_FOUND)
    + 	endif()
    + endif()
    + 
    +-parse_makefile_for_executables(git_builtin_extra "BUILT_INS")
    ++ask_makefile_for_list(git_builtin_extra "BUILT_INS")
    ++list(TRANSFORM git_builtin_extra REPLACE "^git-(.*)" "\\1") #strip `git-` prefix
    ++list(TRANSFORM git_builtin_extra REPLACE "\.exe$" "") #strip $X
    + 
    + option(SKIP_DASHED_BUILT_INS "Skip hardlinking the dashed versions of the built-ins")
    + 
    +@@ contrib/buildsystems/CMakeLists.txt: set(GITWEBDIR ${FALLBACK_RUNTIME_PREFIX}/share/locale)
    + set(INSTLIBDIR ${FALLBACK_RUNTIME_PREFIX}/share/perl5)
    + 
    + #shell scripts
    +-parse_makefile_for_scripts(git_sh_scripts "SCRIPT_SH" ".sh")
    +-parse_makefile_for_scripts(git_shlib_scripts "SCRIPT_LIB" "")
    ++ask_makefile_for_list(git_sh_scripts "SCRIPT_SH_GEN")
    ++ask_makefile_for_list(git_shlib_scripts "SCRIPT_LIB_GEN")
    + set(git_shell_scripts
    + 	${git_sh_scripts} ${git_shlib_scripts} git-instaweb)
    + 
    +@@ contrib/buildsystems/CMakeLists.txt: foreach(script ${git_shell_scripts})
    + endforeach()
    + 
    + #perl scripts
    +-parse_makefile_for_scripts(git_perl_scripts "SCRIPT_PERL" ".perl")
    ++ask_makefile_for_list(git_perl_scripts "SCRIPT_PERL_GEN")
    + 
    + #create perl header
    + file(STRINGS ${CMAKE_SOURCE_DIR}/perl/header_templates/fixed_prefix.template.pl perl_header )
    +@@ contrib/buildsystems/CMakeLists.txt: add_executable(test-fake-ssh ${CMAKE_SOURCE_DIR}/t/helper/test-fake-ssh.c)
    + target_link_libraries(test-fake-ssh common-main)
    + 
    + #test-tool
    +-parse_makefile_for_sources(test-tool_SOURCES "TEST_BUILTINS_OBJS")
    ++ask_makefile_for_list(test-tool_SOURCES "TEST_BUILTINS_OBJS")
    + 
    +-list(TRANSFORM test-tool_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/t/helper/")
    ++list(TRANSFORM test-tool_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
    + add_executable(test-tool ${CMAKE_SOURCE_DIR}/t/helper/test-tool.c ${test-tool_SOURCES})
    + target_link_libraries(test-tool common-main)
    + 
-- 
2.33.1.1570.g069344fdd45

[PATCH v2 1/3] Makefile: rename $(SCRIPT_LIB) to $(SCRIPT_LIB_GEN)

[Ævar Arnfjörð Bjarmason]

Make the $(SCRIPT_LIB) variable like the rest of its siblings, where
we have an extension-less $(*_GEN) variable, but the main one has the
full file name. This will be used in subsequent commits to emit the
filenames in $(SCRIPT_LIB), $(SCRIPT_PERL) etc.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/Makefile b/Makefile
index 12be39ac497..100658dfa43 100644
--- a/Makefile
+++ b/Makefile
@@ -608,9 +608,9 @@ SCRIPT_SH += git-request-pull.sh
 SCRIPT_SH += git-submodule.sh
 SCRIPT_SH += git-web--browse.sh
 
-SCRIPT_LIB += git-mergetool--lib
-SCRIPT_LIB += git-sh-i18n
-SCRIPT_LIB += git-sh-setup
+SCRIPT_LIB += git-mergetool--lib.sh
+SCRIPT_LIB += git-sh-i18n.sh
+SCRIPT_LIB += git-sh-setup.sh
 
 SCRIPT_PERL += git-add--interactive.perl
 SCRIPT_PERL += git-archimport.perl
@@ -624,6 +624,7 @@ SCRIPT_PYTHON += git-p4.py
 
 # Generated files for scripts
 SCRIPT_SH_GEN = $(patsubst %.sh,%,$(SCRIPT_SH))
+SCRIPT_LIB_GEN = $(patsubst %.sh,%,$(SCRIPT_LIB))
 SCRIPT_PERL_GEN = $(patsubst %.perl,%,$(SCRIPT_PERL))
 SCRIPT_PYTHON_GEN = $(patsubst %.py,%,$(SCRIPT_PYTHON))
 
@@ -2141,7 +2142,7 @@ profile-fast: profile-clean
 	$(MAKE) PROFILE=USE all
 
 
-all:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
+all:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB_GEN) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
 ifneq (,$X)
 	$(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) git$X)), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';)
 endif
@@ -2284,7 +2285,7 @@ $(SCRIPT_SH_GEN) : % : %.sh GIT-SCRIPT-DEFINES
 	chmod +x $@+ && \
 	mv $@+ $@
 
-$(SCRIPT_LIB) : % : %.sh GIT-SCRIPT-DEFINES
+$(SCRIPT_LIB_GEN) : % : %.sh GIT-SCRIPT-DEFINES
 	$(QUIET_GEN)$(cmd_munge_script) && \
 	mv $@+ $@
 
@@ -3013,7 +3014,7 @@ install: all
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
 	$(INSTALL) $(INSTALL_STRIP) $(PROGRAMS) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
 	$(INSTALL) $(SCRIPTS) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
+	$(INSTALL) -m 644 $(SCRIPT_LIB_GEN) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
 	$(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)'
 
@@ -3174,7 +3175,7 @@ ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)
 OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll)
 endif
 
-artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
+artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB_GEN) $(OTHER_PROGRAMS) \
 		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
 		$(MOFILES)
 	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
@@ -3232,7 +3233,7 @@ clean: profile-clean coverage-clean cocciclean
 	$(RM) *.res
 	$(RM) $(OBJECTS)
 	$(RM) $(LIB_FILE) $(XDIFF_LIB)
-	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
+	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB_GEN) $(BUILT_INS) git$X
 	$(RM) $(TEST_PROGRAMS)
 	$(RM) $(FUZZ_PROGRAMS)
 	$(RM) $(SP_OBJ)
-- 
2.33.1.1570.g069344fdd45

[PATCH v2 2/3] Makefile: add a utility to dump variables

[Ævar Arnfjörð Bjarmason]

Add handy "print-var-%" and "print-list-%" targets, these can both be
used for ad-hoc debugging, and to integrate the Makefile into some
other build system which needs to extract information from it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Makefile b/Makefile
index 100658dfa43..4139bcf675c 100644
--- a/Makefile
+++ b/Makefile
@@ -590,6 +590,14 @@ TEST_OBJS =
 TEST_PROGRAMS_NEED_X =
 THIRD_PARTY_SOURCES =
 
+# Utility to dump whatever variables are defined here
+print-var-%:
+	@echo $($*)
+
+print-list-%:
+	@echo $* =
+	@for v in $($*); do echo $* += $$v; done
+
 # Having this variable in your environment would break pipelines because
 # you cause "cd" to echo its destination to stdout.  It can also take
 # scripts to unexpected places.  If you like CDPATH, define it for your
-- 
2.33.1.1570.g069344fdd45

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

[Ævar Arnfjörð Bjarmason]

Remove the hardcoded lists of objects in favor of using
$(wildcard). This means that every time a built-in, test tool etc. is
added we won't need to patch the top-level Makefile, except for the
few remaining cases where the asset in question would make it onto one
of our list of exceptions.

Ever since 81b50f3ce40 (Move 'builtin-*' into a 'builtin/'
subdirectory, 2010-02-22) this has been relatively easy to do (and
even before that we could glob builtin-*.c). This pattern of
exhaustively enumerating files was then carried forward for
e.g. TEST_BUILTINS_OBJS in efd71f8913a (t/helper: add an empty
test-tool program, 2018-03-24).

One reason not to do this is that now a new *.c file at the top-level
will be immediately picked up, so if a new *.c file is being worked on
"make" will error if it doesn't compile, whereas before that file
would need to be explicitly listed in the Makefile. I think than small
trade-off is worth it.

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

There's a few small "while we're at it" changes here, since I'm
touching the code in question:

 - Start splitting up the the "Guard against the environment" section
   at the top, but don't move anything that exists there out to avoid
   merge conflicts

 - The $(TEST_BUILTINS_OBJS) variable was needlessly complex, because
   it didn't have the full paths we'd pathsubst it back & forth.

 - Introduce *_SRC in addition to *_OBJ for the variable I'm
   touching. Eventually we'll want to do this for all the *.o files,
   i.e. make the *.c list a source of truth for *.o, which means we can
   e.g. use that exhaustive list for "make TAGS".

 - Add a missing "curl-objs" target. See 029bac01a87 (Makefile: add
   {program,xdiff,test,git,fuzz}-objs & objects targets, 2021-02-23)
   for the commit that added the rest.

 - De-indent an "ifndef" block, we don't usually indent their
   contents.

On the CMake changes here:

 - When CMake support was introduced in was introduced
   061c2240b1b (Introduce CMake support for configuring Git, 2020-06-12)
   there was a discussion about the maintenance burden of maintaining the
   top-level Makefile in parallel with CMakeLists.txt[1] where reviewers
   were assured that doing so would simply be a matter of adding something
   to a list in the CMake recipe.

   Between change and some recent changes of mine where the "vs-build"
   job failed to a divergence between the Makefile and CMakeList.txt I
   can confidently say that that doesn't at all match reality. Even
   seemingly trivial changes to the Makefile like this one are forcing
   us to do a deep-dive into CMake internals to make forward progress
   with our main build system.

 - The promised "We can add a (continue-on-error) to vs-build job to
   make this process less of a hindrance." in [2] never materialized.
   Since 4c2c38e800f (ci: modification of main.yml to use cmake for
   vs-build job, 2020-06-26) got a hard dependency on CMake as far as
   getting the CI to pass goes.

 - The "vs-build" CI doesn't actually require that there be no GNU make
   usage in the job, as it itself has a hard dependency on running a
   "make -n artifacts-tar" command. So as far as any vs-specific special-sauce
   goes we don't need a GNU-make free build system for vs-build.

 - The stated goal in 061c2240b1b of avoiding a GNU make dependency
   for developer because it requires an SDK that "occupies around two
   gigabytes" and "three quarters of a gigabyte worth of Git objects"
   hardly seems worthwhile trade-off given the above. Disk space is cheap,
   developer time required to maintain two parallel build systems isn't.

My attempt to amend/revert 4c2c38e800f to have it use the
pre-4c2c38e800f "make" invocation as a fallback failed, partially
because I don't have a Windows development environment, so any attempt
to change it is a painfully slow round-trip to GitHub CI.

Let's instead have CMake call out to the Makefile asking it what the
definition of various variables lists is, rather than being forced to
maintain those lists in a way that CMake can parse with regexes (which
precludes anything but a giant hardcoded list).

I could familiarize myself enough with CMake to do this in some
CMake-native way, but that would take "just as long as adding it to
the Makefile"[2] (I think that took me <5 minutes, but I'm several
hours into fighting with CMake)

So I consider this both a bugfix to the stated aims of this CMake
integration, and a better way forward for having an alternate build
system. I.e. If someone really does care about a having a
GNU-make-less dependency for the "vs-build" I think this change offers
a much better way forward for that.

Once we invoke the Makefile to spew out e.g. its idea of "LIB_OBJS",
it's going to be trivial to do that via some wrapper script that lives
in "contrib/buildsystems". Such a script would either invoke "make
print-{var,list}-%", or alternatively use an in-tree committed text
file with the last known result of such a "make print-{var,list}-%"
run.

1. https://lore.kernel.org/git/xmqq8sikblv2.fsf@gitster.c.googlers.com
2. https://lore.kernel.org/git/CAKiG+9Xtof8Hj3npsS-M0SnT_dcjtHjP_+avWB4oOHkaMdnSbw@mail.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile                            | 485 ++++------------------------
 contrib/buildsystems/CMakeLists.txt |  53 ++-
 2 files changed, 74 insertions(+), 464 deletions(-)

diff --git a/Makefile b/Makefile
index 4139bcf675c..5d78ab6860a 100644
--- a/Makefile
+++ b/Makefile
@@ -590,6 +590,19 @@ TEST_OBJS =
 TEST_PROGRAMS_NEED_X =
 THIRD_PARTY_SOURCES =
 
+## Guard against env: programs
+TEST_PROGRAMS =
+
+## Guard against env: sources
+CURL_SRC =
+TEST_PROGRAMS_NEED_X_SRC =
+XDIFF_SRC =
+
+## Guard against env: objects
+CONDITIONAL_OBJS =
+CURL_OBJS =
+LIB_OBJS_DIRS =
+
 # Utility to dump whatever variables are defined here
 print-var-%:
 	@echo $($*)
@@ -697,87 +710,23 @@ X =
 
 PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
 
-TEST_BUILTINS_OBJS += test-advise.o
-TEST_BUILTINS_OBJS += test-bitmap.o
-TEST_BUILTINS_OBJS += test-bloom.o
-TEST_BUILTINS_OBJS += test-chmtime.o
-TEST_BUILTINS_OBJS += test-config.o
-TEST_BUILTINS_OBJS += test-crontab.o
-TEST_BUILTINS_OBJS += test-ctype.o
-TEST_BUILTINS_OBJS += test-date.o
-TEST_BUILTINS_OBJS += test-delta.o
-TEST_BUILTINS_OBJS += test-dir-iterator.o
-TEST_BUILTINS_OBJS += test-drop-caches.o
-TEST_BUILTINS_OBJS += test-dump-cache-tree.o
-TEST_BUILTINS_OBJS += test-dump-fsmonitor.o
-TEST_BUILTINS_OBJS += test-dump-split-index.o
-TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
-TEST_BUILTINS_OBJS += test-example-decorate.o
-TEST_BUILTINS_OBJS += test-fast-rebase.o
-TEST_BUILTINS_OBJS += test-genrandom.o
-TEST_BUILTINS_OBJS += test-genzeros.o
-TEST_BUILTINS_OBJS += test-getcwd.o
-TEST_BUILTINS_OBJS += test-hash-speed.o
-TEST_BUILTINS_OBJS += test-hash.o
-TEST_BUILTINS_OBJS += test-hashmap.o
-TEST_BUILTINS_OBJS += test-index-version.o
-TEST_BUILTINS_OBJS += test-json-writer.o
-TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o
-TEST_BUILTINS_OBJS += test-match-trees.o
-TEST_BUILTINS_OBJS += test-mergesort.o
-TEST_BUILTINS_OBJS += test-mktemp.o
-TEST_BUILTINS_OBJS += test-oid-array.o
-TEST_BUILTINS_OBJS += test-oidmap.o
-TEST_BUILTINS_OBJS += test-oidtree.o
-TEST_BUILTINS_OBJS += test-online-cpus.o
-TEST_BUILTINS_OBJS += test-parse-options.o
-TEST_BUILTINS_OBJS += test-parse-pathspec-file.o
-TEST_BUILTINS_OBJS += test-partial-clone.o
-TEST_BUILTINS_OBJS += test-path-utils.o
-TEST_BUILTINS_OBJS += test-pcre2-config.o
-TEST_BUILTINS_OBJS += test-pkt-line.o
-TEST_BUILTINS_OBJS += test-prio-queue.o
-TEST_BUILTINS_OBJS += test-proc-receive.o
-TEST_BUILTINS_OBJS += test-progress.o
-TEST_BUILTINS_OBJS += test-reach.o
-TEST_BUILTINS_OBJS += test-read-cache.o
-TEST_BUILTINS_OBJS += test-read-graph.o
-TEST_BUILTINS_OBJS += test-read-midx.o
-TEST_BUILTINS_OBJS += test-ref-store.o
-TEST_BUILTINS_OBJS += test-regex.o
-TEST_BUILTINS_OBJS += test-repository.o
-TEST_BUILTINS_OBJS += test-revision-walking.o
-TEST_BUILTINS_OBJS += test-run-command.o
-TEST_BUILTINS_OBJS += test-scrap-cache-tree.o
-TEST_BUILTINS_OBJS += test-serve-v2.o
-TEST_BUILTINS_OBJS += test-sha1.o
-TEST_BUILTINS_OBJS += test-sha256.o
-TEST_BUILTINS_OBJS += test-sigchain.o
-TEST_BUILTINS_OBJS += test-simple-ipc.o
-TEST_BUILTINS_OBJS += test-strcmp-offset.o
-TEST_BUILTINS_OBJS += test-string-list.o
-TEST_BUILTINS_OBJS += test-submodule-config.o
-TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o
-TEST_BUILTINS_OBJS += test-subprocess.o
-TEST_BUILTINS_OBJS += test-trace2.o
-TEST_BUILTINS_OBJS += test-urlmatch-normalization.o
-TEST_BUILTINS_OBJS += test-userdiff.o
-TEST_BUILTINS_OBJS += test-wildmatch.o
-TEST_BUILTINS_OBJS += test-windows-named-pipe.o
-TEST_BUILTINS_OBJS += test-write-cache.o
-TEST_BUILTINS_OBJS += test-xml-encode.o
-
 # Do not add more tests here unless they have extra dependencies. Add
 # them in TEST_BUILTINS_OBJS above.
 TEST_PROGRAMS_NEED_X += test-fake-ssh
 TEST_PROGRAMS_NEED_X += test-tool
 
-TEST_PROGRAMS = $(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X))
+TEST_PROGRAMS_NEED_X_SRC += $(TEST_PROGRAMS_NEED_X:%=t/helper/%.c)
+TEST_PROGRAMS += $(TEST_PROGRAMS_NEED_X_SRC:%.c=%$X)
+TEST_BUILTINS_SRC += $(filter-out $(TEST_PROGRAMS_NEED_X_SRC),$(wildcard t/helper/*.c))
+TEST_BUILTINS_OBJS += $(TEST_BUILTINS_SRC:%.c=%.o)
 
-# 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.
+# List built-in command $C whose implementation cmd_$C() is in
+# builtin/$C.o
+BUILTIN_OBJS = $(patsubst %.c,%.o,$(wildcard builtin/*.c))
 BUILT_INS += $(patsubst builtin/%.o,git-%$X,$(BUILTIN_OBJS))
 
+# 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.
 BUILT_INS += git-cherry$X
 BUILT_INS += git-cherry-pick$X
 BUILT_INS += git-format-patch$X
@@ -837,355 +786,28 @@ LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentat
 	-name Documentation -prune -o \
 	-name '*.h' -print)))
 
-LIB_OBJS += abspath.o
-LIB_OBJS += add-interactive.o
-LIB_OBJS += add-patch.o
-LIB_OBJS += advice.o
-LIB_OBJS += alias.o
-LIB_OBJS += alloc.o
-LIB_OBJS += apply.o
-LIB_OBJS += archive-tar.o
-LIB_OBJS += archive-zip.o
-LIB_OBJS += archive.o
-LIB_OBJS += attr.o
-LIB_OBJS += base85.o
-LIB_OBJS += bisect.o
-LIB_OBJS += blame.o
-LIB_OBJS += blob.o
-LIB_OBJS += bloom.o
-LIB_OBJS += branch.o
-LIB_OBJS += bulk-checkin.o
-LIB_OBJS += bundle.o
-LIB_OBJS += cache-tree.o
-LIB_OBJS += cbtree.o
-LIB_OBJS += chdir-notify.o
-LIB_OBJS += checkout.o
-LIB_OBJS += chunk-format.o
-LIB_OBJS += color.o
-LIB_OBJS += column.o
-LIB_OBJS += combine-diff.o
-LIB_OBJS += commit-graph.o
-LIB_OBJS += commit-reach.o
-LIB_OBJS += commit.o
+# LIB_OBJS: compat/* objects that live at the top-level
+CONDITIONAL_OBJS += unix-socket.o
+CONDITIONAL_OBJS += unix-stream-server.o
+CONDITIONAL_OBJS += sha1dc_git.o
+
+# LIB_OBJS: Mostly glob *.c at the top-level, with some exlusions
+LIB_OBJS += $(filter-out \
+	$(CONDITIONAL_OBJS) \
+	git.o common-main.o $(PROGRAM_OBJS) \
+	$(FUZZ_OBJS) $(CURL_OBJS),\
+	$(patsubst %.c,%.o,$(wildcard *.c)))
+
+# LIB_OBJS: Directories that contain only LIB_OBJS
+LIB_OBJS_DIRS += ewah
+LIB_OBJS_DIRS += negotiator
+LIB_OBJS_DIRS += refs
+LIB_OBJS_DIRS += trace2
+LIB_OBJS += $(patsubst %.c,%.o,$(wildcard $(addsuffix /*.c,$(LIB_OBJS_DIRS))))
+
+# LIB_OBJS: unconditional compat/* objects
 LIB_OBJS += compat/obstack.o
 LIB_OBJS += compat/terminal.o
-LIB_OBJS += config.o
-LIB_OBJS += connect.o
-LIB_OBJS += connected.o
-LIB_OBJS += convert.o
-LIB_OBJS += copy.o
-LIB_OBJS += credential.o
-LIB_OBJS += csum-file.o
-LIB_OBJS += ctype.o
-LIB_OBJS += date.o
-LIB_OBJS += decorate.o
-LIB_OBJS += delta-islands.o
-LIB_OBJS += diff-delta.o
-LIB_OBJS += diff-merges.o
-LIB_OBJS += diff-lib.o
-LIB_OBJS += diff-no-index.o
-LIB_OBJS += diff.o
-LIB_OBJS += diffcore-break.o
-LIB_OBJS += diffcore-delta.o
-LIB_OBJS += diffcore-order.o
-LIB_OBJS += diffcore-pickaxe.o
-LIB_OBJS += diffcore-rename.o
-LIB_OBJS += diffcore-rotate.o
-LIB_OBJS += dir-iterator.o
-LIB_OBJS += dir.o
-LIB_OBJS += editor.o
-LIB_OBJS += entry.o
-LIB_OBJS += environment.o
-LIB_OBJS += ewah/bitmap.o
-LIB_OBJS += ewah/ewah_bitmap.o
-LIB_OBJS += ewah/ewah_io.o
-LIB_OBJS += ewah/ewah_rlw.o
-LIB_OBJS += exec-cmd.o
-LIB_OBJS += fetch-negotiator.o
-LIB_OBJS += fetch-pack.o
-LIB_OBJS += fmt-merge-msg.o
-LIB_OBJS += fsck.o
-LIB_OBJS += fsmonitor.o
-LIB_OBJS += gettext.o
-LIB_OBJS += gpg-interface.o
-LIB_OBJS += graph.o
-LIB_OBJS += grep.o
-LIB_OBJS += hash-lookup.o
-LIB_OBJS += hashmap.o
-LIB_OBJS += help.o
-LIB_OBJS += hex.o
-LIB_OBJS += hook.o
-LIB_OBJS += ident.o
-LIB_OBJS += json-writer.o
-LIB_OBJS += kwset.o
-LIB_OBJS += levenshtein.o
-LIB_OBJS += line-log.o
-LIB_OBJS += line-range.o
-LIB_OBJS += linear-assignment.o
-LIB_OBJS += list-objects-filter-options.o
-LIB_OBJS += list-objects-filter.o
-LIB_OBJS += list-objects.o
-LIB_OBJS += ll-merge.o
-LIB_OBJS += lockfile.o
-LIB_OBJS += log-tree.o
-LIB_OBJS += ls-refs.o
-LIB_OBJS += mailinfo.o
-LIB_OBJS += mailmap.o
-LIB_OBJS += match-trees.o
-LIB_OBJS += mem-pool.o
-LIB_OBJS += merge-blobs.o
-LIB_OBJS += merge-ort.o
-LIB_OBJS += merge-ort-wrappers.o
-LIB_OBJS += merge-recursive.o
-LIB_OBJS += merge.o
-LIB_OBJS += mergesort.o
-LIB_OBJS += midx.o
-LIB_OBJS += name-hash.o
-LIB_OBJS += negotiator/default.o
-LIB_OBJS += negotiator/noop.o
-LIB_OBJS += negotiator/skipping.o
-LIB_OBJS += notes-cache.o
-LIB_OBJS += notes-merge.o
-LIB_OBJS += notes-utils.o
-LIB_OBJS += notes.o
-LIB_OBJS += object-file.o
-LIB_OBJS += object-name.o
-LIB_OBJS += object.o
-LIB_OBJS += oid-array.o
-LIB_OBJS += oidmap.o
-LIB_OBJS += oidset.o
-LIB_OBJS += oidtree.o
-LIB_OBJS += pack-bitmap-write.o
-LIB_OBJS += pack-bitmap.o
-LIB_OBJS += pack-check.o
-LIB_OBJS += pack-objects.o
-LIB_OBJS += pack-revindex.o
-LIB_OBJS += pack-write.o
-LIB_OBJS += packfile.o
-LIB_OBJS += pager.o
-LIB_OBJS += parallel-checkout.o
-LIB_OBJS += parse-options-cb.o
-LIB_OBJS += parse-options.o
-LIB_OBJS += patch-delta.o
-LIB_OBJS += patch-ids.o
-LIB_OBJS += path.o
-LIB_OBJS += pathspec.o
-LIB_OBJS += pkt-line.o
-LIB_OBJS += preload-index.o
-LIB_OBJS += pretty.o
-LIB_OBJS += prio-queue.o
-LIB_OBJS += progress.o
-LIB_OBJS += promisor-remote.o
-LIB_OBJS += prompt.o
-LIB_OBJS += protocol.o
-LIB_OBJS += protocol-caps.o
-LIB_OBJS += prune-packed.o
-LIB_OBJS += quote.o
-LIB_OBJS += range-diff.o
-LIB_OBJS += reachable.o
-LIB_OBJS += read-cache.o
-LIB_OBJS += rebase-interactive.o
-LIB_OBJS += rebase.o
-LIB_OBJS += ref-filter.o
-LIB_OBJS += reflog-walk.o
-LIB_OBJS += refs.o
-LIB_OBJS += refs/debug.o
-LIB_OBJS += refs/files-backend.o
-LIB_OBJS += refs/iterator.o
-LIB_OBJS += refs/packed-backend.o
-LIB_OBJS += refs/ref-cache.o
-LIB_OBJS += refspec.o
-LIB_OBJS += remote.o
-LIB_OBJS += replace-object.o
-LIB_OBJS += repo-settings.o
-LIB_OBJS += repository.o
-LIB_OBJS += rerere.o
-LIB_OBJS += reset.o
-LIB_OBJS += resolve-undo.o
-LIB_OBJS += revision.o
-LIB_OBJS += run-command.o
-LIB_OBJS += send-pack.o
-LIB_OBJS += sequencer.o
-LIB_OBJS += serve.o
-LIB_OBJS += server-info.o
-LIB_OBJS += setup.o
-LIB_OBJS += shallow.o
-LIB_OBJS += sideband.o
-LIB_OBJS += sigchain.o
-LIB_OBJS += sparse-index.o
-LIB_OBJS += split-index.o
-LIB_OBJS += stable-qsort.o
-LIB_OBJS += strbuf.o
-LIB_OBJS += streaming.o
-LIB_OBJS += string-list.o
-LIB_OBJS += strmap.o
-LIB_OBJS += strvec.o
-LIB_OBJS += sub-process.o
-LIB_OBJS += submodule-config.o
-LIB_OBJS += submodule.o
-LIB_OBJS += symlinks.o
-LIB_OBJS += tag.o
-LIB_OBJS += tempfile.o
-LIB_OBJS += thread-utils.o
-LIB_OBJS += tmp-objdir.o
-LIB_OBJS += trace.o
-LIB_OBJS += trace2.o
-LIB_OBJS += trace2/tr2_cfg.o
-LIB_OBJS += trace2/tr2_cmd_name.o
-LIB_OBJS += trace2/tr2_dst.o
-LIB_OBJS += trace2/tr2_sid.o
-LIB_OBJS += trace2/tr2_sysenv.o
-LIB_OBJS += trace2/tr2_tbuf.o
-LIB_OBJS += trace2/tr2_tgt_event.o
-LIB_OBJS += trace2/tr2_tgt_normal.o
-LIB_OBJS += trace2/tr2_tgt_perf.o
-LIB_OBJS += trace2/tr2_tls.o
-LIB_OBJS += trailer.o
-LIB_OBJS += transport-helper.o
-LIB_OBJS += transport.o
-LIB_OBJS += tree-diff.o
-LIB_OBJS += tree-walk.o
-LIB_OBJS += tree.o
-LIB_OBJS += unpack-trees.o
-LIB_OBJS += upload-pack.o
-LIB_OBJS += url.o
-LIB_OBJS += urlmatch.o
-LIB_OBJS += usage.o
-LIB_OBJS += userdiff.o
-LIB_OBJS += utf8.o
-LIB_OBJS += varint.o
-LIB_OBJS += version.o
-LIB_OBJS += versioncmp.o
-LIB_OBJS += walker.o
-LIB_OBJS += wildmatch.o
-LIB_OBJS += worktree.o
-LIB_OBJS += wrapper.o
-LIB_OBJS += write-or-die.o
-LIB_OBJS += ws.o
-LIB_OBJS += wt-status.o
-LIB_OBJS += xdiff-interface.o
-LIB_OBJS += zlib.o
-
-BUILTIN_OBJS += builtin/add.o
-BUILTIN_OBJS += builtin/am.o
-BUILTIN_OBJS += builtin/annotate.o
-BUILTIN_OBJS += builtin/apply.o
-BUILTIN_OBJS += builtin/archive.o
-BUILTIN_OBJS += builtin/bisect--helper.o
-BUILTIN_OBJS += builtin/blame.o
-BUILTIN_OBJS += builtin/branch.o
-BUILTIN_OBJS += builtin/bugreport.o
-BUILTIN_OBJS += builtin/bundle.o
-BUILTIN_OBJS += builtin/cat-file.o
-BUILTIN_OBJS += builtin/check-attr.o
-BUILTIN_OBJS += builtin/check-ignore.o
-BUILTIN_OBJS += builtin/check-mailmap.o
-BUILTIN_OBJS += builtin/check-ref-format.o
-BUILTIN_OBJS += builtin/checkout--worker.o
-BUILTIN_OBJS += builtin/checkout-index.o
-BUILTIN_OBJS += builtin/checkout.o
-BUILTIN_OBJS += builtin/clean.o
-BUILTIN_OBJS += builtin/clone.o
-BUILTIN_OBJS += builtin/column.o
-BUILTIN_OBJS += builtin/commit-graph.o
-BUILTIN_OBJS += builtin/commit-tree.o
-BUILTIN_OBJS += builtin/commit.o
-BUILTIN_OBJS += builtin/config.o
-BUILTIN_OBJS += builtin/count-objects.o
-BUILTIN_OBJS += builtin/credential-cache--daemon.o
-BUILTIN_OBJS += builtin/credential-cache.o
-BUILTIN_OBJS += builtin/credential-store.o
-BUILTIN_OBJS += builtin/credential.o
-BUILTIN_OBJS += builtin/describe.o
-BUILTIN_OBJS += builtin/diff-files.o
-BUILTIN_OBJS += builtin/diff-index.o
-BUILTIN_OBJS += builtin/diff-tree.o
-BUILTIN_OBJS += builtin/diff.o
-BUILTIN_OBJS += builtin/difftool.o
-BUILTIN_OBJS += builtin/env--helper.o
-BUILTIN_OBJS += builtin/fast-export.o
-BUILTIN_OBJS += builtin/fast-import.o
-BUILTIN_OBJS += builtin/fetch-pack.o
-BUILTIN_OBJS += builtin/fetch.o
-BUILTIN_OBJS += builtin/fmt-merge-msg.o
-BUILTIN_OBJS += builtin/for-each-ref.o
-BUILTIN_OBJS += builtin/for-each-repo.o
-BUILTIN_OBJS += builtin/fsck.o
-BUILTIN_OBJS += builtin/gc.o
-BUILTIN_OBJS += builtin/get-tar-commit-id.o
-BUILTIN_OBJS += builtin/grep.o
-BUILTIN_OBJS += builtin/hash-object.o
-BUILTIN_OBJS += builtin/help.o
-BUILTIN_OBJS += builtin/index-pack.o
-BUILTIN_OBJS += builtin/init-db.o
-BUILTIN_OBJS += builtin/interpret-trailers.o
-BUILTIN_OBJS += builtin/log.o
-BUILTIN_OBJS += builtin/ls-files.o
-BUILTIN_OBJS += builtin/ls-remote.o
-BUILTIN_OBJS += builtin/ls-tree.o
-BUILTIN_OBJS += builtin/mailinfo.o
-BUILTIN_OBJS += builtin/mailsplit.o
-BUILTIN_OBJS += builtin/merge-base.o
-BUILTIN_OBJS += builtin/merge-file.o
-BUILTIN_OBJS += builtin/merge-index.o
-BUILTIN_OBJS += builtin/merge-ours.o
-BUILTIN_OBJS += builtin/merge-recursive.o
-BUILTIN_OBJS += builtin/merge-tree.o
-BUILTIN_OBJS += builtin/merge.o
-BUILTIN_OBJS += builtin/mktag.o
-BUILTIN_OBJS += builtin/mktree.o
-BUILTIN_OBJS += builtin/multi-pack-index.o
-BUILTIN_OBJS += builtin/mv.o
-BUILTIN_OBJS += builtin/name-rev.o
-BUILTIN_OBJS += builtin/notes.o
-BUILTIN_OBJS += builtin/pack-objects.o
-BUILTIN_OBJS += builtin/pack-redundant.o
-BUILTIN_OBJS += builtin/pack-refs.o
-BUILTIN_OBJS += builtin/patch-id.o
-BUILTIN_OBJS += builtin/prune-packed.o
-BUILTIN_OBJS += builtin/prune.o
-BUILTIN_OBJS += builtin/pull.o
-BUILTIN_OBJS += builtin/push.o
-BUILTIN_OBJS += builtin/range-diff.o
-BUILTIN_OBJS += builtin/read-tree.o
-BUILTIN_OBJS += builtin/rebase.o
-BUILTIN_OBJS += builtin/receive-pack.o
-BUILTIN_OBJS += builtin/reflog.o
-BUILTIN_OBJS += builtin/remote-ext.o
-BUILTIN_OBJS += builtin/remote-fd.o
-BUILTIN_OBJS += builtin/remote.o
-BUILTIN_OBJS += builtin/repack.o
-BUILTIN_OBJS += builtin/replace.o
-BUILTIN_OBJS += builtin/rerere.o
-BUILTIN_OBJS += builtin/reset.o
-BUILTIN_OBJS += builtin/rev-list.o
-BUILTIN_OBJS += builtin/rev-parse.o
-BUILTIN_OBJS += builtin/revert.o
-BUILTIN_OBJS += builtin/rm.o
-BUILTIN_OBJS += builtin/send-pack.o
-BUILTIN_OBJS += builtin/shortlog.o
-BUILTIN_OBJS += builtin/show-branch.o
-BUILTIN_OBJS += builtin/show-index.o
-BUILTIN_OBJS += builtin/show-ref.o
-BUILTIN_OBJS += builtin/sparse-checkout.o
-BUILTIN_OBJS += builtin/stash.o
-BUILTIN_OBJS += builtin/stripspace.o
-BUILTIN_OBJS += builtin/submodule--helper.o
-BUILTIN_OBJS += builtin/symbolic-ref.o
-BUILTIN_OBJS += builtin/tag.o
-BUILTIN_OBJS += builtin/unpack-file.o
-BUILTIN_OBJS += builtin/unpack-objects.o
-BUILTIN_OBJS += builtin/update-index.o
-BUILTIN_OBJS += builtin/update-ref.o
-BUILTIN_OBJS += builtin/update-server-info.o
-BUILTIN_OBJS += builtin/upload-archive.o
-BUILTIN_OBJS += builtin/upload-pack.o
-BUILTIN_OBJS += builtin/var.o
-BUILTIN_OBJS += builtin/verify-commit.o
-BUILTIN_OBJS += builtin/verify-pack.o
-BUILTIN_OBJS += builtin/verify-tag.o
-BUILTIN_OBJS += builtin/worktree.o
-BUILTIN_OBJS += builtin/write-tree.o
 
 # THIRD_PARTY_SOURCES is a list of patterns compatible with the
 # $(filter) and $(filter-out) family of functions. They specify source
@@ -2076,6 +1698,7 @@ endif
 LIBS = $(filter-out %.o, $(GITLIBS)) $(EXTLIBS)
 
 BASIC_CFLAGS += $(COMPAT_CFLAGS)
+LIB_OBJS_NO_COMPAT := $(LIB_OBJS)
 LIB_OBJS += $(COMPAT_OBJS)
 
 # Quote for C
@@ -2436,17 +2059,12 @@ reconfigure config.mak.autogen: config.status
 .PHONY: reconfigure # This is a convenience target.
 endif
 
-XDIFF_OBJS += xdiff/xdiffi.o
-XDIFF_OBJS += xdiff/xemit.o
-XDIFF_OBJS += xdiff/xhistogram.o
-XDIFF_OBJS += xdiff/xmerge.o
-XDIFF_OBJS += xdiff/xpatience.o
-XDIFF_OBJS += xdiff/xprepare.o
-XDIFF_OBJS += xdiff/xutils.o
+XDIFF_SRC += $(wildcard xdiff/*.c)
+XDIFF_OBJS += $(XDIFF_SRC:.c=.o)
 .PHONY: xdiff-objs
 xdiff-objs: $(XDIFF_OBJS)
 
-TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
+TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(TEST_BUILTINS_OBJS)
 .PHONY: test-objs
 test-objs: $(TEST_OBJS)
 
@@ -2457,13 +2075,20 @@ GIT_OBJS += git.o
 .PHONY: git-objs
 git-objs: $(GIT_OBJS)
 
+CURL_SRC += http.c
+CURL_SRC += http-walker.c
+CURL_SRC += remote-curl.c
+CURL_OBJS += $(CURL_SRC:.c=.o)
+.PHONY: curl-objs
+curl-objs: $(CURL_OBJS)
+
 OBJECTS += $(GIT_OBJS)
 OBJECTS += $(PROGRAM_OBJS)
 OBJECTS += $(TEST_OBJS)
 OBJECTS += $(XDIFF_OBJS)
 OBJECTS += $(FUZZ_OBJS)
 ifndef NO_CURL
-	OBJECTS += http.o http-walker.o remote-curl.o
+OBJECTS += $(CURL_OBJS)
 endif
 .PHONY: objects
 objects: $(OBJECTS)
@@ -2900,7 +2525,7 @@ perf: all
 
 .PRECIOUS: $(TEST_OBJS)
 
-t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
+t/helper/test-tool$X: $(TEST_BUILTINS_OBJS)
 
 t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index fd1399c440f..d57c9203b66 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -111,32 +111,15 @@ project(git
 #TODO Add pcre support
 
 #macros for parsing the Makefile for sources and scripts
-macro(parse_makefile_for_sources list_var regex)
-	file(STRINGS ${CMAKE_SOURCE_DIR}/Makefile ${list_var} REGEX "^${regex} \\+=(.*)")
-	string(REPLACE "${regex} +=" "" ${list_var} ${${list_var}})
-	string(REPLACE "$(COMPAT_OBJS)" "" ${list_var} ${${list_var}}) #remove "$(COMPAT_OBJS)" This is only for libgit.
-	string(STRIP ${${list_var}} ${list_var}) #remove trailing/leading whitespaces
-	string(REPLACE ".o" ".c;" ${list_var} ${${list_var}}) #change .o to .c, ; is for converting the string into a list
-	list(TRANSFORM ${list_var} STRIP) #remove trailing/leading whitespaces for each element in list
-	list(REMOVE_ITEM ${list_var} "") #remove empty list elements
-endmacro()
-
-macro(parse_makefile_for_scripts list_var regex lang)
-	file(STRINGS ${CMAKE_SOURCE_DIR}/Makefile ${list_var} REGEX "^${regex} \\+=(.*)")
-	string(REPLACE "${regex} +=" "" ${list_var} ${${list_var}})
-	string(STRIP ${${list_var}} ${list_var}) #remove trailing/leading whitespaces
-	string(REPLACE " " ";" ${list_var} ${${list_var}}) #convert string to a list
-	if(NOT ${lang}) #exclude for SCRIPT_LIB
-		list(TRANSFORM ${list_var} REPLACE "${lang}" "") #do the replacement
-	endif()
-endmacro()
-
-macro(parse_makefile_for_executables list_var regex)
-	file(STRINGS ${CMAKE_SOURCE_DIR}/Makefile ${list_var} REGEX "^${regex} \\+= git-(.*)")
-	string(REPLACE "${regex} +=" "" ${list_var} ${${list_var}})
+macro(ask_makefile_for_list list_var var_name)
+	execute_process(COMMAND make print-list-${var_name}
+			WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
+			OUTPUT_VARIABLE ${list_var})
+	string(REGEX REPLACE "\\.o\n" ".c\n" ${list_var} ${${list_var}}) #change .o to .c
 	string(STRIP ${${list_var}} ${list_var}) #remove trailing/leading whitespaces
-	string(REPLACE "git-" "" ${list_var} ${${list_var}}) #strip `git-` prefix
-	string(REPLACE "\$X" ";" ${list_var} ${${list_var}}) #strip $X, ; is for converting the string into a list
+	## Parse the Makefile print-list-% format
+	string(REGEX REPLACE "${var_name} =\n" "" ${list_var} ${${list_var}})
+	string(REGEX REPLACE "${var_name} \\+?= ([^\n]+)" "\\1;" ${list_var} ${${list_var}})
 	list(TRANSFORM ${list_var} STRIP) #remove trailing/leading whitespaces for each element in list
 	list(REMOVE_ITEM ${list_var} "") #remove empty list elements
 endmacro()
@@ -635,14 +618,14 @@ include_directories(${CMAKE_BINARY_DIR})
 
 #build
 #libgit
-parse_makefile_for_sources(libgit_SOURCES "LIB_OBJS")
+ask_makefile_for_list(libgit_SOURCES "LIB_OBJS_NO_COMPAT")
 
 list(TRANSFORM libgit_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
 list(TRANSFORM compat_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
 add_library(libgit ${libgit_SOURCES} ${compat_SOURCES})
 
 #libxdiff
-parse_makefile_for_sources(libxdiff_SOURCES "XDIFF_OBJS")
+ask_makefile_for_list(libxdiff_SOURCES "XDIFF_OBJS")
 
 list(TRANSFORM libxdiff_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
 add_library(xdiff STATIC ${libxdiff_SOURCES})
@@ -693,7 +676,7 @@ elseif(UNIX)
 endif()
 
 #git
-parse_makefile_for_sources(git_SOURCES "BUILTIN_OBJS")
+ask_makefile_for_list(git_SOURCES "BUILTIN_OBJS")
 
 list(TRANSFORM git_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
 add_executable(git ${CMAKE_SOURCE_DIR}/git.c ${git_SOURCES})
@@ -729,7 +712,9 @@ if(CURL_FOUND)
 	endif()
 endif()
 
-parse_makefile_for_executables(git_builtin_extra "BUILT_INS")
+ask_makefile_for_list(git_builtin_extra "BUILT_INS")
+list(TRANSFORM git_builtin_extra REPLACE "^git-(.*)" "\\1") #strip `git-` prefix
+list(TRANSFORM git_builtin_extra REPLACE "\.exe$" "") #strip $X
 
 option(SKIP_DASHED_BUILT_INS "Skip hardlinking the dashed versions of the built-ins")
 
@@ -766,8 +751,8 @@ set(GITWEBDIR ${FALLBACK_RUNTIME_PREFIX}/share/locale)
 set(INSTLIBDIR ${FALLBACK_RUNTIME_PREFIX}/share/perl5)
 
 #shell scripts
-parse_makefile_for_scripts(git_sh_scripts "SCRIPT_SH" ".sh")
-parse_makefile_for_scripts(git_shlib_scripts "SCRIPT_LIB" "")
+ask_makefile_for_list(git_sh_scripts "SCRIPT_SH_GEN")
+ask_makefile_for_list(git_shlib_scripts "SCRIPT_LIB_GEN")
 set(git_shell_scripts
 	${git_sh_scripts} ${git_shlib_scripts} git-instaweb)
 
@@ -787,7 +772,7 @@ foreach(script ${git_shell_scripts})
 endforeach()
 
 #perl scripts
-parse_makefile_for_scripts(git_perl_scripts "SCRIPT_PERL" ".perl")
+ask_makefile_for_list(git_perl_scripts "SCRIPT_PERL_GEN")
 
 #create perl header
 file(STRINGS ${CMAKE_SOURCE_DIR}/perl/header_templates/fixed_prefix.template.pl perl_header )
@@ -910,9 +895,9 @@ add_executable(test-fake-ssh ${CMAKE_SOURCE_DIR}/t/helper/test-fake-ssh.c)
 target_link_libraries(test-fake-ssh common-main)
 
 #test-tool
-parse_makefile_for_sources(test-tool_SOURCES "TEST_BUILTINS_OBJS")
+ask_makefile_for_list(test-tool_SOURCES "TEST_BUILTINS_OBJS")
 
-list(TRANSFORM test-tool_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/t/helper/")
+list(TRANSFORM test-tool_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
 add_executable(test-tool ${CMAKE_SOURCE_DIR}/t/helper/test-tool.c ${test-tool_SOURCES})
 target_link_libraries(test-tool common-main)
 
-- 
2.33.1.1570.g069344fdd45

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

[Phillip Wood]

Hi Ævar

On 01/11/2021 19:19, Ævar Arnfjörð Bjarmason wrote:
> Remove the hardcoded lists of objects in favor of using
> $(wildcard). This means that every time a built-in, test tool etc. is
> added we won't need to patch the top-level Makefile, except for the
> few remaining cases where the asset in question would make it onto one
> of our list of exceptions.
> 
> Ever since 81b50f3ce40 (Move 'builtin-*' into a 'builtin/'
> subdirectory, 2010-02-22) this has been relatively easy to do (and
> even before that we could glob builtin-*.c). This pattern of
> exhaustively enumerating files was then carried forward for
> e.g. TEST_BUILTINS_OBJS in efd71f8913a (t/helper: add an empty
> test-tool program, 2018-03-24).
> 
> One reason not to do this is that now a new *.c file at the top-level
> will be immediately picked up, so if a new *.c file is being worked on
> "make" will error if it doesn't compile, whereas before that file
> would need to be explicitly listed in the Makefile. I think than small
> trade-off is worth it.

If I need to split up some uncommitted changes into several commits and 
I know it is going to be fiddly to do so I will sometimes copy the 
original file to foo.safe.c and then edit foo.c to create each commit. 
Then I can easily compile and test each commit and editing the file 
directly is often easier than using add -p and editing the hunks. With 
this patch running make will fail in that case I think.

> 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".
> 
> There's a few small "while we're at it" changes here, since I'm
> touching the code in question:
> 
>   - Start splitting up the the "Guard against the environment" section
>     at the top, but don't move anything that exists there out to avoid
>     merge conflicts
> 
>   - The $(TEST_BUILTINS_OBJS) variable was needlessly complex, because
>     it didn't have the full paths we'd pathsubst it back & forth.
> 
>   - Introduce *_SRC in addition to *_OBJ for the variable I'm
>     touching. Eventually we'll want to do this for all the *.o files,
>     i.e. make the *.c list a source of truth for *.o, which means we can
>     e.g. use that exhaustive list for "make TAGS".
> 
>   - Add a missing "curl-objs" target. See 029bac01a87 (Makefile: add
>     {program,xdiff,test,git,fuzz}-objs & objects targets, 2021-02-23)
>     for the commit that added the rest.
> 
>   - De-indent an "ifndef" block, we don't usually indent their
>     contents.
> 
> On the CMake changes here:
> 
>   - When CMake support was introduced in was introduced
>     061c2240b1b (Introduce CMake support for configuring Git, 2020-06-12)
>     there was a discussion about the maintenance burden of maintaining the
>     top-level Makefile in parallel with CMakeLists.txt[1] where reviewers
>     were assured that doing so would simply be a matter of adding something
>     to a list in the CMake recipe.
> 
>     Between change and some recent changes of mine where the "vs-build"
>     job failed to a divergence between the Makefile and CMakeList.txt I
>     can confidently say that that doesn't at all match reality. Even
>     seemingly trivial changes to the Makefile like this one are forcing
>     us to do a deep-dive into CMake internals to make forward progress
>     with our main build system.

My recollection is that the discussions were about not having to touch 
CMakeList.txt when adding new files to the build and I think that 
largely works. I don't think a lot of the changes you have been making 
recently were anticipated in that discussion.

>   - The promised "We can add a (continue-on-error) to vs-build job to
>     make this process less of a hindrance." in [2] never materialized.
>     Since 4c2c38e800f (ci: modification of main.yml to use cmake for
>     vs-build job, 2020-06-26) got a hard dependency on CMake as far as
>     getting the CI to pass goes.
> 
>   - The "vs-build" CI doesn't actually require that there be no GNU make
>     usage in the job, as it itself has a hard dependency on running a
>     "make -n artifacts-tar" command. So as far as any vs-specific special-sauce
>     goes we don't need a GNU-make free build system for vs-build.

We need GNU-make for the ci job but an individual developer using CMake 
does not need GNU-make installed. On linux it is possible to build git 
without having make installed by using cmake and ninja [1]

>   - The stated goal in 061c2240b1b of avoiding a GNU make dependency
>     for developer because it requires an SDK that "occupies around two
>     gigabytes" and "three quarters of a gigabyte worth of Git objects"
>     hardly seems worthwhile trade-off given the above. Disk space is cheap,
>     developer time required to maintain two parallel build systems isn't.

That rather assumes everyone has plenty of disk space and a decent 
network connection.

> My attempt to amend/revert 4c2c38e800f to have it use the
> pre-4c2c38e800f "make" invocation as a fallback failed, partially
> because I don't have a Windows development environment, so any attempt
> to change it is a painfully slow round-trip to GitHub CI.
> 
> Let's instead have CMake call out to the Makefile asking it what the
> definition of various variables lists is, rather than being forced to
> maintain those lists in a way that CMake can parse with regexes (which
> precludes anything but a giant hardcoded list).
> 
> I could familiarize myself enough with CMake to do this in some
> CMake-native way, but that would take "just as long as adding it to
> the Makefile"[2] (I think that took me <5 minutes, but I'm several
> hours into fighting with CMake)
> 
> So I consider this both a bugfix to the stated aims of this CMake
> integration, and a better way forward for having an alternate build
> system. I.e. If someone really does care about a having a
> GNU-make-less dependency for the "vs-build" I think this change offers
> a much better way forward for that.

I don't see how relying on GNU-make is a step forward for the CMake 
integration when it works without it now.

Overall I'm don't think that moving from a known set of dependencies to 
"build whatever C files are lying around in this directory" is an 
improvement.

Best Wishes

Phillip

[1] The CMake integration is currently broken for non-windows builds, 
I've got some fixes at 
https://github.com/phillipwood/git/tree/wip/cmake-fixes

> Once we invoke the Makefile to spew out e.g. its idea of "LIB_OBJS",
> it's going to be trivial to do that via some wrapper script that lives
> in "contrib/buildsystems". Such a script would either invoke "make
> print-{var,list}-%", or alternatively use an in-tree committed text
> file with the last known result of such a "make print-{var,list}-%"
> run.
> 
> 1. https://lore.kernel.org/git/xmqq8sikblv2.fsf@gitster.c.googlers.com
> 2. https://lore.kernel.org/git/CAKiG+9Xtof8Hj3npsS-M0SnT_dcjtHjP_+avWB4oOHkaMdnSbw@mail.gmail.com/
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   Makefile                            | 485 ++++------------------------
>   contrib/buildsystems/CMakeLists.txt |  53 ++-
>   2 files changed, 74 insertions(+), 464 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 4139bcf675c..5d78ab6860a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -590,6 +590,19 @@ TEST_OBJS =
>   TEST_PROGRAMS_NEED_X =
>   THIRD_PARTY_SOURCES =
>   
> +## Guard against env: programs
> +TEST_PROGRAMS =
> +
> +## Guard against env: sources
> +CURL_SRC =
> +TEST_PROGRAMS_NEED_X_SRC =
> +XDIFF_SRC =
> +
> +## Guard against env: objects
> +CONDITIONAL_OBJS =
> +CURL_OBJS =
> +LIB_OBJS_DIRS =
> +
>   # Utility to dump whatever variables are defined here
>   print-var-%:
>   	@echo $($*)
> @@ -697,87 +710,23 @@ X =
>   
>   PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
>   
> -TEST_BUILTINS_OBJS += test-advise.o
> -TEST_BUILTINS_OBJS += test-bitmap.o
> -TEST_BUILTINS_OBJS += test-bloom.o
> -TEST_BUILTINS_OBJS += test-chmtime.o
> -TEST_BUILTINS_OBJS += test-config.o
> -TEST_BUILTINS_OBJS += test-crontab.o
> -TEST_BUILTINS_OBJS += test-ctype.o
> -TEST_BUILTINS_OBJS += test-date.o
> -TEST_BUILTINS_OBJS += test-delta.o
> -TEST_BUILTINS_OBJS += test-dir-iterator.o
> -TEST_BUILTINS_OBJS += test-drop-caches.o
> -TEST_BUILTINS_OBJS += test-dump-cache-tree.o
> -TEST_BUILTINS_OBJS += test-dump-fsmonitor.o
> -TEST_BUILTINS_OBJS += test-dump-split-index.o
> -TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
> -TEST_BUILTINS_OBJS += test-example-decorate.o
> -TEST_BUILTINS_OBJS += test-fast-rebase.o
> -TEST_BUILTINS_OBJS += test-genrandom.o
> -TEST_BUILTINS_OBJS += test-genzeros.o
> -TEST_BUILTINS_OBJS += test-getcwd.o
> -TEST_BUILTINS_OBJS += test-hash-speed.o
> -TEST_BUILTINS_OBJS += test-hash.o
> -TEST_BUILTINS_OBJS += test-hashmap.o
> -TEST_BUILTINS_OBJS += test-index-version.o
> -TEST_BUILTINS_OBJS += test-json-writer.o
> -TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o
> -TEST_BUILTINS_OBJS += test-match-trees.o
> -TEST_BUILTINS_OBJS += test-mergesort.o
> -TEST_BUILTINS_OBJS += test-mktemp.o
> -TEST_BUILTINS_OBJS += test-oid-array.o
> -TEST_BUILTINS_OBJS += test-oidmap.o
> -TEST_BUILTINS_OBJS += test-oidtree.o
> -TEST_BUILTINS_OBJS += test-online-cpus.o
> -TEST_BUILTINS_OBJS += test-parse-options.o
> -TEST_BUILTINS_OBJS += test-parse-pathspec-file.o
> -TEST_BUILTINS_OBJS += test-partial-clone.o
> -TEST_BUILTINS_OBJS += test-path-utils.o
> -TEST_BUILTINS_OBJS += test-pcre2-config.o
> -TEST_BUILTINS_OBJS += test-pkt-line.o
> -TEST_BUILTINS_OBJS += test-prio-queue.o
> -TEST_BUILTINS_OBJS += test-proc-receive.o
> -TEST_BUILTINS_OBJS += test-progress.o
> -TEST_BUILTINS_OBJS += test-reach.o
> -TEST_BUILTINS_OBJS += test-read-cache.o
> -TEST_BUILTINS_OBJS += test-read-graph.o
> -TEST_BUILTINS_OBJS += test-read-midx.o
> -TEST_BUILTINS_OBJS += test-ref-store.o
> -TEST_BUILTINS_OBJS += test-regex.o
> -TEST_BUILTINS_OBJS += test-repository.o
> -TEST_BUILTINS_OBJS += test-revision-walking.o
> -TEST_BUILTINS_OBJS += test-run-command.o
> -TEST_BUILTINS_OBJS += test-scrap-cache-tree.o
> -TEST_BUILTINS_OBJS += test-serve-v2.o
> -TEST_BUILTINS_OBJS += test-sha1.o
> -TEST_BUILTINS_OBJS += test-sha256.o
> -TEST_BUILTINS_OBJS += test-sigchain.o
> -TEST_BUILTINS_OBJS += test-simple-ipc.o
> -TEST_BUILTINS_OBJS += test-strcmp-offset.o
> -TEST_BUILTINS_OBJS += test-string-list.o
> -TEST_BUILTINS_OBJS += test-submodule-config.o
> -TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o
> -TEST_BUILTINS_OBJS += test-subprocess.o
> -TEST_BUILTINS_OBJS += test-trace2.o
> -TEST_BUILTINS_OBJS += test-urlmatch-normalization.o
> -TEST_BUILTINS_OBJS += test-userdiff.o
> -TEST_BUILTINS_OBJS += test-wildmatch.o
> -TEST_BUILTINS_OBJS += test-windows-named-pipe.o
> -TEST_BUILTINS_OBJS += test-write-cache.o
> -TEST_BUILTINS_OBJS += test-xml-encode.o
> -
>   # Do not add more tests here unless they have extra dependencies. Add
>   # them in TEST_BUILTINS_OBJS above.
>   TEST_PROGRAMS_NEED_X += test-fake-ssh
>   TEST_PROGRAMS_NEED_X += test-tool
>   
> -TEST_PROGRAMS = $(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X))
> +TEST_PROGRAMS_NEED_X_SRC += $(TEST_PROGRAMS_NEED_X:%=t/helper/%.c)
> +TEST_PROGRAMS += $(TEST_PROGRAMS_NEED_X_SRC:%.c=%$X)
> +TEST_BUILTINS_SRC += $(filter-out $(TEST_PROGRAMS_NEED_X_SRC),$(wildcard t/helper/*.c))
> +TEST_BUILTINS_OBJS += $(TEST_BUILTINS_SRC:%.c=%.o)
>   
> -# 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.
> +# List built-in command $C whose implementation cmd_$C() is in
> +# builtin/$C.o
> +BUILTIN_OBJS = $(patsubst %.c,%.o,$(wildcard builtin/*.c))
>   BUILT_INS += $(patsubst builtin/%.o,git-%$X,$(BUILTIN_OBJS))
>   
> +# 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.
>   BUILT_INS += git-cherry$X
>   BUILT_INS += git-cherry-pick$X
>   BUILT_INS += git-format-patch$X
> @@ -837,355 +786,28 @@ LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentat
>   	-name Documentation -prune -o \
>   	-name '*.h' -print)))
>   
> -LIB_OBJS += abspath.o
> -LIB_OBJS += add-interactive.o
> -LIB_OBJS += add-patch.o
> -LIB_OBJS += advice.o
> -LIB_OBJS += alias.o
> -LIB_OBJS += alloc.o
> -LIB_OBJS += apply.o
> -LIB_OBJS += archive-tar.o
> -LIB_OBJS += archive-zip.o
> -LIB_OBJS += archive.o
> -LIB_OBJS += attr.o
> -LIB_OBJS += base85.o
> -LIB_OBJS += bisect.o
> -LIB_OBJS += blame.o
> -LIB_OBJS += blob.o
> -LIB_OBJS += bloom.o
> -LIB_OBJS += branch.o
> -LIB_OBJS += bulk-checkin.o
> -LIB_OBJS += bundle.o
> -LIB_OBJS += cache-tree.o
> -LIB_OBJS += cbtree.o
> -LIB_OBJS += chdir-notify.o
> -LIB_OBJS += checkout.o
> -LIB_OBJS += chunk-format.o
> -LIB_OBJS += color.o
> -LIB_OBJS += column.o
> -LIB_OBJS += combine-diff.o
> -LIB_OBJS += commit-graph.o
> -LIB_OBJS += commit-reach.o
> -LIB_OBJS += commit.o
> +# LIB_OBJS: compat/* objects that live at the top-level
> +CONDITIONAL_OBJS += unix-socket.o
> +CONDITIONAL_OBJS += unix-stream-server.o
> +CONDITIONAL_OBJS += sha1dc_git.o
> +
> +# LIB_OBJS: Mostly glob *.c at the top-level, with some exlusions
> +LIB_OBJS += $(filter-out \
> +	$(CONDITIONAL_OBJS) \
> +	git.o common-main.o $(PROGRAM_OBJS) \
> +	$(FUZZ_OBJS) $(CURL_OBJS),\
> +	$(patsubst %.c,%.o,$(wildcard *.c)))
> +
> +# LIB_OBJS: Directories that contain only LIB_OBJS
> +LIB_OBJS_DIRS += ewah
> +LIB_OBJS_DIRS += negotiator
> +LIB_OBJS_DIRS += refs
> +LIB_OBJS_DIRS += trace2
> +LIB_OBJS += $(patsubst %.c,%.o,$(wildcard $(addsuffix /*.c,$(LIB_OBJS_DIRS))))
> +
> +# LIB_OBJS: unconditional compat/* objects
>   LIB_OBJS += compat/obstack.o
>   LIB_OBJS += compat/terminal.o
> -LIB_OBJS += config.o
> -LIB_OBJS += connect.o
> -LIB_OBJS += connected.o
> -LIB_OBJS += convert.o
> -LIB_OBJS += copy.o
> -LIB_OBJS += credential.o
> -LIB_OBJS += csum-file.o
> -LIB_OBJS += ctype.o
> -LIB_OBJS += date.o
> -LIB_OBJS += decorate.o
> -LIB_OBJS += delta-islands.o
> -LIB_OBJS += diff-delta.o
> -LIB_OBJS += diff-merges.o
> -LIB_OBJS += diff-lib.o
> -LIB_OBJS += diff-no-index.o
> -LIB_OBJS += diff.o
> -LIB_OBJS += diffcore-break.o
> -LIB_OBJS += diffcore-delta.o
> -LIB_OBJS += diffcore-order.o
> -LIB_OBJS += diffcore-pickaxe.o
> -LIB_OBJS += diffcore-rename.o
> -LIB_OBJS += diffcore-rotate.o
> -LIB_OBJS += dir-iterator.o
> -LIB_OBJS += dir.o
> -LIB_OBJS += editor.o
> -LIB_OBJS += entry.o
> -LIB_OBJS += environment.o
> -LIB_OBJS += ewah/bitmap.o
> -LIB_OBJS += ewah/ewah_bitmap.o
> -LIB_OBJS += ewah/ewah_io.o
> -LIB_OBJS += ewah/ewah_rlw.o
> -LIB_OBJS += exec-cmd.o
> -LIB_OBJS += fetch-negotiator.o
> -LIB_OBJS += fetch-pack.o
> -LIB_OBJS += fmt-merge-msg.o
> -LIB_OBJS += fsck.o
> -LIB_OBJS += fsmonitor.o
> -LIB_OBJS += gettext.o
> -LIB_OBJS += gpg-interface.o
> -LIB_OBJS += graph.o
> -LIB_OBJS += grep.o
> -LIB_OBJS += hash-lookup.o
> -LIB_OBJS += hashmap.o
> -LIB_OBJS += help.o
> -LIB_OBJS += hex.o
> -LIB_OBJS += hook.o
> -LIB_OBJS += ident.o
> -LIB_OBJS += json-writer.o
> -LIB_OBJS += kwset.o
> -LIB_OBJS += levenshtein.o
> -LIB_OBJS += line-log.o
> -LIB_OBJS += line-range.o
> -LIB_OBJS += linear-assignment.o
> -LIB_OBJS += list-objects-filter-options.o
> -LIB_OBJS += list-objects-filter.o
> -LIB_OBJS += list-objects.o
> -LIB_OBJS += ll-merge.o
> -LIB_OBJS += lockfile.o
> -LIB_OBJS += log-tree.o
> -LIB_OBJS += ls-refs.o
> -LIB_OBJS += mailinfo.o
> -LIB_OBJS += mailmap.o
> -LIB_OBJS += match-trees.o
> -LIB_OBJS += mem-pool.o
> -LIB_OBJS += merge-blobs.o
> -LIB_OBJS += merge-ort.o
> -LIB_OBJS += merge-ort-wrappers.o
> -LIB_OBJS += merge-recursive.o
> -LIB_OBJS += merge.o
> -LIB_OBJS += mergesort.o
> -LIB_OBJS += midx.o
> -LIB_OBJS += name-hash.o
> -LIB_OBJS += negotiator/default.o
> -LIB_OBJS += negotiator/noop.o
> -LIB_OBJS += negotiator/skipping.o
> -LIB_OBJS += notes-cache.o
> -LIB_OBJS += notes-merge.o
> -LIB_OBJS += notes-utils.o
> -LIB_OBJS += notes.o
> -LIB_OBJS += object-file.o
> -LIB_OBJS += object-name.o
> -LIB_OBJS += object.o
> -LIB_OBJS += oid-array.o
> -LIB_OBJS += oidmap.o
> -LIB_OBJS += oidset.o
> -LIB_OBJS += oidtree.o
> -LIB_OBJS += pack-bitmap-write.o
> -LIB_OBJS += pack-bitmap.o
> -LIB_OBJS += pack-check.o
> -LIB_OBJS += pack-objects.o
> -LIB_OBJS += pack-revindex.o
> -LIB_OBJS += pack-write.o
> -LIB_OBJS += packfile.o
> -LIB_OBJS += pager.o
> -LIB_OBJS += parallel-checkout.o
> -LIB_OBJS += parse-options-cb.o
> -LIB_OBJS += parse-options.o
> -LIB_OBJS += patch-delta.o
> -LIB_OBJS += patch-ids.o
> -LIB_OBJS += path.o
> -LIB_OBJS += pathspec.o
> -LIB_OBJS += pkt-line.o
> -LIB_OBJS += preload-index.o
> -LIB_OBJS += pretty.o
> -LIB_OBJS += prio-queue.o
> -LIB_OBJS += progress.o
> -LIB_OBJS += promisor-remote.o
> -LIB_OBJS += prompt.o
> -LIB_OBJS += protocol.o
> -LIB_OBJS += protocol-caps.o
> -LIB_OBJS += prune-packed.o
> -LIB_OBJS += quote.o
> -LIB_OBJS += range-diff.o
> -LIB_OBJS += reachable.o
> -LIB_OBJS += read-cache.o
> -LIB_OBJS += rebase-interactive.o
> -LIB_OBJS += rebase.o
> -LIB_OBJS += ref-filter.o
> -LIB_OBJS += reflog-walk.o
> -LIB_OBJS += refs.o
> -LIB_OBJS += refs/debug.o
> -LIB_OBJS += refs/files-backend.o
> -LIB_OBJS += refs/iterator.o
> -LIB_OBJS += refs/packed-backend.o
> -LIB_OBJS += refs/ref-cache.o
> -LIB_OBJS += refspec.o
> -LIB_OBJS += remote.o
> -LIB_OBJS += replace-object.o
> -LIB_OBJS += repo-settings.o
> -LIB_OBJS += repository.o
> -LIB_OBJS += rerere.o
> -LIB_OBJS += reset.o
> -LIB_OBJS += resolve-undo.o
> -LIB_OBJS += revision.o
> -LIB_OBJS += run-command.o
> -LIB_OBJS += send-pack.o
> -LIB_OBJS += sequencer.o
> -LIB_OBJS += serve.o
> -LIB_OBJS += server-info.o
> -LIB_OBJS += setup.o
> -LIB_OBJS += shallow.o
> -LIB_OBJS += sideband.o
> -LIB_OBJS += sigchain.o
> -LIB_OBJS += sparse-index.o
> -LIB_OBJS += split-index.o
> -LIB_OBJS += stable-qsort.o
> -LIB_OBJS += strbuf.o
> -LIB_OBJS += streaming.o
> -LIB_OBJS += string-list.o
> -LIB_OBJS += strmap.o
> -LIB_OBJS += strvec.o
> -LIB_OBJS += sub-process.o
> -LIB_OBJS += submodule-config.o
> -LIB_OBJS += submodule.o
> -LIB_OBJS += symlinks.o
> -LIB_OBJS += tag.o
> -LIB_OBJS += tempfile.o
> -LIB_OBJS += thread-utils.o
> -LIB_OBJS += tmp-objdir.o
> -LIB_OBJS += trace.o
> -LIB_OBJS += trace2.o
> -LIB_OBJS += trace2/tr2_cfg.o
> -LIB_OBJS += trace2/tr2_cmd_name.o
> -LIB_OBJS += trace2/tr2_dst.o
> -LIB_OBJS += trace2/tr2_sid.o
> -LIB_OBJS += trace2/tr2_sysenv.o
> -LIB_OBJS += trace2/tr2_tbuf.o
> -LIB_OBJS += trace2/tr2_tgt_event.o
> -LIB_OBJS += trace2/tr2_tgt_normal.o
> -LIB_OBJS += trace2/tr2_tgt_perf.o
> -LIB_OBJS += trace2/tr2_tls.o
> -LIB_OBJS += trailer.o
> -LIB_OBJS += transport-helper.o
> -LIB_OBJS += transport.o
> -LIB_OBJS += tree-diff.o
> -LIB_OBJS += tree-walk.o
> -LIB_OBJS += tree.o
> -LIB_OBJS += unpack-trees.o
> -LIB_OBJS += upload-pack.o
> -LIB_OBJS += url.o
> -LIB_OBJS += urlmatch.o
> -LIB_OBJS += usage.o
> -LIB_OBJS += userdiff.o
> -LIB_OBJS += utf8.o
> -LIB_OBJS += varint.o
> -LIB_OBJS += version.o
> -LIB_OBJS += versioncmp.o
> -LIB_OBJS += walker.o
> -LIB_OBJS += wildmatch.o
> -LIB_OBJS += worktree.o
> -LIB_OBJS += wrapper.o
> -LIB_OBJS += write-or-die.o
> -LIB_OBJS += ws.o
> -LIB_OBJS += wt-status.o
> -LIB_OBJS += xdiff-interface.o
> -LIB_OBJS += zlib.o
> -
> -BUILTIN_OBJS += builtin/add.o
> -BUILTIN_OBJS += builtin/am.o
> -BUILTIN_OBJS += builtin/annotate.o
> -BUILTIN_OBJS += builtin/apply.o
> -BUILTIN_OBJS += builtin/archive.o
> -BUILTIN_OBJS += builtin/bisect--helper.o
> -BUILTIN_OBJS += builtin/blame.o
> -BUILTIN_OBJS += builtin/branch.o
> -BUILTIN_OBJS += builtin/bugreport.o
> -BUILTIN_OBJS += builtin/bundle.o
> -BUILTIN_OBJS += builtin/cat-file.o
> -BUILTIN_OBJS += builtin/check-attr.o
> -BUILTIN_OBJS += builtin/check-ignore.o
> -BUILTIN_OBJS += builtin/check-mailmap.o
> -BUILTIN_OBJS += builtin/check-ref-format.o
> -BUILTIN_OBJS += builtin/checkout--worker.o
> -BUILTIN_OBJS += builtin/checkout-index.o
> -BUILTIN_OBJS += builtin/checkout.o
> -BUILTIN_OBJS += builtin/clean.o
> -BUILTIN_OBJS += builtin/clone.o
> -BUILTIN_OBJS += builtin/column.o
> -BUILTIN_OBJS += builtin/commit-graph.o
> -BUILTIN_OBJS += builtin/commit-tree.o
> -BUILTIN_OBJS += builtin/commit.o
> -BUILTIN_OBJS += builtin/config.o
> -BUILTIN_OBJS += builtin/count-objects.o
> -BUILTIN_OBJS += builtin/credential-cache--daemon.o
> -BUILTIN_OBJS += builtin/credential-cache.o
> -BUILTIN_OBJS += builtin/credential-store.o
> -BUILTIN_OBJS += builtin/credential.o
> -BUILTIN_OBJS += builtin/describe.o
> -BUILTIN_OBJS += builtin/diff-files.o
> -BUILTIN_OBJS += builtin/diff-index.o
> -BUILTIN_OBJS += builtin/diff-tree.o
> -BUILTIN_OBJS += builtin/diff.o
> -BUILTIN_OBJS += builtin/difftool.o
> -BUILTIN_OBJS += builtin/env--helper.o
> -BUILTIN_OBJS += builtin/fast-export.o
> -BUILTIN_OBJS += builtin/fast-import.o
> -BUILTIN_OBJS += builtin/fetch-pack.o
> -BUILTIN_OBJS += builtin/fetch.o
> -BUILTIN_OBJS += builtin/fmt-merge-msg.o
> -BUILTIN_OBJS += builtin/for-each-ref.o
> -BUILTIN_OBJS += builtin/for-each-repo.o
> -BUILTIN_OBJS += builtin/fsck.o
> -BUILTIN_OBJS += builtin/gc.o
> -BUILTIN_OBJS += builtin/get-tar-commit-id.o
> -BUILTIN_OBJS += builtin/grep.o
> -BUILTIN_OBJS += builtin/hash-object.o
> -BUILTIN_OBJS += builtin/help.o
> -BUILTIN_OBJS += builtin/index-pack.o
> -BUILTIN_OBJS += builtin/init-db.o
> -BUILTIN_OBJS += builtin/interpret-trailers.o
> -BUILTIN_OBJS += builtin/log.o
> -BUILTIN_OBJS += builtin/ls-files.o
> -BUILTIN_OBJS += builtin/ls-remote.o
> -BUILTIN_OBJS += builtin/ls-tree.o
> -BUILTIN_OBJS += builtin/mailinfo.o
> -BUILTIN_OBJS += builtin/mailsplit.o
> -BUILTIN_OBJS += builtin/merge-base.o
> -BUILTIN_OBJS += builtin/merge-file.o
> -BUILTIN_OBJS += builtin/merge-index.o
> -BUILTIN_OBJS += builtin/merge-ours.o
> -BUILTIN_OBJS += builtin/merge-recursive.o
> -BUILTIN_OBJS += builtin/merge-tree.o
> -BUILTIN_OBJS += builtin/merge.o
> -BUILTIN_OBJS += builtin/mktag.o
> -BUILTIN_OBJS += builtin/mktree.o
> -BUILTIN_OBJS += builtin/multi-pack-index.o
> -BUILTIN_OBJS += builtin/mv.o
> -BUILTIN_OBJS += builtin/name-rev.o
> -BUILTIN_OBJS += builtin/notes.o
> -BUILTIN_OBJS += builtin/pack-objects.o
> -BUILTIN_OBJS += builtin/pack-redundant.o
> -BUILTIN_OBJS += builtin/pack-refs.o
> -BUILTIN_OBJS += builtin/patch-id.o
> -BUILTIN_OBJS += builtin/prune-packed.o
> -BUILTIN_OBJS += builtin/prune.o
> -BUILTIN_OBJS += builtin/pull.o
> -BUILTIN_OBJS += builtin/push.o
> -BUILTIN_OBJS += builtin/range-diff.o
> -BUILTIN_OBJS += builtin/read-tree.o
> -BUILTIN_OBJS += builtin/rebase.o
> -BUILTIN_OBJS += builtin/receive-pack.o
> -BUILTIN_OBJS += builtin/reflog.o
> -BUILTIN_OBJS += builtin/remote-ext.o
> -BUILTIN_OBJS += builtin/remote-fd.o
> -BUILTIN_OBJS += builtin/remote.o
> -BUILTIN_OBJS += builtin/repack.o
> -BUILTIN_OBJS += builtin/replace.o
> -BUILTIN_OBJS += builtin/rerere.o
> -BUILTIN_OBJS += builtin/reset.o
> -BUILTIN_OBJS += builtin/rev-list.o
> -BUILTIN_OBJS += builtin/rev-parse.o
> -BUILTIN_OBJS += builtin/revert.o
> -BUILTIN_OBJS += builtin/rm.o
> -BUILTIN_OBJS += builtin/send-pack.o
> -BUILTIN_OBJS += builtin/shortlog.o
> -BUILTIN_OBJS += builtin/show-branch.o
> -BUILTIN_OBJS += builtin/show-index.o
> -BUILTIN_OBJS += builtin/show-ref.o
> -BUILTIN_OBJS += builtin/sparse-checkout.o
> -BUILTIN_OBJS += builtin/stash.o
> -BUILTIN_OBJS += builtin/stripspace.o
> -BUILTIN_OBJS += builtin/submodule--helper.o
> -BUILTIN_OBJS += builtin/symbolic-ref.o
> -BUILTIN_OBJS += builtin/tag.o
> -BUILTIN_OBJS += builtin/unpack-file.o
> -BUILTIN_OBJS += builtin/unpack-objects.o
> -BUILTIN_OBJS += builtin/update-index.o
> -BUILTIN_OBJS += builtin/update-ref.o
> -BUILTIN_OBJS += builtin/update-server-info.o
> -BUILTIN_OBJS += builtin/upload-archive.o
> -BUILTIN_OBJS += builtin/upload-pack.o
> -BUILTIN_OBJS += builtin/var.o
> -BUILTIN_OBJS += builtin/verify-commit.o
> -BUILTIN_OBJS += builtin/verify-pack.o
> -BUILTIN_OBJS += builtin/verify-tag.o
> -BUILTIN_OBJS += builtin/worktree.o
> -BUILTIN_OBJS += builtin/write-tree.o
>   
>   # THIRD_PARTY_SOURCES is a list of patterns compatible with the
>   # $(filter) and $(filter-out) family of functions. They specify source
> @@ -2076,6 +1698,7 @@ endif
>   LIBS = $(filter-out %.o, $(GITLIBS)) $(EXTLIBS)
>   
>   BASIC_CFLAGS += $(COMPAT_CFLAGS)
> +LIB_OBJS_NO_COMPAT := $(LIB_OBJS)
>   LIB_OBJS += $(COMPAT_OBJS)
>   
>   # Quote for C
> @@ -2436,17 +2059,12 @@ reconfigure config.mak.autogen: config.status
>   .PHONY: reconfigure # This is a convenience target.
>   endif
>   
> -XDIFF_OBJS += xdiff/xdiffi.o
> -XDIFF_OBJS += xdiff/xemit.o
> -XDIFF_OBJS += xdiff/xhistogram.o
> -XDIFF_OBJS += xdiff/xmerge.o
> -XDIFF_OBJS += xdiff/xpatience.o
> -XDIFF_OBJS += xdiff/xprepare.o
> -XDIFF_OBJS += xdiff/xutils.o
> +XDIFF_SRC += $(wildcard xdiff/*.c)
> +XDIFF_OBJS += $(XDIFF_SRC:.c=.o)
>   .PHONY: xdiff-objs
>   xdiff-objs: $(XDIFF_OBJS)
>   
> -TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
> +TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(TEST_BUILTINS_OBJS)
>   .PHONY: test-objs
>   test-objs: $(TEST_OBJS)
>   
> @@ -2457,13 +2075,20 @@ GIT_OBJS += git.o
>   .PHONY: git-objs
>   git-objs: $(GIT_OBJS)
>   
> +CURL_SRC += http.c
> +CURL_SRC += http-walker.c
> +CURL_SRC += remote-curl.c
> +CURL_OBJS += $(CURL_SRC:.c=.o)
> +.PHONY: curl-objs
> +curl-objs: $(CURL_OBJS)
> +
>   OBJECTS += $(GIT_OBJS)
>   OBJECTS += $(PROGRAM_OBJS)
>   OBJECTS += $(TEST_OBJS)
>   OBJECTS += $(XDIFF_OBJS)
>   OBJECTS += $(FUZZ_OBJS)
>   ifndef NO_CURL
> -	OBJECTS += http.o http-walker.o remote-curl.o
> +OBJECTS += $(CURL_OBJS)
>   endif
>   .PHONY: objects
>   objects: $(OBJECTS)
> @@ -2900,7 +2525,7 @@ perf: all
>   
>   .PRECIOUS: $(TEST_OBJS)
>   
> -t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
> +t/helper/test-tool$X: $(TEST_BUILTINS_OBJS)
>   
>   t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS)
>   	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index fd1399c440f..d57c9203b66 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -111,32 +111,15 @@ project(git
>   #TODO Add pcre support
>   
>   #macros for parsing the Makefile for sources and scripts
> -macro(parse_makefile_for_sources list_var regex)
> -	file(STRINGS ${CMAKE_SOURCE_DIR}/Makefile ${list_var} REGEX "^${regex} \\+=(.*)")
> -	string(REPLACE "${regex} +=" "" ${list_var} ${${list_var}})
> -	string(REPLACE "$(COMPAT_OBJS)" "" ${list_var} ${${list_var}}) #remove "$(COMPAT_OBJS)" This is only for libgit.
> -	string(STRIP ${${list_var}} ${list_var}) #remove trailing/leading whitespaces
> -	string(REPLACE ".o" ".c;" ${list_var} ${${list_var}}) #change .o to .c, ; is for converting the string into a list
> -	list(TRANSFORM ${list_var} STRIP) #remove trailing/leading whitespaces for each element in list
> -	list(REMOVE_ITEM ${list_var} "") #remove empty list elements
> -endmacro()
> -
> -macro(parse_makefile_for_scripts list_var regex lang)
> -	file(STRINGS ${CMAKE_SOURCE_DIR}/Makefile ${list_var} REGEX "^${regex} \\+=(.*)")
> -	string(REPLACE "${regex} +=" "" ${list_var} ${${list_var}})
> -	string(STRIP ${${list_var}} ${list_var}) #remove trailing/leading whitespaces
> -	string(REPLACE " " ";" ${list_var} ${${list_var}}) #convert string to a list
> -	if(NOT ${lang}) #exclude for SCRIPT_LIB
> -		list(TRANSFORM ${list_var} REPLACE "${lang}" "") #do the replacement
> -	endif()
> -endmacro()
> -
> -macro(parse_makefile_for_executables list_var regex)
> -	file(STRINGS ${CMAKE_SOURCE_DIR}/Makefile ${list_var} REGEX "^${regex} \\+= git-(.*)")
> -	string(REPLACE "${regex} +=" "" ${list_var} ${${list_var}})
> +macro(ask_makefile_for_list list_var var_name)
> +	execute_process(COMMAND make print-list-${var_name}
> +			WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
> +			OUTPUT_VARIABLE ${list_var})
> +	string(REGEX REPLACE "\\.o\n" ".c\n" ${list_var} ${${list_var}}) #change .o to .c
>   	string(STRIP ${${list_var}} ${list_var}) #remove trailing/leading whitespaces
> -	string(REPLACE "git-" "" ${list_var} ${${list_var}}) #strip `git-` prefix
> -	string(REPLACE "\$X" ";" ${list_var} ${${list_var}}) #strip $X, ; is for converting the string into a list
> +	## Parse the Makefile print-list-% format
> +	string(REGEX REPLACE "${var_name} =\n" "" ${list_var} ${${list_var}})
> +	string(REGEX REPLACE "${var_name} \\+?= ([^\n]+)" "\\1;" ${list_var} ${${list_var}})
>   	list(TRANSFORM ${list_var} STRIP) #remove trailing/leading whitespaces for each element in list
>   	list(REMOVE_ITEM ${list_var} "") #remove empty list elements
>   endmacro()
> @@ -635,14 +618,14 @@ include_directories(${CMAKE_BINARY_DIR})
>   
>   #build
>   #libgit
> -parse_makefile_for_sources(libgit_SOURCES "LIB_OBJS")
> +ask_makefile_for_list(libgit_SOURCES "LIB_OBJS_NO_COMPAT")
>   
>   list(TRANSFORM libgit_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
>   list(TRANSFORM compat_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
>   add_library(libgit ${libgit_SOURCES} ${compat_SOURCES})
>   
>   #libxdiff
> -parse_makefile_for_sources(libxdiff_SOURCES "XDIFF_OBJS")
> +ask_makefile_for_list(libxdiff_SOURCES "XDIFF_OBJS")
>   
>   list(TRANSFORM libxdiff_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
>   add_library(xdiff STATIC ${libxdiff_SOURCES})
> @@ -693,7 +676,7 @@ elseif(UNIX)
>   endif()
>   
>   #git
> -parse_makefile_for_sources(git_SOURCES "BUILTIN_OBJS")
> +ask_makefile_for_list(git_SOURCES "BUILTIN_OBJS")
>   
>   list(TRANSFORM git_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
>   add_executable(git ${CMAKE_SOURCE_DIR}/git.c ${git_SOURCES})
> @@ -729,7 +712,9 @@ if(CURL_FOUND)
>   	endif()
>   endif()
>   
> -parse_makefile_for_executables(git_builtin_extra "BUILT_INS")
> +ask_makefile_for_list(git_builtin_extra "BUILT_INS")
> +list(TRANSFORM git_builtin_extra REPLACE "^git-(.*)" "\\1") #strip `git-` prefix
> +list(TRANSFORM git_builtin_extra REPLACE "\.exe$" "") #strip $X
>   
>   option(SKIP_DASHED_BUILT_INS "Skip hardlinking the dashed versions of the built-ins")
>   
> @@ -766,8 +751,8 @@ set(GITWEBDIR ${FALLBACK_RUNTIME_PREFIX}/share/locale)
>   set(INSTLIBDIR ${FALLBACK_RUNTIME_PREFIX}/share/perl5)
>   
>   #shell scripts
> -parse_makefile_for_scripts(git_sh_scripts "SCRIPT_SH" ".sh")
> -parse_makefile_for_scripts(git_shlib_scripts "SCRIPT_LIB" "")
> +ask_makefile_for_list(git_sh_scripts "SCRIPT_SH_GEN")
> +ask_makefile_for_list(git_shlib_scripts "SCRIPT_LIB_GEN")
>   set(git_shell_scripts
>   	${git_sh_scripts} ${git_shlib_scripts} git-instaweb)
>   
> @@ -787,7 +772,7 @@ foreach(script ${git_shell_scripts})
>   endforeach()
>   
>   #perl scripts
> -parse_makefile_for_scripts(git_perl_scripts "SCRIPT_PERL" ".perl")
> +ask_makefile_for_list(git_perl_scripts "SCRIPT_PERL_GEN")
>   
>   #create perl header
>   file(STRINGS ${CMAKE_SOURCE_DIR}/perl/header_templates/fixed_prefix.template.pl perl_header )
> @@ -910,9 +895,9 @@ add_executable(test-fake-ssh ${CMAKE_SOURCE_DIR}/t/helper/test-fake-ssh.c)
>   target_link_libraries(test-fake-ssh common-main)
>   
>   #test-tool
> -parse_makefile_for_sources(test-tool_SOURCES "TEST_BUILTINS_OBJS")
> +ask_makefile_for_list(test-tool_SOURCES "TEST_BUILTINS_OBJS")
>   
> -list(TRANSFORM test-tool_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/t/helper/")
> +list(TRANSFORM test-tool_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
>   add_executable(test-tool ${CMAKE_SOURCE_DIR}/t/helper/test-tool.c ${test-tool_SOURCES})
>   target_link_libraries(test-tool common-main)
>   
> 

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

[Ævar Arnfjörð Bjarmason]

On Sat, Nov 06 2021, Phillip Wood wrote:

> Hi Ævar
>
> On 01/11/2021 19:19, Ævar Arnfjörð Bjarmason wrote:
>> Remove the hardcoded lists of objects in favor of using
>> $(wildcard). This means that every time a built-in, test tool etc. is
>> added we won't need to patch the top-level Makefile, except for the
>> few remaining cases where the asset in question would make it onto one
>> of our list of exceptions.
>> Ever since 81b50f3ce40 (Move 'builtin-*' into a 'builtin/'
>> subdirectory, 2010-02-22) this has been relatively easy to do (and
>> even before that we could glob builtin-*.c). This pattern of
>> exhaustively enumerating files was then carried forward for
>> e.g. TEST_BUILTINS_OBJS in efd71f8913a (t/helper: add an empty
>> test-tool program, 2018-03-24).
>> One reason not to do this is that now a new *.c file at the
>> top-level
>> will be immediately picked up, so if a new *.c file is being worked on
>> "make" will error if it doesn't compile, whereas before that file
>> would need to be explicitly listed in the Makefile. I think than small
>> trade-off is worth it.
>
> If I need to split up some uncommitted changes into several commits
> and I know it is going to be fiddly to do so I will sometimes copy the 
> original file to foo.safe.c and then edit foo.c to create each
> commit. Then I can easily compile and test each commit and editing the
> file directly is often easier than using add -p and editing the
> hunks. With this patch running make will fail in that case I think.

Would being able to do something like:

    make EXCLUDE_WILDCARD=foo.safe.c

Satisfy this use-case?

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.

>> 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".
>> There's a few small "while we're at it" changes here, since I'm
>> touching the code in question:
>>   - Start splitting up the the "Guard against the environment"
>> section
>>     at the top, but don't move anything that exists there out to avoid
>>     merge conflicts
>>   - The $(TEST_BUILTINS_OBJS) variable was needlessly complex,
>> because
>>     it didn't have the full paths we'd pathsubst it back & forth.
>>   - Introduce *_SRC in addition to *_OBJ for the variable I'm
>>     touching. Eventually we'll want to do this for all the *.o files,
>>     i.e. make the *.c list a source of truth for *.o, which means we can
>>     e.g. use that exhaustive list for "make TAGS".
>>   - Add a missing "curl-objs" target. See 029bac01a87 (Makefile: add
>>     {program,xdiff,test,git,fuzz}-objs & objects targets, 2021-02-23)
>>     for the commit that added the rest.
>>   - De-indent an "ifndef" block, we don't usually indent their
>>     contents.
>> On the CMake changes here:
>>   - When CMake support was introduced in was introduced
>>     061c2240b1b (Introduce CMake support for configuring Git, 2020-06-12)
>>     there was a discussion about the maintenance burden of maintaining the
>>     top-level Makefile in parallel with CMakeLists.txt[1] where reviewers
>>     were assured that doing so would simply be a matter of adding something
>>     to a list in the CMake recipe.
>>     Between change and some recent changes of mine where the
>> "vs-build"
>>     job failed to a divergence between the Makefile and CMakeList.txt I
>>     can confidently say that that doesn't at all match reality. Even
>>     seemingly trivial changes to the Makefile like this one are forcing
>>     us to do a deep-dive into CMake internals to make forward progress
>>     with our main build system.
>
> My recollection is that the discussions were about not having to touch
> CMakeList.txt when adding new files to the build and I think that 
> largely works. I don't think a lot of the changes you have been making
> recently were anticipated in that discussion.

That's part of it, but the concern about needing to maintain two systems
in perpetuity was also brought up, and it not being a hard dependency,
having that vs-{build,test} job soft-fail on it etc. were brought up but
that's not what we've got now.

In any case, having to maintain two build systems and the maintainer(s)
of the CMake being inactive one is the situation we're in now.

>>   - The promised "We can add a (continue-on-error) to vs-build job to
>>     make this process less of a hindrance." in [2] never materialized.
>>     Since 4c2c38e800f (ci: modification of main.yml to use cmake for
>>     vs-build job, 2020-06-26) got a hard dependency on CMake as far as
>>     getting the CI to pass goes.
>>   - The "vs-build" CI doesn't actually require that there be no GNU
>> make
>>     usage in the job, as it itself has a hard dependency on running a
>>     "make -n artifacts-tar" command. So as far as any vs-specific special-sauce
>>     goes we don't need a GNU-make free build system for vs-build.
>
> We need GNU-make for the ci job but an individual developer using
> CMake does not need GNU-make installed. On linux it is possible to
> build git without having make installed by using cmake and ninja [1]

I'd tried that, but didn't know about your [1] topic. If you or someone
is actively willing to fix things up in CMake....

>>   - The stated goal in 061c2240b1b of avoiding a GNU make dependency
>>     for developer because it requires an SDK that "occupies around two
>>     gigabytes" and "three quarters of a gigabyte worth of Git objects"
>>     hardly seems worthwhile trade-off given the above. Disk space is cheap,
>>     developer time required to maintain two parallel build systems isn't.
>
> That rather assumes everyone has plenty of disk space and a decent
> network connection.

I realize that there's going to be a person who's got 1.5GB of disk
space and needs to delete a movie they've downloaded or whatever. I just
think that's worth it v.s. maintenance trade-off.

Doesn't just a Windows base installation need something in the tens of
GB of disk space these days? We're not really talking about embedded
systems.

Just the gcc etc. I've got on my Debian box is approaching a
GB. Whatever we think about other trade-offs optimizing for disk space
doesn't seem very compelling.

>> My attempt to amend/revert 4c2c38e800f to have it use the
>> pre-4c2c38e800f "make" invocation as a fallback failed, partially
>> because I don't have a Windows development environment, so any attempt
>> to change it is a painfully slow round-trip to GitHub CI.
>> Let's instead have CMake call out to the Makefile asking it what the
>> definition of various variables lists is, rather than being forced to
>> maintain those lists in a way that CMake can parse with regexes (which
>> precludes anything but a giant hardcoded list).
>> I could familiarize myself enough with CMake to do this in some
>> CMake-native way, but that would take "just as long as adding it to
>> the Makefile"[2] (I think that took me <5 minutes, but I'm several
>> hours into fighting with CMake)
>> So I consider this both a bugfix to the stated aims of this CMake
>> integration, and a better way forward for having an alternate build
>> system. I.e. If someone really does care about a having a
>> GNU-make-less dependency for the "vs-build" I think this change offers
>> a much better way forward for that.
>
> I don't see how relying on GNU-make is a step forward for the CMake
> integration when it works without it now.

It's not if we assume we've got infinite man hours to maintain these
systems, but we don't.

I have some pending patches to make various common cases when using make
much better, mainly speeding up no-op runs so things in rebase --exec go
faster.

So far I've been submitting the parts of that that don't step on the
toes of this cmake integration, and realistically if I've got to
implement everything in lockstep in two systems I'll probably just give
up on it.

Hence asking if there's some middle ground we can find here.

So you don't want to install "make" on Windows, but how about if we had
a script in contrib/ that generated these extractions of lists from the
Makefile instead of doing it on the fly, we could even commit those to
the repo.

Then I'd effectively get what I'm aiming for here, and cmake users could
just re-run that script, and if one of them did they could push the
result somewhere, and others could just fetch the generated assets.

> Overall I'm don't think that moving from a known set of dependencies
> to "build whatever C files are lying around in this directory" is an 
> improvement.

The caveat you note above with "foo.safe.c" is something we've got
already, see various "$(wildcard)", "find" and "git ls-files" in the
Makefile. So this way we'll at least be consistent. Now we'll add stray
files to TAGS, apply "coccicheck" to them etc.

So one thing I was aiming for here was closing that gap.

I do tihnk having a well understood hierarchy does help a lot, since you
can know that t/helpers/*.c is always one sort of thing etc.

> [1] The CMake integration is currently broken for non-windows builds,
> I've got some fixes at 
> https://github.com/phillipwood/git/tree/wip/cmake-fixes

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

[Phillip Wood]

On 06/11/2021 14:27, Ævar Arnfjörð Bjarmason wrote:
> 
> On Sat, Nov 06 2021, Phillip Wood wrote:
> 
>> Hi Ævar
>>
>> On 01/11/2021 19:19, Ævar Arnfjörð Bjarmason wrote:
>>> Remove the hardcoded lists of objects in favor of using
>>> $(wildcard). This means that every time a built-in, test tool etc. is
>>> added we won't need to patch the top-level Makefile, except for the
>>> few remaining cases where the asset in question would make it onto one
>>> of our list of exceptions.
>>> Ever since 81b50f3ce40 (Move 'builtin-*' into a 'builtin/'
>>> subdirectory, 2010-02-22) this has been relatively easy to do (and
>>> even before that we could glob builtin-*.c). This pattern of
>>> exhaustively enumerating files was then carried forward for
>>> e.g. TEST_BUILTINS_OBJS in efd71f8913a (t/helper: add an empty
>>> test-tool program, 2018-03-24).
>>> One reason not to do this is that now a new *.c file at the
>>> top-level
>>> will be immediately picked up, so if a new *.c file is being worked on
>>> "make" will error if it doesn't compile, whereas before that file
>>> would need to be explicitly listed in the Makefile. I think than small
>>> trade-off is worth it.
>>
>> If I need to split up some uncommitted changes into several commits
>> and I know it is going to be fiddly to do so I will sometimes copy the
>> original file to foo.safe.c and then edit foo.c to create each
>> commit. Then I can easily compile and test each commit and editing the
>> file directly is often easier than using add -p and editing the
>> hunks. With this patch running make will fail in that case I think.
> 
> Would being able to do something like:
> 
>      make EXCLUDE_WILDCARD=foo.safe.c
> 
> Satisfy this use-case?
> 
> 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.

>>> 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".
>>> There's a few small "while we're at it" changes here, since I'm
>>> touching the code in question:
>>>    - Start splitting up the the "Guard against the environment"
>>> section
>>>      at the top, but don't move anything that exists there out to avoid
>>>      merge conflicts
>>>    - The $(TEST_BUILTINS_OBJS) variable was needlessly complex,
>>> because
>>>      it didn't have the full paths we'd pathsubst it back & forth.
>>>    - Introduce *_SRC in addition to *_OBJ for the variable I'm
>>>      touching. Eventually we'll want to do this for all the *.o files,
>>>      i.e. make the *.c list a source of truth for *.o, which means we can
>>>      e.g. use that exhaustive list for "make TAGS".
>>>    - Add a missing "curl-objs" target. See 029bac01a87 (Makefile: add
>>>      {program,xdiff,test,git,fuzz}-objs & objects targets, 2021-02-23)
>>>      for the commit that added the rest.
>>>    - De-indent an "ifndef" block, we don't usually indent their
>>>      contents.
>>> On the CMake changes here:
>>>    - When CMake support was introduced in was introduced
>>>      061c2240b1b (Introduce CMake support for configuring Git, 2020-06-12)
>>>      there was a discussion about the maintenance burden of maintaining the
>>>      top-level Makefile in parallel with CMakeLists.txt[1] where reviewers
>>>      were assured that doing so would simply be a matter of adding something
>>>      to a list in the CMake recipe.
>>>      Between change and some recent changes of mine where the
>>> "vs-build"
>>>      job failed to a divergence between the Makefile and CMakeList.txt I
>>>      can confidently say that that doesn't at all match reality. Even
>>>      seemingly trivial changes to the Makefile like this one are forcing
>>>      us to do a deep-dive into CMake internals to make forward progress
>>>      with our main build system.
>>
>> My recollection is that the discussions were about not having to touch
>> CMakeList.txt when adding new files to the build and I think that
>> largely works. I don't think a lot of the changes you have been making
>> recently were anticipated in that discussion.
> 
> That's part of it, but the concern about needing to maintain two systems
> in perpetuity was also brought up, and it not being a hard dependency,
> having that vs-{build,test} job soft-fail on it etc. were brought up but
> that's not what we've got now.
> 
> In any case, having to maintain two build systems and the maintainer(s)
> of the CMake being inactive one is the situation we're in now.
> 
>>>    - The promised "We can add a (continue-on-error) to vs-build job to
>>>      make this process less of a hindrance." in [2] never materialized.
>>>      Since 4c2c38e800f (ci: modification of main.yml to use cmake for
>>>      vs-build job, 2020-06-26) got a hard dependency on CMake as far as
>>>      getting the CI to pass goes.
>>>    - The "vs-build" CI doesn't actually require that there be no GNU
>>> make
>>>      usage in the job, as it itself has a hard dependency on running a
>>>      "make -n artifacts-tar" command. So as far as any vs-specific special-sauce
>>>      goes we don't need a GNU-make free build system for vs-build.
>>
>> We need GNU-make for the ci job but an individual developer using
>> CMake does not need GNU-make installed. On linux it is possible to
>> build git without having make installed by using cmake and ninja [1]
> 
> I'd tried that, but didn't know about your [1] topic. If you or someone
> is actively willing to fix things up in CMake....

I only started working on that a few days ago. I was interested to see 
if using ninja was faster for building when just a few files have been 
changed but it doesn't make much difference unless absolutely nothing 
has changed - I guess we don't have that many files in the grand scheme 
of things. The CMake support still lags the Makefile as currently things 
like GIT-VERSION-FILE and git-hooks.h are not regenerated when their 
inputs change. I'm not sure if I want to commit to doing much more on 
that at the moment.

>>>    - The stated goal in 061c2240b1b of avoiding a GNU make dependency
>>>      for developer because it requires an SDK that "occupies around two
>>>      gigabytes" and "three quarters of a gigabyte worth of Git objects"
>>>      hardly seems worthwhile trade-off given the above. Disk space is cheap,
>>>      developer time required to maintain two parallel build systems isn't.
>>
>> That rather assumes everyone has plenty of disk space and a decent
>> network connection.
> 
> I realize that there's going to be a person who's got 1.5GB of disk
> space and needs to delete a movie they've downloaded or whatever. I just
> think that's worth it v.s. maintenance trade-off.
> 
> Doesn't just a Windows base installation need something in the tens of
> GB of disk space these days? We're not really talking about embedded
> systems.
> 
> Just the gcc etc. I've got on my Debian box is approaching a
> GB. Whatever we think about other trade-offs optimizing for disk space
> doesn't seem very compelling.
> 
>>> My attempt to amend/revert 4c2c38e800f to have it use the
>>> pre-4c2c38e800f "make" invocation as a fallback failed, partially
>>> because I don't have a Windows development environment, so any attempt
>>> to change it is a painfully slow round-trip to GitHub CI.
>>> Let's instead have CMake call out to the Makefile asking it what the
>>> definition of various variables lists is, rather than being forced to
>>> maintain those lists in a way that CMake can parse with regexes (which
>>> precludes anything but a giant hardcoded list).
>>> I could familiarize myself enough with CMake to do this in some
>>> CMake-native way, but that would take "just as long as adding it to
>>> the Makefile"[2] (I think that took me <5 minutes, but I'm several
>>> hours into fighting with CMake)
>>> So I consider this both a bugfix to the stated aims of this CMake
>>> integration, and a better way forward for having an alternate build
>>> system. I.e. If someone really does care about a having a
>>> GNU-make-less dependency for the "vs-build" I think this change offers
>>> a much better way forward for that.
>>
>> I don't see how relying on GNU-make is a step forward for the CMake
>> integration when it works without it now.
> 
> It's not if we assume we've got infinite man hours to maintain these
> systems, but we don't.
> 
> I have some pending patches to make various common cases when using make
> much better, mainly speeding up no-op runs so things in rebase --exec go
> faster.
> 
> So far I've been submitting the parts of that that don't step on the
> toes of this cmake integration, and realistically if I've got to
> implement everything in lockstep in two systems I'll probably just give
> up on it.
> 
> Hence asking if there's some middle ground we can find here.

What is it about listing the input files explicitly that slows things 
down? Surely it's faster than globbing the filesystem.

> So you don't want to install "make" on Windows, but how about if we had
> a script in contrib/ that generated these extractions of lists from the
> Makefile instead of doing it on the fly, we could even commit those to
> the repo.

If I understand there would be some make rule that generates a list of 
dependencies and we'd commit that list to the repo and consume it in 
CMakeLists.txt. I'd be fine with that

> Then I'd effectively get what I'm aiming for here, and cmake users could
> just re-run that script, and if one of them did they could push the
> result somewhere, and others could just fetch the generated assets.
> 
>> Overall I'm don't think that moving from a known set of dependencies
>> to "build whatever C files are lying around in this directory" is an
>> improvement.
> 
> The caveat you note above with "foo.safe.c" is something we've got
> already, see various "$(wildcard)", "find" and "git ls-files" in the
> Makefile. So this way we'll at least be consistent. Now we'll add stray
> files to TAGS, apply "coccicheck" to them etc.

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.

Best Wishes

Phillip

> So one thing I was aiming for here was closing that gap.
> 
> I do tihnk having a well understood hierarchy does help a lot, since you
> can know that t/helpers/*.c is always one sort of thing etc.
> 
>> [1] The CMake integration is currently broken for non-windows builds,
>> I've got some fixes at
>> https://github.com/phillipwood/git/tree/wip/cmake-fixes
> 

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

[Ævar Arnfjörð Bjarmason]

On Sat, Nov 06 2021, Phillip Wood wrote:

> On 06/11/2021 14:27, Ævar Arnfjörð Bjarmason wrote:
>> On Sat, Nov 06 2021, Phillip Wood wrote:
>> 
>>> Hi Ævar
>>>
>>> On 01/11/2021 19:19, Ævar Arnfjörð Bjarmason wrote:
>>>> Remove the hardcoded lists of objects in favor of using
>>>> $(wildcard). This means that every time a built-in, test tool etc. is
>>>> added we won't need to patch the top-level Makefile, except for the
>>>> few remaining cases where the asset in question would make it onto one
>>>> of our list of exceptions.
>>>> Ever since 81b50f3ce40 (Move 'builtin-*' into a 'builtin/'
>>>> subdirectory, 2010-02-22) this has been relatively easy to do (and
>>>> even before that we could glob builtin-*.c). This pattern of
>>>> exhaustively enumerating files was then carried forward for
>>>> e.g. TEST_BUILTINS_OBJS in efd71f8913a (t/helper: add an empty
>>>> test-tool program, 2018-03-24).
>>>> One reason not to do this is that now a new *.c file at the
>>>> top-level
>>>> will be immediately picked up, so if a new *.c file is being worked on
>>>> "make" will error if it doesn't compile, whereas before that file
>>>> would need to be explicitly listed in the Makefile. I think than small
>>>> trade-off is worth it.
>>>
>>> If I need to split up some uncommitted changes into several commits
>>> and I know it is going to be fiddly to do so I will sometimes copy the
>>> original file to foo.safe.c and then edit foo.c to create each
>>> commit. Then I can easily compile and test each commit and editing the
>>> file directly is often easier than using add -p and editing the
>>> hunks. With this patch running make will fail in that case I think.
>> Would being able to do something like:
>>      make EXCLUDE_WILDCARD=foo.safe.c
>> Satisfy this use-case?
>> 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.

Yes we could use the $(wildcard) in a tarball, the issue of picking up
untracked files is a dev-checkout-only issue, no?

>>> [...]
>>> My recollection is that the discussions were about not having to touch
>>> CMakeList.txt when adding new files to the build and I think that
>>> largely works. I don't think a lot of the changes you have been making
>>> recently were anticipated in that discussion.
>> That's part of it, but the concern about needing to maintain two
>> systems
>> in perpetuity was also brought up, and it not being a hard dependency,
>> having that vs-{build,test} job soft-fail on it etc. were brought up but
>> that's not what we've got now.
>> In any case, having to maintain two build systems and the
>> maintainer(s)
>> of the CMake being inactive one is the situation we're in now.
>> 
>>>>    - The promised "We can add a (continue-on-error) to vs-build job to
>>>>      make this process less of a hindrance." in [2] never materialized.
>>>>      Since 4c2c38e800f (ci: modification of main.yml to use cmake for
>>>>      vs-build job, 2020-06-26) got a hard dependency on CMake as far as
>>>>      getting the CI to pass goes.
>>>>    - The "vs-build" CI doesn't actually require that there be no GNU
>>>> make
>>>>      usage in the job, as it itself has a hard dependency on running a
>>>>      "make -n artifacts-tar" command. So as far as any vs-specific special-sauce
>>>>      goes we don't need a GNU-make free build system for vs-build.
>>>
>>> We need GNU-make for the ci job but an individual developer using
>>> CMake does not need GNU-make installed. On linux it is possible to
>>> build git without having make installed by using cmake and ninja [1]
>> I'd tried that, but didn't know about your [1] topic. If you or
>> someone
>> is actively willing to fix things up in CMake....
>
> I only started working on that a few days ago. I was interested to see
> if using ninja was faster for building when just a few files have been 
> changed but it doesn't make much difference unless absolutely nothing
> has changed - I guess we don't have that many files in the grand
> scheme of things. The CMake support still lags the Makefile as
> currently things like GIT-VERSION-FILE and git-hooks.h are not
> regenerated when their inputs change. I'm not sure if I want to commit
> to doing much more on that at the moment.

I suspect that any other build system is faster because it doesn't
actually do what our Makefile does, which is something I've been trying
to slowly fix.

E.g. getting rid of the FORCE dependencies we've got. I submitted a
series just now to do some prep work for that:
https://lore.kernel.org/git/cover-00.16-00000000000-20211106T205717Z-avarab@gmail.com/

The performance of that:

0 $ hyperfine --warmup 3 -L v master,next,avar/Makefile-perl-python-shell-quote-various-fixes,avar/Makefile-get-rid-of-FORCE-rules-for-gmake-4.2-dependency -p 'git checkout {v} && make -j8 all' 'echo {v} && make -j8 all'
Benchmark #1: echo master && make -j8 all
  Time (mean ± σ):     127.7 ms ±  40.8 ms    [User: 111.5 ms, System: 17.4 ms]
  Range (min … max):   113.2 ms … 243.8 ms    10 runs
 
options.
 
Benchmark #2: echo next && make -j8 all
  Time (mean ± σ):     114.9 ms ±   0.9 ms    [User: 105.6 ms, System: 23.1 ms]
  Range (min … max):   113.5 ms … 116.3 ms    11 runs
 
Benchmark #3: echo avar/Makefile-perl-python-shell-quote-various-fixes && make -j8 all
  Time (mean ± σ):     121.6 ms ±  16.8 ms    [User: 107.9 ms, System: 22.2 ms]
  Range (min … max):   113.6 ms … 167.4 ms    10 runs
 
Benchmark #4: echo avar/Makefile-get-rid-of-FORCE-rules-for-gmake-4.2-dependency && make -j8 all
  Time (mean ± σ):      71.4 ms ±  17.2 ms    [User: 61.7 ms, System: 11.1 ms]
  Range (min … max):    64.2 ms … 131.9 ms    15 runs
 
Summary
  'echo avar/Makefile-get-rid-of-FORCE-rules-for-gmake-4.2-dependency && make -j8 all' ran
    1.61 ± 0.39 times faster than 'echo next && make -j8 all'
    1.70 ± 0.47 times faster than 'echo avar/Makefile-perl-python-shell-quote-various-fixes && make -j8 all'
    1.79 ± 0.72 times faster than 'echo master && make -j8 all'

I.e. so far I haven't really optimized anything much, but the next step
after that series above speeds it up by almost 2x for a noop run, and
there's other low-hanging fruit after that that I've got in some WIP
code somewhere. I think I managed to get it down to the 10-20ms range
for noop runs in one of those.

>>> [...]
>>>> My attempt to amend/revert 4c2c38e800f to have it use the
>>>> pre-4c2c38e800f "make" invocation as a fallback failed, partially
>>>> because I don't have a Windows development environment, so any attempt
>>>> to change it is a painfully slow round-trip to GitHub CI.
>>>> Let's instead have CMake call out to the Makefile asking it what the
>>>> definition of various variables lists is, rather than being forced to
>>>> maintain those lists in a way that CMake can parse with regexes (which
>>>> precludes anything but a giant hardcoded list).
>>>> I could familiarize myself enough with CMake to do this in some
>>>> CMake-native way, but that would take "just as long as adding it to
>>>> the Makefile"[2] (I think that took me <5 minutes, but I'm several
>>>> hours into fighting with CMake)
>>>> So I consider this both a bugfix to the stated aims of this CMake
>>>> integration, and a better way forward for having an alternate build
>>>> system. I.e. If someone really does care about a having a
>>>> GNU-make-less dependency for the "vs-build" I think this change offers
>>>> a much better way forward for that.
>>>
>>> I don't see how relying on GNU-make is a step forward for the CMake
>>> integration when it works without it now.
>> It's not if we assume we've got infinite man hours to maintain these
>> systems, but we don't.
>> I have some pending patches to make various common cases when using
>> make
>> much better, mainly speeding up no-op runs so things in rebase --exec go
>> faster.
>> So far I've been submitting the parts of that that don't step on the
>> toes of this cmake integration, and realistically if I've got to
>> implement everything in lockstep in two systems I'll probably just give
>> up on it.
>> Hence asking if there's some middle ground we can find here.
>
> What is it about listing the input files explicitly that slows things
> down? Surely it's faster than globbing the filesystem.

It doesn't slow things down. I'd like to get rid of these big hardcoded
lists to make it easier to add new [test] commands, anything that cuts
down on the "modify these N files, copy/pasting the last similar commit"
helps with that.

And we've got churn related to these lists being unsorted, being
re-sorted, and occasional tiresome merge conflicts etc.

The initial reason I finished this up was because I had a WIP patch to
add a "sort assertion" for these lists, per the discussion here:
https://lore.kernel.org/git/87mtv2dk18.fsf@evledraar.gmail.com/

But I realized instead of doing that we could just use $(wildcard)
instead.

>> So you don't want to install "make" on Windows, but how about if we had
>> a script in contrib/ that generated these extractions of lists from the
>> Makefile instead of doing it on the fly, we could even commit those to
>> the repo.
>
> If I understand there would be some make rule that generates a list of
> dependencies and we'd commit that list to the repo and consume it in 
> CMakeLists.txt. I'd be fine with that

Ok that certainly helps & is a viable workaround.

>> Then I'd effectively get what I'm aiming for here, and cmake users could
>> just re-run that script, and if one of them did they could push the
>> result somewhere, and others could just fetch the generated assets.
>> 
>>> Overall I'm don't think that moving from a known set of dependencies
>>> to "build whatever C files are lying around in this directory" is an
>>> improvement.
>> The caveat you note above with "foo.safe.c" is something we've got
>> already, see various "$(wildcard)", "find" and "git ls-files" in the
>> Makefile. So this way we'll at least be consistent. Now we'll add stray
>> files to TAGS, apply "coccicheck" to them etc.
>
> 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.

I would like to get rid of those ls-files or whatever globs entirely,
whether that's by moving everything to wildcards or having them consume
the hardcoded lists doesn't really matter.

But it's a a wart that we're e.g. running coccicheck on one globbed set
of files, and compile a hardcoded list of files. Partially it's because
we conflate "list of known files" with "here's what we'd like to compile
on this platform".

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> 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.

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

[Johannes Schindelin]

Hi Junio,

On Tue, 9 Nov 2021, Junio C Hamano wrote:

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

If you ever needed another opinion to back you up on this: I fully agree.

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

Indeed, the ability to build from a `.tar` extract is important. That's
why we were careful to use `ls-files` in `LIB_H` and in
`FIND_SOURCE_FILE`, falling back on using `find` if the `ls-files` call
failed.

And to be honest, even `LIB_H` and `FIND_SOURCE_FILE` would quite
potentially better be hard-coded (with a CI check to ensure that they're
up to date).

Ciao,
Dscho

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

[Ævar Arnfjörð Bjarmason]

On Wed, Nov 10 2021, Johannes Schindelin wrote:

> Hi Junio,
>
> On Tue, 9 Nov 2021, Junio C Hamano wrote:
>
>> 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.
>
> If you ever needed another opinion to back you up on this: I fully agree.

I could go either way on that, but in terms of Makefile maintenance it
does suck a lot less to pick one or the other.

A (I realize, unstated) eventual goal I had was to move these wildcard
declarations to some common list you can include from various Makefiles,
currently we've got dependency bugs in e.g. Makefile &
Documentation/Makefile interaction.

If we're not OK with $(wildcard) as a pattern that would mean changing
all of these to hardcoded (in some cases quite big) lists somewhere:
    
    $ git -P grep -E '^[^~]+\$\(wildcard.+\*' ':!git-gui' ':!gitk-git' ':!contrib'
    Documentation/Makefile:         $(wildcard git-*.txt))
    Documentation/Makefile:HOWTO_TXT += $(wildcard howto/*.txt)
    Documentation/Makefile:DOC_DEP_TXT += $(wildcard *.txt)
    Documentation/Makefile:DOC_DEP_TXT += $(wildcard config/*.txt)
    Documentation/Makefile:API_DOCS = $(patsubst %.txt,%,$(filter-out technical/api-index-skel.txt technical/api-index.txt, $(wildcard technical/api-*.txt)))
    Documentation/Makefile:mergetools-list.made: ../git-mergetool--lib.sh $(wildcard ../mergetools/*)
    Documentation/Makefile:%.1 %.5 %.7 : %.xml manpage-base-url.xsl $(wildcard manpage*.xsl)
    Makefile:command-list.h: $(wildcard Documentation/git*.txt)
    Makefile:POFILES := $(wildcard po/*.po)
    Makefile:LIB_PERL := $(wildcard perl/Git.pm perl/Git/*.pm perl/Git/*/*.pm perl/Git/*/*/*.pm)
    Makefile:LIB_CPAN := $(wildcard perl/FromCPAN/*.pm perl/FromCPAN/*/*.pm)
    Makefile:coccicheck: $(addsuffix .patch,$(filter-out %.pending.cocci,$(wildcard contrib/coccinelle/*.cocci)))
    Makefile:coccicheck-pending: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.pending.cocci))
    t/Makefile:T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))
    t/Makefile:TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))
    t/Makefile:THELPERS = $(sort $(filter-out $(T),$(wildcard *.sh)))
    t/Makefile:TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
    t/Makefile:CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
    t/interop/Makefile:T = $(sort $(wildcard i[0-9][0-9][0-9][0-9]-*.sh))

What do you & Junio think about that?

I don't really mind either way, as long as I stop running into
occasional bugs where I need to run "git clean -dxf" because the
Makeefile was too stupid to properly manage its dependencies.

>> 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.
>
> Indeed, the ability to build from a `.tar` extract is important. That's
> why we were careful to use `ls-files` in `LIB_H` and in
> `FIND_SOURCE_FILE`, falling back on using `find` if the `ls-files` call
> failed.

Why would you need any of that to *build* from a .tar extract? I think
we should remove that LIB_H thing entirely.

Its only purpose is to support someone who:

 1. Wants to do an *incremental* build, not a "build from tar". I.e. you
    build already, changed a header, and now you want to not over-build
    again.

    Your compiler is perfectly capable of locating headers in an -I dir
    for you.

 2. Doesn't have gcc or clang installed. Note "installed", not to
    build.

    Well, currently we require you to build with those to use .depends &
    COMPUTE_HEADER_DEPENDENCIES, but that's an easily fixable
    implementation detail.

    We can easily make the .depend files with gcc/clang and build with
    another compiler. I had a 5-10 line local change at some point to do
    that.

 3. Doesn't find it acceptable to have a fallback of just a glob like
    "**.h" for that "depends" target.

    I.e. we'd over-rebuild if you dropped in a new *.h we're not
    actually using into your extracted tarball, but really, who cares?

 4. Wants to run "make hdr-check" or "make pot", both of which I think
    are OK to say "you need to run this on a box that has .depends (or
    in the case of *.pot, we can use a greedier glob).

> And to be honest, even `LIB_H` and `FIND_SOURCE_FILE` would quite
> potentially better be hard-coded (with a CI check to ensure that they're
> up to date).

That would be a bug, just because I don't build on Windows doesn't mean
that I wouldn't like "make TAGS coccicheck" to find compat/win32/ at
all.

It doesn't do that now for a different reason, but that's a bug that
should be fixed.

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]

Hi Ævar,

On Wed, 10 Nov 2021, Ævar Arnfjörð Bjarmason wrote:

> If we're not OK with $(wildcard) as a pattern that would mean changing
> all of these to hardcoded (in some cases quite big) lists somewhere:
>
>     [...]

No, it would only mean changing these instances if we have a concrete
need. I fail to see a concrete need.

That does not mean that we should make the situation even worse by
converting currently hard-coded lists to wildcards. There is, once again,
no concrete need for that, and there is the good reason Junio brought up
against such a churn: it is too sloppy.

> [...] I think we should remove that LIB_H thing entirely.

I think we should take a break from refactoring code where it is unclear
what purpose the refactoring serves.

> > And to be honest, even `LIB_H` and `FIND_SOURCE_FILE` would quite
> > potentially better be hard-coded (with a CI check to ensure that
> > they're up to date).
>
> That would be a bug, just because I don't build on Windows doesn't mean
> that I wouldn't like "make TAGS coccicheck" to find compat/win32/ at
> all.

Talking about `coccicheck` in the context of the discussion whether we
should make sweeping changes in our Makefiles, all while we're supposedly
in the -rc phase, strikes me as a distant tangent of a distant tangent.

Ciao,
Johannes

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

[Ævar Arnfjörð Bjarmason]

On Wed, Nov 10 2021, Johannes Schindelin wrote:

> Hi Ævar,
>
> On Wed, 10 Nov 2021, Ævar Arnfjörð Bjarmason wrote:
>
>> If we're not OK with $(wildcard) as a pattern that would mean changing
>> all of these to hardcoded (in some cases quite big) lists somewhere:
>>
>>     [...]
>
> No, it would only mean changing these instances if we have a concrete
> need. I fail to see a concrete need.
>
> That does not mean that we should make the situation even worse by
> converting currently hard-coded lists to wildcards. There is, once again,
> no concrete need for that, and there is the good reason Junio brought up
> against such a churn: it is too sloppy.

I don't it's sloppy. It's just a different approach. It's also an
approach we use now. Add a new build in and your addition in t/*.sh and
Documentation/*.txt will be picked up & built. We just won't pick up the
*.h or builtin/*.c implicitly.

So whether we need to do this now is one thing, but saying it's a big
change in workflow seems to be rather exaggerated.

>> [...] I think we should remove that LIB_H thing entirely.
>
> I think we should take a break from refactoring code where it is unclear
> what purpose the refactoring serves.

I'm not advocating ripping LIB_H out right now, and have not submitted
any patches to do so.

I'm asking you a follow-up question about your claim that LIB_H is
needed for building from a tarball. I don't think it is, but perhaps I'm
missing something.

It would be useful to get an answer to that for the list records, so
that while it's fresh in your mind we can get an answer one way or the
other.

I agree there's no a strong reason to change it now, but being able to
do so in the future might be useful.

At that point someone will probably dig up this thread. Whether "do we
need LIB_H for what Johannes suggested?" is a dead end or not I'll leave
to you.

>> > And to be honest, even `LIB_H` and `FIND_SOURCE_FILE` would quite
>> > potentially better be hard-coded (with a CI check to ensure that
>> > they're up to date).
>>
>> That would be a bug, just because I don't build on Windows doesn't mean
>> that I wouldn't like "make TAGS coccicheck" to find compat/win32/ at
>> all.
>
> Talking about `coccicheck` in the context of the discussion whether we
> should make sweeping changes in our Makefiles, all while we're supposedly
> in the -rc phase, strikes me as a distant tangent of a distant tangent.

For the past few releases I think I've probably submitted more
last-minute rc fixes than most. In my case it's mostly a matter of
starting some builds and waiting for them to complete:
https://xkcd.com/303/

So yeah, I think we should focus around release time, but saying that
any other ongoing discussion is a needless distraction seems like a
bridge too far.

That being said the reason I'm submitting these sorts of fixes now is
directly related to making rc testing easier.

I test on some obnoxiously slow VMs or otherwise limited computers on
the GCC farm. When something breaks between releases having each step of
a bisect take 30m or 60m makes a big difference.

So having a Makefile that doesn't over-build stuff is important to
release testing.

But getting that across has been frustrating at times. I've pretty much
stopped testing on AIX because my few-lines of patches to the Makefile
to make that drastically easier were categorically rejected. The
response to some other things like over-building <xyz> has been
somewhere between "I don't see why you care, my computer is fast enough"
and "why don't you port ccache to <90s era *nix OS that barely compiles
Hello World without issues".

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

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 09 2021, Junio C Hamano wrote:

> Phillip Wood <phillip.wood123@gmail.com> 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.

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

[Phillip Wood]

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@gmail.com> 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

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

[Ævar Arnfjörð Bjarmason]

On Fri, Jan 21 2022, Phillip Wood wrote:

> 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@gmail.com> 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.

The difference is that the above is a "good" breakage, i.e. one where we
ended up carrying dead code in-tree, so it wouldn't have escaped the
lab.

Whereas I think that you're concerned that adding a WIP foobar.c to the
tree would break your compilation the next time you invoke 'make'.

Would something like this way of having our cake and eating it too
address your concerns?

I.e. if we have 'git' installed already we could just ask it about any
untracked files, and filter those out:
    
    ifndef NO_WILDCARD_SECOND_GUESSING
    UNTRACKED_FILES := $(shell git status --porcelain=1 2>/dev/null | sed -n -e '/^\?? / { s/^.. //; p}')
    else
    UNTRACKED_FILES =
    endif
    
    REFTABLE_SOURCES = $(filter-out $(UNTRACKED_FILES),$(wildcard reftable/*.c))
    REFTABLE_OBJS += $(filter-out test,$(REFTABLE_SOURCES:%.c=%.o))
    REFTABLE_TEST_OBJS += $(filter test,$(REFTABLE_SOURCES:%.c=%.o))

A non-demo implementation of this would piggy-back on the "git ls-files"
shell-out we already do, but since that requires some refactoring I
thought it would be better to demo it like this.

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

[Junio C Hamano]

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

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

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