Re: [PATCH v5 11/11] run-command: block signals between fork and execve

From: Brandon Williams <bmwill@google.com>
Date: Apr 19, 2017, 16:10 UTC
Message-ID: <20170419161038.GB132229@google.com>
In-Reply-To: <20170419074830.GA25646@starla>

On 04/19, Eric Wong wrote:

Show 16 quoted lines

> Johannes Sixt <j6t@kdbg.org> wrote:
> > Am 19.04.2017 um 01:18 schrieb Brandon Williams:
> > >@@ -400,6 +404,53 @@ static char **prep_childenv(const char *const *deltaenv)
> > > }
> > > #endif
> > >
> > 
> > Does this #endif in this hunk context belong to an #ifndef
> > GIT_WINDOWS_NATIVE? If so, I wonder why these new functions are outside
> > these brackets? An oversight?
> 
> Seems like an oversight, sorry about that.
> All the new atfork stuff I added should be protected by
> #ifndef GIT_WINDOWS_NATIVE.
> 
> Brandon / Johannes: can you fixup on your end?

Correct, this is an oversight I should have caught :) No worries though, I'll fix it up in a reroll (since I'm going to be need to send out another version to fix up another patch in the series for Windows)

Show 8 quoted lines

> 
> I wonder if some of this OS-specific code would be more
> easily maintained if split out further to OS-specific files,
> even at the risk of some code duplication.
> 
> And/or perhaps label all #else and #endif statements with
> comments, and limit the scope of each ifdef block to be
> per-function for with tiny attention spans like me :x

Yeah I'm not sure I know the best way to prevent this from happening, thankfully we have windows folk who keep us honest :D

Show 11 quoted lines

> 
> > >+struct atfork_state {
> > >+#ifndef NO_PTHREADS
> > >+	int cs;
> > >+#endif
> > >+	sigset_t old;
> > >+};
> > ...
> > 
> > -- Hannes
> >

-- 
Brandon Williams