GNU bug report logs - #29303
25.2; vc-git-grep should shell-escape FILES

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 29303 in the body.
You can then email your comments to 29303 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Angus Lees <gus <at> inodes.org>
To: bug-gnu-emacs <at> gnu.org
Subject: 25.2; vc-git-grep should shell-escape FILES
Date: Wed, 15 Nov 2017 06:25:04 +0000

[Message part 1 (text/plain, inline)]

"git grep" is recursive.  Consequently, the globbing for FILES arg needs
to be done *inside* git, and not by the shell invoking git.

Specifically: `vc-git-grep` needs to shell-escape the FILES value after
`grep-read-files` (so `grep-files-aliases` continues to work) and before
calling `grep-expand-template` (which does no escaping itself).

 - Gus


In GNU Emacs 25.2.2 (x86_64-pc-linux-gnu, GTK+ Version 3.22.20)
 of 2017-09-12, modified by Debian built on trouble
Windowing system distributor 'The X.Org Foundation', version 11.0.11905000
System Description: Debian GNU/Linux testing (buster)

Configured using:
 'configure --build x86_64-linux-gnu --prefix=/usr
 --sharedstatedir=/var/lib --libexecdir=/usr/lib
 --localstatedir=/var/lib --infodir=/usr/share/info
 --mandir=/usr/share/man --with-pop=yes
 --enable-locallisppath=/etc/emacs25:/etc/emacs:/usr/local/share/emacs/25.2/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/25.2/site-lisp:/usr/share/emacs/site-lisp
 --with-sound=alsa --without-gconf --build x86_64-linux-gnu
 --prefix=/usr --sharedstatedir=/var/lib --libexecdir=/usr/lib
 --localstatedir=/var/lib --infodir=/usr/share/info
 --mandir=/usr/share/man --with-pop=yes
 --enable-locallisppath=/etc/emacs25:/etc/emacs:/usr/local/share/emacs/25.2/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/25.2/site-lisp:/usr/share/emacs/site-lisp
 --with-sound=alsa --without-gconf --with-x=yes --with-x-toolkit=gtk3
 --with-toolkit-scroll-bars 'CFLAGS=-g -O2
 -fdebug-prefix-map=/build/emacs25-XrMyQe/emacs25-25.2+1=.
-fstack-protector-strong
 -Wformat -Werror=format-security -Wall' 'CPPFLAGS=-Wdate-time
 -D_FORTIFY_SOURCE=2' LDFLAGS=-Wl,-z,relro'

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GSETTINGS NOTIFY
ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11

Important settings:
  value of $LANG: en_AU.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Emacs-Lisp

Minor modes in effect:
  magit-auto-revert-mode: t
  global-git-commit-mode: t
  async-bytecomp-package-mode: t
  shell-dirtrack-mode: t
  diff-auto-refine-mode: t
  icomplete-mode: t
  iswitchb-mode: t
  show-paren-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  line-number-mode: t
  transient-mark-mode: t

Recent messages:
uncompressing vc-git.el.gz...done
Note: file is write protected
Mark saved where search started [3 times]
Quit
Mark saved where search started [3 times]
Type "q" in help window to restore its previous buffer, C-M-v to scroll
help.
Making completion list...

mouse-2, RET: find function's definition
Auto-saving...

Load-path shadows:
/usr/share/emacs25/site-lisp/cmake-data/cmake-mode hides
/usr/share/emacs/site-lisp/cmake-mode
/usr/share/emacs/25.2/site-lisp/debian-startup hides
/usr/share/emacs/site-lisp/debian-startup
/usr/share/emacs25/site-lisp/flim/hex-util hides
/usr/share/emacs/25.2/lisp/hex-util
/usr/share/emacs25/site-lisp/flim/md4 hides /usr/share/emacs/25.2/lisp/md4
/usr/share/emacs/site-lisp/rst hides
/usr/share/emacs/25.2/lisp/textmodes/rst
/usr/share/emacs25/site-lisp/flim/sasl-ntlm hides
/usr/share/emacs/25.2/lisp/net/sasl-ntlm
/usr/share/emacs25/site-lisp/flim/sasl-cram hides
/usr/share/emacs/25.2/lisp/net/sasl-cram
/usr/share/emacs25/site-lisp/flim/ntlm hides
/usr/share/emacs/25.2/lisp/net/ntlm
/usr/share/emacs25/site-lisp/flim/hmac-def hides
/usr/share/emacs/25.2/lisp/net/hmac-def
/usr/share/emacs25/site-lisp/flim/sasl-digest hides
/usr/share/emacs/25.2/lisp/net/sasl-digest
/usr/share/emacs25/site-lisp/flim/hmac-md5 hides
/usr/share/emacs/25.2/lisp/net/hmac-md5
/usr/share/emacs25/site-lisp/flim/sasl hides
/usr/share/emacs/25.2/lisp/net/sasl
/home/gus/.emacs.d/elpa/seq-2.20/seq hides
/usr/share/emacs/25.2/lisp/emacs-lisp/seq
/home/gus/.emacs.d/elpa/let-alist-1.0.5/let-alist hides
/usr/share/emacs/25.2/lisp/emacs-lisp/let-alist

