bug#11757: Acknowledgement (24.1.50; vc-git calls `process-file' too many times)

[Dmitry Gutov <dgutov@yandex.ru>]

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

On 06.07.2012 17:44, Michael Albinus wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
>
>>> If we assume that there are no dangerous vc commands outside Emacs, we
>>> wouldn't have a problem.
>>
>> In this case, the behavior of the first patch I posted here should be
>> acceptable, right? It's simpler, has pretty much the same effect, and
>> should be a tiny bit faster.
>
> That I don't know. Both patches do almost what we expect, and having a
> cached value for `vc-registered' sounds more performant when applied
> often enough.

With the attached patch, `vc-git-registered' is usually called only once 
during the lifetime of a buffer.

For caching to be an improvement, 'git-registered value has to be 
invalidated separately from 'vc-backend, yet less often than 
`vc-git-state' is called.

>>> Yes. I don't know, whether we will be able to handle any surprise when
>>> using caches. There will always be a scenario which lets fail a given
>>> algorithm. I fear.
>>
>> Sure, but I'm just asking for one scenario that works better with
>> explicitly caching 'git-registered, instead of not calling it in
>> vc-git-state'.
>> If `vc-git-state' doesn't call `vc-git-registered' (just assumes it's
>> t), then `vc-registered' is the latter's only client, and so its
>> return value is implicitly cached in 'vc-backend property.
>
> Maybe. Could you show a patch? (Please with ChangeLog entry, I would
> commit if it looks good).

* vc-git.el (vc-git-state): Don't call `vc-git-registered', assume it's 
always t.
(vc-git-registered): Remove caching, the function is only called once.

Thank you,

--Dmitry

[-- Attachment #2: vc-git-registered.diff --]
[-- Type: text/plain, Size: 4717 bytes --]

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 8b48efb..f585f1b 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -173,31 +173,28 @@ matching the resulting Git log output, and KEYWORDS is a list of
 
 (defun vc-git-registered (file)
   "Check whether FILE is registered with git."
-  (or (vc-file-getprop file 'git-registered)
-      (vc-file-setprop
-       file 'git-registered
-       (let ((dir (vc-git-root file)))
-	 (when dir
-	   (with-temp-buffer
-	     (let* (process-file-side-effects
-		    ;; Do not use the `file-name-directory' here: git-ls-files
-		    ;; sometimes fails to return the correct status for relative
-		    ;; path specs.
-		    ;; See also: http://marc.info/?l=git&m=125787684318129&w=2
-		    (name (file-relative-name file dir))
-		    (str (ignore-errors
-			   (cd dir)
-			   (vc-git--out-ok "ls-files" "-c" "-z" "--" name)
-			   ;; If result is empty, use ls-tree to check for deleted
-			   ;; file.
-			   (when (eq (point-min) (point-max))
-			     (vc-git--out-ok "ls-tree" "--name-only" "-z" "HEAD"
-					     "--" name))
-			   (buffer-string))))
-	       (and str
-		    (> (length str) (length name))
-		    (string= (substring str 0 (1+ (length name)))
-			     (concat name "\0"))))))))))
+  (let ((dir (vc-git-root file)))
+    (when dir
+      (with-temp-buffer
+        (let* (process-file-side-effects
+               ;; Do not use the `file-name-directory' here: git-ls-files
+               ;; sometimes fails to return the correct status for relative
+               ;; path specs.
+               ;; See also: http://marc.info/?l=git&m=125787684318129&w=2
+               (name (file-relative-name file dir))
+               (str (ignore-errors
+                      (cd dir)
+                      (vc-git--out-ok "ls-files" "-c" "-z" "--" name)
+                      ;; If result is empty, use ls-tree to check for deleted
+                      ;; file.
+                      (when (eq (point-min) (point-max))
+                        (vc-git--out-ok "ls-tree" "--name-only" "-z" "HEAD"
+                                        "--" name))
+                      (buffer-string))))
+          (and str
+               (> (length str) (length name))
+               (string= (substring str 0 (1+ (length name)))
+                        (concat name "\0"))))))))
 
 (defun vc-git--state-code (code)
   "Convert from a string to a added/deleted/modified state."
@@ -218,23 +215,24 @@ matching the resulting Git log output, and KEYWORDS is a list of
   ;; is direct ancestor of corresponding upstream branch, and the file
   ;; was modified upstream.  But we can't check that without a network
   ;; operation.
-  (if (not (vc-git-registered file))
-      'unregistered
-    (let ((diff (vc-git--run-command-string
-                 file "diff-index" "-p" "--raw" "-z" "HEAD" "--")))
-      (if (and diff
-	       (string-match ":[0-7]\\{6\\} [0-7]\\{6\\} [0-9a-f]\\{40\\} [0-9a-f]\\{40\\} \\([ADMUT]\\)\0[^\0]+\0\\(.*\n.\\)?"
-			     diff))
-          (let ((diff-letter (match-string 1 diff)))
-            (if (not (match-beginning 2))
-                ;; Empty diff: file contents is the same as the HEAD
-                ;; revision, but timestamps are different (eg, file
-                ;; was "touch"ed).  Update timestamp in index:
-                (prog1 'up-to-date
-                  (vc-git--call nil "add" "--refresh" "--"
-                                (file-relative-name file)))
-              (vc-git--state-code diff-letter)))
-	(if (vc-git--empty-db-p) 'added 'up-to-date)))))
+  ;; This assumes that status is known to be not `unregistered' because
+  ;; we've been successfully dispatched here from `vc-state', that
+  ;; means `vc-git-registered' returned t earlier once.  Bug#11757
+  (let ((diff (vc-git--run-command-string
+               file "diff-index" "-p" "--raw" "-z" "HEAD" "--")))
+    (if (and diff
+             (string-match ":[0-7]\\{6\\} [0-7]\\{6\\} [0-9a-f]\\{40\\} [0-9a-f]\\{40\\} \\([ADMUT]\\)\0[^\0]+\0\\(.*\n.\\)?"
+                           diff))
+        (let ((diff-letter (match-string 1 diff)))
+          (if (not (match-beginning 2))
+              ;; Empty diff: file contents is the same as the HEAD
+              ;; revision, but timestamps are different (eg, file
+              ;; was "touch"ed).  Update timestamp in index:
+              (prog1 'up-to-date
+                (vc-git--call nil "add" "--refresh" "--"
+                              (file-relative-name file)))
+            (vc-git--state-code diff-letter)))
+      (if (vc-git--empty-db-p) 'added 'up-to-date))))
 
 (defun vc-git-working-revision (_file)
   "Git-specific version of `vc-working-revision'."