bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Wolfgang Scherer]

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

When a filename contains shell wildcard characters matching one or more files, e.g. `test[56].xx` matching both `test5.xx` and `test6.xx`:

  -rw-r--r--  1 ws ws    0 Feb  6 08:51 test[56].xx
  -rw-r--r--  1 ws ws    0 Feb  6 08:51 test5.xx
  -rw-r--r--  1 ws ws    0 Feb  6 08:51 test6.xx

The command `vc-git-state` does not work correctly.

The attched patch fixes this:

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 2caa287..0314e5e 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -330,7 +330,7 @@ in the order given by `git status'."
             ,@(when (version<= "1.7.6.3" (vc-git--program-version))
                 '("--ignored"))
             "--"))
-        (status (apply #'vc-git--run-command-string file args)))
+        (status (apply #'vc-git--run-command-string (shell-quote-argument file) args)))
     (if (null status)
         ;; If status is nil, there was an error calling git, likely because
         ;; the file is not in a git repo.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-vc-git-state-fails-for-filenames-with-wildcards.patch --]
[-- Type: text/x-patch; name="0001-vc-git-state-fails-for-filenames-with-wildcards.patch", Size: 1047 bytes --]

From d123319e15d5ff24d3353f0d5c1b5f46c22172e2 Mon Sep 17 00:00:00 2001
From: Wolfgang Scherer <wolfgang.scherer@gmx.de>
Date: Thu, 6 Feb 2020 14:41:12 +0100
Subject: [PATCH] vc-git-state fails for filenames with wildcards

* lisp/vc/vc-git.el (vc-git-state): file argument must be quoted to
avoid wildcard expansion by shell.
---
 lisp/vc/vc-git.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 2caa287..0314e5e 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -330,7 +330,7 @@ in the order given by `git status'."
             ,@(when (version<= "1.7.6.3" (vc-git--program-version))
                 '("--ignored"))
             "--"))
