scalar: integrate into core Git

This series completes the initial implementation of the Scalar command as a core component of Git. For readers new to the topic of Scalar, the roadmap/design doc [1] provides some background information including how the project started & evolved, as well as its current intent & high-level design.

Goals
=====

The goal of this series is, as noted in the roadmap linked earlier, to integrate Scalar as a core user-facing component of Git. There's no individual existing precedent that the integration is 100% based on; it's more of an amalgam of the approaches used with builtins installed directly to the bindir (e.g. 'git-upload-pack'), contributed tools (e.g. 'gitk'), and the 'git' executable itself. The resulting behavior is:

* Scalar is built & installed unconditionally (like a builtin).
* Scalar is included in 'bin-wrappers/' and is installed into the 'bin/' directory (like 'git-upload-pack').
* Scalar's documentation is installed as 'man1' docs (like 'gitk').
* Scalar is not invoked via the 'git' executable (it's called directly, like 'git' itself).

The move out of 'contrib/' is also intended to mean that the relationship between Git and Scalar should no longer be a strict one-way dependency. In this series, this architectural change is what permits making a change to 'help.c' specifically for the sake of Scalar. Future contributions can and should (where appropriate!) share code between Scalar and non-Scalar parts of Git.

Organization
============

The series is broken up as follows:

1. Fix a CI-breaking documentation error in 'scalar.txt' (patch 1).
2. Move Scalar code out of 'contrib/' and into core Git, including changes in build definition(s) to build & install Scalar and its resources (patch 2).
3. Make 'git help scalar' open the 'scalar' command documentation, introduce 'scalar help' as an alias. This is done after moving out of 'contrib/' due to it involving Scalar-related changes to the non-Scalar file 'help.c' (patches 3-4).
4. Expand test coverage, including perf testing (patches 5-6)
5. Add a perf test configuration allowing users to register standard perf test repos with Scalar (patch 7).
6. Remove the internally-focused Scalar roadmap from 'Documentation/technical/scalar.txt', include it as one of the HTML technical docs (patch 8).

Prior series
============

* Add 'scalar' command in 'contrib/':
https://lore.kernel.org/git/[email protected]/
* Introduce 'scalar diagnose':
https://lore.kernel.org/git/[email protected]/
* Add '-c/-C' compatibility:
https://lore.kernel.org/git/[email protected]/
* [DROPPED] Integrate Scalar into CI builds:
https://lore.kernel.org/git/[email protected]/
* Document Scalar's role in Git & plan remaining work:
https://lore.kernel.org/git/[email protected]/
* Generalize 'scalar diagnose' into 'git diagnose' builtin & 'git bugreport --diagnose':
https://lore.kernel.org/git/[email protected]/
* Add FSMonitor support to Scalar, refactor enlistment search:
https://lore.kernel.org/git/[email protected]/

Thanks!

* Victoria

[1]
https://lore.kernel.org/git/65eda9755d2a29e222bf7e6cc719921b24377cc6.1657584367.git.gitgitgadget@gmail.com/

Johannes Schindelin (2):
git help: special-case scalar
scalar: implement the help subcommand

Victoria Dye (6):
scalar: fix command documentation section header
scalar: include in standard Git build & installation
scalar-clone: add test coverage
t/perf: add Scalar performance tests
t/perf: add 'GIT_PERF_USE_SCALAR' run option Documentation/technical: include Scalar technical doc

 .gitignore                                    |   1 +
 Documentation/Makefile                        |   2 +
 {contrib/scalar => Documentation}/scalar.txt  |   2 +-
 Documentation/technical/scalar.txt            |  61 --------
 Makefile                                      |  30 ++--
 builtin/help.c                                |   2 +
 contrib/buildsystems/CMakeLists.txt           |   9 +-
 contrib/scalar/.gitignore                     |   2 -
 contrib/scalar/Makefile                       |  35 -----
 contrib/scalar/t/Makefile                     |  81 -----------
 contrib/scalar/scalar.c => scalar.c           |  20 +++
 t/perf/README                                 |   4 +
 t/perf/p9210-scalar.sh                        |  43 ++++++
 t/perf/perf-lib.sh                            |  13 +-
 t/perf/run                                    |   3 +
 .../t/t9099-scalar.sh => t/t9210-scalar.sh    |  10 +-
 t/t9211-scalar-clone.sh                       | 135 ++++++++++++++++++
 17 files changed, 249 insertions(+), 204 deletions(-)
 rename {contrib/scalar => Documentation}/scalar.txt (99%)
 delete mode 100644 contrib/scalar/.gitignore
 delete mode 100644 contrib/scalar/Makefile
 delete mode 100644 contrib/scalar/t/Makefile
 rename contrib/scalar/scalar.c => scalar.c (98%)
 create mode 100755 t/perf/p9210-scalar.sh
 rename contrib/scalar/t/t9099-scalar.sh => t/t9210-scalar.sh (96%)
 create mode 100755 t/t9211-scalar-clone.sh

base-commit: d42b38dfb5edf1a7fddd9542d722f91038407819
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1341%2Fvdye%2Ffeature%2Fscalar-toplevel-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1341/vdye/feature/scalar-toplevel-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1341

gitgitgadget

From: Victoria Dye <[email protected]>

Rename the last section header in 'contrib/scalar/scalar.txt' from "Scalar" to "GIT". The linting rules of the 'documentation' CI build enforce the existence of a "GIT" section in command documentation. Although 'scalar.txt' is not yet checked, it will be in a future patch.

Here, changing the header name is more appropriate than making a Scalar-specific exception to the linting rule. The existing "Scalar" section contains only a link back to the main Git documentation, essentially the same as the "GIT" section in builtin documentation. Changing the section name further clarifies the Scalar-Git association and maintains consistency with the rest of Git.

---
 contrib/scalar/scalar.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 1a12dc45077..505a1cea0fd 100644
@@ -161,6 +161,6 @@ SEE ALSO
 --------
 linkgit:git-clone[1], linkgit:git-maintenance[1].
 
-Scalar
+GIT
 ---
 Associated with the linkgit:git[1] suite

gitgitgadget

From: Johannes Schindelin <[email protected]>

With this commit, git help scalar will open the appropriate manual or HTML page (instead of looking for gitscalar).

---
 builtin/help.c | 2 ++
 1 file changed, 2 insertions(+)

index 09ac4289f13..6f2796f211e 100644
@@ -440,6 +440,8 @@ static const char *cmd_to_page(const char *git_cmd)
 		return git_cmd;
 	else if (is_git_command(git_cmd))
 		return xstrfmt("git-%s", git_cmd);
+	else if (!strcmp("scalar", git_cmd))
+		return xstrdup(git_cmd);
 	else
 		return xstrfmt("git%s", git_cmd);
 }

gitgitgadget

From: Victoria Dye <[email protected]>

Move 'scalar' out of 'contrib/' and into the root of the Git tree. The goal of this change is to build 'scalar' as part of the standard Git build & install processes.

This patch includes both the physical move of Scalar's files out of 'contrib/' ('scalar.c', 'scalar.txt', and 't9xxx-scalar.sh'), and the changes to the build definitions in 'Makefile' and 'CMakelists.txt' to accommodate the new program.

At a high level, Scalar is built so that:
- there is a 'scalar-objs' target (similar to those created in 029bac01a8 (Makefile: add {program,xdiff,test,git,fuzz}-objs & objects targets, 2021-02-23)) for debugging purposes.
- it appears in the root of the install directory (rather than the gitexecdir).
- it is included in the 'bin-wrappers/' directory for use in tests.
- it receives a platform-specific executable suffix (e.g., '.exe'), if applicable.
- 'scalar.txt' is installed as 'man1' documentation.
- the 'clean' target removes the 'scalar' executable.

Additionally, update the root level '.gitignore' file to ignore the Scalar executable.

---
 .gitignore                                    |  1 +
 Documentation/Makefile                        |  1 +
 {contrib/scalar => Documentation}/scalar.txt  |  0
 Makefile                                      | 30 ++++---
 contrib/buildsystems/CMakeLists.txt           |  9 ++-
 contrib/scalar/.gitignore                     |  2 -
 contrib/scalar/Makefile                       | 35 --------
 contrib/scalar/t/Makefile                     | 81 -------------------
 contrib/scalar/scalar.c => scalar.c           |  0
 .../t/t9099-scalar.sh => t/t9210-scalar.sh    | 10 +--
 10 files changed, 28 insertions(+), 141 deletions(-)
 rename {contrib/scalar => Documentation}/scalar.txt (100%)
 delete mode 100644 contrib/scalar/.gitignore
 delete mode 100644 contrib/scalar/Makefile
 delete mode 100644 contrib/scalar/t/Makefile
 rename contrib/scalar/scalar.c => scalar.c (100%)
 rename contrib/scalar/t/t9099-scalar.sh => t/t9210-scalar.sh (96%)

index 80b530bbed2..3d1b880101e 100644
@@ -185,6 +185,7 @@
 /git-whatchanged
 /git-worktree
 /git-write-tree
+/scalar
 /git-core-*/?*
 /git.res
 /gitweb/GITWEB-BUILD-OPTIONS

index bd6b6fcb930..16c9e062390 100644
@@ -21,6 +21,7 @@ MAN1_TXT += $(filter-out \
 MAN1_TXT += git.txt
 MAN1_TXT += gitk.txt
 MAN1_TXT += gitweb.txt
+MAN1_TXT += scalar.txt
 
 # man5 / man7 guides (note: new guides should also be added to command-list.txt)
 MAN5_TXT += gitattributes.txt

similarity index 100%
rename from contrib/scalar/scalar.txt
rename to Documentation/scalar.txt

index eac30126e29..66dd3321f57 100644
@@ -608,7 +608,9 @@ FUZZ_OBJS =
 FUZZ_PROGRAMS =
 GIT_OBJS =
 LIB_OBJS =
+SCALAR_OBJS =
 OBJECTS =
+OTHER_PROGRAMS =
 PROGRAM_OBJS =
 PROGRAMS =
 EXCLUDED_PROGRAMS =

@@ -821,10 +823,12 @@ BUILT_INS += git-switch$X
 BUILT_INS += git-whatchanged$X
 
 # what 'all' will build but not install in gitexecdir
-OTHER_PROGRAMS = git$X
+OTHER_PROGRAMS += git$X
+OTHER_PROGRAMS += scalar$X
 
 # what test wrappers are needed and 'install' will install, in bindir
 BINDIR_PROGRAMS_NEED_X += git
+BINDIR_PROGRAMS_NEED_X += scalar
 BINDIR_PROGRAMS_NEED_X += git-receive-pack
 BINDIR_PROGRAMS_NEED_X += git-shell
 BINDIR_PROGRAMS_NEED_X += git-upload-archive

@@ -2222,7 +2226,7 @@ profile-fast: profile-clean
 
 all:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(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';)
+	$(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) $(OTHER_PROGRAMS))), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';)
 endif
 
 all::

@@ -2545,7 +2549,12 @@ GIT_OBJS += git.o
 .PHONY: git-objs
 git-objs: $(GIT_OBJS)
 
+SCALAR_OBJS += scalar.o
+.PHONY: scalar-objs
+scalar-objs: $(SCALAR_OBJS)
+
 OBJECTS += $(GIT_OBJS)
+OBJECTS += $(SCALAR_OBJS)
 OBJECTS += $(PROGRAM_OBJS)
 OBJECTS += $(TEST_OBJS)
 OBJECTS += $(XDIFF_OBJS)

@@ -2556,10 +2565,6 @@ ifndef NO_CURL
 	OBJECTS += http.o http-walker.o remote-curl.o
 endif
 
-SCALAR_SOURCES := contrib/scalar/scalar.c
-SCALAR_OBJECTS := $(SCALAR_SOURCES:c=o)
-OBJECTS += $(SCALAR_OBJECTS)
-
 .PHONY: objects
 objects: $(OBJECTS)
 

@@ -2691,7 +2696,7 @@ $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)
 
-contrib/scalar/scalar$X: $(SCALAR_OBJECTS) GIT-LDFLAGS $(GITLIBS)
+scalar$X: scalar.o GIT-LDFLAGS $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
 		$(filter %.o,$^) $(LIBS)
 

@@ -2747,7 +2752,7 @@ XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
 XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
 	--keyword=__ --keyword=N__ --keyword="__n:1,2"
 MSGMERGE_FLAGS = --add-location --backup=off --update
-LOCALIZED_C = $(sort $(FOUND_C_SOURCES) $(FOUND_H_SOURCES) $(SCALAR_SOURCES) \
+LOCALIZED_C = $(sort $(FOUND_C_SOURCES) $(FOUND_H_SOURCES) \
 	        $(GENERATED_H))
 LOCALIZED_SH = $(sort $(SCRIPT_SH) git-sh-setup.sh)
 LOCALIZED_PERL = $(sort $(SCRIPT_PERL))

@@ -3062,7 +3067,7 @@ bin-wrappers/%: wrap-for-bin.sh
 	$(call mkdir_p_parent_template)
 	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 	     -e 's|@@BUILD_DIR@@|$(shell pwd)|' \
-	     -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%$(X),$(@F))$(patsubst git%,$(X),$(filter $(@F),$(BINDIR_PROGRAMS_NEED_X)))|' < $< > $@ && \
+	     -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%,$(@F))$(if $(filter-out $(BINDIR_PROGRAMS_NO_X),$(@F)),$(X),)|' < $< > $@ && \
 	chmod +x $@
 
 # GNU make supports exporting all variables by "export" without parameters.

@@ -3276,14 +3281,14 @@ ifndef NO_TCLTK
 	$(MAKE) -C git-gui gitexecdir='$(gitexec_instdir_SQ)' install
 endif
 ifneq (,$X)
-	$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) git$X)), test '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p' -ef '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p$X' || $(RM) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p';)
+	$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) $(OTHER_PROGRAMS))), test '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p' -ef '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p$X' || $(RM) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p';)
 endif
 
 	bindir=$$(cd '$(DESTDIR_SQ)$(bindir_SQ)' && pwd) && \
 	execdir=$$(cd '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' && pwd) && \
 	destdir_from_execdir_SQ=$$(echo '$(gitexecdir_relative_SQ)' | sed -e 's|[^/][^/]*|..|g') && \
 	{ test "$$bindir/" = "$$execdir/" || \
-	  for p in git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); do \
+	  for p in $(OTHER_PROGRAMS) $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); do \
 		$(RM) "$$execdir/$$p" && \
 		test -n "$(INSTALL_SYMLINKS)" && \
 		ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/$$p" "$$execdir/$$p" || \

@@ -3458,7 +3463,7 @@ clean: profile-clean coverage-clean cocciclean
 	$(RM) git.res
 	$(RM) $(OBJECTS)
 	$(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)
-	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
+	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS)
 	$(RM) $(TEST_PROGRAMS)
 	$(RM) $(FUZZ_PROGRAMS)
 	$(RM) $(SP_OBJ)

@@ -3509,6 +3514,7 @@ ALL_COMMANDS += git-citool
 ALL_COMMANDS += git-gui
 ALL_COMMANDS += gitk
 ALL_COMMANDS += gitweb
+ALL_COMMANDS += scalar
 
 .PHONY: check-docs
 check-docs::

index 2237109b57f..bae203c1fb9 100644
@@ -610,7 +610,7 @@ unset(CMAKE_REQUIRED_INCLUDES)
 #programs
 set(PROGRAMS_BUILT
 	git git-daemon git-http-backend git-sh-i18n--envsubst
-	git-shell)
+	git-shell scalar)
 
 if(NOT CURL_FOUND)
 	list(APPEND excluded_progs git-http-fetch git-http-push)

@@ -757,6 +757,9 @@ target_link_libraries(git-sh-i18n--envsubst common-main)
 add_executable(git-shell ${CMAKE_SOURCE_DIR}/shell.c)
 target_link_libraries(git-shell common-main)
 
+add_executable(scalar ${CMAKE_SOURCE_DIR}/scalar.c)
+target_link_libraries(scalar common-main)
+
 if(CURL_FOUND)
 	add_library(http_obj OBJECT ${CMAKE_SOURCE_DIR}/http.c)
 

@@ -903,7 +906,7 @@ list(TRANSFORM git_perl_scripts PREPEND "${CMAKE_BINARY_DIR}/")
 
 #install
 foreach(program ${PROGRAMS_BUILT})
-if(program STREQUAL "git" OR program STREQUAL "git-shell")
+if(program MATCHES "^(git|git-shell|scalar)$")
 install(TARGETS ${program}
 	RUNTIME DESTINATION bin)
 else()

@@ -977,7 +980,7 @@ endif()
 
 #wrapper scripts
 set(wrapper_scripts
-	git git-upload-pack git-receive-pack git-upload-archive git-shell git-remote-ext)
+	git git-upload-pack git-receive-pack git-upload-archive git-shell git-remote-ext scalar)
 
 set(wrapper_test_scripts
 	test-fake-ssh test-tool)

