From: Derrick Stolee <stolee@gmail.com>
Date: Fri, 28 Jun 2024 02:12:08 GMT
Subject: Re: [PATCH v2 4/7] merge-ort: clearer propagation of failure-to-function from merge_submodule
Message-ID: <63caecb4-19cd-4b6f-91f0-bd00df2ecb54@gmail.com>
In-Reply-To: <2813a15b48b70ead7e3fd062d1b49baee665fc9d.1718766019.git.gitgitgadget@gmail.com>

On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> The 'clean' member variable is somewhat of a tri-state (1 = clean, 0 =
> conflicted, -1 = failure-to-determine), but we often like to think of
> it as binary (ignoring the possibility of a negative value) and use
> constructs like '!clean' to reflect this.  However, these constructs
> can make codepaths more difficult to understand, unless we handle the
> negative case early and return pre-emptively; do that in
> handle_content_merge() to make the code a bit easier to read.

This patch is correct and valuable.

Would it be valuable to go a bit further and turn 'clean' into
an enum that reflects these states? Perhaps that would prevent
future changes from slipping into this mistake.

Thanks,
-Stolee