bug#21559: 25.0.50; auto-revert-mode breaks git rebase

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Ben Gamari]

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

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.

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.

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).

git version 2.5.1

In GNU Emacs 25.0.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.16.6)
 of 2015-08-20 on ben-laptop
Windowing system distributor `The X.Org Foundation', version 11.0.11702000
System Description:	Debian GNU/Linux testing (stretch)

Configured using:
 `configure --build x86_64-linux-gnu --prefix=/usr
 --sharedstatedir=/var/lib --libexecdir=/usr/lib
 --localstatedir=/var/lib --infodir=/usr/share/info
 --mandir=/usr/share/man --with-pop=yes
 --enable-locallisppath=/etc/emacs25:/etc/emacs:/usr/local/share/emacs/25.0/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/25.0/site-lisp:/usr/share/emacs/site-lisp
 --build x86_64-linux-gnu --prefix=/usr --sharedstatedir=/var/lib
 --libexecdir=/usr/lib --localstatedir=/var/lib
 --infodir=/usr/share/info --mandir=/usr/share/man --with-pop=yes
 --enable-locallisppath=/etc/emacs25:/etc/emacs:/usr/local/share/emacs/25.0/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/25.0/site-lisp:/usr/share/emacs/site-lisp
 --with-x=yes --with-x-toolkit=gtk3 --with-toolkit-scroll-bars
 'CFLAGS=-g -O2 -fstack-protector-strong -Wformat
 -Werror=format-security -Wall' CPPFLAGS=-D_FORTIFY_SOURCE=2
 LDFLAGS=-Wl,-z,relro'

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GCONF GSETTINGS
NOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Emacs-Lisp

Minor modes in effect:
  elisp-slime-nav-mode: t
  goto-address-prog-mode: t
  auto-highlight-symbol-mode: t
  clean-aindent-mode: t
  highlight-numbers-mode: t
  highlight-parentheses-mode: t
  rainbow-delimiters-mode: t
  yas-global-mode: t
  yas-minor-mode: t
  helm-descbinds-mode: t
  helm-mode: t
  shell-dirtrack-mode: t
  projectile-global-mode: t
  projectile-mode: t
  recentf-mode: t
  winner-mode: t
  window-numbering-mode: t
  volatile-highlights-mode: t
  global-vi-tilde-fringe-mode: t
  global-undo-tree-mode: t
  undo-tree-mode: t
  savehist-mode: t
  popwin-mode: t
  global-page-break-lines-mode: t
  page-break-lines-mode: t
  Info-breadcrumbs-in-mode-line-mode: t
  ido-vertical-mode: t
  flx-ido-mode: t
  global-evil-surround-mode: t
  evil-surround-mode: t
  global-evil-search-highlight-persist: t
  evil-search-highlight-persist: t
  show-smartparens-global-mode: t
  show-smartparens-mode: t
  smartparens-mode: t
  evil-jumper-mode: t
  evil-escape-mode: t
  global-anzu-mode: t
  anzu-mode: t
  eval-sexp-fu-flash-mode: t
  company-mode: t
  global-hl-line-mode: t
  xterm-mouse-mode: t
  global-auto-revert-mode: t
  evil-leader-mode: t
  evil-mode: t
  evil-local-mode: t
  which-key-mode: t
  override-global-mode: t
  spacemacs-additional-leader-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t
  hs-minor-mode: t

Recent messages:
Resetting customization items...done
Creating customization setup...done
Creating customization items...
Creating group...
Creating group entries...done
Creating customization items ...done
Resetting customization items...done
Creating customization setup...done
Info-mouse-follow-link: Args out of range: 2394
Text is read-only [2 times]

