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@xxxxxxxxx>
>
> 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
