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

Hi Dscho,

On Sat, May 7, 2016 at 2:13 PM, Johannes Schindelin
[off-list ref] wrote:

Hi Chris,

On Sat, 7 May 2016, Christian Couder wrote:

quoted

On Fri, May 6, 2016 at 5:34 PM, Johannes Schindelin
[off-list ref] wrote:

quoted

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);

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);

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.

So what's the problem? Isn't it just that you don't want a
dup_devnull() for Windows that would be a few lines long?

You keep saying that what is done in this patch is "ugly" or that
there is a "real problem", but frankly I don't see why. Could you
explain exactly why?
Because the more I look at it, the more it looks to me like the
solution that is the simplest (even for Windows), the safest and the
closest to what the current code is doing.

Thanks,
Christian.