Load-path shadows:
/home/ben/.emacs.d/elpa/helm-20150923.2134/helm-multi-match hides /home/ben/.emacs.d/elpa/helm-core-20150923.959/helm-multi-match
/home/ben/.emacs.d/elpa/notmuch-20150907.527/notmuch-show hides /usr/local/share/emacs/site-lisp/notmuch-show
/home/ben/.emacs.d/elpa/notmuch-20150907.527/notmuch-print hides /usr/local/share/emacs/site-lisp/notmuch-print
/home/ben/.emacs.d/elpa/notmuch-20150907.527/notmuch-wash hides /usr/local/share/emacs/site-lisp/notmuch-wash
/home/ben/.emacs.d/elpa/notmuch-20150907.527/notmuch-jump hides /usr/local/share/emacs/site-lisp/notmuch-jump
/home/ben/.emacs.d/elpa/notmuch-20150907.527/notmuch-crypto hides /usr/local/share/emacs/site-lisp/notmuch-crypto
/home/ben/.emacs.d/elpa/notmuch-20150907.527/notmuch-hello hides /usr/local/share/emacs/site-lisp/notmuch-hello
/home/ben/.emacs.d/elpa/notmuch-20150907.527/notmuch-tree hides /usr/local/share/emacs/site-lisp/notmuch-tree
/home/ben/.emacs.d/elpa/notmuch-20150907.527/notmuch hides /usr/local/share/emacs/site-lisp/notmuch
/home/ben/.emacs.d/elpa/notmuch-20150907.527/notmuch-address hides /usr/local/share/emacs/site-lisp/notmuch-address
/home/ben/.emacs.d/elpa/notmuch-20150907.527/coolj hides /usr/local/share/emacs/site-lisp/coolj
/home/ben/.emacs.d/elpa/notmuch-20150907.527/notmuch-parser hides /usr/local/share/emacs/site-lisp/notmuch-parser
/home/ben/.emacs.d/elpa/notmuch-20150907.527/notmuch-lib hides /usr/local/share/emacs/site-lisp/notmuch-lib
/home/ben/.emacs.d/elpa/notmuch-20150907.527/notmuch-query hides /usr/local/share/emacs/site-lisp/notmuch-query
/home/ben/.emacs.d/elpa/notmuch-20150907.527/notmuch-mua hides /usr/local/share/emacs/site-lisp/notmuch-mua
/home/ben/.emacs.d/elpa/notmuch-20150907.527/notmuch-message hides /usr/local/share/emacs/site-lisp/notmuch-message
/home/ben/.emacs.d/elpa/notmuch-20150907.527/notmuch-tag hides /usr/local/share/emacs/site-lisp/notmuch-tag
/home/ben/.emacs.d/elpa/notmuch-20150907.527/notmuch-maildir-fcc hides /usr/local/share/emacs/site-lisp/notmuch-maildir-fcc
/home/ben/.emacs.d/elpa/cmake-mode-20150817.725/cmake-mode hides /usr/share/emacs24/site-lisp/cmake-data/cmake-mode
/home/ben/.emacs.d/elpa/cmake-mode-20150817.725/cmake-mode hides /usr/share/emacs/site-lisp/cmake-mode
/usr/share/emacs/site-lisp/rst hides /usr/share/emacs/25.0.50/lisp/textmodes/rst
/home/ben/.emacs.d/elpa/seq-20150917.1508/seq hides /usr/share/emacs/25.0.50/lisp/emacs-lisp/seq

