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

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

[Trieu Huynh]

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 <vikingtc4@gmail.com>
---
 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

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

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2200 bytes --]

Trieu Huynh <vikingtc4@gmail.com> 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 <vikingtc4@gmail.com>
> ---
>  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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

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

[Tian Yuchen]

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 <vikingtc4@gmail.com>
> ---
>   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

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

[Trieu Huynh]

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 <vikingtc4@gmail.com>
> > ---
> >   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.