deleted file mode 100644
index ff3d47e84d0..00000000000
@@ -1,2 +0,0 @@
-/*.exe
-/scalar

deleted file mode 100644
index 37f283f35d7..00000000000
@@ -1,35 +0,0 @@
-# The default target of this Makefile is...
-all::
-
-# Import tree-wide shared Makefile behavior and libraries
-include ../../shared.mak
-
-include ../../config.mak.uname
--include ../../config.mak.autogen
--include ../../config.mak
-
-TARGETS = scalar$(X) scalar.o
-GITLIBS = ../../common-main.o ../../libgit.a ../../xdiff/lib.a
-
-all:: scalar$(X) ../../bin-wrappers/scalar
-
-$(GITLIBS):
-	$(QUIET_SUBDIR0)../.. $(QUIET_SUBDIR1) $(subst ../../,,$@)
-
-$(TARGETS): $(GITLIBS) scalar.c
-	$(QUIET_SUBDIR0)../.. $(QUIET_SUBDIR1) $(patsubst %,contrib/scalar/%,$@)
-
-clean:
-	$(RM) $(TARGETS) ../../bin-wrappers/scalar
-
-../../bin-wrappers/scalar: ../../wrap-for-bin.sh Makefile
-	@mkdir -p ../../bin-wrappers
-	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
-	     -e 's|@@BUILD_DIR@@|$(shell cd ../.. && pwd)|' \
-	     -e 's|@@PROG@@|contrib/scalar/scalar$(X)|' < $< > $@ && \
-	chmod +x $@
-
-test: all
-	$(MAKE) -C t
-
-.PHONY: $(GITLIBS) all clean test FORCE

similarity index 100%
rename from contrib/scalar/scalar.c
rename to scalar.c

similarity index 96%
rename from contrib/scalar/t/t9099-scalar.sh
rename to t/t9210-scalar.sh
index dfb949f52ee..14ca575a214 100755
@@ -2,15 +2,9 @@
 
 test_description='test the `scalar` command'
 
-TEST_DIRECTORY=$PWD/../../../t
-export TEST_DIRECTORY
+. ./test-lib.sh
 
-# Make it work with --no-bin-wrappers
-PATH=$PWD/..:$PATH
-
-. ../../../t/test-lib.sh
-
-GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab ../cron.txt,launchctl:true,schtasks:true"
+GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt,launchctl:true,schtasks:true"
 export GIT_TEST_MAINT_SCHEDULER
 
 test_expect_success 'scalar shows a usage' '

gitgitgadget

From: Johannes Schindelin <[email protected]>

It is merely handing off to git help scalar.

---
 scalar.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

index 642d16124eb..675d7a6b0a9 100644
@@ -819,6 +819,25 @@ static int cmd_delete(int argc, const char **argv)
 	return res;
 }
 
+static int cmd_help(int argc, const char **argv)
+{
+	struct option options[] = {
+		OPT_END(),
+	};
+	const char * const usage[] = {
+		N_("scalar help"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, NULL, options,
+			     usage, 0);
+
+	if (argc != 0)
+		usage_with_options(usage, options);
+
+	return run_git("help", "scalar", NULL);
+}
+
 static int cmd_version(int argc, const char **argv)
 {
 	int verbose = 0, build_options = 0;

@@ -858,6 +877,7 @@ static struct {
 	{ "run", cmd_run },
 	{ "reconfigure", cmd_reconfigure },
 	{ "delete", cmd_delete },
+	{ "help", cmd_help },
 	{ "version", cmd_version },
 	{ "diagnose", cmd_diagnose },
 	{ NULL, NULL},

gitgitgadget

From: Victoria Dye <[email protected]>

Create a new test file ('t9211-scalar-clone.sh') to exercise the options and behavior of the 'scalar clone' command.

---
 t/t9211-scalar-clone.sh | 135 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 135 insertions(+)
 create mode 100755 t/t9211-scalar-clone.sh

new file mode 100755
index 00000000000..9fbbc4de2c0
@@ -0,0 +1,135 @@
+#!/bin/sh
+
+test_description='test the `scalar clone` subcommand'
+
+. ./test-lib.sh
+
+GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt,launchctl:true,schtasks:true"
+export GIT_TEST_MAINT_SCHEDULER
+
+test_expect_success 'set up repository to clone' '
+	rm -rf .git &&
+	git init to-clone &&
+	(
+		cd to-clone &&
+		git branch -m base &&
+
+		test_commit first &&
+		test_commit second &&
+		test_commit third &&
+
+		git switch -c parallel first &&
+		mkdir -p 1/2 &&
+		test_commit 1/2/3 &&
+
+		git switch base &&
+
+		# By default, permit
+		git config uploadpack.allowfilter true &&
+		git config uploadpack.allowanysha1inwant true
+	)
+'
+
+cleanup_clone () {
+	rm -rf "$1"
+}
+
+test_expect_success 'creates content in enlistment root' '
+	test_when_finished cleanup_clone cloned &&
+
+	scalar clone "file://$(pwd)/to-clone" cloned &&
+	ls -A cloned >enlistment-root &&
+	test_line_count = 1 enlistment-root &&
+	test_path_is_dir cloned/src &&
+	test_path_is_dir cloned/src/.git
+'
+
+test_expect_success 'with spaces' '
+	test_when_finished cleanup_clone "cloned with space" &&
+
+	scalar clone "file://$(pwd)/to-clone" "cloned with space" &&
+	test_path_is_dir "cloned with space" &&
+	test_path_is_dir "cloned with space/src" &&
+	test_path_is_dir "cloned with space/src/.git"
+'
+
+test_expect_success 'partial clone if supported by server' '
+	test_when_finished cleanup_clone cloned &&
+
+	scalar clone "file://$(pwd)/to-clone" cloned &&
+
+	(
+		cd cloned/src &&
+
+		# Two promisor packs: one for refs, the other for blobs
+		ls .git/objects/pack/pack-*.promisor >promisorlist &&
+		test_line_count = 2 promisorlist
+	)
+'
+
+test_expect_success 'fall back on full clone if partial unsupported' '
+	test_when_finished cleanup_clone cloned &&
+
+	test_config -C to-clone uploadpack.allowfilter false &&
+	test_config -C to-clone uploadpack.allowanysha1inwant false &&
+
+	scalar clone "file://$(pwd)/to-clone" cloned 2>err &&
+	grep "filtering not recognized by server, ignoring" err &&
+
+	(
+		cd cloned/src &&
+
+		# Still get a refs promisor file, but none for blobs
+		ls .git/objects/pack/pack-*.promisor >promisorlist &&
+		test_line_count = 1 promisorlist
+	)
+'
+
+test_expect_success 'initializes sparse-checkout by default' '
+	test_when_finished cleanup_clone cloned &&
+
+	scalar clone "file://$(pwd)/to-clone" cloned &&
+	(
+		cd cloned/src &&
+		test_cmp_config true core.sparseCheckout &&
+		test_cmp_config true core.sparseCheckoutCone
+	)
+'
+
+test_expect_success '--full-clone does not create sparse-checkout' '
+	test_when_finished cleanup_clone cloned &&
+
+	scalar clone --full-clone "file://$(pwd)/to-clone" cloned &&
+	(
+		cd cloned/src &&
+		test_cmp_config "" --default "" core.sparseCheckout &&
+		test_cmp_config "" --default "" core.sparseCheckoutCone
+	)
+'
+
+test_expect_success '--single-branch clones HEAD only' '
+	test_when_finished cleanup_clone cloned &&
+
+	scalar clone --single-branch "file://$(pwd)/to-clone" cloned &&
+	(
+		cd cloned/src &&
+		git for-each-ref refs/remotes/origin >out &&
+		test_line_count = 1 out &&
+		grep "refs/remotes/origin/base" out
+	)
+'
+
+test_expect_success '--no-single-branch clones all branches' '
+	test_when_finished cleanup_clone cloned &&
+
+	scalar clone --no-single-branch "file://$(pwd)/to-clone" cloned &&
+	(
+		cd cloned/src &&
+		git for-each-ref refs/remotes/origin >out &&
+		test_line_count = 2 out &&
+		grep "refs/remotes/origin/base" out &&
+		grep "refs/remotes/origin/parallel" out
+	)
+'
+
+test_done

gitgitgadget

From: Victoria Dye <[email protected]>

Add a 'GIT_PERF_USE_SCALAR' environment variable (and corresponding perf config 'useScalar') to register a repository created with any of:

* test_perf_fresh_repo
* test_perf_default_repo
* test_perf_large_repo

as a Scalar enlistment. This is intended to allow a developer to test the impact of Scalar on already-defined performance scenarios.

---
 t/perf/README      |  4 ++++
 t/perf/perf-lib.sh | 13 ++++++++++++-
 t/perf/run         |  3 +++
 3 files changed, 19 insertions(+), 1 deletion(-)

index fb9127a66f7..8f217d7be7d 100644
@@ -95,6 +95,10 @@ You can set the following variables (also in your config.mak):
 	Git (e.g., performance of index-pack as the number of threads
 	changes). These can be enabled with GIT_PERF_EXTRA.
 
+    GIT_PERF_USE_SCALAR
+	Boolean indicating whether to register test repo(s) with Scalar
+	before executing tests.
+
 You can also pass the options taken by ordinary git tests; the most
 useful one is:
 

index 27c28017921..b960b0f6301 100644
@@ -49,6 +49,9 @@ export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP
 MODERN_GIT=$GIT_BUILD_DIR/bin-wrappers/git
 export MODERN_GIT
 
+MODERN_SCALAR=$GIT_BUILD_DIR/bin-wrappers/scalar
+export MODERN_SCALAR
+
 perf_results_dir=$TEST_RESULTS_DIR
 test -n "$GIT_PERF_SUBSECTION" && perf_results_dir="$perf_results_dir/$GIT_PERF_SUBSECTION"
 mkdir -p "$perf_results_dir"

@@ -120,6 +123,10 @@ test_perf_create_repo_from () {
 			# status" due to a locked index. Since we have
 			# a copy it's fine to remove the lock.
 			rm .git/index.lock
+		fi &&
+		if test_bool_env "$GIT_PERF_USE_SCALAR" false
+		then
+			"$MODERN_SCALAR" register
 		fi
 	) || error "failed to copy repository '$source' to '$repo'"
 }

@@ -130,7 +137,11 @@ test_perf_fresh_repo () {
 	"$MODERN_GIT" init -q "$repo" &&
 	(
 		cd "$repo" &&
-		test_perf_do_repo_symlink_config_
+		test_perf_do_repo_symlink_config_ &&
+		if test_bool_env "$GIT_PERF_USE_SCALAR" false
+		then
+			"$MODERN_SCALAR" register
+		fi
 	)
 }
 

index 55219aa4056..33da4d2aba2 100755
@@ -171,6 +171,9 @@ run_subsection () {
 	get_var_from_env_or_config "GIT_PERF_MAKE_COMMAND" "perf" "makeCommand"
 	get_var_from_env_or_config "GIT_PERF_MAKE_OPTS" "perf" "makeOpts"
 
+	get_var_from_env_or_config "GIT_PERF_USE_SCALAR" "perf" "useScalar" "--bool"
+	export GIT_PERF_USE_SCALAR
+
 	get_var_from_env_or_config "GIT_PERF_REPO_NAME" "perf" "repoName"
 	export GIT_PERF_REPO_NAME
 

gitgitgadget

From: Victoria Dye <[email protected]>

Create 'p9210-scalar.sh' for testing Scalar performance and comparing performance of Git operations in Scalar registrations and standard repositories. Example results:

Test this tree
------------------------------------------------------------------------

9210.2: scalar clone                                   14.82(18.00+3.63)
9210.3: git clone                                      26.15(36.67+6.90)
9210.4: git status (scalar)                            0.04(0.01+0.01)
9210.5: git status (non-scalar)                        0.10(0.02+0.11)
9210.6: test_commit --append --no-tag A (scalar)       0.08(0.02+0.03)
9210.7: test_commit --append --no-tag A (non-scalar)   0.13(0.03+0.11)

---
 t/perf/p9210-scalar.sh | 43 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100755 t/perf/p9210-scalar.sh

new file mode 100755
index 00000000000..a68eb6b223d
@@ -0,0 +1,43 @@
+#!/bin/sh
+
+test_description='test scalar performance'
+. ./perf-lib.sh
+
+test_perf_large_repo "$TRASH_DIRECTORY/to-clone"
+
+test_expect_success 'enable server-side partial clone' '
+	git -C to-clone config uploadpack.allowFilter true &&
+	git -C to-clone config uploadpack.allowAnySHA1InWant true
+'
+
+test_perf 'scalar clone' '
+	rm -rf scalar-clone &&
+	scalar clone "file://$(pwd)/to-clone" scalar-clone
+'
+
+test_perf 'git clone' '
+	rm -rf git-clone &&
+	git clone "file://$(pwd)/to-clone" git-clone
+'
+
+test_compare_perf () {
+	command="$@"
+	test_perf "$command (scalar)" "
+		(
+			cd scalar-clone/src &&
+			$command
+		)
+	"
+
+	test_perf "$command (non-scalar)" "
+		(
+			cd git-clone &&
+			$command
+		)
+	"
+}
+
+test_compare_perf git status
+test_compare_perf test_commit --append --no-tag A
+
+test_done

gitgitgadget

From: Victoria Dye <[email protected]>

Include 'Documentation/technical/scalar.txt' alongside the other HTML technical docs when installing them.

Now that the document is intended as a widely-accessible reference, remove the internal work-in-progress roadmap from the document. Those details should no longer be needed to guide Scalar's development and, if they were left, they could fall out-of-date and be misleading to readers.

---
 Documentation/Makefile             |  1 +
 Documentation/technical/scalar.txt | 61 ------------------------------
 2 files changed, 1 insertion(+), 61 deletions(-)

index 16c9e062390..9ec53afdf18 100644
@@ -116,6 +116,7 @@ TECH_DOCS += technical/parallel-checkout
 TECH_DOCS += technical/partial-clone
 TECH_DOCS += technical/racy-git
 TECH_DOCS += technical/reftable
+TECH_DOCS += technical/scalar
 TECH_DOCS += technical/send-pack-pipeline
 TECH_DOCS += technical/shallow
 TECH_DOCS += technical/trivial-merge

gitgitgadget

I'm happy to see this finally coming. I can say I've thoroughly reviewed it & tested it for the better part of a year now. Since most of it is the same or functionally the same as previous patches I sent at [1] and [2]. It's odd not to see any mention of that here:

1. https://lore.kernel.org/git/[email protected]/
2. https://lore.kernel.org/git/[email protected]/

In any case. I applied this & a rebased version I've kept of [1] locally, and I'll be commenting below on the diff between the two, which I produced with:

	git diff --stat -p avar/scalar-move-build-from-contrib-3 HEAD -- ':!t/t9211-scalar-clone.sh' ':!Documentation/technical/scalar.txt' ':!t/perf/'

I.e. you can get my version at
http://github.com/avar/git/tree/avar/scalar-move-build-from-contrib-3 if you're interested, and I omitted the changes to paths unique to yours:

	 .gitignore                             |  2 +-
	 Documentation/Makefile                 |  4 +-
	 Documentation/cmd-list.perl            |  2 +-
	 Documentation/git.txt                  | 13 ------
	 Documentation/scalar.txt               |  2 +-
	 Makefile                               | 75 ++++++++++++----------------------
	 builtin/help.c                         |  2 +
	 command-list.txt                       |  2 -
	 contrib/buildsystems/CMakeLists.txt    | 20 +++------
	 scalar.c                               | 20 +++++++++
	 t/{t7990-scalar.sh => t9210-scalar.sh} |  2 +-
	 11 files changed, 60 insertions(+), 84 deletions(-)

	diff --git a/.gitignore b/.gitignore
	index b1db05a9207..3d1b880101e 100644
	--- a/.gitignore
	+++ b/.gitignore
	@@ -185,6 +185,7 @@
	 /git-whatchanged
	 /git-worktree
	 /git-write-tree
	+/scalar
	 /git-core-*/?*
	 /git.res
	 /gitweb/GITWEB-BUILD-OPTIONS
	@@ -220,7 +221,6 @@
	 /config.mak.append
	 /configure
	 /.vscode/
	-/scalar
	 /tags
	 /TAGS
	 /cscope*

Functionally the same.

	diff --git a/Documentation/Makefile b/Documentation/Makefile
	index 85b68ab3e9c..9ec53afdf18 100644
	--- a/Documentation/Makefile
	+++ b/Documentation/Makefile
	@@ -21,9 +21,7 @@ MAN1_TXT += $(filter-out \
	 MAN1_TXT += git.txt
	 MAN1_TXT += gitk.txt
	 MAN1_TXT += gitweb.txt
	-ifndef NO_INSTALL_SCALAR_DOC
	 MAN1_TXT += scalar.txt
	-endif

So, this series goes the full way and install scalar unconditionally. I don't mind it, but all previous discussions on the matter were IIRC of some initial optional installation. I think doing it unconditionally makes sense, but that explains this difference.

	 # man5 / man7 guides (note: new guides should also be added to command-list.txt)
	 MAN5_TXT += gitattributes.txt
	@@ -118,6 +116,7 @@ TECH_DOCS += technical/parallel-checkout
	 TECH_DOCS += technical/partial-clone
	 TECH_DOCS += technical/racy-git
	 TECH_DOCS += technical/reftable
	+TECH_DOCS += technical/scalar
	 TECH_DOCS += technical/send-pack-pipeline
	 TECH_DOCS += technical/shallow
	 TECH_DOCS += technical/trivial-merge

Makes sense.

	@@ -290,7 +289,6 @@ endif
	 cmds_txt = cmds-ancillaryinterrogators.txt \
	 	cmds-ancillarymanipulators.txt \
	 	cmds-mainporcelain.txt \
	-	cmds-optionalcontrib.txt \
	 	cmds-plumbinginterrogators.txt \
	 	cmds-plumbingmanipulators.txt \
	 	cmds-synchingrepositories.txt \

The "optional install" also explains this bit, i.e. this was the "scalar still in the proverbial contrib", whatever "contrib" meant.

	diff --git a/Documentation/cmd-list.perl b/Documentation/cmd-list.perl
	index 0f4b1b23cfe..af5da45d287 100755
	--- a/Documentation/cmd-list.perl
	+++ b/Documentation/cmd-list.perl
	@@ -10,7 +10,7 @@ sub format_one {
	 	$state = 0;
	 	open I, '<', "$name.txt" or die "No such file $name.txt";
	 	while (<I>) {
	-		if (/^[a-z0-9-]*\(([0-9])\)$/) {
	+		if (/^git[a-z0-9-]*\(([0-9])\)$/) {
	 			$mansection = $1;
	 			next;
	 		}

This is an artifact of you not having added it to command-list.txt, more on that later.

	diff --git a/Documentation/git.txt b/Documentation/git.txt
	index 40ab70f1381..0ef7f5e4ece 100644
	--- a/Documentation/git.txt
	+++ b/Documentation/git.txt
	@@ -357,19 +357,6 @@ linkgit:git-help[1].
	 
	 include::cmds-developerinterfaces.txt[]
	 
	-Optional contrib commands
	--------------------------
	-
	-The following commands are included with the git sources, but may not
	-be present in your installation.
	-
	-These should be considered "contrib"-level when it comes to
	-maintenance and stability promises. They might not even be included in
	-your installation, and may either drastically change in the future, or
	-go away entirely.
	-
	-include::cmds-optionalcontrib.txt[]
	-
	 Configuration Mechanism
	 -----------------------

The "optional contrib".

	diff --git a/Documentation/scalar.txt b/Documentation/scalar.txt
	index f33436c7f65..505a1cea0fd 100644
	--- a/Documentation/scalar.txt
	+++ b/Documentation/scalar.txt
	@@ -163,4 +163,4 @@ linkgit:git-clone[1], linkgit:git-maintenance[1].
	 
	 GIT
	 ---
	-Part of the linkgit:git[1] suite
	+Associated with the linkgit:git[1] suite

You just kept this, but it really should be the former. The target audience of this bit of the documentation is some sysadmin that's wondering what this "scalar" thing is, and does "man scalar".

Let's not be cute, we're shipping it as part of git, it's not "associated" anymore, it's part of the git suite.

	diff --git a/Makefile b/Makefile
	index c8108ded394..66dd3321f57 100644
	--- a/Makefile
	+++ b/Makefile
	@@ -476,11 +476,6 @@ include shared.mak
	 # INSTALL_STRIP can be set to "-s" to strip binaries during installation,
	 # if your $(INSTALL) command supports the option.
	 #
	-# Define INSTALL_SCALAR if you would like to install the optional
	-# "scalar" command. This command is considered "contrib"-level, see
	-# 'Optional "contrib" commands' in the built (with "make man") git(1)
	-# manual page.
	-#
	 # Define GENERATE_COMPILATION_DATABASE to "yes" to generate JSON compilation
	 # database entries during compilation if your compiler supports it, using the
	 # `-MJ` flag. The JSON entries will be placed in the `compile_commands/`

More "optional install" etc.

	@@ -613,9 +608,10 @@ FUZZ_OBJS =
	 FUZZ_PROGRAMS =
	 GIT_OBJS =
	 LIB_OBJS =
	+SCALAR_OBJS =
	 OBJECTS =
	-PROGRAM_OBJS =
	 OTHER_PROGRAMS =
	+PROGRAM_OBJS =
	 PROGRAMS =
	 EXCLUDED_PROGRAMS =
	 SCRIPT_PERL =
	@@ -832,23 +828,12 @@ OTHER_PROGRAMS += scalar$X
	 
	 # what test wrappers are needed and 'install' will install, in bindir
	 BINDIR_PROGRAMS_NEED_X += git
	+BINDIR_PROGRAMS_NEED_X += scalar
	 BINDIR_PROGRAMS_NEED_X += git-receive-pack
	 BINDIR_PROGRAMS_NEED_X += git-shell
	 BINDIR_PROGRAMS_NEED_X += git-upload-archive
	 BINDIR_PROGRAMS_NEED_X += git-upload-pack
	 
	-# Sometimes we only have a test wrapper, but not a program to
	-# install. This isn't so pretty, and we could refactor the
	-# bin-wrappers/% and install code to make it unnecessary.
	-ifdef INSTALL_SCALAR
	-PROGRAMS += scalar$X
	-BINDIR_PROGRAMS_NEED_X += scalar
	-endif
	-TEST_BINDIR_PROGRAMS_NEED_X = $(BINDIR_PROGRAMS_NEED_X)
	-ifndef INSTALL_SCALAR
	-TEST_BINDIR_PROGRAMS_NEED_X += scalar
	-endif
	-
	 BINDIR_PROGRAMS_NO_X += git-cvsserver
	 
	 # Set paths to tools early so that they can be used for version tests.
	@@ -2241,7 +2226,7 @@ profile-fast: profile-clean
	 
	 all:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(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';)
	+	$(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) $(OTHER_PROGRAMS))), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';)
	 endif
	 
	 all::
	@@ -2564,7 +2549,12 @@ GIT_OBJS += git.o
	 .PHONY: git-objs
	 git-objs: $(GIT_OBJS)

ditto.

	+SCALAR_OBJS += scalar.o
	+.PHONY: scalar-objs
	+scalar-objs: $(SCALAR_OBJS)

This part looks missing from yours. I.e. we do this with the rest of our "objects", just gravy of course...

	+
	 OBJECTS += $(GIT_OBJS)
	+OBJECTS += $(SCALAR_OBJS)
	 OBJECTS += $(PROGRAM_OBJS)
	 OBJECTS += $(TEST_OBJS)
	 OBJECTS += $(XDIFF_OBJS)
	@@ -2575,9 +2565,6 @@ ifndef NO_CURL
	 	OBJECTS += http.o http-walker.o remote-curl.o
	 endif
	 
	-SCALAR_OBJECTS := scalar.o
	-OBJECTS += $(SCALAR_OBJECTS)
	-
	 .PHONY: objects
	 objects: $(OBJECTS)
	 
	@@ -2709,7 +2696,7 @@ $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS
	 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
	 		$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)
	 
	-scalar$X: $(SCALAR_OBJECTS) GIT-LDFLAGS $(GITLIBS)
	+scalar$X: scalar.o GIT-LDFLAGS $(GITLIBS)
	 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
	 		$(filter %.o,$^) $(LIBS)
	 
	@@ -2765,7 +2752,8 @@ XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
	 XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
	 	--keyword=__ --keyword=N__ --keyword="__n:1,2"
	 MSGMERGE_FLAGS = --add-location --backup=off --update
	-LOCALIZED_C = $(sort $(FOUND_C_SOURCES) $(FOUND_H_SOURCES) $(GENERATED_H))
	+LOCALIZED_C = $(sort $(FOUND_C_SOURCES) $(FOUND_H_SOURCES) \
	+	        $(GENERATED_H))

Nit: we can avoid the line-wrap here. FWIW I moved it back to the version pre-9f555783c0b, but obviously functionally the same...

	 LOCALIZED_SH = $(sort $(SCRIPT_SH) git-sh-setup.sh)
	 LOCALIZED_PERL = $(sort $(SCRIPT_PERL))
	 
	@@ -3071,18 +3059,15 @@ GIT-PYTHON-VARS: FORCE
	             fi
	 endif
	 
	-test_bindir_programs := $(patsubst %,bin-wrappers/%,$(TEST_BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
	+test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
	 
	 all:: $(TEST_PROGRAMS) $(test_bindir_programs)
	 
	-# Substitution that's only done on programs named git-* in
	-# bin-wrappers/*
	-GIT_TEST_BINDIR_PROGRAMS_NEED_X = $(filter-out scalar,$(TEST_BINDIR_PROGRAMS_NEED_X))
	 bin-wrappers/%: wrap-for-bin.sh
	 	$(call mkdir_p_parent_template)
	 	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
	 	     -e 's|@@BUILD_DIR@@|$(shell pwd)|' \
	-	     -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%$(X),$(@F))$(patsubst git%,$(X),$(filter $(@F),$(GIT_TEST_BINDIR_PROGRAMS_NEED_X)))|' < $< > $@ && \
	+	     -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%,$(@F))$(if $(filter-out $(BINDIR_PROGRAMS_NO_X),$(@F)),$(X),)|' < $< > $@ && \
	 	chmod +x $@
	 
	 # GNU make supports exporting all variables by "export" without parameters.
	@@ -3296,14 +3281,14 @@ ifndef NO_TCLTK
	 	$(MAKE) -C git-gui gitexecdir='$(gitexec_instdir_SQ)' install
	 endif
	 ifneq (,$X)
	-	$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) git$X)), test '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p' -ef '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p$X' || $(RM) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p';)
	+	$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) $(OTHER_PROGRAMS))), test '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p' -ef '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p$X' || $(RM) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p';)
	 endif

