[patch] vc-find-root with invert

[mail-89]

Here's a patch that fixes a bug I've noticed in vc-find-root. When
called with `invert' equal to `t', the current implementation is
returning `file' verbatim if a file name is given, and also if `witness'
doesn't exist at all.

In the former case we should return the directory where witness resides,
and in the latter, `nil'. This patch implements such behaviour.

diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index ee073bc..5473538 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,8 @@
[phone]  Justin Bogner  <mail@...>
+
+ * vc-hooks.el (vc-find-root):
+ Find the directory when using invert, not the file itself.
+
 2008-07-04  Dan Nicolaescu  <dann@...>
 
  * vc-dir.el (vc-dir-query-replace-regexp): New function.
diff --git a/lisp/vc-hooks.el b/lisp/vc-hooks.el
index 2ccbdcc..0662961 100644
--- a/lisp/vc-hooks.el
+++ b/lisp/vc-hooks.el
@@ -327,39 +327,46 @@ The function walks up the directory tree from FILE looking for WITNESS.
 If WITNESS if not found, return nil, otherwise return the root.
 Optional arg INVERT non-nil reverses the sense of the check;
 the root is the last directory for which WITNESS *is* found."
-  ;; Represent /home/luser/foo as ~/foo so that we don't try to look for
-  ;; witnesses in /home or in /.
-  (setq file (abbreviate-file-name file))
-  (let ((root nil)
-        (prev-file 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)
-        try)
-    (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.
-                    (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)))
-      (setq try (file-exists-p (expand-file-name witness file)))
-      (cond ((and invert (not try)) (setq root prev-file))
-            ((and (not invert) try) (setq root file))
-            ((equal file (setq prev-file file
-                               file (file-name-directory
-                                     (directory-file-name file))))
-             (setq file nil))))
-    ;; Handle the case where ~/WITNESS exists and the original FILE is "~".
-    ;; (This occurs, for example, when placing dotfiles under RCS.)
-    (when (and (not root) invert prev-file)
-      (setq root prev-file))
-    root))
+  (when (not (file-directory-p file))
+    (setq file (file-name-directory file)))
+  ;; In the invert case, we should return nil if WITNESS doesn't exist
+  ;; in the first place.
+  (if (and invert
+           (not (file-exists-p (expand-file-name witness file))))
+      nil
+    ;; Represent /home/luser/foo as ~/foo so that we don't try to look for
+    ;; witnesses in /home or in /.
+    (setq file (abbreviate-file-name file))
+    (let ((root nil)
+          (prev-file 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)
+          try)
+      (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.
+                      (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)))
+        (setq try (file-exists-p (expand-file-name witness file)))
+        (cond ((and invert (not try)) (setq root prev-file))
+              ((and (not invert) try) (setq root file))
+              ((equal file (setq prev-file file
+                                 file (file-name-directory
+                                       (directory-file-name file))))
+               (setq file nil))))
+      ;; Handle the case where ~/WITNESS exists and the original FILE is "~".
+      ;; (This occurs, for example, when placing dotfiles under RCS.)
+      (when (and (not root) invert prev-file)
+        (setq root prev-file))
+      root)))
 
 ;; Access functions to file properties
 ;; (Properties should be _set_ using vc-file-setprop, but

[Stefan Monnier]

> Here's a patch that fixes a bug I've noticed in vc-find-root. When called
> with `invert' equal to `t', the current implementation is returning `file'
> verbatim if a file name is given, and also if `witness' doesn't exist
> at all.

