[PATCH 0/8] scalar: integrate into core Git

[PATCH 0/8] scalar: integrate into core Git

[Victoria Dye via 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.


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/pull.1005.v10.git.1638538470.gitgitgadget@gmail.com/
 * Introduce 'scalar diagnose':
   https://lore.kernel.org/git/pull.1128.v6.git.1653145696.gitgitgadget@gmail.com/
 * Add '-c/-C' compatibility:
   https://lore.kernel.org/git/pull.1130.v2.git.1643380317358.gitgitgadget@gmail.com/
 * [DROPPED] Integrate Scalar into CI builds:
   https://lore.kernel.org/git/pull.1129.git.1654160735.gitgitgadget@gmail.com/
 * Document Scalar's role in Git & plan remaining work:
   https://lore.kernel.org/git/pull.1275.v2.git.1657584367.gitgitgadget@gmail.com/
 * Generalize 'scalar diagnose' into 'git diagnose' builtin & 'git bugreport
   --diagnose':
   https://lore.kernel.org/git/pull.1310.v4.git.1660335019.gitgitgadget@gmail.com/
 * Add FSMonitor support to Scalar, refactor enlistment search:
   https://lore.kernel.org/git/pull.1324.v3.git.1660858853.gitgitgadget@gmail.com/

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

[PATCH 1/8] scalar: fix command documentation section header

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 contrib/scalar/scalar.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/scalar/scalar.txt b/contrib/scalar/scalar.txt
index 1a12dc45077..505a1cea0fd 100644
--- a/contrib/scalar/scalar.txt
+++ b/contrib/scalar/scalar.txt
@@ -161,6 +161,6 @@ SEE ALSO
 --------
 linkgit:git-clone[1], linkgit:git-maintenance[1].
 
-Scalar
+GIT
 ---
 Associated with the linkgit:git[1] suite
-- 
gitgitgadget

[PATCH 2/8] scalar: include in standard Git build & installation

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 .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%)