Looks correct, just different.

	 	bindir=$$(cd '$(DESTDIR_SQ)$(bindir_SQ)' && pwd) && \
	 	execdir=$$(cd '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' && pwd) && \
	 	destdir_from_execdir_SQ=$$(echo '$(gitexecdir_relative_SQ)' | sed -e 's|[^/][^/]*|..|g') && \
	 	{ test "$$bindir/" = "$$execdir/" || \
	-	  for p in git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); do \
	+	  for p in $(OTHER_PROGRAMS) $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); do \
	 		$(RM) "$$execdir/$$p" && \
	 		test -n "$(INSTALL_SYMLINKS)" && \
	 		ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/$$p" "$$execdir/$$p" || \

This part I missed, thanks!

	@@ -3347,18 +3332,11 @@ endif
	 .PHONY: install-doc install-man install-man-perl install-html install-info install-pdf
	 .PHONY: quick-install-doc quick-install-man quick-install-html
	 
	-# We must not "export" this as e.g. "check-docs" needs to know about
	-# scalar.txt. We only exclude scalar.txt for the "install-*" targets.
	-NO_INSTALL_SCALAR_DOC =
	-ifndef INSTALL_SCALAR
	-NO_INSTALL_SCALAR_DOC = NoScalarPlease
	-endif
	-
	 install-doc: install-man-perl
	-	$(MAKE) -C Documentation install NO_INSTALL_SCALAR_DOC='$(NO_INSTALL_SCALAR_DOC)'
	+	$(MAKE) -C Documentation install
	 
	 install-man: install-man-perl
	-	$(MAKE) -C Documentation install-man NO_INSTALL_SCALAR_DOC='$(NO_INSTALL_SCALAR_DOC)'
	+	$(MAKE) -C Documentation install-man
	 
	 install-man-perl: man-perl
	 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mandir_SQ)/man3'
	@@ -3366,22 +3344,22 @@ install-man-perl: man-perl
	 	(cd '$(DESTDIR_SQ)$(mandir_SQ)/man3' && umask 022 && $(TAR) xof -)
	 
	 install-html:
	-	$(MAKE) -C Documentation install-html NO_INSTALL_SCALAR_DOC='$(NO_INSTALL_SCALAR_DOC)'
	+	$(MAKE) -C Documentation install-html
	 
	 install-info:
	-	$(MAKE) -C Documentation install-info NO_INSTALL_SCALAR_DOC='$(NO_INSTALL_SCALAR_DOC)'
	+	$(MAKE) -C Documentation install-info
	 
	 install-pdf:
	-	$(MAKE) -C Documentation install-pdf NO_INSTALL_SCALAR_DOC='$(NO_INSTALL_SCALAR_DOC)'
	+	$(MAKE) -C Documentation install-pdf
	 
	 quick-install-doc:
	-	$(MAKE) -C Documentation quick-install NO_INSTALL_SCALAR_DOC='$(NO_INSTALL_SCALAR_DOC)'
	+	$(MAKE) -C Documentation quick-install
	 
	 quick-install-man:
	-	$(MAKE) -C Documentation quick-install-man NO_INSTALL_SCALAR_DOC='$(NO_INSTALL_SCALAR_DOC)'
	+	$(MAKE) -C Documentation quick-install-man
	 
	 quick-install-html:
	-	$(MAKE) -C Documentation quick-install-html NO_INSTALL_SCALAR_DOC='$(NO_INSTALL_SCALAR_DOC)'
	+	$(MAKE) -C Documentation quick-install-html

This is to do with making the "scalar docs" optional, but if we're not doing that...

	@@ -3485,7 +3463,7 @@ clean: profile-clean coverage-clean cocciclean
	 	$(RM) git.res
	 	$(RM) $(OBJECTS)
	 	$(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)
	-	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
	+	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS)
	 	$(RM) $(TEST_PROGRAMS)
	 	$(RM) $(FUZZ_PROGRAMS)
	 	$(RM) $(SP_OBJ)

Makes sense.

	@@ -3536,6 +3514,7 @@ ALL_COMMANDS += git-citool
	 ALL_COMMANDS += git-gui
	 ALL_COMMANDS += gitk
	 ALL_COMMANDS += gitweb
	+ALL_COMMANDS += scalar
	 
	 .PHONY: check-docs
	 check-docs::
	@@ -3571,7 +3550,7 @@ check-docs::
	 		    -e 's/\.txt//'; \
	 	) | while read how cmd; \
	 	do \
	-		case " $(patsubst %$X,%,$(ALL_COMMANDS) $(BUILT_INS) $(EXCLUDED_PROGRAMS) scalar) " in \
	+		case " $(patsubst %$X,%,$(ALL_COMMANDS) $(BUILT_INS) $(EXCLUDED_PROGRAMS)) " in \
	 		*" $$cmd "*)	;; \
	 		*) echo "removed but $$how: $$cmd" ;; \
	 		esac; \
	diff --git a/builtin/help.c b/builtin/help.c
	index 09ac4289f13..6f2796f211e 100644
	--- a/builtin/help.c
	+++ b/builtin/help.c
	@@ -440,6 +440,8 @@ static const char *cmd_to_page(const char *git_cmd)
	 		return git_cmd;
	 	else if (is_git_command(git_cmd))
	 		return xstrfmt("git-%s", git_cmd);
	+	else if (!strcmp("scalar", git_cmd))
	+		return xstrdup(git_cmd);
	 	else
	 		return xstrfmt("git%s", git_cmd);
	 }

Works, but I wonder if 3/8 here in your series is what we really want, i.e. isn't the point of "git" to be a holistic thing for git, and for "scalar" to be set apart from that?

But OTOH much of the docs would need to cross-link anyway...

	diff --git a/command-list.txt b/command-list.txt
	index 27bd54af49c..f96bdabd7d9 100644
	--- a/command-list.txt
	+++ b/command-list.txt
	@@ -16,7 +16,6 @@
	 #   synchingrepositories
	 #   synchelpers
	 #   purehelpers
	-#   optionalcontrib
	 #
	 # The type names are self explanatory. But if you want to see what
	 # command belongs to what group to get a better picture, have a look
	@@ -236,4 +235,3 @@ gittutorial                             guide
	 gittutorial-2                           guide
	 gitweb                                  ancillaryinterrogators
	 gitworkflows                            guide
	-scalar                                  optionalcontrib

You don't have it in command-list at all, shouldn't you?

	diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
	index a5a1dedab4f..bae203c1fb9 100644
	--- a/contrib/buildsystems/CMakeLists.txt
	+++ b/contrib/buildsystems/CMakeLists.txt
	@@ -610,8 +610,7 @@ unset(CMAKE_REQUIRED_INCLUDES)
	 #programs
	 set(PROGRAMS_BUILT
	 	git git-daemon git-http-backend git-sh-i18n--envsubst
	-	git-shell
	-	scalar)
	+	git-shell scalar)

Just whitespace changes.

	 if(NOT CURL_FOUND)
	 	list(APPEND excluded_progs git-http-fetch git-http-push)
	@@ -746,9 +745,6 @@ list(TRANSFORM git_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
	 add_executable(git ${CMAKE_SOURCE_DIR}/git.c ${git_SOURCES})
	 target_link_libraries(git common-main)
	 
	-add_executable(scalar ${CMAKE_SOURCE_DIR}/scalar.c)
	-target_link_libraries(scalar common-main)
	-
	 add_executable(git-daemon ${CMAKE_SOURCE_DIR}/daemon.c)
	 target_link_libraries(git-daemon common-main)
	 
	@@ -761,6 +757,9 @@ target_link_libraries(git-sh-i18n--envsubst common-main)
	 add_executable(git-shell ${CMAKE_SOURCE_DIR}/shell.c)
	 target_link_libraries(git-shell common-main)
	 
	+add_executable(scalar ${CMAKE_SOURCE_DIR}/scalar.c)
	+target_link_libraries(scalar common-main)
	+
	 if(CURL_FOUND)
	 	add_library(http_obj OBJECT ${CMAKE_SOURCE_DIR}/http.c)
	 
	@@ -906,16 +905,10 @@ list(TRANSFORM git_shell_scripts PREPEND "${CMAKE_BINARY_DIR}/")
	 list(TRANSFORM git_perl_scripts PREPEND "${CMAKE_BINARY_DIR}/")
	 
	 #install
	-option(INSTALL_SCALAR "Install the optional 'scalar' contrib command")
	 foreach(program ${PROGRAMS_BUILT})
	-if(program STREQUAL "git" OR program STREQUAL "git-shell")
	-install(TARGETS ${program}
	-	RUNTIME DESTINATION bin)
	-elseif(program STREQUAL "scalar")
	-if(INSTALL_SCALAR)
	+if(program MATCHES "^(git|git-shell|scalar)$")
	 install(TARGETS ${program}
	 	RUNTIME DESTINATION bin)
	-endif()
	 else()
	 install(TARGETS ${program}
	 	RUNTIME DESTINATION libexec/git-core)
	@@ -987,8 +980,7 @@ endif()
	 
	 #wrapper scripts
	 set(wrapper_scripts
	-	git git-upload-pack git-receive-pack git-upload-archive git-shell git-remote-ext
	-	scalar)
	+	git git-upload-pack git-receive-pack git-upload-archive git-shell git-remote-ext scalar)
	 
	 set(wrapper_test_scripts
	 	test-fake-ssh test-tool)
	diff --git a/scalar.c b/scalar.c
	index 642d16124eb..675d7a6b0a9 100644
	--- a/scalar.c
	+++ b/scalar.c
	@@ -819,6 +819,25 @@ static int cmd_delete(int argc, const char **argv)
	 	return res;
	 }
	 
	+static int cmd_help(int argc, const char **argv)
	+{
	+	struct option options[] = {
	+		OPT_END(),
	+	};
	+	const char * const usage[] = {
	+		N_("scalar help"),
	+		NULL
	+	};
	+
	+	argc = parse_options(argc, argv, NULL, options,
	+			     usage, 0);
	+
	+	if (argc != 0)
	+		usage_with_options(usage, options);
	+
	+	return run_git("help", "scalar", NULL);
	+}
	+
	 static int cmd_version(int argc, const char **argv)
	 {
	 	int verbose = 0, build_options = 0;
	@@ -858,6 +877,7 @@ static struct {
	 	{ "run", cmd_run },
	 	{ "reconfigure", cmd_reconfigure },
	 	{ "delete", cmd_delete },
	+	{ "help", cmd_help },
	 	{ "version", cmd_version },
	 	{ "diagnose", cmd_diagnose },
	 	{ NULL, NULL},
	diff --git a/t/t7990-scalar.sh b/t/t9210-scalar.sh
	similarity index 98%
	rename from t/t7990-scalar.sh
	rename to t/t9210-scalar.sh
	index 62b92d361e2..14ca575a214 100755
	--- a/t/t7990-scalar.sh
	+++ b/t/t9210-scalar.sh
	@@ -4,7 +4,7 @@ test_description='test the `scalar` command'
	 
	 . ./test-lib.sh
	 
	-GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab ../cron.txt,launchctl:true,schtasks:true"
	+GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt,launchctl:true,schtasks:true"
	 export GIT_TEST_MAINT_SCHEDULER
	 
	 test_expect_success 'scalar shows a usage' '

Makes sense.

Thanks.

For what it's worth, the lack of mention wasn't meant as a snub. I intentionally wrote this series (with the exception of patches 3 & 4) in a vacuum and avoided using any prior approaches as a reference. In this case, that includes the series you linked, the 'microsoft/git' implementation with 'INCLUDE_SCALAR' [1], and Dscho's dropped CI integration [2].

Like updating the docs & roadmap, my goal was to start "fresh" and keep myself free of any prior discussions/assumptions about the role/purpose of Scalar that may no longer apply to my approach. Although there's something to be said for learning from prior work, I preferred combing through the code to fully understand the *existing* architecture, then using that understanding to figure out where Scalar best fits in. Then, of course, I would iterate on and revise that design based on review comments. :)

[1] https://github.com/microsoft/git/commit/4f553e0027190484899afba955c7cd3f1de77532
[2] https://lore.kernel.org/git/[email protected]/

For the sake of readability/saving space, I cut out parts of the diff you noted as "ok" or otherwise didn't seem to need comment (like nits). If I neglected to comment on something you'd like me to respond to, though, please let me know!

I originally chose to leave the "associated" to distinguish it from the builtins invoked with 'git <command>' (since 'scalar' is invoked directly). However, given that every single other "GIT" section in the docs (including other directly-invoked commands like 'gitk') says "Part of the linkgit:git[1] suite", it should probably be changed here as well.

This hunk is from my series, I believe it's missing from yours (unless I missed it elsewhere in the diff). It's also mentioned explicitly in the commit message of patch 2 [3].

[3] https://lore.kernel.org/git/4d69e5eaccb8873eece666a3d2bb2b22abdde7ea.1661961746.git.gitgitgadget@gmail.com/

Like you noted earlier, Scalar is *part of* the Git suite, despite being invoked with 'scalar' (rather than the 'git' executable). Having 'git help scalar' work the same way as, e.g., 'git help gitk' is, to me, a sensible approach both from a philosophical "what is Scalar?" perspective and a user experience/ease-of-access perspective.

I missed that part of the docs, but I agree that it should be in the command-list. Thanks for pointing this out!

Hi Victoria,

I probably left that in by mistake. Could I bother you to change this (and the corresponding Signed-off-by: footer) to use my usual email address?

Thank you,
Dscho

Hi Victoria,

A great commit message, and even though the diff is large, it is eminently reviewable, with one exception:

Hi Victoria,

Footnote *1*:
https://colabti.org/irclogger/irclogger_log/git-devel?date=2022-08-29#l50

Hi Victoria,

Great catch!

I have one suggestion, given my experience with debugging test failures:

Hi Victoria,

Excellent!

	test_compare_perf () {
	     command=$1
             shift
	     args="$*"

	     test_perf "$command (scalar)" "
	             $command -C scalar-clone/src $args
	     "

	     test_perf "$command (non-scalar)" "
	             $command -C git-clone $args
	     "
	}

Hi Victoria,

Great idea!

Hi Victoria,

Thank you for giving this a fresh start.

My previous attempts all got mired and stuck in "mine vs yours" contests, and a fresh start was the best that could happen to the Scalar effort to get unstuck.

I reviewed all the patches, provided some commentary, cheering on, and also a few suggestions. Feel free to add my Reviewed-by.

Thank you,
Dscho

We could do this later, but the 3/5 here is my reply to the "fault of the design of the Makfile syntax".

I really don't think that's the case, the problem here is something you'd get any any language.

We have three lists which we'd like to treat differently, but for no particularly good reason other than incrementally building on past changes to end up with this we end up having to on-the-fly guess which list a given item came from.

With this series the end result is instead to do:

	define bin_wrappers_template
	BW_$(1) = $$($(1):%=bin-wrappers/%)
	BIN_WRAPPERS += $$(BW_$(1))
	all:: $$(BW_$(1))
	$$(BW_$(1)): bin-wrappers/% : $(3)%$(4)
	$$(BW_$(1)): wrap-for-bin.sh
		$$(call mkdir_p_parent_template)
		$$(QUIET_GEN)$$(call cmd_munge_bin_wrappers_script,$(2),$(3),$(4))
	endef
	
	$(eval $(call bin_wrappers_template,BINDIR_PROGRAMS_NEED_X,'$$(@F)',,$$X))
	$(eval $(call bin_wrappers_template,BINDIR_PROGRAMS_NO_X,'$$(@F)'))
	$(eval $(call bin_wrappers_template,TEST_PROGRAMS_NEED_X,'$$(@F)',t/helper/,$$X))
	
	all:: $(BIN_WRAPPERS)

This obviously conflicts with Victoria's changes here, but if picked up the conflict can be entirely solved in favor of this series, and this "scalar" series will benefit.

I.e. the only reason this series needs to patch this one liner is because the Makefile is losing track of where the item(s) came from.

Once we're not doing that we're perfectly capable of creating a bin-wrappers/scalar, because we're no longer running into the logic that uses git% as a heuristic to determine whether something is "not from the $(TEST_PROGRAMS_NEED_X) variable".

A CI run, showing that this also works on Windows etc.:
https://github.com/avar/git/runs/8135048620

Ævar Arnfjörð Bjarmason (5):
Makefile: factor sed-powered '#!/bin/sh' munging into a variable
Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier
Makefile: simplify $(test_bindir_programs) rule by splitting it up
Makefile: define bin-wrappers/% rules with a template
Makefile: fix "make clean && make bin-wrappers/$NAME" dependencies

 Makefile | 58 +++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 39 insertions(+), 19 deletions(-)

2.37.3.1426.g360dd7cf8ca

Reduce the amount of magical copy/pasting in the Makefile by factoring the munging of "#!/bin/sh" on the first line of a shellscript into a variable we can re-use in the various rules that need to do so.

---
 Makefile | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

index eac30126e29..b2304aa93a3 100644
@@ -2338,8 +2338,12 @@ GIT-SCRIPT-DEFINES: FORCE
 		echo "$$FLAGS" >$@; \
             fi
 
+define cmd_munge_script_sed_shell_path_arg
+'1s|#!.*/sh|#!$(SHELL_PATH_SQ)|'
+endef
+
 define cmd_munge_script
-sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
+sed -e $(call cmd_munge_script_sed_shell_path_arg) \
     -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
     -e 's|@@DIFF@@|$(DIFF_SQ)|' \
     -e 's|@@LOCALEDIR@@|$(localedir_SQ)|g' \

@@ -2441,7 +2445,7 @@ git-instaweb: git-instaweb.sh GIT-SCRIPT-DEFINES
 else # NO_PERL
 $(SCRIPT_PERL_GEN) git-instaweb: % : unimplemented.sh
 	$(QUIET_GEN) \
-	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
+	sed -e $(call cmd_munge_script_sed_shell_path_arg) \
 	    -e 's|@@REASON@@|NO_PERL=$(NO_PERL)|g' \
 	    unimplemented.sh >$@+ && \
 	chmod +x $@+ && \

@@ -2462,7 +2466,7 @@ $(SCRIPT_PYTHON_GEN): % : %.py
 else # NO_PYTHON
 $(SCRIPT_PYTHON_GEN): % : unimplemented.sh
 	$(QUIET_GEN) \
-	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
+	sed -e $(call cmd_munge_script_sed_shell_path_arg) \
 	    -e 's|@@REASON@@|NO_PYTHON=$(NO_PYTHON)|g' \
 	    unimplemented.sh >$@+ && \
 	chmod +x $@+ && \

@@ -3060,7 +3064,7 @@ all:: $(TEST_PROGRAMS) $(test_bindir_programs)
 
 bin-wrappers/%: wrap-for-bin.sh
 	$(call mkdir_p_parent_template)
-	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
+	$(QUIET_GEN)sed -e $(call cmd_munge_script_sed_shell_path_arg) \
 	     -e 's|@@BUILD_DIR@@|$(shell pwd)|' \
 	     -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%$(X),$(@F))$(patsubst git%,$(X),$(filter $(@F),$(BINDIR_PROGRAMS_NEED_X)))|' < $< > $@ && \
 	chmod +x $@

2.37.3.1426.g360dd7cf8ca

Define the variables that make up TEST_OBJS earlier, and don't go back & forth in their definition. Before we'd first append $X to $(TEST_PROGRAMS), and then substitute $X back out of it to define $(TEST_OBJS). Let's instead add a new $(TEST_PROGRAM_OBJS) variable, which avoids this needless back & forth substitution.

See daa99a91729 (Makefile: make sure test helpers are rebuilt when headers change, 2010-01-26) for how we ended up with the original $(TEST_OBJS).

---
 Makefile | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

index b2304aa93a3..0d6b2caa7d9 100644
@@ -617,7 +617,8 @@ SCRIPT_PYTHON =
 SCRIPT_SH =
 SCRIPT_LIB =
 TEST_BUILTINS_OBJS =
-TEST_OBJS =
+TEST_PROGRAMS =
+TEST_PROGRAM_OBJS =
 TEST_PROGRAMS_NEED_X =
 THIRD_PARTY_SOURCES =
 

@@ -794,6 +795,7 @@ 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
+TEST_PROGRAM_OBJS += $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
 
 # Do not add more tests here unless they have extra dependencies. Add
 # them in TEST_BUILTINS_OBJS above.

@@ -801,6 +803,9 @@ TEST_PROGRAMS_NEED_X += test-fake-ssh
 TEST_PROGRAMS_NEED_X += test-tool
 
 TEST_PROGRAMS = $(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X))
