bug#20637: incompatible, undocumented change to vc-working-revision

[Michael Albinus <michael.albinus@gmx.de>]

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

Dmitry Gutov <dgutov@yandex.ru> writes:

> This has been caused by the commit
> 7f9b037245ddb662ad98685e429a2498ae6b7c62, which made both vc-state and
> vc-working-revision use vc-responsible-backend instead of vc-backend.

Yes.

> As a result, in some backends these functions started return non-nil
> values for unknown files or directories, as long as they lie inside a
> VC repository.

vc-working-revision shall return nil for unregistered files. vc-state
shall return a non-nil value, 'unregistered.

> This change is indeed backward-incompatible, and it breaks the
> previous assumption of some backend functions that if FILE has been
> passed to it, then it's surely registered with the current
> backend.

You cannot guarantee this. Anybody is free to call the functions with
unregistered files. And in the vc-state case, it is even documented that
this could happen.

> In particular, it breaks an assumption I made when fixing #11757, that
> vc-git-state never receives an unregistered file as input. So if you
> evaluate (vc-state "1") now, it'll return `up-to-date'.

This assumption could be kept if vc-state filters such unregistered
files out.

> While reverting the change makes some tests fail, we should fix them
> in different ways.
>
> For some backends, maybe, we should accept that (vc-state
> default-directory) and (vc-working-revision default-directory) will
> return nil. Alternatively, fix that problem inside the respective
> backends, without changing the dispatching functions.
>
> Also, reverting this commit also seems to uncover tests that shouldn't
> pass anyway. Checks like
>
>   (should (eq (vc-state default-directory)
>               (vc-state default-directory backend)))
>
> don't verify much, and in this case they seem to verify the wrong
> thing. More on that in the respective threads in emacs-devel later.
>
> Michael, thoughts?

I've prepared a patch which just covers the case that a file is
unregistered, in both vc-state and vc-working-revision. It is a very
small change, that I believe it could still go into the emacs-25 branch.

Patch towards emacs-25 branch is appended, including modification of
vc-tests.el. Comments?

Best regards, Michael.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: diff --]
[-- Type: text/x-patch, Size: 7217 bytes --]

diff --git a/lisp/vc/vc-hooks.el b/lisp/vc/vc-hooks.el
index 0826744..dc7cc61 100644
--- a/lisp/vc/vc-hooks.el
+++ b/lisp/vc/vc-hooks.el
@@ -475,10 +475,11 @@ vc-state
   ;; FIXME: New (sub)states needed (?):
   ;; - `copied' and `moved' (might be handled by `removed' and `added')
   (or (vc-file-getprop file 'vc-state)
+      (and (not (vc-registered file)) 'unregistered)
       (when (> (length file) 0)         ;Why??  --Stef
 	(setq backend (or backend (vc-responsible-backend file)))
 	(when backend
-          (vc-state-refresh file backend)))))
+	  (vc-state-refresh file backend)))))

 (defun vc-state-refresh (file backend)
   "Quickly recompute the `state' of FILE."
