[PATCH 0/4] Don't lazy-fetch commits when parsing them

[PATCH 0/4] Don't lazy-fetch commits when parsing them

[Jonathan Tan]

This is a follow-up from my previous email about the possibility of not
fetching when we know that we're fetching a commit [1]. I had to refactor a few
things mostly due to replace objects, so the number of patches might be larger
than you would expect. I tried to keep each patch small and easy to understand,
though.

Patches 1-3 contain some forward-compatibility and refactoring changes, and
patch 4 contains the actual logic change.

[1] https://lore.kernel.org/git/20221124004205.1777255-1-jonathantanmy@google.com/

Jonathan Tan (4):
  object-file: reread object with exact same args
  object-file: refactor corrupt object diagnosis
  object-file: refactor replace object lookup
  commit: don't lazy-fetch commits

 commit.c       | 18 ++++++++++++++--
 object-file.c  | 57 ++++++++++++++++++++++++++++++++++----------------
 object-store.h | 10 +++++++++
 3 files changed, 65 insertions(+), 20 deletions(-)

-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 1/4] object-file: reread object with exact same args

[Jonathan Tan]

When an object in do_oid_object_info_extended() is found in a packfile,
but corrupt, that packfile entry is marked as bad and the read is
retried. Currently, this is done by invoking the function again but with
the replace target of the object and with no flags.

This currently works, but will be clumsy when a later patch modifies
this function to also return the "real" object being read (that is, the
replace target). It does not make sense to pass a pointer in order to
receive this information when no replace lookups are requested, which is
exactly what the reinvocation does.

Therefore, change this reinvocation to pass exactly the arguments which
were originally passed. This also makes us forwards compatible with
future flags that may change the behavior of this function. This does
slow down the case when packfile corruption is detected, but that is
expected to be a very rare case.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/object-file.c b/object-file.c
index 26290554bb..1cde477267 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1621,7 +1621,7 @@ static int do_oid_object_info_extended(struct repository *r,
 	rtype = packed_object_info(r, e.p, e.offset, oi);
 	if (rtype < 0) {
 		mark_bad_packed_object(e.p, real);
-		return do_oid_object_info_extended(r, real, oi, 0);
+		return do_oid_object_info_extended(r, oid, oi, flags);
 	} else if (oi->whence == OI_PACKED) {
 		oi->u.packed.offset = e.offset;
 		oi->u.packed.pack = e.p;
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 2/4] object-file: refactor corrupt object diagnosis

[Jonathan Tan]

This functionality will be used from another file in a subsequent patch,
so refactor it into a public function.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-file.c  | 29 ++++++++++++++++++-----------
 object-store.h |  9 +++++++++
 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/object-file.c b/object-file.c
index 1cde477267..37468bc256 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1705,9 +1705,6 @@ void *read_object_file_extended(struct repository *r,
 				int lookup_replace)
 {
 	void *data;
-	const struct packed_git *p;
-	const char *path;
-	struct stat st;
 	const struct object_id *repl = lookup_replace ?
 		lookup_replace_object(r, oid) : oid;
 
@@ -1715,26 +1712,36 @@ void *read_object_file_extended(struct repository *r,
 	data = read_object(r, repl, type, size);
 	if (data)
 		return data;
+	die_if_corrupt(r, oid, repl);
+
+	return NULL;
+}
+
+void die_if_corrupt(struct repository *r,
+		    const struct object_id *oid,
+		    const struct object_id *real_oid)
+{
+	const struct packed_git *p;
+	const char *path;
+	struct stat st;
 
 	obj_read_lock();
 	if (errno && errno != ENOENT)
 		die_errno(_("failed to read object %s"), oid_to_hex(oid));
 
 	/* die if we replaced an object with one that does not exist */
-	if (repl != oid)
+	if (real_oid != oid)
 		die(_("replacement %s not found for %s"),
-		    oid_to_hex(repl), oid_to_hex(oid));
+		    oid_to_hex(real_oid), oid_to_hex(oid));
 
-	if (!stat_loose_object(r, repl, &st, &path))
+	if (!stat_loose_object(r, real_oid, &st, &path))
 		die(_("loose object %s (stored in %s) is corrupt"),
-		    oid_to_hex(repl), path);
+		    oid_to_hex(real_oid), path);
 
-	if ((p = has_packed_and_bad(r, repl)))
+	if ((p = has_packed_and_bad(r, real_oid)))
 		die(_("packed object %s (stored in %s) is corrupt"),
-		    oid_to_hex(repl), p->pack_name);
+		    oid_to_hex(real_oid), p->pack_name);
 	obj_read_unlock();
