See the v2 for a summary and goals: https://lore.kernel.org/git/cover-v2-00.11-00000000000-20220204T135005Z-avarab@xxxxxxxxx/ This early re-roll addresses Junio's comments on it so far, i.e. the minor whitespace/typos, and then having the interface retain the "if (f() < 0)" interface. The reason I'd changed it initially was because the callers used a mixture of "if (f())" and "if (f() < 0)", but let's just keep it as it is, with a couple of new patches to fix the existing callers to use "if (f() < 0)" consistently. A corresponding re-roll to the topic that depends on this one isn't needed, as this part of the API wasn't used by it. For that topic see: https://lore.kernel.org/git/cover-v10-0.6-00000000000-20220204T135538Z-avarab@xxxxxxxxx/ Ævar Arnfjörð Bjarmason (12): object-file.c: split up declaration of unrelated variables object-file API: return "void", not "int" from hash_object_file() object-file API: add a format_object_header() function object-file API: have write_object_file() take "enum object_type" object API: correct "buf" v.s. "map" mismatch in *.c and *.h object API docs: move check_object_signature() docs to cache.h object API users + docs: check <0, not !0 with check_object_signature() object-file API: split up and simplify check_object_signature() object API: rename hash_object_file_literally() to write_*() object-file API: have hash_object_file() take "enum object_type" object-file.c: add a literal version of write_object_file_prepare() object-file API: pass an enum to read_object_with_reference() apply.c | 12 ++-- builtin/cat-file.c | 11 +-- builtin/checkout.c | 2 +- builtin/fast-export.c | 2 +- builtin/fast-import.c | 12 ++-- builtin/grep.c | 4 +- builtin/hash-object.c | 2 +- builtin/index-pack.c | 10 ++- builtin/mktag.c | 9 ++- builtin/mktree.c | 2 +- builtin/notes.c | 2 +- builtin/pack-objects.c | 2 +- builtin/receive-pack.c | 2 +- builtin/replace.c | 4 +- builtin/tag.c | 2 +- builtin/unpack-objects.c | 8 +-- bulk-checkin.c | 4 +- cache-tree.c | 8 +-- cache.h | 20 +++++- commit.c | 2 +- convert.c | 2 +- diffcore-rename.c | 2 +- dir.c | 2 +- http-push.c | 2 +- log-tree.c | 2 +- match-trees.c | 2 +- merge-ort.c | 4 +- merge-recursive.c | 2 +- notes-cache.c | 2 +- notes.c | 8 +-- object-file.c | 145 +++++++++++++++++++++++++-------------- object-store.h | 24 ++++--- object.c | 5 +- pack-check.c | 9 ++- read-cache.c | 2 +- tree-walk.c | 6 +- 36 files changed, 202 insertions(+), 137 deletions(-) Range-diff against v2: -: ----------- > 1: 53334bc970a object-file.c: split up declaration of unrelated variables -: ----------- > 2: 5ba49778ac1 object-file API: return "void", not "int" from hash_object_file() -: ----------- > 3: 92fd020d199 object-file API: add a format_object_header() function 1: d259f901114 ! 4: 795ac3dea2a object-file API: have write_object_file() take "enum object_type" @@ builtin/notes.c: static void prepare_note_data(const struct object_id *object, s - if (write_object_file(d->buf.buf, d->buf.len, blob_type, oid)) { + if (write_object_file(d->buf.buf, d->buf.len, OBJ_BLOB, oid)) { int status = die_message(_("unable to write note object")); -- + if (d->edit_path) - die_message(_("the note contents have been left in %s"), - d->edit_path); ## builtin/receive-pack.c ## @@ builtin/receive-pack.c: static void prepare_push_cert_sha1(struct child_process *proc) 2: 207aec4eb64 ! 5: 6faf46277b5 object API: correct "buf" v.s. "map" mismatch in *.c and *.h @@ Commit message Change the name of the second argument to check_object_signature() to be "buf" in object-file.c, making it consistent with the prototype in - cache..h + cache.h This fixes an inconsistency that's been with us since 2ade9340262 (Add "check_sha1_signature()" helper function, 2005-04-08), and makes a 3: 636a647ac51 ! 6: bdddaa3648f object API: make check_object_signature() oideq()-like, move docs @@ Metadata Author: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## Commit message ## - object API: make check_object_signature() oideq()-like, move docs + object API docs: move check_object_signature() docs to cache.h - Make the return value of check_object_signature() behave like oideq() - and memcmp() instead of returning negative values on failure. - - This reduces the boilerplate required when calling the function, and - makes the calling code behave the same is if though we'd called - oideq(), which is basically what we're doing here. We already had some - callers using "f() < 0", with others using "!f()". Instead of - declaring the latter a bug let's convert all callers to it. - - It is unfortunate that there's also cases where we "return -1" on - various errors, and we can't tell those apart from the expected OID - being less than the real OID, but this was the case already. - - This change is rather dangerous stand-alone as we're changing the - return semantics, but not changing the prototype. Therefore any - out-of-tree code rebased on this change would start doing the opposite - of what it was meant to do. In a subsequent commit we'll make that a - non-issue by changing the signature of the function, ensuring that the - compiler will catch any such misuse. - - While we're at it let's re-flow some of the callers to wrap at 79 - characters, and move the API documentation to cache.h, where the - prototype of this function lives. + Move the API documentation for check_object_signature() to cache.h, + where its prototype is declared. This is in preparation for adding a + companion function. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> - ## builtin/fast-export.c ## -@@ builtin/fast-export.c: static void export_blob(const struct object_id *oid) - buf = read_object_file(oid, &type, &size); - if (!buf) - die("could not read blob %s", oid_to_hex(oid)); -- if (check_object_signature(the_repository, oid, buf, size, -- type_name(type), NULL) < 0) -+ if (!check_object_signature(the_repository, oid, buf, size, -+ type_name(type), NULL)) - die("oid mismatch in blob %s", oid_to_hex(oid)); - object = parse_object_buffer(the_repository, oid, type, - size, buf, &eaten); - - ## builtin/index-pack.c ## -@@ builtin/index-pack.c: static void fix_unresolved_deltas(struct hashfile *f) - if (!data) - continue; - -- if (check_object_signature(the_repository, &d->oid, -- data, size, -- type_name(type), NULL)) -+ if (!check_object_signature(the_repository, &d->oid, data, -+ size, type_name(type), NULL)) - die(_("local object %s is corrupt"), oid_to_hex(&d->oid)); - - /* - - ## builtin/mktag.c ## -@@ builtin/mktag.c: static int verify_object_in_tag(struct object_id *tagged_oid, int *tagged_type) - type_name(*tagged_type), type_name(type)); - - repl = lookup_replace_object(the_repository, tagged_oid); -- ret = check_object_signature(the_repository, repl, -- buffer, size, type_name(*tagged_type), -- NULL); -+ ret = !check_object_signature(the_repository, repl, buffer, size, -+ type_name(*tagged_type), NULL); - free(buffer); - - return ret; - ## cache.h ## @@ cache.h: enum unpack_loose_header_result unpack_loose_header(git_zstream *stream, struct object_info; @@ cache.h: enum unpack_loose_header_result unpack_loose_header(git_zstream *stream + * object name actually matches "oid" to detect object corruption. + * With "buf" == NULL, try reading the object named with "oid" using + * the streaming interface and rehash it to do the same. -+ * -+ * Treat the return value like oideq() (which is like memcmp()), -+ * except that negative values might also indicate a generic error. + */ int check_object_signature(struct repository *r, const struct object_id *oid, void *buf, unsigned long size, const char *type, @@ object-file.c: int format_object_header(char *str, size_t size, enum object_type int check_object_signature(struct repository *r, const struct object_id *oid, void *buf, unsigned long size, const char *type, struct object_id *real_oidp) -@@ object-file.c: int check_object_signature(struct repository *r, const struct object_id *oid, - - if (buf) { - hash_object_file(r->hash_algo, buf, size, type, real_oid); -- return !oideq(oid, real_oid) ? -1 : 0; -+ return oideq(oid, real_oid); - } - - st = open_istream(r, oid, &obj_type, &size, NULL); -@@ object-file.c: int check_object_signature(struct repository *r, const struct object_id *oid, - } - r->hash_algo->final_oid_fn(real_oid, &c); - close_istream(st); -- return !oideq(oid, real_oid) ? -1 : 0; -+ return oideq(oid, real_oid); - } - - int git_open_cloexec(const char *name, int flags) -@@ object-file.c: int read_loose_object(const char *path, - git_inflate_end(&stream); - goto out; - } -- if (check_object_signature(the_repository, expected_oid, -- *contents, *size, -- oi->type_name->buf, real_oid)) -+ if (!check_object_signature(the_repository, expected_oid, -+ *contents, *size, -+ oi->type_name->buf, real_oid)) - goto out; - } - - - ## object.c ## -@@ object.c: struct object *parse_object(struct repository *r, const struct object_id *oid) - if ((obj && obj->type == OBJ_BLOB && repo_has_object_file(r, oid)) || - (!obj && repo_has_object_file(r, oid) && - oid_object_info(r, oid, NULL) == OBJ_BLOB)) { -- if (check_object_signature(r, repl, NULL, 0, NULL, NULL) < 0) { -+ if (!check_object_signature(r, repl, NULL, 0, NULL, NULL)) { - error(_("hash mismatch %s"), oid_to_hex(oid)); - return NULL; - } -@@ object.c: struct object *parse_object(struct repository *r, const struct object_id *oid) - - buffer = repo_read_object_file(r, oid, &type, &size); - if (buffer) { -- if (check_object_signature(r, repl, buffer, size, -- type_name(type), NULL) < 0) { -+ if (!check_object_signature(r, repl, buffer, size, -+ type_name(type), NULL)) { - free(buffer); - error(_("hash mismatch %s"), oid_to_hex(repl)); - return NULL; - - ## pack-check.c ## -@@ pack-check.c: static int verify_packfile(struct repository *r, - err = error("cannot unpack %s from %s at offset %"PRIuMAX"", - oid_to_hex(&oid), p->pack_name, - (uintmax_t)entries[i].offset); -- else if (check_object_signature(r, &oid, data, size, -- type_name(type), NULL)) -+ else if (!check_object_signature(r, &oid, data, size, -+ type_name(type), NULL)) - err = error("packed %s from %s is corrupt", - oid_to_hex(&oid), p->pack_name); - else if (fn) { -: ----------- > 7: 75abf75a437 object API users + docs: check <0, not !0 with check_object_signature() 4: c38af53e889 ! 8: 478d2915731 object-file API: split up and simplify check_object_signature() @@ builtin/fast-export.c @@ builtin/fast-export.c: static void export_blob(const struct object_id *oid) if (!buf) die("could not read blob %s", oid_to_hex(oid)); - if (!check_object_signature(the_repository, oid, buf, size, -- type_name(type), NULL)) -+ type_name(type))) + if (check_object_signature(the_repository, oid, buf, size, +- type_name(type), NULL) < 0) ++ type_name(type)) < 0) die("oid mismatch in blob %s", oid_to_hex(oid)); object = parse_object_buffer(the_repository, oid, type, size, buf, &eaten); @@ builtin/index-pack.c @@ builtin/index-pack.c: static void fix_unresolved_deltas(struct hashfile *f) continue; - if (!check_object_signature(the_repository, &d->oid, data, -- size, type_name(type), NULL)) -+ size, type_name(type))) + if (check_object_signature(the_repository, &d->oid, data, size, +- type_name(type), NULL) < 0) ++ type_name(type)) < 0) die(_("local object %s is corrupt"), oid_to_hex(&d->oid)); /* ## builtin/mktag.c ## @@ builtin/mktag.c: static int verify_object_in_tag(struct object_id *tagged_oid, int *tagged_type) + type_name(*tagged_type), type_name(type)); repl = lookup_replace_object(the_repository, tagged_oid); - ret = !check_object_signature(the_repository, repl, buffer, size, -- type_name(*tagged_type), NULL); -+ type_name(*tagged_type)); +- ret = check_object_signature(the_repository, repl, +- buffer, size, type_name(*tagged_type), +- NULL); ++ ret = check_object_signature(the_repository, repl, buffer, size, ++ type_name(*tagged_type)); free(buffer); return ret; @@ cache.h: int parse_loose_header(const char *hdr, struct object_info *oi); - * With "buf" == NULL, try reading the object named with "oid" using - * the streaming interface and rehash it to do the same. * - * Treat the return value like oideq() (which is like memcmp()), - * except that negative values might also indicate a generic error. + * A negative value indicates an error, usually that the OID is not + * what we expected, but it might also indicate another error. */ int check_object_signature(struct repository *r, const struct object_id *oid, - void *buf, unsigned long size, const char *type, @@ object-file.c: int format_object_header(char *str, size_t size, enum object_type + + hash_object_file(r->hash_algo, buf, size, type, &real_oid); + -+ return oideq(oid, &real_oid); ++ return !oideq(oid, &real_oid) ? -1 : 0; +} + +int stream_object_signature(struct repository *r, const struct object_id *oid) @@ object-file.c: int format_object_header(char *str, size_t size, enum object_type - if (buf) { - hash_object_file(r->hash_algo, buf, size, type, real_oid); -- return oideq(oid, real_oid); +- return !oideq(oid, real_oid) ? -1 : 0; - } - st = open_istream(r, oid, &obj_type, &size, NULL); @@ object-file.c: int check_object_signature(struct repository *r, const struct obj - 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); -+ return oideq(oid, &real_oid); +- return !oideq(oid, real_oid) ? -1 : 0; ++ return !oideq(oid, &real_oid) ? -1 : 0; } int git_open_cloexec(const char *name, int flags) @@ object-file.c: int read_loose_object(const char *path, git_inflate_end(&stream); goto out; } -- if (!check_object_signature(the_repository, expected_oid, -- *contents, *size, -- oi->type_name->buf, real_oid)) -+ +- if (check_object_signature(the_repository, expected_oid, +- *contents, *size, +- oi->type_name->buf, real_oid) < 0) + hash_object_file(the_repository->hash_algo, + *contents, *size, oi->type_name->buf, + real_oid); @@ object.c: struct object *parse_object(struct repository *r, const struct object_ if ((obj && obj->type == OBJ_BLOB && repo_has_object_file(r, oid)) || (!obj && repo_has_object_file(r, oid) && oid_object_info(r, oid, NULL) == OBJ_BLOB)) { -- if (!check_object_signature(r, repl, NULL, 0, NULL, NULL)) { -+ if (!stream_object_signature(r, repl)) { +- if (check_object_signature(r, repl, NULL, 0, NULL, NULL) < 0) { ++ if (stream_object_signature(r, repl) < 0) { error(_("hash mismatch %s"), oid_to_hex(oid)); return NULL; } @@ object.c: struct object *parse_object(struct repository *r, const struct object_id *oid) buffer = repo_read_object_file(r, oid, &type, &size); if (buffer) { - if (!check_object_signature(r, repl, buffer, size, -- type_name(type), NULL)) { -+ type_name(type))) { + if (check_object_signature(r, repl, buffer, size, +- type_name(type), NULL) < 0) { ++ type_name(type)) < 0) { free(buffer); error(_("hash mismatch %s"), oid_to_hex(repl)); return NULL; @@ pack-check.c: static int verify_packfile(struct repository *r, err = error("cannot unpack %s from %s at offset %"PRIuMAX"", oid_to_hex(&oid), p->pack_name, (uintmax_t)entries[i].offset); -- else if (!check_object_signature(r, &oid, data, size, -- type_name(type), NULL)) -+ else if (data && !check_object_signature(r, &oid, data, size, -+ type_name(type))) +- else if (check_object_signature(r, &oid, data, size, +- type_name(type), NULL) < 0) ++ else if (data && check_object_signature(r, &oid, data, size, ++ type_name(type)) < 0) + err = error("packed %s from %s is corrupt", + oid_to_hex(&oid), p->pack_name); -+ else if (!data && !stream_object_signature(r, &oid)) ++ else if (!data && stream_object_signature(r, &oid) < 0) err = error("packed %s from %s is corrupt", oid_to_hex(&oid), p->pack_name); else if (fn) { 5: a5ebd04d462 = 9: 1276c2d1ed6 object API: rename hash_object_file_literally() to write_*() 6: 40647be525b ! 10: 2c936c674d4 object-file API: have hash_object_file() take "enum object_type" @@ builtin/fast-export.c @@ builtin/fast-export.c: static void export_blob(const struct object_id *oid) if (!buf) die("could not read blob %s", oid_to_hex(oid)); - if (!check_object_signature(the_repository, oid, buf, size, -- type_name(type))) -+ type)) + if (check_object_signature(the_repository, oid, buf, size, +- type_name(type)) < 0) ++ type) < 0) die("oid mismatch in blob %s", oid_to_hex(oid)); object = parse_object_buffer(the_repository, oid, type, size, buf, &eaten); @@ builtin/index-pack.c: static struct base_data *resolve_delta(struct object_entry @@ builtin/index-pack.c: static void fix_unresolved_deltas(struct hashfile *f) continue; - if (!check_object_signature(the_repository, &d->oid, data, -- size, type_name(type))) -+ size, type)) + if (check_object_signature(the_repository, &d->oid, data, size, +- type_name(type)) < 0) ++ type) < 0) die(_("local object %s is corrupt"), oid_to_hex(&d->oid)); /* @@ builtin/mktag.c @@ builtin/mktag.c: static int verify_object_in_tag(struct object_id *tagged_oid, int *tagged_type) repl = lookup_replace_object(the_repository, tagged_oid); - ret = !check_object_signature(the_repository, repl, buffer, size, -- type_name(*tagged_type)); + ret = check_object_signature(the_repository, repl, buffer, size, +- type_name(*tagged_type)); + *tagged_type); free(buffer); @@ cache-tree.c: static int verify_one(struct repository *r, ## cache.h ## @@ cache.h: int parse_loose_header(const char *hdr, struct object_info *oi); - * except that negative values might also indicate a generic error. + * what we expected, but it might also indicate another error. */ int check_object_signature(struct repository *r, const struct object_id *oid, - void *buf, unsigned long size, const char *type); @@ object-file.c: int index_path(struct index_state *istate, struct object_id *oid, rc = error(_("%s: failed to insert into database"), path); strbuf_release(&sb); @@ object-file.c: int read_loose_object(const char *path, + git_inflate_end(&stream); goto out; } - - hash_object_file(the_repository->hash_algo, - *contents, *size, oi->type_name->buf, - real_oid); @@ object.c: struct object *parse_object(struct repository *r, const struct object_ buffer = repo_read_object_file(r, oid, &type, &size); if (buffer) { -- if (!check_object_signature(r, repl, buffer, size, -- type_name(type))) { -+ if (!check_object_signature(r, repl, buffer, size, type)) { +- if (check_object_signature(r, repl, buffer, size, +- type_name(type)) < 0) { ++ if (check_object_signature(r, repl, buffer, size, type) < 0) { free(buffer); error(_("hash mismatch %s"), oid_to_hex(repl)); return NULL; @@ pack-check.c @@ pack-check.c: static int verify_packfile(struct repository *r, oid_to_hex(&oid), p->pack_name, (uintmax_t)entries[i].offset); - else if (data && !check_object_signature(r, &oid, data, size, -- type_name(type))) -+ type)) + else if (data && check_object_signature(r, &oid, data, size, +- type_name(type)) < 0) ++ type) < 0) err = error("packed %s from %s is corrupt", oid_to_hex(&oid), p->pack_name); - else if (!data && !stream_object_signature(r, &oid)) + else if (!data && stream_object_signature(r, &oid) < 0) 7: e39edfbce05 = 11: ad1a777f454 object-file.c: add a literal version of write_object_file_prepare() 8: f2b1cb861a0 = 12: d2d355e6bb8 object-file API: pass an enum to read_object_with_reference() -- 2.35.1.940.ge7a5b4b05f2
