Re: [PATCH] odb: do not use "blank" substitute for NULL
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Dec 18, 2025, 08:50 UTC
- Message-ID
- <aUPASOMyxIJjYwTj@pks.im>
- In-Reply-To
- <xmqqpl8cxy0j.fsf@gitster.g>
On Thu, Dec 18, 2025 at 12:35:40PM +0900, Junio C Hamano wrote:
Show 30 quoted lines
> 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.
Show 51 quoted lines
> 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.
Show 11 quoted lines
> 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>