diff mbox series

[v2,6/8] commit.c: don't use deref_tag() -> object_as_type()

Message ID patch-6.8-f337a5442d-20210420T133218Z-avarab@gmail.com (mailing list archive)
State New
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
Change a use of the object_as_type() function introduced in
8ff226a9d5e (add object_as_type helper for casting objects,
2014-07-13) to instead assume that we're not dealing with OBJ_NONE (or
OBJ_BAD) from deref_tag().

This makes this code easier to read, as the reader isn't wondering why
the function would need to deal with that. We're simply doing a check
of OBJ_{COMMIT,TREE,BLOB,TAG} here, not the bare-bones initialization
object_as_type() might be called on to do.

Even though we can read deref_tag() and see that it won't return
OBJ_NONE and friends, let's add a BUG() assertion here to help future
maintenance.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 commit.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

Comments

Jonathan Tan April 21, 2021, 10:26 p.m. UTC | #1
> Change a use of the object_as_type() function introduced in
> 8ff226a9d5e (add object_as_type helper for casting objects,
> 2014-07-13) to instead assume that we're not dealing with OBJ_NONE (or
> OBJ_BAD) from deref_tag().
> 
> This makes this code easier to read, as the reader isn't wondering why
> the function would need to deal with that. We're simply doing a check
> of OBJ_{COMMIT,TREE,BLOB,TAG} here, not the bare-bones initialization
> object_as_type() might be called on to do.

I think the benefit of using object_as_type() here (functionality that
checks the object type, with optional "quiet" behavior) outweighs the
drawback (additional functionality that we don't need). If we're worried
that the reader would wonder about the OBJ_NONE case, we can include the
BUG check as you did.

> Even though we can read deref_tag() and see that it won't return
> OBJ_NONE and friends, let's add a BUG() assertion here to help future
> maintenance.

This is reasonable.

> diff --git a/commit.c b/commit.c
> index 3d7f1fba0c..c3bc6cbec4 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -31,13 +31,22 @@ const char *commit_type = "commit";
>  struct commit *lookup_commit_reference_gently(struct repository *r,
>  		const struct object_id *oid, int quiet)
>  {
> -	struct object *obj = deref_tag(r,
> -				       parse_object(r, oid),
> -				       NULL, 0);
> +	struct object *tmp = parse_object(r, oid);
> +	struct object *obj = deref_tag(r, tmp, NULL, 0);

This change isn't unnecessary, I think. "tmp" isn't used anywhere else.
diff mbox series

Patch

diff --git a/commit.c b/commit.c
index 3d7f1fba0c..c3bc6cbec4 100644
--- a/commit.c
+++ b/commit.c
@@ -31,13 +31,22 @@  const char *commit_type = "commit";
 struct commit *lookup_commit_reference_gently(struct repository *r,
 		const struct object_id *oid, int quiet)
 {
-	struct object *obj = deref_tag(r,
-				       parse_object(r, oid),
-				       NULL, 0);
+	struct object *tmp = parse_object(r, oid);
+	struct object *obj = deref_tag(r, tmp, NULL, 0);
 
 	if (!obj)
 		return NULL;
-	return object_as_type(obj, OBJ_COMMIT, quiet);
+
+	if (obj->type <= 0)
+		BUG("should have initialized obj->type = OBJ_{COMMIT,TREE,BLOB,TAG} from deref_tag()");
+	if (obj->type != OBJ_COMMIT) {
+		if (!quiet) {
+			enum object_type have = obj->type;
+			oid_is_type_or_error(oid, OBJ_COMMIT, &have);
+		}
+		return NULL;
+	}
+	return (struct commit *)obj;
 }
 
 struct commit *lookup_commit_reference(struct repository *r, const struct object_id *oid)