Re: [PATCH v8 00/11] Git filter protocol

From: Jeff King <peff@peff.net>
Date: Oct 6, 2016, 16:01 UTC
Message-ID: <20161006160104.nhpxhfhketr4rb4h@sigill.intra.peff.net>
In-Reply-To: <5C30800A-C821-4B39-BEB7-7D55D86BD7EA@gmail.com>

On Thu, Oct 06, 2016 at 03:13:19PM +0200, Lars Schneider wrote:

Show 13 quoted lines

> >>> Well, it would be good to tell users _why_ Git is hanging, see below.
> >> 
> >> Agreed. Do you think it is OK to write the message to stderr?
> > 
> > On the other hand, this is why GIT_TRACE (and GIT_TRACE_PERFORMANCE)
> > was invented for.  We do not signal troubles with single-shot filters,
> > so I guess doing it for multi-file filters is not needed.
> 
> I am on the fence with this one.
> 
> @Junio/Peff:
> Where would you prefer to see a "Waiting for filter 'XYZ'... " message?
> On stderr or via GIT_TRACE?

I am not sure if I have followed all of this discussion, but I am of the opinion that Git should not be doing any timeouts at all. There are simply too many places where the filter (or any other process that git is depending on) could inexplicably hang, and I do not want to pepper random timeouts for all parts of the procedure where we say "woah, this is taking longer than expected" (nor do I want to have a timeout for _one_ spot, and ignore all the others).

If this is debugging output of the form "now I am calling wait() on all of the filters", without respect to any timeout, that sounds reasonable. Though I would argue that run-command should simply trace_printf() any processes it is waiting for, which covers _any_ process, not just the filters. That seems like a good match for the rest of the GIT_TRACE output, which describes how and when we spawn the sub-processes.

Something like:

diff --git a/run-command.c b/run-command.c
index 5a4dbb6..b884605 100644
--- a/run-command.c
+++ b/run-command.c
@@ -226,6 +226,9 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
 	pid_t waiting;
 	int failed_errno = 0;
 
+	if (!in_signal)
+		trace_printf("waiting for pid %d", (int)pid);
+
 	while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)
 		;	/* nothing */
 	if (in_signal)

but it does not have to be part of this series, I think.

-Peff