Re: [PATCH 80/83] run-command: make dup_devnull() non static

From: Junio C Hamano <gitster@pobox.com>
Date: May 9, 2016, 17:40 UTC
Message-ID: <xmqq4ma7w2nx.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CACsJy8DUcLWQ6AwyLE+CF+TxZtqDLS=82peizSf4TrZ3q=qjJg@mail.gmail.com>

Duy Nguyen <pclouds@gmail.com> writes:

Show 7 quoted lines

> I vote one step at a time, leave multi-thread support for future.
> There's a lot more shared state than file descriptors anyway, at least
> there are object db and index access and probably a couple of hidden
> static variables somewhere. And I'm not sure if multi-thread really
> helps here. Are we really CPU-bound? If object inflation causes that
> (wild guess), can we just inflate ahead in some separate process and
> pass the result back?

I do not particularly care about multi-thread issues, but I have to agree with Dscho that the updated code that claims to be "libified" that futz with the file descriptors like the way this patch does is not a proper libification.

Unfortunately, the anticipated caller of this code that does "this may fail and it is OK because it is merely one of the attempts, so let's not show the errors" is not something we call only when we are falling back, so "why not do this rare codepath via the usual run_command() interface to spawn 'apply' as a separate process?", which would be the most sensible "one step at a time" suggestion if it were the case, would not apply.

As you will be passing the apply state structure throughout the callchain, would it be a viable and reasonable endgame state to have a strbuf in it that accumulates the errors? That is, instead of dup()ing the standard error stream out, you would accumulate the errors for a caller that asks their errors not directly sent to the standard error stream, so that it can choose to either show it at the end, or ignore it altogether.

How far can you go with just set-error-routine? Are there things, other than the file descriptors, that you need to futz with in order to covert that "we'd fallback, so this early round must be silent" codepath?