{"thread":{"id":"64644","subject":"[PATCH] odb: do not use \"blank\" substitute for NULL","startedAt":"2025-12-18T03:35:42Z","lastAt":"2025-12-19T12:25:21Z","messageCount":8,"participants":["Junio C Hamano","Patrick Steinhardt","Aaron Plattner","Kristoffer Haugsbakk","Carlo Marcelo Arenas Belón"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"532407","messageId":"xmqqpl8cxy0j.fsf@gitster.g","threadId":"64644","inReplyTo":null,"subject":"[PATCH] odb: do not use \"blank\" substitute for NULL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-12-18T03:35:40Z","receivedAt":"2025-12-18T03:35:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When various *object_info() functions are given an extended object\ninfo structure as NULL by a caller that does not want any details,\nthe code uses a file-scope static blank_oi to pass it down to the\nhelper functions they use, to avoid handling NULL specifically.\n\nThe ps/object-read-stream topic graduated to 'master' recently\nhowever had a bug that assumed that two identically named file-scope\nstatic variables in two functions are the same, which of course is\nnot the case. This made \"git commit\" take 0.38 seconds to 1508\nseconds in some case, as reported by Aaron Plattner here:\n\n https://lore.kernel.org/git/f4ba7e89-4717-4b36-921f-56537131fd69@nvidia.com/\n\nWe _could_ move the blank_oi variable to a global scope in BSS to\nfix this regression, but explicitly handling the NULL is a much\nsafer fix. It would also reduce the chance of errors that somebody\naccidentally writes into blank_oi, making its contents dirty, which\npotentially will make subsequent calls into the callpath misbehave.\n\nBy explicitly handling NULL input, we no longer have to worry about\nit.\n\nReported-by: Aaron Plattner \nSigned-off-by: Junio C Hamano \n---\n object-file.c | 8 ++++----\n odb.c | 29 +++++++++++++----------------\n packfile.c | 3 +--\n 3 files changed, 18 insertions(+), 22 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 12177a7dd7..e0cce3a62a 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -426,7 +426,7 @@ int odb_source_loose_read_object_info(struct odb_source *source,\n \tunsigned long size_scratch;\n \tenum object_type type_scratch;\n \n-\tif (oi->delta_base_oid)\n+\tif (oi && oi->delta_base_oid)\n \t\toidclr(oi->delta_base_oid, source->odb->repo->hash_algo);\n \n \t/*\n@@ -437,13 +437,13 @@ int odb_source_loose_read_object_info(struct odb_source *source,\n \t * return value implicitly indicates whether the\n \t * object even exists.\n \t */\n-\tif (!oi->typep && !oi->sizep && !oi->contentp) {\n+\tif (!oi || (!oi->typep && !oi->sizep && !oi->contentp)) {\n \t\tstruct stat st;\n-\t\tif (!oi->disk_sizep && (flags & OBJECT_INFO_QUICK))\n+\t\tif ((!oi || !oi->disk_sizep) && (flags & OBJECT_INFO_QUICK))\n \t\t\treturn quick_has_loose(source->loose, oid) ? 0 : -1;\n \t\tif (stat_loose_object(source->loose, oid, &st, &path) < 0)\n \t\t\treturn -1;\n-\t\tif (oi->disk_sizep)\n+\t\tif (oi && oi->disk_sizep)\n \t\t\t*oi->disk_sizep = st.st_size;\n \t\treturn 0;\n \t}\ndiff --git a/odb.c b/odb.c\nindex f4cbee4b04..85dc21b104 100644\n--- a/odb.c\n+++ b/odb.c\n@@ -664,34 +664,31 @@ static int do_oid_object_info_extended(struct object_database *odb,\n \t\t\t\t const struct object_id *oid,\n \t\t\t\t struct object_info *oi, unsigned flags)\n {\n-\tstatic struct object_info blank_oi = OBJECT_INFO_INIT;\n \tconst struct cached_object *co;\n \tconst struct object_id *real = oid;\n \tint already_retried = 0;\n \n-\n \tif (flags & OBJECT_INFO_LOOKUP_REPLACE)\n \t\treal = lookup_replace_object(odb->repo, oid);\n \n \tif (is_null_oid(real))\n \t\treturn -1;\n \n-\tif (!oi)\n-\t\toi = &blank_oi;\n-\n \tco = find_cached_object(odb, real);\n \tif (co) {\n-\t\tif (oi->typep)\n-\t\t\t*(oi->typep) = co->type;\n-\t\tif (oi->sizep)\n-\t\t\t*(oi->sizep) = co->size;\n-\t\tif (oi->disk_sizep)\n-\t\t\t*(oi->disk_sizep) = 0;\n-\t\tif (oi->delta_base_oid)\n-\t\t\toidclr(oi->delta_base_oid, odb->repo->hash_algo);\n-\t\tif (oi->contentp)\n-\t\t\t*oi->contentp = xmemdupz(co->buf, co->size);\n-\t\toi->whence = OI_CACHED;\n+\t\tif (oi) {\n+\t\t\tif (oi->typep)\n+\t\t\t\t*(oi->typep) = co->type;\n+\t\t\tif (oi->sizep)\n+\t\t\t\t*(oi->sizep) = co->size;\n+\t\t\tif (oi->disk_sizep)\n+\t\t\t\t*(oi->disk_sizep) = 0;\n+\t\t\tif (oi->delta_base_oid)\n+\t\t\t\toidclr(oi->delta_base_oid, odb->repo->hash_algo);\n+\t\t\tif (oi->contentp)\n+\t\t\t\t*oi->contentp = xmemdupz(co->buf, co->size);\n+\t\t\toi->whence = OI_CACHED;\n+\t\t}\n \t\treturn 0;\n \t}\n \ndiff --git a/packfile.c b/packfile.c\nindex 7a16aaa90d..2aa6135c3a 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -2095,7 +2095,6 @@ int packfile_store_read_object_info(struct packfile_store *store,\n \t\t\t\t struct object_info *oi,\n \t\t\t\t unsigned flags UNUSED)\n {\n-\tstatic struct object_info blank_oi = OBJECT_INFO_INIT;\n \tstruct pack_entry e;\n \tint rtype;\n \n@@ -2106,7 +2105,7 @@ int packfile_store_read_object_info(struct packfile_store *store,\n \t * We know that the caller doesn't actually need the\n \t * information below, so return early.\n \t */\n-\tif (oi == &blank_oi)\n+\tif (!oi)\n \t\treturn 0;\n \n \trtype = packed_object_info(store->odb->repo, e.p, e.offset, oi);\n-- \n2.52.0-448-g904c30f108\n\n"},{"id":"532422","messageId":"aUOfxNGdkJe8ARM1@pks.im","threadId":"64644","inReplyTo":"xmqqpl8cxy0j.fsf@gitster.g","subject":"Re: [PATCH] odb: do not use \"blank\" substitute for NULL","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-18T06:31:32Z","receivedAt":"2025-12-18T06:31:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 18, 2025 at 12:35:40PM +0900, Junio C Hamano wrote:\n> When various *object_info() functions are given an extended object\n> info structure as NULL by a caller that does not want any details,\n> the code uses a file-scope static blank_oi to pass it down to the\n> helper functions they use, to avoid handling NULL specifically.\n> \n> The ps/object-read-stream topic graduated to 'master' recently\n> however had a bug that assumed that two identically named file-scope\n> static variables in two functions are the same, which of course is\n> not the case. This made \"git commit\" take 0.38 seconds to 1508\n> seconds in some case, as reported by Aaron Plattner here:\n> \n> https://lore.kernel.org/git/f4ba7e89-4717-4b36-921f-56537131fd69@nvidia.com/\n> \n> We _could_ move the blank_oi variable to a global scope in BSS to\n> fix this regression, but explicitly handling the NULL is a much\n> safer fix. It would also reduce the chance of errors that somebody\n> accidentally writes into blank_oi, making its contents dirty, which\n> potentially will make subsequent calls into the callpath misbehave.\n> \n> By explicitly handling NULL input, we no longer have to worry about\n> it.\n\nThanks for handling this, Junio!\n\nI've sent out an alternative fix via a patch series that I already had\ncooking locally in [1]. It also goes a bit further than your series as\nit also recognizes the case where the caller passes a blank object info\nagain.\n\nThat series also contains some other fixes related to reading object\ninfo where we had been inconsistent with returned results, and another\nperformance improvement where we can skip unpacking packed objects.\n\nI'm happy to go either route though -- I can hold off my series for a\nbit longer and rebase it on top of your fix, or we replace your fix with\nmy series. Just let me know your preference.\n\nThanks!\n\nPatrick\n\n[1]: <20251218-b4-pks-odb-read-object-info-improvements-v1-7-81c8368492be@pks.im>\n"},{"id":"532439","messageId":"a31e054e-0eb2-48b9-a802-3592a737d1e3@nvidia.com","threadId":"64644","inReplyTo":"xmqqpl8cxy0j.fsf@gitster.g","subject":"Re: [PATCH] odb: do not use \"blank\" substitute for NULL","fromName":"Aaron Plattner","fromEmail":"aplattner@nvidia.com","sentAt":"2025-12-18T04:51:43Z","receivedAt":"2025-12-18T07:25:20Z","isPatch":true,"sender":{"key":"aplattner@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/343551?v=4"},"body":"On 12/17/25 7:35 PM, Junio C Hamano wrote:\n> When various *object_info() functions are given an extended object\n> info structure as NULL by a caller that does not want any details,\n> the code uses a file-scope static blank_oi to pass it down to the\n> helper functions they use, to avoid handling NULL specifically.\n> \n> The ps/object-read-stream topic graduated to 'master' recently\n> however had a bug that assumed that two identically named file-scope\n> static variables in two functions are the same, which of course is\n> not the case. This made \"git commit\" take 0.38 seconds to 1508\n> seconds in some case, as reported by Aaron Plattner here:\n> \n> https://lore.kernel.org/git/f4ba7e89-4717-4b36-921f-56537131fd69@nvidia.com/\n> \n> We _could_ move the blank_oi variable to a global scope in BSS to\n> fix this regression, but explicitly handling the NULL is a much\n> safer fix. It would also reduce the chance of errors that somebody\n> accidentally writes into blank_oi, making its contents dirty, which\n> potentially will make subsequent calls into the callpath misbehave.\n> \n> By explicitly handling NULL input, we no longer have to worry about\n> it.\n\nThis reasoning makes sense to me.\n\nWould it make sense to add a\n\nFixes: 385e18810f10 (\"packfile: introduce function to read object info \nfrom a store\")\n\nline?\n\n> Reported-by: Aaron Plattner \n> Signed-off-by: Junio C Hamano \n> ---\n> object-file.c | 8 ++++----\n> odb.c | 29 +++++++++++++----------------\n> packfile.c | 3 +--\n> 3 files changed, 18 insertions(+), 22 deletions(-)\n> \n> diff --git a/object-file.c b/object-file.c\n> index 12177a7dd7..e0cce3a62a 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -426,7 +426,7 @@ int odb_source_loose_read_object_info(struct odb_source *source,\n> \tunsigned long size_scratch;\n> \tenum object_type type_scratch;\n> \n> -\tif (oi->delta_base_oid)\n> +\tif (oi && oi->delta_base_oid)\n> \t\toidclr(oi->delta_base_oid, source->odb->repo->hash_algo);\n> \n> \t/*\n> @@ -437,13 +437,13 @@ int odb_source_loose_read_object_info(struct odb_source *source,\n> \t * return value implicitly indicates whether the\n> \t * object even exists.\n> \t */\n> -\tif (!oi->typep && !oi->sizep && !oi->contentp) {\n> +\tif (!oi || (!oi->typep && !oi->sizep && !oi->contentp)) {\n> \t\tstruct stat st;\n> -\t\tif (!oi->disk_sizep && (flags & OBJECT_INFO_QUICK))\n> +\t\tif ((!oi || !oi->disk_sizep) && (flags & OBJECT_INFO_QUICK))\n> \t\t\treturn quick_has_loose(source->loose, oid) ? 0 : -1;\n> \t\tif (stat_loose_object(source->loose, oid, &st, &path) < 0)\n> \t\t\treturn -1;\n> -\t\tif (oi->disk_sizep)\n> +\t\tif (oi && oi->disk_sizep)\n> \t\t\t*oi->disk_sizep = st.st_size;\n> \t\treturn 0;\n> \t}\n> diff --git a/odb.c b/odb.c\n> index f4cbee4b04..85dc21b104 100644\n> --- a/odb.c\n> +++ b/odb.c\n> @@ -664,34 +664,31 @@ static int do_oid_object_info_extended(struct object_database *odb,\n> \t\t\t\t const struct object_id *oid,\n> \t\t\t\t struct object_info *oi, unsigned flags)\n> {\n> -\tstatic struct object_info blank_oi = OBJECT_INFO_INIT;\n> \tconst struct cached_object *co;\n> \tconst struct object_id *real = oid;\n> \tint already_retried = 0;\n> \n> -\n> \tif (flags & OBJECT_INFO_LOOKUP_REPLACE)\n> \t\treal = lookup_replace_object(odb->repo, oid);\n> \n> \tif (is_null_oid(real))\n> \t\treturn -1;\n> \n> -\tif (!oi)\n> -\t\toi = &blank_oi;\n> -\n> \tco = find_cached_object(odb, real);\n> \tif (co) {\n> -\t\tif (oi->typep)\n> -\t\t\t*(oi->typep) = co->type;\n> -\t\tif (oi->sizep)\n> -\t\t\t*(oi->sizep) = co->size;\n> -\t\tif (oi->disk_sizep)\n> -\t\t\t*(oi->disk_sizep) = 0;\n> -\t\tif (oi->delta_base_oid)\n> -\t\t\toidclr(oi->delta_base_oid, odb->repo->hash_algo);\n> -\t\tif (oi->contentp)\n> -\t\t\t*oi->contentp = xmemdupz(co->buf, co->size);\n> -\t\toi->whence = OI_CACHED;\n> +\t\tif (oi) {\n> +\t\t\tif (oi->typep)\n> +\t\t\t\t*(oi->typep) = co->type;\n> +\t\t\tif (oi->sizep)\n> +\t\t\t\t*(oi->sizep) = co->size;\n> +\t\t\tif (oi->disk_sizep)\n> +\t\t\t\t*(oi->disk_sizep) = 0;\n> +\t\t\tif (oi->delta_base_oid)\n> +\t\t\t\toidclr(oi->delta_base_oid, odb->repo->hash_algo);\n> +\t\t\tif (oi->contentp)\n> +\t\t\t\t*oi->contentp = xmemdupz(co->buf, co->size);\n> +\t\t\toi->whence = OI_CACHED;\n> +\t\t}\n> \t\treturn 0;\n> \t}\n> \n> diff --git a/packfile.c b/packfile.c\n> index 7a16aaa90d..2aa6135c3a 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -2095,7 +2095,6 @@ int packfile_store_read_object_info(struct packfile_store *store,\n> \t\t\t\t struct object_info *oi,\n> \t\t\t\t unsigned flags UNUSED)\n> {\n> -\tstatic struct object_info blank_oi = OBJECT_INFO_INIT;\n> \tstruct pack_entry e;\n> \tint rtype;\n> \n> @@ -2106,7 +2105,7 @@ int packfile_store_read_object_info(struct packfile_store *store,\n> \t * We know that the caller doesn't actually need the\n> \t * information below, so return early.\n> \t */\n> -\tif (oi == &blank_oi)\n> +\tif (!oi)\n> \t\treturn 0;\n> \n> \trtype = packed_object_info(store->odb->repo, e.p, e.offset, oi);\n\nThis looks good to me and I verified it restores the original \nperformance, so,\n\nTested-by: Aaron Plattner \nReviewed-by: Aaron Plattner \n\nThanks!\n\n-- Aaron\n"},{"id":"532441","messageId":"0e860421-8f8c-4bf9-8ad8-82fe269a7a9d@app.fastmail.com","threadId":"64644","inReplyTo":"a31e054e-0eb2-48b9-a802-3592a737d1e3@nvidia.com","subject":"Re: [PATCH] odb: do not use \"blank\" substitute for NULL","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-12-18T08:02:59Z","receivedAt":"2025-12-18T08:03:22Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Thu, Dec 18, 2025, at 05:51, Aaron Plattner wrote:\n>>[snip]\n>> By explicitly handling NULL input, we no longer have to worry about\n>> it.\n>\n> This reasoning makes sense to me.\n>\n> Would it make sense to add a\n>\n> Fixes: 385e18810f10 (\"packfile: introduce function to read object info\n> from a store\")\n>\n> line?\n\nThis project typically does not use that trailer/tag. Only trailers that\nattribute people are recommended. There are exceptions, like some\nrecent usages of\n\n Best-viewed-with: