bug#37189: 25.4.1: vc-hg-ignore implementation is missing

[Wolfgang Scherer]

Am 12.02.20 um 19:34 schrieb Eli Zaretskii:
>> From: Wolfgang Scherer <Wolfgang.Scherer@gmx.de>
>> Date: Tue, 11 Feb 2020 23:28:10 +0100
>>
>> I do not actually give up:-). I just think it is easier to
>> convince you that the "file path" use case is already supposed to
>> be supported by Emacs.
> What is the "file path use case"?  To what commands does it pertain?

The command is vc-dir-ignore. You already responded with:

> Since vc-dir-ignore computes the file name(s) to add to the ignore
> file, it indeed will need to escape all the special characters in file
> names it will add, before it invokes vc-ignore.  You are right here.

Let me just remind you, that the key shortcut in vc-dir-mode dates back to 2013:

    vc-dir.el (Xue Fuqiao 2013-07-30  305)     (define-key map "G" 
'vc-dir-ignore)

It is not new, and the intention was always to support ignoring files
from vc-dir-mode wihtout escaping before calling vc-ignore, which is
really not necessary at all.

>>>> A command that locates an ignore file, but can only do so, if the
>>>> default-directory is already the one containing the ignore
>>>> file (always true for SRC, CVS, SVN)
>>> Is locating the ignore file a separate issue?  AFAICT, we currently
>>> always look for the ignore file in the repository root, is that right?
>>> If so, are you proposing a new feature here, where we would support
>>> ignore files in subdirectories?
>> I am not (yet) talking about a new feature, but about the "file path"
>> use case in *vc-dir-mode* for per-directory VCSs (CVS, SVN, SRC).
> Do we support per-directory ignore files in the current codebase?  I
> think we don't: I see that every backend simply looks in the root of
> the repository.

That is only true for per-tree VCs, because per-directory VCs do not
necessarily have a "root":

    vc-bzr.el681:(defun vc-bzr-find-ignore-file (file)
                    (vc-bzr-root file)))
    vc-git.el974:(defun vc-git-find-ignore-file (file)
                    (vc-git-root file)))
    vc-hg.el1210:(defun vc-hg-find-ignore-file (file)
                    (vc-hg-root file)))
    vc-mtn.el105:(defun vc-mtn-find-ignore-file (file)
                    (expand-file-name ".mtnignore" (vc-mtn-root file)))

You are missing the higher priority backend hook vc-backend-ignore,
which is used by CVS and SVN. The original implementations (before my
changes) are very pristine.

