fsck: stop using object_info->type_name strbuf

When fsck-ing a loose object, we use object_info's type_name strbuf to
record the parsed object type as a string. For most objects this is
redundant with the object_type enum, but it does let us report the
string when we encounter an object with an unknown type (for which there
is no matching enum value).

There are a few downsides, though:

  1. The code to report these cases is not actually robust. Since we did
     not pass a strbuf to unpack_loose_header(), we only retrieved types
     from headers up to 32 bytes. In longer cases, we'd simply say
     "object corrupt or missing".

  2. This is the last caller that uses object_info's type_name strbuf
     support. It would be nice to refactor it so that we can simplify
     that code.

  3. Likewise, we'll check the hash of the object using its unknown type
     (again, as long as that type is short enough). That depends on the
     hash_object_file_literally() code, which we'd eventually like to
     get rid of.

So we can simplify things by bailing immediately in read_loose_object()
when we encounter an unknown type. This has a few user-visible effects:

  a. Instead of producing a single line of error output like this:

       error: 26ed13ce3564fbbb44e35bde42c7da717ea004a6: object is of unknown type 'bogus': .git/objects/26/ed13ce3564fbbb44e35bde42c7da717ea004a6

     we'll now issue two lines (the first from read_loose_object() when
     we see the unparsable header, and the second from the fsck code,
     since we couldn't read the object):

       error: unable to parse type from header 'bogus 4' of .git/objects/26/ed13ce3564fbbb44e35bde42c7da717ea004a6
       error: 26ed13ce3564fbbb44e35bde42c7da717ea004a6: object corrupt or missing: .git/objects/26/ed13ce3564fbbb44e35bde42c7da717ea004a6

     This is a little more verbose, but this sort of error should be
     rare (such objects are almost impossible to work with, and cannot
     be transferred between repositories as they are not representable
     in packfiles). And as a bonus, reporting the broken header in full
     could help with debugging other cases (e.g., a header like "blob
     xyzzy\0" would fail in parsing the size, but previously we'd not
     have showed the offending bytes).

  b. An object with an unknown type will be reported as corrupt, without
     actually doing a hash check. Again, I think this is unlikely to
     matter in practice since such objects are totally unusable.

We'll update one fsck test to match the new error strings. And we can
remove another test that covered the case of an object with an unknown
type _and_ a hash corruption. Since we'll skip the hash check now in
this case, the test is no longer interesting.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 6cac28356ce14f3d092c78f11871b1b6108bcde4..e7d96a9c8ea586114ad57edacb1b7c7613b5803a 100644 (file)
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -614,12 +614,11 @@ static void get_default_heads(void)
 struct for_each_loose_cb
 {
        struct progress *progress;
-       struct strbuf obj_type;
 };
 
-static int fsck_loose(const struct object_id *oid, const char *path, void *data)
+static int fsck_loose(const struct object_id *oid, const char *path,
+                     void *data UNUSED)
 {
-       struct for_each_loose_cb *cb_data = data;
        struct object *obj;
        enum object_type type = OBJ_NONE;
        unsigned long size;
@@ -629,8 +628,6 @@ static int fsck_loose(const struct object_id *oid, const char *path, void *data)
        struct object_id real_oid = *null_oid(the_hash_algo);
        int err = 0;
 
-       strbuf_reset(&cb_data->obj_type);
-       oi.type_name = &cb_data->obj_type;
        oi.sizep = &size;
        oi.typep = &type;
 
@@ -642,10 +639,6 @@ static int fsck_loose(const struct object_id *oid, const char *path, void *data)
                        err = error(_("%s: object corrupt or missing: %s"),
                                    oid_to_hex(oid), path);
        }
-       if (type != OBJ_NONE && type < 0)
-               err = error(_("%s: object is of unknown type '%s': %s"),
-                           oid_to_hex(&real_oid), cb_data->obj_type.buf,
-                           path);
        if (err < 0) {
                errors_found |= ERROR_OBJECT;
                free(contents);
@@ -697,7 +690,6 @@ static void fsck_object_dir(const char *path)
 {
        struct progress *progress = NULL;
        struct for_each_loose_cb cb_data = {
-               .obj_type = STRBUF_INIT,
                .progress = progress,
        };
 
@@ -712,7 +704,6 @@ static void fsck_object_dir(const char *path)
                                      &cb_data);
        display_progress(progress, 256);
        stop_progress(&progress);
-       strbuf_release(&cb_data.obj_type);
 }
 
 static int fsck_head_link(const char *head_ref_name,

diff --git a/object-file.c b/object-file.c
index 1127e154f61da54cabd4539a9996840cd72f3677..7a35bde96ef10aa77fdbcc8553dac0976325d5fd 100644 (file)
--- a/object-file.c
+++ b/object-file.c
@@ -1662,6 +1662,12 @@ int read_loose_object(const char *path,
                goto out_inflate;
        }
 
+       if (*oi->typep < 0) {
+               error(_("unable to parse type from header '%s' of %s"),
+                     hdr, path);
+               goto out_inflate;
+       }
+
        if (*oi->typep == OBJ_BLOB &&
            *size > repo_settings_get_big_file_threshold(the_repository)) {
                if (check_stream_oid(&stream, hdr, *size, path, expected_oid) < 0)
@@ -1672,9 +1678,9 @@ int read_loose_object(const char *path,
                        error(_("unable to unpack contents of %s"), path);
                        goto out_inflate;
                }
-               hash_object_file_literally(the_repository->hash_algo,
-                                          *contents, *size,
-                                          oi->type_name->buf, real_oid);
+               hash_object_file(the_repository->hash_algo,
+                                *contents, *size,
+                                *oi->typep, real_oid);
                if (!oideq(expected_oid, real_oid))
                        goto out_inflate;
        }

diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 0105045376245a377ea5e99cab3ab5353ca0c6a2..3f52dd5abc541bafe3062bf4043e953a6c988120 100755 (executable)
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -71,30 +71,6 @@ test_expect_success 'object with hash mismatch' '
        )
 '
 
-test_expect_success 'object with hash and type mismatch' '
-       git init --bare hash-type-mismatch &&
-       (
-               cd hash-type-mismatch &&
-
-               oid=$(echo blob | git hash-object -w --stdin -t garbage --literally) &&
-               oldoid=$oid &&
-               old=$(test_oid_to_path "$oid") &&
-               new=$(dirname $old)/$(test_oid ff_2) &&
-               oid="$(dirname $new)$(basename $new)" &&
-
-               mv objects/$old objects/$new &&
-               git update-index --add --cacheinfo 100644 $oid foo &&
-               tree=$(git write-tree) &&
-               cmt=$(echo bogus | git commit-tree $tree) &&
-               git update-ref refs/heads/bogus $cmt &&
-
-
-               test_must_fail git fsck 2>out &&
-               grep "^error: $oldoid: hash-path mismatch, found at: .*$new" out &&
-               grep "^error: $oldoid: object is of unknown type '"'"'garbage'"'"'" out
-       )
-'
-
 test_expect_success 'zlib corrupt loose object output ' '
        git init --bare corrupt-loose-output &&
        (
@@ -1001,8 +977,9 @@ test_expect_success 'fsck error and recovery on invalid object type' '
 
                test_must_fail git fsck 2>err &&
                grep -e "^error" -e "^fatal" err >errors &&
-               test_line_count = 1 errors &&
-               grep "$garbage_blob: object is of unknown type '"'"'garbage'"'"':" err
+               test_line_count = 2 errors &&
+               test_grep "unable to parse type from header .garbage" err &&
+               test_grep "$garbage_blob: object corrupt or missing:" err
        )
 '