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

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

[Jan Keromnes]

Hello,

I tried running a full profile build of Git 2.8.1, but it looks like
test #32 in `t7300-clean.sh` fails:

Commands:

> curl https://www.kernel.org/pub/software/scm/git/git-2.8.1.tar.xz | tar xJ
> cd git-2.8.1
> make prefix=/usr profile-install install-man -j18

Logs of test-suite that fails:

*** t7300-clean.sh ***
ok 1 - setup
ok 2 - git clean with skip-worktree .gitignore
ok 3 - git clean
ok 4 - git clean src/
ok 5 - git clean src/ src/
ok 6 - git clean with prefix
ok 7 - git clean with relative prefix
ok 8 - git clean with absolute path
ok 9 - git clean with out of work tree relative path
ok 10 - git clean with out of work tree absolute path
ok 11 - git clean -d with prefix and path
ok 12 - git clean symbolic link
ok 13 - git clean with wildcard
ok 14 - git clean -n
ok 15 - git clean -d
ok 16 - git clean -d src/ examples/
ok 17 - git clean -x
ok 18 - git clean -d -x
ok 19 - git clean -d -x with ignored tracked directory
ok 20 - git clean -X
ok 21 - git clean -d -X
ok 22 - git clean -d -X with ignored tracked directory
ok 23 - clean.requireForce defaults to true
ok 24 - clean.requireForce
ok 25 - clean.requireForce and -n
ok 26 - clean.requireForce and -f
ok 27 - core.excludesfile
ok 28 # skip removal failure (missing SANITY)
ok 29 - nested git work tree
ok 30 - should clean things that almost look like git but are not
ok 31 - should not clean submodules
not ok 32 - should avoid cleaning possible submodules
#
#               rm -fr to_clean possible_sub1 &&
#               mkdir to_clean possible_sub1 &&
#               test_when_finished "rm -rf possible_sub*" &&
#               echo "gitdir: foo" >possible_sub1/.git &&
#               >possible_sub1/hello.world &&
#               chmod 0 possible_sub1/.git &&
#               >to_clean/should_clean.this &&
#               git clean -f -d &&
#               test_path_is_file possible_sub1/.git &&
#               test_path_is_file possible_sub1/hello.world &&
#               test_path_is_missing to_clean
#

Best,
Jan

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

[Johannes Schindelin]

Hi Jan,

On Fri, 29 Apr 2016, Jan Keromnes wrote:

> I tried running a full profile build of Git 2.8.1, but it looks like
> test #32 in `t7300-clean.sh` fails:
> 
> Commands:
> 
> > curl https://www.kernel.org/pub/software/scm/git/git-2.8.1.tar.xz | tar xJ
> > cd git-2.8.1
> > make prefix=/usr profile-install install-man -j18
> 
> Logs of test-suite that fails:
> 
> *** t7300-clean.sh ***
> ok 1 - setup
> ok 2 - git clean with skip-worktree .gitignore
> ok 3 - git clean
> ok 4 - git clean src/
> ok 5 - git clean src/ src/
> ok 6 - git clean with prefix
> ok 7 - git clean with relative prefix
> ok 8 - git clean with absolute path
> ok 9 - git clean with out of work tree relative path
> ok 10 - git clean with out of work tree absolute path
> ok 11 - git clean -d with prefix and path
> ok 12 - git clean symbolic link
> ok 13 - git clean with wildcard
> ok 14 - git clean -n
> ok 15 - git clean -d
> ok 16 - git clean -d src/ examples/
> ok 17 - git clean -x
> ok 18 - git clean -d -x
> ok 19 - git clean -d -x with ignored tracked directory
> ok 20 - git clean -X
> ok 21 - git clean -d -X
> ok 22 - git clean -d -X with ignored tracked directory
> ok 23 - clean.requireForce defaults to true
> ok 24 - clean.requireForce
> ok 25 - clean.requireForce and -n
> ok 26 - clean.requireForce and -f
> ok 27 - core.excludesfile
> ok 28 # skip removal failure (missing SANITY)
> ok 29 - nested git work tree
> ok 30 - should clean things that almost look like git but are not
> ok 31 - should not clean submodules
> not ok 32 - should avoid cleaning possible submodules
> #
> #               rm -fr to_clean possible_sub1 &&
> #               mkdir to_clean possible_sub1 &&
> #               test_when_finished "rm -rf possible_sub*" &&
> #               echo "gitdir: foo" >possible_sub1/.git &&
> #               >possible_sub1/hello.world &&
> #               chmod 0 possible_sub1/.git &&
> #               >to_clean/should_clean.this &&
> #               git clean -f -d &&
> #               test_path_is_file possible_sub1/.git &&
> #               test_path_is_file possible_sub1/hello.world &&
> #               test_path_is_missing to_clean
> #

