Subject: Re: [PATCH v2 19/25] sequencer: remember do_recursive_merge()'s return value

From: Johannes Schindelin <[email-masked]>
Date: Wed, 5 Oct 2016 14:35:50 +0200 (CEST)

Hi Junio,

On Mon, 12 Sep 2016, Junio C Hamano wrote:

> Johannes Schindelin <[email-masked]> writes:
> 
> > The return value of do_recursive_merge() may be positive (indicating merge
> > conflicts), so let's OR later error conditions so as not to overwrite them
> > with 0.
> 
> Are the untold assumptions as follows?
> 
>  - The caller wants to act on positive (not quite an error), zero
>  (success) or negative (error);
> 
>  - do_recursive_merge() can return positive (success with reservation),
>  zero or negative, and the call to it would return immediately if it got
>  negative;
> 
>  - all other functions that come later may return either zero or
>  negative, and never positive;
> 
>  - Hence the caller can be assured that "res" being positive can only
>  mean do_recursive_merge() succeeded with reservation and everything
>  else succeeded.

More or less. The idea is that there are problems with the merge, and
there are fatal errors.

Alex and I chose to stay close to the original Python implementation when
we decided that merge_trees() should return 1 for success and 0 for
failure. In hindsight, I regret that, as it disagrees with the C
convention to return 0 for success. However, it would have been yet
another mistake to return -1 in case of merge conflicts: the function did
not have a problem (such as out-of-memory or some such).

I can only guess that the do_recursive_merge() function tried to undo the
damage by reversing the logic: 0 for success, 1 for unclean merge. It is
at least more in line with the C convention.

So much so, in fact, that we can still use negative values to indicate
fatal errors that should be handled accordingly.

> This can be extended if the only thing the caller cares about is
> positive/zero/negative and it does not care what exact positive
> value it gets--in such a case, we can loosen the condition on the
> return values from other functions whose return values are OR'ed
> together; they may also return positive to signal the same "not
> quite an error", i.e. updating the latter two points to
> 
>  - all other functions that come later can return positive (success
>    with reservation), zero or negative.
> 
>  - Hence the caller can be assured that "res" being positive can
>    mean nobody failed with negative return, but it is not an
>    unconditional success, which is signalled by value "res" being
>    0.
> 
> I cannot quite tell which is the case, especially what is planned in
> the future.  The proposed log message is a good place to explain the
> future course this code will take.

Okay, I will try to come up with a better commit message.

Actually, come to think of it, I will change the patch, as it is too
confusing. What I want is to preserve a positive return value in case of
merge conflicts, and that is exactly what I should do instead of playing
games with the Boolean OR operator.

Ciao,
Dscho