Re: [PATCH 0/8] scalar: integrate into core Git
- From
Victoria Dye <vdye@github.com>
- Date
- Aug 31, 2022, 18:42 UTC
- Message-ID
- <85ad26ba-8461-fdda-fb26-c99080dfa10d@github.com>
- In-Reply-To
- <220831.86y1v48h2x.gmgdl@evledraar.gmail.com>
Ævar Arnfjörð Bjarmason wrote:
Show 41 quoted lines
> > 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/
Show 10 quoted lines
> > 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!
Show 18 quoted lines
> > 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.
Show 7 quoted lines
> > +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/
Show 37 quoted lines
>
> @@ -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.
Show 20 quoted lines
> > 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.