GNU bug report logs - #11757
24.1.50; vc-git calls `process-file' too many times

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 11757 in the body.
You can then email your comments to 11757 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: bug-gnu-emacs <at> gnu.org
Subject: 24.1.50; vc-git calls `process-file' too many times
Date: Thu, 21 Jun 2012 06:12:58 +0400

When I open a file under Git version control, `vc-find-file-hook' takes
~1 second to run, even on small repositories, whereas for
Mercurial, for example, it's ~0.3 seconds.

There two two reasons for that:
1) Calling git is ~ twice as expensive as hg.
2) It's called more often.

All this under MS Windows.

Test setup:

(defadvice process-file (around time-it (program &optional infile buffer 
display &rest args) activate)
  (message "process-file %s %s" program args)
  (let ((beg (current-time)))
    (prog1 ad-do-it
      (message "%s elapsed"
               (/ (truncate (* (- (float-time (current-time))
                                  (float-time beg))
                               10000))
                  10000.0)))))

Results:

Open file from Git repo, make small modification, and save it:

  process-file git (ls-files -c -z -- INSTALL)
  0.2639 elapsed
  process-file git (symbolic-ref HEAD)
  0.1849 elapsed
  process-file git (ls-files -c -z -- INSTALL)
  0.1929 elapsed
  process-file git (diff-index -p --raw -z HEAD -- INSTALL)
  0.203 elapsed
  process-file git (rev-parse --verify HEAD)
  0.203 elapsed
  process-file git (symbolic-ref HEAD)
  0.186 elapsed
  Saving file c:/Users/gutov/vc/emacs-master/INSTALL...
  Wrote c:/Users/gutov/vc/emacs-master/INSTALL
  process-file git (ls-files -c -z -- INSTALL)
  0.217 elapsed
  process-file git (diff-index -p --raw -z HEAD -- INSTALL)
  0.233 elapsed
  process-file git (add --refresh -- INSTALL)
  0.2039 elapsed
  process-file git (symbolic-ref HEAD)
  0.21 elapsed

Note that "ls-files -c -z -- INSTALL" and "symbolic-ref HEAD" are both
called twice when the file is opened.

Do the same with Mercurial repo:

  process-file hg (--config alias.status=status --config 
defaults.status= status -A README.txt)
  0.1069 elapsed
  process-file hg (--config alias.status=status --config 
defaults.status= status -A README.txt)
  0.1099 elapsed
  process-file hg (log -l 1 --template {rev} README.txt)
  0.1019 elapsed
  Saving file c:/Users/gutov/vc/jqplot/README.txt...
  Wrote c:/Users/gutov/vc/jqplot/README.txt
  process-file hg (--config alias.status=status --config 
defaults.status= status -A README.txt)
  0.164 elapsed

Note lesser amount of calls (3 vs 6 for opening, 1 vs 4 for writing). Of
the 3, one also seems to be redundant.

In GNU Emacs 24.1.50.1 (i386-mingw-nt6.1.7601)
 of 2012-06-20 on SOL
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --with-gcc (3.4) --cflags -IH:/Apps/System/gnuwin32/include'

Message #8 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50; vc-git calls `process-file'
	too many times)
Date: Tue, 26 Jun 2012 14:54:32 +0400

Update.

After setting vc-git-program explicitly to "C:/Program Files 
(x86)/Git/bin/git.exe" (by default, it was calling "C:/Program Files 
(x86)/Git/cmd/git.CMD"), the average time of git command calls went down 
from 0.2s to 0.03s, so the problem with Git is about 7 times less 
important now.

Still, the issue of redundant calls stands. With Mercurial, for example, 
it would bring the time taken by `vc-find-file-hook' from 0.3s to 0.2s.

And I'd love to see the thing with vc-git-program documented somewhere.
In http://www.gnu.org/software/emacs/windows/faq.html, maybe?

-- Dmitry

Message #11 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50;
	vc-git calls `process-file' too many times)
Date: Tue, 26 Jun 2012 14:14:25 +0200

Dmitry Gutov <dgutov <at> yandex.ru> writes:

> After setting vc-git-program explicitly to "C:/Program Files
> (x86)/Git/bin/git.exe" (by default, it was calling "C:/Program Files
> (x86)/Git/cmd/git.CMD"), the average time of git command calls went
> down from 0.2s to 0.03s, so the problem with Git is about 7 times less
> important now.

If the git repository is remote (accessed via Tramp), this is still a
serious issue. I'm plagued by this every single day. Less `process-file'
invocations would be more than welcome. Maybe vc-git can cache some results?

> -- Dmitry

Best regards, Michael.

Message #14 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50; vc-git calls `process-file'
	too many times)
Date: Thu, 28 Jun 2012 04:51:54 +0400

[Message part 1 (text/plain, inline)]

Hi Michael,

This little patch shaves 2 `process-file' invocations from both 
`vc-find-file-hook' and `vc-after-save'.

It's not fully backward-compatible (it breaks when a previously 
registered file became unregistered), but I think it's a good tradeoff.

