diff mbox series

[v2,04/10] object-file.c: take type id, not string, in read_object_with_reference()

Message ID patch-04.10-48aca62864-20210420T124428Z-avarab@gmail.com (mailing list archive)
State New
Headers show
Series object.c et al: tests, small bug fixes etc. | expand

Commit Message

Ævar Arnfjörð Bjarmason April 20, 2021, 12:50 p.m. UTC
Make the read_object_with_reference() function take "enum object_type"
instead of a "const char *" with a type name that it converted via
type_from_string().

Out of the nine callers of this function, only one wanted to pass a
"const char *". The others were simply passing along the
{commit,tree}_type string constants.

That one caller in builtin/cat-file.c did not expect to pass a "raw"
type (i.e. in invalid "--literally" type, but one gotten from
type_from_string(). Furthermore the read_object_with_reference()
function itself was calling type_from_string(), so this whole thing
amounted to unnecessarily going back and forth.

This API design dates back to f4913f91a96 ([PATCH] Accept commit in
some places when tree is needed., 2005-04-20). At that time there
wasn't an API like type_from_string(). That only arrived in
df8436622fb (formalize typename(), and add its reverse
type_from_string(), 2007-02-26).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/cat-file.c     | 7 ++++---
 builtin/fast-import.c  | 6 +++---
 builtin/grep.c         | 4 ++--
 builtin/pack-objects.c | 2 +-
 cache.h                | 2 +-
 object-file.c          | 7 +++----
 tree-walk.c            | 6 +++---
 7 files changed, 17 insertions(+), 17 deletions(-)

Comments

Junio C Hamano April 29, 2021, 4:37 a.m. UTC | #1
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Make the read_object_with_reference() function take "enum object_type"
> instead of a "const char *" with a type name that it converted via
> type_from_string().
>
> Out of the nine callers of this function, only one wanted to pass a
> "const char *". The others were simply passing along the
> {commit,tree}_type string constants.

s/wanted to pass a/wanted to pass an arbitrary/, I would think.
Other than that, nicely analyzed.

> That one caller in builtin/cat-file.c did not expect to pass a "raw"
> type (i.e. in invalid "--literally" type, but one gotten from
> type_from_string().

It is unclear what you are trying to say here.  Missing closing ')'
that should close '(i.e.' does not help, either, because it muddles
what you wanted to imply by bringing up the "--literally" option.

    The one caller in builtin/cat-file.c passes the typename the
    end-user typed on the command line i.e. "git cat-file <TYPE>
    <NAME>", but read_object_with_reference() called in the codepath
    is *NOT* expected to deal with objects with unknown/bogus type
    crafted with "hash-object --literally" (note: "git cat-file"
    does have the "--allow-unknown-type" option, but it can only be
    used with the "-s" and "-t" options).  It won't result in loss
    of functionality if we restricted the required_type parameter
    given to read_object_with_reference() to the four known types by
    changing the function signature to take the enum instead of
    string.

is probably what you meant to say, but I am only guessing.

> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 5ebf13359e..46fc7a32ba 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -66,7 +66,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
>  			int unknown_type)
>  {
>  	struct object_id oid;
> -	enum object_type type;
> +	enum object_type type, exp_type_id;
>  	char *buf;
>  	unsigned long size;
>  	struct object_context obj_context;
> @@ -154,7 +154,8 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
>  		break;
>  
>  	case 0:
> -		if (type_from_string(exp_type) == OBJ_BLOB) {
> +		exp_type_id = type_from_string(exp_type);
> +		if (exp_type_id == OBJ_BLOB) {
>  			struct object_id blob_oid;
>  			if (oid_object_info(the_repository, &oid, NULL) == OBJ_TAG) {
>  				char *buffer = read_object_file(&oid, &type,
> @@ -177,7 +178,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
>  			 */
>  		}
>  		buf = read_object_with_reference(the_repository,
> -						 &oid, exp_type, &size, NULL);
> +						 &oid, exp_type_id, &size, NULL);
>  		break;
>  
>  	default:

And this is the caller we just discussed.

> diff --git a/object-file.c b/object-file.c
> index 624af408cd..d2f223dcef 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1669,25 +1669,24 @@ void *read_object_file_extended(struct repository *r,
>  
>  void *read_object_with_reference(struct repository *r,
>  				 const struct object_id *oid,
> -				 const char *required_type_name,
> +				 enum object_type object_type,
>  				 unsigned long *size,
>  				 struct object_id *actual_oid_return)
>  {
> -	enum object_type type, required_type;
>  	void *buffer;
>  	unsigned long isize;
>  	struct object_id actual_oid;
>  
> -	required_type = type_from_string(required_type_name);
>  	oidcpy(&actual_oid, oid);
>  	while (1) {
>  		int ref_length = -1;
>  		const char *ref_type = NULL;
> +		enum object_type type;
>  
>  		buffer = repo_read_object_file(r, &actual_oid, &type, &isize);
>  		if (!buffer)
>  			return NULL;
> -		if (type == required_type) {
> +		if (type == object_type) {
>  			*size = isize;
>  			if (actual_oid_return)
>  				oidcpy(actual_oid_return, &actual_oid);

I do not think it is a good change to effectively rename
required_type to object_type.  Swapping the required_type_name
parameter with required_type parameter of type "enum object_type"
and dropping the now unneeded type_from_string() call would have
been much easier to follow.
diff mbox series

Patch

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 5ebf13359e..46fc7a32ba 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -66,7 +66,7 @@  static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 			int unknown_type)
 {
 	struct object_id oid;
-	enum object_type type;
+	enum object_type type, exp_type_id;
 	char *buf;
 	unsigned long size;
 	struct object_context obj_context;
@@ -154,7 +154,8 @@  static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 		break;
 
 	case 0:
-		if (type_from_string(exp_type) == OBJ_BLOB) {
+		exp_type_id = type_from_string(exp_type);
+		if (exp_type_id == OBJ_BLOB) {
 			struct object_id blob_oid;
 			if (oid_object_info(the_repository, &oid, NULL) == OBJ_TAG) {
 				char *buffer = read_object_file(&oid, &type,
@@ -177,7 +178,7 @@  static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 			 */
 		}
 		buf = read_object_with_reference(the_repository,
-						 &oid, exp_type, &size, NULL);
+						 &oid, exp_type_id, &size, NULL);
 		break;
 
 	default:
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 3afa81cf9a..ee52be02f8 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -2481,7 +2481,7 @@  static void note_change_n(const char *p, struct branch *b, unsigned char *old_fa
 		unsigned long size;
 		char *buf = read_object_with_reference(the_repository,
 						       &commit_oid,
-						       commit_type, &size,
+						       OBJ_COMMIT, &size,
 						       &commit_oid);
 		if (!buf || size < the_hash_algo->hexsz + 6)
 			die("Not a valid commit: %s", p);
@@ -2553,7 +2553,7 @@  static void parse_from_existing(struct branch *b)
 		char *buf;
 
 		buf = read_object_with_reference(the_repository,
-						 &b->oid, commit_type, &size,
+						 &b->oid, OBJ_COMMIT, &size,
 						 &b->oid);
 		parse_from_commit(b, buf, size);
 		free(buf);
@@ -2649,7 +2649,7 @@  static struct hash_list *parse_merge(unsigned int *count)
 			unsigned long size;
 			char *buf = read_object_with_reference(the_repository,
 							       &n->oid,
-							       commit_type,
+							       OBJ_COMMIT,
 							       &size, &n->oid);
 			if (!buf || size < the_hash_algo->hexsz + 6)
 				die("Not a valid commit: %s", from);
diff --git a/builtin/grep.c b/builtin/grep.c
index 5de725f904..f2a73c92fa 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -467,7 +467,7 @@  static int grep_submodule(struct grep_opt *opt,
 		object = parse_object_or_die(oid, NULL);
 		obj_read_unlock();
 		data = read_object_with_reference(&subrepo,
-						  &object->oid, tree_type,
+						  &object->oid, OBJ_TREE,
 						  &size, NULL);
 		if (!data)
 			die(_("unable to read tree (%s)"), oid_to_hex(&object->oid));
@@ -635,7 +635,7 @@  static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
 		int hit, len;
 
 		data = read_object_with_reference(opt->repo,
-						  &obj->oid, tree_type,
+						  &obj->oid, OBJ_TREE,
 						  &size, NULL);
 		if (!data)
 			die(_("unable to read tree (%s)"), oid_to_hex(&obj->oid));
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index a1e33d7507..feb0320371 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1649,7 +1649,7 @@  static void add_preferred_base(struct object_id *oid)
 		return;
 
 	data = read_object_with_reference(the_repository, oid,
-					  tree_type, &size, &tree_oid);
+					  OBJ_TREE, &size, &tree_oid);
 	if (!data)
 		return;
 
diff --git a/cache.h b/cache.h
index 148d9ab5f1..dad9792c74 100644
--- a/cache.h
+++ b/cache.h
@@ -1508,7 +1508,7 @@  int cache_name_stage_compare(const char *name1, int len1, int stage1, const char
 
 void *read_object_with_reference(struct repository *r,
 				 const struct object_id *oid,
-				 const char *required_type,
+				 enum object_type object_type,
 				 unsigned long *size,
 				 struct object_id *oid_ret);
 
diff --git a/object-file.c b/object-file.c
index 624af408cd..d2f223dcef 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1669,25 +1669,24 @@  void *read_object_file_extended(struct repository *r,
 
 void *read_object_with_reference(struct repository *r,
 				 const struct object_id *oid,
-				 const char *required_type_name,
+				 enum object_type object_type,
 				 unsigned long *size,
 				 struct object_id *actual_oid_return)
 {
-	enum object_type type, required_type;
 	void *buffer;
 	unsigned long isize;
 	struct object_id actual_oid;
 
-	required_type = type_from_string(required_type_name);
 	oidcpy(&actual_oid, oid);
 	while (1) {
 		int ref_length = -1;
 		const char *ref_type = NULL;
+		enum object_type type;
 
 		buffer = repo_read_object_file(r, &actual_oid, &type, &isize);
 		if (!buffer)
 			return NULL;
-		if (type == required_type) {
+		if (type == object_type) {
 			*size = isize;
 			if (actual_oid_return)
 				oidcpy(actual_oid_return, &actual_oid);
diff --git a/tree-walk.c b/tree-walk.c
index 2d6226d5f1..e5db9291e1 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -89,7 +89,7 @@  void *fill_tree_descriptor(struct repository *r,
 	void *buf = NULL;
 
 	if (oid) {
-		buf = read_object_with_reference(r, oid, tree_type, &size, NULL);
+		buf = read_object_with_reference(r, oid, OBJ_TREE, &size, NULL);
 		if (!buf)
 			die("unable to read tree %s", oid_to_hex(oid));
 	}
@@ -605,7 +605,7 @@  int get_tree_entry(struct repository *r,
 	unsigned long size;
 	struct object_id root;
 
-	tree = read_object_with_reference(r, tree_oid, tree_type, &size, &root);
+	tree = read_object_with_reference(r, tree_oid, OBJ_TREE, &size, &root);
 	if (!tree)
 		return -1;
 
@@ -677,7 +677,7 @@  enum get_oid_result get_tree_entry_follow_symlinks(struct repository *r,
 			unsigned long size;
 			tree = read_object_with_reference(r,
 							  &current_tree_oid,
-							  tree_type, &size,
+							  OBJ_TREE, &size,
 							  &root);
 			if (!tree)
 				goto done;