bug-gnu-utils
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: quotearg.c's shell_quoting_style and MinGW


From: Bruno Haible
Subject: Re: quotearg.c's shell_quoting_style and MinGW
Date: Sun, 06 May 2012 13:49:14 +0200
User-agent: KMail/4.7.4 (Linux/3.1.10-1.9-desktop; KDE/4.7.4; x86_64; ; )

Eli Zaretskii wrote:
>   D:\gnu\diffutils-3.2\src>.\diff3 ../../diffutils-2.8.7/src/system.h 
> system.h "sys tem.$h$"
>   diff: extra operand `tem.$h$''
>   diff: Try `diff --help' for more information.
>   D:\gnu\diffutils-3.2\src/./diff3.exe: subsidiary program `diff' failed 
> (exit status 2)

Thanks. This piece of info is essential for understanding and analyzing
the problem.

diff3 receives the arguments as expected (otherwise it would have
complained about too many arguments).

diff3 invokes diff, in function read_diff(), file src/diff3.c.
Similar code also is in function edit(), file src/sdiff.c,
and in function begin_output(), file src/util.c.
In doing so, it prepares a command line, by using shell_quote() from gnulib,
and passes this command to popen() or system().

And shell_quote() happens to invoke quotearg with shell_quoting_style.

This works fine on Unix, where popen() and system() pass the command to
/bin/sh.

But on native Windows, there are two ways to execute commands:
  a) through the Windows function CreateProcess(); this is what gnulib's
     modules 'execute' and 'spawn-pipe' do.
  b) through the cmd.exe program; this is what popen() and system() do.

Both need different quoting.
  - For a), you find an implementation in gnulib/lib/w32spawn.h.
  - For b), it will be even more complicated, because arguments have to
    be passed through a first CreateProcess() (from diff3 to cmd.exe),
    are then subject to parsing by cmd.exe, and are then passed through
    a second CreateProcess() call (from cmd.exe to diff).

Modifying quotearg.c, like you propose, would have an effect
  - on quotearg, but as Paul said, quotearg's behaviour assumes a POSIX sh,
  - on sh-quote, but here too the comments in sh-quote.h indicate a POSIX
    shell.
In particular, there is another use of shell_quote() in src/diff.c,
function option_list(). It is used here:

$ mkdir foo ; echo Hi > foo/text
$ mkdir gaz ; echo Hello > gaz/text
$ diff -r -x foo\ bar foo gaz
diff -r -x 'foo bar' foo/text gaz/text
1c1
< Hi
---
> Hello

Your proposed patch would lead to a different 'diff' output on native Windows.
Which is obviously undesirable.

I can see two possible actions:

1) Change the three locations in src/diff3.c, src/sdiff.c, src/util.c to
   use the gnulib modules 'execute' and 'spawn-pipe'.

   This will have the following advantages:
     - It will fix the quoting problem at the right place.
     - It will speed up the subprocess invocation, by bypassing cmd.exe.
     - If you apply these changes also to the HAVE_WORKING_FORK code branch,
       it will speed up the subprocess invocation also on Unix, by
       bypassing /bin/sh.

   A command interpreter like /bin/sh or cmd.exe is useful when a command
   has redirections or may be invoking a script. But here you know that
   the command is a simple program invocation without redirections and that
   the program being invoked (diff.exe) is not a script. Therefore it is
   possible to optimize it.

2) If you want to keep the unoptimized subprocess invocation through
   popen() and system(), it would be possible to add two quoting styles
   to quotearg: one for a), one for b). And add gnulib API, similar to
   the 'sh-quote' module, that uses the quoting style b).

Paul, what is your preference? Mine would be 1).

> I only tested the code with MinGW.  But I don't mind using
> 
>  #if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
> 
> instead, if you are okay with assuming it will work with MSVC.

Yes, this is the right #if for native Windows. Nothing in popen() or
system() is specific to mingw (as opposed to MSVC).

Bruno




reply via email to

[Prev in Thread] Current Thread [Next in Thread]