[PATCH STABLE] extdiff: quote only options specified at runtime (issue4463)

[Yuya Nishihara]

On Thu, 04 Dec 2014 20:48:24 +0900, FUJIWARA Katsunori wrote:
> At Wed, 3 Dec 2014 00:05:04 +0900,
> Yuya Nishihara wrote:
> > On Mon, 01 Dec 2014 18:13:15 -0600, Matt Mackall wrote:
> > > On Tue, 2014-12-02 at 02:44 +0900, FUJIWARA Katsunori wrote:
> > > > # HG changeset patch
> > > > # User FUJIWARA Katsunori <foozy at lares.dti.ne.jp>
> > > > # Date 1417454278 -32400
> > > > #      Tue Dec 02 02:17:58 2014 +0900
> > > > # Branch stable
> > > > # Node ID d887d27304009d1cac05a1db4399f69d1778c5f0
> > > > # Parent  edf29f9c15f0f171847f4c7a8184cca4e95c8b31
> > > > extdiff: quote only options specified at runtime (issue4463)
> > > > 
> > > > For arguments and options including white spaces, changeset
> > > > 72a89cf86fcd introduced fully quoting on all command line arguments
> > > > and options for external diff command.
> > > > 
> > > > But this causes unexpected behavior of extdiff with WinMerge, because
> > > > WinMerge can't work correctly, when command line options in Windows
> > > > standard style are quoted: for example, 'WinMerge /r ....' is OK, but
> > > > 'WinMerge "/r" ....' is NG).
> > > 
> > > Presumably this is no good because WinMerge doesn't use the standard
> > > argument parser in Microsoft's C runtime (the thing that gives argc and
> > > argv to main()) and instead uses the raw single-string lpCmdLine passed
> > > to WinMain... and then does its own broken parsing that doesn't handle
> > > quoting on things that aren't filenames. Sigh.
> > 
> > How about making windows.shellquote() not quote known-safe argument?
> > As many GUI applications start from WinMain() (or MFC's equivalent), I think,
> > they can easily invent broken command-line parsers.
> > 
> > subprocess.list2cmdline() might have some hints, which says "using the same
> > rules as the MS C runtime."
> 
> It may work incorrectly, if environment variables are used in
> configuration.
> 
> Also with cmd.exe on Windows, environment variable expansions are
> executed before splitting command line into each arguments. For
> example, when "CONCAT" has "foo bar baz" value:
> 
>   winmerge %CONCAT%   => winmerge foo bar baz   (taking 3 arguments)
>   winmerge "%CONCAT%" => winmerge "foo bar baz" (taking 1 argument)
> 
> If "windows.shellquote()" doesn't quote arguments including %CONCAT%,
> "shlex.split + windows.shellquote" combination doesn't treat %CONCAT%
> as a single argument, even if users quote %CONCAT% explicitly in the
> configuration.
> 
> On the other hand, users may not want to treat %CONCAT% as a single
> argument (shouldn't users do such thing ? maybe, but we shouldn't
> expect so). In this case, quoting %CONCAT% in "windows.shellquote"
> works incorrectly for users.

It's the same for Unix. $CONCAT shouldn't be quoted in that case.

> IMHO, root cause of this problem is that "shlex.split" loses whether
> users really want to quote each of arguments or not.
>
> Keeping the configuration string specified by users as it is (=
> avoiding "shlex.split") isn't the best solution, but seems to be
> better than others in almost all cases.

I misread 72a89cf86fcd.  I thought it really wanted to quote options, but it
seems it just wanted to disable <, >, |, &, or ; that terminates a command.
So I agree that shlex.split()+shellquote() is not appropriate.

Regards,