From: Jeff King <peff@peff.net>
Date: Fri, 22 May 2026 05:10:48 GMT
Subject: Re: [PATCH 4/9] run-command: add support for timeout in command finisher
Message-ID: <20260522051048.GA862219@coredump.intra.peff.net>
In-Reply-To: <cf52154c-1275-4a4b-957e-5aa17f22705c@kdbg.org>

On Thu, May 21, 2026 at 04:36:05PM +0200, Johannes Sixt wrote:

> Am 21.05.26 um 11:59 schrieb Siddh Raman Pant:
> > The timeout is for the failure path, where the external helper has
> > already stopped following that protocol or is blocked on something
> > outside git's control. Since git starts the helper and puts it on the
> > log/grep path, git also needs a bounded way to recover when that helper
> > does not make progress. Otherwise an optional note source can prevent
> > the main git command from completing.
> 
> That Git communicates with a process that looks like it stopped is the
> normal case, for example:
> 
> - Output is sent to the pager. The user can take their time to study the
> output. All the while, git waits patiently for the user to advance the
> pager.
> 
> - Git fetch transfers large amounts of data across the network. Most of
> the time it waits for data to arrive and does nothing. The peer process
> looks like it hangs. Git does not decide to kill the connection at any
> time. It is the user's decision to do so.
> 
> If the notes provider hangs, then it is not on Git to decide when it has
> waited long enough.

Yeah, I agree with your point of view. If I understand this patch series
correctly, it is about adding an external process to map commit ids to
note data. So I can think of some existing features that are quite close
to that in nature, none of which use timeouts:

  - textconv filters and external diffs which process data in the middle
    of a git-log invocation

  - long-lived clean/smudge filters map blobs to arbitrarily large text

  - cat-file's batch mode maps object ids to user-specified data about
    that object

As you note, it's up to the command to be well-behaved. Git should
notice and respond appropriately if the command closes the pipe, of
course. Sometimes a timeout can help with a poorly behaved command, but
IMHO it is not worth the cost of non-determinism that it brings.

Moreover, the bits touching run-command here make me nervous, especially
after the challenges we saw in the child-cleanup topic that was reverted
just after v2.54. There is often a shell interposed between Git and the
sub-command, and we don't always know how the shell will react to
signals. Using SIGKILL will eventually get us _something_ to wait() on,
but it might not even be the process we care about!

I don't really care much about this external-notes feature one way or
the other, but if we are going to do it, I don't see any reason why it
would not behave like all of the other similar parts of Git.

-Peff