GNU bug report logs - #39452
[PATCH] vc-git-state fails for filenames with wildcards

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 39452 in the body.
You can then email your comments to 39452 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Wolfgang Scherer <Wolfgang.Scherer <at> gmx.de>
To: Emacs Bugs <bug-gnu-emacs <at> gnu.org>
Subject: [PATCH] vc-git-state fails for filenames with wildcards
Date: Thu, 6 Feb 2020 14:59:26 +0100

[Message part 1 (text/plain, inline)]

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.

[0001-vc-git-state-fails-for-filenames-with-wildcards.patch (text/x-patch, attachment)]

Message #8 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Wolfgang Scherer <Wolfgang.Scherer <at> gmx.de>, 39452 <at> debbugs.gnu.org
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Fri, 7 Feb 2020 02:00:38 +0300

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.

Message #11 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Fri, 07 Feb 2020 09:57:54 +0200

> From: Dmitry Gutov <dgutov <at> 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.

Message #14 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Fri, 7 Feb 2020 11:43:36 +0300

On 07.02.2020 10:57, Eli Zaretskii wrote:
>> From: Dmitry Gutov <dgutov <at> 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.

Message #17 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Fri, 07 Feb 2020 11:26:54 +0200

> Cc: 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
> From: Dmitry Gutov <dgutov <at> yandex.ru>
> Date: Fri, 7 Feb 2020 11:43:36 +0300
> 
> On 07.02.2020 10:57, Eli Zaretskii wrote:
> >> From: Dmitry Gutov <dgutov <at> 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".

Message #20 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Fri, 7 Feb 2020 14:43:15 +0300

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.

Message #23 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 39452 <at> debbugs.gnu.org,
 Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Fri, 07 Feb 2020 09:43:51 -0500

Dmitry Gutov <dgutov <at> 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.

Message #26 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Wolfgang Scherer <Wolfgang.Scherer <at> gmx.de>
To: Dmitry Gutov <dgutov <at> yandex.ru>, 39452 <at> debbugs.gnu.org
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Fri, 7 Feb 2020 18:25:27 +0100

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.

Message #29 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Wolfgang Scherer <Wolfgang.Scherer <at> gmx.de>
To: Dmitry Gutov <dgutov <at> yandex.ru>, 39452 <at> debbugs.gnu.org
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Fri, 7 Feb 2020 23:31:52 +0100

[Message part 1 (text/plain, inline)]

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))

[0001-vc-git-state-fails-for-filenames-with-wildcards.patch (text/x-patch, attachment)]

Message #32 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Wed, 12 Feb 2020 01:01:20 +0200

On 07.02.2020 16:43, Noam Postavsky wrote:
> Dmitry Gutov <dgutov <at> 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.

Message #35 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Wed, 12 Feb 2020 10:24:28 -0500

Dmitry Gutov <dgutov <at> yandex.ru> writes:

> On 07.02.2020 16:43, Noam Postavsky wrote:
>> Dmitry Gutov <dgutov <at> 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

Message #38 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Wolfgang Scherer <Wolfgang.Scherer <at> gmx.de>
To: Dmitry Gutov <dgutov <at> yandex.ru>, Noam Postavsky <npostavs <at> gmail.com>
Cc: 39452 <at> debbugs.gnu.org
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Thu, 13 Feb 2020 19:34:42 +0100

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.

Message #41 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Wolfgang Scherer <Wolfgang.Scherer <at> gmx.de>,
 Noam Postavsky <npostavs <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 39452 <at> debbugs.gnu.org
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Fri, 14 Feb 2020 01:23:50 +0200

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.

Message #44 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: npostavs <at> gmail.com, 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Fri, 14 Feb 2020 11:37:57 +0200

> Cc: 39452 <at> debbugs.gnu.org
> From: Dmitry Gutov <dgutov <at> 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.

Message #47 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: npostavs <at> gmail.com, 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
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?

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

Message #50 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: npostavs <at> gmail.com, 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Fri, 14 Feb 2020 16:14:41 +0200

> Cc: npostavs <at> gmail.com, 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
> From: Dmitry Gutov <dgutov <at> 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?

