From: Junio C Hamano <gitster@pobox.com>
To: "Carlos Martín Nieto" <cmn@elego.de>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] run-command: don't try to execute directories
Date: Tue, 02 Oct 2012 10:35:16 -0700 [thread overview]
Message-ID: <7vvces93qj.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: <1349189193-25497-1-git-send-email-cmn@elego.de> ("Carlos Martín Nieto"'s message of "Tue, 2 Oct 2012 16:46:33 +0200")
Carlos Martín Nieto <cmn@elego.de> writes:
> 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)
> @@ -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;
next prev parent reply other threads:[~2012-10-02 17:35 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-10-02 14:46 [PATCH] run-command: don't try to execute directories Carlos Martín Nieto
2012-10-02 17:35 ` Junio C Hamano [this message]
2012-10-02 19:32 ` Carlos Martín Nieto
2012-10-02 19:59 ` Junio C Hamano
2012-10-02 21:26 ` 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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7vvces93qj.fsf@alter.siamese.dyndns.org \
--to=gitster@pobox.com \
--cc=cmn@elego.de \
--cc=git@vger.kernel.org \
/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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).