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