Features:
(shadow sort mail-extr warnings emacsbug message rfc822 mml mml-sec
mailabbrev gmm-utils mailheader sendmail mail-utils cus-edit cus-start
cus-load company-files company-keywords company-etags company-gtags
company-template company-dabbrev-code company-dabbrev company-capf
elisp-slime-nav goto-addr auto-highlight-symbol clean-aindent-mode
highlight-numbers parent-mode highlight-parentheses hideshow
rainbow-delimiters yasnippet jka-compr eieio-opt speedbar sb-image
ezimage dframe find-func helm-command helm-elisp helm-eval edebug
helm-descbinds helm-mode helm-files image-dired tramp tramp-compat
tramp-loaddefs trampver shell pcomplete format-spec dired-x dired-aux
ffap helm-buffers helm-elscreen helm-tags helm-bookmark helm-adaptive
helm-info bookmark helm-locate helm-grep helm-regexp helm-plugin
helm-external helm-net browse-url xml url url-proxy url-privacy
url-expand url-methods url-history url-cookie url-domsuf url-util
url-parse auth-source gnus-util password-cache url-vars mailcap
helm-utils helm-help helm-types helm helm-source eieio-compat
helm-multi-match helm-lib dired projectile grep compile ibuf-ext ibuffer
recentf tree-widget disp-table server winner window-numbering etags xref
project volatile-highlights vi-tilde-fringe undo-tree diff solarized
smooth-scrolling smartparens-config saveplace savehist py-yapf powerline
powerline-separators color powerline-themes popwin page-break-lines
info+ ido-vertical-mode flx-ido flx ido exec-path-from-shell
evil-surround evil-search-highlight-persist evil-numbers evil-lisp-state
smartparens evil-jumper evil-indent-textobject evil-exchange evil-escape
evil-args evil-anzu anzu eval-sexp-fu rx highlight diminish company-web
company web-completion-data info tex-site adaptive-wrap hybrid-mode ielm
pp comint ansi-color hl-line xt-mouse autorevert filenotify
core-evilified-state evil-leader evil evil-integration evil-maps
evil-commands evil-command-window evil-types evil-search evil-ex
evil-macros evil-repeat evil-states evil-core evil-common windmove
thingatpt rect evil-digraphs evil-vars ring which-key quelpa
package-build mm-decode mm-bodies mm-encode mail-parse rfc2231 rfc2047
rfc2045 ietf-drums mm-util help-fns mail-prsvr json lisp-mnt use-package
bind-key s ucs-normalize dash wid-edit zenburn-theme
core-configuration-layer cl-seq finder-inf core-dotspacemacs ht cl
package epg-config eieio byte-opt bytecomp byte-compile cl-extra
help-mode easymenu seq cconv eieio-core cl-macs gv core-spacemacs
derived edmacro kmacro cl-loaddefs cl-lib core-evilify-keymap
core-keybindings easy-mmode core-use-package-ext core-micro-state corelv
core-toggle core-fonts-support core-spacemacs-buffer core-funcs
core-themes-support advice core-auto-completion core-release-management
core-emacs-backports subr-x pcase devhelp time-date mule-util tooltip
eldoc electric uniquify ediff-hook vc-hooks lisp-float-type mwheel x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list newcomment elisp-mode lisp-mode prog-mode register page
menu-bar rfn-eshadow timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core frame cl-generic cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese charscript case-table epa-hook jka-cmpr-hook help
simple abbrev minibuffer cl-preloaded nadvice loaddefs button faces
cus-face macroexp files text-properties overlay sha1 md5 base64 format
env code-pages mule custom widget hashtable-print-readable backquote
dbusbind inotify dynamic-setting system-font-setting font-render-setting
move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 854111 669089)
 (symbols 48 49466 68)
 (miscs 40 1968 2565)
 (strings 32 311688 272104)
 (string-bytes 1 11727672)
 (vectors 16 64346)
 (vector-slots 8 1157167 184119)
 (floats 8 1008 1405)
 (intervals 56 4139 1183)
 (buffers 976 26)
 (heap 1024 129296 66437))

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Eli Zaretskii]

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

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Ben Gamari]

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

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Eli Zaretskii]

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

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Ben Gamari]

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

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Michael Albinus]

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.

bug#21559: [PATCH] autorevert: Wait for repository to become idle before calling vc-find-file-hook

[Ben Gamari]

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

bug#21559: [PATCH] autorevert: Wait a while before calling vc-find-file-hook

[Ben Gamari]

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

[PATCH] autorevert: Wait a while before calling vc-find-file-hook

[Ben Gamari]

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

bug#21559: [PATCH] autorevert: Wait a while before calling vc-find-file-hook

[Michael Albinus]

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

bug#21559: [PATCH] autorevert: Wait a while before calling vc-find-file-hook

[Ben Gamari]

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

bug#21559: [PATCH] autorevert: Wait a while before calling vc-find-file-hook

[Michael Albinus]

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.

bug#21559: [PATCH] autorevert: Wait a while before calling vc-find-file-hook

[Ben Gamari]

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

bug#21559: [PATCH] autorevert: Wait a while before calling vc-find-file-hook

[Michael Albinus]

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.

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Mitchel Humpherys]

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

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Ben Gamari]

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

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Mitchel Humpherys]

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

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Mitchel Humpherys]

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".

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Mitchel Humpherys]

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.

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Ben Gamari]

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

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Ben Gamari]

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

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Mitchel Humpherys]

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.

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Ben Gamari]

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

Mitchel Humpherys <mitch.special@gmail.com> writes:

