{"thread":{"id":"65382","subject":"[GSoC PATCH] backfill: error out when HEAD cannot be parsed","startedAt":"2026-03-29T18:36:10Z","lastAt":"2026-03-30T18:36:45Z","messageCount":4,"participants":["Trieu Huynh","Karthik Nayak","Tian Yuchen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"540334","messageId":"20260329183603.538241-1-vikingtc4@gmail.com","threadId":"65382","inReplyTo":null,"subject":"[GSoC PATCH] backfill: error out when HEAD cannot be parsed","fromName":"Trieu Huynh","fromEmail":"vikingtc4@gmail.com","sentAt":"2026-03-29T18:36:03Z","receivedAt":"2026-03-29T18:36:10Z","isPatch":true,"body":"handle_revision_arg() returns non-zero on failure, but do_backfill()\nignored the return value. On an empty repo with no commits, HEAD is\nunborn and handle_revision_arg() fails, but backfill silently\ncontinues with an empty revision walk and exits zero, looks like\nsuccess but did nothing.\n\nCheck the return value and propagate the error, consistent with\nhow builtin/pack-objects.c handles handle_revision_arg() failures.\n\nAdd a test to verify that backfill on an empty repository fails\nwith a clear error message.\n\nSigned-off-by: Trieu Huynh <vikingtc4@gmail.com>\n---\n builtin/backfill.c  | 3 ++-\n t/t5620-backfill.sh | 6 ++++++\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/backfill.c b/builtin/backfill.c\nindex 27a301f9b2..4b2db94173 100644\n--- a/builtin/backfill.c\n+++ b/builtin/backfill.c\n@@ -96,7 +96,8 @@ static int do_backfill(struct backfill_context *ctx)\n \t}\n \n \trepo_init_revisions(ctx->repo, &revs, \"\");\n-\thandle_revision_arg(\"HEAD\", &revs, 0, 0);\n+\tif (handle_revision_arg(\"HEAD\", &revs, 0, 0))\n+\t\treturn error(_(\"unable to parse HEAD revision\"));\n \n \tinfo.blobs = 1;\n \tinfo.tags = info.commits = info.trees = 0;\ndiff --git a/t/t5620-backfill.sh b/t/t5620-backfill.sh\nindex ff67e8ecea..91b5115732 100755\n--- a/t/t5620-backfill.sh\n+++ b/t/t5620-backfill.sh\n@@ -101,6 +101,12 @@ test_expect_success 'backfill no flag on non-TTY is silent' '\n \ttest_grep ! \"Downloading batches\" err\n '\n \n+test_expect_success 'backfill on empty repo fails gracefully' '\n+\tgit init empty-repo &&\n+\ttest_must_fail git -C empty-repo backfill 2>err &&\n+\tgrep \"unable to parse HEAD\" err\n+'\n+\n test_expect_success 'backfill --sparse without sparse-checkout fails' '\n \tgit init not-sparse &&\n \ttest_must_fail git -C not-sparse backfill --sparse 2>err &&\n-- \n2.43.0\n\n"},{"id":"540358","messageId":"CAOLa=ZSRAQtZKNWUw4YhX4bx6MwFccN5oR0tSKQFtex0sKZGmQ@mail.gmail.com","threadId":"65382","inReplyTo":"20260329183603.538241-1-vikingtc4@gmail.com","subject":"Re: [GSoC PATCH] backfill: error out when HEAD cannot be parsed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-03-30T09:48:34Z","receivedAt":"2026-03-30T09:48:37Z","isPatch":true,"body":"Trieu Huynh <vikingtc4@gmail.com> writes:\n\n> handle_revision_arg() returns non-zero on failure, but do_backfill()\n> ignored the return value. On an empty repo with no commits, HEAD is\n\nNit: s/ignored/ignores\n\nWe use present tense in the commit message. See the 'present-tense'\nsection in 'Documentation/SubmittingPatches' for more info.\n\n> unborn and handle_revision_arg() fails, but backfill silently\n> continues with an empty revision walk and exits zero, looks like\n> success but did nothing.\n\n\nPerhaps: we want to say 'and silently exists with a zero return code.'\nto be more explicit.\n\n\n> Check the return value and propagate the error, consistent with\n> how builtin/pack-objects.c handles handle_revision_arg() failures.\n>\n\nNice.\n\n> Add a test to verify that backfill on an empty repository fails\n> with a clear error message.\n>\n> Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>\n> ---\n>  builtin/backfill.c  | 3 ++-\n>  t/t5620-backfill.sh | 6 ++++++\n>  2 files changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/backfill.c b/builtin/backfill.c\n> index 27a301f9b2..4b2db94173 100644\n> --- a/builtin/backfill.c\n> +++ b/builtin/backfill.c\n> @@ -96,7 +96,8 @@ static int do_backfill(struct backfill_context *ctx)\n>  \t}\n>\n>  \trepo_init_revisions(ctx->repo, &revs, \"\");\n> -\thandle_revision_arg(\"HEAD\", &revs, 0, 0);\n> +\tif (handle_revision_arg(\"HEAD\", &revs, 0, 0))\n> +\t\treturn error(_(\"unable to parse HEAD revision\"));\n>\n\nMakes sense.\n\n>  \tinfo.blobs = 1;\n>  \tinfo.tags = info.commits = info.trees = 0;\n> diff --git a/t/t5620-backfill.sh b/t/t5620-backfill.sh\n> index ff67e8ecea..91b5115732 100755\n> --- a/t/t5620-backfill.sh\n> +++ b/t/t5620-backfill.sh\n> @@ -101,6 +101,12 @@ test_expect_success 'backfill no flag on non-TTY is silent' '\n>  \ttest_grep ! \"Downloading batches\" err\n>  '\n>\n> +test_expect_success 'backfill on empty repo fails gracefully' '\n> +\tgit init empty-repo &&\n> +\ttest_must_fail git -C empty-repo backfill 2>err &&\n> +\tgrep \"unable to parse HEAD\" err\n> +'\n> +\n\nLooks good. Thanks\n\n>  test_expect_success 'backfill --sparse without sparse-checkout fails' '\n>  \tgit init not-sparse &&\n>  \ttest_must_fail git -C not-sparse backfill --sparse 2>err &&\n> --\n> 2.43.0\n"},{"id":"540413","messageId":"0af26f29-5643-4ff2-b659-ae0fa234161a@malon.dev","threadId":"65382","inReplyTo":"20260329183603.538241-1-vikingtc4@gmail.com","subject":"Re: [GSoC PATCH] backfill: error out when HEAD cannot be parsed","fromName":"Tian Yuchen","fromEmail":"cat@malon.dev","sentAt":"2026-03-30T17:41:56Z","receivedAt":"2026-03-30T17:42:07Z","isPatch":true,"body":"On 3/30/26 02:36, Trieu Huynh wrote:\n> handle_revision_arg() returns non-zero on failure, but do_backfill()\n> ignored the return value. On an empty repo with no commits, HEAD is\n> unborn and handle_revision_arg() fails, but backfill silently\n> continues with an empty revision walk and exits zero, looks like\n> success but did nothing.\n> \n> Check the return value and propagate the error, consistent with\n> how builtin/pack-objects.c handles handle_revision_arg() failures.\n> \n> Add a test to verify that backfill on an empty repository fails\n> with a clear error message.\n> \n\nAside from the minor flaws Karthik mentioned, I think this commit \nmessage is spot on.\n\n> Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>\n> ---\n>   builtin/backfill.c  | 3 ++-\n>   t/t5620-backfill.sh | 6 ++++++\n>   2 files changed, 8 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/backfill.c b/builtin/backfill.c\n> index 27a301f9b2..4b2db94173 100644\n> --- a/builtin/backfill.c\n> +++ b/builtin/backfill.c\n> @@ -96,7 +96,8 @@ static int do_backfill(struct backfill_context *ctx)\n>   \t}\n>   \n>   \trepo_init_revisions(ctx->repo, &revs, \"\");\n> -\thandle_revision_arg(\"HEAD\", &revs, 0, 0);\n> +\tif (handle_revision_arg(\"HEAD\", &revs, 0, 0))\n> +\t\treturn error(_(\"unable to parse HEAD revision\"));\n>  \n\nLooks good to me.\n\n>   \tinfo.blobs = 1;\n>   \tinfo.tags = info.commits = info.trees = 0;\n> diff --git a/t/t5620-backfill.sh b/t/t5620-backfill.sh\n> index ff67e8ecea..91b5115732 100755\n> --- a/t/t5620-backfill.sh\n> +++ b/t/t5620-backfill.sh\n> @@ -101,6 +101,12 @@ test_expect_success 'backfill no flag on non-TTY is silent' '\n>   \ttest_grep ! \"Downloading batches\" err\n>   '\n>   \n> +test_expect_success 'backfill on empty repo fails gracefully' '\n> +\tgit init empty-repo &&\n> +\ttest_must_fail git -C empty-repo backfill 2>err &&\n> +\tgrep \"unable to parse HEAD\" err\n\nRemember your last patch? Wouldn't it be better to use 'test_grep' here?\nIt's easy to see that the original code uses 'test_grep' (a few lines \nabove):\n\n\t>   \ttest_grep ! \"Downloading batches\" err\n\nWouldn't it be better to maintain consistency? ;)\n\n> +'\n> +\n>   test_expect_success 'backfill --sparse without sparse-checkout fails' '\n>   \tgit init not-sparse &&\n>   \ttest_must_fail git -C not-sparse backfill --sparse 2>err &&\n\nRegards,\n\nYuchen\n\n"},{"id":"540417","messageId":"idaildqkkf26ks6hlibt5e7rhdlbytra3x3o7ovd543mnsthtm@h3um5miwhc4l","threadId":"65382","inReplyTo":"0af26f29-5643-4ff2-b659-ae0fa234161a@malon.dev","subject":"Re: [GSoC PATCH] backfill: error out when HEAD cannot be parsed","fromName":"Trieu Huynh","fromEmail":"vikingtc4@gmail.com","sentAt":"2026-03-30T18:36:39Z","receivedAt":"2026-03-30T18:36:45Z","isPatch":true,"body":"On Tue, Mar 31, 2026 at 01:41:56AM +0800, Tian Yuchen wrote:\n> On 3/30/26 02:36, Trieu Huynh wrote:\n> > handle_revision_arg() returns non-zero on failure, but do_backfill()\n> > ignored the return value. On an empty repo with no commits, HEAD is\n> > unborn and handle_revision_arg() fails, but backfill silently\n> > continues with an empty revision walk and exits zero, looks like\n> > success but did nothing.\n> > \n> > Check the return value and propagate the error, consistent with\n> > how builtin/pack-objects.c handles handle_revision_arg() failures.\n> > \n> > Add a test to verify that backfill on an empty repository fails\n> > with a clear error message.\n> > \n> \n> Aside from the minor flaws Karthik mentioned, I think this commit message is\n> spot on.\n> \n> > Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>\n> > ---\n> >   builtin/backfill.c  | 3 ++-\n> >   t/t5620-backfill.sh | 6 ++++++\n> >   2 files changed, 8 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/builtin/backfill.c b/builtin/backfill.c\n> > index 27a301f9b2..4b2db94173 100644\n> > --- a/builtin/backfill.c\n> > +++ b/builtin/backfill.c\n> > @@ -96,7 +96,8 @@ static int do_backfill(struct backfill_context *ctx)\n> >   \t}\n> >   \trepo_init_revisions(ctx->repo, &revs, \"\");\n> > -\thandle_revision_arg(\"HEAD\", &revs, 0, 0);\n> > +\tif (handle_revision_arg(\"HEAD\", &revs, 0, 0))\n> > +\t\treturn error(_(\"unable to parse HEAD revision\"));\n> \n> Looks good to me.\n> \n> >   \tinfo.blobs = 1;\n> >   \tinfo.tags = info.commits = info.trees = 0;\n> > diff --git a/t/t5620-backfill.sh b/t/t5620-backfill.sh\n> > index ff67e8ecea..91b5115732 100755\n> > --- a/t/t5620-backfill.sh\n> > +++ b/t/t5620-backfill.sh\n> > @@ -101,6 +101,12 @@ test_expect_success 'backfill no flag on non-TTY is silent' '\n> >   \ttest_grep ! \"Downloading batches\" err\n> >   '\n> > +test_expect_success 'backfill on empty repo fails gracefully' '\n> > +\tgit init empty-repo &&\n> > +\ttest_must_fail git -C empty-repo backfill 2>err &&\n> > +\tgrep \"unable to parse HEAD\" err\n> \n> Remember your last patch? Wouldn't it be better to use 'test_grep' here?\n> It's easy to see that the original code uses 'test_grep' (a few lines\n> above):\n> \n> \t>   \ttest_grep ! \"Downloading batches\" err\n> \n> Wouldn't it be better to maintain consistency? ;)\nthanks for the spot.\n> \n> > +'\n> > +\n> >   test_expect_success 'backfill --sparse without sparse-checkout fails' '\n> >   \tgit init not-sparse &&\n> >   \ttest_must_fail git -C not-sparse backfill --sparse 2>err &&\n> \n> Regards,\n> \n> Yuchen\n> \nacked, will submit v2, shortly.\n"}]}