Re: [PATCH v6] Cygwin: rewrite cmdline parser

[Corinna Vinschen <corinna-cygwin@cygwin.com>]

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