Re: [PATCH v3] diff-no-index: exit(1) if 'diff --quiet <repo file> <external file>' finds changes

From: Tim Henigan <tim.henigan@gmail.com>
Date: Jun 19, 2012, 13:05 UTC
Message-ID: <CAFouethcrw3vOF7SPwHxjH4ABmF8U1df0MfyzcUGq2yTYxs4ow@mail.gmail.com>
In-Reply-To: <7vr4tc2xhy.fsf@alter.siamese.dyndns.org>

On Mon, Jun 18, 2012 at 4:09 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

> Tim Henigan <tim.henigan@gmail.com> writes:
>
>> When running 'git diff --quiet <file1> <file2>', if file1 or file2
>> is outside the repository, it will exit(0) even if the files differ.
>> It should exit(1) when they differ.
>> -     exit(revs->diffopt.found_changes);
>> +     int result = diff_result_code(&revs->diffopt, 0);
>> +     exit(result);
>>  }
>
> Decl-after-stmt.

Will eliminate intermediate variable in v4. Thanks to both you and Jeff for pointing this out.

>> +             echo "1 1" > a &&
>
> Please drop extra SP between ">" and "a".

Will fix in v4.

Show 17 quoted lines

>> +             git add . &&
>> +             git commit -m 1
>> +     ) &&
>> +     mkdir -p test-outside/no-repo && (
>> +             cd test-outside/no-repo &&
>> +             echo "1 1" >a &&
>> +             echo "1 1" >matching-file &&
>> +             echo "1 1 " >trailing-space &&
>> +             echo "1   1" >extra-space &&
>> +             echo "2" >never-match
>> +     )
>
> The inspiration of using CEILING comes from the existing t7810-grep
> test, and I would have preferred if you used the same non/git for a
> non-git repository for easier greppability ("git grep CEIL t/" to
> notice the use of the technique and then "git grep non/git t/" to
> verify, for example).

Okay. I still need the non/git directory to be outside the test repo's path, so the new layout will be:

    $TRASH_DIRECTORY/
        test-outside/
            repo/
            non/git/

This adds an extra layer to the non git paths, but won't cause any problems.

Show 8 quoted lines

>> +test_expect_success 'git diff, one file outside repo' '
>> +     (
>> +             GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/test-outside" &&
>> +             export GIT_CEILING_DIRECTORIES &&
>
> Do you even need these two lines for this test?  your test runs
> inside test-outside/repo that _is_ a git repository, and that
> repository knows that ../no-repo is not part of it already.

You are correct, CEILING does not need to be set for tests where one file is inside 'test-outside/repo'.

As a side note, I found that these tests fail if a relative path is used for the file in 'non/git'. In other words, this passes:

    test_expect_code 0 git diff --quiet a
"$TRASH_DIRECTORY/test-outside/non/git/matching-file"

but this fails:

    test_expect_code 0 git diff --quiet a ../non/git/matching-file

This surprised me, but I have not investigated any further.