On Tue, Jul 8, 2025 at 1:17 AM Michael Albinus <michael.albinus@gmx.de> wrote:
>
> Liu Hui <liuhui1610@gmail.com> writes:
>
> Hi,
>
> > The attached patch adds an option 'ffap-prefer-remote-file', which
> > defaults to nil. A non-nil value means ffap always finds remote files
> > for absolute filenames in remote case, instead of finding local files
> > first.
>
> Thank you for the patch. I don't use ffap myself, so just some comments
> from cursory review.

Thanks for your comments!

> > +---
> > +*** New user option 'ffap-prefer-remote-file'.
> > +If non-nil, ffap always finds remote files in buffers with remote
> > +'default-directory'.  If nil, ffap finds local files first for absolute
> > +filenames in above buffers.  The default is nil.
>
> Please spell it out FFAP.

Done.

> > -(defun ffap-file-exists-string (file &optional nomodify)
> > +(defun ffap-file-exists-string (file &optional nomodify remote-host)
>
> A remote file name is not only the host. There are also, at least, a
> method and a user. Please call this argument remote-prefix or alike.

Renamed to remote-prefix.

> > +     ((or (not file)                 ; quietly reject nil
> > +       (zerop (length file)))        ; and also ""
> > +      nil)
>
> aka ((zerop (length file)) nil)

Done.

> > +     ((and remote-host                  ; prepend remote host to file
> > +           (file-name-absolute-p file)
> > +           (setq file (concat remote-host file))
> > +           nil))
>
> What, if file is already remote? Shouldn't the check be
>
> --8<---------------cut here---------------start------------->8---
>      ((and remote-host                  ; prepend remote host to file
>            (file-name-absolute-p file)
>            (not (file-name-remote-p file))
>            (setq file (concat remote-host file))
>            nil))
> --8<---------------cut here---------------end--------------->8---

I examined the callers (e.g. ffap-file-at-point) and they have ruled
out the possibility that file is remote by ffap-file-remote-p. So it
wasn't checked again here. The docstring has been revised to clarify
the usage of the new argument.

> >  (require 'cl-lib)
> >  (require 'ert)
> > +(require 'tramp)
> >  (require 'ert-x)
> >  (require 'ffap)
>
> Why Tramp? I guess it isn't needed.

The comment of ert-remote-temporary-file-directory:

;; If this defvar is used in a test file, `tramp' shall be loaded
;; prior `ert-x'.  There is no default value on w32 systems, which
;; could work out of the box.

Otherwise, the remote test is skipped with 'make test/ffap-tests'.

> > +  (let* ((ffap-prefer-remote-file t)
> > +         (default-directory
> > +          (expand-file-name ert-remote-temporary-file-directory))
> > +         (test-file (expand-file-name "ffap-test" default-directory)))
>
> I would use (make-temp-file "ffap-test")

Done.