| Message ID | patch-04.10-48aca62864-20210420T124428Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | object.c et al: tests, small bug fixes etc. | expand |
Æ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 --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, ¤t_tree_oid, - tree_type, &size, + OBJ_TREE, &size, &root); if (!tree) goto done;
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(-)