bug#26066: 26.0.50; vc-git-status gives wrong result

bug#26066: 26.0.50; vc-git-status gives wrong result

[Jonathan Ganc]

**To reproduce:**

First, we create a simple git repository in a shell:
   $ mkdir -p ~/Desktop/temp
   $ cd ~/Desktop/temp
   $ git init
   $ echo abc > file.txt
   $ git add . && git commit -a -m "Init"

You can verify that the repository is up to date by running, e.g. 'git 
status'.

Next, we load emacs -Q and run the following:
(progn
   (setq default-directory "/")
   (require 'vc-git)
   (vc-git-state "/home/ganc/Desktop/temp/file.txt"))

The response is `added'. It should be `up-to-date'.

**Notes**


I get this same behavior on emacs 24.5, 25.1. I have some ideas about
how to fix the problems but I want to make sure this isn't somehow the
expected behavior.


In GNU Emacs 26.0.50 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.20.9)
  of 2017-03-10 built on lcy01-22
Windowing system distributor 'The X.Org Foundation', version 11.0.11804000
System Description:    Ubuntu 16.10

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Configured using:
  'configure --build=x86_64-linux-gnu --prefix=/usr
  '--includedir=${prefix}/include' '--mandir=${prefix}/share/man'
  '--infodir=${prefix}/share/info' --sysconfdir=/etc --localstatedir=/var
  --disable-silent-rules '--libdir=${prefix}/lib/x86_64-linux-gnu'
  '--libexecdir=${prefix}/lib/x86_64-linux-gnu' --disable-maintainer-mode
  --disable-dependency-tracking --prefix=/usr --sharedstatedir=/var/lib
  --program-suffix=-snapshot --with-modules=yes --with-x=yes
  --with-x-toolkit=gtk3 --with-xwidgets=yes 'CFLAGS=-g -O2
  -fdebug-prefix-map=/build/emacs-snapshot-rvTCtk/emacs-snapshot-93192-26848af-emacs=. -fstack-protector-strong
  -Wformat -Werror=format-security' 'CPPFLAGS=-Wdate-time
  -D_FORTIFY_SOURCE=2' 'LDFLAGS=-Wl,-Bsymbolic-functions -Wl,-z,relro''

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GCONF GSETTINGS
NOTIFY LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 MODULES XWIDGETS LIBSYSTEMD

Important settings:
   value of $LANG: en_US.UTF-8
   value of $XMODIFIERS: @im=ibus
   locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
   tooltip-mode: t
   global-eldoc-mode: t
   electric-indent-mode: t
   mouse-wheel-mode: t
   tool-bar-mode: t
   menu-bar-mode: t
   file-name-shadow-mode: t
   global-font-lock-mode: t
   font-lock-mode: t
   blink-cursor-mode: t
   auto-composition-mode: t
   auto-encryption-mode: t
   auto-compression-mode: t
   line-number-mode: t
   transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message puny seq byte-opt subr-x gv
bytecomp byte-compile cl-extra help-mode cconv cl-loaddefs pcase cl-lib
dired dired-loaddefs format-spec rfc822 mml easymenu mml-sec
password-cache epa derived epg epg-config gnus-util rmail rmail-loaddefs
mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils
mailheader sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr
mail-utils time-date mule-util tooltip eldoc electric uniquify
ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript case-table epa-hook jka-cmpr-hook help
simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs button
faces cus-face macroexp files text-properties overlay sha1 md5 base64
format env code-pages mule custom widget hashtable-print-readable
backquote dbusbind inotify dynamic-setting system-font-setting
font-render-setting xwidget-internal move-toolbar gtk x-toolkit x
multi-tty make-network-process emacs)

Memory information:
((conses 16 96617 5698)
  (symbols 48 20192 1)
  (miscs 40 48 115)
  (strings 32 17613 4552)
  (string-bytes 1 565163)
  (vectors 16 14650)
  (vector-slots 8 482994 4091)
  (floats 8 48 68)
  (intervals 56 241 0)
  (buffers 976 11))

bug#26066: 26.0.50; vc-git-status gives wrong result

[npostavs]

retitle 26066 vc-git-status gives wrong result when called from outside repository
severity 26066 minor
found 26066 24.5
found 26066 25.2
quit

