{"thread":{"id":"65418","subject":"[GSoC PATCH v2] backfill: error out when HEAD cannot be parsed","startedAt":"2026-04-02T19:14:14Z","lastAt":"2026-04-03T17:24:27Z","messageCount":4,"participants":["Trieu Huynh","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"540786","messageId":"20260402191359.11304-1-viking4@gmail.com","threadId":"65418","inReplyTo":null,"subject":"[GSoC PATCH v2] backfill: error out when HEAD cannot be parsed","fromName":"Trieu Huynh","fromEmail":"vikingtc4@gmail.com","sentAt":"2026-04-02T19:13:58Z","receivedAt":"2026-04-02T19:14:14Z","isPatch":true,"body":"From: Trieu Huynh \n\nhandle_revision_arg() returns non-zero on failure, but do_backfill()\nignores 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 exists with a zero return\ncode.\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 \n---\nChanges in v2:\n- Update commit msg (Point out by Karthik Nayak )\n- Use test_grep instead of grep (Point out by Tian Yuchen )\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 e9a33e81be..ca49e188df 100644\n--- a/builtin/backfill.c\n+++ b/builtin/backfill.c\n@@ -92,7 +92,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 58c81556e7..066ee17d39 100755\n--- a/t/t5620-backfill.sh\n+++ b/t/t5620-backfill.sh\n@@ -77,6 +77,12 @@ test_expect_success 'do partial clone 2, backfill min batch size' '\n \ttest_line_count = 0 revs2\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+\ttest_grep \"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":"540792","messageId":"xmqqcy0hp25j.fsf@gitster.g","threadId":"65418","inReplyTo":"20260402191359.11304-1-viking4@gmail.com","subject":"Re: [GSoC PATCH v2] backfill: error out when HEAD cannot be parsed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-04-02T19:56:08Z","receivedAt":"2026-04-02T19:56:10Z","isPatch":true,"body":"Trieu Huynh writes:\n\n> From: Trieu Huynh \n>\n> handle_revision_arg() returns non-zero on failure, but do_backfill()\n> ignores 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 exists with a zero return\n> code.\n\n\"exists\" -> \"exits\", I think.\n\nBut more importantly (with Devil's advocate hat on), what's the\ndownside of the current behaviour?\n\nYou tell the command to backfill, the machinery does not find\nanything necessary to fetch to backfill, and successfully, quickly,\nand quietly exits. That sounds like a graceful exit to me.\n\nIs there anything wrong with that?\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> +\ttest_grep \"unable to parse HEAD\" err\n> +'\n"},{"id":"540793","messageId":"xmqq8qb5p1jq.fsf@gitster.g","threadId":"65418","inReplyTo":"xmqqcy0hp25j.fsf@gitster.g","subject":"Re: [GSoC PATCH v2] backfill: error out when HEAD cannot be parsed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-04-02T20:09:13Z","receivedAt":"2026-04-02T20:09:15Z","isPatch":true,"body":"Junio C Hamano writes:\n\n> Trieu Huynh writes:\n>\n>> From: Trieu Huynh \n>>\n>> handle_revision_arg() returns non-zero on failure, but do_backfill()\n>> ignores 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 exists with a zero return\n>> code.\n>\n> \"exists\" -> \"exits\", I think.\n>\n> But more importantly (with Devil's advocate hat on), what's the\n> downside of the current behaviour?\n>\n> You tell the command to backfill, the machinery does not find\n> anything necessary to fetch to backfill, and successfully, quickly,\n> and quietly exits. That sounds like a graceful exit to me.\n>\n> Is there anything wrong with that?\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>> +\ttest_grep \"unable to parse HEAD\" err\n>> +'\n\nBy the way, a more relevant thing to mention is that this change\nwill probably become totally unnecessary in the presence of the\nds/backfill-revs topic that is already in 'next'.\n\nIt does the usual \"if you do not get revision range, fall back to\nHEAD\", so\n\n git backfill\n\nin an empty repository gracefully does nothing, while giving\nrevision ranges explicitly, like,\n\n git backfill master..next\n git backfill HEAD\n\nin such a repository will be greeted with a more explicit \"bad\nrevision\" error.\n\nAnother lesson to pay closer attention to what others are doing in\nthe same project. This would have been easily discoverable if you\nattempted trial merges to 'next' and to 'seen' after you tested your\nchange standalone (well, that is how I recalled the other topic\nanyway).\n\nThanks.\n\n"},{"id":"540857","messageId":"ktge5fdo52fvf6ajz5i7nggxwomelel2g5oo522r5wz6ky5tkj@ijoeff5ykxxr","threadId":"65418","inReplyTo":"xmqq8qb5p1jq.fsf@gitster.g","subject":"Re: [GSoC PATCH v2] backfill: error out when HEAD cannot be parsed","fromName":"Trieu Huynh","fromEmail":"vikingtc4@gmail.com","sentAt":"2026-04-03T17:24:22Z","receivedAt":"2026-04-03T17:24:27Z","isPatch":true,"body":"On Thu, Apr 02, 2026 at 01:09:13PM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > Trieu Huynh writes:\n> >\n> >> From: Trieu Huynh \n> >>\n> >> handle_revision_arg() returns non-zero on failure, but do_backfill()\n> >> ignores 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 exists with a zero return\n> >> code.\n> >\n> > \"exists\" -> \"exits\", I think.\n> >\n> > But more importantly (with Devil's advocate hat on), what's the\n> > downside of the current behaviour?\n> >\n> > You tell the command to backfill, the machinery does not find\n> > anything necessary to fetch to backfill, and successfully, quickly,\n> > and quietly exits. That sounds like a graceful exit to me.\n> >\n> > Is there anything wrong with that?\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> >> +\ttest_grep \"unable to parse HEAD\" err\n> >> +'\n> \n> By the way, a more relevant thing to mention is that this change\n> will probably become totally unnecessary in the presence of the\n> ds/backfill-revs topic that is already in 'next'.\n> \nAck, it'll be dropped by Derrick's patch:\nhttps://lore.kernel.org/git/610a162973a7ad59eba4ef4d5a9288f1fea1d2e8.1774538094.git.gitgitgadget@gmail.com\n> It does the usual \"if you do not get revision range, fall back to\n> HEAD\", so\n> \n> git backfill\n> \n> in an empty repository gracefully does nothing, while giving\n> revision ranges explicitly, like,\n> \n> git backfill master..next\n> git backfill HEAD\n> \n> in such a repository will be greeted with a more explicit \"bad\n> revision\" error.\n> \n> Another lesson to pay closer attention to what others are doing in\n> the same project. This would have been easily discoverable if you\n> attempted trial merges to 'next' and to 'seen' after you tested your\n> change standalone (well, that is how I recalled the other topic\n> anyway).\n> \nTBH, my initial approach (when preparing for GSoC) is just researching\nthe codebase (related to the topic I choose), read the logic and\nfound some codes likely are not correct, IIUC. Then, I check to see if\nanyone else already report/or on-going work on this in the mailing list.\nIf not, I just wanna give a try to reproduce/re-check the code myself\nfirst and later fix the wrong codes.\nAnd, that's how I dropped Derrick's patch series that refactor/enhance\non this kind of work.\nAnw, I'll follow your suggestions to make sure TAL against 'next' and\n'seen' before submiiting future contributions.\n\nThank you for your guidance. Drop this patch here.\n> Thanks.\n> \n"}]}