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

[Wolfgang Scherer]

Am 13.02.20 um 20:36 schrieb Eli Zaretskii:
>> From: Wolfgang Scherer <Wolfgang.Scherer@gmx.de>
>> Date: Thu, 30 Jan 2020 20:44:00 +0100
>> Cc: 37215@debbugs.gnu.org
>>
>>>> -(defun vc-cvs-ignore (file &optional _directory _remove)
>>>> -  "Ignore FILE under CVS."
>>>> -  (vc-cvs-append-to-ignore (file-name-directory file) file))
>>>> +(defun vc-cvs-ignore (file &optional directory _remove)
>>>> +  "Ignore FILE under CVS.
>>>> +FILE is either absolute or relative to DIRECTORY."
>>>> +  (setq file (directory-file-name (expand-file-name file directory)))
>>>> +  (vc-cvs-append-to-ignore (file-name-directory file) 
>>>> (file-name-nondirectory file)))
>>> This is basically
>>>
>>> (file-name-nondirectory (directory-file-name (expand-file-name "foo" 
>>> directory)))
>>>
>>> isn't it?
>> It is for `file` equal to "foo" (a simple basename).
>>>   In what circumstances does that evaluate to something other
>>> than "foo"?
>> If "foo" is something other than a simple basename (see below).
>>>   That is, what DIRECTORY is doesn't seem to matter, if I'm
>>> reading this right?
>> 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
>   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. (Any other old-timers that can chime in 
here?)
>   And if so, perhaps we should warn about that or
> even error out?
Certainly not.

Here is a long explanation of what is going on:

The latest patch to  vc-dir-ignore  (#37240) uses ewoc to get a list
of marked files.  The files in this list are relative file pathes and
can also be in subdirectories. e.g.:

   VC backend : CVS
   Working dir: /re/po

                ./
       unregistered         .cvsignore
       edited               data
                sub/
       unregistered         sub/.cvsignore
   *   unregistered         sub/sub-file
                sub/sub/
       unregistered         sub/sub/.cvsignore
       edited               sub/sub/data

Pressing  G  while "sub/sub-file" is marked, initiates the call chain
 vc-dir-ignore  =>  vc-ignore  =>  vc-cvs-ignore  with "sub/sub-file"
as FILE argument.

CVS has per-directory ignore files, which can only handle simple
filenames without directory parts. I.e,, it is incorrect to write
"sub/sub-file" into "/re/po/.cvsignore". Instead "sub-file" must be
written into "/re/po/sub/.cvsignore" to have the desired effect.

Therefore, it is necessary to expand the FILE parameter to

   (directory-file-name (expand-file-name "sub/sub-file" "/re/po"))
   => "/re/po/sub/sub-file"

And the resulting absolute file path can be used to determine the
ignore file, which is located in the directory:

   (expand-file-name ".cvsignore" (file-name-directory "/re/po/sub/sub-file"))
   => "/re/po/sub/.cvsignore"

and the string that must be written to it is the basename:

   (file-name-nondirectory "/re/po/sub/sub-file") => "sub-file"

So, the algorithm is correct and I assume that anybody familiar with
CVS would expect just that behavior.

Citing from the CVS texinfo manual:

      CVS has a list of files (or sh(1) file name patterns) that it should
   ignore while running 'update', 'import' and 'release'.  This list is
   constructed in the following way.

      * The list is initialized to include certain file name patterns:
        [...]

      * As CVS traverses through your directories, the contents of any
        '.cvsignore' will be appended to the list.  The patterns found in
        '.cvsignore' are only valid for the directory that contains them,
        not for any sub-directories.