Re: [RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge

From: Phil Hord <hordp@cisco.com>
Date: Jun 10, 2014, 18:51 UTC
Message-ID: <539753C3.2020101@cisco.com>
In-Reply-To: <xmqq8up4abs3.fsf@gitster.dls.corp.google.com>

On 06/10/2014 01:56 PM, Junio C Hamano wrote:

Show 29 quoted lines

> Fabian Ruch <bafain@gmail.com> writes:
>
>> On 05/27/2014 08:42 PM, Junio C Hamano wrote:
>>> Fabian Ruch <bafain@gmail.com> writes:
>>>> [..]
>>>>
>>>> In order to signal the three possible situations (not only success and
>>>> failure to complete) after a pick through porcelain commands such as
>>>> `cherry-pick`, exit with a return value that is neither 0 nor 1. -1 was
>>>> chosen in line with the other situations in which the sequencer
>>>> encounters an error.
>>> Hmph... do we still pass negative to exit() anywhere in our codebase?
>> No, but I thought `cmd_cherry_pick` would just forward the `return -1` from the
>> sequencer to the shell. I had another look and found that `cmd_cherry_pick`
>> calls `die` instead. Now, the commit inserts 128 as exit status in
>> `fast_forward_to`.
>>
>> Would it be appropriate to mention the choice of exit status in the coding
>> guidelines? I didn't know that the int-argument to exit(3) gets truncated and
>> that this is why it is a general rule to only use values in the range from 0 to
>> 255 with exit(3).
> I personally do not think of a reason why it is necessary to mention
> how the argument to exit(3) is expected to be used by the system, but
> if it is common not to know it, I guess it would not hurt to have a
> single paragraph with at most two lines.
>
> In any case, I agree that exiting with 1 that signals "failed with
> conflict" can be confusing to the caller.  Can we have a test to
> demonstrate when this fix matters?

I think you are asking for a test and not for clarification. But a test was provided in 3/3 in this series. Was it not related directly enough?

For clarification, this tri-state return value matters when the caller is planning to do some cleanup and needs to handle the fallout correctly. Maybe changing this return value is not the correct way forward, though. It might be better if the caller could examine the result after-the-fact instead. This would require some reliable state functions which I recall were somewhat scattered last time I looked. Also I cannot think of a reliable test for "the previous cherry-pick failed during pre-condition checks" and I'm not sure anything should exist to track this in .git outside of the return value for this function.