The oldest stuff - the CVS code - was mainly imported from pcl-cvs
(vc-cvs-append-to-ignore), which has always supported the "file path"
use case. In the thin vc-cvs-ignore wrapper the use case is also very
much "file path", albeit incomplete. I.e., for an absolute file path,
the correct .cvsignore is found, which can very well be a sub
directory of the repository root. If the FILE argument were made
relative to the effective directory, the code would actually be
correct. See also bug #37215.

    vc-cvs.el (Glenn Morris 2013-09-11 1223) (defun vc-cvs-ignore (file 
&optional _directory _remove)
    vc-cvs.el (Xue Fuqiao   2013-07-30 1224)   "Ignore FILE under CVS."
    vc-cvs.el (Xue Fuqiao   2013-09-20 1225)   (vc-cvs-append-to-ignore 
(file-name-directory file) file))
    vc-cvs.el (Xue Fuqiao   2013-07-30 1226)
    vc-cvs.el (Xue Fuqiao   2013-09-20 1227) (defun vc-cvs-append-to-ignore 
(dir str &optional old-dir)
    vc-cvs.el (Xue Fuqiao   2013-07-30 1228)   "In DIR, add STR to the 
.cvsignore file.
    vc-cvs.el (Xue Fuqiao   2013-07-30 1229) If OLD-DIR is non-nil, then this 
is a directory that we don't want
    vc-cvs.el (Xue Fuqiao   2013-07-30 1230) to hear about anymore."
    vc-cvs.el (Xue Fuqiao   2013-07-30 1231)   (with-current-buffer
    vc-cvs.el (Xue Fuqiao   2013-07-30 1232)       (find-file-noselect 
(expand-file-name ".cvsignore" dir))
    vc-cvs.el (Xue Fuqiao   2013-07-30 1233)     (when (ignore-errors
    vc-cvs.el (Xue Fuqiao   2013-07-30 1234)        (and buffer-read-only
    vc-cvs.el (Xue Fuqiao   2013-07-30 1235)             (eq 'CVS (vc-backend 
buffer-file-name))
    vc-cvs.el (Xue Fuqiao   2013-07-30 1236)             (not (vc-editable-p 
buffer-file-name))))
    vc-cvs.el (Xue Fuqiao   2013-07-30 1237)       ;; CVSREAD=on special case
    vc-cvs.el (Xue Fuqiao   2013-07-30 1238)       (vc-checkout 
buffer-file-name t))
    vc-cvs.el (Xue Fuqiao   2013-07-30 1239)     (goto-char (point-max))
    vc-cvs.el (Xue Fuqiao   2013-07-30 1240)     (unless (bolp) (insert "\n"))
    vc-cvs.el (Xue Fuqiao   2013-07-30 1241)     (insert str (if old-dir "/\n" 
"\n"))
    vc-cvs.el (Glenn Morris 2013-09-11 1242)     ;; FIXME this is a pcvs 
variable.
    vc-cvs.el (Glenn Morris 2013-09-11 1243)     (if (bound-and-true-p 
cvs-sort-ignore-file)
    vc-cvs.el (Glenn Morris 2013-09-11 1244)         (sort-lines nil 
(point-min) (point-max)))
    vc-cvs.el (Xue Fuqiao   2013-07-30 1245)     (save-buffer)))

