Re: [PATCH 3/3] execv_dashed_external: wait for child on signal death

From: Jeff King <peff@peff.net>
Date: Jan 7, 2017, 07:34 UTC
Message-ID: <20170107073454.mz2pdphipv3gqvfr@sigill.intra.peff.net>
In-Reply-To: <650a25f6-5f22-8efb-3048-6afadbaa7092@kdbg.org>

On Sat, Jan 07, 2017 at 08:28:22AM +0100, Johannes Sixt wrote:

Show 9 quoted lines

> > But when you have a dashed external (or an alias pointing to
> > a builtin, which will re-exec git for the builtin), there's
> > an extra process in the mix. For instance, running:
> > 
> >   git -c alias.l=log log
> 
> This should be
> 
>   git -c alias.l=log l

Yeah, it should be.

> For the complete series:
> 
> Acked-by: Johannes Sixt <j6t@kdbg.org>

Thanks.

> What should we add to Documentation/technical/api-run-command.txt about the
> new flag? "Do not use?" "Understand the commit message of <this commit>
> before setting the flag to true?"

I think it can be explained pretty easily as "after killing all children marked for clean_on_exit, do a blocking waitpid() on any child that is also marked wait_after_clean". But I notice we do not actually document clean_on_exit, either, nor most of the options in "struct child_process".

IMHO it would be an improvement to merge the contents of the technical/api-run-command.txt documentation into the header file, and document each option with a comment above its definition. That makes it a lot easier to have the will to document any newly-added options, because if you don't they look bad next to the others. :)

Mechanics aside, I am not sure if we need a "do not use" or "here are the caveats". I think if we explain what it does, we can rely on list discussion and review to catch any obviously-stupid uses (both of it and of clean_on_exit).

-Peff