diff --git a/.gitignore b/.gitignore
index 80b530bbed2..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
diff --git a/Documentation/Makefile b/Documentation/Makefile
index bd6b6fcb930..16c9e062390 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -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
diff --git a/contrib/scalar/scalar.txt b/Documentation/scalar.txt
similarity index 100%
rename from contrib/scalar/scalar.txt
rename to Documentation/scalar.txt
diff --git a/Makefile b/Makefile
index eac30126e29..66dd3321f57 100644
--- a/Makefile
+++ b/Makefile
@@ -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::
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 2237109b57f..bae203c1fb9 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -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)
diff --git a/contrib/scalar/.gitignore b/contrib/scalar/.gitignore
deleted file mode 100644
index ff3d47e84d0..00000000000
--- a/contrib/scalar/.gitignore
+++ /dev/null
@@ -1,2 +0,0 @@
-/*.exe
-/scalar
diff --git a/contrib/scalar/Makefile b/contrib/scalar/Makefile
deleted file mode 100644
index 37f283f35d7..00000000000
--- a/contrib/scalar/Makefile
+++ /dev/null
@@ -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
diff --git a/contrib/scalar/t/Makefile b/contrib/scalar/t/Makefile
deleted file mode 100644
index 1ed174a8cf3..00000000000
--- a/contrib/scalar/t/Makefile
+++ /dev/null
@@ -1,81 +0,0 @@
-# Import tree-wide shared Makefile behavior and libraries
-include ../../../shared.mak
-
-# Run scalar tests
-#
-# Copyright (c) 2005,2021 Junio C Hamano, Johannes Schindelin
-#
-
--include ../../../config.mak.autogen
--include ../../../config.mak
-
-SHELL_PATH ?= $(SHELL)
-PERL_PATH ?= /usr/bin/perl
-RM ?= rm -f
-PROVE ?= prove
-DEFAULT_TEST_TARGET ?= test
-TEST_LINT ?= test-lint
-
-ifdef TEST_OUTPUT_DIRECTORY
-TEST_RESULTS_DIRECTORY = $(TEST_OUTPUT_DIRECTORY)/test-results
-else
-TEST_RESULTS_DIRECTORY = ../../../t/test-results
-endif
-
-# Shell quote;
-SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
-PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
-TEST_RESULTS_DIRECTORY_SQ = $(subst ','\'',$(TEST_RESULTS_DIRECTORY))
-
-T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))
-
-all: $(DEFAULT_TEST_TARGET)
-
-test: $(TEST_LINT)
-	$(MAKE) aggregate-results-and-cleanup
-
-prove: $(TEST_LINT)
-	@echo "*** prove ***"; GIT_CONFIG=.git/config $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
-	$(MAKE) clean-except-prove-cache
-
-$(T):
-	@echo "*** $@ ***"; GIT_CONFIG=.git/config '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
-
-clean-except-prove-cache:
-	$(RM) -r 'trash directory'.*
-	$(RM) -r valgrind/bin
-
-clean: clean-except-prove-cache
-	$(RM) .prove
-
-test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax
-
-test-lint-duplicates:
-	@dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
-		test -z "$$dups" || { \
-		echo >&2 "duplicate test numbers:" $$dups; exit 1; }
-
-test-lint-executable:
-	@bad=`for i in $(T); do test -x "$$i" || echo $$i; done` && \
-		test -z "$$bad" || { \
-		echo >&2 "non-executable tests:" $$bad; exit 1; }
-
-test-lint-shell-syntax:
-	@'$(PERL_PATH_SQ)' ../../../t/check-non-portable-shell.pl $(T)
-
-aggregate-results-and-cleanup: $(T)
-	$(MAKE) aggregate-results
-	$(MAKE) clean
-
-aggregate-results:
-	for f in '$(TEST_RESULTS_DIRECTORY_SQ)'/t*-*.counts; do \
-		echo "$$f"; \
-	done | '$(SHELL_PATH_SQ)' ../../../t/aggregate-results.sh
-
-valgrind:
-	$(MAKE) GIT_TEST_OPTS="$(GIT_TEST_OPTS) --valgrind"
-
-test-results:
-	mkdir -p test-results
-
-.PHONY: $(T) aggregate-results clean valgrind
diff --git a/contrib/scalar/scalar.c b/scalar.c
similarity index 100%
rename from contrib/scalar/scalar.c
rename to scalar.c
diff --git a/contrib/scalar/t/t9099-scalar.sh b/t/t9210-scalar.sh
similarity index 96%
rename from contrib/scalar/t/t9099-scalar.sh
rename to t/t9210-scalar.sh
index dfb949f52ee..14ca575a214 100755
--- a/contrib/scalar/t/t9099-scalar.sh
+++ b/t/t9210-scalar.sh
@@ -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

[PATCH 3/8] git help: special-case `scalar`

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 builtin/help.c | 2 ++
 1 file changed, 2 insertions(+)

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);
 }
-- 
gitgitgadget

[PATCH 4/8] scalar: implement the `help` subcommand

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johasc@microsoft.com>

It is merely handing off to `git help scalar`.

Signed-off-by: Johannes Schindelin <johasc@microsoft.com>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 scalar.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

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},
-- 
gitgitgadget

[PATCH 5/8] scalar-clone: add test coverage

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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

Signed-off-by: Victoria Dye <vdye@github.com>
---
 t/t9211-scalar-clone.sh | 135 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 135 insertions(+)
 create mode 100755 t/t9211-scalar-clone.sh

diff --git a/t/t9211-scalar-clone.sh b/t/t9211-scalar-clone.sh
new file mode 100755
index 00000000000..9fbbc4de2c0
--- /dev/null
+++ b/t/t9211-scalar-clone.sh
@@ -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

[PATCH 6/8] t/perf: add Scalar performance tests

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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)

Signed-off-by: Victoria Dye <vdye@github.com>
---
 t/perf/p9210-scalar.sh | 43 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100755 t/perf/p9210-scalar.sh

diff --git a/t/perf/p9210-scalar.sh b/t/perf/p9210-scalar.sh
new file mode 100755
index 00000000000..a68eb6b223d
--- /dev/null
+++ b/t/perf/p9210-scalar.sh
@@ -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

[PATCH 7/8] t/perf: add 'GIT_PERF_USE_SCALAR' run option

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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.

Suggested-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 t/perf/README      |  4 ++++
 t/perf/perf-lib.sh | 13 ++++++++++++-
 t/perf/run         |  3 +++
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/t/perf/README b/t/perf/README
index fb9127a66f7..8f217d7be7d 100644
--- a/t/perf/README
+++ b/t/perf/README
@@ -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:
 
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 27c28017921..b960b0f6301 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -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
 	)
 }
 
diff --git a/t/perf/run b/t/perf/run
index 55219aa4056..33da4d2aba2 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -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

[PATCH 8/8] Documentation/technical: include Scalar technical doc

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 Documentation/Makefile             |  1 +
 Documentation/technical/scalar.txt | 61 ------------------------------
 2 files changed, 1 insertion(+), 61 deletions(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 16c9e062390..9ec53afdf18 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -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
diff --git a/Documentation/technical/scalar.txt b/Documentation/technical/scalar.txt
index 0600150b3ad..921cb104c3c 100644
--- a/Documentation/technical/scalar.txt
+++ b/Documentation/technical/scalar.txt
@@ -64,64 +64,3 @@ some "global" `git` options (e.g., `-c` and `-C`).
 Because `scalar` is not invoked as a Git subcommand (like `git scalar`), it is
 built and installed as its own executable in the `bin/` directory, alongside
 `git`, `git-gui`, etc.
-
-Roadmap
--------
-
-NOTE: this section will be removed once the remaining tasks outlined in this
-roadmap are complete.
-
-Scalar is a large enough project that it is being upstreamed incrementally,
-living in `contrib/` until it is feature-complete. So far, the following patch
-series have been accepted:
-
-- `scalar-the-beginning`: The initial patch series which sets up
-  `contrib/scalar/` and populates it with a minimal `scalar` command that
-  demonstrates the fundamental ideas.
-
-- `scalar-c-and-C`: The `scalar` command learns about two options that can be
-  specified before the command, `-c <key>=<value>` and `-C <directory>`.
-
-- `scalar-diagnose`: The `scalar` command is taught the `diagnose` subcommand.
-
-- `scalar-generalize-diagnose`: Move the functionality of `scalar diagnose`
-  into `git diagnose` and `git bugreport --diagnose`.
-
-- 'scalar-add-fsmonitor: Enable the built-in FSMonitor in Scalar
-  enlistments. At the end of this series, Scalar should be feature-complete
-  from the perspective of a user.
-
-Roughly speaking (and subject to change), the following series are needed to
-"finish" this initial version of Scalar:
-
-- Move Scalar to toplevel: Move Scalar out of `contrib/` and into the root of
-  `git`. This includes a variety of related updates, including:
-    - building & installing Scalar in the Git root-level 'make [install]'.
-    - builing & testing Scalar as part of CI.
-    - moving and expanding test coverage of Scalar (including perf tests).
-    - implementing 'scalar help'/'git help scalar' to display scalar
-      documentation.
-
-Finally, there are two additional patch series that exist in Microsoft's fork of
-Git, but there is no current plan to upstream them. There are some interesting
-ideas there, but the implementation is too specific to Azure Repos and/or VFS
-for Git to be of much help in general.
-
-These still exist mainly because the GVFS protocol is what Azure Repos has
-instead of partial clone, while Git is focused on improving partial clone:
-
-- `scalar-with-gvfs`: The primary purpose of this patch series is to support
-  existing Scalar users whose repositories are hosted in Azure Repos (which does
-  not support Git's partial clones, but supports its predecessor, the GVFS
-  protocol, which is used by Scalar to emulate the partial clone).
-
-  Since the GVFS protocol will never be supported by core Git, this patch series
-  will remain in Microsoft's fork of Git.
-
-- `run-scalar-functional-tests`: The Scalar project developed a quite
-  comprehensive set of integration tests (or, "Functional Tests"). They are the
-  sole remaining part of the original C#-based Scalar project, and this patch
-  adds a GitHub workflow that runs them all.
-
-  Since the tests partially depend on features that are only provided in the
-  `scalar-with-gvfs` patch series, this patch cannot be upstreamed.
-- 
gitgitgadget

Re: [PATCH 4/8] scalar: implement the `help` subcommand

[Ævar Arnfjörð Bjarmason]

On Wed, Aug 31 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johasc@microsoft.com>
>
> It is merely handing off to `git help scalar`.
>
> Signed-off-by: Johannes Schindelin <johasc@microsoft.com>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>  scalar.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> 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"),


This should not have N_(), as it's a literal command.

> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, NULL, options,
> +			     usage, 0);
> +
> +	if (argc != 0)

If we're re-rolling anyway we usually just do "if (argc)". We don't need
to worry about argc < 0 (despite the signed type, which is a historical
C wart).

> +		usage_with_options(usage, options);
> +
> +	return run_git("help", "scalar", NULL);

Performance isn't sensitive here, but have you tried just calling
cmd_help() instead with the appropriate arguments? It would avoid
spawning another command..

Re: [PATCH 0/8] scalar: integrate into core Git

[Ævar Arnfjörð Bjarmason]

On Wed, Aug 31 2022, Victoria Dye via GitGitGadget wrote:

> 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.
>
> [...]
>
> Prior series
> ============
>
>  * Add 'scalar' command in 'contrib/':
>    https://lore.kernel.org/git/pull.1005.v10.git.1638538470.gitgitgadget@gmail.com/
>  * Introduce 'scalar diagnose':
>    https://lore.kernel.org/git/pull.1128.v6.git.1653145696.gitgitgadget@gmail.com/
>  * Add '-c/-C' compatibility:
>    https://lore.kernel.org/git/pull.1130.v2.git.1643380317358.gitgitgadget@gmail.com/
>  * [DROPPED] Integrate Scalar into CI builds:
>    https://lore.kernel.org/git/pull.1129.git.1654160735.gitgitgadget@gmail.com/
>  * Document Scalar's role in Git & plan remaining work:
>    https://lore.kernel.org/git/pull.1275.v2.git.1657584367.gitgitgadget@gmail.com/
>  * Generalize 'scalar diagnose' into 'git diagnose' builtin & 'git bugreport
>    --diagnose':
>    https://lore.kernel.org/git/pull.1310.v4.git.1660335019.gitgitgadget@gmail.com/
>  * Add FSMonitor support to Scalar, refactor enlistment search:
>    https://lore.kernel.org/git/pull.1324.v3.git.1660858853.gitgitgadget@gmail.com/
>
> Thanks!
>
>  * Victoria

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/cover-v2-0.1-00000000000-20220623T100554Z-avarab@gmail.com/
	2. https://lore.kernel.org/git/patch-1.1-86fb8d56307-20211028T185016Z-avarab@gmail.com/

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.

Re: [PATCH 0/8] scalar: integrate into core Git

[Victoria Dye]

Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Aug 31 2022, Victoria Dye via GitGitGadget wrote:
> 
>> 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.
>>
>> [...]
>>
>> Prior series
>> ============
>>
>>  * Add 'scalar' command in 'contrib/':
>>    https://lore.kernel.org/git/pull.1005.v10.git.1638538470.gitgitgadget@gmail.com/
>>  * Introduce 'scalar diagnose':
>>    https://lore.kernel.org/git/pull.1128.v6.git.1653145696.gitgitgadget@gmail.com/
>>  * Add '-c/-C' compatibility:
>>    https://lore.kernel.org/git/pull.1130.v2.git.1643380317358.gitgitgadget@gmail.com/
>>  * [DROPPED] Integrate Scalar into CI builds:
>>    https://lore.kernel.org/git/pull.1129.git.1654160735.gitgitgadget@gmail.com/
>>  * Document Scalar's role in Git & plan remaining work:
>>    https://lore.kernel.org/git/pull.1275.v2.git.1657584367.gitgitgadget@gmail.com/
>>  * Generalize 'scalar diagnose' into 'git diagnose' builtin & 'git bugreport
>>    --diagnose':
>>    https://lore.kernel.org/git/pull.1310.v4.git.1660335019.gitgitgadget@gmail.com/
>>  * Add FSMonitor support to Scalar, refactor enlistment search:
>>    https://lore.kernel.org/git/pull.1324.v3.git.1660858853.gitgitgadget@gmail.com/
>>
>> Thanks!
>>
>>  * Victoria
> 
> 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/cover-v2-0.1-00000000000-20220623T100554Z-avarab@gmail.com/
> 	2. https://lore.kernel.org/git/patch-1.1-86fb8d56307-20211028T185016Z-avarab@gmail.com/

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/pull.1129.git.1654160735.gitgitgadget@gmail.com/

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

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!

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

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.

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

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/

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

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.

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

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

> 
> Thanks.

Re: [PATCH 4/8] scalar: implement the `help` subcommand

[Johannes Schindelin]

Hi Victoria,

On Wed, 31 Aug 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johasc@microsoft.com>

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

>
> It is merely handing off to `git help scalar`.
>
> Signed-off-by: Johannes Schindelin <johasc@microsoft.com>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>  scalar.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> 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},
> --
> gitgitgadget
>
>

Re: [PATCH 2/8] scalar: include in standard Git build & installation

[Johannes Schindelin]

Hi Victoria,

On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:

> From: Victoria Dye <vdye@github.com>
>
> 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.

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

> @@ -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),)|' < $< > $@ && \

It took me a good while to wrap my head around this (and let me be clear:
I consider none of this your fault, it's the fault of the design of the
Makefile syntax).

Let me untangle this, for posterity's benefit. We substitute the
placeholder `@@PROG@@` with a concatenation of two strings that are both
derived from `@F`, i.e. the basename of the to-be-wrapped command.

Before, the first string would be the dashed command, verbatim, unless its
name starts with a `test-` prefix, in which case we would add another
prefix (`t/helper/`) _and_ append the `.exe` suffix on Windows.

And the second string would be a `.exe` for all remaining cases that need
it: the commands starting with `git` and listed as needing the suffix.
Otherwise, the second string would be empty.

Convoluted, sure, but it worked.

In the new version, the first string would be the basename of the
to-be-wrapped command, with `t/helper/` prefixed for the `test-` helpers.

The second string would be a `.exe` for _all_ commands, except for those
specifically listed as not wanting that suffix as per
`BINDIR_PROGRAMS_NO_X`.

The new logic is so much simpler to understand!

Feel free to add a `Reviewed-by:` footer for me if you send another
iteration of this patch series.

Thank you,
Dscho

Re: [PATCH 4/8] scalar: implement the `help` subcommand

[Johannes Schindelin]

Hi Victoria,

On Wed, 31 Aug 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johasc@microsoft.com>
>
> It is merely handing off to `git help scalar`.
>
> Signed-off-by: Johannes Schindelin <johasc@microsoft.com>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>  scalar.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> 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 },

Marking this as a tangent ("optional", as some peeps suggested in the Git
standup on IRC [*1*]) with the suggestion to follow up _after_ this here
patch series is done cooking, i.e. once it hits the main branch:

We probably want to migrate `scalar.c` to use the `OPT_SUBCOMMAND` API.

But as I said, please not in this here patch series, so as to separate
concerns properly.

Thanks,
Dscho

>  	{ "version", cmd_version },
>  	{ "diagnose", cmd_diagnose },
>  	{ NULL, NULL},
> --
> gitgitgadget

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

Re: [PATCH 5/8] scalar-clone: add test coverage

[Johannes Schindelin]

Hi Victoria,

On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:

> From: Victoria Dye <vdye@github.com>
>
> Create a new test file ('t9211-scalar-clone.sh') to exercise the options and
> behavior of the 'scalar clone' command.

Great catch!

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

> +test_expect_success 'creates content in enlistment root' '
> +	test_when_finished cleanup_clone cloned &&
> +
> +	scalar clone "file://$(pwd)/to-clone" cloned &&

This pattern of cloning into `cloned` and removing the directory when the
test case is done is repeated throughout this test script.

In instances where all test cases succeed, that poses no problem.

When running the test script with `-i`, also no problem.

But when we run into test failures in CI, those directories will be
removed before the workflow run can tar them up and upload them for later
inspection.

May I suggest an alternative strategy?

If we drop all those `test_when_finished cleanup_clone cloned` calls and
instead `scalar clone` into different directories (whose names reflect the
test cases' intended purpose), I could imagine having a much easier time
not only diagnosing but also reproducing and fixing test failures in the
future.

When discussing code review practices [*1*], we did not come up with any
standard terminology to describe what I am offering here, and I am unsure
how to label this in a catchy way. I want to present this suggestion for
you to consider, and I would be delighted if you take it, at the same time
I will happily let it go should you decide against it.

Ciao,
Dscho

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

Re: [PATCH 6/8] t/perf: add Scalar performance tests

[Johannes Schindelin]

Hi Victoria,

On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:

> From: Victoria Dye <vdye@github.com>
>
> 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)

Excellent!

> [...]
> +
> +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

Given the small numbers presented in the commit message, I suspect that
even so much as running the command in a subshell might skew the timings
at least on Windows, where subshells are very, very expensive.

Given that both `git` and `test_commit` understand the `-C <directory>`
syntax, this variant would resolve my concern:

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

What do you think?

Ciao,
Dscho

Re: [PATCH 7/8] t/perf: add 'GIT_PERF_USE_SCALAR' run option

[Johannes Schindelin]

Hi Victoria,

On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:

> From: Victoria Dye <vdye@github.com>
>
> 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.

Great idea!

> [...]
> @@ -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

Do we need to unregister anything here? My guess is that no, the "global"
config we're using in tests is "$TRASH_DIRECTORY/.gitconfig", and the side
effect of scheduling the maintenance task won't matter in practice. But I
might have missed something and we may want to have an explicit
`unregister` step.

What's your take on this?

Ciao,
Dscho

> +		fi
>  	)
>  }

Re: [PATCH 0/8] scalar: integrate into core Git

[Johannes Schindelin]

Hi Victoria,

On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:

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

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

[PATCH 0/5] Makefile: split up $(test_bindir_programs)

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 01 2022, Johannes Schindelin wrote:

> [...]
> On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:
> [...]
>> @@ -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),)|' < $< > $@ && \
>
> It took me a good while to wrap my head around this (and let me be clear:
> I consider none of this your fault, it's the fault of the design of the
> Makefile syntax).
>
> Let me untangle this, for posterity's benefit. We substitute the
> placeholder `@@PROG@@` with a concatenation of two strings that are both
> derived from `@F`, i.e. the basename of the to-be-wrapped command.

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

[PATCH 1/5] Makefile: factor sed-powered '#!/bin/sh' munging into a variable

[Ævar Arnfjörð Bjarmason]

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.

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

diff --git a/Makefile b/Makefile
index eac30126e29..b2304aa93a3 100644
--- a/Makefile
+++ b/Makefile
@@ -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

[PATCH 2/5] Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier

[Ævar Arnfjörð Bjarmason]

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

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

diff --git a/Makefile b/Makefile
index b2304aa93a3..0d6b2caa7d9 100644
--- a/Makefile
+++ b/Makefile
@@ -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

[PATCH 3/5] Makefile: simplify $(test_bindir_programs) rule by splitting it up

[Ævar Arnfjörð Bjarmason]

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/sso99so6-n28s-rq86-8q20-4456r3pn869r@tzk.qr/
5. 46bac904581 (Do not install shell libraries executable, 2010-01-31)
6. https://lore.kernel.org/git/4d69e5eaccb8873eece666a3d2bb2b22abdde7ea.1661961746.git.gitgitgadget@gmail.com/

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

diff --git a/Makefile b/Makefile
index 0d6b2caa7d9..96de9e55864 100644
--- a/Makefile
+++ b/Makefile
@@ -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

[PATCH 4/5] Makefile: define bin-wrappers/% rules with a template

[Ævar Arnfjörð Bjarmason]

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

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

diff --git a/Makefile b/Makefile
index 96de9e55864..533858ca481 100644
--- a/Makefile
+++ b/Makefile
@@ -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

[PATCH 5/5] Makefile: fix "make clean && make bin-wrappers/$NAME" dependencies

[Ævar Arnfjörð Bjarmason]

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"

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

diff --git a/Makefile b/Makefile
index 533858ca481..07a9901c99d 100644
--- a/Makefile
+++ b/Makefile
@@ -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

Re: [PATCH 0/5] Makefile: split up $(test_bindir_programs)

[Derrick Stolee]

On 9/1/2022 9:17 AM, Ævar Arnfjörð Bjarmason wrote:

> Æ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(-)
 
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

Re: [PATCH 4/8] scalar: implement the `help` subcommand

[Victoria Dye]

Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Aug 31 2022, Johannes Schindelin via GitGitGadget wrote:
> 
>> From: Johannes Schindelin <johasc@microsoft.com>
>>
>> It is merely handing off to `git help scalar`.
>>
>> Signed-off-by: Johannes Schindelin <johasc@microsoft.com>
>> Signed-off-by: Victoria Dye <vdye@github.com>
>> ---
>>  scalar.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> 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"),
> 
> 
> This should not have N_(), as it's a literal command.

Thanks, will fix. 

> 
>> +		NULL
>> +	};
>> +
>> +	argc = parse_options(argc, argv, NULL, options,
>> +			     usage, 0);
>> +
>> +	if (argc != 0)
> 
> If we're re-rolling anyway we usually just do "if (argc)". We don't need
> to worry about argc < 0 (despite the signed type, which is a historical
> C wart).

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

> 
>> +		usage_with_options(usage, options);
>> +
>> +	return run_git("help", "scalar", NULL);
> 
> Performance isn't sensitive here, but have you tried just calling
> cmd_help() instead with the appropriate arguments? It would avoid
> spawning another command..

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. 

Re: [PATCH 6/8] t/perf: add Scalar performance tests

[Victoria Dye]

Johannes Schindelin wrote:
> Hi Victoria,
> 
> On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:
> 
>> [...]
>> +
>> +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
> 
> Given the small numbers presented in the commit message, I suspect that
> even so much as running the command in a subshell might skew the timings
> at least on Windows, where subshells are very, very expensive.
> 
> Given that both `git` and `test_commit` understand the `-C <directory>`
> syntax, this variant would resolve my concern:
> 
> 	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
> 	     "
> 	}
> 
> What do you think?

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

> 
> Ciao,
> Dscho

Re: [PATCH 6/8] t/perf: add Scalar performance tests

[Victoria Dye]

Victoria Dye wrote:
> Johannes Schindelin wrote:
>> Hi Victoria,
>>
>> On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:
>>
>>> [...]
>>> +
>>> +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
>>
>> Given the small numbers presented in the commit message, I suspect that
>> even so much as running the command in a subshell might skew the timings
>> at least on Windows, where subshells are very, very expensive.
>>
>> Given that both `git` and `test_commit` understand the `-C <directory>`
>> syntax, this variant would resolve my concern:
>>
>> 	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
>> 	     "
>> 	}
>>
>> What do you think?
> 
> 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!

> 
>>
>> Ciao,
>> Dscho
> 

Re: [PATCH 6/8] t/perf: add Scalar performance tests

[Junio C Hamano]

"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +test_compare_perf () {
> +	command="$@"
> +	test_perf "$command (scalar)" "
> +		(
> +			cd scalar-clone/src &&
> +			$command
> +		)
> +	"

Our preference is to avoid using "$@" when you are not taking
advantage of the fact that it protects individual parameters from
getting split at $IFS whitespaces.  Use "$*" is preferred here [*].

For example, this is good:

    mytest () {
        for arg in one two "$@" three
        do
            do_something_to "$arg"
        done
    }

    mytest 'a b c' 'd'

Thanks to the use of "$@", 'a b c' stays together and are kept
distinct from 'd'.

The above is not.  command="$@" is used as a misleading synonym for
command="$*" that flattens the arguments to test_compare_perf
function.  If you called it with 'a b c' and 'd' like we called
mytest with, you cannot tell that 'a b c' were together and 'd' was
distinct from the other three.  The only thing test_perf sees would
be that $command without double quotes have four separate tokens.


[Footnote]

* Of course, it is tolerated only in tests and perfs where we are in
  total control of the arguments so that we can declare that no args
  to the shell function have whitespace in them.  In scripts used by
  end-users, we may not be able to get away with "$*".

Re: [PATCH 5/8] scalar-clone: add test coverage

[Victoria Dye]

Johannes Schindelin wrote:
> Hi Victoria,
> 
> On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:
> 
>> From: Victoria Dye <vdye@github.com>
>>
>> Create a new test file ('t9211-scalar-clone.sh') to exercise the options and
>> behavior of the 'scalar clone' command.
> 
> Great catch!
> 
> I have one suggestion, given my experience with debugging test failures:
> 
>> +test_expect_success 'creates content in enlistment root' '
>> +	test_when_finished cleanup_clone cloned &&
>> +
>> +	scalar clone "file://$(pwd)/to-clone" cloned &&
> 
> This pattern of cloning into `cloned` and removing the directory when the
> test case is done is repeated throughout this test script.
> 
> In instances where all test cases succeed, that poses no problem.
> 
> When running the test script with `-i`, also no problem.
> 
> But when we run into test failures in CI, those directories will be
> removed before the workflow run can tar them up and upload them for later
> inspection.
> 
> May I suggest an alternative strategy?
> 
> If we drop all those `test_when_finished cleanup_clone cloned` calls and
> instead `scalar clone` into different directories (whose names reflect the
> test cases' intended purpose), I could imagine having a much easier time
> not only diagnosing but also reproducing and fixing test failures in the
> future.

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? 

> 
> When discussing code review practices [*1*], we did not come up with any
> standard terminology to describe what I am offering here, and I am unsure
> how to label this in a catchy way. I want to present this suggestion for
> you to consider, and I would be delighted if you take it, at the same time
> I will happily let it go should you decide against it.
> 
> Ciao,
> Dscho
> 
> Footnote *1*:
> https://colabti.org/irclogger/irclogger_log/git-devel?date=2022-08-29#l48

Re: [PATCH 7/8] t/perf: add 'GIT_PERF_USE_SCALAR' run option

[Victoria Dye]

Johannes Schindelin wrote:
> Hi Victoria,
> 
> On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:
> 
>> From: Victoria Dye <vdye@github.com>
>>
>> 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.
> 
> Great idea!
> 
>> [...]
>> @@ -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
> 
> Do we need to unregister anything here? My guess is that no, the "global"
> config we're using in tests is "$TRASH_DIRECTORY/.gitconfig", and the side
> effect of scheduling the maintenance task won't matter in practice. But I
> might have missed something and we may want to have an explicit
> `unregister` step.
> 
> What's your take on this?

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!

> 
> Ciao,
> Dscho
> 
>> +		fi
>>  	)
>>  }

Re: [PATCH 5/8] scalar-clone: add test coverage

[Johannes Schindelin]

Hi Victoria,

On Thu, 1 Sep 2022, Victoria Dye wrote:

> Johannes Schindelin wrote:
>
> > On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:
> >
> >> From: Victoria Dye <vdye@github.com>
> >>
> >> Create a new test file ('t9211-scalar-clone.sh') to exercise the options and
> >> behavior of the 'scalar clone' command.
> >
> > Great catch!
> >
> > I have one suggestion, given my experience with debugging test failures:
> >
> >> +test_expect_success 'creates content in enlistment root' '
> >> +	test_when_finished cleanup_clone cloned &&
> >> +
> >> +	scalar clone "file://$(pwd)/to-clone" cloned &&
> >
> > This pattern of cloning into `cloned` and removing the directory when the
> > test case is done is repeated throughout this test script.
> >
> > In instances where all test cases succeed, that poses no problem.
> >
> > When running the test script with `-i`, also no problem.
> >
> > But when we run into test failures in CI, those directories will be
> > removed before the workflow run can tar them up and upload them for later
> > inspection.
> >
> > May I suggest an alternative strategy?
> >
> > If we drop all those `test_when_finished cleanup_clone cloned` calls and
> > instead `scalar clone` into different directories (whose names reflect the
> > test cases' intended purpose), I could imagine having a much easier time
> > not only diagnosing but also reproducing and fixing test failures in the
> > future.
>
> 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).

Thank you!

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

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

>
> >
> > When discussing code review practices [*1*], we did not come up with any
> > standard terminology to describe what I am offering here, and I am unsure
> > how to label this in a catchy way. I want to present this suggestion for
> > you to consider, and I would be delighted if you take it, at the same time
> > I will happily let it go should you decide against it.
> >
> > Ciao,
> > Dscho
> >
> > Footnote *1*:
> > https://colabti.org/irclogger/irclogger_log/git-devel?date=2022-08-29#l48
>
>

Re: [PATCH 6/8] t/perf: add Scalar performance tests

[Johannes Schindelin]

Hi Victoria,

On Thu, 1 Sep 2022, Victoria Dye wrote:

> Victoria Dye wrote:
> > Johannes Schindelin wrote:
> >>
> >> On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:
> >>
> >>> [...]
> >>> +
> >>> +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
> >>
> >> Given the small numbers presented in the commit message, I suspect that
> >> even so much as running the command in a subshell might skew the timings
> >> at least on Windows, where subshells are very, very expensive.
> >>
> >> Given that both `git` and `test_commit` understand the `-C <directory>`
> >> syntax, this variant would resolve my concern:
> >>
> >> 	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
> >> 	     "
> >> 	}
> >>
> >> What do you think?
> >
> > 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'.

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

Re: [PATCH 7/8] t/perf: add 'GIT_PERF_USE_SCALAR' run option

[Johannes Schindelin]

Hi Victoria,

On Thu, 1 Sep 2022, Victoria Dye wrote:

> Johannes Schindelin wrote:
> >
> > On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:
> >
> >> From: Victoria Dye <vdye@github.com>
> >>
> >> 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.
> >
> > Great idea!
> >
> >> [...]
> >> @@ -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
> >
> > Do we need to unregister anything here? My guess is that no, the "global"
> > config we're using in tests is "$TRASH_DIRECTORY/.gitconfig", and the side
> > effect of scheduling the maintenance task won't matter in practice. But I
> > might have missed something and we may want to have an explicit
> > `unregister` step.
> >
> > What's your take on this?
>
> 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.

Thank you for checking!

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

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

Ciao,
Dscho

Re: [PATCH 0/5] Makefile: split up $(test_bindir_programs)

[Johannes Schindelin]

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

Hi Ævar,

On Thu, 1 Sep 2022, Ævar Arnfjörð Bjarmason wrote:

> On Thu, Sep 01 2022, Johannes Schindelin wrote:
>
> > [...]
> > On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:
> > [...]
> >> @@ -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),)|' < $< > $@ && \
> >
> > It took me a good while to wrap my head around this (and let me be clear:
> > I consider none of this your fault, it's the fault of the design of the
> > Makefile syntax).
> >
> > Let me untangle this, for posterity's benefit. We substitute the
> > placeholder `@@PROG@@` with a concatenation of two strings that are both
> > derived from `@F`, i.e. the basename of the to-be-wrapped command.
>
> We could do this later, [... something about touching the same code ...]

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

[PATCH v2 0/9] scalar: integrate into core Git

[Victoria Dye via 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/pull.1005.v10.git.1638538470.gitgitgadget@gmail.com/
 * Introduce 'scalar diagnose':
   https://lore.kernel.org/git/pull.1128.v6.git.1653145696.gitgitgadget@gmail.com/
 * Add '-c/-C' compatibility:
   https://lore.kernel.org/git/pull.1130.v2.git.1643380317358.gitgitgadget@gmail.com/
 * [DROPPED] Integrate Scalar into CI builds:
   https://lore.kernel.org/git/pull.1129.git.1654160735.gitgitgadget@gmail.com/
 * Document Scalar's role in Git & plan remaining work:
   https://lore.kernel.org/git/pull.1275.v2.git.1657584367.gitgitgadget@gmail.com/
 * Generalize 'scalar diagnose' into 'git diagnose' builtin & 'git bugreport
   --diagnose':
   https://lore.kernel.org/git/pull.1310.v4.git.1660335019.gitgitgadget@gmail.com/
 * Add FSMonitor support to Scalar, refactor enlistment search:
   https://lore.kernel.org/git/pull.1324.v3.git.1660858853.gitgitgadget@gmail.com/

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 <johasc@microsoft.com>
     +Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
      
       ## Commit message ##
          scalar: implement the `help` subcommand
      
          It is merely handing off to `git help scalar`.
      
     -    Signed-off-by: Johannes Schindelin <johasc@microsoft.com>
     +    Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
          Signed-off-by: Victoria Dye <vdye@github.com>
      
       ## 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 <johannes.schindelin@gmx.de>
          Signed-off-by: Victoria Dye <vdye@github.com>
      
       ## 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 <johannes.schindelin@gmx.de>
          Signed-off-by: Victoria Dye <vdye@github.com>
      
       ## 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

[PATCH v2 1/9] scalar: fix command documentation section header

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 contrib/scalar/scalar.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/scalar/scalar.txt b/contrib/scalar/scalar.txt
index 1a12dc45077..f33436c7f65 100644
--- a/contrib/scalar/scalar.txt
+++ b/contrib/scalar/scalar.txt
@@ -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

[PATCH v2 2/9] scalar: include in standard Git build & installation

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 .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%)

diff --git a/.gitignore b/.gitignore
index 80b530bbed2..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
diff --git a/Documentation/Makefile b/Documentation/Makefile
index bd6b6fcb930..16c9e062390 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -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
diff --git a/contrib/scalar/scalar.txt b/Documentation/scalar.txt
similarity index 100%
rename from contrib/scalar/scalar.txt
rename to Documentation/scalar.txt
diff --git a/Makefile b/Makefile
index eac30126e29..e03f32ec1e7 100644
--- a/Makefile
+++ b/Makefile
@@ -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::
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 2237109b57f..bae203c1fb9 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -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)
diff --git a/contrib/scalar/.gitignore b/contrib/scalar/.gitignore
deleted file mode 100644
index ff3d47e84d0..00000000000
--- a/contrib/scalar/.gitignore
+++ /dev/null
@@ -1,2 +0,0 @@
-/*.exe
-/scalar
diff --git a/contrib/scalar/Makefile b/contrib/scalar/Makefile
deleted file mode 100644
index 37f283f35d7..00000000000
--- a/contrib/scalar/Makefile
+++ /dev/null
@@ -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
diff --git a/contrib/scalar/t/Makefile b/contrib/scalar/t/Makefile
deleted file mode 100644
index 1ed174a8cf3..00000000000
--- a/contrib/scalar/t/Makefile
+++ /dev/null
@@ -1,81 +0,0 @@
-# Import tree-wide shared Makefile behavior and libraries
-include ../../../shared.mak
-
-# Run scalar tests
-#
-# Copyright (c) 2005,2021 Junio C Hamano, Johannes Schindelin
-#
-
--include ../../../config.mak.autogen
--include ../../../config.mak
-
-SHELL_PATH ?= $(SHELL)
-PERL_PATH ?= /usr/bin/perl
-RM ?= rm -f
-PROVE ?= prove
-DEFAULT_TEST_TARGET ?= test
-TEST_LINT ?= test-lint
-
-ifdef TEST_OUTPUT_DIRECTORY
-TEST_RESULTS_DIRECTORY = $(TEST_OUTPUT_DIRECTORY)/test-results
-else
-TEST_RESULTS_DIRECTORY = ../../../t/test-results
-endif
-
-# Shell quote;
-SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
-PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
-TEST_RESULTS_DIRECTORY_SQ = $(subst ','\'',$(TEST_RESULTS_DIRECTORY))
-
-T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))
-
-all: $(DEFAULT_TEST_TARGET)
-
-test: $(TEST_LINT)
-	$(MAKE) aggregate-results-and-cleanup
-
-prove: $(TEST_LINT)
-	@echo "*** prove ***"; GIT_CONFIG=.git/config $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
-	$(MAKE) clean-except-prove-cache
-
-$(T):
-	@echo "*** $@ ***"; GIT_CONFIG=.git/config '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
-
-clean-except-prove-cache:
-	$(RM) -r 'trash directory'.*
-	$(RM) -r valgrind/bin
-
-clean: clean-except-prove-cache
-	$(RM) .prove
-
-test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax
-
-test-lint-duplicates:
-	@dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
-		test -z "$$dups" || { \
-		echo >&2 "duplicate test numbers:" $$dups; exit 1; }
-
-test-lint-executable:
-	@bad=`for i in $(T); do test -x "$$i" || echo $$i; done` && \
-		test -z "$$bad" || { \
-		echo >&2 "non-executable tests:" $$bad; exit 1; }
-
-test-lint-shell-syntax:
-	@'$(PERL_PATH_SQ)' ../../../t/check-non-portable-shell.pl $(T)
-
-aggregate-results-and-cleanup: $(T)
-	$(MAKE) aggregate-results
-	$(MAKE) clean
-
-aggregate-results:
-	for f in '$(TEST_RESULTS_DIRECTORY_SQ)'/t*-*.counts; do \
-		echo "$$f"; \
-	done | '$(SHELL_PATH_SQ)' ../../../t/aggregate-results.sh
-
-valgrind:
-	$(MAKE) GIT_TEST_OPTS="$(GIT_TEST_OPTS) --valgrind"
-
-test-results:
-	mkdir -p test-results
-
-.PHONY: $(T) aggregate-results clean valgrind
diff --git a/contrib/scalar/scalar.c b/scalar.c
similarity index 100%
rename from contrib/scalar/scalar.c
rename to scalar.c
diff --git a/contrib/scalar/t/t9099-scalar.sh b/t/t9210-scalar.sh
similarity index 96%
rename from contrib/scalar/t/t9099-scalar.sh
rename to t/t9210-scalar.sh
index dfb949f52ee..14ca575a214 100755
--- a/contrib/scalar/t/t9099-scalar.sh
+++ b/t/t9210-scalar.sh
@@ -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

[PATCH v2 3/9] git help: special-case `scalar`

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 builtin/help.c | 2 ++
 1 file changed, 2 insertions(+)

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);
 }
-- 
gitgitgadget

[PATCH v2 4/9] scalar: implement the `help` subcommand

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

It is merely handing off to `git help scalar`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 scalar.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/scalar.c b/scalar.c
index 642d16124eb..c5c1ce68919 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[] = {
+		"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

[PATCH v2 5/9] scalar: add to 'git help -a' command list

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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

Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 Documentation/cmd-list.perl | 2 +-
 command-list.txt            | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/cmd-list.perl b/Documentation/cmd-list.perl
index af5da45d287..9515a499a31 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 (/^git[a-z0-9-]*\(([0-9])\)$/) {
+		if (/^(git|scalar)[a-z0-9-]*\(([0-9])\)$/) {
 			$mansection = $1;
 			next;
 		}
diff --git a/command-list.txt b/command-list.txt
index f96bdabd7d9..93f94e42ab7 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -235,3 +235,4 @@ gittutorial                             guide
 gittutorial-2                           guide
 gitweb                                  ancillaryinterrogators
 gitworkflows                            guide
+scalar                                  mainporcelain
-- 
gitgitgadget

[PATCH v2 6/9] scalar-clone: add test coverage

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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.

Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 t/t9211-scalar-clone.sh | 151 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 151 insertions(+)
 create mode 100755 t/t9211-scalar-clone.sh

diff --git a/t/t9211-scalar-clone.sh b/t/t9211-scalar-clone.sh
new file mode 100755
index 00000000000..dd33d87e9be
--- /dev/null
+++ b/t/t9211-scalar-clone.sh
@@ -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

[PATCH v2 7/9] t/perf: add Scalar performance tests

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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)

Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 t/perf/p9210-scalar.sh | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100755 t/perf/p9210-scalar.sh

diff --git a/t/perf/p9210-scalar.sh b/t/perf/p9210-scalar.sh
new file mode 100755
index 00000000000..265f7cd1fe2
--- /dev/null
+++ b/t/perf/p9210-scalar.sh
@@ -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

[PATCH v2 8/9] t/perf: add 'GIT_PERF_USE_SCALAR' run option

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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.

Suggested-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 t/perf/README      |  4 ++++
 t/perf/perf-lib.sh | 13 ++++++++++++-
 t/perf/run         |  3 +++
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/t/perf/README b/t/perf/README
index fb9127a66f7..8f217d7be7d 100644
--- a/t/perf/README
+++ b/t/perf/README
@@ -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:
 
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 27c28017921..e7786775a90 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -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
 	)
 }
 
diff --git a/t/perf/run b/t/perf/run
index 55219aa4056..33da4d2aba2 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -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

[PATCH v2 9/9] Documentation/technical: include Scalar technical doc

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 Documentation/Makefile             |  1 +
 Documentation/technical/scalar.txt | 61 ------------------------------
 2 files changed, 1 insertion(+), 61 deletions(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 16c9e062390..9ec53afdf18 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -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
diff --git a/Documentation/technical/scalar.txt b/Documentation/technical/scalar.txt
index 0600150b3ad..921cb104c3c 100644
--- a/Documentation/technical/scalar.txt
+++ b/Documentation/technical/scalar.txt
@@ -64,64 +64,3 @@ some "global" `git` options (e.g., `-c` and `-C`).
 Because `scalar` is not invoked as a Git subcommand (like `git scalar`), it is
 built and installed as its own executable in the `bin/` directory, alongside
 `git`, `git-gui`, etc.
-
-Roadmap
--------
-
-NOTE: this section will be removed once the remaining tasks outlined in this
-roadmap are complete.
-
-Scalar is a large enough project that it is being upstreamed incrementally,
-living in `contrib/` until it is feature-complete. So far, the following patch
-series have been accepted:
-
-- `scalar-the-beginning`: The initial patch series which sets up
-  `contrib/scalar/` and populates it with a minimal `scalar` command that
-  demonstrates the fundamental ideas.
-
-- `scalar-c-and-C`: The `scalar` command learns about two options that can be
-  specified before the command, `-c <key>=<value>` and `-C <directory>`.
-
-- `scalar-diagnose`: The `scalar` command is taught the `diagnose` subcommand.
-
-- `scalar-generalize-diagnose`: Move the functionality of `scalar diagnose`
-  into `git diagnose` and `git bugreport --diagnose`.
-
-- 'scalar-add-fsmonitor: Enable the built-in FSMonitor in Scalar
-  enlistments. At the end of this series, Scalar should be feature-complete
-  from the perspective of a user.
-
-Roughly speaking (and subject to change), the following series are needed to
-"finish" this initial version of Scalar:
-
-- Move Scalar to toplevel: Move Scalar out of `contrib/` and into the root of
-  `git`. This includes a variety of related updates, including:
-    - building & installing Scalar in the Git root-level 'make [install]'.
-    - builing & testing Scalar as part of CI.
-    - moving and expanding test coverage of Scalar (including perf tests).
-    - implementing 'scalar help'/'git help scalar' to display scalar
-      documentation.
-
-Finally, there are two additional patch series that exist in Microsoft's fork of
-Git, but there is no current plan to upstream them. There are some interesting
-ideas there, but the implementation is too specific to Azure Repos and/or VFS
-for Git to be of much help in general.
-
-These still exist mainly because the GVFS protocol is what Azure Repos has
-instead of partial clone, while Git is focused on improving partial clone:
-
-- `scalar-with-gvfs`: The primary purpose of this patch series is to support
-  existing Scalar users whose repositories are hosted in Azure Repos (which does
-  not support Git's partial clones, but supports its predecessor, the GVFS
-  protocol, which is used by Scalar to emulate the partial clone).
-
-  Since the GVFS protocol will never be supported by core Git, this patch series
-  will remain in Microsoft's fork of Git.
-
-- `run-scalar-functional-tests`: The Scalar project developed a quite
-  comprehensive set of integration tests (or, "Functional Tests"). They are the
-  sole remaining part of the original C#-based Scalar project, and this patch
-  adds a GitHub workflow that runs them all.
-
-  Since the tests partially depend on features that are only provided in the
-  `scalar-with-gvfs` patch series, this patch cannot be upstreamed.
-- 
gitgitgadget

Re: [PATCH 5/8] scalar-clone: add test coverage

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

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.

Re: [PATCH v2 0/9] scalar: integrate into core Git

[Johannes Schindelin]

Hi Victoria,

On Fri, 2 Sep 2022, Victoria Dye via GitGitGadget wrote:

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

This iteration addresses all concerns I had.

Thank you so much!
Dscho

Re: [PATCH v2 0/9] scalar: integrate into core Git

[Derrick Stolee]

On 9/2/2022 11:56 AM, Victoria Dye via GitGitGadget wrote:
> 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.

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

[PATCH v2 0/3] Makefile: fix issues with bin-wrappers/% rule

[Ævar Arnfjörð Bjarmason]

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/cover-0.5-00000000000-20220901T130817Z-avarab@gmail.com/

Æ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

[PATCH v2 1/3] Makefile: factor sed-powered '#!/bin/sh' munging into a variable

[Ævar Arnfjörð Bjarmason]

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.

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

diff --git a/Makefile b/Makefile
index 85f03c6aed1..45b22d33513 100644
--- a/Makefile
+++ b/Makefile
@@ -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

[PATCH v2 2/3] Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier

[Ævar Arnfjörð Bjarmason]

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

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

diff --git a/Makefile b/Makefile
index 45b22d33513..a8cfa096dc1 100644
--- a/Makefile
+++ b/Makefile
@@ -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

[PATCH v2 3/3] Makefile: simplify $(test_bindir_programs) rule by splitting it up

[Ævar Arnfjörð Bjarmason]

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/sso99so6-n28s-rq86-8q20-4456r3pn869r@tzk.qr/
5. 46bac904581 (Do not install shell libraries executable, 2010-01-31)
6. 7b5c93c6c68" (scalar: include in standard Git build & installation,
   2022-09-02)

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

diff --git a/Makefile b/Makefile
index a8cfa096dc1..d551a89bbff 100644
--- a/Makefile
+++ b/Makefile
@@ -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

Re: [PATCH v2 2/3] Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier

[Junio C Hamano]

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

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

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

>  TEST_PROGRAMS = $(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X))
> +all:: $(TEST_PROGRAMS)

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.

> +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)

Re: [PATCH v2 1/3] Makefile: factor sed-powered '#!/bin/sh' munging into a variable

[Junio C Hamano]

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

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

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

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Makefile | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 85f03c6aed1..45b22d33513 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -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 $@

Re: [PATCH v2 2/3] Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier

[Ævar Arnfjörð Bjarmason]

On Wed, Oct 26 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> 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.
>
> Makes sense, I guess.  So TEST_OBJS is no longer used?

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

>>  TEST_PROGRAMS = $(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X))
>> +all:: $(TEST_PROGRAMS)
>
> 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?

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

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

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

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

Thanks for the quick review!

Re: [PATCH v2 3/3] Makefile: simplify $(test_bindir_programs) rule by splitting it up

[Junio C Hamano]

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

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

We've talked about relying on another feature of GNU make (which we
already depend on) to automatically remove the target build artifact
when a rule to build it fails, but I see quite a many old world best
practice pattern "generate >$@+ && chmod +x $@+ && mv $@+ $@" still
in today's Makefile.  What happens when a rule that uses the above
fails in the middle?  Do we correctly remove the half-built target?

> -all:: $(test_bindir_programs)
> +define bin_wrappers_template
> +
> +## bin_wrappers_template
> +# 1 = $(1)
> +# 2 = $(2)
> +# 3 = $(3)
> +# 4 = $(4)

Whatever the above comment wants to convey to readers, it seems to
fail to do so at least to me, while ...

> +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
>  
> +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)

... what it expands to looked quite straight-forward.

Re: [PATCH v2 2/3] Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier

[Junio C Hamano]

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

> On Wed, Oct 26 2022, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>>> 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.
>>
>> Makes sense, I guess.  So TEST_OBJS is no longer used?
>
> Yes, sorry I'll clarify that in a re-roll.

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

>> 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.
>
> ...here I'm not quite sure if you want to keep it after all or nat...

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

Re: [PATCH v2 3/3] Makefile: simplify $(test_bindir_programs) rule by splitting it up

[Ævar Arnfjörð Bjarmason]

On Wed, Oct 26 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> -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

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

> We've talked about relying on another feature of GNU make (which we
> already depend on) to automatically remove the target build artifact
> when a rule to build it fails, [...]  What happens when a rule that
> uses the above fails in the middle?  Do we correctly remove the
> half-built target?

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

> but I see quite a many old world best practice pattern "generate >$@+
> && chmod +x $@+ && mv $@+ $@" still in today's 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/patch-1.6-3330cdbccc0-20210329T161723Z-avarab@gmail.com/
(it didn't go in).

>> -all:: $(test_bindir_programs)
>> +define bin_wrappers_template
>> +
>> +## bin_wrappers_template
>> +# 1 = $(1)
>> +# 2 = $(2)
>> +# 3 = $(3)
>> +# 4 = $(4)
>
> Whatever the above comment wants to convey to readers, it seems to
> fail to do so at least to me, while ...

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

?

Re: [PATCH v2 3/3] Makefile: simplify $(test_bindir_programs) rule by splitting it up

[Junio C Hamano]

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

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

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.

> 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

OK.  That use pattern was what I was missing.
> ...
> 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

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

Re: [PATCH v2 3/3] Makefile: simplify $(test_bindir_programs) rule by splitting it up

[Ævar Arnfjörð Bjarmason]

On Wed, Oct 26 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> 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?
>
> 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.

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

>> 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
>
> OK.  That use pattern was what I was missing.
>> ...
>> 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
>
> Yeah, it would be totally useless without such an instruction.

Will update it with that.

Re: [PATCH v2 0/3] Makefile: fix issues with bin-wrappers/% rule

[Jeff King]

On Wed, Oct 26, 2022 at 04:42:34PM +0200, Ævar Arnfjörð Bjarmason wrote:

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

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

Re: [PATCH v2 0/3] Makefile: fix issues with bin-wrappers/% rule

[Ævar Arnfjörð Bjarmason]

On Thu, Oct 27 2022, Jeff King wrote:

> On Wed, Oct 26, 2022 at 04:42:34PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> 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".
>
> 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?

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.

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

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.

[PATCH v3 0/4] Makefile: untangle bin-wrappers/% rule complexity

[Ævar Arnfjörð Bjarmason]

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/cover-v2-0.3-00000000000-20221026T143533Z-avarab@gmail.com/

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

[PATCH v3 1/4] Makefile: factor sed-powered '#!/bin/sh' munging into a variable

[Ævar Arnfjörð Bjarmason]

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.

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

diff --git a/Makefile b/Makefile
index 4927379184c..679c24377f4 100644
--- a/Makefile
+++ b/Makefile
@@ -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

[PATCH v3 2/4] Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier

[Ævar Arnfjörð Bjarmason]

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

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

diff --git a/Makefile b/Makefile
index 679c24377f4..911cce6d487 100644
--- a/Makefile
+++ b/Makefile
@@ -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

[PATCH v3 3/4] Makefile: rename "test_bindir_programs" variable, pre-declare

[Ævar Arnfjörð Bjarmason]

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

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

diff --git a/Makefile b/Makefile
index 911cce6d487..032654640c5 100644
--- a/Makefile
+++ b/Makefile
@@ -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

[PATCH v3 4/4] Makefile: simplify $(test_bindir_programs) rule by splitting it up

[Ævar Arnfjörð Bjarmason]

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/sso99so6-n28s-rq86-8q20-4456r3pn869r@tzk.qr/
5. 46bac904581 (Do not install shell libraries executable, 2010-01-31)
6. 7b5c93c6c68" (scalar: include in standard Git build & installation,
   2022-09-02)

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

diff --git a/Makefile b/Makefile
index 032654640c5..d7ab68e3db8 100644
--- a/Makefile
+++ b/Makefile
@@ -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

Re: [PATCH v3 0/4] Makefile: untangle bin-wrappers/% rule complexity

[Taylor Blau]

On Mon, Oct 31, 2022 at 11:28:05PM +0100, Ævar Arnfjörð Bjarmason wrote:
> Æ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(-)

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

Re: [PATCH v3 0/4] Makefile: untangle bin-wrappers/% rule complexity

[Ævar Arnfjörð Bjarmason]

On Mon, Oct 31 2022, Taylor Blau wrote:

> On Mon, Oct 31, 2022 at 11:28:05PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> Æ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(-)
>
> 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.

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/cover-v2-0.3-00000000000-20221026T143533Z-avarab@gmail.com/