Re: [GSoC PATCH] backfill: auto-detect sparse-checkout from config

[Junio C Hamano <gitster@pobox.com>]

Trieu Huynh <vikingtc4@gmail.com> writes:

> git backfill currently initializes the `sparse` field in
> backfill_context to 0. This causes the command to always perform a
> full backfill by default, even when the repository has sparse-checkout
> enabled in its configuration (core.sparseCheckout).
>
> Because 'sparse' is explicitly set to 0 at initialization, any later
> logic intended to auto-detect the setting from the repository
> configuration becomes dead code, as it only triggers if the value
> is negative (sentinel).
>
> Change the initial value of .sparse to -1. This allows the command
> to correctly fallback to the repository's sparse-checkout settings
> when the '--sparse' or '--no-sparse' options are not provided on the
> command line.

The author of bff45557 (backfill: add --sparse option, 2025-02-03),
where this .sparse member originates, CC'ed for more intelligent
input than my review can offer ;-)


> Add a test case in t5620-backfill.sh to verify that 'git backfill'
> automatically respects the sparse-checkout configuration without
> explicit flags.
>
> Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>
> ---
>  builtin/backfill.c  |  2 +-
>  t/t5620-backfill.sh | 15 +++++++++++++++
>  2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/backfill.c b/builtin/backfill.c
> index 4b2db94173..0f31844ce7 100644
> --- a/builtin/backfill.c
> +++ b/builtin/backfill.c
> @@ -124,7 +124,7 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
>  		.repo = repo,
>  		.current_batch = OID_ARRAY_INIT,
>  		.min_batch_size = 50000,
> -		.sparse = 0,
> +		.sparse = -1,
>  		.show_progress = -1,
>  	};
>  	struct option options[] = {

I am a bit confused by this change.  What's the difference between
using -1 (which you picked) and 1 as the initial value for this
member?  From the proposed log message, I would have expected a new
code that says "ah, we notice, from this member being -1, that the
user did not specify --no-sparse or --sparse, so let's figure out if
our working tree is sparsely checked out ourselves and set it either
to 0 or to 1", but there is nothing like that in the code.  It seems
that the updated code relies on the fact that this part of
do_backfill() only cares if .sparse is zero or not, and ...

	if (ctx->sparse) {
		CALLOC_ARRAY(info.pl, 1);
		if (get_sparse_checkout_patterns(info.pl)) {
			path_walk_info_clear(&info);
			return error(_("problem loading sparse-checkout"));
		}
	}

... relies on get_sparse_checkout_patterns() not to do any harm when
the working tree is not sparsely checked out.

I am not sure if we want to call it "auto-detction".  It looks more
like "default to --sparse, relying that --sparse is a no-op in a
non-sparse working tree" at least to me.  Not that it is necessarily
wrong, and when people do "backfill" knowing that the working tree
is sparse, I am sympathetic if they prefer to keep the sparseness,
so such a change of default may be beneficial.

Derrick, what do you think?

> diff --git a/t/t5620-backfill.sh b/t/t5620-backfill.sh
> index 91b5115732..a1a8d736db 100755
> --- a/t/t5620-backfill.sh
> +++ b/t/t5620-backfill.sh
> @@ -149,6 +149,21 @@ test_expect_success 'backfill --sparse' '
>  	test_line_count = 0 missing
>  '
>  
> +test_expect_success 'backfill auto-detects sparse-checkout from config' '
> +	git clone --sparse --filter=blob:none \
> +		--single-branch --branch=main \
> +		"file://$(pwd)/srv.bare" backfill-auto-sparse &&
> +
> +	git -C backfill-auto-sparse rev-list --quiet --objects --missing=print HEAD >missing &&
> +	test_line_count = 44 missing &&
> +
> +	GIT_TRACE2_EVENT="$(pwd)/auto-sparse-trace" git \
> +		-C backfill-auto-sparse backfill &&
> +
> +	test_trace2_data promisor fetch_count 4 <auto-sparse-trace &&
> +	test_trace2_data path-walk paths 5 <auto-sparse-trace
> +'
> +
>  test_expect_success 'backfill --sparse without cone mode (positive)' '
>  	git clone --no-checkout --filter=blob:none		\
>  		--single-branch --branch=main 		\