Re: [PATCH 3/6] run-command: optimize out useless shell calls

From: Jeff King <peff@peff.net>
Date: Dec 31, 2009, 21:41 UTC
Message-ID: <20091231214134.GA31399@coredump.intra.peff.net>
In-Reply-To: <4B3CD74D.7020605@kdbg.org>

On Thu, Dec 31, 2009 at 05:54:37PM +0100, Johannes Sixt wrote:

Show 13 quoted lines

> 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;
> 	}

As a side note to what you are saying, my patch 2/6 will break this. The "new" way to do it would be:

  /* do not set pager_argv[2] specially */
  pager_process.in = -1;
  if (start_command(&pager_process)) {
          pager_process.use_shell = 1;
          pager_process.in = -1;
          if (start_command(&pager_process))
                  return;
  }

though I am happy to also just revert the pager.c changes and leave it to handle the shell itself.

But of course if we use your trick internally in run-command, then your pager-specific change can just go away.

Show 9 quoted lines

> 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?

Hmm. That is somewhat clever. And it would actually solve the backward-compatibility problem for helpers moving to shell execution. If you have a textconv of "/path with space/foo", it will continue to work transparently, but now "/path_without_space/foo --option" will also Just Work.

The two downsides I see are:

  1. You want to run "foo" with "-bar" in your path but you have "foo
     -bar" in your path (your "editor -f" example). This just seems
     insane to me.

  2. There is a little bit of an interface inconsistency. You can do
     PAGER="/path with space/foo", but once you want to add an option,
     PAGER="/path with space/foo -bar" does not do what you mean. You
     have to understand the magic that is going on, and that you now
     need to quote the first part.

     I'm not sure that is not a feature, though. You are paying that
     cost in the transition, but getting the DWYM magic for the common
     case.

> 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).

We could also simply do the path lookup ourselves, decide whether to use the shell, and then exec. Path lookup is not that hard, and I think we already have mingw compat routines for it anyway.

-Peff