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