-
-	return NULL;
 }
 
 void *read_object_with_reference(struct repository *r,
diff --git a/object-store.h b/object-store.h
index 1be57abaf1..88c879c61e 100644
--- a/object-store.h
+++ b/object-store.h
@@ -256,6 +256,15 @@ static inline void *repo_read_object_file(struct repository *r,
 #define read_object_file(oid, type, size) repo_read_object_file(the_repository, oid, type, size)
 #endif
 
+/*
+ * Dies if real_oid is corrupt, not just missing.
+ *
+ * real_oid should be an oid that could not be read.
+ */
+void die_if_corrupt(struct repository *r,
+		    const struct object_id *oid,
+		    const struct object_id *real_oid);
+
 /* Read and unpack an object file into memory, write memory to an object file */
 int oid_object_info(struct repository *r, const struct object_id *, unsigned long *);
 
-- 
2.38.1.584.g0f3c55d4c2-goog

Re: [PATCH 2/4] object-file: refactor corrupt object diagnosis

[Jeff King]

On Wed, Nov 30, 2022 at 12:30:47PM -0800, Jonathan Tan wrote:

> +void die_if_corrupt(struct repository *r,
> +		    const struct object_id *oid,
> +		    const struct object_id *real_oid)
> +{
> +	const struct packed_git *p;
> +	const char *path;
> +	struct stat st;
>  
>  	obj_read_lock();
>  	if (errno && errno != ENOENT)
>  		die_errno(_("failed to read object %s"), oid_to_hex(oid));
>  
>  	/* die if we replaced an object with one that does not exist */
> -	if (repl != oid)
> +	if (real_oid != oid)
>  		die(_("replacement %s not found for %s"),
> -		    oid_to_hex(repl), oid_to_hex(oid));
> +		    oid_to_hex(real_oid), oid_to_hex(oid));

This kind of pointer comparison is a little subtle. Within a single
function, as this code was before this patch, it's probably OK to assume
that we use pointer indirection, and a non-replaced object will use the
original pointer. But for a public function, it seems like a gotcha
that:

  oidcpy(&real_oid, lookup_replace_object(r, &oid));
  die_if_corrupt(r, &oid, &real_oid);

would produce the wrong answer (it would think replacement happened even
if it didn't).

So maybe:

  if (!oideq(real_oid, oid))

instead? It's a little slower, but the point of this is to diagnose and
die, so it's not exactly a hot path. :)

-Peff

Re: [PATCH 2/4] object-file: refactor corrupt object diagnosis

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So maybe:
>
>   if (!oideq(real_oid, oid))
>
> instead? It's a little slower, but the point of this is to diagnose and
> die, so it's not exactly a hot path. :)

Very true, including the part that the original is fine because it
is localized and fairly obvious.  For a public function, we cannot
assume any additional constraints between oid and real_oid (other
than they are of the same "struct object_id *" type) like the two
pointers prepared locally in the original had, and use of oideq()
would be more appropriate here.

Thanks.

Re: [PATCH 2/4] object-file: refactor corrupt object diagnosis

[Jonathan Tan]

Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
> 
> > So maybe:
> >
> >   if (!oideq(real_oid, oid))
> >
> > instead? It's a little slower, but the point of this is to diagnose and
> > die, so it's not exactly a hot path. :)
> 
> Very true, including the part that the original is fine because it
> is localized and fairly obvious.  For a public function, we cannot
> assume any additional constraints between oid and real_oid (other
> than they are of the same "struct object_id *" type) like the two
> pointers prepared locally in the original had, and use of oideq()
> would be more appropriate here.
> 
> Thanks.

Makes sense. I'll make the change.

[PATCH 3/4] object-file: refactor replace object lookup

[Jonathan Tan]

Move the replace object lookup (specifically, the ability for the caller
to know the result of the lookup) from read_object_file_extended()
to one of the functions that it indirectly calls,
do_oid_object_info_extended(), because a subsequent patch will need that
ability from the latter.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-file.c  | 28 +++++++++++++++++++++-------
 object-store.h |  1 +
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/object-file.c b/object-file.c
index 37468bc256..fd394f1ace 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1546,6 +1546,11 @@ static int do_oid_object_info_extended(struct repository *r,
 
 	if (flags & OBJECT_INFO_LOOKUP_REPLACE)
 		real = lookup_replace_object(r, oid);
+	if (oi && oi->real_oidp) {
+		if (!(flags & OBJECT_INFO_LOOKUP_REPLACE))
+			BUG("specifying real_oidp does not make sense without OBJECT_INFO_LOOKUP_REPLACE");
+		*oi->real_oidp = real;
+	}
 
 	if (is_null_oid(real))
 		return -1;
@@ -1659,17 +1664,27 @@ int oid_object_info(struct repository *r,
 	return type;
 }
 
+/*
+ * If real_oid is not NULL, check if oid has a replace object and store the
+ * object that we end up using there.
+ */
 static void *read_object(struct repository *r,
 			 const struct object_id *oid, enum object_type *type,
-			 unsigned long *size)
+			 unsigned long *size, const struct object_id **real_oid)
 {
 	struct object_info oi = OBJECT_INFO_INIT;
 	void *content;
+	unsigned int flags = 0;
 	oi.typep = type;
 	oi.sizep = size;
 	oi.contentp = &content;
 
-	if (oid_object_info_extended(r, oid, &oi, 0) < 0)
+	if (real_oid) {
+		flags |= OBJECT_INFO_LOOKUP_REPLACE;
+		oi.real_oidp = real_oid;
+	}
+
+	if (oid_object_info_extended(r, oid, &oi, flags) < 0)
 		return NULL;
 	return content;
 }
@@ -1705,14 +1720,13 @@ void *read_object_file_extended(struct repository *r,
 				int lookup_replace)
 {
 	void *data;
-	const struct object_id *repl = lookup_replace ?
-		lookup_replace_object(r, oid) : oid;
+	const struct object_id *real_oid;
 
 	errno = 0;
-	data = read_object(r, repl, type, size);
+	data = read_object(r, oid, type, size, &real_oid);
 	if (data)
 		return data;
-	die_if_corrupt(r, oid, repl);
+	die_if_corrupt(r, oid, real_oid);
 
 	return NULL;
 }
@@ -2283,7 +2297,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime)
 
 	if (has_loose_object(oid))
 		return 0;
-	buf = read_object(the_repository, oid, &type, &len);
+	buf = read_object(the_repository, oid, &type, &len, NULL);
 	if (!buf)
 		return error(_("cannot read object for %s"), oid_to_hex(oid));
 	hdrlen = format_object_header(hdr, sizeof(hdr), type, len);
diff --git a/object-store.h b/object-store.h
index 88c879c61e..9684562eb2 100644
--- a/object-store.h
+++ b/object-store.h
@@ -406,6 +406,7 @@ struct object_info {
 	struct object_id *delta_base_oid;
 	struct strbuf *type_name;
 	void **contentp;
+	const struct object_id **real_oidp;
 
 	/* Response */
 	enum {
-- 
2.38.1.584.g0f3c55d4c2-goog

Re: [PATCH 3/4] object-file: refactor replace object lookup

[Jeff King]

On Wed, Nov 30, 2022 at 12:30:48PM -0800, Jonathan Tan wrote:

> diff --git a/object-store.h b/object-store.h
> index 88c879c61e..9684562eb2 100644
> --- a/object-store.h
> +++ b/object-store.h
> @@ -406,6 +406,7 @@ struct object_info {
>  	struct object_id *delta_base_oid;
>  	struct strbuf *type_name;
>  	void **contentp;
> +	const struct object_id **real_oidp;

OK. The double-pointer here is a bit funky as an interface. It may point
back to the "oid" we fed the function, or it may point to long-term
storage owned by the replace mechanism. A more straightforward one would
be to store a single-pointer to caller-owned storage, and copy to that
(like we do for delta_base_oid, for example).

But doing it this way avoids extra oid copies in the normal,
non-replaced case, and matches how the current callers view things. So
while it's a little convoluted, I think it makes sense to do it as you
did.

-Peff

[PATCH 4/4] commit: don't lazy-fetch commits

[Jonathan Tan]

When parsing commits, fail fast when the commit is missing or
corrupt, instead of attempting to fetch them. This is done by inlining
repo_read_object_file() and setting the flag that prevents fetching.

This is motivated by a situation in which through a bug (not necessarily
through Git), there was corruption in the object store of a partial
clone. In this particular case, the problem was exposed when "git gc"
tried to expire reflogs, which calls repo_parse_commit(), which triggers
fetches of the missing commits.

(There are other possible solutions to this problem including passing an
argument from "git gc" to "git reflog" to inhibit all lazy fetches, but
I think that this fix is at the wrong level - fixing "git reflog" means
that this particular command works fine, or so we think (it will fail if
it somehow needs to read a legitimately missing blob, say, a .gitmodules
file), but fixing repo_parse_commit() will fix a whole class of bugs.)

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 commit.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/commit.c b/commit.c
index 572301b80a..17e71f5be4 100644
--- a/commit.c
+++ b/commit.c
@@ -508,6 +508,13 @@ int repo_parse_commit_internal(struct repository *r,
 	enum object_type type;
 	void *buffer;
 	unsigned long size;
+	const struct object_id *real_oid;
+	struct object_info oi = {
+		.typep = &type,
+		.sizep = &size,
+		.contentp = &buffer,
+		.real_oidp = &real_oid,
+	};
 	int ret;
 
 	if (!item)
@@ -516,11 +523,18 @@ int repo_parse_commit_internal(struct repository *r,
 		return 0;
 	if (use_commit_graph && parse_commit_in_graph(r, item))
 		return 0;
-	buffer = repo_read_object_file(r, &item->object.oid, &type, &size);
-	if (!buffer)
+
+	/*
+	 * Git does not support partial clones that exclude commits, so set
+	 * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.
+	 */
+	if (oid_object_info_extended(r, &item->object.oid, &oi,
+	    OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT) < 0) {
+		die_if_corrupt(r, &item->object.oid, real_oid);
 		return quiet_on_missing ? -1 :
 			error("Could not read %s",
 			     oid_to_hex(&item->object.oid));
+	}
 	if (type != OBJ_COMMIT) {
 		free(buffer);
 		return error("Object %s not a commit",
-- 
2.38.1.584.g0f3c55d4c2-goog

Re: [PATCH 4/4] commit: don't lazy-fetch commits

[Jeff King]

On Wed, Nov 30, 2022 at 12:30:49PM -0800, Jonathan Tan wrote:

> When parsing commits, fail fast when the commit is missing or
> corrupt, instead of attempting to fetch them. This is done by inlining
> repo_read_object_file() and setting the flag that prevents fetching.
> 
> This is motivated by a situation in which through a bug (not necessarily
> through Git), there was corruption in the object store of a partial
> clone. In this particular case, the problem was exposed when "git gc"
> tried to expire reflogs, which calls repo_parse_commit(), which triggers
> fetches of the missing commits.

Makes sense.

> diff --git a/commit.c b/commit.c
> index 572301b80a..17e71f5be4 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -508,6 +508,13 @@ int repo_parse_commit_internal(struct repository *r,
>  	enum object_type type;
>  	void *buffer;
>  	unsigned long size;
> +	const struct object_id *real_oid;
> +	struct object_info oi = {
> +		.typep = &type,
> +		.sizep = &size,
> +		.contentp = &buffer,
> +		.real_oidp = &real_oid,
> +	};
>  	int ret;
>  
>  	if (!item)
> @@ -516,11 +523,18 @@ int repo_parse_commit_internal(struct repository *r,
>  		return 0;
>  	if (use_commit_graph && parse_commit_in_graph(r, item))
>  		return 0;
> -	buffer = repo_read_object_file(r, &item->object.oid, &type, &size);
> -	if (!buffer)
> +
> +	/*
> +	 * Git does not support partial clones that exclude commits, so set
> +	 * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.
> +	 */
> +	if (oid_object_info_extended(r, &item->object.oid, &oi,
> +	    OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT) < 0) {
> +		die_if_corrupt(r, &item->object.oid, real_oid);
>  		return quiet_on_missing ? -1 :
>  			error("Could not read %s",
>  			     oid_to_hex(&item->object.oid));
> +	}

OK, so we know we want a commit object because we're in the
commit-parsing function, so we just ask to disable fetching.

Two devil's advocate thoughts:

  1. What if we're wrong that it's a commit? If somebody references a
     blob in a commit "parent" header, the "right" outcome is for us to
     say "oops, the type is wrong" when we try to parse it as a commit.
     But now in a partial clone, we might avoid fetching that supposed
     commit and say "you don't have this object", even though we could
     get it.

     I think I'm OK with that. Either way, the repo is corrupt, and
     we'll have informed the user of that. The fact that this bizarre
     and specific sequence of corruptions might not go as far as it can
     to deduce the root cause is probably fine.

  2. Are there other places where we'd want to do the same thing? E.g.,
     in parse_object() we might ask for an object (not knowing its type)
     only to find out that it is a commit. But we have no idea if we
     lazy-fetched it or not!

     I had somehow imagined your series would be hooking in at the level
     of the lazy-fetch code, and complaining about fetching commits. But
     that may be tricky to do, because we really don't know the type
     until after we fetch it, and selectively removing an object from
     the odb is quite hard. We'd probably have to tell the other side
     "please, don't send me any commits", which requires a protocol
     extension, and...yuck.

     By comparison, your approach is an easy win that may catch problems
     in practice (and is certainly better than the status quo).

-Peff

Re: [PATCH 4/4] commit: don't lazy-fetch commits

[Jonathan Tan]

Jeff King <peff@peff.net> writes:
> OK, so we know we want a commit object because we're in the
> commit-parsing function, so we just ask to disable fetching.
> 
> Two devil's advocate thoughts:
> 
>   1. What if we're wrong that it's a commit? If somebody references a
>      blob in a commit "parent" header, the "right" outcome is for us to
>      say "oops, the type is wrong" when we try to parse it as a commit.
>      But now in a partial clone, we might avoid fetching that supposed
>      commit and say "you don't have this object", even though we could
>      get it.
> 
>      I think I'm OK with that. Either way, the repo is corrupt, and
>      we'll have informed the user of that. The fact that this bizarre
>      and specific sequence of corruptions might not go as far as it can
>      to deduce the root cause is probably fine.
> 
>   2. Are there other places where we'd want to do the same thing? E.g.,
>      in parse_object() we might ask for an object (not knowing its type)
>      only to find out that it is a commit. But we have no idea if we
>      lazy-fetched it or not!
> 
>      I had somehow imagined your series would be hooking in at the level
>      of the lazy-fetch code, and complaining about fetching commits. But
>      that may be tricky to do, because we really don't know the type
>      until after we fetch it, and selectively removing an object from
>      the odb is quite hard. We'd probably have to tell the other side
>      "please, don't send me any commits", which requires a protocol
>      extension, and...yuck.
> 
>      By comparison, your approach is an easy win that may catch problems
>      in practice (and is certainly better than the status quo).
> 
> -Peff

Thanks for taking a look. Let me know if you think that the commit message
could be improved to cover these cases. Right now I think that e.g. "When
parsing an object believed to be a commit in repo_parse_commit_internal()"
instead of "When parsing commits" wouldn't add much value, but I might be
missing something.

Re: [PATCH 4/4] commit: don't lazy-fetch commits

[Jeff King]

On Thu, Dec 01, 2022 at 11:11:50AM -0800, Jonathan Tan wrote:

> Jeff King <peff@peff.net> writes:
> > OK, so we know we want a commit object because we're in the
> > commit-parsing function, so we just ask to disable fetching.
> > 
> > Two devil's advocate thoughts:
> [...]
> 
> Thanks for taking a look. Let me know if you think that the commit message
> could be improved to cover these cases. Right now I think that e.g. "When
> parsing an object believed to be a commit in repo_parse_commit_internal()"
> instead of "When parsing commits" wouldn't add much value, but I might be
> missing something.

I think your commit message is OK as-is. I was mostly just laying out my
thoughts in reviewing. Some of that could go into the commit message as
notes, but I think it is sufficient that they're here in the list
archive.

-Peff

Re: [PATCH 4/4] commit: don't lazy-fetch commits

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> When parsing commits, fail fast when the commit is missing or
> corrupt, instead of attempting to fetch them. This is done by
> inlining repo_read_object_file() and setting the flag that
> prevents fetching.
>
> This is motivated by a situation in which through a bug (not
> necessarily through Git), there was corruption in the object store
> of a partial clone. In this particular case, the problem was
> exposed when "git gc" tried to expire reflogs, which calls
> repo_parse_commit(), which triggers fetches of the missing
> commits.

The assumption is that there will never be a filtering mode that
says "give us tags and we will lazy-fetch everything reachable from
it when we need it", with which the "solution" will break down, I
think, and I would say it is probably good enough.

Re: [PATCH 0/4] Don't lazy-fetch commits when parsing them

[Jeff King]

On Wed, Nov 30, 2022 at 12:30:45PM -0800, Jonathan Tan wrote:

> This is a follow-up from my previous email about the possibility of not
> fetching when we know that we're fetching a commit [1]. I had to refactor a few
> things mostly due to replace objects, so the number of patches might be larger
> than you would expect. I tried to keep each patch small and easy to understand,
> though.
> 
> Patches 1-3 contain some forward-compatibility and refactoring changes, and
> patch 4 contains the actual logic change.

These look pretty good to me. I raised a minor nit in patch 2; if you
agree it should be a trivial re-roll.

I left some thoughts on the approach in patch 4, but I think given that
this is a strict improvement over the status quo, it's a good step
forward, even if it won't catch all such cases.

-Peff

[PATCH v2 0/4] Don't lazy-fetch commits when parsing them

[Jonathan Tan]

Thanks everyone for your reviews. Here is a reroll with the requested change
(just one small one).

Jonathan Tan (4):
  object-file: reread object with exact same args
  object-file: refactor corrupt object diagnosis
  object-file: refactor replace object lookup
  commit: don't lazy-fetch commits

 commit.c       | 18 ++++++++++++++--
 object-file.c  | 57 ++++++++++++++++++++++++++++++++++----------------
 object-store.h | 10 +++++++++
 3 files changed, 65 insertions(+), 20 deletions(-)

Range-diff against v1:
1:  604160e79c = 1:  604160e79c object-file: reread object with exact same args
2:  a8c5fcd9f8 ! 2:  1be60f1bf2 object-file: refactor corrupt object diagnosis
    @@ object-file.c: void *read_object_file_extended(struct repository *r,
      
      	/* die if we replaced an object with one that does not exist */
     -	if (repl != oid)
    -+	if (real_oid != oid)
    ++	if (!oideq(real_oid, oid))
      		die(_("replacement %s not found for %s"),
     -		    oid_to_hex(repl), oid_to_hex(oid));
     +		    oid_to_hex(real_oid), oid_to_hex(oid));
3:  940396307f = 3:  28935ba1b0 object-file: refactor replace object lookup
4:  6af8dcebd1 = 4:  a38229c42a commit: don't lazy-fetch commits
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

[PATCH v2 1/4] object-file: reread object with exact same args

[Jonathan Tan]

When an object in do_oid_object_info_extended() is found in a packfile,
but corrupt, that packfile entry is marked as bad and the read is
retried. Currently, this is done by invoking the function again but with
the replace target of the object and with no flags.

This currently works, but will be clumsy when a later patch modifies
this function to also return the "real" object being read (that is, the
replace target). It does not make sense to pass a pointer in order to
receive this information when no replace lookups are requested, which is
exactly what the reinvocation does.

Therefore, change this reinvocation to pass exactly the arguments which
were originally passed. This also makes us forwards compatible with
future flags that may change the behavior of this function. This does
slow down the case when packfile corruption is detected, but that is
expected to be a very rare case.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/object-file.c b/object-file.c
index 26290554bb..1cde477267 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1621,7 +1621,7 @@ static int do_oid_object_info_extended(struct repository *r,
 	rtype = packed_object_info(r, e.p, e.offset, oi);
 	if (rtype < 0) {
 		mark_bad_packed_object(e.p, real);
-		return do_oid_object_info_extended(r, real, oi, 0);
+		return do_oid_object_info_extended(r, oid, oi, flags);
 	} else if (oi->whence == OI_PACKED) {
 		oi->u.packed.offset = e.offset;
 		oi->u.packed.pack = e.p;
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

[PATCH v2 2/4] object-file: refactor corrupt object diagnosis

[Jonathan Tan]

This functionality will be used from another file in a subsequent patch,
so refactor it into a public function.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-file.c  | 29 ++++++++++++++++++-----------
 object-store.h |  9 +++++++++
 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/object-file.c b/object-file.c
index 1cde477267..36f81c7958 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1705,9 +1705,6 @@ void *read_object_file_extended(struct repository *r,
 				int lookup_replace)
 {
 	void *data;
-	const struct packed_git *p;
-	const char *path;
-	struct stat st;
 	const struct object_id *repl = lookup_replace ?
 		lookup_replace_object(r, oid) : oid;
 
@@ -1715,26 +1712,36 @@ void *read_object_file_extended(struct repository *r,
 	data = read_object(r, repl, type, size);
 	if (data)
 		return data;
+	die_if_corrupt(r, oid, repl);
+
+	return NULL;
+}
+
+void die_if_corrupt(struct repository *r,
+		    const struct object_id *oid,
+		    const struct object_id *real_oid)
+{
+	const struct packed_git *p;
+	const char *path;
+	struct stat st;
 
 	obj_read_lock();
 	if (errno && errno != ENOENT)
 		die_errno(_("failed to read object %s"), oid_to_hex(oid));
 
 	/* die if we replaced an object with one that does not exist */
-	if (repl != oid)
+	if (!oideq(real_oid, oid))
 		die(_("replacement %s not found for %s"),
-		    oid_to_hex(repl), oid_to_hex(oid));
+		    oid_to_hex(real_oid), oid_to_hex(oid));
 
-	if (!stat_loose_object(r, repl, &st, &path))
+	if (!stat_loose_object(r, real_oid, &st, &path))
 		die(_("loose object %s (stored in %s) is corrupt"),
-		    oid_to_hex(repl), path);
+		    oid_to_hex(real_oid), path);
 
-	if ((p = has_packed_and_bad(r, repl)))
+	if ((p = has_packed_and_bad(r, real_oid)))
 		die(_("packed object %s (stored in %s) is corrupt"),
-		    oid_to_hex(repl), p->pack_name);
+		    oid_to_hex(real_oid), p->pack_name);
 	obj_read_unlock();
-
-	return NULL;
 }
 
 void *read_object_with_reference(struct repository *r,
diff --git a/object-store.h b/object-store.h
index 1be57abaf1..88c879c61e 100644
--- a/object-store.h
+++ b/object-store.h
@@ -256,6 +256,15 @@ static inline void *repo_read_object_file(struct repository *r,
 #define read_object_file(oid, type, size) repo_read_object_file(the_repository, oid, type, size)
 #endif
 
+/*
+ * Dies if real_oid is corrupt, not just missing.
+ *
+ * real_oid should be an oid that could not be read.
+ */
+void die_if_corrupt(struct repository *r,
+		    const struct object_id *oid,
+		    const struct object_id *real_oid);
+
 /* Read and unpack an object file into memory, write memory to an object file */
 int oid_object_info(struct repository *r, const struct object_id *, unsigned long *);
 
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

[PATCH v2 3/4] object-file: refactor replace object lookup

[Jonathan Tan]

Move the replace object lookup (specifically, the ability for the caller
to know the result of the lookup) from read_object_file_extended()
to one of the functions that it indirectly calls,
do_oid_object_info_extended(), because a subsequent patch will need that
ability from the latter.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-file.c  | 28 +++++++++++++++++++++-------
 object-store.h |  1 +
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/object-file.c b/object-file.c
index 36f81c7958..8adef99a7c 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1546,6 +1546,11 @@ static int do_oid_object_info_extended(struct repository *r,
 
 	if (flags & OBJECT_INFO_LOOKUP_REPLACE)
 		real = lookup_replace_object(r, oid);
+	if (oi && oi->real_oidp) {
+		if (!(flags & OBJECT_INFO_LOOKUP_REPLACE))
+			BUG("specifying real_oidp does not make sense without OBJECT_INFO_LOOKUP_REPLACE");
+		*oi->real_oidp = real;
+	}
 
 	if (is_null_oid(real))
 		return -1;
@@ -1659,17 +1664,27 @@ int oid_object_info(struct repository *r,
 	return type;
 }
 
+/*
+ * If real_oid is not NULL, check if oid has a replace object and store the
+ * object that we end up using there.
+ */
 static void *read_object(struct repository *r,
 			 const struct object_id *oid, enum object_type *type,
-			 unsigned long *size)
+			 unsigned long *size, const struct object_id **real_oid)
 {
 	struct object_info oi = OBJECT_INFO_INIT;
 	void *content;
+	unsigned int flags = 0;
 	oi.typep = type;
 	oi.sizep = size;
 	oi.contentp = &content;
 
-	if (oid_object_info_extended(r, oid, &oi, 0) < 0)
+	if (real_oid) {
+		flags |= OBJECT_INFO_LOOKUP_REPLACE;
+		oi.real_oidp = real_oid;
+	}
+
+	if (oid_object_info_extended(r, oid, &oi, flags) < 0)
 		return NULL;
 	return content;
 }
@@ -1705,14 +1720,13 @@ void *read_object_file_extended(struct repository *r,
 				int lookup_replace)
 {
 	void *data;
-	const struct object_id *repl = lookup_replace ?
-		lookup_replace_object(r, oid) : oid;
+	const struct object_id *real_oid;
 
 	errno = 0;
-	data = read_object(r, repl, type, size);
+	data = read_object(r, oid, type, size, &real_oid);
 	if (data)
 		return data;
-	die_if_corrupt(r, oid, repl);
+	die_if_corrupt(r, oid, real_oid);
 
 	return NULL;
 }
@@ -2283,7 +2297,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime)
 
 	if (has_loose_object(oid))
 		return 0;
-	buf = read_object(the_repository, oid, &type, &len);
+	buf = read_object(the_repository, oid, &type, &len, NULL);
 	if (!buf)
 		return error(_("cannot read object for %s"), oid_to_hex(oid));
 	hdrlen = format_object_header(hdr, sizeof(hdr), type, len);
diff --git a/object-store.h b/object-store.h
index 88c879c61e..9684562eb2 100644
--- a/object-store.h
+++ b/object-store.h
@@ -406,6 +406,7 @@ struct object_info {
 	struct object_id *delta_base_oid;
 	struct strbuf *type_name;
 	void **contentp;
+	const struct object_id **real_oidp;
 
 	/* Response */
 	enum {
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

[PATCH v2 4/4] commit: don't lazy-fetch commits

[Jonathan Tan]

When parsing commits, fail fast when the commit is missing or
corrupt, instead of attempting to fetch them. This is done by inlining
repo_read_object_file() and setting the flag that prevents fetching.

This is motivated by a situation in which through a bug (not necessarily
through Git), there was corruption in the object store of a partial
clone. In this particular case, the problem was exposed when "git gc"
tried to expire reflogs, which calls repo_parse_commit(), which triggers
fetches of the missing commits.

(There are other possible solutions to this problem including passing an
argument from "git gc" to "git reflog" to inhibit all lazy fetches, but
I think that this fix is at the wrong level - fixing "git reflog" means
that this particular command works fine, or so we think (it will fail if
it somehow needs to read a legitimately missing blob, say, a .gitmodules
file), but fixing repo_parse_commit() will fix a whole class of bugs.)

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 commit.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/commit.c b/commit.c
index 572301b80a..17e71f5be4 100644
--- a/commit.c
+++ b/commit.c
@@ -508,6 +508,13 @@ int repo_parse_commit_internal(struct repository *r,
 	enum object_type type;
 	void *buffer;
 	unsigned long size;
+	const struct object_id *real_oid;
+	struct object_info oi = {
+		.typep = &type,
+		.sizep = &size,
+		.contentp = &buffer,
+		.real_oidp = &real_oid,
+	};
 	int ret;
 
 	if (!item)
@@ -516,11 +523,18 @@ int repo_parse_commit_internal(struct repository *r,
 		return 0;
 	if (use_commit_graph && parse_commit_in_graph(r, item))
 		return 0;
-	buffer = repo_read_object_file(r, &item->object.oid, &type, &size);
-	if (!buffer)
+
+	/*
+	 * Git does not support partial clones that exclude commits, so set
+	 * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.
+	 */
+	if (oid_object_info_extended(r, &item->object.oid, &oi,
+	    OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT) < 0) {
+		die_if_corrupt(r, &item->object.oid, real_oid);
 		return quiet_on_missing ? -1 :
 			error("Could not read %s",
 			     oid_to_hex(&item->object.oid));
+	}
 	if (type != OBJ_COMMIT) {
 		free(buffer);
 		return error("Object %s not a commit",
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

Re: [PATCH v2 0/4] Don't lazy-fetch commits when parsing them

[Jeff King]

On Thu, Dec 01, 2022 at 11:27:29AM -0800, Jonathan Tan wrote:

> Thanks everyone for your reviews. Here is a reroll with the requested change
> (just one small one).

Thanks, this looks OK to me. However Junio noted in "What's cooking"
that it seems to break CI on windows. The problem is in t5318.93:

  2022-12-01T09:26:44.8887018Z ++ cat test_err
  2022-12-01T09:26:44.8887414Z error: Could not read 0000000000000000000000000000000000000000
  2022-12-01T09:26:44.8887825Z error: Could not read 0000000000000000000000000000000000000000
  2022-12-01T09:26:44.8888240Z error: Could not read 0000000000000000000000000000000000000000
  2022-12-01T09:26:44.8888639Z error: Could not read 0000000000000000000000000000000000000000
  2022-12-01T09:26:44.8889052Z error: Could not read 0000000000000000000000000000000000000000
  2022-12-01T09:26:44.8889512Z error: Could not read 0000000000000000000000000000000000000000
  2022-12-01T09:26:44.8889991Z fatal: failed to read object 0000000000000000000000000000000000000000: Function not implemented
  2022-12-01T09:26:44.8890401Z ++ return 1
  2022-12-01T09:26:44.8890761Z error: last command exited with $?=1
  2022-12-01T09:26:44.8891263Z not ok 93 - corrupt commit-graph write (broken parent)

Looks like the check in die_if_corrupt() is seeing a different errno
value than ENOENT. I wonder if we need to take more care to preserve it
across calls. It does look like we hit the same sequence of functions
that read_object_file_extended() did, but perhaps this was buggy all
along, and you're now exposing it through a new code path.

In particular I wonder if obj_read_unlock() might be the culprit here,
and something like this might help:

diff --git a/object-file.c b/object-file.c
index 8adef99a7c..db2d35519e 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1641,9 +1641,12 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,
 			     struct object_info *oi, unsigned flags)
 {
 	int ret;
+	int save_errno;
 	obj_read_lock();
 	ret = do_oid_object_info_extended(r, oid, oi, flags);
+	save_errno = errno;
 	obj_read_unlock();
+	errno = save_errno;
 	return ret;
 }
 

-Peff

Re: [PATCH v2 0/4] Don't lazy-fetch commits when parsing them

[Jonathan Tan]

Jeff King <peff@peff.net> writes:
> On Thu, Dec 01, 2022 at 11:27:29AM -0800, Jonathan Tan wrote:
> 
> > Thanks everyone for your reviews. Here is a reroll with the requested change
> > (just one small one).
> 
> Thanks, this looks OK to me. However Junio noted in "What's cooking"
> that it seems to break CI on windows. The problem is in t5318.93:
> 
>   2022-12-01T09:26:44.8887018Z ++ cat test_err
>   2022-12-01T09:26:44.8887414Z error: Could not read 0000000000000000000000000000000000000000
>   2022-12-01T09:26:44.8887825Z error: Could not read 0000000000000000000000000000000000000000
>   2022-12-01T09:26:44.8888240Z error: Could not read 0000000000000000000000000000000000000000
>   2022-12-01T09:26:44.8888639Z error: Could not read 0000000000000000000000000000000000000000
>   2022-12-01T09:26:44.8889052Z error: Could not read 0000000000000000000000000000000000000000
>   2022-12-01T09:26:44.8889512Z error: Could not read 0000000000000000000000000000000000000000
>   2022-12-01T09:26:44.8889991Z fatal: failed to read object 0000000000000000000000000000000000000000: Function not implemented
>   2022-12-01T09:26:44.8890401Z ++ return 1
>   2022-12-01T09:26:44.8890761Z error: last command exited with $?=1
>   2022-12-01T09:26:44.8891263Z not ok 93 - corrupt commit-graph write (broken parent)
> 
> Looks like the check in die_if_corrupt() is seeing a different errno
> value than ENOENT. I wonder if we need to take more care to preserve it
> across calls. It does look like we hit the same sequence of functions
> that read_object_file_extended() did, but perhaps this was buggy all
> along, and you're now exposing it through a new code path.
> 
> In particular I wonder if obj_read_unlock() might be the culprit here,
> and something like this might help:
> 
> diff --git a/object-file.c b/object-file.c
> index 8adef99a7c..db2d35519e 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1641,9 +1641,12 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,
>  			     struct object_info *oi, unsigned flags)
>  {
>  	int ret;
> +	int save_errno;
>  	obj_read_lock();
>  	ret = do_oid_object_info_extended(r, oid, oi, flags);
> +	save_errno = errno;
>  	obj_read_unlock();
> +	errno = save_errno;
>  	return ret;
>  }
 
Copying die_if_corrupt() until "failed to read object":

> 1734 void die_if_corrupt(struct repository *r,                                                                                                                                                       
> 1735                     const struct object_id *oid,                                                                                                                                                
> 1736                     const struct object_id *real_oid)                                                                                                                                           
> 1737 {                                                                                                                                                                                               
> 1738         const struct packed_git *p;                                                                                                                                                             
> 1739         const char *path;                                                                                                                                                                       
> 1740         struct stat st;                                                                                                                                                                         
> 1741                                                                                                                                                                                                 
> 1742         obj_read_lock();                                                                                                                                                                        
> 1743         if (errno && errno != ENOENT)                                                                                                                                                           
> 1744                 die_errno(_("failed to read object %s"), oid_to_hex(oid));

I wonder if we could just remove this check. Even as it is, I don't think that
there is any guarantee that obj_read_lock() would not clobber errno. Removing
it makes all tests pass locally, but I haven't tried it on CI.

(One argument that could be made is that we shouldn't have any die_if_corrupt()
refactoring or other refactoring of the sort, because previously its contents
was part of a function and it could thus rely on the errno of what has happened
previously. But I think that even without my patches, we couldn't rely on it
in the first place - looking at obj_read_lock(), it looks like it could init a
mutex, and depending on the implementation of that, it could clobber errno.)

Re: [PATCH v2 0/4] Don't lazy-fetch commits when parsing them

[Jeff King]

On Thu, Dec 01, 2022 at 01:26:50PM -0800, Jonathan Tan wrote:

> > 1734 void die_if_corrupt(struct repository *r,
> > 1735                     const struct object_id *oid,
> > 1736                     const struct object_id *real_oid)
> > 1737 {
> > 1738         const struct packed_git *p;
> > 1739         const char *path;
> > 1740         struct stat st;
> > 1741
> > 1742         obj_read_lock();
> > 1743         if (errno && errno != ENOENT)
> > 1744                 die_errno(_("failed to read object %s"), oid_to_hex(oid));
> 
> I wonder if we could just remove this check. Even as it is, I don't think that
> there is any guarantee that obj_read_lock() would not clobber errno. Removing
> it makes all tests pass locally, but I haven't tried it on CI.
> 
> (One argument that could be made is that we shouldn't have any die_if_corrupt()
> refactoring or other refactoring of the sort, because previously its contents
> was part of a function and it could thus rely on the errno of what has happened
> previously. But I think that even without my patches, we couldn't rely on it
> in the first place - looking at obj_read_lock(), it looks like it could init a
> mutex, and depending on the implementation of that, it could clobber errno.)

Yeah, I don't see any difference in the new caller versus what the
original was doing. The errno we care about comes from inside
oid_object_info_extended(). So in any case, we'll see at least
obj_read_unlock() followed by obj_read_lock() between the syscalls of
interest and this check. And I don't even really see any indication that
oid_object_info_extended() tries to set or preserve errno itself. The
likely sequence is:

  - find_pack_entry() fails to find it; errno isn't set at all
  - loose_object_info() tries to open it and probably gets ENOENT
  - we check find_pack_entry() again after reprepare_packed_git()
  - that fails so we return -1, barring submodule or partial clone
    tricks

So it really seems like we're quite likely to get an errno from opening
or mapping packs. Which implies the original suffers from the same
issue, but we simply never triggered it meaningfully in a test.

I'm not entirely sure on just removing the check. It comes from
3ba7a06552 (A loose object is not corrupt if it cannot be read due to
EMFILE, 2010-10-28), so we'd lose what that commit is trying to do.
Though I think even back then, I think it would have suffered from the
same problems (minus the lock/unlock; I'm still unclear which syscall is
the actual culprit here).

If we assume that errno from reading the object isn't reliable, I think
you'd have to actually re-check things. Something like:

  if (find_pack_entry(...) || !stat_loose_object(...))
    /* ok, it's not missing */

but of course we don't have the actual errno that _did_ cause us to
fail, which makes the error message we'd print a lot less useful. Maybe
this check should be ditched and we should complain much closer to the
source of the problem:

diff --git a/object-file.c b/object-file.c
index 26290554bb..743ba8210e 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1460,8 +1460,12 @@ static int loose_object_info(struct repository *r,
 	}
 
 	map = map_loose_object(r, oid, &mapsize);
-	if (!map)
+	if (!map) {
+		if (errno != ENOENT)
+			error_errno("unable to open loose object %s",
+				    oid_to_hex(oid));
 		return -1;
+	}
 
 	if (!oi->sizep)
 		oi->sizep = &size_scratch;

That might make things more verbose for other code paths, but that kind
of seems like a good thing. If you have an object file that we can't
open, we probably _should_ be complaining loudly about it.

We may need to be a little more careful about preserving errno in
map_loose_object_1(), though (gee, another place where the existing
check could run into trouble).

-Peff

Re: [PATCH v2 0/4] Don't lazy-fetch commits when parsing them

[Jonathan Tan]

Jeff King <peff@peff.net> writes:
> Yeah, I don't see any difference in the new caller versus what the
> original was doing. The errno we care about comes from inside
> oid_object_info_extended(). So in any case, we'll see at least
> obj_read_unlock() followed by obj_read_lock() between the syscalls of
> interest and this check. And I don't even really see any indication that
> oid_object_info_extended() tries to set or preserve errno itself. The
> likely sequence is:
> 
>   - find_pack_entry() fails to find it; errno isn't set at all
>   - loose_object_info() tries to open it and probably gets ENOENT
>   - we check find_pack_entry() again after reprepare_packed_git()
>   - that fails so we return -1, barring submodule or partial clone
>     tricks
> 
> So it really seems like we're quite likely to get an errno from opening
> or mapping packs. Which implies the original suffers from the same
> issue, but we simply never triggered it meaningfully in a test.

Thanks for checking. I'm still not sure how the current code passes CI, but my
patches don't. 

> I'm not entirely sure on just removing the check. It comes from
> 3ba7a06552 (A loose object is not corrupt if it cannot be read due to
> EMFILE, 2010-10-28), so we'd lose what that commit is trying to do.
> Though I think even back then, I think it would have suffered from the
> same problems (minus the lock/unlock; I'm still unclear which syscall is
> the actual culprit here).

Ah, thanks for the pointer to that commit. Without that, my patch would report
corruption even if the real issue was EMFILE, as the commit message of that
commit describes.

> If we assume that errno from reading the object isn't reliable, I think
> you'd have to actually re-check things. Something like:
> 
>   if (find_pack_entry(...) || !stat_loose_object(...))
>     /* ok, it's not missing */
> 
> but of course we don't have the actual errno that _did_ cause us to
> fail, which makes the error message we'd print a lot less useful. Maybe
> this check should be ditched and we should complain much closer to the
> source of the problem:
> 
> diff --git a/object-file.c b/object-file.c
> index 26290554bb..743ba8210e 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1460,8 +1460,12 @@ static int loose_object_info(struct repository *r,
>  	}
>  
>  	map = map_loose_object(r, oid, &mapsize);
> -	if (!map)
> +	if (!map) {
> +		if (errno != ENOENT)
> +			error_errno("unable to open loose object %s",
> +				    oid_to_hex(oid));
>  		return -1;
> +	}
>  
>  	if (!oi->sizep)
>  		oi->sizep = &size_scratch;
> 
> That might make things more verbose for other code paths, but that kind
> of seems like a good thing. If you have an object file that we can't
> open, we probably _should_ be complaining loudly about it.
> 
> We may need to be a little more careful about preserving errno in
> map_loose_object_1(), though (gee, another place where the existing
> check could run into trouble).

Besides needing to be careful in map_loose_object_1(), I'm not sure if this
fully solves the problem. This is non-fatal, so the EMFILE commit's work would
still remain undone. If this were made fatal, I think this would change the
behavior of too much code, especially those that can tolerate loose objects
being missing.
 
What do you think of not putting any die_if_corrupt() calls in the commit
parsing code at all? The error message printed would then be different (just a
generic message about being unable to parse a commit, versus the specific one
here) but it does pass CI [1]. Also, I don't think that we should be doing errno
diagnostics separate from what causes the errno anyway.

[1] https://github.com/jonathantanmy/git/actions/runs/3624495729

Re: [PATCH v2 0/4] Don't lazy-fetch commits when parsing them

[Jeff King]

On Mon, Dec 05, 2022 at 04:49:34PM -0800, Jonathan Tan wrote:

> > So it really seems like we're quite likely to get an errno from opening
> > or mapping packs. Which implies the original suffers from the same
> > issue, but we simply never triggered it meaningfully in a test.
> 
> Thanks for checking. I'm still not sure how the current code passes CI, but my
> patches don't. 

Hmm. Actually, I am now, too.

My assumption was that only certain tests would notice the problem,
because both outcomes are an error, and they only differ in what stderr
says ("this does not exist" versus "we got this weird errno"). And so I
assumed that the old spot in read_object_file() did not happen to
trigger any tests which check stderr, but your new caller in
repo_parse_commit_internal() was unlucky enough to do so. But since that
new caller was calling repo_read_object_file() before, I'd think it
would have triggered the same thing.

> > That might make things more verbose for other code paths, but that kind
> > of seems like a good thing. If you have an object file that we can't
> > open, we probably _should_ be complaining loudly about it.
> > 
> > We may need to be a little more careful about preserving errno in
> > map_loose_object_1(), though (gee, another place where the existing
> > check could run into trouble).
> 
> Besides needing to be careful in map_loose_object_1(), I'm not sure if this
> fully solves the problem. This is non-fatal, so the EMFILE commit's work would
> still remain undone. If this were made fatal, I think this would change the
> behavior of too much code, especially those that can tolerate loose objects
> being missing.

True, in the sense that we'd still say "X is corrupt". But I think the
real sin prior to 3ba7a0655 is that we _only_ said "hey, this looks
corrupt". If the output is:

  error: unable to mmap .git/objects/12/3456abcd...
  fatal: object 123456abcd is corrupt or missing

then that is at least not actively misleading (and is broadly similar to
other cases in Git, where higher-level code only knows "I expected us to
have this object and for some reason we don't", but without knowing
whether it was missing, or a system error, or corrupt).

That said I think all of these die() statements that you moved into
die_if_corrupt() are already doing the wrong thing. We should probably
mention errors (besides "missing object") to the user at the lowest
level where we can give the most detail, and then return errors up the
stack. That makes Git more verbose, but remember we're talking about
corrupted or broken repositories here. You shouldn't see these under
normal circumstances.

> What do you think of not putting any die_if_corrupt() calls in the commit
> parsing code at all? The error message printed would then be different (just a
> generic message about being unable to parse a commit, versus the specific one
> here) but it does pass CI [1]. Also, I don't think that we should be doing errno
> diagnostics separate from what causes the errno anyway.

So I think that's a lesser version of what I'm proposing above. ;) In
the sense that yes, I would say that repo_parse_commit_internal() does
not need to do anything more specific than its existing "could not read"
message. But I would go further and say:

  - it would be nice the low-level code where errno _is_ valid said more
    about what happened

  - we should take read_object_file_extended() in the same direction

-Peff

Re: [PATCH v2 0/4] Don't lazy-fetch commits when parsing them

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Dec 01, 2022 at 11:27:29AM -0800, Jonathan Tan wrote:
>
>> Thanks everyone for your reviews. Here is a reroll with the requested change
>> (just one small one).
>
> Thanks, this looks OK to me. However Junio noted in "What's cooking"
> that it seems to break CI on windows. The problem is in t5318.93:
> ...
> In particular I wonder if obj_read_unlock() might be the culprit here,
> and something like this might help:

Thanks for following-up.

[PATCH v2 0/3] Don't lazy-fetch commits when parsing them

[Jonathan Tan]

Thanks everyone for taking a look. In the end, I thought it best to bite the
bullet and move all the corruption diagnostics to where they are detected
instead of re-checking (and thus relying on errno to be preserved) after we
have found out that we couldn't read the object. This does mean a reworking
of all the earlier patches, but overall I think that this puts the code in a
better state.

I have also verified that this passes CI [1].

[1] https://github.com/jonathantanmy/git/actions/runs/3634359088

Jonathan Tan (3):
  object-file: don't exit early if skipping loose
  object-file: emit corruption errors when detected
  commit: don't lazy-fetch commits

 commit.c       | 15 +++++++--
 object-file.c  | 84 ++++++++++++++++++++++++++------------------------
 object-store.h |  3 ++
 3 files changed, 59 insertions(+), 43 deletions(-)

Range-diff against v1:
1:  3a00bc45fd < -:  ---------- object-file: reread object with exact same args
2:  9999e127a0 < -:  ---------- object-file: refactor corrupt object diagnosis
3:  28c7ee2f8c < -:  ---------- object-file: refactor replace object lookup
-:  ---------- > 1:  9ad34a1dce object-file: don't exit early if skipping loose
-:  ---------- > 2:  9ddfff3585 object-file: emit corruption errors when detected
4:  0607fa67d1 ! 3:  c5fe42deb0 commit: don't lazy-fetch commits
    @@ commit.c: int repo_parse_commit_internal(struct repository *r,
      	enum object_type type;
      	void *buffer;
      	unsigned long size;
    -+	const struct object_id *real_oid;
     +	struct object_info oi = {
     +		.typep = &type,
     +		.sizep = &size,
     +		.contentp = &buffer,
    -+		.real_oidp = &real_oid,
     +	};
    ++	/*
    ++	 * Git does not support partial clones that exclude commits, so set
    ++	 * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.
    ++	 */
    ++	int flags = OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT |
    ++		OBJECT_INFO_DIE_IF_CORRUPT;
      	int ret;
      
      	if (!item)
    @@ commit.c: int repo_parse_commit_internal(struct repository *r,
     -	buffer = repo_read_object_file(r, &item->object.oid, &type, &size);
     -	if (!buffer)
     +
    -+	/*
    -+	 * Git does not support partial clones that exclude commits, so set
    -+	 * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.
    -+	 */
    -+	if (oid_object_info_extended(r, &item->object.oid, &oi,
    -+	    OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT) < 0) {
    -+		die_if_corrupt(r, &item->object.oid, real_oid);
    ++	if (oid_object_info_extended(r, &item->object.oid, &oi, flags) < 0)
      		return quiet_on_missing ? -1 :
      			error("Could not read %s",
      			     oid_to_hex(&item->object.oid));
    -+	}
    - 	if (type != OBJ_COMMIT) {
    - 		free(buffer);
    - 		return error("Object %s not a commit",
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

[PATCH v2 1/3] object-file: don't exit early if skipping loose

[Jonathan Tan]

Instead, also search the submodule object stores and promisor remotes.

This also centralizes what happens when the object is not found (the
"return -1"), which is useful for a subsequent patch.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-file.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/object-file.c b/object-file.c
index 26290554bb..596dd049fd 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1575,18 +1575,17 @@ static int do_oid_object_info_extended(struct repository *r,
 		if (find_pack_entry(r, real, &e))
 			break;
 
-		if (flags & OBJECT_INFO_IGNORE_LOOSE)
-			return -1;
-
-		/* Most likely it's a loose object. */
-		if (!loose_object_info(r, real, oi, flags))
-			return 0;
-
-		/* Not a loose object; someone else may have just packed it. */
-		if (!(flags & OBJECT_INFO_QUICK)) {
-			reprepare_packed_git(r);
-			if (find_pack_entry(r, real, &e))
-				break;
+		if (!(flags & OBJECT_INFO_IGNORE_LOOSE)) {
+			/* Most likely it's a loose object. */
+			if (!loose_object_info(r, real, oi, flags))
+				return 0;
+
+			/* Not a loose object; someone else may have just packed it. */
+			if (!(flags & OBJECT_INFO_QUICK)) {
+				reprepare_packed_git(r);
+				if (find_pack_entry(r, real, &e))
+					break;
+			}
 		}
 
 		/*
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

Re: [PATCH v2 1/3] object-file: don't exit early if skipping loose

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> Instead, also search the submodule object stores and promisor remotes.
>
> This also centralizes what happens when the object is not found (the
> "return -1"), which is useful for a subsequent patch.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  object-file.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/object-file.c b/object-file.c
> index 26290554bb..596dd049fd 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1575,18 +1575,17 @@ static int do_oid_object_info_extended(struct repository *r,
>  		if (find_pack_entry(r, real, &e))
>  			break;
>  
> -		if (flags & OBJECT_INFO_IGNORE_LOOSE)
> -			return -1;
> -
> -		/* Most likely it's a loose object. */
> -		if (!loose_object_info(r, real, oi, flags))
> -			return 0;
> -
> -		/* Not a loose object; someone else may have just packed it. */
> -		if (!(flags & OBJECT_INFO_QUICK)) {
> -			reprepare_packed_git(r);
> -			if (find_pack_entry(r, real, &e))
> -				break;
> +		if (!(flags & OBJECT_INFO_IGNORE_LOOSE)) {
> +			/* Most likely it's a loose object. */
> +			if (!loose_object_info(r, real, oi, flags))
> +				return 0;
> +
> +			/* Not a loose object; someone else may have just packed it. */
> +			if (!(flags & OBJECT_INFO_QUICK)) {
> +				reprepare_packed_git(r);
> +				if (find_pack_entry(r, real, &e))
> +					break;
> +			}
>  		}

Hmph, who passes IGNORE_LOOSE and why?  Explaining the answer to
that question would give us confidence why this change is safe.

If the reason IGNORE_LOOSE is set by the callers is because they are
interested only in locally packed objects, then this change would
break them because they end up triggering the lazy fetch in the
updated code, no?  Or do all callers that set IGNORE_LOOSE drop the
fetch_if_missing global before calling us?

Thanks.

Re: [PATCH v2 1/3] object-file: don't exit early if skipping loose

[Jeff King]

On Wed, Dec 07, 2022 at 10:12:13AM +0900, Junio C Hamano wrote:

> Hmph, who passes IGNORE_LOOSE and why?  Explaining the answer to
> that question would give us confidence why this change is safe.
> 
> If the reason IGNORE_LOOSE is set by the callers is because they are
> interested only in locally packed objects, then this change would
> break them because they end up triggering the lazy fetch in the
> updated code, no?  Or do all callers that set IGNORE_LOOSE drop the
> fetch_if_missing global before calling us?

I wondered who those callers might be, too, because it is such a weird
thing for a caller to want to care about (usually we try to abstract the
object database).

It looks like the only user went away in 97b2fa08b6 (fetch-pack: drop
custom loose object cache, 2018-11-12). So I think we just want to drop
it:

diff --git a/object-file.c b/object-file.c
index 26290554bb..cf724bc19b 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1575,9 +1575,6 @@ static int do_oid_object_info_extended(struct repository *r,
 		if (find_pack_entry(r, real, &e))
 			break;
 
-		if (flags & OBJECT_INFO_IGNORE_LOOSE)
-			return -1;
-
 		/* Most likely it's a loose object. */
 		if (!loose_object_info(r, real, oi, flags))
 			return 0;
diff --git a/object-store.h b/object-store.h
index 1be57abaf1..371629c1e1 100644
--- a/object-store.h
+++ b/object-store.h
@@ -434,8 +434,6 @@ struct object_info {
 #define OBJECT_INFO_ALLOW_UNKNOWN_TYPE 2
 /* Do not retry packed storage after checking packed and loose storage */
 #define OBJECT_INFO_QUICK 8
-/* Do not check loose object */
-#define OBJECT_INFO_IGNORE_LOOSE 16
 /*
  * Do not attempt to fetch the object if missing (even if fetch_is_missing is
  * nonzero).


We could also renumber the later flags to keep them compact, but I don't
have a strong opinion there.

-Peff

Re: [PATCH v2 1/3] object-file: don't exit early if skipping loose

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I wondered who those callers might be, too, because it is such a weird
> thing for a caller to want to care about (usually we try to abstract the
> object database).

Exactly.

> It looks like the only user went away in 97b2fa08b6 (fetch-pack: drop
> custom loose object cache, 2018-11-12).

Nice, very nice.

> So I think we just want to drop it:
>
> diff --git a/object-file.c b/object-file.c
> index 26290554bb..cf724bc19b 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1575,9 +1575,6 @@ static int do_oid_object_info_extended(struct repository *r,
>  		if (find_pack_entry(r, real, &e))
>  			break;
>  
> -		if (flags & OBJECT_INFO_IGNORE_LOOSE)
> -			return -1;
> -
>  		/* Most likely it's a loose object. */
>  		if (!loose_object_info(r, real, oi, flags))
>  			return 0;
> diff --git a/object-store.h b/object-store.h
> index 1be57abaf1..371629c1e1 100644
> --- a/object-store.h
> +++ b/object-store.h
> @@ -434,8 +434,6 @@ struct object_info {
>  #define OBJECT_INFO_ALLOW_UNKNOWN_TYPE 2
>  /* Do not retry packed storage after checking packed and loose storage */
>  #define OBJECT_INFO_QUICK 8
> -/* Do not check loose object */
> -#define OBJECT_INFO_IGNORE_LOOSE 16
>  /*
>   * Do not attempt to fetch the object if missing (even if fetch_is_missing is
>   * nonzero).

This would make Jonathan's change a lot transparent and intuitive if
it is based on it, I would think.

Thanks for digging.

Re: [PATCH v2 1/3] object-file: don't exit early if skipping loose

[Jonathan Tan]

Junio C Hamano <gitster@pobox.com> writes:
> > It looks like the only user went away in 97b2fa08b6 (fetch-pack: drop
> > custom loose object cache, 2018-11-12).
> 
> Nice, very nice.
> 
> > So I think we just want to drop it:

[snip]

> This would make Jonathan's change a lot transparent and intuitive if
> it is based on it, I would think.
> 
> Thanks for digging.

Ah, thanks for finding this. I'll make this change.

[PATCH v2 2/3] object-file: emit corruption errors when detected

[Jonathan Tan]

Instead of relying on errno being preserved across function calls, teach
do_oid_object_info_extended() to itself report object corruption when
it first detects it. There are 3 types of corruption being detected:
 - when a replacement object is missing
 - when a loose object is corrupt
 - when a packed object is corrupt and the object cannot be read
   in another way

Note that in the RHS of this patch's diff, a check for ENOENT that was
introduced in 3ba7a06552 (A loose object is not corrupt if it cannot
be read due to EMFILE, 2010-10-28) is also removed. The purpose of this
check is to avoid a false report of corruption if the errno contains
something like EMFILE (or anything that is not ENOENT), in which case
a more generic report is presented. Because, as of this patch, we no
longer rely on such a heuristic to determine corruption, but surface
the error message at the point when we read something that we did not
expect, this check is no longer necessary.

Besides being more resilient, this also prepares for a future patch in
which an indirect caller of do_oid_object_info_extended() will need
such functionality.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-file.c  | 63 ++++++++++++++++++++++++++------------------------
 object-store.h |  3 +++
 2 files changed, 36 insertions(+), 30 deletions(-)

diff --git a/object-file.c b/object-file.c
index 596dd049fd..c7a513d123 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1215,7 +1215,8 @@ static int quick_has_loose(struct repository *r,
  * searching for a loose object named "oid".
  */
 static void *map_loose_object_1(struct repository *r, const char *path,
-			     const struct object_id *oid, unsigned long *size)
+				const struct object_id *oid, unsigned long *size,
+				char **mapped_path)
 {
 	void *map;
 	int fd;
@@ -1224,6 +1225,9 @@ static void *map_loose_object_1(struct repository *r, const char *path,
 		fd = git_open(path);
 	else
 		fd = open_loose_object(r, oid, &path);
+	if (mapped_path)
+		*mapped_path = xstrdup(path);
+
 	map = NULL;
 	if (fd >= 0) {
 		struct stat st;
@@ -1247,7 +1251,7 @@ void *map_loose_object(struct repository *r,
 		       const struct object_id *oid,
 		       unsigned long *size)
 {
-	return map_loose_object_1(r, NULL, oid, size);
+	return map_loose_object_1(r, NULL, oid, size, NULL);
 }
 
 enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,
@@ -1428,6 +1432,7 @@ static int loose_object_info(struct repository *r,
 {
 	int status = 0;
 	unsigned long mapsize;
+	char *mapped_path = NULL;
 	void *map;
 	git_zstream stream;
 	char hdr[MAX_HEADER_LEN];
@@ -1459,9 +1464,11 @@ static int loose_object_info(struct repository *r,
 		return 0;
 	}
 
-	map = map_loose_object(r, oid, &mapsize);
-	if (!map)
+	map = map_loose_object_1(r, NULL, oid, &mapsize, &mapped_path);
+	if (!map) {
+		free(mapped_path);
 		return -1;
+	}
 
 	if (!oi->sizep)
 		oi->sizep = &size_scratch;
@@ -1497,8 +1504,13 @@ static int loose_object_info(struct repository *r,
 		break;
 	}
 
+	if (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
+		die(_("loose object %s (stored in %s) is corrupt"),
+		    oid_to_hex(oid), mapped_path);
+
 	git_inflate_end(&stream);
 cleanup:
+	free(mapped_path);
 	munmap(map, mapsize);
 	if (oi->sizep == &size_scratch)
 		oi->sizep = NULL;
@@ -1608,6 +1620,15 @@ static int do_oid_object_info_extended(struct repository *r,
 			continue;
 		}
 
+		if (flags & OBJECT_INFO_DIE_IF_CORRUPT) {
+			const struct packed_git *p;
+			if ((flags & OBJECT_INFO_LOOKUP_REPLACE) && !oideq(real, oid))
+				die(_("replacement %s not found for %s"),
+				    oid_to_hex(real), oid_to_hex(oid));
+			if ((p = has_packed_and_bad(r, real)))
+				die(_("packed object %s (stored in %s) is corrupt"),
+				    oid_to_hex(real), p->pack_name);
+		}
 		return -1;
 	}
 
@@ -1660,7 +1681,8 @@ int oid_object_info(struct repository *r,
 
 static void *read_object(struct repository *r,
 			 const struct object_id *oid, enum object_type *type,
-			 unsigned long *size)
+			 unsigned long *size,
+			 int die_if_corrupt)
 {
 	struct object_info oi = OBJECT_INFO_INIT;
 	void *content;
@@ -1668,7 +1690,9 @@ static void *read_object(struct repository *r,
 	oi.sizep = size;
 	oi.contentp = &content;
 
-	if (oid_object_info_extended(r, oid, &oi, 0) < 0)
+	if (oid_object_info_extended(r, oid, &oi,
+				     die_if_corrupt ? OBJECT_INFO_DIE_IF_CORRUPT : 0)
+	    < 0)
 		return NULL;
 	return content;
 }
@@ -1704,35 +1728,14 @@ void *read_object_file_extended(struct repository *r,
 				int lookup_replace)
 {
 	void *data;
-	const struct packed_git *p;
-	const char *path;
-	struct stat st;
 	const struct object_id *repl = lookup_replace ?
 		lookup_replace_object(r, oid) : oid;
 
 	errno = 0;
-	data = read_object(r, repl, type, size);
+	data = read_object(r, repl, type, size, 1);
 	if (data)
 		return data;
 
-	obj_read_lock();
-	if (errno && errno != ENOENT)
-		die_errno(_("failed to read object %s"), oid_to_hex(oid));
-
-	/* die if we replaced an object with one that does not exist */
-	if (repl != oid)
-		die(_("replacement %s not found for %s"),
-		    oid_to_hex(repl), oid_to_hex(oid));
-
-	if (!stat_loose_object(r, repl, &st, &path))
-		die(_("loose object %s (stored in %s) is corrupt"),
-		    oid_to_hex(repl), path);
-
-	if ((p = has_packed_and_bad(r, repl)))
-		die(_("packed object %s (stored in %s) is corrupt"),
-		    oid_to_hex(repl), p->pack_name);
-	obj_read_unlock();
-
 	return NULL;
 }
 
@@ -2275,7 +2278,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime)
 
 	if (has_loose_object(oid))
 		return 0;
-	buf = read_object(the_repository, oid, &type, &len);
+	buf = read_object(the_repository, oid, &type, &len, 0);
 	if (!buf)
 		return error(_("cannot read object for %s"), oid_to_hex(oid));
 	hdrlen = format_object_header(hdr, sizeof(hdr), type, len);
@@ -2797,7 +2800,7 @@ int read_loose_object(const char *path,
 	char hdr[MAX_HEADER_LEN];
 	unsigned long *size = oi->sizep;
 
-	map = map_loose_object_1(the_repository, path, NULL, &mapsize);
+	map = map_loose_object_1(the_repository, path, NULL, &mapsize, NULL);
 	if (!map) {
 		error_errno(_("unable to mmap %s"), path);
 		goto out;
diff --git a/object-store.h b/object-store.h
index 1be57abaf1..01134ab5ec 100644
--- a/object-store.h
+++ b/object-store.h
@@ -447,6 +447,9 @@ struct object_info {
  */
 #define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)
 
+/* Die if object corruption (not just an object being missing) was detected. */
+#define OBJECT_INFO_DIE_IF_CORRUPT 64
+
 int oid_object_info_extended(struct repository *r,
 			     const struct object_id *,
 			     struct object_info *, unsigned flags);
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

Re: [PATCH v2 2/3] object-file: emit corruption errors when detected

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> Instead of relying on errno being preserved across function calls, teach
> do_oid_object_info_extended() to itself report object corruption when
> it first detects it. There are 3 types of corruption being detected:
>  - when a replacement object is missing
>  - when a loose object is corrupt
>  - when a packed object is corrupt and the object cannot be read
>    in another way
>
> Note that in the RHS of this patch's diff, a check for ENOENT that was
> introduced in 3ba7a06552 (A loose object is not corrupt if it cannot
> be read due to EMFILE, 2010-10-28) is also removed. The purpose of this
> check is to avoid a false report of corruption if the errno contains
> something like EMFILE (or anything that is not ENOENT), in which case
> a more generic report is presented. Because, as of this patch, we no
> longer rely on such a heuristic to determine corruption, but surface
> the error message at the point when we read something that we did not
> expect, this check is no longer necessary.
>
> Besides being more resilient, this also prepares for a future patch in
> which an indirect caller of do_oid_object_info_extended() will need
> such functionality.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  object-file.c  | 63 ++++++++++++++++++++++++++------------------------
>  object-store.h |  3 +++
>  2 files changed, 36 insertions(+), 30 deletions(-)

The implementation looks very straight-forward.  Nicely done.

Re: [PATCH v2 2/3] object-file: emit corruption errors when detected

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 06 2022, Jonathan Tan wrote:

> Instead of relying on errno being preserved across function calls, teach
> do_oid_object_info_extended() to itself report object corruption when
> it first detects it. There are 3 types of corruption being detected:
>  - when a replacement object is missing
>  - when a loose object is corrupt
>  - when a packed object is corrupt and the object cannot be read
>    in another way
>
> Note that in the RHS of this patch's diff, a check for ENOENT that was
> introduced in 3ba7a06552 (A loose object is not corrupt if it cannot
> be read due to EMFILE, 2010-10-28) is also removed. The purpose of this
> check is to avoid a false report of corruption if the errno contains
> something like EMFILE (or anything that is not ENOENT), in which case
> a more generic report is presented. Because, as of this patch, we no
> longer rely on such a heuristic to determine corruption, but surface
> the error message at the point when we read something that we did not
> expect, this check is no longer necessary.
>
> Besides being more resilient, this also prepares for a future patch in
> which an indirect caller of do_oid_object_info_extended() will need
> such functionality.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  object-file.c  | 63 ++++++++++++++++++++++++++------------------------
>  object-store.h |  3 +++
>  2 files changed, 36 insertions(+), 30 deletions(-)
>
> diff --git a/object-file.c b/object-file.c
> index 596dd049fd..c7a513d123 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1215,7 +1215,8 @@ static int quick_has_loose(struct repository *r,
>   * searching for a loose object named "oid".
>   */
>  static void *map_loose_object_1(struct repository *r, const char *path,
> -			     const struct object_id *oid, unsigned long *size)
> +				const struct object_id *oid, unsigned long *size,
> +				char **mapped_path)
>  {
>  	void *map;
>  	int fd;
> @@ -1224,6 +1225,9 @@ static void *map_loose_object_1(struct repository *r, const char *path,
>  		fd = git_open(path);
>  	else
>  		fd = open_loose_object(r, oid, &path);
> +	if (mapped_path)
> +		*mapped_path = xstrdup(path);
> +
>  	map = NULL;
>  	if (fd >= 0) {
>  		struct stat st;

I find this map_loose_object_1() function to be rather "busy". Part of
it's the pre-image.

Callers at the end of this series are:

   1254:        return map_loose_object_1(r, NULL, oid, size, NULL);
   1467:        map = map_loose_object_1(r, NULL, oid, &mapsize, &mapped_path);
   2803:        map = map_loose_object_1(the_repository, path, NULL, &mapsize, NULL);

So, either we know the path already, and we pass it in, or we don't know
the path, and may or may not be interested in what the path ends up
being.

Which is why we pass in both a "path" and a "mapped_path".

Then, somewhat confusingly (maybe I'm the only one who finds this odd")
the "path" variable itself does double-duty within the function. If we
have a "path" already we leave it alone, but if we don't it's NULL, and
then we write our new path to it.

We *might* then have a path already, *and* write to the "mapped_path",
but in that case we'd be xstrdup() ing a string the user passed in. But
this API use would make no sense.

So shouldn't we at least have a:

	if (path && mapped_path)
		BUG("either tell me the path, or ask me, not both!");

But I think it's better to just separate these concerns. Most of this
refactoring is good, but I think this bit went a step too far, and as a
result we now need to memory manage this "mapped_path". I.e. we'd get a
"struct strbuf"'s "buf" before, but now loose_object_info() needs to
have it xstrdup()'d, just to free() it again.

Isn't the below squashed in better? I.e. just always pass the "path",
but maybe pass a "fd=0", in which case the function might need to
git_open() it.

Then have map_loose_object() and loose_object_info() call
open_loose_object(), and pass in the "path" and "fd".

diff --git a/object-file.c b/object-file.c
index c7a513d123e..24793e1b479 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1214,19 +1214,13 @@ static int quick_has_loose(struct repository *r,
  * Map the loose object at "path" if it is not NULL, or the path found by
  * searching for a loose object named "oid".
  */
-static void *map_loose_object_1(struct repository *r, const char *path,
-				const struct object_id *oid, unsigned long *size,
-				char **mapped_path)
+static void *map_loose_object_1(struct repository *r, const char *const path,
+				int fd, unsigned long *size)
 {
 	void *map;
-	int fd;
 
-	if (path)
+	if (!fd)
 		fd = git_open(path);
-	else
-		fd = open_loose_object(r, oid, &path);
-	if (mapped_path)
-		*mapped_path = xstrdup(path);
 
 	map = NULL;
 	if (fd >= 0) {
@@ -1251,7 +1245,10 @@ void *map_loose_object(struct repository *r,
 		       const struct object_id *oid,
 		       unsigned long *size)
 {
-	return map_loose_object_1(r, NULL, oid, size, NULL);
+	const char *path;
+	int fd = open_loose_object(r, oid, &path);
+
+	return map_loose_object_1(r, path,fd, size);
 }
 
 enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,
@@ -1432,7 +1429,6 @@ static int loose_object_info(struct repository *r,
 {
 	int status = 0;
 	unsigned long mapsize;
-	char *mapped_path = NULL;
 	void *map;
 	git_zstream stream;
 	char hdr[MAX_HEADER_LEN];
@@ -1440,6 +1436,8 @@ static int loose_object_info(struct repository *r,
 	unsigned long size_scratch;
 	enum object_type type_scratch;
 	int allow_unknown = flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE;
+	int fd;
+	const char *path;
 
 	if (oi->delta_base_oid)
 		oidclr(oi->delta_base_oid);
@@ -1464,11 +1462,10 @@ static int loose_object_info(struct repository *r,
 		return 0;
 	}
 
-	map = map_loose_object_1(r, NULL, oid, &mapsize, &mapped_path);
-	if (!map) {
-		free(mapped_path);
+	fd = open_loose_object(r, oid, &path);
+	map = map_loose_object_1(r, path, fd, &mapsize);
+	if (!map)
 		return -1;
-	}
 
 	if (!oi->sizep)
 		oi->sizep = &size_scratch;
@@ -1506,11 +1503,10 @@ static int loose_object_info(struct repository *r,
 
 	if (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
 		die(_("loose object %s (stored in %s) is corrupt"),
-		    oid_to_hex(oid), mapped_path);
+		    oid_to_hex(oid), path);
 
 	git_inflate_end(&stream);
 cleanup:
-	free(mapped_path);
 	munmap(map, mapsize);
 	if (oi->sizep == &size_scratch)
 		oi->sizep = NULL;
@@ -2800,7 +2796,7 @@ int read_loose_object(const char *path,
 	char hdr[MAX_HEADER_LEN];
 	unsigned long *size = oi->sizep;
 
-	map = map_loose_object_1(the_repository, path, NULL, &mapsize, NULL);
+	map = map_loose_object_1(the_repository, path, 0, &mapsize);
 	if (!map) {
 		error_errno(_("unable to mmap %s"), path);
 		goto out;

Re: [PATCH v2 2/3] object-file: emit corruption errors when detected

[Jeff King]

On Wed, Dec 07, 2022 at 05:05:47AM +0100, Ævar Arnfjörð Bjarmason wrote:

> Isn't the below squashed in better? I.e. just always pass the "path",
> but maybe pass a "fd=0", in which case the function might need to
> git_open() it.
> 
> Then have map_loose_object() and loose_object_info() call
> open_loose_object(), and pass in the "path" and "fd".

I like this direction, though I'd give a few small suggestions. One is
to make it unconditional to pass in a valid "fd". These kind of magic
sentinel values sometimes lead to confusion or bugs, and it's easy
enough for the caller to use git_open() itself.

In fact, in the one caller who cares, it lets us produce a nicer
error message:

diff --git a/object-file.c b/object-file.c
index 24793e1b47..7c2a85132b 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1219,9 +1219,6 @@ static void *map_loose_object_1(struct repository *r, const char *const path,
 {
 	void *map;
 
-	if (!fd)
-		fd = git_open(path);
-
 	map = NULL;
 	if (fd >= 0) {
 		struct stat st;
@@ -2790,13 +2787,18 @@ int read_loose_object(const char *path,
 		      struct object_info *oi)
 {
 	int ret = -1;
+	int fd;
 	void *map = NULL;
 	unsigned long mapsize;
 	git_zstream stream;
 	char hdr[MAX_HEADER_LEN];
 	unsigned long *size = oi->sizep;
 
-	map = map_loose_object_1(the_repository, path, 0, &mapsize);
+	fd = git_open(path);
+	if (fd < 0)
+		error_errno(_("unable to open %s"), path);
+
+	map = map_loose_object_1(the_repository, path, fd, &mapsize);
 	if (!map) {
 		error_errno(_("unable to mmap %s"), path);
 		goto out;

> +static void *map_loose_object_1(struct repository *r, const char *const path,
> +				int fd, unsigned long *size)
>  {
>  	void *map;
> -	int fd;
>  
> -	if (path)
> +	if (!fd)
>  		fd = git_open(path);
> -	else
> -		fd = open_loose_object(r, oid, &path);
> -	if (mapped_path)
> -		*mapped_path = xstrdup(path);

The other weird thing here is ownership of "fd". Now some callers pass
it in, but map_loose_object_1() always closes it. I think that's OK
(since we want it closed even on success), but definitely surprising
enough that we'd want to document that in a comment.

> @@ -1251,7 +1245,10 @@ void *map_loose_object(struct repository *r,
>  		       const struct object_id *oid,
>  		       unsigned long *size)
>  {
> -	return map_loose_object_1(r, NULL, oid, size, NULL);
> +	const char *path;
> +	int fd = open_loose_object(r, oid, &path);
> +
> +	return map_loose_object_1(r, path,fd, size);
>  }

It's also kind of weird that map_loose_object_1() is a noop on a
negative descriptor. That technically makes this correct, but I think it
would be much less surprising to always take a valid descriptor, and
this code should do:

  if (fd)
	return -1;
  return map_loose_object_1(r, path, fd, size);

If we are going to make map_loose_object_1() less confusing (and I think
that is worth doing), let's go all the way.

-Peff

Re: [PATCH v2 2/3] object-file: emit corruption errors when detected

[Ævar Arnfjörð Bjarmason]

On Wed, Dec 07 2022, Jeff King wrote:

> On Wed, Dec 07, 2022 at 05:05:47AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> Isn't the below squashed in better? I.e. just always pass the "path",
>> but maybe pass a "fd=0", in which case the function might need to
>> git_open() it.
>> 
>> Then have map_loose_object() and loose_object_info() call
>> open_loose_object(), and pass in the "path" and "fd".
>
> I like this direction, though I'd give a few small suggestions. One is
> to make it unconditional to pass in a valid "fd". These kind of magic
> sentinel values sometimes lead to confusion or bugs, and it's easy
> enough for the caller to use git_open() itself.
>
> In fact, in the one caller who cares, it lets us produce a nicer
> error message:
>
> diff --git a/object-file.c b/object-file.c
> index 24793e1b47..7c2a85132b 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1219,9 +1219,6 @@ static void *map_loose_object_1(struct repository *r, const char *const path,
>  {
>  	void *map;
>  
> -	if (!fd)
> -		fd = git_open(path);
> -
>  	map = NULL;
>  	if (fd >= 0) {
>  		struct stat st;
> @@ -2790,13 +2787,18 @@ int read_loose_object(const char *path,
>  		      struct object_info *oi)
>  {
>  	int ret = -1;
> +	int fd;
>  	void *map = NULL;
>  	unsigned long mapsize;
>  	git_zstream stream;
>  	char hdr[MAX_HEADER_LEN];
>  	unsigned long *size = oi->sizep;
>  
> -	map = map_loose_object_1(the_repository, path, 0, &mapsize);
> +	fd = git_open(path);
> +	if (fd < 0)
> +		error_errno(_("unable to open %s"), path);
> +
> +	map = map_loose_object_1(the_repository, path, fd, &mapsize);
>  	if (!map) {
>  		error_errno(_("unable to mmap %s"), path);
>  		goto out;

Yeah, I think that's even better, although...

>> +static void *map_loose_object_1(struct repository *r, const char *const path,
>> +				int fd, unsigned long *size)
>>  {
>>  	void *map;
>> -	int fd;
>>  
>> -	if (path)
>> +	if (!fd)
>>  		fd = git_open(path);
>> -	else
>> -		fd = open_loose_object(r, oid, &path);
>> -	if (mapped_path)
>> -		*mapped_path = xstrdup(path);
>
> The other weird thing here is ownership of "fd". Now some callers pass
> it in, but map_loose_object_1() always closes it. I think that's OK
> (since we want it closed even on success), but definitely surprising
> enough that we'd want to document that in a comment.
>
>> @@ -1251,7 +1245,10 @@ void *map_loose_object(struct repository *r,
>>  		       const struct object_id *oid,
>>  		       unsigned long *size)
>>  {
>> -	return map_loose_object_1(r, NULL, oid, size, NULL);
>> +	const char *path;
>> +	int fd = open_loose_object(r, oid, &path);
>> +
>> +	return map_loose_object_1(r, path,fd, size);
>>  }
>
> It's also kind of weird that map_loose_object_1() is a noop on a
> negative descriptor. That technically makes this correct, but I think it
> would be much less surprising to always take a valid descriptor, and
> this code should do:
>
>   if (fd)
> 	return -1;
>   return map_loose_object_1(r, path, fd, size);
>
> If we are going to make map_loose_object_1() less confusing (and I think
> that is worth doing), let's go all the way.

...maybe we should go further in the other direction. I.e. with my
earlier suggestion we're left with the mess that the "fd" ownership
isn't clear.

But what I was trying to do was fix up the ownership around the
"mapped_path", but we don't need to xstrdup() it in the first place. We
already have the caller of open_loose_object() not doing that, we can
just say that you're not going to open two loose objects at a time.

Then this becomes easier, and we can just pass the maybe-NULL "const
char **oid_path" all the way to open_loose_object():


diff --git a/object-file.c b/object-file.c
index c7a513d123e..6e900737b76 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1176,7 +1176,7 @@ static int stat_loose_object(struct repository *r, const struct object_id *oid,
  * descriptor. See the caveats on the "path" parameter above.
  */
 static int open_loose_object(struct repository *r,
-			     const struct object_id *oid, const char **path)
+			     const struct object_id *oid, const char **oid_path)
 {
 	int fd;
 	struct object_directory *odb;
@@ -1185,8 +1185,12 @@ static int open_loose_object(struct repository *r,
 
 	prepare_alt_odb(r);
 	for (odb = r->objects->odb; odb; odb = odb->next) {
-		*path = odb_loose_path(odb, &buf, oid);
-		fd = git_open(*path);
+		const char *path;
+
+		path = odb_loose_path(odb, &buf, oid);
+		if (oid_path)
+			*oid_path = path;
+		fd = git_open(path);
 		if (fd >= 0)
 			return fd;
 
@@ -1214,19 +1218,22 @@ static int quick_has_loose(struct repository *r,
  * Map the loose object at "path" if it is not NULL, or the path found by
  * searching for a loose object named "oid".
  */
-static void *map_loose_object_1(struct repository *r, const char *path,
+static void *map_loose_object_1(struct repository *r, const char *const path,
 				const struct object_id *oid, unsigned long *size,
-				char **mapped_path)
+				const char **oid_path)
 {
 	void *map;
 	int fd;
 
+	if (path && oid_path)
+		BUG("don't tell me about the path, and ask me what it is!");
+	else if (!(path || oid))
+		BUG("must get an OID or a path!");
+
 	if (path)
 		fd = git_open(path);
 	else
-		fd = open_loose_object(r, oid, &path);
-	if (mapped_path)
-		*mapped_path = xstrdup(path);
+		fd = open_loose_object(r, oid, oid_path);
 
 	map = NULL;
 	if (fd >= 0) {
@@ -1236,7 +1243,8 @@ static void *map_loose_object_1(struct repository *r, const char *path,
 			*size = xsize_t(st.st_size);
 			if (!*size) {
 				/* mmap() is forbidden on empty files */
-				error(_("object file %s is empty"), path);
+				error(_("object file %s is empty"),
+				      path ? path : *oid_path);
 				close(fd);
 				return NULL;
 			}
@@ -1432,7 +1440,7 @@ static int loose_object_info(struct repository *r,
 {
 	int status = 0;
 	unsigned long mapsize;
-	char *mapped_path = NULL;
+	const char *oid_path;
 	void *map;
 	git_zstream stream;
 	char hdr[MAX_HEADER_LEN];
@@ -1464,11 +1472,9 @@ static int loose_object_info(struct repository *r,
 		return 0;
 	}
 
-	map = map_loose_object_1(r, NULL, oid, &mapsize, &mapped_path);
-	if (!map) {
-		free(mapped_path);
+	map = map_loose_object_1(r, NULL, oid, &mapsize, &oid_path);
+	if (!map)
 		return -1;
-	}
 
 	if (!oi->sizep)
 		oi->sizep = &size_scratch;
@@ -1506,11 +1512,10 @@ static int loose_object_info(struct repository *r,
 
 	if (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
 		die(_("loose object %s (stored in %s) is corrupt"),
-		    oid_to_hex(oid), mapped_path);
+		    oid_to_hex(oid), oid_path);
 
 	git_inflate_end(&stream);
 cleanup:
-	free(mapped_path);
 	munmap(map, mapsize);
 	if (oi->sizep == &size_scratch)
 		oi->sizep = NULL;

Re: [PATCH v2 2/3] object-file: emit corruption errors when detected

[Jonathan Tan]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Yeah, I think that's even better, although...

[snip]
 
> > It's also kind of weird that map_loose_object_1() is a noop on a
> > negative descriptor. That technically makes this correct, but I think it
> > would be much less surprising to always take a valid descriptor, and
> > this code should do:
> >
> >   if (fd)
> > 	return -1;
> >   return map_loose_object_1(r, path, fd, size);
> >
> > If we are going to make map_loose_object_1() less confusing (and I think
> > that is worth doing), let's go all the way.
> 
> ...maybe we should go further in the other direction. I.e. with my
> earlier suggestion we're left with the mess that the "fd" ownership
> isn't clear.

With Peff's suggestion I think we can make the function always close
the fd. If we find it not to be clear, we can rename the function
to ..._close_fd() or something like that.

Thanks to both of you for your suggestions.

Re: [PATCH v2 2/3] object-file: emit corruption errors when detected

[Ævar Arnfjörð Bjarmason]

On Wed, Dec 07 2022, Jonathan Tan wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Yeah, I think that's even better, although...
>
> [snip]
>  
>> > It's also kind of weird that map_loose_object_1() is a noop on a
>> > negative descriptor. That technically makes this correct, but I think it
>> > would be much less surprising to always take a valid descriptor, and
>> > this code should do:
>> >
>> >   if (fd)
>> > 	return -1;
>> >   return map_loose_object_1(r, path, fd, size);
>> >
>> > If we are going to make map_loose_object_1() less confusing (and I think
>> > that is worth doing), let's go all the way.
>> 
>> ...maybe we should go further in the other direction. I.e. with my
>> earlier suggestion we're left with the mess that the "fd" ownership
>> isn't clear.
>
> With Peff's suggestion I think we can make the function always close
> the fd. If we find it not to be clear, we can rename the function
> to ..._close_fd() or something like that.
>
> Thanks to both of you for your suggestions.

I think that was my suggestion.

Peff's on top of that was to never have it *open* the fd, but I'd left
one caller doing that.

I.e. that the ownership would still be passed to it, but at least it
would always be passed, and wouldn't be the mixed affair that my initial
suggestion left it at.

I'll leave it to you to pick through this.

I have a mild preference for my latest suggestion as the ownership of
all the variables seems cleanest in that iteration. I.e. we don't need
to xstrdup(), and the "fd" is always contained within
map_loose_object_1().

We still have the "sometimes a path, sometimes I make a path from an
oid" semantics though, but that seems unavoidable.

Re: [PATCH v2 2/3] object-file: emit corruption errors when detected

[Jeff King]

On Thu, Dec 08, 2022 at 12:50:27AM +0100, Ævar Arnfjörð Bjarmason wrote:

> I have a mild preference for my latest suggestion as the ownership of
> all the variables seems cleanest in that iteration. I.e. we don't need
> to xstrdup(), and the "fd" is always contained within
> map_loose_object_1().
> 
> We still have the "sometimes a path, sometimes I make a path from an
> oid" semantics though, but that seems unavoidable.

Of the two warts, I think "this function consume the fd" is less weird
than the two path variables (one sometimes-in and one sometimes-out).
If the fd thing is too ugly, we could have the function _not_ consume
the fd, but I think that probably makes the callers worse.

At any rate, we can wait and see what Jonathan comes up with.

(As an aside, thank you Jonathan for dealing with some of this
long-standing ugliness; it is not directly related to your goal, but I
think it's adjacent enough to merit doing it as part of the series).

-Peff

Re: [PATCH v2 2/3] object-file: emit corruption errors when detected

[Jeff King]

On Tue, Dec 06, 2022 at 04:40:52PM -0800, Jonathan Tan wrote:

> Note that in the RHS of this patch's diff, a check for ENOENT that was
> introduced in 3ba7a06552 (A loose object is not corrupt if it cannot
> be read due to EMFILE, 2010-10-28) is also removed. The purpose of this
> check is to avoid a false report of corruption if the errno contains
> something like EMFILE (or anything that is not ENOENT), in which case
> a more generic report is presented. Because, as of this patch, we no
> longer rely on such a heuristic to determine corruption, but surface
> the error message at the point when we read something that we did not
> expect, this check is no longer necessary.

You're right that this will not say "oops, the object is corrupted" when
we get EMFILE, etc, which is what 3ba7a06552 was fixing. But I think
after your patch, we would also never actually say "could not open
$path: too many open descriptors". I don't think that kicked in reliably
with the current code, but it seems like something we are losing in this
patch.

I.e., I think you'd want to also complain when map_loose_object()
returns anything but ENOENT. The errno value there is reliable-ish,
though it might be worth spending the extra work to preserve it across
the close() calls, just in case. Or if you split the open/mmap as Ævar
suggested, then it becomes:

  fd = open_loose_object(r, oid, &path);
  if (fd < 0) {
	if (errno != ENOENT)
		error_errno("unable to open loose object %s", path);
	return -1;
  }

  buf = map_loose_object_1(fd, path);
  if (!buf) {
	/* not much point in complaining here, as xmmap will die on
	 * error. In theory this could catch fstat() problems, but
	 * probably map_loose_object_1() itself should do so, since
	 * it already is special-casing empty files.
	 */
        close(fd);
	return -1;
  }

> diff --git a/object-file.c b/object-file.c
> index 596dd049fd..c7a513d123 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1215,7 +1215,8 @@ static int quick_has_loose(struct repository *r,
>   * searching for a loose object named "oid".
>   */
>  static void *map_loose_object_1(struct repository *r, const char *path,
> -			     const struct object_id *oid, unsigned long *size)
> +				const struct object_id *oid, unsigned long *size,
> +				char **mapped_path)
>  {
>  	void *map;
>  	int fd;
> @@ -1224,6 +1225,9 @@ static void *map_loose_object_1(struct repository *r, const char *path,
>  		fd = git_open(path);
>  	else
>  		fd = open_loose_object(r, oid, &path);
> +	if (mapped_path)
> +		*mapped_path = xstrdup(path);
> +

This introduces an extra malloc/free in every object lookup, even in the
success case where we don't even bother using the value. It's probably
not really noticeable, but it kind of feels wrong. Especially when
open_loose_object() already returns a pointer to long-ish term storage.

One solution is...

> @@ -1497,8 +1504,13 @@ static int loose_object_info(struct repository *r,
>  		break;
>  	}
>  
> +	if (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
> +		die(_("loose object %s (stored in %s) is corrupt"),
> +		    oid_to_hex(oid), mapped_path);
> +

...we could just say "loose object %s is corrupt", which is generally
sufficient (outside of alternates, you can only have one copy anyway).

But if we want to retain it, we could just redo the lookup in the error
path, which is what the existing code (that you're deleting) does, via
stat_loose_object(). That's even more wasteful than an extra
malloc/free, but you only pay the cost for a corrupted object. It's
racy, of course, but probably not in a meaningful way in practice.

Alternatively, I like Ævar's suggestion to just split
map_loose_object(). Then this code would naturally have the path, via
calling open_loose_object() itself.

> diff --git a/object-store.h b/object-store.h
> index 1be57abaf1..01134ab5ec 100644
> --- a/object-store.h
> +++ b/object-store.h
> @@ -447,6 +447,9 @@ struct object_info {
>   */
>  #define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)
>  
> +/* Die if object corruption (not just an object being missing) was detected. */
> +#define OBJECT_INFO_DIE_IF_CORRUPT 64

I have a suspicion that the world would be a better place if these die()
calls simply went away, in favor of returning -1 up the stack. But I'm
OK leaving it as-is for the sake of trying not to do too many things at
once (I probably wouldn't have even mentioned it, except that if we do
want to end up there in the long run, we'd eventually have to rip out
this new flag and its associated plumbing).

-Peff

[PATCH v2 3/3] commit: don't lazy-fetch commits

[Jonathan Tan]

When parsing commits, fail fast when the commit is missing or
corrupt, instead of attempting to fetch them. This is done by inlining
repo_read_object_file() and setting the flag that prevents fetching.

This is motivated by a situation in which through a bug (not necessarily
through Git), there was corruption in the object store of a partial
clone. In this particular case, the problem was exposed when "git gc"
tried to expire reflogs, which calls repo_parse_commit(), which triggers
fetches of the missing commits.

(There are other possible solutions to this problem including passing an
argument from "git gc" to "git reflog" to inhibit all lazy fetches, but
I think that this fix is at the wrong level - fixing "git reflog" means
that this particular command works fine, or so we think (it will fail if
it somehow needs to read a legitimately missing blob, say, a .gitmodules
file), but fixing repo_parse_commit() will fix a whole class of bugs.)

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 commit.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/commit.c b/commit.c
index 572301b80a..a02723f06b 100644
--- a/commit.c
+++ b/commit.c
@@ -508,6 +508,17 @@ int repo_parse_commit_internal(struct repository *r,
 	enum object_type type;
 	void *buffer;
 	unsigned long size;
+	struct object_info oi = {
+		.typep = &type,
+		.sizep = &size,
+		.contentp = &buffer,
+	};
+	/*
+	 * Git does not support partial clones that exclude commits, so set
+	 * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.
+	 */
+	int flags = OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT |
+		OBJECT_INFO_DIE_IF_CORRUPT;
 	int ret;
 
 	if (!item)
@@ -516,8 +527,8 @@ int repo_parse_commit_internal(struct repository *r,
 		return 0;
 	if (use_commit_graph && parse_commit_in_graph(r, item))
 		return 0;
-	buffer = repo_read_object_file(r, &item->object.oid, &type, &size);
-	if (!buffer)
+
+	if (oid_object_info_extended(r, &item->object.oid, &oi, flags) < 0)
 		return quiet_on_missing ? -1 :
 			error("Could not read %s",
 			     oid_to_hex(&item->object.oid));
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

Re: [PATCH v2 3/3] commit: don't lazy-fetch commits

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> +	/*
> +	 * Git does not support partial clones that exclude commits, so set
> +	 * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.
> +	 */
> +	int flags = OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT |
> +		OBJECT_INFO_DIE_IF_CORRUPT;

That's a quite helpful comment.

Re: [PATCH v2 3/3] commit: don't lazy-fetch commits

[Jeff King]

On Tue, Dec 06, 2022 at 04:40:53PM -0800, Jonathan Tan wrote:

> @@ -516,8 +527,8 @@ int repo_parse_commit_internal(struct repository *r,
>  		return 0;
>  	if (use_commit_graph && parse_commit_in_graph(r, item))
>  		return 0;
> -	buffer = repo_read_object_file(r, &item->object.oid, &type, &size);
> -	if (!buffer)
> +
> +	if (oid_object_info_extended(r, &item->object.oid, &oi, flags) < 0)

Nice. And this swap-out is much more obviously correct in this version
of the series because read_object_file_extended() is now clearly a thin
wrapper around oid_object_info_extended(), after your patch 2.

I actually think it would be beneficial to do a bit more refactoring
there to eliminate read_object() entirely (in favor of just having the
two callers use oid_object_info_extended() directly), and having
repo_read_object_extended() pass OBJECT_INFO_LOOKUP_REPLACE instead of
doing its own lookup.

But as that's all orthogonal to your goal, I don't mind if we punt on it
for now. We can do it later on top.

-Peff

[PATCH v3 0/4] Don't lazy-fetch commits when parsing them

[Jonathan Tan]

Thanks everyone for your review. map_loose_object_1() definitely looks
less "busy" than before after following your suggestions.

Jonathan Tan (4):
  object-file: remove OBJECT_INFO_IGNORE_LOOSE
  object-file: refactor map_loose_object_1()
  object-file: emit corruption errors when detected
  commit: don't lazy-fetch commits

 commit.c       |  15 ++++++-
 object-file.c  | 111 +++++++++++++++++++++++++------------------------
 object-store.h |   7 ++--
 3 files changed, 73 insertions(+), 60 deletions(-)

Range-diff against v2:
1:  9ad34a1dce < -:  ---------- object-file: don't exit early if skipping loose
-:  ---------- > 1:  be0b08cac2 object-file: remove OBJECT_INFO_IGNORE_LOOSE
-:  ---------- > 2:  7419e4ac70 object-file: refactor map_loose_object_1()
2:  9ddfff3585 ! 3:  7c9ed861e7 object-file: emit corruption errors when detected
    @@ Commit message
         Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
     
      ## object-file.c ##
    -@@ object-file.c: static int quick_has_loose(struct repository *r,
    -  * searching for a loose object named "oid".
    -  */
    - static void *map_loose_object_1(struct repository *r, const char *path,
    --			     const struct object_id *oid, unsigned long *size)
    -+				const struct object_id *oid, unsigned long *size,
    -+				char **mapped_path)
    - {
    - 	void *map;
    - 	int fd;
    -@@ object-file.c: static void *map_loose_object_1(struct repository *r, const char *path,
    - 		fd = git_open(path);
    - 	else
    - 		fd = open_loose_object(r, oid, &path);
    -+	if (mapped_path)
    -+		*mapped_path = xstrdup(path);
    -+
    - 	map = NULL;
    - 	if (fd >= 0) {
    - 		struct stat st;
    -@@ object-file.c: void *map_loose_object(struct repository *r,
    - 		       const struct object_id *oid,
    - 		       unsigned long *size)
    - {
    --	return map_loose_object_1(r, NULL, oid, size);
    -+	return map_loose_object_1(r, NULL, oid, size, NULL);
    - }
    - 
    - enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,
     @@ object-file.c: static int loose_object_info(struct repository *r,
      {
      	int status = 0;
      	unsigned long mapsize;
    -+	char *mapped_path = NULL;
    ++	const char *path = NULL;
      	void *map;
      	git_zstream stream;
      	char hdr[MAX_HEADER_LEN];
    @@ object-file.c: static int loose_object_info(struct repository *r,
      	}
      
     -	map = map_loose_object(r, oid, &mapsize);
    --	if (!map)
    -+	map = map_loose_object_1(r, NULL, oid, &mapsize, &mapped_path);
    -+	if (!map) {
    -+		free(mapped_path);
    ++	map = map_loose_object_1(r, oid, &mapsize, &path);
    + 	if (!map)
      		return -1;
    -+	}
      
    - 	if (!oi->sizep)
    - 		oi->sizep = &size_scratch;
     @@ object-file.c: static int loose_object_info(struct repository *r,
      		break;
      	}
      
     +	if (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
     +		die(_("loose object %s (stored in %s) is corrupt"),
    -+		    oid_to_hex(oid), mapped_path);
    ++		    oid_to_hex(oid), path);
     +
      	git_inflate_end(&stream);
      cleanup:
    -+	free(mapped_path);
      	munmap(map, mapsize);
    - 	if (oi->sizep == &size_scratch)
    - 		oi->sizep = NULL;
     @@ object-file.c: static int do_oid_object_info_extended(struct repository *r,
      			continue;
      		}
    @@ object-file.c: int force_object_loose(const struct object_id *oid, time_t mtime)
      	if (!buf)
      		return error(_("cannot read object for %s"), oid_to_hex(oid));
      	hdrlen = format_object_header(hdr, sizeof(hdr), type, len);
    -@@ object-file.c: int read_loose_object(const char *path,
    - 	char hdr[MAX_HEADER_LEN];
    - 	unsigned long *size = oi->sizep;
    - 
    --	map = map_loose_object_1(the_repository, path, NULL, &mapsize);
    -+	map = map_loose_object_1(the_repository, path, NULL, &mapsize, NULL);
    - 	if (!map) {
    - 		error_errno(_("unable to mmap %s"), path);
    - 		goto out;
     
      ## object-store.h ##
     @@ object-store.h: struct object_info {
    @@ object-store.h: struct object_info {
      #define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)
      
     +/* Die if object corruption (not just an object being missing) was detected. */
    -+#define OBJECT_INFO_DIE_IF_CORRUPT 64
    ++#define OBJECT_INFO_DIE_IF_CORRUPT 32
     +
      int oid_object_info_extended(struct repository *r,
      			     const struct object_id *,
3:  c5fe42deb0 = 4:  5924a5120b commit: don't lazy-fetch commits
-- 
2.39.0.rc1.256.g54fd8350bd-goog

[PATCH v3 1/4] object-file: remove OBJECT_INFO_IGNORE_LOOSE

[Jonathan Tan]

Its last user was removed in 97b2fa08b6 (fetch-pack: drop
custom loose object cache, 2018-11-12), so we can remove it.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-file.c  | 3 ---
 object-store.h | 4 +---
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/object-file.c b/object-file.c
index 26290554bb..cf724bc19b 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1575,9 +1575,6 @@ static int do_oid_object_info_extended(struct repository *r,
 		if (find_pack_entry(r, real, &e))
 			break;
 
-		if (flags & OBJECT_INFO_IGNORE_LOOSE)
-			return -1;
-
 		/* Most likely it's a loose object. */
 		if (!loose_object_info(r, real, oi, flags))
 			return 0;
diff --git a/object-store.h b/object-store.h
index 1be57abaf1..b1ec0bde82 100644
--- a/object-store.h
+++ b/object-store.h
@@ -434,13 +434,11 @@ struct object_info {
 #define OBJECT_INFO_ALLOW_UNKNOWN_TYPE 2
 /* Do not retry packed storage after checking packed and loose storage */
 #define OBJECT_INFO_QUICK 8
-/* Do not check loose object */
-#define OBJECT_INFO_IGNORE_LOOSE 16
 /*
  * Do not attempt to fetch the object if missing (even if fetch_is_missing is
  * nonzero).
  */
-#define OBJECT_INFO_SKIP_FETCH_OBJECT 32
+#define OBJECT_INFO_SKIP_FETCH_OBJECT 16
 /*
  * This is meant for bulk prefetching of missing blobs in a partial
  * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK
-- 
2.39.0.rc1.256.g54fd8350bd-goog

[PATCH v3 2/4] object-file: refactor map_loose_object_1()

[Jonathan Tan]

This function can do 3 things:
 1. Gets an fd given a path
 2. Simultaneously gets a path and fd given an OID
 3. Memory maps an fd

Split this function up. Only one caller needs 1, so inline that. As for
2, a future patch will also need this functionality and, in addition,
the calculated path, so extract this into a separate function with an
out parameter for the path.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-file.c | 60 +++++++++++++++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 26 deletions(-)

diff --git a/object-file.c b/object-file.c
index cf724bc19b..d99d05839f 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1211,43 +1211,48 @@ static int quick_has_loose(struct repository *r,
 }
 
 /*
- * Map the loose object at "path" if it is not NULL, or the path found by
- * searching for a loose object named "oid".
+ * Map and close the given loose object fd. The path argument is used for
+ * error reporting.
  */
-static void *map_loose_object_1(struct repository *r, const char *path,
-			     const struct object_id *oid, unsigned long *size)
+static void *map_fd(int fd, const char *path, unsigned long *size)
 {
-	void *map;
-	int fd;
-
-	if (path)
-		fd = git_open(path);
-	else
-		fd = open_loose_object(r, oid, &path);
-	map = NULL;
-	if (fd >= 0) {
-		struct stat st;
+	void *map = NULL;
+	struct stat st;
 
-		if (!fstat(fd, &st)) {
-			*size = xsize_t(st.st_size);
-			if (!*size) {
-				/* mmap() is forbidden on empty files */
-				error(_("object file %s is empty"), path);
-				close(fd);
-				return NULL;
-			}
-			map = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);
+	if (!fstat(fd, &st)) {
+		*size = xsize_t(st.st_size);
+		if (!*size) {
+			/* mmap() is forbidden on empty files */
+			error(_("object file %s is empty"), path);
+			close(fd);
+			return NULL;
 		}
-		close(fd);
+		map = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);
 	}
+	close(fd);
 	return map;
 }
 
+static void *map_loose_object_1(struct repository *r,
+				const struct object_id *oid,
+				unsigned long *size,
+				const char **path)
+{
+	const char *p;
+	int fd = open_loose_object(r, oid, &p);
+
+	if (fd < 0)
+		return NULL;
+	if (path)
+		*path = p;
+	return map_fd(fd, p, size);
+}
+
 void *map_loose_object(struct repository *r,
 		       const struct object_id *oid,
 		       unsigned long *size)
 {
-	return map_loose_object_1(r, NULL, oid, size);
+	return map_loose_object_1(r, oid, size, NULL);
 }
 
 enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,
@@ -2789,13 +2794,16 @@ int read_loose_object(const char *path,
 		      struct object_info *oi)
 {
 	int ret = -1;
+	int fd;
 	void *map = NULL;
 	unsigned long mapsize;
 	git_zstream stream;
 	char hdr[MAX_HEADER_LEN];
 	unsigned long *size = oi->sizep;
 
-	map = map_loose_object_1(the_repository, path, NULL, &mapsize);
+	fd = git_open(path);
+	if (fd >= 0)
+		map = map_fd(fd, path, &mapsize);
 	if (!map) {
 		error_errno(_("unable to mmap %s"), path);
 		goto out;
-- 
2.39.0.rc1.256.g54fd8350bd-goog

Re: [PATCH v3 2/4] object-file: refactor map_loose_object_1()

[Jeff King]

On Thu, Dec 08, 2022 at 12:57:06PM -0800, Jonathan Tan wrote:

> This function can do 3 things:
>  1. Gets an fd given a path
>  2. Simultaneously gets a path and fd given an OID
>  3. Memory maps an fd
> 
> Split this function up. Only one caller needs 1, so inline that. As for
> 2, a future patch will also need this functionality and, in addition,
> the calculated path, so extract this into a separate function with an
> out parameter for the path.

This is moving in a good direction. I like the name "map_fd" for the
helper. Being able to give it a useful name like that is a good clue
that it is doing a more focused and understandable job than the generic
map_loose_object_1(). :)

In fact...

> +static void *map_loose_object_1(struct repository *r,
> +				const struct object_id *oid,
> +				unsigned long *size,
> +				const char **path)
> +{
> +	const char *p;
> +	int fd = open_loose_object(r, oid, &p);
> +
> +	if (fd < 0)
> +		return NULL;
> +	if (path)
> +		*path = p;
> +	return map_fd(fd, p, size);
> +}
> +
>  void *map_loose_object(struct repository *r,
>  		       const struct object_id *oid,
>  		       unsigned long *size)
>  {
> -	return map_loose_object_1(r, NULL, oid, size);
> +	return map_loose_object_1(r, oid, size, NULL);
>  }

If you take my suggestion on patch 3, then the only other caller of
map_loose_object_1() goes away, and we can fold it all into one
reasonably-named function:

diff --git a/object-file.c b/object-file.c
index d99d05839f..429e3a746d 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1233,28 +1233,18 @@ static void *map_fd(int fd, const char *path, unsigned long *size)
 	return map;
 }
 
-static void *map_loose_object_1(struct repository *r,
-				const struct object_id *oid,
-				unsigned long *size,
-				const char **path)
+void *map_loose_object(struct repository *r,
+		       const struct object_id *oid,
+		       unsigned long *size)
 {
 	const char *p;
 	int fd = open_loose_object(r, oid, &p);
 
 	if (fd < 0)
 		return NULL;
-	if (path)
-		*path = p;
 	return map_fd(fd, p, size);
 }
 
-void *map_loose_object(struct repository *r,
-		       const struct object_id *oid,
-		       unsigned long *size)
-{
-	return map_loose_object_1(r, oid, size, NULL);
-}
-
 enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,
 						    unsigned char *map,
 						    unsigned long mapsize,

-Peff

Re: [PATCH v3 2/4] object-file: refactor map_loose_object_1()

[Jonathan Tan]

Jeff King <peff@peff.net> writes:
> If you take my suggestion on patch 3, then the only other caller of
> map_loose_object_1() goes away, and we can fold it all into one
> reasonably-named function:

Ah, that is true as of this patch, but patch 3 introduces another caller
of this function. I tried to allude to it in the commit message, but if
there is a clearer way to explain that, please let me know.

Re: [PATCH v3 2/4] object-file: refactor map_loose_object_1()

[Jeff King]

On Fri, Dec 09, 2022 at 10:17:04AM -0800, Jonathan Tan wrote:

> Jeff King <peff@peff.net> writes:
> > If you take my suggestion on patch 3, then the only other caller of
> > map_loose_object_1() goes away, and we can fold it all into one
> > reasonably-named function:
> 
> Ah, that is true as of this patch, but patch 3 introduces another caller
> of this function. I tried to allude to it in the commit message, but if
> there is a clearer way to explain that, please let me know.

Yes, that's the "other caller" I was referring to. :) Hopefully it is
more clear after you read my comments on v3.

-Peff

Re: [PATCH v3 2/4] object-file: refactor map_loose_object_1()

[Jeff King]

On Fri, Dec 09, 2022 at 03:27:24PM -0500, Jeff King wrote:

> On Fri, Dec 09, 2022 at 10:17:04AM -0800, Jonathan Tan wrote:
> 
> > Jeff King <peff@peff.net> writes:
> > > If you take my suggestion on patch 3, then the only other caller of
> > > map_loose_object_1() goes away, and we can fold it all into one
> > > reasonably-named function:
> > 
> > Ah, that is true as of this patch, but patch 3 introduces another caller
> > of this function. I tried to allude to it in the commit message, but if
> > there is a clearer way to explain that, please let me know.
> 
> Yes, that's the "other caller" I was referring to. :) Hopefully it is
> more clear after you read my comments on v3.

Er, that should be "...comments on patch 3", of course, not "v3".

-Peff

[PATCH v3 3/4] object-file: emit corruption errors when detected

[Jonathan Tan]

Instead of relying on errno being preserved across function calls, teach
do_oid_object_info_extended() to itself report object corruption when
it first detects it. There are 3 types of corruption being detected:
 - when a replacement object is missing
 - when a loose object is corrupt
 - when a packed object is corrupt and the object cannot be read
   in another way

Note that in the RHS of this patch's diff, a check for ENOENT that was
introduced in 3ba7a06552 (A loose object is not corrupt if it cannot
be read due to EMFILE, 2010-10-28) is also removed. The purpose of this
check is to avoid a false report of corruption if the errno contains
something like EMFILE (or anything that is not ENOENT), in which case
a more generic report is presented. Because, as of this patch, we no
longer rely on such a heuristic to determine corruption, but surface
the error message at the point when we read something that we did not
expect, this check is no longer necessary.

Besides being more resilient, this also prepares for a future patch in
which an indirect caller of do_oid_object_info_extended() will need
such functionality.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-file.c  | 48 ++++++++++++++++++++++--------------------------
 object-store.h |  3 +++
 2 files changed, 25 insertions(+), 26 deletions(-)

diff --git a/object-file.c b/object-file.c
index d99d05839f..f166065f32 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1433,6 +1433,7 @@ static int loose_object_info(struct repository *r,
 {
 	int status = 0;
 	unsigned long mapsize;
+	const char *path = NULL;
 	void *map;
 	git_zstream stream;
 	char hdr[MAX_HEADER_LEN];
@@ -1464,7 +1465,7 @@ static int loose_object_info(struct repository *r,
 		return 0;
 	}
 
-	map = map_loose_object(r, oid, &mapsize);
+	map = map_loose_object_1(r, oid, &mapsize, &path);
 	if (!map)
 		return -1;
 
@@ -1502,6 +1503,10 @@ static int loose_object_info(struct repository *r,
 		break;
 	}
 
+	if (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
+		die(_("loose object %s (stored in %s) is corrupt"),
+		    oid_to_hex(oid), path);
+
 	git_inflate_end(&stream);
 cleanup:
 	munmap(map, mapsize);
@@ -1611,6 +1616,15 @@ static int do_oid_object_info_extended(struct repository *r,
 			continue;
 		}
 
+		if (flags & OBJECT_INFO_DIE_IF_CORRUPT) {
+			const struct packed_git *p;
+			if ((flags & OBJECT_INFO_LOOKUP_REPLACE) && !oideq(real, oid))
+				die(_("replacement %s not found for %s"),
+				    oid_to_hex(real), oid_to_hex(oid));
+			if ((p = has_packed_and_bad(r, real)))
+				die(_("packed object %s (stored in %s) is corrupt"),
+				    oid_to_hex(real), p->pack_name);
+		}
 		return -1;
 	}
 
@@ -1663,7 +1677,8 @@ int oid_object_info(struct repository *r,
 
 static void *read_object(struct repository *r,
 			 const struct object_id *oid, enum object_type *type,
-			 unsigned long *size)
+			 unsigned long *size,
+			 int die_if_corrupt)
 {
 	struct object_info oi = OBJECT_INFO_INIT;
 	void *content;
@@ -1671,7 +1686,9 @@ static void *read_object(struct repository *r,
 	oi.sizep = size;
 	oi.contentp = &content;
 
-	if (oid_object_info_extended(r, oid, &oi, 0) < 0)
+	if (oid_object_info_extended(r, oid, &oi,
+				     die_if_corrupt ? OBJECT_INFO_DIE_IF_CORRUPT : 0)
+	    < 0)
 		return NULL;
 	return content;
 }
@@ -1707,35 +1724,14 @@ void *read_object_file_extended(struct repository *r,
 				int lookup_replace)
 {
 	void *data;
-	const struct packed_git *p;
-	const char *path;
-	struct stat st;
 	const struct object_id *repl = lookup_replace ?
 		lookup_replace_object(r, oid) : oid;
 
 	errno = 0;
-	data = read_object(r, repl, type, size);
+	data = read_object(r, repl, type, size, 1);
 	if (data)
 		return data;
 
-	obj_read_lock();
-	if (errno && errno != ENOENT)
-		die_errno(_("failed to read object %s"), oid_to_hex(oid));
-
-	/* die if we replaced an object with one that does not exist */
-	if (repl != oid)
-		die(_("replacement %s not found for %s"),
-		    oid_to_hex(repl), oid_to_hex(oid));
-
-	if (!stat_loose_object(r, repl, &st, &path))
-		die(_("loose object %s (stored in %s) is corrupt"),
-		    oid_to_hex(repl), path);
-
-	if ((p = has_packed_and_bad(r, repl)))
-		die(_("packed object %s (stored in %s) is corrupt"),
-		    oid_to_hex(repl), p->pack_name);
-	obj_read_unlock();
-
 	return NULL;
 }
 
@@ -2278,7 +2274,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime)
 
 	if (has_loose_object(oid))
 		return 0;
-	buf = read_object(the_repository, oid, &type, &len);
+	buf = read_object(the_repository, oid, &type, &len, 0);
 	if (!buf)
 		return error(_("cannot read object for %s"), oid_to_hex(oid));
 	hdrlen = format_object_header(hdr, sizeof(hdr), type, len);
diff --git a/object-store.h b/object-store.h
index b1ec0bde82..98c1d67946 100644
--- a/object-store.h
+++ b/object-store.h
@@ -445,6 +445,9 @@ struct object_info {
  */
 #define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)
 
+/* Die if object corruption (not just an object being missing) was detected. */
+#define OBJECT_INFO_DIE_IF_CORRUPT 32
+
 int oid_object_info_extended(struct repository *r,
 			     const struct object_id *,
 			     struct object_info *, unsigned flags);
-- 
2.39.0.rc1.256.g54fd8350bd-goog

Re: [PATCH v3 3/4] object-file: emit corruption errors when detected

[Jeff King]

On Thu, Dec 08, 2022 at 12:57:07PM -0800, Jonathan Tan wrote:

> Note that in the RHS of this patch's diff, a check for ENOENT that was
> introduced in 3ba7a06552 (A loose object is not corrupt if it cannot
> be read due to EMFILE, 2010-10-28) is also removed. The purpose of this
> check is to avoid a false report of corruption if the errno contains
> something like EMFILE (or anything that is not ENOENT), in which case
> a more generic report is presented. Because, as of this patch, we no
> longer rely on such a heuristic to determine corruption, but surface
> the error message at the point when we read something that we did not
> expect, this check is no longer necessary.

I think this version still has the small issue that we'll _only_ surface
a generic error return in such a case, and never report EMFILE
specifically. I.e., I think we'd still want something like this on top:

diff --git a/object-file.c b/object-file.c
index dc7665d6fa..36082bc991 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1422,6 +1422,7 @@ static int loose_object_info(struct repository *r,
 			     struct object_info *oi, int flags)
 {
 	int status = 0;
+	int fd;
 	unsigned long mapsize;
 	const char *path = NULL;
 	void *map;
@@ -1455,7 +1456,13 @@ static int loose_object_info(struct repository *r,
 		return 0;
 	}
 
-	map = map_loose_object_1(r, oid, &mapsize, &path);
+	fd = open_loose_object(r, oid, &path);
+	if (fd < 0) {
+		if (errno != ENOENT)
+			error_errno(_("unable to open loose object %s"), path);
+		return -1;
+	}
+	map = map_fd(fd, path, &mapsize);
 	if (!map)
 		return -1;
 

Otherwise ENOENT and EMFILE are indistinguishable from the user's
perspective. And one is normal and routine, but the other points to
something the user probably needs to fix.

-Peff

Re: [PATCH v3 3/4] object-file: emit corruption errors when detected

[Jonathan Tan]

Jeff King <peff@peff.net> writes:
> I think this version still has the small issue that we'll _only_ surface
> a generic error return in such a case, and never report EMFILE
> specifically. I.e., I think we'd still want something like this on top:

[snip]

OK, I'll do this. This also has the advantage of not using
map_loose_object_1, so I'll be able to inline it in the previous patch.

Re: [PATCH v3 3/4] object-file: emit corruption errors when detected

[Ævar Arnfjörð Bjarmason]

On Thu, Dec 08 2022, Jonathan Tan wrote:

> Instead of relying on errno being preserved across function calls, teach
> do_oid_object_info_extended() to itself report object corruption when
> it first detects it. There are 3 types of corruption being detected:
>  - when a replacement object is missing
>  - when a loose object is corrupt
>  - when a packed object is corrupt and the object cannot be read
>    in another way
>
> Note that in the RHS of this patch's diff, a check for ENOENT that was
> introduced in 3ba7a06552 (A loose object is not corrupt if it cannot
> be read due to EMFILE, 2010-10-28) is also removed. The purpose of this
> check is to avoid a false report of corruption if the errno contains
> something like EMFILE (or anything that is not ENOENT), in which case
> a more generic report is presented. Because, as of this patch, we no
> longer rely on such a heuristic to determine corruption, but surface
> the error message at the point when we read something that we did not
> expect, this check is no longer necessary.
>
> Besides being more resilient, this also prepares for a future patch in
> which an indirect caller of do_oid_object_info_extended() will need
> such functionality.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  object-file.c  | 48 ++++++++++++++++++++++--------------------------
>  object-store.h |  3 +++
>  2 files changed, 25 insertions(+), 26 deletions(-)
>
> diff --git a/object-file.c b/object-file.c
> index d99d05839f..f166065f32 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1433,6 +1433,7 @@ static int loose_object_info(struct repository *r,
>  {
>  	int status = 0;
>  	unsigned long mapsize;
> +	const char *path = NULL;

I think the NULL assignment here should either go, or it's incomplete.

Below you chechk the return value of "map", so let's either trust it to
populate "path" if it's returning success (which it does), *or* not
trust it, init this to NULL, and below add....

>  	void *map;
>  	git_zstream stream;
>  	char hdr[MAX_HEADER_LEN];
> @@ -1464,7 +1465,7 @@ static int loose_object_info(struct repository *r,
>  		return 0;
>  	}
>  
> -	map = map_loose_object(r, oid, &mapsize);
> +	map = map_loose_object_1(r, oid, &mapsize, &path);
>  	if (!map)
>  		return -1;

....

	if (!path)
		BUG("map_loose_object_1 should have given us a path");

But I don't think it's good to just hide the potential difference
between the two, especially as....

> @@ -1502,6 +1503,10 @@ static int loose_object_info(struct repository *r,
>  		break;
>  	}
>  
> +	if (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
> +		die(_("loose object %s (stored in %s) is corrupt"),
> +		    oid_to_hex(oid), path);

...I think this is leftover from a previous round (or maybe not, I
didn't check) where we did a free(path), but here we'd end up
segfaulting (not on glibc, but some platforms) if we have a NULL path.

So init-ing it didn't help us, but just helps to hide that potential
(and much worse) bug.

I think this change should also remove the existing "const char *path"
in this function from the "if"'d scope omitted in this context.

The C compiler won't care, but to the human reader it's easier to reason
about not shadowing the variable now, for as it turns out no reason, as
they're effectively independent.

>  	git_inflate_end(&stream);
>  cleanup:
>  	munmap(map, mapsize);
> @@ -1611,6 +1616,15 @@ static int do_oid_object_info_extended(struct repository *r,
>  			continue;
>  		}
>  
> +		if (flags & OBJECT_INFO_DIE_IF_CORRUPT) {
> +			const struct packed_git *p;

Nit: add an extra \n here, between decls and code.

> -	if (oid_object_info_extended(r, oid, &oi, 0) < 0)
> +	if (oid_object_info_extended(r, oid, &oi,
> +				     die_if_corrupt ? OBJECT_INFO_DIE_IF_CORRUPT : 0)
> +	    < 0)
>  		return NULL;
>  	return content;
>  }

This is a very odd coding style/wrapping, to not even end up with a line
shorter than 79 characters. You can instead do:

	if (oid_object_info_extended(r, oid, &oi, die_if_corrupt
				     ? OBJECT_INFO_DIE_IF_CORRUPT : 0) < 0)

Which is in line with our usual style, and does wrap before 79 characters...

> diff --git a/object-store.h b/object-store.h
> index b1ec0bde82..98c1d67946 100644
> --- a/object-store.h
> +++ b/object-store.h
> @@ -445,6 +445,9 @@ struct object_info {
>   */
>  #define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)
>  
> +/* Die if object corruption (not just an object being missing) was detected. */
> +#define OBJECT_INFO_DIE_IF_CORRUPT 32

Personally I wouldn't mind a short cleanup step in this series to change
these to 1<<0, 1<<1 etc., as we do for almost everything els.

I.e. in an earlier step you removed the "16", and changed that "32" to
"16", now we're adding a "32" again.

I also notice that you didn't just add a "4" here, which is an existing
gap, which turns out to be a leftover bit from your 9c8a294a1ae
(sha1-file: remove OBJECT_INFO_SKIP_CACHED, 2020-01-02) ~2 years ago :)

Maybe it's too much, and we could do it later, but something like this
as a first step:

diff --git a/object-file.c b/object-file.c
index 26290554bb4..48eff3850f5 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1534,7 +1534,8 @@ int fetch_if_missing = 1;
 
 static int do_oid_object_info_extended(struct repository *r,
 				       const struct object_id *oid,
-				       struct object_info *oi, unsigned flags)
+				       struct object_info *oi,
+				       enum object_info_flags flags)
 {
 	static struct object_info blank_oi = OBJECT_INFO_INIT;
 	struct cached_object *co;
@@ -1633,7 +1634,7 @@ static int do_oid_object_info_extended(struct repository *r,
 }
 
 int oid_object_info_extended(struct repository *r, const struct object_id *oid,
-			     struct object_info *oi, unsigned flags)
+			     struct object_info *oi, enum object_info_flags flags)
 {
 	int ret;
 	obj_read_lock();
diff --git a/object-store.h b/object-store.h
index 1be57abaf10..a20e00395b9 100644
--- a/object-store.h
+++ b/object-store.h
@@ -428,28 +428,31 @@ struct object_info {
  */
 #define OBJECT_INFO_INIT { 0 }
 
-/* Invoke lookup_replace_object() on the given hash */
-#define OBJECT_INFO_LOOKUP_REPLACE 1
-/* Allow reading from a loose object file of unknown/bogus type */
-#define OBJECT_INFO_ALLOW_UNKNOWN_TYPE 2
-/* Do not retry packed storage after checking packed and loose storage */
-#define OBJECT_INFO_QUICK 8
-/* Do not check loose object */
-#define OBJECT_INFO_IGNORE_LOOSE 16
-/*
- * Do not attempt to fetch the object if missing (even if fetch_is_missing is
- * nonzero).
- */
-#define OBJECT_INFO_SKIP_FETCH_OBJECT 32
-/*
- * This is meant for bulk prefetching of missing blobs in a partial
- * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK
- */
-#define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)
+enum object_info_flags {
+	/* Invoke lookup_replace_object() on the given hash */
+	OBJECT_INFO_LOOKUP_REPLACE = 1<<0,
+	/* Allow reading from a loose object file of unknown/bogus type */
+	OBJECT_INFO_ALLOW_UNKNOWN_TYPE = 1<<1,
+	/* Do not retry packed storage after checking packed and loose storage */
+	OBJECT_INFO_QUICK = 1<<2,
+	/* Do not check loose object */
+	OBJECT_INFO_IGNORE_LOOSE = 1<<3,
+	/*
+	 * Do not attempt to fetch the object if missing (even if fetch_is_missing is
+	 * nonzero).
+	 */
+	OBJECT_INFO_SKIP_FETCH_OBJECT = 1<<4,
+	/*
+	 * This is meant for bulk prefetching of missing blobs in a partial
+	 * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK
+	 */
+	OBJECT_INFO_FOR_PREFETCH = (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK),
+};
 
 int oid_object_info_extended(struct repository *r,
 			     const struct object_id *,
-			     struct object_info *, unsigned flags);
+			     struct object_info *,
+			     enum object_info_flags flags);
 
 /*
  * Iterate over the files in the loose-object parts of the object

Re: [PATCH v3 3/4] object-file: emit corruption errors when detected

[Jonathan Tan]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> > @@ -1433,6 +1433,7 @@ static int loose_object_info(struct repository *r,
> >  {
> >  	int status = 0;
> >  	unsigned long mapsize;
> > +	const char *path = NULL;
> 
> I think the NULL assignment here should either go, or it's incomplete.

[snip]

> So init-ing it didn't help us, but just helps to hide that potential
> (and much worse) bug.

Good catch. I'll remove the assignment.

 
> I think this change should also remove the existing "const char *path"
> in this function from the "if"'d scope omitted in this context.
> 
> The C compiler won't care, but to the human reader it's easier to reason
> about not shadowing the variable now, for as it turns out no reason, as
> they're effectively independent.

Makes sense.

> >  	git_inflate_end(&stream);
> >  cleanup:
> >  	munmap(map, mapsize);
> > @@ -1611,6 +1616,15 @@ static int do_oid_object_info_extended(struct repository *r,
> >  			continue;
> >  		}
> >  
> > +		if (flags & OBJECT_INFO_DIE_IF_CORRUPT) {
> > +			const struct packed_git *p;
> 
> Nit: add an extra \n here, between decls and code.

OK.

> > -	if (oid_object_info_extended(r, oid, &oi, 0) < 0)
> > +	if (oid_object_info_extended(r, oid, &oi,
> > +				     die_if_corrupt ? OBJECT_INFO_DIE_IF_CORRUPT : 0)
> > +	    < 0)
> >  		return NULL;
> >  	return content;
> >  }
> 
> This is a very odd coding style/wrapping, to not even end up with a line
> shorter than 79 characters. You can instead do:
> 
> 	if (oid_object_info_extended(r, oid, &oi, die_if_corrupt
> 				     ? OBJECT_INFO_DIE_IF_CORRUPT : 0) < 0)
> 
> Which is in line with our usual style, and does wrap before 79 characters...

I didn't want to split the ternary expression, but OK, I'll follow your
wrapping. 

> > diff --git a/object-store.h b/object-store.h
> > index b1ec0bde82..98c1d67946 100644
> > --- a/object-store.h
> > +++ b/object-store.h
> > @@ -445,6 +445,9 @@ struct object_info {
> >   */
> >  #define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)
> >  
> > +/* Die if object corruption (not just an object being missing) was detected. */
> > +#define OBJECT_INFO_DIE_IF_CORRUPT 32
> 
> Personally I wouldn't mind a short cleanup step in this series to change
> these to 1<<0, 1<<1 etc., as we do for almost everything els.
> 
> I.e. in an earlier step you removed the "16", and changed that "32" to
> "16", now we're adding a "32" again.
> 
> I also notice that you didn't just add a "4" here, which is an existing
> gap, which turns out to be a leftover bit from your 9c8a294a1ae
> (sha1-file: remove OBJECT_INFO_SKIP_CACHED, 2020-01-02) ~2 years ago :)

Ah...I didn't notice the 4 missing. But I think the series has gone over
enough iterations now that I'd rather leave this as-is and maybe change
this in a future patch.

[PATCH v3 4/4] commit: don't lazy-fetch commits

[Jonathan Tan]

When parsing commits, fail fast when the commit is missing or
corrupt, instead of attempting to fetch them. This is done by inlining
repo_read_object_file() and setting the flag that prevents fetching.

This is motivated by a situation in which through a bug (not necessarily
through Git), there was corruption in the object store of a partial
clone. In this particular case, the problem was exposed when "git gc"
tried to expire reflogs, which calls repo_parse_commit(), which triggers
fetches of the missing commits.

(There are other possible solutions to this problem including passing an
argument from "git gc" to "git reflog" to inhibit all lazy fetches, but
I think that this fix is at the wrong level - fixing "git reflog" means
that this particular command works fine, or so we think (it will fail if
it somehow needs to read a legitimately missing blob, say, a .gitmodules
file), but fixing repo_parse_commit() will fix a whole class of bugs.)

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 commit.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/commit.c b/commit.c
index 572301b80a..a02723f06b 100644
--- a/commit.c
+++ b/commit.c
@@ -508,6 +508,17 @@ int repo_parse_commit_internal(struct repository *r,
 	enum object_type type;
 	void *buffer;
 	unsigned long size;
+	struct object_info oi = {
+		.typep = &type,
+		.sizep = &size,
+		.contentp = &buffer,
+	};
+	/*
+	 * Git does not support partial clones that exclude commits, so set
+	 * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.
+	 */
+	int flags = OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT |
+		OBJECT_INFO_DIE_IF_CORRUPT;
 	int ret;
 
 	if (!item)
@@ -516,8 +527,8 @@ int repo_parse_commit_internal(struct repository *r,
 		return 0;
 	if (use_commit_graph && parse_commit_in_graph(r, item))
 		return 0;
-	buffer = repo_read_object_file(r, &item->object.oid, &type, &size);
-	if (!buffer)
+
+	if (oid_object_info_extended(r, &item->object.oid, &oi, flags) < 0)
 		return quiet_on_missing ? -1 :
 			error("Could not read %s",
 			     oid_to_hex(&item->object.oid));
-- 
2.39.0.rc1.256.g54fd8350bd-goog

Re: [PATCH v3 4/4] commit: don't lazy-fetch commits

[Ævar Arnfjörð Bjarmason]

On Thu, Dec 08 2022, Jonathan Tan wrote:


> diff --git a/commit.c b/commit.c
> index 572301b80a..a02723f06b 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -508,6 +508,17 @@ int repo_parse_commit_internal(struct repository *r,
>  	enum object_type type;
>  	void *buffer;
>  	unsigned long size;
> +	struct object_info oi = {
> +		.typep = &type,
> +		.sizep = &size,
> +		.contentp = &buffer,
> +	};
> +	/*
> +	 * Git does not support partial clones that exclude commits, so set
> +	 * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.
> +	 */
> +	int flags = OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT |
> +		OBJECT_INFO_DIE_IF_CORRUPT;
>  	int ret;
>  
>  	if (!item)
> @@ -516,8 +527,8 @@ int repo_parse_commit_internal(struct repository *r,
>  		return 0;
>  	if (use_commit_graph && parse_commit_in_graph(r, item))
>  		return 0;
> -	buffer = repo_read_object_file(r, &item->object.oid, &type, &size);
> -	if (!buffer)
> +
> +	if (oid_object_info_extended(r, &item->object.oid, &oi, flags) < 0)

Style: you're adding another \n here, usually we'd prefer it, but here
the function already has all these checks bundled together without a \n,
and this "if" is followed by one without it.

But then again those two "if"'s have to do with populating the "oi" and
then reading out the "type", so it's probably fine & OK.

[PATCH v4 0/4] Don't lazy-fetch commits when parsing them

[Jonathan Tan]

Thanks everyone for your comments. Here's a reroll.

Jonathan Tan (4):
  object-file: remove OBJECT_INFO_IGNORE_LOOSE
  object-file: refactor map_loose_object_1()
  object-file: emit corruption errors when detected
  commit: don't lazy-fetch commits

 commit.c       |  15 ++++++-
 object-file.c  | 108 ++++++++++++++++++++++++-------------------------
 object-store.h |   7 ++--
 3 files changed, 69 insertions(+), 61 deletions(-)

Range-diff against v3:
1:  be0b08cac2 = 1:  be0b08cac2 object-file: remove OBJECT_INFO_IGNORE_LOOSE
2:  7419e4ac70 ! 2:  4b2fb68743 object-file: refactor map_loose_object_1()
    @@ Commit message
          2. Simultaneously gets a path and fd given an OID
          3. Memory maps an fd
     
    -    Split this function up. Only one caller needs 1, so inline that. As for
    -    2, a future patch will also need this functionality and, in addition,
    -    the calculated path, so extract this into a separate function with an
    -    out parameter for the path.
    +    Keep 3 (renaming the function accordingly) and inline 1 and 2 into their
    +    respective callers.
     
         Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
     
    @@ object-file.c: static int quick_has_loose(struct repository *r,
      	return map;
      }
      
    -+static void *map_loose_object_1(struct repository *r,
    -+				const struct object_id *oid,
    -+				unsigned long *size,
    -+				const char **path)
    -+{
    +@@ object-file.c: void *map_loose_object(struct repository *r,
    + 		       const struct object_id *oid,
    + 		       unsigned long *size)
    + {
    +-	return map_loose_object_1(r, NULL, oid, size);
     +	const char *p;
     +	int fd = open_loose_object(r, oid, &p);
     +
     +	if (fd < 0)
     +		return NULL;
    -+	if (path)
    -+		*path = p;
     +	return map_fd(fd, p, size);
    -+}
    -+
    - void *map_loose_object(struct repository *r,
    - 		       const struct object_id *oid,
    - 		       unsigned long *size)
    - {
    --	return map_loose_object_1(r, NULL, oid, size);
    -+	return map_loose_object_1(r, oid, size, NULL);
      }
      
      enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,
3:  7c9ed861e7 ! 3:  07d28db92c object-file: emit corruption errors when detected
    @@ Commit message
         which an indirect caller of do_oid_object_info_extended() will need
         such functionality.
     
    +    Helped-by: Jeff King <peff@peff.net>
         Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
     
      ## object-file.c ##
     @@ object-file.c: static int loose_object_info(struct repository *r,
    + 			     struct object_info *oi, int flags)
      {
      	int status = 0;
    ++	int fd;
      	unsigned long mapsize;
    -+	const char *path = NULL;
    ++	const char *path;
      	void *map;
      	git_zstream stream;
      	char hdr[MAX_HEADER_LEN];
    +@@ object-file.c: static int loose_object_info(struct repository *r,
    + 	 * object even exists.
    + 	 */
    + 	if (!oi->typep && !oi->type_name && !oi->sizep && !oi->contentp) {
    +-		const char *path;
    + 		struct stat st;
    + 		if (!oi->disk_sizep && (flags & OBJECT_INFO_QUICK))
    + 			return quick_has_loose(r, oid) ? 0 : -1;
     @@ object-file.c: static int loose_object_info(struct repository *r,
      		return 0;
      	}
      
     -	map = map_loose_object(r, oid, &mapsize);
    -+	map = map_loose_object_1(r, oid, &mapsize, &path);
    ++	fd = open_loose_object(r, oid, &path);
    ++	if (fd < 0) {
    ++		if (errno != ENOENT)
    ++			error_errno(_("unable to open loose object %s"), path);
    ++		return -1;
    ++	}
    ++	map = map_fd(fd, path, &mapsize);
      	if (!map)
      		return -1;
      
    @@ object-file.c: static void *read_object(struct repository *r,
      	oi.contentp = &content;
      
     -	if (oid_object_info_extended(r, oid, &oi, 0) < 0)
    -+	if (oid_object_info_extended(r, oid, &oi,
    -+				     die_if_corrupt ? OBJECT_INFO_DIE_IF_CORRUPT : 0)
    -+	    < 0)
    ++	if (oid_object_info_extended(r, oid, &oi, die_if_corrupt
    ++				     ? OBJECT_INFO_DIE_IF_CORRUPT : 0) < 0)
      		return NULL;
      	return content;
      }
4:  5924a5120b = 4:  1a0cd5b244 commit: don't lazy-fetch commits
-- 
2.39.0.rc1.256.g54fd8350bd-goog

[PATCH v4 1/4] object-file: remove OBJECT_INFO_IGNORE_LOOSE

[Jonathan Tan]

Its last user was removed in 97b2fa08b6 (fetch-pack: drop
custom loose object cache, 2018-11-12), so we can remove it.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-file.c  | 3 ---
 object-store.h | 4 +---
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/object-file.c b/object-file.c
index 26290554bb..cf724bc19b 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1575,9 +1575,6 @@ static int do_oid_object_info_extended(struct repository *r,
 		if (find_pack_entry(r, real, &e))
 			break;
 
-		if (flags & OBJECT_INFO_IGNORE_LOOSE)
-			return -1;
-
 		/* Most likely it's a loose object. */
 		if (!loose_object_info(r, real, oi, flags))
 			return 0;
diff --git a/object-store.h b/object-store.h
index 1be57abaf1..b1ec0bde82 100644
--- a/object-store.h
+++ b/object-store.h
@@ -434,13 +434,11 @@ struct object_info {
 #define OBJECT_INFO_ALLOW_UNKNOWN_TYPE 2
 /* Do not retry packed storage after checking packed and loose storage */
 #define OBJECT_INFO_QUICK 8
-/* Do not check loose object */
-#define OBJECT_INFO_IGNORE_LOOSE 16
 /*
  * Do not attempt to fetch the object if missing (even if fetch_is_missing is
  * nonzero).
  */
-#define OBJECT_INFO_SKIP_FETCH_OBJECT 32
+#define OBJECT_INFO_SKIP_FETCH_OBJECT 16
 /*
  * This is meant for bulk prefetching of missing blobs in a partial
  * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK
-- 
2.39.0.rc1.256.g54fd8350bd-goog

[PATCH v4 2/4] object-file: refactor map_loose_object_1()

[Jonathan Tan]

This function can do 3 things:
 1. Gets an fd given a path
 2. Simultaneously gets a path and fd given an OID
 3. Memory maps an fd

Keep 3 (renaming the function accordingly) and inline 1 and 2 into their
respective callers.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-file.c | 50 ++++++++++++++++++++++++--------------------------
 1 file changed, 24 insertions(+), 26 deletions(-)

diff --git a/object-file.c b/object-file.c
index cf724bc19b..429e3a746d 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1211,35 +1211,25 @@ static int quick_has_loose(struct repository *r,
 }
 
 /*
- * Map the loose object at "path" if it is not NULL, or the path found by
- * searching for a loose object named "oid".
+ * Map and close the given loose object fd. The path argument is used for
+ * error reporting.
  */
-static void *map_loose_object_1(struct repository *r, const char *path,
-			     const struct object_id *oid, unsigned long *size)
+static void *map_fd(int fd, const char *path, unsigned long *size)
 {
-	void *map;
-	int fd;
-
-	if (path)
-		fd = git_open(path);
-	else
-		fd = open_loose_object(r, oid, &path);
-	map = NULL;
-	if (fd >= 0) {
-		struct stat st;
+	void *map = NULL;
+	struct stat st;
 
-		if (!fstat(fd, &st)) {
-			*size = xsize_t(st.st_size);
-			if (!*size) {
-				/* mmap() is forbidden on empty files */
-				error(_("object file %s is empty"), path);
-				close(fd);
-				return NULL;
-			}
-			map = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);
+	if (!fstat(fd, &st)) {
+		*size = xsize_t(st.st_size);
+		if (!*size) {
+			/* mmap() is forbidden on empty files */
+			error(_("object file %s is empty"), path);
+			close(fd);
+			return NULL;
 		}
-		close(fd);
+		map = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);
 	}
+	close(fd);
 	return map;
 }
 
@@ -1247,7 +1237,12 @@ void *map_loose_object(struct repository *r,
 		       const struct object_id *oid,
 		       unsigned long *size)
 {
-	return map_loose_object_1(r, NULL, oid, size);
+	const char *p;
+	int fd = open_loose_object(r, oid, &p);
+
+	if (fd < 0)
+		return NULL;
+	return map_fd(fd, p, size);
 }
 
 enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,
@@ -2789,13 +2784,16 @@ int read_loose_object(const char *path,
 		      struct object_info *oi)
 {
 	int ret = -1;
+	int fd;
 	void *map = NULL;
 	unsigned long mapsize;
 	git_zstream stream;
 	char hdr[MAX_HEADER_LEN];
 	unsigned long *size = oi->sizep;
 
-	map = map_loose_object_1(the_repository, path, NULL, &mapsize);
+	fd = git_open(path);
+	if (fd >= 0)
+		map = map_fd(fd, path, &mapsize);
 	if (!map) {
 		error_errno(_("unable to mmap %s"), path);
 		goto out;
-- 
2.39.0.rc1.256.g54fd8350bd-goog

[PATCH v4 3/4] object-file: emit corruption errors when detected

[Jonathan Tan]

Instead of relying on errno being preserved across function calls, teach
do_oid_object_info_extended() to itself report object corruption when
it first detects it. There are 3 types of corruption being detected:
 - when a replacement object is missing
 - when a loose object is corrupt
 - when a packed object is corrupt and the object cannot be read
   in another way

Note that in the RHS of this patch's diff, a check for ENOENT that was
introduced in 3ba7a06552 (A loose object is not corrupt if it cannot
be read due to EMFILE, 2010-10-28) is also removed. The purpose of this
check is to avoid a false report of corruption if the errno contains
something like EMFILE (or anything that is not ENOENT), in which case
a more generic report is presented. Because, as of this patch, we no
longer rely on such a heuristic to determine corruption, but surface
the error message at the point when we read something that we did not
expect, this check is no longer necessary.

Besides being more resilient, this also prepares for a future patch in
which an indirect caller of do_oid_object_info_extended() will need
such functionality.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-file.c  | 55 +++++++++++++++++++++++++-------------------------
 object-store.h |  3 +++
 2 files changed, 31 insertions(+), 27 deletions(-)

diff --git a/object-file.c b/object-file.c
index 429e3a746d..2a0df39822 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1422,7 +1422,9 @@ static int loose_object_info(struct repository *r,
 			     struct object_info *oi, int flags)
 {
 	int status = 0;
+	int fd;
 	unsigned long mapsize;
+	const char *path;
 	void *map;
 	git_zstream stream;
 	char hdr[MAX_HEADER_LEN];
@@ -1443,7 +1445,6 @@ static int loose_object_info(struct repository *r,
 	 * object even exists.
 	 */
 	if (!oi->typep && !oi->type_name && !oi->sizep && !oi->contentp) {
-		const char *path;
 		struct stat st;
 		if (!oi->disk_sizep && (flags & OBJECT_INFO_QUICK))
 			return quick_has_loose(r, oid) ? 0 : -1;
@@ -1454,7 +1455,13 @@ static int loose_object_info(struct repository *r,
 		return 0;
 	}
 
-	map = map_loose_object(r, oid, &mapsize);
+	fd = open_loose_object(r, oid, &path);
+	if (fd < 0) {
+		if (errno != ENOENT)
+			error_errno(_("unable to open loose object %s"), path);
+		return -1;
+	}
+	map = map_fd(fd, path, &mapsize);
 	if (!map)
 		return -1;
 
@@ -1492,6 +1499,10 @@ static int loose_object_info(struct repository *r,
 		break;
 	}
 
+	if (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
+		die(_("loose object %s (stored in %s) is corrupt"),
+		    oid_to_hex(oid), path);
+
 	git_inflate_end(&stream);
 cleanup:
 	munmap(map, mapsize);
@@ -1601,6 +1612,15 @@ static int do_oid_object_info_extended(struct repository *r,
 			continue;
 		}
 
+		if (flags & OBJECT_INFO_DIE_IF_CORRUPT) {
+			const struct packed_git *p;
+			if ((flags & OBJECT_INFO_LOOKUP_REPLACE) && !oideq(real, oid))
+				die(_("replacement %s not found for %s"),
+				    oid_to_hex(real), oid_to_hex(oid));
+			if ((p = has_packed_and_bad(r, real)))
+				die(_("packed object %s (stored in %s) is corrupt"),
+				    oid_to_hex(real), p->pack_name);
+		}
 		return -1;
 	}
 
@@ -1653,7 +1673,8 @@ int oid_object_info(struct repository *r,
 
 static void *read_object(struct repository *r,
 			 const struct object_id *oid, enum object_type *type,
-			 unsigned long *size)
+			 unsigned long *size,
+			 int die_if_corrupt)
 {
 	struct object_info oi = OBJECT_INFO_INIT;
 	void *content;
@@ -1661,7 +1682,8 @@ static void *read_object(struct repository *r,
 	oi.sizep = size;
 	oi.contentp = &content;
 
-	if (oid_object_info_extended(r, oid, &oi, 0) < 0)
+	if (oid_object_info_extended(r, oid, &oi, die_if_corrupt
+				     ? OBJECT_INFO_DIE_IF_CORRUPT : 0) < 0)
 		return NULL;
 	return content;
 }
@@ -1697,35 +1719,14 @@ void *read_object_file_extended(struct repository *r,
 				int lookup_replace)
 {
 	void *data;
-	const struct packed_git *p;
-	const char *path;
-	struct stat st;
 	const struct object_id *repl = lookup_replace ?
 		lookup_replace_object(r, oid) : oid;
 
 	errno = 0;
-	data = read_object(r, repl, type, size);
+	data = read_object(r, repl, type, size, 1);
 	if (data)
 		return data;
 
-	obj_read_lock();
-	if (errno && errno != ENOENT)
-		die_errno(_("failed to read object %s"), oid_to_hex(oid));
-
-	/* die if we replaced an object with one that does not exist */
-	if (repl != oid)
-		die(_("replacement %s not found for %s"),
-		    oid_to_hex(repl), oid_to_hex(oid));
-
-	if (!stat_loose_object(r, repl, &st, &path))
-		die(_("loose object %s (stored in %s) is corrupt"),
-		    oid_to_hex(repl), path);
-
-	if ((p = has_packed_and_bad(r, repl)))
-		die(_("packed object %s (stored in %s) is corrupt"),
-		    oid_to_hex(repl), p->pack_name);
-	obj_read_unlock();
-
 	return NULL;
 }
 
@@ -2268,7 +2269,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime)
 
 	if (has_loose_object(oid))
 		return 0;
-	buf = read_object(the_repository, oid, &type, &len);
+	buf = read_object(the_repository, oid, &type, &len, 0);
 	if (!buf)
 		return error(_("cannot read object for %s"), oid_to_hex(oid));
 	hdrlen = format_object_header(hdr, sizeof(hdr), type, len);
diff --git a/object-store.h b/object-store.h
index b1ec0bde82..98c1d67946 100644
--- a/object-store.h
+++ b/object-store.h
@@ -445,6 +445,9 @@ struct object_info {
  */
 #define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)
 
+/* Die if object corruption (not just an object being missing) was detected. */
+#define OBJECT_INFO_DIE_IF_CORRUPT 32
+
 int oid_object_info_extended(struct repository *r,
 			     const struct object_id *,
 			     struct object_info *, unsigned flags);
-- 
2.39.0.rc1.256.g54fd8350bd-goog

Re: [PATCH v4 3/4] object-file: emit corruption errors when detected

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> +	fd = open_loose_object(r, oid, &path);
> +	if (fd < 0) {
> +		if (errno != ENOENT)
> +			error_errno(_("unable to open loose object %s"), path);
> +		return -1;
> +	}

I know there was a discussion in the previous round, but is this use
of path truly safe?  Currently it may happen to be as long as there
is at least one element on the odb list, but when thinking things
through with future-proofing point of view, I do not think assuming
that path is always computable is a healthy thing to do in the
longer term.

Our "struct object_id" may be extended in the future and allow us to
express "invalid" object name, in which case the error return we get
may not even be about "loose object file not openable" but "there
will never be a loose object file for such an invalid object name",
in which case there won't be any path returned from the function.

Other than that, the series looks quite clearly written.  Nicely
done.

Thanks.

Re: [PATCH v4 3/4] object-file: emit corruption errors when detected

[Jonathan Tan]

Junio C Hamano <gitster@pobox.com> writes:
> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> > +	fd = open_loose_object(r, oid, &path);
> > +	if (fd < 0) {
> > +		if (errno != ENOENT)
> > +			error_errno(_("unable to open loose object %s"), path);
> > +		return -1;
> > +	}
> 
> I know there was a discussion in the previous round, but is this use
> of path truly safe?  Currently it may happen to be as long as there
> is at least one element on the odb list, but when thinking things
> through with future-proofing point of view, I do not think assuming
> that path is always computable is a healthy thing to do in the
> longer term.
> 
> Our "struct object_id" may be extended in the future and allow us to
> express "invalid" object name, in which case the error return we get
> may not even be about "loose object file not openable" but "there
> will never be a loose object file for such an invalid object name",
> in which case there won't be any path returned from the function.

Ah, good point. I think what I can do is to document the function to
only return a path if a path was involved in the error somehow, and make
anything that uses "path" in the caller check for NULL.
 
> Other than that, the series looks quite clearly written.  Nicely
> done.
> 
> Thanks.

Thanks for taking a look.

Re: [PATCH v4 3/4] object-file: emit corruption errors when detected

[Jeff King]

On Sat, Dec 10, 2022 at 09:16:42AM +0900, Junio C Hamano wrote:

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> > +	fd = open_loose_object(r, oid, &path);
> > +	if (fd < 0) {
> > +		if (errno != ENOENT)
> > +			error_errno(_("unable to open loose object %s"), path);
> > +		return -1;
> > +	}
> 
> I know there was a discussion in the previous round, but is this use
> of path truly safe?  Currently it may happen to be as long as there
> is at least one element on the odb list, but when thinking things
> through with future-proofing point of view, I do not think assuming
> that path is always computable is a healthy thing to do in the
> longer term.
> 
> Our "struct object_id" may be extended in the future and allow us to
> express "invalid" object name, in which case the error return we get
> may not even be about "loose object file not openable" but "there
> will never be a loose object file for such an invalid object name",
> in which case there won't be any path returned from the function.

Actually, I think it is much worse than that. The code as written above
is already buggy (which is my fault, as I suggested it).

In open_loose_object() we'll continue to iterate and pick out the "most
interesting errno". But we'll throw away the path that gave us that
errno. So we might well say:

  unable to open loose object /some/alternate/12/34abcd: permission denied

when the actual problem is in /main/objdir/12/34abcd.

It's fixable, but with some pain in handling the allocations. I think it
would be sufficient to just say:

  error_errno(_("unable to open loose object %s"), oid_to_hex(oid));

here. And possibly put a comment above open_loose_object() that "path"
is only guaranteed to point to something sensible when a non-negative
value is returned.

-Peff

Re: [PATCH v4 3/4] object-file: emit corruption errors when detected

[Jonathan Tan]

Jeff King <peff@peff.net> writes:
> Actually, I think it is much worse than that. The code as written above
> is already buggy (which is my fault, as I suggested it).
> 
> In open_loose_object() we'll continue to iterate and pick out the "most
> interesting errno". But we'll throw away the path that gave us that
> errno. So we might well say:
> 
>   unable to open loose object /some/alternate/12/34abcd: permission denied
> 
> when the actual problem is in /main/objdir/12/34abcd.
> 
> It's fixable, but with some pain in handling the allocations. I think it
> would be sufficient to just say:
> 
>   error_errno(_("unable to open loose object %s"), oid_to_hex(oid));
> 
> here. 

OK, let's go with this.

> And possibly put a comment above open_loose_object() that "path"
> is only guaranteed to point to something sensible when a non-negative
> value is returned.

Junio made a point that there could, for example, be no path when the
odb list is empty (maybe in the future) so I don't think this would be
sufficient. But there is already a comment there pointing to a comment
in another function that states "path ... (if any)" so this is something
that callers should already take care of. In my changes, I'll initialize
it to NULL and whenever I use it, I'll check for non-NULL first.

Re: [PATCH v4 3/4] object-file: emit corruption errors when detected

[Jeff King]

On Mon, Dec 12, 2022 at 12:59:55PM -0800, Jonathan Tan wrote:

> > And possibly put a comment above open_loose_object() that "path"
> > is only guaranteed to point to something sensible when a non-negative
> > value is returned.
> 
> Junio made a point that there could, for example, be no path when the
> odb list is empty (maybe in the future) so I don't think this would be
> sufficient. But there is already a comment there pointing to a comment
> in another function that states "path ... (if any)" so this is something
> that callers should already take care of. In my changes, I'll initialize
> it to NULL and whenever I use it, I'll check for non-NULL first.

If we return a non-negative value, then we opened something, so by
definition, don't we have a path of the thing we opened?

I think the case Junio mentioned was if we for some reason didn't look
at _any_ path. In which case we'd be returning an error.

-Peff

Re: [PATCH v4 3/4] object-file: emit corruption errors when detected

[Jonathan Tan]

Jeff King <peff@peff.net> writes:
> On Mon, Dec 12, 2022 at 12:59:55PM -0800, Jonathan Tan wrote:
> 
> > > And possibly put a comment above open_loose_object() that "path"
> > > is only guaranteed to point to something sensible when a non-negative
> > > value is returned.
> > 
> > Junio made a point that there could, for example, be no path when the
> > odb list is empty (maybe in the future) so I don't think this would be
> > sufficient. But there is already a comment there pointing to a comment
> > in another function that states "path ... (if any)" so this is something
> > that callers should already take care of. In my changes, I'll initialize
> > it to NULL and whenever I use it, I'll check for non-NULL first.
> 
> If we return a non-negative value, then we opened something, so by
> definition, don't we have a path of the thing we opened?
> 
> I think the case Junio mentioned was if we for some reason didn't look
> at _any_ path. In which case we'd be returning an error.

Ah, my reading comprehension is failing me, sorry. We do want "path"
to point to something sensible (well, whenever we can) when an error
occurs, though, since we want to include that path in our error message
when DIE_IF_CORRUPT is used. So guaranteeing "path" when a non-negative
value is returned (and hence, no error occurred) is not so useful.

Re: [PATCH v4 3/4] object-file: emit corruption errors when detected

[Jeff King]

On Mon, Dec 12, 2022 at 01:29:47PM -0800, Jonathan Tan wrote:

> > If we return a non-negative value, then we opened something, so by
> > definition, don't we have a path of the thing we opened?
> > 
> > I think the case Junio mentioned was if we for some reason didn't look
> > at _any_ path. In which case we'd be returning an error.
> 
> Ah, my reading comprehension is failing me, sorry. We do want "path"
> to point to something sensible (well, whenever we can) when an error
> occurs, though, since we want to include that path in our error message
> when DIE_IF_CORRUPT is used. So guaranteeing "path" when a non-negative
> value is returned (and hence, no error occurred) is not so useful.

But we only DIE_IF_CORRUPT when there is actual corruption, which means
we've opened an object file, and "path" is valid.

The only time "path" would be invalid is if open_loose_object() itself
returns an error, which is the message under discussion:

	fd = open_loose_object(r, oid, &path);
	if (fd < 0) {
		if (errno != ENOENT)
			error_errno(_("unable to open loose object %s"), path);
		return -1;
	}

If that stops expecting "path" to be valid (and just mentions the oid),
then the rest of loose_object_info() should be fine.

-Peff

Re: [PATCH v4 3/4] object-file: emit corruption errors when detected

[Jonathan Tan]

Jeff King <peff@peff.net> writes:
> On Mon, Dec 12, 2022 at 12:59:55PM -0800, Jonathan Tan wrote:
> 
> > > And possibly put a comment above open_loose_object() that "path"
> > > is only guaranteed to point to something sensible when a non-negative
> > > value is returned.
> > 
> > Junio made a point that there could, for example, be no path when the
> > odb list is empty (maybe in the future) so I don't think this would be
> > sufficient. But there is already a comment there pointing to a comment
> > in another function that states "path ... (if any)" so this is something
> > that callers should already take care of. In my changes, I'll initialize
> > it to NULL and whenever I use it, I'll check for non-NULL first.
> 
> If we return a non-negative value, then we opened something, so by
> definition, don't we have a path of the thing we opened?

Hmm...are you saying "path is guaranteed when there is no error; when
there is an error, the caller must check"? If yes, I think we are in
agreement. In any case, to make things more concrete, I've just sent a
new version [1].

[1] https://lore.kernel.org/git/cover.1670885252.git.jonathantanmy@google.com/

Re: [PATCH v4 3/4] object-file: emit corruption errors when detected

[Jeff King]

On Mon, Dec 12, 2022 at 02:52:12PM -0800, Jonathan Tan wrote:

> > > Junio made a point that there could, for example, be no path when the
> > > odb list is empty (maybe in the future) so I don't think this would be
> > > sufficient. But there is already a comment there pointing to a comment
> > > in another function that states "path ... (if any)" so this is something
> > > that callers should already take care of. In my changes, I'll initialize
> > > it to NULL and whenever I use it, I'll check for non-NULL first.
> > 
> > If we return a non-negative value, then we opened something, so by
> > definition, don't we have a path of the thing we opened?
> 
> Hmm...are you saying "path is guaranteed when there is no error; when
> there is an error, the caller must check"? If yes, I think we are in
> agreement. In any case, to make things more concrete, I've just sent a
> new version [1].

Almost. I'm saying "path is guaranteed when there is no error; when
there is an error, the value of path is meaningless and should not be
looked at".

If you want to enforce that open_loose_object() sets "path" to NULL on
error, and then say "the caller must check", that would be valid. But
without that, even checking it for NULL is pointless (because you may
see a path which got ENOENT, even though we got an interesting errno
from an earlier path).

-Peff

[PATCH v4 4/4] commit: don't lazy-fetch commits

[Jonathan Tan]

When parsing commits, fail fast when the commit is missing or
corrupt, instead of attempting to fetch them. This is done by inlining
repo_read_object_file() and setting the flag that prevents fetching.

This is motivated by a situation in which through a bug (not necessarily
through Git), there was corruption in the object store of a partial
clone. In this particular case, the problem was exposed when "git gc"
tried to expire reflogs, which calls repo_parse_commit(), which triggers
fetches of the missing commits.

(There are other possible solutions to this problem including passing an
argument from "git gc" to "git reflog" to inhibit all lazy fetches, but
I think that this fix is at the wrong level - fixing "git reflog" means
that this particular command works fine, or so we think (it will fail if
it somehow needs to read a legitimately missing blob, say, a .gitmodules
file), but fixing repo_parse_commit() will fix a whole class of bugs.)

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 commit.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/commit.c b/commit.c
index 572301b80a..a02723f06b 100644
--- a/commit.c
+++ b/commit.c
@@ -508,6 +508,17 @@ int repo_parse_commit_internal(struct repository *r,
 	enum object_type type;
 	void *buffer;
 	unsigned long size;
+	struct object_info oi = {
+		.typep = &type,
+		.sizep = &size,
+		.contentp = &buffer,
+	};
+	/*
+	 * Git does not support partial clones that exclude commits, so set
+	 * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.
+	 */
+	int flags = OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT |
+		OBJECT_INFO_DIE_IF_CORRUPT;
 	int ret;
 
 	if (!item)
@@ -516,8 +527,8 @@ int repo_parse_commit_internal(struct repository *r,
 		return 0;
 	if (use_commit_graph && parse_commit_in_graph(r, item))
 		return 0;
-	buffer = repo_read_object_file(r, &item->object.oid, &type, &size);
-	if (!buffer)
+
+	if (oid_object_info_extended(r, &item->object.oid, &oi, flags) < 0)
 		return quiet_on_missing ? -1 :
 			error("Could not read %s",
 			     oid_to_hex(&item->object.oid));
-- 
2.39.0.rc1.256.g54fd8350bd-goog

[PATCH v5 0/4] Don't lazy-fetch commits when parsing them

[Jonathan Tan]

Thanks everyone for taking a look. Here's a reroll with safer path
handling.

Jonathan Tan (4):
  object-file: remove OBJECT_INFO_IGNORE_LOOSE
  object-file: refactor map_loose_object_1()
  object-file: emit corruption errors when detected
  commit: don't lazy-fetch commits

 commit.c       |  15 ++++++-
 object-file.c  | 108 ++++++++++++++++++++++++-------------------------
 object-store.h |   7 ++--
 3 files changed, 69 insertions(+), 61 deletions(-)

Range-diff against v4:
1:  be0b08cac2 = 1:  be0b08cac2 object-file: remove OBJECT_INFO_IGNORE_LOOSE
2:  4b2fb68743 = 2:  4b2fb68743 object-file: refactor map_loose_object_1()
3:  07d28db92c ! 3:  a229ea0b11 object-file: emit corruption errors when detected
    @@ object-file.c: static int loose_object_info(struct repository *r,
      	int status = 0;
     +	int fd;
      	unsigned long mapsize;
    -+	const char *path;
    ++	const char *path = NULL;
      	void *map;
      	git_zstream stream;
      	char hdr[MAX_HEADER_LEN];
    @@ object-file.c: static int loose_object_info(struct repository *r,
     +	fd = open_loose_object(r, oid, &path);
     +	if (fd < 0) {
     +		if (errno != ENOENT)
    -+			error_errno(_("unable to open loose object %s"), path);
    ++			error_errno(_("unable to open loose object %s"), oid_to_hex(oid));
     +		return -1;
     +	}
     +	map = map_fd(fd, path, &mapsize);
    @@ object-file.c: static int loose_object_info(struct repository *r,
      		break;
      	}
      
    -+	if (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
    ++	if (status && path && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
     +		die(_("loose object %s (stored in %s) is corrupt"),
     +		    oid_to_hex(oid), path);
     +
4:  1a0cd5b244 = 4:  b54972118a commit: don't lazy-fetch commits
-- 
2.39.0.rc1.256.g54fd8350bd-goog

[PATCH v5 1/4] object-file: remove OBJECT_INFO_IGNORE_LOOSE

[Jonathan Tan]

Its last user was removed in 97b2fa08b6 (fetch-pack: drop
custom loose object cache, 2018-11-12), so we can remove it.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-file.c  | 3 ---
 object-store.h | 4 +---
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/object-file.c b/object-file.c
index 26290554bb..cf724bc19b 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1575,9 +1575,6 @@ static int do_oid_object_info_extended(struct repository *r,
 		if (find_pack_entry(r, real, &e))
 			break;
 
-		if (flags & OBJECT_INFO_IGNORE_LOOSE)
-			return -1;
-
 		/* Most likely it's a loose object. */
 		if (!loose_object_info(r, real, oi, flags))
 			return 0;
diff --git a/object-store.h b/object-store.h
index 1be57abaf1..b1ec0bde82 100644
--- a/object-store.h
+++ b/object-store.h
@@ -434,13 +434,11 @@ struct object_info {
 #define OBJECT_INFO_ALLOW_UNKNOWN_TYPE 2
 /* Do not retry packed storage after checking packed and loose storage */
 #define OBJECT_INFO_QUICK 8
-/* Do not check loose object */
-#define OBJECT_INFO_IGNORE_LOOSE 16
 /*
  * Do not attempt to fetch the object if missing (even if fetch_is_missing is
  * nonzero).
  */
-#define OBJECT_INFO_SKIP_FETCH_OBJECT 32
+#define OBJECT_INFO_SKIP_FETCH_OBJECT 16
 /*
  * This is meant for bulk prefetching of missing blobs in a partial
  * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK
-- 
2.39.0.rc1.256.g54fd8350bd-goog

[PATCH v5 2/4] object-file: refactor map_loose_object_1()

[Jonathan Tan]

This function can do 3 things:
 1. Gets an fd given a path
 2. Simultaneously gets a path and fd given an OID
 3. Memory maps an fd

Keep 3 (renaming the function accordingly) and inline 1 and 2 into their
respective callers.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-file.c | 50 ++++++++++++++++++++++++--------------------------
 1 file changed, 24 insertions(+), 26 deletions(-)

diff --git a/object-file.c b/object-file.c
index cf724bc19b..429e3a746d 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1211,35 +1211,25 @@ static int quick_has_loose(struct repository *r,
 }
 
 /*
- * Map the loose object at "path" if it is not NULL, or the path found by
- * searching for a loose object named "oid".
+ * Map and close the given loose object fd. The path argument is used for
+ * error reporting.
  */
-static void *map_loose_object_1(struct repository *r, const char *path,
-			     const struct object_id *oid, unsigned long *size)
+static void *map_fd(int fd, const char *path, unsigned long *size)
 {
-	void *map;
-	int fd;
-
-	if (path)
-		fd = git_open(path);
-	else
-		fd = open_loose_object(r, oid, &path);
-	map = NULL;
-	if (fd >= 0) {
-		struct stat st;
+	void *map = NULL;
+	struct stat st;
 
-		if (!fstat(fd, &st)) {
-			*size = xsize_t(st.st_size);
-			if (!*size) {
-				/* mmap() is forbidden on empty files */
-				error(_("object file %s is empty"), path);
-				close(fd);
-				return NULL;
-			}
-			map = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);
+	if (!fstat(fd, &st)) {
+		*size = xsize_t(st.st_size);
+		if (!*size) {
+			/* mmap() is forbidden on empty files */
+			error(_("object file %s is empty"), path);
+			close(fd);
+			return NULL;
 		}
-		close(fd);
+		map = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);
 	}
+	close(fd);
 	return map;
 }
 
@@ -1247,7 +1237,12 @@ void *map_loose_object(struct repository *r,
 		       const struct object_id *oid,
 		       unsigned long *size)
 {
-	return map_loose_object_1(r, NULL, oid, size);
+	const char *p;
+	int fd = open_loose_object(r, oid, &p);
+
+	if (fd < 0)
+		return NULL;
+	return map_fd(fd, p, size);
 }
 
 enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,
@@ -2789,13 +2784,16 @@ int read_loose_object(const char *path,
 		      struct object_info *oi)
 {
 	int ret = -1;
+	int fd;
 	void *map = NULL;
 	unsigned long mapsize;
 	git_zstream stream;
 	char hdr[MAX_HEADER_LEN];
 	unsigned long *size = oi->sizep;
 
-	map = map_loose_object_1(the_repository, path, NULL, &mapsize);
+	fd = git_open(path);
+	if (fd >= 0)
+		map = map_fd(fd, path, &mapsize);
 	if (!map) {
 		error_errno(_("unable to mmap %s"), path);
 		goto out;
-- 
2.39.0.rc1.256.g54fd8350bd-goog

[PATCH v5 3/4] object-file: emit corruption errors when detected

[Jonathan Tan]

Instead of relying on errno being preserved across function calls, teach
do_oid_object_info_extended() to itself report object corruption when
it first detects it. There are 3 types of corruption being detected:
 - when a replacement object is missing
 - when a loose object is corrupt
 - when a packed object is corrupt and the object cannot be read
   in another way

Note that in the RHS of this patch's diff, a check for ENOENT that was
introduced in 3ba7a06552 (A loose object is not corrupt if it cannot
be read due to EMFILE, 2010-10-28) is also removed. The purpose of this
check is to avoid a false report of corruption if the errno contains
something like EMFILE (or anything that is not ENOENT), in which case
a more generic report is presented. Because, as of this patch, we no
longer rely on such a heuristic to determine corruption, but surface
the error message at the point when we read something that we did not
expect, this check is no longer necessary.

Besides being more resilient, this also prepares for a future patch in
which an indirect caller of do_oid_object_info_extended() will need
such functionality.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-file.c  | 55 +++++++++++++++++++++++++-------------------------
 object-store.h |  3 +++
 2 files changed, 31 insertions(+), 27 deletions(-)

diff --git a/object-file.c b/object-file.c
index 429e3a746d..e0cef8b906 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1422,7 +1422,9 @@ static int loose_object_info(struct repository *r,
 			     struct object_info *oi, int flags)
 {
 	int status = 0;
+	int fd;
 	unsigned long mapsize;
+	const char *path = NULL;
 	void *map;
 	git_zstream stream;
 	char hdr[MAX_HEADER_LEN];
@@ -1443,7 +1445,6 @@ static int loose_object_info(struct repository *r,
 	 * object even exists.
 	 */
 	if (!oi->typep && !oi->type_name && !oi->sizep && !oi->contentp) {
-		const char *path;
 		struct stat st;
 		if (!oi->disk_sizep && (flags & OBJECT_INFO_QUICK))
 			return quick_has_loose(r, oid) ? 0 : -1;
@@ -1454,7 +1455,13 @@ static int loose_object_info(struct repository *r,
 		return 0;
 	}
 
-	map = map_loose_object(r, oid, &mapsize);
+	fd = open_loose_object(r, oid, &path);
+	if (fd < 0) {
+		if (errno != ENOENT)
+			error_errno(_("unable to open loose object %s"), oid_to_hex(oid));
+		return -1;
+	}
+	map = map_fd(fd, path, &mapsize);
 	if (!map)
 		return -1;
 
@@ -1492,6 +1499,10 @@ static int loose_object_info(struct repository *r,
 		break;
 	}
 
+	if (status && path && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
+		die(_("loose object %s (stored in %s) is corrupt"),
+		    oid_to_hex(oid), path);
+
 	git_inflate_end(&stream);
 cleanup:
 	munmap(map, mapsize);
@@ -1601,6 +1612,15 @@ static int do_oid_object_info_extended(struct repository *r,
 			continue;
 		}
 
+		if (flags & OBJECT_INFO_DIE_IF_CORRUPT) {
+			const struct packed_git *p;
+			if ((flags & OBJECT_INFO_LOOKUP_REPLACE) && !oideq(real, oid))
+				die(_("replacement %s not found for %s"),
+				    oid_to_hex(real), oid_to_hex(oid));
+			if ((p = has_packed_and_bad(r, real)))
+				die(_("packed object %s (stored in %s) is corrupt"),
+				    oid_to_hex(real), p->pack_name);
+		}
 		return -1;
 	}
 
@@ -1653,7 +1673,8 @@ int oid_object_info(struct repository *r,
 
 static void *read_object(struct repository *r,
 			 const struct object_id *oid, enum object_type *type,
-			 unsigned long *size)
+			 unsigned long *size,
+			 int die_if_corrupt)
 {
 	struct object_info oi = OBJECT_INFO_INIT;
 	void *content;
@@ -1661,7 +1682,8 @@ static void *read_object(struct repository *r,
 	oi.sizep = size;
 	oi.contentp = &content;
 
-	if (oid_object_info_extended(r, oid, &oi, 0) < 0)
+	if (oid_object_info_extended(r, oid, &oi, die_if_corrupt
+				     ? OBJECT_INFO_DIE_IF_CORRUPT : 0) < 0)
 		return NULL;
 	return content;
 }
@@ -1697,35 +1719,14 @@ void *read_object_file_extended(struct repository *r,
 				int lookup_replace)
 {
 	void *data;
-	const struct packed_git *p;
-	const char *path;
-	struct stat st;
 	const struct object_id *repl = lookup_replace ?
 		lookup_replace_object(r, oid) : oid;
 
 	errno = 0;
-	data = read_object(r, repl, type, size);
+	data = read_object(r, repl, type, size, 1);
 	if (data)
 		return data;
 
-	obj_read_lock();
-	if (errno && errno != ENOENT)
-		die_errno(_("failed to read object %s"), oid_to_hex(oid));
-
-	/* die if we replaced an object with one that does not exist */
-	if (repl != oid)
-		die(_("replacement %s not found for %s"),
-		    oid_to_hex(repl), oid_to_hex(oid));
-
-	if (!stat_loose_object(r, repl, &st, &path))
-		die(_("loose object %s (stored in %s) is corrupt"),
-		    oid_to_hex(repl), path);
-
-	if ((p = has_packed_and_bad(r, repl)))
-		die(_("packed object %s (stored in %s) is corrupt"),
-		    oid_to_hex(repl), p->pack_name);
-	obj_read_unlock();
-
 	return NULL;
 }
 
@@ -2268,7 +2269,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime)
 
 	if (has_loose_object(oid))
 		return 0;
-	buf = read_object(the_repository, oid, &type, &len);
+	buf = read_object(the_repository, oid, &type, &len, 0);
 	if (!buf)
 		return error(_("cannot read object for %s"), oid_to_hex(oid));
 	hdrlen = format_object_header(hdr, sizeof(hdr), type, len);
diff --git a/object-store.h b/object-store.h
index b1ec0bde82..98c1d67946 100644
--- a/object-store.h
+++ b/object-store.h
@@ -445,6 +445,9 @@ struct object_info {
  */
 #define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)
 
+/* Die if object corruption (not just an object being missing) was detected. */
+#define OBJECT_INFO_DIE_IF_CORRUPT 32
+
 int oid_object_info_extended(struct repository *r,
 			     const struct object_id *,
 			     struct object_info *, unsigned flags);
-- 
2.39.0.rc1.256.g54fd8350bd-goog

Re: [PATCH v5 3/4] object-file: emit corruption errors when detected

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> diff --git a/object-file.c b/object-file.c
> index 429e3a746d..e0cef8b906 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1422,7 +1422,9 @@ static int loose_object_info(struct repository *r,
>  			     struct object_info *oi, int flags)
>  {
>  	int status = 0;
> +	int fd;
>  	unsigned long mapsize;
> +	const char *path = NULL;

It may be OK to leave this uninitialized, as long as the contract of
open_loose_object() is that a successful opening will report the path
to the loose object file that was opened.  Because ...

> @@ -1454,7 +1455,13 @@ static int loose_object_info(struct repository *r,
>  		return 0;
>  	}
>  
> -	map = map_loose_object(r, oid, &mapsize);
> +	fd = open_loose_object(r, oid, &path);
> +	if (fd < 0) {
> +		if (errno != ENOENT)
> +			error_errno(_("unable to open loose object %s"), oid_to_hex(oid));
> +		return -1;

... we no longer refer to "path" which may not be populated here, and ...

> +	}

... at this point, we know we successfully opened, and populated path.

> +	map = map_fd(fd, path, &mapsize);
>  	if (!map)
>  		return -1;
>  
> @@ -1492,6 +1499,10 @@ static int loose_object_info(struct repository *r,
>  		break;
>  	}
>  
> +	if (status && path && (flags & OBJECT_INFO_DIE_IF_CORRUPT))

Also, here, "path" should be valid, as we have successfully opened
the loose object file (otherwise we would have hit the early return
that reports only the oid_to_hex(oid)).

> +		die(_("loose object %s (stored in %s) is corrupt"),
> +		    oid_to_hex(oid), path);

If we didn't have path and did not die, we'd end up ignoring
DIE_IF_CORRUPT request and force the caller to handle non-zero
status.  But I do not think that should happen, because path would
be valid here.  No?

Re: [PATCH v5 3/4] object-file: emit corruption errors when detected

[Jeff King]

On Tue, Dec 13, 2022 at 10:51:57AM +0900, Junio C Hamano wrote:

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> > diff --git a/object-file.c b/object-file.c
> > index 429e3a746d..e0cef8b906 100644
> > --- a/object-file.c
> > +++ b/object-file.c
> > @@ -1422,7 +1422,9 @@ static int loose_object_info(struct repository *r,
> >  			     struct object_info *oi, int flags)
> >  {
> >  	int status = 0;
> > +	int fd;
> >  	unsigned long mapsize;
> > +	const char *path = NULL;
> 
> It may be OK to leave this uninitialized, as long as the contract of
> open_loose_object() is that a successful opening will report the path
> to the loose object file that was opened.  Because ...

Yeah, I'd agree that this initialization can be left off (and that the
NULL checks later in the function are not needed).

-Peff

[PATCH v5 4/4] commit: don't lazy-fetch commits

[Jonathan Tan]

When parsing commits, fail fast when the commit is missing or
corrupt, instead of attempting to fetch them. This is done by inlining
repo_read_object_file() and setting the flag that prevents fetching.

This is motivated by a situation in which through a bug (not necessarily
through Git), there was corruption in the object store of a partial
clone. In this particular case, the problem was exposed when "git gc"
tried to expire reflogs, which calls repo_parse_commit(), which triggers
fetches of the missing commits.

(There are other possible solutions to this problem including passing an
argument from "git gc" to "git reflog" to inhibit all lazy fetches, but
I think that this fix is at the wrong level - fixing "git reflog" means
that this particular command works fine, or so we think (it will fail if
it somehow needs to read a legitimately missing blob, say, a .gitmodules
file), but fixing repo_parse_commit() will fix a whole class of bugs.)

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 commit.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/commit.c b/commit.c
index 572301b80a..a02723f06b 100644
--- a/commit.c
+++ b/commit.c
@@ -508,6 +508,17 @@ int repo_parse_commit_internal(struct repository *r,
 	enum object_type type;
 	void *buffer;
 	unsigned long size;
+	struct object_info oi = {
+		.typep = &type,
+		.sizep = &size,
+		.contentp = &buffer,
+	};
+	/*
+	 * Git does not support partial clones that exclude commits, so set
+	 * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.
+	 */
+	int flags = OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT |
+		OBJECT_INFO_DIE_IF_CORRUPT;
 	int ret;
 
 	if (!item)
@@ -516,8 +527,8 @@ int repo_parse_commit_internal(struct repository *r,
 		return 0;
 	if (use_commit_graph && parse_commit_in_graph(r, item))
 		return 0;
-	buffer = repo_read_object_file(r, &item->object.oid, &type, &size);
-	if (!buffer)
+
+	if (oid_object_info_extended(r, &item->object.oid, &oi, flags) < 0)
 		return quiet_on_missing ? -1 :
 			error("Could not read %s",
 			     oid_to_hex(&item->object.oid));
-- 
2.39.0.rc1.256.g54fd8350bd-goog

[PATCH v6 0/4] Don't lazy-fetch commits when parsing them

[Jonathan Tan]

Thanks everyone once again and sorry for the churn. Hopefully I got it
right this time.

open_loose_object() is documented to return the path of the object
we found, so I think we already have that covered (if we detect that
an object is corrupt, it follows that we would already have found the
object in the first place).

Jonathan Tan (4):
  object-file: remove OBJECT_INFO_IGNORE_LOOSE
  object-file: refactor map_loose_object_1()
  object-file: emit corruption errors when detected
  commit: don't lazy-fetch commits

 commit.c       |  15 ++++++-
 object-file.c  | 108 ++++++++++++++++++++++++-------------------------
 object-store.h |   7 ++--
 3 files changed, 69 insertions(+), 61 deletions(-)

Range-diff against v5:
1:  be0b08cac2 = 1:  be0b08cac2 object-file: remove OBJECT_INFO_IGNORE_LOOSE
2:  4b2fb68743 = 2:  4b2fb68743 object-file: refactor map_loose_object_1()
3:  a229ea0b11 ! 3:  811620909a object-file: emit corruption errors when detected
    @@ object-file.c: static int loose_object_info(struct repository *r,
      	int status = 0;
     +	int fd;
      	unsigned long mapsize;
    -+	const char *path = NULL;
    ++	const char *path;
      	void *map;
      	git_zstream stream;
      	char hdr[MAX_HEADER_LEN];
    @@ object-file.c: static int loose_object_info(struct repository *r,
      		break;
      	}
      
    -+	if (status && path && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
    ++	if (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
     +		die(_("loose object %s (stored in %s) is corrupt"),
     +		    oid_to_hex(oid), path);
     +
4:  b54972118a = 4:  8acf1a29e7 commit: don't lazy-fetch commits
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH v6 1/4] object-file: remove OBJECT_INFO_IGNORE_LOOSE

[Jonathan Tan]

Its last user was removed in 97b2fa08b6 (fetch-pack: drop
custom loose object cache, 2018-11-12), so we can remove it.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-file.c  | 3 ---
 object-store.h | 4 +---
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/object-file.c b/object-file.c
index 26290554bb..cf724bc19b 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1575,9 +1575,6 @@ static int do_oid_object_info_extended(struct repository *r,
 		if (find_pack_entry(r, real, &e))
 			break;
 
-		if (flags & OBJECT_INFO_IGNORE_LOOSE)
-			return -1;
-
 		/* Most likely it's a loose object. */
 		if (!loose_object_info(r, real, oi, flags))
 			return 0;
diff --git a/object-store.h b/object-store.h
index 1be57abaf1..b1ec0bde82 100644
--- a/object-store.h
+++ b/object-store.h
@@ -434,13 +434,11 @@ struct object_info {
 #define OBJECT_INFO_ALLOW_UNKNOWN_TYPE 2
 /* Do not retry packed storage after checking packed and loose storage */
 #define OBJECT_INFO_QUICK 8
-/* Do not check loose object */
-#define OBJECT_INFO_IGNORE_LOOSE 16
 /*
  * Do not attempt to fetch the object if missing (even if fetch_is_missing is
  * nonzero).
  */
-#define OBJECT_INFO_SKIP_FETCH_OBJECT 32
+#define OBJECT_INFO_SKIP_FETCH_OBJECT 16
 /*
  * This is meant for bulk prefetching of missing blobs in a partial
  * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH v6 2/4] object-file: refactor map_loose_object_1()

[Jonathan Tan]

This function can do 3 things:
 1. Gets an fd given a path
 2. Simultaneously gets a path and fd given an OID
 3. Memory maps an fd

Keep 3 (renaming the function accordingly) and inline 1 and 2 into their
respective callers.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-file.c | 50 ++++++++++++++++++++++++--------------------------
 1 file changed, 24 insertions(+), 26 deletions(-)

diff --git a/object-file.c b/object-file.c
index cf724bc19b..429e3a746d 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1211,35 +1211,25 @@ static int quick_has_loose(struct repository *r,
 }
 
 /*
- * Map the loose object at "path" if it is not NULL, or the path found by
- * searching for a loose object named "oid".
+ * Map and close the given loose object fd. The path argument is used for
+ * error reporting.
  */
-static void *map_loose_object_1(struct repository *r, const char *path,
-			     const struct object_id *oid, unsigned long *size)
+static void *map_fd(int fd, const char *path, unsigned long *size)
 {
-	void *map;
-	int fd;
-
-	if (path)
-		fd = git_open(path);
-	else
-		fd = open_loose_object(r, oid, &path);
-	map = NULL;
-	if (fd >= 0) {
-		struct stat st;
+	void *map = NULL;
+	struct stat st;
 
-		if (!fstat(fd, &st)) {
-			*size = xsize_t(st.st_size);
-			if (!*size) {
-				/* mmap() is forbidden on empty files */
-				error(_("object file %s is empty"), path);
-				close(fd);
-				return NULL;
-			}
-			map = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);
+	if (!fstat(fd, &st)) {
+		*size = xsize_t(st.st_size);
+		if (!*size) {
+			/* mmap() is forbidden on empty files */
+			error(_("object file %s is empty"), path);
+			close(fd);
+			return NULL;
 		}
-		close(fd);
+		map = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);
 	}
+	close(fd);
 	return map;
 }
 
@@ -1247,7 +1237,12 @@ void *map_loose_object(struct repository *r,
 		       const struct object_id *oid,
 		       unsigned long *size)
 {
-	return map_loose_object_1(r, NULL, oid, size);
+	const char *p;
+	int fd = open_loose_object(r, oid, &p);
+
+	if (fd < 0)
+		return NULL;
+	return map_fd(fd, p, size);
 }
 
 enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,
@@ -2789,13 +2784,16 @@ int read_loose_object(const char *path,
 		      struct object_info *oi)
 {
 	int ret = -1;
+	int fd;
 	void *map = NULL;
 	unsigned long mapsize;
 	git_zstream stream;
 	char hdr[MAX_HEADER_LEN];
 	unsigned long *size = oi->sizep;
 
-	map = map_loose_object_1(the_repository, path, NULL, &mapsize);
+	fd = git_open(path);
+	if (fd >= 0)
+		map = map_fd(fd, path, &mapsize);
 	if (!map) {
 		error_errno(_("unable to mmap %s"), path);
 		goto out;
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH v6 3/4] object-file: emit corruption errors when detected

[Jonathan Tan]

Instead of relying on errno being preserved across function calls, teach
do_oid_object_info_extended() to itself report object corruption when
it first detects it. There are 3 types of corruption being detected:
 - when a replacement object is missing
 - when a loose object is corrupt
 - when a packed object is corrupt and the object cannot be read
   in another way

Note that in the RHS of this patch's diff, a check for ENOENT that was
introduced in 3ba7a06552 (A loose object is not corrupt if it cannot
be read due to EMFILE, 2010-10-28) is also removed. The purpose of this
check is to avoid a false report of corruption if the errno contains
something like EMFILE (or anything that is not ENOENT), in which case
a more generic report is presented. Because, as of this patch, we no
longer rely on such a heuristic to determine corruption, but surface
the error message at the point when we read something that we did not
expect, this check is no longer necessary.

Besides being more resilient, this also prepares for a future patch in
which an indirect caller of do_oid_object_info_extended() will need
such functionality.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-file.c  | 55 +++++++++++++++++++++++++-------------------------
 object-store.h |  3 +++
 2 files changed, 31 insertions(+), 27 deletions(-)

diff --git a/object-file.c b/object-file.c
index 429e3a746d..e55697e378 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1422,7 +1422,9 @@ static int loose_object_info(struct repository *r,
 			     struct object_info *oi, int flags)
 {
 	int status = 0;
+	int fd;
 	unsigned long mapsize;
+	const char *path;
 	void *map;
 	git_zstream stream;
 	char hdr[MAX_HEADER_LEN];
@@ -1443,7 +1445,6 @@ static int loose_object_info(struct repository *r,
 	 * object even exists.
 	 */
 	if (!oi->typep && !oi->type_name && !oi->sizep && !oi->contentp) {
-		const char *path;
 		struct stat st;
 		if (!oi->disk_sizep && (flags & OBJECT_INFO_QUICK))
 			return quick_has_loose(r, oid) ? 0 : -1;
@@ -1454,7 +1455,13 @@ static int loose_object_info(struct repository *r,
 		return 0;
 	}
 
-	map = map_loose_object(r, oid, &mapsize);
+	fd = open_loose_object(r, oid, &path);
+	if (fd < 0) {
+		if (errno != ENOENT)
+			error_errno(_("unable to open loose object %s"), oid_to_hex(oid));
+		return -1;
+	}
+	map = map_fd(fd, path, &mapsize);
 	if (!map)
 		return -1;
 
@@ -1492,6 +1499,10 @@ static int loose_object_info(struct repository *r,
 		break;
 	}
 
+	if (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
+		die(_("loose object %s (stored in %s) is corrupt"),
+		    oid_to_hex(oid), path);
+
 	git_inflate_end(&stream);
 cleanup:
 	munmap(map, mapsize);
@@ -1601,6 +1612,15 @@ static int do_oid_object_info_extended(struct repository *r,
 			continue;
 		}
 
+		if (flags & OBJECT_INFO_DIE_IF_CORRUPT) {
+			const struct packed_git *p;
+			if ((flags & OBJECT_INFO_LOOKUP_REPLACE) && !oideq(real, oid))
+				die(_("replacement %s not found for %s"),
+				    oid_to_hex(real), oid_to_hex(oid));
+			if ((p = has_packed_and_bad(r, real)))
+				die(_("packed object %s (stored in %s) is corrupt"),
+				    oid_to_hex(real), p->pack_name);
+		}
 		return -1;
 	}
 
@@ -1653,7 +1673,8 @@ int oid_object_info(struct repository *r,
 
 static void *read_object(struct repository *r,
 			 const struct object_id *oid, enum object_type *type,
-			 unsigned long *size)
+			 unsigned long *size,
+			 int die_if_corrupt)
 {
 	struct object_info oi = OBJECT_INFO_INIT;
 	void *content;
@@ -1661,7 +1682,8 @@ static void *read_object(struct repository *r,
 	oi.sizep = size;
 	oi.contentp = &content;
 
-	if (oid_object_info_extended(r, oid, &oi, 0) < 0)
+	if (oid_object_info_extended(r, oid, &oi, die_if_corrupt
+				     ? OBJECT_INFO_DIE_IF_CORRUPT : 0) < 0)
 		return NULL;
 	return content;
 }
@@ -1697,35 +1719,14 @@ void *read_object_file_extended(struct repository *r,
 				int lookup_replace)
 {
 	void *data;
-	const struct packed_git *p;
-	const char *path;
-	struct stat st;
 	const struct object_id *repl = lookup_replace ?
 		lookup_replace_object(r, oid) : oid;
 
 	errno = 0;
-	data = read_object(r, repl, type, size);
+	data = read_object(r, repl, type, size, 1);
 	if (data)
 		return data;
 
-	obj_read_lock();
-	if (errno && errno != ENOENT)
-		die_errno(_("failed to read object %s"), oid_to_hex(oid));
-
-	/* die if we replaced an object with one that does not exist */
-	if (repl != oid)
-		die(_("replacement %s not found for %s"),
-		    oid_to_hex(repl), oid_to_hex(oid));
-
-	if (!stat_loose_object(r, repl, &st, &path))
-		die(_("loose object %s (stored in %s) is corrupt"),
-		    oid_to_hex(repl), path);
-
-	if ((p = has_packed_and_bad(r, repl)))
-		die(_("packed object %s (stored in %s) is corrupt"),
-		    oid_to_hex(repl), p->pack_name);
-	obj_read_unlock();
-
 	return NULL;
 }
 
@@ -2268,7 +2269,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime)
 
 	if (has_loose_object(oid))
 		return 0;
-	buf = read_object(the_repository, oid, &type, &len);
+	buf = read_object(the_repository, oid, &type, &len, 0);
 	if (!buf)
 		return error(_("cannot read object for %s"), oid_to_hex(oid));
 	hdrlen = format_object_header(hdr, sizeof(hdr), type, len);
diff --git a/object-store.h b/object-store.h
index b1ec0bde82..98c1d67946 100644
--- a/object-store.h
+++ b/object-store.h
@@ -445,6 +445,9 @@ struct object_info {
  */
 #define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)
 
+/* Die if object corruption (not just an object being missing) was detected. */
+#define OBJECT_INFO_DIE_IF_CORRUPT 32
+
 int oid_object_info_extended(struct repository *r,
 			     const struct object_id *,
 			     struct object_info *, unsigned flags);
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH v6 4/4] commit: don't lazy-fetch commits

[Jonathan Tan]

When parsing commits, fail fast when the commit is missing or
corrupt, instead of attempting to fetch them. This is done by inlining
repo_read_object_file() and setting the flag that prevents fetching.

This is motivated by a situation in which through a bug (not necessarily
through Git), there was corruption in the object store of a partial
clone. In this particular case, the problem was exposed when "git gc"
tried to expire reflogs, which calls repo_parse_commit(), which triggers
fetches of the missing commits.

(There are other possible solutions to this problem including passing an
argument from "git gc" to "git reflog" to inhibit all lazy fetches, but
I think that this fix is at the wrong level - fixing "git reflog" means
that this particular command works fine, or so we think (it will fail if
it somehow needs to read a legitimately missing blob, say, a .gitmodules
file), but fixing repo_parse_commit() will fix a whole class of bugs.)

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 commit.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/commit.c b/commit.c
index 572301b80a..a02723f06b 100644
--- a/commit.c
+++ b/commit.c
@@ -508,6 +508,17 @@ int repo_parse_commit_internal(struct repository *r,
 	enum object_type type;
 	void *buffer;
 	unsigned long size;
+	struct object_info oi = {
+		.typep = &type,
+		.sizep = &size,
+		.contentp = &buffer,
+	};
+	/*
+	 * Git does not support partial clones that exclude commits, so set
+	 * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.
+	 */
+	int flags = OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT |
+		OBJECT_INFO_DIE_IF_CORRUPT;
 	int ret;
 
 	if (!item)
@@ -516,8 +527,8 @@ int repo_parse_commit_internal(struct repository *r,
 		return 0;
 	if (use_commit_graph && parse_commit_in_graph(r, item))
 		return 0;
-	buffer = repo_read_object_file(r, &item->object.oid, &type, &size);
-	if (!buffer)
+
+	if (oid_object_info_extended(r, &item->object.oid, &oi, flags) < 0)
 		return quiet_on_missing ? -1 :
 			error("Could not read %s",
 			     oid_to_hex(&item->object.oid));
-- 
2.39.0.314.g84b9a713c41-goog

Re: [PATCH v6 0/4] Don't lazy-fetch commits when parsing them

[Jeff King]

On Wed, Dec 14, 2022 at 11:17:39AM -0800, Jonathan Tan wrote:

> Thanks everyone once again and sorry for the churn. Hopefully I got it
> right this time.
> 
> open_loose_object() is documented to return the path of the object
> we found, so I think we already have that covered (if we detect that
> an object is corrupt, it follows that we would already have found the
> object in the first place).

This version looks good to me. Thanks for your persistence. :) I think
the end result is very nicely done.

-Peff

Re: [PATCH v6 0/4] Don't lazy-fetch commits when parsing them

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Dec 14, 2022 at 11:17:39AM -0800, Jonathan Tan wrote:
>
>> Thanks everyone once again and sorry for the churn. Hopefully I got it
>> right this time.
>> 
>> open_loose_object() is documented to return the path of the object
>> we found, so I think we already have that covered (if we detect that
>> an object is corrupt, it follows that we would already have found the
>> object in the first place).
>
> This version looks good to me. Thanks for your persistence. :) I think
> the end result is very nicely done.

Yeah, this looks good.  Nothing added to or removed from the
previous round other than what I found questionable during the
review of that round.

Thanks, both.