Re: [PATCH] run-command: treat inaccessible directories as ENOENT

From: Frans Klaver <fransklaver@gmail.com>
Date: Mar 30, 2012, 08:23 UTC
Message-ID: <CAH6sp9N=JsWp7iQ=AAdXHe0J+aB5L9cBq2_0BJgUO=Y-vgAbNg@mail.gmail.com>
In-Reply-To: <20120330075217.GA8384@sigill.intra.peff.net>

On Fri, Mar 30, 2012 at 9:52 AM, Jeff King <peff@peff.net> wrote:

Show 7 quoted lines

>  1. I split the find-in-path function so that Frans can build on it
>     with further checks if he wants. However, note that it will need a
>     little more refactoring; it finds the first entry in the PATH,
>     whereas I think he would want the first executable entry
>     (admittedly, having a non-executable entry followed by an
>     executable one which _also_ fails for a different reason is a
>     pretty wild corner case).

Thanks. I think this corner case is something to fix when someone is running into it. Bash doesn't cover this case either (stops looking at the first found entry) so I would go as far as saying that it doesn't happen.

Show 7 quoted lines

>  2. I pulled the test from what Junio posted earlier. I started to
>     write a full test script that checked each of the cases I
>     mentioned earlier. However, in all but the alias case (which is
>     what is tested here), the behavior is really only distinguishable
>     by the error messages, and I didn't want to get into testing what
>     strerror(EACCES) prints. I can re-roll if we really want to go
>     there.

I wouldn't think there is much point in testing strerror output.

Show 16 quoted lines

> +test_expect_success POSIXPERM 'unreadable directory in PATH' '
> +       mkdir local-command &&
> +       test_when_finished "chmod u+rwx local-command && rm -fr local-command" &&
> +       git config alias.nitfol "!echo frotz" &&
> +       chmod a-rx local-command &&
> +       (
> +               PATH=./local-command:$PATH &&
> +               git nitfol >actual
> +       ) &&
> +       echo frotz >expect &&
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 1.7.9.5.7.g11b89

Hadn't looked into Junio's test earlier (probably missed it entirely), but isn't it rather more sensible from a unit-test perspective to see if start_command returns 127 instead of 128 in this specific case? Aside from that, this test doesn't seem to fit in t0061, looking at the t???? guidelines.

Rest looks sensible to me.