threads / patch / 42195

patcht5510: run auto-gc in the foreground

Subject: [PATCH] t5510: run auto-gc in the foreground

## tl;dr

9 messages between May 1, 2016 and May 7, 2016. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

SZEDER Gábor· May 1, 2016, 15:37 UTC · lore

The last test added to 't5510-fetch' in 0898c9628104 (fetch: release pack files before garbage-collecting, 2016-01-13) may sporadically trigger following error message from the test harness:

  rm: cannot remove 'trash directory.t5510-fetch/auto-gc/.git': Directory not empty

The test in question forces an auto-gc, which, if the system supports it, runs in the background by default, and occasionally takes long enough for the test to finish and for 'test_done' to start housekeeping. This can lead to the test's 'git gc --auto' in the background and 'test_done's 'rm -rf $trash' in the foreground racing each other to create and delete files and directories. It might just happen that 'git gc' re-creates a directory that 'rm -rf' already visited and removed, which ultimately triggers the above error.

Disable detaching the auto-gc process to ensure that it finishes before the test can continue, thus avoiding this racy situation.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 t/t5510-fetch.sh | 1 +
 1 file changed, 1 insertion(+)
Show changes to t/t5510-fetch.sh +1 −0
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 38321d19efbe..454d896390c0 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -682,6 +682,7 @@ test_expect_success 'fetching with auto-gc does not lock up' '
 	(
 		cd auto-gc &&
 		git config gc.autoPackLimit 1 &&
+		git config gc.autoDetach false &&
 		GIT_ASK_YESNO="$D/askyesno" git fetch >fetch.out 2>&1 &&
 		! grep "Should I try again" fetch.out
 	)

-- 
2.8.2.356.ge684b1d
Johannes Schindelin· May 2, 2016, 07:01 UTC · re: SZEDER Gábor · lore

Re: [PATCH] t5510: run auto-gc in the foreground

Hi Gábor,
On Sun, 1 May 2016, SZEDER Gábor wrote:
Show 12 quoted lines
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 38321d19efbe..454d896390c0 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -682,6 +682,7 @@ test_expect_success 'fetching with auto-gc does not lock up' '
>  	(
>  		cd auto-gc &&
>  		git config gc.autoPackLimit 1 &&
> +		git config gc.autoDetach false &&
>  		GIT_ASK_YESNO="$D/askyesno" git fetch >fetch.out 2>&1 &&
>  		! grep "Should I try again" fetch.out
>  	)
Sounds good to me.

Alternatively, we could consider passing `-c gc.autoDetach=false` instead, to limit the scope. I am not insisting on it, of course ;-)

Ciao, Dscho

SZEDER Gábor· May 2, 2016, 23:55 UTC · re: Johannes Schindelin · lore

Re: [PATCH] t5510: run auto-gc in the foreground

Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:
Show 19 quoted lines
> Hi Gábor,
>
> On Sun, 1 May 2016, SZEDER Gábor wrote:
>
>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
>> index 38321d19efbe..454d896390c0 100755
>> --- a/t/t5510-fetch.sh
>> +++ b/t/t5510-fetch.sh
>> @@ -682,6 +682,7 @@ test_expect_success 'fetching with auto-gc does
>> not lock up' '
>> 	(
>> 		cd auto-gc &&
>> 		git config gc.autoPackLimit 1 &&
>> +		git config gc.autoDetach false &&
>> 		GIT_ASK_YESNO="$D/askyesno" git fetch >fetch.out 2>&1 &&
>> 		! grep "Should I try again" fetch.out
>> 	)
>
> Sounds good to me.
There is something still bothering me, though.

I take this was a Windows-specific issue; deleting open files on Linux is no brainer. According to a comment on the original Git for Windows issue at github[1], 'git gc' runs in the background by default on Windows as well.

Now, it's 'git gc --auto' that's trying to delete pack and index files that became redundant after repacking, and when it can't delete those files, then complains. I.e. those "Should I try again" questions the test looks for come from 'git gc', not from 'git fetch'. So far so good.

