Re: git rebase regression: cannot pass a shell expression directly to --exec

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: May 16, 2017, 19:14 UTC
Message-ID: <CA+55aFwB-MWASj7dZWkXWhgd4gvEfoOhL6Fo7kXeJSm9dht4Jg@mail.gmail.com>
In-Reply-To: <20170516172307.36hyshwypomlsubx@sigill.intra.peff.net>

On Tue, May 16, 2017 at 10:23 AM, Jeff King <peff@peff.net> wrote:

Show 14 quoted lines

>
> I think the logic here would be more like:
>
>   1. During prepare_shell_cmd(), even if we optimize out the shell call,
>      still prepare a fallback argv (since we can't allocate memory
>      post-fork).
>
>   2. In the forked child, if we get ENOENT from exec and cmd->use_shell
>      is set, then exec the fallback shell argv instead. Propagate its
>      results, even if it's 127.
>
> That still means we'd prefer a $PATH copy of a command to its shell
> builtin variant, but that can't be helped (and I kind of doubt anybody
> would care too much).

I think it would be better to just

 (a) get rid of the magic strcspn() entirely

 (b) make the 'can we optimize this' test be simply just looking up
'argv[0]' in $PATH

Hmm? If you have executables with special characters in them in your PATH, you have bigger issues.

Also, if people really want to optimize the code that executes an external program (whether in shell or directly), I think it might be worth it to look at replacing the "fork()" with a "vfork()".

Something like this

-       cmd->pid = fork();
+       cmd->pid = (cmd->git_cmd || cmd->env) ? fork() : vfork();

might work (the native git_cmd case needs a real fork, and if we change the environment variables we need it too, but the other cases look like they might work with vfork()).

Using vfork() can be hugely more efficient, because you don't have the extra page table copies and teardown, but also avoid a lot of possible copy-on-write faults.

                 Linus