From: Jeff King <peff@peff•net>
To: Johannes Sixt <j6t@kdbg•org>
Cc: Siddh Raman Pant <siddh.raman.pant@oracle•com>,
"git@vger•kernel.org" <git@vger•kernel.org>,
"gitster@pobox•com" <gitster@pobox•com>,
"newren@gmail•com" <newren@gmail•com>, "ps@pks•im" <ps@pks•im>,
"oswald.buddenhagen@gmx•de" <oswald.buddenhagen@gmx•de>,
"code@khaugsbakk•name" <code@khaugsbakk•name>
Subject: Re: [PATCH 4/9] run-command: add support for timeout in command finisher
Date: Fri, 22 May 2026 01:10:48 -0400 [thread overview]
Message-ID: <20260522051048.GA862219@coredump.intra.peff.net> (raw)
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
next prev parent reply other threads:[~2026-05-22 5:10 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-05-19 16:30 [PATCH 0/9] Add support for an external command for fetching notes Siddh Raman Pant
2026-05-19 16:30 ` [PATCH 1/9] Documentation/git-range-diff: add missing notes options in synopsis Siddh Raman Pant
2026-05-19 23:47 ` Junio C Hamano
2026-05-20 7:00 ` Siddh Raman Pant
2026-05-21 0:28 ` Junio C Hamano
2026-05-21 4:13 ` Siddh Raman Pant
2026-05-19 16:30 ` [PATCH 2/9] notes: convert raw arg in format_display_notes() to bool Siddh Raman Pant
2026-05-19 16:30 ` [PATCH 3/9] wrapper: add sleep_nanosec Siddh Raman Pant
2026-05-19 23:50 ` Junio C Hamano
2026-05-20 7:07 ` Siddh Raman Pant
2026-05-19 16:30 ` [PATCH 4/9] run-command: add support for timeout in command finisher Siddh Raman Pant
2026-05-21 7:21 ` Johannes Sixt
2026-05-21 8:39 ` Oswald Buddenhagen
2026-05-21 9:59 ` Siddh Raman Pant
2026-05-21 14:36 ` Johannes Sixt
2026-05-22 0:10 ` Junio C Hamano
2026-05-22 5:46 ` Siddh Raman Pant
2026-05-22 5:10 ` Jeff King [this message]
2026-05-22 5:59 ` Siddh Raman Pant
2026-05-19 16:30 ` [PATCH 5/9] wrapper: add support for timeout and deadline in read helpers Siddh Raman Pant
2026-05-19 16:30 ` [PATCH 6/9] t3301: cover generic displayed notes behavior Siddh Raman Pant
2026-05-19 16:30 ` [PATCH 7/9] notes: support an external command to display notes Siddh Raman Pant
2026-05-20 0:03 ` Junio C Hamano
2026-05-20 6:59 ` Siddh Raman Pant
2026-05-21 1:12 ` brian m. carlson
2026-05-21 4:12 ` Siddh Raman Pant
2026-05-21 21:18 ` brian m. carlson
2026-05-19 16:30 ` [PATCH 8/9] Documentation: document external notes command options Siddh Raman Pant
2026-05-19 16:30 ` [PATCH 9/9] t: add tests for external notes command Siddh Raman Pant
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260522051048.GA862219@coredump.intra.peff.net \
--to=peff@peff$(echo .)net \
--cc=code@khaugsbakk$(echo .)name \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=j6t@kdbg$(echo .)org \
--cc=newren@gmail$(echo .)com \
--cc=oswald.buddenhagen@gmx$(echo .)de \
--cc=ps@pks$(echo .)im \
--cc=siddh.raman.pant@oracle$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox