On May 14 21:38, Johannes Schindelin wrote: > Hi, > > first of all: thank you for working on this. I have run afoul of the > (de-)quoting differences between MSVCRT and Cygwin on more than one > occasion. > [...] > > * MSVCRT compatibility. Except for the single-quote handling (an > > extension for compatibility with old Cygwin), the parser now > > interprets option boundaries exactly like MSVCR since 2008. This fixes > > the issue where our escaping does not work with our own parsing. > > When I read this, I immediately think: This is probably going to break > backwards-compatibility, OMG this is making my life so much harder than it > already is. > [...] > I ask because as maintainer of Git for Windows (which bundles an MSYS2 > runtime which is based on the Cygwin runtime), it is my responsibility to > take care of backwards-compatibility on behalf of the millions of users > out there.
Did you try it? AFAIU, the patch actually fixes up a Cygwin weirdness, which already results in broken behaviour, rather than breaking backward compat. IIRC we discussed this already a while back, you should find it in the archives. > > * A memory leak in the @file expansion is removed by rewriting it to use > > a stack of buffers. This also simplifies the code since we no longer > > have to move stuff. The "downside" is that tokens can no longer cross > > file boundaries. > > This bullet point sounds as if it cries out loud to be put into a separate > patch, accompanied by the corresponding refactored part of the diff. > I would like to encourage you to disentangle these separate concerns: > > - moving code (`git diff --color-moved` should tell the reader that > nothing was edited) > > - clarifying documentation > > - removing GLOB_NOCHECK > > - introducing an MSVCRT-compatible mode (and make it opt-in!) What? No. There's no point in doing that. We want a single way of handling the CLI when called natively. > - whatever else I missed in the 304 deleted and 367 inserted lines (which > is a tough read, and I have to admit that my attention faded after about > a sixth of the patch) > > In essence, pretend that you are a reviewer who wants to help by ensuring > that this patch (series) does not break anything, and that it does > everything as intended (i.e. no subtle bugs are lurking in there). Now, > how would you like the series to be presented (and I keep referring to it > as a _series_ because that's what it should be, for readability). Ideally > it would be a series of patches that tell an interesting story, in a > manner of speaking. I concur that it would be rather nice to see this patch converted into a series with patches handling one problem at a time. Thanks, Corinna