VC doesn't handle all cases of "outside interference" anyway: for 
example, the cached return value of `vc-working-revision' is invalidated 
only after the file is checked in, moved, or deleted, not after each 
save, and switching to another branch in Git is a much more common 
occurrence.

A more complex solution, though also applicable to vc-hg, would be to 
move most of the code from `vc-git-registered' to `vc-git-state', then 
modify `vc-git-registered' and `vc-hg-registered' to call `vc-state' 
(for caching) and then interpret its return value (what 
`vc-hg-registered' already does). Since `vc-registered' calls 
`vc-xx-registered' functions when selecting the backend, we'd also need 
to clear the cached 'vc-state value after each call with negative result.

This would mean +1 `process-file' invocation with Git in 
`vc-after-save', compared to the attached patch, but -1 with Hg in 
`vc-find-file-hook'.

On 26.06.2012 16:14, Michael Albinus wrote:
> Dmitry Gutov <dgutov <at> yandex.ru> writes:
>
>> After setting vc-git-program explicitly to "C:/Program Files
>> (x86)/Git/bin/git.exe" (by default, it was calling "C:/Program Files
>> (x86)/Git/cmd/git.CMD"), the average time of git command calls went
>> down from 0.2s to 0.03s, so the problem with Git is about 7 times less
>> important now.
>
> If the git repository is remote (accessed via Tramp), this is still a
> serious issue. I'm plagued by this every single day. Less `process-file'
> invocations would be more than welcome. Maybe vc-git can cache some results?
>
>> -- Dmitry
>
> Best regards, Michael.
>
>

[0004-vc-git.el-vc-git-state-Don-t-call-vc-git-registered-.patch (text/plain, attachment)]

Message #17 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50;
	vc-git calls `process-file' too many times)
Date: Fri, 29 Jun 2012 15:46:33 +0200

Dmitry Gutov <dgutov <at> yandex.ru> writes:

> Hi Michael,

Hi Dmitry,

> This little patch shaves 2 `process-file' invocations from both
> vc-find-file-hook' and `vc-after-save'.
>
> It's not fully backward-compatible (it breaks when a previously
> registered file became unregistered), but I think it's a good
> tradeoff.

I don't know whether we shall break the functionality. Instead of, I've
appended a small patch, which uses the cache for vc-git-registered and
vc-git-root (additionally to your patch, which uses the cache of
vc-working-revision). This reduces already the number of process-file
invocations from 6 to 4, when openening a new file. And there's room for
other caches.

