bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows/MS-DOS

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Óscar Fuentes]

On Windows, when find-program points to a GNU-compatible `find'
executable, grep-find-use-xargs is assigned the symbol 'gnu. The
consequence of this is that `rgrep' ends building a command that
contains a pipe: find <args> | xargs -0 grep <args> This ends with
`find' reporting confusing errors about wrong arguments.

An extra test is added to the assigment of grep-find-use-xargs to force
the value 'exec on Windows and MS-DOS.

2010-07-20  Óscar Fuentes  <ofv@wanadoo.es>

	* progmodes/grep.el: (grep-compute-defaults): always assign
        'exec to grep-find-use-xargs on Windows and MS-DOS.

=== modified file 'lisp/progmodes/grep.el'
*** lisp/progmodes/grep.el	2010-05-21 20:43:04 +0000
--- lisp/progmodes/grep.el	2010-07-20 03:42:21 +0000
*************** Set up `compilation-exit-message-functio
*** 552,557 ****
--- 552,560 ----
  	(unless grep-find-use-xargs
  	  (setq grep-find-use-xargs
  		(cond
+ 		 ;; We don't want a shell pipe on those systems:
+ 		 ((or (eq system-type 'windows-nt) (eq system-type 'ms-dos))
+ 		  'exec)
  		 ((and
  		   (grep-probe find-program `(nil nil nil ,null-device "-print0"))
  		   (grep-probe xargs-program `(nil nil nil "-0" "-e" "echo")))

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Juanma Barranquero]

2010/7/20 Óscar Fuentes <ofv@wanadoo.es>:

> On Windows, when find-program points to a GNU-compatible `find'
> executable, grep-find-use-xargs is assigned the symbol 'gnu. The
> consequence of this is that `rgrep' ends building a command that
> contains a pipe: find <args> | xargs -0 grep <args> This ends with
> `find' reporting confusing errors about wrong arguments.

Yes, I get

c:/gnu/bin/find.exe: invalid predicate `-nam'

but the very same command works from the command line. So the right
thing would be to fix how rgrep invokes the command on Windows,
wouldn't it?

    Juanma

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Óscar Fuentes]

Juanma Barranquero <lekktu@gmail.com> writes:

> Yes, I get
>
> c:/gnu/bin/find.exe: invalid predicate `-nam'

This is, most likely, a command-line lenght issue. Do C-u M-x rgrep and
enter the same parameters. When Emacs displays the command to be
executed on the minibuffer, insert an extra space between some of the
first parameters. Then the problem becomes

c:/gnu/bin/find.exe: invalid predicate `-na'

> but the very same command works from the command line. So the right
> thing would be to fix how rgrep invokes the command on Windows,
> wouldn't it?

Agreed. That's a symptom of a more general problem. I have no idea of
what's going on, because if the pipe is not used the command line for
`find' is similar on length and the problem doesn't happen.

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Óscar Fuentes]

Óscar Fuentes <ofv@wanadoo.es> writes:

> Agreed. That's a symptom of a more general problem. I have no idea of
> what's going on, because if the pipe is not used the command line for
> `find' is similar on length and the problem doesn't happen.

Pasting the command on cmd.exe works fine (after replacing / with \ on
the path to `find.exe')

Executing

cmd.exe -c <command>

from cmd.exe fails on a similar way:

"CS" -o -path "*" no se reconoce como un comando interno o externo,
programa o archivo por lotes ejecutable.

This looks like a can of worms. Can we apply the patch on the original
post, so `rgrep' is usable on Windows with the minimal fuss?

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Juanma Barranquero]

On Tue, Jul 20, 2010 at 16:24, Óscar Fuentes <ofv@wanadoo.es> wrote:

> This looks like a can of worms. Can we apply the patch on the original
> post, so `rgrep' is usable on Windows with the minimal fuss?

Let's wait for other people's comments, but if you commit that fix, at
the very least I'd suggest adding a big comment stating clearly that
this is a workaround and the underlying bug is neither well understood
nor fixed.

    Juanma

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Eli Zaretskii]

> From: Óscar Fuentes <ofv@wanadoo.es>
> Date: Tue, 20 Jul 2010 16:24:59 +0200
> Cc: 6674@debbugs.gnu.org
> 
> Óscar Fuentes <ofv@wanadoo.es> writes:
> 
> > Agreed. That's a symptom of a more general problem. I have no idea of
> > what's going on, because if the pipe is not used the command line for
> > `find' is similar on length and the problem doesn't happen.
> 
> Pasting the command on cmd.exe works fine (after replacing / with \ on
> the path to `find.exe')
> 
> Executing
> 
> cmd.exe -c <command>
> 
> from cmd.exe fails on a similar way:
> 
> "CS" -o -path "*" no se reconoce como un comando interno o externo,
> programa o archivo por lotes ejecutable.

Can someone find out what is the command length limit beyond which the
command starts to fail?

My guess would be that the problem is in cmdproxy, btw.

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Eli Zaretskii]

> From: Óscar Fuentes <ofv@wanadoo.es>
> Date: Tue, 20 Jul 2010 16:24:59 +0200
> Cc: 6674@debbugs.gnu.org
> 
> Óscar Fuentes <ofv@wanadoo.es> writes:
> 
> > Agreed. That's a symptom of a more general problem. I have no idea of
> > what's going on, because if the pipe is not used the command line for
> > `find' is similar on length and the problem doesn't happen.
> 
> Pasting the command on cmd.exe works fine (after replacing / with \ on
> the path to `find.exe')
> 
> Executing
> 
> cmd.exe -c <command>
> 
> from cmd.exe fails on a similar way:
> 
> "CS" -o -path "*" no se reconoce como un comando interno o externo,
> programa o archivo por lotes ejecutable.
> 
> This looks like a can of worms. Can we apply the patch on the original
> post, so `rgrep' is usable on Windows with the minimal fuss?

I see the problem.  When the command line includes pipes or
redirection, cmdproxy calls cmd.exe to do the job.  It does so by
invoking the `spawn' function.

By trial and error I established that the limit beyond which rgrep
fails is 1024 characters.  This is consistent with the MSDN
documentation of _spawn, e.g.

  http://msdn.microsoft.com/en-us/library/20y988d2(VS.71).aspx

which says:

  To pass arguments to the new process, give one or more pointers to
  character strings as arguments in the _spawn call. These character
  strings form the argument list for the spawned process. The combined
  length of the strings forming the argument list for the new process
  must not exceed 1024 bytes. [...]

Strangely, we use the same _spawn to run commands when there's no
pipes or redirection.  Perhaps the 1024-character limit is only
imposed when cmd.exe is invoked.

In general, cmd.exe itself is documented to have a much larger limit
on command-line length: 8K on Windows XP.

So we could either (1) install Óscar's patch, with the disadvantage
that rgrep will be slower on MS-Windows (because a separate Grep is
invoked on each file found by Find), or (2) fix cmdproxy to use a
better API, such as CreateProcess, which doesn't have this limitation.

In any case, please do NOT install the patch for the MS-DOS build.  As
I wrote earlier, it does not have this problem.

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Juanma Barranquero]

On Tue, Jul 20, 2010 at 19:28, Eli Zaretskii <eliz@gnu.org> wrote:

> or (2) fix cmdproxy to use a
> better API, such as CreateProcess, which doesn't have this limitation.

I think we should do this, unless there's a pressing need to commit a fix ASAP.

    Juanma

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Óscar Fuentes]

Eli Zaretskii <eliz@gnu.org> writes:

[snip]

> By trial and error I established that the limit beyond which rgrep
> fails is 1024 characters.  This is consistent with the MSDN
> documentation of _spawn, e.g.

[snip]

> So we could either (1) install Óscar's patch, with the disadvantage
> that rgrep will be slower on MS-Windows (because a separate Grep is
> invoked on each file found by Find), or (2) fix cmdproxy to use a
> better API, such as CreateProcess, which doesn't have this limitation.

cmdproxy.c does not call the C runtime `_spawn' function. It defines its
own `spawn' that calls CreateProcess.

The issue is that "cmd.exe -c" seems to have a 1024 char limit when the
command contains a pipe (my guess is that the limit applies separatelly
to every command on the pipe chain) and cmdproxy.c prepends "cmd.exe -c"
to the command when it has a pipe, redirection, etc.

> In any case, please do NOT install the patch for the MS-DOS build.  As
> I wrote earlier, it does not have this problem.

Okay.

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Eli Zaretskii]

> From: Óscar Fuentes <ofv@wanadoo.es>
> Cc: lekktu@gmail.com,  6674@debbugs.gnu.org
> Date: Tue, 20 Jul 2010 21:51:39 +0200
> 
> The issue is that "cmd.exe -c" seems to have a 1024 char limit when the
> command contains a pipe (my guess is that the limit applies separatelly
> to every command on the pipe chain) and cmdproxy.c prepends "cmd.exe -c"
> to the command when it has a pipe, redirection, etc.

If that is true, we could implement the pipes in cmdproxy.  It's not
difficult.

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Eli Zaretskii]

> Date: Wed, 21 Jul 2010 00:41:23 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: lekktu@gmail.com, 6674@debbugs.gnu.org
> 
> > From: Óscar Fuentes <ofv@wanadoo.es>
> > Cc: lekktu@gmail.com,  6674@debbugs.gnu.org
> > Date: Tue, 20 Jul 2010 21:51:39 +0200
> > 
> > The issue is that "cmd.exe -c" seems to have a 1024 char limit when the
> > command contains a pipe (my guess is that the limit applies separatelly
> > to every command on the pipe chain) and cmdproxy.c prepends "cmd.exe -c"
> > to the command when it has a pipe, redirection, etc.
> 
> If that is true, we could implement the pipes in cmdproxy.  It's not
> difficult.

A better (and easier implemented) idea would be to create a temporary
batch file with the command line and run the batch file with cmd.exe.
The original command works just fine if invoked from a batch file.

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Christoph]

On 7/20/2010 4:18 PM, Eli Zaretskii wrote:

> A better (and easier implemented) idea would be to create a temporary
> batch file with the command line and run the batch file with cmd.exe.
> The original command works just fine if invoked from a batch file.

Out of curiosity, why is a batch file solution better than enhancing 
cmdproxy?

Christoph

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Óscar Fuentes]

Christoph <cschol2112@googlemail.com> writes:

> On 7/20/2010 4:18 PM, Eli Zaretskii wrote:
>
>> A better (and easier implemented) idea would be to create a temporary
>> batch file with the command line and run the batch file with cmd.exe.
>> The original command works just fine if invoked from a batch file.
>
> Out of curiosity, why is a batch file solution better than enhancing
> cmdproxy?

Because it is 2 orders of magnitude more simple?

Enhancing cmdproxy for correctly parsing | (and `&&', if you are on the
mood of doing the full job) then creating the pipes, the processes, deal
with error conditions and support Win9X is everything but simple.

Two or three more additions of this class, and Microsoft will replace
cmd.exe with cmdproxy.exe ;-)

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Laimonas Vėbra]

Óscar Fuentes wrote:

> The issue is that "cmd.exe -c" seems to have a 1024 char limit when the
> command contains a pipe (my guess is that the limit applies separatelly
> to every command on the pipe chain) and cmdproxy.c prepends "cmd.exe -c"
> to the command when it has a pipe, redirection, etc.

Isn't it wsprintf (used in cmdproxy.c to quote args and for other purposes):
http://msdn.microsoft.com/en-us/library/ms647550(VS.85).aspx

that imposes 1024 buffer/string length limit...?

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Óscar Fuentes]

Laimonas Vėbra <laimonas.vebra@gmail.com> writes:

> Óscar Fuentes wrote:
>
>> The issue is that "cmd.exe -c" seems to have a 1024 char limit when the
>> command contains a pipe (my guess is that the limit applies separatelly
>> to every command on the pipe chain) and cmdproxy.c prepends "cmd.exe -c"
>> to the command when it has a pipe, redirection, etc.
>
> Isn't it wsprintf (used in cmdproxy.c to quote args and for other purposes):
> http://msdn.microsoft.com/en-us/library/ms647550(VS.85).aspx
>
> that imposes 1024 buffer/string length limit...?

Yes!

Patch in progress...

bug#6674: [PATCH] bug#6674: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Óscar Fuentes]

Laimonas Vėbra <laimonas.vebra@gmail.com> writes:

> Isn't it wsprintf (used in cmdproxy.c to quote args and for other purposes):
> http://msdn.microsoft.com/en-us/library/ms647550(VS.85).aspx
>
> that imposes 1024 buffer/string length limit...?

2010-08-02  Óscar Fuentes  <ofv@wanadoo.es>

	* cmdproxy.c (main): use _snprintf instead of wsprintf. Fixes
	bug#6647. wsprintf has a 1024 char limit on Windows.


--- a/nt/cmdproxy.c
+++ b/nt/cmdproxy.c
@@ -35,6 +35,9 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 #include <stdlib.h>  /* getenv */
 #include <string.h>  /* strlen */
 
+/* We don't want to include stdio.h because we are alreayd duplicating
+   lots of it here */
+int _snprintf(char *buffer, size_t count, const char *format, ...);
 
 /*******  Mock C library routines  *********************************/
 
@@ -604,6 +607,7 @@ main (int argc, char ** argv)
     {
       char * p;
       int    extra_arg_space = 0;
+      int    maxlen, remlen;
       int    run_command_dot_com;
 
       progname = getenv ("COMSPEC");
@@ -635,21 +639,32 @@ main (int argc, char ** argv)
 	     case path contains spaces (fortunately it can't contain
 	     quotes, since they are illegal in path names).  */
 
-	  buf = p = alloca (strlen (progname) + extra_arg_space +
-			    strlen (cmdline) + 16);
+          remlen = maxlen =
+            strlen (progname) + extra_arg_space + strlen (cmdline) + 16;
+	  buf = p = alloca (maxlen + 1);
 
 	  /* Quote progname in case it contains spaces.  */
-	  p += wsprintf (p, "\"%s\"", progname);
+	  p += _snprintf (p, remlen, "\"%s\"", progname);
+          remlen = maxlen - (p - buf);
 
 	  /* Include pass_through_args verbatim; these are just switches
              so should not need quoting.  */
 	  for (argv = pass_through_args; *argv != NULL; ++argv)
-	    p += wsprintf (p, " %s", *argv);
+            {
+              p += _snprintf (p, remlen, " %s", *argv);
+              remlen = maxlen - (p - buf);
+            }
 
 	  if (run_command_dot_com)
-	    wsprintf(p, " /e:%d /c %s", envsize, cmdline);
+            {
+              _snprintf(p, remlen, " /e:%d /c %s", envsize, cmdline);
+              remlen = maxlen - (p - buf);
+            }
 	  else
-	    wsprintf(p, " /c %s", cmdline);
+            {
+              _snprintf(p, remlen, " /c %s", cmdline);
+              remlen = maxlen - (p - buf);
+            }
 	  cmdline = buf;
 	}
       else
@@ -669,19 +684,27 @@ main (int argc, char ** argv)
 	  else
 	    path[0] = '\0';
 
-	  cmdline = p = alloca (strlen (progname) + extra_arg_space +
-				strlen (path) + 13);
+          remlen = maxlen =
+            strlen (progname) + extra_arg_space + strlen (path) + 13;
+	  cmdline = p = alloca (maxlen + 1);
 
 	  /* Quote progname in case it contains spaces.  */
-	  p += wsprintf (p, "\"%s\" %s", progname, path);
+	  p += _snprintf (p, remlen, "\"%s\" %s", progname, path);
+          remlen = maxlen - (p - cmdline);
 
 	  /* Include pass_through_args verbatim; these are just switches
              so should not need quoting.  */
 	  for (argv = pass_through_args; *argv != NULL; ++argv)
-	    p += wsprintf (p, " %s", *argv);
+            {
+              p += _snprintf (p, remlen, " %s", *argv);
+              remlen = maxlen - (p - cmdline);
+            }
 
 	  if (run_command_dot_com)
-	    wsprintf (p, " /e:%d", envsize);
+            {
+              _snprintf (p, remlen, " /e:%d", envsize);
+              remlen = maxlen - (p - cmdline);
+            }
 	}
     }
 