@@ -494,11 +495,12 @@ vc-working-revision
   "Return the repository version from which FILE was checked out.
 If FILE is not registered, this function always returns nil."
   (or (vc-file-getprop file 'vc-working-revision)
-      (progn
-	(setq backend (or backend (vc-responsible-backend file)))
-	(when backend
-	  (vc-file-setprop file 'vc-working-revision
-			   (vc-call-backend backend 'working-revision file))))))
+      (and (vc-registered file)
+	   (progn
+	     (setq backend (or backend (vc-responsible-backend file)))
+	     (when backend
+	       (vc-file-setprop file 'vc-working-revision
+				(vc-call-backend backend 'working-revision file)))))))

 ;; Backward compatibility.
 (define-obsolete-function-alias
diff --git a/test/automated/vc-tests.el b/test/automated/vc-tests.el
index 2faa143..7f6196b 100644
--- a/test/automated/vc-tests.el
+++ b/test/automated/vc-tests.el
@@ -285,10 +285,9 @@ vc-test--state
 	  (make-directory default-directory)
 	  (vc-test--create-repo-function backend)

-	  ;; nil: Hg Mtn RCS
-          ;; added: Git
-          ;; unregistered: CVS SCCS SRC
-	  ;; up-to-date: Bzr SVN
+	  ;; nil: RCS
+          ;; unregistered: Bzr CVS Git Hg Mtn SCCS SRC
+	  ;; up-to-date: SVN
           (message "vc-state1 %s" (vc-state default-directory))
 	  (should (eq (vc-state default-directory)
 		      (vc-state default-directory backend)))
@@ -298,51 +297,43 @@ vc-test--state
 	  (let ((tmp-name (expand-file-name "foo" default-directory)))
 	    ;; Check state of an empty file.

-	    ;; nil: Hg Mtn SRC SVN
-            ;; added: Git
-	    ;; unregistered: RCS SCCS
-	    ;; up-to-date: Bzr CVS
+	    ;; unregistered: Bzr CVS Git Hg Mtn RCS SCCS SRC SVN
             (message "vc-state2 %s" (vc-state tmp-name))
 	    (should (eq (vc-state tmp-name) (vc-state tmp-name backend)))
-	    (should (memq (vc-state tmp-name)
-			  '(nil added unregistered up-to-date)))
+	    (should (eq (vc-state tmp-name) 'unregistered))

 	    ;; Write a new file.  Check state.
 	    (write-region "foo" nil tmp-name nil 'nomessage)

-            ;; nil: Mtn
-            ;; added: Git
-            ;; unregistered: Hg RCS SCCS SRC SVN
-            ;; up-to-date: Bzr CVS
+            ;; unregistered: Bzr CVS Git Hg Mtn RCS SCCS SRC SVN
             (message "vc-state3 %s" (vc-state tmp-name))
 	    (should (eq (vc-state tmp-name) (vc-state tmp-name backend)))
-	    (should (memq (vc-state tmp-name)
-			  '(nil added unregistered up-to-date)))
+	    (should (eq (vc-state tmp-name) 'unregistered))

 	    ;; Register a file.  Check state.
 	    (vc-register
 	     (list backend (list (file-name-nondirectory tmp-name))))

-            ;; added: Git Mtn
-            ;; unregistered: Hg RCS SCCS SRC SVN
-            ;; up-to-date: Bzr CVS
+	    ;; nil: SRC
+            ;; added: Bzr CVS Git Hg Mtn
+            ;; unregistered: SVN
+	    ;; up-to-date: RCS SCCS
             (message "vc-state4 %s" (vc-state tmp-name))
 	    (should (eq (vc-state tmp-name) (vc-state tmp-name backend)))
-	    (should (memq (vc-state tmp-name) '(added unregistered up-to-date)))
+	    (should (memq (vc-state tmp-name)
+			  '(nil added unregistered up-to-date)))

 	    ;; Unregister the file.  Check state.
 	    (condition-case nil
 		(progn
 		  (vc-test--unregister-function backend tmp-name)

-		  ;; added: Git
-		  ;; unregistered: Hg RCS
+		  ;; added: Bzr Git Hg
 		  ;; unsupported: CVS Mtn SCCS SRC SVN
-		  ;; up-to-date: Bzr
+		  ;; up-to-date: RCS
                   (message "vc-state5 %s" (vc-state tmp-name))
 		  (should (eq (vc-state tmp-name) (vc-state tmp-name backend)))
-		  (should (memq (vc-state tmp-name)
-				'(added unregistered up-to-date))))
+		  (should (memq (vc-state tmp-name) '(added up-to-date))))
 	      (vc-not-supported (message "vc-state5 unsupported")))))

       ;; Save exit.
@@ -370,8 +361,8 @@ vc-test--working-revision
 	  (make-directory default-directory)
 	  (vc-test--create-repo-function backend)

-	  ;; nil: CVS Git Mtn RCS SCCS
-	  ;; "0": Bzr Hg SRC SVN
+	  ;; nil: Bzr CVS Git Hg Mtn RCS SCCS SRC
+	  ;; "0": SVN
           (message
            "vc-working-revision1 %s" (vc-working-revision default-directory))
 	  (should (eq (vc-working-revision default-directory)
@@ -382,33 +373,32 @@ vc-test--working-revision
 	    ;; Check initial working revision, should be nil until
             ;; it's registered.

-	    ;; nil: CVS Git Mtn RCS SCCS SVN
-	    ;; "0": Bzr Hg SRC
+	    ;; nil: Bzr CVS Git Hg Mtn RCS SCCS SRC SVN
             (message "vc-working-revision2 %s" (vc-working-revision tmp-name))
 	    (should (eq (vc-working-revision tmp-name)
 			(vc-working-revision tmp-name backend)))
-	    (should (member (vc-working-revision tmp-name) '(nil "0")))
+	    (should-not (vc-working-revision tmp-name))

 	    ;; Write a new file.  Check working revision.
 	    (write-region "foo" nil tmp-name nil 'nomessage)

-	    ;; nil: CVS Git Mtn RCS SCCS SVN
-	    ;; "0": Bzr Hg SRC
+	    ;; nil: Bzr CVS Git Hg Mtn RCS SCCS SRC SVN
             (message "vc-working-revision3 %s" (vc-working-revision tmp-name))
 	    (should (eq (vc-working-revision tmp-name)
 			(vc-working-revision tmp-name backend)))
-	    (should (member (vc-working-revision tmp-name) '(nil "0")))
+	    (should-not (vc-working-revision tmp-name))

 	    ;; Register a file.  Check working revision.
 	    (vc-register
 	     (list backend (list (file-name-nondirectory tmp-name))))

-	    ;; nil: Mtn Git RCS SCCS
+	    ;; nil: Git Mtn
 	    ;; "0": Bzr CVS Hg SRC SVN
+	    ;; "1.1": RCS SCCS
             (message "vc-working-revision4 %s" (vc-working-revision tmp-name))
 	    (should (eq (vc-working-revision tmp-name)
 			(vc-working-revision tmp-name backend)))
-	    (should (member (vc-working-revision tmp-name) '(nil "0")))
+	    (should (member (vc-working-revision tmp-name) '(nil "0" "1.1")))

 	    ;; Unregister the file.  Check working revision.
 	    (condition-case nil
@@ -417,12 +407,13 @@ vc-test--working-revision

 		  ;; nil: Git RCS
 		  ;; "0": Bzr Hg
+		  ;; "1.1": RCS
 		  ;; unsupported: CVS Mtn SCCS SRC SVN
                   (message
                    "vc-working-revision5 %s" (vc-working-revision tmp-name))
 		  (should (eq (vc-working-revision tmp-name)
 			      (vc-working-revision tmp-name backend)))
-		  (should (member (vc-working-revision tmp-name) '(nil "0"))))
+		  (should (member (vc-working-revision tmp-name) '(nil "0" "1.1"))))
 	      (vc-not-supported (message "vc-working-revision5 unsupported")))))

       ;; Save exit.