Re: Git 2.8.1 fails test 32 of t7300-clean.sh, breaks profile build

[Stefan Beller <sbeller@google.com>]

On Tue, May 3, 2016 at 11:48 AM, Jeff King <peff@peff.net> wrote:
> On Tue, May 03, 2016 at 11:05:09AM -0700, Junio C Hamano wrote:
>
>> On Tue, May 3, 2016 at 11:02 AM, Stefan Beller <sbeller@google.com> wrote:
>> >
>> > So I wonder if is_nonbare_repository_dir() is the culprit here.
>> > (We do a chmod 0 on the .git before the `git clean` in the test to confuse Git)
>>
>> Ask if the test is run as root; if so, then mark the test to require
>> SANITY prerequisite.
>
> Yeah, I can easily reproduce the failure with `sudo ./t7300-clean.sh`.
> So the immediate fix is the SANITY prereq.
>
> Looking at Stefan's message, I wondered if the patch he came up with:
>
>         diff --git a/setup.c b/setup.c
>         index 3439ec6..4cfba8f 100644
>         --- a/setup.c
>         +++ b/setup.c
>         @@ -323,8 +323,7 @@ int is_nonbare_repository_dir(struct strbuf *path)
>                 strbuf_addstr(path, ".git");
>                 if (read_gitfile_gently(path->buf, &gitfile_error) ||
> is_git_directory(path->buf))
>                         ret = 1;
>         -       if (gitfile_error == READ_GITFILE_ERR_OPEN_FAILED ||
>         -           gitfile_error == READ_GITFILE_ERR_READ_FAILED)
>         +       if (gitfile_error)
>                         ret = 1;
>                 strbuf_setlen(path, orig_path_len);
>                 return ret;
>
> is related or worth doing on top. But I don't think so. That code is
> just trying to convert some error-cases into "let's err on the side of
> assuming it is a repo". Doing that for all values of gitfile_error is
> definitely the wrong thing (it would treat a totally non-existent
> ".git" file as "yes, it's there", which is clearly bogus).

The proposed change is overly eager indeed.
What if we get back a READ_GITFILE_ERR_STAT_FAILED ?
I would think that is a reasonable indicator of a submodule being there?
(The stat failure may be transient ENOMEM Out of memory (i.e., kernel memory).)

By being overly eager I wanted to make sure the assumptions I had were
wrong.

I'm about to send the SANITY prerequisite patch,

Thanks,
Stefan

>
> -Peff