[PATCH v6] Cygwin: rewrite cmdline parser

[Johannes Schindelin]

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.

Having said this, I hope to make it clear that I care about your work, and
I care about helping to make it as good as we can make it together.

It is a rather big patch, so you will hopefully forgive me for not making
it quite through it (yet). And I fear that my time is unfortunately so
limited that I will have to review it in a piecemeal fashion.

Below are a couple of reactions and suggestions to help you understand
where I am at, for now.

On Thu, 13 May 2021, Mingye Wang wrote:

> This commit rewrites the cmdline parser to achieve the following:
> * 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.

But then I think: Maybe that's just poor phrasing? Maybe this is just an
opt-in behavior? So I get my hopes up.

But then I realize I should just ask, because the commit message does not
manage to answer this question: does this break backwards-compatibility?

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.

> * Clarity. Since globify() is no longer responsible for handling the
>   opening and closing of quotes, the code is much simpler.
> * Sanity. The GLOB_NOCHECK flag is removed, so a failed glob correctly
>   returns the literal value. Without the change, anything path-like
>   would be garbled by globify's escaping.
> * 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.

>
> Some clarifications are made in the documentation for when globs are not
> expanded.

Likewise.

>
> The change fixes two complaints of mine:
> * That cygwin is incompatible with its own escape.[1]
> * That there is no way to echo `C:\"` from win32.[2]
>   [1]: https://cygwin.com/pipermail/cygwin/2020-June/245162.html
>   [2]: https://cygwin.com/pipermail/cygwin/2019-October/242790.html
>
> (It's never the point to spawn cygwin32 from cygwin64. Consistency
> matters: with yourself always, and with the outside world when you are
> supposed to.)
>
> This is the sixth version of the patch, having fixed issues with
> compilation, rebased to the latest version, and tested by replacing
> cygwin1.dll. I provide all my patches to Cygwin,
> including this one and all future ones, under the 2-clause BSD license.
> ---
>  winsup/cygwin/dcrt0.cc   | 299 +--------------------------------
>  winsup/cygwin/winf.cc    | 351 ++++++++++++++++++++++++++++++++++++++-

This looks like it might be an almost literal copy. With that amount of
lines, it is real hard for any reader to figure out what remained the same
(simply copied over) and what was changed. As a consequence, subtle bugs
have an easy time to hide, which makes me uneasy.

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!)

- 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.

Thank you,
Dscho

>  winsup/cygwin/winf.h     |   4 +-
>  winsup/cygwin/winsup.h   |   7 +-
>  winsup/doc/cygwinenv.xml |   8 +-
>  winsup/doc/faq-api.xml   |   2 +-
>  6 files changed, 367 insertions(+), 304 deletions(-)
>
> [... snip patch ...]