Re: [PATCH] scalar: add --no-tags option
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Sep 6, 2024, 07:23 UTC
- Message-ID
- <58ca5257-2e56-830c-c85f-79e4c9e80858@gmx.de>
- In-Reply-To
- <pull.1780.git.1725545614416.gitgitgadget@gmail.com>
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/
Show 15 quoted lines
> 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.
Show 18 quoted lines
> + `--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",
Show 28 quoted lines
> + 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
Show 5 quoted lines
> +' > + > 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