Thank you very much for carefully checking the patch and suggesting
better ways. I'll later revise it and submit a new one.
> > +}
> > +
> > +stop_git_and_watchdog () {
> > + kill $git_pid $watchdog_pid
> > +}
>
> This sends a signal and let the process die. Without waiting to
> make sure they indeed died, at which point we can safely remove the
> $TRASH_DIRECTORY on filesystems that refuse to remove a directory
> when a process still has it as its current working directory.
>
> Shouldn't it loop, like
>
> for pid in $git_pid $watchdog_pid
> do
> until kill -0 $pid
> do
> kill $pid
> done
> done
>
> or something? Or is there a mechanism already to ensure that we
> return after they get killed that I am failing to find?
I agree that we have to wait for pids. I also realized that we should
run git in another process group and kill the group for killing all git
child processes. I'll fix the code.
> > test_expect_success 'explicit daemon start and stop' '
> > test_when_finished "stop_daemon_delete_repo test_explicit" &&
> >
> > @@ -907,6 +929,23 @@ test_expect_success "submodule absorbgitdirs implicitly starts daemon" '
> > test_subcommand git fsmonitor--daemon start <super-sub.trace
> > '
> >
> > +test_expect_success "submodule implicitly starts daemon by pull" '
> > + test_atexit "stop_git_and_watchdog" &&
>
> Hmph, this is _atexit and not _when_finished because...?
This is because README describes _atexit to run unconditionally to clean
up before the test script exits, e.g. to stop (kill) a daemon. More
appropriately, we should kill git before "rm -rf cloned super sub" in
_when_finished and kill watchdog in _atexit. I'll adjust the code.
> > + test_when_finished "rm -rf cloned; \
> > + rm -rf super; \
> > + rm -rf sub" &&
>
> Makes me wonder why it is not written like so:
>
> test_when_finished "rm -rf cloned super sub" &&
>
> which is short enough to still fit on a line. Is there something I
> am missing that these directories must be removed separately and in
> this order?
There is no special reason, I simply followed the style used in
t7527-builtin-fsmonitor.sh. I'll fix this part.
Koji Nakamaru
