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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 5, 2018, 20:48 UTC
Message-ID: <nycvar.QRO.7.76.6.1801052133380.1337@wbunaarf-fpuvaqryva.tvgsbejvaqbjf.bet>
In-Reply-To: <87r2r4azt5.fsf@evledraar.gmail.com>

Hi Ævar,

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

Show 13 quoted lines

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

Show 47 quoted lines

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

Show 7 quoted lines

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

Show 13 quoted lines

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

Show 5 quoted lines

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