Can someone remind me why we have this `invert' monster?


        Stefan

[mail-89]

Stefan Monnier wrote:
>> Here's a patch that fixes a bug I've noticed in vc-find-root. When called
>> with `invert' equal to `t', the current implementation is returning `file'
>> verbatim if a file name is given, and also if `witness' doesn't exist
>> at all.
>
> Can someone remind me why we have this `invert' monster?
>
>
>         Stefan

I believe that it's for systems like CVS, where each subdirectory under
version control has its own CVS directory.
--
Justin Bogner

[Stefan Monnier]

>>> Here's a patch that fixes a bug I've noticed in vc-find-root. When called
>>> with `invert' equal to `t', the current implementation is returning `file'
>>> verbatim if a file name is given, and also if `witness' doesn't exist
>>> at all.
>>
>> Can someone remind me why we have this `invert' monster?

> I believe that it's for systems like CVS, where each subdirectory under
> version control has its own CVS directory.

Sorry, but that doesn't help me ;-(
Anyone has a more complete explanation?


        Stefan

[Tomas Zerolo]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Fri, Jul 04, 2008 at 03:52:48PM -0400, Stefan Monnier wrote:
> >>> Here's a patch that fixes a bug I've noticed in vc-find-root. When called

[...]

> > I believe that it's for systems like CVS, where each subdirectory under
> > version control has its own CVS directory.
>
> Sorry, but that doesn't help me ;-(
> Anyone has a more complete explanation?

Reading the docstring, my guess would be: vc-find-root walks the dir
tree up _until_ it finds WITNESS (as an evidence for the VC tree's root
dir). Unless we have CVS or SVN or similar, where every subdir has
WITNESS, so the root would be the last up-tree directory having WITNESS.

But I might be off-by-one ;-)

Regards
- -- tomás
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iD8DBQFIbyD2Bcgs9XrR2kYRAoMlAJ9kLtd2W2uaZrjCHSiRNUZPxy9XRgCcDtYh
WIccVoYI/fT5wTr32KYunBs=
=3xNV
-----END PGP SIGNATURE-----

[mail-89]

tomas@... wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On Fri, Jul 04, 2008 at 03:52:48PM -0400, Stefan Monnier wrote:
>>>>> Here's a patch that fixes a bug I've noticed in vc-find-root. When called
>
> [...]
>
>>> I believe that it's for systems like CVS, where each subdirectory under
>>> version control has its own CVS directory.
>> Sorry, but that doesn't help me ;-(
>> Anyone has a more complete explanation?
>
> Reading the docstring, my guess would be: vc-find-root walks the dir
> tree up _until_ it finds WITNESS (as an evidence for the VC tree's root
> dir). Unless we have CVS or SVN or similar, where every subdir has
> WITNESS, so the root would be the last up-tree directory having WITNESS.
>
> But I might be off-by-one ;-)
>
> Regards
> - -- tomás
>

This is exactly correct, though in the current trunk there is a bug
such that the function does not behave like this. The patch causes it
to work as documented.

The changes are actually quite small, to demonstrate, the same patch
ignoring whitespace is attached, though the original should be applied
so that the indentation is correct.

diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index cf0472e..aebc149 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -384,6 +384,11 @@
  * textmodes/sgml-mode.el (sgml-font-lock-syntactic-keywords):
  Use syntax-ppss on a position *before* the char we want to change.
 
[phone]  Justin Bogner  <mail@...>
+
+ * vc-hooks.el (vc-find-root):
+ Find the directory when using invert, not the file itself.
+
 2008-07-04  Dan Nicolaescu  <dann@...>
 
  * vc-dir.el (vc-dir-query-replace-regexp): New function.
diff --git a/lisp/vc-hooks.el b/lisp/vc-hooks.el
index 2ccbdcc..0662961 100644
--- a/lisp/vc-hooks.el
+++ b/lisp/vc-hooks.el
@@ -327,6 +327,13 @@ The function walks up the directory tree from FILE looking for WITNESS.
 If WITNESS if not found, return nil, otherwise return the root.
 Optional arg INVERT non-nil reverses the sense of the check;
 the root is the last directory for which WITNESS *is* found."
+  (when (not (file-directory-p file))
+    (setq file (file-name-directory file)))
+  ;; In the invert case, we should return nil if WITNESS doesn't exist
+  ;; in the first place.
+  (if (and invert
+           (not (file-exists-p (expand-file-name witness file))))
+      nil
   ;; Represent /home/luser/foo as ~/foo so that we don't try to look for
   ;; witnesses in /home or in /.
   (setq file (abbreviate-file-name file))
@@ -359,7 +366,7 @@ the root is the last directory for which WITNESS *is* found."
     ;; (This occurs, for example, when placing dotfiles under RCS.)
     (when (and (not root) invert prev-file)
       (setq root prev-file))
-    root))
+      root)))
 
 ;; Access functions to file properties
 ;; (Properties should be _set_ using vc-file-setprop, but

[Stefan Monnier]

>> Reading the docstring, my guess would be: vc-find-root walks the dir
>> tree up _until_ it finds WITNESS (as an evidence for the VC tree's root
>> dir). Unless we have CVS or SVN or similar, where every subdir has
>> WITNESS, so the root would be the last up-tree directory having WITNESS.

That still doesn't tell me what it does.  I mean I can read the code and
understand what is the effect of calling the code like this, but I have
no idea what effect it has for the user.  AFAICT it's a misfeature.

> This is exactly correct, though in the current trunk there is a bug
> such that the function does not behave like this. The patch causes it
> to work as documented.

I think we should remove this misfeature rather than fix it.


        Stefan

[mail-89]

Stefan Monnier wrote:

>>> Reading the docstring, my guess would be: vc-find-root walks the dir
>>> tree up _until_ it finds WITNESS (as an evidence for the VC tree's root
>>> dir). Unless we have CVS or SVN or similar, where every subdir has
>>> WITNESS, so the root would be the last up-tree directory having WITNESS.
>
> That still doesn't tell me what it does.  I mean I can read the code and
> understand what is the effect of calling the code like this, but I have
> no idea what effect it has for the user.  AFAICT it's a misfeature.
>
>> This is exactly correct, though in the current trunk there is a bug
>> such that the function does not behave like this. The patch causes it
>> to work as documented.
>
> I think we should remove this misfeature rather than fix it.
>
>
>         Stefan
>

There are two types of version control directory, ones with a "root"
directory in each directory under version control, and on with a
"root" directory in the root of the version control. Since this
function's purpose is to find the actual root of the tree, the latter
case is simple, where it needs only find the "root" directory.

However, for the other case, we need traverse upwards until we don't
find the directory, returning the last one that does. As it stands, if
we don't find the directory we look, we return wherever we started
searching, which is incorrect.

As far as getting rid of invert, we could do that, and the function
would then return something more reasonable than it does now, but it
wouldn't actually find the root unless you happened to try the root
first. Even so, this would make it work slightly better in some cases
(at least it would be `nil' when it should be). It would also consider
subdirectories that weren't in CVS to be part of CVS, which is probably
wrong, but possibly not common (I don't know about this).

I'm not convinced that `invert' is a misfeature, as it is certainly a
good intention. However, if you are convinced that it's not useful,
then I can come up with a patch that removes it, since I would prefer
not having it to having a broken version as it is now.

[David Kastrup]

Justin Bogner <mail@...> writes:

> Stefan Monnier wrote:
>>>> Reading the docstring, my guess would be: vc-find-root walks the dir
>>>> tree up _until_ it finds WITNESS (as an evidence for the VC tree's root
>>>> dir). Unless we have CVS or SVN or similar, where every subdir has
>>>> WITNESS, so the root would be the last up-tree directory having WITNESS.
>>
>> That still doesn't tell me what it does.  I mean I can read the code and
>> understand what is the effect of calling the code like this, but I have
>> no idea what effect it has for the user.  AFAICT it's a misfeature.
>>
>>> This is exactly correct, though in the current trunk there is a bug
>>> such that the function does not behave like this. The patch causes it
>>> to work as documented.
>>
>> I think we should remove this misfeature rather than fix it.
>
> There are two types of version control directory, ones with a "root"
> directory in each directory under version control, and on with a
> "root" directory in the root of the version control. Since this
> function's purpose is to find the actual root of the tree, the latter
> case is simple, where it needs only find the "root" directory.
>
> However, for the other case, we need traverse upwards until we don't
> find the directory, returning the last one that does.

Why?  CVS or SVN do not do this either.  Subdirectories with .svn in
them are self-contained work directories with associated repository
location.

> As far as getting rid of invert, we could do that, and the function
> would then return something more reasonable than it does now, but it
> wouldn't actually find the root unless you happened to try the root
> first.

There is no dedicated root in CVS or SVN.  You never need to look at
.. in order to do local operations.  You can move your directory out to
a different location under a different "root" and things will work just
the same from there.

Which is pretty much the principal reason for every directory having its
own CVS or .svn subdirectories.

--
David Kastrup, Kriemhildstr. 15, 44793 Bochum

[mail-89]

David Kastrup wrote:

> Justin Bogner <mail@...> writes:
>> However, for the other case, we need traverse upwards until we don't
>> find the directory, returning the last one that does.
>
> Why?  CVS or SVN do not do this either.  Subdirectories with .svn in
> them are self-contained work directories with associated repository
> location.
>
>> As far as getting rid of invert, we could do that, and the function
>> would then return something more reasonable than it does now, but it
>> wouldn't actually find the root unless you happened to try the root
>> first.
>
> There is no dedicated root in CVS or SVN.  You never need to look at
> .. in order to do local operations.  You can move your directory out to
> a different location under a different "root" and things will work just
> the same from there.
>
> Which is pretty much the principal reason for every directory having its
> own CVS or .svn subdirectories.
>

Is this still true if you need to do nonlocal operations? If so then this is
indeed a misfeature.

ps: sent to David and forgot to CC emacs-devel, sorry.

[David Kastrup]

Justin Bogner <mail@...> writes:

> David Kastrup wrote:
>> Justin Bogner <mail@...> writes:
>>> However, for the other case, we need traverse upwards until we don't
>>> find the directory, returning the last one that does.
>>
>> Why?  CVS or SVN do not do this either.  Subdirectories with .svn in
>> them are self-contained work directories with associated repository
>> location.
>>
>>> As far as getting rid of invert, we could do that, and the function
>>> would then return something more reasonable than it does now, but it
>>> wouldn't actually find the root unless you happened to try the root
>>> first.
>>
>> There is no dedicated root in CVS or SVN.  You never need to look at
>> .. in order to do local operations.  You can move your directory out to
>> a different location under a different "root" and things will work just
>> the same from there.
>>
>> Which is pretty much the principal reason for every directory having its
>> own CVS or .svn subdirectories.
>>
>
> Is this still true if you need to do nonlocal operations?

With "local" I meant "local to the current tree location" whether in the
work directory or repository, not "local to this computer".

> If so then this is indeed a misfeature.

The only sort of justification I could imagine is when you are trying to
create a patch and/or work with a file set.  Then you want to have some
sort of common root.  However, just walking upwards is not guaranteed to
give you such a root: it is perfectly feasible to place some directory
under CVS for backup purposes, but have a non-registered subdirectory
_also_ under CVS with a completely different repository.

--
David Kastrup, Kriemhildstr. 15, 44793 Bochum

[Stefan Monnier]

> However, for the other case, we need traverse upwards until we don't
> find the directory, returning the last one that does. As it stands, if
> we don't find the directory we look, we return wherever we started
> searching, which is incorrect.

It's not incorrect at all.  It's how CVS and Svn work.  They don't have
a "root directory".  you may have /foo/baqr/CVS and /foo/CVS, but it
doesn't mean /foo is the root: the two may be simply unrelated.

I.e. it's tryuing to invent something that's simply not there.  No other
CVS or Svn front-end does that as far I know.  The user specifies the
root he wants to use when it starts vc-dir: Emacs doesn't need to
guess it.

I.e. VC's "root dir" is not something for the UI but something for
internal use, fpor those backends that need it (and indeed, it didn't
exist when VC only supported CVS, RCS, and SCCS).


        Stefan

[mail-89]

David Kastrup wrote:
> The only sort of justification I could imagine is when you are trying to
> create a patch and/or work with a file set.  Then you want to have some
> sort of common root.  However, just walking upwards is not guaranteed to
> give you such a root: it is perfectly feasible to place some directory
> under CVS for backup purposes, but have a non-registered subdirectory
> _also_ under CVS with a completely different repository.
>

This is a very valid point, and given Stefan's argument that these
version control systems don't have such concepts, I am somewhat
dismayed. The concept of a root directory of a project is extremely
useful for things like recursive grep, tags, etc. I find it quite
helpful to set the default-directory to the vc root in such circumstances.

Oh well, given the arguments, I vote that this is a misfeature and
should be removed.

[Stefan Monnier]

> This is a very valid point, and given Stefan's argument that these version
> control systems don't have such concepts, I am somewhat dismayed. The
> concept of a root directory of a project is extremely useful for things like
> recursive grep, tags, etc. I find it quite helpful to set the
> default-directory to the vc root in such circumstances.

I agree it's useful.  But there's no reason why this should be
implicitly inferred/guessed from the structure of the revision control
info.  Even with VCS such as Bazaar or Git, you can have a single
project decomposed into several checkouts of various Bazaar and/or Git
subprojects (in Arch, this was supported explicitly (tho poorly) via the
notion of "config").

> Oh well, given the arguments, I vote that this is a misfeature and should
> be removed.

I'm glad we agree.


        Stefan