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

[Wolfgang Scherer]

Hi Eli,

I suggest, we stop talking about the use case, where `vc-ignore`
is called interactively, I will let you have your opinion,
although I strongly disagree and the unsolved problem of
interactively locating the correct ignore file makes it
ultimately hilarious:

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) .Since the escaping must
also be correct, I'm probably better off locating and editing the
ignore file manually. Inserting the output of "ls -1" and editing
away - as I sometimes do - is much more convenient than calling
`vc-ignore` multiple times and repeating more complex editing of
an absolute file path each time in the minibuffer.

However, there is a use case that you keep avoiding to comment
on, namely pressing "G" in `vc-dir-mode`, which calls
`vc-dir-ignore`, which in turn calls `vc-ignore`
**programmatically** with an absolute file path. It has been
officially supported by Emacs since 2013. I did not invent it and
I did not alter its API.

Since there is no interactive prompt whatsoever for a user to

1. properly construct an escaped pattern and
2. specify the directory, where the search for the ignore file
   should start,

the function called to ignore the file must consequently escape
and anchor it properly, just like any command that uses
`shell-quote-args` because the use case requires it and the
purpose of the argument is well-known.

How do you plan to support this use case? (For a humorous
suggestion, see below).

According to your opinion `vc-ignore` can only be used
**interactively** by a learned expert user, who wants to make
ignore file administration even more complicated by editing
random ignore files without any feedback.

