Re: [PATCH] Always check the return value of `repo_read_object_file()`

Karthik Nayak [off-list ref] writes:

Hello,

"Johannes Schindelin via GitGitGadget" [off-list ref] writes:

quoted

diff --git a/bisect.c b/bisect.c
index f1273c787d9..f75e50c3397 100644
--- a/bisect.c
+++ b/bisect.c

@@ -158,6 +158,9 @@ static void show_list(const char *debug, int counted, int nr,
 		const char *subject_start;
 		int subject_len;

+		if (!buf)
+			die(_("unable to read %s"), oid_to_hex(&commit->object.oid));
+

Nit: We know that `repo_read_object_file()` fails on corrupt objects, so
this means that this is only happening when the object doesn't exist. I
wonder if it makes more sense to replace "unable to read %s" which is a
little ambiguous with something like "object %q doesn't exist".

I am not sure if that is a good move in the longer run.  We may
"fix" the called function to return NULL to allow callers to deal
with errors from object corruption better, at which time between
"doesn't exist" and "unable to read", the latter becomes far closer
to what actually happened (it is debatable if a corrupt thing really
exists in the first place, too).

Otherwise, the patch looks good, thanks!

I haven't read the remainder of the patch, but to me this hunk looks
OK.

Thanks.