bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Manuel Giraud]

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


When calling 'vc-next-action' on an unregistered file, VC could choose a
backend that is very far above this file in the directory hierarchy.

In my case, it chooses SVN as backend (because I have one in my homedir)
but I wanted it to choose the Git backend of the directory this file
resides.

This patch solves this issue by selecting the backend with the most
specific (in fact, the longest string) path to the file.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-let-VC-find-a-responsible-backend-with-the-most-spec.patch --]
[-- Type: text/x-patch, Size: 1371 bytes --]

From 9e673edc28f84a75f3bf0f53e7fba01c2f50e51c Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Mon, 13 Sep 2021 17:31:31 +0200
Subject: [PATCH] let VC find a responsible backend with the most specific
 path.

---
 lisp/vc/vc.el | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 4fcba65ab4..7160c98820 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -937,11 +937,16 @@ vc-backend-for-registration
 use."
   (catch 'found
     ;; First try: find a responsible backend, it must be a backend
-    ;; under which FILE is not yet registered.
-    (dolist (backend vc-handled-backends)
-      (and (not (vc-call-backend backend 'registered file))
-	   (vc-call-backend backend 'responsible-p file)
-	   (throw 'found backend)))
+    ;; under which FILE is not yet registered and with the most
+    ;; specific path to FILE.
+    (let ((max 0) bk)
+      (dolist (backend vc-handled-backends)
+        (when (not (vc-call-backend backend 'registered file))
+          (let* ((path (vc-call-backend backend 'responsible-p file))
+                 (len (length path)))
+            (when (and len (> len max))
+              (setq max len bk backend)))))
+      (when bk (throw 'found bk)))
     ;; no responsible backend
     (let* ((possible-backends
 	    (let (pos)
-- 
2.33.0


[-- Attachment #3: Type: text/plain, Size: 7259 bytes --]



In GNU Emacs 28.0.50 (build 1, x86_64-unknown-openbsd7.0, X toolkit, cairo version 1.16.0, Xaw3d scroll bars)
 of 2021-09-13 built on elite.giraud
Repository revision: 5ee05fa75d517fdd13c9795aa78e12489140e388
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12013000
System Description: OpenBSD elite.giraud 7.0 GENERIC.MP#211 amd64

Configured using:
 'configure --prefix=/home/manuel/emacs --bindir=/home/manuel/bin
 --with-x-toolkit=athena --without-sound --without-json
 CPPFLAGS=-I/usr/local/include LDFLAGS=-L/usr/local/lib'

Configured features:
CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG LCMS2
LIBOTF LIBXML2 M17N_FLT MODULES NOTIFY KQUEUE PDUMPER PNG RSVG THREADS
TIFF TOOLKIT_SCROLL_BARS X11 XAW3D XDBE XIM XPM LUCID ZLIB

Important settings:
  value of $LC_ALL: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Outline

Minor modes in effect:
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  auto-revert-mode: t
  bug-reference-mode: t
  show-paren-mode: t
  icomplete-mode: t
  display-time-mode: t
  shell-dirtrack-mode: t
  windmove-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  column-number-mode: t
  line-number-mode: t
  auto-fill-function: do-auto-fill
  indent-tabs-mode: t
  transient-mark-mode: t

Load-path shadows:
/home/manuel/.el/google-maps.el/google-maps hides /home/manuel/.el/google-maps
/home/manuel/.emacs.d/elpa/magit-20210909.434/magit-section-pkg hides /home/manuel/.emacs.d/elpa/magit-section-20210829.1849/magit-section-pkg
/home/manuel/.emacs.d/elpa/transient-20210819.2118/transient hides /home/manuel/emacs/share/emacs/28.0.50/lisp/transient

Features:
(shadow sort bbdb-message mail-extr gnus-msg emacsbug magit-extras
face-remap magit-submodule magit-obsolete magit-blame magit-stash
magit-reflog magit-bisect magit-push magit-pull magit-fetch magit-clone
magit-remote magit-commit magit-sequence magit-notes magit-worktree
magit-tag magit-merge magit-branch magit-reset magit-files magit-refs
magit-status magit magit-repos magit-apply magit-wip magit-log
which-func imenu magit-diff smerge-mode diff git-commit rx log-edit
pcvs-util add-log magit-core magit-autorevert autorevert filenotify
magit-margin magit-transient magit-process magit-mode transient
magit-git magit-section misearch multi-isearch bug-reference cl-print
debug backtrace dabbrev pulse vc-annotate vc-filewise eieio-opt speedbar
ezimage dframe shortdoc help-fns radix-tree vc-mtn vc-hg vc-git
diff-mode vc-bzr vc-src vc-sccs vc-svn vc-cvs vc-rcs vc-dir ewoc vc
vc-dispatcher gnus-dired with-editor em-unix em-term term ehelp
em-script em-prompt em-ls em-hist em-pred em-glob em-cmpl em-dirs
esh-var em-basic em-banner em-alias esh-mode eshell esh-cmd esh-ext
esh-opt esh-proc esh-io esh-arg esh-module esh-groups esh-util
magit-utils dash cl-extra help-mode paredit paren icomplete time battery
cus-load exwm-randr xcb-randr exwm-config exwm exwm-input xcb-keysyms
exwm-manage exwm-floating xcb-cursor xcb-render exwm-layout
exwm-workspace exwm-core xcb-ewmh xcb-icccm xcb xcb-xkb xcb-xproto
xcb-types xcb-debug server modus-operandi-theme modus-themes pcase
google-maps google-maps-static google-maps-geocode google-maps-base
osm-mode url-cache gnuplot info-look transmission color calc-bin
calc-ext calc calc-loaddefs rect calc-macs w3m-load mu4e mu4e-org
mu4e-main mu4e-view mu4e-view-gnus gnus-art mm-uu mml2015 mm-view
mml-smime smime dig gnus-sum gnus-group gnus-undo gnus-start gnus-dbus
dbus gnus-cloud nnimap nnmail mail-source utf7 netrc nnoo gnus-spec
gnus-int gnus-range gnus-win gnus nnheader wid-edit mu4e-view-common
mu4e-headers mu4e-compose mu4e-context mu4e-draft mu4e-actions ido
rfc2368 smtpmail sendmail mu4e-mark mu4e-proc mu4e-utils doc-view
jka-compr image-mode exif mu4e-lists mu4e-message shr kinsoku svg xml
dom flow-fill mule-util hl-line mu4e-vars message rmc puny rfc822 mml
mml-sec epa derived epg rfc6068 epg-config gnus-util rmail
rmail-loaddefs mm-decode mm-bodies mm-encode mail-parse rfc2231 rfc2047
rfc2045 mm-util ietf-drums mail-prsvr mail-utils gmm-utils mailheader
mu4e-meta supercite regi bbdb-anniv diary-lib diary-loaddefs bbdb-mua
bbdb-com crm mailabbrev bbdb bbdb-site timezone org ob ob-tangle ob-ref
ob-lob ob-table ob-exp org-macro org-footnote org-src ob-comint
org-pcomplete org-list org-faces org-entities org-version ob-emacs-lisp
ob-core ob-eval org-table ol org-keys org-compat org-macs org-loaddefs
find-func cal-menu calendar cal-loaddefs visual-basic-mode web-mode
disp-table erlang-start smart-tabs-mode skeleton cc-mode cc-fonts
cc-guess cc-menus cc-cmds cc-styles cc-align cc-engine cc-vars cc-defs
movitz-slime cl slime-asdf grep slime-tramp tramp tramp-loaddefs
trampver tramp-integration files-x tramp-compat shell pcomplete
parse-time iso8601 time-date ls-lisp format-spec slime-fancy
slime-indentation slime-cl-indent cl-indent slime-trace-dialog
slime-fontifying-fu slime-package-fu slime-references
slime-compiler-notes-tree advice slime-scratch slime-presentations
bridge slime-macrostep macrostep slime-mdot-fu slime-enclosing-context
slime-fuzzy slime-fancy-trace slime-fancy-inspector slime-c-p-c
slime-editing-commands slime-autodoc slime-repl slime-parse slime
compile text-property-search etags fileloop generator xref project
arc-mode archive-mode noutline outline pp comint ansi-color ring
hyperspec thingatpt slime-autoloads dired-aux dired-x dired
dired-loaddefs edmacro kmacro windmove easy-mmode tex-site info package
browse-url url url-proxy url-privacy url-expand url-methods url-history
url-cookie url-domsuf url-util mailcap 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 iso-transl tooltip eldoc electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
term/common-win x-dnd 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 easymenu
timer select scroll-bar mouse jit-lock font-lock syntax 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 button
loaddefs faces cus-face macroexp files window text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote threads dbusbind kqueue lcms2
dynamic-setting system-font-setting font-render-setting cairo x-toolkit
x multi-tty make-network-process emacs)

Memory information:
((conses 16 612471 87337)
 (symbols 48 53625 7)
 (strings 32 170780 9764)
 (string-bytes 1 5553540)
 (vectors 16 88025)
 (vector-slots 8 1152335 56459)
 (floats 8 545 390)
 (intervals 56 4332 940)
 (buffers 992 33))

-- 
Manuel Giraud

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Date: Mon, 13 Sep 2021 17:47:19 +0200
> 
> When calling 'vc-next-action' on an unregistered file, VC could choose a
> backend that is very far above this file in the directory hierarchy.
> 
> In my case, it chooses SVN as backend (because I have one in my homedir)
> but I wanted it to choose the Git backend of the directory this file
> resides.
> 
> This patch solves this issue by selecting the backend with the most
> specific (in fact, the longest string) path to the file.

Isn't that already available with "C-u C-x v v"?

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Dmitry Gutov]

On 13.09.2021 19:10, Eli Zaretskii wrote:
>> From: Manuel Giraud<manuel@ledu-giraud.fr>
>> Date: Mon, 13 Sep 2021 17:47:19 +0200
>>
>> When calling 'vc-next-action' on an unregistered file, VC could choose a
>> backend that is very far above this file in the directory hierarchy.
>>
>> In my case, it chooses SVN as backend (because I have one in my homedir)
>> but I wanted it to choose the Git backend of the directory this file
>> resides.
>>
>> This patch solves this issue by selecting the backend with the most
>> specific (in fact, the longest string) path to the file.
> Isn't that already available with "C-u C-x v v"?

A better default is an improvement anyway.

I think the patch is a good idea, especially given that 
vc-responsible-backend has only recently been changed in the same way 
(commit 2697123933).

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Eli Zaretskii]

> Cc: 50572@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 14 Sep 2021 02:48:11 +0300
> 
> >> This patch solves this issue by selecting the backend with the most
> >> specific (in fact, the longest string) path to the file.
> > Isn't that already available with "C-u C-x v v"?
> 
> A better default is an improvement anyway.

But it looks like we've lost that feature of "C-x v v" somehow,
because it doesn't seem to work here.  Which is too bad.

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Date: Mon, 13 Sep 2021 17:47:19 +0200
>> 
>> When calling 'vc-next-action' on an unregistered file, VC could choose a
>> backend that is very far above this file in the directory hierarchy.
>> 
>> In my case, it chooses SVN as backend (because I have one in my homedir)
>> but I wanted it to choose the Git backend of the directory this file
>> resides.
>> 
>> This patch solves this issue by selecting the backend with the most
>> specific (in fact, the longest string) path to the file.
>
> Isn't that already available with "C-u C-x v v"?

It does not work for me. It still tries to register in my homedir
subversion. FWIW, I think the most specific should be the default of
"C-x v v" and maybe I could try to get the least specific for "C-u C-x v
v"
-- 
Manuel Giraud

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Manuel Giraud]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 13.09.2021 19:10, Eli Zaretskii wrote:
>>> From: Manuel Giraud<manuel@ledu-giraud.fr>
>>> Date: Mon, 13 Sep 2021 17:47:19 +0200
>>>
>>> When calling 'vc-next-action' on an unregistered file, VC could choose a
>>> backend that is very far above this file in the directory hierarchy.
>>>
>>> In my case, it chooses SVN as backend (because I have one in my homedir)
>>> but I wanted it to choose the Git backend of the directory this file
>>> resides.
>>>
>>> This patch solves this issue by selecting the backend with the most
>>> specific (in fact, the longest string) path to the file.
>> Isn't that already available with "C-u C-x v v"?
>
> A better default is an improvement anyway.
>
> I think the patch is a good idea, especially given that
> vc-responsible-backend has only recently been changed in the same way
> (commit 2697123933).

I see: this is the same functionality. Should I package this selection
into a function then?
-- 
Manuel Giraud

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: 50572@debbugs.gnu.org
> Date: Tue, 14 Sep 2021 09:50:22 +0200
> 
> > Isn't that already available with "C-u C-x v v"?
> 
> It does not work for me. It still tries to register in my homedir
> subversion.

It didn't work for me, either.  Which is a pity, since the manual says
it should.  I guess we lost this at some point; patches to fix that
are welcome.

> FWIW, I think the most specific should be the default of
> "C-x v v"

I'm worried that we are arbitrarily changing the defaults, for the
sole reason that the new default plays better with someone's
workflows.  AFAIR, it used to be the case that the order of VCS names
in vc-handled-backends determined the way the default is sought out
and set.  This change (and I think the other one Dmitry mentioned in
his response?) change that in a way that doesn't leave any "fire
escape" to users who might be setting things up to have a meaningful
(for them) order in that list.

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: 50572@debbugs.gnu.org
>> Date: Tue, 14 Sep 2021 09:50:22 +0200
>> 
>> > Isn't that already available with "C-u C-x v v"?
>> 
>> It does not work for me. It still tries to register in my homedir
>> subversion.
>
> It didn't work for me, either.  Which is a pity, since the manual says
> it should.  I guess we lost this at some point; patches to fix that
> are welcome.

Ok. I could try to work on this issue then but maybe I should file
another bug report, no?

>> FWIW, I think the most specific should be the default of
>> "C-x v v"
>
> I'm worried that we are arbitrarily changing the defaults, for the
> sole reason that the new default plays better with someone's
> workflows.

I think you are right and as you said the "choose the correct backend"
behaviour is already documented in the manual (in "Advanced Control in
‘C-x v v’").

> AFAIR, it used to be the case that the order of VCS names in
> vc-handled-backends determined the way the default is sought out and
> set.  This change (and I think the other one Dmitry mentioned in his
> response?) change that in a way that doesn't leave any "fire escape"
> to users who might be setting things up to have a meaningful (for
> them) order in that list.

Yes, in "Customizing VC", the manual insists on the order importance of
the vc-handled-backends list. So maybe, just fixing "C-u C-x v v" and
remember this customization is enough here.
-- 
Manuel Giraud

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: 50572@debbugs.gnu.org
> Date: Tue, 14 Sep 2021 14:08:36 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> > Isn't that already available with "C-u C-x v v"?
> >> 
> >> It does not work for me. It still tries to register in my homedir
> >> subversion.
> >
> > It didn't work for me, either.  Which is a pity, since the manual says
> > it should.  I guess we lost this at some point; patches to fix that
> > are welcome.
> 
> Ok. I could try to work on this issue then but maybe I should file
> another bug report, no?

Yes, please.

> > I'm worried that we are arbitrarily changing the defaults, for the
> > sole reason that the new default plays better with someone's
> > workflows.
> 
> I think you are right and as you said the "choose the correct backend"
> behaviour is already documented in the manual (in "Advanced Control in
> ‘C-x v v’").
> 
> > AFAIR, it used to be the case that the order of VCS names in
> > vc-handled-backends determined the way the default is sought out and
> > set.  This change (and I think the other one Dmitry mentioned in his
> > response?) change that in a way that doesn't leave any "fire escape"
> > to users who might be setting things up to have a meaningful (for
> > them) order in that list.
> 
> Yes, in "Customizing VC", the manual insists on the order importance of
> the vc-handled-backends list. So maybe, just fixing "C-u C-x v v" and
> remember this customization is enough here.

Let's wait for Dmitry to chime in, as I expect he will have different
opinions.

Thanks

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: 50572@debbugs.gnu.org
>> Date: Tue, 14 Sep 2021 09:50:22 +0200
>> 
>> > Isn't that already available with "C-u C-x v v"?
>> 
>> It does not work for me. It still tries to register in my homedir
>> subversion.
>
> It didn't work for me, either.  Which is a pity, since the manual says
> it should.  I guess we lost this at some point; patches to fix that
> are welcome.

FTR, I don't if it was lost at some point because reading the code it
seems that prefix is taken into account if the file is already
registered into some backend… but for unregistered file it seems that
nothing is done with the prefix.
-- 
Manuel Giraud

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: 50572@debbugs.gnu.org
> Date: Tue, 14 Sep 2021 17:15:14 +0200
> 
> > It didn't work for me, either.  Which is a pity, since the manual says
> > it should.  I guess we lost this at some point; patches to fix that
> > are welcome.
> 
> FTR, I don't if it was lost at some point because reading the code it
> seems that prefix is taken into account if the file is already
> registered into some backend… but for unregistered file it seems that
> nothing is done with the prefix.

So maybe this never worked for registering, in which case I think it
would be good to add that.  Because the manual says this should work
for "C-x v v" in general, not just under some conditions.  And what's
the most natural occasion to determine the backend if not when you
register a file?

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

> So maybe this never worked for registering, in which case I think it
> would be good to add that.  Because the manual says this should work
> for "C-x v v" in general, not just under some conditions.  And what's
> the most natural occasion to determine the backend if not when you
> register a file?

Yes. I'll look into it and open a new bug report when I have
something.

In fact, it seems hard to me to find an example where choosing a backend
for an already registered file (in another backend) is a useful next
action.
-- 
Manuel Giraud

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Dmitry Gutov]

On 14.09.2021 19:03, Eli Zaretskii wrote:
> So maybe this never worked for registering, in which case I think it
> would be good to add that.  Because the manual says this should work
> for "C-x v v" in general, not just under some conditions.  And what's
> the most natural occasion to determine the backend if not when you
> register a file?

I don't know what the manual says, but vc-next-section's signature calls 
the prefix argument's value VERBOSE, and vc-register doesn't have a way 
to trigger backend-prompting either. So it might be difficult to add 
that feature in a backward-compatible manner.

vc-register doesn't work with C-u at all, though (shows an error), so it 
might be possible to do something in that area.

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Dmitry Gutov]

On 14.09.2021 14:38, Eli Zaretskii wrote:
>> FWIW, I think the most specific should be the default of
>> "C-x v v"
> I'm worried that we are arbitrarily changing the defaults, for the
> sole reason that the new default plays better with someone's
> workflows.  AFAIR, it used to be the case that the order of VCS names
> in vc-handled-backends determined the way the default is sought out
> and set.  This change (and I think the other one Dmitry mentioned in
> his response?) change that in a way that doesn't leave any "fire
> escape" to users who might be setting things up to have a meaningful
> (for them) order in that list.

It's really more of a discussion for bug#42966. Because no matter how 
one feels about that change (I don't have a strong opinion), these two 
pieces of code should follow the same pattern.

Now, the change in vc-responsible-backend might seem controversial, as 
it decreases customizability and performance (a bit), but it was made a 
year ago, and we've received no substantial complaints. And there have 
been multiple reports (all linked) about the previous behavior.

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Eli Zaretskii]

> Cc: 50572@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 15 Sep 2021 13:35:14 +0300
> 
> On 14.09.2021 19:03, Eli Zaretskii wrote:
> > So maybe this never worked for registering, in which case I think it
> > would be good to add that.  Because the manual says this should work
> > for "C-x v v" in general, not just under some conditions.  And what's
> > the most natural occasion to determine the backend if not when you
> > register a file?
> 
> I don't know what the manual says, but vc-next-section's signature calls 
> the prefix argument's value VERBOSE, and vc-register doesn't have a way 
> to trigger backend-prompting either. So it might be difficult to add 
> that feature in a backward-compatible manner.

We could use a different arg value, for example "C-u" will do one
thing and "C-u C-u" another.  There's precedence for that.

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Eli Zaretskii]

> Cc: 50572@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 15 Sep 2021 13:41:57 +0300
> 
> Now, the change in vc-responsible-backend might seem controversial, as 
> it decreases customizability and performance (a bit), but it was made a 
> year ago, and we've received no substantial complaints. And there have 
> been multiple reports (all linked) about the previous behavior.

That doesn't alleviate my worries, unfortunately.  Many people either
don't use VC or use it very "narrowly", so they never bump into these
rarely-used-but-very-useful features.

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Dmitry Gutov]

On 15.09.2021 14:20, Eli Zaretskii wrote:
>> I don't know what the manual says, but vc-next-section's signature calls
>> the prefix argument's value VERBOSE, and vc-register doesn't have a way
>> to trigger backend-prompting either. So it might be difficult to add
>> that feature in a backward-compatible manner.
> We could use a different arg value, for example "C-u" will do one
> thing and "C-u C-u" another.  There's precedence for that.

That's an option, yes.

We can also look into whether "verbose" applies to the "register" step 
of vc-next-action, and if not, or it doesn't conflict, add 
"switch-backend" feature to it.

If, like you said, the manual says 'C-u' will do this for all commands, 
it wouldn't be great to have to use different prefixes for different 
commands. But an escape hatch is better than no escape hatch, of course.

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Dmitry Gutov]

On 15.09.2021 14:22, Eli Zaretskii wrote:
>> Cc:50572@debbugs.gnu.org
>> From: Dmitry Gutov<dgutov@yandex.ru>
>> Date: Wed, 15 Sep 2021 13:41:57 +0300
>>
>> Now, the change in vc-responsible-backend might seem controversial, as
>> it decreases customizability and performance (a bit), but it was made a
>> year ago, and we've received no substantial complaints. And there have
>> been multiple reports (all linked) about the previous behavior.
> That doesn't alleviate my worries, unfortunately.  Many people either
> don't use VC or use it very "narrowly", so they never bump into these
> rarely-used-but-very-useful features.

I don't think we have a better way of making that choice now than 
waiting for more reports.

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Eli Zaretskii]

> Cc: manuel@ledu-giraud.fr, 50572@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 15 Sep 2021 14:37:14 +0300
> 
> On 15.09.2021 14:22, Eli Zaretskii wrote:
> >> Cc:50572@debbugs.gnu.org
> >> From: Dmitry Gutov<dgutov@yandex.ru>
> >> Date: Wed, 15 Sep 2021 13:41:57 +0300
> >>
> >> Now, the change in vc-responsible-backend might seem controversial, as
> >> it decreases customizability and performance (a bit), but it was made a
> >> year ago, and we've received no substantial complaints. And there have
> >> been multiple reports (all linked) about the previous behavior.
> > That doesn't alleviate my worries, unfortunately.  Many people either
> > don't use VC or use it very "narrowly", so they never bump into these
> > rarely-used-but-very-useful features.
> 
> I don't think we have a better way of making that choice now than 
> waiting for more reports.

<Shrug> Well, you have mine.

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Dmitry Gutov]

On 15.09.2021 14:44, Eli Zaretskii wrote:
>> I don't think we have a better way of making that choice now than
>> waiting for more reports.
> <Shrug> Well, you have mine.

Reports with some specific broken scenarios.

So far I don't see anything actionable in yours. Maybe Lars does?

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> So far I don't see anything actionable in yours. Maybe Lars does?

Nope.

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

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Lars Ingebrigtsen]

Manuel Giraud <manuel@ledu-giraud.fr> writes:

> This patch solves this issue by selecting the backend with the most
> specific (in fact, the longest string) path to the file.

I think this makes sense (it makes things more consistent and logical),
and Dmitry agrees, so I've now applied this to Emacs 29.  We'll see
whether anybody complains.

The discussion then went on to the question of what VERBOSE should do.
It's not a parameter that's mentioned in the doc string, and reading the
code, it's pretty unclear.  So perhaps a new bug report should be opened
for that, but I'm closing this one.

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

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Andy Moreton]

On Sat 06 Nov 2021, Lars Ingebrigtsen wrote:

> Manuel Giraud <manuel@ledu-giraud.fr> writes:
>
>> This patch solves this issue by selecting the backend with the most
>> specific (in fact, the longest string) path to the file.
>
> I think this makes sense (it makes things more consistent and logical),
> and Dmitry agrees, so I've now applied this to Emacs 29.  We'll see
> whether anybody complains.

Picking the more specific filename is good, but the longest string is
the wrong measure: what is wanted is the deepest path. For example:

  verylongdirname/foo
  a/b/c/foo

The second is a shorter string, but more specific.

    AndyM

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Lars Ingebrigtsen]

Andy Moreton <andrewjmoreton@gmail.com> writes:

> Picking the more specific filename is good, but the longest string is
> the wrong measure: what is wanted is the deepest path. For example:
>
>   verylongdirname/foo
>   a/b/c/foo
>
> The second is a shorter string, but more specific.

But didn't it just traverse upwards?  In which case longer is more
specific.  (I may be misremembering the patch.)

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

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Andy Moreton]

On Sat 06 Nov 2021, Lars Ingebrigtsen wrote:

> Andy Moreton <andrewjmoreton@gmail.com> writes:
>
>> Picking the more specific filename is good, but the longest string is
>> the wrong measure: what is wanted is the deepest path. For example:
>>
>>   verylongdirname/foo
>>   a/b/c/foo
>>
>> The second is a shorter string, but more specific.
>
> But didn't it just traverse upwards?  In which case longer is more
> specific.  (I may be misremembering the patch.)

Longer is better yes, but the patch counts characters when it should
count directories to determine which path is longer.

    AndyM

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Dmitry Gutov]

On 07.11.2021 01:30, Andy Moreton wrote:
> Longer is better yes, but the patch counts characters when it should
> count directories to determine which path is longer.

Err, yes. That's a problem.

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

>> Longer is better yes, but the patch counts characters when it should
>> count directories to determine which path is longer.
>
> Err, yes. That's a problem.

Manuel, can you rework vc-backend-for-registration to count directory
depth instead of characters?

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

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Manuel Giraud]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Dmitry Gutov <dgutov@yandex.ru> writes:
>
>>> Longer is better yes, but the patch counts characters when it should
>>> count directories to determine which path is longer.
>>
>> Err, yes. That's a problem.
>
> Manuel, can you rework vc-backend-for-registration to count directory
> depth instead of characters?

Sure. But from the discussion with Eli on this bug report, I thought
that it was decided to left `C-x v v' untouched and use `C-u C-x v v'
when needed. A user can also set its prefered backends by ordering
vc-handled-backends as he wanted to.

Anyway, I'll tried to rework that but do you happen to know from the top
of your head a function to count the "directory deepness" of a file?
-- 
Manuel Giraud

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Lars Ingebrigtsen]

Manuel Giraud <manuel@ledu-giraud.fr> writes:

> Sure. But from the discussion with Eli on this bug report, I thought
> that it was decided to left `C-x v v' untouched and use `C-u C-x v v'
> when needed. A user can also set its prefered backends by ordering
> vc-handled-backends as he wanted to.

The commands should be consistent, and that wasn't the case with these
vc commands before the change.

> Anyway, I'll tried to rework that but do you happen to know from the top
> of your head a function to count the "directory deepness" of a file?

Just count slashes?

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

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Manuel Giraud]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Manuel Giraud <manuel@ledu-giraud.fr> writes:
>
>> Sure. But from the discussion with Eli on this bug report, I thought
>> that it was decided to left `C-x v v' untouched and use `C-u C-x v v'
>> when needed. A user can also set its prefered backends by ordering
>> vc-handled-backends as he wanted to.
>
> The commands should be consistent, and that wasn't the case with these
> vc commands before the change.

Ok then.

>> Anyway, I'll tried to rework that but do you happen to know from the top
>> of your head a function to count the "directory deepness" of a file?
>
> Just count slashes?

I thought that we might already have a function for this. Does counting
slashes will work with Windows path?

Side question: as this bug report is closed, should I open a new one
with the new patch?
-- 
Manuel Giraud

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Lars Ingebrigtsen]

Manuel Giraud <manuel@ledu-giraud.fr> writes:

> I thought that we might already have a function for this. Does counting
> slashes will work with Windows path?

Hm...  Actually, I'm not sure.  Perhaps doing a series of
file-name-directory/directory-file-name in a loop (to peel off the
directory components) would be the best algorithm here?  And I don't
think we have a function for this -- at least I can't find anything
promising while poking around.

> Side question: as this bug report is closed, should I open a new one
> with the new patch?

Opening a new bug report is best, but it's not required.

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

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Juri Linkov]

>> I thought that we might already have a function for this. Does counting
>> slashes will work with Windows path?
>
> Hm...  Actually, I'm not sure.  Perhaps doing a series of
> file-name-directory/directory-file-name in a loop (to peel off the
> directory components) would be the best algorithm here?  And I don't
> think we have a function for this -- at least I can't find anything
> promising while poking around.

ffap-list-env contains such comment:

  ;; Similar: dired-split, TeX-split-string, and RHOGEE's psg-list-env
  ;; in ff-paths and bib-cite.  The EMPTY arg may help mimic kpathsea.

And indeed dired-split is used in dired-aux.el like this:

  (dired-split "/" dir1)

I don't know if this works on Windows.

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Manuel Giraud]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Manuel Giraud <manuel@ledu-giraud.fr> writes:
>
>> I thought that we might already have a function for this. Does counting
>> slashes will work with Windows path?
>
> Hm...  Actually, I'm not sure.  Perhaps doing a series of
> file-name-directory/directory-file-name in a loop (to peel off the
> directory components) would be the best algorithm here?  And I don't
> think we have a function for this -- at least I can't find anything
> promising while poking around.

