Volume XXII, number 280Wednesday, October 7, 2026Latest message 55 minutes ago

The Git List

News and archive of git@vger.kernel.org, since April 2005

patchbackfill: error out when HEAD cannot be parsed

4 messages between Mar 29, 2026 and Mar 30, 2026, from Trieu Huynh, Karthik Nayak, Tian Yuchen.

Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.

Trieu HuynhMar 29, 2026, 18:36 UTC on lore

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(-)
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 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 NayakMar 30, 2026, 09:48 UTC in reply to Trieu Huynh on lore

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

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.
Show 21 quoted lines
> 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.
Show 16 quoted lines
>  	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
Show 5 quoted lines
>  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 YuchenMar 30, 2026, 17:41 UTC in reply to Trieu Huynh on lore

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

On 3/30/26 02:36, Trieu Huynh wrote:
Show 12 quoted lines
> 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.

Show 18 quoted lines
> 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.
Show 14 quoted lines
>   	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? ;)
Show 5 quoted lines
> +'
> +
>   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 HuynhMar 30, 2026, 18:36 UTC in reply to Tian Yuchen on lore

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

On Tue, Mar 31, 2026 at 01:41:56AM +0800, Tian Yuchen wrote:
Show 57 quoted lines
> 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.
Show 11 quoted lines
> 
> > +'
> > +
> >   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.

Back to recent threads