Irritation in C-u M-x grep, caused by overprotectiveness

Irritation in C-u M-x grep, caused by overprotectiveness

[Alan Mackenzie]

Hi, Emacs!

On the command line, do this:

    $ CC_FILESEL=cc-{align,awk,bytecomp,cmds,compat,defs,engine,fonts,langs,menus,mode,styles,subword,vars}.el
    $ export CC_FILESEL

, and start (or return) to an emacs session.  Load a file from
.../lisp/progmodes, for example:

    C-x C-f ...../lisp/progmodes/cc-mode.el

.  This establishes the current directory.  With this as the current
buffer, do:

    M-x grep

, which prompts with "Run grep (like this): grep -nH -e " in the
mini-buffer.  Fill in the minibuffer like this:

    Run grep (like this): grep -nH -e kwd-clause-end $CC_FILESEL

, and start the grep with <CR>.  This will bring up, inter alia,
cc-engine.el.  Put point on a symbol in this file and do this:

    C-u M-x grep <CR>

.  Emacs 21 returns this prompt in the minibuffer:

    grep -n c-keyword-member $CC_FILESEL
                             ^^^^^^^^^^^

, which is better than what Emacs 22 currently returns:

    grep -nH c-keyword-member *.el
                              ^^^^

If I give an environment variable as "filename", the chances are I'll
want to use it in subsequent greps.  I think the current behaviour is a
bug, a regression, albeit a small one.  However, it's been irritating
me, water torture style (drip, drip, drip, drip, drip, ......), ever
since the release of Emacs 22 to the point where I just can't take it
any more.

#########################################################################

This manipulation is done in `grep-default-command' in
.../progmodes/grep.el.  In the following patch, I simply add a check for
the first character of "the filename" being ?$.  This doesn't seem very
satisfactory because it might not be portable enough.  There doesn't
seem to be a function `env-variable-name-p' which would be useful here.
    
I've also answered RMS's RFC from V1.65 from a year and 9 hours ago.  In
the process I've renamed `sh-arg-re' to `pattern-re' to make the code
more understandable.


2007-07-29  Alan Mackenzie  <acm@muc.de>

	* progmodes/grep.el (grep-default-command): Add comments.  Rename
	`sh-arg-re' to `pattern-re'.  Don't splat the filename from the
	history if it is an environment variable.