+all:: $(TEST_PROGRAMS)
+TEST_PROGRAM_OBJS += $(patsubst %,t/helper/%.o,$(TEST_PROGRAMS_NEED_X))
+.PRECIOUS: $(TEST_PROGRAM_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.

@@ -2537,10 +2542,8 @@ REFTABLE_TEST_OBJS += reftable/stack_test.o
 REFTABLE_TEST_OBJS += reftable/test_framework.o
 REFTABLE_TEST_OBJS += reftable/tree_test.o
 
-TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
-
 .PHONY: test-objs
-test-objs: $(TEST_OBJS)
+test-objs: $(TEST_PROGRAM_OBJS)
 
 GIT_OBJS += $(LIB_OBJS)
 GIT_OBJS += $(BUILTIN_OBJS)

@@ -2551,7 +2554,7 @@ git-objs: $(GIT_OBJS)
 
 OBJECTS += $(GIT_OBJS)
 OBJECTS += $(PROGRAM_OBJS)
-OBJECTS += $(TEST_OBJS)
+OBJECTS += $(TEST_PROGRAM_OBJS)
 OBJECTS += $(XDIFF_OBJS)
 OBJECTS += $(FUZZ_OBJS)
 OBJECTS += $(REFTABLE_OBJS) $(REFTABLE_TEST_OBJS)

@@ -3060,7 +3063,7 @@ endif
 
 test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
 
-all:: $(TEST_PROGRAMS) $(test_bindir_programs)
+all:: $(test_bindir_programs)
 
 bin-wrappers/%: wrap-for-bin.sh
 	$(call mkdir_p_parent_template)

@@ -3086,8 +3089,6 @@ perf: all
 
 .PHONY: test perf
 
-.PRECIOUS: $(TEST_OBJS)
-
 t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
 
 t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS) $(REFTABLE_TEST_LIB)

2.37.3.1426.g360dd7cf8ca

When the @@PROG@@ substitution was added in [1] it was a simple matter of doing a:

	's|@@PROG@@|$(@F)|'

Then when t/helpers were added in [2] followed by the ".exe" needing to be appended in [3] this previously simple rule ended up as a dense one-liner.

It has been pointed out that this is hard to read[4], but the problem isn't the "design of the Makefile syntax". It's we now have to after-the-fact determine if we were dealing with a bin-wrapper/ that needed to have $(X) appended, a t/helper/, or a non-binary (currently only git-cvsserver).

That would be a problem in any language. We're starting out with three lists, and then end up having to heuristically determine given a member of any of those lists which list that member came from. Let's just stop doing that and keep track of what member belongs to which list.

We can do this by splitting up the single "bin-wrappers/%" rule into a rule for each of the three lists. With the "cmd_munge_script_sed_shell_path_arg" define added in a preceding commit this is easy, we just need to add a sister template to the existing "cmd_munge_script" added in [5].

There is then an an in-flight change[6] that generalizes this to support "scalar". That change needed to further special-case the pre-image here, as we couldn't rely on the "git%" heuristic to mean "not from the $(TEST_PROGRAMS_NEED_X) variable".

With this conflicting change that special-casing becomes unnecessary. The conflict between the two can be resolved entirely in favor of this change, as the other change adds "scalar" to "$(BINDIR_PROGRAMS_NEED_X)", and we are no longer losing track of which members belong to that list.

This change can be tested with e.g.:

	git checkout master &&
	make SHELL_PATH=/bin/bash X=.exe &&
	mv bin-wrappers bin-wrappers.master &&
	<apply this change> &&
	make SHELL_PATH=/bin/bash X=.exe &&
	diff -ru bin-wrappers{.master,}

Which will show an empty diff, i.e. we've correctly dealt with the combination of $(SHELL_PATH), $(X) and these three variables here.

1. ea925196f1b (build dashless "bin-wrappers" directory similar to
installed bindir, 2009-12-02)
2. e6e7530d10b (test helpers: move test-* to t/helper/ subdirectory,
2016-04-13)
3. 3a94cb31d52 (bin-wrappers: append .exe to target paths if
necessary, 2019-07-29)
4. https://lore.kernel.org/git/[email protected]/
5. 46bac904581 (Do not install shell libraries executable, 2010-01-31)
6. https://lore.kernel.org/git/4d69e5eaccb8873eece666a3d2bb2b22abdde7ea.1661961746.git.gitgitgadget@gmail.com/

---
 Makefile | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

index 0d6b2caa7d9..96de9e55864 100644
@@ -597,6 +597,7 @@ export TCL_PATH TCLTK_PATH
 PTHREAD_LIBS = -lpthread
 
 # Guard against environment variables
+BIN_WRAPPERS =
 BUILTIN_OBJS =
 BUILT_INS =
 COMPAT_CFLAGS =

@@ -3061,16 +3062,34 @@ GIT-PYTHON-VARS: FORCE
             fi
 endif
 
-test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
+define cmd_munge_bin_wrappers_script
+sed \
+	-e $(call cmd_munge_script_sed_shell_path_arg) \
+	-e 's|@@BUILD_DIR@@|$(shell pwd)|' \
+	-e 's|@@PROG@@|$(2)$(1)$(3)|' \
+	<$< >$@ && \
+	chmod +x $@
+endef
 
-all:: $(test_bindir_programs)
+BW_BINDIR_PROGRAMS_NEED_X = $(BINDIR_PROGRAMS_NEED_X:%=bin-wrappers/%)
+BIN_WRAPPERS += $(BW_BINDIR_PROGRAMS_NEED_X)
+$(BW_BINDIR_PROGRAMS_NEED_X): wrap-for-bin.sh
+	$(call mkdir_p_parent_template)
+	$(QUIET_GEN)$(call cmd_munge_bin_wrappers_script,$(@F),,$X)
 
-bin-wrappers/%: wrap-for-bin.sh
+BW_BINDIR_PROGRAMS_NO_X = $(BINDIR_PROGRAMS_NO_X:%=bin-wrappers/%)
+BIN_WRAPPERS += $(BW_BINDIR_PROGRAMS_NO_X)
+$(BW_BINDIR_PROGRAMS_NO_X): wrap-for-bin.sh
 	$(call mkdir_p_parent_template)
-	$(QUIET_GEN)sed -e $(call cmd_munge_script_sed_shell_path_arg) \
-	     -e 's|@@BUILD_DIR@@|$(shell pwd)|' \
-	     -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%$(X),$(@F))$(patsubst git%,$(X),$(filter $(@F),$(BINDIR_PROGRAMS_NEED_X)))|' < $< > $@ && \
-	chmod +x $@
+	$(QUIET_GEN)$(call cmd_munge_bin_wrappers_script,$(@F))
+
+BW_TEST_PROGRAMS_NEED_X = $(TEST_PROGRAMS_NEED_X:%=bin-wrappers/%)
+BIN_WRAPPERS += $(BW_TEST_PROGRAMS_NEED_X)
+$(BW_TEST_PROGRAMS_NEED_X): wrap-for-bin.sh
+	$(call mkdir_p_parent_template)
+	$(QUIET_GEN)$(call cmd_munge_bin_wrappers_script,$(@F),t/helper/,$X)
+
+all:: $(BIN_WRAPPERS)
 
 # GNU make supports exporting all variables by "export" without parameters.
 # However, the environment gets quite big, and some programs have problems

@@ -3403,7 +3422,7 @@ OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll)
 endif
 
 artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
-		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
+		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(BIN_WRAPPERS) \
 		$(MOFILES)
 	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
 		SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'

2.37.3.1426.g360dd7cf8ca

The "bin-wrappers/" scripts have never had properly declared dependency information, i.e. this has never worked:

	make clean &&
	make bin-wrappers/git &&
	./bin-wrappers/git

There was no reason not to have that work, just as most things generated by the Makefile have proper dependency information.

Before the preceding commits doing this would have been painful, but now it's quite easy to simply make use of the third parameter to our "bin_wrappers_template"

---
 Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 533858ca481..07a9901c99d 100644
@@ -3075,9 +3075,10 @@ define bin_wrappers_template
 BW_$(1) = $$($(1):%=bin-wrappers/%)
 BIN_WRAPPERS += $$(BW_$(1))
 all:: $$(BW_$(1))
+$$(BW_$(1)): bin-wrappers/% : $(3)%$(4)
 $$(BW_$(1)): wrap-for-bin.sh
 	$$(call mkdir_p_parent_template)
-	$$(QUIET_GEN)$$(call cmd_munge_bin_wrappers_script,$(2),$(3))
+	$$(QUIET_GEN)$$(call cmd_munge_bin_wrappers_script,$(2),$(3),$(4))
 endef
 
 $(eval $(call bin_wrappers_template,BINDIR_PROGRAMS_NEED_X,'$$(@F)',,$$X))

2.37.3.1426.g360dd7cf8ca

Eliminate the copy/pasting among the three rules generating the bin-wrappers/% files by refactoring them into a template.

---
 Makefile | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

index 96de9e55864..533858ca481 100644
@@ -3071,23 +3071,18 @@ sed \
 	chmod +x $@
 endef
 
-BW_BINDIR_PROGRAMS_NEED_X = $(BINDIR_PROGRAMS_NEED_X:%=bin-wrappers/%)
-BIN_WRAPPERS += $(BW_BINDIR_PROGRAMS_NEED_X)
-$(BW_BINDIR_PROGRAMS_NEED_X): wrap-for-bin.sh
-	$(call mkdir_p_parent_template)
-	$(QUIET_GEN)$(call cmd_munge_bin_wrappers_script,$(@F),,$X)
-
-BW_BINDIR_PROGRAMS_NO_X = $(BINDIR_PROGRAMS_NO_X:%=bin-wrappers/%)
-BIN_WRAPPERS += $(BW_BINDIR_PROGRAMS_NO_X)
-$(BW_BINDIR_PROGRAMS_NO_X): wrap-for-bin.sh
-	$(call mkdir_p_parent_template)
-	$(QUIET_GEN)$(call cmd_munge_bin_wrappers_script,$(@F))
+define bin_wrappers_template
+BW_$(1) = $$($(1):%=bin-wrappers/%)
+BIN_WRAPPERS += $$(BW_$(1))
+all:: $$(BW_$(1))
+$$(BW_$(1)): wrap-for-bin.sh
+	$$(call mkdir_p_parent_template)
+	$$(QUIET_GEN)$$(call cmd_munge_bin_wrappers_script,$(2),$(3))
+endef
 
-BW_TEST_PROGRAMS_NEED_X = $(TEST_PROGRAMS_NEED_X:%=bin-wrappers/%)
-BIN_WRAPPERS += $(BW_TEST_PROGRAMS_NEED_X)
-$(BW_TEST_PROGRAMS_NEED_X): wrap-for-bin.sh
-	$(call mkdir_p_parent_template)
-	$(QUIET_GEN)$(call cmd_munge_bin_wrappers_script,$(@F),t/helper/,$X)
+$(eval $(call bin_wrappers_template,BINDIR_PROGRAMS_NEED_X,'$$(@F)',,$$X))
+$(eval $(call bin_wrappers_template,BINDIR_PROGRAMS_NO_X,'$$(@F)'))
+$(eval $(call bin_wrappers_template,TEST_PROGRAMS_NEED_X,'$$(@F)',t/helper/,$$X))
 
 all:: $(BIN_WRAPPERS)
 

2.37.3.1426.g360dd7cf8ca

I find it very distracting when you send a full patch series in the middle of someone else's patch series. Viewing the thread now involves two distinct conversations, assuming that your series gets review and requires multiple versions.

I would rather that you started a new thread with your proposed series and then sent a reply in this thread pointing to that one.

Thanks,
-Stolee

Thanks, will fix.

Normally I'd agree, but in this case there's a readability benefit to explicitly comparing 'argc' to 0. 'scalar help' expects exactly zero positional arguments, and showing the '!= 0' makes that expectation clearer (likewise, 'scalar delete' checks that 'argc != 1' because it expects exactly one positional arg).

As a matter of design preference, I'd rather avoid invoking builtins via their 'cmd_*()' entrypoint. Doing so in 'scalar.c' would also introduce some function name conflicts. While that's an overcomeable problem, precedent across Git doesn't appear to indicate one approach is better than the other, so I'm happy with things as they are.

Makes sense to me! Although, out of curiosity, is there a reason you prefer "$1 -> shift -> $*" over '$1' and '$@'?

Whoops, I completely misread your snippet; the 'shift' is necessary to separate the '$command' out so that we can inject '-C'.

Thanks!

While I like the simplicity of using 'test_when_finished', I hadn't considered the value of having the failed tests' artifacts in the CI results. If you think that would be helpful to developers, I'll update accordingly (although I'd still clean up the clones whose tests pass to avoid archiving more data than we need).

That being said, even if I update 't9211', my experience with Git's test suite is that very few tests preserve test repos this way. Do you expect these artifacts to be especially helpful for 'scalar clone' in particular, or is this more of a "gently nudge contributors to make this standard practice" sort of recommendation?

As you guessed, a '.gitconfig' is created in the trash directory of each test containing the Scalar registration and I haven't seen any issues arising from the scheduled maintenance, so I don't think an 'unregister' is necessary. However, while verifying that, I noticed that the registration wasn't happening *at all* because 'test_bool_env' is currently being used incorrectly. The fix is straightforward - I'll make sure to correct it in the next version.

Thanks!

Hi Victoria,

Thank you!

Thank you for this question, which helps me clarify even to myself what my intention is.

After considering this, yes, I would like this to be a gentle nudge to take a tiny step toward improving Git's test suite by recommending a new standard practice.

Ciao,
Dscho

Hi Victoria,

Yes, and I also changed the "$@" (which would usually expand to a parameter list, except when it is used inside a string, in which case it behaves like $* for convenience) because the "$*" conveys more correctly what we do here.

Whenever I read "$@" anywhere, my mind puts a mental check mark behind the "is this code safe with regards to spaces in arguments?" question.

However, this function would mishandle arguments that contain spaces, and reading "$*" makes me aware of that, so that I can avoid passing such arguments.

So for me, using $* here is the right thing to do: It makes it less likely that someone like me adds code in the future that assumes that even arguments with spaces in them would be handled.

Thanks,
Dscho

Hi Victoria,

Thank you for checking!

Oh, great, then my feedback was at least _somewhat_ helpful... ;-)

Ciao,
Dscho

Hi Ævar,

Yes, and we should.

It would be better if we kept the focus on Scalar in this mail thread, to get it done first.

It is the stated preference in this project anyway to avoid working on the code that is already being worked on in other in-flight patch series, to reduce unnecessary friction.

Ciao,
Johannes

From: Johannes Schindelin <[email protected]>

It is merely handing off to git help scalar.

---
 scalar.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

index 642d16124eb..c5c1ce68919 100644
@@ -819,6 +819,25 @@ static int cmd_delete(int argc, const char **argv)
 	return res;
 }
 
