vc-find-root and nonexistent drives

vc-find-root and nonexistent drives

[Juanma Barranquero]

On Windows, trying to visit a file in a non-existent drive loops in
`vc-find-root'.

The reason is that currently, `vc-find-root' does:

  (while (not (file-directory-p file))
    (setq file (file-name-directory (directory-file-name file))))

which assumes that the output of `file-name-directory' will be
different in each iteration of the while loop. That is not so when the
drive does not exist, for example:

  (file-name-directory (directory-file-name "g:/")) => "g:/"

This is not an obscure bug. "C-x C-f g:/myfile", o "emacsclient
g:/myfile" will trigger it if g: does not exist. It happens in
EMACS_22_BASE too.

             Juanma

Re: vc-find-root and nonexistent drives

[Eli Zaretskii]

> Date: Sat, 16 Feb 2008 04:52:35 +0100
> From: "Juanma Barranquero" <lekktu@gmail.com>
> 
> On Windows, trying to visit a file in a non-existent drive loops in
> `vc-find-root'.
> 
> The reason is that currently, `vc-find-root' does:
> 
>   (while (not (file-directory-p file))
>     (setq file (file-name-directory (directory-file-name file))))
> 
> which assumes that the output of `file-name-directory' will be
> different in each iteration of the while loop. That is not so when the
> drive does not exist, for example:
> 
>   (file-name-directory (directory-file-name "g:/")) => "g:/"

So how about changing the loop condition to

  (while (and (not (file-directory-p file)) (file-exists-p file))

?

Re: vc-find-root and nonexistent drives

[Stefan Monnier]

> The reason is that currently, `vc-find-root' does:

>   (while (not (file-directory-p file))
>     (setq file (file-name-directory (directory-file-name file))))

> which assumes that the output of `file-name-directory' will be
> different in each iteration of the while loop.

I don't follow.  The code in the 22 branch does:

    (while (not (or root
                    (null file)
                    ;; As a heuristic, we stop looking up the hierarchy of
                    ;; directories as soon as we find a directory belonging
                    ;; to another user.  This should save us from looking in
                    ;; things like /net and /afs.  This assumes that all the
                    ;; files inside a project belong to the same user.
                    (not (equal user (nth 2 (file-attributes file))))
                    (string-match vc-ignore-dir-regexp file)))
      (if (file-exists-p (expand-file-name witness file))
          (setq root file)
        (if (equal file
                   (setq file (file-name-directory (directory-file-name file))))
            (setq file nil))))

where you see (last 3 lines) that it checks that `file' does change at
each iteration.

> That is not so when the
> drive does not exist, for example:
>   (file-name-directory (directory-file-name "g:/")) => "g:/"

So the `equal' test should trigger and force exiting the loop.
I.e. there much be something else at play here.


        Stefan

Re: vc-find-root and nonexistent drives

[Juanma Barranquero]

On Feb 16, 2008 3:29 PM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> I don't follow.  The code in the 22 branch does:
>
>     (while (not (or root
>                     (null file)
[snip]

> where you see (last 3 lines) that it checks that `file' does change at
> each iteration.

I think you misread my message. What I'm talking about happens before,
in the first while loop of `vc-find-root'. It is written assuming that
you can keep taking out parts of a filename until you eventually end
with a valid directory, because / always exists. But g:/ does not need
to exist on Windows.

             Juanma

Re: vc-find-root and nonexistent drives

[Juanma Barranquero]

On Feb 16, 2008 2:43 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> So how about changing the loop condition to
>
>   (while (and (not (file-directory-p file)) (file-exists-p file))
>
> ?

Yes, you're right. On one hand, I don't like much touching VC code; on
the other, I wrote my message at 04:52am, which means that, at that
moment, I wasn't aware of `file-exists-p' (I fell asleep over my
keyboard once or twice before that) :)

I'm checking your fix in the EMACS_22_BASE branch.

             Juanma

Re: vc-find-root and nonexistent drives

[Stefan Monnier]

>> So how about changing the loop condition to
>> 
>> (while (and (not (file-directory-p file)) (file-exists-p file))
>> 
>> ?

No, that would defeat the whole purpose of this loop which is to walk up
the hierarchy until we find a real directory.  E.g. walk up from
/some/dir/some/not/yet/existing/subdir/file up to /some/dir.

I'll commit a fix.


        Stefan

Re: vc-find-root and nonexistent drives

[Juanma Barranquero]

On Feb 16, 2008 9:16 PM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> No, that would defeat the whole purpose of this loop which is to walk up
> the hierarchy until we find a real directory.  E.g. walk up from
> /some/dir/some/not/yet/existing/subdir/file up to /some/dir.

Hmm. That's why I don't like changing VC.

> I'll commit a fix.

I'll remove the erroneous patch.

             Juanma

Re: vc-find-root and nonexistent drives

[Stefan Monnier]

> I think you misread my message. What I'm talking about happens before,
> in the first while loop of `vc-find-root'. It is written assuming that
> you can keep taking out parts of a filename until you eventually end
> with a valid directory, because / always exists. But g:/ does not need
> to exist on Windows.

Thanks.  Yes indeed, this initial loop is problematic because it does
not take the precautions that we added to the main loop.

Can you try the patch below?


        Stefan


--- vc-hooks.el.~1.182.2.14.~	2008-01-21 12:15:18.000000000 -0500
+++ vc-hooks.el	2008-02-16 15:30:42.000000000 -0500
@@ -316,11 +316,12 @@
 If WITNESS if not found, return nil, otherwise return the root."
   ;; Represent /home/luser/foo as ~/foo so that we don't try to look for
   ;; witnesses in /home or in /.
-  (while (not (file-directory-p file))
-    (setq file (file-name-directory (directory-file-name file))))
   (setq file (abbreviate-file-name file))
   (let ((root nil)
-        (user (nth 2 (file-attributes file))))
+        ;; `user' is not initialized outside the loop because
+        ;; `file' may not exist, so we may have to walk up part of the
+        ;; hierarchy before we find the "initial UID".
+        (user nil))
     (while (not (or root
                     (null file)
                     ;; As a heuristic, we stop looking up the hierarchy of
@@ -328,7 +329,9 @@
                     ;; to another user.  This should save us from looking in
                     ;; things like /net and /afs.  This assumes that all the
                     ;; files inside a project belong to the same user.
-                    (not (equal user (nth 2 (file-attributes file))))
+                    (let ((prev-user user))
+                      (setq user (nth 2 (file-attributes file)))
+                      (and prev-user (not (equal user prev-user))))
                     (string-match vc-ignore-dir-regexp file)))
       (if (file-exists-p (expand-file-name witness file))
           (setq root file)

Re: vc-find-root and nonexistent drives

[Juanma Barranquero]

On Feb 16, 2008 9:33 PM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> Can you try the patch below?

If fixes the original use case (C-x C-f g:/, or emacsclient g:/).

             Juanma

Re: vc-find-root and nonexistent drives

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sat, 16 Feb 2008 15:33:15 -0500
> Cc: Emacs Devel <emacs-devel@gnu.org>
> 
> Can you try the patch below?

I'd suggest to mention MS-Windows and the specific problem mentioned
by Juanma in the comments.

Re: vc-find-root and nonexistent drives

[Stefan Monnier]

>> Can you try the patch below?

> I'd suggest to mention MS-Windows and the specific problem mentioned
> by Juanma in the comments.

I suspect the bug can also be triggered with other systems.


        Stefan