Let's assume that someone inconsiderately removes that closed_all_pack() call added to cmd_fetch(), basically reverting the fix in 0898c9628104. The test added in the same commit (i.e. not including my fix here) should still be able to catch it, but I think it's possible that it sometimes remains unnoticed. Consider the following sequence:

    1. 'git fetch' does its thing, including opening pack and index files.
    2. 'git fetch' launches 'git gc --auto' which then goes into background.
    3. The scheduler happens to decide that it's 'git fetch's turn again,
       and it finishes, including closing all opened pack and index files.
    4. 'git gc' gets a chance to proceed, repacks and then manages to delete
       all redundant pack and index files successfully.
    5. 'grep' doesn't find the string it looks for.
    6. The test succeeds.

And the background 'git gc --auto' doesn't even have to be delayed that long, because 'git fetch' exits immediately after launching it. That's considerably shorter than the delay necessary for the 'rm -rf' error I described in the commit message, because for the latter 'git fetch' must finish, 'grep' must run, and 'test_done' must write the test results and start 'rm -rf $trash' while 'git gc' is still running in the background.

So, if I'm right, then my fix is not just about avoiding a sporadic error from the test harness, but it's also important for the test's correctness. But am I right? Alas I don't have a Git for Windows dev environment to play around with this.

[1] - https://github.com/git-for-windows/git/issues/500#issuecomment-149933531
Gábor
Show 5 quoted lines
> Alternatively, we could consider passing `-c gc.autoDetach=false` instead,
> to limit the scope. I am not insisting on it, of course ;-)
>
> Ciao,
> Dscho
SZEDER Gábor· May 3, 2016, 11:50 UTC · re: SZEDER Gábor · lore

Re: [PATCH] t5510: run auto-gc in the foreground

Quoting SZEDER Gábor <szeder@ira.uka.de>:
Show 27 quoted lines
> Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>
>> Hi Gábor,
>>
>> On Sun, 1 May 2016, SZEDER Gábor wrote:
>>
>>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
>>> index 38321d19efbe..454d896390c0 100755
>>> --- a/t/t5510-fetch.sh
>>> +++ b/t/t5510-fetch.sh
>>> @@ -682,6 +682,7 @@ test_expect_success 'fetching with auto-gc does
>>> not lock up' '
>>> 	(
>>> 		cd auto-gc &&
>>> 		git config gc.autoPackLimit 1 &&
>>> +		git config gc.autoDetach false &&
>>> 		GIT_ASK_YESNO="$D/askyesno" git fetch >fetch.out 2>&1 &&
>>> 		! grep "Should I try again" fetch.out
>>> 	)
>>
>> Sounds good to me.
>
> There is something still bothering me, though.
>
> I take this was a Windows-specific issue; deleting open files on Linux is
> no brainer.  According to a comment on the original Git for Windows issue
> at github[1], 'git gc' runs in the background by default on Windows as well.
Ok, having slept on it, it was a false alarm.

Though 'git gc --auto' claims "Auto packing the repository in background for optimum performance." on Windows, it does in fact runs in the foreground.

'git gc --auto' first prints that message, unless gc.autoDetach is disabled, and then calls daemonize() to go to the background. However, daemonize() is basically a no-op on Windows, thus 'git gc' will remain in the foreground and the sequence I described below is impossible. Good.

Perhaps it would be worth updating 'git gc' to not lie about going to the background when we can already know in advance that it won't.

