On Fri, Apr 09 2021, Jeff King wrote: > On Fri, Apr 09, 2021 at 10:32:51AM +0200, Ævar Arnfjörð Bjarmason wrote: > >> Change the type_from_string*() functions to return an "enum >> object_type", but don't refactor their callers to check for "== >> OBJ_BAD" instead of "< 0". >> >> Refactoring the check of the return value to check == OBJ_BAD would >> now be equivalent to "ret < 0", but the consensus on an earlier >> version of this patch was to not do that, and to instead use -1 >> consistently as a return value. It just so happens that OBJ_BAD == -1, >> but let's not put a hard reliance on that. > > I think what the patch is doing is good, but this rationale misses the > main point of that discussion, I think. I doubt that the value of > OBJ_BAD would ever change. But the point was that we could grow a new > "failure" value at "-2", and we would want to catch here (I do consider > it relatively unlikely, but that IMHO is the reason to keep the negative > check). > > I think for the same reason that "return OBJ_BAD" instead of "return -1" > would be just fine (it is not "just so happens" that OBJ_BAD is > negative; that was deliberate to allow exactly this convention). But I > am also OK with leaving the "return -1" calls. I'm beginning to think in response to this and the comment on 5/6 that it might be cleaner to split up the object_type enum, as demonstrated for a config.[ch] feature in [1]. Converting back and forth between them is a bit nasty, and having multiple interchangable OBJ_* constants with identical values just to satisfy them being in different enums, but it would allow having the compiler explicitly help check that callers cover all possible cases of values they could get. Most callers just get OBJ_{COMMIT,TREE,BLOB,TAG} some more get that plus OBJ_{BAD,NONE}, almost nobody gets OBJ_{OFS,REF}_DELTA, and AFAICT just the peel code cares about OBJ_ANY. We then have an OBJ_MAX nobody's ever used for anything (I've got some unsubmitted patch somewhere to remove it). What do you think about that sort of approach? I haven't convinced myself that it's a good idea, so far I just thought bridging the gap of things that return "enum" actually having that as part of their signature for human legibility, even if C itself doesn't care about the difference, and we currently can't get much/any of the benefits of the compiler catching non-exhaustive "case" statements (unless every callsite is to include OBJ_OFS etc.). 1. https://lore.kernel.org/git/875z0wicmp.fsf@xxxxxxxxxxxxxxxxxxx/