bug#21383: Static revisions in vc-working-revision

bug#21383: Static revisions in vc-working-revision

[Jonathan H]

[-- Attachment #1.1: Type: text/plain, Size: 980 bytes --]

Hello all!

I've attached a basic patch that adds an option to vc-working-revision. The
option is named *concrete* and if non-nil, it forces vc-working-revision to
return a revision name that will not go stale after new revisions are made.

This is useful for, e.g. git, where vc-working-revision will just return
the branch name, which only refers to the current commit for as long as
it's the head of the branch.

I'm using this in diff-hl #33 <https://github.com/dgutov/diff-hl/issues/33>to
determine when to refresh the current VC highlighting.

I've supplied an implementation for Git, and no-op implementations for all
the other backends. For most systems (i.e. all the other VCS systems I
know), the value of *concrete *does not matter. If you know a backend that
would benefit from a real implementation, please let me know.

Also, this is my first patch, so I'm not entirely sure I've got all my
ducks in a row. Any comments on that would be great too.

Thanks,
Jonathan

[-- Attachment #1.2: Type: text/html, Size: 1202 bytes --]

[-- Attachment #2: 0001-Add-CONCRETE-parameter-to-vc-working-revision.patch --]
[-- Type: application/octet-stream, Size: 7392 bytes --]

From 5fc7dff76102ea1e097bc39cbceb8b7d71df447d Mon Sep 17 00:00:00 2001
From: PythonNut <PythonNut@users.noreply.github.com>
Date: Sat, 22 Aug 2015 01:25:44 +0000
Subject: [PATCH] Add CONCRETE parameter to vc-working-revision

If CONCRETE is non-nil, the revision will be tied to an unambiguous commit
instead of the normal symbolic ref.
---
 lisp/vc/vc-bzr.el   |  2 +-
 lisp/vc/vc-cvs.el   |  2 +-
 lisp/vc/vc-git.el   | 12 ++++++------
 lisp/vc/vc-hg.el    |  2 +-
 lisp/vc/vc-hooks.el | 18 +++++++++++-------
 lisp/vc/vc-mtn.el   |  2 +-
 lisp/vc/vc-rcs.el   |  2 +-
 lisp/vc/vc-sccs.el  |  2 +-
 lisp/vc/vc-src.el   |  2 +-
 lisp/vc/vc-svn.el   |  2 +-
 10 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/lisp/vc/vc-bzr.el b/lisp/vc/vc-bzr.el
index 5f8dd0b..953179e 100644
--- a/lisp/vc/vc-bzr.el
+++ b/lisp/vc/vc-bzr.el
@@ -532,7 +532,7 @@ Returns nil if unable to find this information."
              (looking-at "[0-9]+\0\\([^\0\n]+\\)\0")
              (match-string 1))))))
 
-(defun vc-bzr-working-revision (file)
+(defun vc-bzr-working-revision (file &optional concrete)
   (let* ((rootdir (vc-bzr-root file))
          (branch-format-file (expand-file-name vc-bzr-admin-branch-format-file
                                                rootdir))
diff --git a/lisp/vc/vc-cvs.el b/lisp/vc/vc-cvs.el
index 9a41905..290b8cb 100644
--- a/lisp/vc/vc-cvs.el
+++ b/lisp/vc/vc-cvs.el
@@ -258,7 +258,7 @@ See also variable `vc-cvs-sticky-date-format-string'."
      ((null checkout-time) 'unregistered)
      (t 'edited))))
 
-(defun vc-cvs-working-revision (file)
+(defun vc-cvs-working-revision (file &optional concrete)
   "CVS-specific version of `vc-working-revision'."
   ;; There is no need to consult RCS headers under CVS, because we
   ;; get the workfile version for free when we recognize that a file
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 9522328..84e2025 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -248,15 +248,15 @@ matching the resulting Git log output, and KEYWORDS is a list of
             (vc-git--state-code diff-letter)))
       (if (vc-git--empty-db-p) 'added 'up-to-date))))
 
