Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> Change oid_object_info() to return an "enum object_type", this is what
> it did anyway, except that it hardcoded -1 instead of an
> OBJ_BAD.
>
> Let's instead have it return the "enum object_type", at which point
> callers will expect OBJ_BAD. This allows for refactoring code that
> e.g. expected any "< 0" value, but would only have to deal with
> OBJ_BAD (= -1).
Hmph, I have a mixed feeling about this.
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 5ebf13359e8..1d989c62a4e 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -133,7 +133,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
>
> case 'p':
> type = oid_object_info(the_repository, &oid, NULL);
> - if (type < 0)
> + if (type == OBJ_BAD)
> die("Not a valid object name %s", obj_name);
So, when oid_object_info() starts to return different negative value
to signal new kinds of errors, this codepath need to be changed, or
the control falls through to the rest of the case arm, which would
be worse than what the current code does (which is to die with less
specific error message).
> - int type = oid_object_info(the_repository, &obj->oid, &size);
> - if (type <= 0)
> + enum object_type type = oid_object_info(the_repository, &obj->oid, &size);
> + if (type == OBJ_BAD)
> die(_("did not receive expected object %s"),
> oid_to_hex(&obj->oid));
Ditto.
And the issue is the same for all the other explicit comparison with
OBJ_BAD. If we do it the other way around, i.e. leave these callers
as they are and add new negative return values to the function first,
and then convert "if negative, say error" to "if OBJ_BAD, say so,
else if we have this new type of error, say so", then the risk of
mistake becomes smaller.
But hopefully any such potential issue will be resolved by the end
of this short series, so as long as it won't be left as technical
debt, I am OK.
