bug#42966: 28.0.50; vc-dir: wrong backend

bug#42966: 28.0.50; vc-dir: wrong backend

[Sam Steingold]

My home directory is under `git` and `~/.gitignore` contains `/src` so
that directories under `~/src` can be handled separately, under
different repos.

`~/src/cl/clocc` is a managed by mercurial and files there are identified
by Emacs as such, as indicated in the mode line as `Hg:1234567`.

However, when I do `C-x v d` in a buffer visiting
`~/src/cl/clocc/src/port/proc.lisp`, I get a prompt

VC status for directory: ~/src/cl/clocc/

(note the _correct_ default mercurial root directory!) and hit RET, I
get the `*vc-dr*` buffer with

--8<---------------cut here---------------start------------->8---
VC backend : Git
Working dir: ~/src/cl/clocc/
Branch     : master
Remote     : git@gitlab.com:sam-s/home.git
Stash      : Nothing stashed

                         ./

--8<---------------cut here---------------end--------------->8---

note the incorrect `VC backend` and `Remote` (which should be `Hg` and
`ssh://sds@hg.code.sf.net/p/clocc/hg` resp.)

This is because `vc-responsible-backend` calls `responsible-p` and
`vc-git-responsible-p` is aliased to `vc-git-root` which merely looks
for `.git` above the argument, not paying attention to `.gitignore`.




In GNU Emacs 28.0.50 (build 6, x86_64-apple-darwin19.6.0, NS appkit-1894.60 Version 10.15.6 (Build 19G2021))
 of 2020-08-17 built on BZ-C02XR5CGJG5L
Repository revision: e5d4fae6797330d91e901c7ecb1412551db12f6a
Repository branch: master
Windowing system distributor 'Apple', version 10.3.1894
System Description:  Mac OS X 10.15.6
Configured using:
 'configure --with-imagemagick --with-mailutils --with-ns
 PKG_CONFIG_PATH=/usr/local/opt/libxml2/lib/pkgconfig:/usr/local/opt/imagemagick/lib/pkgconfig:/usr/local/opt/gnutls/lib/pkgconfig:/usr/local/opt/jansson/lib/pkgconfig:/usr/local/opt/libtiff/lib/pkgconfig:/usr/local/opt/libpng/lib/pkgconfig:/usr/local/opt/libjpeg/lib/pkgconfig:/usr/local/opt/freetype/lib/pkgconfig'

Configured features:
JPEG TIFF PNG IMAGEMAGICK NOTIFY KQUEUE ACL GNUTLS LIBXML2 ZLIB
TOOLKIT_SCROLL_BARS NS MODULES THREADS JSON PDUMPER LCMS2

Important settings:
  value of $LANG: C
  locale-coding-system: utf-8-unix

