From: Jeff King <peff@peff.net>
Date: Mon, 24 Mar 2025 03:50:01 GMT
Subject: Re: [PATCH 2/8] pretty: simplify if-else to reduce code duplication
Message-ID: <20250324035001.GC690093@coredump.intra.peff.net>
In-Reply-To: <5f787ddac2d80391feadb8cf6be379fc8e58652f.1742367347.git.martin.agren@gmail.com>

On Wed, Mar 19, 2025 at 08:23:35AM +0100, Martin Ågren wrote:

> First we look for "auto,", then we try "always,", then we fall back to
> the default, which is to do exactly the same thing as we do for "auto,".
> The amount of code duplication isn't huge, but still: reading this code
> carefully requires spending at least *some* time on making sure the two
> blocks of code are indeed identical.
> 
> Rearrange the checks so that we end with the default case,
> opportunistically consuming the "auto," which may or may not be there.

OK. The duplicated lines are not all that long, but I don't mind
collapsing the cases, especially with the explanatory comment that's
there.

> In the "always," case, we don't actually *do* anything, so if we were
> into golfing, we'd just write the whole thing as a single
> 
>   if (!skip_prefix(begin, "always,", &begin)) {
>     ...
>   }
> 
> If we ever learn something new besides "always," and "auto," we'd need
> to pull things apart again. Plus we still need somewhere to place the
> comment. Let's focus on code de-duplication rather than golfing for now.

Yeah, I think what you wrote in the patch is much better than trying to
golf further.

So looks good to me.

-Peff