bug#24082: [PATCH] Use ‘cvs update’ instead ‘cvs status’ for CVS *vc-dir* buffers

[Dmitry Gutov]

On 07.10.2016 22:29, Göktuğ Kayaalp wrote:
> On 2016-10-07 02:25:24 AM +0300, Dmitry Gutov <dgutov@yandex.ru> wrote:
> > Hi! Sorry for the long wait.
>
> No problem, though actually I'm the one who caused the long delay b/c I
> was too late to send in copyright papers, sorry :)

We could have done the review earlier. But anyway...

> On a computer with CVS installed, these commands should create a working
> directory with which the bug can be reproduced:

Thank you, that works.

> The output for the ‘cvs status’ command does not provide adequate
> information to reliably and simply construct the full path to each
> file.  The output from the abovementioned command in the checkout the
> commands I listed created is like follows:
>
> ,----
> | cvs status: Examining .
> | ===================================================================
> | File: testfil                 Status: Locally Modified
> |
> |    Working revision:  1.1.1.1 Fri Oct  7 18:46:53 2016
> |    Repository revision:       1.1.1.1 /tmp/cvsroot/test/testfil,v
> |    Sticky Tag:                (none)
> |    Sticky Date:               (none)
> |    Sticky Options:    (none)
> |
> | cvs status: Examining subdir
> | ===================================================================
> | File: subfil                  Status: Locally Modified
> |
> |    Working revision:  1.1.1.1 Fri Oct  7 18:46:53 2016
> |    Repository revision:       1.1.1.1 /tmp/cvsroot/test/subdir/subfil,v
> |    Sticky Tag:                (none)
> |    Sticky Date:               (none)
> |    Sticky Options:    (none)
> `----
>
> The code was trying to rememeber the ‘cvs status: Examining <directory>’
> line in order to reconstruct the path, a method that's fragile and
> which requires a lot of regexp magic.

Maybe the problem here is that the output we have to deal with does not 
contain "Examining subdir" because the current implementation of 
vc-cvs-dir-status-files passes each individual files name to 'cvs -f 
status'.

And 'cvs -f status testfil subdir/subfil' does not output that line. I 
think the most reasonable approach for that solution would be to parse 
the "Repository revision" lines.

Apparently, vc-cvs-dir-status-files was always broken for this usage, 
and the problem has surfaced now when we've reimplemented dir-status on 
top of dir-status-files.

  ‘cvs update’, instead has a very
> filter-friendly and determinable output syntax also imitated by other
> version control software (output for the same checkout):

That looks reasonable. However, it looks like a significant change, so 
I'm not sure it's appropriate for Emacs 25.2. We'd like to get *a* fix 
into 25.2, however.

> I actually sort-a-kind-a fixed the ‘cvs status’ version initially, but
> it's waay slower than using update.  I don't have that fix anymore, but
> as I said, using ‘cvs update’ is more robust, simpler, and faster.  In
> order to fix the ‘cvs status’ method, I read the relevant files from the
> CVS/ subdir of the checkout, reconstruct the absolute module path using
> info from files therewithin, than subtract that from the third column of
> the ‘ Repository version:...’ lines for each file to find its relative
> path to the checkout's root directory, ending up with a complex and
> fragile hack, that's also slow and incomplete.

Couldn't you subtract those values from default-directory instead?

> Well, I'm unaware of both the history of the command and why the related
> Elisp was commented out, though I do not think that the output format
> changed in a long time, or that anything else really changed in CVS
> recently :)  I recommend asking someone more knowledgeable than me

OK, let's ask Dan.

In the meantime, here's a simple fix we can consider. This would still 
adhere to the dir-status-files protocol, but it's probably slower than 
ideal:

diff --git a/lisp/vc/vc-cvs.el b/lisp/vc/vc-cvs.el
index a2499a2..e949f30 100644
--- a/lisp/vc/vc-cvs.el
+++ b/lisp/vc/vc-cvs.el
@@ -1073,7 +1073,7 @@ vc-cvs-dir-status-files
     (if (and (not files) local (not (eq local 'only-file)))
        (vc-cvs-dir-status-heuristic dir update-function)
       (if (not files) (setq files (vc-expand-dirs (list dir) 'CVS)))
-      (vc-cvs-command (current-buffer) 'async files "-f" "status")
+      (vc-cvs-command (current-buffer) 'async dir "-f" "status")
       ;; Alternative implementation: use the "update" command instead of
       ;; the "status" command.
       ;; (vc-cvs-command (current-buffer) 'async