bug#62693: 28.2; VC: CVS: Fix lost file reporting and enable reverting it

bug#62693: 28.2; VC: CVS: Fix lost file reporting and enable reverting it

[Olivier Certner]

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

Hi,

When using CVS 1.12.3+, VC dir doesn't list correctly some missing file, including quotes produced by CVS in the file name, then preventing to operate on it. The fix works on all "recent" versions of CVS (1.11 and 1.12).

Once this is fixed, it's still not possible to revert such a file yet since `vc-default-revert', called by `vc-cvs-revert', tries to backup the non-existing file (`copy-file' just fails).

While here, fixed `vc-cvs-parse-root', which is not correctly reporting whether some repository is local.

All three patches can be applied to 'master' directly (e579c9cc33d).

Regards.

-- 
Olivier Certner

[-- Attachment #2: 0001-VC-CVS-Fix-ROOT-file-parsing.patch --]
[-- Type: text/x-patch, Size: 1200 bytes --]

From 8302329ab590abe4a8b4f05d32894be93410e10c Mon Sep 17 00:00:00 2001
From: Olivier Certner <olce.emacs@certner.fr>
Date: Thu, 6 Apr 2023 10:16:33 +0200
Subject: [PATCH 1/3] VC: CVS: Fix ROOT file parsing

* lisp/vc/vc-cvs.el (vc-cvs-parse-root): Don't artificially prepend
'x:' in an attempt to preserve ":" at start, and not even compensating
for it later on.  Function `split-string' with an explicit SEPARATORS
doesn't default OMIT-NULLS to t.
---
 lisp/vc/vc-cvs.el | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lisp/vc/vc-cvs.el b/lisp/vc/vc-cvs.el
