From: Siddh Raman Pant <siddh.raman.pant@oracle•com>
To: "j6t@kdbg•org" <j6t@kdbg•org>, "peff@peff•net" <peff@peff•net>
Cc: "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 05:59:20 +0000 [thread overview]
Message-ID: <a0916ed8965d2024ee0f6005c942f77a16f28dbb.camel@oracle.com> (raw)
In-Reply-To: <20260522051048.GA862219@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 2906 bytes --]
On Fri, May 22 2026 at 10:40:48 +0530, Jeff King wrote:
> 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
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2026-05-22 6:08 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
2026-05-22 5:59 ` Siddh Raman Pant [this message]
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=a0916ed8965d2024ee0f6005c942f77a16f28dbb.camel@oracle.com \
--to=siddh.raman.pant@oracle$(echo .)com \
--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=peff@peff$(echo .)net \
--cc=ps@pks$(echo .)im \
/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