bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Peter Povinec]

I despise spaces in directory names like the next guy, but sometimes it
is unavoidable. In my case, I had some elisp files in apple
icloud shared folder, and those are mounted by the system
under a nasty path like
~/Library/Mobile\ Documents/com~apple~CloudDocs.
If I loaded one of those files and tried xref-find-references,
it would never find anything, not even references within the same file.

Here is a contrived scenario to reproduce the issue:
1. create two symlinks like
ln -s ~/Applications/Emacs.app/Contents/Resources/lisp/progmodes
~/space\ dir
ln -s ~/Applications/Emacs.app/Contents/Resources/lisp/progmodes
~/nospacedir
2. C-x C-f ~/space dir/xref.el
3. M-? on xref-location-marker, specify default project and default
directory ~/space dir
4. Observe "No references found for: xref-location-marker"
5. Close the xref.el buffer with C-x k
6. Repeat steps 2-3, but using  ~/nospacedir instead
7. Observe that references are shown correctly

We should be able to handle directories with spaces. If for some reason
we couldn't do that, we'd need to inform the user explicitly.

Workaround to the original problem where the elisp files are
stored under a directory with spaces in it:
1. Create a symlink pointing to a subdir under the directory that
contains the space so you have a pathname to the files without any space.
2. Configure the mapping from the full pathname to the symlink using
directory-abbrev-alist to tell emacs to use the symlink directory name
whenever creating buffers of files under the space-containing directory.



In GNU Emacs 28.1 (build 1, aarch64-apple-darwin21.1.0, NS
appkit-2113.00 Version 12.0.1 (Build 21A559))
 of 2022-04-04 built on armbob.lan
Windowing system distributor 'Apple', version 10.3.2113
System Description:  macOS 12.2.1

Configured using:
 'configure --with-ns '--enable-locallisppath=/Library/Application
 Support/Emacs/${version}/site-lisp:/Library/Application
 Support/Emacs/site-lisp' --with-modules'

Configured features:
ACL GMP GNUTLS JSON LIBXML2 MODULES NOTIFY KQUEUE NS PDUMPER THREADS
TOOLKIT_SCROLL_BARS ZLIB

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

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
rfc822 mml mml-sec epa derived epg rfc6068 epg-config gnus-util rmail
rmail-loaddefs auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache json map text-property-search time-date
subr-x seq byte-opt gv bytecomp byte-compile cconv mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader cl-loaddefs
cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
iso-transl tooltip eldoc paren electric uniquify ediff-hook vc-hooks
lisp-float-type elisp-mode mwheel term/ns-win ns-win ucs-normalize
mule-util term/common-win tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer 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 composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice button
loaddefs faces cus-face macroexp files window text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote threads kqueue cocoa ns multi-tty
make-network-process emacs)

Memory information:
((conses 16 50996 6609)
 (symbols 48 6552 1)
 (strings 32 17843 2090)
 (string-bytes 1 596645)
 (vectors 16 13825)
 (vector-slots 8 184230 11069)
 (floats 8 21 39)
 (intervals 56 301 0)
 (buffers 992 11))

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Eli Zaretskii]

> From: Peter Povinec <spepo.42@gmail.com>
> Date: Mon, 18 Apr 2022 21:52:49 -0700
> 
> Here is a contrived scenario to reproduce the issue:
> 1. create two symlinks like
> ln -s ~/Applications/Emacs.app/Contents/Resources/lisp/progmodes
> ~/space\ dir
> ln -s ~/Applications/Emacs.app/Contents/Resources/lisp/progmodes
> ~/nospacedir
> 2. C-x C-f ~/space dir/xref.el
> 3. M-? on xref-location-marker, specify default project and default
> directory ~/space dir
> 4. Observe "No references found for: xref-location-marker"
> 5. Close the xref.el buffer with C-x k
> 6. Repeat steps 2-3, but using  ~/nospacedir instead
> 7. Observe that references are shown correctly

I'm running half-blind here, because you didn't tell which Xref
backend is being used for this (do you have ID Utils or GNU Global or
Cscope installed and used for the above recipe?).  My guess is that
Emacs uses the default "find | grep" to do the search, in which case
the patch below should help; does it?

diff --git a/lisp/cedet/semantic/symref/grep.el b/lisp/cedet/semantic/symref/grep.el
index 27ea80f..bc96505 100644
--- a/lisp/cedet/semantic/symref/grep.el
+++ b/lisp/cedet/semantic/symref/grep.el
@@ -124,7 +124,7 @@ semantic-symref-grep-use-template
                  grep-find-template)
                pattern
                filepattern
