On 04/13, Jonathan Nieder wrote:

Brandon Williams wrote:

quoted

From what I can see, there are now no calls in the child process (after fork
and before exec/_exit) which are not Async-Signal-Safe.  This means that
fork/exec in a threaded context should work without deadlock

I don't see why the former implies the latter.  Can you explain
further?

You already know my opinions about fork+threads by now.  I continue to
think this is heading in a direction of decreased maintainability that
I dread.

I disagree here.  No one thought it was a bad idea back when I was
implementing grep to fork while running with threads.  Id rather fix the
problem in run_command so that we don't ever have to worry about this
again, especially for contributors who are unaware of this issue while
threading.

That's not to say that this is wasted work.  I would prefer an approach
like the following:

 1. First, make grep work without running fork() from threads.
    Jonathan Tan's approach would be one way to do this.  Another way
    would be to simply disable threads in --recurse-submodules mode.

    This would be the first thing to do because it would make tests
    reliable again, without having to wait for deeper changes.

I'm not much of a fan of Jonathan Tan's suggestion, id rather just fix
the problem at its root instead of adding in an additional hack.  If
this series crashes and burns then yes, lets just shut off threading in
grep with --recurse-submodules is uses, otherwise this series will fix
that case.

 2. Then, teaching run_command to prepare the environment and do $PATH
    lookup before forking.  This might make it possible for run_command
    to use vfork or might not.

 3. Teaching run_command to delegate chdir to the child, using -C for
    git commands and 'cd' for shell commands, and using a shell where
    necessary where it didn't before.

 4. Switching run_command to use posix_spawn on platforms where it is
    available, which would make it possible to use in a threaded
    context on those platforms.

After this series it should be easy enough to convert to posix_spawn if
someone wants to follow up with a patch to do that.

-- 
Brandon Williams