From: Tian Yuchen <cat@malon.dev>
Date: Mon, 30 Mar 2026 17:41:56 GMT
Subject: Re: [GSoC PATCH] backfill: error out when HEAD cannot be parsed
Message-ID: <0af26f29-5643-4ff2-b659-ae0fa234161a@malon.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 <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