Features:
(shadow sort mail-extr emacsbug sendmail debug jka-compr eieio-opt
speedbar sb-image ezimage dframe find-func pp grep pulse dired-x
term/xterm xterm git-rebase dockerfile-mode rx markdown-mode noutline
outline magit-obsolete magit-blame magit-stash magit-bisect magit-remote
magit-commit magit-sequence magit-notes magit-worktree magit-branch
magit-files magit-refs magit-status magit magit-repos magit-apply
magit-wip magit-log magit-diff smerge-mode magit-core magit-autorevert
autorevert filenotify magit-process magit-margin magit-mode magit-git
magit-section magit-popup git-commit magit-utils crm log-edit message
rfc822 mml mml-sec epg mm-decode mm-bodies mm-encode mail-parse rfc2231
rfc2047 rfc2045 ietf-drums mailabbrev mail-utils gmm-utils mailheader
pcvs-util add-log with-editor async-bytecomp async tramp-sh tramp
tramp-compat tramp-loaddefs trampver ucs-normalize shell pcomplete
format-spec dash sh-script smie executable yaml-mode make-mode dired-aux
js advice sgml-mode json map cc-mode cc-fonts cc-guess cc-menus cc-cmds
dabbrev misearch multi-isearch vc-git diff-mode easy-mmode edmacro
kmacro imenu go-mode url url-proxy url-privacy url-expand url-methods
url-history url-cookie url-domsuf url-util mailcap find-file ffap
thingatpt url-parse auth-source gnus-util mm-util help-fns mail-prsvr
password-cache url-vars etags xref cl-seq project eieio eieio-core
cl-macs compile comint ansi-color ring dired server icomplete iswitchb
paren cus-start cus-load cc-styles cc-align cc-engine cc-vars cc-defs
finder-inf info package epg-config seq byte-opt gv bytecomp byte-compile
cl-extra help-mode easymenu cconv cl-loaddefs pcase cl-lib debian-el
debian-el-loaddefs dpkg-dev-el dpkg-dev-el-loaddefs time-date mule-util
tooltip eldoc electric uniquify ediff-hook vc-hooks lisp-float-type
mwheel x-win term/common-win x-dnd tool-bar dnd fontset image regexp-opt
fringe tabulated-list newcomment elisp-mode lisp-mode prog-mode register
page menu-bar rfn-eshadow timer select scroll-bar mouse jit-lock
font-lock syntax facemenu font-core frame cl-generic cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese charscript case-table epa-hook
jka-cmpr-hook help simple abbrev minibuffer cl-preloaded nadvice
loaddefs button faces cus-face macroexp files text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote dbusbind inotify dynamic-setting
system-font-setting font-render-setting move-toolbar gtk x-toolkit x
multi-tty make-network-process emacs)

Memory information:
((conses 16 492363 64694)
 (symbols 48 38692 0)
 (miscs 40 3877 2293)
 (strings 32 92434 6983)
 (string-bytes 1 2738138)
 (vectors 16 59458)
 (vector-slots 8 991712 39447)
 (floats 8 392 835)
 (intervals 56 16373 182)
 (buffers 976 66)

[Message part 2 (text/html, inline)]

Message #8 received at 29303 <at> debbugs.gnu.org (full text, mbox):

From: Robert Pluim <rpluim <at> gmail.com>
To: Angus Lees <gus <at> inodes.org>
Cc: 29303 <at> debbugs.gnu.org
Subject: Re: bug#29303: 25.2; vc-git-grep should shell-escape FILES
Date: Wed, 15 Nov 2017 10:58:19 +0100

[Message part 1 (text/plain, inline)]

Angus Lees <gus <at> inodes.org> writes:

> "git grep" is recursive.  Consequently, the globbing for FILES arg needs
> to be done *inside* git, and not by the shell invoking git.
>
> Specifically: `vc-git-grep` needs to shell-escape the FILES value after
> `grep-read-files` (so `grep-files-aliases` continues to work) and before
> calling `grep-expand-template` (which does no escaping itself).
>

You mean something like the patch below? I considered splitting on
spaces and doing shell-quote-argument, but that seems like overkill.

(this is where someone points me at a function somewhere in emacs that
does exactly this operation already)

[0001-Quote-filenames-to-inhibit-expansion-by-the-shell.patch (text/x-diff, inline)]

From 788126ca723ba2e37553eaf5f17141be3544a5cb Mon Sep 17 00:00:00 2001
From: Robert Pluim <rpluim <at> gmail.com>
Date: Wed, 15 Nov 2017 10:51:37 +0100
Subject: [PATCH] Quote filenames to inhibit expansion by the shell

* lisp/vc/vc-git.el (vc-git-grep): Add quotes around filename patterns
to ensure globbing is done by git rather than the shell. (Bug#29303)
---
 lisp/vc/vc-git.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index ed85603f82..fd5f5d5b63 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1428,7 +1428,8 @@ vc-git-grep
 				   nil nil 'grep-history)
 	     nil))
       (t (let* ((regexp (grep-read-regexp))
-		(files (grep-read-files regexp))
+		(files (concat "'" (replace-regexp-in-string " " "' '"
+				   (grep-read-files regexp)) "'"))
 		(dir (read-directory-name "In directory: "
 					  nil default-directory t)))
 	   (list regexp files dir))))))