Show 44 quoted lines
> Now, it's 'git gc --auto' that's trying to delete pack and index files that
> became redundant after repacking, and when it can't delete those files,
> then complains.  I.e. those "Should I try again" questions the test looks
> for come from 'git gc', not from 'git fetch'.  So far so good.
>
> Let's assume that someone inconsiderately removes that closed_all_pack()
> call added to cmd_fetch(), basically reverting the fix in 0898c9628104.
> The test added in the same commit (i.e. not including my fix here) should
> still be able to catch it, but I think it's possible that it sometimes
> remains unnoticed. Consider the following sequence:
>
>    1. 'git fetch' does its thing, including opening pack and index files.
>    2. 'git fetch' launches 'git gc --auto' which then goes into background.
>    3. The scheduler happens to decide that it's 'git fetch's turn again,
>       and it finishes, including closing all opened pack and index files.
>    4. 'git gc' gets a chance to proceed, repacks and then manages to delete
>       all redundant pack and index files successfully.
>    5. 'grep' doesn't find the string it looks for.
>    6. The test succeeds.
>
> And the background 'git gc --auto' doesn't even have to be delayed that
> long, because 'git fetch' exits immediately after launching it.  That's
> considerably shorter than the delay necessary for the 'rm -rf' error I
> described in the commit message, because for the latter 'git fetch' must
> finish, 'grep' must run, and 'test_done' must write the test results and
> start 'rm -rf $trash' while 'git gc' is still running in the background.
>
> So, if I'm right, then my fix is not just about avoiding a sporadic error
> from the test harness, but it's also important for the test's
> correctness.  But am I right?  Alas I don't have a Git for Windows dev
> environment to play around with this.
>
> [1] -  
> https://github.com/git-for-windows/git/issues/500#issuecomment-149933531
>
>
> Gábor
>
>
>> Alternatively, we could consider passing `-c gc.autoDetach=false` instead,
>> to limit the scope. I am not insisting on it, of course ;-)
>>
>> Ciao,
>> Dscho
Johannes Schindelin· May 4, 2016, 05:48 UTC · re: SZEDER Gábor · lore

Re: [PATCH] t5510: run auto-gc in the foreground

Hi Gábor,
On Tue, 3 May 2016, SZEDER Gábor wrote:
Show 34 quoted lines
> Quoting SZEDER Gábor <szeder@ira.uka.de>:
> 
> >Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> >
> > >Hi Gábor,
> > >
> > >On Sun, 1 May 2016, SZEDER Gábor wrote:
> > >
> > > >diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> > > >index 38321d19efbe..454d896390c0 100755
> > > >--- a/t/t5510-fetch.sh
> > > >+++ b/t/t5510-fetch.sh
> > > >@@ -682,6 +682,7 @@ test_expect_success 'fetching with auto-gc does
> > > >not lock up' '
> > > > (
> > > >  cd auto-gc &&
> > > >  git config gc.autoPackLimit 1 &&
> > > >+		git config gc.autoDetach false &&
> > > >  GIT_ASK_YESNO="$D/askyesno" git fetch >fetch.out 2>&1 &&
> > > >  ! grep "Should I try again" fetch.out
> > > > )
> > >
> > >Sounds good to me.
> >
> >There is something still bothering me, though.
> >
> >I take this was a Windows-specific issue; deleting open files on Linux is
> >no brainer.  According to a comment on the original Git for Windows issue
> >at github[1], 'git gc' runs in the background by default on Windows as well.
> 
> Ok, having slept on it, it was a false alarm.
> 
> Though 'git gc --auto' claims "Auto packing the repository in background for
> optimum performance." on Windows, it does in fact runs in the foreground.
Thanks for checking. I ran out of time yesterday.
> 'git gc --auto' first prints that message, unless gc.autoDetach is disabled,
> and then calls daemonize() to go to the background.  However, daemonize() is
> basically a no-op on Windows, thus 'git gc' will remain in the foreground and
> the sequence I described below is impossible.  Good.

Oh, right. I think this will take a lot of energy to fix: daemonize()'s functionality is not really available, indeed. What *is* available is a spawn() that detaches the new process.

> Perhaps it would be worth updating 'git gc' to not lie about going to the
> background when we can already know in advance that it won't.

Hmm. https://github.com/git/git/blob/master/builtin/gc.c#L372-L373 already looks correct (should it really know that we cannot daemonize()? What about other code paths using that function?):

			if (detach_auto)
				fprintf(stderr, _("Auto packing the repository in background for optimum performance.\n"));
			else
				fprintf(stderr, _("Auto packing the repository for optimum performance.\n"));

Ciao, Dscho

SZEDER Gábor· May 5, 2016, 15:14 UTC · re: Johannes Schindelin · lore

