[GSoC PATCH v2] backfill: error out when HEAD cannot be parsed

[GSoC PATCH v2] backfill: error out when HEAD cannot be parsed

[Trieu Huynh]

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(-)

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

[Junio C Hamano]

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
> +'

Re: [GSoC PATCH v2] backfill: error out when HEAD cannot be parsed

[Junio C Hamano]

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'.

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).

Thanks.

Re: [GSoC PATCH v2] backfill: error out when HEAD cannot be parsed

[Trieu Huynh]

On Thu, Apr 02, 2026 at 01:09:13PM -0700, Junio C Hamano wrote:
> 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
> 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.
>