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

From: Michael Albinus <[email-masked]>
Date: Wed, 18 Jul 2012 16:11:14 +0200

Forwarded to debbugs, for the archives.

Subject: Re: Fwd: Re: bug#11757: Acknowledgement (24.1.50; vc-git calls `process-file' too many times)
From: Dmitry Gutov <[email-masked]>
To: Michael Albinus <[email-masked]>
Date: Wed, 18 Jul 2012 08:07:46 +0400

On 17.07.2012 11:20, Michael Albinus wrote:
> (*): There seems to be at least one `call-process' call in vc-git.el,
> which I regard as an error. Maybe you could correct this in your patch,
> too.

Added to the patch, as well as the ChangeLog entries.

> I haven't prepared test cases yet. The basic idea is to have a (test)
> file under git, and to apply some default scenarios with it. Not just
> open, edit, save, commit (that might be the most simple scenario), but
> something which reflects the usual workflow better. Escpecially,

The above is my usual workflow, actually, when repeated several times.

> keeping the file opened in an Emacs buffer, during several workflow
> cycles - this would give cached values the possibility to show their
> power (or not).

Since I have a clear picture in my mind why this wouldn't make a 
difference (for one, there's no time-based expiration), I'm having a 
hard time choosing a meaningful longer scenario.

So I'm just doing one scenario now. If you write a different one, I can 
do that next.

Scenario:
1. git init test (outside of Emacs)
2. find-file in this directory (creating the new file)
3. edit, save, `vc-next-action' (registering the new file)
4. repeat step 3 (makes first commit)
5. repeat step 4 (makes second commit)
6. `vc-rename-file'
7. `vc-next-action' (commit)
8. `vc-delete-file'

(This automatically closes the buffer and leaves the repo with 2 files 
removed from the disk and staged for deletion.)

> As we both agree, calling processes is the most time consuming
> action. We might concentrate on counting the number of process calls,
> the other Lisp-based actions are negligible. I would trace all vc-*
> function calls, and `process-file' (*), like this:
>
> (require 'trace)
> (dolist (elt (all-completions "vc-" obarray 'functionp))
>    (trace-function-background (intern elt)))
> (trace-function-background 'process-file)
>
> This returns also two different trace buffers, which could be compared
> by ediff easily, in order to see how vc behaves.

Done this twice, logs 1 and 3 - without the patch, 2 and 4 - with the 
patch. 1 ~ 3 and 2 ~ 4, so you may just as well compare only one pair.

The difference: a few getprop/setprop calls, and one full uncached 
`vc-git-registered' call in `vc-register' (after "git add" was called), 
all in favor of the patched version (which doesn't make the above calls).
The last one is interesting, because it shows that `vc-register' doesn't 
require `vc-git-registered' to return `t' before it assigns Git backend 
to the buffer.
So at the same point where the unpatched code calls `vc-git-registered' 
and it finally returns true, the patched code just assumes it's true, 
and doesn't call the function. :) A bit risky, but not wrong, at least 
in this case.

This would have been different if the file existed and was registered in 
the repository before step 2, then the first call to `vc-git-registered' 
would have returned `t' in both versions.

P.S. `ediff' says there are no differences after around 43% mark, but 
there are. For example, line 1021 in trace-3.log shows a call to 
`vc-git-registered', whereas the respective line 979 in trace-4.log 
shows that the call is skipped.
Same with `diff' in the console, it ignores these changes, no idea why. 
Maybe I just fail at diffing.

--Dmitry
diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index 3ac7b2e..45c49c4 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,11 @@
+2012-07-18  Dmitry Gutov  <[email-masked]>
+
+	* vc/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.
+	(vc-git-branches): Use `vc-git--call' instead of `call-process'.
+
 2012-07-14  Dmitry Gutov  <[email-masked]>
 
 	* progmodes/ruby-mode.el (ruby-parse-partial): No error when end
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 4dbc9ea..700adf1 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'."
@@ -576,7 +574,7 @@ or an empty string if none."
   "Return the existing branches, as a list of strings.
 The car of the list is the current branch."
   (with-temp-buffer
-    (call-process vc-git-program nil t nil "branch")
+    (vc-git--call t "branch")
     (goto-char (point-min))
     (let (current-branch branches)
       (while (not (eobp))