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

[Olivier Certner]

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

0001-VC-CVS-Fix-Root-file-parsing.patch
Description: Text Data