From: Stefan Beller <sbeller@google.com>
Date: Wed, 03 May 2017 18:34:50 GMT
Subject: Re: [PATCH 1/7] t7300: skip untracked dirs containing ignored files
Message-ID: <CAGZ79katJKWOY=FdoU2DO-bwyZSA9KL3kcUgw1uKLyytNCXd-Q@mail.gmail.com>
In-Reply-To: <CAJZjrdWLuXF7c=kt4SM1CcOUPpmmZXmOZPUCuzcO0z2nNsVMLQ@mail.gmail.com>

On Wed, May 3, 2017 at 11:26 AM, Samuel Lijin <sxlijin@gmail.com> wrote:
> On Wed, May 3, 2017 at 1:19 PM, Stefan Beller <sbeller@google.com> wrote:
>> On Tue, May 2, 2017 at 8:29 PM, Samuel Lijin <sxlijin@gmail.com> wrote:
>>> If git sees a directory which contains only untracked and ignored
>>> files, clean -d should not remove that directory.
>>
>> Yes that states a fact; it is not clear why we want to have this test here
>> and now. (Is it testing for a recently fixed regression?)
>
> It was recently discovered that this is not true of clean -d (and I'm
> not sure if it ever was, to be honest). See
> http://public-inbox.org/git/xmqqshkof6jd.fsf@gitster.mtv.corp.google.com/T/#mf541c06250724bb000461d210b4ed157e415a596
>
>> Are you just introducing the test to demonstrate it keeps working later on?
>> Do you plan on changing this behavior in a later patch?)
>
> The idea was to introduce the broken test and then have the rest of
> the patch series fix it;

This is valuable information for the commit message.
Also to keep git-bisect happy we want to have all tests passing on all commits,
which this does not. However to further signal your intent, you can mark it
as test_expect_failure. This is marking a test as "if Git was not buggy, this
is a test that would pass", so it actually marks success when it
fails, for example
when running t7300:

...
ok 31 - should not clean submodules
ok 32 - should avoid cleaning possible submodules
ok 33 - nested (empty) git should be kept
ok 34 - nested bare repositories should be cleaned
not ok 35 - nested (empty) bare repositories should be cleaned even
when in .git # TODO known breakage
not ok 36 - nested (non-empty) bare repositories should be cleaned
even when in .git # TODO known breakage
ok 37 - giving path in nested git work tree will remove it
ok 38 - giving path to nested .git will not remove it
ok 39 - giving path to nested .git/ will remove contents
ok 40 - force removal of nested git work tree
...

Similarly if you were to mark that test as expecting failure, we'd see:

no ok  - 45 git clean -d skips untracked dirs containing ignored files
# TODO known breakage

And then in a later patch, when you actually fix the bug, you would just flip it
to test_expect_success.

Thanks,
Stefan