bug#6674: [PATCH] bug#6674: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Andreas Schwab]

Óscar Fuentes <ofv@wanadoo.es> writes:

> +/* We don't want to include stdio.h because we are alreayd duplicating

s/yd/dy/

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#6674: [PATCH fixed] bug#6674: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Óscar Fuentes]

Fixed the typo pointed out by Andreas.

2010-08-02  Óscar Fuentes  <ofv@wanadoo.es>

	* cmdproxy.c (main): use _snprintf instead of wsprintf. Fixes
	bug#6647. wsprintf has a 1024 char limit on Windows.


--- a/nt/cmdproxy.c
+++ b/nt/cmdproxy.c
@@ -35,6 +35,9 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 #include <stdlib.h>  /* getenv */
 #include <string.h>  /* strlen */
 
+/* We don't want to include stdio.h because we are already duplicating
+   lots of it here */
+int _snprintf(char *buffer, size_t count, const char *format, ...);
 
 /*******  Mock C library routines  *********************************/
 
@@ -604,6 +607,7 @@ main (int argc, char ** argv)
     {
       char * p;
       int    extra_arg_space = 0;
+      int    maxlen, remlen;
       int    run_command_dot_com;
 
       progname = getenv ("COMSPEC");
@@ -635,21 +639,32 @@ main (int argc, char ** argv)
 	     case path contains spaces (fortunately it can't contain
 	     quotes, since they are illegal in path names).  */
 
-	  buf = p = alloca (strlen (progname) + extra_arg_space +
-			    strlen (cmdline) + 16);
+          remlen = maxlen =
+            strlen (progname) + extra_arg_space + strlen (cmdline) + 16;
+	  buf = p = alloca (maxlen + 1);
 
 	  /* Quote progname in case it contains spaces.  */
-	  p += wsprintf (p, "\"%s\"", progname);
+	  p += _snprintf (p, remlen, "\"%s\"", progname);
+          remlen = maxlen - (p - buf);
 
 	  /* Include pass_through_args verbatim; these are just switches
              so should not need quoting.  */
 	  for (argv = pass_through_args; *argv != NULL; ++argv)
-	    p += wsprintf (p, " %s", *argv);
+            {
+              p += _snprintf (p, remlen, " %s", *argv);
+              remlen = maxlen - (p - buf);
+            }
 
 	  if (run_command_dot_com)
-	    wsprintf(p, " /e:%d /c %s", envsize, cmdline);
+            {
+              _snprintf(p, remlen, " /e:%d /c %s", envsize, cmdline);
+              remlen = maxlen - (p - buf);
+            }
 	  else
-	    wsprintf(p, " /c %s", cmdline);
+            {
+              _snprintf(p, remlen, " /c %s", cmdline);
+              remlen = maxlen - (p - buf);
+            }
 	  cmdline = buf;
 	}
       else
