Proposed fix for file-saving bug (VC queries remote repository).

Proposed fix for file-saving bug (VC queries remote repository).

[Karl Fogel]

[-- Attachment #1: Type: text/plain, Size: 2956 bytes --]

I know the fix I want to make here, but the code looks like it's
intentionally organized the way it is, so I thought I'd check.

First, this is the bug in today's Emacs (38aaf904c7 on master) -- it's
rather high-severity, since it interferes with saving files:

If you try save a modified buffer visiting an SVN-controlled file
("README.md" in this example), but you have no connectivity to the
upstream repository, the save will spin for a while and then error:

  Running svn --non-interactive status -u README.md...FAILED (status 1)

The cause is clear in the code.  Saving the buffer invokes this call
chain:

  (vc-after-save)
    ==> (vc-state-refresh "/path/to/README.md" 'SVN)
      ==> (vc-call-backend 'SVN 'state "/path/to/README.md")
        ==> (vc-svn-state "/path/to/README.md")
          ==> (vc-svn-command t 0 "/path/to/README.md" "status" "-u")

The problem is the "-u" flag there.  As an option to "svn status", "-u"
means "ask the remote repository for any updates".  If you don't have
network access to the remote repository, svn will hang for a while.

In the Emacs Lisp VC code, the problem is that when `vc-call-backend'
dispatches to `vc-svn-state', the latter has no way of receiving the
`localp' flag:

  (defun vc-svn-state (file &optional localp)
    "SVN-specific version of `vc-state'."
    (let (process-file-side-effects)
      (with-temp-buffer
        (cd (file-name-directory file))
        (vc-svn-command t 0 file "status" (if localp "-v" "-u"))
        (vc-svn-parse-status file))))

In the call path, there's not really any way to insert that flag either.

I see two possible solutions here:

  1) Remove the optional `local' flag from `vc-svn-state' and 
     just always pass "-v" to "svn status", never "-u".

  2) Create a new backend action called `local-state' or `quick-state',
     and have `vc-state-refresh' pass that instead of `state'.

