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

From: Kévin Le Gouguec <[email-masked]>
Date: Thu, 13 Feb 2025 19:23:15 +0100

Eli Zaretskii <[email-masked]> writes:

>> I have a hard time making sense of the sequence of events.  Evidently:
>> 
>> 1. vc-git-test--run did *not* pause for (file-exists-p
>>    ".git/index.lock"),
>> 2. but Git *still* tripped over a preexisting .git/index.lock.
>> 
>> Logging default-directory in the condition-case error handler confirms
>> we are in the correct directory, so I do not understand why
>> file-exists-p did not see the lock that Git reports.  All I can think of
>> is a stray Git process ¿started at some point by someone? that grabs the
>> lock between steps 1 & 2.
>
> Yes, something like that.  A.k.a. "a race condition".
>
>> Barring "turn off background processing" knobs for VC and/or Git that I
>> missed, not sure what to try next beside catching that "index.lock
>> exists" error and re-trying after a delay.
>
> 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?

  $ while make -j1 -C test lisp/vc/vc-git-tests ; do continue ; done
  [… 800 iterations later …]
  Running 8 tests (2025-02-13 19:12:17+0100, selector `(not (or (tag :unstable) (tag :nativecomp)))')
     passed  1/8  vc-git-test-annotate-time (0.002258 sec)
  Test vc-git-test-dir-branch-headers backtrace:
    […]
    vc-git-command(t 0 nil "checkout" "-b" "feature/foo" "master")
    […]
    vc-git-test--run("checkout" "-b" "feature/foo" "master")
    […]
  Test vc-git-test-dir-branch-headers condition:
      (error
       "Failed (status 128): git --no-pager checkout -b feature/foo master .")

(That was run on a fresh master checkout, to dispel interference from my
debugging changes.  See previous messages for more logs, in particular
proof that
(a) checking for index.lock presence before invoking vc-git-command does
not save us,
(b) replacing vc-dir-kill-dir-status-process with a busy loop on
vc-dir-busy does not save us either,
(a+b) together do not save us either)

At any rate, to hopefully contribute something to the discussion if I
misunderstood your comment: given the goals of this test, wondering if
it would be acceptable to have it invoke vc-git-dir-extra-headers
directly, instead of "pretending to be a user" and invoking vc-dir.  If
vc-dir is indeed responsible for whatever async process is thwarting
that test, cutting that middle man out would work around that (and let
us remove some scaffolding from the test to boot).

(If Git is responsible for that async process, then that change alone
will not help, although the scaffolding simplification would still be
nice TBH)