Hi Chris,
On Sat, 7 May 2016, Christian Couder wrote:
> On Sat, May 7, 2016 at 2:13 PM, Johannes Schindelin
> <Johannes.Schindelin@xxxxxx> wrote:
> >
> > On Sat, 7 May 2016, Christian Couder wrote:
> >
> >> On Fri, May 6, 2016 at 5:34 PM, Johannes Schindelin
> >> <Johannes.Schindelin@xxxxxx> wrote:
> >> >
> >> > No, you should change the code that requires that ugly dup()ing so
> >> > that it can be configured to shut up.
> >>
> >> After taking a look, it looks like a routine that does nothing could
> >> be passed to set_error_routine() and that could do part of the trick.
> >>
> >> This part might not be too ugly, but it would anyway be more complex,
> >> less close to what the code is doing now and more error prone, as one
> >> also need to make sure that for example no warning() or
> >> fprintf(stderr, ...) are called and nothing is printed on stdout.
> >
> > I am afraid that you *have* to do that, though, if you truly want to
> > libify the code.
> >
> > Of course you can go with really ugly workarounds instead. Something
> > like a global flag that die() and error() and warning() respect. It
> > would incur some technical debt, but it would make your life easier in
> > the short run.
> >
> > Both the real solution and the workaround would be better than the
> > current version of the patches that dup() back and forth, just to
> > avoid addressing the real problem.
>
> The code that is now in master in builtin/am.c does:
>
> if (state->threeway && !index_file) {
> cp.no_stdout = 1;
> cp.no_stderr = 1;
> }
>
> and in run-command.c there is already:
>
> if (cmd->no_stdout)
> dup_devnull(1);
> [...]
> if (cmd->no_stderr)
> dup_devnull(2);
Of course it does that. Because there is no other way, that's why: you
cannot change the code that is spawned by start_command().
> for Linux and the following for Windows:
>
> if (cmd->no_stderr)
> fherr = open("/dev/null", O_RDWR);
> [...]
> if (cmd->no_stdout)
> fhout = open("/dev/null", O_RDWR);
And it is very well contained on Windows. No other callers. The code
is limited to run_command.c.
> so the current code is already using dup_devnull() for Linux that you
> don't want me to use, and it looks like there is already a simple way
> to do that on Windows.
The difference between the code in master and what your patches try to do
is that in the latter case, you want to dup() just for a while, to shut up
a code path that is not only known very well, but our very own code that
is easily changed, only to dup() it *back* in the end.
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.
And that is why it is ugly: it incurs an enormous technical debt that will
make code changes substantially more complicated down the road.
In essence, you save yourself a little time by sloppily dup()ing back and
forth. At the expense of making the life much harder for the developer who
needs to use your code as a library function.
And actually, hiding even fatal errors might be an ugly side effect of the
current implementation, an unfortunate implementation detail, really, not
something we want to preserve when libifying the code. And actually,
dup()ing the *caller's* stdout is not exactly preserving the current
behavior that dup()s the *called process'* stdout.
So yes, I find the proposed patch inelegant.
If others are okay with this, I will shut up. But I have to point out that
it is ugly code, plain and simple, that silences an entire global file
descriptor, just temporarily, only to avoid a careful set of patches that
introduces a silent mode to the library functions that need to be called,
which might even facilitate other libifying efforts.
I hope you do not take this as a personal attack. It is not intended as
such. It is intended to help end up with the best possible code quality.
Ciao,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html