-               rootdir)))
+               (shell-quote-argument rootdir))))
     cmd))
 
 (defcustom semantic-symref-grep-shell shell-file-name

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Peter Povinec]

On Tue, Apr 19, 2022 at 12:13 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> I'm running half-blind here, because you didn't tell which Xref
> backend is being used for this (do you have ID Utils or GNU Global or
> Cscope installed and used for the above recipe?).  My guess is that

Sorry, should have mentioned that the recipe is with 'emacs -Q',
no special backends.
I believe it is just doing find+grep underneath.

> Emacs uses the default "find | grep" to do the search, in which case
> the patch below should help; does it?
>
> diff --git a/lisp/cedet/semantic/symref/grep.el b/lisp/cedet/semantic/symref/grep.el
> index 27ea80f..bc96505 100644
> --- a/lisp/cedet/semantic/symref/grep.el
> +++ b/lisp/cedet/semantic/symref/grep.el
> @@ -124,7 +124,7 @@ semantic-symref-grep-use-template
>                   grep-find-template)
>                 pattern
>                 filepattern
> -               rootdir)))
> +               (shell-quote-argument rootdir))))
>      cmd))
>
>  (defcustom semantic-symref-grep-shell shell-file-name

The patch actually makes it worse. Now even the 'nospacedir' case fails
the same way (steps 6 and 7 in my recipe).

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Eli Zaretskii]

> From: Peter Povinec <spepo.42@gmail.com>
> Date: Tue, 19 Apr 2022 10:06:28 -0700
> Cc: 55016@debbugs.gnu.org
> 
> > I'm running half-blind here, because you didn't tell which Xref
> > backend is being used for this (do you have ID Utils or GNU Global or
> > Cscope installed and used for the above recipe?).  My guess is that
> 
> Sorry, should have mentioned that the recipe is with 'emacs -Q',
> no special backends.

The backend is independent on your customizations, it depends on the
tools you use in the tree.  For example, if Emacs finds a file named
"ID", it assumes you use ID Utils.

> > Emacs uses the default "find | grep" to do the search, in which case
> > the patch below should help; does it?
> >
> > diff --git a/lisp/cedet/semantic/symref/grep.el b/lisp/cedet/semantic/symref/grep.el
> > index 27ea80f..bc96505 100644
> > --- a/lisp/cedet/semantic/symref/grep.el
> > +++ b/lisp/cedet/semantic/symref/grep.el
> > @@ -124,7 +124,7 @@ semantic-symref-grep-use-template
> >                   grep-find-template)
> >                 pattern
> >                 filepattern
> > -               rootdir)))
> > +               (shell-quote-argument rootdir))))
> >      cmd))
> >
> >  (defcustom semantic-symref-grep-shell shell-file-name
> 
> The patch actually makes it worse. Now even the 'nospacedir' case fails
> the same way (steps 6 and 7 in my recipe).

I guess I will then have to ask you to show the find/grep command we
are generating in that function, before the patch.  (You could do that
by stepping through the code with Edebug.)  It is hard for me to
simulate your recipe because it requires symlinks, and without that I
can only guess what's going on there.

My guess so far was that we produce a command that looks something
like

   find <DIRECTORY> ... | grep ...

in which case quoting <DIRECTORY> should fix your problem.  But I'm
probably missing something.

Thanks.

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Eli Zaretskii]

> From: Peter Povinec <spepo.42@gmail.com>
> Date: Tue, 19 Apr 2022 10:06:28 -0700
> Cc: 55016@debbugs.gnu.org
> 
> The patch actually makes it worse. Now even the 'nospacedir' case fails
> the same way (steps 6 and 7 in my recipe).

Actually, one more idea: perhaps the quoting makes things first
because your file names begin with "~/"?  In that case, doing
something like this instead should do better:

   (shell-quote-argument (expand-file-name rootdir))

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Peter Povinec]

On Tue, Apr 19, 2022 at 10:37 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> Actually, one more idea: perhaps the quoting makes things first
> because your file names begin with "~/"?  In that case, doing
> something like this instead should do better:
>
>    (shell-quote-argument (expand-file-name rootdir))

That works! Both 'space dir' and 'nospace' dir cases work the same way now.

