Re: [PATCH v2 04/10] object-file.c: take type id, not string, in read_object_with_reference()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 29, 2021, 04:37 UTC
- Message-ID
- <xmqqeeet905b.fsf@gitster.g>
- In-Reply-To
- <patch-04.10-48aca62864-20210420T124428Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 7 quoted lines
> 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.
Show 32 quoted lines
> 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.
Show 33 quoted lines
> 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.