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

[Jonathan Ganc <jonganc@gmail.com>]

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