| Message ID | cover-00.17-0000000000-20210520T111610Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | fsck: better "invalid object" error reporting | expand |
> So in some senes this matters to nobody, but I'm doing this as part of > general changes I've been pushing to make fsck/gc error reporting more > graceful, and errors more recoverable. We now have a few more places > in object-file.c where we don't just die(), but properly return > API-like return codes/data to the caller instead. Well, I guess it's useful if somehow your repository got corrupted, and you want to pinpoint where it occurred. > Ævar Arnfjörð Bjarmason (17): > fsck tests: refactor one test to use a sub-repo > fsck tests: add test for fsck-ing an unknown type > cat-file tests: test for missing object with -t and -s > cat-file tests: test that --allow-unknown-type isn't on by default > rev-list tests: test for behavior with invalid object types > cat-file tests: add corrupt loose object test > cat-file tests: test for current --allow-unknown-type behavior > cache.h: move object functions to object-store.h > object-file.c: make parse_loose_header_extended() public > object-file.c: add missing braces to loose_object_info() > object-file.c: stop dying in parse_loose_header() > object-file.c: return -2 on "header too long" in unpack_loose_header() > object-file.c: return -1, not "status" from unpack_loose_header() > fsck: don't hard die on invalid object types > object-store.h: move read_loose_object() below 'struct object_info' > fsck: report invalid types recorded in objects > fsck: report invalid object type-path combinations My main comment as a reviewer is I think that there are a lot of unrelated changes in this patch set - in particular, the first 7 tests (1 fsck test that refactors something unrelated, 1 fsck test that I presume will be overridden later, and 5 tests for other commands unrelated to fsck). I'll also give comments on individual patches.
Jonathan Tan <jonathantanmy@google.com> writes: > My main comment as a reviewer is I think that there are a lot of > unrelated changes in this patch set ... Thanks for reviewing. I share the same feeling, not specifically about this series, but I find that "doing too many while-at-it changes" is shared among the topics by the same author, and I often wish each topic were more focused.
Junio C Hamano wrote: > Jonathan Tan <jonathantanmy@google.com> writes: > > > My main comment as a reviewer is I think that there are a lot of > > unrelated changes in this patch set ... > > Thanks for reviewing. I share the same feeling, not specifically > about this series, but I find that "doing too many while-at-it > changes" is shared among the topics by the same author, and I often > wish each topic were more focused. The author has a name. I understand why as a reviewer you want a small patch series, but as a patch-writer you want your code to land on master. Perhaps if there was an actual incentive to split a patch series more people would do so, but in my personal experience that has not been the case. If I had to name the reason why some of my patches have landed on master I would say it's *arbitrary*. Maybe you catch reviewers on a good mood, or the maintainer in-between release candidates. But regardless of the actual reason, patch-series' size doesn't seem to be a huge factor. As exhibit I can five two patch-series of mine: 1. https://lore.kernel.org/git/20201223144845.143039-1-felipe.contreras@gmail.com/ 2. https://lore.kernel.org/git/20210426161458.49860-1-felipe.contreras@gmail.com/ The first one is 4 patches. The second one is 43. The second one receved feedback from the maintainer. The first one was complerely ignored. Neither were acceped. This is not intended to point fingers at anyone, merely to state the a mathematical fact. Splitting a patch series is usually more work. If there's no real incentive for a submitter to do so, why would she/he? Cheers.
A re-roll of v2's 6 patch series[1], has turned into 17 now. Less scary than one might think though, it's mostly added test coverage + splitting existing commits into more incremental chunks, but see the range-diff. This should address all the feedback on v2 + more. A brief recap summary of what this is about: We now gracefully recover instead of dying when fsck encounters types that aren't blob/commit/tree/tag. Those types don't exist in the wild, but you can manually create them with "git hash-object -t garbage --literally". So in some senes this matters to nobody, but I'm doing this as part of general changes I've been pushing to make fsck/gc error reporting more graceful, and errors more recoverable. We now have a few more places in object-file.c where we don't just die(), but properly return API-like return codes/data to the caller instead. This does not contain any changes to how --allow-unknown-type hash-object's --literally etc. work, as I suggested we could do in [2]. Any such changes will need the API changes here, but these are just the narrow fsck fixes. 1. https://lore.kernel.org/git/cover-0.6-00000000000-20210413T093734Z-avarab@gmail.com 2. https://lore.kernel.org/git/87r1i4qf4h.fsf@evledraar.gmail.com/ Ævar Arnfjörð Bjarmason (17): fsck tests: refactor one test to use a sub-repo fsck tests: add test for fsck-ing an unknown type cat-file tests: test for missing object with -t and -s cat-file tests: test that --allow-unknown-type isn't on by default rev-list tests: test for behavior with invalid object types cat-file tests: add corrupt loose object test cat-file tests: test for current --allow-unknown-type behavior cache.h: move object functions to object-store.h object-file.c: make parse_loose_header_extended() public object-file.c: add missing braces to loose_object_info() object-file.c: stop dying in parse_loose_header() object-file.c: return -2 on "header too long" in unpack_loose_header() object-file.c: return -1, not "status" from unpack_loose_header() fsck: don't hard die on invalid object types object-store.h: move read_loose_object() below 'struct object_info' fsck: report invalid types recorded in objects fsck: report invalid object type-path combinations builtin/fast-export.c | 2 +- builtin/fsck.c | 28 ++++++- builtin/index-pack.c | 2 +- builtin/mktag.c | 3 +- cache.h | 10 --- object-file.c | 156 ++++++++++++++++++------------------- object-store.h | 60 +++++++++++---- object.c | 4 +- pack-check.c | 3 +- streaming.c | 10 ++- t/t1006-cat-file.sh | 169 +++++++++++++++++++++++++++++++++++++++++ t/t1450-fsck.sh | 64 +++++++++++----- t/t6115-rev-list-du.sh | 11 +++ 13 files changed, 387 insertions(+), 135 deletions(-) Range-diff against v2: 2: 5a2cd6cca9 = 1: aa38b2bf9e fsck tests: refactor one test to use a sub-repo -: ---------- > 2: 82b64abd25 fsck tests: add test for fsck-ing an unknown type -: ---------- > 3: 7c3c2fe25d cat-file tests: test for missing object with -t and -s -: ---------- > 4: 871b820003 cat-file tests: test that --allow-unknown-type isn't on by default -: ---------- > 5: b98da9cc89 rev-list tests: test for behavior with invalid object types -: ---------- > 6: 04cc1d20f6 cat-file tests: add corrupt loose object test -: ---------- > 7: 9217320888 cat-file tests: test for current --allow-unknown-type behavior 1: 37c323a241 = 8: 12dd453879 cache.h: move object functions to object-store.h 3: d0d9cb3331 ! 9: 6a5b78dcad fsck: don't hard die on invalid object types @@ Metadata Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com> ## Commit message ## - fsck: don't hard die on invalid object types + object-file.c: make parse_loose_header_extended() public - Change builtin/fsck.c to pass down a - OBJECT_INFO_ALLOW_UNKNOWN_TYPE. This changes this very ungraceful - error: + Make the parse_loose_header_extended() function public and remove the + parse_loose_header() wrapper. The only direct user of it outside of + object-file.c itself was in streaming.c, that caller can simply pass + the required "struct object-info *" instead. - $ git hash-object --stdin -w -t garbage --literally </dev/null - <OID> - $ git fsck - fatal: invalid object type - $ - - Into: - - $ git fsck - error: hash mismatch for <OID_PATH> (expected <OID>) - error: <OID>: object corrupt or missing: <OID_PATH> - [ the rest of the fsck output here, i.e. it didn't hard die ] - - We'll still exit with non-zero, but now we'll finish the rest of the - traversal. The tests that's being added here asserts that we'll still - complain about other fsck issues (e.g. an unrelated dangling blob). - - But why are we complaining about a "hash mismatch" for an object of a - type we don't know about? We shouldn't. This is the bare minimal - change needed to not make fsck hard die on a repository that's been - corrupted in this manner. In subsequent commits we'll teach fsck to - recognize this particular type of corruption and emit a better error - message. - - The parse_loose_header() function being changed here is only used in - builtin/fsck.c, see f6371f92104 (sha1_file: add read_loose_object() - function, 2017-01-13) for its introduction. + This change is being done in preparation for teaching + read_loose_object() to accept a flag to pass to + parse_loose_header(). It isn't strictly necessary for that change, we + could simply use parse_loose_header_extended() there, but will leave + the API in a better end state. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> - ## builtin/fsck.c ## -@@ builtin/fsck.c: static int fsck_loose(const struct object_id *oid, const char *path, void *data) - void *contents; - int eaten; - -- if (read_loose_object(path, oid, &type, &size, &contents) < 0) { -+ if (read_loose_object(path, oid, &type, &size, &contents, -+ OBJECT_INFO_ALLOW_UNKNOWN_TYPE) < 0) { - errors_found |= ERROR_OBJECT; - error(_("%s: object corrupt or missing: %s"), - oid_to_hex(oid), path); - ## object-file.c ## @@ object-file.c: static void *unpack_loose_rest(git_zstream *stream, * too permissive for what we want to check. So do an anal @@ object-file.c: static int loose_object_info(struct repository *r, if (status >= 0 && oi->contentp) { @@ object-file.c: int read_loose_object(const char *path, - const struct object_id *expected_oid, - enum object_type *type, - unsigned long *size, -- void **contents) -+ void **contents, -+ unsigned int oi_flags) - { - int ret = -1; - void *map = NULL; unsigned long mapsize; git_zstream stream; char hdr[MAX_HEADER_LEN]; @@ object-file.c: int read_loose_object(const char *path, } - *type = parse_loose_header(hdr, size); -+ *type = parse_loose_header(hdr, &oi, oi_flags); ++ *type = parse_loose_header(hdr, &oi, 0); if (*type < 0) { error(_("unable to parse header of %s"), path); git_inflate_end(&stream); ## object-store.h ## -@@ object-store.h: int read_loose_object(const char *path, - const struct object_id *expected_oid, - enum object_type *type, - unsigned long *size, -- void **contents); -+ void **contents, -+ unsigned int oi_flags); - - /* Retry packed storage after checking packed and loose storage */ - #define HAS_OBJECT_RECHECK_PACKED 1 @@ object-store.h: int for_each_packed_object(each_packed_object_fn, void *, int unpack_loose_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, @@ object-store.h: int for_each_packed_object(each_packed_object_fn, void *, int finalize_object_file(const char *tmpfile, const char *filename); ## streaming.c ## -@@ streaming.c: static struct stream_vtbl loose_vtbl = { - - static open_method_decl(loose) +@@ streaming.c: static int open_istream_loose(struct git_istream *st, struct repository *r, + const struct object_id *oid, + enum object_type *type) { -+ struct object_info oi2 = OBJECT_INFO_INIT; -+ oi2.sizep = &st->size; ++ struct object_info oi = OBJECT_INFO_INIT; ++ oi.sizep = &st->size; + st->u.loose.mapped = map_loose_object(r, oid, &st->u.loose.mapsize); if (!st->u.loose.mapped) return -1; -@@ streaming.c: static open_method_decl(loose) +@@ streaming.c: static int open_istream_loose(struct git_istream *st, struct repository *r, st->u.loose.mapsize, st->u.loose.hdr, sizeof(st->u.loose.hdr)) < 0) || - (parse_loose_header(st->u.loose.hdr, &st->size) < 0)) { -+ (parse_loose_header(st->u.loose.hdr, &oi2, 0) < 0)) { ++ (parse_loose_header(st->u.loose.hdr, &oi, 0) < 0)) { git_inflate_end(&st->z); munmap(st->u.loose.mapped, st->u.loose.mapsize); return -1; - - ## t/t1450-fsck.sh ## -@@ t/t1450-fsck.sh: test_expect_success 'detect corrupt index file in fsck' ' - test_i18ngrep "bad index file" errors - ' - -+test_expect_success 'fsck error and recovery on invalid object type' ' -+ test_create_repo garbage-type && -+ empty_blob=$(git -C garbage-type hash-object --stdin -w -t blob </dev/null) && -+ garbage_blob=$(git -C garbage-type hash-object --stdin -w -t garbage --literally </dev/null) && -+ test_must_fail git -C garbage-type fsck >out 2>err && -+ grep "$garbage_blob: object corrupt or missing:" err && -+ grep "dangling blob $empty_blob" out -+' -+ - test_done -: ---------- > 10: 5d31d7e1a5 object-file.c: add missing braces to loose_object_info() -: ---------- > 11: ee28089219 object-file.c: stop dying in parse_loose_header() -: ---------- > 12: 77f2cd439c object-file.c: return -2 on "header too long" in unpack_loose_header() -: ---------- > 13: d22d5b8b85 object-file.c: return -1, not "status" from unpack_loose_header() -: ---------- > 14: 260e9888a3 fsck: don't hard die on invalid object types 4: 81fffefcf9 ! 15: e2afb813b2 object-store.h: move read_loose_object() below 'struct object_info' @@ Metadata ## Commit message ## object-store.h: move read_loose_object() below 'struct object_info' - Move the definition of read_loose_object() below "struct + Move the declaration of read_loose_object() below "struct object_info". In the next commit we'll add a "struct object_info *" parameter to it, moving it will avoid a forward declaration of the struct. 5: 5fb6ac4fae ! 16: 328f05c51b fsck: report invalid types recorded in objects @@ Commit message Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> - ## builtin/cat-file.c ## -@@ builtin/cat-file.c: static int cat_one_file(int opt, const char *exp_type, const char *obj_name, - struct strbuf sb = STRBUF_INIT; - unsigned flags = OBJECT_INFO_LOOKUP_REPLACE; - const char *path = force_path; -+ int ret; - - if (unknown_type) - flags |= OBJECT_INFO_ALLOW_UNKNOWN_TYPE; -@@ builtin/cat-file.c: 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); -@@ builtin/cat-file.c: static int cat_one_file(int opt, const char *exp_type, const char *obj_name, - - case 's': - oi.sizep = &size; -- 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"); - printf("%"PRIuMAX"\n", (uintmax_t)size); - return 0; - ## builtin/fsck.c ## @@ builtin/fsck.c: static int fsck_loose(const struct object_id *oid, const char *path, void *data) unsigned long size; @@ builtin/fsck.c: static int fsck_loose(const struct object_id *oid, const char *p ## object-file.c ## -@@ object-file.c: int parse_loose_header(const char *hdr, - * we're obtaining the type using '--allow-unknown-type' - * option. - */ -- if ((flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE) && (type < 0)) -- type = 0; -- else if (type < 0) -+ if (type < 0 && !(flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE)) - die(_("invalid object type")); - if (oi->typep) - *oi->typep = type; -@@ object-file.c: static int loose_object_info(struct repository *r, - } else if (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0) - status = error(_("unable to unpack %s header"), - oid_to_hex(oid)); -- if (status < 0) -+ if (status < 0) { - ; /* Do nothing */ -- else if (hdrbuf.len) { -+ } else if (hdrbuf.len) { - if ((status = parse_loose_header(hdrbuf.buf, oi, flags)) < 0) - status = error(_("unable to parse %s header with --allow-unknown-type"), - oid_to_hex(oid)); -- } else if ((status = parse_loose_header(hdr, oi, flags)) < 0) -- status = error(_("unable to parse %s header"), oid_to_hex(oid)); -+ } else { -+ status = parse_loose_header(hdr, oi, flags); -+ if (status < 0 && !(flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE)) -+ error(_("unable to parse %s header"), oid_to_hex(oid)); -+ } - - if (status >= 0 && oi->contentp) { - *oi->contentp = unpack_loose_rest(&stream, hdr, @@ object-file.c: static int check_stream_oid(git_zstream *stream, int read_loose_object(const char *path, @@ object-file.c: int read_loose_object(const char *path, git_zstream stream; char hdr[MAX_HEADER_LEN]; - struct object_info oi = OBJECT_INFO_INIT; + int allow_unknown = oi_flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE; +- oi.typep = type; - oi.sizep = size; + enum object_type *type = oi->typep; + unsigned long *size = oi->sizep; @@ object-file.c: int read_loose_object(const char *path, goto out; } -- *type = parse_loose_header(hdr, &oi, oi_flags); -- if (*type < 0) { -- error(_("unable to parse header of %s"), path); -+ *type = parse_loose_header(hdr, oi, oi_flags); -+ if (*type < 0 && !(oi_flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE)) { -+ error(_("unable to parse header %s"), path); +- if (parse_loose_header(hdr, &oi) < 0) { ++ if (parse_loose_header(hdr, oi) < 0) { + error(_("unable to parse header of %s"), path); git_inflate_end(&stream); goto out; - } @@ object-file.c: int read_loose_object(const char *path, goto out; } @@ t/t1450-fsck.sh: test_expect_success 'object with hash mismatch' ' git rev-parse HEAD^{tree} >.git/refs/heads/invalid && test_when_finished "git update-ref -d refs/heads/invalid" && @@ t/t1450-fsck.sh: test_expect_success 'fsck error and recovery on invalid object type' ' - empty_blob=$(git -C garbage-type hash-object --stdin -w -t blob </dev/null) && garbage_blob=$(git -C garbage-type hash-object --stdin -w -t garbage --literally </dev/null) && test_must_fail git -C garbage-type fsck >out 2>err && + grep -e "^error" -e "^fatal" err >errors && +- test_line_count = 2 errors && +- grep "error: hash mismatch for" err && - grep "$garbage_blob: object corrupt or missing:" err && ++ test_line_count = 1 errors && + grep "$garbage_blob: object is of unknown type '"'"'garbage'"'"':" err && -+ grep error: err >err.errors && -+ test_line_count = 1 err.errors && grep "dangling blob $empty_blob" out ' 6: 226d2031bc ! 17: c5e6686765 fsck: report invalid object type-path combinations @@ Metadata ## Commit message ## fsck: report invalid object type-path combinations - fsck: improve error on loose object hash mismatch - Improve the error that's emitted in cases where we find a loose object we parse, but which isn't at the location we expect it to be. @@ builtin/fsck.c: static int fsck_loose(const struct object_id *oid, const char *p struct strbuf sb = STRBUF_INIT; unsigned int oi_flags = OBJECT_INFO_ALLOW_UNKNOWN_TYPE; struct object_info oi; -+ struct object_id real_oid = null_oid; ++ struct object_id real_oid = *null_oid(); int found = 0; oi.type_name = &sb; oi.sizep = &size; @@ object-file.c: int check_object_signature(struct repository *r, const struct obj break; r->hash_algo->update_fn(&c, buf, readlen); } -- r->hash_algo->final_fn(real_oid.hash, &c); -+ r->hash_algo->final_fn(real_oid->hash, &c); +- r->hash_algo->final_oid_fn(&real_oid, &c); ++ r->hash_algo->final_oid_fn(real_oid, &c); close_istream(st); - return !oideq(oid, &real_oid) ? -1 : 0; + return !oideq(oid, real_oid) ? -1 : 0; @@ object-file.c: int read_loose_object(const char *path, - error(_("hash mismatch for %s (expected %s)"), path, - oid_to_hex(expected_oid)); + *contents, *size, oi->type_name->buf, real_oid)) { -+ if (oideq(real_oid, &null_oid)) ++ if (oideq(real_oid, null_oid())) + BUG("should only get OID mismatch errors with mapped contents"); free(*contents); goto out; @@ object-store.h: int oid_object_info_extended(struct repository *r, struct object_info *oi, unsigned int oi_flags); @@ object-store.h: int unpack_loose_header(git_zstream *stream, unsigned char *map, - int parse_loose_header(const char *hdr, struct object_info *oi, - unsigned int flags); + int parse_loose_header(const char *hdr, struct object_info *oi); + int check_object_signature(struct repository *r, const struct object_id *oid, - void *buf, unsigned long size, const char *type); + void *buf, unsigned long size, const char *type, @@ pack-check.c: static int verify_packfile(struct repository *r, oid_to_hex(&oid), p->pack_name); else if (fn) { + ## t/t1006-cat-file.sh ## +@@ t/t1006-cat-file.sh: test_expect_success 'cat-file -t and -s on corrupt loose object' ' + # Swap the two to corrupt the repository + mv -v "$other_path" "$empty_path" && + test_must_fail git fsck 2>err.fsck && +- grep "hash mismatch" err.fsck && ++ grep "hash-path mismatch" err.fsck && + + # confirm that cat-file is reading the new swapped-in + # blob... + ## t/t1450-fsck.sh ## @@ t/t1450-fsck.sh: test_expect_success 'object with hash mismatch' ' (