-(defun vc-git-working-revision (file)
+(defun vc-git-working-revision (file &optional concrete)
   "Git-specific version of `vc-working-revision'."
   (let* (process-file-side-effects
-         (str (vc-git--run-command-string nil "symbolic-ref" "HEAD")))
+          (str (vc-git--run-command-string nil "symbolic-ref" "HEAD")))
     (vc-file-setprop file 'vc-git-detached (null str))
-    (if str
-        (if (string-match "^\\(refs/heads/\\)?\\(.+\\)$" str)
-            (match-string 2 str)
-          str)
+    (if (and str (not concrete))
+      (if (string-match "^\\(refs/heads/\\)?\\(.+\\)$" str)
+        (match-string 2 str)
+        str)
       (vc-git--rev-parse "HEAD"))))
 
 (defun vc-git-mode-line-string (file)
diff --git a/lisp/vc/vc-hg.el b/lisp/vc/vc-hg.el
index f634e2e..eee8c85 100644
--- a/lisp/vc/vc-hg.el
+++ b/lisp/vc/vc-hg.el
@@ -237,7 +237,7 @@ highlighting the Log View buffer."
 	 ((eq state ?C) 'up-to-date) ;; Older mercurial versions use this.
 	 (t 'up-to-date))))))
 
-(defun vc-hg-working-revision (file)
+(defun vc-hg-working-revision (file &optional concrete)
   "Hg-specific version of `vc-working-revision'."
   (or (ignore-errors
         (with-output-to-string
diff --git a/lisp/vc/vc-hooks.el b/lisp/vc/vc-hooks.el
index bae9919..504efb9 100644
--- a/lisp/vc/vc-hooks.el
+++ b/lisp/vc/vc-hooks.el
@@ -490,15 +490,19 @@ status of this file.  Otherwise, the value returned is one of:
   "Convenience function that checks whether `vc-state' of FILE is `up-to-date'."
   (eq (vc-state file) 'up-to-date))
 
-(defun vc-working-revision (file &optional backend)
+(defun vc-working-revision (file &optional backend concrete)
   "Return the repository version from which FILE was checked out.
-If FILE is not registered, this function always returns nil."
-  (or (vc-file-getprop file 'vc-working-revision)
+If FILE is not registered, this function always returns nil.
+If CONCRETE is non-nil, the revision will be tied to an unambiguous commit
+instead of the normal symbolic ref."
+  (if concrete
+    (vc-call-backend backend 'working-revision file t)
+    (or (vc-file-getprop file 'vc-working-revision)
       (progn
-	(setq backend (or backend (vc-responsible-backend file)))
-	(when backend
-	  (vc-file-setprop file 'vc-working-revision
-			   (vc-call-backend backend 'working-revision file))))))
+        (setq backend (or backend (vc-responsible-backend file)))
+        (when backend
+          (vc-file-setprop file 'vc-working-revision
+            (vc-call-backend backend 'working-revision file)))))))
 
 ;; Backward compatibility.
 (define-obsolete-function-alias
diff --git a/lisp/vc/vc-mtn.el b/lisp/vc/vc-mtn.el
index 685ef3b..63efb9f 100644
--- a/lisp/vc/vc-mtn.el
+++ b/lisp/vc/vc-mtn.el
@@ -147,7 +147,7 @@ switches."
   (vc-run-delayed
    (vc-mtn-after-dir-status update-function)))
 
-(defun vc-mtn-working-revision (file)
+(defun vc-mtn-working-revision (file &optional concrete)
   ;; If `mtn' fails or returns status>0, or if the search fails, just
   ;; return nil.
   (ignore-errors
diff --git a/lisp/vc/vc-rcs.el b/lisp/vc/vc-rcs.el
index 71ffa55..5a5ed76 100644
--- a/lisp/vc/vc-rcs.el
+++ b/lisp/vc/vc-rcs.el
@@ -168,7 +168,7 @@ For a description of possible values, see `vc-check-master-templates'."
 	  (push (list frel state) result))))
     (funcall update-function result)))
 
-(defun vc-rcs-working-revision (file)
+(defun vc-rcs-working-revision (file &optional concrete)
   "RCS-specific version of `vc-working-revision'."
   (or (and vc-consult-headers
            (vc-rcs-consult-headers file)
diff --git a/lisp/vc/vc-sccs.el b/lisp/vc/vc-sccs.el
index 8d8d9e8..f7fdb73 100644
--- a/lisp/vc/vc-sccs.el
+++ b/lisp/vc/vc-sccs.el
@@ -147,7 +147,7 @@ For a description of possible values, see `vc-check-master-templates'."
 
 (autoload 'vc-master-name "vc-filewise")
 
-(defun vc-sccs-working-revision (file)
+(defun vc-sccs-working-revision (file &optional concrete)
   "SCCS-specific version of `vc-working-revision'."
   (when (and (file-regular-p file) (vc-master-name file))
     (with-temp-buffer
diff --git a/lisp/vc/vc-src.el b/lisp/vc/vc-src.el
index d9aa1b1..d2dd505 100644
--- a/lisp/vc/vc-src.el
+++ b/lisp/vc/vc-src.el
@@ -198,7 +198,7 @@ This function differs from vc-do-command in that it invokes `vc-src-program'."
 	   (setq file-list (cons "--" file-or-list))))
     (apply 'vc-do-command (or buffer "*vc*") 0 vc-src-program file-list flags)))
 
-(defun vc-src-working-revision (file)
+(defun vc-src-working-revision (file &optional concrete)
   "SRC-specific version of `vc-working-revision'."
   (let ((result (ignore-errors
 		  (with-output-to-string
diff --git a/lisp/vc/vc-svn.el b/lisp/vc/vc-svn.el
index 8d6eae5..01cb1e4 100644
--- a/lisp/vc/vc-svn.el
+++ b/lisp/vc/vc-svn.el
@@ -240,7 +240,7 @@ RESULT is a list of conses (FILE . STATE) for directory DIR."
 	     (propertize repo 'face 'font-lock-variable-name-face)))
 	   (t "")))))
 
-(defun vc-svn-working-revision (file)
+(defun vc-svn-working-revision (file &optional concrete)
   "SVN-specific version of `vc-working-revision'."
   ;; There is no need to consult RCS headers under SVN, because we
   ;; get the workfile version for free when we recognize that a file
-- 
2.5.0

bug#21383: Static revisions in vc-working-revision

[Stefan Monnier]

> I've supplied an implementation for Git, and no-op implementations for all
> the other backends.

Actually, I think what you're getting at, is that the current vc-git
backend has a bug: the working-revision should not just be the branch
name, but should be the actual commit id.


        Stefan

bug#21383: Static revisions in vc-working-revision

[Dmitry Gutov]

On 08/31/2015 07:45 AM, Stefan Monnier wrote:

> Actually, I think what you're getting at, is that the current vc-git
> backend has a bug: the working-revision should not just be the branch
> name, but should be the actual commit id.

That's one possible conclusion. But if we simply change 
vc-git-working-revision to always return a concrete revision, 
vc-git-mode-line-string would return it as well.

And being able to see the current branch in the mode-line is pretty handy.

bug#21383: Static revisions in vc-working-revision

[Stefan Monnier]

> And being able to see the current branch in the mode-line is pretty handy.

So we need to change vc-git-mode-line-string to put the branch name
in there.  Doesn't seem particularly hard.


        Stefan

bug#21383: Static revisions in vc-working-revision

[Dmitry Gutov]

On 08/31/2015 08:44 PM, Stefan Monnier wrote:

> So we need to change vc-git-mode-line-string to put the branch name
> in there.  Doesn't seem particularly hard.

That sounds like a plan.

Any misgivings about this patch?


diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 9522328..50c6d96 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -248,26 +248,30 @@ matching the resulting Git log output, and 
KEYWORDS is a list of
              (vc-git--state-code diff-letter)))
        (if (vc-git--empty-db-p) 'added 'up-to-date))))

-(defun vc-git-working-revision (file)
+(defun vc-git-working-revision (_file)
    "Git-specific version of `vc-working-revision'."
-  (let* (process-file-side-effects
-         (str (vc-git--run-command-string nil "symbolic-ref" "HEAD")))
-    (vc-file-setprop file 'vc-git-detached (null str))
-    (if str
-        (if (string-match "^\\(refs/heads/\\)?\\(.+\\)$" str)
-            (match-string 2 str)
-          str)
-      (vc-git--rev-parse "HEAD"))))
+  (let (process-file-side-effects)
+    (vc-git--rev-parse "HEAD")))
+
+(defun vc-git--symbolic-ref (file)
+  (or
+   (vc-file-getprop file 'vc-git-symbolic-ref)
+   (let* (process-file-side-effects
+          (str (vc-git--run-command-string nil "symbolic-ref" "HEAD")))
+     (vc-file-setprop file 'vc-git-symbolic-ref
+                      (if str
+                          (if (string-match 
"^\\(refs/heads/\\)?\\(.+\\)$" str)
+                              (match-string 2 str)
+                            str))))))

  (defun vc-git-mode-line-string (file)
    "Return a string for `vc-mode-line' to put in the mode line for FILE."
    (let* ((rev (vc-working-revision file))
-         (detached (vc-file-getprop file 'vc-git-detached))
+         (disp-rev (or (vc-git--symbolic-ref file)
+                       (substring rev 0 7)))
           (def-ml (vc-default-mode-line-string 'Git file))
           (help-echo (get-text-property 0 'help-echo def-ml)))
-    (propertize (if detached
-                    (substring def-ml 0 (- 7 (length rev)))
-                  def-ml)
+    (propertize (replace-regexp-in-string (concat rev "\\'") disp-rev 
def-ml t t)
                  'help-echo (concat help-echo "\nCurrent revision: " 
rev))))

  (cl-defstruct (vc-git-extra-fileinfo

bug#21383: Static revisions in vc-working-revision

[Stefan Monnier]

> -(defun vc-git-working-revision (file)
> +(defun vc-git-working-revision (_file)

VC was originally designed so as to separate the VC data from the
buffers, so the `file' argument was absolutely indispensable (you
can't/shouldn't rely on things like the current-buffer's default-directory).

I think nowadays the design has been changed enough that indeed the
`file' arg can be dispensed with.

The rest looks OK to me,


        Stefan

bug#21383: Static revisions in vc-working-revision

[Dmitry Gutov]

On 09/01/2015 06:55 AM, Stefan Monnier wrote:

> VC was originally designed so as to separate the VC data from the
> buffers, so the `file' argument was absolutely indispensable (you
> can't/shouldn't rely on things like the current-buffer's default-directory).
>
> I think nowadays the design has been changed enough that indeed the
> `file' arg can be dispensed with.

I think just the current usage, everywhere, makes it okay. But if I were 
to inquire of the status of a file in a different repository, that would 
still fail. We don't don't seem to do that anywhere, though, and 
supporting that usage would require fixes in multiple places.

This patch doesn't change much in this regard: FILE wasn't used for its 
default-directory even before it.

> The rest looks OK to me,

Thanks, installed.

bug#21383: Static revisions in vc-working-revision

[Stefan Monnier]

> I think just the current usage, everywhere, makes it okay. But if I were to
> inquire of the status of a file in a different repository, that would still
> fail. We don't don't seem to do that anywhere, though, and supporting that
> usage would require fixes in multiple places.
> This patch doesn't change much in this regard: FILE wasn't used for its
> default-directory even before it.

That's right.  I was just pointing out this change in VC's practice.
I think the current practice is not well documented (maybe not
completely well defined either).


        Stefan

bug#21383: Static revisions in vc-working-revision

[Dmitry Gutov]

On 09/01/2015 06:45 PM, Stefan Monnier wrote:

> That's right.  I was just pointing out this change in VC's practice.
> I think the current practice is not well documented (maybe not
> completely well defined either).

I'd rather not document it yet: officially limiting API's applicability 
is not ideal.

Rather let's wait for a report from someone who triggers this limitation 
of vc-git and then see if we want to support their use case.

bug#21383: Static revisions in vc-working-revision

[Stefan Monnier]

> Rather let's wait for a report from someone who triggers this limitation of
> vc-git and then see if we want to support their use case.

I think it goes much further than vc-git.
Rather, it permeates most of the VC code.


        Stefan

bug#21383: Static revisions in vc-working-revision

[Dmitry Gutov]

On 09/01/2015 07:52 PM, Stefan Monnier wrote:

> I think it goes much further than vc-git.
> Rather, it permeates most of the VC code.

Maybe you see it better. I only imagined the problem limited to the 
non-file-granularity backends.

And we can't simply remove the FILE argument in many backend commands: 
it's often used for vc-file-get/setprop.

bug#21383: Static revisions in vc-working-revision

[Stefan Monnier]

> Maybe you see it better.  I only imagined the problem limited to the
> non-file-granularity backends.

You mean like most current backends? ;-)

> And we can't simply remove the FILE argument in many backend commands: it's
> often used for vc-file-get/setprop.

I know.

In any case, it's not that bad: it works.  But there's something fishy
that will surely bite at some point.  Maybe those FILE args should be
redefined to be relative to default-directory (and can't use things like
"../..").


        Stefan

bug#21383: Static revisions in vc-working-revision

[Dmitry Gutov]

On 09/02/2015 06:50 AM, Stefan Monnier wrote:
>> Maybe you see it better.  I only imagined the problem limited to the
>> non-file-granularity backends.
>
> You mean like most current backends? ;-)

Yes. But as long as its only limited to the backends (and can be fixed 
there), as opposed to being inherently present in vc.el, log-edit.el, 
etc, it's less of a problem.

>> And we can't simply remove the FILE argument in many backend commands: it's
>> often used for vc-file-get/setprop.
>
> I know.
>
> In any case, it's not that bad: it works.  But there's something fishy
> that will surely bite at some point.  Maybe those FILE args should be
> redefined to be relative to default-directory (and can't use things like
> "../..").

vc-file-setprop won't work on a relative path. Or shouldn't, at least.

And are you talking about FILE arg to vc-status, or e.g. vc-git-status? 
If vc-status requires the path to be relative, that will complicate the 
consumer interface (now I have to worry about producing the relative 
path). If vc-status will be responsible for that before calling 
vc-git-status, it won't work in file-granular backends. Anyway, why 
would we want that extra call, even if it's cached?

And vc-git-working-revision won't care if FILE is absolute or relative, 
which is the crux of the problem. I'd rather backends like Git, if we're 
going to fix this, used FILE's parent directory to change 
default-directory temporarily before calling Git.

bug#21383: Static revisions in vc-working-revision

[Jonathan H]

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

Something seems a bit amiss with the current patch.

I'm using (vc-working-revision buffer-file-name (vc-responsible-backend
buffer-file-name)) to determine the current working revision.

As far as I can tell, vc-working-revision will cache the current working
revision in a file property, but it doesn't know when to invalidate the
cache, so the results can be stale. In fact, in the specific case of git,
you probably can't get any faster than a rev-parse anyway, so caching
doesn't make much sense.

I have a sneaking suspicion that git isn't the only backend where this can
happen.

Thanks,
Jonathan



On Wed, Sep 2, 2015 at 3:49 AM, Dmitry Gutov <dgutov@yandex.ru> wrote:

> On 09/02/2015 06:50 AM, Stefan Monnier wrote:
>
>> Maybe you see it better.  I only imagined the problem limited to the
>>> non-file-granularity backends.
>>>
>>
>> You mean like most current backends? ;-)
>>
>
> Yes. But as long as its only limited to the backends (and can be fixed
> there), as opposed to being inherently present in vc.el, log-edit.el, etc,
> it's less of a problem.
>
> And we can't simply remove the FILE argument in many backend commands: it's
>>> often used for vc-file-get/setprop.
>>>
>>
>> I know.
>>
>> In any case, it's not that bad: it works.  But there's something fishy
>> that will surely bite at some point.  Maybe those FILE args should be
>> redefined to be relative to default-directory (and can't use things like
>> "../..").
>>
>
> vc-file-setprop won't work on a relative path. Or shouldn't, at least.
>
> And are you talking about FILE arg to vc-status, or e.g. vc-git-status? If
> vc-status requires the path to be relative, that will complicate the
> consumer interface (now I have to worry about producing the relative path).
> If vc-status will be responsible for that before calling vc-git-status, it
> won't work in file-granular backends. Anyway, why would we want that extra
> call, even if it's cached?
>
> And vc-git-working-revision won't care if FILE is absolute or relative,
> which is the crux of the problem. I'd rather backends like Git, if we're
> going to fix this, used FILE's parent directory to change default-directory
> temporarily before calling Git.
>

[-- Attachment #2: Type: text/html, Size: 3256 bytes --]

bug#21383: Static revisions in vc-working-revision

[Dmitry Gutov]

On 09/03/2015 01:44 AM, Jonathan H wrote:

> I'm using (vc-working-revision buffer-file-name (vc-responsible-backend
> buffer-file-name)) to determine the current working revision.
>
> As far as I can tell, vc-working-revision will cache the current working
> revision in a file property, but it doesn't know when to invalidate the
> cache, so the results can be stale.

Yes, invalidation of those properties could use some improvement. But if 
you're just worried about up-to-date results returned to your functions, 
you could remove the property before calling `vc-working-revision'.

> In fact, in the specific case of
> git, you probably can't get any faster than a rev-parse anyway,

I wonder if that's true. Caching in a property is obviously fast, and 
process calls are necessarily an order of magnitude slower (and slower 
still on certain platforms).

Maybe you should write a patch and test it on Windows (or have someone 
else do it), and see whether the mode-line updates don't become 
perceptibly slower.

> I have a sneaking suspicion that git isn't the only backend where this
> can happen.

Probably. On the other hand, there are known backends where calling the 
backend program *is* slow. Such as Bazaar or (most likely) CVS.

bug#21383: Static revisions in vc-working-revision

[Stefan Monnier]

> Yes.  But as long as its only limited to the backends (and can be fixed
> there), as opposed to being inherently present in vc.el, log-edit.el, etc,
> it's less of a problem.

I think the problem is in the contract between VC itself and the
backends, because VC mostly assumes that default-directory doesn't
matter and uses absolute file names instead (that was the original
design), whereas for many backends this is sometimes inconvenient so
they occasionally rely on default-directory instead, which happens to
work as well, tho it's mostly an accident.

> vc-file-setprop won't work on a relative path. Or shouldn't, at least.

Clearly, what I suggest would require changes in the core VC code, yes.

> And are you talking about FILE arg to vc-status, or e.g. vc-git-status?

vc-git-status (and other backend operations).

> And vc-git-working-revision won't care if FILE is absolute or relative,
> which is the crux of the problem. I'd rather backends like Git, if we're
> going to fix this, used FILE's parent directory to change default-directory
> temporarily before calling Git.

Right, we could fix the problem by keeping the original design and
making sure the backends actually follow it, but I'm not sure it's the
better design nowadays (and since using default-directory happens to
work in 99% of the cases, it's hard to make sure we really fix all cases
where we incorrectly rely on default-directory being the right parent of
the absolute file names we get).


        Stefan

bug#21383: Static revisions in vc-working-revision

[Stefan Monnier]

> Maybe you should write a patch and test it on Windows (or have someone else
> do it), and see whether the mode-line updates don't become
> perceptibly slower.

Indeed, mode-line updates are *very* frequent (more than once per
command), so we really want to avoid running processes at that point.


        Stefan

bug#21383: Static revisions in vc-working-revision

[Jonathan H]

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

> Indeed, mode-line updates are *very* frequent (more than once per
> command), so we really want to avoid running processes at that point.

I though the new implementation of the vc-git mode line doesn't use
vc-working-revision, so in this specific case, making vc-working-revision
slower won't hurt us there.

> I wonder if that's true. Caching in a property is obviously fast, and
process calls are
necessarily an order of magnitude slower (and slower still on certain
platforms).

True, but you have to check that the cache is invalid somehow. If you want
to be super correct, a rev-parse is probably the fastest way. I suppose it
would be pretty fast to check the mtime of the .git directory inside emacs
and invalidate that when it changes. Although filesystem timestamps usually
make me nervous, it's probably okay in this instance?



On Thu, Sep 3, 2015 at 9:17 AM, Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> > Maybe you should write a patch and test it on Windows (or have someone
> else
> > do it), and see whether the mode-line updates don't become
> > perceptibly slower.
>
> Indeed, mode-line updates are *very* frequent (more than once per
> command), so we really want to avoid running processes at that point.
>
>
>         Stefan
>

[-- Attachment #2: Type: text/html, Size: 1792 bytes --]

bug#21383: Static revisions in vc-working-revision

[Dmitry Gutov]

On 09/03/2015 08:34 PM, Jonathan H wrote:

> I though the new implementation of the vc-git mode line doesn't use
> vc-working-revision,

Please look at the definition. vc-working-revision is called once in the 
beginning of the function, and again through vc-default-mode-line-string.

Even if we inlined the useful parts of vc-default-mode-line-string 
definition, calling vc-git-working-revision instead of 
vc-git--symbolic-ref is still inevitable in "detached" [Git] mode.

> True, but you have to check that the cache is invalid somehow. If you
> want to be super correct, a rev-parse is probably the fastest way. I

We err on the side of speed currently, instead of correctness. The file 
properties are invalidated during certain operations, like saving or 
reverting the buffer.

> suppose it would be pretty fast to check the mtime of the .git directory
> inside emacs and invalidate that when it changes. Although filesystem
> timestamps usually make me nervous, it's probably okay in this instance?

I don't know, but it's probably not too hard to imagine a user with a 
terminally slow HDD. Maybe it'll be fast enough anyway, but someone will 
have to do a patch, and test.

bug#21383: Static revisions in vc-working-revision

[Dmitry Gutov]

On 09/03/2015 07:04 PM, Stefan Monnier wrote:

> I think the problem is in the contract between VC itself and the
> backends, because VC mostly assumes that default-directory doesn't
> matter and uses absolute file names instead (that was the original
> design), whereas for many backends this is sometimes inconvenient so
> they occasionally rely on default-directory instead, which happens to
> work as well, tho it's mostly an accident.

Yes.

>> And are you talking about FILE arg to vc-status, or e.g. vc-git-status?
>
> vc-git-status (and other backend operations).

That might be viable. But the commands that don't use FILE currently 
only need the root, so we could avoid passing the argument in entirely.

> Right, we could fix the problem by keeping the original design and
> making sure the backends actually follow it, but I'm not sure it's the
> better design nowadays (and since using default-directory happens to
> work in 99% of the cases, it's hard to make sure we really fix all cases
> where we incorrectly rely on default-directory being the right parent of
> the absolute file names we get).

That is true. But could we abandon the current design for all backends? 
Some of the older ones still don't have vc-root implemented (because 
it's impossible for some of them?), and until it is, vc-state and 
friends won't know what to set default-directory to.

bug#21383: Static revisions in vc-working-revision

[Stefan Monnier]

> We err on the side of speed currently, instead of correctness. The file
> properties are invalidated during certain operations, like saving or
> reverting the buffer.

Also VC was designed when revisions were per-file, and the way we
extended it is to say that it's OK to have an out-of-date revision
number for a given file, if that file's content is the same in the
current revision.


        Stefan

bug#21383: Static revisions in vc-working-revision

[Dmitry Gutov]

On 09/03/2015 11:07 PM, Stefan Monnier wrote:

> Also VC was designed when revisions were per-file, and the way we
> extended it is to say that it's OK to have an out-of-date revision
> number for a given file, if that file's content is the same in the
> current revision.

This might be one of the more annoying discrepancies, and one that's not 
too hard to improve.

I wonder if I've missed any other good places to reset the root's 
properties.

diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
index ec55867..a27c873 100644
--- a/lisp/vc/vc-dispatcher.el
+++ b/lisp/vc/vc-dispatcher.el
@@ -567,7 +567,10 @@ editing!"
    (if (string= buffer-file-name file)
        (vc-resynch-window file keep noquery reset-vc-info)
      (if (file-directory-p file)
-	(vc-resynch-buffers-in-directory file keep noquery reset-vc-info)
+        (progn
+          (when reset-vc-info
+            (vc-file-clearprops file))
+          (vc-resynch-buffers-in-directory file keep noquery 
reset-vc-info))
        (let ((buffer (get-file-buffer file)))
  	(when buffer
  	  (with-current-buffer buffer
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 8a0f554..33d742c 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -254,15 +254,16 @@ matching the resulting Git log output, and 
KEYWORDS is a list of
      (vc-git--rev-parse "HEAD")))

  (defun vc-git--symbolic-ref (file)
-  (or
-   (vc-file-getprop file 'vc-git-symbolic-ref)
-   (let* (process-file-side-effects
-          (str (vc-git--run-command-string nil "symbolic-ref" "HEAD")))
-     (vc-file-setprop file 'vc-git-symbolic-ref
-                      (if str
-                          (if (string-match 
"^\\(refs/heads/\\)?\\(.+\\)$" str)
-                              (match-string 2 str)
-                            str))))))
+  (let ((root (vc-git-root file)))
+    (or
+     (vc-file-getprop root 'vc-git-symbolic-ref)
+     (let* (process-file-side-effects
+            (str (vc-git--run-command-string nil "symbolic-ref" "HEAD")))
+       (vc-file-setprop root 'vc-git-symbolic-ref
+                        (if str
+                            (if (string-match 
"^\\(refs/heads/\\)?\\(.+\\)$" str)
+                                (match-string 2 str)
+                              str)))))))

  (defun vc-git-mode-line-string (file)
    "Return a string for `vc-mode-line' to put in the mode line for FILE."
diff --git a/lisp/vc/vc-hooks.el b/lisp/vc/vc-hooks.el
index e674f0e..4bee8ad 100644
--- a/lisp/vc/vc-hooks.el
+++ b/lisp/vc/vc-hooks.el
@@ -493,12 +493,15 @@ status of this file.  Otherwise, the value 
returned is one of:
  (defun vc-working-revision (file &optional backend)
    "Return the repository version from which FILE was checked out.
  If FILE is not registered, this function always returns nil."
-  (or (vc-file-getprop file 'vc-working-revision)
-      (progn
-	(setq backend (or backend (vc-responsible-backend file)))
-	(when backend
-	  (vc-file-setprop file 'vc-working-revision
-			   (vc-call-backend backend 'working-revision file))))))
+  (unless backend (setq backend (vc-responsible-backend file)))
+  (when backend
+    (let* ((granularity (vc-call-backend backend 'revision-granularity))
+           (prop-target (if (eq granularity 'repository)
+                            (vc-call-backend backend 'root file)
+                          file)))
+      (or (vc-file-getprop prop-target 'vc-working-revision)
+          (vc-file-setprop prop-target 'vc-working-revision
+                           (vc-call-backend backend 'working-revision 
file))))))

  ;; Backward compatibility.
  (define-obsolete-function-alias

bug#21383: Static revisions in vc-working-revision

[Stefan Monnier]

> That might be viable. But the commands that don't use FILE currently only
> need the root, so we could avoid passing the argument in entirely.

IIUC those operations are things like `working-revision', which should
return values which may depend on the FILE or not, depending on the
semantics of the backend.

So I don't think we can drop the FILE argument, but we can make it
clear that it's OK to ignore it and use default-directory instead.

That's why I'm suggesting to pass FILE as a relative file-name.
It is slightly delicate, tho, since the vc-root for default-directory
may actually be different from the vc-root for (expand-file-name
<relativename>).

> That is true. But could we abandon the current design for all backends? Some
> of the older ones still don't have vc-root implemented (because it's
> impossible for some of them?), and until it is, vc-state and friends won't
> know what to set default-directory to.

I don't think we should impose a constraint that default-directory is
vc-root.  So, backends like Git may still have to find the vc-root
from the default-directory (tho in many cases, the underlying executable
will do that for us).


        Stefan

bug#21383: Static revisions in vc-working-revision

[Stefan Monnier]

>> Also VC was designed when revisions were per-file, and the way we
>> extended it is to say that it's OK to have an out-of-date revision
>> number for a given file, if that file's content is the same in the
>> current revision.
> This might be one of the more annoying discrepancies,

Why?  In which kinds of circumstances can this bite us?


        Stefan

bug#21383: Static revisions in vc-working-revision

[Dmitry Gutov]

On 09/04/2015 05:36 PM, Stefan Monnier wrote:

> Why?  In which kinds of circumstances can this bite us?

It goes somewhere like this: I switch to a different branch (using the 
console), some buffers see that they're out of date and get reverted 
automatically (and/or I `M-x revert-buffer' in some buffer), and then 
some buffers from a project show one branch in their mode-line, and 
other buffers show a different branch.

That can be annoying and makes me question which branch is actually 
checked out.

(Note that the patch I've posted now makes M-x revert-buffer never 
update the current branch; that's a very consistent behavior :)).

bug#21383: Static revisions in vc-working-revision

[Dmitry Gutov]

On 09/04/2015 05:20 AM, Stefan Monnier wrote:

> So I don't think we can drop the FILE argument, but we can make it
> clear that it's OK to ignore it and use default-directory instead.

That, by itself, would be an easy change to make. In the docs.

> That's why I'm suggesting to pass FILE as a relative file-name.
> It is slightly delicate, tho, since the vc-root for default-directory
> may actually be different from the vc-root for (expand-file-name
> <relativename>).

My problem with that is passing a relative file-name doesn't help any 
backend, in any way: if the file-name is relative to the current 
default-directory, vc-git-* will continue behaving exactly the same, 
whether default-directory is the root, or simply the current directory.

If the file-name is relative to some other directory that the current 
(from vc-git's standpoint) default-directory, then vc-git operations 
will simply fail. Either way, the onus is on vc-working-revision and 
other generic functions to bind default-directory. And as long as 
default-directory is right, the file-names might as well stay absolute.

> I don't think we should impose a constraint that default-directory is
> vc-root.  So, backends like Git may still have to find the vc-root
> from the default-directory (tho in many cases, the underlying executable
> will do that for us).

If default-directory is outside of $git_repo, passing a path to a file 
inside it to 'git status' doesn't work. So someone still needs to bind 
default-directory to somewhere inside it.

However - this looks like the easiest solution - binding it to 
(file-name-directory file) should work well enough for backends with 
either type of revision granularity. At least as long as the backend 
program can be called in any subdirectory of the repository.

bug#21383: Static revisions in vc-working-revision

[Stefan Monnier]

>> That's why I'm suggesting to pass FILE as a relative file-name.
>> It is slightly delicate, tho, since the vc-root for default-directory
>> may actually be different from the vc-root for (expand-file-name
>> <relativename>).
> My problem with that is passing a relative file-name doesn't help any
> backend, in any way: if the file-name is relative to the current
> default-directory,

[ By and large, a file name that's not relative to default-directory is
  a dead file name.  ]

Yes, the file names would be relative to default-directory.

> functions to bind default-directory. And as long as default-directory is
> right, the file-names might as well stay absolute.

But that means we pass redundant info, and that's only acceptable if we
can make reasonably sure that the two copies are in sync.
Using relative file names we don't have the problem of keeping duplicate
info in sync.

>> I don't think we should impose a constraint that default-directory is
>> vc-root.  So, backends like Git may still have to find the vc-root
>> from the default-directory (tho in many cases, the underlying executable
>> will do that for us).
> If default-directory is outside of $git_repo, passing a path to a file
> inside it to 'git status' doesn't work.  So someone still needs to bind
> default-directory to somewhere inside it.

No, I think that it's perfectly acceptable to say that the Git branch
used depends solely on default-directory.  So if default-directory is
outside of $git_repo, you get what you asked for.


        Stefan

bug#21383: Static revisions in vc-working-revision

[Dmitry Gutov]

On 09/05/2015 06:12 PM, Stefan Monnier wrote:

> Yes, the file names would be relative to default-directory.

Does vc-status get a relative file name as input? If yes, a) it's 
additional work for the caller, b) how are you going to enforce it?.

Otherwise, (*).

> But that means we pass redundant info, and that's only acceptable if we
> can make reasonably sure that the two copies are in sync.
> Using relative file names we don't have the problem of keeping duplicate
> info in sync.

(*) If we try to eliminate this duplication of info, we introduce 
duplication of effort inside VC. Seems to be pointless complexity.

> No, I think that it's perfectly acceptable to say that the Git branch
> used depends solely on default-directory.  So if default-directory is
> outside of $git_repo, you get what you asked for.

This is true, but I'm not sure what you're getting at. Sorry.

bug#21383: Static revisions in vc-working-revision

[Stefan Monnier]

>> Yes, the file names would be relative to default-directory.
> Does vc-status get a relative file name as input?

Currently, I don't think so.  But that could be changed if needed.

> b) how are you going to enforce it?.

By checking all callers?

> (*) If we try to eliminate this duplication of info, we introduce
> duplication of effort inside VC. Seems to be pointless complexity.

Clearly it's easy to get back the absolute file name with just
(expand-file-name <FILE>), and I don't see where we'd obviously get
duplication of efforts.  So I'm not sure why you think it'd add complexity.

What I do see as a problem is that it would require a careful
study&adjustment of the whole VC code.  I'm not sure if the result would
be more complex or simpler, admittedly.  I think it'd be about the same,
just with cleaner semantics.

> This is true, but I'm not sure what you're getting at. Sorry.

I guess I don't know either, I was just pointing out that the problem
you point out can be defined away as a feature.


        Stefan