+static int cmd_help(int argc, const char **argv)
+{
+	struct option options[] = {
+		OPT_END(),
+	};
+	const char * const usage[] = {
+		"scalar help",
+		NULL
+	};
+
+	argc = parse_options(argc, argv, NULL, options,
+			     usage, 0);
+
+	if (argc != 0)
+		usage_with_options(usage, options);
+
+	return run_git("help", "scalar", NULL);
+}
+
 static int cmd_version(int argc, const char **argv)
 {
 	int verbose = 0, build_options = 0;

@@ -858,6 +877,7 @@ static struct {
 	{ "run", cmd_run },
 	{ "reconfigure", cmd_reconfigure },
 	{ "delete", cmd_delete },
+	{ "help", cmd_help },
 	{ "version", cmd_version },
 	{ "diagnose", cmd_diagnose },
 	{ NULL, NULL},

gitgitgadget

From: Johannes Schindelin <[email protected]>

With this commit, git help scalar will open the appropriate manual or HTML page (instead of looking for gitscalar).

---
 builtin/help.c | 2 ++
 1 file changed, 2 insertions(+)

index 09ac4289f13..6f2796f211e 100644
@@ -440,6 +440,8 @@ static const char *cmd_to_page(const char *git_cmd)
 		return git_cmd;
 	else if (is_git_command(git_cmd))
 		return xstrfmt("git-%s", git_cmd);
+	else if (!strcmp("scalar", git_cmd))
+		return xstrdup(git_cmd);
 	else
 		return xstrfmt("git%s", git_cmd);
 }

gitgitgadget

From: Victoria Dye <[email protected]>

Rename the last section header in 'contrib/scalar/scalar.txt' from "Scalar" to "GIT". The linting rules of the 'documentation' CI build enforce the existence of a "GIT" section in command documentation. Although 'scalar.txt' is not yet checked, it will be in a future patch.

Here, changing the header name is more appropriate than making a Scalar-specific exception to the linting rule. The existing "Scalar" section contains only a link back to the main Git documentation, essentially the same as the "GIT" section in builtin documentation. Changing the section name further clarifies the Scalar-Git association and maintains consistency with the rest of Git.

---
 contrib/scalar/scalar.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 1a12dc45077..f33436c7f65 100644
@@ -161,6 +161,6 @@ SEE ALSO
 --------
 linkgit:git-clone[1], linkgit:git-maintenance[1].
 
-Scalar
+GIT
 ---
-Associated with the linkgit:git[1] suite
+Part of the linkgit:git[1] suite

gitgitgadget

This series completes the initial implementation of the Scalar command as a core component of Git. For readers new to the topic of Scalar, the roadmap/design doc [1] provides some background information including how the project started & evolved, as well as its current intent & high-level design.

Changes since V1
================

* Updated 'scalar.txt' description from "Associated with [git]" -> "Part of [git]".
* Fixed Dscho's "Signed-off-by" and author emails on patches 3 & 4.
* Added 'scalar' to 'git help -a' command list.
* Removed translation tag for literal 'scalar help' usage string.
* Updated 'scalar clone' tests to clone to unique targets and clean up only when test passes.
* Changed 'p9210' perf tests to invoke 'git' and 'scalar' with '-C', rather than creating a subshell to run commands.
* Fixed use of 'test_env_bool' with 'GIT_PERF_USE_SCALAR', locally tested to make sure it was working as-intended.

Goals
=====

The goal of this series is, as noted in the roadmap linked earlier, to integrate Scalar as a core user-facing component of Git. There's no individual existing precedent that the integration is 100% based on; it's more of an amalgam of the approaches used with builtins installed directly to the bindir (e.g. 'git-upload-pack'), contributed tools (e.g. 'gitk'), and the 'git' executable itself. The resulting behavior is:

* Scalar is built & installed unconditionally (like a builtin).
* Scalar is included in 'bin-wrappers/' and is installed into the 'bin/' directory (like 'git-upload-pack').
* Scalar's documentation is installed as 'man1' docs (like 'gitk').
* Scalar is not invoked via the 'git' executable (it's called directly, like 'git' itself).

The move out of 'contrib/' is also intended to mean that the relationship between Git and Scalar should no longer be a strict one-way dependency. In this series, this architectural change is what permits making a change to 'help.c' specifically for the sake of Scalar. Future contributions can and should (where appropriate!) share code between Scalar and non-Scalar parts of Git.

Organization
============

The series is broken up as follows:

1. Fix a CI-breaking documentation error in 'scalar.txt' (patch 1).
2. Move Scalar code out of 'contrib/' and into core Git, including changes in build definition(s) to build & install Scalar and its resources (patch 2).
3. Make 'git help scalar' open the 'scalar' command documentation, introduce 'scalar help' as an alias. This is done after moving out of 'contrib/' due to it involving Scalar-related changes to the non-Scalar file 'help.c' (patches 3-4).
4. Expand test coverage, including perf testing (patches 5-6)
5. Add a perf test configuration allowing users to register standard perf test repos with Scalar (patch 7).
6. Remove the internally-focused Scalar roadmap from 'Documentation/technical/scalar.txt', include it as one of the HTML technical docs (patch 8).

Prior series
============

* Add 'scalar' command in 'contrib/':
https://lore.kernel.org/git/[email protected]/
* Introduce 'scalar diagnose':
https://lore.kernel.org/git/[email protected]/
* Add '-c/-C' compatibility:
https://lore.kernel.org/git/[email protected]/
* [DROPPED] Integrate Scalar into CI builds:
https://lore.kernel.org/git/[email protected]/
* Document Scalar's role in Git & plan remaining work:
https://lore.kernel.org/git/[email protected]/
* Generalize 'scalar diagnose' into 'git diagnose' builtin & 'git bugreport --diagnose':
https://lore.kernel.org/git/[email protected]/
* Add FSMonitor support to Scalar, refactor enlistment search:
https://lore.kernel.org/git/[email protected]/

Thanks!

* Victoria

[1]
https://lore.kernel.org/git/65eda9755d2a29e222bf7e6cc719921b24377cc6.1657584367.git.gitgitgadget@gmail.com/

Johannes Schindelin (2):
git help: special-case scalar
scalar: implement the help subcommand

Victoria Dye (7):
scalar: fix command documentation section header
scalar: include in standard Git build & installation
scalar: add to 'git help -a' command list
scalar-clone: add test coverage
t/perf: add Scalar performance tests
t/perf: add 'GIT_PERF_USE_SCALAR' run option Documentation/technical: include Scalar technical doc

 .gitignore                                    |   1 +
 Documentation/Makefile                        |   2 +
 Documentation/cmd-list.perl                   |   2 +-
 {contrib/scalar => Documentation}/scalar.txt  |   4 +-
 Documentation/technical/scalar.txt            |  61 -------
 Makefile                                      |  31 ++--
 builtin/help.c                                |   2 +
 command-list.txt                              |   1 +
 contrib/buildsystems/CMakeLists.txt           |   9 +-
 contrib/scalar/.gitignore                     |   2 -
 contrib/scalar/Makefile                       |  35 ----
 contrib/scalar/t/Makefile                     |  81 ----------
 contrib/scalar/scalar.c => scalar.c           |  20 +++
 t/perf/README                                 |   4 +
 t/perf/p9210-scalar.sh                        |  39 +++++
 t/perf/perf-lib.sh                            |  13 +-
 t/perf/run                                    |   3 +
 .../t/t9099-scalar.sh => t/t9210-scalar.sh    |  10 +-
 t/t9211-scalar-clone.sh                       | 151 ++++++++++++++++++
 19 files changed, 264 insertions(+), 207 deletions(-)
 rename {contrib/scalar => Documentation}/scalar.txt (99%)
 delete mode 100644 contrib/scalar/.gitignore
 delete mode 100644 contrib/scalar/Makefile
 delete mode 100644 contrib/scalar/t/Makefile
 rename contrib/scalar/scalar.c => scalar.c (98%)
 create mode 100755 t/perf/p9210-scalar.sh
 rename contrib/scalar/t/t9099-scalar.sh => t/t9210-scalar.sh (96%)
 create mode 100755 t/t9211-scalar-clone.sh

base-commit: d42b38dfb5edf1a7fddd9542d722f91038407819
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1341%2Fvdye%2Ffeature%2Fscalar-toplevel-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1341/vdye/feature/scalar-toplevel-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1341

Range-diff vs v1:

  1:  fcf73e77574 !  1:  614cbdc629d scalar: fix command documentation section header
     @@ contrib/scalar/scalar.txt: SEE ALSO
      -Scalar
      +GIT
       ---
     - Associated with the linkgit:git[1] suite
     +-Associated with the linkgit:git[1] suite
     ++Part of the linkgit:git[1] suite
  2:  4d69e5eaccb !  2:  bc2092a7a7a scalar: include in standard Git build & installation
     @@ Makefile: XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
       	--keyword=__ --keyword=N__ --keyword="__n:1,2"
       MSGMERGE_FLAGS = --add-location --backup=off --update
      -LOCALIZED_C = $(sort $(FOUND_C_SOURCES) $(FOUND_H_SOURCES) $(SCALAR_SOURCES) \
     -+LOCALIZED_C = $(sort $(FOUND_C_SOURCES) $(FOUND_H_SOURCES) \
     - 	        $(GENERATED_H))
     +-	        $(GENERATED_H))
     ++LOCALIZED_C = $(sort $(FOUND_C_SOURCES) $(FOUND_H_SOURCES) $(GENERATED_H))
       LOCALIZED_SH = $(sort $(SCRIPT_SH) git-sh-setup.sh)
       LOCALIZED_PERL = $(sort $(SCRIPT_PERL))
     + 
      @@ Makefile: bin-wrappers/%: wrap-for-bin.sh
       	$(call mkdir_p_parent_template)
       	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
  3:  969160650aa =  3:  eb8663e0300 git help: special-case `scalar`
  4:  46d0fddfe8f !  4:  55aba052c34 scalar: implement the `help` subcommand
     @@
       ## Metadata ##
     -Author: Johannes Schindelin <[email protected]>
     +Author: Johannes Schindelin <[email protected]>

       ## Commit message ##
          scalar: implement the `help` subcommand
      
          It is merely handing off to `git help scalar`.
      
     -    Signed-off-by: Johannes Schindelin <[email protected]>
     +    Signed-off-by: Johannes Schindelin <[email protected]>
          Signed-off-by: Victoria Dye <[email protected]>
      
       ## scalar.c ##
     @@ scalar.c: static int cmd_delete(int argc, const char **argv)
      +		OPT_END(),
      +	};
      +	const char * const usage[] = {
     -+		N_("scalar help"),
     ++		"scalar help",
      +		NULL
      +	};
      +
  -:  ----------- >  5:  070f195f027 scalar: add to 'git help -a' command list
  5:  a3b7cb0a3bd !  6:  15acf20274b scalar-clone: add test coverage
     @@ Commit message
          scalar-clone: add test coverage
      
          Create a new test file ('t9211-scalar-clone.sh') to exercise the options and
     -    behavior of the 'scalar clone' command.
     +    behavior of the 'scalar clone' command. Each test clones to a unique target
     +    location and cleans up the cloned repo only when the test passes. This
     +    ensures that failed tests' artifacts are captured in CI artifacts for
     +    further debugging.
      
     +    Helped-by: Johannes Schindelin <[email protected]>
          Signed-off-by: Victoria Dye <[email protected]>
      
       ## t/t9211-scalar-clone.sh (new) ##
     @@ t/t9211-scalar-clone.sh (new)
      +}
      +
      +test_expect_success 'creates content in enlistment root' '
     -+	test_when_finished cleanup_clone cloned &&
     ++	enlistment=cloned &&
      +
     -+	scalar clone "file://$(pwd)/to-clone" cloned &&
     -+	ls -A cloned >enlistment-root &&
     ++	scalar clone "file://$(pwd)/to-clone" $enlistment &&
     ++	ls -A $enlistment >enlistment-root &&
      +	test_line_count = 1 enlistment-root &&
     -+	test_path_is_dir cloned/src &&
     -+	test_path_is_dir cloned/src/.git
     ++	test_path_is_dir $enlistment/src &&
     ++	test_path_is_dir $enlistment/src/.git &&
     ++
     ++	cleanup_clone $enlistment
      +'
      +
      +test_expect_success 'with spaces' '
     -+	test_when_finished cleanup_clone "cloned with space" &&
     ++	enlistment="cloned with space" &&
     ++
     ++	scalar clone "file://$(pwd)/to-clone" "$enlistment" &&
     ++	test_path_is_dir "$enlistment" &&
     ++	test_path_is_dir "$enlistment/src" &&
     ++	test_path_is_dir "$enlistment/src/.git" &&
      +
     -+	scalar clone "file://$(pwd)/to-clone" "cloned with space" &&
     -+	test_path_is_dir "cloned with space" &&
     -+	test_path_is_dir "cloned with space/src" &&
     -+	test_path_is_dir "cloned with space/src/.git"
     ++	cleanup_clone "$enlistment"
      +'
      +
      +test_expect_success 'partial clone if supported by server' '
     -+	test_when_finished cleanup_clone cloned &&
     ++	enlistment=partial-clone &&
      +
     -+	scalar clone "file://$(pwd)/to-clone" cloned &&
     ++	scalar clone "file://$(pwd)/to-clone" $enlistment &&
      +
      +	(
     -+		cd cloned/src &&
     ++		cd $enlistment/src &&
      +
      +		# Two promisor packs: one for refs, the other for blobs
      +		ls .git/objects/pack/pack-*.promisor >promisorlist &&
      +		test_line_count = 2 promisorlist
     -+	)
     ++	) &&
     ++
     ++	cleanup_clone $enlistment
      +'
      +
      +test_expect_success 'fall back on full clone if partial unsupported' '
     -+	test_when_finished cleanup_clone cloned &&
     ++	enlistment=no-partial-support &&
      +
      +	test_config -C to-clone uploadpack.allowfilter false &&
      +	test_config -C to-clone uploadpack.allowanysha1inwant false &&
      +
     -+	scalar clone "file://$(pwd)/to-clone" cloned 2>err &&
     ++	scalar clone "file://$(pwd)/to-clone" $enlistment 2>err &&
      +	grep "filtering not recognized by server, ignoring" err &&
      +
      +	(
     -+		cd cloned/src &&
     ++		cd $enlistment/src &&
      +
      +		# Still get a refs promisor file, but none for blobs
      +		ls .git/objects/pack/pack-*.promisor >promisorlist &&
      +		test_line_count = 1 promisorlist
     -+	)
     ++	) &&
     ++
     ++	cleanup_clone $enlistment
      +'
      +
      +test_expect_success 'initializes sparse-checkout by default' '
     -+	test_when_finished cleanup_clone cloned &&
     ++	enlistment=sparse &&
      +
     -+	scalar clone "file://$(pwd)/to-clone" cloned &&
     ++	scalar clone "file://$(pwd)/to-clone" $enlistment &&
      +	(
     -+		cd cloned/src &&
     ++		cd $enlistment/src &&
      +		test_cmp_config true core.sparseCheckout &&
      +		test_cmp_config true core.sparseCheckoutCone
     -+	)
     ++	) &&
     ++
     ++	cleanup_clone $enlistment
      +'
      +
      +test_expect_success '--full-clone does not create sparse-checkout' '
     -+	test_when_finished cleanup_clone cloned &&
     ++	enlistment=full-clone &&
      +
     -+	scalar clone --full-clone "file://$(pwd)/to-clone" cloned &&
     ++	scalar clone --full-clone "file://$(pwd)/to-clone" $enlistment &&
      +	(
     -+		cd cloned/src &&
     ++		cd $enlistment/src &&
      +		test_cmp_config "" --default "" core.sparseCheckout &&
      +		test_cmp_config "" --default "" core.sparseCheckoutCone
     -+	)
     ++	) &&
     ++
     ++	cleanup_clone $enlistment
      +'
      +
      +test_expect_success '--single-branch clones HEAD only' '
     -+	test_when_finished cleanup_clone cloned &&
     ++	enlistment=single-branch &&
      +
     -+	scalar clone --single-branch "file://$(pwd)/to-clone" cloned &&
     ++	scalar clone --single-branch "file://$(pwd)/to-clone" $enlistment &&
      +	(
     -+		cd cloned/src &&
     ++		cd $enlistment/src &&
      +		git for-each-ref refs/remotes/origin >out &&
      +		test_line_count = 1 out &&
      +		grep "refs/remotes/origin/base" out
     -+	)
     ++	) &&
     ++
     ++	cleanup_clone $enlistment
      +'
      +
      +test_expect_success '--no-single-branch clones all branches' '
     -+	test_when_finished cleanup_clone cloned &&
     ++	enlistment=no-single-branch &&
      +
     -+	scalar clone --no-single-branch "file://$(pwd)/to-clone" cloned &&
     ++	scalar clone --no-single-branch "file://$(pwd)/to-clone" $enlistment &&
      +	(
     -+		cd cloned/src &&
     ++		cd $enlistment/src &&
      +		git for-each-ref refs/remotes/origin >out &&
      +		test_line_count = 2 out &&
      +		grep "refs/remotes/origin/base" out &&
      +		grep "refs/remotes/origin/parallel" out
     -+	)
     ++	) &&
     ++
     ++	cleanup_clone $enlistment
      +'
      +
      +test_done
  6:  42ab39f2121 !  7:  d5b68c65163 t/perf: add Scalar performance tests
     @@ Commit message
          9210.6: test_commit --append --no-tag A (scalar)       0.08(0.02+0.03)
          9210.7: test_commit --append --no-tag A (non-scalar)   0.13(0.03+0.11)
      
     +    Helped-by: Johannes Schindelin <[email protected]>
          Signed-off-by: Victoria Dye <[email protected]>
      
       ## t/perf/p9210-scalar.sh (new) ##
     @@ t/perf/p9210-scalar.sh (new)
      +'
      +
      +test_compare_perf () {
     -+	command="$@"
     -+	test_perf "$command (scalar)" "
     -+		(
     -+			cd scalar-clone/src &&
     -+			$command
     -+		)
     ++	command=$1
     ++	shift
     ++	args=$*
     ++	test_perf "$command $args (scalar)" "
     ++		$command -C scalar-clone/src $args
      +	"
      +
     -+	test_perf "$command (non-scalar)" "
     -+		(
     -+			cd git-clone &&
     -+			$command
     -+		)
     ++	test_perf "$command $args (non-scalar)" "
     ++		$command -C git-clone $args
      +	"
      +}
      +
  7:  96e08a95265 !  8:  2e6dd03beba t/perf: add 'GIT_PERF_USE_SCALAR' run option
     @@ t/perf/perf-lib.sh: test_perf_create_repo_from () {
       			# a copy it's fine to remove the lock.
       			rm .git/index.lock
      +		fi &&
     -+		if test_bool_env "$GIT_PERF_USE_SCALAR" false
     ++		if test_bool_env GIT_PERF_USE_SCALAR false
      +		then
      +			"$MODERN_SCALAR" register
       		fi
     @@ t/perf/perf-lib.sh: test_perf_fresh_repo () {
       		cd "$repo" &&
      -		test_perf_do_repo_symlink_config_
      +		test_perf_do_repo_symlink_config_ &&
     -+		if test_bool_env "$GIT_PERF_USE_SCALAR" false
     ++		if test_bool_env GIT_PERF_USE_SCALAR false
      +		then
      +			"$MODERN_SCALAR" register
      +		fi

8: 8bbe913dccf = 9: 6bedd80c398 Documentation/technical: include Scalar technical doc

gitgitgadget

From: Victoria Dye <[email protected]>

Create 'p9210-scalar.sh' for testing Scalar performance and comparing performance of Git operations in Scalar registrations and standard repositories. Example results:

Test this tree
------------------------------------------------------------------------

9210.2: scalar clone                                   14.82(18.00+3.63)
9210.3: git clone                                      26.15(36.67+6.90)
9210.4: git status (scalar)                            0.04(0.01+0.01)
9210.5: git status (non-scalar)                        0.10(0.02+0.11)
9210.6: test_commit --append --no-tag A (scalar)       0.08(0.02+0.03)
9210.7: test_commit --append --no-tag A (non-scalar)   0.13(0.03+0.11)

---
 t/perf/p9210-scalar.sh | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100755 t/perf/p9210-scalar.sh

new file mode 100755
index 00000000000..265f7cd1fe2
@@ -0,0 +1,39 @@
+#!/bin/sh
+
+test_description='test scalar performance'
+. ./perf-lib.sh
+
+test_perf_large_repo "$TRASH_DIRECTORY/to-clone"
+
+test_expect_success 'enable server-side partial clone' '
+	git -C to-clone config uploadpack.allowFilter true &&
+	git -C to-clone config uploadpack.allowAnySHA1InWant true
+'
+
+test_perf 'scalar clone' '
+	rm -rf scalar-clone &&
+	scalar clone "file://$(pwd)/to-clone" scalar-clone
+'
+
+test_perf 'git clone' '
+	rm -rf git-clone &&
+	git clone "file://$(pwd)/to-clone" git-clone
+'
+
+test_compare_perf () {
+	command=$1
+	shift
+	args=$*
+	test_perf "$command $args (scalar)" "
+		$command -C scalar-clone/src $args
+	"
+
+	test_perf "$command $args (non-scalar)" "
+		$command -C git-clone $args
+	"
+}
+
+test_compare_perf git status
+test_compare_perf test_commit --append --no-tag A
+
+test_done

gitgitgadget

From: Victoria Dye <[email protected]>

Create a new test file ('t9211-scalar-clone.sh') to exercise the options and behavior of the 'scalar clone' command. Each test clones to a unique target location and cleans up the cloned repo only when the test passes. This ensures that failed tests' artifacts are captured in CI artifacts for further debugging.

---
 t/t9211-scalar-clone.sh | 151 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 151 insertions(+)
 create mode 100755 t/t9211-scalar-clone.sh

new file mode 100755
index 00000000000..dd33d87e9be
@@ -0,0 +1,151 @@
+#!/bin/sh
+
+test_description='test the `scalar clone` subcommand'
+
+. ./test-lib.sh
+
+GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt,launchctl:true,schtasks:true"
+export GIT_TEST_MAINT_SCHEDULER
+
+test_expect_success 'set up repository to clone' '
+	rm -rf .git &&
+	git init to-clone &&
+	(
+		cd to-clone &&
+		git branch -m base &&
+
+		test_commit first &&
+		test_commit second &&
+		test_commit third &&
+
+		git switch -c parallel first &&
+		mkdir -p 1/2 &&
+		test_commit 1/2/3 &&
+
+		git switch base &&
+
+		# By default, permit
+		git config uploadpack.allowfilter true &&
+		git config uploadpack.allowanysha1inwant true
+	)
+'
+
+cleanup_clone () {
+	rm -rf "$1"
+}
+
+test_expect_success 'creates content in enlistment root' '
+	enlistment=cloned &&
+
+	scalar clone "file://$(pwd)/to-clone" $enlistment &&
+	ls -A $enlistment >enlistment-root &&
+	test_line_count = 1 enlistment-root &&
+	test_path_is_dir $enlistment/src &&
+	test_path_is_dir $enlistment/src/.git &&
+
+	cleanup_clone $enlistment
+'
+
+test_expect_success 'with spaces' '
+	enlistment="cloned with space" &&
+
+	scalar clone "file://$(pwd)/to-clone" "$enlistment" &&
+	test_path_is_dir "$enlistment" &&
+	test_path_is_dir "$enlistment/src" &&
+	test_path_is_dir "$enlistment/src/.git" &&
+
+	cleanup_clone "$enlistment"
+'
+
+test_expect_success 'partial clone if supported by server' '
+	enlistment=partial-clone &&
+
+	scalar clone "file://$(pwd)/to-clone" $enlistment &&
+
+	(
+		cd $enlistment/src &&
+
+		# Two promisor packs: one for refs, the other for blobs
+		ls .git/objects/pack/pack-*.promisor >promisorlist &&
+		test_line_count = 2 promisorlist
+	) &&
+
+	cleanup_clone $enlistment
+'
+
+test_expect_success 'fall back on full clone if partial unsupported' '
+	enlistment=no-partial-support &&
+
+	test_config -C to-clone uploadpack.allowfilter false &&
+	test_config -C to-clone uploadpack.allowanysha1inwant false &&
+
+	scalar clone "file://$(pwd)/to-clone" $enlistment 2>err &&
+	grep "filtering not recognized by server, ignoring" err &&
+
+	(
+		cd $enlistment/src &&
+
+		# Still get a refs promisor file, but none for blobs
+		ls .git/objects/pack/pack-*.promisor >promisorlist &&
+		test_line_count = 1 promisorlist
+	) &&
+
+	cleanup_clone $enlistment
+'
+
+test_expect_success 'initializes sparse-checkout by default' '
+	enlistment=sparse &&
+
+	scalar clone "file://$(pwd)/to-clone" $enlistment &&
+	(
+		cd $enlistment/src &&
+		test_cmp_config true core.sparseCheckout &&
+		test_cmp_config true core.sparseCheckoutCone
+	) &&
+
+	cleanup_clone $enlistment
+'
+
+test_expect_success '--full-clone does not create sparse-checkout' '
+	enlistment=full-clone &&
+
+	scalar clone --full-clone "file://$(pwd)/to-clone" $enlistment &&
+	(
+		cd $enlistment/src &&
+		test_cmp_config "" --default "" core.sparseCheckout &&
+		test_cmp_config "" --default "" core.sparseCheckoutCone
+	) &&
+
+	cleanup_clone $enlistment
+'
+
+test_expect_success '--single-branch clones HEAD only' '
+	enlistment=single-branch &&
+
+	scalar clone --single-branch "file://$(pwd)/to-clone" $enlistment &&
+	(
+		cd $enlistment/src &&
+		git for-each-ref refs/remotes/origin >out &&
+		test_line_count = 1 out &&
+		grep "refs/remotes/origin/base" out
+	) &&
+
+	cleanup_clone $enlistment
+'
+
+test_expect_success '--no-single-branch clones all branches' '
+	enlistment=no-single-branch &&
+
+	scalar clone --no-single-branch "file://$(pwd)/to-clone" $enlistment &&
+	(
+		cd $enlistment/src &&
+		git for-each-ref refs/remotes/origin >out &&
+		test_line_count = 2 out &&
+		grep "refs/remotes/origin/base" out &&
+		grep "refs/remotes/origin/parallel" out
+	) &&
+
+	cleanup_clone $enlistment
+'
+
+test_done

gitgitgadget

From: Victoria Dye <[email protected]>

Move 'scalar' out of 'contrib/' and into the root of the Git tree. The goal of this change is to build 'scalar' as part of the standard Git build & install processes.

This patch includes both the physical move of Scalar's files out of 'contrib/' ('scalar.c', 'scalar.txt', and 't9xxx-scalar.sh'), and the changes to the build definitions in 'Makefile' and 'CMakelists.txt' to accommodate the new program.

At a high level, Scalar is built so that:
- there is a 'scalar-objs' target (similar to those created in 029bac01a8 (Makefile: add {program,xdiff,test,git,fuzz}-objs & objects targets, 2021-02-23)) for debugging purposes.
- it appears in the root of the install directory (rather than the gitexecdir).
- it is included in the 'bin-wrappers/' directory for use in tests.
- it receives a platform-specific executable suffix (e.g., '.exe'), if applicable.
- 'scalar.txt' is installed as 'man1' documentation.
- the 'clean' target removes the 'scalar' executable.

Additionally, update the root level '.gitignore' file to ignore the Scalar executable.

---
 .gitignore                                    |  1 +
 Documentation/Makefile                        |  1 +
 {contrib/scalar => Documentation}/scalar.txt  |  0
 Makefile                                      | 31 ++++---
 contrib/buildsystems/CMakeLists.txt           |  9 ++-
 contrib/scalar/.gitignore                     |  2 -
 contrib/scalar/Makefile                       | 35 --------
 contrib/scalar/t/Makefile                     | 81 -------------------
 contrib/scalar/scalar.c => scalar.c           |  0
 .../t/t9099-scalar.sh => t/t9210-scalar.sh    | 10 +--
 10 files changed, 28 insertions(+), 142 deletions(-)
 rename {contrib/scalar => Documentation}/scalar.txt (100%)
 delete mode 100644 contrib/scalar/.gitignore
 delete mode 100644 contrib/scalar/Makefile
 delete mode 100644 contrib/scalar/t/Makefile
 rename contrib/scalar/scalar.c => scalar.c (100%)
 rename contrib/scalar/t/t9099-scalar.sh => t/t9210-scalar.sh (96%)

index 80b530bbed2..3d1b880101e 100644
@@ -185,6 +185,7 @@
 /git-whatchanged
 /git-worktree
 /git-write-tree
+/scalar
 /git-core-*/?*
 /git.res
 /gitweb/GITWEB-BUILD-OPTIONS

index bd6b6fcb930..16c9e062390 100644
@@ -21,6 +21,7 @@ MAN1_TXT += $(filter-out \
 MAN1_TXT += git.txt
 MAN1_TXT += gitk.txt
 MAN1_TXT += gitweb.txt
+MAN1_TXT += scalar.txt
 
 # man5 / man7 guides (note: new guides should also be added to command-list.txt)
 MAN5_TXT += gitattributes.txt

similarity index 100%
rename from contrib/scalar/scalar.txt
rename to Documentation/scalar.txt

index eac30126e29..e03f32ec1e7 100644
@@ -608,7 +608,9 @@ FUZZ_OBJS =
 FUZZ_PROGRAMS =
 GIT_OBJS =
 LIB_OBJS =
+SCALAR_OBJS =
 OBJECTS =
+OTHER_PROGRAMS =
 PROGRAM_OBJS =
 PROGRAMS =
 EXCLUDED_PROGRAMS =

@@ -821,10 +823,12 @@ BUILT_INS += git-switch$X
 BUILT_INS += git-whatchanged$X
 
 # what 'all' will build but not install in gitexecdir
-OTHER_PROGRAMS = git$X
+OTHER_PROGRAMS += git$X
+OTHER_PROGRAMS += scalar$X
 
 # what test wrappers are needed and 'install' will install, in bindir
 BINDIR_PROGRAMS_NEED_X += git
+BINDIR_PROGRAMS_NEED_X += scalar
 BINDIR_PROGRAMS_NEED_X += git-receive-pack
 BINDIR_PROGRAMS_NEED_X += git-shell
 BINDIR_PROGRAMS_NEED_X += git-upload-archive

@@ -2222,7 +2226,7 @@ profile-fast: profile-clean
 
 all:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(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';)
+	$(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) $(OTHER_PROGRAMS))), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';)
 endif
 
 all::

@@ -2545,7 +2549,12 @@ GIT_OBJS += git.o
 .PHONY: git-objs
 git-objs: $(GIT_OBJS)
 
+SCALAR_OBJS += scalar.o
+.PHONY: scalar-objs
+scalar-objs: $(SCALAR_OBJS)
+
 OBJECTS += $(GIT_OBJS)
+OBJECTS += $(SCALAR_OBJS)
 OBJECTS += $(PROGRAM_OBJS)
 OBJECTS += $(TEST_OBJS)
 OBJECTS += $(XDIFF_OBJS)

@@ -2556,10 +2565,6 @@ ifndef NO_CURL
 	OBJECTS += http.o http-walker.o remote-curl.o
 endif
 
-SCALAR_SOURCES := contrib/scalar/scalar.c
-SCALAR_OBJECTS := $(SCALAR_SOURCES:c=o)
-OBJECTS += $(SCALAR_OBJECTS)
-
 .PHONY: objects
 objects: $(OBJECTS)
 

@@ -2691,7 +2696,7 @@ $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)
 
-contrib/scalar/scalar$X: $(SCALAR_OBJECTS) GIT-LDFLAGS $(GITLIBS)
+scalar$X: scalar.o GIT-LDFLAGS $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
 		$(filter %.o,$^) $(LIBS)
 

@@ -2747,8 +2752,7 @@ XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
 XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
 	--keyword=__ --keyword=N__ --keyword="__n:1,2"
 MSGMERGE_FLAGS = --add-location --backup=off --update
-LOCALIZED_C = $(sort $(FOUND_C_SOURCES) $(FOUND_H_SOURCES) $(SCALAR_SOURCES) \
-	        $(GENERATED_H))
+LOCALIZED_C = $(sort $(FOUND_C_SOURCES) $(FOUND_H_SOURCES) $(GENERATED_H))
 LOCALIZED_SH = $(sort $(SCRIPT_SH) git-sh-setup.sh)
 LOCALIZED_PERL = $(sort $(SCRIPT_PERL))
 

@@ -3062,7 +3066,7 @@ bin-wrappers/%: wrap-for-bin.sh
 	$(call mkdir_p_parent_template)
 	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 	     -e 's|@@BUILD_DIR@@|$(shell pwd)|' \
-	     -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%$(X),$(@F))$(patsubst git%,$(X),$(filter $(@F),$(BINDIR_PROGRAMS_NEED_X)))|' < $< > $@ && \
+	     -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%,$(@F))$(if $(filter-out $(BINDIR_PROGRAMS_NO_X),$(@F)),$(X),)|' < $< > $@ && \
 	chmod +x $@
 
 # GNU make supports exporting all variables by "export" without parameters.

