Hi Adam, On Tue, 27 Apr 2021, Adam Dinwoodie wrote: > 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. Ah, that would explain things. Thank you so much for digging! Ciao, Dscho > > > > > 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 >
