Re: [PATCH 4/9] run-command: add support for timeout in command finisher

From: Jeff King <peff@peff.net>
Date: May 22, 2026, 05:10 UTC
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:

Show 22 quoted lines

> 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