That was what I found also inspired from dired-up-directory. So here is
what I've done. What bothers me is that it introduce two general purpose
(and probably buggy) functions that should reside somewhere else IMO.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Use-file-depth-for-most-specific-path-in-vc-backend-.patch --]
[-- Type: text/x-patch, Size: 1636 bytes --]

From 326c4c0f2f087a1be75657425a362aa05fc47557 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Tue, 9 Nov 2021 09:44:26 +0100
Subject: [PATCH] Use file depth for most specific path in
 vc-backend-for-registration

---
 lisp/vc/vc.el | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index c9500f454a..70c9434a20 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -927,6 +927,15 @@ with-vc-properties
                   property (cdr setting))))))))
 
 ;;; Code for deducing what fileset and backend to assume
+(defun parent-directory (filename)
+  (file-name-directory (directory-file-name filename)))
+
+(defun file-depth (filename)
+  (cl-loop for oparent = nil then parent
+	   for parent = (parent-directory (expand-file-name filename)) then (parent-directory parent)
+	   for i from 0
+	   until (string= parent oparent)
+	   finally (return i)))
 
 (defun vc-backend-for-registration (file)
   "Return a backend that can be used for registering FILE.
@@ -944,9 +953,9 @@ vc-backend-for-registration
       (dolist (backend vc-handled-backends)
         (when (not (vc-call-backend backend 'registered file))
           (let* ((path (vc-call-backend backend 'responsible-p file))
-                 (len (length path)))
-            (when (and len (> len max))
-              (setq max len bk backend)))))
+                 (depth (and path (file-depth path))))
+            (when (and depth (> depth max))
+              (setq max depth bk backend)))))
       (when bk
         (throw 'found bk)))
     ;; no responsible backend
-- 
2.33.1


[-- Attachment #3: Type: text/plain, Size: 18 bytes --]

-- 
Manuel Giraud

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> And indeed dired-split is used in dired-aux.el like this:
>
>   (dired-split "/" dir1)
>
> I don't know if this works on Windows.

If I'm reading the code of dired-split correctly, it's just an
implementation of split-string?  Hm... yup.  I think I'll just adjust
the one caller (in dired-aux) to use split-string instead and mark
dired-split as obsolete.

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

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Lars Ingebrigtsen]

Manuel Giraud <manuel@ledu-giraud.fr> writes:

> That was what I found also inspired from dired-up-directory. So here is
> what I've done. What bothers me is that it introduce two general purpose
> (and probably buggy) functions that should reside somewhere else IMO.

😀

I think a new, general function would be useful here (it's something
I've sloppily implemented a couple of times before for private usage),
so I've now made a better version and called it 'file-name-split', which
should hopefully work on all systems.  (Actually, I'm not 100% sure
whether there are any systems that use \ for file names internally these
days?)

It's pushed to the trunk now -- can you adjust the patch to use that
instead?

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

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Dmitry Gutov]

On 10.11.2021 02:30, Lars Ingebrigtsen wrote:
> I've sloppily implemented a couple of times before for private usage),
> so I've now made a better version and called it 'file-name-split', which
> should hopefully work on all systems.

FWIW, it's not very fast with Tramp. Even just visiting a local file 
over the 'sudo' protocol,

   (benchmark 1 '(file-name-split buffer-file-name))

prints '0.013030s' here. With some random deviations based on GC.

Probably not terrible for this particular command, but a general "split 
file name" function could do better. I think.

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Probably not terrible for this particular command, but a general
> "split file name" function could do better. I think.

What is the situation with path separators on Windows, anyway?  If it
uses "/", too, then this function isn't necessary -- split-string will
do the job just as well.

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

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Dmitry Gutov]

On 10.11.2021 03:20, Lars Ingebrigtsen wrote:
> What is the situation with path separators on Windows, anyway?  If it
> uses "/", too, then this function isn't necessary -- split-string will
> do the job just as well.

IIRC, it allows backslashes (with such file names printed as 
"abc\\def"), but canonical file names use slashes even on Windows.

So... IDK, call some function which "canonicalizes" the file name 
(expand-file-name?) and then use split-string?

This works much faster in the same 'sudo' session:

   (split-string (expand-file-name buffer-file-name) "/")

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> IIRC, it allows backslashes (with such file names printed as
> "abc\\def"), but canonical file names use slashes even on Windows.
>
> So... IDK, call some function which "canonicalizes" the file name
> (expand-file-name?) and then use split-string?

Yeah, that sounds like a better implementation.

> This works much faster in the same 'sudo' session:
>
>   (split-string (expand-file-name buffer-file-name) "/")

expand-file-name wouldn't be the right thing here, though.  Do we have a
function somewhere that just does the slash translation and nothing
else?

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

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Wed, 10 Nov 2021 01:39:36 +0100
> Cc: 50572@debbugs.gnu.org, Andy Moreton <andrewjmoreton@gmail.com>,
>  Manuel Giraud <manuel@ledu-giraud.fr>
> 
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
> > This works much faster in the same 'sudo' session:
> >
> >   (split-string (expand-file-name buffer-file-name) "/")
> 
> expand-file-name wouldn't be the right thing here, though.

Why isn't expand-file-name TRT?  If the file names are not absolute,
you cannot reliably compare their "depth" anyway.

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Why isn't expand-file-name TRT?  If the file names are not absolute,
> you cannot reliably compare their "depth" anyway.

For this particular thing, you probably need absolute paths.  But a
general file-name-splitting functions should have that limitation.

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

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: dgutov@yandex.ru,  50572@debbugs.gnu.org,  andrewjmoreton@gmail.com,
>   manuel@ledu-giraud.fr
> Date: Wed, 10 Nov 2021 13:59:24 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Why isn't expand-file-name TRT?  If the file names are not absolute,
> > you cannot reliably compare their "depth" anyway.
> 
> For this particular thing, you probably need absolute paths.  But a
> general file-name-splitting functions should have that limitation.

We can always use "/\\" as SEPARATORS on MS-Windows, can't we?

Alternatively, we could loop with file-name-directory, I guess.

But if we want a function that returns the "depth" of a file in a
filesystem, then it will need to call expand-file-name internally
anyway, for the same reason I point out above, right?

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> We can always use "/\\" as SEPARATORS on MS-Windows, can't we?

I was wondering about that -- are both of those characters disallowed in
file name components on Windows?  If so, then yes.  What would be the
test to identify the systems where this is the case?

> Alternatively, we could loop with file-name-directory, I guess.

That's what the current implementation does, and it's slow (since it
consults the file name handlers).

> But if we want a function that returns the "depth" of a file in a
> filesystem, then it will need to call expand-file-name internally
> anyway, for the same reason I point out above, right?

Yes.  This function can be used as a component in that calculation, but
if that's the purpose of the call, then the caller has to make the file
name non-relative first.

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

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: dgutov@yandex.ru,  50572@debbugs.gnu.org,  andrewjmoreton@gmail.com,
>   manuel@ledu-giraud.fr
> Date: Thu, 11 Nov 2021 03:56:29 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > We can always use "/\\" as SEPARATORS on MS-Windows, can't we?
> 
> I was wondering about that -- are both of those characters disallowed in
> file name components on Windows?

Yes.  Windows APIs recognize '/' as (another) directory separator, and
so using it in file names is not allowed.

> If so, then yes.  What would be the test to identify the systems
> where this is the case?

 (memq system-type '(windows-nt ms-dos))

> > But if we want a function that returns the "depth" of a file in a
> > filesystem, then it will need to call expand-file-name internally
> > anyway, for the same reason I point out above, right?
> 
> Yes.  This function can be used as a component in that calculation, but
> if that's the purpose of the call, then the caller has to make the file
> name non-relative first.

If we will require the caller to provide an absolute file name, then
why not call expand-file-name internally?  We are not saving anyone
from any potentially expensive operation, because ensuring a file name
is an absolute one without calling expand-file-name is notoriously
hard and unreliable in Emacs.  So in practice, if the requirement is
to provide an absolute file name, the callers will always call
expand-file-name anyway.

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> If so, then yes.  What would be the test to identify the systems
>> where this is the case?
>
>  (memq system-type '(windows-nt ms-dos))

This doesn't recognize remote file names.

Best regards, Michael.

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,  50572@debbugs.gnu.org,
>   andrewjmoreton@gmail.com,  manuel@ledu-giraud.fr,  dgutov@yandex.ru
> Date: Thu, 11 Nov 2021 08:46:50 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> If so, then yes.  What would be the test to identify the systems
> >> where this is the case?
> >
> >  (memq system-type '(windows-nt ms-dos))
> 
> This doesn't recognize remote file names.

The issue was about backslashes being directory separators, I believe?
If so, what does that have to do with remote file names?

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> >> If so, then yes.  What would be the test to identify the systems
>> >> where this is the case?
>> >
>> >  (memq system-type '(windows-nt ms-dos))
>>
>> This doesn't recognize remote file names.
>
> The issue was about backslashes being directory separators, I believe?
> If so, what does that have to do with remote file names?

Remote file names do not use "\\" as directory separator, that's all
what I want to say. I don't know whether this is relevant for the
intended bug fix; I didn't follow the discussion completely.

Best regards, Michael.

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> If we will require the caller to provide an absolute file name, then
> why not call expand-file-name internally?

We don't require that.  It's a perfectly valid thing to wonder (for
instance) "what's the last two elements in this directory structure I
have here", and the answer is:

(file-name-split "foo/bar") => ("foo" "bar")

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

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: dgutov@yandex.ru,  50572@debbugs.gnu.org,  andrewjmoreton@gmail.com,
>   manuel@ledu-giraud.fr
> Date: Thu, 11 Nov 2021 13:21:22 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > If we will require the caller to provide an absolute file name, then
> > why not call expand-file-name internally?
> 
> We don't require that.  It's a perfectly valid thing to wonder (for
> instance) "what's the last two elements in this directory structure I
> have here", and the answer is:
> 
> (file-name-split "foo/bar") => ("foo" "bar")

We were mis-communicating: I was talking about a function that
determines which file is "deeper" in the filesystem.

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> (file-name-split "foo/bar") => ("foo" "bar")
>
> We were mis-communicating: I was talking about a function that
> determines which file is "deeper" in the filesystem.

And that function will have to expand the file first.  Functions that
don't want that do not have to expand first.  

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

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Manuel Giraud]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

[...]

> It's pushed to the trunk now -- can you adjust the patch to use that
> instead?

There you go.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Find-most-specific-backend-for-vc-backend-for-regist.patch --]
[-- Type: text/x-patch, Size: 875 bytes --]

From 2e4e651b722673439be2654c7aae4202e8e73cd4 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Fri, 12 Nov 2021 10:46:56 +0100
Subject: [PATCH] Find most specific backend for `vc-backend-for-registration'.

---
 lisp/vc/vc.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 4b56f1b795..b8f0425393 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -945,7 +945,7 @@ vc-backend-for-registration
       (dolist (backend vc-handled-backends)
         (when (not (vc-call-backend backend 'registered file))
           (let* ((path (vc-call-backend backend 'responsible-p file))
-                 (len (length path)))
+                 (len (and path (length (file-name-split path)))))
             (when (and len (> len max))
               (setq max len bk backend)))))
       (when bk
-- 
2.33.1

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Date: Fri, 12 Nov 2021 10:51:36 +0100
> Cc: 50572@debbugs.gnu.org, Andy Moreton <andrewjmoreton@gmail.com>,
>  Dmitry Gutov <dgutov@yandex.ru>
> 
> > It's pushed to the trunk now -- can you adjust the patch to use that
> > instead?
> 
> There you go.
> 
> >From 2e4e651b722673439be2654c7aae4202e8e73cd4 Mon Sep 17 00:00:00 2001
> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Date: Fri, 12 Nov 2021 10:46:56 +0100
> Subject: [PATCH] Find most specific backend for `vc-backend-for-registration'.
> 
> ---
>  lisp/vc/vc.el | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
> index 4b56f1b795..b8f0425393 100644
> --- a/lisp/vc/vc.el
> +++ b/lisp/vc/vc.el
> @@ -945,7 +945,7 @@ vc-backend-for-registration
>        (dolist (backend vc-handled-backends)
>          (when (not (vc-call-backend backend 'registered file))
>            (let* ((path (vc-call-backend backend 'responsible-p file))
> -                 (len (length path)))
> +                 (len (and path (length (file-name-split path)))))
>              (when (and len (> len max))
>                (setq max len bk backend)))))
>        (when bk

Please don't use "path" for anything that is not PATH-style directory
lists.

Also, shouldn't the file name(s) be run through expand-file-name, to
make sure they are absolute?  Otherwise comparing the length will not
DTRT.

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

>> diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
>> index 4b56f1b795..b8f0425393 100644
>> --- a/lisp/vc/vc.el
>> +++ b/lisp/vc/vc.el
>> @@ -945,7 +945,7 @@ vc-backend-for-registration
>>        (dolist (backend vc-handled-backends)
>>          (when (not (vc-call-backend backend 'registered file))
>>            (let* ((path (vc-call-backend backend 'responsible-p file))
>> -                 (len (length path)))
>> +                 (len (and path (length (file-name-split path)))))
>>              (when (and len (> len max))
>>                (setq max len bk backend)))))
>>        (when bk
>
> Please don't use "path" for anything that is not PATH-style directory
> lists.

Ok but I have hard time figuring out what would be a correct name here
because (vc-call-backend backend 'responsible-p file) returns a path (or
nil) if the backend is responsible. Does "path-string" will do?

> Also, shouldn't the file name(s) be run through expand-file-name, to
> make sure they are absolute?  Otherwise comparing the length will not
> DTRT.

Ok, I'll do this but I'll also need a new name here (in case of a nil
returned by vc-call-backend): "expanded-path-string" ?
-- 
Manuel Giraud

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: 50572@debbugs.gnu.org,  larsi@gnus.org,  andrewjmoreton@gmail.com,
>   dgutov@yandex.ru
> Date: Fri, 12 Nov 2021 16:09:57 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Please don't use "path" for anything that is not PATH-style directory
> > lists.
> 
> Ok but I have hard time figuring out what would be a correct name here
> because (vc-call-backend backend 'responsible-p file) returns a path (or
> nil) if the backend is responsible. Does "path-string" will do?

Either "file-name" or "dir-name", or something along those lines,
should do, I think.  We are talking about variable names, yes?

> > Also, shouldn't the file name(s) be run through expand-file-name, to
> > make sure they are absolute?  Otherwise comparing the length will not
> > DTRT.
> 
> Ok, I'll do this but I'll also need a new name here (in case of a nil
> returned by vc-call-backend): "expanded-path-string" ?

Maybe I'm missing something, but why not test the value for being a
string, and only call expand-file-name if it is?

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

> Either "file-name" or "dir-name", or something along those lines,
> should do, I think.  We are talking about variable names, yes?

Yes, of the two hard problems of computer science :-)

[...]

> Maybe I'm missing something, but why not test the value for being a
> string, and only call expand-file-name if it is?

👍
-- 
Manuel Giraud

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Manuel Giraud]

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

Here is a new version.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Find-most-specific-backend-for-vc-backend-for-regist.patch --]
[-- Type: text/x-patch, Size: 1025 bytes --]

From 76e164e96552bbd8f1821dec03f0e0d35af457d7 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Fri, 12 Nov 2021 17:13:51 +0100
Subject: [PATCH] Find most specific backend for `vc-backend-for-registration'.

---
 lisp/vc/vc.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 4b56f1b795..6f5839736a 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -944,8 +944,9 @@ vc-backend-for-registration
           bk)
       (dolist (backend vc-handled-backends)
         (when (not (vc-call-backend backend 'registered file))
-          (let* ((path (vc-call-backend backend 'responsible-p file))
-                 (len (length path)))
+          (let* ((dir-name (vc-call-backend backend 'responsible-p file))
+                 (len (and dir-name
+                           (length (file-name-split (expand-file-name dir-name))))))
             (when (and len (> len max))
               (setq max len bk backend)))))
       (when bk
-- 
2.33.1


[-- Attachment #3: Type: text/plain, Size: 18 bytes --]

-- 
Manuel Giraud

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: 50572@debbugs.gnu.org,  larsi@gnus.org,  andrewjmoreton@gmail.com,
>   dgutov@yandex.ru
> Date: Fri, 12 Nov 2021 20:21:33 +0100
> 
> Here is a new version.

Thanks, I have no more comments.

bug#50572: 28.0.50; [PATCH] fix VC to find the responsible backend with the most specific path.

[Lars Ingebrigtsen]

Manuel Giraud <manuel@ledu-giraud.fr> writes:

> Here is a new version.

Thanks; applied to Emacs 29.

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