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

Hi Ævar,

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

On Fri, Jan 05 2018, Johannes Schindelin jotted:

quoted

[...]

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?

quoted

-- 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.

quoted

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.

quoted

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.

quoted

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