Upstreaming the Scalar command #1005
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
derrickstolee
left a comment
derrickstolee
left a comment
There was a problem hiding this comment.
Really well organized! I found a few points that would be good to fix before sending upstream.
dscho
force-pushed
the
scalar-the-beginning
branch
2 times, most recently
from
0648bf6 to
ee893f2
Compare
|
I'm happy with your latest version. Thanks! |
dscho
force-pushed
the
scalar-the-beginning
branch
2 times, most recently
from
0833924 to
4f609b2
Compare
Member
Author
Sorry, I had to go through all of the commits, and decided to make extensive (although admittedly only cosmetic) changes. The most important difference is that I integrated @vdye's fix for the Scalar enlistment discovery. I also decided to squash a couple of commits that could be perceived as "oops, let's correct this" fixups to earlier commits in the topic branch. In other words, the shape is sufficiently different enough to merit a re-review... |
dscho
force-pushed
the
scalar-the-beginning
branch
from
4f609b2 to
6455b18
Compare
Member
Author
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): |
|
User |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -0,0 +1,730 @@ | |||
| /* | |||
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Mon, Aug 30, 2021 at 5:35 PM Johannes Schindelin via GitGitGadget
<[email protected]> wrote:
> After a Scalar upgrade, it can come in really handy if there is an easy
> way to reconfigure all Scalar enlistments. This new option offers this
> functionality.
>
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
> diff --git a/contrib/scalar/scalar.txt b/contrib/scalar/scalar.txt
> @@ -121,6 +121,10 @@ After a Scalar upgrade, or when the configuration of a Scalar enlistment
> +With the `--all` option, all enlistments currently registered with Scalar
> +will be reconfigured. This option is meant to to be run every time Scalar
> +was upgraded.
s/was/is/
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Eric,
On Tue, 31 Aug 2021, Eric Sunshine wrote:
> On Mon, Aug 30, 2021 at 5:35 PM Johannes Schindelin via GitGitGadget
> <[email protected]> wrote:
> > After a Scalar upgrade, it can come in really handy if there is an easy
> > way to reconfigure all Scalar enlistments. This new option offers this
> > functionality.
> >
> > Signed-off-by: Johannes Schindelin <[email protected]>
> > ---
> > diff --git a/contrib/scalar/scalar.txt b/contrib/scalar/scalar.txt
> > @@ -121,6 +121,10 @@ After a Scalar upgrade, or when the configuration of a Scalar enlistment
> > +With the `--all` option, all enlistments currently registered with Scalar
> > +will be reconfigured. This option is meant to to be run every time Scalar
> > +was upgraded.
>
> s/was/is/
I wanted to convey a temporal order, so I changed it to "every time after
Scalar is upgraded". Okay?
Ciao,
Dscho
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Fri, Sep 3, 2021 at 11:23 AM Johannes Schindelin
<[email protected]> wrote:
> On Tue, 31 Aug 2021, Eric Sunshine wrote:
> > On Mon, Aug 30, 2021 at 5:35 PM Johannes Schindelin via GitGitGadget
> > > +With the `--all` option, all enlistments currently registered with Scalar
> > > +will be reconfigured. This option is meant to to be run every time Scalar
> > > +was upgraded.
> >
> > s/was/is/
>
> I wanted to convey a temporal order, so I changed it to "every time after
> Scalar is upgraded". Okay?
I think I understood the intent of the original, but it causes a
grammatical hiccup. Your revised version can work, although I might
write it this way:
This option is meant to be run each time Scalar is upgraded.
However, perhaps that is too ambiguous and some users may think that
the process of upgrading Scalar will automatically run this command,
and you'd like to make it clear that it is the user's responsibility.
So, perhaps:
Use this option after each Scalar upgrade.
or something.
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Eric,
On Fri, 3 Sep 2021, Eric Sunshine wrote:
> On Fri, Sep 3, 2021 at 11:23 AM Johannes Schindelin
> <[email protected]> wrote:
> > On Tue, 31 Aug 2021, Eric Sunshine wrote:
> > > On Mon, Aug 30, 2021 at 5:35 PM Johannes Schindelin via GitGitGadget
> > > > +With the `--all` option, all enlistments currently registered with Scalar
> > > > +will be reconfigured. This option is meant to to be run every time Scalar
> > > > +was upgraded.
> > >
> > > s/was/is/
> >
> > I wanted to convey a temporal order, so I changed it to "every time after
> > Scalar is upgraded". Okay?
>
> I think I understood the intent of the original, but it causes a
> grammatical hiccup. Your revised version can work, although I might
> write it this way:
>
> This option is meant to be run each time Scalar is upgraded.
>
> However, perhaps that is too ambiguous and some users may think that
> the process of upgrading Scalar will automatically run this command,
> and you'd like to make it clear that it is the user's responsibility.
> So, perhaps:
>
> Use this option after each Scalar upgrade.
>
> or something.
I like the last one best, too.
Thank you,
Dscho
|
User |
| @@ -0,0 +1,824 @@ | |||
| /* | |||
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Mon, Aug 30, 2021 at 5:35 PM Johannes Schindelin via GitGitGadget
<[email protected]> wrote:
> The .NET version of Scalar has a `version` command. This was necessary
> because it was versioned independently of Git.
>
> Since Scalar is now tightly coupled with Git, it does not make sense for
> them to show different versions. Therefore, it shows the same output as
> `git versions`. For backwards-compatibility with the .NET version,
s/versions/version/
> `scalar version` prints to `stderr`, though (`git version` prints to
> `stdout` instead).
>
> Signed-off-by: Johannes Schindelin <[email protected]>
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Eric,
On Tue, 31 Aug 2021, Eric Sunshine wrote:
> On Mon, Aug 30, 2021 at 5:35 PM Johannes Schindelin via GitGitGadget
> <[email protected]> wrote:
> > The .NET version of Scalar has a `version` command. This was necessary
> > because it was versioned independently of Git.
> >
> > Since Scalar is now tightly coupled with Git, it does not make sense for
> > them to show different versions. Therefore, it shows the same output as
> > `git versions`. For backwards-compatibility with the .NET version,
>
> s/versions/version/
Thank you!
Dscho
>
> > `scalar version` prints to `stderr`, though (`git version` prints to
> > `stdout` instead).
> >
> > Signed-off-by: Johannes Schindelin <[email protected]>
>
| @@ -0,0 +1,292 @@ | |||
| /* | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Mon, Aug 30 2021, Derrick Stolee via GitGitGadget wrote:
> [...]
> +#ifndef WIN32
> + { "core.untrackedCache", "true" },
> +#else
> + /*
> + * Unfortunately, Scalar's Functional Tests demonstrated
> + * that the untracked cache feature is unreliable on Windows
> + * (which is a bummer because that platform would benefit the
> + * most from it). For some reason, freshly created files seem
> + * not to update the directory's `lastModified` time
> + * immediately, but the untracked cache would need to rely on
> + * that.
> + *
> + * Therefore, with a sad heart, we disable this very useful
> + * feature on Windows.
> + */
> + { "core.untrackedCache", "false" },
> +#endif
> [...]
Ok, but why the need to set it to "false" explicitly? Does it need to be
so opinionated as to overwrite existing user-set config in these cases?
> + { "core.bare", "false" },
Shouldn't this be set by "git init" already?
> [...]
> + { "core.logAllRefUpdates", "true" },
An opinionated thing unrelated to performance?
> [...]
> + { "feature.manyFiles", "false" },
> + { "feature.experimental", "false" },
Ditto the question about the need to set this, these are false by
default, right?
> [...]
> + if (git_config_get_string(config[i].key, &value)) {
> + trace2_data_string("scalar", the_repository, config[i].key, "created");
> + if (git_config_set_gently(config[i].key,
> + config[i].value) < 0)
> + return error(_("could not configure %s=%s"),
> + config[i].key, config[i].value);
> + } else {
> + trace2_data_string("scalar", the_repository, config[i].key, "exists");
> + free(value);
> + }
The commit message doesn't discuss these trace2 additions, these in
particular seem like they might be useful, but better done as as some
more general trace2 intergration in config.c, i.e. if the functions
being called here did the same logging on config set/get.
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 8/31/2021 4:11 AM, Ævar Arnfjörð Bjarmason wrote:
>
> On Mon, Aug 30 2021, Derrick Stolee via GitGitGadget wrote:
>
>> [...]
>> +#ifndef WIN32
>> + { "core.untrackedCache", "true" },
>> +#else
>> + /*
>> + * Unfortunately, Scalar's Functional Tests demonstrated
>> + * that the untracked cache feature is unreliable on Windows
>> + * (which is a bummer because that platform would benefit the
>> + * most from it). For some reason, freshly created files seem
>> + * not to update the directory's `lastModified` time
>> + * immediately, but the untracked cache would need to rely on
>> + * that.
>> + *
>> + * Therefore, with a sad heart, we disable this very useful
>> + * feature on Windows.
>> + */
>> + { "core.untrackedCache", "false" },
>> +#endif
>> [...]
>
> Ok, but why the need to set it to "false" explicitly? Does it need to be
> so opinionated as to overwrite existing user-set config in these cases?
Users can overwrite this local config value, but this is placed to avoid
a global config value from applying specifically within Scalar-created
repos.
>> + { "core.bare", "false" },
>
> Shouldn't this be set by "git init" already?
This one is probably a bit _too_ defensive. It can be removed.
>> [...]
>> + { "core.logAllRefUpdates", "true" },
>
> An opinionated thing unrelated to performance?
It's an opinionated thing related to supporting monorepo users. It helps
us diagnose issues they have by recreating a sequence of events.
>> [...]
>> + { "feature.manyFiles", "false" },
>> + { "feature.experimental", "false" },
>
> Ditto the question about the need to set this, these are false by
> default, right?
But if a user has them on globally, then we don't want them to apply
locally (in favor of the settings that we set explicitly).
>> [...]
>> + if (git_config_get_string(config[i].key, &value)) {
>> + trace2_data_string("scalar", the_repository, config[i].key, "created");
>> + if (git_config_set_gently(config[i].key,
>> + config[i].value) < 0)
>> + return error(_("could not configure %s=%s"),
>> + config[i].key, config[i].value);
>> + } else {
>> + trace2_data_string("scalar", the_repository, config[i].key, "exists");
>> + free(value);
>> + }
>
> The commit message doesn't discuss these trace2 additions, these in
> particular seem like they might be useful, but better done as as some
> more general trace2 intergration in config.c, i.e. if the functions
> being called here did the same logging on config set/get.
If we want to do such a tracing change within git_config_set*(), then
that would be an appropriate replacement. The biggest reason to include
them here is to trace that an existing value already exists, for the
case of running 'scalar reconfigure' during an upgrade. That part
doesn't make much sense to put into config.c.
Thanks,
-Stolee
|
User |
| @@ -0,0 +1,57 @@ | |||
| QUIET_SUBDIR0 = +$(MAKE) -C # space to separate -C and subdir | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Mon, Aug 30 2021, Johannes Schindelin via GitGitGadget wrote:
> To test the Scalar command, create a test script in contrib/scalar/t
> that is executed as `make -C contrib/scalar test`. Since Scalar has no
> meaningful capabilities yet, the only test is rather simple. We will add
> more tests in subsequent commits that introduce corresponding, new
> functionality.
As a comment on 01..03/15: I'd really prefer if we stop using this
pattern of sub-Makefile, the dependencies are a pain to manage, and we
end up copy/pasting large sets of functionality.
That would mean just adding the build of this command to the top-level
Makefile behind some "CONTRIB_SCALAR" flag or whatever, but I find that
much cleaner than....
> @@ -21,7 +22,7 @@ include ../../config.mak.uname
> TARGETS = scalar$(X) scalar.o
> GITLIBS = ../../common-main.o ../../libgit.a ../../xdiff/lib.a
>
> -all: scalar$X
> +all: scalar$X ../../bin-wrappers/scalar
>
> [...]
> +../../bin-wrappers/scalar: ../../wrap-for-bin.sh Makefile
> [...]
> scalar.html: | scalar.1 # prevent them from trying to build `doc.dep` in parallel
...things like this, which refer to assets built by other Makefiles, and
need to plaster over the dependency issues...
> +++ b/contrib/scalar/t/Makefile
> @@ -0,0 +1,78 @@
> +# Run scalar tests
> +#
> +# Copyright (c) 2005,2021 Junio C Hamano, Johannes Schindelin
> +#
> +
> +-include ../../../config.mak.autogen
> +-include ../../../config.mak
> +
> +SHELL_PATH ?= $(SHELL)
> +PERL_PATH ?= /usr/bin/perl
> +RM ?= rm -f
> +PROVE ?= prove
> +DEFAULT_TEST_TARGET ?= test
> +TEST_LINT ?= test-lint
> +
> +ifdef TEST_OUTPUT_DIRECTORY
> +TEST_RESULTS_DIRECTORY = $(TEST_OUTPUT_DIRECTORY)/test-results
> +else
> +TEST_RESULTS_DIRECTORY = ../../../t/test-results
> +endif
> +
> +# Shell quote;
> +SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
> +PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
> +TEST_RESULTS_DIRECTORY_SQ = $(subst ','\'',$(TEST_RESULTS_DIRECTORY))
> +
> +T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))
> +
> +all: $(DEFAULT_TEST_TARGET)
> +
> +test: $(TEST_LINT)
> + $(MAKE) aggregate-results-and-cleanup
> +
> +prove: $(TEST_LINT)
> + @echo "*** prove ***"; GIT_CONFIG=.git/config $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
> + $(MAKE) clean-except-prove-cache
> +
> +$(T):
> + @echo "*** $@ ***"; GIT_CONFIG=.git/config '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
> +
> +clean-except-prove-cache:
> + $(RM) -r 'trash directory'.* '$(TEST_RESULTS_DIRECTORY_SQ)'
> + $(RM) -r valgrind/bin
> +
> +clean: clean-except-prove-cache
> + $(RM) .prove
> +
> +test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax
> +
> +test-lint-duplicates:
> + @dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
> + test -z "$$dups" || { \
> + echo >&2 "duplicate test numbers:" $$dups; exit 1; }
> +
> +test-lint-executable:
> + @bad=`for i in $(T); do test -x "$$i" || echo $$i; done` && \
> + test -z "$$bad" || { \
> + echo >&2 "non-executable tests:" $$bad; exit 1; }
> +
> +test-lint-shell-syntax:
> + @'$(PERL_PATH_SQ)' ../../../t/check-non-portable-shell.pl $(T)
> +
> +aggregate-results-and-cleanup: $(T)
> + $(MAKE) aggregate-results
> + $(MAKE) clean
> +
> +aggregate-results:
> + for f in '$(TEST_RESULTS_DIRECTORY_SQ)'/t*-*.counts; do \
> + echo "$$f"; \
> + done | '$(SHELL_PATH_SQ)' ../../../t/aggregate-results.sh
> +
> +valgrind:
> + $(MAKE) GIT_TEST_OPTS="$(GIT_TEST_OPTS) --valgrind"
> +
> +test-results:
> + mkdir -p test-results
> +
> +.PHONY: $(T) aggregate-results clean valgrind
...and this entire copy/pasting & adjusting of t/Makefile.
| @@ -0,0 +1,583 @@ | |||
| /* | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Mon, Aug 30 2021, Johannes Schindelin via GitGitGadget wrote:
> This implements Scalar's opinionated `clone` command: it tries to use a
> partial clone and sets up a sparse checkout by default. In contrast to
> `git clone`, `scalar clone` sets up the worktree in the `src/`
> subdirectory, to encourage a separation between the source files and the
> build output (which helps Git tremendously because it avoids untracked
> files that have to be specifically ignored when refreshing the index).
Perhaps nobody else wondered this while reading this, but I thought this
might be some sparse/worktree magic where cloning into "foo" would have
"foo/.git", but the worktree was somehow magically mapped at foo/src/".
But no, it just takes your "scalar clone <url> foo" and translates it to
"foo/src", so you'll get a directory at "foo".
> Note: We intentionally use a slightly wasteful `set_config()` function
> (which does not reuse a single `strbuf`, for example, though performance
> _really_ does not matter here) for convenience and readability.
FWIW I think the commit message could do without this, that part of the
code is obviously not performance sensitive at all. But maybe an
explicit note helps anyway...
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Tue, Aug 31, 2021 at 8:04 AM Ævar Arnfjörð Bjarmason
<[email protected]> wrote:
> On Mon, Aug 30 2021, Johannes Schindelin via GitGitGadget wrote:
> > Note: We intentionally use a slightly wasteful `set_config()` function
> > (which does not reuse a single `strbuf`, for example, though performance
> > _really_ does not matter here) for convenience and readability.
>
> FWIW I think the commit message could do without this, that part of the
> code is obviously not performance sensitive at all. But maybe an
> explicit note helps anyway...
FWIW, I also found this distracting; it takes the reader's attention
away from more important aspects of the patch. (But it alone is not
worth a re-roll; it was just a minor hiccup.)
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
This message is in MIME format. The first part should be readable text,
while the remaining parts are likely unreadable without MIME-aware tools.
--8323328-74038682-1630682474=:55
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Hi Eric,
On Tue, 31 Aug 2021, Eric Sunshine wrote:
> On Tue, Aug 31, 2021 at 8:04 AM =C3=86var Arnfj=C3=B6r=C3=B0 Bjarmason
> <[email protected]> wrote:
> > On Mon, Aug 30 2021, Johannes Schindelin via GitGitGadget wrote:
> > > Note: We intentionally use a slightly wasteful `set_config()` functi=
on
> > > (which does not reuse a single `strbuf`, for example, though perform=
ance
> > > _really_ does not matter here) for convenience and readability.
> >
> > FWIW I think the commit message could do without this, that part of th=
e
> > code is obviously not performance sensitive at all. But maybe an
> > explicit note helps anyway...
>
> FWIW, I also found this distracting; it takes the reader's attention
> away from more important aspects of the patch. (But it alone is not
> worth a re-roll; it was just a minor hiccup.)
Since I reworked the remote default branch parsing anyway, I removed this
paragraph from the commit message.
Ciao,
Dscho
--8323328-74038682-1630682474=:55--
| @@ -0,0 +1,652 @@ | |||
| /* | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Mon, Aug 30 2021, Derrick Stolee via GitGitGadget wrote:
> + const char *usagestr[] = { NULL, NULL };
Missing usage strings?
> + if (argc == 0)
Style nit (per style guide): s/argc == 0/!argc/g.
> + if (!strcmp("all", argv[0]))
> + i = -1;
Style nit (per style guide): missing braces here.
(Just noting the style nits once, but more in this patch, and presumably
the rest of the series...)
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
This message is in MIME format. The first part should be readable text,
while the remaining parts are likely unreadable without MIME-aware tools.
--8323328-1090042449-1630684217=:55
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Hi =C3=86var,
On Tue, 31 Aug 2021, =C3=86var Arnfj=C3=B6r=C3=B0 Bjarmason wrote:
> On Mon, Aug 30 2021, Derrick Stolee via GitGitGadget wrote:
>
> > + const char *usagestr[] =3D { NULL, NULL };
>
> Missing usage strings?
This command will show a generated usage, i.e. a non-static string. It
therefore cannot be specified here already. See the `strbuf_*()` calls
populating `buf` and the `usagestr[0] =3D buf.buf;` assignment.
> > + if (argc =3D=3D 0)
>
> Style nit (per style guide): s/argc =3D=3D 0/!argc/g.
It is true that we often do this, but in this instance it would be
misleading: `argc` is a counter, not a Boolean.
> > + if (!strcmp("all", argv[0]))
> > + i =3D -1;
>
> Style nit (per style guide): missing braces here.
The style guide specifically allows my preference to leave single-line
blocks without curlies.
Ciao,
Johannes
--8323328-1090042449-1630684217=:55--
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Johannes Schindelin <[email protected]> writes:
> Hi Ævar,
>
> On Tue, 31 Aug 2021, Ævar Arnfjörð Bjarmason wrote:
>
>> On Mon, Aug 30 2021, Derrick Stolee via GitGitGadget wrote:
>>
>> > + const char *usagestr[] = { NULL, NULL };
>>
>> Missing usage strings?
>
> This command will show a generated usage, i.e. a non-static string. It
> therefore cannot be specified here already. See the `strbuf_*()` calls
> populating `buf` and the `usagestr[0] = buf.buf;` assignment.
>
>> > + if (argc == 0)
>>
>> Style nit (per style guide): s/argc == 0/!argc/g.
>
> It is true that we often do this, but in this instance it would be
> misleading: `argc` is a counter, not a Boolean.
That argument could be a plausible excuse to deviate from the style
if it were
if (argc == 0)
do no args case;
else if (argc == 1)
do one arg case;
else if (argc == 2)
do two args case;
...
Replacing the first one with "if (!argc)" may make it less readable.
But I do not think the reasoning applies here
if (argc == 0)
do a thing that applies only to no args case;
without "else". This is talking about "do we have any argument? Yes
or no?" Boolean here.
>> > + if (!strcmp("all", argv[0]))
>> > + i = -1;
>>
>> Style nit (per style guide): missing braces here.
>
> The style guide specifically allows my preference to leave single-line
> blocks without curlies.
Actually, the exception goes the other way, no?
We generally want to avoid such an unnecessary braces around a
single statement block. But when we have an else clause that has a
block with multiple statements (hence braces are required), as an
exception, the guide asks you to write braces around the body of the
if side for consistency.
When you only have just a couple of lines on the "else {}" side, I
do not think it matters too much either way for readability, though.
I cannot see the "else" side in the above clause, but IIRC it wasn't
just a few lines, was it?
Thanks.
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
This message is in MIME format. The first part should be readable text,
while the remaining parts are likely unreadable without MIME-aware tools.
--8323328-1395184817-1631128285=:55
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Hi Junio,
On Fri, 3 Sep 2021, Junio C Hamano wrote:
> Johannes Schindelin <[email protected]> writes:
>
> > Hi =C3=86var,
> >
> > On Tue, 31 Aug 2021, =C3=86var Arnfj=C3=B6r=C3=B0 Bjarmason wrote:
> >
> >> On Mon, Aug 30 2021, Derrick Stolee via GitGitGadget wrote:
> >>
> >> > + const char *usagestr[] =3D { NULL, NULL };
> >>
> >> Missing usage strings?
> >
> > This command will show a generated usage, i.e. a non-static string. It
> > therefore cannot be specified here already. See the `strbuf_*()` calls
> > populating `buf` and the `usagestr[0] =3D buf.buf;` assignment.
> >
> >> > + if (argc =3D=3D 0)
> >>
> >> Style nit (per style guide): s/argc =3D=3D 0/!argc/g.
> >
> > It is true that we often do this, but in this instance it would be
> > misleading: `argc` is a counter, not a Boolean.
>
> That argument could be a plausible excuse to deviate from the style
> if it were
>
> if (argc =3D=3D 0)
> do no args case;
> else if (argc =3D=3D 1)
> do one arg case;
> else if (argc =3D=3D 2)
> do two args case;
> ...
>
> Replacing the first one with "if (!argc)" may make it less readable.
>
> But I do not think the reasoning applies here
>
> if (argc =3D=3D 0)
> do a thing that applies only to no args case;
>
> without "else". This is talking about "do we have any argument? Yes
> or no?" Boolean here.
Well, I offer a differing opinion. But you're right, we are at least
consistent in Git's source code in using `!i` where other projects would
use `i =3D=3D 0`, and consistency is definitely something I'd like to see =
more
in Git, not less.
So I changed it as you suggested.
>
> >> > + if (!strcmp("all", argv[0]))
> >> > + i =3D -1;
> >>
> >> Style nit (per style guide): missing braces here.
> >
> > The style guide specifically allows my preference to leave single-line
> > blocks without curlies.
>
> Actually, the exception goes the other way, no?
>
> We generally want to avoid such an unnecessary braces around a
> single statement block. But when we have an else clause that has a
> block with multiple statements (hence braces are required), as an
> exception, the guide asks you to write braces around the body of the
> if side for consistency.
You're right. I am somehow still using the previous style where we
_required_ single-line blocks _not_ to have curly brackets (see e.g.
aa1c48df817 ([PATCH] ls-tree enhancements, 2005-04-15), the `else` part of
the added `if (! eltbuf)` block).
>
> When you only have just a couple of lines on the "else {}" side, I
> do not think it matters too much either way for readability, though.
> I cannot see the "else" side in the above clause, but IIRC it wasn't
> just a few lines, was it?
It depends what you count as "just a few lines". There are seven lines
enclosed within the curly brackets of the `else` block.
But as much as I enjoy thorough reviews of the Scalar code, I am failing
at getting excited about code style discussions, therefore I simply went
with your suggestion to enclose even the single-line block in curly
brackets.
Thanks,
Dscho
--8323328-1395184817-1631128285=:55--
| @@ -0,0 +1,675 @@ | |||
| /* | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Mon, Aug 30 2021, Johannes Schindelin via GitGitGadget wrote:
> This comes in handy during Scalar upgrades, or when config settings were
> messed up by mistake.
> [...]
> const char *key;
> const char *value;
> + int overwrite_on_reconfigure;
If you make this a "keep_on_reconfigure", then ...
> } config[] = {
> - { "am.keepCR", "true" },
> - { "core.FSCache", "true" },
> - { "core.multiPackIndex", "true" },
> - { "core.preloadIndex", "true" },
> + /* Required */
> + { "am.keepCR", "true", 1 },
> + { "core.FSCache", "true", 1 },
> + { "core.multiPackIndex", "true", 1 },
> + { "core.preloadIndex", "true", 1 },
You won't need the churn/boilerplate of adding "1" to everything here,
but can just change the initial patch to use designated initializers.
That along with a throwaway macro like:
#define SCALAR_CFG_TRUE(k) (.key = k, .value = "true")
#define SCALAR_CFG_FALSE(k) (.key = k, .value = "false")
Might (or might not) make this even easier to eyeball...
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
This message is in MIME format. The first part should be readable text,
while the remaining parts are likely unreadable without MIME-aware tools.
--8323328-1470407212-1630684427=:55
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Hi =C3=86var,
On Tue, 31 Aug 2021, =C3=86var Arnfj=C3=B6r=C3=B0 Bjarmason wrote:
>
> On Mon, Aug 30 2021, Johannes Schindelin via GitGitGadget wrote:
>
> > This comes in handy during Scalar upgrades, or when config settings we=
re
> > messed up by mistake.
>
> > [...]
> > const char *key;
> > const char *value;
> > + int overwrite_on_reconfigure;
>
> If you make this a "keep_on_reconfigure", then ...
I do not think that this would be a better name, or that renaming this
field would do anything except cause more work for me.
>
> > } config[] =3D {
> > - { "am.keepCR", "true" },
> > - { "core.FSCache", "true" },
> > - { "core.multiPackIndex", "true" },
> > - { "core.preloadIndex", "true" },
> > + /* Required */
> > + { "am.keepCR", "true", 1 },
> > + { "core.FSCache", "true", 1 },
> > + { "core.multiPackIndex", "true", 1 },
> > + { "core.preloadIndex", "true", 1 },
>
> You won't need the churn/boilerplate of adding "1" to everything here,
> but can just change the initial patch to use designated initializers.
>
> That along with a throwaway macro like:
>
> #define SCALAR_CFG_TRUE(k) (.key =3D k, .value =3D "true")
> #define SCALAR_CFG_FALSE(k) (.key =3D k, .value =3D "false")
>
> Might (or might not) make this even easier to eyeball...
To me, it makes things less readable. There is an entire section with the
header `/* Optional */` below, and I want this list to stay as readable as
it is now.
Ciao,
Dscho
--8323328-1470407212-1630684427=:55--
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Fri, Sep 03 2021, Johannes Schindelin wrote:
> Hi Ævar,
>
> On Tue, 31 Aug 2021, Ævar Arnfjörð Bjarmason wrote:
>
>>
>> On Mon, Aug 30 2021, Johannes Schindelin via GitGitGadget wrote:
>>
>> > This comes in handy during Scalar upgrades, or when config settings were
>> > messed up by mistake.
>>
>> > [...]
>> > const char *key;
>> > const char *value;
>> > + int overwrite_on_reconfigure;
>>
>> If you make this a "keep_on_reconfigure", then ...
>
> I do not think that this would be a better name, or that renaming this
> field would do anything except cause more work for me.
It would also result in more readable code, i.e. why add boilerplate ",
1" to a boolean field in this case if every single setting is set to
"1"? Doesn't it make more sense to invert the variable name & save on
the verbosity?
>>
>> > } config[] = {
>> > - { "am.keepCR", "true" },
>> > - { "core.FSCache", "true" },
>> > - { "core.multiPackIndex", "true" },
>> > - { "core.preloadIndex", "true" },
>> > + /* Required */
>> > + { "am.keepCR", "true", 1 },
>> > + { "core.FSCache", "true", 1 },
>> > + { "core.multiPackIndex", "true", 1 },
>> > + { "core.preloadIndex", "true", 1 },
>>
>> You won't need the churn/boilerplate of adding "1" to everything here,
>> but can just change the initial patch to use designated initializers.
>>
>> That along with a throwaway macro like:
>>
>> #define SCALAR_CFG_TRUE(k) (.key = k, .value = "true")
>> #define SCALAR_CFG_FALSE(k) (.key = k, .value = "false")
>>
>> Might (or might not) make this even easier to eyeball...
>
> To me, it makes things less readable. There is an entire section with the
> header `/* Optional */` below, and I want this list to stay as readable as
> it is now.
Yeah, I think those macros are probably less readable too. I should have
phrased that as a "one could even...", but just the smaller change of
avoiding the ", 1" everywhere seems worthwhile.
| @@ -0,0 +1,844 @@ | |||
| /* | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Mon, Aug 30 2021, Johannes Schindelin via GitGitGadget wrote:
> The `git` executable has these two very useful options:
>
> -C <directory>:
> switch to the specified directory before performing any actions
>
> -c <key>=<value>:
> temporarily configure this setting for the duration of the
> specified scalar subcommand
>
> With this commit, we teach the `scalar` executable the same trick.
> [...]
> + while (argc > 1 && *argv[1] == '-') {
> + if (!strcmp(argv[1], "-C")) {
> + if (argc < 3)
> + die(_("-C requires a <directory>"));
> + if (chdir(argv[2]) < 0)
> + die_errno(_("could not change to '%s'"),
> + argv[2]);
> + argc -= 2;
> + argv += 2;
> + } else if (!strcmp(argv[1], "-c")) {
> + if (argc < 3)
> + die(_("-c requires a <key>=<value> argument"));
> + git_config_push_parameter(argv[2]);
> + argc -= 2;
> + argv += 2;
> + } else
> + break;
> + }
This along with my earlier comment about the Makefile copy/pasting makes
me wonder if an easier way to integrate this wouldn't be to refactor
git.c a bit to have it understand either "git" or "scalar", then instead
of "ls-tree" etc. as "git" the subcommands would become "built-ins".
Which would give us both "[git|scalar] [-c ...] <cmd>" for free, and
elimante the need for the inevetable future divergence of wanting -p,
-P, --exec-path etc. in both.
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 8/31/2021 4:32 AM, Ævar Arnfjörð Bjarmason wrote:
>
> On Mon, Aug 30 2021, Johannes Schindelin via GitGitGadget wrote:
>
>> The `git` executable has these two very useful options:
>>
>> -C <directory>:
>> switch to the specified directory before performing any actions
>>
>> -c <key>=<value>:
>> temporarily configure this setting for the duration of the
>> specified scalar subcommand
>>
>> With this commit, we teach the `scalar` executable the same trick.
>> [...]
>> + while (argc > 1 && *argv[1] == '-') {
>> + if (!strcmp(argv[1], "-C")) {
>> + if (argc < 3)
>> + die(_("-C requires a <directory>"));
>> + if (chdir(argv[2]) < 0)
>> + die_errno(_("could not change to '%s'"),
>> + argv[2]);
>> + argc -= 2;
>> + argv += 2;
>> + } else if (!strcmp(argv[1], "-c")) {
>> + if (argc < 3)
>> + die(_("-c requires a <key>=<value> argument"));
>> + git_config_push_parameter(argv[2]);
>> + argc -= 2;
>> + argv += 2;
>> + } else
>> + break;
>> + }
>
> This along with my earlier comment about the Makefile copy/pasting makes
> me wonder if an easier way to integrate this wouldn't be to refactor
> git.c a bit to have it understand either "git" or "scalar", then instead
> of "ls-tree" etc. as "git" the subcommands would become "built-ins".
>
> Which would give us both "[git|scalar] [-c ...] <cmd>" for free, and
> elimante the need for the inevetable future divergence of wanting -p,
> -P, --exec-path etc. in both.
Such a change would likely eliminate the ability to not include Scalar
when building the Git codebase, which we tried to avoid by keeping it
within contrib and have it be compiled via an opt-in flag.
If we want to talk about integrating Scalar into Git in a deeper way,
then that is an interesting discussion to have, but it lives at a much
higher level than Makefile details.
The questions we are really looking to answer in this RFC are:
1. Will the Git project accept Scalar into its codebase?
2. What is the best place for Scalar to live in the Git codebase?
We erred on the side of keeping Scalar as optional as possible. If
the community is more interested in a deeper integration, then that
could be an interesting direction.
In my opinion, I think the current tactic is safest. We could always
decide on a deeper integration later by moving the code around. It
seems harder to do the reverse.
Thanks,
-Stolee
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Tue, Aug 31 2021, Derrick Stolee wrote:
> On 8/31/2021 4:32 AM, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Mon, Aug 30 2021, Johannes Schindelin via GitGitGadget wrote:
>>
>>> The `git` executable has these two very useful options:
>>>
>>> -C <directory>:
>>> switch to the specified directory before performing any actions
>>>
>>> -c <key>=<value>:
>>> temporarily configure this setting for the duration of the
>>> specified scalar subcommand
>>>
>>> With this commit, we teach the `scalar` executable the same trick.
>>> [...]
>>> + while (argc > 1 && *argv[1] == '-') {
>>> + if (!strcmp(argv[1], "-C")) {
>>> + if (argc < 3)
>>> + die(_("-C requires a <directory>"));
>>> + if (chdir(argv[2]) < 0)
>>> + die_errno(_("could not change to '%s'"),
>>> + argv[2]);
>>> + argc -= 2;
>>> + argv += 2;
>>> + } else if (!strcmp(argv[1], "-c")) {
>>> + if (argc < 3)
>>> + die(_("-c requires a <key>=<value> argument"));
>>> + git_config_push_parameter(argv[2]);
>>> + argc -= 2;
>>> + argv += 2;
>>> + } else
>>> + break;
>>> + }
>>
>> This along with my earlier comment about the Makefile copy/pasting makes
>> me wonder if an easier way to integrate this wouldn't be to refactor
>> git.c a bit to have it understand either "git" or "scalar", then instead
>> of "ls-tree" etc. as "git" the subcommands would become "built-ins".
>>
>> Which would give us both "[git|scalar] [-c ...] <cmd>" for free, and
>> elimante the need for the inevetable future divergence of wanting -p,
>> -P, --exec-path etc. in both.
>
> Such a change would likely eliminate the ability to not include Scalar
> when building the Git codebase, which we tried to avoid by keeping it
> within contrib and have it be compiled via an opt-in flag.
I mean to still have it behind a flag, but to handle it similar to how
we handle NO_CURL, EXCLUDED_PROGRAMS and the like, i.e. not requiring
parallel maintenance of copy/pasted Makefile logic in contrib/.
> If we want to talk about integrating Scalar into Git in a deeper way,
> then that is an interesting discussion to have, but it lives at a much
> higher level than Makefile details.
To be clear I'm proposing no change at all in term of what happens when
you run "make install", just commenting on the implementation details of
how we arrange for things to be built and configured before that step.
I realize that this is following some prior art of
e.g. contrib/subtree/Makefile, but IMNSHO that approach is a historical
mistake we should be backing out of. There was some recent discussion of
this here:
https://lore.kernel.org/git/[email protected]/
E.g. now we have some painful management of the depencency graph between
/Makefile and Documentation/Makefile requiring fixes like 56550ea7180
(Makefile: add missing dependencies of 'config-list.h', 2021-04-08),
adding yet another Makefile into the mix which (to take one example)
depends on doc.dep, which in turn depends on ...; It's all a bunch of
needless complexity we can avoid.
> The questions we are really looking to answer in this RFC are:
>
> 1. Will the Git project accept Scalar into its codebase?
>
> 2. What is the best place for Scalar to live in the Git codebase?
>
> We erred on the side of keeping Scalar as optional as possible. If
> the community is more interested in a deeper integration, then that
> could be an interesting direction.
Indeed, to be clear I realize I'm entirely punting on the real questions
you're interested in. I just gave this an initial cursory skimming for
now, I have not formed an informed opinion on your #1, but just a little
bit of #2.
My initial reaction to #1 without having looked into it deeply is some
combination of "sure, why not?", and that the people/group contributing
major scalability work to git.git should be given the benefit of the
doubt. Maybe we won't keep "scalar" long-term, or change its UI etc.,
all of that can be handled in some carefully worded documentation
somewhere.
Of course all these suggestions I'm making about Makefile arrangement
are rather pointless if there isn't consensus to get past the hurdle of
your #1.
> In my opinion, I think the current tactic is safest. We could always
> decide on a deeper integration later by moving the code around. It
> seems harder to do the reverse.
I think "deeper integration" is the reverse of what you think it is.
I.e. if I'm patching or maintaining part of the Makefile logic to it's
deeper (or perhaps "gnarlier" is the righ word?) integration to need to
duplicate that work in two places, or always take into account that some
not-built-by-default-but-quite-common command's *.txt docs and *.sh
tests live in some unusual place for the purposes of CI, lint, tooling
etc.
In other words, it's a question of how much net complexity is being
added to the (build) system. That complexity doesn't automatically
reduce just because some files live in another directory, sometimes
that's an increase in complexity.
Whereas just conditionally adding it to some list in the top-level
Makefile (or Documentation/Makefile) is relatively maintenance-free, and
to our users / packagers the result should be the same or near enough.
It won't matter to them if building the optional thing is another "make"
command or just a flag to the existing "make" command.
|
On the Git mailing list, Elijah Newren wrote (reply to this): |
|
User |
| @@ -0,0 +1,292 @@ | |||
| /* | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Derrick Stolee via GitGitGadget" <[email protected]> writes:
> +static void setup_enlistment_directory(int argc, const char **argv,
> + const char * const *usagestr,
> + const struct option *options,
> + struct strbuf *enlistment_root)
> +{
> + struct strbuf path = STRBUF_INIT;
> + char *root;
> + int enlistment_found = 0;
> +
> + if (startup_info->have_repository)
> + BUG("gitdir already set up?!?");
> +
> + if (argc > 1)
> + usage_with_options(usagestr, options);
> +
> + /* find the worktree, determine its corresponding root */
> + if (argc == 1)
> + strbuf_add_absolute_path(&path, argv[0]);
> + else if (strbuf_getcwd(&path) < 0)
> + die(_("need a working directory"));
> +
> + strbuf_trim_trailing_dir_sep(&path);
> + do {
> + const size_t len = path.len;
> +
> + /* check if currently in enlistment root with src/ workdir */
> + strbuf_addstr(&path, "/src/.git");
> + if (is_git_directory(path.buf)) {
> + strbuf_strip_suffix(&path, "/.git");
> +
> + if (enlistment_root)
> + strbuf_add(enlistment_root, path.buf, len);
> +
> + enlistment_found = 1;
> + break;
> + }
This special casing of "normally the top of the working tree is
enlisted, but if the repository is called src/, then we enslist
one level up" is a bit of eyesore because
(1) it is unclear why such a directory with 'src/' subdirectory is
so special, and
(2) it fails to serve those who has the same need but named their
source subdirectory differently (like 'source/').
"The design decisions we made are all part of being opinionated" can
all explain it away, but at least we should let the users know where
the opinionated choices scalar makes want to lead them to, and this
"src/" stuff needs a bit of clarification. Perhaps a documentation
will be added in later steps?
> + for (i = 0; config[i].key; i++) {
> + if (git_config_get_string(config[i].key, &value)) {
> + trace2_data_string("scalar", the_repository, config[i].key, "created");
> + if (git_config_set_gently(config[i].key,
> + config[i].value) < 0)
> + return error(_("could not configure %s=%s"),
> + config[i].key, config[i].value);
> + } else {
> + trace2_data_string("scalar", the_repository, config[i].key, "exists");
> + free(value);
> + }
I wonder if we should have a table of configuration variables and
their default values. The above code implements a skewed "we only
avoid overriding what is explicitly configured". A variable that
the user left unconfigured because the user found its default
satisfactory will be overridden, and if the value scalar wants to
use happens to be the default value, we leave an explicit
configuration to that default value in the resulting configuration
file.
But I think the above is the best we can do without such a central
registry of configuration variables.
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Junio,
On Wed, 1 Sep 2021, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <[email protected]> writes:
>
> > +static void setup_enlistment_directory(int argc, const char **argv,
> > + const char * const *usagestr,
> > + const struct option *options,
> > + struct strbuf *enlistment_root)
> > +{
> > + struct strbuf path = STRBUF_INIT;
> > + char *root;
> > + int enlistment_found = 0;
> > +
> > + if (startup_info->have_repository)
> > + BUG("gitdir already set up?!?");
> > +
> > + if (argc > 1)
> > + usage_with_options(usagestr, options);
> > +
> > + /* find the worktree, determine its corresponding root */
> > + if (argc == 1)
> > + strbuf_add_absolute_path(&path, argv[0]);
> > + else if (strbuf_getcwd(&path) < 0)
> > + die(_("need a working directory"));
> > +
> > + strbuf_trim_trailing_dir_sep(&path);
> > + do {
> > + const size_t len = path.len;
> > +
> > + /* check if currently in enlistment root with src/ workdir */
> > + strbuf_addstr(&path, "/src/.git");
> > + if (is_git_directory(path.buf)) {
> > + strbuf_strip_suffix(&path, "/.git");
> > +
> > + if (enlistment_root)
> > + strbuf_add(enlistment_root, path.buf, len);
> > +
> > + enlistment_found = 1;
> > + break;
> > + }
>
> This special casing of "normally the top of the working tree is
> enlisted, but if the repository is called src/, then we enslist
> one level up" is a bit of eyesore because
>
> (1) it is unclear why such a directory with 'src/' subdirectory is
> so special, and
>
> (2) it fails to serve those who has the same need but named their
> source subdirectory differently (like 'source/').
All true. I wish we had come up with a better way, or with a way to
override this via an option.
Unfortunately, we are now bound by the fact that there are already users
out there...
> "The design decisions we made are all part of being opinionated" can
> all explain it away, but at least we should let the users know where
> the opinionated choices scalar makes want to lead them to, and this
> "src/" stuff needs a bit of clarification. Perhaps a documentation
> will be added in later steps?
I had hoped that the initial blurb of the manual page was sufficient, but
you're right, the `register` subcommand is particular in that it allows to
force Scalar to consider the worktree to be identical to the Scalar
enlistment. I added this:
diff --git a/contrib/scalar/scalar.txt b/contrib/scalar/scalar.txt
index 1593da45eae..568987064b2 100644
--- a/contrib/scalar/scalar.txt
+++ b/contrib/scalar/scalar.txt
@@ -40,6 +40,10 @@ register [<enlistment>]::
and starts background maintenance. If `<enlistment>` is not provided,
then the enlistment associated with the current working directory is
registered.
++
+Note: when this subcommand is called in a worktree that is called `src/`, its
+parent directory is considered to be the Scalar enlistment. If the worktree is
+_not_ called `src/`, it itself will be considered to be the Scalar enlistment.
> > + for (i = 0; config[i].key; i++) {
> > + if (git_config_get_string(config[i].key, &value)) {
> > + trace2_data_string("scalar", the_repository, config[i].key, "created");
> > + if (git_config_set_gently(config[i].key,
> > + config[i].value) < 0)
> > + return error(_("could not configure %s=%s"),
> > + config[i].key, config[i].value);
> > + } else {
> > + trace2_data_string("scalar", the_repository, config[i].key, "exists");
> > + free(value);
> > + }
>
> I wonder if we should have a table of configuration variables and
> their default values. The above code implements a skewed "we only
> avoid overriding what is explicitly configured". A variable that
> the user left unconfigured because the user found its default
> satisfactory will be overridden, and if the value scalar wants to
> use happens to be the default value, we leave an explicit
> configuration to that default value in the resulting configuration
> file.
>
> But I think the above is the best we can do without such a central
> registry of configuration variables.
Even with such a central registry, there would still be the question
whether the user, by staying with the default, wanted Git (or in this
instance, Scalar) to keep using the old default. The intention is
unfortunately not clear just from setting the variable.
So I think this is the best we can do.
Ciao,
Dscho
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Johannes Schindelin <[email protected]> writes:
>> "The design decisions we made are all part of being opinionated" can
>> all explain it away, but at least we should let the users know where
>> the opinionated choices scalar makes want to lead them to, and this
>> "src/" stuff needs a bit of clarification. Perhaps a documentation
>> will be added in later steps?
>
> I had hoped that the initial blurb of the manual page was sufficient, but
> you're right, the `register` subcommand is particular in that it allows to
> force Scalar to consider the worktree to be identical to the Scalar
> enlistment. I added this:
Sorry, if it weren't clear that I was commenting on each step as I
read along without peeking later steps. I think I saw it was
written somewhere that this was to encourage use of read-only
directory that keeps the sources with build artifacts and crufts
created outside it (so forests of projects will not have the source
directories, each of which has its own .git/, next to each other---
instead we would have shell directories, each with its own src/ and
src/.git, next to each other). The additional documentation below
is a good thing to have handy when readers learn how to use
"register" (or more generally, what an "enlistment" is). As long as
the motivation behind that design is given somewhere (not necessarily
here) for readers to discover, I am OK with the design.
> diff --git a/contrib/scalar/scalar.txt b/contrib/scalar/scalar.txt
> index 1593da45eae..568987064b2 100644
> --- a/contrib/scalar/scalar.txt
> +++ b/contrib/scalar/scalar.txt
> @@ -40,6 +40,10 @@ register [<enlistment>]::
> and starts background maintenance. If `<enlistment>` is not provided,
> then the enlistment associated with the current working directory is
> registered.
> ++
> +Note: when this subcommand is called in a worktree that is called `src/`, its
> +parent directory is considered to be the Scalar enlistment. If the worktree is
> +_not_ called `src/`, it itself will be considered to be the Scalar enlistment.
Thanks.
|
On the Git mailing list, Jeff King wrote (reply to this): |
|
On the Git mailing list, Jeff King wrote (reply to this): |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
|
On the Git mailing list, Elijah Newren wrote (reply to this): |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): |
|
There was a status update in the "Cooking" section about the branch Add pieces from "scalar" to contrib/. Will merge to 'master'. source: <[email protected]> |
|
On the Git mailing list, Elijah Newren wrote (reply to this): |
|
On the Git mailing list, Bagas Sanjaya wrote (reply to this): |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Jeff King wrote (reply to this): |
|
On the Git mailing list, Jeff King wrote (reply to this): |
|
This patch series was integrated into seen via git@383d342. |
|
This patch series was integrated into next via git@6248603. |
|
This patch series was integrated into seen via git@ae4fb07. |
|
This patch series was integrated into seen via git@b46ea1e. |
|
There was a status update in the "Cooking" section about the branch Add pieces from "scalar" to contrib/. Will merge to 'master'. source: <[email protected]> |
|
This patch series was integrated into seen via git@62e83d4. |
|
This patch series was integrated into next via git@62e83d4. |
|
This patch series was integrated into master via git@62e83d4. |
|
Closed via 62e83d4. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
tl;dr: This series contributes the core part of the Scalar command to the Git project. This command provides a convenient way to clone/initialize very large repositories (think: monorepos).
Note: This patch series' focus is entirely on Scalar, on choosing sensible defaults and offering a delightful user experience around working with monorepos, and not about changing any existing paradigms for
contrib/(even if catching up on the mail thread is likely to give interested readers that false impression).Changes since v9:
Changes since v8:
merge.renamestotrue. This is now the case.ab/ci-updateswhich invalidates assumptions made by this patch series that would still hold true with v2.34.0 but are no longer valid inseenand would trigger CI build breakages.Changes since v7:
contrib/scalar/.README.md.Changes since v6:
contrib/scalar/README.md, containing the roadmap of what I have planned for Scalar.GIT_TEST_MAINT_SCHEDULERhas been adjusted to accommodate for a change in v2.32.0..v2.34.0.fetch.showForcedUpdates=false, which has been identified as helping with a performance issue in large repositories.-c <key>=<value>and-C <directory>was moved to its own add-on patch series: While it is obvious that those options are valuable to have, an open question is whether there are other "pre-command" options ingitthat would be useful, too, and I would like to postpone that discussion to that date.Changes since v5:
make -C contrib/scalar/Makefile.git ls-treeinvocation suggested in the manual forscalar clone.make -C contrib/scalar, then changing a source file oflibgit.aand then immediately invokingmake -C contrib/scalaragain will now implicitly rebuildlibgit.a.Changes since v4:
scalar deletenow refuses to delete anything if it was started from within the enlistment.scalar deletereleases any handles to the object store before deleting the enlistment.OBJECTSlist in theMakefilewill now include Scalar.scalar registernow supports secondary worktrees, in addition to the primary worktree.Changes since v3:
[RFC]prefix because I did not hear any objections against putting this intocontrib/.Changes since v2:
listcommand in the manual page , as suggested by Bagas.cmd_run().git reconfigure -awas improved.Changes since v1:
ls-remotewas made more readablescalar.txtnow consistently uses tabscore.bare = falsewhen registering with ScalarBackground
Microsoft invested a lot of effort into scaling Git to the needs of the Windows operating system source code. Based on the experience of the first approach, VFS for Git, the Scalar project was started. Scalar specifically has as its core goal to funnel all improvements into core Git.
The present
The Scalar project provides a completely functional non-virtual experience for monorepos. But why stop there. The Scalar project was designed to be a self-destructing vehicle to allow those key concepts to be moved into core Git itself for the benefit of all. For example, partial clone, sparse-checkout, and scheduled background maintenance have already been upstreamed and removed from Scalar proper. This patch series provides a C-based implementation of the final remaining portions of the Scalar command. This will make it easier for users to experiment with the Scalar command. It will also make it substantially easier to experiment with moving functionality from Scalar into core Git, while maintaining backwards-compatibility for existing Scalar users.
The C-based Scalar has been shipped to Scalar users, and can be tested by any interested reader: https://github.com/microsoft/git/releases/ (it offers a Git for Windows installer, a macOS package and an Ubuntu package, Scalar has been included since v2.33.0.vfs.0.0).
Next steps
Since there are existing Scalar users, I want to ensure backwards-compatibility with its existing command-line interface. Keeping that in mind, everything in this series is up for discussion.
I obviously believe that Scalar brings a huge benefit, and think that it would be ideal for all of Scalar's learnings to end up in
git clone/git init/git maintenanceeventually. It is also conceivable, however, that thescalarcommand could graduate to be a core part of Git at some stage in the future (such a decision would probably depend highly on users' feedback). See also the discussion about the architecture of Scalar, kicked off by Stolee.On top of this patch series, I have lined up a few more:
scalar diagnosecommand.scalaras part of a regular Git install (also teachinggit help scalarto find the Scalar documentationThese are included in my
vfs-with-scalarbranch thicket. On top of that, this branch thicket also includes patches I do not plan on upstreaming, mainly because they are too specific either to VFS for Git, or they support Azure Repos (which does not offer partial clones but speaks the GVFS protocol, which can be used to emulate partial clones).One other thing is very interesting about that
vfs-with-scalarbranch thicket: it contains a GitHub workflow which will run Scalar's quite extensive Functional Tests suite. This test suite is quite comprehensive and caught us a lot of bugs in the past, not only in the Scalar code, but also core Git.Epilogue
Now, to address some questions that I imagine every reader has who made it this far:
cc: Derrick Stolee [email protected]
cc: Eric Sunshine [email protected]
cc: Ævar Arnfjörð Bjarmason [email protected]
cc: Elijah Newren [email protected]
cc: Bagas Sanjaya [email protected]
cc: "Theodore Ts'o" [email protected]
cc: Matt Rogers [email protected]
cc: Jeff King [email protected]