| Message ID | pull.1780.git.1725545614416.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | scalar: add --no-tags option | expand |
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Derrick Stolee <stolee@gmail.com> > > Some large repositories use tags to track a huge list of release > versions. While this is choice is costly on the ref advertisement, it is > further wasteful for clients who do not need those tags. Allow clients > to optionally skip the tag advertisement. > > This behavior is similar to that of 'git clone --no-tags' implemented in > 0dab2468ee5 (clone: add a --no-tags option to clone without tags, > 2017-04-26), including the modification of the remote.origin.tagOpt > config value to include "--no-tags". > > One thing that is opposite of the 'git clone' implementation is that > this allows '--tags' as an assumed option, which can be naturally negated > with '--no-tags'. The clone command does not accept '--tags' but allows > "--no-no-tags" as the negation of its '--no-tags' option. Yuck. The loophole may be something we may want to close later, though (and replaced with a proper "--tags" support). > While testing this option, combine the test with the previously untested > '--no-src' option introduced in 4527db8ff8c (scalar: add --[no-]src > option, 2023-08-28). > > Signed-off-by: Derrick Stolee <stolee@gmail.com> > --- Makes sense. This is a tangent, but the "--[no-]tags" option in "git fetch" is misdesigned; the default is to auto-follow tags that would annotate objects that are being fetched, and "--no-tags" is a way to decline the auto-following. But "--tags" is to say that all tags must be fetched. There is no obvious way to say "I want the auto-following behaviour" (e.g., to override an earlier "--no-tags" or "--tags").
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Subject: Re: [PATCH] scalar: add --no-tags option
Micronit. Shouldn't we make it clear that this is about the clone
subcommand? E.g.
Subject: scalar: add --no-tags option to "scalar clone"
or something?
Hi Stolee, the patch makes sense to me. The commit message is delightfully thorough. I'll just offer minor suggestions below: On Thu, 5 Sep 2024, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <stolee@gmail.com> > > Some large repositories use tags to track a huge list of release > versions. While this is choice is costly on the ref advertisement, it is s/is \(choice is\)/\1/ > further wasteful for clients who do not need those tags. Allow clients > to optionally skip the tag advertisement. > > [...] > diff --git a/Documentation/scalar.txt b/Documentation/scalar.txt > index 361f51a6473..507ed2ae669 100644 > --- a/Documentation/scalar.txt > +++ b/Documentation/scalar.txt > @@ -86,6 +86,12 @@ cloning. If the HEAD at the remote did not point at any branch when > `<entlistment>/src` directory. Use `--no-src` to place the cloned > repository directly in the `<enlistment>` directory. > > +--[no-]tags:: > + By default, `scalar clone` will fetch the tag objects advertised by > + the remote and future `git fetch` commands will do the same. Use It might be helpful to mention that `git fetch --tags` can be used to override this when the need arises. > + `--no-tags` to avoid fetching tags in `scalar clone` and to configure > + the repository to avoid fetching tags in the future. > + > --[no-]full-clone:: > A sparse-checkout is initialized by default. This behavior can be > turned off via `--full-clone`. > diff --git a/scalar.c b/scalar.c > index 6166a8dd4c8..c6dd746b5b2 100644 > --- a/scalar.c > +++ b/scalar.c > [...] > @@ -513,7 +520,9 @@ static int cmd_clone(int argc, const char **argv) > > if ((res = run_git("fetch", "--quiet", > show_progress ? "--progress" : "--no-progress", > - "origin", NULL))) { > + "origin", > + (!tags ? "--no-tags" : NULL), I find double negatives challenging, and would prefer this instead: tags ? NULL : "--no-tags", > + NULL))) { > warning(_("partial clone failed; attempting full clone")); > > if (set_config("remote.origin.promisor") || > diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh > index a41b4fcc085..e8613990e13 100755 > --- a/t/t9210-scalar.sh > +++ b/t/t9210-scalar.sh > @@ -169,6 +169,24 @@ test_expect_success 'scalar clone' ' > ) > ' > > +test_expect_success 'scalar clone --no-... opts' ' > + # Note: redirect stderr always to avoid having a verbose test > + # run result in a difference in the --[no-]progress option. > + GIT_TRACE2_EVENT="$(pwd)/no-opt-trace" scalar clone \ > + --no-tags --no-src \ > + "file://$(pwd)" no-opts --single-branch 2>/dev/null && > + > + test_subcommand git fetch --quiet --no-progress \ > + origin --no-tags <no-opt-trace && > + ( > + cd no-opts && > + > + test_cmp_config --no-tags remote.origin.tagopt && > + git for-each-ref --format="%(refname)" refs/tags/ >tags && > + test_line_count = 0 tags > + ) For readability (and to avoid an unnecessary subshell), this could be written as: test_cmp_config -C no-opts --no-tags remote.origin.tagopt && git -C no-opts for-each-ref --format="%(refname)" refs/tags/ >tags && test_line_count = 0 tags > +' > + > test_expect_success 'scalar reconfigure' ' > git init one/src && > scalar register one && None of these suggestions, in isolation or in conjunction, seem big enough to me to warrant a new iteration; I just offer them here in case you want to iterate on the patch. If you want to keep the patch as-is, I am totally on board with that. Ciao, Johannes
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > None of these suggestions, in isolation or in conjunction, seem big enough > to me to warrant a new iteration; I just offer them here in case you want > to iterate on the patch. If you want to keep the patch as-is, I am totally > on board with that. Everything other than "prefer repeated use of -C instead of a single subshell in there" (which I think worsens readability, even though it may save one process), I agree with your suggestions. Thanks.
On 9/6/24 11:13 AM, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > >> None of these suggestions, in isolation or in conjunction, seem big enough >> to me to warrant a new iteration; I just offer them here in case you want >> to iterate on the patch. If you want to keep the patch as-is, I am totally >> on board with that. > > Everything other than "prefer repeated use of -C instead of a single > subshell in there" (which I think worsens readability, even though > it may save one process), I agree with your suggestions. Thanks, both, for your review comments. v2 is on the way with these suggestions. -Stolee
diff --git a/Documentation/scalar.txt b/Documentation/scalar.txt index 361f51a6473..507ed2ae669 100644 --- a/Documentation/scalar.txt +++ b/Documentation/scalar.txt @@ -86,6 +86,12 @@ cloning. If the HEAD at the remote did not point at any branch when `<entlistment>/src` directory. Use `--no-src` to place the cloned repository directly in the `<enlistment>` directory. +--[no-]tags:: + By default, `scalar clone` will fetch the tag objects advertised by + the remote and future `git fetch` commands will do the same. Use + `--no-tags` to avoid fetching tags in `scalar clone` and to configure + the repository to avoid fetching tags in the future. + --[no-]full-clone:: A sparse-checkout is initialized by default. This behavior can be turned off via `--full-clone`. diff --git a/scalar.c b/scalar.c index 6166a8dd4c8..c6dd746b5b2 100644 --- a/scalar.c +++ b/scalar.c @@ -410,7 +410,7 @@ static int cmd_clone(int argc, const char **argv) { const char *branch = NULL; int full_clone = 0, single_branch = 0, show_progress = isatty(2); - int src = 1; + int src = 1, tags = 1; struct option clone_options[] = { OPT_STRING('b', "branch", &branch, N_("<branch>"), N_("branch to checkout after clone")), @@ -421,11 +421,13 @@ static int cmd_clone(int argc, const char **argv) "be checked out")), OPT_BOOL(0, "src", &src, N_("create repository within 'src' directory")), + OPT_BOOL(0, "tags", &tags, + N_("specify if tags should be fetched during clone")), OPT_END(), }; const char * const clone_usage[] = { N_("scalar clone [--single-branch] [--branch <main-branch>] [--full-clone]\n" - "\t[--[no-]src] <url> [<enlistment>]"), + "\t[--[no-]src] [--[no-]tags] <url> [<enlistment>]"), NULL }; const char *url; @@ -504,6 +506,11 @@ static int cmd_clone(int argc, const char **argv) goto cleanup; } + if (!tags && set_config("remote.origin.tagOpt=--no-tags")) { + res = error(_("could not disable tags in '%s'"), dir); + goto cleanup; + } + if (!full_clone && (res = run_git("sparse-checkout", "init", "--cone", NULL))) goto cleanup; @@ -513,7 +520,9 @@ static int cmd_clone(int argc, const char **argv) if ((res = run_git("fetch", "--quiet", show_progress ? "--progress" : "--no-progress", - "origin", NULL))) { + "origin", + (!tags ? "--no-tags" : NULL), + NULL))) { warning(_("partial clone failed; attempting full clone")); if (set_config("remote.origin.promisor") || diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh index a41b4fcc085..e8613990e13 100755 --- a/t/t9210-scalar.sh +++ b/t/t9210-scalar.sh @@ -169,6 +169,24 @@ test_expect_success 'scalar clone' ' ) ' +test_expect_success 'scalar clone --no-... opts' ' + # Note: redirect stderr always to avoid having a verbose test + # run result in a difference in the --[no-]progress option. + GIT_TRACE2_EVENT="$(pwd)/no-opt-trace" scalar clone \ + --no-tags --no-src \ + "file://$(pwd)" no-opts --single-branch 2>/dev/null && + + test_subcommand git fetch --quiet --no-progress \ + origin --no-tags <no-opt-trace && + ( + cd no-opts && + + test_cmp_config --no-tags remote.origin.tagopt && + git for-each-ref --format="%(refname)" refs/tags/ >tags && + test_line_count = 0 tags + ) +' + test_expect_success 'scalar reconfigure' ' git init one/src && scalar register one &&