git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Jeff King <peff@peff.net>
Cc: Brandon Williams <bmwill@google.com>,
	Jonathan Nieder <jrnieder@gmail.com>,
	Eric Rannaud <eric.rannaud@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Johannes Schindelin <johannes.schindelin@gmx.de>,
	Jeremy Serror <jeremy.serror@gmail.com>
Subject: Re: git rebase regression: cannot pass a shell expression directly to --exec
Date: Tue, 16 May 2017 12:14:38 -0700	[thread overview]
Message-ID: <CA+55aFwB-MWASj7dZWkXWhgd4gvEfoOhL6Fo7kXeJSm9dht4Jg@mail.gmail.com> (raw)
In-Reply-To: <20170516172307.36hyshwypomlsubx@sigill.intra.peff.net>

On Tue, May 16, 2017 at 10:23 AM, Jeff King <peff@peff.net> wrote:
>
> 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

  parent reply	other threads:[~2017-05-16 19:14 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-15 18:08 git rebase regression: cannot pass a shell expression directly to --exec Eric Rannaud
2017-05-16  3:25 ` Jeff King
2017-05-16  3:37   ` Jeff King
2017-05-16 16:41     ` Jonathan Nieder
2017-05-16 16:47       ` Jeff King
2017-05-16 17:11         ` Eric Rannaud
2017-05-16 17:15         ` Brandon Williams
2017-05-16 17:23           ` Jeff King
2017-05-16 17:30             ` Brandon Williams
2017-05-16 19:14             ` Linus Torvalds [this message]
2017-05-16 19:35               ` [TANGENT] run-command: use vfork instead of fork Eric Wong
2017-05-16 20:47                 ` Linus Torvalds
2017-05-16 21:11                   ` Brandon Williams
2017-05-16 20:12               ` git rebase regression: cannot pass a shell expression directly to --exec Johannes Schindelin
2017-05-16 20:27                 ` Linus Torvalds
2017-05-16 17:37         ` Jonathan Nieder
2017-05-16  3:40   ` Junio C Hamano
2017-05-16  3:53     ` Jeff King
2017-05-16  4:08       ` Jeff King
2017-05-16 16:45       ` Eric Rannaud
2017-05-16 10:46     ` Johannes Schindelin
2017-05-16 10:23 ` Johannes Schindelin
2017-05-16 16:18   ` Jeff King
2017-05-16 16:59     ` Eric Rannaud
2017-05-16 17:14       ` Kevin Daudt
2017-05-16 17:29         ` Eric Rannaud
2017-05-16 17:41           ` Jeff King
2017-05-16 17:21       ` Eric Rannaud
2017-05-16 17:37         ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CA+55aFwB-MWASj7dZWkXWhgd4gvEfoOhL6Fo7kXeJSm9dht4Jg@mail.gmail.com \
    --to=torvalds@linux-foundation.org \
    --cc=bmwill@google.com \
    --cc=eric.rannaud@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=jeremy.serror@gmail.com \
    --cc=johannes.schindelin@gmx.de \
    --cc=jrnieder@gmail.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).