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

Hi Duy,

On Sun, 8 May 2016, Duy Nguyen wrote:

On Sun, May 8, 2016 at 1:33 PM, Johannes Schindelin
[off-list ref] wrote:

quoted

The claim is that this libifies the procedure. But it makes the code
really nasty for use as a library: if this is run in a thread (and you
know that we are going to have to do this in the near future, for
performance reasons), it will completely mess up all the other threads
because it messes with the global file descriptors.

I vote one step at a time, leave multi-thread support for future.

Oh, but I never said that we have to do that now!

All I said was that using this dup() dance instead of truly libifying the
functions would slam the door almost shut for future multi-threading.
Which is a strong hint in my book that we should *not* do that dup()
dance, but fix our code by introducing a silent mode.

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.

Sure. And do we change those shared states temporarily in our functions?
No, we don't. The object db is not made temporarily inaccessible while a
certain function runs. The index access is not temporarily disabled while
a certain function runs.

And this is what that dup() dance does: it disables *all* output, not only
from the current thread.

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?

Again. I did *not* suggest to introduce multi-threading. I was making a
case for *avoiding* that ugly dup() to /dev/null and then dup() it back to
the original state. That would just ask for unintended side effects.

Ciao,
Dscho