Features:
(shadow bbdb-message mailalias cookie1 emacsbug sendmail cl-indent
face-remap inf-lisp vc-hg url-http url-gw url-auth url-queue url-cache
gnus-fun time nndoc flow-fill tramp-cmds sort gnus-cite smiley
mm-archive gnus-async gnus-bcklg gnus-dup qp mail-extr gnus-ml hl-line
disp-table spam spam-stat gnus-uu yenc nndraft nnmh gnus-agent gnus-srvr
gnus-score score-mode nnvirtual gnus-msg gnus-art mm-uu mml2015 mm-view
mml-smime smime dig utf-7 gnus-cache gnus-sum url url-proxy url-privacy
url-expand url-methods url-history mailcap shr url-cookie url-domsuf
url-util svg xml dom bbdb-gnus gnutls network-stream nsm nntp gnus-group
gnus-undo gnus-start gnus-cloud nnimap nnmail mail-source utf7 netrc
nnoo gnus-spec gnus-int gnus-range gnus-win doc-view jka-compr
image-mode exif vc-annotate pulse tabify cl-print debug backtrace
tramp-cache cal-move cal-x view cal-china cal-bahai cal-islam holidays
hol-loaddefs bbdb-anniv cal-iso cal-hebrew lunar cal-julian solar
cal-dst appt diary-lib diary-loaddefs cal-menu calendar cal-loaddefs
find-dired bug-reference eieio-opt speedbar ezimage dframe find-func
misearch multi-isearch log-view skeleton dabbrev log-edit message rmc
puny rfc822 mml mml-sec epa derived epg epg-config mm-decode mm-bodies
mm-encode mail-parse rfc2231 gmm-utils mailheader pcvs-util smerge-mode
diff vc-bzr vc-src vc-sccs vc-svn vc-cvs vc-rcs add-log remember
tex-mode dired-aux dired dired-loaddefs inf-ruby ruby-mode yaml-mode
vc-dir ewoc vc vc-dispatcher sh-script smie executable conf-mode
company-oddmuse company-keywords company-etags company-gtags
company-dabbrev-code company-dabbrev company-files company-clang
company-template company-cmake company-bbdb yasnippet-snippets cl-extra
yasnippet flymake-proc flymake thingatpt company-capf company pcase
help-fns radix-tree help-mode elpy edmacro kmacro elpy-rpc pyvenv eshell
esh-cmd esh-ext esh-opt esh-proc esh-io esh-arg esh-module esh-groups
esh-util elpy-shell elpy-profile elpy-django s elpy-refactor ido grep
compile etags fileloop generator xref project cus-edit python tramp-sh
tramp tramp-loaddefs trampver tramp-integration files-x tramp-compat
shell pcomplete parse-time iso8601 ls-lisp format-spec comint ansi-color
vc-git diff-mode easy-mmode flyspell ispell midnight warnings gnus
nnheader gnus-util rmail rmail-loaddefs rfc2047 rfc2045 ietf-drums
text-property-search time-date mail-utils mm-util mail-prsvr wid-edit
bbdb-mua bbdb-com crm mailabbrev bbdb bbdb-site timezone edit-server
advice server winner ring which-func imenu paren help-at-pt desktop
frameset cus-start cus-load info package easymenu browse-url
url-handlers url-parse auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache json subr-x map url-vars seq byte-opt gv
bytecomp byte-compile cconv cl-loaddefs cl-lib tooltip eldoc electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel term/ns-win ns-win
ucs-normalize mule-util term/common-win tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode elisp-mode
lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow isearch
timer select scroll-bar mouse jit-lock font-lock syntax facemenu
font-core term/tty-colors frame minibuffer cl-generic cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote threads kqueue cocoa ns
lcms2 multi-tty make-network-process emacs)

Memory information:
((conses 16 679755 110407)
 (symbols 48 32339 6)
 (strings 32 227857 7588)
 (string-bytes 1 6102300)
 (vectors 16 88150)
 (vector-slots 8 2148274 178424)
 (floats 8 1143 845)
 (intervals 56 46064 762)
 (buffers 992 121))