BTW, the symlinks are not essential to my recipe. You can instead
create two new
directories and copy in xref.el in Step 1.

Thanks for your help and super fast response.
Peter

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Eli Zaretskii]

> From: Peter Povinec <spepo.42@gmail.com>
> Date: Tue, 19 Apr 2022 10:57:29 -0700
> Cc: 55016@debbugs.gnu.org
> 
> On Tue, Apr 19, 2022 at 10:37 AM Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > Actually, one more idea: perhaps the quoting makes things first
> > because your file names begin with "~/"?  In that case, doing
> > something like this instead should do better:
> >
> >    (shell-quote-argument (expand-file-name rootdir))
> 
> That works! Both 'space dir' and 'nospace' dir cases work the same way now.

OK, thanks.  Now I understand what's going on, and can work on a real
fix for Emacs 29.

> BTW, the symlinks are not essential to my recipe. You can instead
> create two new
> directories and copy in xref.el in Step 1.

That has other complications, like the command asks me for a project
etc.

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Eli Zaretskii]

> Date: Tue, 19 Apr 2022 21:17:03 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 55016@debbugs.gnu.org
> 
> > > Actually, one more idea: perhaps the quoting makes things first
> > > because your file names begin with "~/"?  In that case, doing
> > > something like this instead should do better:
> > >
> > >    (shell-quote-argument (expand-file-name rootdir))
> > 
> > That works! Both 'space dir' and 'nospace' dir cases work the same way now.
> 
> OK, thanks.  Now I understand what's going on, and can work on a real
> fix for Emacs 29.

Dmitry, there's something here I don't understand.  In
semantic-symref-perform-search method that uses find/grep, we do this:

    (with-current-buffer b
      (erase-buffer)
      (setq default-directory rootdir)
      (let ((cmd (semantic-symref-grep-use-template
                  (directory-file-name (file-local-name rootdir))
                  filepattern grepflags greppat)))
        (process-file semantic-symref-grep-shell nil b nil
                      shell-command-switch cmd)))

Since we bind default-directory to ROOTDIR, why do we also need to
pass ROOTDIR to semantic-symref-grep-use-template?  Why not use ".",
or even nil (which gets expanded to "." AFAIU)?  Then this entire
issue with embedded blanks in ROOTDIR would not have happened, because
the problematic directory name would not be exposed to the shell.

What am I missing here?

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Dmitry Gutov]

Hi Eli,

Sorry for the slow reply.

On 19.04.2022 21:24, Eli Zaretskii wrote:
> Dmitry, there's something here I don't understand.  In
> semantic-symref-perform-search method that uses find/grep, we do this:
> 
>      (with-current-buffer b
>        (erase-buffer)
>        (setq default-directory rootdir)
>        (let ((cmd (semantic-symref-grep-use-template
>                    (directory-file-name (file-local-name rootdir))
>                    filepattern grepflags greppat)))
>          (process-file semantic-symref-grep-shell nil b nil
>                        shell-command-switch cmd)))
> 
> Since we bind default-directory to ROOTDIR, why do we also need to
> pass ROOTDIR to semantic-symref-grep-use-template?  Why not use ".",
> or even nil (which gets expanded to "." AFAIU)?  Then this entire
> issue with embedded blanks in ROOTDIR would not have happened, because
> the problematic directory name would not be exposed to the shell.
> 
> What am I missing here?

This approach dates back to before CEDET was added.

But I imagine the logic was similar to what I used in: 
xref-matches-in-directory that it's easier to handle absolute file names 
which Grep outputs this way, rather that concatenate them later.

Nowadays, though, that function has come full circle with in 
71f8b55f46a, for various reasons, including macOS having a very old 
version of 'find'. Note that we fixed this particular bug in ab3ba912fc7.

symref/grep.el doesn't use ignore instructions, though, so it can easily 
use either approach.

Due to how semantic-symref-* defmethods are currently structured, 
though, the current xref-matches-in-directory's approach seems like more 
of a pain: semantic-symref-parse-tool-output-one-line cannot use lexical 
context from semantic-symref-perform-search (where we would bind a 
local-dir variable once to subsequently use when parsing every line). A 
dynamic var seems to work, though.

With we could avoid having to use 'substring'. That should lead to a 
little less consing. No idea how, though.

(Should this also use 'file-name-unquote'?)

