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

[Göktuğ Kayaalp]

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 :)

>
> On 28.08.2016 23:17, Göktuğ Kayaalp wrote:
>> Hi, attached is a patch that fixes bug#24082.
>
> Could you help me reproduce the issue locally first?
>
> Either by pointing to a publicly available CVS repo with submodules, or 
> by providing a sequence of commands that would create such repo locally 
> (we do something like that in vc-tests.el).

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

    $ cd /tmp
    $ mkdir cvsroot
    $ cvs -d /tmp/cvsroot init
    $ mkdir -p test/subdir
    $ touch test/testfil test/subdir/subfil
    $ cd test/
    $ cvs -d /tmp/cvsroot/ import $(basename $(pwd)) $(whoami) start
    $ cd ../
    $ mv test test.bkp
    $ cvs -d /tmp/cvsroot/ co test
    $ echo test > test/testfil 
    $ echo test > test/subdir/subfil 

Then, in Emacs, run [ C-x v d /tmp/test RET ], and on ‘subfil’, hit ‘=’
(i.e. ask for a diff).

>> The reason of the bug was that, in function ‘vc-cvs-after-dir-status’,
>> the CVS status line ‘cvs status: Examining <dir>’ was excluded when the
>> function narrows to match, and when it tries to set the local variable
>> ‘subdir’, as it does not find this line, it skips setting it.  As
>> ‘subdir’ defaults to ‘default-directory’, which is previously set to
>> repo root (i.e. the argument to function ‘vc-dir’, when ‘subdir’ is used
>> to construct the relative path to file, concatenating it with the
>> already-known file base name, it returns the basename, i.e. in
>> the form ‘name.ext’, with no directory path.  This because it constructs
>> the relative path like ‘(file-relative-name basename subdir)’.
>
> Could we just fix that, to address this specific bug?

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

,----
| $ cvs -fn update -dP
| cvs update: Updating .
| M testfil
| cvs update: Updating subdir
| M subdir/subfil
`----

The ‘-n’ argument makes sure the ‘cvs update’ command does not alter in
any way the working directory (i.e. the checkout), and -f surpresses the
user CVS config (~/.cvsrc).

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.

>> The patch uses ‘cvs update’ command instead.  The implementation was
>> already there, commented out.  I enabled and corrected it.  The result
>> is more correct than the ‘cvs status’ approach, i.e. includes
>> unregistered and missing, and is faster compared to ‘cvs status’ way.
>
> If that change is mostly unrelated to the reported bug, could you send 
> it as two separate patches?

That actually is the fix.  ‘cvs status’ output does not lend itself to
being computed in a simple, reliable manner.  The patched function's
first half was the implementation of an incomplete and buggy parser for
the abovementioned command, and the other half was commented out code to
parse the output of ‘cvs update’.

> Further, do you have any inkling why the 'cvs update' implementation was 
> commented out? Did it require a particular recent version of CVS, maybe?
>
> We can ask Dan if you don't.
>

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 as I
can't see why it was commented out.  The related commits are: 798dafb4
and 769303ae, both don't say anything about why.

-- 
İ. Göktuğ Kayaalp.
http://gkayaalp.com/