Re: [PATCH 80/83] run-command: make dup_devnull() non static
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- May 8, 2016, 06:33 UTC
- Message-ID
- <alpine.DEB.2.20.1605080811380.4092@virtualbox>
- In-Reply-To
- <CAP8UFD2wFZgOzmadm7X2988RyDGve92TsoMHPf2S5Ydyhc=Nyg@mail.gmail.com>
Hi Chris,
On Sat, 7 May 2016, Christian Couder wrote:
Show 45 quoted lines
> On Sat, May 7, 2016 at 2:13 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Sat, 7 May 2016, Christian Couder wrote:
> >
> >> On Fri, May 6, 2016 at 5:34 PM, Johannes Schindelin
> >> <Johannes.Schindelin@gmx.de> 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().
Show 7 quoted lines
> 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