diff --git a/lisp/cedet/semantic/symref/grep.el 
b/lisp/cedet/semantic/symref/grep.el
index 27ea80fc32..025faf1042 100644
--- a/lisp/cedet/semantic/symref/grep.el
+++ b/lisp/cedet/semantic/symref/grep.el
@@ -139,6 +139,8 @@ semantic-symref-grep--quote-grep
                              (lambda (s) (concat "\\" s))
                              string nil t))

+(defvar semantic-symref-grep-local-dir nil)
+
  (cl-defmethod semantic-symref-perform-search ((tool 
semantic-symref-tool-grep))
    "Perform a search with Grep."
    ;; Grep doesn't support some types of searches.
@@ -170,11 +172,12 @@ semantic-symref-perform-search
        (erase-buffer)
        (setq default-directory rootdir)
        (let ((cmd (semantic-symref-grep-use-template
-                  (directory-file-name (file-local-name rootdir))
+                  "."
                    filepattern grepflags greppat)))
          (process-file semantic-symref-grep-shell nil b nil
                        shell-command-switch cmd)))
-    (setq ans (semantic-symref-parse-tool-output tool b))
+    (let ((semantic-symref-grep-local-dir (directory-file-name 
(file-local-name rootdir))))
+      (setq ans (semantic-symref-parse-tool-output tool b)))
      ;; Return the answer
      ans))

@@ -190,12 +193,12 @@ semantic-symref-parse-tool-output-one-line
            ((eq (oref tool resulttype) 'line-and-text)
             (when (re-search-forward grep-re nil t)
               (list (string-to-number (match-string line-group))
-                   (match-string file-group)
+                   (concat semantic-symref-grep-local-dir (substring 
(match-string file-group) 1))
                     (buffer-substring-no-properties (point) 
(line-end-position)))))
  	  (t
  	   (when (re-search-forward grep-re nil t)
  	     (cons (string-to-number (match-string line-group))
-		   (match-string file-group))
+		   (concat semantic-symref-grep-local-dir (substring (match-string 
file-group) 1)))
  	     )))))

  (provide 'semantic/symref/grep)

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Eli Zaretskii]

> Date: Sun, 24 Apr 2022 05:00:09 +0300
> Cc: 55016@debbugs.gnu.org, spepo.42@gmail.com
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> Hi Eli,
> 
> Sorry for the slow reply.

No sweat.

> Nowadays, though, that function has come full circle with in 
> 71f8b55f46a, for various reasons, including macOS having a very old 
> version of 'find'. Note that we fixed this particular bug in ab3ba912fc7.
> 
> symref/grep.el doesn't use ignore instructions, though, so it can easily 
> use either approach.
> 
> Due to how semantic-symref-* defmethods are currently structured, 
> though, the current xref-matches-in-directory's approach seems like more 
> of a pain: semantic-symref-parse-tool-output-one-line cannot use lexical 
> context from semantic-symref-perform-search (where we would bind a 
> local-dir variable once to subsequently use when parsing every line). A 
> dynamic var seems to work, though.
> 
> With we could avoid having to use 'substring'. That should lead to a 
> little less consing. No idea how, though.

Thanks.  Peter, can you try the patch proposed by Dmitry and see if it
works in your case?

> (Should this also use 'file-name-unquote'?)

I'm not sure I follow: where would we use that and for what purpose?

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Dmitry Gutov]

On 24.04.2022 08:31, Eli Zaretskii wrote:
>> (Should this also use 'file-name-unquote'?)
> I'm not sure I follow: where would we use that and for what purpose?

xref-matches-in-directory uses it since the discussion in bug#47799.

To... obtain a local directory name which would be understood by the 
shell, I guess?

It should make sense for symref/grep.el to also do it, but if it should, 
I'm surprised this kind of problem has never been reported. bug#47799 is 
not very new, after all.

Philipp? Does xref-find-references work fine for you inside directories 
that are supposed to be quoted? I'm not sure how this works, TBH, e.g. 
where the quoted file names usually come from.

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Eli Zaretskii]

> Date: Mon, 25 Apr 2022 05:08:56 +0300
> Cc: 55016@debbugs.gnu.org, spepo.42@gmail.com,
>  Philipp Stephani <p.stephani2@gmail.com>
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> On 24.04.2022 08:31, Eli Zaretskii wrote:
> >> (Should this also use 'file-name-unquote'?)
> > I'm not sure I follow: where would we use that and for what purpose?
> 
> xref-matches-in-directory uses it since the discussion in bug#47799.
> 
> To... obtain a local directory name which would be understood by the 
> shell, I guess?

