From: Trieu Huynh <vikingtc4@gmail.com>
Date: Mon, 30 Mar 2026 18:36:39 GMT
Subject: Re: [GSoC PATCH] backfill: error out when HEAD cannot be parsed
Message-ID: <idaildqkkf26ks6hlibt5e7rhdlbytra3x3o7ovd543mnsthtm@h3um5miwhc4l>
In-Reply-To: <0af26f29-5643-4ff2-b659-ae0fa234161a@malon.dev>

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.