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

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

[SZEDER Gábor]

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(+)

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 686 bytes --]

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.

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

Ciao,
Dscho

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

[SZEDER Gábor]

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.

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

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

[SZEDER Gábor]

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.

'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.



> 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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2419 bytes --]

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()?
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

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

[SZEDER Gábor]

Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:

> 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

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

[SZEDER Gábor]

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

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

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

[Junio C Hamano]

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

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.

>  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 */

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

[SZEDER Gábor]

Quoting Junio C Hamano <gitster@pobox.com>:

> 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.