Ah, okay.  But, after the changes you posted, which file names may
need unquoting, and why?  The whole point of the changes is not to use
any file names literally in the command line passed to the shell.  And
our low-level primitives that invoke the shell already know to unquote
file names, so this should "just work", no?

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Peter Povinec]

On Sat, Apr 23, 2022 at 10:32 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> Thanks.  Peter, can you try the patch proposed by Dmitry and see if it
> works in your case?

I am having a hard time applying that patch. First, it has long lines that
have been wrapped at 80.  Second, even after manually fixing those I get
this error from git apply:
error: patch fragment without header at line 29: @@ -190,10 +193,10 @@
semantic-symref-parse-tool-output-one-line

Is it just me, or does it fail for you too?
What is the best way to apply such a patch to my local sources?

Thanks,
Peter

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Dmitry Gutov]

On 25.04.2022 14:48, Eli Zaretskii wrote:
> Ah, okay.  But, after the changes you posted, which file names may
> need unquoting, and why?  The whole point of the changes is not to use
> any file names literally in the command line passed to the shell.  And
> our low-level primitives that invoke the shell already know to unquote
> file names, so this should "just work", no?

All right, seems so. The difference with local-dir in 
xref-matches-in-directory, is that in that function the value did get 
inserted into a shell command string.

At least it did before 71f8b55f46a. Now that it doesn't, I suppose it 
doesn't need to unquote either. But then again, the previous version of 
the code didn't do any "requoting" of the file names returned in the 
Find+Grep output (if they are supposed to be requoted, that is).

Nor does xref-matches-in-files do any "requoting". And this function is 
frequently-used, so any problems with that approach should have already 
come up.

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Dmitry Gutov]

[-- Attachment #1: Type: text/plain, Size: 493 bytes --]

On 26.04.2022 04:05, Peter Povinec wrote:
> Is it just me, or does it fail for you too?
> What is the best way to apply such a patch to my local sources?

I haven't tried, but 'C-c C-a' in diff-mode usually helps with this kind 
of issues. So you save the patch from the email into a file with 
extension .diff, visit in in Emacs, and then press the above key sequence.

Anyway, here's the patch in attachment which should be much easier to 
apply either way. It also contains a tiny renaming.

[-- Attachment #2: semantic-symref-grep--local-dir.diff --]
[-- Type: text/x-patch, Size: 1995 bytes --]

diff --git a/lisp/cedet/semantic/symref/grep.el b/lisp/cedet/semantic/symref/grep.el
index 27ea80fc32..076775bfec 100644
--- a/lisp/cedet/semantic/symref/grep.el
+++ b/lisp/cedet/semantic/symref/grep.el
@@ -139,6 +139,8 @@ semantic-symref-grep--quote-grep
                             (lambda (s) (concat "\\" s))
                             string nil t))
 
+(defvar semantic-symref-grep--local-dir nil)
+
 (cl-defmethod semantic-symref-perform-search ((tool semantic-symref-tool-grep))
   "Perform a search with Grep."
   ;; Grep doesn't support some types of searches.
@@ -170,11 +172,12 @@ semantic-symref-perform-search
       (erase-buffer)
       (setq default-directory rootdir)
       (let ((cmd (semantic-symref-grep-use-template
-                  (directory-file-name (file-local-name rootdir))
+                  "."
                   filepattern grepflags greppat)))
         (process-file semantic-symref-grep-shell nil b nil
                       shell-command-switch cmd)))
-    (setq ans (semantic-symref-parse-tool-output tool b))
+    (let ((semantic-symref-grep--local-dir (directory-file-name (file-local-name rootdir))))
+      (setq ans (semantic-symref-parse-tool-output tool b)))
     ;; Return the answer
     ans))
 
@@ -190,12 +193,12 @@ semantic-symref-parse-tool-output-one-line
           ((eq (oref tool resulttype) 'line-and-text)
            (when (re-search-forward grep-re nil t)
              (list (string-to-number (match-string line-group))
-                   (match-string file-group)
+                   (concat semantic-symref-grep--local-dir (substring (match-string file-group) 1))
                    (buffer-substring-no-properties (point) (line-end-position)))))
 	  (t
 	   (when (re-search-forward grep-re nil t)
 	     (cons (string-to-number (match-string line-group))
-		   (match-string file-group))
+		   (concat semantic-symref-grep--local-dir (substring (match-string file-group) 1)))
 	     )))))
 
 (provide 'semantic/symref/grep)

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Peter Povinec]

