[PATCH v2 06/11] object API: make check_object_signature() oideq()-like, move docs

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 4, 2022, 13:51 UTC
Message-ID: <patch-v2-06.11-636a647ac51-20220204T135005Z-avarab@gmail.com>
In-Reply-To: <cover-v2-00.11-00000000000-20220204T135005Z-avarab@gmail.com>

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/fast-export.c |  4 ++--
 builtin/index-pack.c  |  5 ++---
 builtin/mktag.c       |  5 ++---
 cache.h               |  9 +++++++++
 object-file.c         | 16 +++++-----------
 object.c              |  6 +++---
 pack-check.c          |  4 ++--
 7 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 9f1c730e587..7a79cb186b1 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -299,8 +299,8 @@ 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);
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 01574378ce2..6db3e728ff4 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1412,9 +1412,8 @@ 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));
 
 		/*
diff --git a/builtin/mktag.c b/builtin/mktag.c
index 96a3686af53..a715bf53cf0 100644
--- a/builtin/mktag.c
+++ b/builtin/mktag.c
@@ -61,9 +61,8 @@ 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;
diff --git a/cache.h b/cache.h
index 281f00ab1b1..3a156dcb37b 100644
--- a/cache.h
+++ b/cache.h
@@ -1319,6 +1319,15 @@ enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,
 struct object_info;
 int parse_loose_header(const char *hdr, struct object_info *oi);
 
+/**
+ * With in-core object data in "buf", rehash it to make sure the
+ * 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,
 			   struct object_id *real_oidp);
diff --git a/object-file.c b/object-file.c
index 271acf4dd15..4c38ddad5dc 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1066,12 +1066,6 @@ int format_object_header(char *str, size_t size, enum object_type type,
 	return format_object_header_literally(str, size, name, objsize);
 }
 
-/*
- * With in-core object data in "buf", rehash it to make sure the
- * 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.
- */
 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)
@@ -1086,7 +1080,7 @@ 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);
@@ -1113,7 +1107,7 @@ 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)
@@ -2617,9 +2611,9 @@ 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;
 	}
 
diff --git a/object.c b/object.c
index c37501fc120..b778b32407d 100644
--- a/object.c
+++ b/object.c
@@ -279,7 +279,7 @@ 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;
 		}
@@ -289,8 +289,8 @@ 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;
diff --git a/pack-check.c b/pack-check.c
index 3f418e3a6af..35cca10057c 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -142,8 +142,8 @@ 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) {

-- 
2.35.1.940.ge7a5b4b05f2