index 6e0246ea762..695442b98d4 100644
--- a/lisp/vc/vc-cvs.el
+++ b/lisp/vc/vc-cvs.el
@@ -846,9 +846,7 @@ vc-cvs-parse-root
 is `ext'.
 For an empty string, nil is returned (invalid CVS root)."
   ;; Split CVS root into colon separated fields (0-4).
-  ;; The `x:' makes sure, that leading colons are not lost;
-  ;; `HOST:/PATH' is then different from `:METHOD:/PATH'.
-  (let* ((root-list (cdr (split-string (concat "x:" root) ":")))
+  (let* ((root-list (cdr (split-string root ":")))
          (len (length root-list))
          ;; All syntactic varieties will get a proper METHOD.
          (root-list
-- 
2.39.2


[-- Attachment #3: 0002-VC-CVS-Fix-parsing-of-cvs-qn-update-for-missing-file.patch --]
[-- Type: text/x-patch, Size: 1735 bytes --]

From 64fa9e81277663581e88e6093a8833c4038b170f Mon Sep 17 00:00:00 2001
From: Olivier Certner <olce.emacs@certner.fr>
Date: Thu, 6 Apr 2023 10:52:23 +0200
Subject: [PATCH 2/3] VC: CVS: Fix parsing of 'cvs -qn update' for missing
 files for 1.12

* lisp/vc/vc-cvs.el (vc-cvs-after-dir-status): Fix the name reported
for missing files in the case of CVS 1.12.3+ where name is quoted in
the warning line (it was not before this version).  Use instead the
following U line, where the name is never quoted on all versions.
---
 lisp/vc/vc-cvs.el | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/lisp/vc/vc-cvs.el b/lisp/vc/vc-cvs.el
index 695442b98d4..039015a50b7 100644
--- a/lisp/vc/vc-cvs.el
+++ b/lisp/vc/vc-cvs.el
@@ -953,13 +953,16 @@ vc-cvs-after-dir-status
                       (cdr (assoc (char-after) translation)))
                 result)
         (cond
-         ((looking-at "cvs update: warning: \\(.*\\) was lost")
+         ((looking-at "cvs update: warning: .* was lost")
           ;; Format is:
           ;; cvs update: warning: FILENAME was lost
           ;; U FILENAME
-          (push (list (match-string 1) 'missing) result)
-          ;; Skip the "U" line
-          (forward-line 1))
+          ;; with FILENAME in the first line possibly enclosed in
+          ;; quotes (since CVS 1.12.3). To avoid problems, use the U
+          ;; line where name is never quoted.
+          (forward-line 1)
+          (when (looking-at "^U \\(.*\\)$")
+            (push (list (match-string 1) 'missing) result)))
          ((looking-at "cvs update: New directory `\\(.*\\)' -- ignored")
           (push (list (match-string 1) 'unregistered) result))))
       (forward-line 1))
-- 
2.39.2


[-- Attachment #4: 0003-VC-Allow-vc-default-revert-and-CVS-to-revert-a-missi.patch --]
[-- Type: text/x-patch, Size: 1149 bytes --]

From 7840ef7059a160be850aa58086eed992881ea30d Mon Sep 17 00:00:00 2001
From: Olivier Certner <olce.emacs@certner.fr>
Date: Thu, 6 Apr 2023 11:25:33 +0200
Subject: [PATCH 3/3] VC: Allow `vc-default-revert' (and CVS) to revert a
 missing file

* lisp/vc/vc.el (vc-default-revert): Fix reverting a missing file case
by not trying to create a backup through `copy-file'.  Notably impacts
CVS, where `vc-cvs-revert' calls `vc-default-revert'.
---
 lisp/vc/vc.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 90905edb887..ff42a9e6394 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -3604,7 +3604,8 @@ vc-default-revert
           (file-buffer (or (get-file-buffer file) (current-buffer))))
       (message "Checking out %s..." file)
       (let ((failed t)
-            (backup-name (car (find-backup-file-name file))))
+            (backup-name (when (file-exists-p file)
+                           (car (find-backup-file-name file)))))
         (when backup-name
           (copy-file file backup-name 'ok-if-already-exists 'keep-date)
           (unless (file-writable-p file)
-- 
2.39.2

bug#62693: 28.2; VC: CVS: Fix lost file reporting and enable reverting it

[Dmitry Gutov]

Hi! Thanks for the report.

On 06/04/2023 12:51, Olivier Certner wrote:

> When using CVS 1.12.3+, VC dir doesn't list correctly some missing file, including quotes produced by CVS in the file name, then preventing to operate on it. The fix works on all "recent" versions of CVS (1.11 and 1.12).
> 
> Once this is fixed, it's still not possible to revert such a file yet since `vc-default-revert', called by `vc-cvs-revert', tries to backup the non-existing file (`copy-file' just fails).
> 
> While here, fixed `vc-cvs-parse-root', which is not correctly reporting whether some repository is local.
> 
> All three patches can be applied to 'master' directly (e579c9cc33d).

Regarding patches 1 and 3:

- Could you explain what kind of files are not listed and when? I'm 
guessing this has to do with the file name? I've tried to reproduce the 
problem with a file name that contained a space, and couldn't see it.

- Could you give an example of the repository CVS root which is 
recognized by the current code incorrectly?

bug#62693: 28.2; VC: CVS: Fix lost file reporting and enable reverting it

[Olivier Certner]

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

Hi,

Thanks for looking at this.

> - Could you explain what kind of files are not listed and when? I'm 
> guessing this has to do with the file name? I've tried to reproduce the 
> problem with a file name that contained a space, and couldn't see it.

It's not that files are not listed, they are so always. As said, the problem 
is their name. For example, if I remove an INSTALL file in a repository, 
`INSTALL' is the name listed in a VC dir buffer, *quotes included*. Then, no 
VC operation can work because there is indeed no file by this name (with 
quotes, both in the filesystem or known by CVS).

> - Could you give an example of the repository CVS root which is 
> recognized by the current code incorrectly?

Just run (vc-cvs-parse-root "/your/local/path/to/some/CVS/repo") and you'll 
see...

I've found more occurences of problems with this function, so my initial fix 
is not enough. I went ahead and rewrote it completely. So please forget about 
the original first patch, and consider it replaced by the attached one. The 
commit message there explains how you can reproduce the bugs I found and shows 
that the new version fixes them.

Regards.

-- 
Olivier Certner

[-- Attachment #2: 0001-VC-CVS-Fix-Root-file-parsing.patch --]
[-- Type: text/x-patch, Size: 11896 bytes --]

From 8d5fc570d135c118af820db1e54a56aa7123e554 Mon Sep 17 00:00:00 2001
From: Olivier Certner <olce.emacs@certner.fr>
Date: Thu, 6 Apr 2023 10:16:33 +0200
Subject: [PATCH] VC: CVS: Fix "Root" file parsing

The new "Root" file parsing has been based on CVS' documentation,
which gives the following format for *remote* repositories:
[:method:][[user][:password]@]hostname[:[port]]/path/to/repository
and for local ones:
:local:/path
or
:local:c:/path
or alternatively ':local:' replaced by ':fork:', or ':local:' omitted
when the path starts with a slash.

[The actual parsing code in CVS is actually a bit more restrictive.
See 'root.c'.]

The end result is that the following test expression:
  (mapcar (lambda (p)
            (list p (vc-cvs-parse-root p)))
          '("/home/joe/repo"
            ":local:c:/users/joe/repo"
            "host/home/serv/repo"
            ":pserver:host/home/serv/repo"
            ":pserver:host:/home/serv/repo"
            "usr@host/home/serv/repo"
            "usr@host:/home/serv/repo"
            ":pserver:usr:passwd@host:/home/serv/repo"
            ":pserver:usr:passwd@host/home/serv/repo"
            ":pserver:usr:passwd@host:28/home/serv/repo"
            "usr:passwd@host:/home/serv/repo"
            "usr:passwd@host/home/serv/repo"
            "usr:passwd@host:28/home/serv/repo"))

gives the expected results:
  (("/home/joe/repo"
    ("local" nil nil "/home/joe/repo"))
   (":local:c:/users/joe/repo"
    ("local" nil nil "c:/users/joe/repo"))
   ("host/home/serv/repo"
    ("ext" nil "host" "/home/serv/repo"))
   (":pserver:host/home/serv/repo"
    ("pserver" nil "host" "/home/serv/repo"))
   (":pserver:host:/home/serv/repo"
    ("pserver" nil "host" "/home/serv/repo"))
   ("usr@host/home/serv/repo"
    ("ext" "usr" "host" "/home/serv/repo"))
   ("usr@host:/home/serv/repo"
    ("ext" "usr" "host" "/home/serv/repo"))
   (":pserver:usr:passwd@host:/home/serv/repo"
    ("pserver" "usr" "host" "/home/serv/repo"))
   (":pserver:usr:passwd@host/home/serv/repo"
    ("pserver" "usr" "host" "/home/serv/repo"))
   (":pserver:usr:passwd@host:28/home/serv/repo"
    ("pserver" "usr" "host" "/home/serv/repo"))
   ("usr:passwd@host:/home/serv/repo"
    ("ext" "usr" "host" "/home/serv/repo"))
   ("usr:passwd@host/home/serv/repo"
    ("ext" "usr" "host" "/home/serv/repo"))
   ("usr:passwd@host:28/home/serv/repo"
    ("ext" "usr" "host" "/home/serv/repo")))

instead of the current and erroneous:
  (("/home/joe/repo"
    ("ext" nil "home" "/joe/repo"))
   (":local:c:/users/joe/repo"
    ("local" nil nil "c:/users/joe/repo"))
   ("host/home/serv/repo"
    ("ext" nil "host" "/home/serv/repo"))
   (":pserver:host/home/serv/repo"
    ("pserver" nil "host" "/home/serv/repo"))
   (":pserver:host:/home/serv/repo"
    ("pserver" nil "host" "/home/serv/repo"))
   ("usr@host/home/serv/repo"
    ("ext" "usr" "host" "/home/serv/repo"))
   ("usr@host:/home/serv/repo"
    ("ext" "usr" "host" "/home/serv/repo"))
   (":pserver:usr:passwd@host:/home/serv/repo"
    ("pserver" nil "usr" "passwd@host"))
   (":pserver:usr:passwd@host/home/serv/repo"
    ("pserver" nil "usr" "passwd@host/home/serv/repo"))
   (":pserver:usr:passwd@host:28/home/serv/repo"
    ("pserver" nil "usr" "passwd@host"))
   ("usr:passwd@host:/home/serv/repo"
    ("passwd@host" nil "home" "/serv/repo"))
   ("usr:passwd@host/home/serv/repo"
    ("ext" nil "usr" "passwd@host/home/serv/repo"))
   ("usr:passwd@host:28/home/serv/repo"
    ("passwd@host" nil "28" "/home/serv/repo")))

* lisp/vc/vc-cvs.el (vc-cvs-parse-root): Rewrite.

(vc-cvs-parse-uhp): Remove this standalone function formerly used only
by `vc-cvs-parse-root' and which doesn't allow correct parsing anyway.
---
 lisp/vc/vc-cvs.el | 177 +++++++++++++++++++++++++++++++---------------
 1 file changed, 121 insertions(+), 56 deletions(-)

diff --git a/lisp/vc/vc-cvs.el b/lisp/vc/vc-cvs.el
index 1a47722867f..71e86a0076c 100644
--- a/lisp/vc/vc-cvs.el
+++ b/lisp/vc/vc-cvs.el
@@ -26,6 +26,7 @@
 
 (require 'vc-rcs)
 (eval-when-compile (require 'vc))
+(eval-when-compile (require 'cl-lib))
 
 (declare-function vc-checkout "vc" (file &optional rev))
 (declare-function vc-expand-dirs "vc" (file-or-dir-list backend))
@@ -825,69 +826,133 @@ vc-cvs-repository-hostname
 		(buffer-substring (point)
 				  (line-end-position))))))))
 
-(defun vc-cvs-parse-uhp (path)
-  "Parse user@host/path into (user@host /path)."
-  (if (string-match "\\([^/]+\\)\\(/.*\\)" path)
-      (list (match-string 1 path) (match-string 2 path))
-      (list nil path)))
-
-(defun vc-cvs-parse-root (root)
+(cl-defun vc-cvs-parse-root (root)
   "Split CVS ROOT specification string into a list of fields.
+
 A CVS root specification of the form
-  [:METHOD:][[USER@]HOSTNAME]:?/path/to/repository
+  [:METHOD:][[[USER][:PASSWORD]@]HOSTNAME][:[PORT]]/path/to/repository
 is converted to a normalized record with the following structure:
-  \(METHOD USER HOSTNAME CVS-ROOT).
+  \(METHOD USER HOSTNAME PATH).
+
 The default METHOD for a CVS root of the form
   /path/to/repository
-is `local'.
+is \"local\".
 The default METHOD for a CVS root of the form
   [USER@]HOSTNAME:/path/to/repository
-is `ext'.
-For an empty string, nil is returned (invalid CVS root)."
-  ;; Split CVS root into colon separated fields (0-4).
-  ;; The `x:' makes sure, that leading colons are not lost;
-  ;; `HOST:/PATH' is then different from `:METHOD:/PATH'.
-  (let* ((root-list (cdr (split-string (concat "x:" root) ":")))
-         (len (length root-list))
-         ;; All syntactic varieties will get a proper METHOD.
-         (root-list
-          (cond
-           ((= len 0)
-            ;; Invalid CVS root
-            nil)
-           ((= len 1)
-            (let ((uhp (vc-cvs-parse-uhp (car root-list))))
-              (cons (if (car uhp) "ext" "local") uhp)))
-           ((= len 2)
-            ;; [USER@]HOST:PATH => method `ext'
-            (and (not (equal (car root-list) ""))
-                 (cons "ext" root-list)))
-           ((= len 3)
-            ;; :METHOD:PATH or :METHOD:USER@HOSTNAME/PATH
-            (cons (cadr root-list)
-                  (vc-cvs-parse-uhp (nth 2 root-list))))
-           (t
-            ;; :METHOD:[USER@]HOST:PATH
-            (cdr root-list)))))
-    (if root-list
-        (let ((method (car root-list))
-              (uhost (or (cadr root-list) ""))
-              (root (nth 2 root-list))
-              user host)
-          ;; Split USER@HOST
-          (if (string-match "\\(.*\\)@\\(.*\\)" uhost)
-              (setq user (match-string 1 uhost)
-                    host (match-string 2 uhost))
-            (setq host uhost))
-          ;; Remove empty HOST
-          (and (equal host "")
-               (setq host nil))
-          ;; Fix windows style CVS root `:local:C:\\project\\cvs\\some\\dir'
-          (and host
-               (equal method "local")
-               (setq root (concat host ":" root) host nil))
-          ;; Normalize CVS root record
-          (list method user host root)))))
+is \"ext\".
+
+If METHOD is explicitly \"local\" or \"fork\", then the path starts
+immediately after the method block. This is used on Windows platforms
+when paths start with a drive letter.
+
+Note that, except for METHOD, which is defaulted if not present,
+other optional fields are returned as nil if not syntactically
+present, or as the empty string if delimited but empty.
+
+Returns nil in case of an unparsable CVS root (including the
+empty string) and issues a warning. This function doesn't check
+that an explicit method is valid, or that some fields are empty
+or nil but should not for a given method."
+  (let (method user password hostname port path
+               ;; IDX set by `next-delim' as a side-effect
+               idx)
+    (cl-labels
+        ((invalid (reason &rest args)
+           (apply #'lwarn '(vc-cvs) :warning
+                  (concat "vc-cvs-parse-root: Can't parse '%s': " reason)
+                  root args)
+           (cl-return-from vc-cvs-parse-root))
+         (no-path ()
+           (invalid "No path"))
+         (next-delim (start)
+           ;; Search for a :, @ or /
+           (setq idx (string-match-p "[:@/]" root start))
+           (if idx (aref root idx) (no-path)))
+         (grab-user (start end)
+           (setq user (substring root start end)))
+         (at-hostname-block (start)
+           (let ((cand (next-delim start)))
+             (cl-ecase cand
+               (?:
+                ;; Could be : before PORT and PATH, or before PASSWORD. We
+                ;; search for a @ to disambiguate.
+                (let ((colon-idx idx)
+                      (cand (next-delim (1+ idx))))
+                  (cl-ecase cand
+                    (?:
+                     (invalid (concat "Hostname block: Superfluous : at %s "
+                                      "or missing @ before")
+                              idx))
+                    (?@
+                     ;; USER:PASSWORD case
+                     (grab-user start colon-idx)
+                     (delimited-password (1+ colon-idx) idx))
+                    (?/
+                     ;; HOSTNAME[:[PORT]] case
+                     (grab-hostname start colon-idx)
+                     (delimited-port (1+ colon-idx) idx)))))
+               (?@
+                (grab-user start idx)
+                (at-hostname (1+ idx)))
+               (?/
+                (if (/= idx start)
+                    (grab-hostname start idx))
+                (at-path idx)))))
+         (delimited-password (start end)
+           (setq password (substring root start end))
+           (at-hostname (1+ end)))
+         (grab-hostname (start end)
+           (setq hostname (substring root start end)))
+         (at-hostname (start)
+           (let ((cand (next-delim start)))
+             (cl-ecase cand
+               (?:
+                (grab-hostname start idx)
+                (at-port (1+ idx)))
+               (?@
+                (invalid "Hostname: Superfluous @ after index %s" start))
+               (?/
+                (grab-hostname start idx)
+                (at-path idx)))))
+         (delimited-port (start end)
+           (setq port (substring root start end))
+           (at-path end))
+         (at-port (start)
+           (let ((end (string-match-p "/" root start)))
+             (if end (delimited-port start end) (no-path))))
+         (at-path (start)
+           (setq path (substring root start))))
+      (when (string= root "")
+        (invalid "Empty string"))
+      ;; Check for a starting ":"
+      (if (= (aref root 0) ?:)
+          ;; 3 possible cases:
+          ;; - :METHOD: at start. METHOD doesn't have any @.
+          ;; - :PASSWORD@ at start. Must be followed by HOSTNAME.
+          ;; - :[PORT] at start. Must be followed immediately by a "/".
+          ;; So, find the next character equal to ":", "@" or "/".
+          (let ((cand (next-delim 1)))
+            (cl-ecase cand
+              (?:
+               ;; :METHOD: case
+               (setq method (substring root 1 idx))
+               ;; Continue
+               (if (member method '("local" "fork"))
+                   (at-path (1+ idx))
+                 (at-hostname-block (1+ idx))))
+              (?@
+               ;; :PASSWORD@HOSTNAME case
+               (delimited-password 1 idx))
+              (?/
+               ;; :[PORT] case.
+               (at-port 1 idx))))
+        ;; No starting ":", there can't be any METHOD.
+        (at-hostname-block 0)))
+    (unless method
+      ;; Default the method if not specified
+      (setq method
+            (if (or user password hostname port) "ext" "local")))
+    (list method user hostname path)))
 
 ;; XXX: This does not work correctly for subdirectories.  "cvs status"
 ;; information is context sensitive, it contains lines like:
-- 
2.39.2

bug#62693: 28.2; VC: CVS: Fix lost file reporting and enable reverting it

[Dmitry Gutov]

On 13/04/2023 19:40, Olivier Certner wrote:
> Hi,
> 
> Thanks for looking at this.
> 
>> - Could you explain what kind of files are not listed and when? I'm
>> guessing this has to do with the file name? I've tried to reproduce the
>> problem with a file name that contained a space, and couldn't see it.
> 
> It's not that files are not listed, they are so always. As said, the problem
> is their name. For example, if I remove an INSTALL file in a repository,
> `INSTALL' is the name listed in a VC dir buffer, *quotes included*. Then, no
> VC operation can work because there is indeed no file by this name (with
> quotes, both in the filesystem or known by CVS).

So the actual file name is not the problem -- the problem happens with 
removed files?

Okay, I can see that now: when the file is removed, it's displayed 
(incorrectly) with quotes around its name. Patches 2 and 3 do fix the 
remove - vc-dir - vc-revert scenario.

Eli, do we want these fixes in emacs-29? These are again not 
regressions, but I'm guessing users of CVS are foremost to benefit from 
adding this to the upcoming release.

>> - Could you give an example of the repository CVS root which is
>> recognized by the current code incorrectly?
> 
> Just run (vc-cvs-parse-root "/your/local/path/to/some/CVS/repo") and you'll
> see...
> 
> I've found more occurences of problems with this function, so my initial fix
> is not enough. I went ahead and rewrote it completely. So please forget about
> the original first patch, and consider it replaced by the attached one. The
> commit message there explains how you can reproduce the bugs I found and shows
> that the new version fixes them.

Thank you. What was the effect of wrong local/nonlocal recognition, 
though? Some misbehavior related to cvs-stay-local?

Regarding the new patch, it's great to see the list of examples, but 
could you instead move it to a test or several inside 
test/lisp/vc/vc-cvs-tests.el? This file does not exist yet, but you can 
use vc-git-test.el as an example, in the same directory.

bug#62693: 28.2; VC: CVS: Fix lost file reporting and enable reverting it

[Eli Zaretskii]

> Date: Sat, 15 Apr 2023 01:59:28 +0300
> Cc: 62693@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 13/04/2023 19:40, Olivier Certner wrote:
> > Hi,
> > 
> > Thanks for looking at this.
> > 
> >> - Could you explain what kind of files are not listed and when? I'm
> >> guessing this has to do with the file name? I've tried to reproduce the
> >> problem with a file name that contained a space, and couldn't see it.
> > 
> > It's not that files are not listed, they are so always. As said, the problem
> > is their name. For example, if I remove an INSTALL file in a repository,
> > `INSTALL' is the name listed in a VC dir buffer, *quotes included*. Then, no
> > VC operation can work because there is indeed no file by this name (with
> > quotes, both in the filesystem or known by CVS).
> 
> So the actual file name is not the problem -- the problem happens with 
> removed files?
> 
> Okay, I can see that now: when the file is removed, it's displayed 
> (incorrectly) with quotes around its name. Patches 2 and 3 do fix the 
> remove - vc-dir - vc-revert scenario.
> 
> Eli, do we want these fixes in emacs-29? These are again not 
> regressions, but I'm guessing users of CVS are foremost to benefit from 
> adding this to the upcoming release.

The patch is AFAICS basically a complete rewrite of an important
function, so I don't see how I could agree applying this to the
release branch.  Sorry.  (Was the introduction of so many CL-isms
really necessary, btw?)

As a minor nit, please don't use "path" or "PATH" for anything except
$PATH-style lists of directories, as GNU Coding Standards frown on
such use of this word.  We use "file name" or "directory name" or
"leading directories" (as the case may be) instead.

> Regarding the new patch, it's great to see the list of examples, but 
> could you instead move it to a test or several inside 
> test/lisp/vc/vc-cvs-tests.el? This file does not exist yet, but you can 
> use vc-git-test.el as an example, in the same directory.

Yes, having a test for this would be most welcome.

bug#62693: 28.2; VC: CVS: Fix lost file reporting and enable reverting it

[Olivier Certner]

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

Hello Eli,

> The patch is AFAICS basically a complete rewrite of an important
> function, so I don't see how I could agree applying this to the
> release branch.  Sorry.  (Was the introduction of so many CL-isms
> really necessary, btw?)

This paragraph leaves me astonished.

First, because it is wrong: `vs-cvs-parse-root' is not a major function, it is 
a very minor one. It's not used anywhere except in `vc-cvs-repository-
hostname', which itself is not used anywhere except in `vc-cvs-stay-local-p' 
with the sole goal to deactivate Emacs caching when the CVS repository is 
local. Emacs caching can be an annoyance in certain specific cases (that's why 
I submitted the particular change you're commenting on) but in normal usage it 
is transparent (it doesn't affect correctness). Furthermore, its functionality 
is simple, very well contained, and its operation has been tested and shown to 
be better than the existing one.

Second, because "Was the introduction of so many CL-isms really necessary, 
btw?" is both unproductive and rude, and as such doubly inappropriate. If 
there is a policy of banishing CL forms, then say so, and even better, write 
it in the documentation. If there is already one (I'm not aware of any), then 
point me to it. You even didn't bother naming the constructs you're 
questioning. So let me review all candidates for you. The "so many" (!) CL-
isms are of 3 kinds only: `cl-defun', `cl-labels' and `cl-ecase'. The first is 
used to bail out early from the function without complicating code. It is much 
more appropriate from a language point of view than the `throw'/`catch' it 
expands to (to readers, and implementors as well if at some point Emacs gains 
true lexical non-local exits). The second fills an Emacs Lisp gap (definition 
of internal recursive functions), so is necessary (unless you want me to 
define these functions with `defun' whereas they are only internal and not 
meant to be called standalone, or use `let' with `lambda` forms and 
funcalls?). The third is the most concise way to express the intent, even 
before `pcase'. Sure, I could use the latter and define a catch-all case with 
an internal error, but then I'll have to do that the 4 times `cl-ecase' is 
used: 4 additional lines with no added value. Does using `cl-ecase' really 
change the face of Emacs world?

To be frank, I'm quite worried that I have to explain all that to an Emacs 
maintainer.
 
> As a minor nit, please don't use "path" or "PATH" for anything except
> $PATH-style lists of directories, as GNU Coding Standards frown on
> such use of this word.  We use "file name" or "directory name" or
> "leading directories" (as the case may be) instead.

I was not aware of it, and it took me some time to find where the GNU Coding 
Standards document says so: In a documentation section only, which is easily 
overlooked when writing code. I certainly see a logic in abiding by it in code 
as well (since code is also in part documentation). But I'll also note that 
the GNU project on this particular point goes against all established 
traditions and for dubious reasons.

I find "file name" or "directory name" even worse because they are ambiguous. 
So I've settled on using "pathname" instead (the (UNIX/Windows) "path" to some 
file, not just the filename (i.e., no slashes)). Is that OK for you?

> > Regarding the new patch, it's great to see the list of examples, but 
> > could you instead move it to a test or several inside 
> > test/lisp/vc/vc-cvs-tests.el? This file does not exist yet, but you can 
> > use vc-git-test.el as an example, in the same directory.
> 
> Yes, having a test for this would be most welcome.

The examples in the commit messages have been removed and replaced by a test 
file.

New patch attached. The only new functional change is to test for an empty 
hostname in `vc-cvs-repository-hostname', in an attempt to make it easier for 
you to see that nothing can be broken as long as `vc-cvs-parse-root' works 
correctly. Compared to the old code, the new implementation can, on an 
*invalid* CVS/Root specifications, return an empty hostname where the older 
would return nil (assuming a correct parsing case). This has no real practical 
consequences since 'cvs' commands are anyway bound to fail at a later point in 
such a case, but may reassure you about the innocuity of this change.

-- 
Olivier Certner

[-- Attachment #2: 0001-VC-CVS-Fix-Root-file-parsing.patch --]
[-- Type: text/x-patch, Size: 14736 bytes --]

From 350718599dd34d3c06dc6c12d254e37875fe4b6a Mon Sep 17 00:00:00 2001
From: Olivier Certner <olce.emacs@certner.fr>
Date: Thu, 6 Apr 2023 10:16:33 +0200
Subject: [PATCH] VC: CVS: Fix "Root" file parsing

The new "Root" file parsing has been based on CVS' documentation,
which gives the following format for *remote* repositories:
[:method:][[user][:password]@]hostname[:[port]]/pathname/to/repository
and for local ones:
:local:/pathname/to/repository
or
:local:c:/pathname/to/repository
or alternatively ':local:' replaced by ':fork:', or ':local:' omitted
when the path starts with a slash.

[The actual parsing code in CVS is actually a bit more restrictive.
See 'root.c'.]

Most notably, the previous version could not parse an absolute
pathname without an explicit :local: method or :pserver: lines with
passwords.

* lisp/vc/vc-cvs.el (vc-cvs-parse-root): Rewrite.

(vc-cvs-repository-hostname): Cope with `vc-cvs-parse-root' returning
an empty hostname (can only happen if the "Root" file is invalid),
returning nil in this case.

(vc-cvs-parse-uhp): Remove this standalone function formerly used only
by `vc-cvs-parse-root' and which doesn't allow correct parsing anyway.

* test/lisp/vc/vc-cvs-tests.el: New file, with tests for common "Root"
file content.
---
 lisp/vc/vc-cvs.el            | 200 ++++++++++++++++++++++++-----------
 test/lisp/vc/vc-cvs-tests.el | 107 +++++++++++++++++++
 2 files changed, 244 insertions(+), 63 deletions(-)
 create mode 100644 test/lisp/vc/vc-cvs-tests.el

diff --git a/lisp/vc/vc-cvs.el b/lisp/vc/vc-cvs.el
index ceebf2b2211..c6056c1e5bd 100644
--- a/lisp/vc/vc-cvs.el
+++ b/lisp/vc/vc-cvs.el
@@ -26,6 +26,7 @@
 
 (require 'vc-rcs)
 (eval-when-compile (require 'vc))
+(eval-when-compile (require 'cl-lib))
 (require 'log-view)
 
 (declare-function vc-checkout "vc" (file &optional rev))
@@ -813,7 +814,10 @@ vc-cvs-stay-local-p
                             'yes 'no))))))))))))
 
 (defun vc-cvs-repository-hostname (dirname)
-  "Hostname of the CVS server associated to workarea DIRNAME."
+  "Hostname of the CVS server associated to workarea DIRNAME.
+
+Returns nil if there is not hostname or the hostname could not be
+determined because the CVS/Root specification is invalid."
   (let ((rootname (expand-file-name "CVS/Root" dirname)))
     (when (file-readable-p rootname)
       (with-temp-buffer
@@ -822,73 +826,143 @@ vc-cvs-repository-hostname
 		   default-file-name-coding-system)))
 	  (vc-insert-file rootname))
 	(goto-char (point-min))
-	(nth 2 (vc-cvs-parse-root
-		(buffer-substring (point)
-				  (line-end-position))))))))
+        (let ((hostname
+	       (nth 2 (vc-cvs-parse-root
+		       (buffer-substring (point)
+				         (line-end-position))))))
+          (unless (string= hostname "")
+            hostname))))))
 
-(defun vc-cvs-parse-uhp (path)
-  "Parse user@host/path into (user@host /path)."
-  (if (string-match "\\([^/]+\\)\\(/.*\\)" path)
-      (list (match-string 1 path) (match-string 2 path))
-      (list nil path)))
+(cl-defun vc-cvs-parse-root (root)
+  "Split CVS Root specification string into a list of fields.
 
-(defun vc-cvs-parse-root (root)
-  "Split CVS ROOT specification string into a list of fields.
-A CVS root specification of the form
-  [:METHOD:][[USER@]HOSTNAME]:?/path/to/repository
+A CVS Root specification of the form
+  [:METHOD:][[[USER][:PASSWORD]@]HOSTNAME][:[PORT]]/pathname/to/repository
 is converted to a normalized record with the following structure:
-  \(METHOD USER HOSTNAME CVS-ROOT).
+  \(METHOD USER HOSTNAME PATHNAME).
+
 The default METHOD for a CVS root of the form
-  /path/to/repository
-is `local'.
+  /pathname/to/repository
+is \"local\".
 The default METHOD for a CVS root of the form
-  [USER@]HOSTNAME:/path/to/repository
-is `ext'.
-For an empty string, nil is returned (invalid CVS root)."
-  ;; Split CVS root into colon separated fields (0-4).
-  ;; The `x:' makes sure, that leading colons are not lost;
-  ;; `HOST:/PATH' is then different from `:METHOD:/PATH'.
-  (let* ((root-list (cdr (split-string (concat "x:" root) ":")))
-         (len (length root-list))
-         ;; All syntactic varieties will get a proper METHOD.
-         (root-list
-          (cond
-           ((= len 0)
-            ;; Invalid CVS root
-            nil)
-           ((= len 1)
-            (let ((uhp (vc-cvs-parse-uhp (car root-list))))
-              (cons (if (car uhp) "ext" "local") uhp)))
-           ((= len 2)
-            ;; [USER@]HOST:PATH => method `ext'
-            (and (not (equal (car root-list) ""))
-                 (cons "ext" root-list)))
-           ((= len 3)
-            ;; :METHOD:PATH or :METHOD:USER@HOSTNAME/PATH
-            (cons (cadr root-list)
-                  (vc-cvs-parse-uhp (nth 2 root-list))))
-           (t
-            ;; :METHOD:[USER@]HOST:PATH
-            (cdr root-list)))))
-    (if root-list
-        (let ((method (car root-list))
-              (uhost (or (cadr root-list) ""))
-              (root (nth 2 root-list))
-              user host)
-          ;; Split USER@HOST
-          (if (string-match "\\(.*\\)@\\(.*\\)" uhost)
-              (setq user (match-string 1 uhost)
-                    host (match-string 2 uhost))
-            (setq host uhost))
-          ;; Remove empty HOST
-          (and (equal host "")
-               (setq host nil))
-          ;; Fix windows style CVS root `:local:C:\\project\\cvs\\some\\dir'
-          (and host
-               (equal method "local")
-               (setq root (concat host ":" root) host nil))
-          ;; Normalize CVS root record
-          (list method user host root)))))
+  [USER@]HOSTNAME:/pathname/to/repository
+is \"ext\".
+
+If METHOD is explicitly \"local\" or \"fork\", then the pathname
+starts immediately after the method block. This must be used on
+Windows platforms when pathnames start with a drive letter.
+
+Note that, except for METHOD, which is defaulted if not present,
+other optional fields are returned as nil if not syntactically
+present, or as the empty string if delimited but empty.
+
+Returns nil in case of an unparsable CVS root (including the
+empty string) and issues a warning. This function doesn't check
+that an explicit method is valid, or that some fields are empty
+or nil but should not for a given method."
+  (let (method user password hostname port pathname
+               ;; IDX set by `next-delim' as a side-effect
+               idx)
+    (cl-labels
+        ((invalid (reason &rest args)
+           (apply #'lwarn '(vc-cvs) :warning
+                  (concat "vc-cvs-parse-root: Can't parse '%s': " reason)
+                  root args)
+           (cl-return-from vc-cvs-parse-root))
+         (no-pathname ()
+           (invalid "No pathname"))
+         (next-delim (start)
+           ;; Search for a :, @ or /. If none is found, there can be
+           ;; no path at the end, which is an error.
+           (setq idx (string-match-p "[:@/]" root start))
+           (if idx (aref root idx) (no-pathname)))
+         (grab-user (start end)
+           (setq user (substring root start end)))
+         (at-hostname-block (start)
+           (let ((cand (next-delim start)))
+             (cl-ecase cand
+               (?:
+                ;; Could be : before PORT and PATHNAME, or before
+                ;; PASSWORD. We search for a @ to disambiguate.
+                (let ((colon-idx idx)
+                      (cand (next-delim (1+ idx))))
+                  (cl-ecase cand
+                    (?:
+                     (invalid
+                      (eval-when-compile
+                        (concat "Hostname block: Superfluous : at %s "
+                                "or missing @ before"))
+                      idx))
+                    (?@
+                     ;; USER:PASSWORD case
+                     (grab-user start colon-idx)
+                     (delimited-password (1+ colon-idx) idx))
+                    (?/
+                     ;; HOSTNAME[:[PORT]] case
+                     (grab-hostname start colon-idx)
+                     (delimited-port (1+ colon-idx) idx)))))
+               (?@
+                (grab-user start idx)
+                (at-hostname (1+ idx)))
+               (?/
+                (if (/= idx start)
+                    (grab-hostname start idx))
+                (at-pathname idx)))))
+         (delimited-password (start end)
+           (setq password (substring root start end))
+           (at-hostname (1+ end)))
+         (grab-hostname (start end)
+           (setq hostname (substring root start end)))
+         (at-hostname (start)
+           (let ((cand (next-delim start)))
+             (cl-ecase cand
+               (?:
+                (grab-hostname start idx)
+                (at-port (1+ idx)))
+               (?@
+                (invalid "Hostname: Unexpected @ after index %s" start))
+               (?/
+                (grab-hostname start idx)
+                (at-pathname idx)))))
+         (delimited-port (start end)
+           (setq port (substring root start end))
+           (at-pathname end))
+         (at-port (start)
+           (let ((end (string-match-p "/" root start)))
+             (if end (delimited-port start end) (no-pathname))))
+         (at-pathname (start)
+           (setq pathname (substring root start))))
+      (when (string= root "")
+        (invalid "Empty string"))
+      ;; Check for a starting ":"
+      (if (= (aref root 0) ?:)
+          ;; 3 possible cases:
+          ;; - :METHOD: at start. METHOD doesn't have any @.
+          ;; - :PASSWORD@ at start. Must be followed by HOSTNAME.
+          ;; - :[PORT] at start. Must be followed immediately by a "/".
+          ;; So, find the next character equal to ":", "@" or "/".
+          (let ((cand (next-delim 1)))
+            (cl-ecase cand
+              (?:
+               ;; :METHOD: case
+               (setq method (substring root 1 idx))
+               ;; Continue
+               (if (member method '("local" "fork"))
+                   (at-pathname (1+ idx))
+                 (at-hostname-block (1+ idx))))
+              (?@
+               ;; :PASSWORD@HOSTNAME case
+               (delimited-password 1 idx))
+              (?/
+               ;; :[PORT] case.
+               (at-port 1 idx))))
+        ;; No starting ":", there can't be any METHOD.
+        (at-hostname-block 0)))
+    (unless method
+      ;; Default the method if not specified
+      (setq method
+            (if (or user password hostname port) "ext" "local")))
+    (list method user hostname pathname)))
 
 ;; XXX: This does not work correctly for subdirectories.  "cvs status"
 ;; information is context sensitive, it contains lines like:
diff --git a/test/lisp/vc/vc-cvs-tests.el b/test/lisp/vc/vc-cvs-tests.el
new file mode 100644
index 00000000000..99ac9c8eb96
--- /dev/null
+++ b/test/lisp/vc/vc-cvs-tests.el
@@ -0,0 +1,107 @@
+;;; vc-cvs-tests.el --- tests for vc/vc-cvs.el  -*- lexical-binding:t -*-
+
+;; Copyright (C) 2023 Free Software Foundation, Inc.
+
+;; Author: Olivier Certner <olce.emacs@certner.fr>
+;; Maintainer: emacs-devel@gnu.org
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'vc-cvs)
+
+(ert-deftest vc-cvs-test-parse-root--local-no-method ()
+  (vc-cvs-test--check-parse-root
+   "/home/joe/repo"
+   '("local" nil nil "/home/joe/repo")))
+
+(ert-deftest vc-cvs-test-parse-root--local-windows-drive-letter ()
+  (vc-cvs-test--check-parse-root
+   ":local:c:/users/joe/repo"
+   '("local" nil nil "c:/users/joe/repo")))
+
+(ert-deftest vc-cvs-test-parse-root--ext-no-method-host-no-port-colon ()
+  (vc-cvs-test--check-parse-root
+   "host/home/serv/repo"
+   '("ext" nil "host" "/home/serv/repo")))
+
+(ert-deftest vc-cvs-test-parse-root--pserver-host-no-port-colon ()
+  (vc-cvs-test--check-parse-root
+   ":pserver:host/home/serv/repo"
+   '("pserver" nil "host" "/home/serv/repo")))
+
+(ert-deftest vc-cvs-test-parse-root--pserver-host-port-colon ()
+  (vc-cvs-test--check-parse-root
+   ":pserver:host:/home/serv/repo"
+   '("pserver" nil "host" "/home/serv/repo")))
+
+(ert-deftest vc-cvs-test-parse-root--ext-no-method-user-host-no-port-colon ()
+  (vc-cvs-test--check-parse-root
+   "usr@host/home/serv/repo"
+   '("ext" "usr" "host" "/home/serv/repo")))
+
+(ert-deftest vc-cvs-test-parse-root--ext-no-method-user-host-port-colon ()
+  (vc-cvs-test--check-parse-root
+   "usr@host:/home/serv/repo"
+   '("ext" "usr" "host" "/home/serv/repo")))
+
+(ert-deftest vc-cvs-test-parse-root--pserver-user-password-host-no-port-colon ()
+  (vc-cvs-test--check-parse-root
+   ":pserver:usr:passwd@host/home/serv/repo"
+   '("pserver" "usr" "host" "/home/serv/repo")))
+
+(ert-deftest vc-cvs-test-parse-root--pserver-user-password-host-port-colon ()
+  (vc-cvs-test--check-parse-root
+   ":pserver:usr:passwd@host:/home/serv/repo"
+   '("pserver" "usr" "host" "/home/serv/repo")))
+
+(ert-deftest vc-cvs-test-parse-root--pserver-user-password-host-port ()
+  (vc-cvs-test--check-parse-root
+   ":pserver:usr:passwd@host:28/home/serv/repo"
+   '("pserver" "usr" "host" "/home/serv/repo")))
+
+;; Next 3 tests are just to err on the side of caution. It doesn't
+;; seem that CVS 1.12 can ever produce such lines.
+
+(ert-deftest
+    vc-cvs-test-parse-root--ext-no-method-user-password-host-no-port-colon
+    ()
+  (vc-cvs-test--check-parse-root
+   "usr:passwd@host/home/serv/repo"
+   '("ext" "usr" "host" "/home/serv/repo")))
+
+(ert-deftest
+    vc-cvs-test-parse-root--ext-no-method-user-password-host-port-colon
+    ()
+  (vc-cvs-test--check-parse-root
+   "usr:passwd@host:/home/serv/repo"
+   '("ext" "usr" "host" "/home/serv/repo")))
+
+(ert-deftest
+    vc-cvs-test-parse-root--ext-no-method-user-password-host-port
+    ()
+  (vc-cvs-test--check-parse-root
+   "usr:passwd@host:28/home/serv/repo"
+   '("ext" "usr" "host" "/home/serv/repo")))
+
+
+(defun vc-cvs-test--check-parse-root (input expected-output)
+  (should (equal (vc-cvs-parse-root input) expected-output)))
+
+;;; vc-cvs-tests.el ends here
-- 
2.39.2

bug#62693: 28.2; VC: CVS: Fix lost file reporting and enable reverting it

[Eli Zaretskii]

> From: Olivier Certner <ocert.dev@free.fr>
> Cc: 62693@debbugs.gnu.org
> Date: Mon, 17 Apr 2023 11:24:26 +0200
> 
> > The patch is AFAICS basically a complete rewrite of an important
> > function, so I don't see how I could agree applying this to the
> > release branch.  Sorry.  (Was the introduction of so many CL-isms
> > really necessary, btw?)
> 
> This paragraph leaves me astonished.
> 
> First, because it is wrong: `vs-cvs-parse-root' is not a major function, it is 
> a very minor one.

I should have said "non-trivial".  Sorry for potentially confusing
selection of words.

> Second, because "Was the introduction of so many CL-isms really necessary, 
> btw?" is both unproductive and rude, and as such doubly inappropriate.

Dmitry asked whether it was okay to install this on the release
branch, which is currently undergoing pretest.  So I looked at the
changes from the POV of the potential for unintended consequences and
regressions.  This job is much harder when the code is thoroughly
rewritten, let alone uses a different macro system and style.  That is
why I asked the question: I presume that if the changes were less
radical, they could perhaps have been deemed appropriate for the
release branch, or at least the chances for that were higher.

> If 
> there is a policy of banishing CL forms, then say so, and even better, write 
> it in the documentation. If there is already one (I'm not aware of any), then 
> point me to it. You even didn't bother naming the constructs you're 
> questioning. So let me review all candidates for you. The "so many" (!) CL-
> isms are of 3 kinds only: `cl-defun', `cl-labels' and `cl-ecase'. The first is 
> used to bail out early from the function without complicating code. It is much 
> more appropriate from a language point of view than the `throw'/`catch' it 
> expands to (to readers, and implementors as well if at some point Emacs gains 
> true lexical non-local exits). The second fills an Emacs Lisp gap (definition 
> of internal recursive functions), so is necessary (unless you want me to 
> define these functions with `defun' whereas they are only internal and not 
> meant to be called standalone, or use `let' with `lambda` forms and 
> funcalls?). The third is the most concise way to express the intent, even 
> before `pcase'. Sure, I could use the latter and define a catch-all case with 
> an internal error, but then I'll have to do that the 4 times `cl-ecase' is 
> used: 4 additional lines with no added value. Does using `cl-ecase' really 
> change the face of Emacs world?
> 
> To be frank, I'm quite worried that I have to explain all that to an Emacs 
> maintainer.

You didn't have to.  That's not why I asked the question.

> I find "file name" or "directory name" even worse because they are ambiguous. 
> So I've settled on using "pathname" instead (the (UNIX/Windows) "path" to some 
> file, not just the filename (i.e., no slashes)). Is that OK for you?

Not really, but that's a minor point.  We can always fix the
terminology later.

Thanks.

bug#62693: 28.2; VC: CVS: Fix lost file reporting and enable reverting it

[Olivier Certner]

Hi,

> I should have said "non-trivial".  Sorry for potentially confusing
> selection of words.

Non-trivial certainly, but not that complex either (and this thanks to the style used). And, as importantly, this is a component with very delimited and clear functionality and interface, making it simple to reason about from the outside (and this has not been changed by the patch) and see the impacts, especially with a single chain of callers.
 
> Dmitry asked whether it was okay to install this on the release
> branch, which is currently undergoing pretest.  So I looked at the
> changes from the POV of the potential for unintended consequences and
> regressions.  This job is much harder when the code is thoroughly
> rewritten, let alone uses a different macro system and style.  That is
> why I asked the question: I presume that if the changes were less
> radical, they could perhaps have been deemed appropriate for the
> release branch, or at least the chances for that were higher.

That I can perfectly understand, even if my take would probably be different. That said, I've re-read your mail and it really comes out very differently without such context. So at least thanks for providing it.
 
> You didn't have to.  That's not why I asked the question.

You didn't state the underpinnings, and were apparently adressing to me, not Dmitry, in the rest of the mail, so this is what I inferred for the first paragraph as well. Which then made me lose some time (and subsequently temper), and perhaps made you as well. Proper, transparent communication is key here.
 
> Not really, but that's a minor point.  We can always fix the
> terminology later.

So I guess this means that I'll leave it to you to possibly change names later. Given the previous miscommunication, let me explicitly invite you to manifest yourself if my guess is wrong and you are expecting something else from me on this topic.

Thanks.

-- 
Olivier Certner

bug#62693: 28.2; VC: CVS: Fix lost file reporting and enable reverting it

[Dmitry Gutov]

Version: 30.1

On 17/04/2023 12:24, Olivier Certner wrote:
>>> Regarding the new patch, it's great to see the list of examples, but
>>> could you instead move it to a test or several inside
>>> test/lisp/vc/vc-cvs-tests.el? This file does not exist yet, but you can
>>> use vc-git-test.el as an example, in the same directory.
>> Yes, having a test for this would be most welcome.
> The examples in the commit messages have been removed and replaced by a test
> file.
> 
> New patch attached. The only new functional change is to test for an empty
> hostname in `vc-cvs-repository-hostname', in an attempt to make it easier for
> you to see that nothing can be broken as long as `vc-cvs-parse-root' works
> correctly. Compared to the old code, the new implementation can, on an
> *invalid*  CVS/Root specifications, return an empty hostname where the older
> would return nil (assuming a correct parsing case). This has no real practical
> consequences since 'cvs' commands are anyway bound to fail at a later point in
> such a case, but may reassure you about the innocuity of this change.

Very good, I've pushed the three patches to 'master'.

Thank you for your work, and closing.

bug#62693: 28.2; VC: CVS: Fix lost file reporting and enable reverting it

[Olivier Certner]

> Very good, I've pushed the three patches to 'master'.

Great! Thanks. 

-- 
Olivier Certner

bug#62693: 28.2; VC: CVS: Fix lost file reporting and enable reverting it

[Eli Zaretskii]

> Date: Wed, 19 Apr 2023 03:54:15 +0300
> Cc: 62693-done@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> Very good, I've pushed the three patches to 'master'.

Thank, but please always mention the bug number in the commit log
messages.

bug#62693: 28.2; VC: CVS: Fix lost file reporting and enable reverting it

[Olivier Certner]

Hi,

> Thank, but please always mention the bug number in the commit log
> messages.

This is what I did in all my previous fixes, by first sending the bug 
description and only then patches with the commit messages having the bug 
number. I didn't do it this time because this is a (minor) annoyance consuming 
a bit of time. In order to help maintainers, I'll make sure I revert to my 
previous behavior in future submissions.

Thanks.

PS: Don't know if this was intentional, but you sent the mail to 
62693@debbugs.gnu.org, which AFAIU reopens the bug. This answer, sent to 
62693-done@debbugs.gnu.org instead, should close it again (hopefully fixing 
the unintentional change).

-- 
Olivier Certner

bug#62693: 28.2; VC: CVS: Fix lost file reporting and enable reverting it

[Dmitry Gutov]

On 20/04/2023 12:42, Olivier Certner wrote:
> Hi,
> 
>> Thank, but please always mention the bug number in the commit log
>> messages.
> 
> This is what I did in all my previous fixes, by first sending the bug
> description and only then patches with the commit messages having the bug
> number. I didn't do it this time because this is a (minor) annoyance consuming
> a bit of time. In order to help maintainers, I'll make sure I revert to my
> previous behavior in future submissions.

Thank you. This was more of an omission on my part (I forgot to check), 
given that at the time when the bug is first submitted, the bug number 
is not known anyway.

> Thanks.
> 
> PS: Don't know if this was intentional, but you sent the mail to
> 62693@debbugs.gnu.org, which AFAIU reopens the bug.

Nah, it doesn't. When closed, it stays closed.

bug#62693: 28.2; VC: CVS: Fix lost file reporting and enable reverting it

[Olivier Certner]

Hi,

> Thank you. What was the effect of wrong local/nonlocal recognition, 
> though? Some misbehavior related to cvs-stay-local?

Yes, exactly. I don't want Emacs to start caching information on local repositories, which causes inconsistencies when the repository is manipulated outside of Emacs concurrently.
 
> Regarding the new patch, it's great to see the list of examples, but 
> could you instead move it to a test or several inside 
> test/lisp/vc/vc-cvs-tests.el? This file does not exist yet, but you can 
> use vc-git-test.el as an example, in the same directory.

Yes. See next mail.

Regards.

-- 
Olivier Certner