vc with svn and git

vc with svn and git

[Alfred M. Szmidt]

If /home/ams contains a .svn directory, doing (vc-dir
"/home/ams/emacs") (which is git repository) results in:

  svn: warning: W155010: The node '/home/ams/emacs' was not found.
  
  svn: E200009: Could not display info for all targets because some targets don't exist

This patch puts the SVN backend to come into effect last.


2017-02-24  Alfred M. Szmidt  <ams@gnu.org>  (tiny change)

	* lisp/vc/vc-hooks.el (vc-handled-backends): Check SVN backend last.


diff --git a/lisp/vc/vc-hooks.el b/lisp/vc/vc-hooks.el
index 47e923c209..7da1244ef5 100644
--- a/lisp/vc/vc-hooks.el
+++ b/lisp/vc/vc-hooks.el
@@ -107,7 +107,7 @@ vc-ignore-dir-regexp
   :type 'regexp
   :group 'vc)
 
-(defcustom vc-handled-backends '(RCS CVS SVN SCCS SRC Bzr Git Hg Mtn)
+(defcustom vc-handled-backends '(RCS CVS SCCS SRC Bzr Git Hg Mtn SVN)
   ;; RCS, CVS, SVN, SCCS, and SRC come first because they are per-dir
   ;; rather than per-tree.  RCS comes first because of the multibackend
   ;; support intended to use RCS for local commits (with a remote CVS server).

Re: vc with svn and git

[Andreas Schwab]

On Feb 24 2017, ams@gnu.org (Alfred M. Szmidt) wrote:

> 2017-02-24  Alfred M. Szmidt  <ams@gnu.org>  (tiny change)
>
> 	* lisp/vc/vc-hooks.el (vc-handled-backends): Check SVN backend last.
>
>
> diff --git a/lisp/vc/vc-hooks.el b/lisp/vc/vc-hooks.el
> index 47e923c209..7da1244ef5 100644
> --- a/lisp/vc/vc-hooks.el
> +++ b/lisp/vc/vc-hooks.el
> @@ -107,7 +107,7 @@ vc-ignore-dir-regexp
>    :type 'regexp
>    :group 'vc)
>  
> -(defcustom vc-handled-backends '(RCS CVS SVN SCCS SRC Bzr Git Hg Mtn)
> +(defcustom vc-handled-backends '(RCS CVS SCCS SRC Bzr Git Hg Mtn SVN)
>    ;; RCS, CVS, SVN, SCCS, and SRC come first because they are per-dir
>    ;; rather than per-tree.  RCS comes first because of the multibackend
>    ;; support intended to use RCS for local commits (with a remote CVS server).

Did you read the comment?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: vc with svn and git

[Alfred M. Szmidt]

The comment is only true for Subversion < 1.7.  See
https://subversion.apache.org/docs/release-notes/1.7.html#single-db.

Re: vc with svn and git

[Karl Fogel]

ams@gnu.org (Alfred M. Szmidt) writes:
>If /home/ams contains a .svn directory, doing (vc-dir
>"/home/ams/emacs") (which is git repository) results in:
>
>  svn: warning: W155010: The node '/home/ams/emacs' was not found.
>  
>  svn: E200009: Could not display info for all targets because some targets don't exist
>
>This patch puts the SVN backend to come into effect last.

This surprised me.  I don't know the VC code well, but something seems odd here:

Shouldn't VC check in order from deeper to shallower anyway?  That is, if you run

  (vc-dir "/foo/bar/baz/qux")

then it should *first* check "qux", then "baz", then "bar", then "foo", in that order.  If there is a .git subdir anywhere along that upward climb, then the check can stop, because the answer is determined.  It doesn't done matter that some parent above might have an .svn dir, since SVN does not "shadow" a deeper Git (nor vice versa -- the example is the same if you swap .svn and .git).

So the fact that the order in which backends are listed in 'vc-handled-backends' affects anything in this scenario is surprising.  Also, going from deeper to shallower means you don't have to care whether SVN is pre- or post-1.7, i.e., whether the .svn administrative subdirs are per-directory or per-tree.  It works the same either way.

Now, if the way VC determines ownership is by invoking 'svn', 'git', etc externally on the full target, then yes, the order in which it tries backends would matter.  That would be problematic from an efficiency standpoint (and in if implemented without enough error-handling it also has correctness problem, as this thread indicates).  I suppose it has the advantage that it defers knowledge of version control ownership details to the program best positioned to have that knowledge, but in practice, I think ownership is always indicated by a dot-subdir of a certain name, right?  I think at this point we can count on that.  In any case, if an external program is going to be invoked, then if the first one returns error, VC should catch that error and just try the next backend in the list, and so
  on until the list is exhausted, rather than confront the user with the first error.

But IMHO it would be better just to look for the dot directories, in a deeper-to-shallower search.  Is there *any* supported VC backend that isn't just using a dot-directory to store version control metadata?

-Karl

>2017-02-24  Alfred M. Szmidt  <ams@gnu.org>  (tiny change)
>
>	* lisp/vc/vc-hooks.el (vc-handled-backends): Check SVN backend last.
>
>
>diff --git a/lisp/vc/vc-hooks.el b/lisp/vc/vc-hooks.el
>index 47e923c209..7da1244ef5 100644
>--- a/lisp/vc/vc-hooks.el
>+++ b/lisp/vc/vc-hooks.el
>@@ -107,7 +107,7 @@ vc-ignore-dir-regexp
>   :type 'regexp
>   :group 'vc)
> 
>-(defcustom vc-handled-backends '(RCS CVS SVN SCCS SRC Bzr Git Hg Mtn)
>+(defcustom vc-handled-backends '(RCS CVS SCCS SRC Bzr Git Hg Mtn SVN)
>   ;; RCS, CVS, SVN, SCCS, and SRC come first because they are per-dir
>   ;; rather than per-tree.  RCS comes first because of the multibackend
>   ;; support intended to use RCS for local commits (with a remote CVS server).

Re: vc with svn and git

[Stefan Monnier]

> Shouldn't VC check in order from deeper to shallower anyway?

Yes, it should.  It's a long standing limitation: the double loop is

    (forall backends
      (forall directory-parents
        ...))

instead of

    (forall directory-parents
      (forall backends
        ...))


-- Stefan

Re: vc with svn and git

[Alfred M. Szmidt]

   But IMHO it would be better just to look for the dot directories,
   in a deeper-to-shallower search.  

That would be a very nice thing.

   Is there *any* supported VC backend that isn't just using a
   dot-directory to store version control metadata?

The only thing that comes to mind is RCS/SCCS which can store the
version data beside the original file.

Re: vc with svn and git

[Karl Fogel]

ams@gnu.org (Alfred M. Szmidt) writes:
>   But IMHO it would be better just to look for the dot directories,
>   in a deeper-to-shallower search.  
>
>That would be a very nice thing.
>
>   Is there *any* supported VC backend that isn't just using a
>   dot-directory to store version control metadata?
>
>The only thing that comes to mind is RCS/SCCS which can store the
>version data beside the original file.

Ah, right.  Sorry, I asked the question in an oversimplified way.  What I really meant is: don't all supported VC backends store their metadata in a way that can be detected reliably in a walk from deeper to shallower?

The sibling ",v" and "s." files (for RCS- and SCCS-controlled files, respectively) can be detected that way, although in those cases it's a two-step dance:

  1) First see that both "foo" and "foo,v" (or "s.foo") are present
  2) Then invoke the corresponding backend to make absolutely sure

Stefan Monnier <monnier@iro.umontreal.ca> writes:
> Yes, it should.  It's a long standing limitation: the double loop is
> 
>     (forall backends
>       (forall directory-parents
>         ...))
> 
> instead of
> 
>     (forall directory-parents
>       (forall backends
>         ...))

Okay.  Thanks for confirming that there wasn't some subtle Reason Why Things Are The Way They Are going on here.

Alfred, want to have a try at that patch instead?  I think it might not be very large.  It looks like `vc-registered' in lisp/vc/vc-hooks.el is the key function.

I think there's a caching layer going on -- that appears to be what this block is about:

     ((and (boundp 'file-name-handler-alist)
          (setq handler (find-file-name-handler file 'vc-registered)))
      ;; handler should set vc-backend and return t if registered
      (funcall handler 'vc-registered file))

There's no reason the caching layer can't stay in place.  The only thing we're concerned with is when there's a cache miss; that's where we get to the `t' clause of the `cond':

     (t
      ;; There is no file name handler.
      ;; Try vc-BACKEND-registered for each handled BACKEND.
      ...)

My guess is that's the part Stefan is referring to above.  There's some extra complexity in that clause because (I think?) it uses `vc-file-getprop' to check if perhaps there's some special backend for this target or something -- so that's a second kind of cache check? -- but then we get to the heart of the matter, namely, the actual backend invocation:

	  (mapc
	   (lambda (b)
	     (and (vc-call-backend b 'registered file)
		  (vc-file-setprop file 'vc-backend b)
		  (throw 'found t)))

I don't fully understand the caching layer(s).  It almost looks like there are *two* possible caches to check: the `vc-registered' handler for that file in `file-name-handler-alist', and if that misses, then the `vc-backend' property maintained by the VC code itself?  But whatever's going on there, it can stay as is.  I think the entire fix here might be just to replace that 'mapc' call with a different loop, as per Stefan's comment above.

Best regards,
-Karl

Re: vc with svn and git

[Richard Copley]

On 24 February 2017 at 18:45, Karl Fogel <kfogel@red-bean.com> wrote:
> ams@gnu.org (Alfred M. Szmidt) writes:
>>   But IMHO it would be better just to look for the dot directories,
>>   in a deeper-to-shallower search.
>>
>>That would be a very nice thing.
>>
>>   Is there *any* supported VC backend that isn't just using a
>>   dot-directory to store version control metadata?
>>
>>The only thing that comes to mind is RCS/SCCS which can store the
>>version data beside the original file.
>
> Ah, right.  Sorry, I asked the question in an oversimplified way.  What I really meant is: don't all supported VC backends store their metadata in a way that can be detected reliably in a walk from deeper to shallower?
>
> The sibling ",v" and "s." files (for RCS- and SCCS-controlled files, respectively) can be detected that way, although in those cases it's a two-step dance:
>
>   1) First see that both "foo" and "foo,v" (or "s.foo") are present
>   2) Then invoke the corresponding backend to make absolutely sure
>
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> Yes, it should.  It's a long standing limitation: the double loop is
>>
>>     (forall backends
>>       (forall directory-parents
>>         ...))
>>
>> instead of
>>
>>     (forall directory-parents
>>       (forall backends
>>         ...))
>
> Okay.  Thanks for confirming that there wasn't some subtle Reason Why Things Are The Way They Are going on here.
>
> Alfred, want to have a try at that patch instead?  I think it might not be very large.  It looks like `vc-registered' in lisp/vc/vc-hooks.el is the key function.
>
> I think there's a caching layer going on -- that appears to be what this block is about:
>
>      ((and (boundp 'file-name-handler-alist)
>           (setq handler (find-file-name-handler file 'vc-registered)))
>       ;; handler should set vc-backend and return t if registered
>       (funcall handler 'vc-registered file))
>
> There's no reason the caching layer can't stay in place.  The only thing we're concerned with is when there's a cache miss; that's where we get to the `t' clause of the `cond':
>
>      (t
>       ;; There is no file name handler.
>       ;; Try vc-BACKEND-registered for each handled BACKEND.
>       ...)
>
> My guess is that's the part Stefan is referring to above.  There's some extra complexity in that clause because (I think?) it uses `vc-file-getprop' to check if perhaps there's some special backend for this target or something -- so that's a second kind of cache check? -- but then we get to the heart of the matter, namely, the actual backend invocation:
>
>           (mapc
>            (lambda (b)
>              (and (vc-call-backend b 'registered file)
>                   (vc-file-setprop file 'vc-backend b)
>                   (throw 'found t)))
>
> I don't fully understand the caching layer(s).  It almost looks like there are *two* possible caches to check: the `vc-registered' handler for that file in `file-name-handler-alist', and if that misses, then the `vc-backend' property maintained by the VC code itself?  But whatever's going on there, it can stay as is.  I think the entire fix here might be just to replace that 'mapc' call with a different loop, as per Stefan's comment above.
>
> Best regards,
> -Karl
>

There's no particular reason for even a single directory not to
contain more than one of .svn, .git, RCS, CVS subdirectories and/or ,v
and s. files.
VC should ask what backend to use in case of ambiguity, and there
should be a way to force reprompt.

This would also fix the example of a subdirectory called "rcs" which
is not a place for master files but simply a subdirectory for a
project called "rcs" (the MSYS2-packages repository
https://github.com/Alexpux/MSYS2-packages has such a subdirectory). On
case-insensitive filesystems it's not possible to use that repo
without temporarily taking RCS out of vc-handled backends.

Re: vc with svn and git

[Stefan Monnier]

>> I think there's a caching layer going on -- that appears to be what
>> this block is about:
>> 
>> ((and (boundp 'file-name-handler-alist)
>> (setq handler (find-file-name-handler file 'vc-registered)))
>> ;; handler should set vc-backend and return t if registered
>> (funcall handler 'vc-registered file))

This is not a cache.  It's a hook that was meant (I think) to support VC
on remote file names (or maybe even within tarballs).  I don't know that
it's ever been used (then again, I'm biased because I never liked this
hook, so don't take my word for it).

> There's no particular reason for even a single directory not to
> contain more than one of .svn, .git, RCS, CVS subdirectories and/or ,v
> and s. files.

That's right.  It's perfectly normal to have more than one backend
covering the same files.

> VC should ask what backend to use in case of ambiguity,

That tends to be rather annoying, in my experience.

> and there should be a way to force reprompt.

There is, it's called `vc-switch-backend` (except it doesn't prompt, but
lets you cycle, at least if it still works: I haven't used it in a long
while).


        Stefan

Re: vc with svn and git

[Richard Copley]

> That's right.  It's perfectly normal to have more than one backend
> covering the same files.
>
>> VC should ask what backend to use in case of ambiguity,
>
> That tends to be rather annoying, in my experience.

Hypothetically? You're probably right.

>> and there should be a way to force reprompt.
>
> There is, it's called `vc-switch-backend` (except it doesn't prompt, but
> lets you cycle, at least if it still works: I haven't used it in a long
> while).

Ah, thanks. Seems to work (for files, not directories).

Re: vc with svn and git

[Stefan Monnier]

>> That's right.  It's perfectly normal to have more than one backend
>> covering the same files.
>>> VC should ask what backend to use in case of ambiguity,
>> That tends to be rather annoying, in my experience.
> Hypothetically?

No: I've used a setup with combined CVS+RCS setup for quite a while
(hence `vc-switch-backend`), and tried the "prompt" route but even by
making the prompt lazy (i.e. only prompt when you want to actually run
a command) I found it too annoying (and making the prompt lazy is
actually hard to do with the current system, I only experimented with
it via big ugly hack).

> Ah, thanks. Seems to work (for files, not directories).

It predates vc-dir, but it shouldn't be difficult to make it work in
vc-dir.


        Stefan

Re: vc with svn and git

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> I think there's a caching layer going on -- that appears to be what
>>> this block is about:
>>>
>>> ((and (boundp 'file-name-handler-alist)
>>> (setq handler (find-file-name-handler file 'vc-registered)))
>>> ;; handler should set vc-backend and return t if registered
>>> (funcall handler 'vc-registered file))
>
> This is not a cache.  It's a hook that was meant (I think) to support VC
> on remote file names (or maybe even within tarballs).  I don't know that
> it's ever been used (then again, I'm biased because I never liked this
> hook, so don't take my word for it).

I don't know the situation with tarballs, but for Tramp it is
implemented and useful. Tramp's implementation uses some caching indeed
for performance improvement. See the comment in tramp-sh.el, heading
`tramp-sh-handle-vc-registered'.

>         Stefan

Best regards, Michael.

Re: vc with svn and git

[Stefan Monnier]

> I don't know the situation with tarballs, but for Tramp it is
> implemented and useful. Tramp's implementation uses some caching indeed
> for performance improvement. See the comment in tramp-sh.el, heading
> `tramp-sh-handle-vc-registered'.

That's odd.  Why doesn't it rely on the file-process and
start-file-process handlers?


        Stefan

Re: vc with svn and git

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I don't know the situation with tarballs, but for Tramp it is
>> implemented and useful. Tramp's implementation uses some caching indeed
>> for performance improvement. See the comment in tramp-sh.el, heading
>> `tramp-sh-handle-vc-registered'.
>
> That's odd.  Why doesn't it rely on the file-process and
> start-file-process handlers?

For backends calling processes in order to check `vc-registered', it
doesn't optimize. But for other backends, like cvs or svn, which do not
call processes, it is *really* a performance boost.

IIRC, it was recommended by a Tramp user running svn. If you're
interested, I could try to find old messages comparing performance
numbers w/ and w/o this optimization. Ten years ago, I guess.

>         Stefan

Best regards, Michael.

tramp-sh-handle-vc-registered (was: vc with svn and git)

[Stefan Monnier]

>> I don't know the situation with tarballs, but for Tramp it is
>> implemented and useful. Tramp's implementation uses some caching indeed
>> for performance improvement. See the comment in tramp-sh.el, heading
>> `tramp-sh-handle-vc-registered'.
> That's odd.  Why doesn't it rely on the file-process and
> start-file-process handlers?

OK, I now looked at the code and I see why it's used.  I guess the
caching part of tramp-sh-handle-vc-registered is not indispensable
(i.e. there must be a way to make the generic caching mechanism work
well enough for vc-registered), but batching all requests into a single
one is indeed pretty much impossible without some ad-hoc hack like this
one (which relies on the property that vc-registered doesn't have too
many side-effects, so you can do a "dry run" just to see what it would
do).

The only way I can see to get a similar result without ad-hoc hacks
would be to introduce async file operations and then make vc-registered
use them (since IIUC the benefit of tramp-sh-handle-vc-registered's
batching is to avoid paying the latency N times).  So vc-registered
would start by firing all file-exists-p tests (an other operations)
asynchronously and then collect their answers.


        Stefan

Re: vc with svn and git

[Alfred M. Szmidt]

   Alfred, want to have a try at that patch instead?  I think it might
   not be very large.  It looks like `vc-registered' in
   lisp/vc/vc-hooks.el is the key function.

I'm swamped currently. :(

Re: vc with svn and git

[Karl Fogel]

ams@gnu.org (Alfred M. Szmidt) writes:
>   Alfred, want to have a try at that patch instead?  I think it might
>   not be very large.  It looks like `vc-registered' in
>   lisp/vc/vc-hooks.el is the key function.
>
>I'm swamped currently. :(

Me too, alas.  But this thread has produced some useful knowledge:

1) The proposed improvement is welcome (i.e., no one considers the current way of doing things to be optimal).

2) Remember to correctly handle directories that are under the control of more than one VCS (I do that trick sometimes myself, so I'm embarrassed I didn't think of it in my original followup; thanks to Richard Copley for pointing it out).

3) A thing looked like it might be a caching layer is actually a hook system, that some people are using.