On Wed, Oct 2, 2024 at 3:04 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: >>> > +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). I see. I'll fix this part. >>> > + 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. Thank you for the clarification and the support. Koji Nakamaru
