On Fri, Apr 09, 2021 at 09:42:17PM +0200, Ævar Arnfjörð Bjarmason wrote:
> > 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.).
I suspect you'll end up with a lot of awkward spots. I do agree that
_most_ callers only care about getting one of the actual 4 object types,
or an error. But those values are tied to the delta ones internally
(when we see a delta type, and then later decide to promote it to the
"real" type). And of course those are all used to read and write the
on-disk bits, too.
So while there might be some way of doing this cleaner, it hasn't
historically been a place we've seen a lot of problems (at least not
that I recall). So it seems like a pretty deep rabbit hole that is not
likely to give a lot of benefit.
-Peff