-- 
2.15.0

[Message part 3 (text/plain, inline)]

Robert

Message #11 received at 29303 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: 29303 <at> debbugs.gnu.org, gus <at> inodes.org
Subject: Re: bug#29303: 25.2; vc-git-grep should shell-escape FILES
Date: Wed, 15 Nov 2017 19:42:17 +0200

> From: Robert Pluim <rpluim <at> gmail.com>
> Date: Wed, 15 Nov 2017 10:58:19 +0100
> Cc: 29303 <at> debbugs.gnu.org
> 
> diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
> index ed85603f82..fd5f5d5b63 100644
> --- a/lisp/vc/vc-git.el
> +++ b/lisp/vc/vc-git.el
> @@ -1428,7 +1428,8 @@ vc-git-grep
>  				   nil nil 'grep-history)
>  	     nil))
>        (t (let* ((regexp (grep-read-regexp))
> -		(files (grep-read-files regexp))
> +		(files (concat "'" (replace-regexp-in-string " " "' '"
> +				   (grep-read-files regexp)) "'"))
>  		(dir (read-directory-name "In directory: "
>  					  nil default-directory t)))
>  	   (list regexp files dir))))))

This cannot be right, because this style of quoting only works with
Posix shells.

Message #14 received at 29303 <at> debbugs.gnu.org (full text, mbox):

From: Robert Pluim <rpluim <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 29303 <at> debbugs.gnu.org, gus <at> inodes.org
Subject: Re: bug#29303: 25.2; vc-git-grep should shell-escape FILES
Date: Wed, 15 Nov 2017 19:45:52 +0100

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

> This cannot be right, because this style of quoting only works with
> Posix shells.

There are people who run Emacs on Windows who use cmd.exe as the shell
to invoke git? Takes all sorts, I guess. How about this then?

[0001-Shell-quote-filenames-to-inhibit-globbing-by-the-she.patch (text/x-patch, inline)]

From 0e3b4ee74bebae702bade0f1715fbb96b46bbec6 Mon Sep 17 00:00:00 2001
From: Robert Pluim <rpluim <at> gmail.com>
Date: Wed, 15 Nov 2017 10:51:37 +0100
Subject: [PATCH] Shell quote filenames to inhibit globbing by the shell

* lisp/vc/vc-git.el (vc-git-grep): Apply shell quoting to filename
patterns to ensure globbing is done by git rather than the
shell. (Bug#29303)
---
 lisp/vc/vc-git.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index ed85603f82..43164b4fcf 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1428,7 +1428,7 @@ vc-git-grep
 				   nil nil 'grep-history)
 	     nil))
       (t (let* ((regexp (grep-read-regexp))
-		(files (grep-read-files regexp))
+		(files (mapconcat #'shell-quote-argument (split-string (grep-read-files regexp)) " "))
 		(dir (read-directory-name "In directory: "
 					  nil default-directory t)))
 	   (list regexp files dir))))))
-- 
2.15.0.276.g89ea799ff

Message #17 received at 29303 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: 29303 <at> debbugs.gnu.org, gus <at> inodes.org
Subject: Re: bug#29303: 25.2; vc-git-grep should shell-escape FILES
Date: Wed, 15 Nov 2017 21:58:49 +0200

> From: Robert Pluim <rpluim <at> gmail.com>
> Cc: 29303 <at> debbugs.gnu.org,  gus <at> inodes.org
> Date: Wed, 15 Nov 2017 19:45:52 +0100
> 
> > This cannot be right, because this style of quoting only works with
> > Posix shells.
> 
> There are people who run Emacs on Windows who use cmd.exe as the shell
> to invoke git?

Yes, most of them, because that's the default.  But that's not
relevant, because shell wildcards are expanded on Windows by the
startup code of the invoked program, not by the shell.

> diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
> index ed85603f82..43164b4fcf 100644
> --- a/lisp/vc/vc-git.el
> +++ b/lisp/vc/vc-git.el
> @@ -1428,7 +1428,7 @@ vc-git-grep
>  				   nil nil 'grep-history)
>  	     nil))
>        (t (let* ((regexp (grep-read-regexp))
> -		(files (grep-read-files regexp))
> +		(files (mapconcat #'shell-quote-argument (split-string (grep-read-files regexp)) " "))
>  		(dir (read-directory-name "In directory: "
>  					  nil default-directory t)))
>  	   (list regexp files dir))))))

That's okay portability-wise, but why do you need split-string?
AFAIU, grep-read-files reads a single pattern, no?

Message #20 received at 29303 <at> debbugs.gnu.org (full text, mbox):

From: Robert Pluim <rpluim <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 29303 <at> debbugs.gnu.org, gus <at> inodes.org
Subject: Re: bug#29303: 25.2; vc-git-grep should shell-escape FILES
Date: Wed, 15 Nov 2017 21:17:35 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Robert Pluim <rpluim <at> gmail.com>
>> diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
>> index ed85603f82..43164b4fcf 100644
>> --- a/lisp/vc/vc-git.el
>> +++ b/lisp/vc/vc-git.el
>> @@ -1428,7 +1428,7 @@ vc-git-grep
>>  				   nil nil 'grep-history)
>>  	     nil))
>>        (t (let* ((regexp (grep-read-regexp))
>> -		(files (grep-read-files regexp))
>> +		(files (mapconcat #'shell-quote-argument (split-string (grep-read-files regexp)) " "))
>>  		(dir (read-directory-name "In directory: "
>>  					  nil default-directory t)))
>>  	   (list regexp files dir))))))
>
> That's okay portability-wise, but why do you need split-string?
> AFAIU, grep-read-files reads a single pattern, no?

grep-read-files has support for grep-files-aliases which allows you to
eg say 'cc' and have it expand to "*.cc *.cxx *.cpp *.C *.CC *.c++"

It's also possible to enter multiple patterns using grep-read-files,
although you have to do things like ^Q<SPC> to get a space into the
string. That might be worth fixing separately.

Robert

Message #23 received at 29303 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: 29303 <at> debbugs.gnu.org, gus <at> inodes.org
Subject: Re: bug#29303: 25.2; vc-git-grep should shell-escape FILES
Date: Wed, 15 Nov 2017 22:26:12 +0200

> From: Robert Pluim <rpluim <at> gmail.com>
> Cc: 29303 <at> debbugs.gnu.org,  gus <at> inodes.org
> Date: Wed, 15 Nov 2017 21:17:35 +0100
> 
> > That's okay portability-wise, but why do you need split-string?
> > AFAIU, grep-read-files reads a single pattern, no?
> 
> grep-read-files has support for grep-files-aliases which allows you to
> eg say 'cc' and have it expand to "*.cc *.cxx *.cpp *.C *.CC *.c++"

Ah.  So we lose support of patterns with embedded whitespace in order
to support the aliases?  Is that desirable?

Message #26 received at 29303 <at> debbugs.gnu.org (full text, mbox):

From: Robert Pluim <rpluim <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 29303 <at> debbugs.gnu.org, gus <at> inodes.org
Subject: Re: bug#29303: 25.2; vc-git-grep should shell-escape FILES
Date: Wed, 15 Nov 2017 21:36:09 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Robert Pluim <rpluim <at> gmail.com>
>> Cc: 29303 <at> debbugs.gnu.org,  gus <at> inodes.org
>> Date: Wed, 15 Nov 2017 21:17:35 +0100
>> 
>> > That's okay portability-wise, but why do you need split-string?
>> > AFAIU, grep-read-files reads a single pattern, no?
>> 
>> grep-read-files has support for grep-files-aliases which allows you to
>> eg say 'cc' and have it expand to "*.cc *.cxx *.cpp *.C *.CC *.c++"
>
> Ah.  So we lose support of patterns with embedded whitespace in order
> to support the aliases?  Is that desirable?

That's a good question. Note that interactively entering patterns with
a space is currently a pain, since grep-read-files uses
read-file-name-internal, which attempts to do completion if you type
<SPC>. So this would presumably only affect people who have changed
grep-files-aliases and have patterns with embedded whitespace.

Robert

Message #29 received at 29303 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: 29303 <at> debbugs.gnu.org, gus <at> inodes.org
Subject: Re: bug#29303: 25.2; vc-git-grep should shell-escape FILES
Date: Thu, 16 Nov 2017 17:38:20 +0200

> From: Robert Pluim <rpluim <at> gmail.com>
> Cc: 29303 <at> debbugs.gnu.org,  gus <at> inodes.org
> Date: Wed, 15 Nov 2017 21:36:09 +0100
> 
> > Ah.  So we lose support of patterns with embedded whitespace in order
> > to support the aliases?  Is that desirable?
> 
> That's a good question. Note that interactively entering patterns with
> a space is currently a pain, since grep-read-files uses
> read-file-name-internal, which attempts to do completion if you type
> <SPC>. So this would presumably only affect people who have changed
> grep-files-aliases and have patterns with embedded whitespace.