I prefer (1), but maybe that's because I don't know where VC is actually
using that "-u" in `vc-svn-state'?  On the other hand, I don't see any
explicit callers of `vc-svn-state' in Emacs, other than
`vc-call-backend', so maybe the "-u" is just obsolete now?

Here's why I think the solution is one of the above two:

The naive patch for this bug would be to add the `local' flag earlier in
the call chain (I've attached an example of the naive patch, just to
show why it doesn't quite work).  The problem is that while we know what
the `local' flag means for `vc-svn-state' in the SVN-specific backend,
we can't just pass it to other backends hoping it will have the same
effect -- in fact, other backends don't even have an optional `local'
argument to their state actions (see `vc-git-state' and `vc-bzr-state'
for example).  So presumably it would just cause an error for other
backends ("wrong number of arguments").

So, how about it?  Can I just remove the non-local option from
`vc-svn-state' and solve this the simple way?  Pretty please? :-)

Best,
-Karl


[-- Attachment #2: Naive patch (DO NOT APPLY) for VC bug under discussion. --]
[-- Type: text/plain, Size: 1726 bytes --]

[[[
Naive way to prevent VC from contacting remote repository when saving
a file.  *** FOR DISCUSSION ONLY; DO NOT APPLY THIS PATCH ***

* lisp/vc/vc-hooks.el 
  (vc-state-refresh): Take new 'local flag, pass it along to
    `vc-call-backend'... But this is problematic because
    it probably doesn't mean the right thing for backends 
    other than SVN.
  (vc-after-save): Pass new flag to `vc-state-refresh'.
]]]

diff --git a/lisp/vc/vc-hooks.el b/lisp/vc/vc-hooks.el
index 61918c9..03a45eb 100644
--- a/lisp/vc/vc-hooks.el
+++ b/lisp/vc/vc-hooks.el
@@ -486,11 +486,13 @@ status of this file.  Otherwise, the value returned is one of:
 	(when backend
           (vc-state-refresh file backend)))))
 
-(defun vc-state-refresh (file backend)
-  "Quickly recompute the `state' of FILE."
+(defun vc-state-refresh (file backend &optional local)
+  "Quickly recompute the `state' of FILE.
+Optional argument LOCAL means compute state in a way that does
+not involve contacting any remote repository."
   (vc-file-setprop
    file 'vc-state
-   (vc-call-backend backend 'state file)))
+   (vc-call-backend backend 'state file local)))
 
 (defsubst vc-up-to-date-p (file)
   "Convenience function that checks whether `vc-state' of FILE is `up-to-date'."
@@ -661,7 +663,7 @@ Before doing that, check if there are any old backups and get rid of them."
       (if (equal (vc-file-getprop file 'vc-checkout-time)
 		 (nth 5 (file-attributes file)))
 	  (vc-file-setprop file 'vc-checkout-time nil))
-      (if (vc-state-refresh file backend)
+      (if (vc-state-refresh file backend 'local)
 	  (vc-mode-line file backend)))
      ;; If we saved an unlocked file on a locking based VCS, that
      ;; file is not longer up-to-date.

Re: Proposed fix for file-saving bug (VC queries remote repository).

[Karl Fogel]

Should have added:

I expected this bug to be related to this recent thread:

  "Re: master 2a81c5d 1/2: Confine vc-stay-local to CVS, because it was unusable in SVN."
  http://lists.gnu.org/archive/html/emacs-devel/2014-12/threads.html#00028

...but when I looked over the thread, it actually didn't seem that
related.  However, the fact that vc-stay-local got pushed down into the
CVS backend indicates that the `local' flag remaining in `vc-svn-state'
might just be an oversight.

I'm happy to remove the flag, and the "-u" functionality that it
suppresses, unless anyone objects.

-K

>I know the fix I want to make here, but the code looks like it's
>intentionally organized the way it is, so I thought I'd check.
>
>First, this is the bug in today's Emacs (38aaf904c7 on master) -- it's
>rather high-severity, since it interferes with saving files:
>
>If you try save a modified buffer visiting an SVN-controlled file
>("README.md" in this example), but you have no connectivity to the
>upstream repository, the save will spin for a while and then error:
>
>  Running svn --non-interactive status -u README.md...FAILED (status 1)
>
>The cause is clear in the code.  Saving the buffer invokes this call
>chain:
>
>  (vc-after-save)
>    ==> (vc-state-refresh "/path/to/README.md" 'SVN)
>      ==> (vc-call-backend 'SVN 'state "/path/to/README.md")
>        ==> (vc-svn-state "/path/to/README.md")
>          ==> (vc-svn-command t 0 "/path/to/README.md" "status" "-u")
>
>The problem is the "-u" flag there.  As an option to "svn status", "-u"
>means "ask the remote repository for any updates".  If you don't have
>network access to the remote repository, svn will hang for a while.
>
>In the Emacs Lisp VC code, the problem is that when `vc-call-backend'
>dispatches to `vc-svn-state', the latter has no way of receiving the
>`localp' flag:
>
>  (defun vc-svn-state (file &optional localp)
>    "SVN-specific version of `vc-state'."
>    (let (process-file-side-effects)
>      (with-temp-buffer
>        (cd (file-name-directory file))
>        (vc-svn-command t 0 file "status" (if localp "-v" "-u"))
>        (vc-svn-parse-status file))))
>
>In the call path, there's not really any way to insert that flag either.
>
>I see two possible solutions here:
>
>  1) Remove the optional `local' flag from `vc-svn-state' and 
>     just always pass "-v" to "svn status", never "-u".
>
>  2) Create a new backend action called `local-state' or `quick-state',
>     and have `vc-state-refresh' pass that instead of `state'.
>
>I prefer (1), but maybe that's because I don't know where VC is actually
>using that "-u" in `vc-svn-state'?  On the other hand, I don't see any
>explicit callers of `vc-svn-state' in Emacs, other than
>`vc-call-backend', so maybe the "-u" is just obsolete now?
>
>Here's why I think the solution is one of the above two:
>
>The naive patch for this bug would be to add the `local' flag earlier in
>the call chain (I've attached an example of the naive patch, just to
>show why it doesn't quite work).  The problem is that while we know what
>the `local' flag means for `vc-svn-state' in the SVN-specific backend,
>we can't just pass it to other backends hoping it will have the same
>effect -- in fact, other backends don't even have an optional `local'
>argument to their state actions (see `vc-git-state' and `vc-bzr-state'
>for example).  So presumably it would just cause an error for other
>backends ("wrong number of arguments").
>
>So, how about it?  Can I just remove the non-local option from
>`vc-svn-state' and solve this the simple way?  Pretty please? :-)

Re: Proposed fix for file-saving bug (VC queries remote repository).

[Eric S. Raymond]

Karl Fogel <kfogel@red-bean.com>:
> So, how about it?  Can I just remove the non-local option from
> `vc-svn-state' and solve this the simple way?  Pretty please? :-)

Yes, go ahead.
-- 
		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>

Re: Proposed fix for file-saving bug (VC queries remote repository).

[Eric S. Raymond]

Karl Fogel <kfogel@red-bean.com>:
> I'm happy to remove the flag, and the "-u" functionality that it
> suppresses, unless anyone objects.

Works for me.  You're the expert on SVN interfacing, as far as I'm concerned.
-- 
		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>

Re: Proposed fix for file-saving bug (VC queries remote repository).

[Karl Fogel]

"Eric S. Raymond" <esr@thyrsus.com> writes:
>Karl Fogel <kfogel@red-bean.com>:
>> I'm happy to remove the flag, and the "-u" functionality that it
>> suppresses, unless anyone objects.
>
>Works for me.  You're the expert on SVN interfacing, as far as I'm concerned.

Done in commit b3298507f92f8cc17dc35090e4036aac787af682.  Thanks for the
quick answer, Eric.

Best,
-Karl