bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[dima]

Hi. I'm using emacs built from git. I'm observing that it's no longer
possible to "C-x v l" when looking at version-controlled files over
TRAMP. This is a regression. "git bisect" tells me that the breaking
commit is this:

  3572613550f5d1d0b3392dbc809b32f3989e2981 is the first bad commit
  commit 3572613550f5d1d0b3392dbc809b32f3989e2981
  Author: Wolfgang Scherer <wolfgang.scherer@gmx.de>
  Date:   Sun Aug 15 04:02:23 2021 +0300

      Fix vc-git-state 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 (bug#39452).

      (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 | 63 +++++++++++++++++++++++++++++++------------------------
   1 file changed, 36 insertions(+), 27 deletions(-)

Recipe to reproduce:

1. emacs -Q /ssh:some_server:some_file
   where the remote file is in a git repo

2. C-x v l
   I should see the git log, but instead I get this in the *Messages*:

   vc-deduce-fileset-1: File is not under version control

"C-x v L" still works

Thanks.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Lars Ingebrigtsen]

dima@secretsauce.net writes:

> Hi. I'm using emacs built from git. I'm observing that it's no longer
> possible to "C-x v l" when looking at version-controlled files over
> TRAMP. This is a regression. "git bisect" tells me that the breaking
> commit is this:

[...]

> 1. emacs -Q /ssh:some_server:some_file
>    where the remote file is in a git repo
>
> 2. C-x v l
>    I should see the git log, but instead I get this in the *Messages*:
>
>    vc-deduce-fileset-1: File is not under version control

I'm unable to reproduce this on the current trunk.

I tried:

C-x C-f /ssh:stories:/home/larsi/src/emacs/trunk/etc/NEWS RET
C-x v l

and I got the vc-change-log buffer.

I did see something similar a few weeks ago, but it went away after I
said "make bootstrap" (so it's either a problem that shows up only
sometimes or it's a genuine build thing).  Can you try "make bootstrap"
to see whether that has any effect?

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

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dmitry Gutov]

On 30.10.2021 15:48, Lars Ingebrigtsen wrote:
> I did see something similar a few weeks ago, but it went away after I
> said "make bootstrap" (so it's either a problem that shows up only
> sometimes or it's a genuine build thing).

I think it was bug#51112, which we fixed.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dima Kogan]

Hi. Thanks for replying. Notes inline


Lars Ingebrigtsen <larsi@gnus.org> writes:

> I'm unable to reproduce this on the current trunk.
> I did see something similar a few weeks ago, but it went away after I
> said "make bootstrap" (so it's either a problem that shows up only
> sometimes or it's a genuine build thing).  Can you try "make bootstrap"
> to see whether that has any effect?

This is still a problem for me here:

  commit c3499b8ddc357544a58917bfd3846f88caf5d97c
  Author: Eli Zaretskii <eliz@gnu.org>
  Date:   Fri Oct 29 22:07:27 2021 +0300

I've been building from scratch, which is the same, as "make bootstrap"
I imagine. This was my test in "git bisect"

  git clean -idx &&
  git reset --hard &&
  ./autogen.sh &&
  ./configure --with-gnutls=ifavailable &&
  make -j19 &&
  src/emacs -nw -Q /ssh:SERVER:FILE

I haven't done any debugging other than the bisection. Would you like me
to dig into it in some way?


Dmitry Gutov <dgutov@yandex.ru> writes:

> On 30.10.2021 15:48, Lars Ingebrigtsen wrote:
>> I did see something similar a few weeks ago, but it went away after I
>> said "make bootstrap" (so it's either a problem that shows up only
>> sometimes or it's a genuine build thing).
>
> I think it was bug#51112, which we fixed.

I'm at a later revision than any notes in that bug, so I would guess
this is different.

Thanks.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dmitry Gutov]

On 30.10.2021 22:01, Dima Kogan wrote:
> I haven't done any debugging other than the bisection. Would you like me
> to dig into it in some way?

If you (setq vc-command-messages t), you should be able to see all VC 
commands Emacs tries to run.

If you can catch the exact command (or several) which were constructed 
incorrectly, that would help a lot.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dima Kogan]

Hi. I figured this out.

(setq vc-command-messages t) doesn't do anything: emacs doesn't
recognize this as a valid file in git, so it only runs git commands at
file open time, which apparently aren't reported with
vc-command-messages.

Instead I ran with (setq tramp-verbose 6) and the key line is this:

  23:46:27.247358 tramp-send-command (6) # ( cd DIR && unset GIT_DIR && git --no-pager ls-files -c -z -- \:\(literal\)FILE.c </dev/null 2>/dev/null; echo tramp_exit_status $? )
  23:46:27.258502 tramp-wait-for-regexp (6) # 
  tramp_exit_status 128

Note the :(literal) stuff. This is what was added in the commit that the
bisection found to be the cause of the issue. I can try to run this
command directly (outside of emacs) on the target box:

  $ git --no-pager ls-files -c -z -- ':(literal)FILE.c' </dev/null

  fatal: Invalid pathspec magic 'literal' in ':(literal)FILE.c'

The issue is that this target box has a version of git too old to know
about :(literal):

  kogan@aargh:~/stereo-server$ git --version
  git version 1.8.3.1

Yeah, it's ancient, but I don't control this particular machine, and I
don't think basic stuff like "C-x v l" should be non-functional here.
Can we add some logic to emacs to not hard-depend on this stuff?

Thanks

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Michael Albinus]

Dima Kogan <dima@secretsauce.net> writes:

> Hi. I figured this out.

Hi,

> I can try to run this
> command directly (outside of emacs) on the target box:
>
>   $ git --no-pager ls-files -c -z -- ':(literal)FILE.c' </dev/null
>
>   fatal: Invalid pathspec magic 'literal' in ':(literal)FILE.c'
>
> The issue is that this target box has a version of git too old to know
> about :(literal):
>
>   kogan@aargh:~/stereo-server$ git --version
>   git version 1.8.3.1
>
> Yeah, it's ancient, but I don't control this particular machine, and I
> don't think basic stuff like "C-x v l" should be non-functional here.
> Can we add some logic to emacs to not hard-depend on this stuff?

vc-git.el could declare a variable which determines, whether the git
command supports :(literal) (perhaps it does already). A user could
change this variable via connection-local settings.

> Thanks

Best regards, Michael.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dmitry Gutov]

On 31.10.2021 11:16, Michael Albinus wrote:
> vc-git.el could declare a variable which determines, whether the git
> command supports :(literal) (perhaps it does already). A user could
> change this variable via connection-local settings.

We normally use 'vc-git--program-version' for this.

Can we define the variable 'vc-git--program-version' as connection-local 
or something?

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

Hi Dmitry,

>> vc-git.el could declare a variable which determines, whether the git
>> command supports :(literal) (perhaps it does already). A user could
>> change this variable via connection-local settings.
>
> We normally use 'vc-git--program-version' for this.
>
> Can we define the variable 'vc-git--program-version' as
> connection-local or something?

