Re: [PATCH] run-command: don't try to execute directories
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 2, 2012, 17:35 UTC
- Message-ID
- <7vvces93qj.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1349189193-25497-1-git-send-email-cmn@elego.de>
Carlos Martín Nieto <cmn@elego.de> writes:
Show 30 quoted lines
> When looking through $PATH to try to find an external command, > locate_in_PATH doesn't check that it's trying to execute a file. Add a > check to make sure we won't try to execute a directory. > > This also stops us from looking further and maybe finding that the > user meant an alias, as in the case where the user has > /home/user/bin/git-foo/git-foo.pl and an alias > > [alias] foo = !/home/user/bin/git-foo/git-foo.pl > > Running 'git foo' will currently will try to execute ~/bin/git-foo and > fail because you can't execute a directory. By making sure we don't do > that, we realise that it's an alias and do the right thing > > Signed-off-by: Carlos Martín Nieto <cmn@elego.de> > > --- > > This comes from a case in #git. Not sure if this is worth it, or the > better solution is just to say no to dirs in $PATH. > > After writing all of that, I thought to check the shell, and indeed > > % git-foo > zsh: permission denied: git-foo > > so if the shell doesn't do it, the benefits probably don't outweigh > having a dozen stat instead of access calls. strace reveals that zsh > does what git currently does. bash uses stat and says 'command not > found'.
Hrm, I do not use zsh but it does not seem to reproduce for me.
$ mkdir -p /var/tmp/xx/git
$ zsh
% PATH=/var/tmp/xx:$PATH
% type git
git is /home/junio/bin/git
% git version
git version 1.8.0.rc0.45.g7ce8dc5
% zsh --version
zsh 4.3.10 (x86_64-unknown-linux-gnu)Show 8 quoted lines
> @@ -101,8 +102,9 @@ static char *locate_in_PATH(const char *file)
> }
> strbuf_addstr(&buf, file);
>
> - if (!access(buf.buf, F_OK))
> + if (!stat(buf.buf, &st) && !S_ISDIR(st.st_mode)) {
> return strbuf_detach(&buf, NULL);
> + }So we used to say "if it exists and accessible, return that". Now we say "if it exists and is not a directory, return that".
I have to wonder what would happen if it exists as a non-directory but we cannot access it. Is that a regression?
> if (!*end) > break;