[PATCH] odb: do not use "blank" substitute for NULL

[PATCH] odb: do not use "blank" substitute for NULL

[Junio C Hamano]

When various *object_info() functions are given an extended object
info structure as NULL by a caller that does not want any details,
the code uses a file-scope static blank_oi to pass it down to the
helper functions they use, to avoid handling NULL specifically.

The ps/object-read-stream topic graduated to 'master' recently
however had a bug that assumed that two identically named file-scope
static variables in two functions are the same, which of course is
not the case.  This made "git commit" take 0.38 seconds to 1508
seconds in some case, as reported by Aaron Plattner here:

  https://lore.kernel.org/git/f4ba7e89-4717-4b36-921f-56537131fd69@nvidia.com/

We _could_ move the blank_oi variable to a global scope in BSS to
fix this regression, but explicitly handling the NULL is a much
safer fix.  It would also reduce the chance of errors that somebody
accidentally writes into blank_oi, making its contents dirty, which
potentially will make subsequent calls into the callpath misbehave.

By explicitly handling NULL input, we no longer have to worry about
it.

Reported-by: Aaron Plattner <aplattner@nvidia.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 object-file.c |  8 ++++----
 odb.c         | 29 +++++++++++++----------------
 packfile.c    |  3 +--
 3 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/object-file.c b/object-file.c