Message #53 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: npostavs <at> gmail.com, 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
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).

Message #56 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: npostavs <at> gmail.com, 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Fri, 14 Feb 2020 17:45:56 +0200

> Cc: npostavs <at> gmail.com, 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
> From: Dmitry Gutov <dgutov <at> 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.

Message #59 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: npostavs <at> gmail.com, 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Fri, 14 Feb 2020 22:37:42 +0200

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.

Message #62 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Noam Postavsky <npostavs <at> gmail.com>,
 39452 <at> debbugs.gnu.org, Wolfgang Scherer <Wolfgang.Scherer <at> gmx.de>
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Sun, 20 Sep 2020 11:54:06 +0200

Dmitry Gutov <dgutov <at> 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

Message #65 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: Wolfgang.Scherer <at> gmx.de, 39452 <at> debbugs.gnu.org,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Wed, 12 May 2021 18:04:47 +0200

Noam Postavsky <npostavs <at> 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

Message #68 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Lars Ingebrigtsen <larsi <at> gnus.org>, Noam Postavsky <npostavs <at> gmail.com>
Cc: 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Mon, 17 May 2021 04:05:15 +0300

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.

Message #71 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: Noam Postavsky <npostavs <at> gmail.com>, 39452 <at> debbugs.gnu.org,
 Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Thu, 22 Jul 2021 14:42:41 +0200

Dmitry Gutov <dgutov <at> 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

Message #76 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Noam Postavsky <npostavs <at> gmail.com>, 39452 <at> debbugs.gnu.org,
 Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Sat, 14 Aug 2021 03:11:56 +0300

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")))

Message #79 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: Noam Postavsky <npostavs <at> gmail.com>, 39452 <at> debbugs.gnu.org,
 Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Sat, 14 Aug 2021 13:56:36 +0200

Dmitry Gutov <dgutov <at> 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

Message #82 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Noam Postavsky <npostavs <at> gmail.com>, 39452 <at> debbugs.gnu.org,
 Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Sun, 15 Aug 2021 04:25:20 +0300

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.

Message #85 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, Noam Postavsky <npostavs <at> gmail.com>,
 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Fri, 27 Aug 2021 09:05:31 +0300

>> 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"))

Message #88 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Juri Linkov <juri <at> linkov.net>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, Noam Postavsky <npostavs <at> gmail.com>,
 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Fri, 27 Aug 2021 15:51:51 +0300

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.

Message #91 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, Noam Postavsky <npostavs <at> gmail.com>,
 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Fri, 27 Aug 2021 20:10:13 +0300

>> -  (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.

Message #94 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Juri Linkov <juri <at> linkov.net>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, Wolfgang.Scherer <at> gmx.de,
 Noam Postavsky <npostavs <at> gmail.com>, 39452 <at> debbugs.gnu.org,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Fri, 27 Aug 2021 21:57:06 +0200

On Fri, 27 Aug 2021 20:10:13 +0300 Juri Linkov <juri <at> 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

Message #97 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Juri Linkov <juri <at> linkov.net>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, Noam Postavsky <npostavs <at> gmail.com>,
 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Sat, 28 Aug 2021 01:47:12 +0300

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.

Message #100 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: Wolfgang.Scherer <at> gmx.de, Dmitry Gutov <dgutov <at> yandex.ru>,
 Noam Postavsky <npostavs <at> gmail.com>, 39452 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Sat, 28 Aug 2021 17:07:01 +0200

Stephen Berman <stephen.berman <at> 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

Message #103 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Wolfgang.Scherer <at> gmx.de, Dmitry Gutov <dgutov <at> yandex.ru>,
 Noam Postavsky <npostavs <at> gmail.com>, 39452 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Sat, 28 Aug 2021 17:44:35 +0200

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

> Stephen Berman <stephen.berman <at> 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

Message #106 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: Wolfgang.Scherer <at> gmx.de, Dmitry Gutov <dgutov <at> yandex.ru>,
 Noam Postavsky <npostavs <at> gmail.com>, 39452 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Sat, 28 Aug 2021 17:48:15 +0200

Stephen Berman <stephen.berman <at> 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

Message #109 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Wolfgang.Scherer <at> gmx.de, Dmitry Gutov <dgutov <at> yandex.ru>,
 Noam Postavsky <npostavs <at> gmail.com>, 39452 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Sat, 28 Aug 2021 18:02:27 +0200

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

> Stephen Berman <stephen.berman <at> 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

Message #112 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Lars Ingebrigtsen <larsi <at> gnus.org>, Stephen Berman <stephen.berman <at> gmx.net>
Cc: Wolfgang.Scherer <at> gmx.de, Noam Postavsky <npostavs <at> gmail.com>,
 39452 <at> debbugs.gnu.org, Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Sun, 29 Aug 2021 01:19:21 +0300

On 28.08.2021 18:48, Lars Ingebrigtsen wrote:
> Stephen Berman <stephen.berman <at> 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).

