v2backfill: error out when HEAD cannot be parsed
4 messages between Apr 2, 2026 and Apr 3, 2026, from Trieu Huynh, Junio C Hamano.
Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.
From: Trieu Huynh <vikingtc4@gmail.com>
handle_revision_arg() returns non-zero on failure, but do_backfill() ignores 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 exists with a zero return code.
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 <vikingtc4@gmail.com>
---
Changes in v2:
- Update commit msg (Point out by Karthik Nayak <karthik.188@gmail.com>)
- Use test_grep instead of grep (Point out by Tian Yuchen <cat@malon.dev>)
builtin/backfill.c | 3 ++-
t/t5620-backfill.sh | 6 ++++++
2 files changed, 8 insertions(+), 1 deletion(-)
Show changes to 2 files +8 −1
builtin/backfill.c, t/t5620-backfill.sh
diff --git a/builtin/backfill.c b/builtin/backfill.c
index e9a33e81be..ca49e188df 100644
--- a/builtin/backfill.c
+++ b/builtin/backfill.c
@@ -92,7 +92,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 58c81556e7..066ee17d39 100755
--- a/t/t5620-backfill.sh
+++ b/t/t5620-backfill.sh
@@ -77,6 +77,12 @@ test_expect_success 'do partial clone 2, backfill min batch size' '
test_line_count = 0 revs2
'
+test_expect_success 'backfill on empty repo fails gracefully' '
+ git init empty-repo &&
+ test_must_fail git -C empty-repo backfill 2>err &&
+ test_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
Re: [GSoC PATCH v2] backfill: error out when HEAD cannot be parsed
Trieu Huynh <vikingtc4@gmail.com> writes:
Show 7 quoted lines
> From: Trieu Huynh <vikingtc4@gmail.com>
>
> handle_revision_arg() returns non-zero on failure, but do_backfill()
> ignores 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 exists with a zero return
> code.
"exists" -> "exits", I think.
But more importantly (with Devil's advocate hat on), what's the downside of the current behaviour?
You tell the command to backfill, the machinery does not find anything necessary to fetch to backfill, and successfully, quickly, and quietly exits. That sounds like a graceful exit to me.
Is there anything wrong with that?
Show 5 quoted lines
> +test_expect_success 'backfill on empty repo fails gracefully' '
> + git init empty-repo &&
> + test_must_fail git -C empty-repo backfill 2>err &&
> + test_grep "unable to parse HEAD" err
> +'
Re: [GSoC PATCH v2] backfill: error out when HEAD cannot be parsed
Junio C Hamano <gitster@pobox.com> writes:
Show 26 quoted lines
> Trieu Huynh <vikingtc4@gmail.com> writes:
>
>> From: Trieu Huynh <vikingtc4@gmail.com>
>>
>> handle_revision_arg() returns non-zero on failure, but do_backfill()
>> ignores 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 exists with a zero return
>> code.
>
> "exists" -> "exits", I think.
>
> But more importantly (with Devil's advocate hat on), what's the
> downside of the current behaviour?
>
> You tell the command to backfill, the machinery does not find
> anything necessary to fetch to backfill, and successfully, quickly,
> and quietly exits. That sounds like a graceful exit to me.
>
> Is there anything wrong with that?
>
>> +test_expect_success 'backfill on empty repo fails gracefully' '
>> + git init empty-repo &&
>> + test_must_fail git -C empty-repo backfill 2>err &&
>> + test_grep "unable to parse HEAD" err
>> +'
By the way, a more relevant thing to mention is that this change will probably become totally unnecessary in the presence of the ds/backfill-revs topic that is already in 'next'.
It does the usual "if you do not get revision range, fall back to HEAD", so
git backfill<RET>
in an empty repository gracefully does nothing, while giving revision ranges explicitly, like,
git backfill master..next
git backfill HEADin such a repository will be greeted with a more explicit "bad revision" error.
Another lesson to pay closer attention to what others are doing in the same project. This would have been easily discoverable if you attempted trial merges to 'next' and to 'seen' after you tested your change standalone (well, that is how I recalled the other topic anyway).
Thanks.
Re: [GSoC PATCH v2] backfill: error out when HEAD cannot be parsed
On Thu, Apr 02, 2026 at 01:09:13PM -0700, Junio C Hamano wrote:
Show 33 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Trieu Huynh <vikingtc4@gmail.com> writes:
> >
> >> From: Trieu Huynh <vikingtc4@gmail.com>
> >>
> >> handle_revision_arg() returns non-zero on failure, but do_backfill()
> >> ignores 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 exists with a zero return
> >> code.
> >
> > "exists" -> "exits", I think.
> >
> > But more importantly (with Devil's advocate hat on), what's the
> > downside of the current behaviour?
> >
> > You tell the command to backfill, the machinery does not find
> > anything necessary to fetch to backfill, and successfully, quickly,
> > and quietly exits. That sounds like a graceful exit to me.
> >
> > Is there anything wrong with that?
> >
> >> +test_expect_success 'backfill on empty repo fails gracefully' '
> >> + git init empty-repo &&
> >> + test_must_fail git -C empty-repo backfill 2>err &&
> >> + test_grep "unable to parse HEAD" err
> >> +'
>
> By the way, a more relevant thing to mention is that this change
> will probably become totally unnecessary in the presence of the
> ds/backfill-revs topic that is already in 'next'.
>
Ack, it'll be dropped by Derrick's patch: https://lore.kernel.org/git/610a162973a7ad59eba4ef4d5a9288f1fea1d2e8.1774538094.git.gitgitgadget@gmail.com
Show 20 quoted lines
> It does the usual "if you do not get revision range, fall back to
> HEAD", so
>
> git backfill<RET>
>
> in an empty repository gracefully does nothing, while giving
> revision ranges explicitly, like,
>
> git backfill master..next
> git backfill HEAD
>
> in such a repository will be greeted with a more explicit "bad
> revision" error.
>
> Another lesson to pay closer attention to what others are doing in
> the same project. This would have been easily discoverable if you
> attempted trial merges to 'next' and to 'seen' after you tested your
> change standalone (well, that is how I recalled the other topic
> anyway).
>
TBH, my initial approach (when preparing for GSoC) is just researching the codebase (related to the topic I choose), read the logic and found some codes likely are not correct, IIUC. Then, I check to see if anyone else already report/or on-going work on this in the mailing list. If not, I just wanna give a try to reproduce/re-check the code myself first and later fix the wrong codes. And, that's how I dropped Derrick's patch series that refactor/enhance on this kind of work. Anw, I'll follow your suggestions to make sure TAL against 'next' and 'seen' before submiiting future contributions.
Thank you for your guidance. Drop this patch here.
> Thanks.
>