From: Victoria Dye <vdye@github.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, derrickstolee@github.com,
johannes.schindelin@gmx.de, gitster@pobox.com
Subject: Re: [PATCH 0/8] scalar: integrate into core Git
Date: Wed, 31 Aug 2022 11:42:20 -0700 [thread overview]
Message-ID: <85ad26ba-8461-fdda-fb26-c99080dfa10d@github.com> (raw)
In-Reply-To: <220831.86y1v48h2x.gmgdl@evledraar.gmail.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.
next prev parent reply other threads:[~2022-08-31 18:43 UTC|newest]
Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-31 16:02 [PATCH 0/8] scalar: integrate into core Git Victoria Dye via GitGitGadget
2022-08-31 16:02 ` [PATCH 1/8] scalar: fix command documentation section header Victoria Dye via GitGitGadget
2022-08-31 16:02 ` [PATCH 2/8] scalar: include in standard Git build & installation Victoria Dye via GitGitGadget
2022-09-01 9:11 ` Johannes Schindelin
2022-09-01 13:17 ` [PATCH 0/5] Makefile: split up $(test_bindir_programs) Ævar Arnfjörð Bjarmason
2022-09-01 13:17 ` [PATCH 1/5] Makefile: factor sed-powered '#!/bin/sh' munging into a variable Ævar Arnfjörð Bjarmason
2022-09-01 13:17 ` [PATCH 2/5] Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier Ævar Arnfjörð Bjarmason
2022-09-01 13:17 ` [PATCH 3/5] Makefile: simplify $(test_bindir_programs) rule by splitting it up Ævar Arnfjörð Bjarmason
2022-09-01 13:17 ` [PATCH 4/5] Makefile: define bin-wrappers/% rules with a template Ævar Arnfjörð Bjarmason
2022-09-01 13:17 ` [PATCH 5/5] Makefile: fix "make clean && make bin-wrappers/$NAME" dependencies Ævar Arnfjörð Bjarmason
2022-09-01 15:02 ` [PATCH 0/5] Makefile: split up $(test_bindir_programs) Derrick Stolee
2022-09-02 12:38 ` Johannes Schindelin
2022-10-26 14:42 ` [PATCH v2 0/3] Makefile: fix issues with bin-wrappers/% rule Ævar Arnfjörð Bjarmason
2022-10-26 14:42 ` [PATCH v2 1/3] Makefile: factor sed-powered '#!/bin/sh' munging into a variable Ævar Arnfjörð Bjarmason
2022-10-26 17:51 ` Junio C Hamano
2022-10-26 14:42 ` [PATCH v2 2/3] Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier Ævar Arnfjörð Bjarmason
2022-10-26 16:47 ` Junio C Hamano
2022-10-26 18:47 ` Ævar Arnfjörð Bjarmason
2022-10-26 19:13 ` Junio C Hamano
2022-10-26 14:42 ` [PATCH v2 3/3] Makefile: simplify $(test_bindir_programs) rule by splitting it up Ævar Arnfjörð Bjarmason
2022-10-26 18:48 ` Junio C Hamano
2022-10-26 19:14 ` Ævar Arnfjörð Bjarmason
2022-10-26 20:28 ` Junio C Hamano
2022-10-26 20:43 ` Ævar Arnfjörð Bjarmason
2022-10-28 0:57 ` [PATCH v2 0/3] Makefile: fix issues with bin-wrappers/% rule Jeff King
2022-10-28 3:03 ` Ævar Arnfjörð Bjarmason
2022-10-31 22:28 ` [PATCH v3 0/4] Makefile: untangle bin-wrappers/% rule complexity Ævar Arnfjörð Bjarmason
2022-10-31 22:28 ` [PATCH v3 1/4] Makefile: factor sed-powered '#!/bin/sh' munging into a variable Ævar Arnfjörð Bjarmason
2022-10-31 22:28 ` [PATCH v3 2/4] Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier Ævar Arnfjörð Bjarmason
2022-10-31 22:28 ` [PATCH v3 3/4] Makefile: rename "test_bindir_programs" variable, pre-declare Ævar Arnfjörð Bjarmason
2022-10-31 22:28 ` [PATCH v3 4/4] Makefile: simplify $(test_bindir_programs) rule by splitting it up Ævar Arnfjörð Bjarmason
2022-10-31 23:54 ` [PATCH v3 0/4] Makefile: untangle bin-wrappers/% rule complexity Taylor Blau
2022-11-01 1:29 ` Ævar Arnfjörð Bjarmason
2022-08-31 16:02 ` [PATCH 3/8] git help: special-case `scalar` Johannes Schindelin via GitGitGadget
2022-08-31 16:02 ` [PATCH 4/8] scalar: implement the `help` subcommand Johannes Schindelin via GitGitGadget
2022-08-31 16:48 ` Ævar Arnfjörð Bjarmason
2022-09-01 16:08 ` Victoria Dye
2022-09-01 8:51 ` Johannes Schindelin
2022-09-01 9:17 ` Johannes Schindelin
2022-08-31 16:02 ` [PATCH 5/8] scalar-clone: add test coverage Victoria Dye via GitGitGadget
2022-09-01 9:32 ` Johannes Schindelin
2022-09-01 23:49 ` Victoria Dye
2022-09-02 9:07 ` Johannes Schindelin
2022-09-02 16:52 ` Junio C Hamano
2022-08-31 16:02 ` [PATCH 6/8] t/perf: add Scalar performance tests Victoria Dye via GitGitGadget
2022-09-01 9:39 ` Johannes Schindelin
2022-09-01 16:15 ` Victoria Dye
2022-09-01 16:21 ` Victoria Dye
2022-09-02 9:16 ` Johannes Schindelin
2022-09-01 16:43 ` Junio C Hamano
2022-08-31 16:02 ` [PATCH 7/8] t/perf: add 'GIT_PERF_USE_SCALAR' run option Victoria Dye via GitGitGadget
2022-09-01 9:43 ` Johannes Schindelin
2022-09-02 4:00 ` Victoria Dye
2022-09-02 9:17 ` Johannes Schindelin
2022-08-31 16:02 ` [PATCH 8/8] Documentation/technical: include Scalar technical doc Victoria Dye via GitGitGadget
2022-08-31 17:03 ` [PATCH 0/8] scalar: integrate into core Git Ævar Arnfjörð Bjarmason
2022-08-31 18:42 ` Victoria Dye [this message]
2022-09-01 9:56 ` Johannes Schindelin
2022-09-02 15:56 ` [PATCH v2 0/9] " Victoria Dye via GitGitGadget
2022-09-02 15:56 ` [PATCH v2 1/9] scalar: fix command documentation section header Victoria Dye via GitGitGadget
2022-09-02 15:56 ` [PATCH v2 2/9] scalar: include in standard Git build & installation Victoria Dye via GitGitGadget
2022-09-02 15:56 ` [PATCH v2 3/9] git help: special-case `scalar` Johannes Schindelin via GitGitGadget
2022-09-02 15:56 ` [PATCH v2 4/9] scalar: implement the `help` subcommand Johannes Schindelin via GitGitGadget
2022-09-02 15:56 ` [PATCH v2 5/9] scalar: add to 'git help -a' command list Victoria Dye via GitGitGadget
2022-09-02 15:56 ` [PATCH v2 6/9] scalar-clone: add test coverage Victoria Dye via GitGitGadget
2022-09-02 15:56 ` [PATCH v2 7/9] t/perf: add Scalar performance tests Victoria Dye via GitGitGadget
2022-09-02 15:56 ` [PATCH v2 8/9] t/perf: add 'GIT_PERF_USE_SCALAR' run option Victoria Dye via GitGitGadget
2022-09-02 15:56 ` [PATCH v2 9/9] Documentation/technical: include Scalar technical doc Victoria Dye via GitGitGadget
2022-09-05 10:36 ` [PATCH v2 0/9] scalar: integrate into core Git Johannes Schindelin
2022-09-08 20:54 ` Derrick Stolee
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=85ad26ba-8461-fdda-fb26-c99080dfa10d@github.com \
--to=vdye@github.com \
--cc=avarab@gmail.com \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=johannes.schindelin@gmx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).