Re: [GSoC PATCH] backfill: auto-detect sparse-checkout from config
- From
Trieu Huynh <vikingtc4@gmail.com>
- Date
- Apr 1, 2026, 19:31 UTC
- Message-ID
- <buisigjsw3zrcy6bqaic2zefypq37kimju32eufquppsvkgkvx@cqd3cwj6an6t>
- In-Reply-To
- <xmqqo6k40wbl.fsf@gitster.g>
On Tue, Mar 31, 2026 at 09:59:10AM -0700, Junio C Hamano wrote:
Show 74 quoted lines
> 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.
> actually, the logic IIUC here is: - first, ctx.sprase originally is set to 0. - then, it check user's options. Assume, there is no option passed, still 0. - then, it check repo's config (core.sparseCheckout (default to 0 in enviroment.c) but it doesn't since the guard: if (ctx.sparse < 0) ctx.sparse = cfg->apply_sparse_checkout; - evenly. ctx.sparse still 0 eventhough in case the core.sparseCheckout = 1 (git config core.sparseCheckout true)
IMHO, this change set the default value to -1, then it can fallback to repo's config value if user has no-op passing (default to 0 (full backfill if user doesnt intent to config previously either).
Show 28 quoted lines
> 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 \