Re: [PATCH v4 7/7] wildmatch test: create & test files on disk in addition to in-memory

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 6229 bytes --]

Hi Ævar,

On Fri, 5 Jan 2018, Ævar Arnfjörð Bjarmason wrote:

> On Fri, Jan 05 2018, Johannes Schindelin jotted:
> 
> > [...]
> >
> > In short: the Unix shell script t3070 manages to write what it thinks is a
> > file called 'foo*', but Git only sees 'foo<some-undisplayable-character>'.
> >
> > I tried to address this problem with this patch:
> 
> ...I don't see any particular value in trying to do these full roundtrip
> tests on platforms like Windows. Perhaps we should just do these on a
> whitelist of POSIX systems for now, and leave expanding that list to
> some future step.

I don't think so. Windows is already handled as a second-class citizen, as
if nobody developed on it. As a consequence, only very few of the
gazillions of Windows developers... develop Git. We could worsify the
situation, of course, but why? Shouldn't we at least pretend to try the
opposite?

> > -- snip --
> > diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
> > index f606f91acbb..51dcb675e7b 100755
> > --- a/t/t3070-wildmatch.sh
> > +++ b/t/t3070-wildmatch.sh
> > @@ -4,6 +4,14 @@ test_description='wildmatch tests'
> >
> >  . ./test-lib.sh
> >
> > +if test_have_prereq !MINGW && touch -- 'a*b' 2>/dev/null
> > +then
> > +	test_set_prereq FILENAMESWITHSTARS
> > +else
> > +	say 'Your filesystem does not allow stars in filenames.'
> > +fi
> > +rm -f 'a*b'
> > +
> >  create_test_file() {
> >  	file=$1
> >
> > @@ -28,6 +36,17 @@ create_test_file() {
> >  	*/)
> >  		return 1
> >  		;;
> > +	# On Windows, stars are not allowed in filenames. Git for Windows'
> > +	# Bash, however, is based on Cygwin which plays funny names with a
> > +	# private Unicode page to emulate stars in filenames. Meaning that
> > +	# the shell script will "succeed" to write the file, but Git will
> > +	# not see it. Nor any other, regular Windows process.
> > +	*\**|*\[*)
> > +		if ! test_have_prereq FILENAMESWITHSTARS
> > +		then
> > +			return 1
> > +		fi
> > +		;;
> >  	# On Windows, \ in paths is silently converted to /, which
> >  	# would result in the "touch" below working, but the test
> >  	# itself failing. See 6fd1106aa4 ("t3700: Skip a test with
> > -- snap --
> >
> > This gets us further. But not by much:
> 
> Okey, that's very weird. So you can:
> 
>     touch "./*"; echo $?
> 
> And it'll return 0 but then the file won't exist?

Almost. The file exists, but it won't have the name '*'. It will have as
name a Unicode character that is in a private page, not standardized by
the Unicode specification.

> How about this:
> 
>     touch "./*" && test -e "./*"; echo $?

Would return 0. Why? Because *you are still in a Unix shell script, so the
Cygwin cuteness still applies*.

> The reason this latest version stopped creating files with "\" in them
> unless under BSLASHPSPEC is because Cygwin would silently translate it,
> so it would create the file but it would actually mean something the
> tests didn't expect.

I understand that. And I would wish that the test would be designed in a
more cross-platform-aware mindset.

> For anything else, such as stars not being allowed in filenames I was
> expecting that "touch" command to return an error, but if that's not the
> case maybe we need the "test -e" as well, unless I'm missing something
> here.

This is one of the many bad consequences of Git relying so much on Unix
shell scripting. Despite what many, many, many Git developers think: shell
scripting is not portable.

Cygwin does a good job of pretending that it is, and MSYS2 exacerbates
that notion, but it comes back to haunt you right here and right now. The
`touch` invocation will *report success*, but it will have done something
different than you wanted. It's like the Thinking: Fast and Slow.

> > fatal: Invalid path '\[ab]': No such file or directory
> >
> > You see, any path starting with a backslash *is* an absolute path on
> > Windows. It is relative to the current drive.
> 
> Right, which I was trying to avoid by not actually creating \[ab], but
> "./\[ab]", is that the same filename on Windows?

Whoops. I managed to copy-paste the *wrong* command's error message. Sorry
about that. The correct one:

$ git --glob-pathspecs ls-files -z -- '\[ab]'
fatal: \[ab]: '\[ab]' is outside repository

Note how it is Git reporting that you asked for a path that is outside?
That's because it thinks you are referring to C:\[ab] (if your current
directory is on the C: drive).

And it would be correct to complain on Windows: the `\[ab]` parameter
refers to an absolute path.

> > This affects *quite* a few of the test cases you added.
> >
> > And even if I just comment all of those out, I run into the next problem
> > where you try to create a file whose name consists of a single space,
> > which is also illegal on Windows.
> 
> Okey, but ditto above about touch not catching it, does:
> 
>     touch "./ "; echo $?
> 
> Not return an error? Then how about:
> 
>     touch "./ " && test -e "./ "; echo $?

Again: as long as you stay within the bounds of the Unix shell script (did
I point out enough yet how non-portable this design is? Even Subversion
knew better than to implement parts of its operations as Unix shell
scripts. I mean, for PoCing, okay, but for production code?) you fall prey
to Cygwin's emulation of POSIX-y filenames. As soon as you leave that
bubble (e.g. by calling git.exe), you're not going to see those illegal
file names, but the ones with the unprintable Unicode characters.

> > These woes demonstrate one problem with the approach of overzealously
> > testing *everything*[...]
> 
> I think the rest of this gets into topics I've covered above. I.e. that
> this increased test coverage has caught bugs.

That's all good and dandy, but what about regressions? I know how much I
will curse in your vague direction when I encounter the next
wildmatch-related bug in, say, half a year and have to wade through the
jungle of unintuitive tests in t3070.

Can't we do a lot better than this? Shouldn't it be a lot more obvious
what the heck went wrong when running t3070 with -i -v -x?

Ciao,
Johannes