> VC doesn't handle all cases of "outside interference" anyway: for
> example, the cached return value of `vc-working-revision' is
> invalidated only after the file is checked in, moved, or deleted, not
> after each save, and switching to another branch in Git is a much more
> common occurrence.

A stale cache is bad, of course. We must carefully check, where a cached
value has to be invalidated. But why should vc-working-revision being
invalidated after saving? It is still the same, I believe. Switching to
another branch shall be observed by Emacs, 'cause there is another
version of the file on the disk, and Emacs warns you before editing.

> A more complex solution, though also applicable to vc-hg, would be to
> move most of the code from `vc-git-registered' to `vc-git-state', then
> modify `vc-git-registered' and `vc-hg-registered' to call `vc-state'
> (for caching) and then interpret its return value (what
> vc-hg-registered' already does). Since `vc-registered' calls
> vc-xx-registered' functions when selecting the backend, we'd also need
> to clear the cached 'vc-state value after each call with negative
> result.

This I cannot answer (yet). We could apply this, when the other changes
work robustly.

> -- Dmitry

Best regards, Michael.

*** /usr/local/src/emacs/lisp/vc/vc-git.el.~108799~  2012-06-29 15:39:15.897374680 +0200
--- /usr/local/src/emacs/lisp/vc/vc-git.el	2012-06-29 15:24:04.389401221 +0200
***************
*** 173,200 ****

  (defun vc-git-registered (file)
    "Check whether FILE is registered with git."
!   (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."
--- 173,203 ----

  (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"))))))))))

  (defun vc-git--state-code (code)
    "Convert from a string to a added/deleted/modified state."
***************
*** 248,254 ****

  (defun vc-git-mode-line-string (file)
    "Return a string for `vc-mode-line' to put in the mode line for FILE."
!   (let* ((branch (vc-git-working-revision file))
           (def-ml (vc-default-mode-line-string 'Git file))
           (help-echo (get-text-property 0 'help-echo def-ml)))
      (if (zerop (length branch))
--- 251,257 ----

  (defun vc-git-mode-line-string (file)
    "Return a string for `vc-mode-line' to put in the mode line for FILE."
!   (let* ((branch (vc-working-revision file))
           (def-ml (vc-default-mode-line-string 'Git file))
           (help-echo (get-text-property 0 'help-echo def-ml)))
      (if (zerop (length branch))
***************
*** 959,965 ****
  (defun vc-git-extra-status-menu () vc-git-extra-menu-map)

  (defun vc-git-root (file)
!   (vc-find-root file ".git"))

  ;; Derived from `lgrep'.
  (defun vc-git-grep (regexp &optional files dir)
--- 962,969 ----
  (defun vc-git-extra-status-menu () vc-git-extra-menu-map)

  (defun vc-git-root (file)
!   (or (vc-file-getprop file 'git-root)
!       (vc-file-setprop file 'git-root (vc-find-root file ".git"))))

  ;; Derived from `lgrep'.
  (defun vc-git-grep (regexp &optional files dir)

Message #20 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50; vc-git calls `process-file'
	too many times)
Date: Fri, 29 Jun 2012 20:06:56 +0400

On 29.06.2012 17:46, Michael Albinus wrote:
>> This little patch shaves 2 `process-file' invocations from both
>> vc-find-file-hook' and `vc-after-save'.
>>
>> It's not fully backward-compatible (it breaks when a previously
>> registered file became unregistered), but I think it's a good
>> tradeoff.
>
> I don't know whether we shall break the functionality. Instead of, I've
> appended a small patch, which uses the cache for vc-git-registered and
> vc-git-root (additionally to your patch, which uses the cache of
> vc-working-revision). This reduces already the number of process-file
> invocations from 6 to 4, when openening a new file. And there's room for
> other caches.

Looks like the win is the same here.

I'm not sure about caching vc-git-root, since at least in local scenario 
it's a fast operation. Is it that slow with Tramp?
Other backends don't cache it either.

>> VC doesn't handle all cases of "outside interference" anyway: for
>> example, the cached return value of `vc-working-revision' is
>> invalidated only after the file is checked in, moved, or deleted, not
>> after each save, and switching to another branch in Git is a much more
>> common occurrence.
>
> A stale cache is bad, of course. We must carefully check, where a cached
> value has to be invalidated. But why should vc-working-revision being
> invalidated after saving? It is still the same, I believe. Switching to
> another branch shall be observed by Emacs, 'cause there is another
> version of the file on the disk, and Emacs warns you before editing.

This won't happen in following cases:
1) We switch to revision when the opened file is the same.
2) It doesn't exist there.
3) We just delete it from disk from outside of Emacs.
So the file isn't changed, and you see no warning or update, even after 
you write it to disk from Emacs again.

And the latter two cases (the last one - with a small modification) are 
the only situations I can think of when an open buffer in which 
(vc-git-registered) returned t some time ago (so it has vc-backend 
property set to Git) now should return nil.
But the properties won't be reset, so the cached value will be outdated.

Can you describe a scenario in which 'git-registered cached value will 
be invalidated, and the function will then return nil?

P.S. I can't find a way to apply context diff with my current setup, so 
if it's not too hard, please send a unified one next time.

-- Dmitry

Message #23 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50;
	vc-git calls `process-file' too many times)
Date: Fri, 29 Jun 2012 18:40:47 +0200

Dmitry Gutov <dgutov <at> yandex.ru> writes:

> Looks like the win is the same here.

Yes. And when there are several operations for the same file, the cache
pays even more.

> I'm not sure about caching vc-git-root, since at least in local
> scenario it's a fast operation. Is it that slow with Tramp?

Yes. Every remote operation is slow, compared with whatever lisp code.

> Other backends don't cache it either.

They should do also. It is very unlikely, that the root changes while
the cache is active.

>> A stale cache is bad, of course. We must carefully check, where a cached
>> value has to be invalidated. But why should vc-working-revision being
>> invalidated after saving? It is still the same, I believe. Switching to
>> another branch shall be observed by Emacs, 'cause there is another
>> version of the file on the disk, and Emacs warns you before editing.
>
> This won't happen in following cases:
> 1) We switch to revision when the opened file is the same.
> 2) It doesn't exist there.
> 3) We just delete it from disk from outside of Emacs.
> So the file isn't changed, and you see no warning or update, even
> after you write it to disk from Emacs again.

I see. Maybe we find a hook, where we could invalidate the vc cache when
a file is written which does not exist on the disk?

> And the latter two cases (the last one - with a small modification)
> are the only situations I can think of when an open buffer in which
> (vc-git-registered) returned t some time ago (so it has vc-backend
> property set to Git) now should return nil.
> But the properties won't be reset, so the cached value will be outdated.
>
> Can you describe a scenario in which 'git-registered cached value will
> be invalidated, and the function will then return nil?

When the file is removed from git outside Emacs. In this case,
git-registered must be nil.

> P.S. I can't find a way to apply context diff with my current setup,
> so if it's not too hard, please send a unified one next time.

I try to remember. The Emacs maintainers prefer context diffs, that's
why ediff-custom-diff-options is set to "-c" by default.

> -- Dmitry

Best regards, Michael.

Message #26 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50; vc-git calls `process-file'
	too many times)
Date: Fri, 29 Jun 2012 21:03:20 +0400

On 29.06.2012 20:40, Michael Albinus wrote:
>>> A stale cache is bad, of course. We must carefully check, where a cached
>>> value has to be invalidated. But why should vc-working-revision being
>>> invalidated after saving? It is still the same, I believe. Switching to
>>> another branch shall be observed by Emacs, 'cause there is another
>>> version of the file on the disk, and Emacs warns you before editing.
>>
>> This won't happen in following cases:
>> 1) We switch to revision when the opened file is the same.
>> 2) It doesn't exist there.
>> 3) We just delete it from disk from outside of Emacs.
>> So the file isn't changed, and you see no warning or update, even
>> after you write it to disk from Emacs again.
>
> I see. Maybe we find a hook, where we could invalidate the vc cache when
> a file is written which does not exist on the disk?

(vc-before-save) might be the place to do that.

>> And the latter two cases (the last one - with a small modification)
>> are the only situations I can think of when an open buffer in which
>> (vc-git-registered) returned t some time ago (so it has vc-backend
>> property set to Git) now should return nil.
>> But the properties won't be reset, so the cached value will be outdated.
>>
>> Can you describe a scenario in which 'git-registered cached value will
>> be invalidated, and the function will then return nil?
>
> When the file is removed from git outside Emacs. In this case,
> git-registered must be nil.

I meant, would that happen with your patch?
If vc-before-save would invalidate the cache, that should be ok.

>> P.S. I can't find a way to apply context diff with my current setup,
>> so if it's not too hard, please send a unified one next time.
>
> I try to remember. The Emacs maintainers prefer context diffs, that's
> why ediff-custom-diff-options is set to "-c" by default.

How important is that, I wonder? It's what CONTRIBUTE says, but I've 
seen many of the diffs posted in unified format, and no one ever asked 
me to convert a patch specifically into context format.

Message #29 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50;
	vc-git calls `process-file' too many times)
Date: Fri, 29 Jun 2012 19:38:47 +0200

Dmitry Gutov <dgutov <at> yandex.ru> writes:

> (vc-before-save) might be the place to do that.

Well. I'll check how to do.

>>> P.S. I can't find a way to apply context diff with my current setup,
>>> so if it's not too hard, please send a unified one next time.
>>
>> I try to remember. The Emacs maintainers prefer context diffs, that's
>> why ediff-custom-diff-options is set to "-c" by default.
>
> How important is that, I wonder? It's what CONTRIBUTE says, but I've
> seen many of the diffs posted in unified format, and no one ever asked
> me to convert a patch specifically into context format.

Years ago, I have been asked to do so. And as said, it is the default
value in vanilla Emacs; I simply don't think about.

But I try to remember when I prepare a patch for you.

Best regards, Michael.

Message #32 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50;
	vc-git calls `process-file' too many times)
Date: Sat, 30 Jun 2012 11:03:17 +0200

Dmitry Gutov <dgutov <at> yandex.ru> writes:

> On 29.06.2012 20:40, Michael Albinus wrote:
>>>> A stale cache is bad, of course. We must carefully check, where a cached
>>>> value has to be invalidated. But why should vc-working-revision being
>>>> invalidated after saving? It is still the same, I believe. Switching to
>>>> another branch shall be observed by Emacs, 'cause there is another
>>>> version of the file on the disk, and Emacs warns you before editing.
>>>
>>> This won't happen in following cases:
>>> 1) We switch to revision when the opened file is the same.
>>> 2) It doesn't exist there.
>>> 3) We just delete it from disk from outside of Emacs.
>>> So the file isn't changed, and you see no warning or update, even
>>> after you write it to disk from Emacs again.
>>
>> I see. Maybe we find a hook, where we could invalidate the vc cache when
>> a file is written which does not exist on the disk?
>
> (vc-before-save) might be the place to do that.

In vc-after-save, vc-git-state is called. Wouldn't it be sufficient to
invalidate the cache there, when it detects that the file is not up-to-date?

Best regards, Michael.

Message #35 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50; vc-git calls `process-file'
	too many times)
Date: Sat, 30 Jun 2012 16:56:03 +0400

On 30.06.2012 13:03, Michael Albinus wrote:
> Dmitry Gutov <dgutov <at> yandex.ru> writes:
>
>> On 29.06.2012 20:40, Michael Albinus wrote:
>>>>> A stale cache is bad, of course. We must carefully check, where a cached
>>>>> value has to be invalidated. But why should vc-working-revision being
>>>>> invalidated after saving? It is still the same, I believe. Switching to
>>>>> another branch shall be observed by Emacs, 'cause there is another
>>>>> version of the file on the disk, and Emacs warns you before editing.
>>>>
>>>> This won't happen in following cases:
>>>> 1) We switch to revision when the opened file is the same.
>>>> 2) It doesn't exist there.
>>>> 3) We just delete it from disk from outside of Emacs.
>>>> So the file isn't changed, and you see no warning or update, even
>>>> after you write it to disk from Emacs again.
>>>
>>> I see. Maybe we find a hook, where we could invalidate the vc cache when
>>> a file is written which does not exist on the disk?
>>
>> (vc-before-save) might be the place to do that.
>
> In vc-after-save, vc-git-state is called. Wouldn't it be sufficient to
> invalidate the cache there, when it detects that the file is not up-to-date?

Why there? Problem with working-revision is common for all backends, as 
far as I can see.

Also, how? After the file is written, we can't check if it didn't exist 
on disk anymore.

Here's what I had in mind:

diff --git a/lisp/vc/vc-hooks.el b/lisp/vc/vc-hooks.el
index bba7217..4bff5fc 100644
--- a/lisp/vc/vc-hooks.el
+++ b/lisp/vc/vc-hooks.el
@@ -703,7 +703,9 @@ Before doing that, check if there are any old 
backups and get rid of them."
   ;; another name.  This enables local diffs and local reverting.
   (let ((file buffer-file-name)
         backend)
-    (ignore-errors               ;Be careful not to prevent saving the 
file.
+    (ignore-errors      ;Be careful not to prevent saving the file.
+      (unless (file-exists-p file)
+        (vc-file-clearprops file))
       (and (setq backend (vc-backend file))
            (vc-up-to-date-p file)
            (eq (vc-checkout-model backend (list file)) 'implicit)

Message #38 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50;
	vc-git calls `process-file' too many times)
Date: Sat, 30 Jun 2012 15:19:30 +0200

Dmitry Gutov <dgutov <at> yandex.ru> writes:

> Here's what I had in mind:

Well, I've committed both your and my patch. Can we close the ticket now?

Best regards, Michael.

Message #41 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50; vc-git calls `process-file'
	too many times)
Date: Sat, 30 Jun 2012 21:42:31 +0400

On 30.06.2012 17:19, Michael Albinus wrote:
> Dmitry Gutov <dgutov <at> yandex.ru> writes:
>
>> Here's what I had in mind:
>
> Well, I've committed both your and my patch. Can we close the ticket now?

Uh, probably not. This was not enough to fully fix the scenario of 
writing to a file that was removed from Git externally.

This way, `vc-before-save' clears 'vc-backend property too and 
(vc-backend file) retuns nil in `vc-after-save', and the mode-line isn't 
getting updated.

I'm not sure what we should do. Call (vc-mode-line) anyway? That would work.

Clearing only some properties in `vc-before-save' wouldn't be very sane, 
since 'git-registered is backend-specific.

There's a major inconsistency in `vc-backend' logic: we have a way to 
display mode-line, refresh state, etc, for a file that's been registered 
in VC but then was removed, as long as the buffer wasn't killed, but 
kill it and open the file again - now (vc-backend) returns nil.

-- Dmitry

Message #44 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50;
	vc-git calls `process-file' too many times)
Date: Sat, 30 Jun 2012 20:46:05 +0200

Dmitry Gutov <dgutov <at> yandex.ru> writes:

> This way, `vc-before-save' clears 'vc-backend property too and
> (vc-backend file) retuns nil in `vc-after-save', and the mode-line
> isn't getting updated.

So we might let-bind the 'vc-backend property to a local variable, and
reset it after cleanup.

> I'm not sure what we should do. Call (vc-mode-line) anyway? That would work.

Nope. This is expansive, because it recomputes `vc-working-revision'. We
would loose all improvements from using the cache.

> -- Dmitry

Best regards, Michael.

Message #47 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50; vc-git calls `process-file'
	too many times)
Date: Sat, 30 Jun 2012 23:14:18 +0400

On 30.06.2012 22:46, Michael Albinus wrote:
> Dmitry Gutov <dgutov <at> yandex.ru> writes:
>
>> This way, `vc-before-save' clears 'vc-backend property too and
>> (vc-backend file) retuns nil in `vc-after-save', and the mode-line
>> isn't getting updated.
>
> So we might let-bind the 'vc-backend property to a local variable, and
> reset it after cleanup.

It would work, but this, yet again, complicates the logic.

>> I'm not sure what we should do. Call (vc-mode-line) anyway? That would work.
>
> Nope. This is expansive, because it recomputes `vc-working-revision'. We
> would loose all improvements from using the cache.

I don't think so.

If we hadn't reset all properties in vc-before-save (file existed), 
nothing changes.
If we did reset them, then yes, vc-working-revision will recompute 
'vc-working-revision property, but only once after the reset.
Which is what we want to do anyway, since the file's state has changed, 
and the working revision could have changed as well, so we need to know 
them to update mode-line.

By the way, this last patch I sent doesn't help if the user just removed 
the file from repository while leaving it on disk (git rm --cached ... 
&& git commit ..., for example), but whatever.

Message #50 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 11757 <at> debbugs.gnu.org, Michael Albinus <michael.albinus <at> gmx.de>
Subject: Re: bug#11757: Acknowledgement (24.1.50;
	vc-git calls `process-file' too many times)
Date: Sat, 30 Jun 2012 19:01:07 -0400

> This won't happen in following cases:
> 1) We switch to revision when the opened file is the same.
> 2) It doesn't exist there.
> 3) We just delete it from disk from outside of Emacs.
> So the file isn't changed, and you see no warning or update, even after you
> write it to disk from Emacs again.

While that is suboptimal, VC's state is often suboptimal like that.
The only cases where VC's state is (or at least really should)
up-to-date is:
- when it was changed by an explicit VC action.
- when we need to know the state in order to make an important decision.
Updating the state in other circumstances is good, but not necessary.
Improving VC so that the first case is more frequent would actually
be preferable.


        Stefan

Message #53 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 11757 <at> debbugs.gnu.org, Michael Albinus <michael.albinus <at> gmx.de>
Subject: Re: bug#11757: Acknowledgement (24.1.50; vc-git calls `process-file'
	too many times)
Date: Sun, 01 Jul 2012 03:38:15 +0400

On 01.07.2012 3:01, Stefan Monnier wrote:
>> This won't happen in following cases:
>> 1) We switch to revision when the opened file is the same.
>> 2) It doesn't exist there.
>> 3) We just delete it from disk from outside of Emacs.
>> So the file isn't changed, and you see no warning or update, even after you
>> write it to disk from Emacs again.
>
> While that is suboptimal, VC's state is often suboptimal like that.
> The only cases where VC's state is (or at least really should)
> up-to-date is:
> - when it was changed by an explicit VC action.
> - when we need to know the state in order to make an important decision.
> Updating the state in other circumstances is good, but not necessary.
> Improving VC so that the first case is more frequent would actually
> be preferable.

AFAIK, the second case with `vc-git-registered' only occurs in 
`vc-next-action', where it can call `vc-register' if appropriate.

For that to happen, in `vc-deduce-fileset' either (vc-backend 
buffer-file-name) should return nil, or (vc-state ...) - 'unregistered.

So, in all variations on the theme "file unchanged, but not in git index 
anymore" this won't be true, neither with my original patch, nor with 
the currently installed code.

Invalidating property 'git-registered in `vc-deduce-fileset' might fix 
that, but having it know about Git backend is probably not good at all, 
and AFAIK most other backends just detect all states with a single 
process call, including 'unregistered (so they don't call their 
-registered function from -state function).

-- Dmitry

Message #56 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50;
	vc-git calls `process-file' too many times)
Date: Sun, 01 Jul 2012 11:58:28 +0200

Dmitry Gutov <dgutov <at> yandex.ru> writes:

>>> I'm not sure what we should do. Call (vc-mode-line) anyway? That would work.
>>
>> Nope. This is expansive, because it recomputes `vc-working-revision'. We
>> would loose all improvements from using the cache.
>
> I don't think so.
>
> If we hadn't reset all properties in vc-before-save (file existed),
> nothing changes.
> If we did reset them, then yes, vc-working-revision will recompute
> vc-working-revision property, but only once after the reset.
> Which is what we want to do anyway, since the file's state has
> changed, and the working revision could have changed as well, so we
> need to know them to update mode-line.

Likely, the best option is to call (vc-registered file) after clearing
the file cache. This recomputes the 'vc-backend property as well, what
we want.

Calling (vc-mode-line file) at this point would be for the side-effect
of that function, which is bad in my experience. It would harden
maintenance, 'cause nobody will know why we want to refresh the modeline
at this point.

> By the way, this last patch I sent doesn't help if the user just
> removed the file from repository while leaving it on disk (git rm
> --cached ... && git commit ..., for example), but whatever.

If we use the cache, there will always be a constellation that the cache
is stale due to external operations. As Stefan said, this is mostly
uncritical.

Best regards, Michael.

Message #59 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50; vc-git calls `process-file'
	too many times)
