On Tue, Jul 16, 2024 at 01:02:54PM -0700, Junio C Hamano wrote: > > 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). > 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). > 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
