From: Aaron Plattner Date: Thu, 18 Dec 2025 04:51:43 GMT Subject: Re: [PATCH] odb: do not use "blank" substitute for NULL Message-ID: In-Reply-To: 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 > Signed-off-by: Junio C Hamano > --- > 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 Reviewed-by: Aaron Plattner Thanks! -- Aaron