patch, 8 partsscalar: integrate into core Git

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

Goals =====

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

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

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

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

The series is broken up as follows:

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

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

 * Add 'scalar' command in 'contrib/':
   https://lore.kernel.org/git/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

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

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

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

Hi Victoria,

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

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

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

On Thu, Sep 01 2022, Johannes Schindelin wrote:

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

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

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

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

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

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

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

 
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

Hi Ævar,

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

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

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

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

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

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

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

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

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?

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

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

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

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

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

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

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

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

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

"Mostly" because one thing they still do is not replace the file until it's ready, we had a previous discussion on a related topic in reply to https://lore.kernel.org/git/patch-1.6-3330cdbccc0-20210329T161723Z-avarab@gmail.com/ (it didn't go in).

I can drop them, FWIW I've found it quite handy to add these to ad-hoc
debug templates. E.g. you can run:
	
	$ make -f /dev/null -E '$(eval $(file <Makefile))' -E '$(error $(call bin_wrappers_template,a,b,c,d))'
	make: *** 
	## bin_wrappers_template
	# 1 = a
	# 2 = b
	# 3 = c
	# 4 = d
	BW_a = $(a:%=bin-wrappers/%)
	BIN_WRAPPERS += $(BW_a)
	all:: $(BW_a)
	$(BW_a): bin-wrappers/% : c%d
	$(BW_a): wrap-for-bin.sh
	        $(call mkdir_p_parent_template)
	        $(QUIET_GEN)$(call cmd_munge_bin_wrappers_script,b,c,d).  Stop.

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

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

?

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

Not at all.

I was wondering why the defined sequance does not end in

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

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

OK.  That use pattern was what I was missing.

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

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

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

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

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

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

Will update it with that.

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

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

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

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

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

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

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

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

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

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

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

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

Thanks for the quick review!

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

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

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

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

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

On Thu, Oct 27 2022, Jeff King wrote:

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

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

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

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

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

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

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

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

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

1. https://lore.kernel.org/git/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)'

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

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

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

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

On Mon, Oct 31, 2022 at 11:28:05PM +0100, Ævar Arnfjörð Bjarmason wrote:

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

On Mon, Oct 31 2022, Taylor Blau wrote:

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/

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

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

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

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

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

Ævar Arnfjörð Bjarmason wrote:

Thanks, will fix. 

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

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

Hi Victoria,

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

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

Thank you, Dscho

Hi Victoria,

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

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

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

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

Hi Victoria,

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

Great catch!

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

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

Johannes Schindelin wrote:

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?

Hi Victoria,

On Thu, 1 Sep 2022, Victoria Dye wrote:

Thank you!

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

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

Ciao, Dscho

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

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.

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

Hi Victoria,

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

Great idea!

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

Johannes Schindelin wrote:

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

Thanks!

Hi Victoria,

On Thu, 1 Sep 2022, Victoria Dye wrote:

Thank you for checking!

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

Ciao, Dscho

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

Hi Victoria,

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

Excellent!

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

Johannes Schindelin wrote:

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

Victoria Dye wrote:

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

Thanks!

Hi Victoria,

On Thu, 1 Sep 2022, Victoria Dye wrote:

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

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

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

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

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

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.

Ævar Arnfjörð Bjarmason wrote:

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/

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

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

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

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

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

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

Hi Victoria,

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

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

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

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

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

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

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

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

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

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

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

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

Hi Victoria,

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

This iteration addresses all concerns I had.

Thank you so much! Dscho

On 9/2/2022 11:56 AM, Victoria Dye via GitGitGadget wrote:

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