Re: [PATCH v6 18/22] object-file.c: use "enum" return type for unpack_loose_header()

On Tue, Sep 07, 2021 at 12:58:13PM +0200, Ævar Arnfjörð Bjarmason wrote:

In the preceding commits we changed and documented
unpack_loose_header() from return any negative value or zero, to only
-2, -1 or 0. Let's instead add an "enum unpack_loose_header_result"
type and use it, and have the compiler assert that we're exhaustively
covering all return values. This gets rid of the need for having a
"default" BUG() case in loose_object_info().

I'm on the fence about whether this is more readable or worth it, but
since it was suggested in [1] to do this let's go for it.

:-). The first hunk is quite a long line, but I think that only suggests
the enum has a long name. I also can't think of anything shorter, so I
think what you have is just fine.

I do think that this is an improvement in readability, and for what it's
worth I am a fan of the previous two changes as well.

As a workflow comment, I would have perhaps done these conversions a
little earlier, maybe in these steps:

  - First a patch to introduce unpack_loose_header_result with just OK
    and BAD, and then converted all callers that return negative numbers
    to return BAD (and all others to return OK).

  - Then a second patch to convert some of the BAD returns into
    BAD_TOO_LONG.

That gets things done in two patches, instead of three, at the cost of a
slightly more complicated first patch. But I think you also get some
more insight into why we're making the change in the first place instead
of having to read through a couple of commits to get there.

In any case, what you have is certainly fine, and I don't think that one
approach is any better or worse than the other. Just mentioning it in
case it's something may try in the future.

This patch looks good.

Thanks,
Taylor