[Re-adding the previous Cc list that I'd failed to copy on my previous email, sorry!] On Mon, 26 Apr 2021 at 20:56, Adam Dinwoodie wrote: > > On Mon, 26 Apr 2021 at 15:08, Johannes Schindelin wrote: > > > > Hi Adam, > > > > On Sat, 24 Apr 2021, Adam Dinwoodie wrote: > > > Notes: > > > The patch to read-cache.c is the one I've applied downstream as the Cygwin Git > > > maintainer to resolve this vulnerability, and I've manually tested that it > > > resolves the vulnerability, so that's the change I'd recommend anyone who needs > > > to build Git on Cygwin themselves take until there's something officially in > > > the Git source code. > > > > > > I'm much less convinced by my approach for the test script. I definitely think > > > it's worth having a test here, but the test as written still fails, as the test > > > seems to be looking for the error message "directory not empty", but running > > > the test on Cygwin produces the error "cannot create submodule directory d\a". > > > I'm not sure why that difference exists, and whether the correct approach would > > > be to (a) ensure the error messages are consistent across platforms or (b) to > > > change the test to expect the appropriate error on the appropriate platform. > > > > Wasn't there something in Cygwin that _allowed_ backslashes as file name > > characters? I vaguely remember that the ASCII characters forbidden by > > Windows were mapped into some "private page". > > > > Maybe that is responsible for the difference here? > > So there is special handling of a bunch of characters like ":" that > are valid as parts of filenames on most *nix systems, but which aren't > valid on Windows, by substituting them for characters in the Unicode > "private use area" space. Backslash isn't one of those characters, > though; quoting > https://cygwin.com/cygwin-ug-net/using-specialnames.html (which I just > checked myself to be sure): "The backslash has to be exempt from this > conversion, because Cygwin accepts Win32 filenames including > backslashes as path separators on input." > > Which is not to say this special handling _isn't_ the cause of the > difference here, but it's not so simple as that. If nobody spots an > explanation I've missed, I'll start digging into the code and strace > to work out exactly what's causing the difference in behaviour. I've worked out what's going wrong here: the "prevent git~1 squatting on Windows" test is actually testing a selection of different Windows path oddities, which are handled differently between Git for Windows and Cygwin Git. The specific behaviour here is the handling of a directory called "d."; Git for Windows (I assume in the MSYS2 layer) follows the standard Windows convention of treating "d." and "d" as identical filenames, while Cygwin sticks to its general design philosophy of mostly emulating *nix systems, allowing objects with both filenames to exist in the same directory (and causing pain for most non-Cygwin applications that try to interact with them). Essentially this test is checking a bunch of different oddities about path handling on Windows. Some things – such as handling backslashes – are common to both Cygwin and MSYS2; some – such as handling trailing periods – aren't. So I expect the solution here will be to have separate tests for (a) Git for Windows, (b) Cygwin Git, and (c) common behaviour. > > > I'm also not convinced by my approach of adding a "WINDOWS" prerequisite to > > > test-lib.sh. I went with this as I couldn't immediately see a way to pass > > > prerequisites on an "any" rather than "all" basis to test_expect_success, and > > > this would allow us to simplify all the tests that currently have > > > "!MINGW,!CYGWIN" as prerequisites, but it still feels a bit clunky to me. > > > > Right, the only way I could think of it would be > > > > test_lazy_prereq 'test_have_prereq MINGW || test_have_prereq CYGWIN' > > > > Your approach looks fine to me, though. > > Grand, okay. I'll stick with that for now, then, and follow up with a > patch to tidy up the other prerequisites at some point in the future. > > Adam
