diff mbox series

[v2,2/8] object.c: add a utility function for "expected type X, got Y"

Message ID patch-2.8-1b472fcd85-20210420T133218Z-avarab@gmail.com (mailing list archive)
State New, archived
Headers show
Series object.c: add and use "is expected" utility function + object_as_type() use | expand

Commit Message

Ævar Arnfjörð Bjarmason April 20, 2021, 1:36 p.m. UTC 
Refactor various "Object X is not Y" error messages so that they use
the same message as the long-standing object_as_type() error
message. Now we'll consistently report e.g. that we got a commit when
we expected a tag, not just that the object is not a tag.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/index-pack.c |  9 +++------
 combine-diff.c       |  3 +--
 commit.c             | 10 ++++------
 merge-recursive.c    |  1 +
 object.c             | 25 ++++++++++++++++++++++++-
 object.h             |  5 +++++
 tree.c               |  7 ++++---
 7 files changed, 42 insertions(+), 18 deletions(-)

Comments

Jonathan Tan April 21, 2021, 10:02 p.m. UTC | #1 
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 7618303f7b..b952106203 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -2999,6 +2999,7 @@ static int read_oid_strbuf(struct merge_options *opt,
>  	if (!buf)
>  		return err(opt, _("cannot read object %s"), oid_to_hex(oid));
>  	if (type != OBJ_BLOB) {
> +		const char* msg = oid_is_type_or_die_msg(oid, OBJ_BLOB, &type);
>  		free(buf);
>  		return err(opt, _("object %s is not a blob"), oid_to_hex(oid));
>  	}

Stray extra line.

> +void oid_is_type_or_die(const struct object_id *oid,
> +			enum object_type want,
> +			enum object_type *type)
> +{

Thanks - this looks like a good simplification.

Why is type a pointer? Maybe it's to better distinguish the values at
the call site (one pointer, one not), but this solution is confusing
too.

> +int oid_is_type_or_error(const struct object_id *oid,
> +			 enum object_type want,
> +			 enum object_type *type)
> +{

Same comment.
Ævar Arnfjörð Bjarmason April 22, 2021, 6:10 a.m. UTC | #2 
On Thu, Apr 22 2021, Jonathan Tan wrote:

>> diff --git a/merge-recursive.c b/merge-recursive.c
>> index 7618303f7b..b952106203 100644
>> --- a/merge-recursive.c
>> +++ b/merge-recursive.c
>> @@ -2999,6 +2999,7 @@ static int read_oid_strbuf(struct merge_options *opt,
>>  	if (!buf)
>>  		return err(opt, _("cannot read object %s"), oid_to_hex(oid));
>>  	if (type != OBJ_BLOB) {
>> +		const char* msg = oid_is_type_or_die_msg(oid, OBJ_BLOB, &type);
>>  		free(buf);
>>  		return err(opt, _("object %s is not a blob"), oid_to_hex(oid));
>>  	}
>
> Stray extra line.
>
>> +void oid_is_type_or_die(const struct object_id *oid,
>> +			enum object_type want,
>> +			enum object_type *type)
>> +{
>
> Thanks - this looks like a good simplification.
>
> Why is type a pointer? Maybe it's to better distinguish the values at
> the call site (one pointer, one not), but this solution is confusing
> too.

Yeah I came up with it because of that, so you wouldn't confuse the
OBJ_COMMIT with (presumably) a variable with the same.

But in some other cases I end up having to do:

    enum object_type type = OBJ_COMMIT;

And then pass that &type in, do you think it's worth it? Maybe I should
just change it...

>> +int oid_is_type_or_error(const struct object_id *oid,
>> +			 enum object_type want,
>> +			 enum object_type *type)
>> +{
>
> Same comment.
diff mbox series

Patch

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index c0e3768c32..eabd9d4677 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -218,8 +218,8 @@  static int mark_link(struct object *obj, enum object_type type,
 	if (!obj)
 		return -1;
 
-	if (type != OBJ_ANY && obj->type != type)
-		die(_("object type mismatch at %s"), oid_to_hex(&obj->oid));
+	if (type != OBJ_ANY)
+		oid_is_type_or_die(&obj->oid, obj->type, &type);
 
 	obj->flags |= FLAG_LINK;
 	return 0;
@@ -241,10 +241,7 @@  static unsigned check_object(struct object *obj)
 		if (type <= 0)
 			die(_("did not receive expected object %s"),
 			      oid_to_hex(&obj->oid));
-		if (type != obj->type)
-			die(_("object %s: expected type %s, found %s"),
-			    oid_to_hex(&obj->oid),
-			    type_name(obj->type), type_name(type));
+		oid_is_type_or_die(&obj->oid, obj->type, &type);
 		obj->flags |= FLAG_CHECKED;
 		return 1;
 	}
diff --git a/combine-diff.c b/combine-diff.c
index 06635f91bc..aa767dbb8e 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -333,8 +333,7 @@  static char *grab_blob(struct repository *r,
 		free_filespec(df);
 	} else {
 		blob = read_object_file(oid, &type, size);
-		if (type != OBJ_BLOB)
-			die("object '%s' is not a blob!", oid_to_hex(oid));
+		oid_is_type_or_die(oid, OBJ_BLOB, &type);
 	}
 	return blob;
 }
diff --git a/commit.c b/commit.c
index 3580c62b92..3d7f1fba0c 100644
--- a/commit.c
+++ b/commit.c
@@ -304,9 +304,7 @@  const void *repo_get_commit_buffer(struct repository *r,
 		if (!ret)
 			die("cannot read commit object %s",
 			    oid_to_hex(&commit->object.oid));
-		if (type != OBJ_COMMIT)
-			die("expected commit for %s, got %s",
-			    oid_to_hex(&commit->object.oid), type_name(type));
+		oid_is_type_or_die(&commit->object.oid, OBJ_COMMIT, &type);
 		if (sizep)
 			*sizep = size;
 	}
@@ -494,10 +492,10 @@  int repo_parse_commit_internal(struct repository *r,
 		return quiet_on_missing ? -1 :
 			error("Could not read %s",
 			     oid_to_hex(&item->object.oid));
-	if (type != OBJ_COMMIT) {
+	ret = oid_is_type_or_error(&item->object.oid, OBJ_COMMIT, &type);
+	if (ret) {
 		free(buffer);
-		return error("Object %s not a commit",
-			     oid_to_hex(&item->object.oid));
+		return ret;
 	}
 
 	ret = parse_commit_buffer(r, item, buffer, size, 0);
diff --git a/merge-recursive.c b/merge-recursive.c
index 7618303f7b..b952106203 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2999,6 +2999,7 @@  static int read_oid_strbuf(struct merge_options *opt,
 	if (!buf)
 		return err(opt, _("cannot read object %s"), oid_to_hex(oid));
 	if (type != OBJ_BLOB) {
+		const char* msg = oid_is_type_or_die_msg(oid, OBJ_BLOB, &type);
 		free(buf);
 		return err(opt, _("object %s is not a blob"), oid_to_hex(oid));
 	}
diff --git a/object.c b/object.c
index 3c962da6c9..9e06c0ee92 100644
--- a/object.c
+++ b/object.c
@@ -153,6 +153,29 @@  void *create_object(struct repository *r, const struct object_id *oid, void *o)
 	return obj;
 }
 
+static const char *object_type_mismatch_msg = N_("object %s is a %s, not a %s");
+
+void oid_is_type_or_die(const struct object_id *oid,
+			enum object_type want,
+			enum object_type *type)
+{
+	if (want == *type)
+		return;
+	die(_(object_type_mismatch_msg), oid_to_hex(oid),
+	    type_name(*type), type_name(want));
+}
+
+int oid_is_type_or_error(const struct object_id *oid,
+			 enum object_type want,
+			 enum object_type *type)
+{
+	if (want == *type)
+		return 0;
+	return error(_(object_type_mismatch_msg),
+		     oid_to_hex(oid), type_name(*type),
+		     type_name(want));
+}
+
 void *object_as_type(struct object *obj, enum object_type type, int quiet)
 {
 	if (obj->type == type)
@@ -166,7 +189,7 @@  void *object_as_type(struct object *obj, enum object_type type, int quiet)
 	}
 	else {
 		if (!quiet)
-			error(_("object %s is a %s, not a %s"),
+			error(_(object_type_mismatch_msg),
 			      oid_to_hex(&obj->oid),
 			      type_name(obj->type), type_name(type));
 		return NULL;
diff --git a/object.h b/object.h
index 85e7491815..f8609a8518 100644
--- a/object.h
+++ b/object.h
@@ -123,6 +123,11 @@  void *create_object(struct repository *r, const struct object_id *oid, void *obj
 
 void *object_as_type(struct object *obj, enum object_type type, int quiet);
 
+void oid_is_type_or_die(const struct object_id *oid, enum object_type want,
+			enum object_type *type);
+int oid_is_type_or_error(const struct object_id *oid, enum object_type want,
+			 enum object_type *type);
+
 /*
  * Returns the object, having parsed it to find out what it is.
  *
diff --git a/tree.c b/tree.c
index e9d2bd7ffd..f1c6e8f647 100644
--- a/tree.c
+++ b/tree.c
@@ -131,6 +131,7 @@  int parse_tree_gently(struct tree *item, int quiet_on_missing)
 	enum object_type type;
 	void *buffer;
 	unsigned long size;
+	int ret;
 
 	if (item->object.parsed)
 		return 0;
@@ -139,10 +140,10 @@  int parse_tree_gently(struct tree *item, int quiet_on_missing)
 		return quiet_on_missing ? -1 :
 			error("Could not read %s",
 			     oid_to_hex(&item->object.oid));
-	if (type != OBJ_TREE) {
+	ret = oid_is_type_or_error(&item->object.oid, OBJ_TREE, &type);
+	if (ret) {
 		free(buffer);
-		return error("Object %s not a tree",
-			     oid_to_hex(&item->object.oid));
+		return ret;
 	}
 	return parse_tree_buffer(item, buffer, size);
 }