This log does not really help, in particular because your report does not
include vital information such as host OS, installed libraries, etc.

To debug further on your side (which is really the only logical thing to
do from here), have a look at this documentation:

	https://github.com/git-for-windows/git/wiki/Running-Git's-regression-tests#running-individual-tests

(even if it is on Git for Windows' wiki, you will find that the
suggestions apply to any development environment.)

Ciao,
Johannes

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

[Stefan Beller]

On Fri, Apr 29, 2016 at 5:53 AM, Jan Keromnes <janx@linux.com> wrote:
> Hello,
>
> I tried running a full profile build of Git 2.8.1, but it looks like
> test #32 in `t7300-clean.sh` fails:
>
> Commands:
>
>> curl https://www.kernel.org/pub/software/scm/git/git-2.8.1.tar.xz | tar xJ
>> cd git-2.8.1
>> make prefix=/usr profile-install install-man -j18
>
> Logs of test-suite that fails:
>
> *** t7300-clean.sh ***
> ok 1 - setup
> ok 2 - git clean with skip-worktree .gitignore
> ok 3 - git clean
> ok 4 - git clean src/
> ok 5 - git clean src/ src/
> ok 6 - git clean with prefix
> ok 7 - git clean with relative prefix
> ok 8 - git clean with absolute path
> ok 9 - git clean with out of work tree relative path
> ok 10 - git clean with out of work tree absolute path
> ok 11 - git clean -d with prefix and path
> ok 12 - git clean symbolic link
> ok 13 - git clean with wildcard
> ok 14 - git clean -n
> ok 15 - git clean -d
> ok 16 - git clean -d src/ examples/
> ok 17 - git clean -x
> ok 18 - git clean -d -x
> ok 19 - git clean -d -x with ignored tracked directory
> ok 20 - git clean -X
> ok 21 - git clean -d -X
> ok 22 - git clean -d -X with ignored tracked directory
> ok 23 - clean.requireForce defaults to true
> ok 24 - clean.requireForce
> ok 25 - clean.requireForce and -n
> ok 26 - clean.requireForce and -f
> ok 27 - core.excludesfile
> ok 28 # skip removal failure (missing SANITY)
> ok 29 - nested git work tree
> ok 30 - should clean things that almost look like git but are not
> ok 31 - should not clean submodules
> not ok 32 - should avoid cleaning possible submodules
> #
> #               rm -fr to_clean possible_sub1 &&
> #               mkdir to_clean possible_sub1 &&
> #               test_when_finished "rm -rf possible_sub*" &&
> #               echo "gitdir: foo" >possible_sub1/.git &&
> #               >possible_sub1/hello.world &&
> #               chmod 0 possible_sub1/.git &&
> #               >to_clean/should_clean.this &&
> #               git clean -f -d &&
> #               test_path_is_file possible_sub1/.git &&
> #               test_path_is_file possible_sub1/hello.world &&
> #               test_path_is_missing to_clean
> #
>
> Best,
> Jan

Thanks for reporting the bug!

Have a look at t/README to run the tests with command line arguments.
(I usually run tests as ./tXXXfoo.sh -d -i -v -x with these arguments,
though I cannot remember what each of that does. One of it makes the
test suite stop on a failing test, such that you can cd into the testing
directory and check the state of the file. (Which are present, which are gone?)

With these arguments it is also very verbose, and it would tell
you what is wrong (is the assertion wrong in the `test_path_is_file/missing`
or is it `git clean` segfaulting?)

As Johannes said, it makes sense that you debug into that as
no one could reproduce it thus far on their system.

Thanks,
Stefan

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

[Jan Keromnes]

Thanks for your replies! I was able to reproduce to failure on Git 2.8.2.

Steps:

# Build Git 2.8.2 and run t/t7300-clean.sh in a Dockerfile based on
ubuntu:14.04.
RUN mkdir /tmp/git \
 && cd /tmp/git \
 && curl https://www.kernel.org/pub/software/scm/git/git-2.8.2.tar.xz | tar xJ \
 && cd git-2.8.2 \
 && make all && cd t && ./t7300-clean.sh -d -i -v -x

Logs:

expecting success:
        rm -fr to_clean possible_sub1 &&
        mkdir to_clean possible_sub1 &&
        test_when_finished "rm -rf possible_sub*" &&
        echo "gitdir: foo" >possible_sub1/.git &&
        >possible_sub1/hello.world &&
        chmod 0 possible_sub1/.git &&
        >to_clean/should_clean.this &&
        git clean -f -d &&
        test_path_is_file possible_sub1/.git &&
        test_path_is_file possible_sub1/hello.world &&
        test_path_is_missing to_clean

+ rm -fr to_clean possible_sub1
+ mkdir to_clean possible_sub1
+ test_when_finished rm -rf possible_sub*
+ test 0 = 0
+ test_cleanup={ rm -rf possible_sub*
                } && (exit "$eval_ret"); eval_ret=$?; :
+ echo gitdir: foo
+
+ chmod 0 possible_sub1/.git
+
+ git clean -f -d
Skipping repository baz/boo
Skipping repository foo/
Removing possible_sub1/
Skipping repository repo/
Skipping repository sub2/
Removing to_clean/
+ test_path_is_file possible_sub1/.git
+ test -f possible_sub1/.git
+ echo File possible_sub1/.git doesn't exist.
+ false
error: last command exited with $?=1
File possible_sub1/.git doesn't exist.
not ok 32 - should avoid cleaning possible submodules
#
#               rm -fr to_clean possible_sub1 &&
#               mkdir to_clean possible_sub1 &&
#               test_when_finished "rm -rf possible_sub*" &&
#               echo "gitdir: foo" >possible_sub1/.git &&
#               >possible_sub1/hello.world &&
#               chmod 0 possible_sub1/.git &&
#               >to_clean/should_clean.this &&
#               git clean -f -d &&
#               test_path_is_file possible_sub1/.git &&
#               test_path_is_file possible_sub1/hello.world &&
#               test_path_is_missing to_clean
#

Judging from the line "Removing possible_sub1/", it looks like Git
2.8.2 removes a possible submodule while executing `git clean -f -d`,
whereas the test expects it not to.

Is it possible to make Git's output even more verbose, so that it
tells why it decides to remove "possible_sub1"? Are you able to
reproduce this test fail on your side?

This prevents doing a full profile build.

Best,
Jan

On Fri, Apr 29, 2016 at 7:06 PM, Stefan Beller <sbeller@google.com> wrote:
> On Fri, Apr 29, 2016 at 5:53 AM, Jan Keromnes <janx@linux.com> wrote:
>> Hello,
>>
>> I tried running a full profile build of Git 2.8.1, but it looks like
>> test #32 in `t7300-clean.sh` fails:
>>
>> Commands:
>>
>>> curl https://www.kernel.org/pub/software/scm/git/git-2.8.1.tar.xz | tar xJ
>>> cd git-2.8.1
>>> make prefix=/usr profile-install install-man -j18
>>
>> Logs of test-suite that fails:
>>
>> *** t7300-clean.sh ***
>> ok 1 - setup
>> ok 2 - git clean with skip-worktree .gitignore
>> ok 3 - git clean
>> ok 4 - git clean src/
>> ok 5 - git clean src/ src/
>> ok 6 - git clean with prefix
>> ok 7 - git clean with relative prefix
>> ok 8 - git clean with absolute path
>> ok 9 - git clean with out of work tree relative path
>> ok 10 - git clean with out of work tree absolute path
>> ok 11 - git clean -d with prefix and path
>> ok 12 - git clean symbolic link
>> ok 13 - git clean with wildcard
>> ok 14 - git clean -n
>> ok 15 - git clean -d
>> ok 16 - git clean -d src/ examples/
>> ok 17 - git clean -x
>> ok 18 - git clean -d -x
>> ok 19 - git clean -d -x with ignored tracked directory
>> ok 20 - git clean -X
>> ok 21 - git clean -d -X
>> ok 22 - git clean -d -X with ignored tracked directory
>> ok 23 - clean.requireForce defaults to true
>> ok 24 - clean.requireForce
>> ok 25 - clean.requireForce and -n
>> ok 26 - clean.requireForce and -f
>> ok 27 - core.excludesfile
>> ok 28 # skip removal failure (missing SANITY)
>> ok 29 - nested git work tree
>> ok 30 - should clean things that almost look like git but are not
>> ok 31 - should not clean submodules
>> not ok 32 - should avoid cleaning possible submodules
>> #
>> #               rm -fr to_clean possible_sub1 &&
>> #               mkdir to_clean possible_sub1 &&
>> #               test_when_finished "rm -rf possible_sub*" &&
>> #               echo "gitdir: foo" >possible_sub1/.git &&
>> #               >possible_sub1/hello.world &&
>> #               chmod 0 possible_sub1/.git &&
>> #               >to_clean/should_clean.this &&
>> #               git clean -f -d &&
>> #               test_path_is_file possible_sub1/.git &&
>> #               test_path_is_file possible_sub1/hello.world &&
>> #               test_path_is_missing to_clean
>> #
>>
>> Best,
>> Jan
>
> Thanks for reporting the bug!
>
> Have a look at t/README to run the tests with command line arguments.
> (I usually run tests as ./tXXXfoo.sh -d -i -v -x with these arguments,
> though I cannot remember what each of that does. One of it makes the
> test suite stop on a failing test, such that you can cd into the testing
> directory and check the state of the file. (Which are present, which are gone?)
>
> With these arguments it is also very verbose, and it would tell
> you what is wrong (is the assertion wrong in the `test_path_is_file/missing`
> or is it `git clean` segfaulting?)
>
> As Johannes said, it makes sense that you debug into that as
> no one could reproduce it thus far on their system.
>
> Thanks,
> Stefan

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

[Stefan Beller]

On Tue, May 3, 2016 at 2:19 AM, Jan Keromnes <janx@linux.com> wrote:
> Thanks for your replies! I was able to reproduce to failure on Git 2.8.2.
>
> Steps:
>
> # Build Git 2.8.2 and run t/t7300-clean.sh in a Dockerfile based on
> ubuntu:14.04.
> RUN mkdir /tmp/git \
>  && cd /tmp/git \
>  && curl https://www.kernel.org/pub/software/scm/git/git-2.8.2.tar.xz | tar xJ \
>  && cd git-2.8.2 \
>  && make all && cd t && ./t7300-clean.sh -d -i -v -x

I am on a Ubuntu 14.04.1 LTS derivative (plain, not inside a docker) and running
that list of commands (getting that tarball from kernel.org and testing) doesn't
reproduce the failure for me.

expecting success:
         rm -fr to_clean possible_sub1 &&
         mkdir to_clean possible_sub1 &&
         test_when_finished "rm -rf possible_sub*" &&
         echo "gitdir: foo" >possible_sub1/.git &&
         >possible_sub1/hello.world &&
         chmod 0 possible_sub1/.git &&
         >to_clean/should_clean.this &&
         git clean -f -d &&
         test_path_is_file possible_sub1/.git &&
         test_path_is_file possible_sub1/hello.world &&
         test_path_is_missing to_clean

++ rm -fr to_clean possible_sub1
++ mkdir to_clean possible_sub1
++ test_when_finished 'rm -rf possible_sub*'
++ test 0 = 0
++ test_cleanup='{ rm -rf possible_sub*
} && (exit "$eval_ret"); eval_ret=$?; :'
++ echo 'gitdir: foo'
++ chmod 0 possible_sub1/.git
++ git clean -f -d
Skipping repository baz/boo
Skipping repository foo/
Skipping repository possible_sub1/
Skipping repository repo/
Skipping repository sub2/
Removing to_clean/
++ test_path_is_file possible_sub1/.git
++ test -f possible_sub1/.git
++ test_path_is_file possible_sub1/hello.world
++ test -f possible_sub1/hello.world
++ test_path_is_missing to_clean
++ test -e to_clean
++ rm -rf possible_sub1
++ exit 0
++ eval_ret=0
++ :
ok 32 - should avoid cleaning possible submodules

>
> Judging from the line "Removing possible_sub1/", it looks like Git
> 2.8.2 removes a possible submodule while executing `git clean -f -d`,
> whereas the test expects it not to.

Yeah that is my conclusion as well. (The successful test doesn't remove it)
Reading the clean code at [1], specifically the loop starting at line 958,
there are two cases. Either the entry is a directory (submodules are
treated as special directories) or files.


So the line 975 is executed for that submodule as remove_dirs has the printing
in both cases.

Apparently the remove_dirs exits early for me in the condition

    if ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) &&
is_nonbare_repository_dir(path)) {

and your case doesn't. (I assume it falls through and later produces
the output in line 243.

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)

