# [GSoC PATCH] backfill: error out when HEAD cannot be parsed 4 messages from 2026-03-29 to 2026-03-30. Participants: Trieu Huynh, Karthik Nayak, Tian Yuchen. Thread: https://gitlist.dev/t/65382 ## Trieu Huynh, 2026-03-29 18:36 Subject: [GSoC PATCH] backfill: error out when HEAD cannot be parsed Message-ID: <20260329183603.538241-1-vikingtc4@gmail.com> URL: https://gitlist.dev/e/20260329183603.538241-1-vikingtc4%40gmail.com ``` handle_revision_arg() returns non-zero on failure, but do_backfill() ignored the return value. On an empty repo with no commits, HEAD is unborn and handle_revision_arg() fails, but backfill silently continues with an empty revision walk and exits zero, looks like success but did nothing. Check the return value and propagate the error, consistent with how builtin/pack-objects.c handles handle_revision_arg() failures. Add a test to verify that backfill on an empty repository fails with a clear error message. Signed-off-by: Trieu Huynh --- builtin/backfill.c | 3 ++- t/t5620-backfill.sh | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/builtin/backfill.c b/builtin/backfill.c index 27a301f9b2..4b2db94173 100644 --- a/builtin/backfill.c +++ b/builtin/backfill.c @@ -96,7 +96,8 @@ static int do_backfill(struct backfill_context *ctx) } repo_init_revisions(ctx->repo, &revs, ""); - handle_revision_arg("HEAD", &revs, 0, 0); + if (handle_revision_arg("HEAD", &revs, 0, 0)) + return error(_("unable to parse HEAD revision")); info.blobs = 1; info.tags = info.commits = info.trees = 0; diff --git a/t/t5620-backfill.sh b/t/t5620-backfill.sh index ff67e8ecea..91b5115732 100755 --- a/t/t5620-backfill.sh +++ b/t/t5620-backfill.sh @@ -101,6 +101,12 @@ test_expect_success 'backfill no flag on non-TTY is silent' ' test_grep ! "Downloading batches" err ' +test_expect_success 'backfill on empty repo fails gracefully' ' + git init empty-repo && + test_must_fail git -C empty-repo backfill 2>err && + grep "unable to parse HEAD" err +' + test_expect_success 'backfill --sparse without sparse-checkout fails' ' git init not-sparse && test_must_fail git -C not-sparse backfill --sparse 2>err && -- 2.43.0 ``` ## Karthik Nayak, 2026-03-30 09:48 Subject: Re: [GSoC PATCH] backfill: error out when HEAD cannot be parsed Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZSRAQtZKNWUw4YhX4bx6MwFccN5oR0tSKQFtex0sKZGmQ%40mail.gmail.com In-Reply-To: <20260329183603.538241-1-vikingtc4@gmail.com> ``` Trieu Huynh writes: > handle_revision_arg() returns non-zero on failure, but do_backfill() > ignored the return value. On an empty repo with no commits, HEAD is Nit: s/ignored/ignores We use present tense in the commit message. See the 'present-tense' section in 'Documentation/SubmittingPatches' for more info. > unborn and handle_revision_arg() fails, but backfill silently > continues with an empty revision walk and exits zero, looks like > success but did nothing. Perhaps: we want to say 'and silently exists with a zero return code.' to be more explicit. > Check the return value and propagate the error, consistent with > how builtin/pack-objects.c handles handle_revision_arg() failures. > Nice. > Add a test to verify that backfill on an empty repository fails > with a clear error message. > > Signed-off-by: Trieu Huynh > --- > builtin/backfill.c | 3 ++- > t/t5620-backfill.sh | 6 ++++++ > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/builtin/backfill.c b/builtin/backfill.c > index 27a301f9b2..4b2db94173 100644 > --- a/builtin/backfill.c > +++ b/builtin/backfill.c > @@ -96,7 +96,8 @@ static int do_backfill(struct backfill_context *ctx) > } > > repo_init_revisions(ctx->repo, &revs, ""); > - handle_revision_arg("HEAD", &revs, 0, 0); > + if (handle_revision_arg("HEAD", &revs, 0, 0)) > + return error(_("unable to parse HEAD revision")); > Makes sense. > info.blobs = 1; > info.tags = info.commits = info.trees = 0; > diff --git a/t/t5620-backfill.sh b/t/t5620-backfill.sh > index ff67e8ecea..91b5115732 100755 > --- a/t/t5620-backfill.sh > +++ b/t/t5620-backfill.sh > @@ -101,6 +101,12 @@ test_expect_success 'backfill no flag on non-TTY is silent' ' > test_grep ! "Downloading batches" err > ' > > +test_expect_success 'backfill on empty repo fails gracefully' ' > + git init empty-repo && > + test_must_fail git -C empty-repo backfill 2>err && > + grep "unable to parse HEAD" err > +' > + Looks good. Thanks > test_expect_success 'backfill --sparse without sparse-checkout fails' ' > git init not-sparse && > test_must_fail git -C not-sparse backfill --sparse 2>err && > -- > 2.43.0 ``` ## Tian Yuchen, 2026-03-30 17:41 Subject: Re: [GSoC PATCH] backfill: error out when HEAD cannot be parsed Message-ID: <0af26f29-5643-4ff2-b659-ae0fa234161a@malon.dev> URL: https://gitlist.dev/e/0af26f29-5643-4ff2-b659-ae0fa234161a%40malon.dev In-Reply-To: <20260329183603.538241-1-vikingtc4@gmail.com> ``` On 3/30/26 02:36, Trieu Huynh wrote: > handle_revision_arg() returns non-zero on failure, but do_backfill() > ignored the return value. On an empty repo with no commits, HEAD is > unborn and handle_revision_arg() fails, but backfill silently > continues with an empty revision walk and exits zero, looks like > success but did nothing. > > Check the return value and propagate the error, consistent with > how builtin/pack-objects.c handles handle_revision_arg() failures. > > Add a test to verify that backfill on an empty repository fails > with a clear error message. > Aside from the minor flaws Karthik mentioned, I think this commit message is spot on. > Signed-off-by: Trieu Huynh > --- > builtin/backfill.c | 3 ++- > t/t5620-backfill.sh | 6 ++++++ > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/builtin/backfill.c b/builtin/backfill.c > index 27a301f9b2..4b2db94173 100644 > --- a/builtin/backfill.c > +++ b/builtin/backfill.c > @@ -96,7 +96,8 @@ static int do_backfill(struct backfill_context *ctx) > } > > repo_init_revisions(ctx->repo, &revs, ""); > - handle_revision_arg("HEAD", &revs, 0, 0); > + if (handle_revision_arg("HEAD", &revs, 0, 0)) > + return error(_("unable to parse HEAD revision")); > Looks good to me. > info.blobs = 1; > info.tags = info.commits = info.trees = 0; > diff --git a/t/t5620-backfill.sh b/t/t5620-backfill.sh > index ff67e8ecea..91b5115732 100755 > --- a/t/t5620-backfill.sh > +++ b/t/t5620-backfill.sh > @@ -101,6 +101,12 @@ test_expect_success 'backfill no flag on non-TTY is silent' ' > test_grep ! "Downloading batches" err > ' > > +test_expect_success 'backfill on empty repo fails gracefully' ' > + git init empty-repo && > + test_must_fail git -C empty-repo backfill 2>err && > + grep "unable to parse HEAD" err Remember your last patch? Wouldn't it be better to use 'test_grep' here? It's easy to see that the original code uses 'test_grep' (a few lines above): > test_grep ! "Downloading batches" err Wouldn't it be better to maintain consistency? ;) > +' > + > test_expect_success 'backfill --sparse without sparse-checkout fails' ' > git init not-sparse && > test_must_fail git -C not-sparse backfill --sparse 2>err && Regards, Yuchen ``` ## Trieu Huynh, 2026-03-30 18:36 Subject: Re: [GSoC PATCH] backfill: error out when HEAD cannot be parsed Message-ID: URL: https://gitlist.dev/e/idaildqkkf26ks6hlibt5e7rhdlbytra3x3o7ovd543mnsthtm%40h3um5miwhc4l In-Reply-To: <0af26f29-5643-4ff2-b659-ae0fa234161a@malon.dev> ``` On Tue, Mar 31, 2026 at 01:41:56AM +0800, Tian Yuchen wrote: > On 3/30/26 02:36, Trieu Huynh wrote: > > handle_revision_arg() returns non-zero on failure, but do_backfill() > > ignored the return value. On an empty repo with no commits, HEAD is > > unborn and handle_revision_arg() fails, but backfill silently > > continues with an empty revision walk and exits zero, looks like > > success but did nothing. > > > > Check the return value and propagate the error, consistent with > > how builtin/pack-objects.c handles handle_revision_arg() failures. > > > > Add a test to verify that backfill on an empty repository fails > > with a clear error message. > > > > Aside from the minor flaws Karthik mentioned, I think this commit message is > spot on. > > > Signed-off-by: Trieu Huynh > > --- > > builtin/backfill.c | 3 ++- > > t/t5620-backfill.sh | 6 ++++++ > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/builtin/backfill.c b/builtin/backfill.c > > index 27a301f9b2..4b2db94173 100644 > > --- a/builtin/backfill.c > > +++ b/builtin/backfill.c > > @@ -96,7 +96,8 @@ static int do_backfill(struct backfill_context *ctx) > > } > > repo_init_revisions(ctx->repo, &revs, ""); > > - handle_revision_arg("HEAD", &revs, 0, 0); > > + if (handle_revision_arg("HEAD", &revs, 0, 0)) > > + return error(_("unable to parse HEAD revision")); > > Looks good to me. > > > info.blobs = 1; > > info.tags = info.commits = info.trees = 0; > > diff --git a/t/t5620-backfill.sh b/t/t5620-backfill.sh > > index ff67e8ecea..91b5115732 100755 > > --- a/t/t5620-backfill.sh > > +++ b/t/t5620-backfill.sh > > @@ -101,6 +101,12 @@ test_expect_success 'backfill no flag on non-TTY is silent' ' > > test_grep ! "Downloading batches" err > > ' > > +test_expect_success 'backfill on empty repo fails gracefully' ' > > + git init empty-repo && > > + test_must_fail git -C empty-repo backfill 2>err && > > + grep "unable to parse HEAD" err > > Remember your last patch? Wouldn't it be better to use 'test_grep' here? > It's easy to see that the original code uses 'test_grep' (a few lines > above): > > > test_grep ! "Downloading batches" err > > Wouldn't it be better to maintain consistency? ;) thanks for the spot. > > > +' > > + > > test_expect_success 'backfill --sparse without sparse-checkout fails' ' > > git init not-sparse && > > test_must_fail git -C not-sparse backfill --sparse 2>err && > > Regards, > > Yuchen > acked, will submit v2, shortly. ```