So it is the wrong candidate for a noob user of `vc-dir-mode`
who just wants to ignore the selected files literally without
worrying about ignore files and escaping. 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`.

So, if you do not plan to drop this feature from `vc-dir-mode`
altogether, then a new function must be implemented, which does
exactly what my implementation does. Therefore, it only fixes the
broken behavior in `vc-dir-mode`.

If you are just worried, that somebody will use `vc-ignore` and
expect the old broken behavior, I suggest to accept my
implementaton and add a custom variable
`vc-ignore-posixly-correct` with a default value of `t` to prefer
the same behavior as before (see below for a draft
implementation).

I don't think it is really necessary, since I removed interactive
from `vc-ignore` and `C-x v G` calls `vc-ignore-pattern` which
essentially behaves like the old `vc-ignore` minus the real bugs.
So a user would only realize the API change when executing `M-x
vc-ignore`.

Am 10.02.20 um 17:02 schrieb Eli Zaretskii:
>> From: Wolfgang Scherer <Wolfgang.Scherer@gmx.de>
>> Cc: dgutov@yandex.ru, 37189@debbugs.gnu.org
>> Date: Sun, 9 Feb 2020 14:57:12 +0100
>>
>>     
>> +-------------+-----------------+---------------+-----------------+--------------------+
>>     | `file path` | glob(7)         | anchored glob | Hg `regex`      | Bzr 
>> `regex`        |
>>     
>> +=============+=================+===============+=================+====================+
>>     | test[56].xx |   test\[56].xx  | /test\[56].xx | ^test\[56]\.xx$ | 
>> RE:^test\[56]\.xx$ |
>>     |             |   test[[]56].xx |               |                 |      
>>               |
>>     
>> +-------------+-----------------+---------------+-----------------+--------------------+
>>     | simple.txt  |   simple.txt    | /simple.txt   | ^simple\.txt$   | 
>> RE:^simple\.txt$   |
>>     |             |   simple[.]txt  |               |                 |      
>>               |
>>     
>> +-------------+-----------------+---------------+-----------------+--------------------+
> Just a note: this table might be interpreted to mean that hg and bzr
> only support regexes in their ignore files, but that is of course
> false: they also support glob-style widlcards.
This is not an exhaustive table of ignore specification
syntaxes. It is meant to illustrate that the case

  file path === ignore specification

is really the exception.
>>     The correct escaping of FILE can only be determined by the
>>     backend. Therefore neither vc-dir-ignore nor lisp code calling
>>     vc-ignore can escape the FILE parameter correctly without support
>>     from the backend. This makes pattern input for FILE only useful
>>     during interactive calls.
>>
>> Even, if it was magically possible to determine the correct
>> pattern in the frontend, submitting an anchored
>> glob "/some-sub/file.txt" to `vc-ignore` would be interpreted as
>> an absolute path.
>>
>> In other words, the API specificaton
>>
>>   [...] FILE is a wildcard specification, either relative to
>>   DIRECTORY or absolute.
>>
>> which asks for implementing the pattern use case inextricably
>> mixed with the file path use case, is nonsense.
>>
>> It also means, that all of the backend functions which currently
>> demand a pattern are absolutely useless.
> I don't think I agree.  While the direction in which you propose to
> move -- which AFAIU is to offer 2 different commands, one to ignore a
> file name, the other to ignore a file pattern -- is definitely
> possible,

(Sorry, I did not have time to clean the ReStructured Text source, it is 
rendered in HTML at 
http://sw-amt.ws/emacs/doc/_build/html/emacs-vc-ignore-feature.html#implementation-of-vc-ignore-parameter-extension)

The actual implementation is in a single function

:defun:`vc-ignore`. The use cases "pattern" and "file path" are
selected with the flag AS-IS.

For interactive use, there are 2 separate commands
:defun:`vc-ignore-file` and :defun:`vc-ignore-pattern`, both of which
just call :defun:`vc-ignore` with a different value for the flag
AS-IS.

This decision was made, because the prefix argument as usual choice to
specify a flag parameter interactively is already appropriated for the
flag REMOVE. Instead of an interactive y/n question for each
invocation - which is pretty annoying - 2 separate functions also
facilitate keymap definitions. It also has the advantage that only the
relevant information for each use case can be presented in the doc
string.

Granted that typing :kbd:`C-x v F` instead of :kbd:`C-x v G` or
:kbd:`F` instead of :kbd:`G` is also a decision, but it does not
necessarily have to be made each time, especially if a user only cares
about a single use case. That concludes the argument for 2 separate
interactive commands as user interface.

The implementation consists of a linear call chain with two short code
path variations. Besides backend specific implementations for some
functions, there are **never** 2 separate functions in the entire
chain.

.. uml::
   :caption: :defun:`vc-ignore` call chain

    @startuml
    participant "vc-default-\nignore" as VCDI
    participant "vc-default-\nget-ignore-file-\nand-pattern" as VCDIAP
    participant "vc-backend-\nignore-param" as VCBIP
    participant "vc-backend-\nfind-ignore-file" as VCBFIF
    participant "vc-default-\nmod-ignores" as VCDMI

    VCDI     ->  VCDIAP : (vc-call-backend FILE-OR-PATTERN\n DIRECTORY AS-IS 
REMOVE)
    alt AS-IS non-nil
      VCDIAP ->  VCDIAP : absolute FILE-PATH\nnormalize DIRECTORY
    end
    VCDIAP   ->  VCBFIF : (vcb DIRECTORY)
    VCDIAP   <-- VCBFIF :IGNORE-FILE
    alt AS-IS nil
      VCDIAP ->  VCDIAP : null ignore spec processing parameters
    else AS-IS non-nil
      VCDIAP ->  VCDIAP : relative FILE-PATH
      VCDIAP ->  VCBIP : (vcb IGNORE-FILE)
      VCDIAP <-- VCBIP : ignore spec processing parameters
    end
    VCDIAP   ->  VCDIAP : escape/anchor pattern
    VCDI     <-- VCDIAP : '(IGNORE-FILE PATTERN REMOVE)
    VCDI     ->  VCDMI : (vcb IGNORE-FILE PATTERN REMOVE)
    VCDI     <-- VCDMI
    @enduml

The code path variations based on the AS-IS flag are confined to
:defun:`vc-default-get-ignore-file-and-pattern`.

First, if AS-IS is nil, the FILE-OR-PATTERN argument is expanded to an
absolute FILE-PATH. DIRECTORY is set to the immediate parent directory
of :elisp:`(vc-no-final-slash FILE-PATH)`: This normalization is
necessary, because the search for an ignore file starts at DIRECTORY.

.. code-block:: elisp

   (when (not as-is)
     (setq file-or-pattern (expand-file-name file-or-pattern directory))
     (setq directory (file-name-directory (vc-no-final-slash file-or-pattern))))

Second, if AS-IS is non-nil, the parameters for escaping and anchoring
an ignore pattern are set to an identity function.

If AS-IS is nil, FILE-PATH is made relative to the path of the
directory where the pattern will be stored.  Parameters for escaping
and anchoring an ignore pattern are obtained from the VC backend.

.. code-block:: elisp

   (if as-is
       (setq ignore-param vc-ignore-param-none)
     (when (not (string= (substring file-or-pattern 0 (length ignore-dir))
                         ignore-dir))
       (error "Ignore spec %s is not below project root %s"
              file-or-pattern ignore-dir))
     (setq is-dir (or (file-directory-p file-or-pattern)
                      (vc-has-final-slash file-or-pattern)))
     (setq file-or-pattern (vc-no-final-slash
                            (substring file-or-pattern (length ignore-dir))))
     (setq ignore-param (vc-call-backend backend 'ignore-param ignore-file)))

E.g. an invocation of :elisp:`(vc-ignore "some/rel/path/" "/re/po")`
translates to:

.. code-block:: elisp

   ;; normalize FILE-PATH and DIRECTORY
   (setq file-or-pattern "/re/po/some/rel/path/" )
   (setq directory "/re/po/some/rel/" )

   ;; determine, whether file path is a directory
   (setq is-dir t)
   ;; prepare pattern as relative file path to directory of ignore file
   (setq file-or-pattern "path" )

   ;; obtain parameters for escaping and anchoring an ignore pattern from VC 
backend

All further processing is identical for verbatim patterns and for file paths.

If you insist on calling a code path difference of 3 lines versus 13
lines two independent functions, then technically you are correct and
we should just remove the extra 13 lines of code (and also the two
separate interactive commands) to get a **properly working**
implementation of the pattern use case for **all** VC backends, which
is still needed, because right now, the vc ignore feature is
incomplete and very buggy.
>  I question its necessity.

>   In the use cases I have in mind,
> the ignore file-or-pattern always comes from the user, because only
> the users can decide which files they want to ignore.
These are my major use cases. I do not have a real use case involving Lisp 
code, but it is always worth considering.
>   And the users
> always know both which backend they are working with, and whether the
> file-or-pattern is a filename or a pattern.
Yes, I do. 99.9% of the time I want to ignore one or more files in 
`vc-dir-mode` or `dired-mode`
>   It follows that we can
> ask the users to escape and anchor the file-or-pattern argument they
> type,

Are you aware, that the current implementation of
`vc-dir-ignore` calls `vc-ignore` with a plain absolute file
path obtained from (vc-dir-current-file)?  There is no prompting
whatsoever that would let the user escape and anchor anything.

How would you propose to handle this officially supported use
case (since 2013)?

   (defun vc-dir-ignore (&optional arg)
     "Ignore the current file."
     (interactive "P")
     (vc-ignore
      (if (called-interactively-p 'any)
      (read-string "Make the path relative to the ignore file,
    which may or may not be at the location you expect. Then guess
    the currently active pattern escape syntax, apply it to the file
    path and anchor it: " (vc-dir-current-file))
    (error "This will fail, when called from lisp code."))))

>  and that is not an unreasonable expectation.
If you analyze the above humorous prompt, you will see, that it
is entirely unreasonable, since it is no longer possible to
determine the correct ignore file for a given pattern, unless the
DIRECTORY parameter is also correct, which is always necessary for
CVS, SVN, SRC. So before calling `vc-ignore` I have to change
into the correct directory?
           
1. Locating the correct ignore file is no simple task for Git or
   Hg, which support sub-tree ignore files. The current
   implementation via

   : (expand-file-name ".hgignore" (vc-root-dir))

   is extremely simplistic and should be augmented in a separate
   effort.

2. The current syntax in an Hg ignore file may be either regex
   or glob. Since we are in a collaborative environment, after
   all, anybody may have changed the default syntax since we last
   checked.

3. Nobody knows all ignore spec syntaxes all the time (memory is
   always fading).

4. If I have to contribute to a project that uses an unfamiliar
   VC, it would be extremely reasonable, if my development
   environment (Emacs) would spare me the trouble of totally
   avoidable "ignore file" mistakes. It would be so extremely
   reasonable, that it would be actually unreasonable not to have
   it implemented.

5. I have users, which are only just learning how to program. I
   do not wish for them to delve into just another distraction
   from the task at hand. It would be very reasonable, if Emacs
   helped me keep them on point.
   ("Don't worry, Emacs will handle that for you!")
>   In fact, your
> approach expects the same from the users, because you are asking the
> users to decide which of the two commands to invoke in each case.
I handled that above. If you only ever add files from
`vc-dir-mode` or `dired-mode` you only have to make that decision
once in your lifetime.
> If there are no flaws in my way of reasoning, then I think the
> resulting changes will be much less extensive, because the same
> vc-ignore command can then be used for both ignoring a specific file
> and for ignoring a pattern of any kind.

I can give you that with my implementation as a bonus added interactive command:

   (defun vc-ignore-posixly-correct (file &optional directory remove as-is)
     (interactive
      (let ((remove current-prefix-arg) file as-is)
    (if (not (y-or-n-p "Ignore a file?"))
        (setq file (read-string "Pattern: "))
      (setq as-is (not (y-or-n-p "Escape manually?")))
      (setq file (read-file-name "File: ")))
    (list file nil remove as-is)))
     (vcignore file directory remove as-is))

>   We just need to document that
> if the argument is a pattern of any kind, the user will have to make
> sure it is escaped and anchored for the backend to DTRT.
>
> A further advantage of my proposal is that we don't need to write any
> backend-specific code to escape special characters in patterns,
> because we expect the users to do that.
Since we have to check for the active syntax anyway, it may be
better to abandon the entire ignore feature and just open the
appropriate ignore file for editing. It is a lot simpler and much
more convenient than the minibuffer for editing multiple files.
It would also help the user to determine what the correct
relative file name should be.
>
> This concept is similar to what we do in commands such as "M-x grep",
> where we expect the users to escape special characters in the
> command-line arguments to be passed to Grep the program via the shell.
That is not at all comparable, since there is no use case "search
for file path in a bunch of files". If such a command
`grep-for-file-name` were derived from `grep`, a user could
reasonably expect that the file argument is properly quoted as a
grep pattern.

The correct comparison would be with various employments of
`shell-quote-argument`, when passing a string to the shell, which
is known to be interpreted verbatim. Or as just recently reported
in bug #35492, where file arguments for Git commands must be
properly quoted to work correctly (this is not because of the
shell but because of Git!). If this quoting is omitted,
`vc-dir-mode` becomes entirely unreliable for Git. If it is done
quick and dirty instead of properly, it causes other problems.
> Am I missing something?