* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2015-09-25 12:45 bug#21559: 25.0.50; auto-revert-mode breaks git rebase Ben Gamari
@ 2015-09-26 7:41 ` Eli Zaretskii
2015-09-28 14:11 ` Ben Gamari
2015-10-26 18:43 ` bug#21559: [PATCH] autorevert: Wait for repository to become idle before calling vc-find-file-hook Ben Gamari
` (5 subsequent siblings)
6 siblings, 1 reply; 49+ messages in thread
From: Eli Zaretskii @ 2015-09-26 7:41 UTC (permalink / raw)
To: Ben Gamari; +Cc: 21559
> From: Ben Gamari <ben@smart-cactus.org>
> Date: Fri, 25 Sep 2015 14:45:56 +0200
>
> One imperfect workaround would be to instead schedule a worker to call
> `vc-fine-file-hook` at some point in the future when the repository is
> more likely to be idle (for instance, when there have been no change
> events for a second or so).
But autorevert already does exactly that: it doesn't act on file
changes immediately, only once every N seconds. N defaults to 5,
perhaps you could try customizing auto-revert-interval to a larger
value to see if that solves the problem.
In any case, the issue here is that Emacs doesn't know when "the
repository is idle". If someone can suggest how to tell that,
autorevert could be augmented not to revert VC files while the
repository is "busy". From your description I understand that just
looking at index.lock is not good enough, since Git releases it more
than once in this scenario, while the rebase operation is still
on-going.
Btw, what is your value of auto-revert-check-vc-info? If it's
non-nil, perhaps resetting it to nil will also avoid these problems.
^ permalink raw reply [flat|nested] 49+ messages in thread* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2015-09-26 7:41 ` Eli Zaretskii
@ 2015-09-28 14:11 ` Ben Gamari
2015-09-28 14:35 ` Eli Zaretskii
2015-09-29 8:47 ` Michael Albinus
0 siblings, 2 replies; 49+ messages in thread
From: Ben Gamari @ 2015-09-28 14:11 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 21559
[-- Attachment #1: Type: text/plain, Size: 2430 bytes --]
Eli Zaretskii <eliz@gnu.org> writes:
>> From: Ben Gamari <ben@smart-cactus.org>
>> Date: Fri, 25 Sep 2015 14:45:56 +0200
>>
>> One imperfect workaround would be to instead schedule a worker to call
>> `vc-fine-file-hook` at some point in the future when the repository is
>> more likely to be idle (for instance, when there have been no change
>> events for a second or so).
>
> But autorevert already does exactly that: it doesn't act on file
> changes immediately, only once every N seconds. N defaults to 5,
> perhaps you could try customizing auto-revert-interval to a larger
> value to see if that solves the problem.
>
This is not true is auto-revert-use-notify is enabled.
If I'm not mistaken, your suggestion amounts to increasing the poll
interval. This will never solve the problem, it will only mean that the
race has fewer opportunities to pop up. Of course, the same can be said
of my suggestion, which only narrows the race window, not remove it
altogether (which is sadly not possible without fixing git rebase).
As an aside, it came as quite a surprise that autorevert (if I
understand the code correctly) still polls even if
auto-revert-use-notify is set. It seems to me that this is itself a bug.
As a laptop user who often works on battery, I keep tabs on which
processes eat CPU time and I have noticed that emacs is consistently
behind only firefox and Xorg in CPU time demanded at idle.
> In any case, the issue here is that Emacs doesn't know when "the
> repository is idle". If someone can suggest how to tell that,
> autorevert could be augmented not to revert VC files while the
> repository is "busy". From your description I understand that just
> looking at index.lock is not good enough, since Git releases it more
> than once in this scenario, while the rebase operation is still
> on-going.
>
Right.
I threw together a patch [1] implementing the suggestion I presented
above. I have yet to rigorously test it but I have yet to experience the
issue since starting to use it.
> Btw, what is your value of auto-revert-check-vc-info? If it's
> non-nil, perhaps resetting it to nil will also avoid these problems.
It is nil but this doesn't matter as vc-find-file-hook is called if
either auto-revert-check-vc-info or the revert flag are set.
Cheers,
- Ben
[1] https://github.com/bgamari/emacs/compare/master...auto-revert-vc-idle
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2015-09-28 14:11 ` Ben Gamari
@ 2015-09-28 14:35 ` Eli Zaretskii
2015-09-28 15:05 ` Ben Gamari
2015-09-29 8:47 ` Michael Albinus
1 sibling, 1 reply; 49+ messages in thread
From: Eli Zaretskii @ 2015-09-28 14:35 UTC (permalink / raw)
To: Ben Gamari; +Cc: 21559
> From: Ben Gamari <ben@smart-cactus.org>
> Cc: 21559@debbugs.gnu.org
> Date: Mon, 28 Sep 2015 16:11:16 +0200
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> >> From: Ben Gamari <ben@smart-cactus.org>
> >> Date: Fri, 25 Sep 2015 14:45:56 +0200
> >>
> >> One imperfect workaround would be to instead schedule a worker to call
> >> `vc-fine-file-hook` at some point in the future when the repository is
> >> more likely to be idle (for instance, when there have been no change
> >> events for a second or so).
> >
> > But autorevert already does exactly that: it doesn't act on file
> > changes immediately, only once every N seconds. N defaults to 5,
> > perhaps you could try customizing auto-revert-interval to a larger
> > value to see if that solves the problem.
> >
> This is not true is auto-revert-use-notify is enabled.
You are right, I forgot: things were originally like I described, but
were later changed to fix bug#18958.
> I threw together a patch [1] implementing the suggestion I presented
> above. I have yet to rigorously test it but I have yet to experience the
> issue since starting to use it.
Thanks.
However, it looks like your suggested patch reverts back to what we
had before we fixed bug 18958, doesn't it? If so, we will have to
look for some other solution, because we don't want to reintroduce
that bug. See http://debbugs.gnu.org/cgi/bugreport.cgi?bug=18958#5
for the details.
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2015-09-28 14:35 ` Eli Zaretskii
@ 2015-09-28 15:05 ` Ben Gamari
0 siblings, 0 replies; 49+ messages in thread
From: Ben Gamari @ 2015-09-28 15:05 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 21559
[-- Attachment #1: Type: text/plain, Size: 884 bytes --]
Eli Zaretskii <eliz@gnu.org> writes:
>> From: Ben Gamari <ben@smart-cactus.org>
>> Cc: 21559@debbugs.gnu.org
>> Date: Mon, 28 Sep 2015 16:11:16 +0200
>>
>> I threw together a patch [1] implementing the suggestion I presented
>> above. I have yet to rigorously test it but I have yet to experience the
>> issue since starting to use it.
>
> Thanks.
>
> However, it looks like your suggested patch reverts back to what we
> had before we fixed bug 18958, doesn't it? If so, we will have to
> look for some other solution, because we don't want to reintroduce
> that bug. See http://debbugs.gnu.org/cgi/bugreport.cgi?bug=18958#5
> for the details.
I'm not sure that this is true. Note that my patch defers *only* the
call to vc-find-file-hook, not reverting the buffer (which, correct me
if I'm wrong, appears to be what this bug is about).
Cheers,
- Ben
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2015-09-28 14:11 ` Ben Gamari
2015-09-28 14:35 ` Eli Zaretskii
@ 2015-09-29 8:47 ` Michael Albinus
1 sibling, 0 replies; 49+ messages in thread
From: Michael Albinus @ 2015-09-29 8:47 UTC (permalink / raw)
To: Ben Gamari; +Cc: 21559
Ben Gamari <ben@smart-cactus.org> writes:
> As an aside, it came as quite a surprise that autorevert (if I
> understand the code correctly) still polls even if
> auto-revert-use-notify is set. It seems to me that this is itself a bug.
> As a laptop user who often works on battery, I keep tabs on which
> processes eat CPU time and I have noticed that emacs is consistently
> behind only firefox and Xorg in CPU time demanded at idle.
I remember there were some reasons to keep this polling behavior. Must
check years-old conversation in order to remember the reason.
> I threw together a patch [1] implementing the suggestion I presented
> above. I have yet to rigorously test it but I have yet to experience the
> issue since starting to use it.
Wouldn't it be better to provide a buffer-stale-function for files or
directories under vc control? Mode-specific functionality shouldn't be
added to auto-revert.el, I believe.
> Cheers,
>
> - Ben
Best regards, Michael.
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: [PATCH] autorevert: Wait for repository to become idle before calling vc-find-file-hook
2015-09-25 12:45 bug#21559: 25.0.50; auto-revert-mode breaks git rebase Ben Gamari
2015-09-26 7:41 ` Eli Zaretskii
@ 2015-10-26 18:43 ` Ben Gamari
2015-10-26 18:43 ` bug#21559: [PATCH] autorevert: Wait a while " Ben Gamari
2016-02-06 0:01 ` bug#21559: 25.0.50; auto-revert-mode breaks git rebase Mitchel Humpherys
` (4 subsequent siblings)
6 siblings, 1 reply; 49+ messages in thread
From: Ben Gamari @ 2015-10-26 18:43 UTC (permalink / raw)
To: emacs-devel; +Cc: 21559, michael.albinus
Hi all,
This is a patch that I've been using locally for a few months now to resolve
#21599, where auto-revert and vc conspire to break git-rebase. Ultimately the
problem is a race-condition which would require a change in git to resolve
completely, but in practice this timeout-based approach eliminates the issue.
Cheers,
- Ben
^ permalink raw reply [flat|nested] 49+ messages in thread* bug#21559: [PATCH] autorevert: Wait a while before calling vc-find-file-hook
2015-10-26 18:43 ` bug#21559: [PATCH] autorevert: Wait for repository to become idle before calling vc-find-file-hook Ben Gamari
@ 2015-10-26 18:43 ` Ben Gamari
2015-10-27 8:25 ` Michael Albinus
0 siblings, 1 reply; 49+ messages in thread
From: Ben Gamari @ 2015-10-26 18:43 UTC (permalink / raw)
To: emacs-devel; +Cc: Ben Gamari, 21559, michael.albinus
This provides a resolution for Bug #21559.
emacs running with `auto-revert-mode` enabled breaks `git rebase` in
repositories where files are open. The problem appears to be that
`auto-revert-mode` attempts to refresh version control information with
`vc-find-file-hook` on revert events. `vc-find-file-hook` calls out to
`git`, taking the repository's index lock.
This interferes badly with `git rebase`, which performs many git
commands in quick succession. When `auto-revert-mode` is enabled there
is a very high chance that the following race will occur,
git emacs
---------------------- -----------------------------
1. git rebase checks out
a commit, releases
`index.lock`
2. `auto-revert-mode` notices
change, firing off a `git`
process and taking
`index.lock`.
3. git rebase applies patch
and attempts to commit.
Notices that `index.lock`
is taken and fails.
4. emacs' `git` process
finishes, releasing lock
In the end the user is left with a badly broken rebase process and an
error message complaining that `index.lock` exists, which he then goes
to confirm and finds no such file as emacs has already released the
lock.
We work around this by scheduling a worker to call `vc-fine-file-hook`
at some point in the future when the repository is more likely to be
idle (for instance, when there have been no change events for a second
or so).
Signed-Off-By: Ben Gamari <ben@smart-cactus.org>
---
lisp/autorevert.el | 35 ++++++++++++++++++++++++++++++++++-
1 file changed, 34 insertions(+), 1 deletion(-)
diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index 37ee8ee..0c25ac3 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -136,6 +136,11 @@ auto-revert-tail-mode
(defvar auto-revert-timer nil
"Timer used by Auto-Revert Mode.")
+(defvar auto-revert-vc-check-timer nil
+ "Timer used by Auto-Revert Mode to schedule
+checks of version control information. See
+`auto-revert-schedule-vc-check' for details.")
+
(defcustom auto-revert-interval 5
"Time, in seconds, between Auto-Revert Mode file checks.
The value may be an integer or floating point number.
@@ -260,6 +265,13 @@ auto-revert-check-vc-info
:type 'boolean
:version "22.1")
+(defcustom auto-revert-vc-check-idle-time 2
+ "How much time to wait after noticing a changed file before calling
+`vc-find-file-hook' or nil to check immediately."
+ :group 'auto-revert
+ :type 'number
+ :version "25.1")
+
(defvar-local global-auto-revert-ignore-buffer nil
"When non-nil, Global Auto-Revert Mode will not revert this buffer.
This variable becomes buffer local when set in any fashion.")
@@ -671,7 +683,28 @@ auto-revert-handler
;; `preserve-modes' avoids changing the (minor) modes. But we do
;; want to reset the mode for VC, so we do it manually.
(when (or revert auto-revert-check-vc-info)
- (vc-find-file-hook))))
+ (auto-revert-schedule-vc-check))))
+
+(defun auto-revert-schedule-vc-check ()
+ "Schedule a call to `vc-find-file-hook'.
+
+We need to be careful when calling `vc-find-file-hook' after file changes
+as some version control utilities (e.g. git rebase) have a tendency
+to do many successive calls and will fail ungracefully if they find
+we have taken the repository lock.
+
+For this reason we wait for the repository to be idle for at least
+`auto-revert-vc-check-idle-time' seconds before calling
+`vc-find-file-hook'."
+ (if auto-revert-vc-check-idle-time
+ (progn
+ (when (timerp auto-revert-vc-check-timer)
+ (cancel-timer auto-revert-vc-check-timer))
+ (setq auto-revert-vc-check-idle-timer
+ (run-at-time auto-revert-vc-check-idle-time nil
+ 'vc-find-file-hook))
+ )
+ (vc-find-file-hook)))
(defun auto-revert-tail-handler (size)
(let ((modified (buffer-modified-p))
--
2.6.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* bug#21559: [PATCH] autorevert: Wait a while before calling vc-find-file-hook
2015-10-26 18:43 ` bug#21559: [PATCH] autorevert: Wait a while " Ben Gamari
@ 2015-10-27 8:25 ` Michael Albinus
2015-10-28 11:59 ` Ben Gamari
0 siblings, 1 reply; 49+ messages in thread
From: Michael Albinus @ 2015-10-27 8:25 UTC (permalink / raw)
To: Ben Gamari; +Cc: 21559
[removing emacs-devel, discussion in one ML is sufficient]
Ben Gamari <ben@smart-cactus.org> writes:
Hi Ben,
> This provides a resolution for Bug #21559.
Thanks for the patch.
But as I said already, I'm not happy to insert so much hard-wired vc
related operations into autorevert.el. And this is triggered just by
git, IIUC the other vc backends do not suffer from this problem.
Have you considered another solution, maybe by providing vc(-git)
specific buffer-stale-function and revert-buffer? This would be much
more in the spirit how autorevert is supposed to work. And this would
simplify maintenance, when git changes its behaviour, and we need to
adapt vc-git accordingly.
Best regards, Michael.
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: [PATCH] autorevert: Wait a while before calling vc-find-file-hook
2015-10-27 8:25 ` Michael Albinus
@ 2015-10-28 11:59 ` Ben Gamari
2015-10-28 14:45 ` Michael Albinus
0 siblings, 1 reply; 49+ messages in thread
From: Ben Gamari @ 2015-10-28 11:59 UTC (permalink / raw)
To: Michael Albinus; +Cc: 21559
[-- Attachment #1: Type: text/plain, Size: 1075 bytes --]
Hi Michael,
Michael Albinus <michael.albinus@gmx.de> writes:
> [removing emacs-devel, discussion in one ML is sufficient]
>
Okay. Works for me.
> Ben Gamari <ben@smart-cactus.org> writes:
>
> Hi Ben,
>
>> This provides a resolution for Bug #21559.
>
> Thanks for the patch.
>
> But as I said already, I'm not happy to insert so much hard-wired vc
> related operations into autorevert.el. And this is triggered just by
> git, IIUC the other vc backends do not suffer from this problem.
Fair enough.
> Have you considered another solution, maybe by providing vc(-git)
> specific buffer-stale-function and revert-buffer? This would be much
> more in the spirit how autorevert is supposed to work. And this would
> simplify maintenance,
Could you perhaps describe in a bit detail what the semantics of these
functions would be? I'm not entirely sure I understand what you are
proposing.
> when git changes its behaviour, and we need to adapt vc-git
> accordingly.
You are more optimistic than me on whether git will be fixed ;)
Thanks for your feedback!
Cheers,
- Ben
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: [PATCH] autorevert: Wait a while before calling vc-find-file-hook
2015-10-28 11:59 ` Ben Gamari
@ 2015-10-28 14:45 ` Michael Albinus
2015-10-28 17:05 ` Ben Gamari
0 siblings, 1 reply; 49+ messages in thread
From: Michael Albinus @ 2015-10-28 14:45 UTC (permalink / raw)
To: Ben Gamari; +Cc: 21559
Ben Gamari <ben@smart-cactus.org> writes:
> Hi Michael,
Hi Ben,
>> Have you considered another solution, maybe by providing vc(-git)
>> specific buffer-stale-function and revert-buffer? This would be much
>> more in the spirit how autorevert is supposed to work. And this would
>> simplify maintenance,
>
> Could you perhaps describe in a bit detail what the semantics of these
> functions would be? I'm not entirely sure I understand what you are
> proposing.
As starting point you might read about `revert-buffer-function' in
(info "(elisp)Reverting") and about `buffer-stale-function' in
(info "(emacs)Supporting additional buffers")
See also in a dired buffer the values for both variables, and how the
functions work for dired.
>> when git changes its behaviour, and we need to adapt vc-git
>> accordingly.
>
> You are more optimistic than me on whether git will be fixed ;)
I have no special opinion about git. I just use it, without being impressed.
> Cheers,
>
> - Ben
Best regards, Michael.
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: [PATCH] autorevert: Wait a while before calling vc-find-file-hook
2015-10-28 14:45 ` Michael Albinus
@ 2015-10-28 17:05 ` Ben Gamari
2015-10-29 8:20 ` Michael Albinus
0 siblings, 1 reply; 49+ messages in thread
From: Ben Gamari @ 2015-10-28 17:05 UTC (permalink / raw)
To: Michael Albinus; +Cc: 21559
[-- Attachment #1: Type: text/plain, Size: 1533 bytes --]
Hi Michael,
Michael Albinus <michael.albinus@gmx.de> writes:
>>> Have you considered another solution, maybe by providing vc(-git)
>>> specific buffer-stale-function and revert-buffer? This would be much
>>> more in the spirit how autorevert is supposed to work. And this would
>>> simplify maintenance,
>>
>> Could you perhaps describe in a bit detail what the semantics of these
>> functions would be? I'm not entirely sure I understand what you are
>> proposing.
>
> As starting point you might read about `revert-buffer-function' in
> (info "(elisp)Reverting") and about `buffer-stale-function' in
> (info "(emacs)Supporting additional buffers")
>
> See also in a dired buffer the values for both variables, and how the
> functions work for dired.
Thanks for the references.
I now have a better idea of what you are proposing. I do, however, fear
that your solution might be too large a hammer for the problem at hand.
Ultimately the problem here is an annoyingly narrow one, namely the fact
that auto-revert's action is correlated with file modifications due to
rebasing. For this reason I think it may be best to keep the solution
confined to auto-revert.
In particular, I'm afraid that the changing the semantics of
revert-buffer may break other users of this function who expect
its effect to be apparent immediately after invocation. It seems like
this approach may easily and unwittingly trade one subtle form of
breakage for another (even harder to find) one.
Cheers,
- Ben
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: [PATCH] autorevert: Wait a while before calling vc-find-file-hook
2015-10-28 17:05 ` Ben Gamari
@ 2015-10-29 8:20 ` Michael Albinus
0 siblings, 0 replies; 49+ messages in thread
From: Michael Albinus @ 2015-10-29 8:20 UTC (permalink / raw)
To: Ben Gamari; +Cc: 21559
Ben Gamari <ben@smart-cactus.org> writes:
> Hi Michael,
Hi Ben,
> Ultimately the problem here is an annoyingly narrow one, namely the fact
> that auto-revert's action is correlated with file modifications due to
> rebasing. For this reason I think it may be best to keep the solution
> confined to auto-revert.
>
> In particular, I'm afraid that the changing the semantics of
> revert-buffer may break other users of this function who expect
> its effect to be apparent immediately after invocation. It seems like
> this approach may easily and unwittingly trade one subtle form of
> breakage for another (even harder to find) one.
I agree with you not to change the behaviour of revert-buffer for other
use cases but auto-revert. One could do it by introducing a new local
variable auto-revert-buffer-in-progress-p (similar to
revert-buffer-in-progress-p), which is bound during auto-revert. Then it
would be possible to define a new function
(defun vc-git-revert-buffer-function (ignore-auto noconfirm)
"vc-git specific revert function."
(if (and auto-revert-buffer-in-progress-p
(check-for-git-rebase))
(do-something-special)
(revert-buffer--default ignore-auto noconfirm)))
One could also check, whether adding a function to before-revert-hook
could be helpful.
An alternative, w/o touching revert-buffer for vc-git files, would be
the provisiong of a special buffer-stale-function for vc-git. Something
like
(defun vc-git-buffer-stale-p (&optional noconfirm)
"vc-git specific buffer-stale function."
(check-that-git-needs-revert-and-it-is-safe-to-do-so))
This function returns t, when a buffer with a file under vc-git mode
needs a revert, and it is safe to revert it (no git rebase is
running). When autorevert either polls, or it is indicated by a file
notification event, it calls this function and reverts the buffer if the
function returns non-nil. If the function returns nil, auto-revert is
skipped for that buffer, and it is checked again by the next poll (after
5 seconds). By this, autorevert.el would stay clean (no additional vc
related code), and you would even not need to implement an own timer.
> Cheers,
>
> - Ben
Best regards, Michael.
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2015-09-25 12:45 bug#21559: 25.0.50; auto-revert-mode breaks git rebase Ben Gamari
2015-09-26 7:41 ` Eli Zaretskii
2015-10-26 18:43 ` bug#21559: [PATCH] autorevert: Wait for repository to become idle before calling vc-find-file-hook Ben Gamari
@ 2016-02-06 0:01 ` Mitchel Humpherys
2016-02-06 12:34 ` Ben Gamari
2016-09-09 20:56 ` Jason Merrill
` (3 subsequent siblings)
6 siblings, 1 reply; 49+ messages in thread
From: Mitchel Humpherys @ 2016-02-06 0:01 UTC (permalink / raw)
To: Ben Gamari; +Cc: 21559
On Fri, Sep 25 2015 at 03:45:56 PM, Ben Gamari <ben@smart-cactus.org> wrote:
> Arguably `git rebase` should be holding the `index.lock` for the entire
> duration of the process (or be more resilient to the lock being taken)
> but sadly this isn't the case. Emacs should behave appropriately to
> accomodate this behavior.
I don't think this is quite right. As I understand it, git only takes
index.lock for operations that mutate the index [1].
It wouldn't make sense for git to hold index.lock for the entirety of
the rebase operation since you can stop in the middle of an interactive
rebase and do anything you want (mess with the index, add new commits,
anything).
I think the real question is: why is vc-find-file-hook (or
vc-refresh-state) trying to mutate the index? Shouldn't refreshing a
buffer be a read-only operation (at least with respect to the VCS)?
[1] http://git.661346.n2.nabble.com/Git-rebase-dies-with-fatal-Unable-to-create-git-index-lock-File-exists-td7596365.html
^ permalink raw reply [flat|nested] 49+ messages in thread* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2016-02-06 0:01 ` bug#21559: 25.0.50; auto-revert-mode breaks git rebase Mitchel Humpherys
@ 2016-02-06 12:34 ` Ben Gamari
2016-02-07 1:34 ` Mitchel Humpherys
0 siblings, 1 reply; 49+ messages in thread
From: Ben Gamari @ 2016-02-06 12:34 UTC (permalink / raw)
To: Mitchel Humpherys; +Cc: 21559
[-- Attachment #1: Type: text/plain, Size: 1155 bytes --]
Mitchel Humpherys <mitch.special@gmail.com> writes:
> On Fri, Sep 25 2015 at 03:45:56 PM, Ben Gamari <ben@smart-cactus.org> wrote:
>> Arguably `git rebase` should be holding the `index.lock` for the entire
>> duration of the process (or be more resilient to the lock being taken)
>> but sadly this isn't the case. Emacs should behave appropriately to
>> accomodate this behavior.
>
> I don't think this is quite right. As I understand it, git only takes
> index.lock for operations that mutate the index [1].
>
> It wouldn't make sense for git to hold index.lock for the entirety of
> the rebase operation since you can stop in the middle of an interactive
> rebase and do anything you want (mess with the index, add new commits,
> anything).
>
Ahh, yes. You are quite right.
> I think the real question is: why is vc-find-file-hook (or
> vc-refresh-state) trying to mutate the index? Shouldn't refreshing a
> buffer be a read-only operation (at least with respect to the VCS)?
>
As far as I can tell vc-find-file-hook is merely calling `git ls-files
-c -z -- $FILE`, which sounds to me like it should indeed be a read-only
operation.
Cheers,
- Ben
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2016-02-06 12:34 ` Ben Gamari
@ 2016-02-07 1:34 ` Mitchel Humpherys
2016-02-07 5:03 ` Mitchel Humpherys
0 siblings, 1 reply; 49+ messages in thread
From: Mitchel Humpherys @ 2016-02-07 1:34 UTC (permalink / raw)
To: Ben Gamari; +Cc: 21559
On Sat, Feb 06 2016 at 01:34:06 PM, Ben Gamari <ben@smart-cactus.org> wrote:
> Mitchel Humpherys <mitch.special@gmail.com> writes:
>
>> On Fri, Sep 25 2015 at 03:45:56 PM, Ben Gamari <ben@smart-cactus.org> wrote:
>>> Arguably `git rebase` should be holding the `index.lock` for the entire
>>> duration of the process (or be more resilient to the lock being taken)
>>> but sadly this isn't the case. Emacs should behave appropriately to
>>> accomodate this behavior.
>>
>> I don't think this is quite right. As I understand it, git only takes
>> index.lock for operations that mutate the index [1].
>>
>> It wouldn't make sense for git to hold index.lock for the entirety of
>> the rebase operation since you can stop in the middle of an interactive
>> rebase and do anything you want (mess with the index, add new commits,
>> anything).
>>
> Ahh, yes. You are quite right.
>
>> I think the real question is: why is vc-find-file-hook (or
>> vc-refresh-state) trying to mutate the index? Shouldn't refreshing a
>> buffer be a read-only operation (at least with respect to the VCS)?
>>
> As far as I can tell vc-find-file-hook is merely calling `git ls-files
> -c -z -- $FILE`, which sounds to me like it should indeed be a read-only
> operation.
I'm able to reliably reproduce this on the tip of the emacs-25 branch
with emacs -Q with:
$ cd /path/to/emacs
$ ./src/emacs -Q lisp/*.el
[ M-x global-auto-revert-mode ]
$ for i in $(seq 30); do for f in lisp/*.el; do echo "; $i" >> $f; done; git commit -am "test $i"; done
You might need to increase the `30' value in order to see it happen
depending on your disk speed, etc.
I took a quick look at the git source and the main source of this
particular error seems to be from the `git update-index' command. I see
that vc-git.el is calling `git update-index' in a few places but my
attempts at instrumenting the code to track down where it was coming
from were fruitless. I tried:
(setq vc-command-messages t)
as well as:
(defun vc-git-command (buffer okstatus file-or-list &rest flags)
"A wrapper around `vc-do-command' for use in vc-git.el.
The difference to vc-do-command is that this function always invokes
`vc-git-program'."
(let ((coding-system-for-read vc-git-commits-coding-system)
(coding-system-for-write vc-git-commits-coding-system))
+ (message "git: %s %s" file-or-list flags)
Any ideas on how we can trace every git command that vc-git.el is
running? I'm suspicious that we're calling `git update-index' in the
auto revert path somewhere...
^ permalink raw reply [flat|nested] 49+ messages in thread* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2016-02-07 1:34 ` Mitchel Humpherys
@ 2016-02-07 5:03 ` Mitchel Humpherys
2016-02-07 5:21 ` Mitchel Humpherys
0 siblings, 1 reply; 49+ messages in thread
From: Mitchel Humpherys @ 2016-02-07 5:03 UTC (permalink / raw)
To: Ben Gamari; +Cc: 21559
On Sat, Feb 06 2016 at 05:34:49 PM, Mitchel Humpherys <mitch.special@gmail.com> wrote:
> I took a quick look at the git source and the main source of this
> particular error seems to be from the `git update-index' command. I see
> that vc-git.el is calling `git update-index' in a few places but my
> attempts at instrumenting the code to track down where it was coming
> from were fruitless. I tried:
>
> (setq vc-command-messages t)
>
> as well as:
>
> (defun vc-git-command (buffer okstatus file-or-list &rest flags)
> "A wrapper around `vc-do-command' for use in vc-git.el.
> The difference to vc-do-command is that this function always invokes
> `vc-git-program'."
> (let ((coding-system-for-read vc-git-commits-coding-system)
> (coding-system-for-write vc-git-commits-coding-system))
> + (message "git: %s %s" file-or-list flags)
>
> Any ideas on how we can trace every git command that vc-git.el is
> running? I'm suspicious that we're calling `git update-index' in the
> auto revert path somewhere...
Sorry, I take it all back. `git update-index' is the source of that one
particular index.lock error message, but it's certainly not the only
thing holding index.lock... And actually it looks like `git ls-files'
does take index.lock:
$ cd /path/to/emacs/
$ git ls-files # in another shell after running the following inotifywait:
$ inotifywait -m .git | grep index.lock
Setting up watches.
Watches established.
.git/ CREATE index.lock
.git/ OPEN index.lock
.git/ CLOSE_WRITE,CLOSE index.lock
.git/ DELETE index.lock
.git/ CREATE index.lock
.git/ OPEN index.lock
.git/ CLOSE_WRITE,CLOSE index.lock
.git/ DELETE index.lock
I did a bit of git debugging and it looks like someone must be
registering an atexit handler or something that takes index.lock,
because it doesn't get taken in the ls-files code itself. It's taken
sometime after the `exit' function gets called in `handle_builtin' in
git.c.
Anyways, I'm more inclined now to agree that this a git bug. I don't
see why `git ls-files' would be taking index.lock... So we're back to
the question of "how can Emacs handle this more gracefully".
^ permalink raw reply [flat|nested] 49+ messages in thread* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2016-02-07 5:03 ` Mitchel Humpherys
@ 2016-02-07 5:21 ` Mitchel Humpherys
2016-02-07 10:22 ` Ben Gamari
0 siblings, 1 reply; 49+ messages in thread
From: Mitchel Humpherys @ 2016-02-07 5:21 UTC (permalink / raw)
To: Ben Gamari; +Cc: 21559
On Sat, Feb 06 2016 at 09:03:35 PM, Mitchel Humpherys <mitch.special@gmail.com> wrote:
> On Sat, Feb 06 2016 at 05:34:49 PM, Mitchel Humpherys <mitch.special@gmail.com> wrote:
>> I took a quick look at the git source and the main source of this
>> particular error seems to be from the `git update-index' command. I see
>> that vc-git.el is calling `git update-index' in a few places but my
>> attempts at instrumenting the code to track down where it was coming
>> from were fruitless. I tried:
>>
>> (setq vc-command-messages t)
>>
>> as well as:
>>
>> (defun vc-git-command (buffer okstatus file-or-list &rest flags)
>> "A wrapper around `vc-do-command' for use in vc-git.el.
>> The difference to vc-do-command is that this function always invokes
>> `vc-git-program'."
>> (let ((coding-system-for-read vc-git-commits-coding-system)
>> (coding-system-for-write vc-git-commits-coding-system))
>> + (message "git: %s %s" file-or-list flags)
>>
>> Any ideas on how we can trace every git command that vc-git.el is
>> running? I'm suspicious that we're calling `git update-index' in the
>> auto revert path somewhere...
>
> Sorry, I take it all back. `git update-index' is the source of that one
> particular index.lock error message, but it's certainly not the only
> thing holding index.lock... And actually it looks like `git ls-files'
> does take index.lock:
>
> $ cd /path/to/emacs/
> $ git ls-files # in another shell after running the following inotifywait:
> $ inotifywait -m .git | grep index.lock
> Setting up watches.
> Watches established.
> .git/ CREATE index.lock
> .git/ OPEN index.lock
> .git/ CLOSE_WRITE,CLOSE index.lock
> .git/ DELETE index.lock
> .git/ CREATE index.lock
> .git/ OPEN index.lock
> .git/ CLOSE_WRITE,CLOSE index.lock
> .git/ DELETE index.lock
>
> I did a bit of git debugging and it looks like someone must be
> registering an atexit handler or something that takes index.lock,
> because it doesn't get taken in the ls-files code itself. It's taken
> sometime after the `exit' function gets called in `handle_builtin' in
> git.c.
>
> Anyways, I'm more inclined now to agree that this a git bug. I don't
> see why `git ls-files' would be taking index.lock... So we're back to
> the question of "how can Emacs handle this more gracefully".
SORRY! I take it all back AGAIN. It was actually my shell's PS1 that
was locking index.lock (due to a `git status'), not `git ls-files'!
That's why it looked like it was happening when the program exited.
Because it really was after the program exited :). That's
embarassing...
After re-running my experiment above in a non-fancy shell I don't see
index.lock being taken at all when I run `git ls-files'.
So we're back to instrumenting vc{,-git}.el to see exactly which git
commands are being run when auto-revert refreshes a buffer.
^ permalink raw reply [flat|nested] 49+ messages in thread* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2016-02-07 5:21 ` Mitchel Humpherys
@ 2016-02-07 10:22 ` Ben Gamari
2016-02-07 10:55 ` Ben Gamari
0 siblings, 1 reply; 49+ messages in thread
From: Ben Gamari @ 2016-02-07 10:22 UTC (permalink / raw)
To: Mitchel Humpherys; +Cc: 21559
[-- Attachment #1: Type: text/plain, Size: 1342 bytes --]
Mitchel Humpherys <mitch.special@gmail.com> writes:
> So we're back to instrumenting vc{,-git}.el to see exactly which git
> commands are being run when auto-revert refreshes a buffer.
I recently revisited this and came up with this hack (after realizing
that my previous tracing approach was badly broken),
(defun my-tracing-function (orig &rest args)
(message "call-process %s" args))
(advice-add 'call-process :before #'my-tracing-function)
(message "installed")
This produces,
call-process (nil (t nil) nil ls-files -c -z -- COPYING)
call-process (nil (t nil) nil rev-parse HEAD)
call-process (nil (t nil) nil symbolic-ref HEAD)
call-process (nil (t nil) nil diff-index -p --raw -z HEAD -- COPYING)
call-process (nil (t nil) nil status --porcelain -- COPYING)
call-process (nil (t nil) nil ls-files -c -z -- COPYING)
call-process (nil (t nil) nil rev-parse HEAD)
call-process (nil (t nil) nil symbolic-ref HEAD)
call-process (nil (t nil) nil diff-index -p --raw -z HEAD -- COPYING)
call-process (nil (t nil) nil status --porcelain -- COPYING)
Quit
call-process (nil (t nil) nil ls-files -c -z -- COPYING)
After stracing each of these I've found that status --porcelain indeed
takes index.lock. Unfortunately I can't find a good explanation of why
this is necessary.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]
^ permalink raw reply [flat|nested] 49+ messages in thread* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2016-02-07 10:22 ` Ben Gamari
@ 2016-02-07 10:55 ` Ben Gamari
2016-02-07 17:06 ` Mitchel Humpherys
2016-02-08 21:19 ` Daniel Colascione
0 siblings, 2 replies; 49+ messages in thread
From: Ben Gamari @ 2016-02-07 10:55 UTC (permalink / raw)
To: Mitchel Humpherys; +Cc: 21559
[-- Attachment #1: Type: text/plain, Size: 942 bytes --]
I've spoken with some folks in #git about this issue.
* the index lock is held "because it needs to read the index and
compare it to the worktree. If it doesn't take the index lock, other
things could change either the index or the worktree underneath it,
making git status lie (or even crash)"
* It sounds as though a patch refactoring `git rebase` such that it
holds the index lock may be considered, although this is a
non-trivial refactoring as `rebase` is currently a shell script
* One option would be to check whether a rebase is in progress before
auto-reverting. In pseudo-shell,
if [ -e .git/rebase-apply ] || [ -e .git/rebase-merge ]; then
do_not_call_git_status_because_we_are_rebasing;
else
auto_revert
fi
Unfortunately it sounds like this wouldn't do the right thing when a
rebase pauses due to conflict (when you'd ideally want to
auto-revert).
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]
^ permalink raw reply [flat|nested] 49+ messages in thread* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2016-02-07 10:55 ` Ben Gamari
@ 2016-02-07 17:06 ` Mitchel Humpherys
2016-02-07 17:22 ` Ben Gamari
2016-02-08 21:19 ` Daniel Colascione
1 sibling, 1 reply; 49+ messages in thread
From: Mitchel Humpherys @ 2016-02-07 17:06 UTC (permalink / raw)
To: Ben Gamari; +Cc: 21559
On Sun, Feb 07 2016 at 11:55:07 AM, Ben Gamari <ben@smart-cactus.org> wrote:
> I've spoken with some folks in #git about this issue.
>
> * the index lock is held "because it needs to read the index and
> compare it to the worktree. If it doesn't take the index lock, other
> things could change either the index or the worktree underneath it,
> making git status lie (or even crash)"
Makes sense...
> * It sounds as though a patch refactoring `git rebase` such that it
> holds the index lock may be considered, although this is a
> non-trivial refactoring as `rebase` is currently a shell script
>
> * One option would be to check whether a rebase is in progress before
> auto-reverting. In pseudo-shell,
>
> if [ -e .git/rebase-apply ] || [ -e .git/rebase-merge ]; then
> do_not_call_git_status_because_we_are_rebasing;
> else
> auto_revert
> fi
>
> Unfortunately it sounds like this wouldn't do the right thing when a
> rebase pauses due to conflict (when you'd ideally want to
> auto-revert).
Yeah that would definitely be a problem. The other problem with this is
fix is that it wouldn't fix any other use case that locks the index,
like my repro earlier:
$ cd /path/to/emacs
$ ./src/emacs -Q lisp/*.el
[ M-x global-auto-revert-mode ]
$ for i in $(seq 30); do for f in lisp/*.el; do echo "; $i" >> $f; done; git commit -am "test $i"; done
There's no rebase there, so detecting a rebase to skip auto-revert
wouldn't help...
Hopefully we can get rid of the `git status' from the auto-revert path
altogether. It looks like the only place we do a `git status' is in
`vc-git-conflicted-files'. We call that from `vc-git-find-file-hook' to
see if we should start smerge. But I don't see how we get there from
auto-revert.
^ permalink raw reply [flat|nested] 49+ messages in thread* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2016-02-07 10:55 ` Ben Gamari
2016-02-07 17:06 ` Mitchel Humpherys
@ 2016-02-08 21:19 ` Daniel Colascione
1 sibling, 0 replies; 49+ messages in thread
From: Daniel Colascione @ 2016-02-08 21:19 UTC (permalink / raw)
To: Ben Gamari, Mitchel Humpherys; +Cc: 21559
[-- Attachment #1: Type: text/plain, Size: 1071 bytes --]
On 02/07/2016 02:55 AM, Ben Gamari wrote:
> I've spoken with some folks in #git about this issue.
>
> * the index lock is held "because it needs to read the index and
> compare it to the worktree. If it doesn't take the index lock, other
> things could change either the index or the worktree underneath it,
> making git status lie (or even crash)"
>
> * It sounds as though a patch refactoring `git rebase` such that it
> holds the index lock may be considered, although this is a
> non-trivial refactoring as `rebase` is currently a shell script
This problem is really a git bug. For any VCS, I should be able to, in
one terminal, run
while true; do $VCS status; done
and in another terminal, run
$VCS any-damn-operation
and not cause repository corruption or mysterious operation failures.
Maybe it's a good idea for Emacs to work around this particular bug in
git, but there is nothing semantically wrong with what vc-git is doing here.
FWIW, whatever its other faults, hg at least operates correctly here.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2015-09-25 12:45 bug#21559: 25.0.50; auto-revert-mode breaks git rebase Ben Gamari
` (2 preceding siblings ...)
2016-02-06 0:01 ` bug#21559: 25.0.50; auto-revert-mode breaks git rebase Mitchel Humpherys
@ 2016-09-09 20:56 ` Jason Merrill
2018-02-14 10:08 ` Alexei Khlebnikov
` (2 subsequent siblings)
6 siblings, 0 replies; 49+ messages in thread
From: Jason Merrill @ 2016-09-09 20:56 UTC (permalink / raw)
To: 21559
[-- Attachment #1: Type: text/plain, Size: 1186 bytes --]
The problematic invocation of git status comes from
(defun vc-git-conflicted-files (directory)
"Return the list of files with conflicts in DIRECTORY."
(let* ((status
(vc-git--run-command-string directory "status" "--porcelain"
"--"))
I'm working around this issue by changing vc-git-conflicted-files to use
diff-files --name-status, which doesn't lock the index:
(defun vc-git-conflicted-files (directory)
"Return the list of files with conflicts in DIRECTORY."
(let* ((status
(vc-git--run-command-string directory "diff-files"
"--name-status"))
(lines (when status (split-string status "\n" 'omit-nulls)))
files)
;; TODO: Look into reimplementing `vc-git-state', as well as
;; `vc-git-dir-status-files', based on this output, thus making the
;; extra process call in `vc-git-find-file-hook' unnecessary.
(dolist (line lines files)
(when (string-match "\\([ MADRCU?!]\\)[ \t]+\\(.+\\)" line)
(let ((state (match-string 1 line))
(file (match-string 2 line)))
;; See git-status(1).
(when (equal state "U")
(push (expand-file-name file directory) files)))))))
[-- Attachment #2: Type: text/html, Size: 1999 bytes --]
^ permalink raw reply [flat|nested] 49+ messages in thread* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2015-09-25 12:45 bug#21559: 25.0.50; auto-revert-mode breaks git rebase Ben Gamari
` (3 preceding siblings ...)
2016-09-09 20:56 ` Jason Merrill
@ 2018-02-14 10:08 ` Alexei Khlebnikov
2018-02-15 19:08 ` Alexei Khlebnikov
2018-02-19 10:24 ` bug#21559: PATCH review needed: lisp/vc/vc-git.el (vc-git-state, vc-git-conflicted-files) Alexei Khlebnikov
2018-09-25 12:23 ` bug#21559: additional patch needed to set GIT_OPTIONAL_LOCKS=0 in all cases Andrew Ruder
6 siblings, 1 reply; 49+ messages in thread
From: Alexei Khlebnikov @ 2018-02-14 10:08 UTC (permalink / raw)
To: 21559
[-- Attachment #1: Type: text/plain, Size: 1256 bytes --]
Since some version, Git supports "--no-optional-locks" switch and
"GIT_OPTIONAL_LOCKS" environment variable for avoiding locking during "git
status".
That's how they are documented:
----------------------------------------
https://git-scm.com/docs/git#git---no-optional-locks
--no-optional-locks
Do not perform optional operations that require locks. This is equivalent
to setting the GIT_OPTIONAL_LOCKS to 0.
https://git-scm.com/docs/git-status#_background_refresh
BACKGROUND REFRESH
By default, git status will automatically refresh the index, updating the
cached stat information from the working tree and writing out the result.
Writing out the updated index is an optimization that isn’t strictly
necessary (status computes the values for itself, but writing them out is
just to save subsequent programs from repeating our computation). When
status is run in the background, the lock held during the write may
conflict with other simultaneous processes, causing them to fail. Scripts
running status in the background should consider using git
--no-optional-locks status (see git[1] for details).
----------------------------------------
Using this "--no-optional-locks" switch looks like a solution to the issue?
[-- Attachment #2: Type: text/html, Size: 1745 bytes --]
^ permalink raw reply [flat|nested] 49+ messages in thread* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2018-02-14 10:08 ` Alexei Khlebnikov
@ 2018-02-15 19:08 ` Alexei Khlebnikov
2018-02-15 22:32 ` Alexei Khlebnikov
2018-02-19 23:41 ` Dmitry Gutov
0 siblings, 2 replies; 49+ messages in thread
From: Alexei Khlebnikov @ 2018-02-15 19:08 UTC (permalink / raw)
To: 21559
[-- Attachment #1: Type: text/plain, Size: 374 bytes --]
Judging from the comment of the commit implementing the
"--no-optional-locks" switch,
https://github.com/git/git/commit/27344d6a6c8056664966e11acf674e5da6dd7ee3
, the switch was implemented exactly for background refresh in "tools like
IDEs or fancy editors".
I.e. for mitigating this particular bug! Now we only have to use this
switch in our "fancy editor".
[-- Attachment #2: Type: text/html, Size: 551 bytes --]
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2018-02-15 19:08 ` Alexei Khlebnikov
@ 2018-02-15 22:32 ` Alexei Khlebnikov
2018-02-19 23:41 ` Dmitry Gutov
1 sibling, 0 replies; 49+ messages in thread
From: Alexei Khlebnikov @ 2018-02-15 22:32 UTC (permalink / raw)
To: 21559
[-- Attachment #1.1: Type: text/plain, Size: 1148 bytes --]
I am sending patch that fixes the issue. It fixes the issue for me on
Emacs' current master branch. The patch also cleanly applies to emacs-26
branch.
I tested using Mitchel Humpherys' repro method, i.e.
$ cd /path/to/emacs
$ ./src/emacs -Q lisp/*.el
[ M-x global-auto-revert-mode ]
$ for i in $(seq 30); do for f in lisp/*.el; do echo "; $i" >> $f;
done; git commit -am "test $i"; done
Without the fix I could reproduce the issue, and with the fix I could not
reproduce even if running the loop 100 times instead of 30. Emacs was
reloading a lot of files very fast, but no locking errors happened in the
shell which ran the loop.
2018-02-15 20:08 GMT+01:00 Alexei Khlebnikov <alexei.khlebnikov@gmail.com>:
> Judging from the comment of the commit implementing the
> "--no-optional-locks" switch,
>
> https://github.com/git/git/commit/27344d6a6c8056664966e11acf674e5da6dd7ee3
>
> , the switch was implemented exactly for background refresh in "tools like
> IDEs or fancy editors".
> I.e. for mitigating this particular bug! Now we only have to use this
> switch in our "fancy editor".
>
>
[-- Attachment #1.2: Type: text/html, Size: 1728 bytes --]
[-- Attachment #2: 0001-Fix-for-25.0.50-auto-revert-mode-breaks-git-rebase-B.patch --]
[-- Type: text/x-patch, Size: 2310 bytes --]
From 4bba51ded42954a62c99696453c17e7fa3f2b730 Mon Sep 17 00:00:00 2001
From: Alexei Khlebnikov <alexei.khlebnikov@gmail.com>
Date: Thu, 15 Feb 2018 21:18:01 +0100
Subject: [PATCH] Fix for: "25.0.50; auto-revert-mode breaks git rebase"
(Bug#21559)
* lisp/vc/vc-git.el (vc-git-state, vc-git-conflicted-files)
Use "--no-optional-locks" switch when doing "git status".
---
lisp/vc/vc-git.el | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 47172dd52f..06db43c311 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -292,13 +292,16 @@ vc-git-state
;; upstream. We'd need to check against the upstream tracking
;; branch for that (an extra process call or two).
(let* ((args
- `("status" "--porcelain" "-z"
+ `(;; Avoid locking repository during "git status" (bug#21559).
+ ,@(when (version<= "2.15.0" (vc-git--program-version))
+ '("--no-optional-locks"))
+ "status" "--porcelain" "-z"
;; Just to be explicit, it's the default anyway.
"--untracked-files"
,@(when (version<= "1.7.6.3" (vc-git--program-version))
'("--ignored"))
"--"))
- (status (apply #'vc-git--run-command-string file args)))
+ (status (apply #'vc-git--run-command-string file args)))
;; Alternatively, the `ignored' state could be detected with 'git
;; ls-files -i -o --exclude-standard', but that's an extra process
;; call, and the `ignored' state is rarely needed.
@@ -939,8 +942,12 @@ vc-git-merge-branch
(defun vc-git-conflicted-files (directory)
"Return the list of files with conflicts in DIRECTORY."
- (let* ((status
- (vc-git--run-command-string directory "status" "--porcelain" "--"))
+ (let* ((args
+ `(;; Avoid locking repository during "git status" (bug#21559).
+ ,@(when (version<= "2.15.0" (vc-git--program-version))
+ '("--no-optional-locks"))
+ "status" "--porcelain" "--"))
+ (status (apply #'vc-git--run-command-string directory args))
(lines (when status (split-string status "\n" 'omit-nulls)))
files)
;; TODO: Look into reimplementing `vc-git-state', as well as
--
2.15.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2018-02-15 19:08 ` Alexei Khlebnikov
2018-02-15 22:32 ` Alexei Khlebnikov
@ 2018-02-19 23:41 ` Dmitry Gutov
2018-02-20 0:06 ` Alexei Khlebnikov
2018-02-20 7:40 ` Michael Albinus
1 sibling, 2 replies; 49+ messages in thread
From: Dmitry Gutov @ 2018-02-19 23:41 UTC (permalink / raw)
To: Alexei Khlebnikov, 21559
On 2/15/18 9:08 PM, Alexei Khlebnikov wrote:
> Judging from the comment of the commit implementing the
> "--no-optional-locks" switch,
>
> https://github.com/git/git/commit/27344d6a6c8056664966e11acf674e5da6dd7ee3
>
> , the switch was implemented exactly for background refresh in "tools
> like IDEs or fancy editors".
> I.e. for mitigating this particular bug! Now we only have to use this
> switch in our "fancy editor".
OK, here's my question: what is a "background refresh"? Must be consider
every VC operation to be "background"?
From what I see of this switch's description, it's going to (slightly?
imperceptibly?) slow down the VC operations. It would be cool to see
some measurement of that effect.
Failing that, why don't we try something else first? If the problem
occurs solely due to auto-revert-mode's calls to Git, let's try adding
that environment variable binding inside auto-revert-handler. The patch
is below. Does it solve the problem as well?
diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index cf145e0ee3..41e9f00049 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -709,7 +709,9 @@ auto-revert-handler
;; `preserve-modes' avoids changing the (minor) modes. But we do
;; want to reset the mode for VC, so we do it manually.
(when (or revert auto-revert-check-vc-info)
- (vc-refresh-state))))
+ (let ((process-environment
+ (cons "GIT_OPTIONAL_LOCKS=0" process-environment)))
+ (vc-refresh-state)))))
(defun auto-revert-tail-handler (size)
(let ((modified (buffer-modified-p))
^ permalink raw reply related [flat|nested] 49+ messages in thread* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2018-02-19 23:41 ` Dmitry Gutov
@ 2018-02-20 0:06 ` Alexei Khlebnikov
2018-02-20 0:17 ` Dmitry Gutov
2018-02-20 7:40 ` Michael Albinus
1 sibling, 1 reply; 49+ messages in thread
From: Alexei Khlebnikov @ 2018-02-20 0:06 UTC (permalink / raw)
To: 21559; +Cc: Michael Albinus, Dmitry Gutov
[-- Attachment #1: Type: text/plain, Size: 382 bytes --]
2018-02-20 0:41 GMT+01:00 Dmitry Gutov <dgutov@yandex.ru>:
> Failing that, why don't we try something else first? If the problem occurs
> solely due to auto-revert-mode's calls to Git, let's try adding that
> environment variable binding inside auto-revert-handler. The patch is
> below. Does it solve the problem as well?
>
I've just tested - yes, it solves the problem as well!
[-- Attachment #2: Type: text/html, Size: 686 bytes --]
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2018-02-20 0:06 ` Alexei Khlebnikov
@ 2018-02-20 0:17 ` Dmitry Gutov
2018-02-20 4:07 ` Eli Zaretskii
0 siblings, 1 reply; 49+ messages in thread
From: Dmitry Gutov @ 2018-02-20 0:17 UTC (permalink / raw)
To: Alexei Khlebnikov, 21559; +Cc: Michael Albinus
On 2/20/18 2:06 AM, Alexei Khlebnikov wrote:
> 2018-02-20 0:41 GMT+01:00 Dmitry Gutov <dgutov@yandex.ru
> <mailto:dgutov@yandex.ru>>:
>
> Failing that, why don't we try something else first? If the problem
> occurs solely due to auto-revert-mode's calls to Git, let's try
> adding that environment variable binding inside auto-revert-handler.
> The patch is below. Does it solve the problem as well?
>
>
> I've just tested - yes, it solves the problem as well!
Terrific, thank you.
What do others think? Michael, Eli?
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2018-02-20 0:17 ` Dmitry Gutov
@ 2018-02-20 4:07 ` Eli Zaretskii
0 siblings, 0 replies; 49+ messages in thread
From: Eli Zaretskii @ 2018-02-20 4:07 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 21559, alexei.khlebnikov, michael.albinus
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 20 Feb 2018 02:17:51 +0200
> Cc: Michael Albinus <michael.albinus@gmx.de>
>
> On 2/20/18 2:06 AM, Alexei Khlebnikov wrote:
> > 2018-02-20 0:41 GMT+01:00 Dmitry Gutov <dgutov@yandex.ru
> > <mailto:dgutov@yandex.ru>>:
> >
> > Failing that, why don't we try something else first? If the problem
> > occurs solely due to auto-revert-mode's calls to Git, let's try
> > adding that environment variable binding inside auto-revert-handler.
> > The patch is below. Does it solve the problem as well?
> >
> >
> > I've just tested - yes, it solves the problem as well!
>
> Terrific, thank you.
>
> What do others think? Michael, Eli?
Fine with me, but please add a comment there explaining why we do
that, as the relation between auto-revert and Git is entirely not
obvious.
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2018-02-19 23:41 ` Dmitry Gutov
2018-02-20 0:06 ` Alexei Khlebnikov
@ 2018-02-20 7:40 ` Michael Albinus
2018-02-20 11:37 ` Dmitry Gutov
1 sibling, 1 reply; 49+ messages in thread
From: Michael Albinus @ 2018-02-20 7:40 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 21559, Alexei Khlebnikov
Dmitry Gutov <dgutov@yandex.ru> writes:
Hi Dmitry,
> From what I see of this switch's description, it's going to (slightly?
> imperceptibly?) slow down the VC operations. It would be cool to see
> some measurement of that effect.
>
> Failing that, why don't we try something else first? If the problem
> occurs solely due to auto-revert-mode's calls to Git, let's try adding
> that environment variable binding inside auto-revert-handler. The
> patch is below. Does it solve the problem as well?
I'm not so happy seeing git specific code in autorevert.el (yes, I know,
it's just an environment variable). All git specifics shall be kept in
vc-git.el. Otherwise, we would go into a dependency nightmare, soon. Do
you know, that magit would like to see this setting?
If this variable hurts for standard operation of vc-git, we could set it
depending whether (auto)revert is running, or not. There is the variable
`revert-buffer-in-progress-p', which is non-nil if reverting is in
progress. I could set it also before calling `vc-refresh-state' in
`auto-revert-handler'. And in vc-git.el, `process-environment' could be
extended or not, depending on the value of `revert-buffer-in-progress-p'.
Best regards, Michael.
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2018-02-20 7:40 ` Michael Albinus
@ 2018-02-20 11:37 ` Dmitry Gutov
2018-02-20 11:53 ` Michael Albinus
0 siblings, 1 reply; 49+ messages in thread
From: Dmitry Gutov @ 2018-02-20 11:37 UTC (permalink / raw)
To: Michael Albinus; +Cc: 21559, Alexei Khlebnikov
On 2/20/18 9:40 AM, Michael Albinus wrote:
> I'm not so happy seeing git specific code in autorevert.el (yes, I know,
> it's just an environment variable). All git specifics shall be kept in
> vc-git.el. Otherwise, we would go into a dependency nightmare, soon.
But this way the patch is much smaller, isn't it? That has
maintainability benefits too.
And using a variable would make more sense if we determine that other
facilities, not just autorevert, make VC calls that we want to consider
"background".
> Do
> you know, that magit would like to see this setting?
I imagine it would, if it makes Git calls during autorevert (and if
doesn't, the var won't do anything). But we could let it make that
choice by itself, of course.
> If this variable hurts for standard operation of vc-git, we could set it
> depending whether (auto)revert is running, or not. There is the variable
> `revert-buffer-in-progress-p', which is non-nil if reverting is in
> progress. I could set it also before calling `vc-refresh-state' in
> `auto-revert-handler'. And in vc-git.el, `process-environment' could be
> extended or not, depending on the value of `revert-buffer-in-progress-p'.
In which part of vc-git.el? Changing the implementation of two commands,
like the original patch proposed, makes for a bigger change.
We could do that in vc-git-command, I suppose...
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2018-02-20 11:37 ` Dmitry Gutov
@ 2018-02-20 11:53 ` Michael Albinus
2018-02-20 22:28 ` Dmitry Gutov
0 siblings, 1 reply; 49+ messages in thread
From: Michael Albinus @ 2018-02-20 11:53 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 21559, Alexei Khlebnikov
Dmitry Gutov <dgutov@yandex.ru> writes:
Hi Dmitry,
>> I'm not so happy seeing git specific code in autorevert.el (yes, I know,
>> it's just an environment variable). All git specifics shall be kept in
>> vc-git.el. Otherwise, we would go into a dependency nightmare, soon.
>
> But this way the patch is much smaller, isn't it? That has
> maintainability benefits too.
A dependency nightmare counts much more, IMHO.
> And using a variable would make more sense if we determine that other
> facilities, not just autorevert, make VC calls that we want to
> consider "background".
autorevert wouldn't care who uses this variable. In a broader sense, it
could let-bind it for the whole auto-revert-handler body.
>> If this variable hurts for standard operation of vc-git, we could set it
>> depending whether (auto)revert is running, or not. There is the variable
>> `revert-buffer-in-progress-p', which is non-nil if reverting is in
>> progress. I could set it also before calling `vc-refresh-state' in
>> `auto-revert-handler'. And in vc-git.el, `process-environment' could be
>> extended or not, depending on the value of `revert-buffer-in-progress-p'.
>
> In which part of vc-git.el? Changing the implementation of two
> commands, like the original patch proposed, makes for a bigger change.
>
> We could do that in vc-git-command, I suppose...
Perhaps. autorevert shouldn't know anything about vc-git, it should just
let-bind the variable, and let other packages decide whether they use
it.
FWIW, I'm also not so enthusiastic, that aut-revert-handler calls
vc-refresh-state directly. This would be better organized by a hook.
Best regards, Michael.
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2018-02-20 11:53 ` Michael Albinus
@ 2018-02-20 22:28 ` Dmitry Gutov
2018-02-21 22:07 ` Alexei Khlebnikov
0 siblings, 1 reply; 49+ messages in thread
From: Dmitry Gutov @ 2018-02-20 22:28 UTC (permalink / raw)
To: Michael Albinus; +Cc: 21559, Alexei Khlebnikov
On 2/20/18 1:53 PM, Michael Albinus wrote:
> A dependency nightmare counts much more, IMHO.
A nightmare isn't going to arrive overnight.
>> And using a variable would make more sense if we determine that other
>> facilities, not just autorevert, make VC calls that we want to
>> consider "background".
>
> autorevert wouldn't care who uses this variable. In a broader sense, it
> could let-bind it for the whole auto-revert-handler body.
I meant that if there's going to be more places that are going to *bind*
this variable. In that case, moving the relevant code into vc/* would be
unavoidable.
>> We could do that in vc-git-command, I suppose...
>
> Perhaps. autorevert shouldn't know anything about vc-git, it should just
> let-bind the variable, and let other packages decide whether they use
> it.
All right, so you just want to move the responsibility.
> FWIW, I'm also not so enthusiastic, that aut-revert-handler calls
> vc-refresh-state directly. This would be better organized by a hook.
On the other hand, since it already has this direct call, an extra let
binding isn't going to change much.
Anyway, it's your choice here. Do you want to show an alternative patch?
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2018-02-20 22:28 ` Dmitry Gutov
@ 2018-02-21 22:07 ` Alexei Khlebnikov
2018-02-22 11:24 ` Michael Albinus
0 siblings, 1 reply; 49+ messages in thread
From: Alexei Khlebnikov @ 2018-02-21 22:07 UTC (permalink / raw)
To: 21559; +Cc: Michael Albinus, Dmitry Gutov
[-- Attachment #1.1: Type: text/plain, Size: 231 bytes --]
Here is version 4 of the patch.
"revert-buffer-in-progress-p" flag is set in "auto-revert-handler",
according to Michael's idea and then checked in "vc-git-command", according
to Dmitry's idea.
I have tested the patch, it works.
[-- Attachment #1.2: Type: text/html, Size: 323 bytes --]
[-- Attachment #2: v4-0001-Fix-for-25.0.50-auto-revert-mode-breaks-git-rebas.patch --]
[-- Type: text/x-patch, Size: 2062 bytes --]
From d6cffe9f7cb0c7827eb47b13b922291418ea8ebf Mon Sep 17 00:00:00 2001
From: Alexei Khlebnikov <alexei.khlebnikov@gmail.com>
Date: Wed, 21 Feb 2018 22:34:22 +0100
Subject: [PATCH v4] Fix for: "25.0.50; auto-revert-mode breaks git rebase"
(Bug#21559)
* lisp/autorevert.el (auto-revert-handler):
Set "revert-buffer-in-progress-p" flag
before calling "vc-refresh-state".
* lisp/vc/vc-git.el (vc-git-command):
If "revert-buffer-in-progress-p" flag is set,
prepend "GIT_OPTIONAL_LOCKS=0" to "process-environment".
---
lisp/autorevert.el | 3 ++-
lisp/vc/vc-git.el | 9 ++++++++-
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index cf145e0ee3..0a9d3bef54 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -709,7 +709,8 @@ auto-revert-handler
;; `preserve-modes' avoids changing the (minor) modes. But we do
;; want to reset the mode for VC, so we do it manually.
(when (or revert auto-revert-check-vc-info)
- (vc-refresh-state))))
+ (let ((revert-buffer-in-progress-p t))
+ (vc-refresh-state)))))
(defun auto-revert-tail-handler (size)
(let ((modified (buffer-modified-p))
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 47172dd52f..b1a57ab6f4 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1566,7 +1566,14 @@ vc-git-command
(or coding-system-for-read vc-git-log-output-coding-system))
(coding-system-for-write
(or coding-system-for-write vc-git-commits-coding-system))
- (process-environment (cons "GIT_DIR" process-environment)))
+ (process-environment
+ (append
+ `("GIT_DIR"
+ ;; Avoid repository locking during background operations
+ ;; (bug#21559).
+ ,@(when revert-buffer-in-progress-p
+ '("GIT_OPTIONAL_LOCKS=0")))
+ process-environment)))
(apply 'vc-do-command (or buffer "*vc*") okstatus vc-git-program
;; https://debbugs.gnu.org/16897
(unless (and (not (cdr-safe file-or-list))
--
2.15.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2018-02-21 22:07 ` Alexei Khlebnikov
@ 2018-02-22 11:24 ` Michael Albinus
2018-02-22 11:45 ` Dmitry Gutov
0 siblings, 1 reply; 49+ messages in thread
From: Michael Albinus @ 2018-02-22 11:24 UTC (permalink / raw)
To: Alexei Khlebnikov; +Cc: 21559-done, Dmitry Gutov
Version: 27.1
Alexei Khlebnikov <alexei.khlebnikov@gmail.com> writes:
Hi Alexei,
> Here is version 4 of the patch.
>
> "revert-buffer-in-progress-p" flag is set in "auto-revert-handler",
> according to Michael's idea and then checked in "vc-git-command",
> according to Dmitry's idea.
LGTM, I've pushed this to master.
> I have tested the patch, it works.
Thanks. I'm closing the bug.
Best regards, Michael.
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: 25.0.50; auto-revert-mode breaks git rebase
2018-02-22 11:24 ` Michael Albinus
@ 2018-02-22 11:45 ` Dmitry Gutov
0 siblings, 0 replies; 49+ messages in thread
From: Dmitry Gutov @ 2018-02-22 11:45 UTC (permalink / raw)
To: Michael Albinus, Alexei Khlebnikov; +Cc: 21559-done
On 2/22/18 1:24 PM, Michael Albinus wrote:
>> Here is version 4 of the patch.
>>
>> "revert-buffer-in-progress-p" flag is set in "auto-revert-handler",
>> according to Michael's idea and then checked in "vc-git-command",
>> according to Dmitry's idea.
>
> LGTM, I've pushed this to master.
Great, thank you both.
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: PATCH review needed: lisp/vc/vc-git.el (vc-git-state, vc-git-conflicted-files)
2015-09-25 12:45 bug#21559: 25.0.50; auto-revert-mode breaks git rebase Ben Gamari
` (4 preceding siblings ...)
2018-02-14 10:08 ` Alexei Khlebnikov
@ 2018-02-19 10:24 ` Alexei Khlebnikov
2018-02-19 12:39 ` Michael Albinus
2018-02-19 15:29 ` Eli Zaretskii
2018-09-25 12:23 ` bug#21559: additional patch needed to set GIT_OPTIONAL_LOCKS=0 in all cases Andrew Ruder
6 siblings, 2 replies; 49+ messages in thread
From: Alexei Khlebnikov @ 2018-02-19 10:24 UTC (permalink / raw)
To: 21559, Dmitry Gutov, Eli Zaretskii, Michael Albinus
[-- Attachment #1: Type: text/plain, Size: 542 bytes --]
Hi All,
How can I get my patch reviewed?
The patch is sent in the previous message, several days ago:
https://debbugs.gnu.org/cgi/bugreport.cgi?msg=83;bug=21559;att=2;filename=0001-Fix-for-25.0.50-auto-revert-mode-breaks-git-rebase-B.patch
The same patch on GitHub:
https://github.com/alexeikh/emacs/commit/4bba51ded42954a62c99696453c17e7fa3f2b730
Adding to the discussion:
* Dmitry Gutov, because he is the maintainer for lisp/vc/* .
* Eli Zaretskii and Michael Albinus, because they participated in the
discussion on this bug before.
[-- Attachment #2: Type: text/html, Size: 917 bytes --]
^ permalink raw reply [flat|nested] 49+ messages in thread* bug#21559: PATCH review needed: lisp/vc/vc-git.el (vc-git-state, vc-git-conflicted-files)
2018-02-19 10:24 ` bug#21559: PATCH review needed: lisp/vc/vc-git.el (vc-git-state, vc-git-conflicted-files) Alexei Khlebnikov
@ 2018-02-19 12:39 ` Michael Albinus
2018-02-19 15:29 ` Eli Zaretskii
1 sibling, 0 replies; 49+ messages in thread
From: Michael Albinus @ 2018-02-19 12:39 UTC (permalink / raw)
To: Alexei Khlebnikov; +Cc: 21559, Dmitry Gutov
Alexei Khlebnikov <alexei.khlebnikov@gmail.com> writes:
> Hi All,
Hi Alexei,
> https://debbugs.gnu.org/cgi/bugreport.cgi?msg=83;bug=21559;att=2;filename=0001-Fix-for-25.0.50-auto-revert-mode-breaks-git-rebase-B.patch
I don't know git in detail, so I don't know whether this argument is
useful.
However, I don't believe this argument shall be added, depending on git
version number. A better approach would be to prepend
"GIT_OPTIONAL_LOCKS=0" in `process-environment'. This would work for any
git version, w/o further check. For git version <= 2.15.0, it would
simply do nothing.
Best regards, Michael.
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: PATCH review needed: lisp/vc/vc-git.el (vc-git-state, vc-git-conflicted-files)
2018-02-19 10:24 ` bug#21559: PATCH review needed: lisp/vc/vc-git.el (vc-git-state, vc-git-conflicted-files) Alexei Khlebnikov
2018-02-19 12:39 ` Michael Albinus
@ 2018-02-19 15:29 ` Eli Zaretskii
2018-02-19 18:39 ` Alexei Khlebnikov
1 sibling, 1 reply; 49+ messages in thread
From: Eli Zaretskii @ 2018-02-19 15:29 UTC (permalink / raw)
To: Alexei Khlebnikov; +Cc: 21559, michael.albinus, dgutov
> From: Alexei Khlebnikov <alexei.khlebnikov@gmail.com>
> Date: Mon, 19 Feb 2018 11:24:57 +0100
>
> How can I get my patch reviewed?
I already reviewed it. I was waiting for a few days to give others a
chance to comment.
> Adding to the discussion:
> * Dmitry Gutov, because he is the maintainer for lisp/vc/* .
> * Eli Zaretskii and Michael Albinus, because they participated in the discussion on this bug before.
Thanks, but there's no need to add me: I read all the messages sent to
the bug list.
I think Michael's suggestion makes a lot of sense, btw.
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: PATCH review needed: lisp/vc/vc-git.el (vc-git-state, vc-git-conflicted-files)
2018-02-19 15:29 ` Eli Zaretskii
@ 2018-02-19 18:39 ` Alexei Khlebnikov
2018-02-19 18:50 ` Michael Albinus
0 siblings, 1 reply; 49+ messages in thread
From: Alexei Khlebnikov @ 2018-02-19 18:39 UTC (permalink / raw)
To: 21559; +Cc: Michael Albinus, Dmitry Gutov
[-- Attachment #1: Type: text/plain, Size: 647 bytes --]
Eli, Michael, thanks for the review.
> A better approach would be to prepend "GIT_OPTIONAL_LOCKS=0" in `process-environment'.
Thanks for the suggestion, I am attaching version 2 of the patch which
implements this suggestion.
> > Adding to the discussion:
> > * Dmitry Gutov, because he is the maintainer for lisp/vc/* .
> > * Eli Zaretskii and Michael Albinus, because they participated in the discussion on this bug before.
>
> Thanks, but there's no need to add me: I read all the messages sent to
> the bug list.
OK, I've removed you from the addressees. I'm not sure if I should do
the same for others, so I left Dmitry and Michael in CC.
[-- Attachment #2: v2-0001-Fix-for-25.0.50-auto-revert-mode-breaks-git-rebas.patch --]
[-- Type: text/x-patch, Size: 2151 bytes --]
From 5265d04f908cdcab12b24402221b18d83fab1f43 Mon Sep 17 00:00:00 2001
From: Alexei Khlebnikov <alexei.khlebnikov@gmail.com>
Date: Mon, 19 Feb 2018 19:07:58 +0100
Subject: [PATCH v2] Fix for: "25.0.50; auto-revert-mode breaks git rebase"
(Bug#21559)
* lisp/vc/vc-git.el (vc-git-state, vc-git-conflicted-files):
Prepend "GIT_OPTIONAL_LOCKS=0" to process-environment
when doing "git status".
---
lisp/vc/vc-git.el | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 47172dd52f..35cb7defca 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -291,14 +291,17 @@ vc-git-state
;; corresponding upstream branch, and the file was modified
;; upstream. We'd need to check against the upstream tracking
;; branch for that (an extra process call or two).
- (let* ((args
+ (let* (;; Avoid repository locking during "git status" (bug#21559).
+ (process-environment
+ (cons "GIT_OPTIONAL_LOCKS=0" process-environment))
+ (args
`("status" "--porcelain" "-z"
;; Just to be explicit, it's the default anyway.
"--untracked-files"
,@(when (version<= "1.7.6.3" (vc-git--program-version))
'("--ignored"))
"--"))
- (status (apply #'vc-git--run-command-string file args)))
+ (status (apply #'vc-git--run-command-string file args)))
;; Alternatively, the `ignored' state could be detected with 'git
;; ls-files -i -o --exclude-standard', but that's an extra process
;; call, and the `ignored' state is rarely needed.
@@ -939,7 +942,10 @@ vc-git-merge-branch
(defun vc-git-conflicted-files (directory)
"Return the list of files with conflicts in DIRECTORY."
- (let* ((status
+ (let* (;; Avoid repository locking during "git status" (bug#21559).
+ (process-environment
+ (cons "GIT_OPTIONAL_LOCKS=0" process-environment))
+ (status
(vc-git--run-command-string directory "status" "--porcelain" "--"))
(lines (when status (split-string status "\n" 'omit-nulls)))
files)
--
2.15.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* bug#21559: PATCH review needed: lisp/vc/vc-git.el (vc-git-state, vc-git-conflicted-files)
2018-02-19 18:39 ` Alexei Khlebnikov
@ 2018-02-19 18:50 ` Michael Albinus
2018-02-19 19:05 ` Alexei Khlebnikov
2018-02-19 23:35 ` Dmitry Gutov
0 siblings, 2 replies; 49+ messages in thread
From: Michael Albinus @ 2018-02-19 18:50 UTC (permalink / raw)
To: Alexei Khlebnikov; +Cc: 21559, Dmitry Gutov
Alexei Khlebnikov <alexei.khlebnikov@gmail.com> writes:
> Eli, Michael, thanks for the review.
Hi Alexei,
>> A better approach would be to prepend "GIT_OPTIONAL_LOCKS=0" in
>> `process-environment'.
>
> Thanks for the suggestion, I am attaching version 2 of the patch which
> implements this suggestion.
Looks OK to me.
>> Thanks, but there's no need to add me: I read all the messages sent to
>> the bug list.
>
> OK, I've removed you from the addressees. I'm not sure if I should do
> the same for others, so I left Dmitry and Michael in CC.
I don't care. I try to react on relevant messages in the respective
mailing lists, but it could happen that I oversee something.
Unlike Eli, I don't read *every* message. The subject must tell me that
I shall do.
Best regards, Michael.
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: PATCH review needed: lisp/vc/vc-git.el (vc-git-state, vc-git-conflicted-files)
2018-02-19 18:50 ` Michael Albinus
@ 2018-02-19 19:05 ` Alexei Khlebnikov
2018-02-19 23:35 ` Dmitry Gutov
1 sibling, 0 replies; 49+ messages in thread
From: Alexei Khlebnikov @ 2018-02-19 19:05 UTC (permalink / raw)
To: Michael Albinus; +Cc: 21559, Dmitry Gutov
2018-02-19 19:50 GMT+01:00 Michael Albinus <michael.albinus@gmx.de>:
> Looks OK to me.
Cool, so quick answer, thanks! :)
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: PATCH review needed: lisp/vc/vc-git.el (vc-git-state, vc-git-conflicted-files)
2018-02-19 18:50 ` Michael Albinus
2018-02-19 19:05 ` Alexei Khlebnikov
@ 2018-02-19 23:35 ` Dmitry Gutov
1 sibling, 0 replies; 49+ messages in thread
From: Dmitry Gutov @ 2018-02-19 23:35 UTC (permalink / raw)
To: Michael Albinus, Alexei Khlebnikov; +Cc: 21559
On 2/19/18 8:50 PM, Michael Albinus wrote:
> I try to react on relevant messages in the respective
> mailing lists, but it could happen that I oversee something.
Same here, except I slightly prefer to be in Cc. That makes the thread a
bit easier to notice.
^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#21559: additional patch needed to set GIT_OPTIONAL_LOCKS=0 in all cases
2015-09-25 12:45 bug#21559: 25.0.50; auto-revert-mode breaks git rebase Ben Gamari
` (5 preceding siblings ...)
2018-02-19 10:24 ` bug#21559: PATCH review needed: lisp/vc/vc-git.el (vc-git-state, vc-git-conflicted-files) Alexei Khlebnikov
@ 2018-09-25 12:23 ` Andrew Ruder
2018-09-26 3:16 ` Phil Sainty
6 siblings, 1 reply; 49+ messages in thread
From: Andrew Ruder @ 2018-09-25 12:23 UTC (permalink / raw)
To: 21559; +Cc: alexei.khlebnikov, michael.albinus, dgutov
[-- Attachment #1: Type: text/plain, Size: 463 bytes --]
Please see attached patch. There is also some early reports of
success at https://github.com/magit/magit/issues/2708
In short, I believe the analysis for this bug was thorough (and
correct!), but it turns out that the call to `git status` doesn't
actually go through the patched function so we'd end up with a few
commands getting GIT_OPTIONAL_LOCKS=0 but not the most important one.
- Andrew Ruder
P.S. still figuring out the contribution process for emacs.
[-- Attachment #2: 0001-Fix-for-25.0.50-auto-revert-mode-breaks-git-rebase-B.patch --]
[-- Type: text/plain, Size: 1456 bytes --]
From b2a4c67cdd6fe834317416120b96ab943d6a1f7a Mon Sep 17 00:00:00 2001
From: Andrew Ruder <andy@aeruder.net>
Date: Mon, 24 Sep 2018 21:29:00 -0700
Subject: [PATCH] Fix for: "25.0.50; auto-revert-mode breaks git rebase"
(Bug#21559)
The fix for Bug#21559 is mostly right, but the one command we were
trying to protect, git status --porcelain, doesn't go through
vc-git-command and wasn't actually getting the GIT_OPTIONAL_LOCKS=0
treatment.
* lisp/vc/vc-git.el (vc-git--call): Override GIT_OPTIONAL_LOCKS=0
during revert-buffer-in-progress-p
---
lisp/vc/vc-git.el | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index ca457fb3d1..cc137b2304 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1628,8 +1628,15 @@ vc-git--call
(or coding-system-for-read vc-git-log-output-coding-system))
(coding-system-for-write
(or coding-system-for-write vc-git-commits-coding-system))
- (process-environment (cons "PAGER=" process-environment)))
- (push "GIT_DIR" process-environment)
+ (process-environment
+ (append
+ `("GIT_DIR"
+ "PAGER="
+ ;; Avoid repository locking during background operations
+ ;; (bug#21559).
+ ,@(when revert-buffer-in-progress-p
+ '("GIT_OPTIONAL_LOCKS=0")))
+ process-environment)))
(apply 'process-file vc-git-program nil buffer nil command args)))
(defun vc-git--out-ok (command &rest args)
--
2.17.0
^ permalink raw reply related [flat|nested] 49+ messages in thread