Re: [PATCH 2/2] test-lib: exhaustively insert non-alnum ASCII into the TRASH_DIRECTORY name

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Apr 10, 2017 at 1:19 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> On Mon, Apr 10, 2017 at 10:02 AM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> On Mon, Apr 10, 2017 at 3:47 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
>>>> Change the test library to insert non-alphanumeric ASCII characters
>>>> into the TRASH_DIRECTORY name, that's the directory the test library
>>>> creates, chdirs to and runs each individual test from.
>>>>
>>>> Unless test_fails_on_unusual_directory_names=1 is declared before
>>>> importing test-lib.sh (and if perl isn't available on the system), the
>>>> trash directory will contain every non-alphanumeric character in
>>>> ASCII, in order.
>>>
>>> At the very least there must be an easier way to disable this, e.g. a
>>> command line option.
>>>
>>> This change is sure effective in smoking out bugs, but it's a major
>>> annoyance during development when it comes to debugging a test.  At
>>> first I could not even cd into the trash directory, because TAB
>>> completing the directory name with all those non-printable characters
>>> didn't work (this may be a bug in the bash-completion package).  And
>>> simply copy-pasting the dirname didn't work either, because 'ls'
>>>
>>>   trash directory.t9902-completion.??????????????????????????????? !"#$%&'()*+,-:;<=>?@[\]^_`{|}~?
>
> Btw, it seems most of the failures in t9902-completion are triggered
> by remote URL parsing.  The trash directory's new name contains '[',
> ']' and even "@[", all of which are treated special by
> connect.c:host_end(), a helper function of parse_connect_url(),
> basically breaking anything trying to e.g.:
>
>   git fetch "$(pwd)/other"

I'm going to work on this patch so that I can report on tests by type
of character that triggers a failure.

> What puzzles me most is that parse_connect_url() recognizes right at
> its beginning that a remote URL like this is not actually an URL, so
> why does it continue parsing it as if it were one?
>
> A few other failures are triggered by the ':' in the trash directory's
> name, breaking the following commonly used pattern:
>
>   export GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY" &&
>   cd subdir &&
>   test-git-pretending-it's-run-outside-of-a-repository

Does GIT_CEILING_DIRECTORIES support escaping somehow? E.g.
"foo\:bar". If so maybe we could use a wrapper to set it, if not
that's a bug in the ceiling dir feature, surely.

> I think ':' should therefore be excluded from the trash directory, too.

I think it's preferable to have some mode to use : in dirnames for
those tests that don't fail already, to protect them against future
regressions. Disabling the use of a tricky character like ":" invites
future bugs & regressions.

>>> After some headscratching, Sunday night may be my excuse, I figured
>>> out that 'cd tr*' works...  only to be greeted with the ugliest-ever
>>> three-line(!) shell prompt.
>>>
>>> Therefore I would say that this should not even be enabled by default
>>> in test-lib.sh, so at least running a test directly from the command
>>> line as ./t1234-foo.sh would considerately give us an easily
>>> accessible trash directory even without any command line options.  We
>>> could enable it for 'make test' by default via GIT_TEST_OPTS in
>>> t/Makefile, though.
>>
>> This definitely needs some tweaking as you and Joachim point out. E.g.
>> some capabilities check in the test suite to check if we can even
>> create these sorts of paths on the local filesystem.
>>
>> A couple of comments on the above though:
>>
>> a) If we have something that's a more strict mode that makes tests
>> fail due to buggy code in various scenarios, we gain the most from
>> having it on by default
>
> I know, and I basically agree...
>
>> and having some optional mode to have devs
>> e.g. disable it for manual inspection of the test directories.
>
> ... but this is just too gross to live as default outside of a CI
> environment.
>
>> Most of the running of the test suite that really matters, i.e. just
>> before the software is delivered to end users, is going to be running
>> in some non-interactive build system preparing a package.
>>
>> b) I think any sort of magic like using it with 'make test', but not
>> when the *.sh is manually run, will just lead to frustrating seemingly
>> heisenbugs from people trying to debug the test suite when things do
>> fail, i.e. you run 'make test' on some obscure platform we haven't
>> fixed path bugs on, 10 fail, you manually inspect them and every one
>> of them succeeds, because some --use-garbage-dirs option wasn't
>> passed.
>
> That's not really an issue.  When a test fails during 'make test' with
> garbage in trash dir names, the dev comes and attempts to cd into the
> trash dir, and will be instantly reminded that non-printable
> characters might play a role in the failure when he can't do so with
> ordinary means.