is_nonbare_repository_dir is only used in `git clean` as well as for the
files backend (which is rather new. I don't know the state of the usage there).

What I find suspicious about is_nonbare_repository_dir (found in setup.c:316),
is the assumption of only READ_GITFILE_ERR_OPEN_FAILED and
READ_GITFILE_ERR_READ_FAILED
leading to a repository.

I have cc'd Jeff and Erik, who have touched the code there, maybe they
have a thought on it.
In the mean time I would be interested if this change helps for you:

        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;

Do you do anything fancy with your file system?

Thanks,
Stefan


[1] https://github.com/git/git/blob/v2.8.2/builtin/clean.c#L854


>
> Is it possible to make Git's output even more verbose, so that it
> tells why it decides to remove "possible_sub1"? Are you able to
> reproduce this test fail on your side?
>
> This prevents doing a full profile build.
>
> Best,
> Jan
>
> On Fri, Apr 29, 2016 at 7:06 PM, Stefan Beller <sbeller@google.com> wrote:
>> On Fri, Apr 29, 2016 at 5:53 AM, Jan Keromnes <janx@linux.com> wrote:
>>> Hello,
>>>
>>> I tried running a full profile build of Git 2.8.1, but it looks like
>>> test #32 in `t7300-clean.sh` fails:
>>>
>>> Commands:
>>>
>>>> curl https://www.kernel.org/pub/software/scm/git/git-2.8.1.tar.xz | tar xJ
>>>> cd git-2.8.1
>>>> make prefix=/usr profile-install install-man -j18
>>>
>>> Logs of test-suite that fails:
>>>
>>> *** t7300-clean.sh ***
>>> ok 1 - setup
>>> ok 2 - git clean with skip-worktree .gitignore
>>> ok 3 - git clean
>>> ok 4 - git clean src/
>>> ok 5 - git clean src/ src/
>>> ok 6 - git clean with prefix
>>> ok 7 - git clean with relative prefix
>>> ok 8 - git clean with absolute path
>>> ok 9 - git clean with out of work tree relative path
>>> ok 10 - git clean with out of work tree absolute path
>>> ok 11 - git clean -d with prefix and path
>>> ok 12 - git clean symbolic link
>>> ok 13 - git clean with wildcard
>>> ok 14 - git clean -n
>>> ok 15 - git clean -d
>>> ok 16 - git clean -d src/ examples/
>>> ok 17 - git clean -x
>>> ok 18 - git clean -d -x
>>> ok 19 - git clean -d -x with ignored tracked directory
>>> ok 20 - git clean -X
>>> ok 21 - git clean -d -X
>>> ok 22 - git clean -d -X with ignored tracked directory
>>> ok 23 - clean.requireForce defaults to true
>>> ok 24 - clean.requireForce
>>> ok 25 - clean.requireForce and -n
>>> ok 26 - clean.requireForce and -f
>>> ok 27 - core.excludesfile
>>> ok 28 # skip removal failure (missing SANITY)
>>> ok 29 - nested git work tree
>>> ok 30 - should clean things that almost look like git but are not
>>> ok 31 - should not clean submodules
>>> not ok 32 - should avoid cleaning possible submodules
>>> #
>>> #               rm -fr to_clean possible_sub1 &&
>>> #               mkdir to_clean possible_sub1 &&
>>> #               test_when_finished "rm -rf possible_sub*" &&
>>> #               echo "gitdir: foo" >possible_sub1/.git &&
>>> #               >possible_sub1/hello.world &&
>>> #               chmod 0 possible_sub1/.git &&
>>> #               >to_clean/should_clean.this &&
>>> #               git clean -f -d &&
>>> #               test_path_is_file possible_sub1/.git &&
>>> #               test_path_is_file possible_sub1/hello.world &&
>>> #               test_path_is_missing to_clean
>>> #
>>>
>>> Best,
>>> Jan
>>
>> Thanks for reporting the bug!
>>
>> Have a look at t/README to run the tests with command line arguments.
>> (I usually run tests as ./tXXXfoo.sh -d -i -v -x with these arguments,
>> though I cannot remember what each of that does. One of it makes the
>> test suite stop on a failing test, such that you can cd into the testing
>> directory and check the state of the file. (Which are present, which are gone?)
>>
>> With these arguments it is also very verbose, and it would tell
>> you what is wrong (is the assertion wrong in the `test_path_is_file/missing`
>> or is it `git clean` segfaulting?)
>>
>> As Johannes said, it makes sense that you debug into that as
>> no one could reproduce it thus far on their system.
>>
>> Thanks,
>> Stefan

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

[Junio C Hamano]

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.

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

[Jeff King]

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

-Peff

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

[Stefan Beller]

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

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

[Jeff King]

On Tue, May 03, 2016 at 11:53:36AM -0700, Stefan Beller wrote:

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

That would certainly be wrong with read_gitfile_gently() as it is today;
it does not distinguish various values of errno for stat(), so that
would get the "there's not even a .git file here at all" case wrong.

So the first step would be to have read_gitfile_gently() start looking
for ENOENT versus other errors. I don't know if that's worth the
trouble; we're pretty cavalier about treating stat failure as "file does
not exist" in the rest of the code.

-Peff

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

[erik elfström]

Thanks for fixing the missing SANITY prerequisite Stefan.

As for the error handling logic in setup.c: is_nonbare_repository_dir
(was clean.c: is_git_repository) my reasoning is as follows:

READ_GITFILE_ERR_STAT_FAILED
READ_GITFILE_ERR_NOT_A_FILE:

When checking random paths for .git files these are the common error
modes, file is not there or it is a directory. This should not be
interpreted as a valid .git file.


READ_GITFILE_ERR_OPEN_FAILED
READ_GITFILE_ERR_READ_FAILED:

Here we found a .git file but could not open and read it to verify
that it is valid. Treating it as valid is the safest option for clean.
For resolve_gitlink_ref I think it maybe leads to the creation of a
redundant ref cache entries but I don't think this is a problem unless
someone has a huge amount of unreadable .git files lying around.


READ_GITFILE_ERR_TOO_LARGE:

File is absurdly large (1MB), very unlikely to be a valid .git file.


READ_GITFILE_ERR_INVALID_FORMAT
READ_GITFILE_ERR_NO_PATH:

File is malformed in some way, either the "gitdir:" prefix is missing
or the path is missing. Could theoretically be a corrupted .git file
but seems unlikely.


READ_GITFILE_ERR_NOT_A_REPO:

This is maybe a bit more suspicious. Here we have found a .git file,
it has the format "gitdir: some/path" but
is_git_directory("some/path") returned false, meaning that "some/path"
does not fulfill:

/*
 * Test if it looks like we're at a git directory.
 * We want to see:
 *
 *  - either an objects/ directory _or_ the proper
 *    GIT_OBJECT_DIRECTORY environment variable
 *  - a refs/ directory
 *  - either a HEAD symlink or a HEAD file that is formatted as
 *    a proper "ref:", or a regular file HEAD that has a properly
 *    formatted sha1 object name.
*/

Technically we don't have a valid .git file here but we have something
that really tries to be. I guess it is debatable what the correct
conservative choice is here and if it is the same for all callers.

/Erik