The SVN implementation is somewhat confusing, because for the "file
path" use case a relative path with possible subdirectories is
constructed, but not normalized.

    vc-svn.el (Dmitry Gutov 2014-10-03 354) (defun vc-svn-ignore (file 
&optional directory remove)
    vc-svn.el (Xue Fuqiao   2013-08-04 355)   "Ignore FILE under Subversion.
    vc-svn.el (Xue Fuqiao   2013-09-04 356) FILE is a file wildcard, relative 
to the root directory of DIRECTORY."
    vc-svn.el (Dmitry Gutov 2014-10-03 357)   (let* ((ignores 
(vc-svn-ignore-completion-table directory))
    vc-svn.el (Dmitry Gutov 2014-10-03 358)          (file (file-relative-name 
file directory))
    vc-svn.el (Dmitry Gutov 2014-10-03 359)          (ignores (if remove
    vc-svn.el (Dmitry Gutov 2014-10-03 360)                       (delete file 
ignores)
    vc-svn.el (Dmitry Gutov 2014-10-03 361)                     (push file 
ignores))))
    vc-svn.el (Dmitry Gutov 2014-10-03 362)     (vc-svn-command nil 0 nil nil 
"propset" "svn:ignore"
    vc-svn.el (Dmitry Gutov 2014-10-03 363)                     (mapconcat 
#'identity ignores "\n")
    vc-svn.el (Dmitry Gutov 2014-10-03 364)                     
(expand-file-name directory))))

Since I never thought of a "pattern" use case, I submitted a fix for
SVN, which adds the basename to the correct sub directory:

    vc-svn.el (Wolf Scherer 2019-10-07 357) FILE is a wildcard specification, 
either relative to
    vc-svn.el (Wolf Scherer 2019-10-07 358) DIRECTORY or absolute."
    vc-svn.el (Wolf Scherer 2019-10-07 359)   (let* ((path (directory-file-name 
(expand-file-name file directory)))
    vc-svn.el (Wolf Scherer 2019-10-07 360)          (directory 
(file-name-directory path))
    vc-svn.el (Wolf Scherer 2019-10-07 361)          (file 
(file-name-nondirectory path))
    vc-svn.el (Wolf Scherer 2019-10-07 362)          (ignores 
(vc-svn-ignore-completion-table directory))

I concede, that for simple glob(7) expressions, it is pretty much
possible to get away with treating the wildcard specification as a
filename. And sure, by declaring a use case *rare* (although there are
infinitely many instances and one is enough to trigger an error) the
need for escaping literal filenames can be argued away.

However, with regular expression syntax, multiple syntax options and
sub-tree ignore files those times are long gone.

>   If I'm right, then supporting per-directory ignore
> files is an enhancement, which should then be considered separately
> from fixing bugs in the current code.

Per-directory means specifically, that the scope of the ignore
specification is the directory of the ignore file. And that has
always been supported.

The per-sub-tree ignore files are a feature of Git and Hg, where the
scope of the ignore specifications also extends into further
subdirectories. These are not supported and should definitely not be
discussed here.

>> E.g., in this *vc-dir-mode* buffer it is not possible to use *vc-ignore*
>> in "pattern" mode:for adding an ignore pattern for SVN to `sub/`
>> or for SVN or CVS in `sub/sub2/`:
>>
>> ``` {.sourceCode .text}
>> VC backend : SVN
>> Working dir: /the/top
>>
>>                   .svn
>>                   sub/CVS
>>    edited         sub/.cvsignore
>>                   sub/sub2/CVS
>>                   sub/sub2/RCS
>>    edited         sub/sub2/.cvsignore
>> ```
>>
>> In case of nested repositories with different VC backends (e.g. in
>> directories `sub` and `sub/sub2` above), it may even become necessary to
>> invoke *vc-dir* with a prefix argument to specify the appropriate VC
>> manually.
> I don't think we support such nesting at this time, do we?

Well, we actually do. With the "file path" use case (as implemented in
vc-svn-ignore) it is perfectly possible to just press "G" on say
"sub/something" or "sub/sub2/else" and they will be ignored for SVN
correctly.

The VC backend nesting is also supported perfectly (I was
over-dramatizing for effect, sorry). The variable vc-handled-backends
determines what backends are recognized and what their priority
is. So, to solve the problem of using "pattern" mode in the above
example of nested repositories,

   (setq vc-handled-backends '(SVN CVS RCS))

is sufficient to suppress recognition of the inner VC backends. Then
you could "pattern" away for SVN to your hearts content.

>>> Since vc-dir-ignore computes the file name(s) to add to the ignore
>>> file, it indeed will need to escape all the special characters in file
>>> names it will add, before it invokes vc-ignore.  You are right here.
>> In order to know, what part of the filename must be used, it is
>> necessary to know the location of the ignore file, which is only known
>> by the backend.
> If I'm right in saying that we currently support only ignore files in
> the root of the repository, then this is not an issue.

Since you are not right, that could have been the end of it ;-)

> And even if it
> is an issue, we already have a backend method to find the ignore file,
> so we could use it (or modify it if needed).

You obviously have not looked at the implementation yet. I introduce
all the missing backend methods to find ignore files (CVS, SRC, SVN
(virtual, just for directory determination)) to make the
implementation totally uniform.

If you use these methods in a front end, you obviously have to
determine the backend. And after locating the ignore file, vc-ignore
can no longer be called, because neither the backend, nor the
ignorefile can be specified. So you have to call vc-default-ignore
with the backend. You are actually programming a backend function now
and much of the frontend/backend separation goes out the window.

Then you would have to cut and paste the same code into other
functions that also need to do the same thing. Then you will realize,
that it is better to move the stuff to the backend default
implementations where it really belongs. And so you will just end up
with the optimized implementation that I am offering ;-)

>>>> They also expect a visual feedback, that the operation had the
>>>> desired effect, as they have come to expect from all the other
>>>> commands in `vc-dir-mode`.
>>> AFAICT, the command does provide feedback.  Or maybe I misunderstand
>>> what feedback you had in mind.
>> vc-dir-ignore currently does not provide the normal feedback, I only just 
>> now put it in:
>>
>>       (vc-dir-resynch-file file)
> OK, so you _did_ mean a different kind of feedback.  What I meant is
> the "/foo/bar/.ignore written" feedback.

I repeated that in the vc-ignore pattern prompt, where it is much more
useful to know, which ignore file is actually used ;-)

>>> The old behavior of vc-ignore was not broken for interactive
>>> invocations.  It was broken (in rare cases) for invocations from
>>> vc-dir-ignore, and that can IMO be fixed without affecting user-facing
>>> behavior.  So I see no backward-incompatible changes here.
>> Sorry, not *rare* but **all** cases! The invocation from vc-dir-ignore
>> placed an **absolute file path** into the ignore file.
> That's not what I see, at least not with Git as the backend.  I see
> only the basename of the file being added to the ignore file.

Right, and that is incorrect for all cases, because it is

1. not anchored, i.e.it also matches files with the same name in
   subdirectories,
2. if the basename comes from a file in a subdirectory, it also
   matches files with the same name in the root directory.

These reasons also apply to Bzr,Hg, Mtn, since the same functions are
used. BTW, you are seeing these incorrect filenames so flawlessly,
because I fixed the underlying functions in #37185 extensively.

Originally, SVN at least got it right for files in the root directory,
but none of the ignore specs for files in a subdirectory ever matched
anything, which is probably better than matching the wrong thing..

> Can you
> show a use case where an absolute file name is written into the ignore
> file by vc-dir-ignore?

Yes, CVS is still broken in that manner, because the reviewer of
#37215 thinks, that FILE is always a basename only.

And there we are, *all* backend implementations fail utterly for
vc-dir-ignore. Not just *rare*, but *almost all* cases (except root
directory of SVN repo).

>>> My point is that Grep patterns can include characters special for the
>>> shell, but we never escape them ourselves, we rely on the user to
>>> escape them as needed.
>> The pattern is not prompted for separately!
> True, but I don't think this detail matters for the purposes of the
> analogy.
>> There is no way, that the pattern argument for the *grep* command
>> could be reliably parsed and quoted.
> Of course it's possible.

No it is not! Without quoting, you cannot decide programmatically
where the argument ends and the first filename begins, if the argument
contains spaces (or a multitude of other characters). I will not
accept "machine learning" as an answer ;-).