@@ -669,19 +684,27 @@ main (int argc, char ** argv)
 	  else
 	    path[0] = '\0';
 
-	  cmdline = p = alloca (strlen (progname) + extra_arg_space +
-				strlen (path) + 13);
+          remlen = maxlen =
+            strlen (progname) + extra_arg_space + strlen (path) + 13;
+	  cmdline = p = alloca (maxlen + 1);
 
 	  /* Quote progname in case it contains spaces.  */
-	  p += wsprintf (p, "\"%s\" %s", progname, path);
+	  p += _snprintf (p, remlen, "\"%s\" %s", progname, path);
+          remlen = maxlen - (p - cmdline);
 
 	  /* Include pass_through_args verbatim; these are just switches
              so should not need quoting.  */
 	  for (argv = pass_through_args; *argv != NULL; ++argv)
-	    p += wsprintf (p, " %s", *argv);
+            {
+              p += _snprintf (p, remlen, " %s", *argv);
+              remlen = maxlen - (p - cmdline);
+            }
 
 	  if (run_command_dot_com)
-	    wsprintf (p, " /e:%d", envsize);
+            {
+              _snprintf (p, remlen, " /e:%d", envsize);
+              remlen = maxlen - (p - cmdline);
+            }
 	}
     }
 

bug#6674: [PATCH fixed] bug#6674: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Eli Zaretskii]

> From: Óscar Fuentes <ofv@wanadoo.es>
> Date: Mon, 02 Aug 2010 19:48:29 +0200
> Cc: 
> 
> Fixed the typo pointed out by Andreas.
> 
> 2010-08-02  Óscar Fuentes  <ofv@wanadoo.es>
> 
> 	* cmdproxy.c (main): use _snprintf instead of wsprintf. Fixes
> 	bug#6647. wsprintf has a 1024 char limit on Windows.

Thanks.

wsprintf supports wide character (UTF-16) strings, whereas _snprintf
does not.  At the very least, please leave a comment there about that.
Better yet, why not use memcpy etc. instead of _snprintf; you don't
really need any formatting features anyway.

bug#6674: [PATCH fixed] bug#6674: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Juanma Barranquero]

On Mon, Aug 2, 2010 at 21:07, Eli Zaretskii <eliz@gnu.org> wrote:

> wsprintf supports wide character (UTF-16) strings, whereas _snprintf
> does not.  At the very least, please leave a comment there about that.
> Better yet, why not use memcpy etc. instead of _snprintf; you don't
> really need any formatting features anyway.

I've already committed the change on emacs-23, because it fixes the
original report. I'll install any followup patch

    Juanma

bug#6674: [PATCH fixed] bug#6674: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Óscar Fuentes]