Maybe we should mention this in the doc string (assuming that your
proposal indeed fixes the original problem).

Message #32 received at 29303 <at> debbugs.gnu.org (full text, mbox):

From: Robert Pluim <rpluim <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 29303 <at> debbugs.gnu.org, gus <at> inodes.org
Subject: Re: bug#29303: 25.2; vc-git-grep should shell-escape FILES
Date: Thu, 16 Nov 2017 16:43:19 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Robert Pluim <rpluim <at> gmail.com>
>> Cc: 29303 <at> debbugs.gnu.org,  gus <at> inodes.org
>> Date: Wed, 15 Nov 2017 21:36:09 +0100
>> 
>> > Ah.  So we lose support of patterns with embedded whitespace in order
>> > to support the aliases?  Is that desirable?
>> 
>> That's a good question. Note that interactively entering patterns with
>> a space is currently a pain, since grep-read-files uses
>> read-file-name-internal, which attempts to do completion if you type
>> <SPC>. So this would presumably only affect people who have changed
>> grep-files-aliases and have patterns with embedded whitespace.
>
> Maybe we should mention this in the doc string (assuming that your
> proposal indeed fixes the original problem).

It fixes my test case, at least. Gus, have you had a chance to try the
second patch?

Thanks

Robert

Message #35 received at 29303 <at> debbugs.gnu.org (full text, mbox):

From: Andreas Schwab <schwab <at> suse.de>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Robert Pluim <rpluim <at> gmail.com>, 29303 <at> debbugs.gnu.org, gus <at> inodes.org
Subject: Re: bug#29303: 25.2; vc-git-grep should shell-escape FILES
Date: Thu, 16 Nov 2017 16:55:33 +0100

On Nov 15 2017, Eli Zaretskii <eliz <at> gnu.org> wrote:

> AFAIU, grep-read-files reads a single pattern, no?

All current uses of grep-read-files either use split-string or pass it
unquoted to the shell.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab <at> suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Message #38 received at 29303 <at> debbugs.gnu.org (full text, mbox):

From: Angus Lees <gus <at> inodes.org>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 29303 <at> debbugs.gnu.org
Subject: Re: bug#29303: 25.2; vc-git-grep should shell-escape FILES
Date: Fri, 17 Nov 2017 07:13:04 +0000

[Message part 1 (text/plain, inline)]

On Fri, 17 Nov 2017 at 02:43 Robert Pluim <rpluim <at> gmail.com> wrote:

> It fixes my test case, at least. Gus, have you had a chance to try the
> second patch?


Yes, works exactly as I'd hoped :)
(tried both with a literal (default) value and a value from
`grep-file-aliases`)

Thanks for the swift responses - I will be happy indeed to be able to just
hit RET at that prompt instead of always having to manually escape the glob.

 - Gus

[Message part 2 (text/html, inline)]

Message #41 received at 29303 <at> debbugs.gnu.org (full text, mbox):

From: Robert Pluim <rpluim <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 29303 <at> debbugs.gnu.org, gus <at> inodes.org
Subject: Re: bug#29303: 25.2; vc-git-grep should shell-escape FILES
Date: Fri, 17 Nov 2017 09:38:33 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Robert Pluim <rpluim <at> gmail.com>
>> That's a good question. Note that interactively entering patterns with
>> a space is currently a pain, since grep-read-files uses
>> read-file-name-internal, which attempts to do completion if you type
>> <SPC>. So this would presumably only affect people who have changed
>> grep-files-aliases and have patterns with embedded whitespace.
>
> Maybe we should mention this in the doc string

For vc-git-grep, or for every function that uses grep-read-files?

Robert

Message #44 received at 29303 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: 29303 <at> debbugs.gnu.org, gus <at> inodes.org
Subject: Re: bug#29303: 25.2; vc-git-grep should shell-escape FILES
Date: Fri, 17 Nov 2017 11:06:09 +0200

> From: Robert Pluim <rpluim <at> gmail.com>
> Cc: 29303 <at> debbugs.gnu.org,  gus <at> inodes.org
> Date: Fri, 17 Nov 2017 09:38:33 +0100
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> From: Robert Pluim <rpluim <at> gmail.com>
> >> That's a good question. Note that interactively entering patterns with
> >> a space is currently a pain, since grep-read-files uses
> >> read-file-name-internal, which attempts to do completion if you type
> >> <SPC>. So this would presumably only affect people who have changed
> >> grep-files-aliases and have patterns with embedded whitespace.
> >
> > Maybe we should mention this in the doc string
> 
> For vc-git-grep, or for every function that uses grep-read-files?

