Re: [PATCH] ll-merge: killing the external merge driver aborts the merge

From: Joshua Hudson <jhudson@cedaron.com>
Date: Jun 25, 2023, 13:40 UTC
Message-ID: <ac9b1ccb-82ba-4046-dd2a-e10b38a528a3@cedaron.com>
In-Reply-To: <xmqqjzvt92nw.fsf@gitster.g>

On 6/23/2023 4:31 PM, Junio C Hamano wrote:

Show 20 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Elijah Newren <newren@gmail.com> writes:
>>
>>> Reviewed-by: Elijah Newren <newren@gmail.com>
>>
>> Thanks for a quick review.
> Unfortunately Windows does not seem to correctly detect the aborting
> merge driver.  Does run_command() there report process death due to
> signals differently, I wonder?
>
> https://github.com/git/git/actions/runs/5360400800/jobs/9725341775#step:6:285
>
> shows that on Windows, aborted external merge driver is not noticed
> and we happily take the auto-merged result, ouch.
>
> I am tempted to protect this step of the test with a prerequisite to
> skip it on Windows for now.  Anybody with better idea?
>
> Thanks.

I would suggest putting in the correct test harness on Windows. abort() doesn't work very well.

(Sample code only--read description below)

#ifdef WIN32
     TerminateProcess(GetCurrentProcess(), 131); /* something that looks 
like it passes the test */
     TerminateProcess(GetCurrentProcess(), 0x80070485); /* actual exit 
code for process that cannot start because its missing a shared library */
#else
     abort();
#endif

I only want to fight so hard with the Unix to Windows translation layer you use. Strictly speaking, the second TerminateProcess() line is what should be in the test harness, but if it doesn't work go with the first one. Then I at least have something to work with.

I'm not going to lie to you. We are doing development on Windows, and the merge driver is written using a different portability layer. I am prepared to build a native shim to make it work if I have to. I am not in a position where I can build git and test any development code; getting the fix to us will be a long journey.