-        (status (apply #'vc-git--run-command-string file args)))
+        (status (apply #'vc-git--run-command-string (shell-quote-argument file) args)))
     (if (null status)
         ;; If status is nil, there was an error calling git, likely because
         ;; the file is not in a git repo.
--
2.7.4

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Dmitry Gutov]

Hi Wolfgang,

On 06.02.2020 16:59, Wolfgang Scherer wrote:
> When a filename contains shell wildcard characters matching one or more files, e.g. `test[56].xx` matching both `test5.xx` and `test6.xx`:
> 
>    -rw-r--r--  1 ws ws    0 Feb  6 08:51 test[56].xx
>    -rw-r--r--  1 ws ws    0 Feb  6 08:51 test5.xx
>    -rw-r--r--  1 ws ws    0 Feb  6 08:51 test6.xx
> 
> The command `vc-git-state` does not work correctly.
> 
> The attched patch fixes this:
> 
> diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
> index 2caa287..0314e5e 100644
> --- a/lisp/vc/vc-git.el
> +++ b/lisp/vc/vc-git.el
> @@ -330,7 +330,7 @@ in the order given by `git status'."
>               ,@(when (version<= "1.7.6.3" (vc-git--program-version))
>                   '("--ignored"))
>               "--"))
> -        (status (apply #'vc-git--run-command-string file args)))
> +        (status (apply #'vc-git--run-command-string (shell-quote-argument file) args)))
>       (if (null status)
>           ;; If status is nil, there was an error calling git, likely because
>           ;; the file is not in a git repo.
> 

Thanks for the report and the patch.

I wonder how many other backends commands are broken for files like 
that: we basically never shell-quote file names.

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 7 Feb 2020 02:00:38 +0300
> 
> I wonder how many other backends commands are broken for files like 
> that: we basically never shell-quote file names.

Whenever we run commands via the shell, the prudent thing is to always
quote file names (and in general any argument that might include
wildcard characters).  One advantage of call-process is that you don't
have to do that.

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Dmitry Gutov]

On 07.02.2020 10:57, Eli Zaretskii wrote:
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Fri, 7 Feb 2020 02:00:38 +0300
>>
>> I wonder how many other backends commands are broken for files like
>> that: we basically never shell-quote file names.
> 
> Whenever we run commands via the shell, the prudent thing is to always
> quote file names (and in general any argument that might include
> wildcard characters).  One advantage of call-process is that you don't
> have to do that.

It's not so simple. FILE already goes through call-process. But Git 
expects a pathspec, not just a file name. So if it's a glob, it is expanded.

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Eli Zaretskii]

> Cc: 39452@debbugs.gnu.org, Wolfgang.Scherer@gmx.de
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 7 Feb 2020 11:43:36 +0300
> 
> On 07.02.2020 10:57, Eli Zaretskii wrote:
> >> From: Dmitry Gutov <dgutov@yandex.ru>
> >> Date: Fri, 7 Feb 2020 02:00:38 +0300
> >>
> >> I wonder how many other backends commands are broken for files like
> >> that: we basically never shell-quote file names.
> > 
> > Whenever we run commands via the shell, the prudent thing is to always
> > quote file names (and in general any argument that might include
> > wildcard characters).  One advantage of call-process is that you don't
> > have to do that.
> 
> It's not so simple. FILE already goes through call-process. But Git 
> expects a pathspec, not just a file name. So if it's a glob, it is expanded.

What I wrote was a response to your "we basically never quote".  Let
me correct my perhaps too-general response by saying "the prudent
thing is to always quote file names, unless the command expects a
wildcard in that argument".

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Dmitry Gutov]

On 07.02.2020 12:26, Eli Zaretskii wrote:
> What I wrote was a response to your "we basically never quote".

...file names in vc-git.el because we've been relying on call-process.

> Let
> me correct my perhaps too-general response by saying "the prudent
> thing is to always quote file names, unless the command expects a
> wildcard in that argument".

I would say that differently:

...to always quote files names when the command expects a wildcard, but 
we want the file name to be treated verbatim.

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Noam Postavsky]

Dmitry Gutov <dgutov@yandex.ru> writes:

> It's not so simple. FILE already goes through call-process. But Git
> expects a pathspec, not just a file name. So if it's a glob, it is
> expanded.

You can pass --literal-pathspecs to tell git not to expand.  Magit does
this.  But there is a downside due to the way git implements it, which
is by setting an environment variable: it affects all subprocesses git
calls, including git-hook scripts which tends to trip people up.

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Wolfgang Scherer]

Hi Dmitry,

Am 07.02.20 um 00:00 schrieb Dmitry Gutov:
>
> On 06.02.2020 16:59, Wolfgang Scherer wrote:
>> When a filename contains shell wildcard characters matching one or more files, e.g. `test[56].xx` matching both `test5.xx` and `test6.xx`:
>> The command `vc-git-state` does not work correctly.
>>
>> The attched patch fixes this:
>>
>> -        (status (apply #'vc-git--run-command-string file args)))
>> +        (status (apply #'vc-git--run-command-string (shell-quote-argument file) args)))
>>
>
> Thanks for the report and the patch.
>
> I wonder how many other backends commands are broken for files like that: we basically never shell-quote file names.

I finally decided to fully implement the vc ignore feature for all backends. So once I am finished, I will have tested all vs-backend-state functions with filenames containing glob special characters.

Since call-process is already used in vc-git, the function shell-quote-argument is not really appropriate.

For the ongoing vc ignore implementation I needed a glob escape function, which only escapes special glob characters and backslash (see http://sw-amt.ws/emacs/doc/_build/html/emacs-vc-ignore-feature.html):

(defun vc-glob-escape (string)
  "Escape special glob characters in STRING."
  (save-match-data
    (if (string-match "[\\?*[]" string)
        (mapconcat (lambda (c)
                     (pcase c
                       (?\\ "\\\\")
                       (?? "\\?")
                       (?* "\\*")
                       (?\[ "\\[")
                       (_ (char-to-string c))))
                   string "")
      string)))

As for other occurences of glob errors in vc-git, The function vc-git-dir-status-files also suffers from this bug, when the FILES argument is non-nil:

;; (let ((default-directory "/srv/install/linux/emacs/check-git/")) (vc-git-dir-status-files nil '("/srv/install/linux/emacs/check-git/test[56].xx") (lambda (&rest args) args)))

fatal: pathspec 'test[56].xx' did not match any files
test5.xx^@test6.xx^@test[56].xx^@

Various other git commands, like vc-git-revert, vc-git-checkin also use glob expansion and are therefore broken.

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Wolfgang Scherer]

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

Am 07.02.20 um 18:25 schrieb Wolfgang Scherer:
> Hi Dmitry,
>
> Am 07.02.20 um 00:00 schrieb Dmitry Gutov:
>> On 06.02.2020 16:59, Wolfgang Scherer wrote:
>>> When a filename contains shell wildcard characters matching one or more files, e.g. `test[56].xx` matching both `test5.xx` and `test6.xx`:
>>> The command `vc-git-state` does not work correctly.
>>>
>>> The attched patch fixes this:
>>>
>>> -        (status (apply #'vc-git--run-command-string file args)))
>>> +        (status (apply #'vc-git--run-command-string (shell-quote-argument file) args)))
>>>
>> Thanks for the report and the patch.
>>
>> I wonder how many other backends commands are broken for files like that: we basically never shell-quote file names.

After some research, it seems that adding a pathspec magic to commands that support this feature is the best solution.

Here is a patch that applies vc-git--literal-pathspec, vc-git--literal-pathspecs to some git commands in vc-git.el. I have tested all augmented commands in the shell and some in emacs.

(defun vc-git--literal-pathspec-inner (pathspec)
  "Prepend :(literal) path magic to PATHSPEC."
  (concat ":(literal)" pathspec))

(defun vc-git--literal-pathspec (pathspec)
  "Prepend :(literal) path magic to PATHSPEC."
  (and pathspec (vc-git--literal-pathspec-inner pathspec)))

(defun vc-git--literal-pathspecs (pathspecs)
  "Prepend :(literal) path magic to PATHSPECS."
  (mapcar #'vc-git--literal-pathspec-inner pathspecs))
\x01


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-vc-git-state-fails-for-filenames-with-wildcards.patch --]
[-- Type: text/x-patch; name="0001-vc-git-state-fails-for-filenames-with-wildcards.patch", Size: 11112 bytes --]

From eab5ef41ff06471d3cf9387d96a09c70e586b0e6 Mon Sep 17 00:00:00 2001
From: Wolfgang Scherer <wolfgang.scherer@gmx.de>
Date: Fri, 7 Feb 2020 23:24:27 +0100
Subject: [PATCH] vc-git-state fails for filenames with wildcards

* lisp/vc/vc-git.el: (vc-git--literal-pathspec-inner),
(vc-git--literal-pathspec), (vc-git--literal-pathspecs) new functions
to add ":(literal)" pathspec magic.

(vc-git-registered), (vc-git-state), (vc-git-dir-status-goto-stage),
(vc-git-register), (vc-git-unregister), (vc-git-checkin),
(vc-git-find-revision), (vc-git-checkout), (vc-git-revert),
(vc-git-conflicted-files), (vc-git-print-log), (vc-git-diff),
(vc-git-previous-revision), (vc-git-next-revision),
(vc-git-delete-file), (vc-git-rename-file) functions
vc-git--literal-pathspec, vc-git--literal-pathspecs applied.
---
 lisp/vc/vc-git.el | 65 ++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 40 insertions(+), 25 deletions(-)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 2caa287..1a38cef 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -217,6 +217,21 @@ toggle display of the entire list."

 ;;; BACKEND PROPERTIES

+(defun vc-git--literal-pathspec-inner (pathspec)
+  "Prepend :(literal) path magic to PATHSPEC."
+  (concat ":(literal)" pathspec))
+;; (vc-git--literal-pathspec-inner "test[56].xx")
+
+(defun vc-git--literal-pathspec (pathspec)
+  "Prepend :(literal) path magic to PATHSPEC."
+  (and pathspec (vc-git--literal-pathspec-inner pathspec)))
+;; (vc-git--literal-pathspec nil)
+;; (vc-git--literal-pathspec "test[56].xx")
+
+(defun vc-git--literal-pathspecs (pathspecs)
+  "Prepend :(literal) path magic to PATHSPECS."
+  (mapcar #'vc-git--literal-pathspec-inner pathspecs))
+
 (defun vc-git-revision-granularity () 'repository)
 (defun vc-git-checkout-model (_files) 'implicit)
 (defun vc-git-update-on-retrieve-tag () nil)
@@ -243,12 +258,12 @@ toggle display of the entire list."
                (name (file-relative-name file dir))
                (str (ignore-errors
                       (cd dir)
-                      (vc-git--out-ok "ls-files" "-c" "-z" "--" name)
+                      (vc-git--out-ok "ls-files" "-c" "-z" "--" (vc-git--literal-pathspec name))
                       ;; If result is empty, use ls-tree to check for deleted
                       ;; file.
                       (when (eq (point-min) (point-max))
                         (vc-git--out-ok "ls-tree" "--name-only" "-z" "HEAD"
-                                        "--" name))
+                                        "--" (vc-git--literal-pathspec name)))
                       (buffer-string))))
           (and str
                (> (length str) (length name))
@@ -330,7 +345,7 @@ in the order given by `git status'."
             ,@(when (version<= "1.7.6.3" (vc-git--program-version))
                 '("--ignored"))
             "--"))
-        (status (apply #'vc-git--run-command-string file args)))
+        (status (apply #'vc-git--run-command-string (vc-git--literal-pathspec file) args)))
     (if (null status)
         ;; If status is nil, there was an error calling git, likely because
         ;; the file is not in a git repo.
@@ -606,28 +621,28 @@ or an empty string if none."
     (pcase (vc-git-dir-status-state->stage git-state)
       ('update-index
        (if files
-           (vc-git-command (current-buffer) 'async files "add" "--refresh" "--")
+           (vc-git-command (current-buffer) 'async (vc-git--literal-pathspecs files) "add" "--refresh" "--")
          (vc-git-command (current-buffer) 'async nil
                          "update-index" "--refresh")))
       ('ls-files-added
-       (vc-git-command (current-buffer) 'async files
+       (vc-git-command (current-buffer) 'async (vc-git--literal-pathspecs files)
                        "ls-files" "-z" "-c" "-s" "--"))
       ('ls-files-up-to-date
-       (vc-git-command (current-buffer) 'async files
+       (vc-git-command (current-buffer) 'async (vc-git--literal-pathspecs files)
                        "ls-files" "-z" "-c" "-s" "--"))
       ('ls-files-conflict
-       (vc-git-command (current-buffer) 'async files
+       (vc-git-command (current-buffer) 'async (vc-git--literal-pathspecs files)
                        "ls-files" "-z" "-u" "--"))
       ('ls-files-unknown
-       (vc-git-command (current-buffer) 'async files
+       (vc-git-command (current-buffer) 'async (vc-git--literal-pathspecs files)
                        "ls-files" "-z" "-o" "--exclude-standard" "--"))
       ('ls-files-ignored
-       (vc-git-command (current-buffer) 'async files
+       (vc-git-command (current-buffer) 'async (vc-git--literal-pathspecs files)
                        "ls-files" "-z" "-o" "-i" "--directory"
                        "--no-empty-directory" "--exclude-standard" "--"))
       ;; --relative added in Git 1.5.5.
       ('diff-index
-       (vc-git-command (current-buffer) 'async files
+       (vc-git-command (current-buffer) 'async (vc-git--literal-pathspecs files)
                        "diff-index" "--relative" "-z" "-M" "HEAD" "--")))
     (vc-run-delayed
       (vc-git-after-dir-status-stage git-state))))
@@ -861,12 +876,12 @@ The car of the list is the current branch."
     (when flist
       (vc-git-command nil 0 flist "update-index" "--add" "--"))
     (when dlist
-      (vc-git-command nil 0 dlist "add"))))
+      (vc-git-command nil 0 (vc-git--literal-pathspecs dlist) "add"))))

 (defalias 'vc-git-responsible-p 'vc-git-root)

 (defun vc-git-unregister (file)
-  (vc-git-command nil 0 file "rm" "-f" "--cached" "--"))
+  (vc-git-command nil 0 (vc-git--literal-pathspec file) "rm" "-f" "--cached" "--"))

 (declare-function log-edit-mode "log-edit" ())
 (declare-function log-edit-toggle-header "log-edit" (header value))
@@ -933,7 +948,7 @@ It is based on `log-edit-mode', and has Git-specific extensions.")
                (lambda (value) (when (equal value "yes") (list argument)))))
       ;; When operating on the whole tree, better pass "-a" than ".", since "."
       ;; fails when we're committing a merge.
-      (apply 'vc-git-command nil 0 (if only files)
+      (apply 'vc-git-command nil 0 (if only (vc-git--literal-pathspecs files))
              (nconc (if msg-file (list "commit" "-F"
                                        (file-local-name msg-file))
                       (list "commit" "-m"))
@@ -960,7 +975,7 @@ It is based on `log-edit-mode', and has Git-specific extensions.")
 	 (coding-system-for-write 'binary)
 	 (fullname
 	  (let ((fn (vc-git--run-command-string
-		     file "ls-files" "-z" "--full-name" "--")))
+		     (vc-git--literal-pathspec file) "ls-files" "-z" "--full-name" "--")))
 	    ;; ls-files does not return anything when looking for a
 	    ;; revision of a file that has been renamed or removed.
 	    (if (string= fn "")
@@ -977,14 +992,14 @@ It is based on `log-edit-mode', and has Git-specific extensions.")
 		    (vc-git-root file)))

 (defun vc-git-checkout (file &optional rev)
-  (vc-git-command nil 0 file "checkout" (or rev "HEAD")))
+  (vc-git-command nil 0 (vc-git--literal-pathspec file) "checkout" (or rev "HEAD")))

 (defun vc-git-revert (file &optional contents-done)
   "Revert FILE to the version stored in the git repository."
   (if contents-done
       (vc-git-command nil 0 file "update-index" "--")
-    (vc-git-command nil 0 file "reset" "-q" "--")
-    (vc-git-command nil nil file "checkout" "-q" "--")))
+    (vc-git-command nil 0 (vc-git--literal-pathspec file) "reset" "-q" "--")
+    (vc-git-command nil nil (vc-git--literal-pathspec file) "checkout" "-q" "--")))

 (defvar vc-git-error-regexp-alist
   '(("^ \\(.+\\)\\> *|" 1 nil nil 0))
@@ -1068,7 +1083,7 @@ This prompts for a branch to merge from."
 (defun vc-git-conflicted-files (directory)
   "Return the list of files with conflicts in DIRECTORY."
   (let* ((status
-          (vc-git--run-command-string directory "status" "--porcelain" "--"))
+          (vc-git--run-command-string (vc-git--literal-pathspec directory) "status" "--porcelain" "--"))
          (lines (when status (split-string status "\n" 'omit-nulls)))
          files)
     (dolist (line lines files)
@@ -1140,7 +1155,7 @@ If LIMIT is a revision string, use it as an end-revision."
     (let ((inhibit-read-only t))
       (with-current-buffer buffer
 	(apply 'vc-git-command buffer
-	       'async files
+	       'async (vc-git--literal-pathspecs files)
 	       (append
 		'("log" "--no-color")
                 (when (and vc-git-print-log-follow
@@ -1392,7 +1407,7 @@ This requires git 1.8.4 or later, for the \"-L\" option of \"git log\"."
     (if vc-git-diff-switches
         (apply #'vc-git-command (or buffer "*vc-diff*")
 	       1 ; bug#21969
-	       files
+	       (vc-git--literal-pathspecs files)
                command
                "--exit-code"
                (append (vc-switches 'git 'diff)
@@ -1475,7 +1490,7 @@ This requires git 1.8.4 or later, for the \"-L\" option of \"git log\"."
       (let* ((fname (file-relative-name file))
              (prev-rev (with-temp-buffer
                          (and
-                          (vc-git--out-ok "rev-list" "-2" rev "--" fname)
+                          (vc-git--out-ok "rev-list" "-2" rev "--" (vc-git--literal-pathspec fname))
                           (goto-char (point-max))
                           (bolp)
                           (zerop (forward-line -1))
@@ -1503,7 +1518,7 @@ This requires git 1.8.4 or later, for the \"-L\" option of \"git log\"."
          (current-rev
           (with-temp-buffer
             (and
-             (vc-git--out-ok "rev-list" "-1" rev "--" file)
+             (vc-git--out-ok "rev-list" "-1" rev "--" (vc-git--literal-pathspec file))
              (goto-char (point-max))
              (bolp)
              (zerop (forward-line -1))
@@ -1515,7 +1530,7 @@ This requires git 1.8.4 or later, for the \"-L\" option of \"git log\"."
           (and current-rev
                (with-temp-buffer
                  (and
-                  (vc-git--out-ok "rev-list" "HEAD" "--" file)
+                  (vc-git--out-ok "rev-list" "HEAD" "--" (vc-git--literal-pathspec file))
                   (goto-char (point-min))
                   (search-forward current-rev nil t)
                   (zerop (forward-line -1))
@@ -1525,10 +1540,10 @@ This requires git 1.8.4 or later, for the \"-L\" option of \"git log\"."
     (or (vc-git-symbolic-commit next-rev) next-rev)))

 (defun vc-git-delete-file (file)
-  (vc-git-command nil 0 file "rm" "-f" "--"))
+  (vc-git-command nil 0 (vc-git--literal-pathspec file) "rm" "-f" "--"))

 (defun vc-git-rename-file (old new)
-  (vc-git-command nil 0 (list old new) "mv" "-f" "--"))
+  (vc-git-command nil 0 (vc-git--literal-pathspecs (list old new)) "mv" "-f" "--"))

 (defvar vc-git-extra-menu-map
   (let ((map (make-sparse-keymap)))
--
2.7.4

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Dmitry Gutov]

On 07.02.2020 16:43, Noam Postavsky wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
>> It's not so simple. FILE already goes through call-process. But Git
>> expects a pathspec, not just a file name. So if it's a glob, it is
>> expanded.
> 
> You can pass --literal-pathspecs to tell git not to expand.  Magit does
> this.  But there is a downside due to the way git implements it, which
> is by setting an environment variable: it affects all subprocesses git
> calls, including git-hook scripts which tends to trip people up.

I wonder how bad the latter problem is. After all, even if it happens, 
it *can* be worked around in the same scripts.

The patch is much smaller than the proposed alternative:

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 61e6c642d1..bbfdbfbe52 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1751,6 +1751,7 @@ vc-git-command
          (process-environment
           (append
            `("GIT_DIR"
+            "GIT_LITERAL_PATHSPECS=1"
              ;; Avoid repository locking during background operations
              ;; (bug#21559).
              ,@(when revert-buffer-in-progress-p
@@ -1785,6 +1786,7 @@ vc-git--call
  	(process-environment
  	 (append
  	  `("GIT_DIR"
+            "GIT_LITERAL_PATHSPECS=1"
  	    ;; Avoid repository locking during background operations
  	    ;; (bug#21559).
  	    ,@(when revert-buffer-in-progress-p

And if Magit does it, it's probably okay for most of VC users too.

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Noam Postavsky]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 07.02.2020 16:43, Noam Postavsky wrote:
>> Dmitry Gutov <dgutov@yandex.ru> writes:
>>
>>> It's not so simple. FILE already goes through call-process. But Git
>>> expects a pathspec, not just a file name. So if it's a glob, it is
>>> expanded.
>>
>> You can pass --literal-pathspecs to tell git not to expand.  Magit does
>> this.  But there is a downside due to the way git implements it, which
>> is by setting an environment variable: it affects all subprocesses git
>> calls, including git-hook scripts which tends to trip people up.
>
> I wonder how bad the latter problem is.  After all, even if it
> happens, it *can* be worked around in the same scripts.

Yes, it's common enough to have a FAQ for it.
https://magit.vc/manual/magit/My-Git-hooks-work-on-the-command_002dline-but-not-inside-Magit.html

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Wolfgang Scherer]

Am 12.02.20 um 00:01 schrieb Dmitry Gutov:
> The patch is much smaller than the proposed alternative:

The patch works as expected.

> And if Magit does it, it's probably okay for most of VC users too.

It might be worth documenting in the manual, since the problem comes up quite a lot in issues and Stackoverflow.

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Dmitry Gutov]

On 13.02.2020 20:34, Wolfgang Scherer wrote:

>> And if Magit does it, it's probably okay for most of VC users too.
> 
> It might be worth documenting in the manual, since the problem comes up quite a lot in issues and Stackoverflow.

You mean the problem inside git hooks, if such solution is applied?

Weird how I never seen it mentioned before.

Anyway, at this point I'd rather hear more opinion.

Eli, what do you think is better, applying a more verbose fix (which 
prepends stuff to every file name) and spending some of our complexity 
budged on that.

Or using the simple fix and documenting the git hooks erratum in the 
manual (or somewhere else).

I'm inclining toward the latter, esp. considering that the problem 
should be familiar to Magit users already, and thus the fix is 
prominently documented on the Internet.

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Eli Zaretskii]

> Cc: 39452@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 14 Feb 2020 01:23:50 +0200
> 
> Eli, what do you think is better, applying a more verbose fix (which 
> prepends stuff to every file name) and spending some of our complexity 
> budged on that.
> 
> Or using the simple fix and documenting the git hooks erratum in the 
> manual (or somewhere else).
> 
> I'm inclining toward the latter, esp. considering that the problem 
> should be familiar to Magit users already, and thus the fix is 
> prominently documented on the Internet.

I also tend towards the latter, especially since (AFAIU) we don't
prevent users from using the Git magic signatures in file-name specs
(where it makes sense), in which case we'd need some code to detect
and combine signatures.

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Dmitry Gutov]

On 14.02.2020 11:37, Eli Zaretskii wrote:
> I also tend towards the latter, especially since (AFAIU) we don't
> prevent users from using the Git magic signatures in file-name specs
> (where it makes sense), in which case we'd need some code to detect
> and combine signatures.

Could you clarify? What commands and backend actions expect or 
intentionally allow file-name specs instead of file-names verbatim?

Aside from vc-ignore, I suppose. Which won't be affected either way.

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Eli Zaretskii]

> Cc: npostavs@gmail.com, 39452@debbugs.gnu.org, Wolfgang.Scherer@gmx.de
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 14 Feb 2020 15:59:41 +0200
> 
> On 14.02.2020 11:37, Eli Zaretskii wrote:
> > I also tend towards the latter, especially since (AFAIU) we don't
> > prevent users from using the Git magic signatures in file-name specs
> > (where it makes sense), in which case we'd need some code to detect
> > and combine signatures.
> 
> Could you clarify? What commands and backend actions expect or 
> intentionally allow file-name specs instead of file-names verbatim?

Any command that prompts for a file name, I guess.  vc-delete-file and
vc-rename-file come to mind.

But my comment was more general: we don't plan on not supporting Git
specs in file names, do we?

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Dmitry Gutov]

On 14.02.2020 16:14, Eli Zaretskii wrote:
> Any command that prompts for a file name, I guess.  vc-delete-file and
> vc-rename-file come to mind.

In both of these commands entering a non-trivial pathspec is both 
undocumented and hard to do: look at the interactive form, it calls 
read-file-name with MUSTMATCH t. In other words, it doesn't let you 
input interactively anything that's not an existing file name.

> But my comment was more general: we don't plan on not supporting Git
> specs in file names, do we?

I don't see how we'd keep supporting them in these two particular 
commands without keeping bugs similar to this one unfixed (e.g. 'M-x 
vc-delete-file test[56].xx' where test[56].xx is an existing filename).

AFAICS, they're working purely by accident. That's not to say we can't 
introduce new versions of these commands that would accept pathspecs (or 
do it with C-u, etc).

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Eli Zaretskii]

> Cc: npostavs@gmail.com, 39452@debbugs.gnu.org, Wolfgang.Scherer@gmx.de
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 14 Feb 2020 16:40:04 +0200
> 
> On 14.02.2020 16:14, Eli Zaretskii wrote:
> > Any command that prompts for a file name, I guess.  vc-delete-file and
> > vc-rename-file come to mind.
> 
> In both of these commands entering a non-trivial pathspec is both 
> undocumented and hard to do: look at the interactive form, it calls 
> read-file-name with MUSTMATCH t. In other words, it doesn't let you 
> input interactively anything that's not an existing file name.
> 
> > But my comment was more general: we don't plan on not supporting Git
> > specs in file names, do we?
> 
> I don't see how we'd keep supporting them in these two particular 
> commands without keeping bugs similar to this one unfixed (e.g. 'M-x 
> vc-delete-file test[56].xx' where test[56].xx is an existing filename).
> 
> AFAICS, they're working purely by accident. That's not to say we can't 
> introduce new versions of these commands that would accept pathspecs (or 
> do it with C-u, etc).

I won't argue.  I just wanted to point out that using Git signatures
internally might get in the way, whereas environment variables and
command-line switches are free from that disadvantage.

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Dmitry Gutov]

On 14.02.2020 17:45, Eli Zaretskii wrote:
> I won't argue.  I just wanted to point out that using Git signatures
> internally might get in the way, whereas environment variables and
> command-line switches are free from that disadvantage.

Um, I think they're about the same in the level of convenience for us to 
be able to disable either: we can add a global var which would affect 
whether specs are interpreted literally.

If anything, approach #1 is slightly easier if we wanted to support 
opting out of literal-quoting the specs at the level of VC backend 
actions: certain action implementations can simply avoid calling the 
proposed functions (like vc-git--literal-pathspec). But then, I'm not 
sure that we want this capability at exactly that abstraction level.

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Eli, what do you think is better, applying a more verbose fix (which
> prepends stuff to every file name) and spending some of our complexity
> budged on that.
>
> Or using the simple fix and documenting the git hooks erratum in the
> manual (or somewhere else).
>
> I'm inclining toward the latter, esp. considering that the problem
> should be familiar to Magit users already, and thus the fix is
> prominently documented on the Internet.

It seems like everybody agreed that adding the environment variable and
documenting the problem would be the best fix here, but it doesn't look
like that was applied?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

>> I wonder how bad the latter problem is.  After all, even if it
>> happens, it *can* be worked around in the same scripts.
>
> Yes, it's common enough to have a FAQ for it.
> https://magit.vc/manual/magit/My-Git-hooks-work-on-the-command_002dline-but-not-inside-Magit.html

Wolfgang's patch is larger, but doesn't have these drawbacks (if I
understand things correctly) -- so wouldn't it make sense to apply
Wolfgang's patch here?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Dmitry Gutov]

On 12.05.2021 19:04, Lars Ingebrigtsen wrote:
> Wolfgang's patch is larger, but doesn't have these drawbacks (if I
> understand things correctly) -- so wouldn't it make sense to apply
> Wolfgang's patch here?

It's a pretty big fix for a straightforward (if fundamental) problem.

The existence of the FAQ entry personally just tells me that the other 
approach has been proven in the field (by Magit), and if a FAQ entry is 
enough, the side-effects are probably not that common or serious. 
Because users, in general, don't read documentation.

But other opinions welcome.

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 12.05.2021 19:04, Lars Ingebrigtsen wrote:
>> Wolfgang's patch is larger, but doesn't have these drawbacks (if I
>> understand things correctly) -- so wouldn't it make sense to apply
>> Wolfgang's patch here?
>
> It's a pretty big fix for a straightforward (if fundamental) problem.
>
> The existence of the FAQ entry personally just tells me that the other
> approach has been proven in the field (by Magit), and if a FAQ entry
> is enough, the side-effects are probably not that common or
> serious. Because users, in general, don't read documentation.
>
> But other opinions welcome.

Nobody had any opinions, and the "if it's good enough for Magit"
argument is a good one.  So I went ahead and applied Dmitry's patch to
Emacs 28, and we'll see whether there's any push back on that...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Dmitry Gutov]

On 22.07.2021 15:42, Lars Ingebrigtsen wrote:
> Nobody had any opinions, and the "if it's good enough for Magit"
> argument is a good one.  So I went ahead and applied Dmitry's patch to
> Emacs 28, and we'll see whether there's any push back on that...

As luck would have it, I have a bit of code (namely 
project--vc-list-files) that got broken with that change.

Because, when EXTRA-IGNORES are present, it constructs some non-literal 
pathspecs, which naturally fail (get misinterpreted) with 
GIT_LITERAL_PATHSPECS=1.

So we need an escape hatch to turn off this feature, which could take 
form of a dynamic variable, like in the patch below.

Or we could revert to the other approach. What do people think?


diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 714edeba5f..824ea55e7b 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -523,6 +523,7 @@ project--vc-list-files
      (`Git
       (let ((default-directory (expand-file-name 
(file-name-as-directory dir)))
             (args '("-z"))
+           vc-git-use-literal-pathspecs
             files)
         ;; Include unregistered.
         (setq args (append args
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 143087122f..1082e724ff 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -220,6 +220,9 @@ vc-git-revision-complete-only-branches
    :type 'boolean
    :version "28.1")

+(defvar vc-git-use-literal-pathspecs t
+  "Non-nil to interpret all Git pathspecs literally.")
+
  ;; History of Git commands.
  (defvar vc-git-history nil)

@@ -1772,7 +1775,8 @@ vc-git-command
          (process-environment
           (append
            `("GIT_DIR"
-            "GIT_LITERAL_PATHSPECS=1"
+            ,@(when vc-git-use-literal-pathspecs
+                '("GIT_LITERAL_PATHSPECS=1"))
              ;; Avoid repository locking during background operations
              ;; (bug#21559).
              ,@(when revert-buffer-in-progress-p
@@ -1807,8 +1811,9 @@ vc-git--call
  	(process-environment
  	 (append
  	  `("GIT_DIR"
-            "GIT_LITERAL_PATHSPECS=1"
-	    ;; Avoid repository locking during background operations
+            ,@(when vc-git-use-literal-pathspecs
+                '("GIT_LITERAL_PATHSPECS=1"))
+            ;; Avoid repository locking during background operations
  	    ;; (bug#21559).
  	    ,@(when revert-buffer-in-progress-p
  		'("GIT_OPTIONAL_LOCKS=0")))

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

>> Nobody had any opinions, and the "if it's good enough for Magit"
>> argument is a good one.  So I went ahead and applied Dmitry's patch to
>> Emacs 28, and we'll see whether there's any push back on that...
>
> As luck would have it, I have a bit of code (namely
> project--vc-list-files) that got broken with that change.
>
> Because, when EXTRA-IGNORES are present, it constructs some
> non-literal pathspecs, which naturally fail (get misinterpreted) with
> GIT_LITERAL_PATHSPECS=1.
>
> So we need an escape hatch to turn off this feature, which could take
> form of a dynamic variable, like in the patch below.
>
> Or we could revert to the other approach. What do people think?

If we've seen one piece of code break here already, then perhaps
reverting and moving to the other (safer, but more invasive) approach is
the right way to go.  

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Dmitry Gutov]

On 14.08.2021 14:56, Lars Ingebrigtsen wrote:
> If we've seen one piece of code break here already, then perhaps
> reverting and moving to the other (safer, but more invasive) approach is
> the right way to go.

Fair enough.

Pushed the other patch with a couple of small tweaks.

Thanks again to Wolfgang.

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Juri Linkov]

>> If we've seen one piece of code break here already, then perhaps
>> reverting and moving to the other (safer, but more invasive) approach is
>> the right way to go.
>
> Fair enough.
>
> Pushed the other patch with a couple of small tweaks.

This broke vc-rename-file that now fails with

  (error Failed (status 128): git --no-pager mv -f -- :(literal)/tmp/gitrepo/subdir/file1 :(literal)/tmp/gitrepo/subdir/file2)
  fatal: bad source, source=subdir/:(literal)/tmp/gitrepo/subdir/file1, destination=subdir/:(literal)/tmp/gitrepo/subdir/file2

Can be fixed with

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 02ca022ad4..88e015fc9d 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1559,7 +1559,7 @@ vc-git-delete-file
   (vc-git-command nil 0 (vc-git--literal-pathspecs file) "rm" "-f" "--"))
 
 (defun vc-git-rename-file (old new)
-  (vc-git-command nil 0 (vc-git--literal-pathspecs (list old new)) "mv" "-f" "--"))
+  (vc-git-command nil 0 (list old new) "mv" "-f" "--"))
 
 (defun vc-git-mark-resolved (files)
   (vc-git-command nil 0 (vc-git--literal-pathspecs files) "add"))

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Dmitry Gutov]

On 27.08.2021 09:05, Juri Linkov wrote:
>>> If we've seen one piece of code break here already, then perhaps
>>> reverting and moving to the other (safer, but more invasive) approach is
>>> the right way to go.
>>
>> Fair enough.
>>
>> Pushed the other patch with a couple of small tweaks.
> 
> This broke vc-rename-file that now fails with
> 
>    (error Failed (status 128): git --no-pager mv -f -- :(literal)/tmp/gitrepo/subdir/file1 :(literal)/tmp/gitrepo/subdir/file2)
>    fatal: bad source, source=subdir/:(literal)/tmp/gitrepo/subdir/file1, destination=subdir/:(literal)/tmp/gitrepo/subdir/file2

It was probably broken by the original change, no?

Just not fixed by any of the subsequent repairs.

> Can be fixed with
> 
> diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
> index 02ca022ad4..88e015fc9d 100644
> --- a/lisp/vc/vc-git.el
> +++ b/lisp/vc/vc-git.el
> @@ -1559,7 +1559,7 @@ vc-git-delete-file
>     (vc-git-command nil 0 (vc-git--literal-pathspecs file) "rm" "-f" "--"))
>   
>   (defun vc-git-rename-file (old new)
> -  (vc-git-command nil 0 (vc-git--literal-pathspecs (list old new)) "mv" "-f" "--"))
> +  (vc-git-command nil 0 (list old new) "mv" "-f" "--"))
>   
>   (defun vc-git-mark-resolved (files)
>     (vc-git-command nil 0 (vc-git--literal-pathspecs files) "add"))

Looks like the proper fix, thanks. Feel free to push it right away, if 
you like.

Would be great to add some test, though. vc-tests.el currently doesn't 
exercise vc-rename-file at all.

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Juri Linkov]

>> -  (vc-git-command nil 0 (vc-git--literal-pathspecs (list old new)) "mv" "-f" "--"))
>> +  (vc-git-command nil 0 (list old new) "mv" "-f" "--"))
>
> Looks like the proper fix, thanks. Feel free to push it right away, if
> you like.

Pushed now.

I wonder how many git commands still remain broken
and will go unnoticed to the release?  Such as
vc-git-delete-file and vc-git-mark-resolved, etc.

> Would be great to add some test, though. vc-tests.el currently doesn't
> exercise vc-rename-file at all.

Indeed, covering all git commands will avoid the danger of breaking
some commands.

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Stephen Berman]

On Fri, 27 Aug 2021 20:10:13 +0300 Juri Linkov <juri@linkov.net> wrote:

>>> - (vc-git-command nil 0 (vc-git--literal-pathspecs (list old new)) "mv"
>>> "-f" "--"))
>>> +  (vc-git-command nil 0 (list old new) "mv" "-f" "--"))
>>
>> Looks like the proper fix, thanks. Feel free to push it right away, if
>> you like.
>
> Pushed now.
>
> I wonder how many git commands still remain broken
> and will go unnoticed to the release?  Such as
> vc-git-delete-file and vc-git-mark-resolved, etc.
>
>> Would be great to add some test, though. vc-tests.el currently doesn't
>> exercise vc-rename-file at all.
>
> Indeed, covering all git commands will avoid the danger of breaking
> some commands.

I just discovered that some code I have that uses vc-print-log-internal
broke after the literal-pathspecs change; specifically, my code passes a
directory name beginning with "~/" to vc-print-log-internal, and this
had worked fine till that change, which broke it, and I found I have to
wrap the directory name in expand-file-name to make the code work again.
Is this expected fallout from that change or was I perhaps misusing
vc-print-log-internal and was just lucky that it had worked before?

Steve Berman

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Dmitry Gutov]

On 27.08.2021 20:10, Juri Linkov wrote:
>>> -  (vc-git-command nil 0 (vc-git--literal-pathspecs (list old new)) "mv" "-f" "--"))
>>> +  (vc-git-command nil 0 (list old new) "mv" "-f" "--"))
>>
>> Looks like the proper fix, thanks. Feel free to push it right away, if
>> you like.
> 
> Pushed now.

Thank you.

> I wonder how many git commands still remain broken
> and will go unnoticed to the release?  Such as
> vc-git-delete-file and vc-git-mark-resolved, etc.

Those two -- probably not. But you're welcome to try and report any 
problems.

I'll try to fix and regressions now, but if people think we should go 
back to a different approach (for example, go back to the other 
solution, but use it in an opt-in fashion by passing --literal-pathspecs 
from every applicable command), we can do so too.

>> Would be great to add some test, though. vc-tests.el currently doesn't
>> exercise vc-rename-file at all.
> 
> Indeed, covering all git commands will avoid the danger of breaking
> some commands.

Volunteers welcome.

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Lars Ingebrigtsen]

Stephen Berman <stephen.berman@gmx.net> writes:

> I just discovered that some code I have that uses vc-print-log-internal
> broke after the literal-pathspecs change; specifically, my code passes a
> directory name beginning with "~/" to vc-print-log-internal, and this
> had worked fine till that change, which broke it, and I found I have to
> wrap the directory name in expand-file-name to make the code work again.
> Is this expected fallout from that change or was I perhaps misusing
> vc-print-log-internal and was just lucky that it had worked before?

I think it's reasonable to expect parameters to `vc-print-log-internal'
to have been expanded first, but that should be documented, at least.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Stephen Berman]

On Sat, 28 Aug 2021 17:07:01 +0200 Lars Ingebrigtsen <larsi@gnus.org> wrote:

> Stephen Berman <stephen.berman@gmx.net> writes:
>
>> I just discovered that some code I have that uses vc-print-log-internal
>> broke after the literal-pathspecs change; specifically, my code passes a
>> directory name beginning with "~/" to vc-print-log-internal, and this
>> had worked fine till that change, which broke it, and I found I have to
>> wrap the directory name in expand-file-name to make the code work again.
>> Is this expected fallout from that change or was I perhaps misusing
>> vc-print-log-internal and was just lucky that it had worked before?
>
> I think it's reasonable to expect parameters to `vc-print-log-internal'
> to have been expanded first, but that should be documented, at least.

I'm not sure it needs to be documented, it is internal, after all.  I'm
using it because I want a different UI than what vc-print-log provides.
The latter passes the file name to vc-print-log-internal already
expanded; I should have checked that when I decided to use the internal
function.  But I don't see at first glance why the unexpanded file name
in the latter worked prior to the literal-pathspecs change but not
afterwards.

Steve Berman

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Lars Ingebrigtsen]

Stephen Berman <stephen.berman@gmx.net> writes:

> But I don't see at first glance why the unexpanded file name
> in the latter worked prior to the literal-pathspecs change but not
> afterwards.

Isn't that what that change does -- make git interpret paths literally?
(So that you can have file names like "~" and "*" in your repo.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Stephen Berman]

On Sat, 28 Aug 2021 17:48:15 +0200 Lars Ingebrigtsen <larsi@gnus.org> wrote:

> Stephen Berman <stephen.berman@gmx.net> writes:
>
>> But I don't see at first glance why the unexpanded file name
>> in the latter worked prior to the literal-pathspecs change but not
>> afterwards.
>
> Isn't that what that change does -- make git interpret paths literally?
> (So that you can have file names like "~" and "*" in your repo.)

Ah, that makes sense and explains why it broke my code, thanks.  (My
first glance at the change was very superficial, and you've saved me
from having to take a second glance :-).

Steve Berman

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Dmitry Gutov]

On 28.08.2021 18:48, Lars Ingebrigtsen wrote:
> Stephen Berman <stephen.berman@gmx.net> writes:
> 
>> But I don't see at first glance why the unexpanded file name
>> in the latter worked prior to the literal-pathspecs change but not
>> afterwards.
> 
> Isn't that what that change does -- make git interpret paths literally?
> (So that you can have file names like "~" and "*" in your repo.)

Yes and no: Git never received file names starting with "~" anyway - 
because vc-do-command converted all file names to relative ones.

But a file name starting with ":(literal)..." is not something 
recognized by Emacs, so file-relative-name doesn't work anymore.

If instead of altering file names we switch to the --literal-pathnames 
argument, this problem should go away. But we will return to the 
original concern that "the way git implements it, which
is by setting an environment variable: it affects all subprocesses git
calls, including git-hook scripts which tends to trip people up" 
(quoting Noam).

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Dmitry Gutov]

On 27.08.2021 15:51, Dmitry Gutov wrote:
> Would be great to add some test, though. vc-tests.el currently doesn't 
> exercise vc-rename-file at all.

Just added the tests for vc-rename-file.

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Juri Linkov]

>> Would be great to add some test, though. vc-tests.el currently doesn't
>> exercise vc-rename-file at all.
>
> Just added the tests for vc-rename-file.

Thanks, I tried to run the test, but it fails:

  Test vc-test-rcs05-rename-file condition:
      (ert-test-failed
       ((should
         (equal
          (vc-state new-name)
          'added))
        :form
        (equal up-to-date added)
        :value nil :explanation
        (different-atoms up-to-date added)))
     FAILED  12/12  vc-test-rcs05-rename-file

Another question: after removing vc-git--literal-pathspecs from
vc-git-rename-file, does this mean that vc-git-rename-file
now doesn't support literal paths?  Maybe it could be possible
to fix vc-git--literal-pathspecs to support relative literal paths
for vc-git-rename-file?

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Dmitry Gutov]

On 29.08.2021 19:44, Juri Linkov wrote:
>>> Would be great to add some test, though. vc-tests.el currently doesn't
>>> exercise vc-rename-file at all.
>>
>> Just added the tests for vc-rename-file.
> 
> Thanks, I tried to run the test, but it fails:
> 
>    Test vc-test-rcs05-rename-file condition:
>        (ert-test-failed
>         ((should
>           (equal
>            (vc-state new-name)
>            'added))
>          :form
>          (equal up-to-date added)
>          :value nil :explanation
>          (different-atoms up-to-date added)))
>       FAILED  12/12  vc-test-rcs05-rename-file

Could you go ahead and fix the expectation?

The check is near the end of vc-test--rename-file, and the expected 
value can be made to depend on the current backend.

I could only run the tests with Git, Hg and Bzr, and I couldn't find any 
CI builds for Emacs that are still working.

> Another question: after removing vc-git--literal-pathspecs from
> vc-git-rename-file, does this mean that vc-git-rename-file
> now doesn't support literal paths?  Maybe it could be possible
> to fix vc-git--literal-pathspecs to support relative literal paths
> for vc-git-rename-file?

The original problem just meant that 'git mv' never supported pathspecs 
(which makes sense), that's why it broke after the change. Now we pass 
file names to it (which it interprets literally), and all is well.

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Could you go ahead and fix the expectation?
>
> The check is near the end of vc-test--rename-file, and the expected
> value can be made to depend on the current backend.

OK, now done.  (But RCS returns up-to-date her instead of added -- is
that correct?)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Dmitry Gutov]

On 29.08.2021 22:57, Lars Ingebrigtsen wrote:
> Dmitry Gutov<dgutov@yandex.ru>  writes:
> 
>> Could you go ahead and fix the expectation?
>>
>> The check is near the end of vc-test--rename-file, and the expected
>> value can be made to depend on the current backend.
> OK, now done.  (But RCS returns up-to-date her instead of added -- is
> that correct?)

I guess so.

I'm not familiar with RCS, but 'added' does not occur even once in 
vc-rsc.el, so apparently it doesn't have a separate state like that.

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Dmitry Gutov]

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

On 27.08.2021 22:57, Stephen Berman wrote:
> I just discovered that some code I have that uses vc-print-log-internal
> broke after the literal-pathspecs change; specifically, my code passes a
> directory name beginning with "~/" to vc-print-log-internal, and this
> had worked fine till that change, which broke it, and I found I have to
> wrap the directory name in expand-file-name to make the code work again.
> Is this expected fallout from that change or was I perhaps misusing
> vc-print-log-internal and was just lucky that it had worked before?

Here's a patch which restores vc-print-log-internal's behavior without 
major changes. Check it out (attached).

[-- Attachment #2: vc-print-log-internal-restore.diff --]
[-- Type: text/x-patch, Size: 1836 bytes --]

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 1cd200cd13..a5431abb40 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -245,7 +245,11 @@ vc-git-update-on-retrieve-tag
 (defun vc-git--literal-pathspec (file)
   "Prepend :(literal) path magic to FILE."
   ;; Good example of file name that needs this: "test[56].xx".
-  (and file (concat ":(literal)" (file-local-name file))))
+  (let ((lname (file-local-name file)))
+    ;; Expand abbreviated file names.
+    (when (file-name-absolute-p lname)
+      (setq lname (expand-file-name lname)))
+    (and file (concat ":(literal)" lname))))
 
 (defun vc-git--literal-pathspecs (files)
   "Prepend :(literal) path magic to FILES."
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index b75862e8a5..f0f5809d99 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -2055,9 +2055,9 @@ vc-root-diff
       ;; here, this way the *vc-diff* buffer is setup correctly, so
       ;; relative file names work.
       (let ((default-directory rootdir))
-	(vc-diff-internal
-	 t (list backend (list (expand-file-name rootdir)) working-revision) nil nil
-	 (called-interactively-p 'interactive))))))
+        (vc-diff-internal
+         t (list backend (list rootdir) working-revision) nil nil
+         (called-interactively-p 'interactive))))))
 
 ;;;###autoload
 (defun vc-root-dir ()
@@ -2603,8 +2603,8 @@ vc-print-root-log
       (setq backend (vc-responsible-backend rootdir))
       (unless backend
         (error "Directory is not version controlled")))
-    (setq default-directory (expand-file-name rootdir))
-    (vc-print-log-internal backend (list default-directory) revision revision limit
+    (setq default-directory rootdir)
+    (vc-print-log-internal backend (list rootdir) revision revision limit
                            (when with-diff 'with-diff))))
 
 ;;;###autoload

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Stephen Berman]

On Mon, 30 Aug 2021 05:36:06 +0300 Dmitry Gutov <dgutov@yandex.ru> wrote:

> On 27.08.2021 22:57, Stephen Berman wrote:
>> I just discovered that some code I have that uses vc-print-log-internal
>> broke after the literal-pathspecs change; specifically, my code passes a
>> directory name beginning with "~/" to vc-print-log-internal, and this
>> had worked fine till that change, which broke it, and I found I have to
>> wrap the directory name in expand-file-name to make the code work again.
>> Is this expected fallout from that change or was I perhaps misusing
>> vc-print-log-internal and was just lucky that it had worked before?
>
> Here's a patch which restores vc-print-log-internal's behavior without major
> changes. Check it out (attached).

Yeah, with that patch my code works again when passing a directory name
beginning "~/" to vc-print-log-internal.  Thanks.

Steve Berman

bug#39452: [PATCH] vc-git-state fails for filenames with wildcards

[Dmitry Gutov]

On 30.08.2021 16:34, Stephen Berman wrote:
> Yeah, with that patch my code works again when passing a directory name
> beginning "~/" to vc-print-log-internal.  Thanks.

Thanks for testing. Pushed.