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

[Victoria Dye <vdye@github.com>]

Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Aug 31 2022, Victoria Dye via GitGitGadget wrote:
> 
>> This series completes the initial implementation of the Scalar command as a
>> core component of Git. For readers new to the topic of Scalar, the
>> roadmap/design doc [1] provides some background information including how
>> the project started & evolved, as well as its current intent & high-level
>> design.
>>
>> [...]
>>
>> Prior series
>> ============
>>
>>  * Add 'scalar' command in 'contrib/':
>>    https://lore.kernel.org/git/pull.1005.v10.git.1638538470.gitgitgadget@gmail.com/
>>  * Introduce 'scalar diagnose':
>>    https://lore.kernel.org/git/pull.1128.v6.git.1653145696.gitgitgadget@gmail.com/
>>  * Add '-c/-C' compatibility:
>>    https://lore.kernel.org/git/pull.1130.v2.git.1643380317358.gitgitgadget@gmail.com/
>>  * [DROPPED] Integrate Scalar into CI builds:
>>    https://lore.kernel.org/git/pull.1129.git.1654160735.gitgitgadget@gmail.com/
>>  * Document Scalar's role in Git & plan remaining work:
>>    https://lore.kernel.org/git/pull.1275.v2.git.1657584367.gitgitgadget@gmail.com/
>>  * Generalize 'scalar diagnose' into 'git diagnose' builtin & 'git bugreport
>>    --diagnose':
>>    https://lore.kernel.org/git/pull.1310.v4.git.1660335019.gitgitgadget@gmail.com/
>>  * Add FSMonitor support to Scalar, refactor enlistment search:
>>    https://lore.kernel.org/git/pull.1324.v3.git.1660858853.gitgitgadget@gmail.com/
>>
>> Thanks!
>>
>>  * Victoria
> 
> I'm happy to see this finally coming. I can say I've thoroughly reviewed
> it & tested it for the better part of a year now. Since most of it is
> the same or functionally the same as previous patches I sent at [1] and
> [2]. It's odd not to see any mention of that here:
> 
> 	1. https://lore.kernel.org/git/cover-v2-0.1-00000000000-20220623T100554Z-avarab@gmail.com/
> 	2. https://lore.kernel.org/git/patch-1.1-86fb8d56307-20211028T185016Z-avarab@gmail.com/

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

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

[1] https://github.com/microsoft/git/commit/4f553e0027190484899afba955c7cd3f1de77532
[2] https://lore.kernel.org/git/pull.1129.git.1654160735.gitgitgadget@gmail.com/

> 
> In any case. I applied this & a rebased version I've kept of [1]
> locally, and I'll be commenting below on the diff between the two, which
> I produced with:
> 
> 	git diff --stat -p avar/scalar-move-build-from-contrib-3 HEAD -- ':!t/t9211-scalar-clone.sh' ':!Documentation/technical/scalar.txt' ':!t/perf/'
> 
> I.e. you can get my version at
> http://github.com/avar/git/tree/avar/scalar-move-build-from-contrib-3 if
> you're interested, and I omitted the changes to paths unique to yours:

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

> 	 
> 	diff --git a/Documentation/scalar.txt b/Documentation/scalar.txt
> 	index f33436c7f65..505a1cea0fd 100644
> 	--- a/Documentation/scalar.txt
> 	+++ b/Documentation/scalar.txt
> 	@@ -163,4 +163,4 @@ linkgit:git-clone[1], linkgit:git-maintenance[1].
> 	 
> 	 GIT
> 	 ---
> 	-Part of the linkgit:git[1] suite
> 	+Associated with the linkgit:git[1] suite
> 
> You just kept this, but it really should be the former. The target
> audience of this bit of the documentation is some sysadmin that's
> wondering what this "scalar" thing is, and does "man scalar".
> 
> Let's not be cute, we're shipping it as part of git, it's not
> "associated" anymore, it's part of the git suite.

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

> 	 
> 	+SCALAR_OBJS += scalar.o
> 	+.PHONY: scalar-objs
> 	+scalar-objs: $(SCALAR_OBJS)
> 
> This part looks missing from yours. I.e. we do this with the rest of our
> "objects", just gravy of course...

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

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

> 
> 	@@ -3536,6 +3514,7 @@ ALL_COMMANDS += git-citool
> 	 ALL_COMMANDS += git-gui
> 	 ALL_COMMANDS += gitk
> 	 ALL_COMMANDS += gitweb
> 	+ALL_COMMANDS += scalar
> 	 
> 	 .PHONY: check-docs
> 	 check-docs::
> 	@@ -3571,7 +3550,7 @@ check-docs::
> 	 		    -e 's/\.txt//'; \
> 	 	) | while read how cmd; \
> 	 	do \
> 	-		case " $(patsubst %$X,%,$(ALL_COMMANDS) $(BUILT_INS) $(EXCLUDED_PROGRAMS) scalar) " in \
> 	+		case " $(patsubst %$X,%,$(ALL_COMMANDS) $(BUILT_INS) $(EXCLUDED_PROGRAMS)) " in \
> 	 		*" $$cmd "*)	;; \
> 	 		*) echo "removed but $$how: $$cmd" ;; \
> 	 		esac; \
> 	diff --git a/builtin/help.c b/builtin/help.c
> 	index 09ac4289f13..6f2796f211e 100644
> 	--- a/builtin/help.c
> 	+++ b/builtin/help.c
> 	@@ -440,6 +440,8 @@ static const char *cmd_to_page(const char *git_cmd)
> 	 		return git_cmd;
> 	 	else if (is_git_command(git_cmd))
> 	 		return xstrfmt("git-%s", git_cmd);
> 	+	else if (!strcmp("scalar", git_cmd))
> 	+		return xstrdup(git_cmd);
> 	 	else
> 	 		return xstrfmt("git%s", git_cmd);
> 	 }
> 
> Works, but I wonder if 3/8 here in your series is what we really want,
> i.e. isn't the point of "git" to be a holistic thing for git, and for
> "scalar" to be set apart from that?
> 
> But OTOH much of the docs would need to cross-link anyway...

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

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

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

> 
> Thanks.