git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Siddh Raman Pant <siddh.raman.pant@oracle.com>
Date
May 22, 2026, 05:59 UTC
Message-ID
<a0916ed8965d2024ee0f6005c942f77a16f28dbb.camel@oracle.com>
In-Reply-To
<20260522051048.GA862219@coredump.intra.peff.net>
On Fri, May 22 2026 at 10:40:48 +0530, Jeff King wrote:
Show 55 quoted lines
> 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

Okay, since the consensus here is pretty clear, I will remove this commit and send a v2.

Thanks, Siddh

Previous: Siddh Raman Pant
Message 29 of 29 in “Add support for an external command for fetching notes”
  1. 0/9 Add support for an external command for fetching notesSiddh Raman Pant, May 19, 2026
  2. 1/9 Documentation/git-range-diff: add missing notes options in synopsisSiddh Raman Pant, May 19, 2026
  3. 4/9 run-command: add support for timeout in command finisherSiddh Raman Pant, May 19, 2026
  4. 5/9 wrapper: add support for timeout and deadline in read helpersSiddh Raman Pant, May 19, 2026
  5. 6/9 t3301: cover generic displayed notes behaviorSiddh Raman Pant, May 19, 2026
  6. 7/9 notes: support an external command to display notesSiddh Raman Pant, May 19, 2026
  7. 2/9 notes: convert raw arg in format_display_notes() to boolSiddh Raman Pant, May 19, 2026
  8. 3/9 wrapper: add sleep_nanosecSiddh Raman Pant, May 19, 2026
  9. 8/9 Documentation: document external notes command optionsSiddh Raman Pant, May 19, 2026
  10. 9/9 t: add tests for external notes commandSiddh Raman Pant, May 19, 2026
  11. Junio C HamanoMay 19, 2026
  12. Junio C HamanoMay 19, 2026
  13. Junio C HamanoMay 20, 2026
  14. Siddh Raman PantMay 20, 2026
  15. Siddh Raman PantMay 20, 2026
  16. Siddh Raman PantMay 20, 2026
  17. Junio C HamanoMay 21, 2026
  18. brian m. carlsonMay 21, 2026
  19. Siddh Raman PantMay 21, 2026
  20. Siddh Raman PantMay 21, 2026
  21. Johannes SixtMay 21, 2026
  22. Oswald BuddenhagenMay 21, 2026
  23. Siddh Raman PantMay 21, 2026
  24. Johannes SixtMay 21, 2026
  25. brian m. carlsonMay 21, 2026
  26. Junio C HamanoMay 22, 2026
  27. Jeff KingMay 22, 2026
  28. Siddh Raman PantMay 22, 2026
  29. Siddh Raman PantMay 22, 2026

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.