When a test fails for me I cd to t/ and re-run the test *.sh manually.
I don't go straight to inspecting the existing trash.

If those manual invocations were running in some different mode &
succeeded that would be very confusing.

In any case, I'll try to come up with something more granular, e.g. to
categorize tests by failure type.



>>>> This includes all the control characters, !, [], {} etc. the "."
>>>> character isn't included because it's already in the directory name,
>>>> and nor is "/" for obvious reasons, although that would actually work,
>>>> we'd just create a subdirectory, which would make the tests harder to
>>>> inspect when they fail.i
>>>
>>> 1. Heh.  How an additional subdirectory would make the tests harder to
>>>    inspect is nothing compared to the effect of all the other
>>>    characters.
>>>
>>> 2. s/i$//
>>>
>>>> This change is inspired by the "submodule: prevent backslash expantion
>>>> in submodule names" patch[1]. If we'd had backslashes in the
>>>> TRASH_DIRECTORY all along that bug would have been fixed a long time
>>>> ago. This will flag such issues by marking tests that currently fail
>>>> with "test_fails_on_unusual_directory_names=1", ensure that new tests
>>>> aren't added unless a discussion is had about why the code can't
>>>> handle unusual pathnames, and prevent future regressions.
>>>>
>>>> 1. <20170407172306.172673-1-bmwill@google.com>
>>>> ---
>>>>  t/README      | 12 ++++++++++++
>>>>  t/test-lib.sh |  4 ++++
>>>>  2 files changed, 16 insertions(+)
>>>>
>>>> diff --git a/t/README b/t/README
>>>> index ab386c3681..314dd40221 100644
>>>> --- a/t/README
>>>> +++ b/t/README
>>>> @@ -345,6 +345,18 @@ assignment to variable 'test_description', like this:
>>>>       This test registers the following structure in the cache
>>>>       and tries to run git-ls-files with option --frotz.'
>>>>
>>>> +By default the tests will be run from a directory with a highly
>>>> +unusual filename that includes control characters, a newline, various
>>>> +punctuation etc., this is done to smoke out any bugs related to path
>>>> +handling. If for whatever reason the tests can't deal with such
>>>> +unusual path names, set:
>>>> +
>>>> +    test_fails_on_unusual_directory_names=1
>>>> +
>>>> +Before sourcing 'test-lib.sh' as described below. This option is
>>>> +mainly intended to grandfather in existing broken tests & code, and
>>>> +should usually not be used in new code, instead your tests or code
>>>> +probably need fixing.
>>>>
>>>>  Source 'test-lib.sh'
>>>>  --------------------
>>>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>>>> index 13b5696822..089ff5ac7d 100644
>>>> --- a/t/test-lib.sh
>>>> +++ b/t/test-lib.sh
>>>> @@ -914,6 +914,10 @@ fi
>>>>
>>>>  # Test repository
>>>>  TRASH_DIRECTORY="trash directory.$(basename "$0" .sh)"
>>>> +if test -z "$test_fails_on_unusual_directory_names" -a "$(perl -e 'print 1+1' 2>/dev/null)" = "2"
>>>> +then
>>>> +   TRASH_DIRECTORY="$TRASH_DIRECTORY.$(perl -e 'print join q[], grep { /[^[:alnum:]]/ and !m<[./]> } map chr, 0x01..0x7f')"
>>>> +fi
>>>>  test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
>>>>  case "$TRASH_DIRECTORY" in
>>>>  /*) ;; # absolute path is good
>>>> --
>>>> 2.11.0
>>>
>>>