index 12177a7dd7..e0cce3a62a 100644
--- a/object-file.c
+++ b/object-file.c
@@ -426,7 +426,7 @@ int odb_source_loose_read_object_info(struct odb_source *source,
 	unsigned long size_scratch;
 	enum object_type type_scratch;
 
-	if (oi->delta_base_oid)
+	if (oi && oi->delta_base_oid)
 		oidclr(oi->delta_base_oid, source->odb->repo->hash_algo);
 
 	/*
@@ -437,13 +437,13 @@ int odb_source_loose_read_object_info(struct odb_source *source,
 	 * return value implicitly indicates whether the
 	 * object even exists.
 	 */
-	if (!oi->typep && !oi->sizep && !oi->contentp) {
+	if (!oi || (!oi->typep && !oi->sizep && !oi->contentp)) {
 		struct stat st;
-		if (!oi->disk_sizep && (flags & OBJECT_INFO_QUICK))
+		if ((!oi || !oi->disk_sizep) && (flags & OBJECT_INFO_QUICK))
 			return quick_has_loose(source->loose, oid) ? 0 : -1;
 		if (stat_loose_object(source->loose, oid, &st, &path) < 0)
 			return -1;
-		if (oi->disk_sizep)
+		if (oi && oi->disk_sizep)
 			*oi->disk_sizep = st.st_size;
 		return 0;
 	}
diff --git a/odb.c b/odb.c
index f4cbee4b04..85dc21b104 100644
--- a/odb.c
+++ b/odb.c
@@ -664,34 +664,31 @@ static int do_oid_object_info_extended(struct object_database *odb,
 				       const struct object_id *oid,
 				       struct object_info *oi, unsigned flags)
 {
-	static struct object_info blank_oi = OBJECT_INFO_INIT;
 	const struct cached_object *co;
 	const struct object_id *real = oid;
 	int already_retried = 0;
 
-
 	if (flags & OBJECT_INFO_LOOKUP_REPLACE)
 		real = lookup_replace_object(odb->repo, oid);
 
 	if (is_null_oid(real))
 		return -1;
 
-	if (!oi)
-		oi = &blank_oi;
-
 	co = find_cached_object(odb, real);
 	if (co) {
-		if (oi->typep)
-			*(oi->typep) = co->type;
-		if (oi->sizep)
-			*(oi->sizep) = co->size;
-		if (oi->disk_sizep)
-			*(oi->disk_sizep) = 0;
-		if (oi->delta_base_oid)
-			oidclr(oi->delta_base_oid, odb->repo->hash_algo);
-		if (oi->contentp)
-			*oi->contentp = xmemdupz(co->buf, co->size);
-		oi->whence = OI_CACHED;
+		if (oi) {
+			if (oi->typep)
+				*(oi->typep) = co->type;
+			if (oi->sizep)
+				*(oi->sizep) = co->size;
+			if (oi->disk_sizep)
+				*(oi->disk_sizep) = 0;
+			if (oi->delta_base_oid)
+				oidclr(oi->delta_base_oid, odb->repo->hash_algo);
+			if (oi->contentp)
+				*oi->contentp = xmemdupz(co->buf, co->size);
+			oi->whence = OI_CACHED;
+		}
 		return 0;
 	}
 
diff --git a/packfile.c b/packfile.c
index 7a16aaa90d..2aa6135c3a 100644
--- a/packfile.c
+++ b/packfile.c
@@ -2095,7 +2095,6 @@ int packfile_store_read_object_info(struct packfile_store *store,
 				    struct object_info *oi,
 				    unsigned flags UNUSED)
 {
-	static struct object_info blank_oi = OBJECT_INFO_INIT;
 	struct pack_entry e;
 	int rtype;
 
@@ -2106,7 +2105,7 @@ int packfile_store_read_object_info(struct packfile_store *store,
 	 * We know that the caller doesn't actually need the
 	 * information below, so return early.
 	 */
-	if (oi == &blank_oi)
+	if (!oi)
 		return 0;
 
 	rtype = packed_object_info(store->odb->repo, e.p, e.offset, oi);
-- 
2.52.0-448-g904c30f108

Re: [PATCH] odb: do not use "blank" substitute for NULL

[Aaron Plattner]

On 12/17/25 7:35 PM, Junio C Hamano wrote:
> When various *object_info() functions are given an extended object
> info structure as NULL by a caller that does not want any details,
> the code uses a file-scope static blank_oi to pass it down to the
> helper functions they use, to avoid handling NULL specifically.
> 
> The ps/object-read-stream topic graduated to 'master' recently
> however had a bug that assumed that two identically named file-scope
> static variables in two functions are the same, which of course is
> not the case.  This made "git commit" take 0.38 seconds to 1508
> seconds in some case, as reported by Aaron Plattner here:
> 
>    https://lore.kernel.org/git/f4ba7e89-4717-4b36-921f-56537131fd69@nvidia.com/
> 
> We _could_ move the blank_oi variable to a global scope in BSS to
> fix this regression, but explicitly handling the NULL is a much
> safer fix.  It would also reduce the chance of errors that somebody
> accidentally writes into blank_oi, making its contents dirty, which
> potentially will make subsequent calls into the callpath misbehave.
> 
> By explicitly handling NULL input, we no longer have to worry about
> it.

This reasoning makes sense to me.

Would it make sense to add a

Fixes: 385e18810f10 ("packfile: introduce function to read object info 
from a store")

line?

> Reported-by: Aaron Plattner <aplattner@nvidia.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>   object-file.c |  8 ++++----
>   odb.c         | 29 +++++++++++++----------------
>   packfile.c    |  3 +--
>   3 files changed, 18 insertions(+), 22 deletions(-)
> 
> diff --git a/object-file.c b/object-file.c
> index 12177a7dd7..e0cce3a62a 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -426,7 +426,7 @@ int odb_source_loose_read_object_info(struct odb_source *source,
>   	unsigned long size_scratch;
>   	enum object_type type_scratch;
>   
> -	if (oi->delta_base_oid)
> +	if (oi && oi->delta_base_oid)
>   		oidclr(oi->delta_base_oid, source->odb->repo->hash_algo);
>   
>   	/*
> @@ -437,13 +437,13 @@ int odb_source_loose_read_object_info(struct odb_source *source,
>   	 * return value implicitly indicates whether the
>   	 * object even exists.
>   	 */
> -	if (!oi->typep && !oi->sizep && !oi->contentp) {
> +	if (!oi || (!oi->typep && !oi->sizep && !oi->contentp)) {
>   		struct stat st;
> -		if (!oi->disk_sizep && (flags & OBJECT_INFO_QUICK))
> +		if ((!oi || !oi->disk_sizep) && (flags & OBJECT_INFO_QUICK))
>   			return quick_has_loose(source->loose, oid) ? 0 : -1;
>   		if (stat_loose_object(source->loose, oid, &st, &path) < 0)
>   			return -1;
> -		if (oi->disk_sizep)
> +		if (oi && oi->disk_sizep)
>   			*oi->disk_sizep = st.st_size;
>   		return 0;
>   	}
> diff --git a/odb.c b/odb.c
> index f4cbee4b04..85dc21b104 100644
> --- a/odb.c
> +++ b/odb.c
> @@ -664,34 +664,31 @@ static int do_oid_object_info_extended(struct object_database *odb,
>   				       const struct object_id *oid,
>   				       struct object_info *oi, unsigned flags)
>   {
> -	static struct object_info blank_oi = OBJECT_INFO_INIT;
>   	const struct cached_object *co;
>   	const struct object_id *real = oid;
>   	int already_retried = 0;
>   
> -
>   	if (flags & OBJECT_INFO_LOOKUP_REPLACE)
>   		real = lookup_replace_object(odb->repo, oid);
>   
>   	if (is_null_oid(real))
>   		return -1;
>   
> -	if (!oi)
> -		oi = &blank_oi;
> -
>   	co = find_cached_object(odb, real);
>   	if (co) {
> -		if (oi->typep)
> -			*(oi->typep) = co->type;
> -		if (oi->sizep)
> -			*(oi->sizep) = co->size;
> -		if (oi->disk_sizep)
> -			*(oi->disk_sizep) = 0;
> -		if (oi->delta_base_oid)
> -			oidclr(oi->delta_base_oid, odb->repo->hash_algo);
> -		if (oi->contentp)
> -			*oi->contentp = xmemdupz(co->buf, co->size);
> -		oi->whence = OI_CACHED;
> +		if (oi) {
> +			if (oi->typep)
> +				*(oi->typep) = co->type;
> +			if (oi->sizep)
> +				*(oi->sizep) = co->size;
> +			if (oi->disk_sizep)
> +				*(oi->disk_sizep) = 0;
> +			if (oi->delta_base_oid)
> +				oidclr(oi->delta_base_oid, odb->repo->hash_algo);
> +			if (oi->contentp)
> +				*oi->contentp = xmemdupz(co->buf, co->size);
> +			oi->whence = OI_CACHED;
> +		}
>   		return 0;
>   	}
>   
> diff --git a/packfile.c b/packfile.c
> index 7a16aaa90d..2aa6135c3a 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -2095,7 +2095,6 @@ int packfile_store_read_object_info(struct packfile_store *store,
>   				    struct object_info *oi,
>   				    unsigned flags UNUSED)
>   {
> -	static struct object_info blank_oi = OBJECT_INFO_INIT;
>   	struct pack_entry e;
>   	int rtype;
>   
> @@ -2106,7 +2105,7 @@ int packfile_store_read_object_info(struct packfile_store *store,
>   	 * We know that the caller doesn't actually need the
>   	 * information below, so return early.
>   	 */
> -	if (oi == &blank_oi)
> +	if (!oi)
>   		return 0;
>   
>   	rtype = packed_object_info(store->odb->repo, e.p, e.offset, oi);

This looks good to me and I verified it restores the original 
performance, so,

Tested-by: Aaron Plattner <aplattner@nvidia.com>
Reviewed-by: Aaron Plattner <aplattner@nvidia.com>

Thanks!

-- Aaron

Re: [PATCH] odb: do not use "blank" substitute for NULL

[Patrick Steinhardt]

On Thu, Dec 18, 2025 at 12:35:40PM +0900, Junio C Hamano wrote:
> When various *object_info() functions are given an extended object
> info structure as NULL by a caller that does not want any details,
> the code uses a file-scope static blank_oi to pass it down to the
> helper functions they use, to avoid handling NULL specifically.
> 
> The ps/object-read-stream topic graduated to 'master' recently
> however had a bug that assumed that two identically named file-scope
> static variables in two functions are the same, which of course is
> not the case.  This made "git commit" take 0.38 seconds to 1508
> seconds in some case, as reported by Aaron Plattner here:
> 
>   https://lore.kernel.org/git/f4ba7e89-4717-4b36-921f-56537131fd69@nvidia.com/
> 
> We _could_ move the blank_oi variable to a global scope in BSS to
> fix this regression, but explicitly handling the NULL is a much
> safer fix.  It would also reduce the chance of errors that somebody
> accidentally writes into blank_oi, making its contents dirty, which
> potentially will make subsequent calls into the callpath misbehave.
> 
> By explicitly handling NULL input, we no longer have to worry about
> it.

Thanks for handling this, Junio!

I've sent out an alternative fix via a patch series that I already had
cooking locally in [1]. It also goes a bit further than your series as
it also recognizes the case where the caller passes a blank object info
again.

That series also contains some other fixes related to reading object
info where we had been inconsistent with returned results, and another
performance improvement where we can skip unpacking packed objects.

I'm happy to go either route though -- I can hold off my series for a
bit longer and rebase it on top of your fix, or we replace your fix with
my series. Just let me know your preference.

Thanks!

Patrick

[1]: <20251218-b4-pks-odb-read-object-info-improvements-v1-7-81c8368492be@pks.im>

Re: [PATCH] odb: do not use "blank" substitute for NULL

[Kristoffer Haugsbakk]

On Thu, Dec 18, 2025, at 05:51, Aaron Plattner wrote:
>>[snip]
>> By explicitly handling NULL input, we no longer have to worry about
>> it.
>
> This reasoning makes sense to me.
>
> Would it make sense to add a
>
> Fixes: 385e18810f10 ("packfile: introduce function to read object info
> from a store")
>
> line?

This project typically does not use that trailer/tag. Only trailers that
attribute people are recommended. There are exceptions, like some
recent usages of

    Best-viewed-with: <option to git-log(1)/git-show(1)>

If a commit fixes some other commit it might be referenced somewhere in
the message text.

Commits are referenced with:[1]

     git show -s --pretty=reference <commit>

The maintainer uses `--abbrev=8` (simplified):[2]

    git show --date=short -s --abbrev=8 --pretty='format:%h (%s, %ad)' "$1"

† 1: Documentation/SubmittingPatches
[2]: https://lore.kernel.org/git/xmqq34j5h7v9.fsf@gitster.g/

>[snip]

Re: [PATCH] odb: do not use "blank" substitute for NULL

[Patrick Steinhardt]

On Thu, Dec 18, 2025 at 12:35:40PM +0900, Junio C Hamano wrote:
> diff --git a/object-file.c b/object-file.c
> index 12177a7dd7..e0cce3a62a 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -426,7 +426,7 @@ int odb_source_loose_read_object_info(struct odb_source *source,
>  	unsigned long size_scratch;
>  	enum object_type type_scratch;
>  
> -	if (oi->delta_base_oid)
> +	if (oi && oi->delta_base_oid)
>  		oidclr(oi->delta_base_oid, source->odb->repo->hash_algo);
>  
>  	/*
> @@ -437,13 +437,13 @@ int odb_source_loose_read_object_info(struct odb_source *source,
>  	 * return value implicitly indicates whether the
>  	 * object even exists.
>  	 */
> -	if (!oi->typep && !oi->sizep && !oi->contentp) {
> +	if (!oi || (!oi->typep && !oi->sizep && !oi->contentp)) {
>  		struct stat st;
> -		if (!oi->disk_sizep && (flags & OBJECT_INFO_QUICK))
> +		if ((!oi || !oi->disk_sizep) && (flags & OBJECT_INFO_QUICK))
>  			return quick_has_loose(source->loose, oid) ? 0 : -1;
>  		if (stat_loose_object(source->loose, oid, &st, &path) < 0)
>  			return -1;
> -		if (oi->disk_sizep)
> +		if (oi && oi->disk_sizep)
>  			*oi->disk_sizep = st.st_size;
>  		return 0;
>  	}

Okay, here we know to exit early in case `oi == NULL`. So any subsequent
code can assume that `oi` is non-NULL. Good.

> diff --git a/odb.c b/odb.c
> index f4cbee4b04..85dc21b104 100644
> --- a/odb.c
> +++ b/odb.c
> @@ -664,34 +664,31 @@ static int do_oid_object_info_extended(struct object_database *odb,
>  				       const struct object_id *oid,
>  				       struct object_info *oi, unsigned flags)
>  {
> -	static struct object_info blank_oi = OBJECT_INFO_INIT;
>  	const struct cached_object *co;
>  	const struct object_id *real = oid;
>  	int already_retried = 0;
>  
> -
>  	if (flags & OBJECT_INFO_LOOKUP_REPLACE)
>  		real = lookup_replace_object(odb->repo, oid);
>  
>  	if (is_null_oid(real))
>  		return -1;
>  
> -	if (!oi)
> -		oi = &blank_oi;
> -
>  	co = find_cached_object(odb, real);
>  	if (co) {
> -		if (oi->typep)
> -			*(oi->typep) = co->type;
> -		if (oi->sizep)
> -			*(oi->sizep) = co->size;
> -		if (oi->disk_sizep)
> -			*(oi->disk_sizep) = 0;
> -		if (oi->delta_base_oid)
> -			oidclr(oi->delta_base_oid, odb->repo->hash_algo);
> -		if (oi->contentp)
> -			*oi->contentp = xmemdupz(co->buf, co->size);
> -		oi->whence = OI_CACHED;
> +		if (oi) {
> +			if (oi->typep)
> +				*(oi->typep) = co->type;
> +			if (oi->sizep)
> +				*(oi->sizep) = co->size;
> +			if (oi->disk_sizep)
> +				*(oi->disk_sizep) = 0;
> +			if (oi->delta_base_oid)
> +				oidclr(oi->delta_base_oid, odb->repo->hash_algo);
> +			if (oi->contentp)
> +				*oi->contentp = xmemdupz(co->buf, co->size);
> +			oi->whence = OI_CACHED;
> +		}
>  		return 0;
>  	}

Looks reasonable. We pass down `oi` to both the loose backend and the
packfile store, but you teach both of them to handle this alright.

> diff --git a/packfile.c b/packfile.c
> index 7a16aaa90d..2aa6135c3a 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -2106,7 +2105,7 @@ int packfile_store_read_object_info(struct packfile_store *store,
>  	 * We know that the caller doesn't actually need the
>  	 * information below, so return early.
>  	 */
> -	if (oi == &blank_oi)
> +	if (!oi)
>  		return 0;

And this here is fixing the actual performance regression.

All of this looks as expected to me, so let's merge this patch down
fastish. I'll rebase my bigger patch series at [1] on top of your patch.

Thanks!

Patrick

[1]: <20251218-b4-pks-odb-read-object-info-improvements-v1-0-81c8368492be@pks.im>

Re: [PATCH] odb: do not use "blank" substitute for NULL

[Carlo Marcelo Arenas Belón]

On Thu, Dec 18, 2025 at 09:02:59AM -0800, Kristoffer Haugsbakk wrote:
> On Thu, Dec 18, 2025, at 05:51, Aaron Plattner wrote:
> >>[snip]
> >> By explicitly handling NULL input, we no longer have to worry about
> >> it.
> >
> > This reasoning makes sense to me.
> >
> > Would it make sense to add a
> >
> > Fixes: 385e18810f10 ("packfile: introduce function to read object info
> > from a store")
> >
> > line?
> 
> This project typically does not use that trailer/tag.

While factually correct, I think the "why" is more interesting in this case.
anf the answer IMHO is: not, because it is not needed.

% git describe 385e18810f10 
v2.52.0-25-g385e18810f

shows that this bug is only present after 2.52.0 was released so unless you
are using unreleased version of git (ex: some development version, including
ones that are based on "next"), there is no need to "backport" this fix, as
the next version you will use will include it.

Carlo


 Only trailers that
> attribute people are recommended. There are exceptions, like some
> recent usages of
> 
>     Best-viewed-with: <option to git-log(1)/git-show(1)>
> 
> If a commit fixes some other commit it might be referenced somewhere in
> the message text.
> 
> Commits are referenced with:[1]
> 
>      git show -s --pretty=reference <commit>
> 
> The maintainer uses `--abbrev=8` (simplified):[2]
> 
>     git show --date=short -s --abbrev=8 --pretty='format:%h (%s, %ad)' "$1"
> 
> † 1: Documentation/SubmittingPatches
> [2]: https://lore.kernel.org/git/xmqq34j5h7v9.fsf@gitster.g/
> 
> >[snip]

Re: [PATCH] odb: do not use "blank" substitute for NULL

[Kristoffer Haugsbakk]

On Thu, Dec 18, 2025, at 11:59, Carlo Marcelo Arenas Belón wrote:
> On Thu, Dec 18, 2025 at 09:02:59AM -0800, Kristoffer Haugsbakk wrote:
>>[snip]
>>
>> This project typically does not use that trailer/tag.
>
> While factually correct, I think the "why" is more interesting in this case.
> anf the answer IMHO is: not, because it is not needed.
>
> % git describe 385e18810f10
> v2.52.0-25-g385e18810f
>
> shows that this bug is only present after 2.52.0 was released so unless you
> are using unreleased version of git (ex: some development version, including
> ones that are based on "next"), there is no need to "backport" this fix, as
> the next version you will use will include it.

So the Linux Kernel (presumably) uses `Fixes` for backporting and/or
does *not* use it for commits that fix changes that have not been
released yet. Got it.

Re: [PATCH] odb: do not use "blank" substitute for NULL

[Junio C Hamano]

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:

> On Thu, Dec 18, 2025, at 11:59, Carlo Marcelo Arenas Belón wrote:
>> On Thu, Dec 18, 2025 at 09:02:59AM -0800, Kristoffer Haugsbakk wrote:
>>>[snip]
>>>
>>> This project typically does not use that trailer/tag.
>>
>> While factually correct, I think the "why" is more interesting in this case.
>> anf the answer IMHO is: not, because it is not needed.
>>
>> % git describe 385e18810f10
>> v2.52.0-25-g385e18810f
>>
>> shows that this bug is only present after 2.52.0 was released so unless you
>> are using unreleased version of git (ex: some development version, including
>> ones that are based on "next"), there is no need to "backport" this fix, as
>> the next version you will use will include it.
>
> So the Linux Kernel (presumably) uses `Fixes` for backporting and/or
> does *not* use it for commits that fix changes that have not been
> released yet. Got it.

I do not run, and I am not involved in, the Linux Kernel project.  I
am not sure if "is this fix something backporting folks should care
about?" is the criterion they use in their project, but if it is, I
think it does make a certain sense.

I have mentioned my displeasure with use of "Fixes" in _this_
project before, but that was primarily based on the fact that you do
not really know if a proposed commit really fixes or makes something
else worse until your alleged "fix" cooks sufficiently long in the
field, and I find it distasteful to make such an unsure thing easier
to mechanically process.

Thanks.