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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, May 3, 2016 at 2:19 AM, Jan Keromnes <janx@xxxxxxxxx> 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@xxxxxxxxxx> wrote:
>> On Fri, Apr 29, 2016 at 5:53 AM, Jan Keromnes <janx@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]