Eli Zaretskii <eliz@gnu.org> writes:

>> Fixed the typo pointed out by Andreas.
>> 
>> 2010-08-02  Óscar Fuentes  <ofv@wanadoo.es>
>> 
>> 	* cmdproxy.c (main): use _snprintf instead of wsprintf. Fixes
>> 	bug#6647. wsprintf has a 1024 char limit on Windows.
>
> Thanks.
>
> wsprintf supports wide character (UTF-16) strings, whereas _snprintf
> does not.

wsprintf supports UTF-16 iff the application is compiled with Unicode
support on. cmdproxy uses char, not wchar_t, so if the build switches on
Unicode the compilation will fail.

> At the very least, please leave a comment there about that.

It should be obvious from the type used (char)

> Better yet, why not use memcpy etc. instead of _snprintf; you don't
> really need any formatting features anyway.

It uses

"\"%s\""
" %s"
" /e:%d /c %s"

and more.

bug#6674: [PATCH fixed] bug#6674: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Juanma Barranquero]

2010/8/2 Óscar Fuentes <ofv@wanadoo.es>:

> wsprintf supports UTF-16 iff the application is compiled with Unicode
> support on. cmdproxy uses char, not wchar_t, so if the build switches on
> Unicode the compilation will fail.

If there's nothing more to do, please close bug#6674.

    Juanma

