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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Duy,

On Sun, 8 May 2016, Duy Nguyen wrote:

> On Sun, May 8, 2016 at 1:33 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > 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