Re: [PATCH v2 2/4] t3000: move non-submodule repo test to separate file

From: Kyle Meyer <kyle@kyleam.com>
Date: Apr 3, 2019, 22:21 UTC
Message-ID: <87d0m2bui0.fsf@kyleam.com>
In-Reply-To: <xmqqlg0rsent.fsf@gitster-ct.c.googlers.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 12 quoted lines

> Kyle Meyer <kyle@kyleam.com> writes:
>
>> a2d5156c2b (resolve_gitlink_ref: ignore non-repository paths,
>> 2016-01-22) added a test to t3000-ls-files-others.sh to check that
>> 'ls-files -o' does not die() when given a subdirectory that looks like
>> a repository but is actually a subdirectory containing a bogus .git
>> file.
>>
>> Move this test to a separate file in preparation for testing scenarios
>> with non-submodule repositories that are not bogus.
>
> It is unclear to me why this is needed.

It's not needed. My thinking, which I didn't do a good job of spelling out above, is

    We're going to be adding a test that checks how 'ls-files -o'
    handles a few different scenarios involving untracked repositories.
    This new test should go into a separate file rather than
    t3000-ls-files-others.sh because it substantially changes the shared
    directory layout that the t3000 tests work on.  Like the upcoming
    test, the "non-submodule .git" test from t3000 deals with a (bogus)
    untracked repository, so let's split it off into a separate test
    file that will be extended with the other scenarios.

Perhaps that's not a good reason to touch t3000, though. I could drop this patch, as well as the next one, and just add the new test file in the final patch.

Show 18 quoted lines

>> +++ b/t/t3009-ls-files-others-nonsubmodule.sh
>> @@ -0,0 +1,21 @@
>> +#!/bin/sh
>> +
>> +test_description='test git ls-files --others with non-submodule repositories'
>> +
>> +. ./test-lib.sh
>> +
>> +test_expect_success 'setup: expected output' '
>> +	cat >expected <<-EOF
>> +	expected
>> +	output
>> +	EOF
>> +'
>
> I think this is overkill.  Usually we have one expectation for a
> single test, so having the above inside the actual test below makes
> more sense.

OK, I'll move this into the test.