Re: [PATCH v2] fsmonitor OSX: fix hangs for submodules

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 1, 2024, 18:04 UTC
Message-ID: <xmqqmsjnya1c.fsf@gitster.g>
In-Reply-To: <CAOTNsDyg2SB-wd+a7vrctXck46jyfqV4uME6nf4YQZEafWbxMw@mail.gmail.com>

Koji Nakamaru <koji.nakamaru@gree.net> writes:

Show 15 quoted lines

>> > +test_expect_success "submodule implicitly starts daemon by pull" '
>> > + test_atexit "stop_watchdog" &&
>> > + test_when_finished "stop_git && rm -rf cloned super sub" &&
>>
>> If stop_git ever returns with non-zero status, "rm -rf" will be
>> skipped, which I am not sure is a good idea.
>>
>> The whole test_when_finished would fail in such a case, so you would
>> notice the problem right away, which is a plus, though.
>
> t/README discusses that test_when_finished and test_atexit differ about
> the "--immediate" option. As git and its subprocesses are the test
> target, I moved stop_git to the current place. This might be however
> confusing when someone later reads this test. Should we simply put
> stop_git and stop_watchdong in test_atexit?

That is not what I meant.

I was merely questioning the &&-chaining that stops "rm -fr" from running if stop_git ever fails (and your earlier iteration you had multiple "rm -fr" ;-chained, not &&-chained---not using && is often more appropriate in a when_finished handler).

Show 12 quoted lines

>> > + set -m &&
>>
>> I have to wonder how portable (and necessary) this is.
>>
>> POSIX says it shall be supported if the implementation supports the
>> User Portability Utilities option.  It also says that it was added
>> to apply only to the UPE because it applies primarily to interactive
>> use, not shell script applications.  And our test scripts are of
>> course not interactive.
>
> How about the following modification? It still utilizes $git_pgid to
> filter processes, but avoids "set -m".

Nah, your original reads much better, and the code is grabbing and using the process group information anyway (and my question about "-m" was more about "should we be relying on process group features in this test to kill them all?").

I am OK with the idea that we can assume, at least among the platforms that support fsmonitor, that sending a signal to a process group would cause the signal delivered to the member processes just as we expect.

Thanks.