Subject: Re: bug#76187: vc-git-test-dir-branch-headers failure on Fedora

From: Kévin Le Gouguec <[email-masked]>
Date: Fri, 14 Feb 2025 08:51:39 +0100

Kévin Le Gouguec <[email-masked]> writes:

>>> Eli Zaretskii <[email-masked]> writes:
>>> 
>>> > How about just ignoring the error in this case?  AFAIU, it's rare
>>> > enough to not be important, right?  AFAIK, you cannot have more than
>>> > one Git instance modifying the repository at any given time, so this
>>> > problem cannot be fixed in principle if we are running tow or more Git
>>> > sessions in parallel.  IOW, these tests must NOT be run via parallel
>>> > make processing.
>>> 
>>> Apologies if my brain is too fried and I misunderstood: didn't I show
>>> that this error shows up without parallelism?
>>
>> If that is true, it means _my_ brain is fried, but then how is that
>> other Git process invoked, and by whom?  IOW, which process locked the
>> repository exactly while we were running Git from the test?
>
> No clue thus far.  You mention Git's automatic GC and that's one of my
> scapegoats; another is vc-dir spawning more things than I realize.

With the attached debugging patch (same as [1], plus binding
vc-command-messages to t), I get these traces when the failure happens:

    Running 8 tests (2025-02-14 08:17:56+0100, selector `(not (or (tag :unstable) (tag :nativecomp)))')
       passed  1/8  vc-git-test-annotate-time (0.002302 sec)
    Running in foreground: git --no-pager clone /tmp/emacs-test-go8R94-vc-git/ /tmp/emacs-test-NVSvcg-vc-git/ .
    Done (status=0): git --no-pager clone /tmp/emacs-test-go8R94-vc-git/ /tmp/emacs-test-NVSvcg-vc-git/ .
    Running in foreground: git --no-pager config --get remote.origin.url
    Done (status=0): git --no-pager config --get remote.origin.url
    Running in background: git --no-pager update-index --refresh .
    Done in background: git --no-pager update-index --refresh .
    Running in background: git --no-pager diff-index --relative -z -M HEAD -- .
    Done in background: git --no-pager diff-index --relative -z -M HEAD -- .
    Running in background: git --no-pager ls-files -z -u -- .
    Done in background: git --no-pager ls-files -z -u -- .
    Running in background: git --no-pager ls-files -z -o --exclude-standard -- .
    Done in background: git --no-pager ls-files -z -o --exclude-standard -- .
    Paused 1 time(s) waiting for vc-dir-busy
    Running in foreground: git --no-pager config --get remote.origin.url
    Done (status=0): git --no-pager config --get remote.origin.url
    Running in background: git --no-pager update-index --refresh .
    Running in foreground: git --no-pager checkout -b feature/foo master .
    err in /tmp/emacs-test-NVSvcg-vc-git/: (error Failed (status 128): git --no-pager checkout -b feature/foo master .)
    (buffer-string:
    fatal: Unable to create '/tmp/emacs-test-NVSvcg-vc-git/.git/index.lock': File exists.

    Another git process seems to be running in this repository, e.g.
    an editor opened by 'git commit'. Please make sure all processes
    are terminated then try again. If it still fails, a git process
    may have crashed in this repository earlier:
    remove the file manually to continue.

    )

Looking at vc-dir-refresh (invoked from vc-dir-revert-buffer-function,
invoked by the testcase reverting the vc-dir buffer), I'd conclude that
the sequence of events is (lighting a candle to the Muse of Manual
Whitespace Adjustment; citing evidence with "> TRACES")

* vc-git-test--dir-headers
  * revert-buffer
    * vc-dir-refresh
      * vc-dir-headers
        * vc-git-dir-extra-headers (👈 the code under test)
          > Running in foreground: git --no-pager config --get remote.origin.url
          > Done (status=0): git --no-pager config --get remote.origin.url
      * "asynchronous-looking things"
        > Running in background: git --no-pager update-index --refresh .
        (⚠️ no proof of termination) 
* vc-git-test--run "checkout" "-b" "feature/foo" main-branch
  * vc-git-test--wait (lambda () (file-exists-p ".git/index.lock"))
    > 🦗 [no log, ergo no lock file detected]
  * vc-git-command 
    > Running in foreground: git --no-pager checkout -b feature/foo master .
    (⚠️ presumably the concurrent locking happens around here)
    > fatal: Unable to create '/tmp/emacs-test-NVSvcg-vc-git/.git/index.lock': File exists.

I think 'git update-index' is a good suspect: cursory glance at
builtin/update-index.c:cmd_update_index confirms that it unconditionally
does:

	newfd = repo_hold_locked_index(the_repository, &lock_file, 0);

Bottomline:

* maybe we could amend the testcase to sync with this background
command,
* unclear whether we should; barring fault in my logic, I stand by my
opinion in [2] and would prefer to install the patch in that message, on
the grounds that contending with vc-dir's asynchrony is outside the
scope of this test (to wit: testing the extra-headers logic).

[1] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=76187#35
[2] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=76187#53

diff --git a/test/lisp/vc/vc-git-tests.el b/test/lisp/vc/vc-git-tests.el
index 4b5cb75df01..1212186d00d 100644
--- a/test/lisp/vc/vc-git-tests.el
+++ b/test/lisp/vc/vc-git-tests.el
@@ -103,10 +103,32 @@ vc-git-test--with-repo
        (vc-create-repo 'Git)
        ,@body)))
 
+(defun vc-git-test--wait (what desc)
+  "TODO"
+  (let ((i 0)
+        (max 20))
+    (while (funcall what)
+      (when (> (cl-incf i) max)
+        (error "Timed out waiting for %s" desc))
+      (sleep-for .05))
+    (when (> i 0)
+      (message "Paused %d time(s) waiting for %s" i desc))))
+
 (defun vc-git-test--run (&rest args)
   "Run git ARGS…, check for non-zero status, and return output."
+  ;; bug#76187: Git sometimes reports an index.lock file and bails,
+  ;; suggesting another lingering Git process in the same repo.  Since
+  ;; this test runs in a temporarily-generated repo, that process is
+  ;; probably an asynchronous one initiated earlier by this test; for
+  ;; lack of better understanding, wait until the lock is released.
+  (vc-git-test--wait (lambda () (file-exists-p ".git/index.lock"))
+                     "index.lock")
   (with-temp-buffer
-    (apply 'vc-git-command t 0 nil args)
+    (condition-case err
+        (apply 'vc-git-command t 0 nil args)
+      (t (message "err in %s: %s" default-directory err)
+         (message "(buffer-string:\n%s\n)" (buffer-string))
+         (signal (car err) (cdr err))))
     (buffer-string)))
 
 (defun vc-git-test--start-branch ()
@@ -126,10 +148,10 @@ vc-git-test--dir-headers
 return a list of (NAME . VALUE) pairs, where VALUE is nil if the header
 is absent."
   ;; FIXME: to reproduce interactive sessions faithfully, we would need
-  ;; to wait for the dir-status-files process to terminate; have not
-  ;; found a reliable way to do this.  As a workaround, kill pending
-  ;; processes and revert the `vc-dir' buffer.
-  (vc-dir-kill-dir-status-process)
+  ;; to synchronize with the dir-status-files process; have not found a
+  ;; reliable way to do this.  As a workaround, wait until vc-dir stops
+  ;; being "busy" then revert the `vc-dir' buffer.
+  (vc-git-test--wait #'vc-dir-busy "vc-dir-busy")
   (revert-buffer)
   (mapcar
    (lambda (header)
@@ -148,7 +170,8 @@ vc-git-test-dir-branch-headers
   (skip-unless (executable-find vc-git-program))
   ;; Create a repository that will serve as the "remote".
   (vc-git-test--with-repo origin-repo
-    (let ((main-branch (vc-git-test--start-branch)))
+    (let ((main-branch (vc-git-test--start-branch))
+          (vc-command-messages t))
       ;; 'git clone' this repository and test things in this clone.
       (ert-with-temp-directory clone-repo
         (vc-git-test--run "clone" origin-repo clone-repo)