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

[Jonathan Ganc <jonganc@gmail.com>]

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