The latter, I think.  If there are too many of those callers, then we
could describe this once, say in grep-read-files, and then reference
that, with something like

  Including whitespace in the pattern requires quoting, see
  `grep-read-files' for the details.

Thanks.

Message #47 received at 29303 <at> debbugs.gnu.org (full text, mbox):

From: Robert Pluim <rpluim <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 29303 <at> debbugs.gnu.org, gus <at> inodes.org
Subject: Re: bug#29303: 25.2; vc-git-grep should shell-escape FILES
Date: Fri, 17 Nov 2017 10:54:59 +0100

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Robert Pluim <rpluim <at> gmail.com>
>> For vc-git-grep, or for every function that uses grep-read-files?
>
> The latter, I think.  If there are too many of those callers, then we
> could describe this once, say in grep-read-files, and then reference
> that, with something like
>
>   Including whitespace in the pattern requires quoting, see
>   `grep-read-files' for the details.

There are only 4 such (5 if you count zrgrep, which basically says
'see rgrep') and the addition is small, so I documented it in the
callers, and in grep-read-files.

Robert

[0001-Explain-how-to-enter-whitespace-when-using-grep-read.patch (text/x-patch, inline)]

From 2b0535557316518ea75945e0e7b20576b2bbea47 Mon Sep 17 00:00:00 2001
From: Robert Pluim <rpluim <at> gmail.com>
Date: Fri, 17 Nov 2017 10:51:26 +0100
Subject: [PATCH] Explain how to enter whitespace when using grep-read-files

