git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Johannes Sixt <j6t@kdbg.org>
To: Jeff King <peff@peff.net>
Cc: Junio C Hamano <gitster@pobox.com>,
	Nanako Shiraishi <nanako3@lavabit.com>,
	git@vger.kernel.org
Subject: Re: [PATCH 3/6] run-command: optimize out useless shell calls
Date: Thu, 31 Dec 2009 17:54:37 +0100	[thread overview]
Message-ID: <4B3CD74D.7020605@kdbg.org> (raw)
In-Reply-To: <20091230105536.GC22959@coredump.intra.peff.net>

Jeff King schrieb:
> If there are no metacharacters in the program to be run, we
> can just skip running the shell entirely and directly exec
> the program.
> 
> The metacharacter test is pulled verbatim from
> launch_editor, which already implements this optimization.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Something inside me feels wrong with a catch-known-metacharacter test
> instead of an allow-known-good check. But this is the same test we have
> been using with launch_editor for some time, so I decided not to mess
> with it.

The git version that msysgit ships (and the one that I use on Windows) has 
this sequence in pager.c:

static const char *pager_argv[] = { "sh", "-c", NULL, NULL };
...
	pager_process.argv = &pager_argv[2];
	pager_process.in = -1;
	if (start_command(&pager_process)) {
		pager_process.argv = pager_argv;
		pager_process.in = -1;
		if (start_command(&pager_process))
			return;
	}

to help people set

  PAGER=C:\Program Files\cygwin\bin\less

That is, we first try to run the program without the shell, then retry 
wrapped in sh -c.

Wouldn't it be possible to do the same here, assuming that we don't have 
programs such as "editor -f" in the path?

It does assume that we are able to detect execvp failure due to ENOENT 
which is currently proposed elsewhere by Ilari Liusvaara (and which is 
already possible on Windows).

-- Hannes

  reply	other threads:[~2009-12-31 16:54 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-12-14 22:17 Giving command line parameter to textconv command? Nanako Shiraishi
2009-12-14 23:31 ` Junio C Hamano
2009-12-15  3:11   ` Nanako Shiraishi
2009-12-15  5:56     ` Junio C Hamano
2009-12-15 16:49       ` Jeff King
2009-12-16  1:05         ` Junio C Hamano
2009-12-16  1:13           ` Jeff King
2009-12-15 17:03   ` Jeff King
2009-12-15 17:23     ` Junio C Hamano
2009-12-30  3:13   ` Nanako Shiraishi
2009-12-30  8:05     ` Junio C Hamano
2009-12-30  9:56       ` Jeff King
2009-12-30 10:53         ` [PATCH 1/6] run-command: add "use shell" option Jeff King
2009-12-30 13:55           ` Erik Faye-Lund
2010-01-01 22:12           ` Johannes Sixt
2009-12-30 10:53         ` [PATCH 2/6] run-command: convert simple callsites to use_shell Jeff King
2009-12-30 10:55         ` [PATCH 3/6] run-command: optimize out useless shell calls Jeff King
2009-12-31 16:54           ` Johannes Sixt [this message]
2009-12-31 19:47             ` Junio C Hamano
2009-12-31 21:41               ` Johannes Sixt
2009-12-31 21:41             ` Jeff King
2009-12-31 22:16               ` Johannes Sixt
2010-01-01  4:50                 ` Jeff King
2010-01-01 10:08                   ` Johannes Sixt
2009-12-30 10:56         ` [PATCH 4/6] editor: use run_command's shell feature Jeff King
2009-12-30 11:01         ` [PATCH 5/6] textconv: use shell to run helper Jeff King
2009-12-30 11:03         ` [PATCH 6/6] diff: run external diff helper with shell Jeff King
2010-01-01 22:14           ` [PATCH 7/6] t0021: use $SHELL_PATH for the filter script Johannes Sixt
2010-01-01 22:15             ` [PATCH 8/6] t4030, t4031: work around bogus MSYS bash path conversion Johannes Sixt
2010-01-03  7:24             ` [PATCH 7/6] t0021: use $SHELL_PATH for the filter script Jeff King
2010-01-04 15:50               ` Johannes Sixt
2010-01-04 16:03                 ` Jeff King
2010-01-04 16:46                   ` Johannes Sixt

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

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

  git send-email \
    --in-reply-to=4B3CD74D.7020605@kdbg.org \
    --to=j6t@kdbg.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=nanako3@lavabit.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.
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 NNTP newsgroup(s).