On Mon, Apr 25, 2022 at 7:07 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
>
> Anyway, here's the patch in attachment which should be much easier to
> apply either way. It also contains a tiny renaming.

Indeed. It applied successfully, thanks for that.

I can confirm that the patch fixes the bug. However, I noticed a subtle
change in behavior: With the patch, the xref buffer shows the file names
with '~' in them, whereas before the patch, the '~' was resolved to
'/Users/username'. Functionally, it doesn't seem to make a difference
though -- at least as far as I can tell.

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Eli Zaretskii]

> From: Peter Povinec <spepo.42@gmail.com>
> Date: Mon, 25 Apr 2022 21:57:59 -0700
> Cc: Eli Zaretskii <eliz@gnu.org>, 55016@debbugs.gnu.org
> 
> On Mon, Apr 25, 2022 at 7:07 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
> >
> > Anyway, here's the patch in attachment which should be much easier to
> > apply either way. It also contains a tiny renaming.
> 
> Indeed. It applied successfully, thanks for that.
> 
> I can confirm that the patch fixes the bug. However, I noticed a subtle
> change in behavior: With the patch, the xref buffer shows the file names
> with '~' in them, whereas before the patch, the '~' was resolved to
> '/Users/username'. Functionally, it doesn't seem to make a difference
> though -- at least as far as I can tell.

Thanks for testing.

We could use abbreviate-file-name, perhaps.  But I'm not sure the
change you describe is not for the better, since absolute file names
are absolutely understandable and interpreted correctly in all
situations, whereas file names that begin with "~/" need to be
interpreted either by Emacs or by a reasonably functional shell before
they can be safely passed to any other program.

I'm curious what does Dmitry think about this consequence of the
change.

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Eli Zaretskii]

> Date: Tue, 26 Apr 2022 05:05:18 +0300
> Cc: 55016@debbugs.gnu.org, p.stephani2@gmail.com, spepo.42@gmail.com
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> On 25.04.2022 14:48, Eli Zaretskii wrote:
> > Ah, okay.  But, after the changes you posted, which file names may
> > need unquoting, and why?  The whole point of the changes is not to use
> > any file names literally in the command line passed to the shell.  And
> > our low-level primitives that invoke the shell already know to unquote
> > file names, so this should "just work", no?
> 
> All right, seems so. The difference with local-dir in 
> xref-matches-in-directory, is that in that function the value did get 
> inserted into a shell command string.
> 
> At least it did before 71f8b55f46a. Now that it doesn't, I suppose it 
> doesn't need to unquote either. But then again, the previous version of 
> the code didn't do any "requoting" of the file names returned in the 
> Find+Grep output (if they are supposed to be requoted, that is).
> 
> Nor does xref-matches-in-files do any "requoting". And this function is 
> frequently-used, so any problems with that approach should have already 
> come up.

If we just show those file names in some window, they don't need to be
re-quoted, I think.  Quoting is only needed if the resulting file
names will be passed to Lisp APIs to act on them.

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Dmitry Gutov]

On 26.04.2022 15:00, Eli Zaretskii wrote:
> If we just show those file names in some window, they don't need to be
> re-quoted, I think.  Quoting is only needed if the resulting file
> names will be passed to Lisp APIs to act on them.

They are not just shown. The file names are used when the user tries to 
visit the reference (e.g. with RET).

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Dmitry Gutov]

On 26.04.2022 14:18, Eli Zaretskii wrote:
> I'm curious what does Dmitry think about this consequence of the
> change.

I think Peter is saying that the patch made the file names displayed in 
the abbreviated form, not vice versa.

Which seems like a good change (more compact display).

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Eli Zaretskii]

> Date: Tue, 26 Apr 2022 15:28:57 +0300
> Cc: 55016@debbugs.gnu.org, p.stephani2@gmail.com, spepo.42@gmail.com
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> On 26.04.2022 15:00, Eli Zaretskii wrote:
> > If we just show those file names in some window, they don't need to be
> > re-quoted, I think.  Quoting is only needed if the resulting file
> > names will be passed to Lisp APIs to act on them.
> 
> They are not just shown. The file names are used when the user tries to 
> visit the reference (e.g. with RET).