Date: Sun, 01 Jul 2012 18:58:01 +0400

Sorry, forgot to cc.

On 01.07.2012 13:58, Michael Albinus wrote:
> Dmitry Gutov <dgutov <at> yandex.ru> writes:
>
>>>> I'm not sure what we should do. Call (vc-mode-line) anyway? That would work.
>>>
>>> Nope. This is expansive, because it recomputes `vc-working-revision'. We
>>> would loose all improvements from using the cache.
>>
>> I don't think so.
>>
>> If we hadn't reset all properties in vc-before-save (file existed),
>> nothing changes.
>> If we did reset them, then yes, vc-working-revision will recompute
>> vc-working-revision property, but only once after the reset.
>> Which is what we want to do anyway, since the file's state has
>> changed, and the working revision could have changed as well, so we
>> need to know them to update mode-line.
>
> Likely, the best option is to call (vc-registered file) after clearing
> the file cache. This recomputes the 'vc-backend property as well, what
> we want.

Have you tried this? (vc-backend file) gets called just a bit later, in 
`vc-after-save`, and it calls `vc-registered' in turn, but since the 
file is no longer registered with Git, it returns nil, and the mode-line 
is not updated.

Also, I don't think we should call (vc-backend file) before the file is 
written to disk.

> Calling (vc-mode-line file) at this point would be for the side-effect
> of that function, which is bad in my experience. It would harden
> maintenance, 'cause nobody will know why we want to refresh the modeline
> at this point.

