Re: [PATCH 1/1] t6500(mingw): use the Windows PID of the shell

From: Junio C Hamano <gitster@pobox.com>
Date: May 8, 2019, 03:53 UTC
Message-ID: <xmqqef59iou2.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CAPig+cQG4+A+G+i+8RqpDAY2PveULVJ5iNR4HYEUPAd_4Ub04w@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 23 quoted lines

> On Tue, May 7, 2019 at 5:51 PM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> [...]
>> Let's fix this by making sure that the Windows PID is written into
>> `gc.pid` in this test script soo that `git.exe` is able to understand
>> that that process does indeed still exist.
>>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>> @@ -162,7 +162,15 @@ test_expect_success 'background auto gc respects lock for all operations' '
>> +       shell_pid=$$ &&
>> +       if test_have_prereq MINGW && test -f /proc/$shell_pid/winpid
>> +       then
>> +               # In Git for Windows, Bash (actually, the MSYS2 runtime) has a
>> +               # different idea of PIDs than git.exe (actually Windows). Use
>> +               # the Windows PID in this case.
>> +               shell_pid=$(cat /proc/$shell_pid/winpid)
>> +       fi &&
>> +       printf "%d %s" "$shell_pid" "$hostname" >.git/gc.pid &&
>
> I wonder if it would make sense to abstract this away behind a shell
> function named shell_pid() which can be specialized for MINGW, much
> like the shell function pwd() is specialized on Windows.

It would be, especially when we need the next such invocation. I'd find it easier to follow if it were

	if test_have_prereq ...
	then
		shell_pid=$(cat /proc/$$/winpid)
	else
		shell_pid=$$
	fi &&
	...

simply because in each of the cases the mental burden gets smaller (those trying to see what happens on MINGW do not have to recall shell_pid was originally taken from $$ after seeing 'then'; others do not have to wonder if lack of 'else' to cover the platforms they are reading for is deliberate and shell_pid is the only thing expected out of the if/then/fi construct)

And I would suggest doing such a rewrite when it becomes a helper shell function, but what is written here is good enough until then, I would think.