Re: [PATCH v2 5/6] fsck: report invalid types recorded in objects

On Mon, Apr 26, 2021 at 04:28:30PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

quoted

@@ -92,7 +93,8 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 	switch (opt) {
 	case 't':
 		oi.type_name = &sb;
-		if (oid_object_info_extended(the_repository, &oid, &oi, flags) < 0)
+		ret = oid_object_info_extended(the_repository, &oid, &oi, flags);
+		if (!unknown_type && ret < 0)
 			die("git cat-file: could not get object info");
 		if (sb.len) {
 			printf("%s\n", sb.buf);

Surprised to see changes to cat-file here, since the commit message is
all about fsck. Did the semantics of oid_object_info_extended() change?
I.e., this hunk implies to me that it is now returning -1 when we said
unknown types were OK, and we got one. But in that case, how do we
distinguish that from a real error?

Or more concretely, this patch causes this:

  $ git cat-file -t 1234567890123456789012345678901234567890
  fatal: git cat-file: could not get object info

  $ git.compile cat-file --allow-unknown-type -t 1234567890123456789012345678901234567890
  fatal: git cat-file 1234567890123456789012345678901234567890: bad file

Or much worse, from the next hunk:

  $ git cat-file -s 1234567890123456789012345678901234567890
  fatal: git cat-file: could not get object info

  $ git cat-file --allow-unknown-type -s 1234567890123456789012345678901234567890
  140732113568960

That seems wrong (so I think my "this hunk implies" is not true, but
then I am left with: what is the point of this hunk?).

That's very well spotted.

I started re-rolling this today but ran out of time. For what it's worth
the combination of this and 6/6 "makes sense" in the sense that all
tests pass at the end of this series.

But the cases you're pointing out are ones we don't have tests for,
i.e. the combination of "allow unknown" and a non-existing object, as
opposed to a garbage one.

Hence the bug with passing up an invalid (uninitialized) size in that
case. It's fallout from other partial lib-ification changes of these
APIs, i.e. making them return bad values upstream instead of dying right
away.

I'm not sure I understand. The problem seems solely in the hunk above.
Before, if we got an error from oid_object_info_extended(), we stopped
immediately. But after, we look at the results even though it told us
there was an error. In general, I would think that a "-1" return value
from oid_object_info_extended() is "all bets are off" (remember that
unlike oid_object_info(), this is a strict error return, and not trying
to force the object type into the return value).

And that's independent of what the other patches in the series are
doing, I think.

I'll sort that out in some sensible way. Starting with adding meaningful
test coverage for the existing behavior.

Yeah, that sounds fine. I think the current behavior there is perfectly
reasonable (fail with "could not get object info").

-Peff