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

From: Stefan Beller <sbeller@google.com>
Date: May 3, 2016, 18:02 UTC
Message-ID: <CAGZ79kav27h+_eswTd3hRWjYhH5aApVcERa1bSxKD8p2R50HTw@mail.gmail.com>
In-Reply-To: <CAA6PgK4M2Ag6anEFYakW9NJe0ehTtS1UEQmDWe387W_uV-MCXA@mail.gmail.com>

On Tue, May 3, 2016 at 2:19 AM, Jan Keromnes <janx@linux.com> wrote:

Show 11 quoted lines

> 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

Show 92 quoted lines

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