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

From: Christian Couder <christian.couder@gmail.com>
Date: May 7, 2016, 10:12 UTC
Message-ID: <CAP8UFD0ht+ozCMguGwBc19dd9ViXG4tTE0mdaOQ+JVXhSOk02Q@mail.gmail.com>
In-Reply-To: <alpine.DEB.2.20.1605061733170.2963@virtualbox>

Hi Dscho,

On Fri, May 6, 2016 at 5:34 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 36 quoted lines

> Hi Chris,
>
> On Fri, 6 May 2016, Christian Couder wrote:
>
>> On Thu, May 5, 2016 at 10:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> > Am 05.05.2016 um 11:50 schrieb Christian Couder:
>> >>
>> >> On Mon, Apr 25, 2016 at 5:05 PM, Johannes Schindelin
>> >> <Johannes.Schindelin@gmx.de> wrote:
>> >>>
>> >>> Hi Chris,
>> >>>
>> >>> On Sun, 24 Apr 2016, Christian Couder wrote:
>> >>>
>> >>>> diff --git a/run-command.c b/run-command.c
>> >>>> index 8c7115a..29d2bda 100644
>> >>>> --- a/run-command.c
>> >>>> +++ b/run-command.c
>> >>>> @@ -85,7 +85,7 @@ static inline void close_pair(int fd[2])
>> >>>>   }
>> >>>>
>> >>>>   #ifndef GIT_WINDOWS_NATIVE
>> >>>> -static inline void dup_devnull(int to)
>> >>>> +void dup_devnull(int to)
>> >>>>   {
>> >>>
>> >>>
>> >>> The #ifndef GIT_WINDOWS_NATIVE rings very, very loud alarm bells.
>> >>
>> >>
>> >> Yeah, but I must say that I don't know what I should do about this.
>> >> Do you have a suggestion? Should I try to implement the same function
>> >> for Windows?
>
> 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.

By the way I took a look and there are 11 calls to fprintf(stderr, ...) and 10 calls to warning() in different places in builtin/apply.c. There might also be such calls in functions outside builtin/apply.c that are called by the functions in builtin/apply.c.

So I'd much rather keep doing what I am doing now. If you or someone else want to contribute patches on top of the series to do it in another way, maybe they might be integrated at the same time by Junio, so that the whole thing would appear in the same release and there would be no feature discrepancy between Windows and the other platforms, and you wouldn't need to implement anything special for Windows.

But anyway, even though I don't know much about Windows, I think if you have some code already in compat/mingw.c to handle redirections, it might be easier and safer overall to just implement the redirections in Windows.

Thanks, Christian.