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

From: Taylor Blau <[email-masked]>
Date: Thu, 16 Sep 2021 22:45:17 -0400

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