All of lore.kernel.org
 help / color / mirror / Atom feed
From: Brandon Williams <bmwill@google.com>
To: Jeff King <peff@peff.net>
Cc: Jonathan Nieder <jrnieder@gmail.com>,
	Eric Rannaud <eric.rannaud@gmail.com>,
	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 10:30:41 -0700	[thread overview]
Message-ID: <20170516173041.GH79147@google.com> (raw)
In-Reply-To: <20170516172307.36hyshwypomlsubx@sigill.intra.peff.net>

On 05/16, Jeff King wrote:
> On Tue, May 16, 2017 at 10:15:40AM -0700, Brandon Williams wrote:
> 
> > > > > So I was thinking something like the patch below, though I guess
> > > > > technically you could look for BASH_FUNC_$argv[0]%%, which seems to be
> > > > > bash's magic variable name. I hate to get too intimate with those
> > > > > details, though.
> > 
> > One concern with that is what about all other shells that are not BASH?
> > I'm sure they use a different env var for storing functions so we may
> > need to handle other shell's too? That is assuming we want to keep the
> > old behavior.
> 
> Most other shells don't do function-exporting at all. Certainly dash and
> most traditional bourne shells don't. I wouldn't be surprised if zsh
> does. But yeah, we'd have to support them one by one (and possibly
> variants across different versions of each shell). Workable, but gross.
> 
> > > When execvp(foo) falls back on ENOEXEC, it is not running "sh -c foo".
> > > It is actually running "sh foo" to run the script contents. So it's
> > > about letting you do:
> > > 
> > >   echo "no shebang line" >script
> > >   chmod +x script
> > >   ./script
> > > 
> > > And nothing to do with shell builtins.
> > 
> > That's correct, and is the exact behavior I was trying to mimic with the
> > changes to run_command.
> >   1. try executing the command.
> >   2. if it fails with ENOEXEC then attempt to execute it with a shell
> 
> I think the logic here would be more like:

Oh yes, I was just saying what I did not taking into account how we
would solve this issue.

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

-- 
Brandon Williams

  reply	other threads:[~2017-05-16 17:30 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 [this message]
2017-05-16 19:14             ` Linus Torvalds
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

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

  git send-email \
    --in-reply-to=20170516173041.GH79147@google.com \
    --to=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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.