(list 
 (let ((default-directory "/"))
   (require 'vc-git)
   (vc-git-state "/home/npostavs/src/emacs/emacs-master/README"))
 (let ((default-directory "/home/npostavs/src/emacs/emacs-master/"))
   (require 'vc-git)
   (vc-git-state "README"))) ;=> (added up-to-date)

Jonathan Ganc <jonganc@gmail.com> writes:

> I get this same behavior on emacs 24.5, 25.1. I have some ideas about
> how to fix the problems but I want to make sure this isn't somehow the
> expected behavior.

I doubt this particular behaviour is expected, but it's probably
unexpected that you would call vc-git-status from outside the repo.  If
you can fix things to handle this case without harming the normal case,
that sounds fine to me.

bug#26066: 26.0.50; vc-git-status gives wrong result

[npostavs]

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

[Please use Reply All to keep 26066@debbugs.gnu.org on Cc]


[-- Attachment #2: Type: message/rfc822, Size: 2248 bytes --]

From: Jonathan Ganc <jonganc@gmail.com>
To: npostavs@users.sourceforge.net
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Tue, 14 Mar 2017 20:25:57 -0400
Message-ID: <6fb773c4-5f05-1965-8fb1-a425cefbfddd@gmail.com>

The actual problem is with the function vc-git--empty-db-p, which acts 
in the directory default-directory (implicitly, through vc-git--call and 
then process-file). Thus, I think replacing (in vc-git-state)
(vc-git--empty-db-p)
with
(let ((default-directory (file-name-directory file)))
   (vc-git--empty-db-p))
should resolve the issue without side effect.

As an aside, is there a reason that vc-git-status does not use "git 
status -z --porcelain --ignored"? It seems more natural/straightfoward 
and it would also deal with ignored and removed files. It could also 
work with directories, which the current function doesn't.

[-- Attachment #3: Type: text/plain, Size: 217 bytes --]



Let-binding default-directory sounds okay.

Regarding 'status -z --porcelain', I think it might not have been
available at the time te code was written.  I'm not sure how far back we
need to maintain compatibility.

bug#26066: 26.0.50; vc-git-status gives wrong result

[Dmitry Gutov]

On 16.03.2017 02:42, npostavs@users.sourceforge.net wrote:
> As an aside, is there a reason that vc-git-status does not use "git
> status -z --porcelain --ignored"? It seems more natural/straightfoward
> and it would also deal with ignored and removed files. It could also
> work with directories, which the current function doesn't.

Like mentioned in a discussion on a different bug, we'll accept a patch 
like that. You can even reuse some code from `vc-git-conflicted-files`.

bug#26066: 26.0.50; vc-git-status gives wrong result

[Jonathan Ganc]

First of all, I realized that the let binding needs to surround the 
entire vc-git-state function, not just (vc-git--empty-db-p) (because 
vc-git--run-command-string, vc-git--call have the same issue with 
default-directory). E.g., change

(let ((diff (vc-git--run-command-string
                file "diff-index" "-p" "--raw" "-z" "HEAD" "--")))

to

(let* ((default-directory (file-name-directory file))
        (diff (vc-git--run-command-string
              file "diff-index" "-p" "--raw" "-z" "HEAD" "--")))

But yes, I will work on an improved version using git status and try 
incorporate/improve on vc-git-conflicted-files.

(My ultimate goal is to try to get something like atom's status for git 
files into emacs, which is how I noticed the problem to begin with).


On 03/15/2017 08:48 PM, Dmitry Gutov wrote:
> On 16.03.2017 02:42, npostavs@users.sourceforge.net wrote:
>> As an aside, is there a reason that vc-git-status does not use "git
>> status -z --porcelain --ignored"? It seems more natural/straightfoward
>> and it would also deal with ignored and removed files. It could also
>> work with directories, which the current function doesn't.
>
> Like mentioned in a discussion on a different bug, we'll accept a 
> patch like that. You can even reuse some code from 
> `vc-git-conflicted-files`.

bug#26066: 26.0.50; vc-git-status gives wrong result

[Dmitry Gutov]

Hi Jonathan,

On 16.03.2017 04:40, Jonathan Ganc wrote:

> (My ultimate goal is to try to get something like atom's status for git 
> files into emacs, which is how I noticed the problem to begin with).

Could you expand on what you are trying to do, and how this bug stops 
you from doing it?

bug#26066: 26.0.50; vc-git-status gives wrong result

[Jonathan Ganc]

Hi,

Well, the ideal situation is if could get a listing like the one atom 
has for projects, where you get a visual view of directories and can see 
folders with changed files as well as the status of files (orangish 
means modified, green means new, greyed means ignored):

http://imgur.com/a/PiiGF

Actually, emacs has something not so dissimilar in the Neotree package. 
(Install it and use `(setq neo-vc-integration '(face char))` ). It has a 
few shortcomings:

1. Because of this bug, it was giving me the wrong status for files, 
i.e. they showed up wrong. That is how I noticed the bug in the first place.

2. I would like to be able to color directories as well as files. I 
don't know if that is something that would have a component in vc-git.el 
/ vc-....el or would go entirely in some package (e.g. Neotree)

3. It would be nice to be able to show mutiple directory trees at once 
in Neotree, though this is not as important.


The first, big problem is that, because of this bug,


On 03/21/2017 05:19 AM, Dmitry Gutov wrote:
> Hi Jonathan,
>
> On 16.03.2017 04:40, Jonathan Ganc wrote:
>
>> (My ultimate goal is to try to get something like atom's status for 
>> git files into emacs, which is how I noticed the problem to begin with).
>
> Could you expand on what you are trying to do, and how this bug stops 
> you from doing it?

bug#26066: 26.0.50; vc-git-status gives wrong result

[Michael Heerdegen]

Jonathan Ganc <jonganc@gmail.com> writes:

> http://imgur.com/a/PiiGF

I didn't quite follow the whole thread, but you might also want to try
`diff-hl-dired-mode' implemented by the diff-hl package in Gnu Elpa.


Michael.

bug#26066: 26.0.50; vc-git-status gives wrong result

[Dmitry Gutov]

On 21.03.2017 18:10, Jonathan Ganc wrote:

> Well, the ideal situation is if could get a listing like the one atom 
> has for projects, where you get a visual view of directories and can see 
> folders with changed files as well as the status of files (orangish 
> means modified, green means new, greyed means ignored):
> 
> http://imgur.com/a/PiiGF
> 
> Actually, emacs has something not so dissimilar in the Neotree package. 
> (Install it and use `(setq neo-vc-integration '(face char))` ). It has a 
> few shortcomings:
> 
> 1. Because of this bug, it was giving me the wrong status for files, 
> i.e. they showed up wrong. That is how I noticed the bug in the first 
> place.

I see, thanks. Neotree does have an optional feature that colors the 
tree leaves according to their VC statuses.

The problem might have gone unnoticed until now because file's status is 
cached, and because it can only be apparent if several projects are 
opened at the same time.

While the problem is fairly obvious, a proper fix would most likely 
touch other backends and commands, to the point that the 
default-directory binding might have to be done inside vc-call-backend.

There is a very simple workaround on the caller's side, though: bind 
default-directory inside the Neotree code, to the respective project 
root (or just the file's parent directory). That will be necessary 
anyway for it to work in the released Emacs versions.

> 2. I would like to be able to color directories as well as files. I 
> don't know if that is something that would have a component in vc-git.el 
> / vc-....el or would go entirely in some package (e.g. Neotree)

This differs from one version control system to another, but Git doesn't 
actually track directories. So the notion of "directory status" is 
poorly defined. But see the previously mentioned diff-hl-dired-mode.

> 3. It would be nice to be able to show mutiple directory trees at once 
> in Neotree, though this is not as important.

If it's unable to show the non-current projects, how is the bug 
triggered? default-directory would have to be outside of the project.

bug#26066: 26.0.50; vc-git-status gives wrong result

[Jonathan Ganc]

Hi,

Thanks for both responses.


>> 3. It would be nice to be able to show mutiple directory trees at 
>> once in Neotree, though this is not as important.
>
> If it's unable to show the non-current projects, how is the bug 
> triggered? default-directory would have to be outside of the project.

I'm not actually sure why it is causing me problems. I just tried using 
Neotree and encountered this problem. I didn't try anything special 
besides opening the window. I'm not sure how/why Neotree sets 
default-directory. I need to look into this.


> The problem might have gone unnoticed until now because file's status 
> is cached, and because it can only be apparent if several projects are 
> opened at the same time.
>
> While the problem is fairly obvious, a proper fix would most likely 
> touch other backends and commands, to the point that the 
> default-directory binding might have to be done inside vc-call-backend.

Yeah, I'm not really sure what the workflow should be; I'm just getting 
started with the vc functions in emacs. One issue, though, is that 
default-directory can only be set for functions that identify a 
filename, because we need to have a directory to set things to. For this 
reason, I don't know that one could generally set default-directory in 
vc-call-backend. It could make sense to set default-directory for 
vc-git--run-command-string (although to get vc-git-state to work, one 
would still need to set it for vc-git--empty-db-p).

It's worth nothing there seems to be inconsistency within vc-git. Some 
commands like vc-git-checkin, vc-git-next-revision, do set 
default-directory to the directory of the input file; other comands like 
vc-git-merge-branch seem to do the opposite and assume the root is 
already given by default-directory.

At the very least, the documentation for vc-git-state should note that 
default-directory needs to be set.

>
> There is a very simple workaround on the caller's side, though: bind 
> default-directory inside the Neotree code, to the respective project 
> root (or just the file's parent directory). That will be necessary 
> anyway for it to work in the released Emacs versions.
>

That may be the best answer. I am not so familiar with the Neotree code 
but it should be doable.

>> 2. I would like to be able to color directories as well as files. I 
>> don't know if that is something that would have a component in 
>> vc-git.el / vc-....el or would go entirely in some package (e.g. 
>> Neotree)
>
> This differs from one version control system to another, but Git 
> doesn't actually track directories. So the notion of "directory 
> status" is poorly defined. But see the previously mentioned 
> diff-hl-dired-mode.
>

You're right, in principle, but atom handles it by coloring directories 
based on if they have modified files within. I don't know if there is a 
"canonical" way to do this, although one idea would be to return a list 
of all file status in the directory, e.g. `(up-to-date modified ignored)`.

I started looking through diff-hl. It looks like it has a bunch of neat 
and useful features, though I admit I was a bit confused by the workflow.

bug#26066: 26.0.50; vc-git-status gives wrong result

[Jonathan Ganc]

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

I have attached my proposed patch to use 'git status' for finding the 
git status of files.

I have also attached git-test.sh, which generates a subdirectory 
git-test with files in all the relevant git states I could think of (if 
you use it, look at the mods, repo2, repo3 directories for interesting 
files).

I then verified that 1) my function gives the same result as 
vc-git-dir-status-files where vc-git-dir-status-files shows the file, 
with one exception: if a file has a merge conflict (i.e. the status is 
"UU"), my function returns conflict; 2) it is the same speed as the 
current vc-git-state.

I may try eventually try rewriting vc-git-dir-status-files using this 
since it appears to be about an order of magnitude faster than the 
current implementation.

On 03/22/2017 10:18 PM, Jonathan Ganc wrote:
> Hi,
>
> Thanks for both responses.
>
>
>>> 3. It would be nice to be able to show mutiple directory trees at 
>>> once in Neotree, though this is not as important.
>>
>> If it's unable to show the non-current projects, how is the bug 
>> triggered? default-directory would have to be outside of the project.
>
> I'm not actually sure why it is causing me problems. I just tried 
> using Neotree and encountered this problem. I didn't try anything 
> special besides opening the window. I'm not sure how/why Neotree sets 
> default-directory. I need to look into this.
>
>
>> The problem might have gone unnoticed until now because file's status 
>> is cached, and because it can only be apparent if several projects 
>> are opened at the same time.
>>
>> While the problem is fairly obvious, a proper fix would most likely 
>> touch other backends and commands, to the point that the 
>> default-directory binding might have to be done inside vc-call-backend.
>
> Yeah, I'm not really sure what the workflow should be; I'm just 
> getting started with the vc functions in emacs. One issue, though, is 
> that default-directory can only be set for functions that identify a 
> filename, because we need to have a directory to set things to. For 
> this reason, I don't know that one could generally set 
> default-directory in vc-call-backend. It could make sense to set 
> default-directory for vc-git--run-command-string (although to get 
> vc-git-state to work, one would still need to set it for 
> vc-git--empty-db-p).
>
> It's worth nothing there seems to be inconsistency within vc-git. Some 
> commands like vc-git-checkin, vc-git-next-revision, do set 
> default-directory to the directory of the input file; other comands 
> like vc-git-merge-branch seem to do the opposite and assume the root 
> is already given by default-directory.
>
> At the very least, the documentation for vc-git-state should note that 
> default-directory needs to be set.
>
>>
>> There is a very simple workaround on the caller's side, though: bind 
>> default-directory inside the Neotree code, to the respective project 
>> root (or just the file's parent directory). That will be necessary 
>> anyway for it to work in the released Emacs versions.
>>
>
> That may be the best answer. I am not so familiar with the Neotree 
> code but it should be doable.
>
>>> 2. I would like to be able to color directories as well as files. I 
>>> don't know if that is something that would have a component in 
>>> vc-git.el / vc-....el or would go entirely in some package (e.g. 
>>> Neotree)
>>
>> This differs from one version control system to another, but Git 
>> doesn't actually track directories. So the notion of "directory 
>> status" is poorly defined. But see the previously mentioned 
>> diff-hl-dired-mode.
>>
>
> You're right, in principle, but atom handles it by coloring 
> directories based on if they have modified files within. I don't know 
> if there is a "canonical" way to do this, although one idea would be 
> to return a list of all file status in the directory, e.g. 
> `(up-to-date modified ignored)`.
>
> I started looking through diff-hl. It looks like it has a bunch of 
> neat and useful features, though I admit I was a bit confused by the 
> workflow.
>


[-- Attachment #2: git-test.sh --]
[-- Type: application/x-shellscript, Size: 9069 bytes --]

[-- Attachment #3: 0001-Use-git-status-in-vc-git-status.patch --]
[-- Type: text/x-patch, Size: 4768 bytes --]

From ba77e001b719ff289cf0b1d3dcffd647af6d175d Mon Sep 17 00:00:00 2001
From: Jonathan Ganc <jonganc@gmail.com>
Date: Thu, 30 Mar 2017 23:13:15 -0400
Subject: [PATCH] Use 'git status' in vc-git-status

---
 lisp/vc/vc-git.el | 79 +++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 53 insertions(+), 26 deletions(-)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 1a3f1bf..ce662fb 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -231,34 +231,61 @@ (defun vc-git--state-code (code)
     (?U 'edited)     ;; FIXME
     (?T 'edited)))   ;; FIXME
 
+(defun vc-git--git-status-to-vc-state (code-list)
+  "Convert a list CODE-LIST of two-letter git status strings to a vc status.
+
+Each element of CODE-LIST comes from the first two characters of
+a line returned by 'git status' and should be passed in the order given by 'git status'. Elements that are nil are simply ignored.
+
+Note that this function may destroy and/or alter `code-list'.
+
+\(It is necessary to allow CODE-LIST to be a list because
+sometimes git status returns multiple lines, e.g. for a file that
+is removed from the index but is present in the HEAD and working
+tree.) "
+  (setq code-list (remq nil code-list))
+  (pcase code-list
+    ('nil 'up-to-date)
+    (`(,code)
+     (pcase code
+       ("!!" 'ignored)
+       ("??" 'unregistered)
+       ;; have only seen this with a file that is only present in the
+       ;; index. let us call this `removed'
+       ("AD" 'removed)
+       (_ (cond
+           ((string-match-p "^[ RD]+$" code) 'removed)
+           ((string-match-p "^[ M]+$" code) 'edited)
+           ((string-match-p "^[ A]+$" code) 'added)
+           ((string-match-p "^[ U]+$" code) 'conflict)
+           (t 'edited)))))
+    ;;  I know of two times when git state returns more than one element,
+    ;;  in both cases returning '("D " "??")':
+    ;;  1. when a file is removed from the index but present in the
+    ;;     HEAD and working tree, the CODE-LIST will be 
+    ;;  2. when a file A is renamed to B in the index and then back to A
+    ;;     in the working tree, the CODE-LIST will be '("??" "RD")
+    ;;  In both these instances, `unregistered' is a reasonable response.
+    (`("D " "??") 'unregistered)
+    ;;  In other cases, let us return `edited'
+    (_ 'edited)))
+
 (defun vc-git-state (file)
   "Git-specific version of `vc-state'."
-  ;; FIXME: This can't set 'ignored or 'conflict yet
-  ;; The 'ignored state could be detected with `git ls-files -i -o
-  ;; --exclude-standard` It also can't set 'needs-update or
-  ;; 'needs-merge. The rough equivalent would be that upstream branch
-  ;; for current branch is in fast-forward state i.e. current branch
-  ;; is direct ancestor of corresponding upstream branch, and the file
-  ;; was modified upstream.  But we can't check that without a network
-  ;; operation.
-  ;; This assumes that status is known to be not `unregistered' because
-  ;; we've been successfully dispatched here from `vc-state', that
-  ;; means `vc-git-registered' returned t earlier once.  Bug#11757
-  (let ((diff (vc-git--run-command-string
-               file "diff-index" "-p" "--raw" "-z" "HEAD" "--")))
-    (if (and diff
-             (string-match ":[0-7]\\{6\\} [0-7]\\{6\\} [0-9a-f]\\{40\\} [0-9a-f]\\{40\\} \\([ADMUT]\\)\0[^\0]+\0\\(.*\n.\\)?"
-                           diff))
-        (let ((diff-letter (match-string 1 diff)))
-          (if (not (match-beginning 2))
-              ;; Empty diff: file contents is the same as the HEAD
-              ;; revision, but timestamps are different (eg, file
-              ;; was "touch"ed).  Update timestamp in index:
-              (prog1 'up-to-date
-                (vc-git--call nil "add" "--refresh" "--"
-                              (file-relative-name file)))
-            (vc-git--state-code diff-letter)))
-      (if (vc-git--empty-db-p) 'added 'up-to-date))))
+  
+  (save-match-data
+    (let* ((default-directory (file-name-directory (expand-file-name file)))
+           (status
+            (vc-git--run-command-string file "status" "--porcelain" "-z"
+                                        "--untracked-files" "--ignored" "--"))
+           code-list)
+      ;; if this code is adapted to parse git-status for a directory, note
+      ;; that a renamed file takes up two null values and needs to be
+      ;; treated slightly more carefully
+      (while (string-match "^\\(..\\)[^\0]+\0\\(\\(?:a\\|[^a]\\)*\\)$" status)
+        (add-to-list 'code-list (match-string 1 status) t 'ignore)
+        (setq status (match-string 2 status)))
+      (vc-git--git-status-to-vc-state code-list))))
 
 (defun vc-git-working-revision (_file)
   "Git-specific version of `vc-working-revision'."
-- 
2.9.3

bug#26066: 26.0.50; vc-git-status gives wrong result

[Jonathan Ganc]

Hi,

I am just checking if anyone has had a chance to review my proposed 
improvement to vc-git-status? I have been using it without problem but I 
would love to get feedback.

Jonathan


On 03/30/2017 11:16 PM, Jonathan Ganc wrote:
> I have attached my proposed patch to use 'git status' for finding the 
> git status of files.
>
> I have also attached git-test.sh, which generates a subdirectory 
> git-test with files in all the relevant git states I could think of 
> (if you use it, look at the mods, repo2, repo3 directories for 
> interesting files).
>
> I then verified that 1) my function gives the same result as 
> vc-git-dir-status-files where vc-git-dir-status-files shows the file, 
> with one exception: if a file has a merge conflict (i.e. the status is 
> "UU"), my function returns conflict; 2) it is the same speed as the 
> current vc-git-state.
>
> I may try eventually try rewriting vc-git-dir-status-files using this 
> since it appears to be about an order of magnitude faster than the 
> current implementation.
>
> On 03/22/2017 10:18 PM, Jonathan Ganc wrote:
>> Hi,
>>
>> Thanks for both responses.
>>
>>
>>>> 3. It would be nice to be able to show mutiple directory trees at 
>>>> once in Neotree, though this is not as important.
>>>
>>> If it's unable to show the non-current projects, how is the bug 
>>> triggered? default-directory would have to be outside of the project.
>>
>> I'm not actually sure why it is causing me problems. I just tried 
>> using Neotree and encountered this problem. I didn't try anything 
>> special besides opening the window. I'm not sure how/why Neotree sets 
>> default-directory. I need to look into this.
>>
>>
>>> The problem might have gone unnoticed until now because file's 
>>> status is cached, and because it can only be apparent if several 
>>> projects are opened at the same time.
>>>
>>> While the problem is fairly obvious, a proper fix would most likely 
>>> touch other backends and commands, to the point that the 
>>> default-directory binding might have to be done inside vc-call-backend.
>>
>> Yeah, I'm not really sure what the workflow should be; I'm just 
>> getting started with the vc functions in emacs. One issue, though, is 
>> that default-directory can only be set for functions that identify a 
>> filename, because we need to have a directory to set things to. For 
>> this reason, I don't know that one could generally set 
>> default-directory in vc-call-backend. It could make sense to set 
>> default-directory for vc-git--run-command-string (although to get 
>> vc-git-state to work, one would still need to set it for 
>> vc-git--empty-db-p).
>>
>> It's worth nothing there seems to be inconsistency within vc-git. 
>> Some commands like vc-git-checkin, vc-git-next-revision, do set 
>> default-directory to the directory of the input file; other comands 
>> like vc-git-merge-branch seem to do the opposite and assume the root 
>> is already given by default-directory.
>>
>> At the very least, the documentation for vc-git-state should note 
>> that default-directory needs to be set.
>>
>>>
>>> There is a very simple workaround on the caller's side, though: bind 
>>> default-directory inside the Neotree code, to the respective project 
>>> root (or just the file's parent directory). That will be necessary 
>>> anyway for it to work in the released Emacs versions.
>>>
>>
>> That may be the best answer. I am not so familiar with the Neotree 
>> code but it should be doable.
>>
>>>> 2. I would like to be able to color directories as well as files. I 
>>>> don't know if that is something that would have a component in 
>>>> vc-git.el / vc-....el or would go entirely in some package (e.g. 
>>>> Neotree)
>>>
>>> This differs from one version control system to another, but Git 
>>> doesn't actually track directories. So the notion of "directory 
>>> status" is poorly defined. But see the previously mentioned 
>>> diff-hl-dired-mode.
>>>
>>
>> You're right, in principle, but atom handles it by coloring 
>> directories based on if they have modified files within. I don't know 
>> if there is a "canonical" way to do this, although one idea would be 
>> to return a list of all file status in the directory, e.g. 
>> `(up-to-date modified ignored)`.
>>
>> I started looking through diff-hl. It looks like it has a bunch of 
>> neat and useful features, though I admit I was a bit confused by the 
>> workflow.
>>
>

bug#26066: 26.0.50; vc-git-status gives wrong result

[npostavs]

Jonathan Ganc <jonganc@gmail.com> writes:

>  
> +(defun vc-git--git-status-to-vc-state (code-list)

> +  (setq code-list (remq nil code-list))
> +  (pcase code-list
> +    ('nil 'up-to-date)

You seem to be handling nil in 2 different ways.

> +       ;; have only seen this with a file that is only present in the
> +       ;; index. let us call this `removed'

Comments should be full sentences (start with uppercase, end with
period), and sentences should be separated with double spaces.

>  (defun vc-git-state (file)
>    "Git-specific version of `vc-state'."
> +  
> +  (save-match-data
> +    (let* ((default-directory (file-name-directory (expand-file-name file)))
> +           (status
> +            (vc-git--run-command-string file "status" "--porcelain" "-z"
> +                                        "--untracked-files" "--ignored" "--"))
> +           code-list)

> +      (while (string-match "^\\(..\\)[^\0]+\0\\(\\(?:a\\|[^a]\\)*\\)$" status)

I think you're missing a space after the first 2 characters.

    alternate -z format recommended for machine parsing. [...] Second, a
    NUL (ASCII 0) follows each filename, [...] (but a space still
    separates the status field from the first filename).

> +        (add-to-list 'code-list (match-string 1 status) t 'ignore)

Don't use `add-to-list' on local variables.  The usual idiom for
collecting a series of items in a list would be

    (let ((code-list nil))
      (while ...
        (push ... code-list))
      (setq code-list (nreverse code-list)))

> +        (setq status (match-string 2 status)))

Instead of matching the rest of the string in the regexp, just set a
position variable to (match-end 0), and then pass that position as the
string-match's START parameter.

bug#26066: 26.0.50; vc-git-status gives wrong result

[Dmitry Gutov]

On 31.03.2017 06:16, Jonathan Ganc wrote:
> +  (save-match-data
> +    (let* ((default-directory (file-name-directory (expand-file-name file)))

We don't require or expect that any particular function keeps the match 
data intact. I don't think that save-match-data is needed here.

Aside from this an Noam's comments, the patch is long enough to require 
a copyright assignment. I didn't find yours on file.

Will you sign one?

bug#26066: 26.0.50; vc-git-status gives wrong result

[Jonathan Ganc]

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

I agree with the suggestions from both of you and have made the 
corrections. I have attached a new patch. Please let me know if you have 
further thoughts.

I will gladly sign a copyright assignment, if you point me to where I 
can do that.


On 04/09/2017 11:26 PM, Dmitry Gutov wrote:
> On 31.03.2017 06:16, Jonathan Ganc wrote:
>> +  (save-match-data
>> +    (let* ((default-directory (file-name-directory (expand-file-name 
>> file)))
>
> We don't require or expect that any particular function keeps the 
> match data intact. I don't think that save-match-data is needed here.
>
> Aside from this an Noam's comments, the patch is long enough to 
> require a copyright assignment. I didn't find yours on file.
>
> Will you sign one?


[-- Attachment #2: 0001-update-vc-git.patch --]
[-- Type: text/x-patch, Size: 4541 bytes --]

From 78bcdd00927bc8f14ef2999b8bf0b4ad25ab6dac Mon Sep 17 00:00:00 2001
From: Jonathan Ganc <jonganc@gmail.com>
Date: Mon, 10 Apr 2017 00:38:52 -0400
Subject: [PATCH] update vc-git

---
 lisp/vc/vc-git.el | 75 ++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 49 insertions(+), 26 deletions(-)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 1a3f1bf..a8d686a 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -231,34 +231,57 @@ (defun vc-git--state-code (code)
     (?U 'edited)     ;; FIXME
     (?T 'edited)))   ;; FIXME
 
+(defun vc-git--git-status-to-vc-state (code-list)
+  "Convert a list CODE-LIST of two-letter git status strings to a vc status.
+
+Each element of CODE-LIST comes from the first two characters of
+a line returned by 'git status' and should be passed in the order given by 'git status'.
+
+\(It is necessary to allow CODE-LIST to be a list because
+sometimes git status returns multiple lines, e.g. for a file that
+is removed from the index but is present in the HEAD and working
+tree.) "
+  (pcase code-list
+    ('nil 'up-to-date)
+    (`(,code)
+     (pcase code
+       ("!!" 'ignored)
+       ("??" 'unregistered)
+       ;; I have only seen this with a file that is only present in the
+       ;; index.  Let us call this `removed'
+       ("AD" 'removed)
+       (_ (cond
+           ((string-match-p "^[ RD]+$" code) 'removed)
+           ((string-match-p "^[ M]+$" code) 'edited)
+           ((string-match-p "^[ A]+$" code) 'added)
+           ((string-match-p "^[ U]+$" code) 'conflict)
+           (t 'edited)))))
+    ;;  I know of two times when git state returns more than one element,
+    ;;  in both cases returning '("D " "??")':
+    ;;  1. when a file is removed from the index but present in the
+    ;;     HEAD and working tree
+    ;;  2. when a file A is renamed to B in the index and then back to A
+    ;;     in the working tree
+    ;;  In both these instances, `unregistered' is a reasonable response.
+    (`("D " "??") 'unregistered)
+    ;;  In other cases, let us return `edited'.
+    (_ 'edited)))
+
 (defun vc-git-state (file)
   "Git-specific version of `vc-state'."
-  ;; FIXME: This can't set 'ignored or 'conflict yet
-  ;; The 'ignored state could be detected with `git ls-files -i -o
-  ;; --exclude-standard` It also can't set 'needs-update or
-  ;; 'needs-merge. The rough equivalent would be that upstream branch
-  ;; for current branch is in fast-forward state i.e. current branch
-  ;; is direct ancestor of corresponding upstream branch, and the file
-  ;; was modified upstream.  But we can't check that without a network
-  ;; operation.
-  ;; This assumes that status is known to be not `unregistered' because
-  ;; we've been successfully dispatched here from `vc-state', that
-  ;; means `vc-git-registered' returned t earlier once.  Bug#11757
-  (let ((diff (vc-git--run-command-string
-               file "diff-index" "-p" "--raw" "-z" "HEAD" "--")))
-    (if (and diff
-             (string-match ":[0-7]\\{6\\} [0-7]\\{6\\} [0-9a-f]\\{40\\} [0-9a-f]\\{40\\} \\([ADMUT]\\)\0[^\0]+\0\\(.*\n.\\)?"
-                           diff))
-        (let ((diff-letter (match-string 1 diff)))
-          (if (not (match-beginning 2))
-              ;; Empty diff: file contents is the same as the HEAD
-              ;; revision, but timestamps are different (eg, file
-              ;; was "touch"ed).  Update timestamp in index:
-              (prog1 'up-to-date
-                (vc-git--call nil "add" "--refresh" "--"
-                              (file-relative-name file)))
-            (vc-git--state-code diff-letter)))
-      (if (vc-git--empty-db-p) 'added 'up-to-date))))
+  (let* ((default-directory (file-name-directory (expand-file-name file)))
+           (status
+            (vc-git--run-command-string file "status" "--porcelain" "-z"
+                                        "--untracked-files" "--ignored" "--"))
+           code-list (next-match 0))
+      ;; If this code is adapted to parse git-status for a directory, note
+      ;; that a renamed file takes up two null values and needs to be
+      ;; treated slightly more carefully.
+      (while (string-match "\\(..\\) [^\0]+\0" status next-match)
+        (push (match-string 1 status) code-list)
+        (setq next-match (match-end 0)))
+      (setq code-list (nreverse code-list))
+      (vc-git--git-status-to-vc-state code-list)))
 
 (defun vc-git-working-revision (_file)
   "Git-specific version of `vc-working-revision'."
-- 
2.9.3

bug#26066: 26.0.50; vc-git-status gives wrong result

[Eli Zaretskii]

> From: Jonathan Ganc <jonganc@gmail.com>
> Date: Mon, 10 Apr 2017 00:41:15 -0400
> 
> I will gladly sign a copyright assignment, if you point me to where I 
> can do that.

I sent the form off-list.

Thanks.

bug#26066: 26.0.50; vc-git-status gives wrong result

[Thien-Thi Nguyen]

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


() Jonathan Ganc <jonganc@gmail.com>
() Mon, 10 Apr 2017 00:41:15 -0400

   +      (while (string-match "\\(..\\) [^\0]+\0" status next-match)
   +        (push (match-string 1 status) code-list)
   +        (setq next-match (match-end 0)))
   +      (setq code-list (nreverse code-list))

You can also use something like:

 (mapcar (lambda (s)
           (substring s 0 2))
         (split-string status "\0"))

to compute the value of ‘code-list’.  Might be quicker, too.

-- 
Thien-Thi Nguyen -----------------------------------------------
 (defun responsep (query)
   (pcase (context query)
     (`(technical ,ml) (correctp ml))
     ...))                              748E A0E8 1CB8 A748 9BFA
--------------------------------------- 6CE4 6703 2224 4C80 7502


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

bug#26066: 26.0.50; vc-git-status gives wrong result

[Dmitry Gutov]

On 10.04.2017 07:41, Jonathan Ganc wrote:
> I agree with the suggestions from both of you and have made the 
> corrections. I have attached a new patch. Please let me know if you have 
> further thoughts.

Thanks. I guess the main thing left is to decide whether vc-git-state 
should bind default-directory. For example, vc-bzr-state doesn't, while 
vc-hg-state does.

Does this binding affect the command output, in this particular case?

bug#26066: 26.0.50; vc-git-status gives wrong result

[Jonathan Ganc]

BTW, Dmitry, I've started using Diff-Hl and it's really nice!


On 04/09/2017 11:26 PM, Dmitry Gutov wrote:
> On 31.03.2017 06:16, Jonathan Ganc wrote:
>> +  (save-match-data
>> +    (let* ((default-directory (file-name-directory (expand-file-name 
>> file)))
>
> We don't require or expect that any particular function keeps the 
> match data intact. I don't think that save-match-data is needed here.
>
> Aside from this an Noam's comments, the patch is long enough to 
> require a copyright assignment. I didn't find yours on file.
>
> Will you sign one?

bug#26066: 26.0.50; vc-git-status gives wrong result

[Dmitry Gutov]

On 10.04.2017 07:43, Jonathan Ganc wrote:
> BTW, Dmitry, I've started using Diff-Hl and it's really nice!

I'm glad you like it :)

bug#26066: 26.0.50; vc-git-status gives wrong result

[Dmitry Gutov]

On 23.03.2017 04:18, Jonathan Ganc wrote:

> Yeah, I'm not really sure what the workflow should be; I'm just getting 
> started with the vc functions in emacs. One issue, though, is that 
> default-directory can only be set for functions that identify a 
> filename, because we need to have a directory to set things to. For this 
> reason, I don't know that one could generally set default-directory in 
> vc-call-backend.

You are right. But we could set it in vc-state-refresh.

> It could make sense to set default-directory for 
> vc-git--run-command-string (although to get vc-git-state to work, one 
> would still need to set it for vc-git--empty-db-p).

That's also an option, yes. But binding it on the higher level would be 
better, I think.

> It's worth nothing there seems to be inconsistency within vc-git. Some 
> commands like vc-git-checkin, vc-git-next-revision, do set 
> default-directory to the directory of the input file; other comands like 
> vc-git-merge-branch seem to do the opposite and assume the root is 
> already given by default-directory.

True. Some of those occurrences are most likely there just so the DIR 
argument is not left unused. Some have less obvious reasons.

vc-git-checkin, for instance, does it to call 'git commit' from the 
repository root, instead of an arbitrary directory in it. I'm sure it's 
really necessary, though.

> At the very least, the documentation for vc-git-state should note that 
> default-directory needs to be set.

The backend-specific functions are largely internal, we don't expect 
clients to call them directly. If anything, the documentation can be 
clarified on whether an arbitrary backend's 'state' command should set 
default-directory, or use the current one.

> You're right, in principle, but atom handles it by coloring directories 
> based on if they have modified files within. I don't know if there is a 
> "canonical" way to do this, although one idea would be to return a list 
> of all file status in the directory, e.g. `(up-to-date modified ignored)`.

Yeah, diff-hl-dired does something like that. If all the "interesting" 
statuses in a directory are the same, we assign it that status. If 
they're mixed, we mark it as "changed".

bug#26066: 26.0.50; vc-git-status gives wrong result

[Jonathan Ganc]

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

I've incorporated Thien-Thi's suggestion, as well as an if to check 
status for nil (which indicates unregistered).


On 04/10/2017 08:07 PM, Dmitry Gutov wrote:
> You are right. But we could set it in vc-state-refresh.
>

On 04/10/2017 07:46 PM, Dmitry Gutov wrote:
>
> Thanks. I guess the main thing left is to decide whether vc-git-state 
> should bind default-directory. For example, vc-bzr-state doesn't, 
> while vc-hg-state does.
>
> Does this binding affect the command output, in this particular case?

The binding affects the output if default-directory is not set inside 
the file's repository.

Since, in principle, the vc functions should be agnostic to the choice 
of vcs, either a) vc-state documentation should state that 
default-directory should be set to get generally correct responses or b) 
it should be set in some function (and I agree that vc-state-refresh 
makes sense).

I think the overhead of setting the directory is rather low. In some 
admittedly rudimentary benchmarks, there is almost no difference in 
performance setting default-directory.

There's also the question of how to handle default-directory. You cannot 
simply do (file-name-directory file), because that fails if FILE is 
given without a directory. I think the correct one is 
(file-name-directory (expand-file-name file)) (which, surprisingly, is 
slighly faster than (file-name-directory (concat default-directory file)) ).

[-- Attachment #2: 0001-update-vc-git.patch --]
[-- Type: text/x-patch, Size: 4643 bytes --]

From 8892f98fc0a8a956bc1adefcc0394ac6cf5e47d3 Mon Sep 17 00:00:00 2001
From: Jonathan Ganc <jonganc@gmail.com>
Date: Mon, 10 Apr 2017 00:38:52 -0400
Subject: [PATCH] update vc-git

---
 lisp/vc/vc-git.el | 79 +++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 53 insertions(+), 26 deletions(-)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 1a3f1bf..7c16125 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -231,34 +231,61 @@ (defun vc-git--state-code (code)
     (?U 'edited)     ;; FIXME
     (?T 'edited)))   ;; FIXME
 
+(defun vc-git--git-status-to-vc-state (code-list)
+  "Convert a list CODE-LIST of two-letter git status strings to a vc status.
+
+Each element of CODE-LIST comes from the first two characters of
+a line returned by 'git status' and should be passed in the order given by 'git status'.
+
+\(It is necessary to allow CODE-LIST to be a list because
+sometimes git status returns multiple lines, e.g. for a file that
+is removed from the index but is present in the HEAD and working
+tree.) "
+  (pcase code-list
+    ('nil 'up-to-date)
+    (`(,code)
+     (pcase code
+       ("!!" 'ignored)
+       ("??" 'unregistered)
+       ;; I have only seen this with a file that is only present in the
+       ;; index.  Let us call this `removed'
+       ("AD" 'removed)
+       (_ (cond
+           ((string-match-p "^[ RD]+$" code) 'removed)
+           ((string-match-p "^[ M]+$" code) 'edited)
+           ((string-match-p "^[ A]+$" code) 'added)
+           ((string-match-p "^[ U]+$" code) 'conflict)
+           (t 'edited)))))
+    ;;  I know of two times when git state returns more than one element,
+    ;;  in both cases returning '("D " "??")':
+    ;;  1. when a file is removed from the index but present in the
+    ;;     HEAD and working tree
+    ;;  2. when a file A is renamed to B in the index and then back to A
+    ;;     in the working tree
+    ;;  In both these instances, `unregistered' is a reasonable response.
+    (`("D " "??") 'unregistered)
+    ;;  In other cases, let us return `edited'.
+    (_ 'edited)))
+
 (defun vc-git-state (file)
   "Git-specific version of `vc-state'."
-  ;; FIXME: This can't set 'ignored or 'conflict yet
-  ;; The 'ignored state could be detected with `git ls-files -i -o
-  ;; --exclude-standard` It also can't set 'needs-update or
-  ;; 'needs-merge. The rough equivalent would be that upstream branch
-  ;; for current branch is in fast-forward state i.e. current branch
-  ;; is direct ancestor of corresponding upstream branch, and the file
-  ;; was modified upstream.  But we can't check that without a network
-  ;; operation.
-  ;; This assumes that status is known to be not `unregistered' because
-  ;; we've been successfully dispatched here from `vc-state', that
-  ;; means `vc-git-registered' returned t earlier once.  Bug#11757
-  (let ((diff (vc-git--run-command-string
-               file "diff-index" "-p" "--raw" "-z" "HEAD" "--")))
-    (if (and diff
-             (string-match ":[0-7]\\{6\\} [0-7]\\{6\\} [0-9a-f]\\{40\\} [0-9a-f]\\{40\\} \\([ADMUT]\\)\0[^\0]+\0\\(.*\n.\\)?"
-                           diff))
-        (let ((diff-letter (match-string 1 diff)))
-          (if (not (match-beginning 2))
-              ;; Empty diff: file contents is the same as the HEAD
-              ;; revision, but timestamps are different (eg, file
-              ;; was "touch"ed).  Update timestamp in index:
-              (prog1 'up-to-date
-                (vc-git--call nil "add" "--refresh" "--"
-                              (file-relative-name file)))
-            (vc-git--state-code diff-letter)))
-      (if (vc-git--empty-db-p) 'added 'up-to-date))))
+  (let* ((default-directory (file-name-directory (expand-file-name file)))
+         (status
+          (vc-git--run-command-string file "status" "--porcelain" "-z"
+                                      "--untracked-files" "--ignored" "--"))
+         code-list)
+    (if (null status)
+        ;; If status is nil, there was an error running git, likely because
+        ;; the file is not in a git repo.
+        'unregistered
+      ;; If this code is adapted to parse 'git status' for a directory,
+      ;; note that a renamed file takes up two null values and needs to be
+      ;; treated slightly more carefully.
+      (setq code-list
+            (mapcar (lambda (s)
+                      (substring s 0 2))
+                    (delete "" (split-string status "\0"))))
+      (vc-git--git-status-to-vc-state code-list))))
 
 (defun vc-git-working-revision (_file)
   "Git-specific version of `vc-working-revision'."
-- 
2.9.3

bug#26066: 26.0.50; vc-git-status gives wrong result

[Jonathan Ganc]

I think I've changed my mind about where/whether to bind 
default-directory. Since it is not universally bound by all vcs engines, 
it should probably be considered "backend specific", which would 
therefore suggest that it not be bound before the backend specific 
functions, e.g. vc-git-state. On the other hand, since it sometimes is 
necessary for correct output, I do think it should be bound in vc-git-state.


On 04/10/2017 11:52 PM, Jonathan Ganc wrote:
>
> The binding affects the output if default-directory is not set inside 
> the file's repository.
>
> Since, in principle, the vc functions should be agnostic to the choice 
> of vcs, either a) vc-state documentation should state that 
> default-directory should be set to get generally correct responses or 
> b) it should be set in some function (and I agree that 
> vc-state-refresh makes sense).
>
> I think the overhead of setting the directory is rather low. In some 
> admittedly rudimentary benchmarks, there is almost no difference in 
> performance setting default-directory.
>
> There's also the question of how to handle default-directory. You 
> cannot simply do (file-name-directory file), because that fails if 
> FILE is given without a directory. I think the correct one is 
> (file-name-directory (expand-file-name file)) (which, surprisingly, is 
> slighly faster than (file-name-directory (concat default-directory 
> file)) ).

bug#26066: 26.0.50; vc-git-status gives wrong result

[Dmitry Gutov]

On 11.04.2017 16:08, Jonathan Ganc wrote:
> I think I've changed my mind about where/whether to bind 
> default-directory. Since it is not universally bound by all vcs engines, 

That doesn't necessarily imply that they shouldn't.

> On the other hand, since it sometimes is 
> necessary for correct output, I do think it should be bound in 
> vc-git-state.

Is it necessary for correct output in other backends, in similar scenarios?

I agree that fixing this makes sense, but it should be done in an 
organized fashion, and separately from this patch.

If vc-git supports calling vc-git-state from outside of the repository, 
but not some other commands, or if vc-git-state does but some other 
backends' vc-xx-state does not, this will increase inconsistency and 
make it harder for the programmers to write VCS-agnostic code, which is 
one of the main goals of VC.

vc-state is also among the first functions a piece of third-party code 
would call. So it becomes the choke point which forces the caller to get 
the value of default-directory right, before any other backend commands 
are called. Until we take inventory of them, it's better to keep the bug 
in vc-state, I think.

>> Since, in principle, the vc functions should be agnostic to the choice 
>> of vcs, either a) vc-state documentation should state that 
>> default-directory should be set to get generally correct responses or

If we decide that, that should be the rule we declare for all VC backend 
commands, not just vc-state.

>> b) it should be set in some function (and I agree that 
>> vc-state-refresh makes sense).
>>
>> I think the overhead of setting the directory is rather low. In some 
>> admittedly rudimentary benchmarks, there is almost no difference in 
>> performance setting default-directory.

Yeah, I'm not worried about performance here either.

>> There's also the question of how to handle default-directory. You 
>> cannot simply do (file-name-directory file), because that fails if 
>> FILE is given without a directory. I think the correct one is 
>> (file-name-directory (expand-file-name file))

Yup, that should work. I'm not sure any of these commands are ever 
called with relative names, though.

> (which, surprisingly, is 
>> slighly faster than (file-name-directory (concat default-directory 
>> file)) ).

That probably depends on the file. The first option was faster in the 
example I've tried.

bug#26066: 26.0.50; vc-git-status gives wrong result

[Dmitry Gutov]

On 12.04.2017 02:27, Dmitry Gutov wrote:

>> (which, surprisingly, is
>>> slighly faster than (file-name-directory (concat default-directory 
>>> file)) ).
> 
> That probably depends on the file. The first option was faster in the 
> example I've tried.

I meant the second one, sorry.

bug#26066: 26.0.50; vc-git-status gives wrong result

[Jonathan Ganc]

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

First, I'm sorry for this delayed response. I've had a busy few days 
(and I needed to think about the issue)!

I'm fine with decoupling the question about default-directory from the 
patch for git status. I have attached a patch that does not bind 
default-directory. (And I now have turned in the assignment paperwork to 
the FSF!).

I continue to think that binding default-directory in vc-git-state makes 
the most sense.

> If vc-git supports calling vc-git-state from outside of the 
> repository, but not some other commands, or if vc-git-state does but 
> some other backends' vc-xx-state does not, this will increase 
> inconsistency and make it harder for the programmers to write 
> VCS-agnostic code, which is one of the main goals of VC.

But being vcs-agnostic is more for people who use vc-state or 
vc-state-refresh, rather than people actually writing the vc-specific 
functions, no? If this bug is unchanged, people using vc-state have to 
ask themselves: "if the backend is git, I have to bind default-directory 
but if it's not, I don't." That is inconsistent with vc agnosticism.

> Is it necessary for correct output in other backends, in similar 
> scenarios?
>
> I agree that fixing this makes sense, but it should be done in an 
> organized fashion

That's a valid question and a valid point. But I actually think that 
adding the binding in vc-git-state furthers the goal of systematically 
answering them. I doubt we'll be able to find, at one time, people with 
knowledge of more than 4 or 5 of the vc's (e.g. I know only git) who 
have the time to address this issue. What we can do instead, though, is 
to encourage people to properly write the backend specific functions. 
If, at some point, we notice that every vc-BACKEND-state binds 
default-directory, then we can move the binding to vc-state.

However, this question reinforces my opinion that binding 
default-directory can be treated satisfactorily as a backend-specific 
question. The fact that I can't argue in general that an arbitrary vc 
system needs default-directory set suggests to me that one should leave 
it to the vc system to decide. For example, I could imagine a backend 
that wants the current directory to be the root directory of the 
repository, rather than the directory containing the file.

[-- Attachment #2: 0001-update-vc-git.patch --]
[-- Type: text/x-patch, Size: 4563 bytes --]

From cc492722e9ae974e568a2bf79121dfd8664a766e Mon Sep 17 00:00:00 2001
From: Jonathan Ganc <jonganc@gmail.com>
Date: Mon, 10 Apr 2017 00:38:52 -0400
Subject: [PATCH] update vc-git

---
 lisp/vc/vc-git.el | 78 ++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 52 insertions(+), 26 deletions(-)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 1a3f1bf..213bdaa 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -231,34 +231,60 @@ (defun vc-git--state-code (code)
     (?U 'edited)     ;; FIXME
     (?T 'edited)))   ;; FIXME
 
+(defun vc-git--git-status-to-vc-state (code-list)
+  "Convert a list CODE-LIST of two-letter git status strings to a vc status.
+
+Each element of CODE-LIST comes from the first two characters of
+a line returned by 'git status' and should be passed in the order given by 'git status'.
+
+\(It is necessary to allow CODE-LIST to be a list because
+sometimes git status returns multiple lines, e.g. for a file that
+is removed from the index but is present in the HEAD and working
+tree.) "
+  (pcase code-list
+    ('nil 'up-to-date)
+    (`(,code)
+     (pcase code
+       ("!!" 'ignored)
+       ("??" 'unregistered)
+       ;; I have only seen this with a file that is only present in the
+       ;; index.  Let us call this `removed'
+       ("AD" 'removed)
+       (_ (cond
+           ((string-match-p "^[ RD]+$" code) 'removed)
+           ((string-match-p "^[ M]+$" code) 'edited)
+           ((string-match-p "^[ A]+$" code) 'added)
+           ((string-match-p "^[ U]+$" code) 'conflict)
+           (t 'edited)))))
+    ;;  I know of two times when git state returns more than one element,
+    ;;  in both cases returning '("D " "??")':
+    ;;  1. when a file is removed from the index but present in the
+    ;;     HEAD and working tree
+    ;;  2. when a file A is renamed to B in the index and then back to A
+    ;;     in the working tree
+    ;;  In both these instances, `unregistered' is a reasonable response.
+    (`("D " "??") 'unregistered)
+    ;;  In other cases, let us return `edited'.
+    (_ 'edited)))
+
 (defun vc-git-state (file)
   "Git-specific version of `vc-state'."
-  ;; FIXME: This can't set 'ignored or 'conflict yet
-  ;; The 'ignored state could be detected with `git ls-files -i -o
-  ;; --exclude-standard` It also can't set 'needs-update or
-  ;; 'needs-merge. The rough equivalent would be that upstream branch
-  ;; for current branch is in fast-forward state i.e. current branch
-  ;; is direct ancestor of corresponding upstream branch, and the file
-  ;; was modified upstream.  But we can't check that without a network
-  ;; operation.
-  ;; This assumes that status is known to be not `unregistered' because
-  ;; we've been successfully dispatched here from `vc-state', that
-  ;; means `vc-git-registered' returned t earlier once.  Bug#11757
-  (let ((diff (vc-git--run-command-string
-               file "diff-index" "-p" "--raw" "-z" "HEAD" "--")))
-    (if (and diff
-             (string-match ":[0-7]\\{6\\} [0-7]\\{6\\} [0-9a-f]\\{40\\} [0-9a-f]\\{40\\} \\([ADMUT]\\)\0[^\0]+\0\\(.*\n.\\)?"
-                           diff))
-        (let ((diff-letter (match-string 1 diff)))
-          (if (not (match-beginning 2))
-              ;; Empty diff: file contents is the same as the HEAD
-              ;; revision, but timestamps are different (eg, file
-              ;; was "touch"ed).  Update timestamp in index:
-              (prog1 'up-to-date
-                (vc-git--call nil "add" "--refresh" "--"
-                              (file-relative-name file)))
-            (vc-git--state-code diff-letter)))
-      (if (vc-git--empty-db-p) 'added 'up-to-date))))
+  (let ((status
+         (vc-git--run-command-string file "status" "--porcelain" "-z"
+                                     "--untracked-files" "--ignored" "--"))
+        code-list)
+    (if (null status)
+        ;; If status is nil, there was an error running git, likely because
+        ;; the file is not in a git repo.
+        'unregistered
+      ;; If this code is adapted to parse 'git status' for a directory,
+      ;; note that a renamed file takes up two null values and needs to be
+      ;; treated slightly more carefully.
+      (setq code-list
+            (mapcar (lambda (s)
+                      (substring s 0 2))
+                    (delete "" (split-string status "\0"))))
+      (vc-git--git-status-to-vc-state code-list))))
 
 (defun vc-git-working-revision (_file)
   "Git-specific version of `vc-working-revision'."
-- 
2.9.3

bug#26066: 26.0.50; vc-git-status gives wrong result

[Jonathan Ganc]

Since there have been no more objections to my patch (the one in my 
previous note does not bind default-directory), could someone merge it in?

bug#26066: 26.0.50; vc-git-status gives wrong result

[Noam Postavsky]

> Subject: [PATCH] update vc-git
>
> ---

Could you add a meaningful commit message please?  See CONTRIBUTE (under
"Commit messages") for details of Emacs' standard format.

I think the patch is basically okay now, just a few minor nitpicks below.

> +(defun vc-git--git-status-to-vc-state (code-list)
> +  "Convert a list CODE-LIST of two-letter git status strings to a vc status.

This line is too long, I think it should be fine to shorten to just
"Convert CODE-LIST to a vc status".  You explain the format of
CODE-LIST in the next paragraph anyway.

> +Each element of CODE-LIST comes from the first two characters of
> +a line returned by 'git status' and should be passed in the order given by 'git status'.

This paragraph looks unfilled, hit M-q on it.

> +       ;; I have only seen this with a file that is only present in the
> +       ;; index.  Let us call this `removed'

Missing period.

> +      (setq code-list
> +            (mapcar (lambda (s)
> +                      (substring s 0 2))
> +                    (delete "" (split-string status "\0"))))

If you pass a non-nil OMIT-NULLS parameter to split-string, the
(delete ""...) should become unnecessary.

> +      (vc-git--git-status-to-vc-state code-list))))

I would suggest dropping the temporary code-list variable here, and
just do

      (vc-git--git-status-to-vc-state
       (mapcar (lambda (s) (substring s 0 2))
               (split-string status "\0" t)))

bug#26066: 26.0.50; vc-git-status gives wrong result

[Stefan Monnier]

>> +                    (delete "" (split-string status "\0"))))
> If you pass a non-nil OMIT-NULLS parameter to split-string, the
> (delete ""...) should become unnecessary.

Or use "\0+".


        Stefan

bug#26066: 26.0.50; vc-git-status gives wrong result

[Jonathan Ganc]

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

Thanks for the feedback!

Updated with the suggested changes.


On 04/23/2017 06:28 PM, Noam Postavsky wrote:
>> Subject: [PATCH] update vc-git
>>
>> ---
> Could you add a meaningful commit message please?  See CONTRIBUTE (under
> "Commit messages") for details of Emacs' standard format.
>
> I think the patch is basically okay now, just a few minor nitpicks below.
>
>> +(defun vc-git--git-status-to-vc-state (code-list)
>> +  "Convert a list CODE-LIST of two-letter git status strings to a vc status.
> This line is too long, I think it should be fine to shorten to just
> "Convert CODE-LIST to a vc status".  You explain the format of
> CODE-LIST in the next paragraph anyway.
>
>> +Each element of CODE-LIST comes from the first two characters of
>> +a line returned by 'git status' and should be passed in the order given by 'git status'.
> This paragraph looks unfilled, hit M-q on it.
>
>> +       ;; I have only seen this with a file that is only present in the
>> +       ;; index.  Let us call this `removed'
> Missing period.
>
>> +      (setq code-list
>> +            (mapcar (lambda (s)
>> +                      (substring s 0 2))
>> +                    (delete "" (split-string status "\0"))))
> If you pass a non-nil OMIT-NULLS parameter to split-string, the
> (delete ""...) should become unnecessary.
>
>> +      (vc-git--git-status-to-vc-state code-list))))
> I would suggest dropping the temporary code-list variable here, and
> just do
>
>        (vc-git--git-status-to-vc-state
>         (mapcar (lambda (s) (substring s 0 2))
>                 (split-string status "\0" t)))


[-- Attachment #2: 0001-Update-vc-git-state-to-use-git-status.patch --]
[-- Type: text/x-patch, Size: 4704 bytes --]

From 5c123593c110323edfe357c26ee136de31c14ab8 Mon Sep 17 00:00:00 2001
From: Jonathan Ganc <jonganc@gmail.com>
Date: Mon, 10 Apr 2017 00:38:52 -0400
Subject: [PATCH] Update vc-git-state to use 'git status'

* lisp/vc/vc-git.el (vc-git-state, vc-git--git-status-to-vc-state): Update
'vc-git-state' to use 'git status', so that 'vc-git-state' can now return
'ignored', 'conflict', or 'unregistered' if appropriate. Related to
bug#26066.
---
 lisp/vc/vc-git.el | 78 ++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 52 insertions(+), 26 deletions(-)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 1a3f1bf..9fb35bd 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -231,34 +231,60 @@ (defun vc-git--state-code (code)
     (?U 'edited)     ;; FIXME
     (?T 'edited)))   ;; FIXME
 
+(defun vc-git--git-status-to-vc-state (code-list)
+  "Convert CODE-LIST to a vc status.
+
+Each element of CODE-LIST comes from the first two characters of
+a line returned by 'git status' and should be passed in the order
+given by 'git status'.
+
+\(It is necessary to allow CODE-LIST to be a list because
+sometimes git status returns multiple lines, e.g. for a file that
+is removed from the index but is present in the HEAD and working
+tree.) "
+  (pcase code-list
+    ('nil 'up-to-date)
+    (`(,code)
+     (pcase code
+       ("!!" 'ignored)
+       ("??" 'unregistered)
+       ;; I have only seen this with a file that is only present in the
+       ;; index.  Let us call this `removed'.
+       ("AD" 'removed)
+       (_ (cond
+           ((string-match-p "^[ RD]+$" code) 'removed)
+           ((string-match-p "^[ M]+$" code) 'edited)
+           ((string-match-p "^[ A]+$" code) 'added)
+           ((string-match-p "^[ U]+$" code) 'conflict)
+           (t 'edited)))))
+    ;;  I know of two times when git state returns more than one element,
+    ;;  in both cases returning '("D " "??")':
+    ;;  1. when a file is removed from the index but present in the
+    ;;     HEAD and working tree
+    ;;  2. when a file A is renamed to B in the index and then back to A
+    ;;     in the working tree
+    ;;  In both these instances, `unregistered' is a reasonable response.
+    (`("D " "??") 'unregistered)
+    ;;  In other cases, let us return `edited'.
+    (_ 'edited)))
+
 (defun vc-git-state (file)
   "Git-specific version of `vc-state'."
-  ;; FIXME: This can't set 'ignored or 'conflict yet
-  ;; The 'ignored state could be detected with `git ls-files -i -o
-  ;; --exclude-standard` It also can't set 'needs-update or
-  ;; 'needs-merge. The rough equivalent would be that upstream branch
-  ;; for current branch is in fast-forward state i.e. current branch
-  ;; is direct ancestor of corresponding upstream branch, and the file
-  ;; was modified upstream.  But we can't check that without a network
-  ;; operation.
-  ;; This assumes that status is known to be not `unregistered' because
-  ;; we've been successfully dispatched here from `vc-state', that
-  ;; means `vc-git-registered' returned t earlier once.  Bug#11757
-  (let ((diff (vc-git--run-command-string
-               file "diff-index" "-p" "--raw" "-z" "HEAD" "--")))
-    (if (and diff
-             (string-match ":[0-7]\\{6\\} [0-7]\\{6\\} [0-9a-f]\\{40\\} [0-9a-f]\\{40\\} \\([ADMUT]\\)\0[^\0]+\0\\(.*\n.\\)?"
-                           diff))
-        (let ((diff-letter (match-string 1 diff)))
-          (if (not (match-beginning 2))
-              ;; Empty diff: file contents is the same as the HEAD
-              ;; revision, but timestamps are different (eg, file
-              ;; was "touch"ed).  Update timestamp in index:
-              (prog1 'up-to-date
-                (vc-git--call nil "add" "--refresh" "--"
-                              (file-relative-name file)))
-            (vc-git--state-code diff-letter)))
-      (if (vc-git--empty-db-p) 'added 'up-to-date))))
+  (let ((status
+         (vc-git--run-command-string file "status" "--porcelain" "-z"
+                                     "--untracked-files" "--ignored" "--")))
+    (if (null status)
+        ;; If status is nil, there was an error running git, likely because
+        ;; the file is not in a git repo.
+        'unregistered
+      ;; If this code is adapted to parse 'git status' for a directory,
+      ;; note that a renamed file takes up two null values and needs to be
+      ;; treated slightly more carefully. 
+      (vc-git--git-status-to-vc-state
+       (mapcar (lambda (s)
+                 (substring s 0 2))
+               (split-string status "\0" t))))))
+
 
 (defun vc-git-working-revision (_file)
   "Git-specific version of `vc-working-revision'."
-- 
2.9.3

bug#26066: 26.0.50; vc-git-status gives wrong result

[Andreas Schwab]

On Apr 23 2017, Jonathan Ganc <jonganc@gmail.com> wrote:

> +(defun vc-git--git-status-to-vc-state (code-list)
> +  "Convert CODE-LIST to a vc status.
> +
> +Each element of CODE-LIST comes from the first two characters of
> +a line returned by 'git status' and should be passed in the order
> +given by 'git status'.
> +
> +\(It is necessary to allow CODE-LIST to be a list because
> +sometimes git status returns multiple lines, e.g. for a file that
> +is removed from the index but is present in the HEAD and working
> +tree.) "

I think the last paragraph could just be a code comment, given that you
even put it in parens to de-emphasize it.  Also, extra space at the end.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#26066: 26.0.50; vc-git-status gives wrong result

[Jonathan Ganc]

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

Good point. Changed.


On 04/24/2017 05:20 AM, Andreas Schwab wrote:
> On Apr 23 2017, Jonathan Ganc <jonganc@gmail.com> wrote:
>
>> +(defun vc-git--git-status-to-vc-state (code-list)
>> +  "Convert CODE-LIST to a vc status.
>> +
>> +Each element of CODE-LIST comes from the first two characters of
>> +a line returned by 'git status' and should be passed in the order
>> +given by 'git status'.
>> +
>> +\(It is necessary to allow CODE-LIST to be a list because
>> +sometimes git status returns multiple lines, e.g. for a file that
>> +is removed from the index but is present in the HEAD and working
>> +tree.) "
> I think the last paragraph could just be a code comment, given that you
> even put it in parens to de-emphasize it.  Also, extra space at the end.
>
> Andreas.
>


[-- Attachment #2: 0001-Update-vc-git-state-to-use-git-status.patch --]
[-- Type: text/x-patch, Size: 4712 bytes --]

From 6407a6dee770a3981977625c9b0648ca29c4f99c Mon Sep 17 00:00:00 2001
From: Jonathan Ganc <jonganc@gmail.com>
Date: Mon, 10 Apr 2017 00:38:52 -0400
Subject: [PATCH] Update vc-git-state to use 'git status'

* lisp/vc/vc-git.el (vc-git-state, vc-git--git-status-to-vc-state): Update
'vc-git-state' to use 'git status', so that 'vc-git-state' can now return
'ignored', 'conflict', or 'unregistered' if appropriate. Related to
bug#26066.
---
 lisp/vc/vc-git.el | 76 ++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 50 insertions(+), 26 deletions(-)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 1a3f1bf..6604220 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -231,34 +231,58 @@ (defun vc-git--state-code (code)
     (?U 'edited)     ;; FIXME
     (?T 'edited)))   ;; FIXME
 
+(defun vc-git--git-status-to-vc-state (code-list)
+  "Convert CODE-LIST to a vc status.
+
+Each element of CODE-LIST comes from the first two characters of
+a line returned by 'git status' and should be passed in the order
+given by 'git status'."
+  ;; It is necessary to allow CODE-LIST to be a list because sometimes git
+  ;; status returns multiple lines, e.g. for a file that is removed from
+  ;; the index but is present in the HEAD and working tree.
+  (pcase code-list
+    ('nil 'up-to-date)
+    (`(,code)
+     (pcase code
+       ("!!" 'ignored)
+       ("??" 'unregistered)
+       ;; I have only seen this with a file that is only present in the
+       ;; index.  Let us call this `removed'.
+       ("AD" 'removed)
+       (_ (cond
+           ((string-match-p "^[ RD]+$" code) 'removed)
+           ((string-match-p "^[ M]+$" code) 'edited)
+           ((string-match-p "^[ A]+$" code) 'added)
+           ((string-match-p "^[ U]+$" code) 'conflict)
+           (t 'edited)))))
+    ;;  I know of two times when git state returns more than one element,
+    ;;  in both cases returning '("D " "??")':
+    ;;  1. when a file is removed from the index but present in the
+    ;;     HEAD and working tree
+    ;;  2. when a file A is renamed to B in the index and then back to A
+    ;;     in the working tree
+    ;;  In both these instances, `unregistered' is a reasonable response.
+    (`("D " "??") 'unregistered)
+    ;;  In other cases, let us return `edited'.
+    (_ 'edited)))
+
 (defun vc-git-state (file)
   "Git-specific version of `vc-state'."
-  ;; FIXME: This can't set 'ignored or 'conflict yet
-  ;; The 'ignored state could be detected with `git ls-files -i -o
-  ;; --exclude-standard` It also can't set 'needs-update or
-  ;; 'needs-merge. The rough equivalent would be that upstream branch
-  ;; for current branch is in fast-forward state i.e. current branch
-  ;; is direct ancestor of corresponding upstream branch, and the file
-  ;; was modified upstream.  But we can't check that without a network
-  ;; operation.
-  ;; This assumes that status is known to be not `unregistered' because
-  ;; we've been successfully dispatched here from `vc-state', that
-  ;; means `vc-git-registered' returned t earlier once.  Bug#11757
-  (let ((diff (vc-git--run-command-string
-               file "diff-index" "-p" "--raw" "-z" "HEAD" "--")))
-    (if (and diff
-             (string-match ":[0-7]\\{6\\} [0-7]\\{6\\} [0-9a-f]\\{40\\} [0-9a-f]\\{40\\} \\([ADMUT]\\)\0[^\0]+\0\\(.*\n.\\)?"
-                           diff))
-        (let ((diff-letter (match-string 1 diff)))
-          (if (not (match-beginning 2))
-              ;; Empty diff: file contents is the same as the HEAD
-              ;; revision, but timestamps are different (eg, file
-              ;; was "touch"ed).  Update timestamp in index:
-              (prog1 'up-to-date
-                (vc-git--call nil "add" "--refresh" "--"
-                              (file-relative-name file)))
-            (vc-git--state-code diff-letter)))
-      (if (vc-git--empty-db-p) 'added 'up-to-date))))
+  (let ((status
+         (vc-git--run-command-string file "status" "--porcelain" "-z"
+                                     "--untracked-files" "--ignored" "--")))
+    (if (null status)
+        ;; If status is nil, there was an error running git, likely because
+        ;; the file is not in a git repo.
+        'unregistered
+      ;; If this code is adapted to parse 'git status' for a directory,
+      ;; note that a renamed file takes up two null values and needs to be
+      ;; treated slightly more carefully. 
+      (vc-git--git-status-to-vc-state
+       (mapcar (lambda (s)
+                 (substring s 0 2))
+               (split-string status "\0" t))))))
+
 
 (defun vc-git-working-revision (_file)
   "Git-specific version of `vc-working-revision'."
-- 
2.9.3

bug#26066: 26.0.50; vc-git-status gives wrong result

[Dmitry Gutov]

On 25.04.2017 18:38, Jonathan Ganc wrote:
> Good point. Changed.

Thank you very much, Jonathan, and thanks to the other reviewers!

I'll install the patch shortly.

bug#26066: 26.0.50; vc-git-status gives wrong result

[Jonathan Ganc]

Thanks Dmitry! And to all the reviewers, I very much appreciated the 
feedback!


On 04/25/2017 07:49 PM, Dmitry Gutov wrote:
> On 25.04.2017 18:38, Jonathan Ganc wrote:
>> Good point. Changed.
>
> Thank you very much, Jonathan, and thanks to the other reviewers!
>
> I'll install the patch shortly.

bug#26066: 26.0.50; vc-git-status gives wrong result

[Dmitry Gutov]

On 26.04.2017 6:18, Jonathan Ganc wrote:
> Thanks Dmitry! And to all the reviewers, I very much appreciated the 
> feedback!

Patch installed, with some minor copy edits. Keeping the bug report open 
because it's actually about the default-directory binding.

bug#26066: 26.0.50; vc-git-status gives wrong result

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 26.04.2017 6:18, Jonathan Ganc wrote:
>> Thanks Dmitry! And to all the reviewers, I very much appreciated the
>> feedback!
>
> Patch installed, with some minor copy edits. Keeping the bug report
> open because it's actually about the default-directory binding.

The original problem here was fixed and the patch applied -- is the
default-directory binding still being considered?

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

bug#26066: 26.0.50; vc-git-status gives wrong result

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Dmitry Gutov <dgutov@yandex.ru> writes:
>
>> On 26.04.2017 6:18, Jonathan Ganc wrote:
>>> Thanks Dmitry! And to all the reviewers, I very much appreciated the
>>> feedback!
>>
>> Patch installed, with some minor copy edits. Keeping the bug report
>> open because it's actually about the default-directory binding.
>
> The original problem here was fixed and the patch applied -- is the
> default-directory binding still being considered?

I asked this a year ago, and there was no response, so I'm guessing not,
and I'm closing the bug report.  Please reopen (or refile a new report,
since this is rather long and involved) if this is still an issue.

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

bug#26066: 26.0.50; vc-git-status gives wrong result

[Dmitry Gutov]

On 14.04.2017 3:42, Jonathan Ganc wrote:
> First, I'm sorry for this delayed response. I've had a busy few days 
> (and I needed to think about the issue)!

I'm sorry about the delay as well.

> I'm fine with decoupling the question about default-directory from the 
> patch for git status. I have attached a patch that does not bind 
> default-directory. (And I now have turned in the assignment paperwork to 
> the FSF!).

Thanks! That part is applied now.

>> If vc-git supports calling vc-git-state from outside of the 
>> repository, but not some other commands, or if vc-git-state does but 
>> some other backends' vc-xx-state does not, this will increase 
>> inconsistency and make it harder for the programmers to write 
>> VCS-agnostic code, which is one of the main goals of VC.
> 
> But being vcs-agnostic is more for people who use vc-state or 
> vc-state-refresh, rather than people actually writing the vc-specific 
> functions, no?

The vc-specific functions are supposed to conform to the common 
interface. To be as indistinguishable as feasible.

> If this bug is unchanged, people using vc-state have to 
> ask themselves: "if the backend is git, I have to bind default-directory 
> but if it's not, I don't." That is inconsistent with vc agnosticism.

If that was only the Git backend's problem, I might have agreed.

However, not all other backends bind default-directory. Bzr doesn't, as 
one example. Hg and SVN seem to do that, though. If you have a chance to 
experiment with some other popular ones, please do.

But what about the problems like "I don't have to bind default-directory 
when calling vc-state, but I still have to do that when calling other 
backend commands"?

> That's a valid question and a valid point. But I actually think that 
> adding the binding in vc-git-state furthers the goal of systematically 
> answering them.

Since we'll have to hunt down every other command that needs it (or, 
worse, wait for bug reports and thus spread that effort across several 
Emacs releases), I'm not so sure it would be a great step.

> I doubt we'll be able to find, at one time, people with 
> knowledge of more than 4 or 5 of the vc's (e.g. I know only git) who 
> have the time to address this issue.

I think we only have 4-5 backends which correspond to reasonably popular 
version control systems. The rest can simply follow their example.

> What we can do instead, though, is 
> to encourage people to properly write the backend specific functions.

How?

> The fact that I can't argue in general that an arbitrary vc 
> system needs default-directory set suggests to me that one should leave 
> it to the vc system to decide. For example, I could imagine a backend 
> that wants the current directory to be the root directory of the 
> repository, rather than the directory containing the file.

Such backend (I'm not aware of any, though) could still rebind 
default-directory inside its function.

bug#26066: 26.0.50; vc-git-status gives wrong result

[Jonathan Ganc]

On 04/30/2017 09:57 PM, Dmitry Gutov wrote:
>
> I'm sorry about the delay as well.
>
No worries.
>
>> If this bug is unchanged, people using vc-state have to ask 
>> themselves: "if the backend is git, I have to bind default-directory 
>> but if it's not, I don't." That is inconsistent with vc agnosticism.
>
> However, not all other backends bind default-directory. Bzr doesn't, 
> as one example. Hg and SVN seem to do that, though. If you have a 
> chance to experiment with some other popular ones, please do.
>
But it's not clear to me that bzr needs to bind it. In other words, git 
needs to bind it get a working answer and perhaps bzr doesn't. 
Unfortunately, I only know git. If I get a chance I'll look at some 
others but I've never used them.

Community message: If anyone else who uses other VC systems is reading 
this, it'd great to know if vc-state gives the correct answer if 
default-directory is bound to a random directory!

> But what about the problems like "I don't have to bind 
> default-directory when calling vc-state, but I still have to do that 
> when calling other backend commands"?
>
This is just about clarification: are you thinking that one possible 
solution is not to have any of the vc code bind default-directory, i.e. 
that the caller might always be responsible for that?

bug#26066: 26.0.50; vc-git-status gives wrong result

[Eli Zaretskii]

> From: Jonathan Ganc <jonganc@gmail.com>
> Date: Mon, 1 May 2017 15:22:39 -0400
> 
> Community message: If anyone else who uses other VC systems is reading 
> this, it'd great to know if vc-state gives the correct answer if 
> default-directory is bound to a random directory!

I tried that with bzr, and vc-state produces a wrong result if
default-directory is not set to the correct value.

bug#26066: 26.0.50; vc-git-status gives wrong result

[Dmitry Gutov]

On 01.05.2017 22:22, Jonathan Ganc wrote:

>> But what about the problems like "I don't have to bind 
>> default-directory when calling vc-state, but I still have to do that 
>> when calling other backend commands"?
>>
> This is just about clarification: are you thinking that one possible 
> solution is not to have any of the vc code bind default-directory, i.e. 
> that the caller might always be responsible for that?

Suppose so. I do not particularly like any of the obvious options.

Making the caller bind default-directory is not ideal, but sprinkling 
default-directory bindings over all VC command implementations doesn't 
sound great.

The former seems less error-prone in the end, though. And in that 
approach, the caller package might find just one place they have to put 
the default-directory binding at, so that it works for multiple calls.

bug#26066: 26.0.50; vc-git-status gives wrong result

[Jonathan Ganc]

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

Hi.

I have never submitted an emacs patch before so I don't know if I am 
doing this correctly (or if one of you has already submitted a fix). I 
have attached a patch file.

Later, I will submit the more elegant fix.


On 03/15/2017 08:48 PM, Dmitry Gutov wrote:
> On 16.03.2017 02:42, npostavs@users.sourceforge.net wrote:
>> As an aside, is there a reason that vc-git-status does not use "git
>> status -z --porcelain --ignored"? It seems more natural/straightfoward
>> and it would also deal with ignored and removed files. It could also
>> work with directories, which the current function doesn't.
>
> Like mentioned in a discussion on a different bug, we'll accept a 
> patch like that. You can even reuse some code from 
> `vc-git-conflicted-files`.


[-- Attachment #2: 0001-Fix-default-directory-for-vc-git-Bug-2606.patch --]
[-- Type: text/x-patch, Size: 1251 bytes --]

From 85a9df8a15b32dc7d884efa74e53c6d20fabaee6 Mon Sep 17 00:00:00 2001
From: Jonathan Ganc <jonganc@gmail.com>
Date: Fri, 17 Mar 2017 22:35:46 -0400
Subject: [PATCH] Fix default-directory for vc-git (Bug#2606)

---
 lisp/vc/vc-git.el | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 1a3f1bf..6f20a8d 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -244,8 +244,12 @@ (defun vc-git-state (file)
   ;; This assumes that status is known to be not `unregistered' because
   ;; we've been successfully dispatched here from `vc-state', that
   ;; means `vc-git-registered' returned t earlier once.  Bug#11757
-  (let ((diff (vc-git--run-command-string
-               file "diff-index" "-p" "--raw" "-z" "HEAD" "--")))
+
+  ;; `default-directory' determines where vc-git-... looks for the
+  ;; local git repository
+  (let* ((default-directory (file-name-directory file))
+         (diff (vc-git--run-command-string
+                file "diff-index" "-p" "--raw" "-z" "HEAD" "--")))
     (if (and diff
              (string-match ":[0-7]\\{6\\} [0-7]\\{6\\} [0-9a-f]\\{40\\} [0-9a-f]\\{40\\} \\([ADMUT]\\)\0[^\0]+\0\\(.*\n.\\)?"
                            diff))
-- 
2.9.3