Which command is invoked by RET in those cases?

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Dmitry Gutov]

On 26.04.2022 15:36, Eli Zaretskii wrote:
>> Date: Tue, 26 Apr 2022 15:28:57 +0300
>> Cc:55016@debbugs.gnu.org,p.stephani2@gmail.com,spepo.42@gmail.com
>> From: Dmitry Gutov<dgutov@yandex.ru>
>>
>> On 26.04.2022 15:00, Eli Zaretskii wrote:
>>> If we just show those file names in some window, they don't need to be
>>> re-quoted, I think.  Quoting is only needed if the resulting file
>>> names will be passed to Lisp APIs to act on them.
>> They are not just shown. The file names are used when the user tries to
>> visit the reference (e.g. with RET).
> Which command is invoked by RET in those cases?

xref-goto-xref

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Peter Povinec]

On Tue, Apr 26, 2022 at 5:30 AM Dmitry Gutov <dgutov@yandex.ru> wrote:
>
> On 26.04.2022 14:18, Eli Zaretskii wrote:
> > I'm curious what does Dmitry think about this consequence of the
> > change.
>
> I think Peter is saying that the patch made the file names displayed in
> the abbreviated form, not vice versa.
>
> Which seems like a good change (more compact display).

That's right, with the patch, the filenames start with "~/".

I actually like that change too, but I am curious if there is an
Emacs wide design guideline on such a thing.
It seems that the behavior varies from place to place.
E.g. when I
'C-x C-f' /Users/spepo42/test.txt
it shows up as  "~/test.txt" in the buffer list.
On the other hand, when I
'C-x C-f' ~/
dired tells me in the header line it is looking at
/Users/spepo42, but shows "~/" in the buffer list...

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Eli Zaretskii]

> Date: Wed, 27 Apr 2022 04:57:14 +0300
> Cc: 55016@debbugs.gnu.org, p.stephani2@gmail.com, spepo.42@gmail.com
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> On 26.04.2022 15:36, Eli Zaretskii wrote:
> >> Date: Tue, 26 Apr 2022 15:28:57 +0300
> >> Cc:55016@debbugs.gnu.org,p.stephani2@gmail.com,spepo.42@gmail.com
> >> From: Dmitry Gutov<dgutov@yandex.ru>
> >>
> >> On 26.04.2022 15:00, Eli Zaretskii wrote:
> >>> If we just show those file names in some window, they don't need to be
> >>> re-quoted, I think.  Quoting is only needed if the resulting file
> >>> names will be passed to Lisp APIs to act on them.
> >> They are not just shown. The file names are used when the user tries to
> >> visit the reference (e.g. with RET).
> > Which command is invoked by RET in those cases?
> 
> xref-goto-xref

Thanks.  AFAIR, that command shouldn't have problems with file names
that in other places need quoting.  Of course, actually testing with
such a file name would be best, so if someone can come up with a test
case, we could be sure.

bug#55016: 28.1; xref-find-references finds no matches if project dir contains a space

[Dmitry Gutov]

Hi again,

On 27.04.2022 06:00, Peter Povinec wrote:
> On Tue, Apr 26, 2022 at 5:30 AM Dmitry Gutov <dgutov@yandex.ru> wrote:
>>
>> On 26.04.2022 14:18, Eli Zaretskii wrote:
>>> I'm curious what does Dmitry think about this consequence of the
>>> change.
>>
>> I think Peter is saying that the patch made the file names displayed in
>> the abbreviated form, not vice versa.
>>
>> Which seems like a good change (more compact display).
> 
> That's right, with the patch, the filenames start with "~/".
> 
> I actually like that change too, but I am curious if there is an
> Emacs wide design guideline on such a thing.
> It seems that the behavior varies from place to place.
> E.g. when I
> 'C-x C-f' /Users/spepo42/test.txt
> it shows up as  "~/test.txt" in the buffer list.
> On the other hand, when I
> 'C-x C-f' ~/
> dired tells me in the header line it is looking at
> /Users/spepo42, but shows "~/" in the buffer list...

Sorry about the wait. I've pushed the patch now in commit a691e811e2, to 
get it in time for the next release.

Regarding a guideline, not sure if we had one (though it sounds good), 
but I think the only times where it would matter, is when a directory 
name is repeated multiple times (e.g. Compilation buffer).

And in places line a header line where it's only printed once, it 
doesn't matter as much, but can can show the expanded version, to make 
it doubly clear.