bug#37215: [PATCH] vc-cvs-ignore writes absolute filenames and duplicate strings

[Wolfgang Scherer]

Am 14.02.20 um 09:33 schrieb Eli Zaretskii:
>> Cc: larsi@gnus.org, 37215@debbugs.gnu.org
>> From: Wolfgang Scherer <Wolfgang.Scherer@gmx.de>
>> Date: Fri, 14 Feb 2020 02:24:12 +0100
>>
>>>> Your assumption, that `file` is always a simple basename is wrong.
>>> Yes, but when does it make sense to have FILE not absolute and not
>>> just a basename (i.e. with leading directories)?  Do we have such use
>>> cases?
>> vc-dir-ignore with patch from #37240
> OK, but then please document this use case and how DIRECTORY is used
> in this function.  The various -ignore functions in vc.el and in
> backends assign different semantics to their DIRECTORY argument, and I
> think these (largely undocumented) differences are a source of some
> confusion and bugs in this area.
This is one of the first errors I ran into and I know a lot more now,
after doing the research.

There is a problem with describing the use case, because
*vc-cvs-append-to-ignore* has several of them. So I think it is best,
that Dmitry comes up with an overall solution for *vc-ignore* - as he
has planned - which would naturally solve this problem, too. Since CVS
is a special case, because another package, PCL-CVS, is also involved,
therefore increasing the danger that something breaks, I have put my
findings at the end for your convenience.
>
>>>   Because if that happens, the file's name will be added to
>>> .cvsignore not in DIRECTORY but in one of its subdirectories.  Would
>>> that be surprising?
>> Not for anybody familiar with CVS.
> This should be documented, IMO.  The existing documentation of
> .cvsignore is minimal, and doesn't mention several important aspects.
> For example, the fact that only basenames are allowed is only hinted
> upon, and there's no information whatsoever AFAICT whether characters
> special to wildcards can be escaped.  So I think we should provide
> this minimal information either in doc strings or at least in comments
> in the code.

As for the pattern syntax for CVS, it is following POSIX and is fully
described in the man page for glob(7) including the escape mechanism
with backslash. There is one quirk in the .cvsignore file parser, which
breaks patterns not only at lines but also at other whitespace on the
same line. It is therefore better to match a filename containing spaces
with a `?` for each space character.

As for the use cases:

Besides **vc** there is also PCL-CVS (**pcvs**), which is also part of
GNU Emacs. PCL-CVS handles strictly CVS, nothing else.

When the *vc-ignore* feature was introduced, the function
*cvs-append-to-ignore* was moved to `vc-cvs.el` and renamed to
*vc-cvs-append-to-ignore*:

```elisp
(define-obsolete-function-alias 'cvs-append-to-ignore 'vc-cvs-append-to-ignore
  "24.4")
```

1.  The sole PCL-CVS use case can be found in *cvs-mode-ignore*:

    ```elisp
    (defun-cvs-mode cvs-mode-ignore ()
      "Arrange so that CVS ignores the selected files.
    This command ignores files that are not flagged as `Unknown'."
      (interactive)
      (dolist (fi (cvs-mode-marked 'ignore))
        (vc-cvs-append-to-ignore (cvs-fileinfo->dir fi) (cvs-fileinfo->file fi)
                              (eq (cvs-fileinfo->subtype fi) 'NEW-DIR))
        (setf (cvs-fileinfo->type fi) 'DEAD))
      (cvs-cleanup-collection cvs-cookies nil nil nil))
    ```

    When *cvs-examine* is called in a CVS repository, PCL-CVS creates a
    buffer `*cvs*`, which looks simiilar to *vc-dir-mode*, e.g.:

    ```text
    Repository : /re/po/root-cvs
    Module     : check-cvs
    Working dir: /re/po/check-cvs/

    In directory .:
                  Unknown                 .cvsignore
                  Modified                data
                * Unknown                 test2.xx
    In directory sub:
                  Unknown                 sub/.cvsignore
                  Modified                sub/data
                * Unknown                 sub/test2.xx
    In directory sub/sub:
                  Unknown                 sub/sub/.cvsignore
                  Modified                sub/sub/data
                  Unknown                 sub/sub/test2.xx
    ```

    With `test2.xx` and `sub/test2.xx` marked, invoking
    *cvs-mode-ignore* by pressing `i` to ignore the marked files

    -   writes "test2.xx" into `/re/po/check-cvs/.cvsignore`
    -   and "test2.xx" into `/re/po/check-cvs/sub/.cvsignore`.

    As far as I am concerned, this use case is sufficiently documented
    both in *cvs-mode-ignore* and *vc-cvs-append-to-ignore*, which were
    written to fit together.

2.  When *vc-cvs-append-to-ignore* was imported from **pcvs**, the
    function *vc-dir-ignore* - when pressing `G` in *vc-dir-mode* -

    ```elisp
    (defun vc-dir-ignore ()
      "Ignore the current file."
      (interactive)
      (vc-ignore (vc-dir-current-file)))
    ```

    only used the current file - not all marked files - and sent the
    absolute pathname effectively to *vc-cvs-ignore*

    ```elisp
    (defun vc-cvs-ignore (file &optional _directory _remove)
      "Ignore FILE under CVS."
      (vc-cvs-append-to-ignore (file-name-directory file) file))
    ```

    As can be seen from *cvs-mode-ignore*, the invocation of
    *vc-cvs-append-to-ignore* for the case of an absolute pathname
    should have been:

    ```elisp
    (vc-cvs-append-to-ignore (file-name-directory file)
                           (file-name-nondirectory file)))
    ```

3.  Pressing `C-x v G` to invoke *vc-ignore* interactively, prompts for
    an absolute pathname FILE, which is described as:

        Normally, FILE is a wildcard specification that matches the files
        to be ignored..

    This actually works with the current implementation, if FILE does
    not contain any directory components. It does not work, if the
    default absolute pathname is sent as is (same use case as with
    *vc-dir-ignore*).

So it seems the correct solution for all three use cases requires that
*vc-dir-cvsignore* call *vc-ignore* in the same manner as
*cvs-mode-ignore* calls *vc-cvs-append-to-ignore*:

```elisp
(defun vc-dir-ignore ()
  "Ignore the current file."
  (interactive)
  (let ((fi (vc-dir-current-file)))
    (vc-ignore (file-name-nondirectory fi) (file-name-directory fi))))
```

and *vc-cvs-ignore* just passes on both DIRECTORY and FILE:

```elisp
(defun vc-cvs-ignore (file &optional directory _remove)
  "Ignore FILE under CVS."
  (vc-cvs-append-to-ignore directory file))
```

However, this would mean that all modifications so far (except maybe for
SVN) have to be mostly rolled back, which will lead to other problems.

For my own needs, which is to cover the range of Emacsen from 22 to 26
and beyond, I have prepared a standalone extension package which is
mostly independent from the:current defun:vc-ignore subsystem, It shows,
that the problem can be solved and you are welcome to refer to the
escape and anchoring mechanisms there [wolfmanx/vc-ign: Emacs VC ignore
feature](https://github.com/wolfmanx/vc-ign).