bug#6674: [PATCH fixed] bug#6674: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Óscar Fuentes]

Óscar Fuentes <ofv@wanadoo.es> writes:

>>> 	* cmdproxy.c (main): use _snprintf instead of wsprintf. Fixes
>>> 	bug#6647. wsprintf has a 1024 char limit on Windows.
>>
>> Thanks.
>>
>> wsprintf supports wide character (UTF-16) strings, whereas _snprintf
>> does not.
>
> wsprintf supports UTF-16 iff the application is compiled with Unicode
> support on. cmdproxy uses char, not wchar_t, so if the build switches on
> Unicode the compilation will fail.

This is poorly explained. To begin, cmdproxy, as it was before the
patch, does not support UTF-16. For fixing that, you need to start
by implementing

int wmain(int argc, wchar_t *argv[])

and revise all string handling from there. The workload the patch adds
for migrating cmdproxy to UTF-16 would consist on
s/_snprintf/_snwprintf. Not doing that would cause compilation errors.

[snip]

bug#6674: [PATCH fixed] bug#6674: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Eli Zaretskii]

> From: Óscar Fuentes <ofv@wanadoo.es>
> Date: Mon, 02 Aug 2010 21:57:16 +0200
> Cc: 
> 
> > Better yet, why not use memcpy etc. instead of _snprintf; you don't
> > really need any formatting features anyway.
> 
> It uses
> 
> "\"%s\""
> " %s"
> " /e:%d /c %s"
> 
> and more.

Of which only %d really needs printf-like facility.  The rest just
copy strings to a buffer.

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows/MS-DOS

[Eli Zaretskii]