Re: [PATCH] t5510: run auto-gc in the foreground

Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:
Show 64 quoted lines
> Hi Gábor,
>
> On Tue, 3 May 2016, SZEDER Gábor wrote:
>
>> Quoting SZEDER Gábor <szeder@ira.uka.de>:
>>
>> >Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>> >
>> > >Hi Gábor,
>> > >
>> > >On Sun, 1 May 2016, SZEDER Gábor wrote:
>> > >
>> > > >diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
>> > > >index 38321d19efbe..454d896390c0 100755
>> > > >--- a/t/t5510-fetch.sh
>> > > >+++ b/t/t5510-fetch.sh
>> > > >@@ -682,6 +682,7 @@ test_expect_success 'fetching with auto-gc does
>> > > >not lock up' '
>> > > > (
>> > > >  cd auto-gc &&
>> > > >  git config gc.autoPackLimit 1 &&
>> > > >+		git config gc.autoDetach false &&
>> > > >  GIT_ASK_YESNO="$D/askyesno" git fetch >fetch.out 2>&1 &&
>> > > >  ! grep "Should I try again" fetch.out
>> > > > )
>> > >
>> > >Sounds good to me.
>> >
>> >There is something still bothering me, though.
>> >
>> >I take this was a Windows-specific issue; deleting open files on Linux is
>> >no brainer.  According to a comment on the original Git for Windows issue
>> >at github[1], 'git gc' runs in the background by default on  
>> Windows as well.
>>
>> Ok, having slept on it, it was a false alarm.
>>
>> Though 'git gc --auto' claims "Auto packing the repository in background for
>> optimum performance." on Windows, it does in fact runs in the foreground.
>
> Thanks for checking. I ran out of time yesterday.
>
>> 'git gc --auto' first prints that message, unless gc.autoDetach is disabled,
>> and then calls daemonize() to go to the background.  However, daemonize() is
>> basically a no-op on Windows, thus 'git gc' will remain in the  
>> foreground and
>> the sequence I described below is impossible.  Good.
>
> Oh, right. I think this will take a lot of energy to fix: daemonize()'s
> functionality is not really available, indeed. What *is* available is a
> spawn() that detaches the new process.
>
>> Perhaps it would be worth updating 'git gc' to not lie about going to the
>> background when we can already know in advance that it won't.
>
> Hmm.  https://github.com/git/git/blob/master/builtin/gc.c#L372-L373
> already looks correct (should it really know that we cannot daemonize()?
>
> 			if (detach_auto)
> 				fprintf(stderr, _("Auto packing the repository in background for  
> optimum performance.\n"));
> 			else
> 				fprintf(stderr, _("Auto packing the repository for optimum  
> performance.\n"));

It is only correct as far as the value of the 'gc.autoDetach' config variable is concerned, which is enabled by default, even on Windows, where it can't go to the background.

> (should it really know that we cannot daemonize()?
> What about other code paths using that function?):

daemonize() is currently only used in the 'git gc --auto' code path and in 'git daemon', which doesn't announce that it is about to go in the background. Then there is the WIP index-helper, which will use daemonize() as well, but it won't announce that either.

Being the sole callsite that makes such an announcement, I think it should know or at least should check whether "daemonizing" is possible. (as opposed to e.g. teaching daemonize() to print such messages)

Gábor
SZEDER Gábor· May 5, 2016, 15:16 UTC · re: SZEDER Gábor · lore

[RFC PATCH] gc --auto: don't lie about running in background on Windows

When 'git gc --auto' is invoked it tells the user whether it is about to auto pack the repository in the background or in the foreground depending on 'gc.autoDetach' (enabled by default). However, going to the background is not supported at all on Windows, yet 'git gc --auto' by default claims that it will do so.

Add a helper function that can tell whether the platform supports going to the background and use it in the 'git gc --auto' codepath to print an honest message.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---

I built and tested with 'NO_POSIX_GOODIES=UnfortunatelyYes': it worked. However, I did not test it on Windows due to lack of resources, hence the RFC out of caution.

Arguably this helper function could be just a simple variable. I opted for a function because:

  - I preferred a single '#ifdef NO_POSIX_GOODIES', and putting a
    static variable so near to EOF felt just wrong.  (And this is why
    it's not an inline-able function defined in a header file.)
  - currently we know already at compile time that Windows can't
    daemonize, but in the future we might want to extend this helper
    function to perform some runtime checks, too.  But this is perhaps
    like preparing for crossing a bridge where we'll never get to.
 builtin/gc.c |  1 +
 cache.h      |  1 +
 setup.c      | 17 +++++++++++++++--
 3 files changed, 17 insertions(+), 2 deletions(-)
Show changes to 3 files +17 −2

builtin/gc.c, cache.h, setup.c

diff --git a/builtin/gc.c b/builtin/gc.c
index c583aad6ec28..79a0f6dc1126 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -368,6 +368,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		 */
 		if (!need_to_gc())
 			return 0;
+		detach_auto &= can_daemonize();
 		if (!quiet) {
 			if (detach_auto)
 				fprintf(stderr, _("Auto packing the repository in background for optimum performance.\n"));
diff --git a/cache.h b/cache.h
index fd728f079320..3662e5aabb98 100644
--- a/cache.h
+++ b/cache.h
@@ -524,6 +524,7 @@ extern int set_git_dir_init(const char *git_dir, const char *real_git_dir, int);
 extern int init_db(const char *template_dir, unsigned int flags);
 
 extern void sanitize_stdfds(void);
+extern int can_daemonize(void);
 extern int daemonize(void);
 
 #define alloc_nr(x) (((x)+16)*3/2)
diff --git a/setup.c b/setup.c
index c86bf5c9fabe..6187a4ad9c47 100644
--- a/setup.c
+++ b/setup.c
@@ -1033,12 +1033,25 @@ void sanitize_stdfds(void)
 		close(fd);
 }
 
+#ifdef NO_POSIX_GOODIES
+int can_daemonize(void)
+{
+	return 0;
+}
+
 int daemonize(void)
 {
-#ifdef NO_POSIX_GOODIES
 	errno = ENOSYS;
 	return -1;
+}
 #else
+int can_daemonize(void)
+{
+	return 1;
+}
+
+int daemonize(void)
+{
 	switch (fork()) {
 		case 0:
 			break;
@@ -1054,5 +1067,5 @@ int daemonize(void)
 	close(2);
 	sanitize_stdfds();
 	return 0;
-#endif
 }
+#endif /* #ifdef NO_POSIX_GOODIES */

-- 
2.8.2.356.ge684b1d
Junio C Hamano· May 5, 2016, 16:28 UTC · re: SZEDER Gábor · lore

Re: [RFC PATCH] gc --auto: don't lie about running in background on Windows

SZEDER Gábor <szeder@ira.uka.de> writes:
Show 11 quoted lines
> Arguably this helper function could be just a simple variable.  I
> opted for a function because:
>
>   - I preferred a single '#ifdef NO_POSIX_GOODIES', and putting a
>     static variable so near to EOF felt just wrong.  (And this is why
>     it's not an inline-able function defined in a header file.)
>
>   - currently we know already at compile time that Windows can't
>     daemonize, but in the future we might want to extend this helper
>     function to perform some runtime checks, too.  But this is perhaps
>     like preparing for crossing a bridge where we'll never get to.

Alternatively, the implementation of daemonize() and can_daemonize() can live in compat/ and have the #ifdef switch in git-compat-util.h, e.g. something along the lines of these:

	<< git-compat-util.h >>
	... after conditional inclusion of compat/mingw.h ...
	#ifndef can_daemonize
        #define can_daemonize() 1
	#endif
        
	<< compat/mingw.h >>
	#define can_daemonize() 0
	#define daemonize() mingw_daemonize()
	<< setup.c >>
        ...
        #ifndef NO_POSIX_GOODIES
        int daemonize(void)
        {
            ... no ifdef around here ...
	}
	#endif

We can be even more purist and move the daemonize() implementation for POSIX to compat/posix.c to keep the generic part even more platform agnostic, which would remove the only #ifdef in setup.c, but that might be going a bit too far.

These possible implementation choices aside, the goal of this patch is a worthwhile thing to do, I would think.

Thanks.
Show 67 quoted lines
>  builtin/gc.c |  1 +
>  cache.h      |  1 +
>  setup.c      | 17 +++++++++++++++--
>  3 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index c583aad6ec28..79a0f6dc1126 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -368,6 +368,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>  		 */
>  		if (!need_to_gc())
>  			return 0;
> +		detach_auto &= can_daemonize();
>  		if (!quiet) {
>  			if (detach_auto)
>  				fprintf(stderr, _("Auto packing the repository in background for optimum performance.\n"));
> diff --git a/cache.h b/cache.h
> index fd728f079320..3662e5aabb98 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -524,6 +524,7 @@ extern int set_git_dir_init(const char *git_dir, const char *real_git_dir, int);
>  extern int init_db(const char *template_dir, unsigned int flags);
>  
>  extern void sanitize_stdfds(void);
> +extern int can_daemonize(void);
>  extern int daemonize(void);
>  
>  #define alloc_nr(x) (((x)+16)*3/2)
> diff --git a/setup.c b/setup.c
> index c86bf5c9fabe..6187a4ad9c47 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1033,12 +1033,25 @@ void sanitize_stdfds(void)
>  		close(fd);
>  }
>  
> +#ifdef NO_POSIX_GOODIES
> +int can_daemonize(void)
> +{
> +	return 0;
> +}
> +
>  int daemonize(void)
>  {
> -#ifdef NO_POSIX_GOODIES
>  	errno = ENOSYS;
>  	return -1;
> +}
>  #else
> +int can_daemonize(void)
> +{
> +	return 1;
> +}
> +
> +int daemonize(void)
> +{
>  	switch (fork()) {
>  		case 0:
>  			break;
> @@ -1054,5 +1067,5 @@ int daemonize(void)
>  	close(2);
>  	sanitize_stdfds();
>  	return 0;
> -#endif
>  }
> +#endif /* #ifdef NO_POSIX_GOODIES */
SZEDER Gábor· May 7, 2016, 14:44 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH] gc --auto: don't lie about running in background on Windows

Quoting Junio C Hamano <gitster@pobox.com>:
Show 36 quoted lines
> SZEDER Gábor <szeder@ira.uka.de> writes:
>
>> Arguably this helper function could be just a simple variable.  I
>> opted for a function because:
>>
>>   - I preferred a single '#ifdef NO_POSIX_GOODIES', and putting a
>>     static variable so near to EOF felt just wrong.  (And this is why
>>     it's not an inline-able function defined in a header file.)
>>
>>   - currently we know already at compile time that Windows can't
>>     daemonize, but in the future we might want to extend this helper
>>     function to perform some runtime checks, too.  But this is perhaps
>>     like preparing for crossing a bridge where we'll never get to.
>
> Alternatively, the implementation of daemonize() and can_daemonize()
> can live in compat/ and have the #ifdef switch in git-compat-util.h,
> e.g. something along the lines of these:
>
> 	<< git-compat-util.h >>
> 	... after conditional inclusion of compat/mingw.h ...
> 	#ifndef can_daemonize
>         #define can_daemonize() 1
> 	#endif
>
> 	<< compat/mingw.h >>
> 	#define can_daemonize() 0
> 	#define daemonize() mingw_daemonize()
>
> 	<< setup.c >>
>         ...
>         #ifndef NO_POSIX_GOODIES
>         int daemonize(void)
>         {
>             ... no ifdef around here ...
> 	}
> 	#endif

config.mak.uname sets NO_POSIX_GOODIES only for Windows builds, but NO_POSIX_GOODIES doesn't sound Windows-specific at all. Currently if somebody were to decide to build with NO_POSIX_GOODIES=UnfortunatelyYes, then he would get a working git in the end, even on non-Windows platforms. With the proposed alternative above we would only provide an alternative daemonize() implementation for MINGW, breaking the build of those setting NO_POSIX_GOODIES by themselves.

I don't know whether there are platforms out there besides Windows that need the NO_POSIX_GOODIES treatment, but we don't explicitly support them in config.mak.uname, or people who for whatever reason build with this knob turned on.

← back to recent threads