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
