{"thread":{"id":"65437","subject":"[PATCH] object-file: don't use object database without a repository","startedAt":"2026-04-04T17:28:25Z","lastAt":"2026-04-06T20:38:06Z","messageCount":10,"participants":["Luca Stefani","Pushkar Singh","Jeff King","Justin Tobler"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"540908","messageId":"20260404172817.2995133-1-luca.stefani.ge1@gmail.com","threadId":"65437","inReplyTo":null,"subject":"[PATCH] object-file: don't use object database without a repository","fromName":"Luca Stefani","fromEmail":"luca.stefani.ge1@gmail.com","sentAt":"2026-04-04T17:28:17Z","receivedAt":"2026-04-04T17:28:25Z","isPatch":true,"body":"When running `git diff -- $file1 $file2' on large enough files,\nindex_fd() attempts to use 'the_repository->objects', assuming it\nis initialized, but that's not the case for non-repository usecases.\n\nWhen git diff is invoked without a backing repository,\nINDEX_WRITE_OBJECT is never set in flags, meaning only the hash is\nneeded and nothing should be written to the object store.\n\nEnforce the use of index_core() in this case.\n\nSigned-off-by: Luca Stefani <luca.stefani.ge1@gmail.com>\n---\n object-file.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex f0b029ff0b..68303aa99c 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1654,7 +1654,8 @@ int index_fd(struct index_state *istate, struct object_id *oid,\n \t} else if ((st->st_size >= 0 &&\n \t\t    (size_t)st->st_size <= repo_settings_get_big_file_threshold(istate->repo)) ||\n \t\t   type != OBJ_BLOB ||\n-\t\t   (path && would_convert_to_git(istate, path))) {\n+\t\t   (path && would_convert_to_git(istate, path)) ||\n+\t\t   !(flags & INDEX_WRITE_OBJECT)) {\n \t\tret = index_core(istate, oid, fd, xsize_t(st->st_size),\n \t\t\t\t type, path, flags);\n \t} else {\n-- \n2.54.0.rc0.dirty\n\n"},{"id":"540941","messageId":"CALE2CrSP0poB2u=SuWuhXNt-FLgqOTV0rmZoWYX8p6OOzpodOw@mail.gmail.com","threadId":"65437","inReplyTo":"20260404172817.2995133-1-luca.stefani.ge1@gmail.com","subject":"Re: [PATCH] object-file: don't use object database without a repository","fromName":"Pushkar Singh","fromEmail":"pushkarkumarsingh1970@gmail.com","sentAt":"2026-04-05T06:03:07Z","receivedAt":"2026-04-05T06:03:19Z","isPatch":true,"body":"Hi Luca,\n\nThanks for the patch, this was interesting to read.\n\n[snip]\n> When git diff is invoked without a backing repository,\n> INDEX_WRITE_OBJECT is never set in flags, meaning only the hash is\n> needed and nothing should be written to the object store.\n\nFrom my understanding, this avoids using the object database in\nnon-repository scenarios by forcing the use of index_core() when\nINDEX_WRITE_OBJECT is not set, which makes sense since we only\nneed the hash in that case.\n\nI had a small question regarding coverage:\n\n- Do we already have tests for cases like:\n  git diff -- <file1> <file2> outside a repository,\n  especially with large files triggering this path?\n\nIt might be useful to add one to ensure this behavior is\npreserved.\n\nAlso, are there any other callers of index_fd() that might\nrely on similar assumptions about repository initialization?\n\nThanks,\nPushkar\n"},{"id":"540943","messageId":"20260405064651.GA1452907@coredump.intra.peff.net","threadId":"65437","inReplyTo":"20260404172817.2995133-1-luca.stefani.ge1@gmail.com","subject":"Re: [PATCH] object-file: don't use object database without a repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-04-05T06:46:51Z","receivedAt":"2026-04-05T06:46:53Z","isPatch":true,"body":"On Sat, Apr 04, 2026 at 07:28:17PM +0200, Luca Stefani wrote:\n\n> When running `git diff -- $file1 $file2' on large enough files,\n> index_fd() attempts to use 'the_repository->objects', assuming it\n> is initialized, but that's not the case for non-repository usecases.\n> \n> When git diff is invoked without a backing repository,\n> INDEX_WRITE_OBJECT is never set in flags, meaning only the hash is\n> needed and nothing should be written to the object store.\n> \n> Enforce the use of index_core() in this case.\n\nI don't think we want to use index_core() for a large file, though. A\ntest like this:\n\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex 15076dfe0d..7ef5604430 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -413,4 +413,10 @@ test_expect_success 'diff --no-index with pathspec glob and exclude' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'diff --no-index on a huge file' '\n+\tdd if=/dev/zero bs=1M count=4000 >big.file &&\n+\techo whatever >small.file &&\n+\ttest_expect_code 1 git diff --no-index big.file small.file\n+'\n+\n test_done\n\n\nwill now fail on a 32-bit system, because we try to mmap the whole file,\nwhich will fail.  We really do want to follow the streaming code path\n(which knows to respect the lack of a WRITE_OBJECT flag and works\nwithout an odb in that case).\n\nIt's kind of an expensive test, though, so we probably don't want to\nactually include it in the test suite.\n\n-Peff\n\nPS I'd expect a 4GB+ file to work, too, but it looks like the diff code\n   barfs when trying to stuff the file into a diff_filespec. A simpler\n   example is:\n\n     dd if=/dev/zero bs=1G count=5 >big.file\n     git hash-object big.file\n\n   but that dies, too! It looks like the streaming helper uses a size_t\n   to take the size, which is wrong. It really should be an off_t. So I\n   dunno, maybe nobody cares about ever working with 4GB files on 32-bit\n   systems these days. It still feels like we should avoid a large mmap,\n   though.\n"},{"id":"540952","messageId":"145b6c7f-c037-4a87-b561-d2b4d8c5a0cd@gmail.com","threadId":"65437","inReplyTo":"20260405064651.GA1452907@coredump.intra.peff.net","subject":"Re: [PATCH] object-file: don't use object database without a repository","fromName":"Luca Stefani","fromEmail":"luca.stefani.ge1@gmail.com","sentAt":"2026-04-05T16:10:33Z","receivedAt":"2026-04-05T16:10:38Z","isPatch":true,"body":"\nOn 05/04/2026 08:46, Jeff King wrote:\n> On Sat, Apr 04, 2026 at 07:28:17PM +0200, Luca Stefani wrote:\n>\n>> When running `git diff -- $file1 $file2' on large enough files,\n>> index_fd() attempts to use 'the_repository->objects', assuming it\n>> is initialized, but that's not the case for non-repository usecases.\n>>\n>> When git diff is invoked without a backing repository,\n>> INDEX_WRITE_OBJECT is never set in flags, meaning only the hash is\n>> needed and nothing should be written to the object store.\n>>\n>> Enforce the use of index_core() in this case.\n> I don't think we want to use index_core() for a large file, though. A\n> test like this:\n\nI don't know what would be the right approach, index_core sure is slow, \nbut maybe that's expected for those sizes.\n\nThis fix by itself simply avoids entering into the broken case, and it \nstill gives me a working diff.\n\n> diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\n> index 15076dfe0d..7ef5604430 100755\n> --- a/t/t4053-diff-no-index.sh\n> +++ b/t/t4053-diff-no-index.sh\n> @@ -413,4 +413,10 @@ test_expect_success 'diff --no-index with pathspec glob and exclude' '\n>   \ttest_cmp expect actual\n>   '\n>   \n> +test_expect_success 'diff --no-index on a huge file' '\n> +\tdd if=/dev/zero bs=1M count=4000 >big.file &&\n> +\techo whatever >small.file &&\n> +\ttest_expect_code 1 git diff --no-index big.file small.file\n> +'\n> +\n>   test_done\n\nIf you want  I can send a V2 with that, but given it's your test suit \nI'd rather you handle it.\n\nEspecially when it comes to multi-arch, as I only really care about amd64\n\n>\n> will now fail on a 32-bit system, because we try to mmap the whole file,\n> which will fail.  We really do want to follow the streaming code path\n> (which knows to respect the lack of a WRITE_OBJECT flag and works\n> without an odb in that case).\n>\n> It's kind of an expensive test, though, so we probably don't want to\n> actually include it in the test suite.\n>\n> -Peff\n>\n> PS I'd expect a 4GB+ file to work, too, but it looks like the diff code\n>     barfs when trying to stuff the file into a diff_filespec. A simpler\n>     example is:\n>\n>       dd if=/dev/zero bs=1G count=5 >big.file\n>       git hash-object big.file\n>\n>     but that dies, too! It looks like the streaming helper uses a size_t\n>     to take the size, which is wrong. It really should be an off_t. So I\n>     dunno, maybe nobody cares about ever working with 4GB files on 32-bit\n>     systems these days. It still feels like we should avoid a large mmap,\n>     though.\n\nAh I I just happened to have a 3G file and I threw it at 'git diff' :)\n\nWith `#define _FILE_OFFSET_BITS 64` off_t is properly sized, but if \nother places downcast it then there's little hope on 32bit.\n\nNow even with that in mind not sure if it's worth fixing...\n"},{"id":"540955","messageId":"20260405191750.GA1525850@coredump.intra.peff.net","threadId":"65437","inReplyTo":"145b6c7f-c037-4a87-b561-d2b4d8c5a0cd@gmail.com","subject":"Re: [PATCH] object-file: don't use object database without a repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-04-05T19:17:50Z","receivedAt":"2026-04-05T19:17:52Z","isPatch":true,"body":"On Sun, Apr 05, 2026 at 06:10:33PM +0200, Luca Stefani wrote:\n\n> > > Enforce the use of index_core() in this case.\n> > I don't think we want to use index_core() for a large file, though. A\n> > test like this:\n> \n> I don't know what would be the right approach, index_core sure is slow, but\n> maybe that's expected for those sizes.\n\nIt's always going to be slow because we're hashing a lot of data. But\nthe point is that we should be streaming it, and trying to allocate a\nhuge buffer (even via mmap). So it's not that it's slow, it's that some\ncases which used to work will not do so any longer.\n\nWe want to keep going into the streaming code path, and not\nindex_core(). But the streaming code path has been broken outside of a\nrepo, by ce1661f9da (odb: add transaction interface, 2025-09-16) and its\nfollow-on patches. And we should fix that instead of avoiding it.\n\n> This fix by itself simply avoids entering into the broken case, and it\n> still gives me a working diff.\n\nFor some files, yes, but it breaks other cases (like the one I\ndemonstrated).\n\n> > diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\n> > index 15076dfe0d..7ef5604430 100755\n> > --- a/t/t4053-diff-no-index.sh\n> > +++ b/t/t4053-diff-no-index.sh\n> > @@ -413,4 +413,10 @@ test_expect_success 'diff --no-index with pathspec glob and exclude' '\n> >   \ttest_cmp expect actual\n> >   '\n> > +test_expect_success 'diff --no-index on a huge file' '\n> > +\tdd if=/dev/zero bs=1M count=4000 >big.file &&\n> > +\techo whatever >small.file &&\n> > +\ttest_expect_code 1 git diff --no-index big.file small.file\n> > +'\n> > +\n> >   test_done\n> \n> If you want  I can send a V2 with that, but given it's your test suit I'd\n> rather you handle it.\n\nIf you sent a v2 with this, the tests would not pass. ;)\n\nBut like I said, I don't think we want that in the test suite because it\nis too expensive to run. What we probably do want is a cheap\ndemonstration of the segfault, which is this:\n\ndiff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh\nindex c824c1a25c..31965908a6 100755\n--- a/t/t1517-outside-repo.sh\n+++ b/t/t1517-outside-repo.sh\n@@ -149,4 +149,9 @@ test_expect_success 'fmt-merge-msg does not crash with -h' '\n \ttest_grep \"[Uu]sage: git fmt-merge-msg \" usage\n '\n \n+test_expect_success 'indexing large file outside repo' '\n+\tnongit dd if=/dev/zero of=big.file bs=10k count=1 &&\n+\tnongit git -c core.bigfilethreshold=5k hash-object big.file\n+'\n+\n test_done\n\nBut I think the actual code change in your patch is the wrong thing, so\nI also don't think we'd want to just squash that test in. I'm hoping\nJustin has some insights on how to do a more complete fix.\n\n-Peff\n"},{"id":"541008","messageId":"adP0hnV7Gl08qqqf@denethor","threadId":"65437","inReplyTo":"20260405191750.GA1525850@coredump.intra.peff.net","subject":"Re: [PATCH] object-file: don't use object database without a repository","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-04-06T18:17:17Z","receivedAt":"2026-04-06T18:17:21Z","isPatch":true,"body":"On 26/04/05 03:17PM, Jeff King wrote:\n> But I think the actual code change in your patch is the wrong thing, so\n> I also don't think we'd want to just squash that test in. I'm hoping\n> Justin has some insights on how to do a more complete fix.\n\nI agree with Peff here that the correct fix should continue to use the\nobject streaming mechanisms. To avoid this segfault, we really should\navoid using ODB transactions when there isn't an ODB in the first place.\n\nI replied in another thread[1] with how we could go about fixing. To\nsummarize, it just so happens that I already have a patch[2] out on the\nlist that appears to resolve this issue.\n\nFor the use case here, git-diff(1) is only interested in generating the\nhash for the \"large\" blobs and not actually writing anything to the ODB.\nThis patch introduces a separate \"hash-only\" variant of\n`index_blob_packfile_transaction()` and is used to bypass creating an\nODB transaction when object writes are not needed.\n\nIf this is the route we want to go down, I can extract this patch from\nthe current series and send it as a separate fix. :)\n\n-Justin\n\n[1]: https://lore.kernel.org/git/adPjXKGIT5O7SK6E@denethor/T/#m9cee420941b66abfb0244ea4b7762ba8d0ff7b52\n[2]: https://lore.kernel.org/git/20260402213220.2651523-5-jltobler@gmail.com/\n"},{"id":"541015","messageId":"568cb40a-373e-4ad1-a6a0-fb7289da92e2@gmail.com","threadId":"65437","inReplyTo":"adP0hnV7Gl08qqqf@denethor","subject":"Re: [PATCH] object-file: don't use object database without a repository","fromName":"Luca Stefani","fromEmail":"luca.stefani.ge1@gmail.com","sentAt":"2026-04-06T19:31:39Z","receivedAt":"2026-04-06T19:31:44Z","isPatch":true,"body":"\nOn 06/04/2026 20:17, Justin Tobler wrote:\n> On 26/04/05 03:17PM, Jeff King wrote:\n>> But I think the actual code change in your patch is the wrong thing, so\n>> I also don't think we'd want to just squash that test in. I'm hoping\n>> Justin has some insights on how to do a more complete fix.\n> I agree with Peff here that the correct fix should continue to use the\n> object streaming mechanisms. To avoid this segfault, we really should\n> avoid using ODB transactions when there isn't an ODB in the first place.\n>\n> I replied in another thread[1] with how we could go about fixing. To\n> summarize, it just so happens that I already have a patch[2] out on the\n> list that appears to resolve this issue.\n\nThanks, just verified it works as expected.\n\n>\n> For the use case here, git-diff(1) is only interested in generating the\n> hash for the \"large\" blobs and not actually writing anything to the ODB.\n> This patch introduces a separate \"hash-only\" variant of\n> `index_blob_packfile_transaction()` and is used to bypass creating an\n> ODB transaction when object writes are not needed.\n>\n> If this is the route we want to go down, I can extract this patch from\n> the current series and send it as a separate fix. :)\nIf this ends up happening CC me and I'll gladly stamp it with Tested-by :)\n>\n> -Justin\n>\n> [1]: https://lore.kernel.org/git/adPjXKGIT5O7SK6E@denethor/T/#m9cee420941b66abfb0244ea4b7762ba8d0ff7b52\n> [2]: https://lore.kernel.org/git/20260402213220.2651523-5-jltobler@gmail.com/\n"},{"id":"541016","messageId":"20260406200651.GA26091@coredump.intra.peff.net","threadId":"65437","inReplyTo":"adP0hnV7Gl08qqqf@denethor","subject":"Re: [PATCH] object-file: don't use object database without a repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-04-06T20:06:51Z","receivedAt":"2026-04-06T20:06:58Z","isPatch":true,"body":"On Mon, Apr 06, 2026 at 01:17:17PM -0500, Justin Tobler wrote:\n\n> On 26/04/05 03:17PM, Jeff King wrote:\n> > But I think the actual code change in your patch is the wrong thing, so\n> > I also don't think we'd want to just squash that test in. I'm hoping\n> > Justin has some insights on how to do a more complete fix.\n> \n> I agree with Peff here that the correct fix should continue to use the\n> object streaming mechanisms. To avoid this segfault, we really should\n> avoid using ODB transactions when there isn't an ODB in the first place.\n> \n> I replied in another thread[1] with how we could go about fixing. To\n> summarize, it just so happens that I already have a patch[2] out on the\n> list that appears to resolve this issue.\n> \n> For the use case here, git-diff(1) is only interested in generating the\n> hash for the \"large\" blobs and not actually writing anything to the ODB.\n> This patch introduces a separate \"hash-only\" variant of\n> `index_blob_packfile_transaction()` and is used to bypass creating an\n> ODB transaction when object writes are not needed.\n> \n> If this is the route we want to go down, I can extract this patch from\n> the current series and send it as a separate fix. :)\n\nYeah, I think this is a good path forward. I took a look at making the\ntransaction begin/end conditional, but that's not nearly enough anymore.\nThe transaction object stores state which is used under the hood by\nindex_blob_packfile_transaction(). So we'd really need some kind of fake\nnoop transaction that understands how to stream.\n\nJust having the caller divert to a \"hash this without having an odb\"\ninterface is way simpler (especially since this is the only spot that\nneeds it, so we are only paying the price once either way).\n\nI gave a cursory look at the patch you linked. For a maint fix like this\nI think we could probably slim it down a bit: introduce the new\nhash-only helper but _don't_ actually rip flag support out of\nindex_blob_packfile_transaction(), so we know that we can't accidentally\nbreak it. Though maybe that is being overly cautious; it only has one\ncaller, and that caller would no longer be passing in any meaningful\nflags.\n\n-Peff\n"},{"id":"541019","messageId":"adQX82EuEbVhpf8r@denethor","threadId":"65437","inReplyTo":"568cb40a-373e-4ad1-a6a0-fb7289da92e2@gmail.com","subject":"Re: [PATCH] object-file: don't use object database without a repository","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-04-06T20:31:57Z","receivedAt":"2026-04-06T20:31:59Z","isPatch":true,"body":"On 26/04/06 09:31PM, Luca Stefani wrote:\n> \n> On 06/04/2026 20:17, Justin Tobler wrote:\n> > On 26/04/05 03:17PM, Jeff King wrote:\n> > > But I think the actual code change in your patch is the wrong thing, so\n> > > I also don't think we'd want to just squash that test in. I'm hoping\n> > > Justin has some insights on how to do a more complete fix.\n> > I agree with Peff here that the correct fix should continue to use the\n> > object streaming mechanisms. To avoid this segfault, we really should\n> > avoid using ODB transactions when there isn't an ODB in the first place.\n> > \n> > I replied in another thread[1] with how we could go about fixing. To\n> > summarize, it just so happens that I already have a patch[2] out on the\n> > list that appears to resolve this issue.\n> \n> Thanks, just verified it works as expected.\n\nThanks for testing! :)\n\n> > \n> > For the use case here, git-diff(1) is only interested in generating the\n> > hash for the \"large\" blobs and not actually writing anything to the ODB.\n> > This patch introduces a separate \"hash-only\" variant of\n> > `index_blob_packfile_transaction()` and is used to bypass creating an\n> > ODB transaction when object writes are not needed.\n> > \n> > If this is the route we want to go down, I can extract this patch from\n> > the current series and send it as a separate fix. :)\n> If this ends up happening CC me and I'll gladly stamp it with Tested-by :)\n\nWill do!\n\n-Justin\n"},{"id":"541020","messageId":"adQYS_ThpOzxCLTi@denethor","threadId":"65437","inReplyTo":"20260406200651.GA26091@coredump.intra.peff.net","subject":"Re: [PATCH] object-file: don't use object database without a repository","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-04-06T20:38:05Z","receivedAt":"2026-04-06T20:38:06Z","isPatch":true,"body":"On 26/04/06 04:06PM, Jeff King wrote:\n> On Mon, Apr 06, 2026 at 01:17:17PM -0500, Justin Tobler wrote:\n> \n> > I agree with Peff here that the correct fix should continue to use the\n> > object streaming mechanisms. To avoid this segfault, we really should\n> > avoid using ODB transactions when there isn't an ODB in the first place.\n> > \n> > I replied in another thread[1] with how we could go about fixing. To\n> > summarize, it just so happens that I already have a patch[2] out on the\n> > list that appears to resolve this issue.\n> > \n> > For the use case here, git-diff(1) is only interested in generating the\n> > hash for the \"large\" blobs and not actually writing anything to the ODB.\n> > This patch introduces a separate \"hash-only\" variant of\n> > `index_blob_packfile_transaction()` and is used to bypass creating an\n> > ODB transaction when object writes are not needed.\n> > \n> > If this is the route we want to go down, I can extract this patch from\n> > the current series and send it as a separate fix. :)\n> \n> Yeah, I think this is a good path forward. I took a look at making the\n> transaction begin/end conditional, but that's not nearly enough anymore.\n> The transaction object stores state which is used under the hood by\n> index_blob_packfile_transaction(). So we'd really need some kind of fake\n> noop transaction that understands how to stream.\n> \n> Just having the caller divert to a \"hash this without having an odb\"\n> interface is way simpler (especially since this is the only spot that\n> needs it, so we are only paying the price once either way).\n> \n> I gave a cursory look at the patch you linked. For a maint fix like this\n> I think we could probably slim it down a bit: introduce the new\n> hash-only helper but _don't_ actually rip flag support out of\n> index_blob_packfile_transaction(), so we know that we can't accidentally\n> break it. Though maybe that is being overly cautious; it only has one\n> caller, and that caller would no longer be passing in any meaningful\n> flags.\n\nYa, I think slimming down the patch probably makes sense. I'll start\nworking on it and make sure to include some tests too. :)\n\nThanks,\n-Justin\n"}]}