I meant, always calling (vc-mode-line file) in `vc-after-save', moving 
it outside of the (and backend ...) check.
This does have a performance downside with non-VC file buffers, of a 
couple additional function calls, 'vc-backend cache lookup, and 
unconditional mode-line update.

So from this standpoint, saving and restoring 'vc-backend value around 
clearing props in `vc-before-save' might indeed be the best solution.

>> By the way, this last patch I sent doesn't help if the user just
>> removed the file from repository while leaving it on disk (git rm
>> --cached ... && git commit ..., for example), but whatever.
>
> If we use the cache, there will always be a constellation that the cache
> is stale due to external operations. As Stefan said, this is mostly
> uncritical.

I'm bringing attention to these cases because they could be the actual 
benefit of your patch (caching 'git-registered) over my initial patch 
(not calling `vc-git-registered' from `vc-git-state' at all).
The original code supported these edge cases, at the cost of additional 
process call.

-- Dmitry

Message #62 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50; vc-git calls `process-file'
	too many times)
Date: Mon, 02 Jul 2012 16:42:28 +0400

On 02.07.2012 13:08, Michael Albinus wrote:
> Dmitry Gutov <dgutov <at> yandex.ru> writes:
>
>>> Likely, the best option is to call (vc-registered file) after clearing
>>> the file cache. This recomputes the 'vc-backend property as well, what
>>> we want.
>>
>> Have you tried this? (vc-backend file) gets called just a bit later,
>> in `vc-after-save`, and it calls `vc-registered' in turn, but since
>> the file is no longer registered with Git, it returns nil, and the
>> mode-line is not updated.
>
> Well, I've played with it. Scenario:

I believe both scenarios below would work just as well with my original 
patch.

> - Have a git-controlled file on disk
> - Load it in Emacs.
> - Remove it on disk, outside Emacs.
> - Save it from Emacs
>
> In vc-before-save, the file cache is cleared 'cause the file doesn't
> exist any longer. As you said, vc-backend is called afterwards,
> recomputing some of the properties. The file shows the correct modeline,
> "Git:master".

If you just removed it on disk, without committing the change, doing 
(vc-state-refresh) would correctly recompute its status as 'removed even 
without calling (vc-registered).
That's not the status we have a problem with ('unregistered is).

> 2nd scenario:
>
> - Switch to another branch ("git checkout test")
> - Remove the file on disk, outside Emacs.
> - Save it from Emacs
>
> And now the modeline shows "Git:test", as expected.

Was the file absent in the branch test after checkout? If not, this case 
is no different from the first.
Basically, we need a scenario in which `vc-next-action' will need to 
call `vc-git-register' on a file that recently has been considered 
'up-to-date.

>> So from this standpoint, saving and restoring 'vc-backend value around
>> clearing props in `vc-before-save' might indeed be the best solution.
>
> I believe, we even don't need this. The current code DTRT, and clearing
> the properties happens only when the file has been removed.

I believe we can either not call 'vc-git-registered' from `vc-git-state' 
at all (caching 'vc-registered is not needed in this case), or we should 
fix some scenario which benefits from us doing that.

The logic is rather complicated there, so I might easily be missing some 
examples. If so, please tell.

-- Dmitry

Message #65 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50; vc-git calls `process-file'
	too many times)
