git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: git@vger.kernel.org
Cc: Patrick Steinhardt <ps@pks.im>,  Aaron Plattner <aplattner@nvidia.com>
Subject: [PATCH] odb: do not use "blank" substitute for NULL
Date: Thu, 18 Dec 2025 12:35:40 +0900	[thread overview]
Message-ID: <xmqqpl8cxy0j.fsf@gitster.g> (raw)

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


             reply	other threads:[~2025-12-18  3:35 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-12-18  3:35 Junio C Hamano [this message]
2025-12-18  4:51 ` [PATCH] odb: do not use "blank" substitute for NULL Aaron Plattner
2025-12-18  8:02   ` Kristoffer Haugsbakk
2025-12-18 10:59     ` Carlo Marcelo Arenas Belón
2025-12-19  7:39       ` Kristoffer Haugsbakk
2025-12-19 12:25         ` Junio C Hamano
2025-12-18  6:31 ` Patrick Steinhardt
2025-12-18  8:50 ` Patrick Steinhardt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqpl8cxy0j.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=aplattner@nvidia.com \
    --cc=git@vger.kernel.org \
    --cc=ps@pks.im \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).