-- 
Sam Steingold (http://sds.podval.org/) on darwin Ns 10.3.1894
http://childpsy.net http://calmchildstories.com http://steingoldpsychology.com
https://mideasttruth.com https://ffii.org https://www.dhimmitude.org
Daddy, why doesn't this magnet pick up this floppy disk?

bug#42966: 28.0.50; vc-dir: wrong backend

[Lars Ingebrigtsen]

Sam Steingold <sds@gnu.org> writes:

> My home directory is under `git` and `~/.gitignore` contains `/src` so
> that directories under `~/src` can be handled separately, under
> different repos.
>
> `~/src/cl/clocc` is a managed by mercurial and files there are identified
> by Emacs as such, as indicated in the mode line as `Hg:1234567`.
>
> However, when I do `C-x v d` in a buffer visiting
> `~/src/cl/clocc/src/port/proc.lisp`, I get a prompt
>
> VC status for directory: ~/src/cl/clocc/
>
> (note the _correct_ default mercurial root directory!) and hit RET, I
> get the `*vc-dr*` buffer with
>
> VC backend : Git
> Working dir: ~/src/cl/clocc/
> Branch     : master
> Remote     : git@gitlab.com:sam-s/home.git
> Stash      : Nothing stashed

I don't think the .gitignore helps much here -- Emacs doesn't look that
hard at the various backend's ignore capabilities.

But:

      (catch 'found
	;; First try: find a responsible backend.  If this is for registration,
	;; it must be a backend under which FILE is not yet registered.
	(dolist (backend vc-handled-backends)
	  (and (vc-call-backend backend 'responsible-p file)
	       (throw 'found backend))))

This just goes through the backends and the first one that happens to be
able to say "yes" wins.  Shouldn't this instead go through all the
backends, and if more than one says "yes", then choose the one with the
most specific path?

Looking at the code, that shouldn't be too hard to implement, because it
seems like responsible-p returns the root path? 

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#42966: 28.0.50; vc-dir: wrong backend

[Dmitry Gutov]

On 16.10.2020 11:55, Lars Ingebrigtsen wrote:

> But:
> 
>        (catch 'found
> 	;; First try: find a responsible backend.  If this is for registration,
> 	;; it must be a backend under which FILE is not yet registered.
> 	(dolist (backend vc-handled-backends)
> 	  (and (vc-call-backend backend 'responsible-p file)
> 	       (throw 'found backend))))
> 
> This just goes through the backends and the first one that happens to be
> able to say "yes" wins.  Shouldn't this instead go through all the
> backends, and if more than one says "yes", then choose the one with the
> most specific path?
> 
> Looking at the code, that shouldn't be too hard to implement, because it
> seems like responsible-p returns the root path?

The code should be straightforward, but I'd like to see some performance 
measurements: both for the local case, and for the remote one (Tramp).

The difference can be small, though, given that we already try a number 
of other backends first (Git is near the end of vc-handled-backends; we 
might want to change that, BTW).

bug#42966: 28.0.50; vc-dir: wrong backend

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> The code should be straightforward, but I'd like to see some
> performance measurements: both for the local case, and for the remote
> one (Tramp).
>
> The difference can be small, though, given that we already try a
> number of other backends first (Git is near the end of
> vc-handled-backends; we might want to change that, BTW).

As you point out, Git is already towards the end, so the typical case
would just be two more tests, and both of the ones after Git (Hg and
Mtn) look pretty trivial, too.

So it looks rather unlikely that there'd be a noticeable performance
impact, I think?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#42966: 28.0.50; vc-dir: wrong backend

[Dmitry Gutov]

On 16.10.2020 17:41, Lars Ingebrigtsen wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
>> The code should be straightforward, but I'd like to see some
>> performance measurements: both for the local case, and for the remote
>> one (Tramp).
>>
>> The difference can be small, though, given that we already try a
>> number of other backends first (Git is near the end of
>> vc-handled-backends; we might want to change that, BTW).
> 
> As you point out, Git is already towards the end, so the typical case
> would just be two more tests, and both of the ones after Git (Hg and
> Mtn) look pretty trivial, too.
> 
> So it looks rather unlikely that there'd be a noticeable performance
> impact, I think?

Let's measure it anyway, because the potential impact is big (an extra 
delay when opening any file).

bug#42966: 28.0.50; vc-dir: wrong backend

[Lars Ingebrigtsen]

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

Dmitry Gutov <dgutov@yandex.ru> writes:

> Let's measure it anyway, because the potential impact is big (an extra
> delay when opening any file).

Sure.  I've set up a directory structure with a hg repo inside a git
repo (tar file included as an attachment).

Here's the benchmarks on a local machine and a remote machine with the
current code:

(benchmark-run 1000
  (vc-responsible-backend "/tmp/git-dir/dir1/dir2/hg-dir/bar"))
(0.47081299800000004 10 0.07627535899999999)

(benchmark-run 100
  (vc-responsible-backend "/ssh:stories:/tmp/git-dir/dir1/dir2/hg-dir/bar"))
(2.8259669379999997 99 0.912024865)

Benchmarking for the rewritten code to follow, once I've rewritten it.  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

[-- Attachment #2: vc-test.tgz --]
[-- Type: application/x-gtar-compressed, Size: 10828 bytes --]

bug#42966: 28.0.50; vc-dir: wrong backend

[Lars Ingebrigtsen]

And here's the benching with the patch applied:

(benchmark-run 1000
  (vc-responsible-backend "/tmp/git-dir/dir1/dir2/hg-dir/bar"))
=> (0.375446369 10 0.07836344099999998)

(benchmark-run 100
  (vc-responsible-backend "/ssh:stories:/tmp/git-dir/dir1/dir2/hg-dir/bar"))
=> (3.485639896 110 1.00616348)

Er...  the local version is now faster?  Is a throw expensive, somehow?
Probably not very significant.

But as expected, the tramp version is slower, because it does more
lookups remotely.  But not hugely.

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 39d0fab391..899f260089 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -979,12 +979,20 @@ vc-responsible-backend
 If NO-ERROR is nil, signal an error that no VC backend is
 responsible for the given file."
   (or (and (not (file-directory-p file)) (vc-backend file))
-      (catch 'found
-	;; First try: find a responsible backend.  If this is for registration,
-	;; it must be a backend under which FILE is not yet registered.
-	(dolist (backend vc-handled-backends)
-	  (and (vc-call-backend backend 'responsible-p file)
-	       (throw 'found backend))))
+      ;; First try: find a responsible backend.  If this is for registration,
+      ;; it must be a backend under which FILE is not yet registered.
+      (let ((dirs (delq nil
+                        (mapcar
+                         (lambda (backend)
+                           (vc-call-backend backend 'responsible-p file))
+                         vc-handled-backends))))
+        ;; Just a single response (or none); use it.
+        (if (< (length dirs) 2)
+            (car dirs)
+          ;; Several roots; we seem to have one vc inside another's
+          ;; directory.  Choose the most specific.
+          (car (sort dirs (lambda (d1 d2)
+                            (< (length d2) (length d1)))))))
       (unless no-error
         (error "No VC backend is responsible for %s" file))))
 

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#42966: 28.0.50; vc-dir: wrong backend

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> And here's the benching with the patch applied:

Fixed patch:

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 39d0fab391..8def7da377 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -979,12 +979,22 @@ vc-responsible-backend
 If NO-ERROR is nil, signal an error that no VC backend is
 responsible for the given file."
   (or (and (not (file-directory-p file)) (vc-backend file))
-      (catch 'found
-	;; First try: find a responsible backend.  If this is for registration,
-	;; it must be a backend under which FILE is not yet registered.
-	(dolist (backend vc-handled-backends)
-	  (and (vc-call-backend backend 'responsible-p file)
-	       (throw 'found backend))))
+      ;; First try: find a responsible backend.  If this is for registration,
+      ;; it must be a backend under which FILE is not yet registered.
+      (let ((dirs (delq nil
+                        (mapcar
+                         (lambda (backend)
+                           (when-let ((dir (vc-call-backend
+                                            backend 'responsible-p file)))
+                             (cons backend dir)))
+                         vc-handled-backends))))
+        ;; Just a single response (or none); use it.
+        (if (< (length dirs) 2)
+            (caar dirs)
+          ;; Several roots; we seem to have one vc inside another's
+          ;; directory.  Choose the most specific.
+          (caar (sort dirs (lambda (d1 d2)
+                             (< (length (cdr d2)) (length (cdr d1))))))))
       (unless no-error
         (error "No VC backend is responsible for %s" file))))
 


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#42966: 28.0.50; vc-dir: wrong backend

[Dmitry Gutov]

On 17.10.2020 10:19, Lars Ingebrigtsen wrote:
> Fixed patch:

Could you send an attachment?

If I just copy this, diff-apply-hunk first proposes to fix whitespace, 
and then doesn't apply anything.

bug#42966: 28.0.50; vc-dir: wrong backend

[Lars Ingebrigtsen]

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

Dmitry Gutov <dgutov@yandex.ru> writes:

> Could you send an attachment?

Included here.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 39d0fab391..8def7da377 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -979,12 +979,22 @@ vc-responsible-backend
 If NO-ERROR is nil, signal an error that no VC backend is
 responsible for the given file."
   (or (and (not (file-directory-p file)) (vc-backend file))
-      (catch 'found
-	;; First try: find a responsible backend.  If this is for registration,
-	;; it must be a backend under which FILE is not yet registered.
-	(dolist (backend vc-handled-backends)
-	  (and (vc-call-backend backend 'responsible-p file)
-	       (throw 'found backend))))
+      ;; First try: find a responsible backend.  If this is for registration,
+      ;; it must be a backend under which FILE is not yet registered.
+      (let ((dirs (delq nil
+                        (mapcar
+                         (lambda (backend)
+                           (when-let ((dir (vc-call-backend
+                                            backend 'responsible-p file)))
+                             (cons backend dir)))
+                         vc-handled-backends))))
+        ;; Just a single response (or none); use it.
+        (if (< (length dirs) 2)
+            (caar dirs)
+          ;; Several roots; we seem to have one vc inside another's
+          ;; directory.  Choose the most specific.
+          (caar (sort dirs (lambda (d1 d2)
+                             (< (length (cdr d2)) (length (cdr d1))))))))
       (unless no-error
         (error "No VC backend is responsible for %s" file))))
 

bug#42966: 28.0.50; vc-dir: wrong backend

[Dmitry Gutov]

On 17.10.2020 10:13, Lars Ingebrigtsen wrote:
> And here's the benching with the patch applied:
> 
> (benchmark-run 1000
>    (vc-responsible-backend "/tmp/git-dir/dir1/dir2/hg-dir/bar"))
> => (0.375446369 10 0.07836344099999998)
> 
> (benchmark-run 100
>    (vc-responsible-backend "/ssh:stories:/tmp/git-dir/dir1/dir2/hg-dir/bar"))
> => (3.485639896 110 1.00616348)
> 
> Er...  the local version is now faster?  Is a throw expensive, somehow?
> Probably not very significant.

Is it possible that you didn't restart Emacs between the tests?

vc-git-root (at least) does some caching, which muddies the waters.

Or try this test, with both versions of code:

(benchmark-run 1000
   (progn (vc-file-clearprops FILE)
          (vc-responsible-backend FILE)))

> But as expected, the tramp version is slower, because it does more
> lookups remotely.  But not hugely.

By 23%? That's a bit more than I expected just by looking at 
vc-handled-backends, which has 9 elements. 1/9 => 11%.

I don't have a strong opinion on the remote performance, but we might 
want to ask Michael. Looking at the code, it seems to have forced him to 
unfortunate measures in the past (such as adding said caching to 
vc-git-root).

bug#42966: 28.0.50; vc-dir: wrong backend

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

Hi Dmitry,

> I don't have a strong opinion on the remote performance, but we might
> want to ask Michael. Looking at the code, it seems to have forced him
> to unfortunate measures in the past (such as adding said caching to
> vc-git-root).

I didn't follow the thread closely. Could you pls explain, what you find
unfortunate in Tramp, and how to fix it?

Best regards, Michael.

bug#42966: 28.0.50; vc-dir: wrong backend

[Dmitry Gutov]

Hi Michael,

On 18.10.2020 10:38, Michael Albinus wrote:

>> I don't have a strong opinion on the remote performance, but we might
>> want to ask Michael. Looking at the code, it seems to have forced him
>> to unfortunate measures in the past (such as adding said caching to
>> vc-git-root).
> 
> I didn't follow the thread closely. Could you pls explain, what you find
> unfortunate in Tramp, and how to fix it?

Not in Tramp, but I see an old change in VC that was most likely 
informed by a performance problem in Tramp.

See the commit a40c87a0093. It adds caching of the result of vc-git-root 
to a VC property 'git-root' on the file name.

There are a couple problems with that:

- If FILE is a directory, this cache will never be invalidated (for 
plain files, vc-file-clearprops is called from a number of functions, 
including vc-refresh-state).

- We don't do this for any other backends, which leads to 
inconsistencies in behavior, as well as surprising results, like when we 
did that performance test earlier. I think this is the reason for it.

If we did this uniformly for all backends, we could perhaps find better 
opportunities for caching and invalidation.

But before we look into that, could you try reproducing the original 
problem?

Does the change below still make some scenario perceptibly slower?

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index b1880c0f7b..91554bb6d8 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1573,8 +1573,7 @@ vc-git-extra-menu
  (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"))))
+  (vc-find-root file ".git"))

  ;; grep-compute-defaults autoloads grep.
  (declare-function grep-read-regexp "grep" ())

bug#42966: 28.0.50; vc-dir: wrong backend

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Hi Michael,

Hi Dmitry,

> Not in Tramp, but I see an old change in VC that was most likely
> informed by a performance problem in Tramp.
>
> See the commit a40c87a0093. It adds caching of the result of
> vc-git-root to a VC property 'git-root' on the file name.

This seems to be bug#11757.

> Does the change below still make some scenario perceptibly slower?
>
> diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
> index b1880c0f7b..91554bb6d8 100644
> --- a/lisp/vc/vc-git.el
> +++ b/lisp/vc/vc-git.el
> @@ -1573,8 +1573,7 @@ vc-git-extra-menu
>  (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"))))
> +  (vc-find-root file ".git"))
>
>  ;; grep-compute-defaults autoloads grep.
>  (declare-function grep-read-regexp "grep" ())

I haven't tested. But this means to call process-file several times,
it would be a performance degradation, for sure.

Best regards, Michael.

bug#42966: 28.0.50; vc-dir: wrong backend

[Dmitry Gutov]

On 24.10.2020 16:41, Michael Albinus wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
>> Hi Michael,
> 
> Hi Dmitry,
> 
>> Not in Tramp, but I see an old change in VC that was most likely
>> informed by a performance problem in Tramp.
>>
>> See the commit a40c87a0093. It adds caching of the result of
>> vc-git-root to a VC property 'git-root' on the file name.
> 
> This seems to be bug#11757.

Huh. Looks familiar ;-)

But that report was about process calls (and git.cmd being expensive), 
whereas vc-git-root doesn't call any external programs, it just 
traverses the filesystem.

>> Does the change below still make some scenario perceptibly slower?
>>
>> diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
>> index b1880c0f7b..91554bb6d8 100644
>> --- a/lisp/vc/vc-git.el
>> +++ b/lisp/vc/vc-git.el
>> @@ -1573,8 +1573,7 @@ vc-git-extra-menu
>>   (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"))))
>> +  (vc-find-root file ".git"))
>>
>>   ;; grep-compute-defaults autoloads grep.
>>   (declare-function grep-read-regexp "grep" ())
> 
> I haven't tested. But this means to call process-file several times,
> it would be a performance degradation, for sure.

Could you test it, please?

AFAICT locate-dominating-file doesn't call process-file.

bug#42966: 28.0.50; vc-dir: wrong backend

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

Hi Dmitry,

>> I haven't tested. But this means to call process-file several times,
>> it would be a performance degradation, for sure.
>
> Could you test it, please?
>
> AFAICT locate-dominating-file doesn't call process-file.

I've tried benchmark over vc-registered and over vc-git-root, with and
w/o that change. Results are similar.

Best regards, Michael.

bug#42966: 28.0.50; vc-dir: wrong backend

[Dmitry Gutov]

Hi Michael,

On 25.10.2020 19:49, Michael Albinus wrote:
> I've tried benchmark over vc-registered and over vc-git-root, with and
> w/o that change. Results are similar.

So, we can safely apply that change?

bug#42966: 28.0.50; vc-dir: wrong backend

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Hi Michael,

Hi Dmitry,

>> I've tried benchmark over vc-registered and over vc-git-root, with and
>> w/o that change. Results are similar.
>
> So, we can safely apply that change?

I don't know whether we can apply it "safely". But I agree we shall do
it. And then, we will observe, whether there are regression reports.

Best regards, Michael.

bug#42966: (no subject)

[Lars Ingebrigtsen]

Michael Albinus <michael.albinus@gmx.de> writes:

> I don't know whether we can apply it "safely". But I agree we shall do
> it. And then, we will observe, whether there are regression reports.

OK; I've now pushed the change.

There was some discussion about implementing this "inside out", which
sounds like a good idea, but would mainly be a performance tweak.

So I'm closing this bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#42966: (no subject)

[Lars Ingebrigtsen]

Michael Albinus <michael.albinus@gmx.de> writes:

> I don't know whether we can apply it "safely". But I agree we shall do
> it. And then, we will observe, whether there are regression reports.

Oops; you were talking about the other cache patch here, not my proposed
patch.  Oh, well.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#42966:

[Dmitry Gutov]

On 26.10.2020 22:13, Lars Ingebrigtsen wrote:
> Oops; you were talking about the other cache patch here, not my proposed
> patch.  Oh, well.

Yes, it was a side investigation to clear up the irregularity in your 
testing.

bug#42966:

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 26.10.2020 22:13, Lars Ingebrigtsen wrote:
>> Oops; you were talking about the other cache patch here, not my proposed
>> patch.  Oh, well.
>
> Yes, it was a side investigation to clear up the irregularity in your
> testing.

I guess we'll find out if my patch leads to performance regressions now,
then.  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#42966:

[Dmitry Gutov]

On 26.10.2020 23:02, Lars Ingebrigtsen wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
>> On 26.10.2020 22:13, Lars Ingebrigtsen wrote:
>>> Oops; you were talking about the other cache patch here, not my proposed
>>> patch.  Oh, well.
>>
>> Yes, it was a side investigation to clear up the irregularity in your
>> testing.
> 
> I guess we'll find out if my patch leads to performance regressions now,
> then.  :-)

Yeah, all right.

In my testing locally it's fast enough (1000 iterations is plenty).

I'd test with an actual remote host, though: when the ping is >100ms (a 
regular occurrence in my life: ping 8.8.8.8 is 73ms, though I don't 
often use Tramp), the cost of one process call would look different.

But anyway, if Michael doesn't object to this change, I definitely won't 
argue.

bug#42966: 28.0.50; vc-dir: wrong backend

[Dmitry Gutov]

On 26.10.2020 17:17, Michael Albinus wrote:
> I don't know whether we can apply it "safely". But I agree we shall do
> it. And then, we will observe, whether there are regression reports.

And done.

bug#42966: 28.0.50; vc-dir: wrong backend

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Is it possible that you didn't restart Emacs between the tests?

No, I restarted between tests...  But perhaps I should have done more
iterations.

>> But as expected, the tramp version is slower, because it does more
>> lookups remotely.  But not hugely.
>
> By 23%? That's a bit more than I expected just by looking at
> vc-handled-backends, which has 9 elements. 1/9 => 11%.

Yes, it's a larger slowdown than expected.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#42966: 28.0.50; vc-dir: wrong backend

[Glenn Morris]

This is a long standing known issue.
See eg https://debbugs.gnu.org/3807#21 from 11 years ago.

Or even https://debbugs.gnu.org/8179. :)

bug#42966: 28.0.50; vc-dir: wrong backend

[Dmitry Gutov]

On 16.10.2020 18:35, Glenn Morris wrote:
> See eghttps://debbugs.gnu.org/3807#21  from 11 years ago.

Stefan's suggestion is pretty sensible.

Though it'll require a rework of the corresponding VC backend actions. 
Not sure if it's possible to do in a backward-compatible fashion.

bug#42966: 28.0.50; vc-dir: wrong backend

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 16.10.2020 18:35, Glenn Morris wrote:
>> See eghttps://debbugs.gnu.org/3807#21  from 11 years ago.
>
> Stefan's suggestion is pretty sensible.
>
> Though it'll require a rework of the corresponding VC backend
> actions. Not sure if it's possible to do in a backward-compatible
> fashion.

(The suggestion is to recurse upwards and ask each backend "are you
responsible for this directory, then?")

That makes sense, but it's just a performance hack, isn't it?  The
result should be the same as the less invasive "loop over all the
backends and collect the most specific one".

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#42966: 28.0.50; vc-dir: wrong backend

[Dmitry Gutov]

On 17.10.2020 09:06, Lars Ingebrigtsen wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
>> On 16.10.2020 18:35, Glenn Morris wrote:
>>> See eghttps://debbugs.gnu.org/3807#21  from 11 years ago.
>>
>> Stefan's suggestion is pretty sensible.
>>
>> Though it'll require a rework of the corresponding VC backend
>> actions. Not sure if it's possible to do in a backward-compatible
>> fashion.
> 
> (The suggestion is to recurse upwards and ask each backend "are you
> responsible for this directory, then?")

Or, more low-level, if we find that every backend follows the pattern of 
aliasing vc-xyz-responsible-p to vc-xyz-root, and calling vc-find-root 
in the latter's implementation, we could opt for creating a backend 
action that returns the "witness" file name (e.g. ".git"), and then 
construct a regexp from all witness file names, and pass it to 
'directory-files' as MATCH. Depending on the cost of certain things, 
this could end up being much faster, both locally and remotely.

> That makes sense, but it's just a performance hack, isn't it?  The
> result should be the same as the less invasive "loop over all the
> backends and collect the most specific one".

Pretty much. Except it should naturally limit the traversal up the 
directory tree, so it feels like a good architecture, not just a "hack".

The backward compatibility headache might not be worth it, though.

bug#42966: 28.0.50; vc-dir: wrong backend

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Or, more low-level, if we find that every backend follows the pattern
> of aliasing vc-xyz-responsible-p to vc-xyz-root, and calling
> vc-find-root in the latter's implementation, we could opt for creating
> a backend action that returns the "witness" file name (e.g. ".git"),
> and then construct a regexp from all witness file names, and pass it
> to 'directory-files' as MATCH. Depending on the cost of certain
> things, this could end up being much faster, both locally and
> remotely.

That does sound a lot faster.  Let's see...

./lisp/obsolete/vc-arch.el491:(defalias 'vc-arch-responsible-p 'vc-arch-root)
./lisp/vc/vc-git.el873:(defalias 'vc-git-responsible-p 'vc-git-root)
./lisp/vc/vc-bzr.el646:(defalias 'vc-bzr-responsible-p 'vc-bzr-root
./lisp/vc/vc-hg.el1177:(defalias 'vc-hg-responsible-p 'vc-hg-root)
./lisp/vc/vc-svn.el313:(defalias 'vc-svn-responsible-p 'vc-svn-root)
./lisp/vc/vc-mtn.el201:(defun vc-mtn-responsible-p (file) (vc-mtn-root file))

Then:

./lisp/vc/vc-rcs.el284:(defun vc-rcs-responsible-p (file)

(defun vc-rcs-responsible-p (file)
  (file-directory-p (expand-file-name "RCS"
                                      (if (file-directory-p file)
                                          file
                                        (file-name-directory file)))))

So, basically the same.

./lisp/vc/vc-dav.el142:(defun vc-dav-responsible-p (url)

(defun vc-dav-responsible-p (url)
  t)

:-/

./lisp/vc/vc-src.el248:(defun vc-src-responsible-p (file)

(defun vc-src-responsible-p (file)
  (file-directory-p (expand-file-name ".src"
                                      (if (file-directory-p file)
                                          file
                                        (file-name-directory file)))))


./lisp/vc/vc-sccs.el218:(defun vc-sccs-responsible-p (file)

This one is more complicated:

(defun vc-sccs-responsible-p (file)
  (or (file-directory-p (expand-file-name "SCCS" (file-name-directory file)))
      (stringp (vc-sccs-search-project-dir (or (file-name-directory file) "")
					   (file-name-nondirectory file)))))


./lisp/vc/vc-cvs.el319:(defun vc-cvs-responsible-p (file)

(defun vc-cvs-responsible-p (file)
  (file-directory-p (expand-file-name "CVS"
				      (if (file-directory-p file)
					  file
					(file-name-directory file)))))

So it looks like the only possibly problematic backend is SCCS?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#42966: 28.0.50; vc-dir: wrong backend

[Glenn Morris]

Dmitry Gutov wrote:

>> That makes sense, but it's just a performance hack, isn't it?  The
>> result should be the same as the less invasive "loop over all the
>> backends and collect the most specific one".
>
> Pretty much. Except it should naturally limit the traversal up the
> directory tree, so it feels like a good architecture, not just a
> "hack".

Indeed, it just seems like the Right Thing to Do, not a hack.
Not having been paying attention, I was surprised to see the adopted solution
goes for "loop over every VC backend, and every directory up the tree,
then filter the results", rather than "walk up the directory tree,
stopping when a backend claims responsibility".

I would think efficiency matters in such a frequent operation.
As a (completely unscientific) data point, my first single core
bootstrap build after this change took about 5% longer than before
(22m5s v 21m4s). But of course a single measurement means nothing.

bug#42966: 28.0.50; vc-dir: wrong backend

[Lars Ingebrigtsen]

Glenn Morris <rgm@gnu.org> writes:

> Indeed, it just seems like the Right Thing to Do, not a hack.
> Not having been paying attention, I was surprised to see the adopted solution
> goes for "loop over every VC backend, and every directory up the tree,
> then filter the results", rather than "walk up the directory tree,
> stopping when a backend claims responsibility".

That would be the natural thing to do, but would be an incompatible
interface change.  (Only important for out-of-tree backends, though.)

And it would require tweaking a handful of the in-tree backends; see the
overview I posted the other week, but it didn't look complicated.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#42966: 28.0.50; vc-dir: wrong backend

[Dmitry Gutov]

On 26.10.2020 23:58, Lars Ingebrigtsen wrote:
> And it would require tweaking a handful of the in-tree backends; see the
> overview I posted the other week, but it didn't look complicated.

If only we could delete some problematic older backends as well. ;-)

bug#42966: 28.0.50; vc-dir: wrong backend

[Dmitry Gutov]

On 26.10.2020 23:54, Glenn Morris wrote:
> Dmitry Gutov wrote:
> 
>>> That makes sense, but it's just a performance hack, isn't it?  The
>>> result should be the same as the less invasive "loop over all the
>>> backends and collect the most specific one".
>>
>> Pretty much. Except it should naturally limit the traversal up the
>> directory tree, so it feels like a good architecture, not just a
>> "hack".
> 
> Indeed, it just seems like the Right Thing to Do, not a hack.
> Not having been paying attention, I was surprised to see the adopted solution
> goes for "loop over every VC backend, and every directory up the tree,
> then filter the results", rather than "walk up the directory tree,
> stopping when a backend claims responsibility".

I didn't want to insist on it because upon some thinking it seemed to me 
that the remote case might be the only problematic one. And 
one-traversal-per-backend might be more optimizable by Tramp (e.g. via a 
mini-program) than one-check-per-directory-level. But perhaps the latter 
could be optimized using a file handler or an advice just as well.

> I would think efficiency matters in such a frequent operation.
> As a (completely unscientific) data point, my first single core
> bootstrap build after this change took about 5% longer than before
> (22m5s v 21m4s). But of course a single measurement means nothing.

Was that with a rotating disk?

A few more experiments should help, to establish whether that was a 
fluke or not.