bug#36502: 27.0.50; infinite loop in file-name-case-insensitive-p

classic Classic list List threaded Threaded
23 messages Options
12
Reply | Threaded
Open this post in threaded view
|

bug#36502: 27.0.50; infinite loop in file-name-case-insensitive-p

Daniel Sutton-2
There seems to be a bug with `file-name-case-insensitive-p` when given a
file name that doesn't exist and does not have a root that exists. For
instance `(file-name-case-insensitive-p "some/project")`.

Unfortunately I'm unable to give a simple reproduction as this only
seemed to happen when running tests for CIDER under cask. This behavior
seems to have been introduced in
81d6418e6b7c3e637dccf9c856d9c4b94bd43b97 with the commit message:

Fix file-name-case-insensitive-p on non-existent files

* src/fileio.c (Ffile_name_case_insensitive_p): If the file
doesn't exist, move up the filesystem tree until an existing
directory is found.  Then test that directory for
case-insensitivity.  (Bug#32246)

https://github.com/clojure-emacs/cider/pull/2669/commits/2039abd3c48214200a9e50a5288246011d48eb3e

Configured features:
XPM JPEG TIFF GIF PNG SOUND DBUS GSETTINGS GLIB NOTIFY INOTIFY
LIBSELINUX GNUTLS LIBXML2 FREETYPE HARFBUZZ XFT ZLIB TOOLKIT_SCROLL_BARS
GTK3 X11 XDBE XIM THREADS LIBSYSTEMD PDUMPER LCMS2 GMP

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Dired by name

Minor modes in effect:
  recentf-mode: t
  global-magit-file-mode: t
  magit-auto-revert-mode: t
  global-git-commit-mode: t
  async-bytecomp-package-mode: t
  shell-dirtrack-mode: t
  ivy-mode: t
  projectile-mode: t
  yas-global-mode: t
  yas-minor-mode: t
  global-undo-tree-mode: t
  delete-selection-mode: t
  company-quickhelp-mode: t
  company-quickhelp-local-mode: t
  global-company-mode: t
  company-mode: t
  which-key-mode: t
  global-hl-line-mode: t
  minions-mode: t
  pixel-scroll-mode: t
  save-place-mode: t
  winner-mode: t
  show-paren-mode: t
  global-auto-revert-mode: t
  auto-compile-on-load-mode: t
  override-global-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-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
  buffer-read-only: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
~/projects/dev/clojure-mode/clojure-mode hides /home/dan/.emacs.d/elpa/clojure-mode-20190508.1522/clojure-mode

Features:
(shadow sort mail-extr emacsbug sendmail whitespace tabify magit-patch
loccur cl-print eieio-opt speedbar sb-image ezimage dframe recentf
bug-reference cc-mode cc-fonts cc-guess cc-menus cc-cmds cc-styles
cc-align cc-engine cc-vars cc-defs help-fns radix-tree cider tramp-sh
cider-debug cider-inspector cider-browse-ns cider-mode cider-completion
cider-profile cider-eval cider-repl-history pulse cider-repl
cider-resolve cider-test cider-overlays cider-stacktrace cider-doc
cider-browse-spec org-table org-element avl-tree org org-macro
org-footnote org-pcomplete org-list org-faces org-entities org-version
ob-emacs-lisp ob ob-tangle org-src ob-ref ob-lob ob-table ob-keys ob-exp
ob-comint ob-core ob-eval org-compat org-macs org-loaddefs cal-menu
calendar cal-loaddefs cider-grimoire cider-popup cider-eldoc
cider-client cider-common cider-util cider-connection sesman-browser
nrepl-client nrepl-dict cider-compat crux tramp tramp-loaddefs trampver
tramp-integration files-x tramp-compat parse-time ls-lisp vc-git
checkdoc magit-extras magit-bookmark 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 magit-diff smerge-mode
diff-mode magit-core magit-autorevert magit-margin magit-transient
magit-process magit-mode transient git-commit magit-git magit-section
magit-utils crm log-edit message rfc822 mml mml-sec epa epg gnus-util
rmail rmail-loaddefs text-property-search time-date mm-decode mm-bodies
mm-encode mailabbrev mail-utils gmm-utils mailheader pcvs-util add-log
with-editor async-bytecomp async shell pcomplete counsel xdg dired
dired-loaddefs swiper ivy colir ivy-overlay ffap smart-mode-line
rich-minority projectile grep ibuf-ext ibuffer ibuffer-loaddefs
company-oddmuse company-keywords company-etags etags fileloop generator
company-gtags company-dabbrev-code company-dabbrev company-files
company-capf company-cmake company-xcode company-clang company-semantic
company-eclim company-template company-bbdb lsp-clojure lsp-mode
markdown-mode noutline outline tree-widget xref network-stream inline ht
f s em-glob esh-util dash-functional flymake-proc flymake thingatpt
clojure-mode project align imenu buttercup warnings ert ewoc debug
backtrace buttercup-compat flycheck-joker sesman vc vc-dispatcher
spinner queue pkg-info url-http url url-proxy url-privacy url-expand
url-methods url-history mailcap url-auth mail-parse rfc2231 rfc2047
rfc2045 mm-util ietf-drums mail-prsvr url-cookie url-domsuf url-util
url-gw nsm rmc puny lisp-mnt epl parseedn parseclj-parser parseclj-lex a
yasnippet elec-pair flycheck find-func rainbow-delimiters paredit
pcre2el rxt re-builder rx pdf-tools compile comint ansi-color pdf-view
bookmark pp pdf-cache pdf-info tq pdf-util format-spec image-mode
undo-tree diff delsel browse-kill-ring derived cl all-the-icons
all-the-icons-faces data-material data-weathericons data-octicons
data-fileicons data-faicons data-alltheicons memoize company-quickhelp
pos-tip company pcase hydra lv diminish which-key gruvbox-theme gruvbox
sublime-themes kaolin-themes kaolin-themes-lib darktooth-theme
autothemer solarized-theme solarized color hl-line moody minions dash
pixel-scroll advice saveplace winner ring paren autorevert filenotify
edmacro kmacro resize-window cl-extra help-mode jka-compr server
auto-compile packed use-package use-package-ensure use-package-delight
use-package-diminish use-package-bind-key bind-key easy-mmode
use-package-core finder-inf cus-edit cus-start cus-load wid-edit
mule-util info package easymenu epg-config url-handlers url-parse
auth-source cl-seq eieio eieio-core cl-macs eieio-loaddefs
password-cache json subr-x map url-vars seq byte-opt gv bytecomp
byte-compile cconv cl-loaddefs cl-lib tooltip eldoc electric uniquify
ediff-hook vc-hooks lisp-float-type 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 elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame 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 charscript charprop case-table epa-hook jka-cmpr-hook
help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote threads dbusbind inotify lcms2 dynamic-setting
system-font-setting font-render-setting move-toolbar gtk x-toolkit x
multi-tty make-network-process emacs)

Memory information:
((conses 16 641932 908328)
 (symbols 48 45885 1184)
 (strings 32 196006 120504)
 (string-bytes 1 5710715)
 (vectors 16 81080)
 (vector-slots 8 1615861 884472)
 (floats 8 972 10098)
 (intervals 56 13745 17676)
 (buffers 992 85))
Reply | Threaded
Open this post in threaded view
|

bug#36502: 27.0.50; infinite loop in file-name-case-insensitive-p

Ken Brown-6
On 7/4/2019 12:48 PM, Daniel Sutton wrote:
> There seems to be a bug with `file-name-case-insensitive-p` when given a
> file name that doesn't exist and does not have a root that exists. For
> instance `(file-name-case-insensitive-p "some/project")`.

The definition of file-name-case-insensitive-p begins by converting the argument
to an absolute file name via the C equivalent of

   (expand-file-name filename nil)

This is supposed to produce a file name that has a root that exists.

> Unfortunately I'm unable to give a simple reproduction as this only
> seemed to happen when running tests for CIDER under cask.

Can you investigate what goes wrong with expand-file-name in your setting?

Ken
Reply | Threaded
Open this post in threaded view
|

bug#36502: 27.0.50; infinite loop in file-name-case-insensitive-p

Ken Brown-6
Please keep the bug address in the Cc.

On 7/4/2019 3:35 PM, Daniel Sutton wrote:
> Locally when running `(expand-file-name "path/to/dirA/" nil)` it expands
> to "/home/dan/projects/dev/cider/test/path/to/dirA/"
>
> when running under cask it expands to "/path/to/dirA/"

So moving up the file system tree should eventually lead you to "/".  I don't
see why you're getting an infinite loop.

Ken
Reply | Threaded
Open this post in threaded view
|

bug#36502: Fwd: bug#36502: 27.0.50; infinite loop in file-name-case-insensitive-p

Daniel Sutton-2
(repeated email as I omitted the bug email address)

Ken I believe I've figured it out and there was an important bit I left out.

The test in CIDER was ensuring a buffer was named correctly based on the current project, and it set the current project's directory by setq'ing default-directory. This seems to be a big no-no as the default directory expects a well formed path. The doc string of default-directory being:

Name of default directory of current buffer.
It should be an absolute directory name; on GNU and Unix systems,
these names start with `/' or `~' and end with `/'.
To interactively change the default directory, use command `cd'. 

So the "bug" can be reproduced by the following, which was the important bits of the test:

(let ((default-directory "made/up/project/location"))
  (file-name-case-insensitive-p default-directory))

* note this will freeze your emacs

I believe your change caused the behavior here but it seems like a reasonable change and CIDER is definitely not honoring the assumptions in `default-directory`. We had some careless usages of this variable in our tests and it seems like it has finally caught up to us. Unless you consider this an edge case to watch for (and I can't imagine you do) it seems like this should be closed.

Sorry for the noise and thanks for your help
Dan Sutton

On Thu, Jul 4, 2019 at 9:36 PM Daniel Sutton <[hidden email]> wrote:
First thanks so much for your quick attention, Ken. I really appreciate it. I threw in some printlning and seeing the following output:

originally: path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/

filename from c: path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA

filename from c: path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA

filename from c: path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA

filename from c: path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA

filename from c: path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA/path/to/dirA

from the following:

if (NILP (Ffile_exists_p (filename)))
  {
    filename = Ffile_name_directory (filename);
    while (NILP (Ffile_exists_p (filename)))
      {
        Lisp_Object newname = expand_and_dir_to_file (filename);
        /* Avoid infinite loop if the root is reported as non-existing
           (impossible?).  */
        if (!NILP (Fstring_equal (newname, filename)))
          break;
        filename = newname;
        message_with_string("filename from c: %s\n", filename, 1);
      }
  }

So it seems to be growing the wrong way. I suppose this is the problem actually in expand_and_dir_to_file but i can't imagine why.

On Thu, Jul 4, 2019 at 8:32 PM Ken Brown <[hidden email]> wrote:
Please keep the bug address in the Cc.

On 7/4/2019 3:35 PM, Daniel Sutton wrote:
> Locally when running `(expand-file-name "path/to/dirA/" nil)` it expands
> to "/home/dan/projects/dev/cider/test/path/to/dirA/"
>
> when running under cask it expands to "/path/to/dirA/"

So moving up the file system tree should eventually lead you to "/".  I don't
see why you're getting an infinite loop.

Ken
Reply | Threaded
Open this post in threaded view
|

bug#36502: Fwd: bug#36502: 27.0.50; infinite loop in file-name-case-insensitive-p

Ken Brown-6
On 7/4/2019 11:05 PM, Daniel Sutton wrote:
> Sorry for the noise and thanks for your help

No problem.  I'm glad you solved it.

Closing.

Ken
Reply | Threaded
Open this post in threaded view
|

bug#36502: Fwd: bug#36502: 27.0.50; infinite loop in file-name-case-insensitive-p

Noam Postavsky
In reply to this post by Daniel Sutton-2
Daniel Sutton <[hidden email]> writes:

> So the "bug" can be reproduced by the following, which was the important
> bits of the test:
>
> (let ((default-directory "made/up/project/location"))
>   (file-name-case-insensitive-p default-directory))
>
> * note this will freeze your emacs
>
> I believe your change caused the behavior here but it seems like a
> reasonable change and CIDER is definitely not honoring the assumptions in
> `default-directory`.

>> From: Ken Brown <[hidden email]>
>
>> On 7/4/2019 11:05 PM, Daniel Sutton wrote:
>> > Sorry for the noise and thanks for your help
>
>> No problem.  I'm glad you solved it.
>
>> Closing.

Wouldn't it be better not to infloop in this case though?  E.g.,

--- i/src/fileio.c
+++ w/src/fileio.c
@@ -2408,7 +2408,10 @@ DEFUN ("file-name-case-insensitive-p", Ffile_name_case_insensitive_p,
 
   /* If the file doesn't exist, move up the filesystem tree until we
      reach an existing directory or the root.  */
-  if (NILP (Ffile_exists_p (filename)))
+  if (NILP (Ffile_exists_p (filename))
+      /* If default-directory is relative, expand-file-name can give
+         a relative name, in which case we can't move up.  */
+      && !NILP (Ffile_name_absolute_p (filename)))
     {
       filename = Ffile_name_directory (filename);
       while (NILP (Ffile_exists_p (filename)))

Or maybe signal an error, either way seems better than just getting stuck.




Reply | Threaded
Open this post in threaded view
|

bug#36502: Fwd: bug#36502: 27.0.50; infinite loop in file-name-case-insensitive-p

Ken Brown-6
On 7/6/2019 9:27 AM, Noam Postavsky wrote:

> Wouldn't it be better not to infloop in this case though?  E.g.,
>
> --- i/src/fileio.c
> +++ w/src/fileio.c
> @@ -2408,7 +2408,10 @@ DEFUN ("file-name-case-insensitive-p", Ffile_name_case_insensitive_p,
>  
>     /* If the file doesn't exist, move up the filesystem tree until we
>        reach an existing directory or the root.  */
> -  if (NILP (Ffile_exists_p (filename)))
> +  if (NILP (Ffile_exists_p (filename))
> +      /* If default-directory is relative, expand-file-name can give
> +         a relative name, in which case we can't move up.  */

This doesn't seem right to me.  expand-file-name is documented to return
an absolute file name, so I don't think callers should have to check for
that.

> +      && !NILP (Ffile_name_absolute_p (filename)))
>       {
>         filename = Ffile_name_directory (filename);
>         while (NILP (Ffile_exists_p (filename)))
>
> Or maybe signal an error, either way seems better than just getting stuck.

Maybe expand-file-name should signal an error if default-directory is
relative?

Ken
Reply | Threaded
Open this post in threaded view
|

bug#36502: Fwd: bug#36502: 27.0.50; infinite loop in file-name-case-insensitive-p

Eli Zaretskii
> From: Ken Brown <[hidden email]>
> Date: Sat, 6 Jul 2019 15:38:14 +0000
> Cc: "[hidden email]" <[hidden email]>
>
> > +      && !NILP (Ffile_name_absolute_p (filename)))
> >       {
> >         filename = Ffile_name_directory (filename);
> >         while (NILP (Ffile_exists_p (filename)))
> >
> > Or maybe signal an error, either way seems better than just getting stuck.
>
> Maybe expand-file-name should signal an error if default-directory is
> relative?

That'd not be my first preference.  My first preference would be to
detect the cases where we are about to loop.  Can we do that?



Reply | Threaded
Open this post in threaded view
|

bug#36502: Fwd: bug#36502: 27.0.50; infinite loop in file-name-case-insensitive-p

Ken Brown-6
On 7/6/2019 12:14 PM, Eli Zaretskii wrote:

>> From: Ken Brown <[hidden email]>
>> Date: Sat, 6 Jul 2019 15:38:14 +0000
>> Cc: "[hidden email]" <[hidden email]>
>>
>>> +      && !NILP (Ffile_name_absolute_p (filename)))
>>>        {
>>>          filename = Ffile_name_directory (filename);
>>>          while (NILP (Ffile_exists_p (filename)))
>>>
>>> Or maybe signal an error, either way seems better than just getting stuck.
>>
>> Maybe expand-file-name should signal an error if default-directory is
>> relative?
>
> That'd not be my first preference.  My first preference would be to
> detect the cases where we are about to loop.  Can we do that?

The only case I can think of where we might loop is the case we're discussing,
in which expand-file-name fails to return an absolute name.  Noam's patch is
fine for that case.  But wouldn't it be better to fix expand-file-name so that
it always returns an absolute name, as it's documented to do?

Ken
Reply | Threaded
Open this post in threaded view
|

bug#36502: Fwd: bug#36502: 27.0.50; infinite loop in file-name-case-insensitive-p

Eli Zaretskii
> From: Ken Brown <[hidden email]>
> CC: "[hidden email]" <[hidden email]>, "[hidden email]"
> <[hidden email]>, "[hidden email]" <[hidden email]>
> Date: Sun, 7 Jul 2019 14:09:03 +0000
>
> wouldn't it be better to fix expand-file-name so that it always
> returns an absolute name, as it's documented to do?

I think it's better, yes.  Do we have an idea for how to do that?



Reply | Threaded
Open this post in threaded view
|

bug#36502: Fwd: bug#36502: 27.0.50; infinite loop in file-name-case-insensitive-p

Ken Brown-6
On 7/7/2019 10:37 AM, Eli Zaretskii wrote:
>> From: Ken Brown <[hidden email]>
>> CC: "[hidden email]" <[hidden email]>, "[hidden email]"
>> <[hidden email]>, "[hidden email]" <[hidden email]>
>> Date: Sun, 7 Jul 2019 14:09:03 +0000
>>
>> wouldn't it be better to fix expand-file-name so that it always
>> returns an absolute name, as it's documented to do?
>
> I think it's better, yes.  Do we have an idea for how to do that?

How's this?

--- a/src/fileio.c
+++ b/src/fileio.c
@@ -804,7 +804,11 @@ DEFUN ("expand-file-name", Fexpand_file_name,
Sexpand_file_name, 1, 2, 0,

    /* Use the buffer's default-directory if DEFAULT_DIRECTORY is omitted.  */
    if (NILP (default_directory))
-    default_directory = BVAR (current_buffer, directory);
+    {
+      default_directory = BVAR (current_buffer, directory);
+      if (NILP (Ffile_name_absolute_p (default_directory)))
+       default_directory = Qnil;
+    }
    if (! STRINGP (default_directory))
      {
  #ifdef DOS_NT
Reply | Threaded
Open this post in threaded view
|

bug#36502: Fwd: bug#36502: 27.0.50; infinite loop in file-name-case-insensitive-p

Eli Zaretskii
> From: Ken Brown <[hidden email]>
> CC: "[hidden email]" <[hidden email]>, "[hidden email]"
> <[hidden email]>, "[hidden email]" <[hidden email]>
> Date: Sun, 7 Jul 2019 19:30:00 +0000
>
> --- a/src/fileio.c
> +++ b/src/fileio.c
> @@ -804,7 +804,11 @@ DEFUN ("expand-file-name", Fexpand_file_name,
> Sexpand_file_name, 1, 2, 0,
>
>     /* Use the buffer's default-directory if DEFAULT_DIRECTORY is omitted.  */
>     if (NILP (default_directory))
> -    default_directory = BVAR (current_buffer, directory);
> +    {
> +      default_directory = BVAR (current_buffer, directory);
> +      if (NILP (Ffile_name_absolute_p (default_directory)))
> +       default_directory = Qnil;
> +    }

Hmm... why nullify it?  Why not simply call expand-file-name
recursively?



Reply | Threaded
Open this post in threaded view
|

bug#36502: Fwd: bug#36502: 27.0.50; infinite loop in file-name-case-insensitive-p

Ken Brown-6
On 7/8/2019 8:25 AM, Eli Zaretskii wrote:

>> From: Ken Brown <[hidden email]>
>> CC: "[hidden email]" <[hidden email]>, "[hidden email]"
>> <[hidden email]>, "[hidden email]" <[hidden email]>
>> Date: Sun, 7 Jul 2019 19:30:00 +0000
>>
>> --- a/src/fileio.c
>> +++ b/src/fileio.c
>> @@ -804,7 +804,11 @@ DEFUN ("expand-file-name", Fexpand_file_name,
>> Sexpand_file_name, 1, 2, 0,
>>
>>      /* Use the buffer's default-directory if DEFAULT_DIRECTORY is omitted.  */
>>      if (NILP (default_directory))
>> -    default_directory = BVAR (current_buffer, directory);
>> +    {
>> +      default_directory = BVAR (current_buffer, directory);
>> +      if (NILP (Ffile_name_absolute_p (default_directory)))
>> +       default_directory = Qnil;
>> +    }
>
> Hmm... why nullify it?  Why not simply call expand-file-name
> recursively?

If the current buffer's default-directory is not absolute, then that variable is
invalid and we can't use it.  (This was perhaps not clear until last November,
when the word "absolute" was added to its doc string.)

Nullifying it guarantees that the code starting with "if (! STRINGP
(default_directory))" is used.  Maybe I should have put in a comment explaining
that.

My thinking was that an invalid value of the default-directory variable should
be treated the same way we treat a non-string value of default_directory.

Ken
Reply | Threaded
Open this post in threaded view
|

bug#36502: Fwd: bug#36502: 27.0.50; infinite loop in file-name-case-insensitive-p

Eli Zaretskii
> From: Ken Brown <[hidden email]>
> CC: "[hidden email]" <[hidden email]>, "[hidden email]"
> <[hidden email]>, "[hidden email]" <[hidden email]>
> Date: Mon, 8 Jul 2019 13:36:38 +0000
>
> >>      if (NILP (default_directory))
> >> -    default_directory = BVAR (current_buffer, directory);
> >> +    {
> >> +      default_directory = BVAR (current_buffer, directory);
> >> +      if (NILP (Ffile_name_absolute_p (default_directory)))
> >> +       default_directory = Qnil;
> >> +    }
> >
> > Hmm... why nullify it?  Why not simply call expand-file-name
> > recursively?
>
> If the current buffer's default-directory is not absolute, then that variable is
> invalid and we can't use it.

I'm asking why not do this instead:

  if (NILP (default_directory))
    {
      default_directory = BVAR (current_buffer, directory);
      if (NILP (Ffile_name_absolute_p (default_directory)))
        default_directory = Fexpand_file_name (default_directory,
                                               Vinvocation_directory);
    }

Or will the above not work for some reason?

> Nullifying it guarantees that the code starting with "if (! STRINGP
> (default_directory))" is used.  Maybe I should have put in a comment explaining
> that.

Sure, but this loses information.  Of course, if we cannot do better
reliably, it's an okay solution.

Thanks.



Reply | Threaded
Open this post in threaded view
|

bug#36502: Fwd: bug#36502: 27.0.50; infinite loop in file-name-case-insensitive-p

Andreas Schwab
In reply to this post by Ken Brown-6
On Jul 07 2019, Ken Brown <[hidden email]> wrote:

> --- a/src/fileio.c
> +++ b/src/fileio.c
> @@ -804,7 +804,11 @@ DEFUN ("expand-file-name", Fexpand_file_name,
> Sexpand_file_name, 1, 2, 0,
>
>     /* Use the buffer's default-directory if DEFAULT_DIRECTORY is omitted.  */
>     if (NILP (default_directory))
> -    default_directory = BVAR (current_buffer, directory);
> +    {
> +      default_directory = BVAR (current_buffer, directory);
> +      if (NILP (Ffile_name_absolute_p (default_directory)))

This will signal an error if default-directory is not a string.

> +       default_directory = Qnil;
> +    }
>     if (! STRINGP (default_directory))

Just move the test here.

Andreas.

--
Andreas Schwab, SUSE Labs, [hidden email]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."



Reply | Threaded
Open this post in threaded view
|

bug#36502: Fwd: bug#36502: 27.0.50; infinite loop in file-name-case-insensitive-p

Ken Brown-6
In reply to this post by Eli Zaretskii
On 7/8/2019 9:59 AM, Eli Zaretskii wrote:

>> From: Ken Brown <[hidden email]>
>> CC: "[hidden email]" <[hidden email]>, "[hidden email]"
>> <[hidden email]>, "[hidden email]" <[hidden email]>
>> Date: Mon, 8 Jul 2019 13:36:38 +0000
>>
>>>>       if (NILP (default_directory))
>>>> -    default_directory = BVAR (current_buffer, directory);
>>>> +    {
>>>> +      default_directory = BVAR (current_buffer, directory);
>>>> +      if (NILP (Ffile_name_absolute_p (default_directory)))
>>>> +       default_directory = Qnil;
>>>> +    }
>>>
>>> Hmm... why nullify it?  Why not simply call expand-file-name
>>> recursively?
>>
>> If the current buffer's default-directory is not absolute, then that variable is
>> invalid and we can't use it.
>
> I'm asking why not do this instead:
>
>    if (NILP (default_directory))
>      {
>        default_directory = BVAR (current_buffer, directory);
>        if (NILP (Ffile_name_absolute_p (default_directory)))
>          default_directory = Fexpand_file_name (default_directory,
>                                       Vinvocation_directory);
>      }

Oh, I see.  I misunderstood what you were suggesting.

> Or will the above not work for some reason?

I think something like this should work, with some care.  First,
invocation-directory might be nil, so we have to avoid an infinite loop in that
case.  Second, the code in emacs.c that sets Vinvocation_directory calls
Fexpand_file_name in some cases, so there's another potential infinite loop
resulting from that.

I'll see what I can come up with, also taking Andreas's comment into account.

Ken
Reply | Threaded
Open this post in threaded view
|

bug#36502: Fwd: bug#36502: 27.0.50; infinite loop in file-name-case-insensitive-p

Ken Brown-6
On 7/8/2019 11:17 AM, Ken Brown wrote:

> On 7/8/2019 9:59 AM, Eli Zaretskii wrote:
>> I'm asking why not do this instead:
>>
>>     if (NILP (default_directory))
>>       {
>>         default_directory = BVAR (current_buffer, directory);
>>         if (NILP (Ffile_name_absolute_p (default_directory)))
>>           default_directory = Fexpand_file_name (default_directory,
>>                                       Vinvocation_directory);
>>       }
>
> Oh, I see.  I misunderstood what you were suggesting.
>
>> Or will the above not work for some reason?
>
> I think something like this should work, with some care.  First,
> invocation-directory might be nil, so we have to avoid an infinite loop in that
> case.  Second, the code in emacs.c that sets Vinvocation_directory calls
> Fexpand_file_name in some cases, so there's another potential infinite loop
> resulting from that.
>
> I'll see what I can come up with, also taking Andreas's comment into account.

New attempt:

--- a/src/fileio.c
+++ b/src/fileio.c
@@ -804,7 +804,22 @@ DEFUN ("expand-file-name", Fexpand_file_name,
Sexpand_file_name, 1, 2, 0,

    /* Use the buffer's default-directory if DEFAULT_DIRECTORY is omitted.  */
    if (NILP (default_directory))
-    default_directory = BVAR (current_buffer, directory);
+    {
+      Lisp_Object dir = BVAR (current_buffer, directory);
+      /* The buffer's default-directory should be absolute.  If it
+        isn't, try to expand it relative to invocation-directory.
+        But we have to be careful to avoid an infinite loop, because
+        the code in emacs.c that sets Vinvocation_directory might
+        call Fexpand_file_name.  */
+      if (STRINGP (dir))
+       {
+         if (!NILP (Ffile_name_absolute_p (dir)))
+           default_directory = dir;
+         else if (STRINGP (Vinvocation_directory)
+                  && !NILP (Ffile_name_absolute_p (Vinvocation_directory)))
+           default_directory = Fexpand_file_name (dir, Vinvocation_directory);
+       }
+    }
    if (! STRINGP (default_directory))
      {
  #ifdef DOS_NT
Reply | Threaded
Open this post in threaded view
|

bug#36502: Fwd: bug#36502: 27.0.50; infinite loop in file-name-case-insensitive-p

Eli Zaretskii
> From: Ken Brown <[hidden email]>
> CC: "[hidden email]" <[hidden email]>, "[hidden email]"
> <[hidden email]>, "[hidden email]" <[hidden email]>, Andreas
>  Schwab <[hidden email]>
> Date: Mon, 8 Jul 2019 16:44:23 +0000
>
> --- a/src/fileio.c
> +++ b/src/fileio.c
> @@ -804,7 +804,22 @@ DEFUN ("expand-file-name", Fexpand_file_name,
> Sexpand_file_name, 1, 2, 0,
>
>     /* Use the buffer's default-directory if DEFAULT_DIRECTORY is omitted.  */
>     if (NILP (default_directory))
> -    default_directory = BVAR (current_buffer, directory);
> +    {
> +      Lisp_Object dir = BVAR (current_buffer, directory);
> +      /* The buffer's default-directory should be absolute.  If it
> +        isn't, try to expand it relative to invocation-directory.
> +        But we have to be careful to avoid an infinite loop, because
> +        the code in emacs.c that sets Vinvocation_directory might
> +        call Fexpand_file_name.  */
> +      if (STRINGP (dir))
> +       {
> +         if (!NILP (Ffile_name_absolute_p (dir)))
> +           default_directory = dir;
> +         else if (STRINGP (Vinvocation_directory)
> +                  && !NILP (Ffile_name_absolute_p (Vinvocation_directory)))
> +           default_directory = Fexpand_file_name (dir, Vinvocation_directory);
> +       }
> +    }
>     if (! STRINGP (default_directory))
>       {
>   #ifdef DOS_NT

LGTM, thanks.  Can we have a test for this subtle use case, so that we
never regress?



Reply | Threaded
Open this post in threaded view
|

bug#36502: Fwd: bug#36502: 27.0.50; infinite loop in file-name-case-insensitive-p

Ken Brown-6
On 7/8/2019 1:23 PM, Eli Zaretskii wrote:
> LGTM, thanks.  Can we have a test for this subtle use case, so that we
> never regress?

I just found another case where expand-file-name can yield a non-absolute
result.  Assuming there is no user "foo", we have

(let ((default-directory "~foo"))
   (expand-file-name "bar"))
=> "~foo/~foo/bar"

This is a ridiculous result, in addition to not being absolute.

Part of what's confusing here is that file-name-absolute-p returns t on file
names starting with "~", even though its doc string explicitly states that such
a file name is not absolute.

My inclination is that if default-directory starts with "~", then we should try
to convert it to a (truly) absolute file name.  If this doesn't work, then we
should forget the special interpretation of "~" and just treat default-directory
the same as any other relative name.  This means we would get
"<invocation-directory>/~foo" if invocation-directory is absolute, and "/~foo"
otherwise.

Does this seem reasonable?  Or are there other suggestions?

Ken
Reply | Threaded
Open this post in threaded view
|

bug#36502: Fwd: bug#36502: 27.0.50; infinite loop in file-name-case-insensitive-p

Richard Stallman
[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > (let ((default-directory "~foo"))
  >    (expand-file-name "bar"))
  > => "~foo/~foo/bar"

That is not right, but I think "~foo/bar" would be right.

  > Part of what's confusing here is that file-name-absolute-p returns
  > t on file names starting with "~", even though its doc string
  > explicitly states that such a file name is not absolute.

This contradiction is not good.  But I would like to point out
that, in a certain sesne, a name starting with ~ is absolute.
It is not relative to the current directory.

We may have a problem with two different meanings of "absolute",
each being pertinent to some situations.

  > My inclination is that if default-directory starts with "~", then we should try
  > to convert it to a (truly) absolute file name.

There may be some circumstances where that is better, but not always.



--
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)





12