Message #115 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Juri Linkov <juri <at> linkov.net>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, Noam Postavsky <npostavs <at> gmail.com>,
 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Sun, 29 Aug 2021 03:18:39 +0300

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.

Message #118 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, Noam Postavsky <npostavs <at> gmail.com>,
 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Sun, 29 Aug 2021 19:44:50 +0300

>> 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?

Message #121 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Juri Linkov <juri <at> linkov.net>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, Noam Postavsky <npostavs <at> gmail.com>,
 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Sun, 29 Aug 2021 22:50:11 +0300

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.

Message #124 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: Wolfgang.Scherer <at> gmx.de, Noam Postavsky <npostavs <at> gmail.com>,
 39452 <at> debbugs.gnu.org, Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Sun, 29 Aug 2021 21:57:00 +0200

Dmitry Gutov <dgutov <at> 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

Message #127 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Wolfgang.Scherer <at> gmx.de, Noam Postavsky <npostavs <at> gmail.com>,
 39452 <at> debbugs.gnu.org, Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Sun, 29 Aug 2021 23:11:38 +0300

On 29.08.2021 22:57, Lars Ingebrigtsen wrote:
> Dmitry Gutov<dgutov <at> 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.

Message #130 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Stephen Berman <stephen.berman <at> gmx.net>, Juri Linkov <juri <at> linkov.net>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, Noam Postavsky <npostavs <at> gmail.com>,
 39452 <at> debbugs.gnu.org, Wolfgang.Scherer <at> gmx.de
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Mon, 30 Aug 2021 05:36:06 +0300

[Message part 1 (text/plain, inline)]

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).

[vc-print-log-internal-restore.diff (text/x-patch, attachment)]

Message #133 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, Wolfgang.Scherer <at> gmx.de,
 Noam Postavsky <npostavs <at> gmail.com>, 39452 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Mon, 30 Aug 2021 15:34:04 +0200

On Mon, 30 Aug 2021 05:36:06 +0300 Dmitry Gutov <dgutov <at> 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

Message #136 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, Wolfgang.Scherer <at> gmx.de,
 Noam Postavsky <npostavs <at> gmail.com>, 39452 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#39452: [PATCH] vc-git-state fails for filenames with wildcards
Date: Tue, 31 Aug 2021 02:48:31 +0300

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.

Message #139 received at 39452 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Tino Calancha <tino.calancha <at> gmail.com>, 50422-done <at> debbugs.gnu.org
Cc: 39452 <at> debbugs.gnu.org
Subject: Re: bug#50422: 28.0.50; vc-git-checkout: accept nil as first argument
Date: Wed, 8 Sep 2021 02:34:09 +0300

Hi Tino,

On 06.09.2021 10:11, Tino Calancha wrote:
> X-Debbugs-Cc:39452 <at> debbugs.gnu.org
> 
> Before the fix from bug#39452, the following was a valid call:
> 
> (vc-git-checkout nil "master")
> 
> Note that `vc-git-command' handles a nil 3rd argument.
> (I rely in such a behavior).
> 
> Since we call now `vc-git--literal-pathspec' in several commands, then
> we could add a check at the beginning of this function:

Yes, I broke this with the most recent fix in that area. Sorry.

Should be fixed now with commit ff2c4a8353.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.