[PATCH] gc --auto: release pack files before auto packing

[PATCH] gc --auto: release pack files before auto packing

[Kim Gybels]

Teach gc --auto to release pack files before auto packing the repository
to prevent failures when removing them.

Also teach the test 'fetching with auto-gc does not lock up' to complain
when it is no longer triggering an auto packing of the repository.

Fixes https://github.com/git-for-windows/git/issues/500

Signed-off-by: Kim Gybels <kgybels@infogroep.be>
---

Patch based on master, since problem doesn't reproduce on maint,
however, the improvement to the test might be valuable on maint.

 builtin/gc.c     | 1 +
 t/t5510-fetch.sh | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/builtin/gc.c b/builtin/gc.c
index ccfb1ceaeb..63b62ab57c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -612,6 +612,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		return -1;
 
 	if (!repository_format_precious_objects) {
+		close_all_packs(the_repository->objects);
 		if (run_command_v_opt(repack.argv, RUN_GIT_CMD))
 			return error(FAILED_RUN, repack.argv[0]);
 
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index e402aee6a2..b91637e48f 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -828,9 +828,11 @@ test_expect_success 'fetching with auto-gc does not lock up' '
 	test_commit test2 &&
 	(
 		cd auto-gc &&
+		git config fetch.unpackLimit 1 &&
 		git config gc.autoPackLimit 1 &&
 		git config gc.autoDetach false &&
 		GIT_ASK_YESNO="$D/askyesno" git fetch >fetch.out 2>&1 &&
+		grep "Auto packing the repository" fetch.out &&
 		! grep "Should I try again" fetch.out
 	)
 '
-- 
2.18.0.windows.1

Re: [PATCH] gc --auto: release pack files before auto packing

[Duy Nguyen]

On Sat, Jun 30, 2018 at 03:38:21PM +0200, Kim Gybels wrote:
> Teach gc --auto to release pack files before auto packing the repository
> to prevent failures when removing them.
> 
> Also teach the test 'fetching with auto-gc does not lock up' to complain
> when it is no longer triggering an auto packing of the repository.
> 
> Fixes https://github.com/git-for-windows/git/issues/500
> 
> Signed-off-by: Kim Gybels <kgybels@infogroep.be>
> ---
> 
> Patch based on master, since problem doesn't reproduce on maint,
> however, the improvement to the test might be valuable on maint.
> 
>  builtin/gc.c     | 1 +
>  t/t5510-fetch.sh | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/builtin/gc.c b/builtin/gc.c
> index ccfb1ceaeb..63b62ab57c 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -612,6 +612,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>  		return -1;
>  
>  	if (!repository_format_precious_objects) {
> +		close_all_packs(the_repository->objects);

We have repo_clear() which does this and potentially closing file
descriptors on other things as well. I suggest we use it, and before
any external command is run. Something like

diff --git a/builtin/gc.c b/builtin/gc.c
index ccfb1ceaeb..a852c71da6 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -473,6 +473,13 @@ static int report_last_gc_error(void)
 
 static int gc_before_repack(void)
 {
+	/*
+	 * Shut down everything, we should have all the info we need
+	 * at this point. Leaving some file descriptors open may
+	 * prevent them from being removed on Windows.
+	 */
+	repo_clear(the_repository);
+
 	if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
 		return error(FAILED_RUN, pack_refs_cmd.argv[0]);
 


>  		if (run_command_v_opt(repack.argv, RUN_GIT_CMD))
>  			return error(FAILED_RUN, repack.argv[0]);
>  
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index e402aee6a2..b91637e48f 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -828,9 +828,11 @@ test_expect_success 'fetching with auto-gc does not lock up' '
>  	test_commit test2 &&
>  	(
>  		cd auto-gc &&
> +		git config fetch.unpackLimit 1 &&
>  		git config gc.autoPackLimit 1 &&
>  		git config gc.autoDetach false &&
>  		GIT_ASK_YESNO="$D/askyesno" git fetch >fetch.out 2>&1 &&
> +		grep "Auto packing the repository" fetch.out &&

Not sure if this should be test_i18ngrep

>  		! grep "Should I try again" fetch.out
>  	)
>  '
> -- 
> 2.18.0.windows.1
> 

[PATCH v2] gc --auto: clear repository before auto packing

[Kim Gybels]

Teach gc --auto to clear the repository before auto packing it to
prevent failures when removing files on Windows.

Also teach the test 'fetching with auto-gc does not lock up' to complain
when it is no longer triggering an auto packing of the repository.

Fixes https://github.com/git-for-windows/git/issues/500

Signed-off-by: Kim Gybels <kgybels@infogroep.be>
---

Updated after Duy Nguyen's comments:
- use repo_clear instead of close_all_packs, and add the call in
  gc_before_repack intead of just before executing git repack
- use test_i18ngrep instead of grep in updated test

 builtin/gc.c     | 7 +++++++
 t/t5510-fetch.sh | 4 +++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index ccfb1ceaeb..22a6fc4863 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -473,6 +473,13 @@ static int report_last_gc_error(void)
 
 static int gc_before_repack(void)
 {
+	/*
+	 * Shut down everything, we should have all the info we need
+	 * at this point. Leaving some file descriptors open may
+	 * prevent them from being removed on Windows.
+	 */
+	repo_clear(the_repository);
+
 	if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
 		return error(FAILED_RUN, pack_refs_cmd.argv[0]);
 
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index e402aee6a2..ef599c11cd 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -828,10 +828,12 @@ test_expect_success 'fetching with auto-gc does not lock up' '
 	test_commit test2 &&
 	(
 		cd auto-gc &&
+		git config fetch.unpackLimit 1 &&
 		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
+		test_i18ngrep "Auto packing the repository" fetch.out &&
+		test_i18ngrep ! "Should I try again" fetch.out
 	)
 '
 
-- 
2.18.0.windows.1

Re: [PATCH] gc --auto: release pack files before auto packing

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Sat, Jun 30, 2018 at 03:38:21PM +0200, Kim Gybels wrote:
>> Teach gc --auto to release pack files before auto packing the repository
>> to prevent failures when removing them.
>> 
>> Also teach the test 'fetching with auto-gc does not lock up' to complain
>> when it is no longer triggering an auto packing of the repository.
>> 
>> Fixes https://github.com/git-for-windows/git/issues/500
>> 
>> Signed-off-by: Kim Gybels <kgybels@infogroep.be>
>> ---
>> 
>> Patch based on master, since problem doesn't reproduce on maint,
>> however, the improvement to the test might be valuable on maint.
>> 
>>  builtin/gc.c     | 1 +
>>  t/t5510-fetch.sh | 2 ++
>>  2 files changed, 3 insertions(+)
>> 
>> diff --git a/builtin/gc.c b/builtin/gc.c
>> index ccfb1ceaeb..63b62ab57c 100644
>> --- a/builtin/gc.c
>> +++ b/builtin/gc.c
>> @@ -612,6 +612,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>>  		return -1;
>>  
>>  	if (!repository_format_precious_objects) {
>> +		close_all_packs(the_repository->objects);
>
> We have repo_clear() which does this and potentially closing file
> descriptors on other things as well. I suggest we use it, and before
> any external command is run. Something like
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index ccfb1ceaeb..a852c71da6 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -473,6 +473,13 @@ static int report_last_gc_error(void)
>  
>  static int gc_before_repack(void)
>  {
> +	/*
> +	 * Shut down everything, we should have all the info we need
> +	 * at this point. Leaving some file descriptors open may
> +	 * prevent them from being removed on Windows.
> +	 */
> +	repo_clear(the_repository);
> +
>  	if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
>  		return error(FAILED_RUN, pack_refs_cmd.argv[0]);

With 

  https://public-inbox.org/git/20180509170409.13666-1-pclouds@gmail.com/

the call to repo_clear() on the_repository itself may be safe [*1*],
but if command line parsing code etc. has pointers to in-core object
etc. obtained before this function was called, the codeflow after
this function returns will be quite disturbed.  Has anybody in this
review thread audited the codeflow _after_ this function is run to
make sure that invalidating in-core repository here does not cause
us problems?  Just checking, because I won't queue this change until
I hear that somebody familiar with the codepath actually did so (I
may get around to do so myself later).

Thanks.


[Footnote]

*1* ... and of course changing the_index to &the_repo->index would
    make it even safer ;-)

Re: [PATCH] gc --auto: release pack files before auto packing

[SZEDER Gábor]

On Fri, Jul 6, 2018 at 6:39 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Duy Nguyen <pclouds@gmail.com> writes:
>
> > On Sat, Jun 30, 2018 at 03:38:21PM +0200, Kim Gybels wrote:
> >> Teach gc --auto to release pack files before auto packing the repository
> >> to prevent failures when removing them.
> >>
> >> Also teach the test 'fetching with auto-gc does not lock up' to complain
> >> when it is no longer triggering an auto packing of the repository.
> >>
> >> Fixes https://github.com/git-for-windows/git/issues/500
> >>
> >> Signed-off-by: Kim Gybels <kgybels@infogroep.be>
> >> ---
> >>
> >> Patch based on master, since problem doesn't reproduce on maint,
> >> however, the improvement to the test might be valuable on maint.
> >>
> >>  builtin/gc.c     | 1 +
> >>  t/t5510-fetch.sh | 2 ++
> >>  2 files changed, 3 insertions(+)
> >>
> >> diff --git a/builtin/gc.c b/builtin/gc.c
> >> index ccfb1ceaeb..63b62ab57c 100644
> >> --- a/builtin/gc.c
> >> +++ b/builtin/gc.c
> >> @@ -612,6 +612,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
> >>              return -1;
> >>
> >>      if (!repository_format_precious_objects) {
> >> +            close_all_packs(the_repository->objects);
> >
> > We have repo_clear() which does this and potentially closing file
> > descriptors on other things as well. I suggest we use it, and before
> > any external command is run. Something like
> >
> > diff --git a/builtin/gc.c b/builtin/gc.c
> > index ccfb1ceaeb..a852c71da6 100644
> > --- a/builtin/gc.c
> > +++ b/builtin/gc.c
> > @@ -473,6 +473,13 @@ static int report_last_gc_error(void)
> >
> >  static int gc_before_repack(void)
> >  {
> > +     /*
> > +      * Shut down everything, we should have all the info we need
> > +      * at this point. Leaving some file descriptors open may
> > +      * prevent them from being removed on Windows.
> > +      */
> > +     repo_clear(the_repository);
> > +
> >       if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
> >               return error(FAILED_RUN, pack_refs_cmd.argv[0]);
>
> With
>
>   https://public-inbox.org/git/20180509170409.13666-1-pclouds@gmail.com/
>
> the call to repo_clear() on the_repository itself may be safe [*1*],
> but if command line parsing code etc. has pointers to in-core object
> etc. obtained before this function was called, the codeflow after
> this function returns will be quite disturbed.  Has anybody in this
> review thread audited the codeflow _after_ this function is run to
> make sure that invalidating in-core repository here does not cause
> us problems?

It does create us problems, unfortunately.

Among other things, a call to repo_clear(the_repository) does this:

  raw_object_store_clear(repo->objects);
  FREE_AND_NULL(repo->objects);

Later on cmd_gc() calls reprepare_packed_git(the_repository), which
then attempts to:

  r->objects->approximate_object_count_valid = 0;
  r->objects->packed_git_initialized = 0;

but with r->objects being NULL at this point that won't work, and in
turn causes failures in several test scripts.


FWIW, the original patch appears to be working fine, at least the test
suite passes.

Re: [PATCH v2] gc --auto: clear repository before auto packing

[SZEDER Gábor]

On Wed, Jul 4, 2018 at 10:16 PM Kim Gybels <kgybels@infogroep.be> wrote:

> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index e402aee6a2..ef599c11cd 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -828,10 +828,12 @@ test_expect_success 'fetching with auto-gc does not lock up' '
>         test_commit test2 &&
>         (
>                 cd auto-gc &&
> +               git config fetch.unpackLimit 1 &&
>                 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
> +               test_i18ngrep "Auto packing the repository" fetch.out &&
> +               test_i18ngrep ! "Should I try again" fetch.out

The messages containing "Auto packing the repository" are indeed
translated, thus they have to be checked with 'test_i18ngrep', good.

However, none of the "Should I try again" messages are translated, so
checking them with bare 'grep' is fine and it shouldn't be changed.


>         )
>  '
>
> --
> 2.18.0.windows.1
>

Re: [PATCH] gc --auto: release pack files before auto packing

[Kim Gybels]

On (07/07/18 11:40), SZEDER Gábor wrote:
> 
> On Fri, Jul 6, 2018 at 6:39 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Duy Nguyen <pclouds@gmail.com> writes:
> >
> > > On Sat, Jun 30, 2018 at 03:38:21PM +0200, Kim Gybels wrote:
> > >> Teach gc --auto to release pack files before auto packing the repository
> > >> to prevent failures when removing them.
> > >>
> > >> Also teach the test 'fetching with auto-gc does not lock up' to complain
> > >> when it is no longer triggering an auto packing of the repository.
> > >>
> > >> Fixes https://github.com/git-for-windows/git/issues/500
> > >>
> > >> Signed-off-by: Kim Gybels <kgybels@infogroep.be>
> > >> ---
> > >>
> > >> Patch based on master, since problem doesn't reproduce on maint,
> > >> however, the improvement to the test might be valuable on maint.
> > >>
> > >>  builtin/gc.c     | 1 +
> > >>  t/t5510-fetch.sh | 2 ++
> > >>  2 files changed, 3 insertions(+)
> > >>
> > >> diff --git a/builtin/gc.c b/builtin/gc.c
> > >> index ccfb1ceaeb..63b62ab57c 100644
> > >> --- a/builtin/gc.c
> > >> +++ b/builtin/gc.c
> > >> @@ -612,6 +612,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
> > >>              return -1;
> > >>
> > >>      if (!repository_format_precious_objects) {
> > >> +            close_all_packs(the_repository->objects);
> > >
> > > We have repo_clear() which does this and potentially closing file
> > > descriptors on other things as well. I suggest we use it, and before
> > > any external command is run. Something like
> > >
> > > diff --git a/builtin/gc.c b/builtin/gc.c
> > > index ccfb1ceaeb..a852c71da6 100644
> > > --- a/builtin/gc.c
> > > +++ b/builtin/gc.c
> > > @@ -473,6 +473,13 @@ static int report_last_gc_error(void)
> > >
> > >  static int gc_before_repack(void)
> > >  {
> > > +     /*
> > > +      * Shut down everything, we should have all the info we need
> > > +      * at this point. Leaving some file descriptors open may
> > > +      * prevent them from being removed on Windows.
> > > +      */
> > > +     repo_clear(the_repository);
> > > +
> > >       if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
> > >               return error(FAILED_RUN, pack_refs_cmd.argv[0]);
> >
> > With
> >
> >   https://public-inbox.org/git/20180509170409.13666-1-pclouds@gmail.com/
> >
> > the call to repo_clear() on the_repository itself may be safe [*1*],
> > but if command line parsing code etc. has pointers to in-core object
> > etc. obtained before this function was called, the codeflow after
> > this function returns will be quite disturbed.  Has anybody in this
> > review thread audited the codeflow _after_ this function is run to
> > make sure that invalidating in-core repository here does not cause
> > us problems?
> 
> It does create us problems, unfortunately.
> 
> Among other things, a call to repo_clear(the_repository) does this:
> 
>   raw_object_store_clear(repo->objects);
>   FREE_AND_NULL(repo->objects);
> 
> Later on cmd_gc() calls reprepare_packed_git(the_repository), which
> then attempts to:
> 
>   r->objects->approximate_object_count_valid = 0;
>   r->objects->packed_git_initialized = 0;
> 
> but with r->objects being NULL at this point that won't work, and in
> turn causes failures in several test scripts.
> 
> 
> FWIW, the original patch appears to be working fine, at least the test
> suite passes.

Should I post a v3 that goes back to the original fix, but uses
test_i18ngrep instead of grep?

In addition to not breaking any tests, close_all_packs is already used
in a similar way in am and fetch just before running "gc --auto".

-Kim

Re: [PATCH] gc --auto: release pack files before auto packing

[Duy Nguyen]

On Sun, Jul 8, 2018 at 1:16 AM Kim Gybels <kgybels@infogroep.be> wrote:
> Should I post a v3 that goes back to the original fix, but uses
> test_i18ngrep instead of grep?

Yes please. In my comment I did write we didn't need the repo anymore
(or something along that line) which turns out to be wrong.

> In addition to not breaking any tests, close_all_packs is already used
> in a similar way in am and fetch just before running "gc --auto".
>
> -Kim



-- 
Duy

[PATCH v3] gc --auto: release pack files before auto packing

[Kim Gybels]

Teach gc --auto to release pack files before auto packing the repository
to prevent failures when removing them.

Also teach the test 'fetching with auto-gc does not lock up' to complain
when it is no longer triggering an auto packing of the repository.

Fixes https://github.com/git-for-windows/git/issues/500

Signed-off-by: Kim Gybels <kgybels@infogroep.be>
---

Changes since v2:
- revert fix back to v1: use close_all_packs instead of repo_clear
- use test_i18ngrep only for translated string

 builtin/gc.c     | 1 +
 t/t5510-fetch.sh | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/builtin/gc.c b/builtin/gc.c
index ccfb1ceaeb..63b62ab57c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -612,6 +612,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		return -1;
 
 	if (!repository_format_precious_objects) {
+		close_all_packs(the_repository->objects);
 		if (run_command_v_opt(repack.argv, RUN_GIT_CMD))
 			return error(FAILED_RUN, repack.argv[0]);
 
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index e402aee6a2..6f1450eb69 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -828,9 +828,11 @@ test_expect_success 'fetching with auto-gc does not lock up' '
 	test_commit test2 &&
 	(
 		cd auto-gc &&
+		git config fetch.unpackLimit 1 &&
 		git config gc.autoPackLimit 1 &&
 		git config gc.autoDetach false &&
 		GIT_ASK_YESNO="$D/askyesno" git fetch >fetch.out 2>&1 &&
+		test_i18ngrep "Auto packing the repository" fetch.out &&
 		! grep "Should I try again" fetch.out
 	)
 '
-- 
2.18.0.windows.1

Re: [PATCH] gc --auto: release pack files before auto packing

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Sun, Jul 8, 2018 at 1:16 AM Kim Gybels <kgybels@infogroep.be> wrote:
>> Should I post a v3 that goes back to the original fix, but uses
>> test_i18ngrep instead of grep?
>
> Yes please. In my comment I did write we didn't need the repo anymore
> (or something along that line) which turns out to be wrong.
>
>> In addition to not breaking any tests, close_all_packs is already used
>> in a similar way in am and fetch just before running "gc --auto".
>>
>> -Kim

Sound good.  

I recall that "clear repo should treat the_repository special" was
discussed when we saw the patch that became 74373b5f ("repository:
fix free problem with repo_clear(the_repository)", 2018-05-10),
instead of treating only the index portion specially.  Perhaps it
was a more correct approach after all?

Re: [PATCH] gc --auto: release pack files before auto packing

[Duy Nguyen]

On Mon, Jul 9, 2018 at 11:10 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Duy Nguyen <pclouds@gmail.com> writes:
>
> > On Sun, Jul 8, 2018 at 1:16 AM Kim Gybels <kgybels@infogroep.be> wrote:
> >> Should I post a v3 that goes back to the original fix, but uses
> >> test_i18ngrep instead of grep?
> >
> > Yes please. In my comment I did write we didn't need the repo anymore
> > (or something along that line) which turns out to be wrong.
> >
> >> In addition to not breaking any tests, close_all_packs is already used
> >> in a similar way in am and fetch just before running "gc --auto".
> >>
> >> -Kim
>
> Sound good.
>
> I recall that "clear repo should treat the_repository special" was
> discussed when we saw the patch that became 74373b5f ("repository:
> fix free problem with repo_clear(the_repository)", 2018-05-10),
> instead of treating only the index portion specially.  Perhaps it
> was a more correct approach after all?

I think it's good that we have a way to "shut down the repo" when we
run an external command. But what we lack is "reinitialize the repo"
after the external command is done. We could treat the_repository
special in this case so shutting down does not require
reinitialization. Then repo_clear() should work well here. We could
also add a flag in repo_clear() to say "release all the resources you
are holding, but keep the repo settings/location..., we're not done
with this repo yet" then we don't need to re-initialize the repo
afterwards and still don't make the_repository so special.
-- 
Duy