Date: Mon, 02 Jul 2012 16:44:22 +0400

On 02.07.2012 13:08, Michael Albinus wrote:
> Dmitry Gutov <dgutov <at> yandex.ru> writes:
>
>>> Likely, the best option is to call (vc-registered file) after clearing
>>> the file cache. This recomputes the 'vc-backend property as well, what
>>> we want.
>>
>> Have you tried this? (vc-backend file) gets called just a bit later,
>> in `vc-after-save`, and it calls `vc-registered' in turn, but since
>> the file is no longer registered with Git, it returns nil, and the
>> mode-line is not updated.
>
> Well, I've played with it. Scenario:

I believe both scenarios below would work just as well with my original 
patch.

> - Have a git-controlled file on disk
> - Load it in Emacs.
> - Remove it on disk, outside Emacs.
> - Save it from Emacs
>
> In vc-before-save, the file cache is cleared 'cause the file doesn't
> exist any longer. As you said, vc-backend is called afterwards,
> recomputing some of the properties. The file shows the correct modeline,
> "Git:master".

If you just removed it on disk, without committing the change, doing 
(vc-state-refresh) would correctly recompute its status as 'removed even 
without calling (vc-registered).
That's not the status we have a problem with ('unregistered is).

> 2nd scenario:
>
> - Switch to another branch ("git checkout test")
> - Remove the file on disk, outside Emacs.
> - Save it from Emacs
>
> And now the modeline shows "Git:test", as expected.

Was the file absent in the branch test after checkout? If not, this case 
is no different from the first.
Basically, we need a scenario in which `vc-next-action' will need to 
call `vc-git-register' on a file that recently has been considered 
'up-to-date.

>> So from this standpoint, saving and restoring 'vc-backend value around
>> clearing props in `vc-before-save' might indeed be the best solution.
>
> I believe, we even don't need this. The current code DTRT, and clearing
> the properties happens only when the file has been removed.

