Re: Can dependency on /bin/sh be removed?

From: Jeff King <peff@peff.net>
Date: Jul 17, 2024, 05:52 UTC
Message-ID: <20240717055203.GB547635@coredump.intra.peff.net>
In-Reply-To: <xmqqo76x6r69.fsf@gitster.g>

On Tue, Jul 16, 2024 at 01:02:54PM -0700, Junio C Hamano wrote:

Show 10 quoted lines

> > Again, it's possible that we could detect that no shell metacharacters
> > are in play and do the word-splitting ourselves. But at that point I
> > think it should go into run-command's prepare_shell_cmd(). That is, I I
> > think it could take space out of the list of metachars that force us to
> > invoke the shell, and do the word-splitting there. But not having
> > thought very hard about it, there are probably corner cases where that
> > optimization is detectable by the user (presumably unusual IFS, but
> > maybe more?).
> 
> Well, I strongly object to an approach for us to "parse" anything.

OK, it makes me nervous, too, so I am happy to leave it be (though it is interesting to hear that "make" does a similar optimization).

Show 18 quoted lines

> But even then it would be sensible to formulate:
> 
> 	argv[0] = sh
> 	argv[1] = -c
> 	argv[2] = git-credential-cache --socket=/path/	--timeout=123 "$@"
> 	argv[3] = -
> 	argv[4] = NULL
> 
> and if there is an argument say "get", extend it to
> 
> 	argv[0] = sh
> 	argv[1] = -c
> 	argv[2] = git-credential-cache --socket=/path/	--timeout=123 "$@"
> 	argv[3] = -
> 	argv[4] = get
> 	argv[5] = NULL
> 
> before passing the array to execv(), no?

Hmm. I think that is OK. It is a little funny to have some arguments pasted in and some via "$@", but I think in the end it should be indistinguishable to the user.

In your example it is "git-credential-cache", whereas we do "git credential-cache" now. I _think_ the two should be equivalent at this point (since the parent process running this code would already have set up GIT_EXEC_PATH to find dashed forms), but it does give me pause.

We could keep saying "git credential-cache", but then in most cases we would never trigger the metacharacter optimization, because of the space.

And of course:

Splitting it out like this:

  argv[0] = sh
  argv[1] = -c
  argv[2] = git "$@"
  argv[3] = -
  argv[4] = credential-cache --socket=/path/ --timeout=123
  argv[5] = get
  argv[6] = NULL

is wrong, because argv[4] is really a shell snippet, not a single argument (and we cannot split it ourselves without doing shell-like parsing).

Show 8 quoted lines

> And with the metacharacter optimization to drop .use_shell we
> already have, a single-token /bin/myhelper case would then become
> 
> 	argv[0] = /bin/myhelper
> 	argv[1] = get
> 	argv[2] = NULL
> 
> naturally.

Yes, I think that is the one case that would benefit. I do wonder how often people point to an absolute path, though, rather than git-credential-* or a more complex shell invocation.

-Peff