* lisp/progmodes/grep.el (lgrep): Explain how to enter
whitespace when using grep-read-files.
(rgrep): Likewise.
(grep-read-files): Likewise.
* lisp/progmodes/project.el (project-find-regexp): Likewise.
* lisp/vc/vc-git.el (vc-git-grep): Likewise.
---
 lisp/progmodes/grep.el    | 12 +++++++++---
 lisp/progmodes/project.el |  6 +++++-
 lisp/vc/vc-git.el         |  4 +++-
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/lisp/progmodes/grep.el b/lisp/progmodes/grep.el
index d0404fdeaf..56aef15a08 100644
--- a/lisp/progmodes/grep.el
+++ b/lisp/progmodes/grep.el
@@ -904,7 +904,9 @@ grep-read-regexp
 
 (defun grep-read-files (regexp)
   "Read a file-name pattern arg for interactive grep.
-The pattern can include shell wildcards."
+The pattern can include shell wildcards.  As whitespace triggers
+completion when entering a pattern, including it requires
+quoting, eg `\C-q<space>'."
   (let* ((bn (or (buffer-file-name)
 		 (replace-regexp-in-string "<[0-9]+>\\'" "" (buffer-name))))
 	 (fn (and bn
@@ -954,7 +956,9 @@ lgrep
   "Run grep, searching for REGEXP in FILES in directory DIR.
 The search is limited to file names matching shell pattern FILES.
 FILES may use abbreviations defined in `grep-files-aliases', e.g.
-entering `ch' is equivalent to `*.[ch]'.
+entering `ch' is equivalent to `*.[ch]'.  As whitespace triggers
+completion when entering a pattern, including it requires
+quoting, eg `\C-q<space>'.
 
 With \\[universal-argument] prefix, you can edit the constructed shell command line
 before it is executed.
@@ -1032,7 +1036,9 @@ rgrep
   "Recursively grep for REGEXP in FILES in directory tree rooted at DIR.
 The search is limited to file names matching shell pattern FILES.
 FILES may use abbreviations defined in `grep-files-aliases', e.g.
-entering `ch' is equivalent to `*.[ch]'.
+entering `ch' is equivalent to `*.[ch]'.  As whitespace triggers
+completion when entering a pattern, including it requires
+quoting, eg `\C-q<space>'.
 
 With \\[universal-argument] prefix, you can edit the constructed shell command line
 before it is executed.
diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 9dc0da4ad5..06f5aa5c94 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -307,7 +307,11 @@ project--value-in-dir
 (defun project-find-regexp (regexp)
   "Find all matches for REGEXP in the current project's roots.
 With \\[universal-argument] prefix, you can specify the directory
-to search in, and the file name pattern to search for."
+to search in, and the file name pattern to search for.  The
+pattern may use abbreviations defined in `grep-files-aliases',
+e.g.  entering `ch' is equivalent to `*.[ch]'.  As whitespace
+triggers completion when entering a pattern, including it
+requires quoting, eg `\C-q<space>'."
   (interactive (list (project--read-regexp)))
   (let* ((pr (project-current t))
          (dirs (if current-prefix-arg
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 43164b4fcf..eaa31c5def 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1407,7 +1407,9 @@ vc-git-grep
   "Run git grep, searching for REGEXP in FILES in directory DIR.
 The search is limited to file names matching shell pattern FILES.
 FILES may use abbreviations defined in `grep-files-aliases', e.g.
-entering `ch' is equivalent to `*.[ch]'.
+entering `ch' is equivalent to `*.[ch]'.  As whitespace triggers
+completion when entering a pattern, including it requires
+quoting, eg `\C-q<space>'.
 
 With \\[universal-argument] prefix, you can edit the constructed shell command line
 before it is executed.
-- 
2.15.0.276.g89ea799ff

Message #50 received at 29303 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: 29303 <at> debbugs.gnu.org, gus <at> inodes.org
Subject: Re: bug#29303: 25.2; vc-git-grep should shell-escape FILES
Date: Fri, 17 Nov 2017 12:13:46 +0200

> From: Robert Pluim <rpluim <at> gmail.com>
> Cc: 29303 <at> debbugs.gnu.org,  gus <at> inodes.org
> Date: Fri, 17 Nov 2017 10:54:59 +0100
> 
> >> For vc-git-grep, or for every function that uses grep-read-files?
> >
> > The latter, I think.  If there are too many of those callers, then we
> > could describe this once, say in grep-read-files, and then reference
> > that, with something like
> >
> >   Including whitespace in the pattern requires quoting, see
> >   `grep-read-files' for the details.
> 
> There are only 4 such (5 if you count zrgrep, which basically says
> 'see rgrep') and the addition is small, so I documented it in the
> callers, and in grep-read-files.

Thanks, this is fine, except that please use \\[quoted-insert] instead
of a literal \C-q, to DTRT when quoted-insert is rebound to some other
key.

Message #53 received at 29303 <at> debbugs.gnu.org (full text, mbox):

From: Robert Pluim <rpluim <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 29303 <at> debbugs.gnu.org, gus <at> inodes.org
Subject: Re: bug#29303: 25.2; vc-git-grep should shell-escape FILES
Date: Fri, 17 Nov 2017 11:33:08 +0100

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

> Thanks, this is fine, except that please use \\[quoted-insert] instead
> of a literal \C-q, to DTRT when quoted-insert is rebound to some other
> key.

Attached. I fixed my laziness about 'e.g.' for good measure. I'm
hoping this can make emacs-26.

[0001-Explain-how-to-enter-whitespace-when-using-grep-read.patch (text/x-patch, inline)]

From 69a68ceed22ab78c1f6d578c3ad9781f32b0f218 Mon Sep 17 00:00:00 2001
From: Robert Pluim <rpluim <at> gmail.com>
Date: Fri, 17 Nov 2017 10:51:26 +0100
Subject: [PATCH] Explain how to enter whitespace when using grep-read-files

* lisp/progmodes/grep.el (lgrep): Explain how to enter
whitespace when using grep-read-files.
(rgrep): Likewise.
(grep-read-files): Likewise.
* lisp/progmodes/project.el (project-find-regexp): Likewise.
* lisp/vc/vc-git.el (vc-git-grep): Likewise.
---
 lisp/progmodes/grep.el    | 12 +++++++++---
 lisp/progmodes/project.el |  6 +++++-
 lisp/vc/vc-git.el         |  4 +++-
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/lisp/progmodes/grep.el b/lisp/progmodes/grep.el
index d0404fdeaf..c2d8022354 100644
--- a/lisp/progmodes/grep.el
+++ b/lisp/progmodes/grep.el
@@ -904,7 +904,9 @@ grep-read-regexp
 
 (defun grep-read-files (regexp)
   "Read a file-name pattern arg for interactive grep.
-The pattern can include shell wildcards."
+The pattern can include shell wildcards.  As whitespace triggers
+completion when entering a pattern, including it requires
+quoting, e.g. `\\[quoted-insert]<space>'."
   (let* ((bn (or (buffer-file-name)
 		 (replace-regexp-in-string "<[0-9]+>\\'" "" (buffer-name))))
 	 (fn (and bn
@@ -954,7 +956,9 @@ lgrep
   "Run grep, searching for REGEXP in FILES in directory DIR.
 The search is limited to file names matching shell pattern FILES.
 FILES may use abbreviations defined in `grep-files-aliases', e.g.
-entering `ch' is equivalent to `*.[ch]'.
+entering `ch' is equivalent to `*.[ch]'.  As whitespace triggers
+completion when entering a pattern, including it requires
+quoting, e.g. `\\[quoted-insert]<space>'.
 
 With \\[universal-argument] prefix, you can edit the constructed shell command line
 before it is executed.
@@ -1032,7 +1036,9 @@ rgrep
   "Recursively grep for REGEXP in FILES in directory tree rooted at DIR.
 The search is limited to file names matching shell pattern FILES.
 FILES may use abbreviations defined in `grep-files-aliases', e.g.
-entering `ch' is equivalent to `*.[ch]'.
+entering `ch' is equivalent to `*.[ch]'.  As whitespace triggers
+completion when entering a pattern, including it requires
+quoting, e.g. `\\[quoted-insert]<space>'.
 
 With \\[universal-argument] prefix, you can edit the constructed shell command line
 before it is executed.
diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 9dc0da4ad5..2dc1d9a070 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -307,7 +307,11 @@ project--value-in-dir
 (defun project-find-regexp (regexp)
   "Find all matches for REGEXP in the current project's roots.
 With \\[universal-argument] prefix, you can specify the directory
-to search in, and the file name pattern to search for."
+to search in, and the file name pattern to search for.  The
+pattern may use abbreviations defined in `grep-files-aliases',
+e.g.  entering `ch' is equivalent to `*.[ch]'.  As whitespace
+triggers completion when entering a pattern, including it
+requires quoting, e.g. `\\[quoted-insert]<space>'."
   (interactive (list (project--read-regexp)))
   (let* ((pr (project-current t))
          (dirs (if current-prefix-arg
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 43164b4fcf..bcb10a544f 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1407,7 +1407,9 @@ vc-git-grep
   "Run git grep, searching for REGEXP in FILES in directory DIR.
 The search is limited to file names matching shell pattern FILES.
 FILES may use abbreviations defined in `grep-files-aliases', e.g.
-entering `ch' is equivalent to `*.[ch]'.
+entering `ch' is equivalent to `*.[ch]'.  As whitespace triggers
+completion when entering a pattern, including it requires
+quoting, e.g. `\\[quoted-insert]<space>'.
 
 With \\[universal-argument] prefix, you can edit the constructed shell command line
 before it is executed.
-- 
2.15.0.276.g89ea799ff

Message #56 received at 29303 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: 29303 <at> debbugs.gnu.org, gus <at> inodes.org
Subject: Re: bug#29303: 25.2; vc-git-grep should shell-escape FILES
Date: Fri, 17 Nov 2017 15:41:46 +0200

> From: Robert Pluim <rpluim <at> gmail.com>
> Cc: 29303 <at> debbugs.gnu.org,  gus <at> inodes.org
> Date: Fri, 17 Nov 2017 11:33:08 +0100
> 
> > Thanks, this is fine, except that please use \\[quoted-insert] instead
> > of a literal \C-q, to DTRT when quoted-insert is rebound to some other
> > key.
> 
> Attached. I fixed my laziness about 'e.g.' for good measure.

Thanks, pushed.

> I'm hoping this can make emacs-26.

Documentation changes are always okay for the release branch.

Message #59 received at 29303 <at> debbugs.gnu.org (full text, mbox):

From: Robert Pluim <rpluim <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 29303 <at> debbugs.gnu.org, gus <at> inodes.org
Subject: Re: bug#29303: 25.2; vc-git-grep should shell-escape FILES
Date: Fri, 17 Nov 2017 15:21:52 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Robert Pluim <rpluim <at> gmail.com>
>> Cc: 29303 <at> debbugs.gnu.org,  gus <at> inodes.org
>> Date: Fri, 17 Nov 2017 11:33:08 +0100
>> 
>> > Thanks, this is fine, except that please use \\[quoted-insert] instead
>> > of a literal \C-q, to DTRT when quoted-insert is rebound to some other
>> > key.
>> 
>> Attached. I fixed my laziness about 'e.g.' for good measure.
>
> Thanks, pushed.
>

Thanks

>> I'm hoping this can make emacs-26.
>
> Documentation changes are always okay for the release branch.

I meant the patch to quote arguments to git grep, since that's fixing
an actual problem, but it's a change in behaviour, so it might be too
late for that.

Robert

Message #62 received at 29303 <at> debbugs.gnu.org (full text, mbox):

From: Robert Pluim <rpluim <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 29303 <at> debbugs.gnu.org, gus <at> inodes.org
Subject: Re: bug#29303: 25.2; vc-git-grep should shell-escape FILES
Date: Tue, 28 Nov 2017 14:37:58 +0100

Robert Pluim <rpluim <at> gmail.com> writes:

> I meant the patch to quote arguments to git grep, since that's fixing
> an actual problem, but it's a change in behaviour, so it might be too
> late for that.

Eli, is patch #2 ok to go into emacs-26 or master? Do you need a NEWS
entry?

Thanks

Robert

Message #67 received at 29303-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: 29303-done <at> debbugs.gnu.org, gus <at> inodes.org
Subject: Re: bug#29303: 25.2; vc-git-grep should shell-escape FILES
Date: Tue, 28 Nov 2017 19:49:33 +0200

> From: Robert Pluim <rpluim <at> gmail.com>
> Cc: 29303 <at> debbugs.gnu.org,  gus <at> inodes.org
> Gmane-Reply-To-List: yes
> Date: Tue, 28 Nov 2017 14:37:58 +0100
> 
> Eli, is patch #2 ok to go into emacs-26 or master?

I pushed it, thanks.

> Do you need a NEWS entry?

No, it's a bug fix.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.