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

[Dmitry Gutov <dgutov@yandex.ru>]

[-- Attachment #1: Type: text/plain, Size: 4159 bytes --]

On 14.02.2020 11:23, Eli Zaretskii wrote:
>> Cc: 37189@debbugs.gnu.org
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Fri, 14 Feb 2020 01:40:31 +0200
>>
>> I think the first thing I'll have to do is revert a part of an earlier
>> patch to vc-default-ignore which changed its semantics a little, because
>> it doesn't look like this discussion is going to culminate in a patch
>> small enough for emacs-27 anyway.
> 
> Which part?  Can you show a proposed patch?

See https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37217.

The patch fixed a legitimate scenario, but made an existing one work 
worse. Trying to vc-ignore, say, '*.c' from a subdirectory in a Git 
repo, for instance, will now prepend the intermediary directories to it.

(Mentioned this before: 
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37189#41)

Now, one could argue that one use case is more important than the other 
one, and that vc-ignore has for a while been geared toward entering 
plain file names rather than glob patterns. I have little opinion on 
this subject, however, since I mostly edit ignore files by hand, and do 
so very rarely. So as far as I'm concerned, we could remove this feature 
altogether and not lose much.

But maybe it's more valuable for SVN users? Where the ignore 
configuration is more tricky and the DIRECTORY argument is actually 
important.

So the options at hand are:

- Revert almost all of the patch from bug#37217, reverting to the 
previous, admittedly broken behavior, and continue to discuss a better 
improvement for Emacs 28.
- Try to resolve the ambiguity of purpose in favor either entering 
patterns on file names only. Probably the latter, later vc-ignore to 
vc-ignore-file.
- Try to sit on both chairs... Basically, that means using the user 
input unaltered. Allowing them to enter a file name as well, but treat 
it as a pattern, without escaping or the like. This would be close to 
the original intent behind vc-ignore, AFAICT.

To do the last one, read-file-name would need to be called with the 
second argument provided, the directory against which the file path 
should be relative. For most backend, we can reuse the find-ignore-file 
backend command, but SVN (and RCS, etc) don't have it defined. Roughly 
and handwavy, we can take this case to mean "use default-directory".

As you noted, the use of read-file-name at all in vc-ignore is somewhat 
problematic, but let's see if we can keep the function sane without 
removing it first.

Attaching a patch. Eli, Wolfgang, any objections?

>> Then, naturally, we'll have to look for small changes that improve the
>> situation but provide as little breakage as possible.
> 
> Agreed.

Alas, the attached patch is probably not a "small" one. Option 1 would 
pass this criterion, though.

> 1) vc-dir-ignore:
> 
> It calculates the file name for the entry into the ignore file, and
> thus must escape any characters in the file name that are special in
> ignore files -- which is backend-specific.
> 
> The file name should also be "anchored", at least ideally, so that no
> other file is accidentally ignored.  This is also backend-specific.

I think we can add a new backend action that would escape and anchor a 
file name (and maybe turn it from an absolute into a relative one). That 
should take care of it.

> VC's support for directory-specific ignore files is inconsistent: they
> are supported for CVS (and actually mandatory there) and maybe SVN,
> but not for Git/Bazaar/Mercurial/Monotone -- for the latter we only
> support ignore files in the repository root.  This could be improved
> in the future, but for now I don't see an immediate need.

Agreed.

> For the same reason, using read-file-name here is too naïve, as well
> as yielding an absolute file name from what the user types.

For now, I kept the former but decided against the latter.

> If you agree with the above, we then need to decide what, if anything,
> of this should be fixed for Emacs 27.  E.g., vc-dir-ignore seems to be
> OK if the file name doesn't include special characters, does it?

With the attached patch, it needed a small adjustment, but with that 
should work okay-ish again.

[-- Attachment #2: vc-ignore.diff --]
[-- Type: text/x-patch, Size: 3346 bytes --]

diff --git a/lisp/vc/vc-dir.el b/lisp/vc/vc-dir.el
index 033cb27e33..e5c5e16a17 100644
--- a/lisp/vc/vc-dir.el
+++ b/lisp/vc/vc-dir.el
@@ -879,7 +879,9 @@ vc-dir-ignore
 	   (vc-ignore (vc-dir-fileinfo->name filearg))
 	   t))
        vc-ewoc)
-    (vc-ignore (vc-dir-current-file))))
+    (vc-ignore
+     (file-relative-name (vc-dir-current-file))
+     default-directory)))
 
 (defun vc-dir-current-file ()
   (let ((node (ewoc-locate vc-ewoc)))
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index ec252b74d4..72bd4d3910 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1406,16 +1406,21 @@ vc-ignore
 prefix argument is given, in which case prompt for a file FILE to
 remove from the list of ignored files."
   (interactive
-   (list
-    (if (not current-prefix-arg)
-        (read-file-name "File to ignore: ")
-      (completing-read
-       "File to remove: "
-       (vc-call-backend
-        (or (vc-responsible-backend default-directory)
-            (error "Unknown backend"))
-        'ignore-completion-table default-directory)))
-    nil current-prefix-arg))
+   (let* ((backend (vc-responsible-backend default-directory))
+          (rel-dir
+           (condition-case nil
+               (file-name-directory
+                (vc-call-backend backend 'find-ignore-file
+                                 default-directory))
+             (vc-not-supported
+              default-directory)))
+          (file (read-file-name "File to ignore: ")))
+     (when (and (file-name-absolute-p file)
+                (file-in-directory-p file rel-dir))
+       (setq file (file-relative-name file rel-dir)))
+     (list file
+           rel-dir
+           current-prefix-arg)))
   (let* ((directory (or directory default-directory))
 	 (backend (or (vc-responsible-backend default-directory)
                       (error "Unknown backend"))))
@@ -1423,23 +1428,18 @@ vc-ignore
 
 (defun vc-default-ignore (backend file &optional directory remove)
   "Ignore FILE under the VCS of DIRECTORY (default is `default-directory').
-FILE is a wildcard specification, either relative to
-DIRECTORY or absolute.
+FILE is a wildcard specification relative to DIRECTORY.
 When called from Lisp code, if DIRECTORY is non-nil, the
 repository to use will be deduced by DIRECTORY; if REMOVE is
 non-nil, remove FILE from ignored files.
 Argument BACKEND is the backend you are using."
   (let ((ignore
-	 (vc-call-backend backend 'find-ignore-file (or directory default-directory)))
-	file-path root-dir pattern)
-    (setq file-path (expand-file-name file directory))
-    (setq root-dir (file-name-directory ignore))
-    (when (not (string= (substring file-path 0 (length root-dir)) root-dir))
-      (error "Ignore spec %s is not below project root %s" file-path root-dir))
-    (setq pattern (substring file-path (length root-dir)))
+         (vc-call-backend backend
+                          'find-ignore-file
+                          (or directory default-directory))))
     (if remove
-	(vc--remove-regexp (concat "^" (regexp-quote pattern ) "\\(\n\\|$\\)") ignore)
-      (vc--add-line pattern ignore))))
+        (vc--remove-regexp (concat "^" (regexp-quote file) "\\(\n\\|$\\)") ignore)
+      (vc--add-line file ignore))))
 
 (defun vc-default-ignore-completion-table (backend file)
   "Return the list of ignored files under BACKEND."