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

Commit Message

Elijah Newren June 19, 2024, 3 a.m. UTC

From: Elijah Newren <[e-mail]>

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.

Signed-off-by: Elijah Newren <[e-mail]>
---
 merge-ort.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Derrick Stolee June 28, 2024, 2:12 a.m. UTC | #1

On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <[e-mail]>
> 
> 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

Elijah Newren June 28, 2024, 2:38 a.m. UTC | #2

On Thu, Jun 27, 2024 at 7:12 PM Derrick Stolee <[e-mail]> wrote:
>
> On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <[e-mail]>
> >
> > 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.

Derrick Stolee June 28, 2024, 2:47 a.m. UTC | #3

On 6/27/24 10:38 PM, Elijah Newren wrote:
> On Thu, Jun 27, 2024 at 7:12 PM Derrick Stolee <[e-mail]> wrote:
>>
>> On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:
>>> From: Elijah Newren <[e-mail]>
>>>
>>> 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.

Your fast response to this message means that I didn't see this when
I mentioned it in my closing of the review (in response to your
cover letter).

Reducing the size of the conversion would definitely be good to do,
and then you could also consider using the existing ll_merge_result
enum, though it technically has four states.

Thanks,
-Stolee

Patch

diff --git a/merge-ort.c b/merge-ort.c
index be0f5bc3838..d187c966c6a 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2193,6 +2193,8 @@  static int handle_content_merge(struct merge_options *opt,
 		clean = merge_submodule(opt, pathnames[0],
 					two_way ? null_oid() : &o->oid,
 					&a->oid, &b->oid, &result->oid);
+		if (clean < 0)
+			return -1;
 		if (opt->priv->call_depth && two_way && !clean) {
 			result->mode = o->mode;
 			oidcpy(&result->oid, &o->oid);