> It's just very complex and tedious, and more
> importantly, requires the user to play by certain rules: e.g., the
> user must agree never to quote/escape the patterns he/she types.
> Instead, we give the user the freedom to decide when to quote and
> when not to quote.  The same should be done with vc-ignore
> (but not with vc-dir-ignore).

Aaah, so! It's not a *bug*, it's a *feature*! ;-)

Anyway, that is exactly the point I am trying to make.

As it is and always will be, vc-ignore does what it does and I am not
changing it.  I am just adding another option, a freedom of choice, if
you will. Invoke vc-ignore under a different name, supply a file path
and magically, the correct ignore file is located and the proper
quoting is applied. This enables vc-dir-mode to work correctly.

And as I said before, once you think it through you will end up at the
same conclusion: It is always best to have a single call chaing
that covers all cases. If it works, it works for all of them. No need
to maintain multiple cut and paste code snippets in various frontend
functions and backends. (The confusion with the old vc-cvs-ignore and
vc-svn-ignore that you have overlooked previously shows that).

I have implemented some options for prompts from vc-ignore in
vc-dir-mode (the completion collection contains an empty line and the
unquoted file name).

    VC backend : Hg
    Working dir: /srv/install/linux/emacs/

        edited               README-emacs-vc-ignore-feature.txt

                             doc/_static/
        unregistered         doc/_static/x-vc-repair.el-002
        unregistered         doc/_static/x-vc-repair.el-003

Pressing "G" on the line "doc/_static/x-vc-repair.el-002" displays:

    Add pattern verbatim to .hgignore: ^doc/_static/x\-vc\-repair\.el\-002$

Pressing :kbd:`M-n` or :kbd:`<down>` clears the prompt:

    Add pattern verbatim to .hgignore:

Pressing :kbd:`M-n` or :kbd:`<down>` shows the unquoted file name:

    Add pattern verbatim to .hgignore: doc/_static/x-vc-repair.el-002

Invoking :kbd:`C-x v G` in subdirectory "doc/_static" of the
repository prompts with:

    Add pattern verbatim to ../../.hgignore:

Which tells you where the pattern will go when you press ENTER.

How is that for freedom of choice?