GNU bug report logs - #78925
31.0.50; ffap's filename prompt problem in remote files

Previous Next

Full log

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Liu Hui <liuhui1610 <at> gmail.com>
Cc: 78925-done <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#78925: 31.0.50; ffap's filename prompt problem in remote
 files, [PATCH] Add option 'ffap-prefer-remote-file' (bug#78925)
Date: Sat, 12 Jul 2025 11:49:53 +0300

> From: Liu Hui <liuhui1610 <at> gmail.com>
> Date: Tue, 8 Jul 2025 18:19:39 +0800
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 78925 <at> debbugs.gnu.org
> 
> On Tue, Jul 8, 2025 at 1:17 AM Michael Albinus <michael.albinus <at> gmx.de> wrote:
> >
> > Liu Hui <liuhui1610 <at> 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.

Installed on master (after reverting the previous one).

Previous Next

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