I believe we can either not call 'vc-git-registered' from `vc-git-state' 
at all (caching 'git-registered is not needed in this case), or we 
should fix some scenario which benefits from us doing that.

The logic is rather complicated there, so I might easily be missing some 
examples. If so, please tell.

-- Dmitry

Message #68 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50;
	vc-git calls `process-file' too many times)
Date: Wed, 04 Jul 2012 17:10:12 +0200

Dmitry Gutov <dgutov <at> yandex.ru> writes:

> Was the file absent in the branch test after checkout? If not, this
> case is no different from the first.
> Basically, we need a scenario in which `vc-next-action' will need to
> call `vc-git-register' on a file that recently has been considered
> up-to-date.

If we assume, any command outside Emacs can happen which invalidates the
cached status of a file, we must clear all caches and recompute all
files state when vc-next-action is called. To the given cost.

In Tramp, I have similar problems with stale caches. Finally, I've added
timestamps to every cached value, and I use cheap tests to check whether
the cache is out of date. No idea, whether we want go this direction in
vc, too.

If we assume that there are no dangerous vc commands outside Emacs, we
wouldn't have a problem.

> The logic is rather complicated there, so I might easily be missing
> some examples.

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.

> -- Dmitry

Best regards, Michael.

Message #71 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50; vc-git calls `process-file'
	too many times)