@@ -3276,14 +3280,14 @@ ifndef NO_TCLTK
 	$(MAKE) -C git-gui gitexecdir='$(gitexec_instdir_SQ)' install
 endif
 ifneq (,$X)
-	$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) git$X)), test '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p' -ef '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p$X' || $(RM) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p';)
+	$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) $(OTHER_PROGRAMS))), test '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p' -ef '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p$X' || $(RM) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p';)
 endif
 
 	bindir=$$(cd '$(DESTDIR_SQ)$(bindir_SQ)' && pwd) && \
 	execdir=$$(cd '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' && pwd) && \
 	destdir_from_execdir_SQ=$$(echo '$(gitexecdir_relative_SQ)' | sed -e 's|[^/][^/]*|..|g') && \
 	{ test "$$bindir/" = "$$execdir/" || \
-	  for p in git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); do \
+	  for p in $(OTHER_PROGRAMS) $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); do \
 		$(RM) "$$execdir/$$p" && \
 		test -n "$(INSTALL_SYMLINKS)" && \
 		ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/$$p" "$$execdir/$$p" || \

@@ -3458,7 +3462,7 @@ clean: profile-clean coverage-clean cocciclean
 	$(RM) git.res
 	$(RM) $(OBJECTS)
 	$(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)
-	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
+	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS)
 	$(RM) $(TEST_PROGRAMS)
 	$(RM) $(FUZZ_PROGRAMS)
 	$(RM) $(SP_OBJ)

@@ -3509,6 +3513,7 @@ ALL_COMMANDS += git-citool
 ALL_COMMANDS += git-gui
 ALL_COMMANDS += gitk
 ALL_COMMANDS += gitweb
+ALL_COMMANDS += scalar
 
 .PHONY: check-docs
 check-docs::

index 2237109b57f..bae203c1fb9 100644
@@ -610,7 +610,7 @@ unset(CMAKE_REQUIRED_INCLUDES)
 #programs
 set(PROGRAMS_BUILT
 	git git-daemon git-http-backend git-sh-i18n--envsubst
-	git-shell)
+	git-shell scalar)
 
 if(NOT CURL_FOUND)
 	list(APPEND excluded_progs git-http-fetch git-http-push)

@@ -757,6 +757,9 @@ target_link_libraries(git-sh-i18n--envsubst common-main)
 add_executable(git-shell ${CMAKE_SOURCE_DIR}/shell.c)
 target_link_libraries(git-shell common-main)
 
+add_executable(scalar ${CMAKE_SOURCE_DIR}/scalar.c)
+target_link_libraries(scalar common-main)
+
 if(CURL_FOUND)
 	add_library(http_obj OBJECT ${CMAKE_SOURCE_DIR}/http.c)
 

@@ -903,7 +906,7 @@ list(TRANSFORM git_perl_scripts PREPEND "${CMAKE_BINARY_DIR}/")
 
 #install
 foreach(program ${PROGRAMS_BUILT})
-if(program STREQUAL "git" OR program STREQUAL "git-shell")
+if(program MATCHES "^(git|git-shell|scalar)$")
 install(TARGETS ${program}
 	RUNTIME DESTINATION bin)
 else()

@@ -977,7 +980,7 @@ endif()
 
 #wrapper scripts
 set(wrapper_scripts
-	git git-upload-pack git-receive-pack git-upload-archive git-shell git-remote-ext)
+	git git-upload-pack git-receive-pack git-upload-archive git-shell git-remote-ext scalar)
 
 set(wrapper_test_scripts
 	test-fake-ssh test-tool)

deleted file mode 100644
index ff3d47e84d0..00000000000
@@ -1,2 +0,0 @@
-/*.exe
-/scalar

deleted file mode 100644
index 37f283f35d7..00000000000
@@ -1,35 +0,0 @@
-# The default target of this Makefile is...
-all::
-
-# Import tree-wide shared Makefile behavior and libraries
-include ../../shared.mak
-
-include ../../config.mak.uname
--include ../../config.mak.autogen
--include ../../config.mak
-
-TARGETS = scalar$(X) scalar.o
-GITLIBS = ../../common-main.o ../../libgit.a ../../xdiff/lib.a
-
-all:: scalar$(X) ../../bin-wrappers/scalar
-
-$(GITLIBS):
-	$(QUIET_SUBDIR0)../.. $(QUIET_SUBDIR1) $(subst ../../,,$@)
-
-$(TARGETS): $(GITLIBS) scalar.c
-	$(QUIET_SUBDIR0)../.. $(QUIET_SUBDIR1) $(patsubst %,contrib/scalar/%,$@)
-
-clean:
-	$(RM) $(TARGETS) ../../bin-wrappers/scalar
-
-../../bin-wrappers/scalar: ../../wrap-for-bin.sh Makefile
-	@mkdir -p ../../bin-wrappers
-	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
-	     -e 's|@@BUILD_DIR@@|$(shell cd ../.. && pwd)|' \
-	     -e 's|@@PROG@@|contrib/scalar/scalar$(X)|' < $< > $@ && \
-	chmod +x $@
-
-test: all
-	$(MAKE) -C t
-
-.PHONY: $(GITLIBS) all clean test FORCE

similarity index 100%
rename from contrib/scalar/scalar.c
rename to scalar.c

similarity index 96%
rename from contrib/scalar/t/t9099-scalar.sh
rename to t/t9210-scalar.sh
index dfb949f52ee..14ca575a214 100755
@@ -2,15 +2,9 @@
 
 test_description='test the `scalar` command'
 
-TEST_DIRECTORY=$PWD/../../../t
-export TEST_DIRECTORY
+. ./test-lib.sh
 
-# Make it work with --no-bin-wrappers
-PATH=$PWD/..:$PATH
-
-. ../../../t/test-lib.sh
-
-GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab ../cron.txt,launchctl:true,schtasks:true"
+GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt,launchctl:true,schtasks:true"
 export GIT_TEST_MAINT_SCHEDULER
 
 test_expect_success 'scalar shows a usage' '

gitgitgadget

From: Victoria Dye <[email protected]>

Add a 'GIT_PERF_USE_SCALAR' environment variable (and corresponding perf config 'useScalar') to register a repository created with any of:

* test_perf_fresh_repo
* test_perf_default_repo
* test_perf_large_repo

as a Scalar enlistment. This is intended to allow a developer to test the impact of Scalar on already-defined performance scenarios.

---
 t/perf/README      |  4 ++++
 t/perf/perf-lib.sh | 13 ++++++++++++-
 t/perf/run         |  3 +++
 3 files changed, 19 insertions(+), 1 deletion(-)

index fb9127a66f7..8f217d7be7d 100644
@@ -95,6 +95,10 @@ You can set the following variables (also in your config.mak):
 	Git (e.g., performance of index-pack as the number of threads
 	changes). These can be enabled with GIT_PERF_EXTRA.
 
+    GIT_PERF_USE_SCALAR
+	Boolean indicating whether to register test repo(s) with Scalar
+	before executing tests.
+
 You can also pass the options taken by ordinary git tests; the most
 useful one is:
 

index 27c28017921..e7786775a90 100644
@@ -49,6 +49,9 @@ export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP
 MODERN_GIT=$GIT_BUILD_DIR/bin-wrappers/git
 export MODERN_GIT
 
+MODERN_SCALAR=$GIT_BUILD_DIR/bin-wrappers/scalar
+export MODERN_SCALAR
+
 perf_results_dir=$TEST_RESULTS_DIR
 test -n "$GIT_PERF_SUBSECTION" && perf_results_dir="$perf_results_dir/$GIT_PERF_SUBSECTION"
 mkdir -p "$perf_results_dir"

@@ -120,6 +123,10 @@ test_perf_create_repo_from () {
 			# status" due to a locked index. Since we have
 			# a copy it's fine to remove the lock.
 			rm .git/index.lock
+		fi &&
+		if test_bool_env GIT_PERF_USE_SCALAR false
+		then
+			"$MODERN_SCALAR" register
 		fi
 	) || error "failed to copy repository '$source' to '$repo'"
 }

@@ -130,7 +137,11 @@ test_perf_fresh_repo () {
 	"$MODERN_GIT" init -q "$repo" &&
 	(
 		cd "$repo" &&
-		test_perf_do_repo_symlink_config_
+		test_perf_do_repo_symlink_config_ &&
+		if test_bool_env GIT_PERF_USE_SCALAR false
+		then
+			"$MODERN_SCALAR" register
+		fi
 	)
 }
 

index 55219aa4056..33da4d2aba2 100755
@@ -171,6 +171,9 @@ run_subsection () {
 	get_var_from_env_or_config "GIT_PERF_MAKE_COMMAND" "perf" "makeCommand"
 	get_var_from_env_or_config "GIT_PERF_MAKE_OPTS" "perf" "makeOpts"
 
+	get_var_from_env_or_config "GIT_PERF_USE_SCALAR" "perf" "useScalar" "--bool"
+	export GIT_PERF_USE_SCALAR
+
 	get_var_from_env_or_config "GIT_PERF_REPO_NAME" "perf" "repoName"
 	export GIT_PERF_REPO_NAME
 

gitgitgadget

From: Victoria Dye <[email protected]>

Include 'Documentation/technical/scalar.txt' alongside the other HTML technical docs when installing them.

Now that the document is intended as a widely-accessible reference, remove the internal work-in-progress roadmap from the document. Those details should no longer be needed to guide Scalar's development and, if they were left, they could fall out-of-date and be misleading to readers.

---
 Documentation/Makefile             |  1 +
 Documentation/technical/scalar.txt | 61 ------------------------------
 2 files changed, 1 insertion(+), 61 deletions(-)

index 16c9e062390..9ec53afdf18 100644
@@ -116,6 +116,7 @@ TECH_DOCS += technical/parallel-checkout
 TECH_DOCS += technical/partial-clone
 TECH_DOCS += technical/racy-git
 TECH_DOCS += technical/reftable
+TECH_DOCS += technical/scalar
 TECH_DOCS += technical/send-pack-pipeline
 TECH_DOCS += technical/shallow
 TECH_DOCS += technical/trivial-merge

gitgitgadget

From: Victoria Dye <[email protected]>

Add 'scalar' as a 'mainporcelain' command in the Git command list. Update the regex in 'cmd-list.perl' used to match the first line of command documentation to find 'scalar(1)'.

---
 Documentation/cmd-list.perl | 2 +-
 command-list.txt            | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

index af5da45d287..9515a499a31 100755
@@ -10,7 +10,7 @@ sub format_one {
 	$state = 0;
 	open I, '<', "$name.txt" or die "No such file $name.txt";
 	while (<I>) {
-		if (/^git[a-z0-9-]*\(([0-9])\)$/) {
+		if (/^(git|scalar)[a-z0-9-]*\(([0-9])\)$/) {
 			$mansection = $1;
 			next;
 		}

index f96bdabd7d9..93f94e42ab7 100644
@@ -235,3 +235,4 @@ gittutorial                             guide
 gittutorial-2                           guide
 gitweb                                  ancillaryinterrogators
 gitworkflows                            guide
+scalar                                  mainporcelain

gitgitgadget

I agree that it would in general be a good idea to leave more cruft for those who want to postmortem. It will nudge a sequence of tests to use distinct test directories and output files, in order to avoid letting the later ones overwrite the earlier ones, which would be a deviation from what we have done historically.

For those who diagnose breakage manually after seeing "tXXXX.sh -i" fail, cruft left by earlier successful tests are quite distracting nuisance and that certainly was why we tried to remove them after each test piece succeeds. These days, we seem to be relying much more on unattended bulk tests at CI than before, which changes the equation.

Hi Victoria,

This iteration addresses all concerns I had.

Thank you so much!
Dscho

I just took a read through this series, and everything seemed well justified and cleanly implemented. I built and installed this version of Scalar locally as a way to see that all the build steps worked as expected.

Thanks!
-Stolee

This simple topic fixes issues with the bin-wrappers/% rules, as seen in the range-diff below this never worked properly:

make bin-wrappers/git

I.e. we'd make the scirpt, but not "git".

I originally sent this as [1] in response to a topic that's since landed, i.e. that topic needed to add more conditions to the "match this, but not that" part of the current rule.

As 3/3 notes that's because we were previously squashing 3 Makefile variables into one, and then having to heurisitcally match their contents to figure out which item originally came from which variable.

The 3/3 here simply avoids squashing all that data together, so we don't need to guess after the fact.

For the v2 I squashed the previous 3 commits together, they were incrementally improving the rule, now we do it all at once.

1. https://lore.kernel.org/git/[email protected]/

Ævar Arnfjörð Bjarmason (3):
Makefile: factor sed-powered '#!/bin/sh' munging into a variable
Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier
Makefile: simplify $(test_bindir_programs) rule by splitting it up

 Makefile | 67 ++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 48 insertions(+), 19 deletions(-)

Range-diff against v1:

1:  40033143cdd = 1:  fc6c5a6a8df Makefile: factor sed-powered '#!/bin/sh' munging into a variable
2:  fe54dacaad2 = 2:  6dcb49f25c4 Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier
3:  9d4ac628f0c ! 3:  400f487e30d Makefile: simplify $(test_bindir_programs) rule by splitting it up
    @@ Commit message
         Which will show an empty diff, i.e. we've correctly dealt with the
         combination of $(SHELL_PATH), $(X) and these three variables here.
     
    +    This also fixes an issue with the "bin-wrappers/" scripts have never had properly declared
    +    dependency information, i.e. this has never worked:
    +
    +            make clean &&
    +            make bin-wrappers/git &&
    +            # the script is there, but no "./git" is built
    +            ./bin-wrappers/git
    +
    +    There was no reason not to have that work, just as most things
    +    generated by the Makefile have proper dependency information.
    +
    +    Before this commit doing this would have been painful, but now it's
    +    easy to pass this as a parameter to our "bin_wrappers_template"
    +
         1. ea925196f1b (build dashless "bin-wrappers" directory similar to
            installed bindir, 2009-12-02)
         2. e6e7530d10b (test helpers: move test-* to t/helper/ subdirectory,
    @@ Makefile: GIT-PYTHON-VARS: FORCE
     +endef
      
     -all:: $(test_bindir_programs)
    -+BW_BINDIR_PROGRAMS_NEED_X = $(BINDIR_PROGRAMS_NEED_X:%=bin-wrappers/%)
    -+BIN_WRAPPERS += $(BW_BINDIR_PROGRAMS_NEED_X)
    -+$(BW_BINDIR_PROGRAMS_NEED_X): wrap-for-bin.sh
    -+	$(call mkdir_p_parent_template)
    -+	$(QUIET_GEN)$(call cmd_munge_bin_wrappers_script,$(@F),,$X)
    ++define bin_wrappers_template
    ++
    ++## bin_wrappers_template
    ++# 1 = $(1)
    ++# 2 = $(2)
    ++# 3 = $(3)
    ++# 4 = $(4)
    ++BW_$(1) = $$($(1):%=bin-wrappers/%)
    ++BIN_WRAPPERS += $$(BW_$(1))
    ++all:: $$(BW_$(1))
    ++$$(BW_$(1)): bin-wrappers/% : $(3)%$(4)
    ++$$(BW_$(1)): wrap-for-bin.sh
    ++	$$(call mkdir_p_parent_template)
    ++	$$(QUIET_GEN)$$(call cmd_munge_bin_wrappers_script,$(2),$(3),$(4))
    ++endef
      
     -bin-wrappers/%: wrap-for-bin.sh
    -+BW_BINDIR_PROGRAMS_NO_X = $(BINDIR_PROGRAMS_NO_X:%=bin-wrappers/%)
    -+BIN_WRAPPERS += $(BW_BINDIR_PROGRAMS_NO_X)
    -+$(BW_BINDIR_PROGRAMS_NO_X): wrap-for-bin.sh
    - 	$(call mkdir_p_parent_template)
    +-	$(call mkdir_p_parent_template)
     -	$(QUIET_GEN)sed -e $(call cmd_munge_script_sed_shell_path_arg) \
     -	     -e 's|@@BUILD_DIR@@|$(shell pwd)|' \
     -	     -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%,$(@F))$(if $(filter-out $(BINDIR_PROGRAMS_NO_X),$(@F)),$(X),)|' < $< > $@ && \
     -	chmod +x $@
    -+	$(QUIET_GEN)$(call cmd_munge_bin_wrappers_script,$(@F))
    -+
    -+BW_TEST_PROGRAMS_NEED_X = $(TEST_PROGRAMS_NEED_X:%=bin-wrappers/%)
    -+BIN_WRAPPERS += $(BW_TEST_PROGRAMS_NEED_X)
    -+$(BW_TEST_PROGRAMS_NEED_X): wrap-for-bin.sh
    -+	$(call mkdir_p_parent_template)
    -+	$(QUIET_GEN)$(call cmd_munge_bin_wrappers_script,$(@F),t/helper/,$X)
    ++define bin_wrappers_templates
    ++$(call bin_wrappers_template,BINDIR_PROGRAMS_NEED_X,'$$(@F)',,$$X)
    ++$(call bin_wrappers_template,BINDIR_PROGRAMS_NO_X,'$$(@F)')
    ++$(call bin_wrappers_template,TEST_PROGRAMS_NEED_X,'$$(@F)',t/helper/,$$X)
    ++endef
    ++$(eval $(call bin_wrappers_templates))
     +
     +all:: $(BIN_WRAPPERS)

4:  cbbf458433f < -:  ----------- Makefile: define bin-wrappers/% rules with a template
5:  560dee80b4a < -:  ----------- Makefile: fix "make clean && make bin-wrappers/$NAME" dependencies

2.38.0.1251.g3eefdfb5e7a

Reduce the amount of magical copy/pasting in the Makefile by factoring the munging of "#!/bin/sh" on the first line of a shellscript into a variable we can re-use in the various rules that need to do so.

---
 Makefile | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

index 85f03c6aed1..45b22d33513 100644
@@ -2344,8 +2344,12 @@ GIT-SCRIPT-DEFINES: FORCE
 		echo "$$FLAGS" >$@; \
             fi
 
+define cmd_munge_script_sed_shell_path_arg
+'1s|#!.*/sh|#!$(SHELL_PATH_SQ)|'
+endef
+
 define cmd_munge_script
-sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
+sed -e $(call cmd_munge_script_sed_shell_path_arg) \
     -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
     -e 's|@@DIFF@@|$(DIFF_SQ)|' \
     -e 's|@@LOCALEDIR@@|$(localedir_SQ)|g' \

@@ -2447,7 +2451,7 @@ git-instaweb: git-instaweb.sh GIT-SCRIPT-DEFINES
 else # NO_PERL
 $(SCRIPT_PERL_GEN) git-instaweb: % : unimplemented.sh
 	$(QUIET_GEN) \
-	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
+	sed -e $(call cmd_munge_script_sed_shell_path_arg) \
 	    -e 's|@@REASON@@|NO_PERL=$(NO_PERL)|g' \
 	    unimplemented.sh >$@+ && \
 	chmod +x $@+ && \

@@ -2468,7 +2472,7 @@ $(SCRIPT_PYTHON_GEN): % : %.py
 else # NO_PYTHON
 $(SCRIPT_PYTHON_GEN): % : unimplemented.sh
 	$(QUIET_GEN) \
-	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
+	sed -e $(call cmd_munge_script_sed_shell_path_arg) \
 	    -e 's|@@REASON@@|NO_PYTHON=$(NO_PYTHON)|g' \
 	    unimplemented.sh >$@+ && \
 	chmod +x $@+ && \

@@ -3061,7 +3065,7 @@ all:: $(TEST_PROGRAMS) $(test_bindir_programs)
 
 bin-wrappers/%: wrap-for-bin.sh
 	$(call mkdir_p_parent_template)
-	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
+	$(QUIET_GEN)sed -e $(call cmd_munge_script_sed_shell_path_arg) \
 	     -e 's|@@BUILD_DIR@@|$(shell pwd)|' \
 	     -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%,$(@F))$(if $(filter-out $(BINDIR_PROGRAMS_NO_X),$(@F)),$(X),)|' < $< > $@ && \
 	chmod +x $@

2.38.0.1251.g3eefdfb5e7a

When the @@PROG@@ substitution was added in [1] it was a simple matter of doing a:

	's|@@PROG@@|$(@F)|'

Then when t/helpers were added in [2] followed by the ".exe" needing to be appended in [3] this previously simple rule ended up as a dense one-liner.

It has been pointed out that this is hard to read[4], but the problem isn't the "design of the Makefile syntax". It's we now have to after-the-fact determine if we were dealing with a bin-wrapper/ that needed to have $(X) appended, a t/helper/, or a non-binary (currently only git-cvsserver).

That would be a problem in any language. We're starting out with three lists, and then end up having to heuristically determine given a member of any of those lists which list that member came from. Let's just stop doing that and keep track of what member belongs to which list.

We can do this by splitting up the single "bin-wrappers/%" rule into a rule for each of the three lists. With the "cmd_munge_script_sed_shell_path_arg" define added in a preceding commit this is easy, we just need to add a sister template to the existing "cmd_munge_script" added in [5].

The "filter-out" added in [6] has now become unnecessary per the above, it was an artifact of us losing track of what was in which of our lists to begin with.

This change can be tested with e.g.:

	git checkout master &&
	make SHELL_PATH=/bin/bash X=.exe &&
	mv bin-wrappers bin-wrappers.master &&
	<apply this change> &&
	make SHELL_PATH=/bin/bash X=.exe &&
	diff -ru bin-wrappers{.master,}

Which will show an empty diff, i.e. we've correctly dealt with the combination of $(SHELL_PATH), $(X) and these three variables here.

This also fixes an issue with the "bin-wrappers/" scripts have never had properly declared dependency information, i.e. this has never worked:

	make clean &&
	make bin-wrappers/git &&
	# the script is there, but no "./git" is built
	./bin-wrappers/git

There was no reason not to have that work, just as most things generated by the Makefile have proper dependency information.

Before this commit doing this would have been painful, but now it's easy to pass this as a parameter to our "bin_wrappers_template"

1. ea925196f1b (build dashless "bin-wrappers" directory similar to installed bindir, 2009-12-02)
2. e6e7530d10b (test helpers: move test-* to t/helper/ subdirectory, 2016-04-13)
3. 3a94cb31d52 (bin-wrappers: append .exe to target paths if necessary, 2019-07-29)
4. https://lore.kernel.org/git/[email protected]/
5. 46bac904581 (Do not install shell libraries executable, 2010-01-31)
6. 7b5c93c6c68" (scalar: include in standard Git build & installation, 2022-09-02)

---
 Makefile | 42 +++++++++++++++++++++++++++++++++---------
 1 file changed, 33 insertions(+), 9 deletions(-)

index a8cfa096dc1..d551a89bbff 100644
@@ -595,6 +595,7 @@ export TCL_PATH TCLTK_PATH
 PTHREAD_LIBS = -lpthread
 
 # Guard against environment variables
+BIN_WRAPPERS =
 BUILTIN_OBJS =
 BUILT_INS =
 COMPAT_CFLAGS =

@@ -3062,16 +3063,39 @@ GIT-PYTHON-VARS: FORCE
             fi
 endif
 
-test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
+define cmd_munge_bin_wrappers_script
+sed \
+	-e $(call cmd_munge_script_sed_shell_path_arg) \
+	-e 's|@@BUILD_DIR@@|$(shell pwd)|' \
+	-e 's|@@PROG@@|$(2)$(1)$(3)|' \
+	<$< >$@ && \
+	chmod +x $@
+endef
 
-all:: $(test_bindir_programs)
+define bin_wrappers_template
+
+## bin_wrappers_template
+# 1 = $(1)
+# 2 = $(2)
+# 3 = $(3)
+# 4 = $(4)
+BW_$(1) = $$($(1):%=bin-wrappers/%)
+BIN_WRAPPERS += $$(BW_$(1))
+all:: $$(BW_$(1))
+$$(BW_$(1)): bin-wrappers/% : $(3)%$(4)
+$$(BW_$(1)): wrap-for-bin.sh
+	$$(call mkdir_p_parent_template)
+	$$(QUIET_GEN)$$(call cmd_munge_bin_wrappers_script,$(2),$(3),$(4))
+endef
 
-bin-wrappers/%: wrap-for-bin.sh
-	$(call mkdir_p_parent_template)
-	$(QUIET_GEN)sed -e $(call cmd_munge_script_sed_shell_path_arg) \
-	     -e 's|@@BUILD_DIR@@|$(shell pwd)|' \
-	     -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%,$(@F))$(if $(filter-out $(BINDIR_PROGRAMS_NO_X),$(@F)),$(X),)|' < $< > $@ && \
-	chmod +x $@
+define bin_wrappers_templates
+$(call bin_wrappers_template,BINDIR_PROGRAMS_NEED_X,'$$(@F)',,$$X)
+$(call bin_wrappers_template,BINDIR_PROGRAMS_NO_X,'$$(@F)')
+$(call bin_wrappers_template,TEST_PROGRAMS_NEED_X,'$$(@F)',t/helper/,$$X)
+endef
+$(eval $(call bin_wrappers_templates))
+
+all:: $(BIN_WRAPPERS)
 
 # GNU make supports exporting all variables by "export" without parameters.
 # However, the environment gets quite big, and some programs have problems

@@ -3404,7 +3428,7 @@ OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll)
 endif
 
 artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
-		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
+		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(BIN_WRAPPERS) \
 		$(MOFILES)
 	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
 		SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'

2.38.0.1251.g3eefdfb5e7a

Define the variables that make up TEST_OBJS earlier, and don't go back & forth in their definition. Before we'd first append $X to $(TEST_PROGRAMS), and then substitute $X back out of it to define $(TEST_OBJS). Let's instead add a new $(TEST_PROGRAM_OBJS) variable, which avoids this needless back & forth substitution.

See daa99a91729 (Makefile: make sure test helpers are rebuilt when headers change, 2010-01-26) for how we ended up with the original $(TEST_OBJS).

---
 Makefile | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

index 45b22d33513..a8cfa096dc1 100644
@@ -617,7 +617,8 @@ SCRIPT_PYTHON =
 SCRIPT_SH =
 SCRIPT_LIB =
 TEST_BUILTINS_OBJS =
-TEST_OBJS =
+TEST_PROGRAMS =
+TEST_PROGRAM_OBJS =
 TEST_PROGRAMS_NEED_X =
 THIRD_PARTY_SOURCES =
 

@@ -795,6 +796,7 @@ 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
+TEST_PROGRAM_OBJS += $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
 
 # Do not add more tests here unless they have extra dependencies. Add
 # them in TEST_BUILTINS_OBJS above.

@@ -802,6 +804,9 @@ TEST_PROGRAMS_NEED_X += test-fake-ssh
 TEST_PROGRAMS_NEED_X += test-tool
 
 TEST_PROGRAMS = $(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X))
+all:: $(TEST_PROGRAMS)
+TEST_PROGRAM_OBJS += $(patsubst %,t/helper/%.o,$(TEST_PROGRAMS_NEED_X))
+.PRECIOUS: $(TEST_PROGRAM_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.

@@ -2543,10 +2548,8 @@ REFTABLE_TEST_OBJS += reftable/stack_test.o
 REFTABLE_TEST_OBJS += reftable/test_framework.o
 REFTABLE_TEST_OBJS += reftable/tree_test.o
 
-TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
-
 .PHONY: test-objs
-test-objs: $(TEST_OBJS)
+test-objs: $(TEST_PROGRAM_OBJS)
 
 GIT_OBJS += $(LIB_OBJS)
 GIT_OBJS += $(BUILTIN_OBJS)

@@ -2562,7 +2565,7 @@ scalar-objs: $(SCALAR_OBJS)
 OBJECTS += $(GIT_OBJS)
 OBJECTS += $(SCALAR_OBJS)
 OBJECTS += $(PROGRAM_OBJS)
-OBJECTS += $(TEST_OBJS)
+OBJECTS += $(TEST_PROGRAM_OBJS)
 OBJECTS += $(XDIFF_OBJS)
 OBJECTS += $(FUZZ_OBJS)
 OBJECTS += $(REFTABLE_OBJS) $(REFTABLE_TEST_OBJS)

@@ -3061,7 +3064,7 @@ endif
 
 test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
 
-all:: $(TEST_PROGRAMS) $(test_bindir_programs)
+all:: $(test_bindir_programs)
 
 bin-wrappers/%: wrap-for-bin.sh
 	$(call mkdir_p_parent_template)

@@ -3087,8 +3090,6 @@ perf: all
 
 .PHONY: test perf
 
-.PRECIOUS: $(TEST_OBJS)
-
 t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
 
 t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS) $(REFTABLE_TEST_LIB)

2.38.0.1251.g3eefdfb5e7a

Makes sense, I guess. So TEST_OBJS is no longer used?

This change is not necessary to achieve the stated goal of this step, though. It is one of those "while at it" distraction that consumes our already constrained reviewer bandwidth, no?

Having said that, "all::" being able to be built up with independent pieces shine here in this split from the original. It probably is easier to reason about while seeing this isolated area of Makefile what is being done to TEST_PROGRAMS.

The rest of the patch is quite straight-forward renaming of TEST_OBJS to TEST_PROGRAM_OBJS and an improvement of how the elements on the list are computed from the source-of-truth list that is TEST_PROGRAMS_NEED_X that looks correct.

At least when taken standalone, this looks more like replacing one magical copy pasting with another magical one, the difference being that the latter is not immediately obvious without referring back to the definition of the variable.

If we need to change the replacement wholesale in a later step, then it might give us a good trade off, but otherwise I am not sure why this is a good idea that is worth the churn.

When adding or updating these actions, in the original, you can typo SHELL_PATH_SQ, but you can typo cmd_munge_script_sed_shell_path_arg in the updated to break it the same way. So...?

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

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

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

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

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

Thanks for the quick review!

It invites the question why it needed to be renamed in the first place, though. If we needed to name something that is different from the original TEST_OBJS, it makes perfect sense to add a variable with a new name, but if it is not the case here, then...?

I'll let it pass this time ;-) but my patience is not infinite.

[Re-arranged your question a bit for the reply...]

Are you asking if "define"'s execute in some context outside the purview of rules, so that if you $(call) one from within a rule and it errors, that we won't clean up the file?

No, that won't happen, they'll be cleaned up properly. First, these are basically macros, but secondly it wouldn't matter if they weren't. E.g. you can execute an arbitrary program that happens to write to a file that matches your $@.

The only thing that matters is that we carry forward the exit code, which we do here. I.e. the "&&" after the $(call).

