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

From: Koji Nakamaru <koji.nakamaru@gree.net>
Date: Oct 1, 2024, 04:11 UTC
Message-ID: <CAOTNsDwe=RXy5sH7myzj6g4JZpVfHm7F1ch38jqVE+YUV=Efmg@mail.gmail.com>
In-Reply-To: <xmqqcykl82fr.fsf@gitster.g>

Thank you very much for carefully checking the patch and suggesting better ways. I'll later revise it and submit a new one.

Show 23 quoted lines

> > +}
> > +
> > +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.

Show 11 quoted lines

> >  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.

Show 11 quoted lines

> > +     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