Date: Wed, 04 Jul 2012 20:42:40 +0400

On 04.07.2012 19:10, Michael Albinus wrote:
>> Was the file absent in the branch test after checkout? If not, this
>> case is no different from the first.
>> Basically, we need a scenario in which `vc-next-action' will need to
>> call `vc-git-register' on a file that recently has been considered
>> up-to-date.
>
> If we assume, any command outside Emacs can happen which invalidates the
> cached status of a file, we must clear all caches and recompute all
> files state when vc-next-action is called. To the given cost.

I'd have no problem with this, actually - this function is called 
considerably less often than `find-file' or `save-buffer'.

> In Tramp, I have similar problems with stale caches. Finally, I've added
> timestamps to every cached value, and I use cheap tests to check whether
> the cache is out of date. No idea, whether we want go this direction in
> vc, too.

Can't commend on that.

> 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.

>> The logic is rather complicated there, so I might easily be missing
>> some examples.
>
> 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.

Message #74 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50;
	vc-git calls `process-file' too many times)
Date: Fri, 06 Jul 2012 15:44:44 +0200

Dmitry Gutov <dgutov <at> 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.

>> 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).

Best regards, Michael.

Message #77 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50; vc-git calls `process-file'
	too many times)
Date: Fri, 06 Jul 2012 19:55:54 +0400

[Message part 1 (text/plain, inline)]

On 06.07.2012 17:44, Michael Albinus wrote:
> Dmitry Gutov <dgutov <at> 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

[vc-git-registered.diff (text/plain, attachment)]

Message #80 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: 11757 <at> debbugs.gnu.org
Subject: Fwd: bug#11757: Acknowledgement (24.1.50;
	vc-git calls `process-file' too many times)
Date: Wed, 18 Jul 2012 16:11:14 +0200

[Message part 1 (text/plain, inline)]

Forwarded to debbugs, for the archives.

[Message part 2 (message/rfc822, inline)]

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Michael Albinus <michael.albinus <at> gmx.de>
Subject: Re: Fwd: Re: bug#11757: Acknowledgement (24.1.50; vc-git calls `process-file'
 too many times)
Date: Wed, 18 Jul 2012 08:07:46 +0400

[Message part 3 (text/plain, inline)]

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

[vc-git.diff (text/plain, attachment)]

[trace-1.log (text/plain, attachment)]

[trace-2.log (text/plain, attachment)]

[trace-3.log (text/plain, attachment)]

[trace-4.log (text/plain, attachment)]

Message #83 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50;
	vc-git calls `process-file' too many times)
Date: Wed, 18 Jul 2012 16:38:27 +0200

Dmitry Gutov <dgutov <at> yandex.ru> writes:

> 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 scenario seems to be sufficient.

> 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).

Yes. Just naively counting `process-file' calls, the unpatched version
uses 27 calls, and the patched version needs 26. This is likely, because
the patched version doesn't call `vc-git-registered' in `vc-git-state'.

> 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.

Likely, it doesn't matter. Such a scenario is not what we can expect to
happen often (open an unregistered file in Emacs, it is registered
outside Emacs, and we try to register it inside Emacs). It will result
in an error, that's it.

Finally, I will apply your patch to vc-git.el. Thanks for all your
tests!

> 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.

No problem here. That chunk is shown to me by ediff.

> --Dmitry

Best regards, Michael.

Message #86 received at 11757 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 11757 <at> debbugs.gnu.org
Subject: Re: bug#11757: Acknowledgement (24.1.50; vc-git calls `process-file'
	too many times)
Date: Wed, 18 Jul 2012 21:01:47 +0400

On 18.07.2012 18:38, Michael Albinus wrote:
> Finally, I will apply your patch to vc-git.el. Thanks for all your
> tests!

Cool, thanks! I'll try to close this now.

>> 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.
>
> No problem here. That chunk is shown to me by ediff.

Must be my version of diff, then.

--Dmitry

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.