{"thread":{"id":"65395","subject":"[RFC GSoC PATCH] backfill: skip downloading for empty batches","startedAt":"2026-03-31T12:12:10Z","lastAt":"2026-04-01T19:45:03Z","messageCount":3,"participants":["Trieu Huynh","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"540514","messageId":"20260331121204.787826-1-vikingtc4@gmail.com","threadId":"65395","inReplyTo":null,"subject":"[RFC GSoC PATCH] backfill: skip downloading for empty batches","fromName":"Trieu Huynh","fromEmail":"vikingtc4@gmail.com","sentAt":"2026-03-31T12:12:04Z","receivedAt":"2026-03-31T12:12:10Z","isPatch":true,"body":"When git backfill finishes its object walk, it unconditionally calls\ndownload_batch to process any remaining objects. If the repository\nis already up-to-date (no missing objects found), this call still\nperforms an unnecessary directory scan via odb_reprepare.\n\nFix it by adding a check in do_backfill to ensure download_batch is only\ncalled if the current batch actually contains objects (nr > 0).\n\nTo facilitate testing and provide better telemetry, add a trace2 data\nevent for batches_requested. This allows us to verify that no batches\nare processed when the command is run on an up-to-date repository.\n\nAdd a test case in t5620-backfill.sh to ensure silence and efficiency\nwhen no objects are missing.\n\nSigned-off-by: Trieu Huynh <vikingtc4@gmail.com>\n---\nNeed discussion:\n1. Is adding trace2_data_intmax() the preferred way to verify this \n   behavior in our test suite, or should we rely on redirection of \n   stderr to check for progress messages when the progress option\n   is supported?\n\n builtin/backfill.c  |  3 ++-\n t/t5620-backfill.sh | 16 ++++++++++++++++\n 2 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/backfill.c b/builtin/backfill.c\nindex 0f31844ce7..67f9f28daf 100644\n--- a/builtin/backfill.c\n+++ b/builtin/backfill.c\n@@ -58,6 +58,7 @@ static void download_batch(struct backfill_context *ctx)\n \t */\n \todb_reprepare(ctx->repo->objects);\n \tdisplay_progress(ctx->progress, ++ctx->batches_requested);\n+\ttrace2_data_intmax(\"backfill\", ctx->repo, \"batches_requested\", ctx->batches_requested);\n }\n \n static int fill_missing_blobs(const char *path UNUSED,\n@@ -109,7 +110,7 @@ static int do_backfill(struct backfill_context *ctx)\n \tret = walk_objects_by_path(&info);\n \n \t/* Download the objects that did not fill a batch. */\n-\tif (!ret)\n+\tif ( (!ret) && (ctx->current_batch.nr > 0) )\n \t\tdownload_batch(ctx);\n \n \tpath_walk_info_clear(&info);\ndiff --git a/t/t5620-backfill.sh b/t/t5620-backfill.sh\nindex a1a8d736db..d3cc4022bf 100755\n--- a/t/t5620-backfill.sh\n+++ b/t/t5620-backfill.sh\n@@ -221,6 +221,22 @@ test_expect_success 'backfill --sparse without cone mode (negative)' '\n \ttest_line_count = 12 missing\n '\n \n+test_expect_success 'backfill does not request batches when up-to-date' '\n+\tgit clone --no-checkout --filter=blob:none \\\n+\t\t--single-branch --branch=main \\\n+\t\t\"file://$(pwd)/srv.bare\" backfill-up-to-date &&\n+\n+\t# First trigger to have a full download\n+\tgit -C backfill-up-to-date backfill &&\n+\n+\t# Second trigger to verify when already have a full download previously\n+\tGIT_TRACE2_EVENT=\"$(pwd)/up-to-date-trace\" git \\\n+\t\t-C backfill-up-to-date backfill &&\n+\n+\t# Verify no  batches_request occurr\n+\ttest_grep ! \"batches_requested\" up-to-date-trace\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.43.0\n\n"},{"id":"540635","messageId":"ac0GnzQgZMfu8aGL@pks.im","threadId":"65395","inReplyTo":"20260331121204.787826-1-vikingtc4@gmail.com","subject":"Re: [RFC GSoC PATCH] backfill: skip downloading for empty batches","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-01T11:50:55Z","receivedAt":"2026-04-01T11:51:01Z","isPatch":true,"body":"On Tue, Mar 31, 2026 at 09:12:04PM +0900, Trieu Huynh wrote:\n> When git backfill finishes its object walk, it unconditionally calls\n> download_batch to process any remaining objects. If the repository\n> is already up-to-date (no missing objects found), this call still\n> performs an unnecessary directory scan via odb_reprepare.\n> \n> Fix it by adding a check in do_backfill to ensure download_batch is only\n> called if the current batch actually contains objects (nr > 0).\n> \n> To facilitate testing and provide better telemetry, add a trace2 data\n> event for batches_requested. This allows us to verify that no batches\n> are processed when the command is run on an up-to-date repository.\n> \n> Add a test case in t5620-backfill.sh to ensure silence and efficiency\n> when no objects are missing.\n> \n> Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>\n> ---\n> Need discussion:\n> 1. Is adding trace2_data_intmax() the preferred way to verify this \n>    behavior in our test suite, or should we rely on redirection of \n>    stderr to check for progress messages when the progress option\n>    is supported?\n\nI think adding a call to trace2 only for the test itself doesn't make a\nlot of sense if we already have another way to verify. But would we\nactually see any progress messages? `promisor_remote_get_direct()` knows\nto bail out early in case there is nothing to be downloaded, so the only\ndifference really is the call to `odb_reprepare()`.\n\nOr is it? This part here...\n\n> diff --git a/builtin/backfill.c b/builtin/backfill.c\n> index 0f31844ce7..67f9f28daf 100644\n> --- a/builtin/backfill.c\n> +++ b/builtin/backfill.c\n> @@ -58,6 +58,7 @@ static void download_batch(struct backfill_context *ctx)\n>  \t */\n>  \todb_reprepare(ctx->repo->objects);\n>  \tdisplay_progress(ctx->progress, ++ctx->batches_requested);\n> +\ttrace2_data_intmax(\"backfill\", ctx->repo, \"batches_requested\", ctx->batches_requested);\n>  }\n>  \n>  static int fill_missing_blobs(const char *path UNUSED,\n\n... looks different. What commit is this patch based on? There is no\ncall to `display_progress()` on \"master\", and you didn't mention any\nother dependency in your cover letter. Please note such dependencies\nwhen you post a patch that has any requirements.\n\nBut in any case, this here would cause us to print \"batches_requested\"\nevents repeatedly, which doesn't make a lot of sense.\n\n> @@ -109,7 +110,7 @@ static int do_backfill(struct backfill_context *ctx)\n>  \tret = walk_objects_by_path(&info);\n>  \n>  \t/* Download the objects that did not fill a batch. */\n> -\tif (!ret)\n> +\tif ( (!ret) && (ctx->current_batch.nr > 0) )\n>  \t\tdownload_batch(ctx);\n>  \n>  \tpath_walk_info_clear(&info);\n\nPlease pay attention to our coding guidlines, see\n\"Documentation/CodingGuidelines\".\n\nI guess a more robust fix would add the check in `download_batch()`\nitself, but I guess both alternatives work. But overall, it's sensible\nto avoid repreparing the ODB in case we know nothing has changed.\n\n> diff --git a/t/t5620-backfill.sh b/t/t5620-backfill.sh\n> index a1a8d736db..d3cc4022bf 100755\n> --- a/t/t5620-backfill.sh\n> +++ b/t/t5620-backfill.sh\n> @@ -221,6 +221,22 @@ test_expect_success 'backfill --sparse without cone mode (negative)' '\n>  \ttest_line_count = 12 missing\n>  '\n>  \n> +test_expect_success 'backfill does not request batches when up-to-date' '\n> +\tgit clone --no-checkout --filter=blob:none \\\n> +\t\t--single-branch --branch=main \\\n> +\t\t\"file://$(pwd)/srv.bare\" backfill-up-to-date &&\n> +\n> +\t# First trigger to have a full download\n> +\tgit -C backfill-up-to-date backfill &&\n> +\n> +\t# Second trigger to verify when already have a full download previously\n> +\tGIT_TRACE2_EVENT=\"$(pwd)/up-to-date-trace\" git \\\n> +\t\t-C backfill-up-to-date backfill &&\n> +\n> +\t# Verify no  batches_request occurr\n> +\ttest_grep ! \"batches_requested\" up-to-date-trace\n> +'\n\nI'm ultimately not sure whether this change even needs a test. We're not\nchanging any user-visible behaviour, we're simply skipping some\npointless busywork that doesn't do much, but that shouldn't really hurt\nmuch, either.\n\nPatrick\n"},{"id":"540667","messageId":"lwsskrhd2prb577xrpcse3f7oureuztmp4kyegn4gziu63zvcj@h4pqpympiava","threadId":"65395","inReplyTo":"ac0GnzQgZMfu8aGL@pks.im","subject":"Re: [RFC GSoC PATCH] backfill: skip downloading for empty batches","fromName":"Trieu Huynh","fromEmail":"vikingtc4@gmail.com","sentAt":"2026-04-01T19:44:57Z","receivedAt":"2026-04-01T19:45:03Z","isPatch":true,"body":"On Wed, Apr 01, 2026 at 01:50:55PM +0200, Patrick Steinhardt wrote:\n> On Tue, Mar 31, 2026 at 09:12:04PM +0900, Trieu Huynh wrote:\n> > When git backfill finishes its object walk, it unconditionally calls\n> > download_batch to process any remaining objects. If the repository\n> > is already up-to-date (no missing objects found), this call still\n> > performs an unnecessary directory scan via odb_reprepare.\n> > \n> > Fix it by adding a check in do_backfill to ensure download_batch is only\n> > called if the current batch actually contains objects (nr > 0).\n> > \n> > To facilitate testing and provide better telemetry, add a trace2 data\n> > event for batches_requested. This allows us to verify that no batches\n> > are processed when the command is run on an up-to-date repository.\n> > \n> > Add a test case in t5620-backfill.sh to ensure silence and efficiency\n> > when no objects are missing.\n> > \n> > Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>\n> > ---\n> > Need discussion:\n> > 1. Is adding trace2_data_intmax() the preferred way to verify this \n> >    behavior in our test suite, or should we rely on redirection of \n> >    stderr to check for progress messages when the progress option\n> >    is supported?\n> \n> I think adding a call to trace2 only for the test itself doesn't make a\n> lot of sense if we already have another way to verify. But would we\n> actually see any progress messages? `promisor_remote_get_direct()` knows\n> to bail out early in case there is nothing to be downloaded, so the only\n> difference really is the call to `odb_reprepare()`.\n> \n> Or is it? This part here...\n> \ncurrently, I have no idea to verify the change, so I'm adding a trace2 here.\n> > diff --git a/builtin/backfill.c b/builtin/backfill.c\n> > index 0f31844ce7..67f9f28daf 100644\n> > --- a/builtin/backfill.c\n> > +++ b/builtin/backfill.c\n> > @@ -58,6 +58,7 @@ static void download_batch(struct backfill_context *ctx)\n> >  \t */\n> >  \todb_reprepare(ctx->repo->objects);\n> >  \tdisplay_progress(ctx->progress, ++ctx->batches_requested);\n> > +\ttrace2_data_intmax(\"backfill\", ctx->repo, \"batches_requested\", ctx->batches_requested);\n> >  }\n> >  \n> >  static int fill_missing_blobs(const char *path UNUSED,\n> \n> ... looks different. What commit is this patch based on? There is no\n> call to `display_progress()` on \"master\", and you didn't mention any\n> other dependency in your cover letter. Please note such dependencies\n> when you post a patch that has any requirements.\n> \nI was submit another patch to support --[no-]progress option.\nhttps://lore.kernel.org/git/20260329152443.525493-1-vikingtc4@gmail.com/\nit should be based on master's latest rather than this change, sorry for\nthe confusion, will rebase on v2.\n> But in any case, this here would cause us to print \"batches_requested\"\n> events repeatedly, which doesn't make a lot of sense.\n> \nack, but I wonder if it should be defined method to verify if it can skip\nwhen no objects are missing or not here.\n> > @@ -109,7 +110,7 @@ static int do_backfill(struct backfill_context *ctx)\n> >  \tret = walk_objects_by_path(&info);\n> >  \n> >  \t/* Download the objects that did not fill a batch. */\n> > -\tif (!ret)\n> > +\tif ( (!ret) && (ctx->current_batch.nr > 0) )\n> >  \t\tdownload_batch(ctx);\n> >  \n> >  \tpath_walk_info_clear(&info);\n> \n> Please pay attention to our coding guidlines, see\n> \"Documentation/CodingGuidelines\".\n> \nack, I got it.\n> I guess a more robust fix would add the check in `download_batch()`\n> itself, but I guess both alternatives work. But overall, it's sensible\n> to avoid repreparing the ODB in case we know nothing has changed.\n> \nack, waiting for other reviews.\n> > diff --git a/t/t5620-backfill.sh b/t/t5620-backfill.sh\n> > index a1a8d736db..d3cc4022bf 100755\n> > --- a/t/t5620-backfill.sh\n> > +++ b/t/t5620-backfill.sh\n> > @@ -221,6 +221,22 @@ test_expect_success 'backfill --sparse without cone mode (negative)' '\n> >  \ttest_line_count = 12 missing\n> >  '\n> >  \n> > +test_expect_success 'backfill does not request batches when up-to-date' '\n> > +\tgit clone --no-checkout --filter=blob:none \\\n> > +\t\t--single-branch --branch=main \\\n> > +\t\t\"file://$(pwd)/srv.bare\" backfill-up-to-date &&\n> > +\n> > +\t# First trigger to have a full download\n> > +\tgit -C backfill-up-to-date backfill &&\n> > +\n> > +\t# Second trigger to verify when already have a full download previously\n> > +\tGIT_TRACE2_EVENT=\"$(pwd)/up-to-date-trace\" git \\\n> > +\t\t-C backfill-up-to-date backfill &&\n> > +\n> > +\t# Verify no  batches_request occurr\n> > +\ttest_grep ! \"batches_requested\" up-to-date-trace\n> > +'\n> \n> I'm ultimately not sure whether this change even needs a test. We're not\n> changing any user-visible behaviour, we're simply skipping some\n> pointless busywork that doesn't do much, but that shouldn't really hurt\n> much, either.\n> \nack, can provide steps to verify in commit msg instead of adding a test.\n> Patrick\n"}]}