bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

[Philip Kaludercic]

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


When invoking a command that respects xref-search-program via TRAMP,
e.g. on a remote system that doesn't have (in my case ripgrep)
installed, an error is signalled indicating that the search query
couldn't be executed.

I managed to temporarily circumvent the issue with this patch


[-- Attachment #2: Type: text/plain, Size: 618 bytes --]

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index c812f28c1b..92c3d5c9d5 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -794,6 +794,11 @@ project-find-regexp
   (let* ((caller-dir default-directory)
          (pr (project-current t))
          (default-directory (project-root pr))
+         (xref-search-program
+          (if (and (eq xref-search-program 'ripgrep)
+                   (not (executable-find "rg" t)))
+              'grep
+              xref-search-program))
          (files
           (if (not current-prefix-arg)
               (project-files pr)

[-- Attachment #3: Type: text/plain, Size: 7492 bytes --]


but just assuming that grep is available might just push the actual
problem aside.  Should xref-search-program be able to indicate a
failback, and perhaps even eventually fall back onto a elisp grep that
might be slow but at least would do the job?

In GNU Emacs 29.0.50 (build 4, x86_64-pc-linux-gnu, X toolkit, cairo version 1.16.0, Xaw scroll bars)
 of 2022-01-21 built on icterid
Repository revision: 98355833ba0d7dc20742122334be1bfaefac7873
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Debian GNU/Linux 11 (bullseye)

Configured using:
 'configure --with-x-toolkit=athena --with-native-compilation
 'CFLAGS=-Os -march=native -mtune=native -pipe' LDFLAGS=-flto'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES
NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3
THREADS TIFF TOOLKIT_SCROLL_BARS WEBP X11 XDBE XIM XPM LUCID ZLIB

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

Major mode: ELisp/l

Minor modes in effect:
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  auto-revert-mode: t
  shell-dirtrack-mode: t
  bug-reference-prog-mode: t
  rcirc-track-minor-mode: t
  outline-minor-mode: t
  corfu-mode: t
  flymake-mode: t
  flyspell-mode: t
  recentf-mode: t
  repeat-mode: t
  display-time-mode: t
  diff-hl-flydiff-mode: t
  diff-hl-mode: t
  winner-mode: t
  windmove-mode: t
  vertico-multiform-mode: t
  vertico-mode: t
  electric-pair-mode: t
  save-place-mode: t
  savehist-mode: t
  xterm-mouse-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tab-bar-mode: t
  file-name-shadow-mode: t
  context-menu-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  temp-buffer-resize-mode: t
  line-number-mode: t
  transient-mark-mode: t
  auto-save-visited-mode: t

Load-path shadows:
/home/philip/.config/emacs/site-lisp/eglot/eglot hides /home/philip/.config/emacs/elpa/eglot-1.8/eglot
/home/philip/.config/emacs/site-lisp/eglot/eglot-tests hides /home/philip/.config/emacs/elpa/eglot-1.8/eglot-tests
/home/philip/.config/emacs/site-lisp/vc-backup/vc-backup hides /home/philip/.config/emacs/elpa/vc-backup-1.1.0/vc-backup
/home/philip/.config/emacs/elpa/transient-0.3.7/transient hides /home/philip/Source/emacs/lisp/transient
~/.config/emacs/site-lisp/autoload hides /home/philip/Source/emacs/lisp/emacs-lisp/autoload

Features:
(shadow emacsbug etags fileloop generator eglot array jsonrpc ert
flymake-shellcheck sh-script smie executable perl-mode vc-mtn vc-hg
vc-bzr vc-src vc-sccs vc-svn vc-cvs vc-rcs grep flymake-cc
yasnippet-snippets yasnippet cc-mode cc-fonts cc-guess cc-menus cc-cmds
cc-styles cc-align cc-engine cc-vars cc-defs debug shell-command+
dired-aux gnus-dired org ob ob-tangle ob-ref ob-lob ob-table ob-exp
org-macro org-footnote org-src ob-comint org-pcomplete org-list
org-faces org-entities org-version ob-emacs-lisp ob-core ob-eval
org-table oc-basic bibtex ol org-keys oc org-compat advice org-macs
org-loaddefs cal-menu calendar cal-loaddefs debbugs-gnu debbugs
soap-client rng-xsd rng-dt rng-util xsd-regexp tar-mode arc-mode
archive-mode url-http url-gw url-cache url-auth cus-edit pp cus-start
finder-inf cl-print edebug backtrace whitespace vc-annotate qp
magit-extras face-remap magit-submodule magit-obsolete magit-blame
magit-stash magit-reflog magit-bisect magit-push magit-pull magit-fetch
magit-clone magit-remote magit-commit magit-sequence magit-notes
magit-worktree magit-tag magit-merge magit-branch magit-reset
magit-files magit-refs magit-status magit magit-repos magit-apply
magit-wip magit-log which-func imenu magit-diff smerge-mode git-commit
log-edit add-log magit-core magit-autorevert autorevert filenotify
magit-margin magit-transient magit-process with-editor shell pcomplete
server magit-mode transient edmacro kmacro format-spec magit-git
magit-section magit-utils crm dash mailalias smtpmail avy mule-util
char-fold misearch multi-isearch pulse color xref vc-git bug-reference
find-func help-fns autocrypt-message copyright vc-backup log-view
pcvs-util time-stamp sort smiley gnus-cite mm-archive mail-extr textsec
uni-scripts idna-mapping ucs-normalize uni-confusable textsec-check
gnus-async gnus-bcklg gnus-ml autocrypt-gnus autocrypt nndraft nnmh
utf-7 nnfolder gnus-agent gnus-srvr gnus-score score-mode nnvirtual
gnus-msg gnus-art mm-uu mml2015 mm-view mml-smime smime dig nntp
gnus-cache gnus-sum shr pixel-fill kinsoku svg dom gnus-group gnus-undo
gnus-start gnus-dbus dbus xml gnus-cloud nnimap nnmail mail-source utf7
netrc nnoo gnus-spec gnus-int gnus-range message yank-media rfc822 mml
mml-sec mm-decode mm-bodies mm-encode mailabbrev gmm-utils mailheader
gnus-win modus-vivendi-theme gnutls network-stream puny nsm rmc epa-file
epa epg rfc6068 epg-config rcirc parse-time iso8601 vertico-directory
vertico-flat noutline outline corfu checkdoc flymake-proc flymake
derived project thingatpt flyspell ispell comp comp-cstr warnings
cl-extra auth-source-pass recentf tree-widget repeat dired-x dired
dired-loaddefs time sendmail gnus nnheader gnus-util time-date
mail-utils range wid-edit help-at-pt diff-hl-flydiff diff diff-hl vc-dir
ewoc vc vc-dispatcher diff-mode hippie-exp winner windmove rx
vertico-multiform vertico easy-mmode pcase elec-pair saveplace savehist
xt-mouse modus-operandi-theme modus-themes rot13 disp-table icomplete
cus-load setup help-mode compile text-property-search comint ansi-color
ring autoload radix-tree lisp-mnt mail-parse rfc2231 rfc2047 rfc2045
mm-util ietf-drums mail-prsvr slime-autoloads info package browse-url
url url-proxy url-privacy url-expand url-methods url-history url-cookie
url-domsuf url-util mailcap url-handlers url-parse auth-source cl-seq
eieio eieio-core cl-macs eieio-loaddefs password-cache json map url-vars
seq gv subr-x byte-opt bytecomp byte-compile cconv cl-loaddefs cl-lib
iso-transl tooltip eldoc paren electric uniquify ediff-hook vc-hooks
lisp-float-type elisp-mode mwheel term/x-win x-win term/common-win x-dnd
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 keymap hashtable-print-readable backquote threads
dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting cairo x-toolkit x multi-tty make-network-process
native-compile emacs)

Memory information:
((conses 16 1126377 131479)
 (symbols 48 44180 90)
 (strings 32 188808 12506)
 (string-bytes 1 5609582)
 (vectors 16 107028)
 (vector-slots 8 2610956 134057)
 (floats 8 793 733)
 (intervals 56 49401 625)
 (buffers 992 94))

-- 
	Philip Kaludercic

bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

[Dmitry Gutov]

Hi Philip,

On 31.01.2022 01:38, Philip Kaludercic wrote:
> When invoking a command that respects xref-search-program via TRAMP,
> e.g. on a remote system that doesn't have (in my case ripgrep)
> installed, an error is signalled indicating that the search query
> couldn't be executed.

One way to work around this will probably involve an addition to 
find-file-hook and some code which checks (file-remote-p 
buffer-file-name) and sets xref-search-program to a particular value 
buffer-locally depending on the result.

Or an around-advice for xref-matches-in-files.

> I managed to temporarily circumvent the issue with this patch
> 
> 
> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
> index c812f28c1b..92c3d5c9d5 100644
> --- a/lisp/progmodes/project.el
> +++ b/lisp/progmodes/project.el
> @@ -794,6 +794,11 @@ project-find-regexp
>     (let* ((caller-dir default-directory)
>            (pr (project-current t))
>            (default-directory (project-root pr))
> +         (xref-search-program
> +          (if (and (eq xref-search-program 'ripgrep)
> +                   (not (executable-find "rg" t)))
> +              'grep
> +              xref-search-program))
>            (files
>             (if (not current-prefix-arg)
>                 (project-files pr)
> 
> 
> but just assuming that grep is available might just push the actual
> problem aside.  Should xref-search-program be able to indicate a
> failback, and perhaps even eventually fall back onto a elisp grep that
> might be slow but at least would do the job?

This seems like it will have to call 'executable-find' every time the 
command is used on a remote host, and I imagine that's not free. 
Especially with high network lag. For fast searches (and distant/slow 
hosts) it might almost double the execution time.

A proper solution would probably look more similar to 
grep-host-defaults-alist and grep-compute-defaults.

I'm not sure whether xref should grow a separate facility like that, 
though, or whether xref-search-program can become more assimilated into 
grep.el instead.

bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Hi Philip,

Hi Dmitry,

>> When invoking a command that respects xref-search-program via TRAMP,
>> e.g. on a remote system that doesn't have (in my case ripgrep)
>> installed, an error is signalled indicating that the search query
>> couldn't be executed.
>
> One way to work around this will probably involve an addition to
> find-file-hook and some code which checks (file-remote-p
> buffer-file-name) and sets xref-search-program to a particular value
> buffer-locally depending on the result.
>
> Or an around-advice for xref-matches-in-files.

There are connection-local variables exactly for this use case.

> A proper solution would probably look more similar to
> grep-host-defaults-alist and grep-compute-defaults.

On my wishlist, there is moving grep-*-defaults to connection-local
variables. But I fear it will break too many existing configurations.

Best regards, Michael.

bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

[Dmitry Gutov]

Hi Michael,

On 04.02.2022 10:15, Michael Albinus wrote:

>>> When invoking a command that respects xref-search-program via TRAMP,
>>> e.g. on a remote system that doesn't have (in my case ripgrep)
>>> installed, an error is signalled indicating that the search query
>>> couldn't be executed.
>>
>> One way to work around this will probably involve an addition to
>> find-file-hook and some code which checks (file-remote-p
>> buffer-file-name) and sets xref-search-program to a particular value
>> buffer-locally depending on the result.
>>
>> Or an around-advice for xref-matches-in-files.
> 
> There are connection-local variables exactly for this use case.

Is there a documented way on how to make the variable's value on remote 
hosts customizable for the user too?

Or maybe it's not exactly necessary for this custom var.

>> A proper solution would probably look more similar to
>> grep-host-defaults-alist and grep-compute-defaults.
> 
> On my wishlist, there is moving grep-*-defaults to connection-local
> variables. But I fear it will break too many existing configurations.
> 
> Best regards, Michael.

Cheers,
Dmitry.

bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Hi Michael,

Hi Dmitry,

>>>> When invoking a command that respects xref-search-program via TRAMP,
>>>> e.g. on a remote system that doesn't have (in my case ripgrep)
>>>> installed, an error is signalled indicating that the search query
>>>> couldn't be executed.
>>>
>>> One way to work around this will probably involve an addition to
>>> find-file-hook and some code which checks (file-remote-p
>>> buffer-file-name) and sets xref-search-program to a particular value
>>> buffer-locally depending on the result.
>>>
>>> Or an around-advice for xref-matches-in-files.
>> There are connection-local variables exactly for this use case.
>
> Is there a documented way on how to make the variable's value on
> remote hosts customizable for the user too?

Something like

--8<---------------cut here---------------start------------->8---
(connection-local-set-profile-variables
 'remote-xref-variables
 '((xref-search-program . "/bin/grep")))

(with-eval-after-load 'xref
  (connection-local-set-profiles
   '(:application tramp :machine "myhost")
   'remote-xref-variables))
--8<---------------cut here---------------end--------------->8---

> Cheers,
> Dmitry.

Best regards, Michael.

bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

[Dmitry Gutov]

On 05.02.2022 16:38, Michael Albinus wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
>> Hi Michael,
> 
> Hi Dmitry,
> 
>>>>> When invoking a command that respects xref-search-program via TRAMP,
>>>>> e.g. on a remote system that doesn't have (in my case ripgrep)
>>>>> installed, an error is signalled indicating that the search query
>>>>> couldn't be executed.
>>>>
>>>> One way to work around this will probably involve an addition to
>>>> find-file-hook and some code which checks (file-remote-p
>>>> buffer-file-name) and sets xref-search-program to a particular value
>>>> buffer-locally depending on the result.
>>>>
>>>> Or an around-advice for xref-matches-in-files.
>>> There are connection-local variables exactly for this use case.
>>
>> Is there a documented way on how to make the variable's value on
>> remote hosts customizable for the user too?
> 
> Something like
> 
> --8<---------------cut here---------------start------------->8---
> (connection-local-set-profile-variables
>   'remote-xref-variables
>   '((xref-search-program . "/bin/grep")))
> 
> (with-eval-after-load 'xref
>    (connection-local-set-profiles
>     '(:application tramp :machine "myhost")
>     'remote-xref-variables))
> --8<---------------cut here---------------end--------------->8---

Nice. Some integration with the Customize UI probably wouldn't hurt, though.

Philip, would you like to try writing a patch along the lines of 
Michael's suggestion?

It would need to use version checks, though, given that connection-local 
vars were only added in Emacs 27.

To get you started, here's one example of this feature's usage: 
https://github.com/company-mode/company-mode/blob/c25f1fbc3850e36e6521b77fa1641d5583365d8b/company-gtags.el#L71-L96

And you can search in Emacs's own repository, of course, for other examples.

bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

Hi Dmitry,

> Nice. Some integration with the Customize UI probably wouldn't hurt, though.

Good idea. I'll see whether I can assemble something useful.

Best regards, Michael.

bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

[Michael Albinus]

Michael Albinus <michael.albinus@gmx.de> writes:

Hi Dmitry,

>> Nice. Some integration with the Customize UI probably wouldn't hurt, though.
>
> Good idea. I'll see whether I can assemble something useful.

I've converted connection-local-profile-alist and
connection-local-criteria-alist to user options. This should help a
little bit.

Best regards, Michael.

bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

[Philip Kaludercic]

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

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 05.02.2022 16:38, Michael Albinus wrote:
>> Dmitry Gutov <dgutov@yandex.ru> writes:
>> 
>>> Hi Michael,
>> Hi Dmitry,
>> 
>>>>>> When invoking a command that respects xref-search-program via TRAMP,
>>>>>> e.g. on a remote system that doesn't have (in my case ripgrep)
>>>>>> installed, an error is signalled indicating that the search query
>>>>>> couldn't be executed.
>>>>>
>>>>> One way to work around this will probably involve an addition to
>>>>> find-file-hook and some code which checks (file-remote-p
>>>>> buffer-file-name) and sets xref-search-program to a particular value
>>>>> buffer-locally depending on the result.
>>>>>
>>>>> Or an around-advice for xref-matches-in-files.
>>>> There are connection-local variables exactly for this use case.
>>>
>>> Is there a documented way on how to make the variable's value on
>>> remote hosts customizable for the user too?
>> Something like
>> --8<---------------cut here---------------start------------->8---
>> (connection-local-set-profile-variables
>>   'remote-xref-variables
>>   '((xref-search-program . "/bin/grep")))
>> (with-eval-after-load 'xref
>>    (connection-local-set-profiles
>>     '(:application tramp :machine "myhost")
>>     'remote-xref-variables))
>> --8<---------------cut here---------------end--------------->8---
>
> Nice. Some integration with the Customize UI probably wouldn't hurt, though.
>
> Philip, would you like to try writing a patch along the lines of
> Michael's suggestion?
>
> It would need to use version checks, though, given that
> connection-local vars were only added in Emacs 27.

The following works, with one issue:


[-- Attachment #2: Type: text/plain, Size: 4976 bytes --]

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 6677b4f004..b2f82f1889 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -1751,15 +1751,7 @@ xref-matches-in-files
        (remote-id (file-remote-p dir))
        ;; The 'auto' default would be fine too, but ripgrep can't handle
        ;; the options we pass in that case.
-       (grep-highlight-matches nil)
-       (command (grep-expand-template (cdr
-                                       (or
-                                        (assoc
-                                         xref-search-program
-                                         xref-search-program-alist)
-                                        (user-error "Unknown search program `%s'"
-                                                    xref-search-program)))
-                                      (xref--regexp-to-extended regexp))))
+       (grep-highlight-matches nil))
     (when remote-id
       (require 'tramp)
       (setq files (mapcar
@@ -1770,25 +1762,56 @@ xref-matches-in-files
     (when (file-name-quoted-p (car files))
       (setq files (mapcar #'file-name-unquote files)))
     (with-current-buffer output
-      (erase-buffer)
       (with-temp-buffer
         (insert (mapconcat #'identity files "\0"))
-        (setq default-directory dir)
-        (setq status
-              (xref--process-file-region (point-min)
-                                         (point-max)
-                                         shell-file-name
-                                         output
-                                         nil
-                                         shell-command-switch
-                                         command)))
-      (goto-char (point-min))
-      (when (and (/= (point-min) (point-max))
-                 (not (looking-at grep-re))
-                 ;; TODO: Show these matches as well somehow?
-                 (not (looking-at "Binary file .* matches")))
-        (user-error "Search failed with status %d: %s" status
-                    (buffer-substring (point-min) (line-end-position))))
+        (let* ((default-directory dir)
+               (xref-search-program-alist xref-search-program-alist)
+               (program (if (version<= "27" emacs-version)
+                            (with-connection-local-variables
+                             xref-search-program)
+                          xref-search-program)))
+          ;; In case `xref-search-program' is not installed on a
+          ;; remote system, we will speculatively try to start a
+          ;; different searcher to see if it is installed and works.
+          (catch 'ok
+            (while xref-search-program-alist
+              (with-current-buffer output
+                (erase-buffer))
+              (setq status
+                    (xref--process-file-region
+                     (point-min) (point-max)
+                     shell-file-name
+                     output nil shell-command-switch
+                     (grep-expand-template
+                      (or (alist-get program xref-search-program-alist)
+                          (user-error "Unknown search program `%s'"
+                                      program))
+                      (xref--regexp-to-extended regexp))))
+              (with-current-buffer output
+                (goto-char (point-min))
+                (unless (and (/= (point-min) (point-max))
+                             (not (looking-at grep-re))
+                             ;; TODO: Show these matches as well somehow?
+                             (not (looking-at "Binary file .* matches")))
+                  (when (and (not (eq program xref-search-program))
+                             (version<= "27" emacs-version))
+                    ;; If possible and necessary, save whatever program
+                    ;; was found as a connection local variable.
+                    (let* ((host (file-remote-p default-directory 'host))
+                           (profile (intern (concat "xref-remote-" host))))
+                      (connection-local-set-profile-variables
+                       profile `((xref-search-program . ,program)))
+                      (connection-local-set-profiles
+                       (list :machine host) profile)))
+                  (throw 'ok t)))
+              (setq xref-search-program-alist
+                    (remq (assq program xref-search-program-alist)
+                          xref-search-program-alist)
+                    program (caar xref-search-program-alist)))
+            (with-current-buffer output
+              (user-error "Search failed with status %d: %s" status
+                          (buffer-string)
+                          (buffer-substring (point-min) (line-end-position)))))))
       (while (re-search-forward grep-re nil t)
         (push (list (string-to-number (match-string line-group))
                     (match-string file-group)

[-- Attachment #3: Type: text/plain, Size: 677 bytes --]


xref--process-file-region takes shell-file-name, that should depend on
the remote system.  I could use something like

   (with-parsed-tramp-file-name default-directory d
     (tramp-get-method-parameter d 'tramp-remote-shell))

but it seems that `with-parsed-tramp-file-name' is not used outside of
tramp, and byte-compiling triggers a warning.  Is there some better way
to do this?
  
> To get you started, here's one example of this feature's usage:
> https://github.com/company-mode/company-mode/blob/c25f1fbc3850e36e6521b77fa1641d5583365d8b/company-gtags.el#L71-L96
>
> And you can search in Emacs's own repository, of course, for other examples.

-- 
	Philip Kaludercic

bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

[Michael Albinus]

Philip Kaludercic <philipk@posteo.net> writes:

Hi Philip,

> +                            (with-connection-local-variables
> +                             xref-search-program)
> +                          xref-search-program)))
>
> xref--process-file-region takes shell-file-name, that should depend on
> the remote system.  I could use something like
>
>    (with-parsed-tramp-file-name default-directory d
>      (tramp-get-method-parameter d 'tramp-remote-shell))
>
> but it seems that `with-parsed-tramp-file-name' is not used outside of
> tramp, and byte-compiling triggers a warning.  Is there some better way
> to do this?

`shell-file-name' is already handled as connection-local variable, so
you could apply `with-connection-local-variables shell-file-name' as well.

bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

[Philip Kaludercic]

Michael Albinus <michael.albinus@gmx.de> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
> Hi Philip,
>
>> +                            (with-connection-local-variables
>> +                             xref-search-program)
>> +                          xref-search-program)))
>>
>> xref--process-file-region takes shell-file-name, that should depend on
>> the remote system.  I could use something like
>>
>>    (with-parsed-tramp-file-name default-directory d
>>      (tramp-get-method-parameter d 'tramp-remote-shell))
>>
>> but it seems that `with-parsed-tramp-file-name' is not used outside of
>> tramp, and byte-compiling triggers a warning.  Is there some better way
>> to do this?
>
> `shell-file-name' is already handled as connection-local variable, so
> you could apply `with-connection-local-variables shell-file-name' as well.

Ok, I missed that.  But the question still remains for versions of Emacs
prior to 27.1.  What would you advise to do there?

For context: In my specific case, I am using Guix so shell-file-name
something like
/gnu/store/87kif0bpf0anwbsaw0jvg8fyciw4sz67-bash-5.0.16/bin/bash.
Virtually every server I might connect to does not have this path (tough
"/bin/sh" works in that case (which would break other systems like
adb)).  So I don't think a version check would suffice.  All it does in
the patch I provided above is provide a speedup for all greps after the
first one.

-- 
	Philip Kaludercic

bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

[Michael Albinus]

Philip Kaludercic <philipk@posteo.net> writes:

Hi Philip,

> Ok, I missed that.  But the question still remains for versions of Emacs
> prior to 27.1.  What would you advise to do there?
>
> For context: In my specific case, I am using Guix so shell-file-name
> something like
> /gnu/store/87kif0bpf0anwbsaw0jvg8fyciw4sz67-bash-5.0.16/bin/bash.
> Virtually every server I might connect to does not have this path (tough
> "/bin/sh" works in that case (which would break other systems like
> adb)).  So I don't think a version check would suffice.  All it does in
> the patch I provided above is provide a speedup for all greps after the
> first one.

I would do something like this (untested!):

--8<---------------cut here---------------start------------->8---
(defmacro my-with-connection-local-variables (&rest body)
  "Ensure, that `shell-file-name' and `xref-search-program' have proper values."
  (if (bound-and-true-p enable-connection-local-variables)
      `(with-connection-local-variables ,@body)
    `(if (file-remote-p default-directory)
	 (let ((shell-file-name "/bin/sh") ;; Adapt
	       (xref-search-program "/bin/gerep")) ;; Adapt
	   ,@body)
       ,@body)))
--8<---------------cut here---------------end--------------->8---

Best regards, Michael.

bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

[Philip Kaludercic]

Michael Albinus <michael.albinus@gmx.de> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
> Hi Philip,
>
>> Ok, I missed that.  But the question still remains for versions of Emacs
>> prior to 27.1.  What would you advise to do there?
>>
>> For context: In my specific case, I am using Guix so shell-file-name
>> something like
>> /gnu/store/87kif0bpf0anwbsaw0jvg8fyciw4sz67-bash-5.0.16/bin/bash.
>> Virtually every server I might connect to does not have this path (tough
>> "/bin/sh" works in that case (which would break other systems like
>> adb)).  So I don't think a version check would suffice.  All it does in
>> the patch I provided above is provide a speedup for all greps after the
>> first one.
>
> I would do something like this (untested!):
>
> (defmacro my-with-connection-local-variables (&rest body)
>   "Ensure, that `shell-file-name' and `xref-search-program' have proper values."
>   (if (bound-and-true-p enable-connection-local-variables)
>       `(with-connection-local-variables ,@body)
>     `(if (file-remote-p default-directory)
> 	 (let ((shell-file-name "/bin/sh") ;; Adapt
> 	       (xref-search-program "/bin/gerep")) ;; Adapt

But the question here remains precisely what to use instead of the
literal "/bin/sh"?

> 	   ,@body)
>        ,@body)))
>
> Best regards, Michael.
>

-- 
	Philip Kaludercic

bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

[Michael Albinus]

Philip Kaludercic <philipk@posteo.net> writes:

Hi Philip,

> But the question here remains precisely what to use instead of the
> literal "/bin/sh"?

Well, if there isn't a better value, "/bin/sh" should serve as
fallback. In my experience with Tramp, it works on most of the remote
systems. It doesn't work on remote Android devices, for example. But the
number of Emacs < 27 users, running xref-* on a remote Android device,
is rather limited I believe.

POSIX declines this assumptions. It recommends to call "command -v sh"
in order to determine the shell path, see
<https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sh.html>.

Best regards, Michael.

bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

[Philip Kaludercic]

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

Michael Albinus <michael.albinus@gmx.de> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
> Hi Philip,
>
>> But the question here remains precisely what to use instead of the
>> literal "/bin/sh"?
>
> Well, if there isn't a better value, "/bin/sh" should serve as
> fallback. In my experience with Tramp, it works on most of the remote
> systems. It doesn't work on remote Android devices, for example. But
> the
> number of Emacs < 27 users, running xref-* on a remote Android device,
> is rather limited I believe.
>
> POSIX declines this assumptions. It recommends to call "command -v sh"
> in order to determine the shell path, see
> <https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sh.html>.
>
> Best regards, Michael.

In that case, I'd propose this patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Check-if-xref-search-program-exists-on-a-remote-syst.patch --]
[-- Type: text/x-patch, Size: 6133 bytes --]

From da83d403688b4b5771d08c95975c73a1cfd593c1 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Wed, 9 Feb 2022 10:14:38 +0100
Subject: [PATCH] Check if xref-search-program exists on a remote system

* lisp/progmodes/xref.el (xref-matches-in-files): Loop through all
programmes in xref-search-program-alist as a fallback before raising
an error that the search query couldn't be executed.
---
 lisp/progmodes/xref.el | 84 ++++++++++++++++++++++++++++--------------
 1 file changed, 57 insertions(+), 27 deletions(-)

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 6677b4f004..759dbb9778 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -1751,44 +1751,74 @@ xref-matches-in-files
        (remote-id (file-remote-p dir))
        ;; The 'auto' default would be fine too, but ripgrep can't handle
        ;; the options we pass in that case.
-       (grep-highlight-matches nil)
-       (command (grep-expand-template (cdr
-                                       (or
-                                        (assoc
-                                         xref-search-program
-                                         xref-search-program-alist)
-                                        (user-error "Unknown search program `%s'"
-                                                    xref-search-program)))
-                                      (xref--regexp-to-extended regexp))))
+       (grep-highlight-matches nil))
     (when remote-id
       (require 'tramp)
       (setq files (mapcar
                    (if (tramp-tramp-file-p dir)
                        #'tramp-file-local-name
-                       #'file-local-name)
+                     #'file-local-name)
                    files)))
     (when (file-name-quoted-p (car files))
       (setq files (mapcar #'file-name-unquote files)))
     (with-current-buffer output
-      (erase-buffer)
       (with-temp-buffer
         (insert (mapconcat #'identity files "\0"))
-        (setq default-directory dir)
-        (setq status
-              (xref--process-file-region (point-min)
-                                         (point-max)
-                                         shell-file-name
-                                         output
-                                         nil
-                                         shell-command-switch
-                                         command)))
-      (goto-char (point-min))
-      (when (and (/= (point-min) (point-max))
-                 (not (looking-at grep-re))
-                 ;; TODO: Show these matches as well somehow?
-                 (not (looking-at "Binary file .* matches")))
-        (user-error "Search failed with status %d: %s" status
-                    (buffer-substring (point-min) (line-end-position))))
+        (let* ((default-directory dir)
+               (xref-search-program-alist xref-search-program-alist)
+               (program (if (bound-and-true-p enable-connection-local-variables)
+                            (with-connection-local-variables
+                             xref-search-program)
+                          xref-search-program))
+               (process-file-side-effects nil))
+          ;; In case `xref-search-program' is not installed on a
+          ;; remote system, we will speculatively try to start a
+          ;; different searcher to see if it is installed and works.
+          (catch 'ok
+            (while xref-search-program-alist
+              (with-current-buffer output
+                (erase-buffer))
+              (setq status
+                    (let ((sfn shell-file-name)
+                          (scs shell-command-switch))
+                      (when remote-id
+                        (if (bound-and-true-p enable-connection-local-variables)
+                            (with-connection-local-variables
+                             (setq sfn shell-file-name
+                                   scs shell-command-switch))
+                          (setq sfn "/bin/sh")))
+                      (xref--process-file-region
+                       (point-min) (point-max)
+                       sfn output nil scs
+                       (grep-expand-template
+                        (or (alist-get program xref-search-program-alist)
+                            (user-error "Unknown search program `%s'" program))
+                        (xref--regexp-to-extended regexp)))))
+              (with-current-buffer output
+                (goto-char (point-min))
+                (unless (and (/= (point-min) (point-max))
+                             (not (looking-at grep-re))
+                             ;; TODO: Show these matches as well somehow?
+                             (not (looking-at "Binary file .* matches")))
+                  (when (and (not (eq program xref-search-program))
+                             (version<= "27" emacs-version))
+                    ;; If possible and necessary, save whatever program
+                    ;; was found as a connection local variable.
+                    (let* ((host (file-remote-p dir 'host))
+                           (profile (intern (concat "xref-remote-" host))))
+                      (connection-local-set-profile-variables
+                       profile `((xref-search-program . ,program)))
+                      (connection-local-set-profiles
+                       (list :machine host) profile)))
+                  (throw 'ok t)))
+              (setq xref-search-program-alist
+                    (remq (assq program xref-search-program-alist)
+                          xref-search-program-alist)
+                    program (caar xref-search-program-alist)))
+            (with-current-buffer output
+              (user-error "Search failed with status %d: %s" status
+                          (buffer-string)
+                          (buffer-substring (point-min) (line-end-position)))))))
       (while (re-search-forward grep-re nil t)
         (push (list (string-to-number (match-string line-group))
                     (match-string file-group)
-- 
2.34.0


[-- Attachment #3: Type: text/plain, Size: 24 bytes --]


-- 
	Philip Kaludercic

bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

[Dmitry Gutov]

On 09.02.2022 11:17, Philip Kaludercic wrote:
> In that case, I'd propose this patch:

Thanks!

Could you extract the detection logic to outside of the main function? I 
think the result would be easier to read. The diff, too.

And use a simpler test: (only when the host is remote) write some text 
to a file in the temp dir, then search through it. Only doing it once, 
of course, when the connection-local value is initialized.

I'd prefer an even simpler test than that, but I guess the current 
structure of xref-search-program-alist doesn't give us specific values 
to call 'executable-find' on. And that's probably fine.

bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

[Philip Kaludercic]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 09.02.2022 11:17, Philip Kaludercic wrote:
>> In that case, I'd propose this patch:
>
> Thanks!
>
> Could you extract the detection logic to outside of the main function?
> I think the result would be easier to read. The diff, too.

Ok.

> And use a simpler test: (only when the host is remote) write some text
> to a file in the temp dir, then search through it. Only doing it once,
> of course, when the connection-local value is initialized.

I am afraid I don't understand what you mean here, specifically "some
text".

> I'd prefer an even simpler test than that, but I guess the current
> structure of xref-search-program-alist doesn't give us specific values
> to call 'executable-find' on. And that's probably fine.
>

-- 
	Philip Kaludercic

bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

[Dmitry Gutov]

On 14.02.2022 15:57, Philip Kaludercic wrote:
>> And use a simpler test: (only when the host is remote) write some text
>> to a file in the temp dir, then search through it. Only doing it once,
>> of course, when the connection-local value is initialized.
> I am afraid I don't understand what you mean here, specifically "some
> text".
> 

Well, we need some file to search and some knowledge about its contents 
in advance (so the search can succeed).

Since we don't know anything about the remote system, we probably have 
to create the file ourselves. Put something like "aaa\nbbb\nccc" in it, 
and then search for "bbb".

bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

[Philip Kaludercic]

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

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 14.02.2022 15:57, Philip Kaludercic wrote:
>>> And use a simpler test: (only when the host is remote) write some text
>>> to a file in the temp dir, then search through it. Only doing it once,
>>> of course, when the connection-local value is initialized.
>> I am afraid I don't understand what you mean here, specifically "some
>> text".
>> 
>
> Well, we need some file to search and some knowledge about its
> contents in advance (so the search can succeed).

I guess this is what confuses me, what about the contents is relevant to
the query?  `xref-matches-in-files' is already passed a list of files
that can be concatenated into the input for xargs.  The current version
already works and is reasonably fast, so I don't recognise the
improvement.

> Since we don't know anything about the remote system, we probably have
> to create the file ourselves. Put something like "aaa\nbbb\nccc" in
> it, and then search for "bbb".

My apologies, I feel stupid for not understanding, but what would aaa,
bbb and ccc be?

---

Here also the updated version of the patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Check-if-xref-search-program-exists-on-a-remote-syst.patch --]
[-- Type: text/x-patch, Size: 8910 bytes --]

From 4469098b1dcaefca9b7f190b5e1c1a8ef53791cd Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Wed, 9 Feb 2022 10:14:38 +0100
Subject: [PATCH] Check if xref-search-program exists on a remote system

* xref.el (xref--do-search): Loop through all
programmes in xref-search-program-alist as a fallback before raising
an error that the search query couldn't be executed.
(xref-matches-in-files): Extract functionality into xref--do-search
---
 lisp/progmodes/xref.el | 141 ++++++++++++++++++++++++++---------------
 1 file changed, 89 insertions(+), 52 deletions(-)

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 6677b4f004..4e4718f5f8 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -1729,6 +1729,79 @@ xref-search-program
   :version "28.1"
   :package-version '(xref . "1.0.4"))
 
+(defun xref--do-search (regexp files dir remotep)
+  "Search for REGEXP in FILES within DIR.
+If REMOTEP is non-nil, gracefully handle the non-existence of the
+preferred searcher (per `xref-search-program'), and attempt to
+restart the query with another program in
+`xref-search-program-alist', only failing if none of these could
+be found either."
+  (pcase-let ((`(,grep-re ,file-group ,line-group . ,_) (car grep-regexp-alist))
+              (input (mapconcat #'identity files "\0"))
+              (output (get-buffer-create " *xref grep output*"))
+              (status) (hits))
+    (with-current-buffer output
+      (let* ((default-directory dir)
+             (xref-search-program-alist
+              (if remotep
+                  xref-search-program-alist
+                (list (assoc xref-search-program xref-search-program-alist))))
+             (program (if (bound-and-true-p enable-connection-local-variables)
+                          (with-connection-local-variables
+                           xref-search-program)
+                        xref-search-program))
+             (process-file-side-effects nil))
+        ;; In case `xref-search-program' is not installed on a
+        ;; remote system, we will speculatively try to start a
+        ;; different searcher to see if it is installed and works.
+        (catch 'done
+          (while xref-search-program-alist
+            (erase-buffer)
+            (setq status
+                  (let ((sfn shell-file-name)
+                        (scs shell-command-switch))
+                    (when remotep
+                      (if (bound-and-true-p enable-connection-local-variables)
+                          (with-connection-local-variables
+                           (setq sfn shell-file-name
+                                 scs shell-command-switch))
+                        (setq sfn "/bin/sh")))
+                    (xref--process-file-region
+                     input nil sfn output nil scs
+                     (grep-expand-template
+                      (or (alist-get program xref-search-program-alist)
+                          (user-error "Unknown search program `%s'" program))
+                      (xref--regexp-to-extended regexp)))))
+            (goto-char (point-min))
+            (unless (and (/= (point-min) (point-max))
+                         (not (looking-at grep-re))
+                         ;; TODO: Show these matches as well somehow?
+                         (not (looking-at "Binary file .* matches")))
+              (when (and (not (eq program xref-search-program))
+                         (version<= "27" emacs-version))
+                ;; If possible and necessary, save whatever program
+                ;; was found as a connection local variable.
+                (let* ((host (file-remote-p dir 'host))
+                       (profile (intern (concat "xref-remote-" host))))
+                  (connection-local-set-profile-variables
+                   profile `((xref-search-program . ,program)))
+                  (connection-local-set-profiles
+                   (list :machine host) profile)))
+              (throw 'done t))
+            (setq xref-search-program-alist
+                  (remq (assq program xref-search-program-alist)
+                        xref-search-program-alist)
+                  program (caar xref-search-program-alist)))
+          (user-error "Search failed with status %d: %s" status
+                      (buffer-string)
+                      (buffer-substring (point-min) (line-end-position)))))
+      (while (re-search-forward grep-re nil t)
+        (push (list (string-to-number (match-string line-group))
+                    (match-string file-group)
+                    (buffer-substring-no-properties (point) (line-end-position)))
+              hits)))
+    hits))
+
 ;;;###autoload
 (defun xref-matches-in-files (regexp files)
   "Find all matches for REGEXP in FILES.
@@ -1741,69 +1814,33 @@ xref-matches-in-files
   (require 'grep)
   (defvar grep-highlight-matches)
   (pcase-let*
-      ((output (get-buffer-create " *project grep output*"))
-       (`(,grep-re ,file-group ,line-group . ,_) (car grep-regexp-alist))
-       (status nil)
-       (hits nil)
-       ;; Support for remote files.  The assumption is that, if the
+      (;; Support for remote files.  The assumption is that, if the
        ;; first file is remote, they all are, and on the same host.
        (dir (file-name-directory (car files)))
-       (remote-id (file-remote-p dir))
+       (remotep (file-remote-p dir))
        ;; The 'auto' default would be fine too, but ripgrep can't handle
        ;; the options we pass in that case.
-       (grep-highlight-matches nil)
-       (command (grep-expand-template (cdr
-                                       (or
-                                        (assoc
-                                         xref-search-program
-                                         xref-search-program-alist)
-                                        (user-error "Unknown search program `%s'"
-                                                    xref-search-program)))
-                                      (xref--regexp-to-extended regexp))))
-    (when remote-id
+       (grep-highlight-matches nil))
+    (when remotep
       (require 'tramp)
       (setq files (mapcar
                    (if (tramp-tramp-file-p dir)
                        #'tramp-file-local-name
-                       #'file-local-name)
+                     #'file-local-name)
                    files)))
     (when (file-name-quoted-p (car files))
       (setq files (mapcar #'file-name-unquote files)))
-    (with-current-buffer output
-      (erase-buffer)
-      (with-temp-buffer
-        (insert (mapconcat #'identity files "\0"))
-        (setq default-directory dir)
-        (setq status
-              (xref--process-file-region (point-min)
-                                         (point-max)
-                                         shell-file-name
-                                         output
-                                         nil
-                                         shell-command-switch
-                                         command)))
-      (goto-char (point-min))
-      (when (and (/= (point-min) (point-max))
-                 (not (looking-at grep-re))
-                 ;; TODO: Show these matches as well somehow?
-                 (not (looking-at "Binary file .* matches")))
-        (user-error "Search failed with status %d: %s" status
-                    (buffer-substring (point-min) (line-end-position))))
-      (while (re-search-forward grep-re nil t)
-        (push (list (string-to-number (match-string line-group))
-                    (match-string file-group)
-                    (buffer-substring-no-properties (point) (line-end-position)))
-              hits)))
-    ;; By default, ripgrep's output order is non-deterministic
-    ;; (https://github.com/BurntSushi/ripgrep/issues/152)
-    ;; because it does the search in parallel.
-    ;; Grep's output also comes out in seemingly arbitrary order,
-    ;; though stable one. Let's sort both for better UI.
-    (setq hits
-          (sort (nreverse hits)
-                (lambda (h1 h2)
-                  (string< (cadr h1) (cadr h2)))))
-    (xref--convert-hits hits regexp)))
+    (let ((hits (xref--do-search regexp files dir remotep)))
+      ;; By default, ripgrep's output order is non-deterministic
+      ;; (https://github.com/BurntSushi/ripgrep/issues/152)
+      ;; because it does the search in parallel.
+      ;; Grep's output also comes out in seemingly arbitrary order,
+      ;; though stable one. Let's sort both for better UI.
+      (setq hits
+            (sort (nreverse hits)
+                  (lambda (h1 h2)
+                    (string< (cadr h1) (cadr h2)))))
+      (xref--convert-hits hits regexp))))
 
 (defun xref--process-file-region ( start end program
                                    &optional buffer display
-- 
2.34.0


[-- Attachment #3: Type: text/plain, Size: 25 bytes --]



-- 
	Philip Kaludercic

bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

[Dmitry Gutov]

On 14.02.2022 19:32, Philip Kaludercic wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
>> On 14.02.2022 15:57, Philip Kaludercic wrote:
>>>> And use a simpler test: (only when the host is remote) write some text
>>>> to a file in the temp dir, then search through it. Only doing it once,
>>>> of course, when the connection-local value is initialized.
>>> I am afraid I don't understand what you mean here, specifically "some
>>> text".
>>>
>>
>> Well, we need some file to search and some knowledge about its
>> contents in advance (so the search can succeed).
> 
> I guess this is what confuses me, what about the contents is relevant to
> the query?  `xref-matches-in-files' is already passed a list of files
> that can be concatenated into the input for xargs.  The current version
> already works and is reasonably fast, so I don't recognise the
> improvement.

Sorry, I guess I wasn't clear enough.

When I said "extract the detection logic", I meant implement something 
similar to 'grep-compute-defaults' where there is a bunch of "probing" 
code which detects which commands work on the given system (and which 
arguments, etc). But a shorter function, of course, since it would only 
need to choose between two alternatives -- and return it.

And it seems to be it would be simpler (conceptually) if the said 
function didn't itself depend on xref-matches-in-files (the 
implementation or the return type). Though it's certainly possible to 
use it as well.

...so that function (let's call it xref--choose-search-program, perhaps) 
would write to a file in the temporary directory on the remote system, 
and then search in it using the configured search program, and then fall 
back to the default one if the first one fails.

WDYT?

>> Since we don't know anything about the remote system, we probably have
>> to create the file ourselves. Put something like "aaa\nbbb\nccc" in
>> it, and then search for "bbb".
> 
> My apologies, I feel stupid for not understanding, but what would aaa,
> bbb and ccc be?

Those are characters. "aaa\nbbb\nccc" would be the temporary file's 
contents, verbatim.

To clarify, I think your code quality is just fine, but the way the main 
function gets two responsibilities intertwined (both program detection 
and the actual search) seems a bit too much for me, clarity-wise.

bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

[Philip Kaludercic]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 14.02.2022 19:32, Philip Kaludercic wrote:
>> Dmitry Gutov <dgutov@yandex.ru> writes:
>> 
>>> On 14.02.2022 15:57, Philip Kaludercic wrote:
>>>>> And use a simpler test: (only when the host is remote) write some text
>>>>> to a file in the temp dir, then search through it. Only doing it once,
>>>>> of course, when the connection-local value is initialized.
>>>> I am afraid I don't understand what you mean here, specifically "some
>>>> text".
>>>>
>>>
>>> Well, we need some file to search and some knowledge about its
>>> contents in advance (so the search can succeed).
>> I guess this is what confuses me, what about the contents is
>> relevant to
>> the query?  `xref-matches-in-files' is already passed a list of files
>> that can be concatenated into the input for xargs.  The current version
>> already works and is reasonably fast, so I don't recognise the
>> improvement.
>
> Sorry, I guess I wasn't clear enough.
>
> When I said "extract the detection logic", I meant implement something
> similar to 'grep-compute-defaults' where there is a bunch of "probing"
> code which detects which commands work on the given system (and which
> arguments, etc). But a shorter function, of course, since it would
> only need to choose between two alternatives -- and return it.
>
> And it seems to be it would be simpler (conceptually) if the said
> function didn't itself depend on xref-matches-in-files (the
> implementation or the return type). Though it's certainly possible to
> use it as well.
>
> ...so that function (let's call it xref--choose-search-program,
> perhaps) would write to a file in the temporary directory on the
> remote system, and then search in it using the configured search
> program, and then fall back to the default one if the first one fails.
>
> WDYT?

The current design (subsuming probing into executing by speculatively
starting a process) was intentional, mainly to avoid the overhead of
executable-find calls.  I guess one could argue that this is only
necessary once, and after that can be stored as a connection local
variable.  Setting this aside, I see little difference to the
xref--choose-search-program approach, unless there is some other context
that might want to use this function.

>>> Since we don't know anything about the remote system, we probably have
>>> to create the file ourselves. Put something like "aaa\nbbb\nccc" in
>>> it, and then search for "bbb".
>> My apologies, I feel stupid for not understanding, but what would
>> aaa,
>> bbb and ccc be?
>
> Those are characters. "aaa\nbbb\nccc" would be the temporary file's
> contents, verbatim.
>
> To clarify, I think your code quality is just fine, but the way the
> main function gets two responsibilities intertwined (both program
> detection and the actual search) seems a bit too much for me,
> clarity-wise.

I am biased, but I do prefer the current state of the patch.  If you
think it would help, I could comment it out more?

-- 
	Philip Kaludercic

bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

[Dmitry Gutov]

On 15.02.2022 18:32, Philip Kaludercic wrote:
> The current design (subsuming probing into executing by speculatively
> starting a process) was intentional, mainly to avoid the overhead of
> executable-find calls.

At the expense of complicating the execution flow, yes.

bug#53644: 29.0.50; xref-search-program breaks if programm not installed on a remote host

[Dmitry Gutov]

On 15.02.2022 18:32, Philip Kaludercic wrote:
> I am biased, but I do prefer the current state of the patch.  If you
> think it would help, I could comment it out more?

It's not like I don't understand what it's doing on every line of the 
code (roughly), more like it exceeds the max level of complexity I'd 
like to see in a function. Which is not a problem for us now, but is 
likely to become such for someone who comes later.

It could slow down some future changes too, though.

I'm obviously biased myself, so more opinions welcome.