> From: Óscar Fuentes <ofv@wanadoo.es>
> Date: Tue, 20 Jul 2010 05:49:35 +0200
> Cc: 
> 
> On Windows, when find-program points to a GNU-compatible `find'
> executable, grep-find-use-xargs is assigned the symbol 'gnu. The
> consequence of this is that `rgrep' ends building a command that
> contains a pipe: find <args> | xargs -0 grep <args> This ends with
> `find' reporting confusing errors about wrong arguments.
> 
> An extra test is added to the assigment of grep-find-use-xargs to force
> the value 'exec on Windows and MS-DOS.

I saw this problem on MS-Windows and intended to look into it.  But
one thing is certain: the MS-DOS build does not need this patch, it
works just fine with the original command line that includes the pipe
to xargs.

> + 		 ((or (eq system-type 'windows-nt) (eq system-type 'ms-dos))

What's wrong with memq?

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows

[Óscar Fuentes]

rgrep is broken on Windows. Although it is obvious that a general fix
for the shell issue is the most correct resolution for this bug, I
propose to apply the patch below and revert it once cmdproxy.c is
enhanced for dealing with the problem.

Otherwise, a workaround is to add

(setq grep-find-use-xargs 'exec)

to the user's .emacs file.


2010-07-21  Óscar Fuentes  <ofv@wanadoo.es>

	* progmodes/grep.el (grep-compute-defaults): always assign
        'exec to grep-find-use-xargs on Windows.

=== modified file 'lisp/progmodes/grep.el'
*** lisp/progmodes/grep.el	2010-01-31 21:47:47 +0000
--- lisp/progmodes/grep.el	2010-07-20 23:04:56 +0000
*************** Set up `compilation-exit-message-functio
*** 552,557 ****
--- 552,561 ----
  	(unless grep-find-use-xargs
  	  (setq grep-find-use-xargs
  		(cond
+ 		 ;; Windows' shell has problems with long commands and
+ 		 ;; pipes. See bug #6674.
+ 		 ((eq system-type 'windows-nt)
+ 		  'exec)
  		 ((and
  		   (grep-probe find-program `(nil nil nil ,null-device "-print0"))
  		   (grep-probe xargs-program `(nil nil nil "-0" "-e" "echo")))

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows

[Christoph]

On 7/20/2010 5:18 PM, Óscar Fuentes wrote:

> rgrep is broken on Windows.

rgrep works fine on Windows with the cygwin port of find (see bug 
#6665). I did however, run into the issue this patch is describing, too 
using the GnuWin32 port.

 > Although it is obvious that a general fix
 > for the shell issue is the most correct resolution for this bug, I
 > propose to apply the patch below and revert it once cmdproxy.c is
 > enhanced for dealing with the problem.

If nobody else is already working on it, I'd want to give it a try to 
fix this. Either by enhancing cmdproxy or implementing Eli's batch file 
solution.

Christoph

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows

[Juanma Barranquero]

On Wed, Jul 21, 2010 at 02:44, Christoph <cschol2112@googlemail.com> wrote:

> If nobody else is already working on it, I'd want to give it a try to fix
> this. Either by enhancing cmdproxy or implementing Eli's batch file
> solution.

Thanks!

    Juanma

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows

[Óscar Fuentes]

Christoph <cschol2112@googlemail.com> writes:

> rgrep works fine on Windows with the cygwin port of find (see bug
> #6665).

I guess you are using cygwin's shell as well, aren't you? Or otherwise
the command line for cygwin's `find' is shortened enough to not upset
cmd.exe

I tried MSYS' find (which is a fork of Cygwin) and it had the same
issue.

[snip]

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows

[Eli Zaretskii]

> Date: Tue, 20 Jul 2010 18:44:55 -0600
> From: Christoph <cschol2112@googlemail.com>
> CC: Eli Zaretskii <eliz@gnu.org>, Juanma Barranquero <lekktu@gmail.com>, 
>  Óscar Fuentes <ofv@wanadoo.es>
> 
> If nobody else is already working on it, I'd want to give it a try to 
> fix this. Either by enhancing cmdproxy or implementing Eli's batch file 
> solution.

Thanks.  To make my intent clear: I meant to enhance cmdproxy to use a
batch file when invoking the windows shell.  You will see that there's
a variable need_shell in cmdproxy's `main' function which gets set to
a non-zero value when cmdproxy decides it needs to pass the command
the the shell instead of invoking it directly.  What I suggested is to
modify the code in this case to put the command on a temporary batch
file, then invoke the shell on that batch file rather than on the
command itself.

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows

[Christoph]

On 7/20/2010 10:02 PM, Eli Zaretskii wrote:
>> Date: Tue, 20 Jul 2010 18:44:55 -0600
>> From: Christoph<cschol2112@googlemail.com>
>> CC: Eli Zaretskii<eliz@gnu.org>, Juanma Barranquero<lekktu@gmail.com>,
>>   Óscar Fuentes<ofv@wanadoo.es>
>>
>> If nobody else is already working on it, I'd want to give it a try to
>> fix this. Either by enhancing cmdproxy or implementing Eli's batch file
>> solution.
>
> Thanks.  To make my intent clear: I meant to enhance cmdproxy to use a
> batch file when invoking the windows shell.  You will see that there's
> a variable need_shell in cmdproxy's `main' function which gets set to
> a non-zero value when cmdproxy decides it needs to pass the command
> the the shell instead of invoking it directly.  What I suggested is to
> modify the code in this case to put the command on a temporary batch
> file, then invoke the shell on that batch file rather than on the
> command itself.

Thanks for the clarification. I will have a look at that.

Christoph

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows

[Christoph]

On 7/20/2010 10:02 PM, Eli Zaretskii wrote:

> Thanks.  To make my intent clear: I meant to enhance cmdproxy to use a
> batch file when invoking the windows shell.  You will see that there's
> a variable need_shell in cmdproxy's `main' function which gets set to
> a non-zero value when cmdproxy decides it needs to pass the command
> the the shell instead of invoking it directly.  What I suggested is to
> modify the code in this case to put the command on a temporary batch
> file, then invoke the shell on that batch file rather than on the
> command itself.

Eli,
I remember a comment of yours earlier in the thread that DOS does not 
have the problem. I assume that this is due to a difference in 
command.com vs cmd.exe?

Do we want to execute the batch file only in case of cmd.exe or for both 
command.com and cmd.exe?

Christoph

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows

[Eli Zaretskii]

> Date: Sun, 01 Aug 2010 20:21:43 -0600
> From: Christoph <cschol2112@googlemail.com>
> CC: bug-gnu-emacs@gnu.org, lekktu@gmail.com, ofv@wanadoo.es
> 
> On 7/20/2010 10:02 PM, Eli Zaretskii wrote:
> 
> > Thanks.  To make my intent clear: I meant to enhance cmdproxy to use a
> > batch file when invoking the windows shell.  You will see that there's
> > a variable need_shell in cmdproxy's `main' function which gets set to
> > a non-zero value when cmdproxy decides it needs to pass the command
> > the the shell instead of invoking it directly.  What I suggested is to
> > modify the code in this case to put the command on a temporary batch
> > file, then invoke the shell on that batch file rather than on the
> > command itself.
> 
> Eli,
> I remember a comment of yours earlier in the thread that DOS does not 
> have the problem.

That's right.

> I assume that this is due to a difference in command.com vs cmd.exe?

No.  That's because the DOS port doesn't call command.com at all.  It
has its own implementation of a shell as part of the `system' function
in the standard library it links against.  That implementation
supports pipes, redirection, quoting, long (up to 16KB) command lines,
and a few other minor Posix features, like /dev/null.

> Do we want to execute the batch file only in case of cmd.exe or for both 
> command.com and cmd.exe?

Yes, for the benefit of Windows 9X.

Thanks.

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows

[Christoph]

On 8/1/2010 9:10 PM, Eli Zaretskii wrote:

> No.  That's because the DOS port doesn't call command.com at all.  It
> has its own implementation of a shell as part of the `system' function
> in the standard library it links against.  That implementation
> supports pipes, redirection, quoting, long (up to 16KB) command lines,
> and a few other minor Posix features, like /dev/null.

I see. Just out of curiosity, why can't this shell implementation be 
used for Windows?

>> Do we want to execute the batch file only in case of cmd.exe or for both
>> command.com and cmd.exe?
>
> Yes, for the benefit of Windows 9X.

Sounds good. Thanks.

bug#6674: PATCH: fix assignment of grep-find-use-xargs on Windows

[Eli Zaretskii]

> Date: Sun, 01 Aug 2010 22:03:29 -0600
> From: Christoph <cschol2112@googlemail.com>
> CC: bug-gnu-emacs@gnu.org
> 
> On 8/1/2010 9:10 PM, Eli Zaretskii wrote:
> 
> > No.  That's because the DOS port doesn't call command.com at all.  It
> > has its own implementation of a shell as part of the `system' function
> > in the standard library it links against.  That implementation
> > supports pipes, redirection, quoting, long (up to 16KB) command lines,
> > and a few other minor Posix features, like /dev/null.
> 
> I see. Just out of curiosity, why can't this shell implementation be 
> used for Windows?

(Looks like the original bug is fixed already, but I will answer this
anyway, for the record.)

Yes, we could do that.  The source of the DJGPP implementation used by
the DOS port is Free Software.  However, this is a non-trivial job,
because porting that code to Windows will need some reimplementation.
E.g., pipes are implemented with temporary files and commands are run
sequentially, one by one (this is DOS, right?), but on Windows we have
real pipes and we should run both sides of the pipe concurrently.

bug#6674: Closing bug 6674

[Óscar Fuentes]

Patch committed.