GNU bug report logs -
#21383
Static revisions in vc-working-revision
Previous Next
Reported by: Jonathan H <pythonnut <at> gmail.com>
Date: Mon, 31 Aug 2015 00:47:01 UTC
Severity: wishlist
Done: Dmitry Gutov <dgutov <at> yandex.ru>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 21383 in the body.
You can then email your comments to 21383 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Mon, 31 Aug 2015 00:47:01 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Jonathan H <pythonnut <at> gmail.com>:
New bug report received and forwarded. Copy sent to
bug-gnu-emacs <at> gnu.org.
(Mon, 31 Aug 2015 00:47:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
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
[Message part 2 (text/html, inline)]
[0001-Add-CONCRETE-parameter-to-vc-working-revision.patch (application/octet-stream, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Mon, 31 Aug 2015 04:46:02 GMT)
Full text and
rfc822 format available.
Message #8 received at 21383 <at> debbugs.gnu.org (full text, mbox):
> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Mon, 31 Aug 2015 08:48:02 GMT)
Full text and
rfc822 format available.
Message #11 received at 21383 <at> debbugs.gnu.org (full text, mbox):
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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Mon, 31 Aug 2015 17:45:02 GMT)
Full text and
rfc822 format available.
Message #14 received at 21383 <at> debbugs.gnu.org (full text, mbox):
> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Tue, 01 Sep 2015 02:12:01 GMT)
Full text and
rfc822 format available.
Message #17 received at 21383 <at> debbugs.gnu.org (full text, mbox):
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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Tue, 01 Sep 2015 03:56:01 GMT)
Full text and
rfc822 format available.
Message #20 received at 21383 <at> debbugs.gnu.org (full text, mbox):
> -(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
Reply sent
to
Dmitry Gutov <dgutov <at> yandex.ru>:
You have taken responsibility.
(Tue, 01 Sep 2015 12:06:01 GMT)
Full text and
rfc822 format available.
Notification sent
to
Jonathan H <pythonnut <at> gmail.com>:
bug acknowledged by developer.
(Tue, 01 Sep 2015 12:06:02 GMT)
Full text and
rfc822 format available.
Message #25 received at 21383-done <at> debbugs.gnu.org (full text, mbox):
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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Tue, 01 Sep 2015 15:46:02 GMT)
Full text and
rfc822 format available.
Message #28 received at 21383-done <at> debbugs.gnu.org (full text, mbox):
> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Tue, 01 Sep 2015 15:55:01 GMT)
Full text and
rfc822 format available.
Message #31 received at 21383-done <at> debbugs.gnu.org (full text, mbox):
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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Tue, 01 Sep 2015 16:53:02 GMT)
Full text and
rfc822 format available.
Message #34 received at 21383-done <at> debbugs.gnu.org (full text, mbox):
> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Tue, 01 Sep 2015 17:24:02 GMT)
Full text and
rfc822 format available.
Message #37 received at 21383-done <at> debbugs.gnu.org (full text, mbox):
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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Wed, 02 Sep 2015 03:51:02 GMT)
Full text and
rfc822 format available.
Message #40 received at 21383-done <at> debbugs.gnu.org (full text, mbox):
> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Wed, 02 Sep 2015 10:50:02 GMT)
Full text and
rfc822 format available.
Message #43 received at 21383-done <at> debbugs.gnu.org (full text, mbox):
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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Wed, 02 Sep 2015 22:45:02 GMT)
Full text and
rfc822 format available.
Message #46 received at 21383-done <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
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 <at> 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.
>
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Thu, 03 Sep 2015 12:57:02 GMT)
Full text and
rfc822 format available.
Message #49 received at 21383-done <at> debbugs.gnu.org (full text, mbox):
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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Thu, 03 Sep 2015 16:05:02 GMT)
Full text and
rfc822 format available.
Message #52 received at 21383-done <at> debbugs.gnu.org (full text, mbox):
> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Thu, 03 Sep 2015 16:18:01 GMT)
Full text and
rfc822 format available.
Message #55 received at 21383-done <at> debbugs.gnu.org (full text, mbox):
> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Thu, 03 Sep 2015 17:35:02 GMT)
Full text and
rfc822 format available.
Message #58 received at 21383-done <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
> 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 <at> 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
>
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Thu, 03 Sep 2015 18:42:02 GMT)
Full text and
rfc822 format available.
Message #61 received at 21383-done <at> debbugs.gnu.org (full text, mbox):
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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Thu, 03 Sep 2015 19:26:01 GMT)
Full text and
rfc822 format available.
Message #64 received at 21383-done <at> debbugs.gnu.org (full text, mbox):
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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Thu, 03 Sep 2015 20:08:02 GMT)
Full text and
rfc822 format available.
Message #67 received at 21383-done <at> debbugs.gnu.org (full text, mbox):
> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Thu, 03 Sep 2015 22:33:02 GMT)
Full text and
rfc822 format available.
Message #70 received at 21383-done <at> debbugs.gnu.org (full text, mbox):
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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Fri, 04 Sep 2015 02:22:01 GMT)
Full text and
rfc822 format available.
Message #73 received at 21383-done <at> debbugs.gnu.org (full text, mbox):
> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Fri, 04 Sep 2015 14:37:02 GMT)
Full text and
rfc822 format available.
Message #76 received at 21383-done <at> debbugs.gnu.org (full text, mbox):
>> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Sat, 05 Sep 2015 02:31:02 GMT)
Full text and
rfc822 format available.
Message #79 received at 21383-done <at> debbugs.gnu.org (full text, mbox):
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 :)).
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Sat, 05 Sep 2015 03:09:01 GMT)
Full text and
rfc822 format available.
Message #82 received at 21383-done <at> debbugs.gnu.org (full text, mbox):
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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Sat, 05 Sep 2015 15:14:02 GMT)
Full text and
rfc822 format available.
Message #85 received at 21383-done <at> debbugs.gnu.org (full text, mbox):
>> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Sat, 05 Sep 2015 20:31:01 GMT)
Full text and
rfc822 format available.
Message #88 received at 21383-done <at> debbugs.gnu.org (full text, mbox):
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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#21383; Package
emacs.
(Sun, 06 Sep 2015 22:30:04 GMT)
Full text and
rfc822 format available.
Message #91 received at 21383-done <at> debbugs.gnu.org (full text, mbox):
>> 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
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Mon, 05 Oct 2015 11:24:04 GMT)
Full text and
rfc822 format available.
This bug report was last modified 9 years and 262 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.