Re: [PATCH v2 4/7] merge-ort: clearer propagation of failure-to-function from merge_submodule

> On Thu, Jun 27, 2024 at 7:12 PM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> 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.
> 
> That may make sense to investigate, but I suspect it may be a bigger
> change and would recommend making such a clean up a separate series.
> 
> Also, I'm curious if it makes sense to finish off replacing recursive
> with ort first; as long as recursive exists, it has the same problem
> and in fact was the source of using a tri-state 'clean' variable and
> thus would need the same cleanup.  But if we replace recursive with
> ort (making explicit requests for 'recursive' be handled by 'ort', as
> originally designed and intended), that cuts the number of sites
> needing this cleanup in half.