--8<---------------cut here---------------start------------->8---
(defun vc-git--program-version ()
  (let ((current
	 (if (file-remote-p default-directory)
	     (with-connection-local-variables
	      (and (local-variable-p 'vc-git--program-version)
		   vc-git--program-version))
	   vc-git--program-version)))
    (or current
	(let ((version-string
               (vc-git--run-command-string nil "version")))
          (setq version-string
		(if (and version-string
			 ;; Git for Windows appends ".windows.N" to the
			 ;; numerical version reported by Git.
			 (string-match
                          "git version \\([0-9.]+\\)\\(\\.windows\\.[0-9]+\\)?$"
                          version-string))
                    (match-string 1 version-string)
                  "0"))
	  (if (file-remote-p default-directory)
	      (let ((profile (gensym "connection-local-profile-")))
		(connection-local-set-profile-variables
		 profile `((vc-git--program-version . ,version-string))) ;
		(connection-local-set-profiles
		 (connection-local-criteria-for-default-directory) profile))
	    (setq vc-git--program-version version-string))
	  version-string))))
--8<---------------cut here---------------end--------------->8---

Best regards, Michael.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dmitry Gutov]

Hi Michael,

On 31.10.2021 19:05, Michael Albinus wrote:
> (defun vc-git--program-version ()
>    (let ((current
> 	 (if (file-remote-p default-directory)
> 	     (with-connection-local-variables
> 	      (and (local-variable-p 'vc-git--program-version)
> 		   vc-git--program-version))
> 	   vc-git--program-version)))
>      (or current
> 	(let ((version-string
>                 (vc-git--run-command-string nil "version")))
>            (setq version-string
> 		(if (and version-string
> 			 ;; Git for Windows appends ".windows.N" to the
> 			 ;; numerical version reported by Git.
> 			 (string-match
>                            "git version \\([0-9.]+\\)\\(\\.windows\\.[0-9]+\\)?$"
>                            version-string))
>                      (match-string 1 version-string)
>                    "0"))
> 	  (if (file-remote-p default-directory)
> 	      (let ((profile (gensym "connection-local-profile-")))
> 		(connection-local-set-profile-variables
> 		 profile `((vc-git--program-version . ,version-string))) ;
> 		(connection-local-set-profiles
> 		 (connection-local-criteria-for-default-directory) profile))
> 	    (setq vc-git--program-version version-string))
> 	  version-string))))

I wonder if we could have some helper that is more succinct. One that 
didn't require the client code to auto-generate the profile name, for 
instance.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dmitry Gutov]

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

On 31.10.2021 09:58, Dima Kogan wrote:
> Note the :(literal) stuff. This is what was added in the commit that the
> bisection found to be the cause of the issue. I can try to run this
> command directly (outside of emacs) on the target box:
> 
>    $ git --no-pager ls-files -c -z -- ':(literal)FILE.c' </dev/null
> 
>    fatal: Invalid pathspec magic 'literal' in ':(literal)FILE.c'
> 
> The issue is that this target box has a version of git too old to know
> about :(literal):
> 
>    kogan@aargh:~/stereo-server$ git --version
>    git version 1.8.3.1

Sounds like CentOS 7. Released 7 years ago, but updated last year, even.

> Yeah, it's ancient, but I don't control this particular machine, and I
> don't think basic stuff like "C-x v l" should be non-functional here.
> Can we add some logic to emacs to not hard-depend on this stuff?

Any idea which version of Git introduced literal pathspecs?

The docs on the official website only go back to 2.1.4 (also from 2014), 
which had it already.

Anyway, with Michael's code, see the attached patch you can try.


