Re: [PATCH 3/7] oid_object_info(): return "enum object_type"
From: Junio C Hamano <hidden>
Date: 2021-03-08 21:55:47
Ævar Arnfjörð Bjarmason [off-list ref] 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.
quoted hunk
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).
quoted hunk
- 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.