(Even then we'd still error properly, unless you ran "make -i" to ignore errors in the rule, the && is more in service of the $(QUIET_*) in our Makefile).

Those are mostly there because nobody's cleaned them up, we don't need them anymore.

"Mostly" because one thing they still do is not replace the file until it's ready, we had a previous discussion on a related topic in reply to
https://lore.kernel.org/git/[email protected]/
(it didn't go in).

I can drop them, FWIW I've found it quite handy to add these to ad-hoc debug templates. E.g. you can run:

	$ make -f /dev/null -E '$(eval $(file <Makefile))' -E '$(error $(call bin_wrappers_template,a,b,c,d))'
	make: *** 
	## bin_wrappers_template
	# 1 = a
	# 2 = b
	# 3 = c
	# 4 = d
	BW_a = $(a:%=bin-wrappers/%)
	BIN_WRAPPERS += $(BW_a)
	all:: $(BW_a)
	$(BW_a): bin-wrappers/% : c%d
	$(BW_a): wrap-for-bin.sh
	        $(call mkdir_p_parent_template)
	        $(QUIET_GEN)$(call cmd_munge_bin_wrappers_script,b,c,d).  Stop.

So you see what the parameters expand to. Maybe just changing the heading to:

	## bin_wrappers_template: $(1..N) below for manual "$(error $(call ..." deubgging

?

Not at all.

I was wondering why the defined sequance does not end in

	...
	<$> >$@+ && \
	chmod +x $@+ && \
	mv $@+ $@

like many other command sequences in the Makefile. As I said, I did remember we had discussed delete-on-error; I just didn't recall if we do depend on it already.

OK. That use pattern was what I was missing.

Yeah, it would be totally useless without such an instruction.

Ah! For the purposes of this patch the actual answer to that is that it's just moving existing code around.

So this happens to be the pattern we should prefer with .DELETE_ON_ERROR before & after 3/3.

But if it was doing that "$@+ $@" mv dance I'd have just blindly retained that too, to avoid "while at it" and all that...

The code I touched in 1/3 uses the "mv $@+ $@" for no good post-.DELETE_ON_ERROR reason, but I just left that in place as-is...

Will update it with that.

Maybe I'm missing something, but the current behavior is what I'd expect to happen. Sure, bin-wrappers/git depends on "git" at run-time if you want it to do something useful. But AFAIK it doesn't at build-time. Why do we want a build-time dependency? Are we expecting rules to depend on bin-wrappers/foo but not also on foo? Or people to ask manually for those targets?

This kind of feels like making git-repack depend on git-pack-objects. One will call the other at run-time, but there's no reason the builds have to be together. And if you try to build and run one without building the other, well...if it hurts, don't do it.

-Peff

I didn't have an immediate practical reason for it beyond just not leaving holes in the dependency DAG.

But yes, nothing is only building bin-wrappers/% now, so nothing needs this. It just helps to not have to worry about Makefile targets not needing other targets to work.

Or wget depending on your router being plugged in? Beacuse that's what you use it for?:)

I get the general point, but I don't think these are comparable to those sorts of loose or semi-loose dependencies. For repack it'll service up e.g. "-h" without pack-objects, at least that's something...

Whereas these scripts all end with invoking the binary corresponding to their name, so they really are useless without that target. So I think it makes sense to make it a dependency of the target in the Makefile.

This series untangles the gnarly rule we use to generate bin-wrappers/%.

It's now complex because we generate it from 3x separate variables, and then end up having to do inline pattern matching to decide which value comes from where.

Instead, we can avoid squashing those values together, so we don't have to guess.

See[1] for the v2. This hopefully addresses all outstanding issues/points that were raised. An added 3/4 here makes the eventual 4/4 smaler.

1. https://lore.kernel.org/git/[email protected]/

Ævar Arnfjörð Bjarmason (4):
Makefile: factor sed-powered '#!/bin/sh' munging into a variable
Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier
Makefile: rename "test_bindir_programs" variable, pre-declare
Makefile: simplify $(test_bindir_programs) rule by splitting it up

 Makefile | 70 +++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 51 insertions(+), 19 deletions(-)

Range-diff against v2:

1:  fc6c5a6a8df = 1:  c9ce5b78a3a Makefile: factor sed-powered '#!/bin/sh' munging into a variable
2:  6dcb49f25c4 = 2:  b95c296b6de Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier
-:  ----------- > 3:  fea93c45898 Makefile: rename "test_bindir_programs" variable, pre-declare
3:  400f487e30d ! 4:  0ff09495476 Makefile: simplify $(test_bindir_programs) rule by splitting it up
    @@ Commit message
         Which will show an empty diff, i.e. we've correctly dealt with the
         combination of $(SHELL_PATH), $(X) and these three variables here.
     
    -    This also fixes an issue with the "bin-wrappers/" scripts have never had properly declared
    -    dependency information, i.e. this has never worked:
    +    This also fixes an issue with the "bin-wrappers/" scripts have never
    +    had properly declared dependency information, i.e. this has never
    +    worked:
     
                 make clean &&
                 make bin-wrappers/git &&
    @@ Commit message
         Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]>
     
      ## Makefile ##
    -@@ Makefile: export TCL_PATH TCLTK_PATH
    - PTHREAD_LIBS = -lpthread
    - 
    - # Guard against environment variables
    -+BIN_WRAPPERS =
    - BUILTIN_OBJS =
    - BUILT_INS =
    - COMPAT_CFLAGS =
     @@ Makefile: GIT-PYTHON-VARS: FORCE
                  fi
      endif
      
    --test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
    +-BIN_WRAPPERS = $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
     +define cmd_munge_bin_wrappers_script
     +sed \
     +	-e $(call cmd_munge_script_sed_shell_path_arg) \
    @@ Makefile: GIT-PYTHON-VARS: FORCE
     +	chmod +x $@
     +endef
      
    --all:: $(test_bindir_programs)
    +-all:: $(BIN_WRAPPERS)
     +define bin_wrappers_template
     +
    -+## bin_wrappers_template
    -+# 1 = $(1)
    -+# 2 = $(2)
    -+# 3 = $(3)
    -+# 4 = $(4)
    ++### bin_wrappers_template; Parameters:
    ++## E.g. "BINDIR_PROGRAMS_NEED_X": Variable reference
    ++# 1='$(1)'
    ++## E.g. "$(@F)": Passed as $$(1)) to "cmd_munge_bin_wrappers_script"
    ++# 2='$(2)'
    ++## E.g. "" or "t/helper": Directory prefix for the wrapped binary
    ++# 3='$(3)'
    ++## E.g. "" or "$$X": If $$X: wrapped binary needs X=.exe (for Windows)
    ++# 4='$(4)'
     +BW_$(1) = $$($(1):%=bin-wrappers/%)
     +BIN_WRAPPERS += $$(BW_$(1))
    -+all:: $$(BW_$(1))
     +$$(BW_$(1)): bin-wrappers/% : $(3)%$(4)
     +$$(BW_$(1)): wrap-for-bin.sh
     +	$$(call mkdir_p_parent_template)
    @@ Makefile: GIT-PYTHON-VARS: FORCE
      
      # GNU make supports exporting all variables by "export" without parameters.
      # However, the environment gets quite big, and some programs have problems
    -@@ Makefile: OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll)
    - endif
    - 
    - artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
    --		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
    -+		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(BIN_WRAPPERS) \
    - 		$(MOFILES)
    - 	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
    - 		SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'

2.38.0.1280.g8136eb6fab2

Reduce the amount of magical copy/pasting in the Makefile by factoring the munging of "#!/bin/sh" on the first line of a shellscript into a variable we can re-use in the various rules that need to do so.

---
 Makefile | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

index 4927379184c..679c24377f4 100644
@@ -2347,8 +2347,12 @@ GIT-SCRIPT-DEFINES: FORCE
 		echo "$$FLAGS" >$@; \
             fi
 
+define cmd_munge_script_sed_shell_path_arg
+'1s|#!.*/sh|#!$(SHELL_PATH_SQ)|'
+endef
+
 define cmd_munge_script
-sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
+sed -e $(call cmd_munge_script_sed_shell_path_arg) \
     -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
     -e 's|@@DIFF@@|$(DIFF_SQ)|' \
     -e 's|@@LOCALEDIR@@|$(localedir_SQ)|g' \

@@ -2450,7 +2454,7 @@ git-instaweb: git-instaweb.sh GIT-SCRIPT-DEFINES
 else # NO_PERL
 $(SCRIPT_PERL_GEN) git-instaweb: % : unimplemented.sh
 	$(QUIET_GEN) \
-	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
+	sed -e $(call cmd_munge_script_sed_shell_path_arg) \
 	    -e 's|@@REASON@@|NO_PERL=$(NO_PERL)|g' \
 	    unimplemented.sh >$@+ && \
 	chmod +x $@+ && \

@@ -2471,7 +2475,7 @@ $(SCRIPT_PYTHON_GEN): % : %.py
 else # NO_PYTHON
 $(SCRIPT_PYTHON_GEN): % : unimplemented.sh
 	$(QUIET_GEN) \
-	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
+	sed -e $(call cmd_munge_script_sed_shell_path_arg) \
 	    -e 's|@@REASON@@|NO_PYTHON=$(NO_PYTHON)|g' \
 	    unimplemented.sh >$@+ && \
 	chmod +x $@+ && \

@@ -3065,7 +3069,7 @@ all:: $(TEST_PROGRAMS) $(test_bindir_programs)
 
 bin-wrappers/%: wrap-for-bin.sh
 	$(call mkdir_p_parent_template)
-	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
+	$(QUIET_GEN)sed -e $(call cmd_munge_script_sed_shell_path_arg) \
 	     -e 's|@@BUILD_DIR@@|$(shell pwd)|' \
 	     -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%,$(@F))$(if $(filter-out $(BINDIR_PROGRAMS_NO_X),$(@F)),$(X),)|' < $< > $@ && \
 	chmod +x $@

2.38.0.1280.g8136eb6fab2

Pre-declare the "BIN-WRAPPERS" variable, renamed from "test_bindir_programs", which is more in line with our usual naming conventions in the Makefile.

There's also no reason to use a "simply expanded variable" here, so let's use a normal "=" assignment. It doesn't make a difference in the end as we're about to use to to define a rule, so it'll have "IMMEDIATE" expansion here.

See ea925196f1b (build dashless "bin-wrappers" directory similar to installed bindir, 2009-12-02) for the introduction of "test_bindir_programs".

---
 Makefile | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

index 911cce6d487..032654640c5 100644
@@ -595,6 +595,7 @@ export TCL_PATH TCLTK_PATH
 PTHREAD_LIBS = -lpthread
 
 # Guard against environment variables
+BIN_WRAPPERS =
 BUILTIN_OBJS =
 BUILT_INS =
 COMPAT_CFLAGS =

@@ -3066,9 +3067,9 @@ GIT-PYTHON-VARS: FORCE
             fi
 endif
 
-test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
+BIN_WRAPPERS = $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
 
-all:: $(test_bindir_programs)
+all:: $(BIN_WRAPPERS)
 
 bin-wrappers/%: wrap-for-bin.sh
 	$(call mkdir_p_parent_template)

@@ -3408,7 +3409,7 @@ OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll)
 endif
 
 artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
-		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
+		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(BIN_WRAPPERS) \
 		$(MOFILES)
 	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
 		SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'

2.38.0.1280.g8136eb6fab2

Define the variables that make up TEST_OBJS earlier, and don't go back & forth in their definition. Before we'd first append $X to $(TEST_PROGRAMS), and then substitute $X back out of it to define $(TEST_OBJS). Let's instead add a new $(TEST_PROGRAM_OBJS) variable, which avoids this needless back & forth substitution.

See daa99a91729 (Makefile: make sure test helpers are rebuilt when headers change, 2010-01-26) for how we ended up with the original $(TEST_OBJS).

---
 Makefile | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

index 679c24377f4..911cce6d487 100644
@@ -617,7 +617,8 @@ SCRIPT_PYTHON =
 SCRIPT_SH =
 SCRIPT_LIB =
 TEST_BUILTINS_OBJS =
-TEST_OBJS =
+TEST_PROGRAMS =
+TEST_PROGRAM_OBJS =
 TEST_PROGRAMS_NEED_X =
 THIRD_PARTY_SOURCES =
 

@@ -796,6 +797,7 @@ 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
+TEST_PROGRAM_OBJS += $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
 
 # Do not add more tests here unless they have extra dependencies. Add
 # them in TEST_BUILTINS_OBJS above.

@@ -803,6 +805,9 @@ TEST_PROGRAMS_NEED_X += test-fake-ssh
 TEST_PROGRAMS_NEED_X += test-tool
 
 TEST_PROGRAMS = $(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X))
+all:: $(TEST_PROGRAMS)
+TEST_PROGRAM_OBJS += $(patsubst %,t/helper/%.o,$(TEST_PROGRAMS_NEED_X))
+.PRECIOUS: $(TEST_PROGRAM_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.

@@ -2546,10 +2551,8 @@ REFTABLE_TEST_OBJS += reftable/stack_test.o
 REFTABLE_TEST_OBJS += reftable/test_framework.o
 REFTABLE_TEST_OBJS += reftable/tree_test.o
 
-TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
-
 .PHONY: test-objs
-test-objs: $(TEST_OBJS)
+test-objs: $(TEST_PROGRAM_OBJS)
 
 GIT_OBJS += $(LIB_OBJS)
 GIT_OBJS += $(BUILTIN_OBJS)

@@ -2565,7 +2568,7 @@ scalar-objs: $(SCALAR_OBJS)
 OBJECTS += $(GIT_OBJS)
 OBJECTS += $(SCALAR_OBJS)
 OBJECTS += $(PROGRAM_OBJS)
-OBJECTS += $(TEST_OBJS)
+OBJECTS += $(TEST_PROGRAM_OBJS)
 OBJECTS += $(XDIFF_OBJS)
 OBJECTS += $(FUZZ_OBJS)
 OBJECTS += $(REFTABLE_OBJS) $(REFTABLE_TEST_OBJS)

@@ -3065,7 +3068,7 @@ endif
 
 test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
 
-all:: $(TEST_PROGRAMS) $(test_bindir_programs)
+all:: $(test_bindir_programs)
 
 bin-wrappers/%: wrap-for-bin.sh
 	$(call mkdir_p_parent_template)

@@ -3091,8 +3094,6 @@ perf: all
 
 .PHONY: test perf
 
-.PRECIOUS: $(TEST_OBJS)
-
 t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
 
 t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS) $(REFTABLE_TEST_LIB)

2.38.0.1280.g8136eb6fab2

When the @@PROG@@ substitution was added in [1] it was a simple matter of doing a:

	's|@@PROG@@|$(@F)|'

Then when t/helpers were added in [2] followed by the ".exe" needing to be appended in [3] this previously simple rule ended up as a dense one-liner.

It has been pointed out that this is hard to read[4], but the problem isn't the "design of the Makefile syntax". It's we now have to after-the-fact determine if we were dealing with a bin-wrapper/ that needed to have $(X) appended, a t/helper/, or a non-binary (currently only git-cvsserver).

That would be a problem in any language. We're starting out with three lists, and then end up having to heuristically determine given a member of any of those lists which list that member came from. Let's just stop doing that and keep track of what member belongs to which list.

We can do this by splitting up the single "bin-wrappers/%" rule into a rule for each of the three lists. With the "cmd_munge_script_sed_shell_path_arg" define added in a preceding commit this is easy, we just need to add a sister template to the existing "cmd_munge_script" added in [5].

The "filter-out" added in [6] has now become unnecessary per the above, it was an artifact of us losing track of what was in which of our lists to begin with.

This change can be tested with e.g.:

	git checkout master &&
	make SHELL_PATH=/bin/bash X=.exe &&
	mv bin-wrappers bin-wrappers.master &&
	<apply this change> &&
	make SHELL_PATH=/bin/bash X=.exe &&
	diff -ru bin-wrappers{.master,}

Which will show an empty diff, i.e. we've correctly dealt with the combination of $(SHELL_PATH), $(X) and these three variables here.

This also fixes an issue with the "bin-wrappers/" scripts have never had properly declared dependency information, i.e. this has never worked:

	make clean &&
	make bin-wrappers/git &&
	# the script is there, but no "./git" is built
	./bin-wrappers/git

There was no reason not to have that work, just as most things generated by the Makefile have proper dependency information.

Before this commit doing this would have been painful, but now it's easy to pass this as a parameter to our "bin_wrappers_template"

1. ea925196f1b (build dashless "bin-wrappers" directory similar to installed bindir, 2009-12-02)
2. e6e7530d10b (test helpers: move test-* to t/helper/ subdirectory, 2016-04-13)
3. 3a94cb31d52 (bin-wrappers: append .exe to target paths if necessary, 2019-07-29)
4. https://lore.kernel.org/git/[email protected]/
5. 46bac904581 (Do not install shell libraries executable, 2010-01-31)
6. 7b5c93c6c68" (scalar: include in standard Git build & installation, 2022-09-02)

---
 Makefile | 42 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 8 deletions(-)

index 032654640c5..d7ab68e3db8 100644
@@ -3067,16 +3067,42 @@ GIT-PYTHON-VARS: FORCE
             fi
 endif
 
-BIN_WRAPPERS = $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
+define cmd_munge_bin_wrappers_script
+sed \
+	-e $(call cmd_munge_script_sed_shell_path_arg) \
+	-e 's|@@BUILD_DIR@@|$(shell pwd)|' \
+	-e 's|@@PROG@@|$(2)$(1)$(3)|' \
+	<$< >$@ && \
+	chmod +x $@
+endef
 
-all:: $(BIN_WRAPPERS)
+define bin_wrappers_template
+
+### bin_wrappers_template; Parameters:
+## E.g. "BINDIR_PROGRAMS_NEED_X": Variable reference
+# 1='$(1)'
+## E.g. "$(@F)": Passed as $$(1)) to "cmd_munge_bin_wrappers_script"
+# 2='$(2)'
+## E.g. "" or "t/helper": Directory prefix for the wrapped binary
+# 3='$(3)'
+## E.g. "" or "$$X": If $$X: wrapped binary needs X=.exe (for Windows)
+# 4='$(4)'
+BW_$(1) = $$($(1):%=bin-wrappers/%)
+BIN_WRAPPERS += $$(BW_$(1))
+$$(BW_$(1)): bin-wrappers/% : $(3)%$(4)
+$$(BW_$(1)): wrap-for-bin.sh
+	$$(call mkdir_p_parent_template)
+	$$(QUIET_GEN)$$(call cmd_munge_bin_wrappers_script,$(2),$(3),$(4))
+endef
 
-bin-wrappers/%: wrap-for-bin.sh
-	$(call mkdir_p_parent_template)
-	$(QUIET_GEN)sed -e $(call cmd_munge_script_sed_shell_path_arg) \
-	     -e 's|@@BUILD_DIR@@|$(shell pwd)|' \
-	     -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%,$(@F))$(if $(filter-out $(BINDIR_PROGRAMS_NO_X),$(@F)),$(X),)|' < $< > $@ && \
-	chmod +x $@
+define bin_wrappers_templates
+$(call bin_wrappers_template,BINDIR_PROGRAMS_NEED_X,'$$(@F)',,$$X)
+$(call bin_wrappers_template,BINDIR_PROGRAMS_NO_X,'$$(@F)')
+$(call bin_wrappers_template,TEST_PROGRAMS_NEED_X,'$$(@F)',t/helper/,$$X)
+endef
+$(eval $(call bin_wrappers_templates))
+
+all:: $(BIN_WRAPPERS)
 
 # GNU make supports exporting all variables by "export" without parameters.
 # However, the environment gets quite big, and some programs have problems

2.38.0.1280.g8136eb6fab2

Thanks. I replaced the earlier round with this one and pushed the result out to ttaylorr/git.

But having read the topic over again, I have to say that I also find the pre-existing behavior to be as expected. "make bin-wrappers/git" produces its target as expected, but the target is useless because the relationship it has with "git" is a runtime dependency, not a Make-time dependency.

So I'm not inclined to pick this one up, honestly. Perhaps other reviewers feel differently.

Thanks,
Taylor

I'm fine with changing that. The part where it makes:

	make clean &&
	make bin-wrappers/git &&
	./git ...

work is a 1-line change to remove, i.e.:

	diff --git a/Makefile b/Makefile
	index d7ab68e3db8..d3c0a66b4b4 100644
	--- a/Makefile
	+++ b/Makefile
	@@ -3089,7 +3089,6 @@ define bin_wrappers_template
	 # 4='$(4)'
	 BW_$(1) = $$($(1):%=bin-wrappers/%)
	 BIN_WRAPPERS += $$(BW_$(1))
	-$$(BW_$(1)): bin-wrappers/% : $(3)%$(4)
	 $$(BW_$(1)): wrap-for-bin.sh
	 	$$(call mkdir_p_parent_template)
	 	$$(QUIET_GEN)$$(call cmd_munge_bin_wrappers_script,$(2),$(3),$(4))

This topic is mainly about untangling the dense $(filter) mess in the existing rule, which last came up as we need do to special-case "scalar", and we'd need to special-case any future binaries.

But I see it's my own fault for making that clear, the "as seen in the range-diff below" in v2's CL [1] was meant to refer to "here's what's new in v2", not the topic's reason for existing.

So, while I'm happy to re-roll it and remove that one line (and the associated part of the commit message) I'm surprised that there's such a hang-up about this aspect of it, to the point where I think I'm missing something.

Nothing depends on "bin-wrappers/%" now, so having this harms nothing, but just seems to me to nicely tie off a loose end.

I'm aware that e.g. git-submodule which depends on git-submodule.sh doesn't depend on e.g. "submodule--helper" being built, so in that sense this is inconsistent with the rest of our shellscripts.

But in the case of wrap-for-bin.sh its generation is driven by the Makefile, and it's *only* there to to exec() the target binary (at least some of those other *.sh's can serve up -h on their own).

And if we don't include that line, then (like on "master"):

	$ make clean >/dev/null; make bin-wrappers/git; ./git; echo $?
	GIT_VERSION = 2.38.GIT-dev
	    MKDIR -p bin-wrappers
	    GEN bin-wrappers/git
	bash: ./git: No such file or directory
	127

But *shrug*, I just don't see how it's useful, when it's so easy to make it do something useful.

1. https://lore.kernel.org/git/[email protected]/