Index: grep.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/progmodes/grep.el,v
retrieving revision 1.76
diff -c -r1.76 grep.el
*** grep.el	26 Jul 2007 05:27:27 -0000	1.76
--- grep.el	29 Jul 2007 10:44:14 -0000
***************
*** 537,550 ****
  (defun grep-default-command ()
    "Compute the default grep command for C-u M-x grep to offer."
    (let ((tag-default (shell-quote-argument (grep-tag-default)))
! 	;; This a regexp to match single shell arguments.
! 	;; Could someone please add comments explaining it?
! 	(sh-arg-re "\\(\\(?:\"\\(?:[^\"]\\|\\\\\"\\)+\"\\|'[^']+'\\|[^\"' \t\n]\\)+\\)")
  	(grep-default (or (car grep-history) grep-command)))
      ;; In the default command, find the arg that specifies the pattern.
      (when (or (string-match
! 	       (concat "[^ ]+\\s +\\(?:-[^ ]+\\s +\\)*"
! 		       sh-arg-re "\\(\\s +\\(\\S +\\)\\)?")
  	       grep-default)
  	      ;; If the string is not yet complete.
  	      (string-match "\\(\\)\\'" grep-default))
--- 537,551 ----
  (defun grep-default-command ()
    "Compute the default grep command for C-u M-x grep to offer."
    (let ((tag-default (shell-quote-argument (grep-tag-default)))
! 	;; pattern-re is a regexp to match a grep pattern; that's a sequence
! 	;; of single or double quoted strings and non-WS characters.  It
! 	;; doesn't try to handle backslashes properly; maybe it should.
! 	(pattern-re "\\(\\(?:\"\\(?:[^\"]\\|\\\\\"\\)+\"\\|'[^']+'\\|[^\"' \t\n]\\)+\\)")
  	(grep-default (or (car grep-history) grep-command)))
      ;; In the default command, find the arg that specifies the pattern.
      (when (or (string-match
! 	       (concat "[^ ]+\\s +\\(?:-[^ ]+\\s +\\)*" ; (POSIX) command with options
! 		       pattern-re "\\(\\s +\\(\\S +\\)\\)?") ; pattern + optional filename
  	       grep-default)
  	      ;; If the string is not yet complete.
  	      (string-match "\\(\\)\\'" grep-default))
***************
*** 552,564 ****
        ;; But first, maybe replace the file name pattern.
        (condition-case nil
  	  (unless (or (not (stringp buffer-file-name))
! 		      (when (match-beginning 2)
! 			(save-match-data
! 			  (string-match
! 			   (wildcard-to-regexp
! 			    (file-name-nondirectory
! 			     (match-string 3 grep-default)))
! 			   (file-name-nondirectory buffer-file-name)))))
  	    (setq grep-default (concat (substring grep-default
  						  0 (match-beginning 2))
  				       " *."
--- 553,566 ----
        ;; But first, maybe replace the file name pattern.
        (condition-case nil
  	  (unless (or (not (stringp buffer-file-name))
! 		      (when (match-beginning 3) ; "filename"(s) in grep-default?
! 			(or (save-match-data
! 			      (string-match
! 			       (wildcard-to-regexp
! 				(file-name-nondirectory
! 				 (match-string 3 grep-default)))
! 			       (file-name-nondirectory buffer-file-name)))
! 			    (eq (aref grep-default (match-beginning 3)) ?$))))
  	    (setq grep-default (concat (substring grep-default
  						  0 (match-beginning 2))
  				       " *."



-- 
Alan Mackenzie (Ittersbach, Germany).

Re: Irritation in C-u M-x grep, caused by overprotectiveness

[Stefan Monnier]

> If I give an environment variable as "filename", the chances are I'll
> want to use it in subsequent greps.

I don't see why that'd generally be the case, but I guess it's just as good
a default as the current one.

Maybe a better and more general approach to the problem goes as follows:

Currently the code checks whether the previous "list of files" (typically
a global pattern) matches the current buffer's file name.  If it does then
the previous list of files is reused, otherwise the previous list of files
is ignored and replaced by a new glob pattern.

Now in your case, the list of files which *you* wrote did not "match" the
current buffer's file name, so clearly, the above heuristic shouldn't be
applied anyway.  I.e. we should only check "does it match the current file?"
if it did match the current file in its previous use.


        Stefan

Re: Irritation in C-u M-x grep, caused by overprotectiveness

[Alan Mackenzie]

Hi, Stefan!

On Sun, Jul 29, 2007 at 09:49:33AM -0400, Stefan Monnier wrote:
> > If I give an environment variable as "filename", the chances are I'll
> > want to use it in subsequent greps.

> I don't see why that'd generally be the case, but I guess it's just as
> good a default as the current one.

Well, there's nothing certain in Emacs users other than taxes and death.
I think it's a reasonable assumption, because setting up such a variable
is a lot of work; much more than just typing a list of filenames into the
minibuffer - anybody who does this will be expecting to use the variable
more than once.

> Maybe a better and more general approach to the problem goes as follows:

> Currently the code checks whether the previous "list of files"
> (typically a global pattern) matches the current buffer's file name.

No, it doesn't quite do this.  If several "filenames" have been given on
the top command line of grep's history, it only uses the first one,
(match-string 3 grep-default).  Maybe it should be using (substring
grep-default (match-begin 3)) instead.  But this is a separate issue.

> If it does then the previous list of files is reused, otherwise the
> previous list of files is ignored and replaced by a new glob pattern.

> Now in your case, the list of files which *you* wrote did not "match"
> the current buffer's file name, so clearly, the above heuristic
> shouldn't be applied anyway.  I.e. we should only check "does it match
> the current file?" if it did match the current file in its previous
> use.

Maybe $VARIABLEs should be evaluated first.  What do you think?
Something like (getenv (substring grep-default (1+ (match-begin 3)))) fed
into `regexp-opt', each element having been through `wildcard-to-regexp'.
That might be heavy overkill, though.

>         Stefan

-- 
Alan.

Re: Irritation in C-u M-x grep, caused by overprotectiveness

[Stefan Monnier]

>> Currently the code checks whether the previous "list of files"
>> (typically a global pattern) matches the current buffer's file name.

> No, it doesn't quite do this.  If several "filenames" have been given on
> the top command line of grep's history, it only uses the first one,
> (match-string 3 grep-default).  Maybe it should be using (substring
> grep-default (match-begin 3)) instead.  But this is a separate issue.

Yes, the code could be improved.

>> If it does then the previous list of files is reused, otherwise the
>> previous list of files is ignored and replaced by a new glob pattern.

>> Now in your case, the list of files which *you* wrote did not "match"
>> the current buffer's file name, so clearly, the above heuristic
>> shouldn't be applied anyway.  I.e. we should only check "does it match
>> the current file?" if it did match the current file in its previous
>> use.

> Maybe $VARIABLEs should be evaluated first.  What do you think?
> Something like (getenv (substring grep-default (1+ (match-begin 3)))) fed
> into `regexp-opt', each element having been through `wildcard-to-regexp'.
> That might be heavy overkill, though.

Again, I agree that the implementation of the heuristic can be improved, but
in any case the implementation won't be perfect and the heuristic isn't
always correct, so it seems that if the heuristic doesn't apply to the
original case (either because of imperfect implementation or for some other
reason), then there's no reason to think it'll apply to the next case.


        Stefan

Re: Irritation in C-u M-x grep, caused by overprotectiveness

[Alan Mackenzie]

Hi, Richard!

On Mon, Jul 30, 2007 at 12:44:26PM -0400, Richard Stallman wrote:
>     I've also answered RMS's RFC from V1.65 from a year and 9 hours ago.  In
>     the process I've renamed `sh-arg-re' to `pattern-re' to make the code
>     more understandable.
> 
> Thank you.

Well, thanks!  Should I install the patch or not, and if so to just the
trunk or Emacs 22 as well?

I think I should definitely install the comment enhancements.  But what
about the prime change, i.e. causing grep to remember a filename
argument like "$CC_FILESEL" when started with C-u M-x grep?  So far,
only Stefan has commented - he didn't seem to think the change was good
or bad.

-- 
Alan.

Re: Irritation in C-u M-x grep, caused by overprotectiveness

[Stefan Monnier]

>> I've also answered RMS's RFC from V1.65 from a year and 9 hours ago.  In
>> the process I've renamed `sh-arg-re' to `pattern-re' to make the code
>> more understandable.
>> 
>> Thank you.

> Well, thanks!  Should I install the patch or not, and if so to just the
> trunk or Emacs 22 as well?

> I think I should definitely install the comment enhancements.  But what
> about the prime change, i.e. causing grep to remember a filename
> argument like "$CC_FILESEL" when started with C-u M-x grep?  So far,
> only Stefan has commented - he didn't seem to think the change was good
> or bad.

Yes, I think the behavior implemented by your patch is correct.  I also
think it would be even better to implement the behavior I suggested, which
would cover your case as well as several others.


        Stefan

Re: Irritation in C-u M-x grep, caused by overprotectiveness

[Richard Stallman]

    I think I should definitely install the comment enhancements.  But what
    about the prime change, i.e. causing grep to remember a filename
    argument like "$CC_FILESEL" when started with C-u M-x grep?  So far,
    only Stefan has commented - he didn't seem to think the change was good
    or bad.

That change should not go in Emacs 22.  Please see if others respond
positively before installing it in the trunk.

Please do install the comment enhancements in the trunk.