[-- Attachment #2: no-literal-pathspecs-on-centos.diff --]
[-- Type: text/x-patch, Size: 2651 bytes --]

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 3f89fad235..80455b2010 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -249,7 +249,9 @@ vc-git--literal-pathspec
     ;; Expand abbreviated file names.
     (when (file-name-absolute-p file)
       (setq file (expand-file-name file)))
-    (concat ":(literal)" (file-local-name file))))
+    (if (version<= "2.1.4" (vc-git--program-version))
+        (concat ":(literal)" (file-local-name file))
+      (file-local-name file))))
 
 (defun vc-git--literal-pathspecs (files)
   "Prepend :(literal) path magic to FILES."
@@ -293,18 +295,32 @@ vc-git--state-code
 (defvar vc-git--program-version nil)
 
 (defun vc-git--program-version ()
-  (or vc-git--program-version
-      (let ((version-string
-             (vc-git--run-command-string nil "version")))
-        (setq vc-git--program-version
-              (if (and version-string
-                       ;; Git for Windows appends ".windows.N" to the
-                       ;; numerical version reported by Git.
-                       (string-match
-                        "git version \\([0-9.]+\\)\\(\\.windows\\.[0-9]+\\)?$"
-                        version-string))
-                  (match-string 1 version-string)
-                "0")))))
+  (let ((current
+         (if (file-remote-p default-directory)
+             (with-connection-local-variables
+              (and (local-variable-p 'vc-git--program-version)
+                   vc-git--program-version))
+           vc-git--program-version)))
+    (or current
+        (let ((version-string
+               (vc-git--run-command-string nil "version")))
+          (setq version-string
+                (if (and version-string
+                         ;; Git for Windows appends ".windows.N" to the
+                         ;; numerical version reported by Git.
+                         (string-match
+                          "git version \\([0-9.]+\\)\\(\\.windows\\.[0-9]+\\)?$"
+                          version-string))
+                    (match-string 1 version-string)
+                  "0"))
+          (if (file-remote-p default-directory)
+              (let ((profile (gensym "connection-local-profile-")))
+                (connection-local-set-profile-variables
+                 profile `((vc-git--program-version . ,version-string))) ;
+                (connection-local-set-profiles
+                 (connection-local-criteria-for-default-directory) profile))
+            (setq vc-git--program-version version-string))
+          version-string))))
 
 (defun vc-git--git-status-to-vc-state (code-list)
   "Convert CODE-LIST to a VC status.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dima Kogan]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 31.10.2021 09:58, Dima Kogan wrote:
>>    kogan@aargh:~/stereo-server$ git --version
>>    git version 1.8.3.1
>
> Sounds like CentOS 7. Released 7 years ago, but updated last year,
> even.

Yessir. I wasn't a fan of this even 7 years ago, but it's not a battle I
want to fight.


>> Yeah, it's ancient, but I don't control this particular machine, and I
>> don't think basic stuff like "C-x v l" should be non-functional here.
>> Can we add some logic to emacs to not hard-depend on this stuff?
>
> Any idea which version of Git introduced literal pathspecs?

I think 1.8.5:

  https://github.com/git/git/commit/5c6933d201fab183a9779dca0fe43bf2f1eca098


> Anyway, with Michael's code, see the attached patch you can try.

It fixes the problem. Thank you very much.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dmitry Gutov]

On 03.11.2021 06:03, Dima Kogan wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
>> On 31.10.2021 09:58, Dima Kogan wrote:
>>>     kogan@aargh:~/stereo-server$ git --version
>>>     git version 1.8.3.1
>>
>> Sounds like CentOS 7. Released 7 years ago, but updated last year,
>> even.
> 
> Yessir. I wasn't a fan of this even 7 years ago, but it's not a battle I
> want to fight.
> 
> 
>>> Yeah, it's ancient, but I don't control this particular machine, and I
>>> don't think basic stuff like "C-x v l" should be non-functional here.
>>> Can we add some logic to emacs to not hard-depend on this stuff?
>>
>> Any idea which version of Git introduced literal pathspecs?
> 
> I think 1.8.5:
> 
>    https://github.com/git/git/commit/5c6933d201fab183a9779dca0fe43bf2f1eca098

Looks like it. Thanks!

>> Anyway, with Michael's code, see the attached patch you can try.
> 
> It fixes the problem. Thank you very much.

All right.

Lars, Eli, can we put it in Emacs 28?

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Hi Michael,

Hi Dmitry,

> I wonder if we could have some helper that is more succinct. One that
> didn't require the client code to auto-generate the profile name, for
> instance.

Perhaps. I have some health problems these days, so I will check next
week or so. However, a helper function (in files-x.el) would be good for
Emacs 29 only.

Best regards, Michael.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dmitry Gutov]

Hi Michael,

On 03.11.2021 20:09, Michael Albinus wrote:
>> I wonder if we could have some helper that is more succinct. One that
>> didn't require the client code to auto-generate the profile name, for
>> instance.
> 
> Perhaps. I have some health problems these days, so I will check next
> week or so. However, a helper function (in files-x.el) would be good for
> Emacs 29 only.

Of course.

Take care,
Dmitry.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dmitry Gutov]

On 03.11.2021 15:06, Dmitry Gutov wrote:
> Lars, Eli, can we put it in Emacs 28?

Ping.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Cc: 51497@debbugs.gnu.org, Wolfgang Scherer <wolfgang.scherer@gmx.de>,
>  Dima Kogan <lists@dima.secretsauce.net>
> Date: Sat, 6 Nov 2021 16:22:56 +0300
> 
> On 03.11.2021 15:06, Dmitry Gutov wrote:
> > Lars, Eli, can we put it in Emacs 28?
> 
> Ping.

Sorry for missing the original question.

I'm a bit worried by the function relying on the fact that
default-directory is the directory of the repository.  Wouldn't it be
better to explicitly let-bind it inside the function?

A (perhaps safer) alternative for emacs-28 would be not to use
:(literal) for remote repositories.  What are the disadvantages of
that?

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dmitry Gutov]

On 06.11.2021 18:51, Eli Zaretskii wrote:
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Cc: 51497@debbugs.gnu.org, Wolfgang Scherer <wolfgang.scherer@gmx.de>,
>>   Dima Kogan <lists@dima.secretsauce.net>
>> Date: Sat, 6 Nov 2021 16:22:56 +0300
>>
>> On 03.11.2021 15:06, Dmitry Gutov wrote:
>>> Lars, Eli, can we put it in Emacs 28?
>>
>> Ping.
> 
> Sorry for missing the original question.
> 
> I'm a bit worried by the function relying on the fact that
> default-directory is the directory of the repository.  Wouldn't it be
> better to explicitly let-bind it inside the function?

We could, but notice how most of vc-git-* functions don't bind 
default-directory, thus relying on its implicit value. It just how VC 
works: expecting default-directory to have the right value around the calls.

The only current caller of vc-git--program-version (vc-git-state) does 
not either. The backend methods that do, seem to do that with some 
additional purpose (like having default-directory point to the 
repository root, rather than be a random directory inside it).

> A (perhaps safer) alternative for emacs-28 would be not to use
> :(literal) for remote repositories.  What are the disadvantages of
> that?

That would mean leaving bug#39452 unfixed on remote hosts. Seems like a 
significant disadvantage to me (inconsistent behavior leads to more 
difficult reproduction and reporting of bugs, in particular for those 
who will notice this problem remotely but would not be able to reproduce 
locally). Given that the code complexity added by fixing this bug would 
remain with us, seems more like worst-of-both-worlds kind of situation.

But it would make VC work on remote CentOS 7 hosts again, there's that.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Eli Zaretskii]

> Cc: lists@dima.secretsauce.net, 51497@debbugs.gnu.org, larsi@gnus.org,
>  wolfgang.scherer@gmx.de
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sat, 6 Nov 2021 22:44:55 +0300
> 
> > I'm a bit worried by the function relying on the fact that
> > default-directory is the directory of the repository.  Wouldn't it be
> > better to explicitly let-bind it inside the function?
> 
> We could, but notice how most of vc-git-* functions don't bind 
> default-directory, thus relying on its implicit value. It just how VC 
> works: expecting default-directory to have the right value around the calls.

How certain are you that default-directory has the right value?
Because if it doesn't, AFAIU all the connection-specific stuff will
fall apart.

> > A (perhaps safer) alternative for emacs-28 would be not to use
> > :(literal) for remote repositories.  What are the disadvantages of
> > that?
> 
> That would mean leaving bug#39452 unfixed on remote hosts.

Only for files with wildcard characters in their names.  How
frequently does that happen?  Also, it will be only unsolved in Emacs
28.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Andy Moreton]

On Sat 06 Nov 2021, Eli Zaretskii wrote:

>> Cc: lists@dima.secretsauce.net, 51497@debbugs.gnu.org, larsi@gnus.org,
>>  wolfgang.scherer@gmx.de
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Sat, 6 Nov 2021 22:44:55 +0300
>> 
>> > I'm a bit worried by the function relying on the fact that
>> > default-directory is the directory of the repository.  Wouldn't it be
>> > better to explicitly let-bind it inside the function?
>> 
>> We could, but notice how most of vc-git-* functions don't bind 
>> default-directory, thus relying on its implicit value. It just how VC 
>> works: expecting default-directory to have the right value around the calls.
>
> How certain are you that default-directory has the right value?
> Because if it doesn't, AFAIU all the connection-specific stuff will
> fall apart.
>
>> > A (perhaps safer) alternative for emacs-28 would be not to use
>> > :(literal) for remote repositories.  What are the disadvantages of
>> > that?
>> 
>> That would mean leaving bug#39452 unfixed on remote hosts.
>
> Only for files with wildcard characters in their names.  How
> frequently does that happen?  Also, it will be only unsolved in Emacs
> 28.

I missed the discussion in bug#39452 at the time, but while the solution
was being worked on, I used advice to stub out the literal pathspec
functions:
  (advice-add 'vc-git--literal-pathspec  :override #'identity)
  (advice-add 'vc-git--literal-pathspecs :override #'identity)

That workaround is still needed for me. Without that, nothing in vc-git
works in my environment (64bit minw64 build on win10, using cygwin bash
and git together with cygwin-mount.el from emacs wiki).

While I realise my setup is somewhat non-standard, other users may also
find that the literal pathspec code also misbehaves.

    AndyM

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dmitry Gutov]

On 06.11.2021 22:52, Eli Zaretskii wrote:
>> Cc: lists@dima.secretsauce.net, 51497@debbugs.gnu.org, larsi@gnus.org,
>>   wolfgang.scherer@gmx.de
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Sat, 6 Nov 2021 22:44:55 +0300
>>
>>> I'm a bit worried by the function relying on the fact that
>>> default-directory is the directory of the repository.  Wouldn't it be
>>> better to explicitly let-bind it inside the function?
>>
>> We could, but notice how most of vc-git-* functions don't bind
>> default-directory, thus relying on its implicit value. It just how VC
>> works: expecting default-directory to have the right value around the calls.
> 
> How certain are you that default-directory has the right value?
> Because if it doesn't, AFAIU all the connection-specific stuff will
> fall apart.

Reasonably, but not 100%. Especially with third-party code which calls 
into VC (it could adapt independently from Emacs releases, though).

We could try to bind default-directory inside vc-git--literal-pathspec, 
but this approach is not 100% reliable either: for all I know, sometimes 
FILE will be a relative name (we even have a file-name-absolute-p check 
inside).

But what's the worst thing that can happen because of this? Suppose some 
caller will leave default-directory at a wrong value. Then 
vc-git--program-version will return the version from a wrong host. And 
some particular command (probably a less popular one) will remain broken 
on remote CentOS 7 machines. That's still an improvement compared to the 
current sutuation.

So I suggest we push the proposed change to emacs-28 and then maybe back 
it out (or modify as necessary) if problems arise.

>>> A (perhaps safer) alternative for emacs-28 would be not to use
>>> :(literal) for remote repositories.  What are the disadvantages of
>>> that?
>>
>> That would mean leaving bug#39452 unfixed on remote hosts.
> 
> Only for files with wildcard characters in their names.  How
> frequently does that happen?  Also, it will be only unsolved in Emacs
> 28.

I've never seen this in practice, but some other categories of users 
might encounter it more often (e.g. science people, who tend to use more 
exotic files names). And when one does see it, it must be very annoying 
to debug.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dmitry Gutov]

On 07.11.2021 01:11, Andy Moreton wrote:
> That workaround is still needed for me. Without that, nothing in vc-git
> works in my environment (64bit minw64 build on win10, using cygwin bash
> and git together with cygwin-mount.el from emacs wiki).

Does that environment also have an old version of Git? Or is there 
another reason why it's broken?

> While I realise my setup is somewhat non-standard, other users may also
> find that the literal pathspec code also misbehaves.

I would like to fix it for all users, but debugging would fall on your 
shoulders, I'm afraid.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Andy Moreton]

On Sun 07 Nov 2021, Dmitry Gutov wrote:

> On 07.11.2021 01:11, Andy Moreton wrote:
>> That workaround is still needed for me. Without that, nothing in vc-git
>> works in my environment (64bit minw64 build on win10, using cygwin bash
>> and git together with cygwin-mount.el from emacs wiki).
>
> Does that environment also have an old version of Git? Or is there another
> reason why it's broken?

I have git 2.33 in cygwin and MSYS2, so git is not old. I'll look at
this again now that the changes have stablised.

>> While I realise my setup is somewhat non-standard, other users may also
>> find that the literal pathspec code also misbehaves.
>
> I would like to fix it for all users, but debugging would fall on your
> shoulders, I'm afraid.

My note was more to warn that adding this to emacs-28 may bring
problems.

Looking at this again, Trying "C-x v l" for INSTALL in the repo master
branch gives (rewrapped for clarity):

  fatal: :(literal)c:/emacs/git/emacs/master/nt/INSTALL:
  'c:/emacs/git/emacs/master/nt/INSTALL' is outside repository at
  '/c/emacs/git/emacs/master'

This appears to be due to the translation between win32 and cygwin
(posix) filenames.

    AndyM

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dmitry Gutov]

On 07.11.2021 02:03, Andy Moreton wrote:
> On Sun 07 Nov 2021, Dmitry Gutov wrote:
> 
>> On 07.11.2021 01:11, Andy Moreton wrote:
>>> That workaround is still needed for me. Without that, nothing in vc-git
>>> works in my environment (64bit minw64 build on win10, using cygwin bash
>>> and git together with cygwin-mount.el from emacs wiki).
>>
>> Does that environment also have an old version of Git? Or is there another
>> reason why it's broken?
> 
> I have git 2.33 in cygwin and MSYS2, so git is not old. I'll look at
> this again now that the changes have stablised.

It would have been nice to receive this feedback before the emacs-28 
branch was cut, when we had more freedom to alter the implementation.

>>> While I realise my setup is somewhat non-standard, other users may also
>>> find that the literal pathspec code also misbehaves.
>>
>> I would like to fix it for all users, but debugging would fall on your
>> shoulders, I'm afraid.
> 
> My note was more to warn that adding this to emacs-28 may bring
> problems.

Adding what? The literal pathspec stuff is already there.

> Looking at this again, Trying "C-x v l" for INSTALL in the repo master
> branch gives (rewrapped for clarity):
> 
>    fatal: :(literal)c:/emacs/git/emacs/master/nt/INSTALL:
>    'c:/emacs/git/emacs/master/nt/INSTALL' is outside repository at
>    '/c/emacs/git/emacs/master'
> 
> This appears to be due to the translation between win32 and cygwin
> (posix) filenames.

It might be fixable inside vc-git--literal-pathspec. Is there some other 
more general path conversion function we should use instead of 
'file-local-name' there? A tested patch would help a lot.

Failing that, I think we'll need to change the "literal pathspecs" 
implementation to yet another approach (adding --literl-pathspecs flag 
instead of manipulating file names). It comes with the same general 
drawbacks as the env var (which is used under the hood), but the 
explicit approach of specifying it in every command would avoid the 
problem of my original fix for that bug.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Sat, 06 Nov 2021 22:11:03 +0000
> 
> I missed the discussion in bug#39452 at the time, but while the solution
> was being worked on, I used advice to stub out the literal pathspec
> functions:
>   (advice-add 'vc-git--literal-pathspec  :override #'identity)
>   (advice-add 'vc-git--literal-pathspecs :override #'identity)
> 
> That workaround is still needed for me. Without that, nothing in vc-git
> works in my environment (64bit minw64 build on win10, using cygwin bash
> and git together with cygwin-mount.el from emacs wiki).

We need to understand why the original code doesn't work for you,
otherwise we cannot decide what to do about that issue.  FWIW, Git
works for me on Windows from Emacs without any changes.  But I don't
use the Cygwin Bash and cygwin-mount.el, which I always warned people
about.  If you want to use Cygwin, why not also use Cygwin Emacs and
Git, and save yourself those troubles?

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Sat, 06 Nov 2021 23:03:24 +0000
> 
> Looking at this again, Trying "C-x v l" for INSTALL in the repo master
> branch gives (rewrapped for clarity):
> 
>   fatal: :(literal)c:/emacs/git/emacs/master/nt/INSTALL:
>   'c:/emacs/git/emacs/master/nt/INSTALL' is outside repository at
>   '/c/emacs/git/emacs/master'
> 
> This appears to be due to the translation between win32 and cygwin
> (posix) filenames.

Right, and so the immediate suspect is cygwin-mount.el.  That command
works flawlessly for me, FWIW.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sun, 7 Nov 2021 03:11:43 +0300
> 
> Failing that, I think we'll need to change the "literal pathspecs" 
> implementation to yet another approach (adding --literl-pathspecs flag 
> instead of manipulating file names). It comes with the same general 
> drawbacks as the env var (which is used under the hood), but the 
> explicit approach of specifying it in every command would avoid the 
> problem of my original fix for that bug.

Why wasn't --literal-pathspecs used in the first place? what are the
downsides?  IME, using magic file names is always worse, because it
can run afoul of various shells that consider some characters special.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Andy Moreton]

On Sun 07 Nov 2021, Eli Zaretskii wrote:

>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Sun, 7 Nov 2021 03:11:43 +0300
>> 
>> Failing that, I think we'll need to change the "literal pathspecs" 
>> implementation to yet another approach (adding --literl-pathspecs flag 
>> instead of manipulating file names). It comes with the same general 
>> drawbacks as the env var (which is used under the hood), but the 
>> explicit approach of specifying it in every command would avoid the 
>> problem of my original fix for that bug.
>
> Why wasn't --literal-pathspecs used in the first place? what are the
> downsides?  IME, using magic file names is always worse, because it
> can run afoul of various shells that consider some characters special.

Indeed. I agree it is much simpler to use the flag.

    AndyM

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Andy Moreton]

On Sun 07 Nov 2021, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Sat, 06 Nov 2021 23:03:24 +0000
>> 
>> Looking at this again, Trying "C-x v l" for INSTALL in the repo master
>> branch gives (rewrapped for clarity):
>> 
>>   fatal: :(literal)c:/emacs/git/emacs/master/nt/INSTALL:
>>   'c:/emacs/git/emacs/master/nt/INSTALL' is outside repository at
>>   '/c/emacs/git/emacs/master'
>> 
>> This appears to be due to the translation between win32 and cygwin
>> (posix) filenames.
>
> Right, and so the immediate suspect is cygwin-mount.el.  That command
> works flawlessly for me, FWIW.

Yes. It is probably due to the ":(literal)" prefix names not matching
the patterns that cygwin-mount.el adds to file-name-handler-alist.

    AndyM

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dmitry Gutov]

On 07.11.2021 09:47, Eli Zaretskii wrote:
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Sun, 7 Nov 2021 03:11:43 +0300
>>
>> Failing that, I think we'll need to change the "literal pathspecs"
>> implementation to yet another approach (adding --literl-pathspecs flag
>> instead of manipulating file names). It comes with the same general
>> drawbacks as the env var (which is used under the hood), but the
>> explicit approach of specifying it in every command would avoid the
>> problem of my original fix for that bug.
> 
> Why wasn't --literal-pathspecs used in the first place? what are the
> downsides?  IME, using magic file names is always worse, because it
> can run afoul of various shells that consider some characters special.

It wasn't among the proposed solutions.

I went with the env var solution initially (because it required less 
code and brought fewer -- none -- Git version compatibility problems), 
but it didn't yield itself as easily to the per-action opt-in as the 
other proposal (currently installed).

But now that I think about it, it would be possible to do this without a 
new macro, just adding a new variable that default to nil, and set it to 
t in every backend method that needs it.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Eli Zaretskii]

> Cc: 51497@debbugs.gnu.org, andrewjmoreton@gmail.com
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 8 Nov 2021 01:36:18 +0300
> 
> > Why wasn't --literal-pathspecs used in the first place? what are the
> > downsides?  IME, using magic file names is always worse, because it
> > can run afoul of various shells that consider some characters special.
> 
> It wasn't among the proposed solutions.
> 
> I went with the env var solution initially (because it required less 
> code and brought fewer -- none -- Git version compatibility problems), 
> but it didn't yield itself as easily to the per-action opt-in as the 
> other proposal (currently installed).
> 
> But now that I think about it, it would be possible to do this without a 
> new macro, just adding a new variable that default to nil, and set it to 
> t in every backend method that needs it.

But would that solve our problems for which :(literal) was introduced?
AFAIU, the difference between that and --literal-pathspecs is that the
latter is global: it affects all the file names of the Git command,
while the former can be applied only to some file names.  Do we have
valid use cases where only some of the file names need to be treated
as literal?

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dmitry Gutov]

On 08.11.2021 15:49, Eli Zaretskii wrote:
>> Cc: 51497@debbugs.gnu.org, andrewjmoreton@gmail.com
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Mon, 8 Nov 2021 01:36:18 +0300
>>
>>> Why wasn't --literal-pathspecs used in the first place? what are the
>>> downsides?  IME, using magic file names is always worse, because it
>>> can run afoul of various shells that consider some characters special.
>>
>> It wasn't among the proposed solutions.
>>
>> I went with the env var solution initially (because it required less
>> code and brought fewer -- none -- Git version compatibility problems),
>> but it didn't yield itself as easily to the per-action opt-in as the
>> other proposal (currently installed).
>>
>> But now that I think about it, it would be possible to do this without a
>> new macro, just adding a new variable that default to nil, and set it to
>> t in every backend method that needs it.
> 
> But would that solve our problems for which :(literal) was introduced?
> AFAIU, the difference between that and --literal-pathspecs is that the
> latter is global: it affects all the file names of the Git command,
> while the former can be applied only to some file names.

Both can be used per-command, but indeed it's true: the :(literal) 
syntax can also be used to apply to individual specs only.

>Do we have
> valid use cases where only some of the file names need to be treated
> as literal?

Even though it's plausible, I haven't encountered this particular use 
case so far. Perhaps when we do, we could mix-and-match :(literal) and 
--literal-pathspecs.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Eli Zaretskii]

> Cc: 51497@debbugs.gnu.org, andrewjmoreton@gmail.com
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 8 Nov 2021 20:30:55 +0300
> 
> >> But now that I think about it, it would be possible to do this without a
> >> new macro, just adding a new variable that default to nil, and set it to
> >> t in every backend method that needs it.
> > 
> > But would that solve our problems for which :(literal) was introduced?
> > AFAIU, the difference between that and --literal-pathspecs is that the
> > latter is global: it affects all the file names of the Git command,
> > while the former can be applied only to some file names.
> 
> Both can be used per-command, but indeed it's true: the :(literal) 
> syntax can also be used to apply to individual specs only.
> 
> >Do we have
> > valid use cases where only some of the file names need to be treated
> > as literal?
> 
> Even though it's plausible, I haven't encountered this particular use 
> case so far. Perhaps when we do, we could mix-and-match :(literal) and 
> --literal-pathspecs.

So what would you suggest as the way forward, for both emacs-28 and
the master branches (the 2 solutions could be different)?  Do you
still prefer to go with your original patch for emacs-28?

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> Even though it's plausible, I haven't encountered this particular use 
>> case so far. Perhaps when we do, we could mix-and-match :(literal) and 
>> --literal-pathspecs.
>
> So what would you suggest as the way forward, for both emacs-28 and
> the master branches (the 2 solutions could be different)?  Do you
> still prefer to go with your original patch for emacs-28?

This was the final message in this thread, and it was six weeks ago.  I
haven't paid attention to the patches in this area -- is this still an
issue, or has it been fixed?

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

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dmitry Gutov]

On 23.12.2021 13:28, Lars Ingebrigtsen wrote:
> Eli Zaretskii<eliz@gnu.org>  writes:
> 
>>> Even though it's plausible, I haven't encountered this particular use
>>> case so far. Perhaps when we do, we could mix-and-match :(literal) and
>>> --literal-pathspecs.
>> So what would you suggest as the way forward, for both emacs-28 and
>> the master branches (the 2 solutions could be different)?  Do you
>> still prefer to go with your original patch for emacs-28?
> This was the final message in this thread, and it was six weeks ago.  I
> haven't paid attention to the patches in this area -- is this still an
> issue, or has it been fixed?

Not fixed, no.

I'll send a patch which reverts to my original approach with an escape 
hatch, tomorrow.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dmitry Gutov]

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

On 08.11.2021 21:18, Eli Zaretskii wrote:
> So what would you suggest as the way forward, for both emacs-28 and
> the master branches (the 2 solutions could be different)?  Do you
> still prefer to go with your original patch for emacs-28?

I'm still of two minds a little bit: conceptually, the current approach 
is a little cleaner because it forces the opt-in approach, and thus 
won't affect any command (or use of functions like 
vc-git--run-command-string outside of vc-git.el) that didn't opt into 
using literal pathspecs.

But my original approach is simpler and shorter, and together with an 
opt-out var seems to solve every problem so far. It doesn't need version 
detection either (a patch to have it work on remote hosts was discussed 
previously here).

So here's the patch (my current preferred solution for both emacs-28 and 
master).

Waiting for feedback from AndyM.

[-- Attachment #2: redo-literal-pathspecs.diff --]
[-- Type: text/x-patch, Size: 11333 bytes --]

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 3b634471ac..4c4eb915ed 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -502,10 +502,12 @@ project-files
 (declare-function vc-hg-command "vc-hg")
 
 (defun project--vc-list-files (dir backend extra-ignores)
+  (defvar vc-git-use-literal-pathspecs)
   (pcase backend
     (`Git
      (let ((default-directory (expand-file-name (file-name-as-directory dir)))
            (args '("-z"))
+           (vc-git-use-literal-pathspecs nil)
            files)
        ;; Include unregistered.
        (setq args (append args '("-c" "-o" "--exclude-standard")))
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 5c6a39aec9..19264c9d3c 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -223,6 +223,12 @@ vc-git-revision-complete-only-branches
 ;; History of Git commands.
 (defvar vc-git-history nil)
 
+;; Default to t because commands which don't support literal pathspecs
+;; ignore the environment variable silently.
+(defvar vc-git-use-literal-pathspecs t
+  "Non-nil to treat pathspecs in commands literally.
+Good example of file name that needs this: \"test[56].xx\".")
+
 ;; Clear up the cache to force vc-call to check again and discover
 ;; new functions when we reload this file.
 (put 'Git 'vc-functions nil)
@@ -242,20 +248,6 @@ vc-git-update-on-retrieve-tag
 ;;;###autoload         (load "vc-git" nil t)
 ;;;###autoload         (vc-git-registered file))))
 
-;; Good example of file name that needs this: "test[56].xx".
-(defun vc-git--literal-pathspec (file)
-  "Prepend :(literal) path magic to FILE."
-  (when file
-    ;; Expand abbreviated file names.
-    (when (file-name-absolute-p file)
-      (setq file (expand-file-name file)))
-    (concat ":(literal)" (file-local-name file))))
-
-(defun vc-git--literal-pathspecs (files)
-  "Prepend :(literal) path magic to FILES."
-  (unless (vc-git--file-list-is-rootdir files)
-    (mapcar #'vc-git--literal-pathspec files)))
-
 (defun vc-git-registered (file)
   "Check whether FILE is registered with git."
   (let ((dir (vc-git-root file)))
@@ -269,12 +261,12 @@ vc-git-registered
                (name (file-relative-name file dir))
                (str (with-demoted-errors "Error: %S"
                       (cd dir)
-                      (vc-git--out-ok "ls-files" "-c" "-z" "--" (vc-git--literal-pathspec name))
+                      (vc-git--out-ok "ls-files" "-c" "-z" "--" 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"
-                                        "--" (vc-git--literal-pathspec name)))
+                                        "--" name))
                       (buffer-string))))
           (and str
                (> (length str) (length name))
@@ -358,7 +350,7 @@ vc-git-state
             ,@(when (version<= "1.7.6.3" (vc-git--program-version))
                 '("--ignored"))
             "--"))
-        (status (apply #'vc-git--run-command-string (vc-git--literal-pathspec file) args)))
+        (status (apply #'vc-git--run-command-string 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.
@@ -636,28 +628,28 @@ vc-git-dir-status-goto-stage
     (pcase (vc-git-dir-status-state->stage git-state)
       ('update-index
        (if files
-           (vc-git-command (current-buffer) 'async (vc-git--literal-pathspecs files) "add" "--refresh" "--")
+           (vc-git-command (current-buffer) 'async files "add" "--refresh" "--")
          (vc-git-command (current-buffer) 'async nil
                          "update-index" "--refresh")))
       ('ls-files-added
-       (vc-git-command (current-buffer) 'async (vc-git--literal-pathspecs files)
+       (vc-git-command (current-buffer) 'async files
                        "ls-files" "-z" "-c" "-s" "--"))
       ('ls-files-up-to-date
-       (vc-git-command (current-buffer) 'async (vc-git--literal-pathspecs files)
+       (vc-git-command (current-buffer) 'async files
                        "ls-files" "-z" "-c" "-s" "--"))
       ('ls-files-conflict
-       (vc-git-command (current-buffer) 'async (vc-git--literal-pathspecs files)
+       (vc-git-command (current-buffer) 'async files
                        "ls-files" "-z" "-u" "--"))
       ('ls-files-unknown
-       (vc-git-command (current-buffer) 'async (vc-git--literal-pathspecs files)
+       (vc-git-command (current-buffer) 'async files
                        "ls-files" "-z" "-o" "--exclude-standard" "--"))
       ('ls-files-ignored
-       (vc-git-command (current-buffer) 'async (vc-git--literal-pathspecs files)
+       (vc-git-command (current-buffer) 'async 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 (vc-git--literal-pathspecs files)
+       (vc-git-command (current-buffer) 'async files
                        "diff-index" "--relative" "-z" "-M" "HEAD" "--")))
     (vc-run-delayed
       (vc-git-after-dir-status-stage git-state))))
@@ -885,12 +877,12 @@ vc-git-register
     (when flist
       (vc-git-command nil 0 flist "update-index" "--add" "--"))
     (when dlist
-      (vc-git-command nil 0 (vc-git--literal-pathspecs dlist) "add"))))
+      (vc-git-command nil 0 dlist "add"))))
 
 (defalias 'vc-git-responsible-p #'vc-git-root)
 
 (defun vc-git-unregister (file)
-  (vc-git-command nil 0 (vc-git--literal-pathspec file) "rm" "-f" "--cached" "--"))
+  (vc-git-command nil 0 file "rm" "-f" "--cached" "--"))
 
 (declare-function log-edit-mode "log-edit" ())
 (declare-function log-edit-toggle-header "log-edit" (header value))
@@ -956,7 +948,7 @@ vc-git-checkin
                (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 (vc-git--literal-pathspecs files))
+      (apply #'vc-git-command nil 0 (if only files)
              (nconc (if msg-file (list "commit" "-F"
                                        (file-local-name msg-file))
                       (list "commit" "-m"))
@@ -983,7 +975,7 @@ vc-git-find-revision
 	 (coding-system-for-write 'binary)
 	 (fullname
 	  (let ((fn (vc-git--run-command-string
-		     (vc-git--literal-pathspec file) "ls-files" "-z" "--full-name" "--")))
+		     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 "")
@@ -1000,14 +992,14 @@ vc-git-find-ignore-file
 		    (vc-git-root file)))
 
 (defun vc-git-checkout (file &optional rev)
-  (vc-git-command nil 0 (vc-git--literal-pathspec file) "checkout" (or rev "HEAD")))
+  (vc-git-command nil 0 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 (vc-git--literal-pathspec file) "reset" "-q" "--")
-    (vc-git-command nil nil (vc-git--literal-pathspec file) "checkout" "-q" "--")))
+    (vc-git-command nil 0 file "reset" "-q" "--")
+    (vc-git-command nil nil file "checkout" "-q" "--")))
 
 (defvar vc-git-error-regexp-alist
   '(("^ \\(.+\\)\\> *|" 1 nil nil 0))
@@ -1091,7 +1083,7 @@ vc-git-merge-branch
 (defun vc-git-conflicted-files (directory)
   "Return the list of files with conflicts in DIRECTORY."
   (let* ((status
-          (vc-git--run-command-string (vc-git--literal-pathspec directory) "status" "--porcelain" "--"))
+          (vc-git--run-command-string directory "status" "--porcelain" "--"))
          (lines (when status (split-string status "\n" 'omit-nulls)))
          files)
     (dolist (line lines files)
@@ -1180,7 +1172,7 @@ vc-git-print-log
     (let ((inhibit-read-only t))
       (with-current-buffer buffer
 	(apply #'vc-git-command buffer
-	       'async (vc-git--literal-pathspecs files)
+	       'async files
 	       (append
 		'("log" "--no-color")
                 (when (and vc-git-print-log-follow
@@ -1434,7 +1426,7 @@ vc-git-diff
     (if vc-git-diff-switches
         (apply #'vc-git-command (or buffer "*vc-diff*")
 	       1 ; bug#21969
-	       (vc-git--literal-pathspecs files)
+               files
                command
                "--exit-code"
                (append (vc-switches 'git 'diff)
@@ -1519,7 +1511,7 @@ vc-git-previous-revision
       (let* ((fname (file-relative-name file))
              (prev-rev (with-temp-buffer
                          (and
-                          (vc-git--out-ok "rev-list" "-2" rev "--" (vc-git--literal-pathspec fname))
+                          (vc-git--out-ok "rev-list" "-2" rev "--" fname)
                           (goto-char (point-max))
                           (bolp)
                           (zerop (forward-line -1))
@@ -1547,7 +1539,7 @@ vc-git-next-revision
          (current-rev
           (with-temp-buffer
             (and
-             (vc-git--out-ok "rev-list" "-1" rev "--" (vc-git--literal-pathspec file))
+             (vc-git--out-ok "rev-list" "-1" rev "--" file)
              (goto-char (point-max))
              (bolp)
              (zerop (forward-line -1))
@@ -1559,7 +1551,7 @@ vc-git-next-revision
           (and current-rev
                (with-temp-buffer
                  (and
-                  (vc-git--out-ok "rev-list" "HEAD" "--" (vc-git--literal-pathspec file))
+                  (vc-git--out-ok "rev-list" "HEAD" "--" file)
                   (goto-char (point-min))
                   (search-forward current-rev nil t)
                   (zerop (forward-line -1))
@@ -1569,13 +1561,13 @@ vc-git-next-revision
     (or (vc-git-symbolic-commit next-rev) next-rev)))
 
 (defun vc-git-delete-file (file)
-  (vc-git-command nil 0 (vc-git--literal-pathspec file) "rm" "-f" "--"))
+  (vc-git-command nil 0 file "rm" "-f" "--"))
 
 (defun vc-git-rename-file (old new)
   (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"))
+  (vc-git-command nil 0 files "add"))
 
 (defvar vc-git-extra-menu-map
   (let ((map (make-sparse-keymap)))
@@ -1797,6 +1789,8 @@ vc-git-command
         (process-environment
          (append
           `("GIT_DIR"
+            ,@(when vc-git-use-literal-pathspecs
+                "GIT_LITERAL_PATHSPECS=1")
             ;; Avoid repository locking during background operations
             ;; (bug#21559).
             ,@(when revert-buffer-in-progress-p
@@ -1834,6 +1828,8 @@ vc-git--call
 	(process-environment
 	 (append
 	  `("GIT_DIR"
+            ,@(when vc-git-use-literal-pathspecs
+                "GIT_LITERAL_PATHSPECS=1")
 	    ;; Avoid repository locking during background operations
 	    ;; (bug#21559).
 	    ,@(when revert-buffer-in-progress-p

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dmitry Gutov]

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

On 27.12.2021 04:36, Dmitry Gutov wrote:
> So here's the patch (my current preferred solution for both emacs-28 and 
> master).
> 
> Waiting for feedback from AndyM.

Sorry, here's a version of the patch that doesn't break vc-checkin.

Waiting for Andy's feedback, I wouldn't mind some testing from Dima as 
well, BTW.

[-- Attachment #2: redo-literal-pathspecs.diff --]
[-- Type: text/x-patch, Size: 10688 bytes --]

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 2d35061b26..4c873b7264 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -223,6 +223,12 @@ vc-git-revision-complete-only-branches
 ;; History of Git commands.
 (defvar vc-git-history nil)
 
+;; Default to t because commands which don't support literal pathspecs
+;; ignore the environment variable silently.
+(defvar vc-git-use-literal-pathspecs t
+  "Non-nil to treat pathspecs in commands literally.
+Good example of file name that needs this: \"test[56].xx\".")
+
 ;; Clear up the cache to force vc-call to check again and discover
 ;; new functions when we reload this file.
 (put 'Git 'vc-functions nil)
@@ -242,20 +248,6 @@ vc-git-update-on-retrieve-tag
 ;;;###autoload         (load "vc-git" nil t)
 ;;;###autoload         (vc-git-registered file))))
 
-;; Good example of file name that needs this: "test[56].xx".
-(defun vc-git--literal-pathspec (file)
-  "Prepend :(literal) path magic to FILE."
-  (when file
-    ;; Expand abbreviated file names.
-    (when (file-name-absolute-p file)
-      (setq file (expand-file-name file)))
-    (concat ":(literal)" (file-local-name file))))
-
-(defun vc-git--literal-pathspecs (files)
-  "Prepend :(literal) path magic to FILES."
-  (unless (vc-git--file-list-is-rootdir files)
-    (mapcar #'vc-git--literal-pathspec files)))
-
 (defun vc-git-registered (file)
   "Check whether FILE is registered with git."
   (let ((dir (vc-git-root file)))
@@ -269,12 +261,12 @@ vc-git-registered
                (name (file-relative-name file dir))
                (str (with-demoted-errors "Error: %S"
                       (cd dir)
-                      (vc-git--out-ok "ls-files" "-c" "-z" "--" (vc-git--literal-pathspec name))
+                      (vc-git--out-ok "ls-files" "-c" "-z" "--" 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"
-                                        "--" (vc-git--literal-pathspec name)))
+                                        "--" name))
                       (buffer-string))))
           (and str
                (> (length str) (length name))
@@ -356,7 +348,7 @@ vc-git-state
             ,@(when (version<= "1.7.6.3" (vc-git--program-version))
                 '("--ignored"))
             "--"))
-        (status (apply #'vc-git--run-command-string (vc-git--literal-pathspec file) args)))
+        (status (apply #'vc-git--run-command-string 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.
@@ -634,28 +626,28 @@ vc-git-dir-status-goto-stage
     (pcase (vc-git-dir-status-state->stage git-state)
       ('update-index
        (if files
-           (vc-git-command (current-buffer) 'async (vc-git--literal-pathspecs files) "add" "--refresh" "--")
+           (vc-git-command (current-buffer) 'async files "add" "--refresh" "--")
          (vc-git-command (current-buffer) 'async nil
                          "update-index" "--refresh")))
       ('ls-files-added
-       (vc-git-command (current-buffer) 'async (vc-git--literal-pathspecs files)
+       (vc-git-command (current-buffer) 'async files
                        "ls-files" "-z" "-c" "-s" "--"))
       ('ls-files-up-to-date
-       (vc-git-command (current-buffer) 'async (vc-git--literal-pathspecs files)
+       (vc-git-command (current-buffer) 'async files
                        "ls-files" "-z" "-c" "-s" "--"))
       ('ls-files-conflict
-       (vc-git-command (current-buffer) 'async (vc-git--literal-pathspecs files)
+       (vc-git-command (current-buffer) 'async files
                        "ls-files" "-z" "-u" "--"))
       ('ls-files-unknown
-       (vc-git-command (current-buffer) 'async (vc-git--literal-pathspecs files)
+       (vc-git-command (current-buffer) 'async files
                        "ls-files" "-z" "-o" "--exclude-standard" "--"))
       ('ls-files-ignored
-       (vc-git-command (current-buffer) 'async (vc-git--literal-pathspecs files)
+       (vc-git-command (current-buffer) 'async 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 (vc-git--literal-pathspecs files)
+       (vc-git-command (current-buffer) 'async files
                        "diff-index" "--relative" "-z" "-M" "HEAD" "--")))
     (vc-run-delayed
       (vc-git-after-dir-status-stage git-state))))
@@ -883,12 +875,12 @@ vc-git-register
     (when flist
       (vc-git-command nil 0 flist "update-index" "--add" "--"))
     (when dlist
-      (vc-git-command nil 0 (vc-git--literal-pathspecs dlist) "add"))))
+      (vc-git-command nil 0 dlist "add"))))
 
 (defalias 'vc-git-responsible-p #'vc-git-root)
 
 (defun vc-git-unregister (file)
-  (vc-git-command nil 0 (vc-git--literal-pathspec file) "rm" "-f" "--cached" "--"))
+  (vc-git-command nil 0 file "rm" "-f" "--cached" "--"))
 
 (declare-function log-edit-mode "log-edit" ())
 (declare-function log-edit-toggle-header "log-edit" (header value))
@@ -954,7 +946,7 @@ vc-git-checkin
                (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 (vc-git--literal-pathspecs files))
+      (apply #'vc-git-command nil 0 (if only files)
              (nconc (if msg-file (list "commit" "-F"
                                        (file-local-name msg-file))
                       (list "commit" "-m"))
@@ -981,7 +973,7 @@ vc-git-find-revision
 	 (coding-system-for-write 'binary)
 	 (fullname
 	  (let ((fn (vc-git--run-command-string
-		     (vc-git--literal-pathspec file) "ls-files" "-z" "--full-name" "--")))
+		     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 "")
@@ -998,14 +990,14 @@ vc-git-find-ignore-file
 		    (vc-git-root file)))
 
 (defun vc-git-checkout (file &optional rev)
-  (vc-git-command nil 0 (vc-git--literal-pathspec file) "checkout" (or rev "HEAD")))
+  (vc-git-command nil 0 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 (vc-git--literal-pathspec file) "reset" "-q" "--")
-    (vc-git-command nil nil (vc-git--literal-pathspec file) "checkout" "-q" "--")))
+    (vc-git-command nil 0 file "reset" "-q" "--")
+    (vc-git-command nil nil file "checkout" "-q" "--")))
 
 (defvar vc-git-error-regexp-alist
   '(("^ \\(.+\\)\\> *|" 1 nil nil 0))
@@ -1089,7 +1081,7 @@ vc-git-merge-branch
 (defun vc-git-conflicted-files (directory)
   "Return the list of files with conflicts in DIRECTORY."
   (let* ((status
-          (vc-git--run-command-string (vc-git--literal-pathspec directory) "status" "--porcelain" "--"))
+          (vc-git--run-command-string directory "status" "--porcelain" "--"))
          (lines (when status (split-string status "\n" 'omit-nulls)))
          files)
     (dolist (line lines files)
@@ -1178,7 +1170,7 @@ vc-git-print-log
     (let ((inhibit-read-only t))
       (with-current-buffer buffer
 	(apply #'vc-git-command buffer
-	       'async (vc-git--literal-pathspecs files)
+	       'async files
 	       (append
 		'("log" "--no-color")
                 (when (and vc-git-print-log-follow
@@ -1432,7 +1424,7 @@ vc-git-diff
     (if vc-git-diff-switches
         (apply #'vc-git-command (or buffer "*vc-diff*")
 	       1 ; bug#21969
-	       (vc-git--literal-pathspecs files)
+               files
                command
                "--exit-code"
                (append (vc-switches 'git 'diff)
@@ -1517,7 +1509,7 @@ vc-git-previous-revision
       (let* ((fname (file-relative-name file))
              (prev-rev (with-temp-buffer
                          (and
-                          (vc-git--out-ok "rev-list" "-2" rev "--" (vc-git--literal-pathspec fname))
+                          (vc-git--out-ok "rev-list" "-2" rev "--" fname)
                           (goto-char (point-max))
                           (bolp)
                           (zerop (forward-line -1))
@@ -1545,7 +1537,7 @@ vc-git-next-revision
          (current-rev
           (with-temp-buffer
             (and
-             (vc-git--out-ok "rev-list" "-1" rev "--" (vc-git--literal-pathspec file))
+             (vc-git--out-ok "rev-list" "-1" rev "--" file)
              (goto-char (point-max))
              (bolp)
              (zerop (forward-line -1))
@@ -1557,7 +1549,7 @@ vc-git-next-revision
           (and current-rev
                (with-temp-buffer
                  (and
-                  (vc-git--out-ok "rev-list" "HEAD" "--" (vc-git--literal-pathspec file))
+                  (vc-git--out-ok "rev-list" "HEAD" "--" file)
                   (goto-char (point-min))
                   (search-forward current-rev nil t)
                   (zerop (forward-line -1))
@@ -1567,13 +1559,13 @@ vc-git-next-revision
     (or (vc-git-symbolic-commit next-rev) next-rev)))
 
 (defun vc-git-delete-file (file)
-  (vc-git-command nil 0 (vc-git--literal-pathspec file) "rm" "-f" "--"))
+  (vc-git-command nil 0 file "rm" "-f" "--"))
 
 (defun vc-git-rename-file (old new)
   (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"))
+  (vc-git-command nil 0 files "add"))
 
 (defvar vc-git-extra-menu-map
   (let ((map (make-sparse-keymap)))
@@ -1796,6 +1788,8 @@ vc-git-command
         (process-environment
          (append
           `("GIT_DIR"
+            ,@(when vc-git-use-literal-pathspecs
+                '("GIT_LITERAL_PATHSPECS=1"))
             ;; Avoid repository locking during background operations
             ;; (bug#21559).
             ,@(when revert-buffer-in-progress-p
@@ -1833,6 +1827,8 @@ vc-git--call
 	(process-environment
 	 (append
 	  `("GIT_DIR"
+            ,@(when vc-git-use-literal-pathspecs
+                '("GIT_LITERAL_PATHSPECS=1"))
 	    ;; Avoid repository locking during background operations
 	    ;; (bug#21559).
 	    ,@(when revert-buffer-in-progress-p

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dima Kogan]

Dmitry Gutov <dgutov@yandex.ru> writes:

> I wouldn't mind some testing from Dima as well, BTW.

I just tested it: it works. I started up a very recent emacs, opened a
remote file on an old box, and saw that vc-git doesn't work. Then I
applied the patch, refreshed, and vc-git works again.

Thanks!

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Andy Moreton]

On Mon 03 Jan 2022, Dmitry Gutov wrote:

> On 27.12.2021 04:36, Dmitry Gutov wrote:
>> So here's the patch (my current preferred solution for both emacs-28 and
>> master).
>> Waiting for feedback from AndyM.
>
> Sorry, here's a version of the patch that doesn't break vc-checkin.
>
> Waiting for Andy's feedback, I wouldn't mind some testing from Dima as well,
> BTW.

I've not tested checkin, but some light testing for looking at repo
history with this patch applied works nicely for me on both emacs-28 and
master.

Thanks,

    AndyM

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dmitry Gutov]

On 04.01.2022 00:15, Dima Kogan wrote:
> I just tested it: it works. I started up a very recent emacs, opened a
> remote file on an old box, and saw that vc-git doesn't work. Then I
> applied the patch, refreshed, and vc-git works again.

On 04.01.2022 00:16, Andy Moreton wrote:
 > I've not tested checkin, but some light testing for looking at repo
 > history with this patch applied works nicely for me on both emacs-28 and
 > master.

Thanks for checking, fellas.

Eli, good for emacs-28?

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Eli Zaretskii]

> Cc: 51497@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 4 Jan 2022 00:51:18 +0200
> 
> On 04.01.2022 00:15, Dima Kogan wrote:
> > I just tested it: it works. I started up a very recent emacs, opened a
> > remote file on an old box, and saw that vc-git doesn't work. Then I
> > applied the patch, refreshed, and vc-git works again.
> 
> On 04.01.2022 00:16, Andy Moreton wrote:
>  > I've not tested checkin, but some light testing for looking at repo
>  > history with this patch applied works nicely for me on both emacs-28 and
>  > master.
> 
> Thanks for checking, fellas.
> 
> Eli, good for emacs-28?

Yes, thanks.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dmitry Gutov]

On 04.01.2022 06:28, Eli Zaretskii wrote:
>> Cc: 51497@debbugs.gnu.org
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Tue, 4 Jan 2022 00:51:18 +0200
>>
>> On 04.01.2022 00:15, Dima Kogan wrote:
>>> I just tested it: it works. I started up a very recent emacs, opened a
>>> remote file on an old box, and saw that vc-git doesn't work. Then I
>>> applied the patch, refreshed, and vc-git works again.
>>
>> On 04.01.2022 00:16, Andy Moreton wrote:
>>   > I've not tested checkin, but some light testing for looking at repo
>>   > history with this patch applied works nicely for me on both emacs-28 and
>>   > master.
>>
>> Thanks for checking, fellas.
>>
>> Eli, good for emacs-28?
> 
> Yes, thanks.

Done!

Thanks all, I'm closing this bug.

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Thanks all, I'm closing this bug.

Looks like the bug was still open (perhaps due to the debbugs server
problems some weeks back?), so I'm closing it now.

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

bug#51497: 29.0.50; (vc-print-log) broken over TRAMP

[Dmitry Gutov]

On 21.01.2022 15:50, Lars Ingebrigtsen wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
>> Thanks all, I'm closing this bug.
> 
> Looks like the bug was still open (perhaps due to the debbugs server
> problems some weeks back?), so I'm closing it now.

Sorry. My current version of Thunderbird occasionally undoes edits in 
the participants' email addresses.