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

From: Christian Couder <christian.couder@gmail.com>
Date: May 7, 2016, 13:46 UTC
Message-ID: <CAP8UFD2wFZgOzmadm7X2988RyDGve92TsoMHPf2S5Ydyhc=Nyg@mail.gmail.com>
In-Reply-To: <alpine.DEB.2.20.1605071409490.2963@virtualbox>

Hi Dscho,

On Sat, May 7, 2016 at 2:13 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 29 quoted lines

> Hi Chris,
>
> 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);

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.