> There's no rebase there, so detecting a rebase to skip auto-revert
> wouldn't help...
>
Indeed. This is a good point.

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

I believe the path is,

    auto-revert-handler
    vc-refresh-state
    vc-call-backend 'file-life-hook
    vc-git-find-file-hook

I'm not sure whether it is possible to eliminate the `git status' call.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Daniel Colascione]

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

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Jason Merrill]

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

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Alexei Khlebnikov]

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

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Alexei Khlebnikov]

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

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Alexei Khlebnikov]

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

bug#21559: PATCH review needed: lisp/vc/vc-git.el (vc-git-state, vc-git-conflicted-files)

[Alexei Khlebnikov]

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

bug#21559: PATCH review needed: lisp/vc/vc-git.el (vc-git-state, vc-git-conflicted-files)

[Michael Albinus]

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.

bug#21559: PATCH review needed: lisp/vc/vc-git.el (vc-git-state, vc-git-conflicted-files)

[Eli Zaretskii]

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

bug#21559: PATCH review needed: lisp/vc/vc-git.el (vc-git-state, vc-git-conflicted-files)

[Alexei Khlebnikov]

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

bug#21559: PATCH review needed: lisp/vc/vc-git.el (vc-git-state, vc-git-conflicted-files)

[Michael Albinus]

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.

bug#21559: PATCH review needed: lisp/vc/vc-git.el (vc-git-state, vc-git-conflicted-files)

[Alexei Khlebnikov]

2018-02-19 19:50 GMT+01:00 Michael Albinus <michael.albinus@gmx.de>:
> Looks OK to me.

Cool, so quick answer, thanks! :)

bug#21559: PATCH review needed: lisp/vc/vc-git.el (vc-git-state, vc-git-conflicted-files)

[Dmitry Gutov]

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.

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Dmitry Gutov]

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

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Alexei Khlebnikov]

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

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Dmitry Gutov]

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?

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Eli Zaretskii]

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

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Michael Albinus]

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.

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Dmitry Gutov]

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

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Michael Albinus]

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.

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Dmitry Gutov]

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?

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Alexei Khlebnikov]

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

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Michael Albinus]

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.

bug#21559: 25.0.50; auto-revert-mode breaks git rebase

[Dmitry Gutov]

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.

bug#21559: additional patch needed to set GIT_OPTIONAL_LOCKS=0 in all cases

[Andrew Ruder]

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

bug#21559: additional patch needed to set GIT_OPTIONAL_LOCKS=0 in all cases

[Phil Sainty]

On 2018-09-26 00:23, Andrew Ruder wrote:
> Please see attached patch.  There is also some early reports of
> success at https://github.com/magit/magit/issues/2708

Just to reiterate my comments from there, I believe this new patch
does indeed resolve this bug.

I've been using Emacs 26.1 with the earlier patch applied:
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=21559#131
And git version 2.16.3

With that, I was very regularly encountering the error "Unable to
create '.git/index.lock': File exists." when rebasing in Magit with
global-auto-revert-mode enabled.

With this new patch applied too, I am not seeing the error at all.

For testing I repeatedly rebased a moderately large branch, with and
without the new patch (3 times each).  Without the new patch I was
encountering "Unable to create '.git/index.lock': File exists."
between 3 and 5 times during each rebase.  With the new patch I didn't
encounter the problem at all in any of the rebases.


-Phil

bug#21559: additional patch needed to set GIT_OPTIONAL_LOCKS=0 in all cases

[Michael Albinus]

Phil Sainty <psainty@orcon.net.nz> writes:

> On 2018-09-26 00:23, Andrew Ruder wrote:
>> Please see attached patch.  There is also some early reports of
>> success at https://github.com/magit/magit/issues/2708
>
> Just to reiterate my comments from there, I believe this new patch
> does indeed resolve this bug.

It looks also good to me. If nobody opposes, I'll commit it next days.

> -Phil

Best regards, Michael.

bug#21559: additional patch needed to set GIT_OPTIONAL_LOCKS=0 in all cases

[Michael Albinus]

Version: 27.1

> It looks also good to me. If nobody opposes, I'll commit it next days.

Pushed to master. I'm closing the bug.

Best regards, Michael.