git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH] gc --auto: release pack files before auto packing
@ 2018-06-30 13:38 Kim Gybels
  2018-06-30 14:58 ` Duy Nguyen
  2018-07-04 20:16 ` [PATCH v2] gc --auto: clear repository " Kim Gybels
  0 siblings, 2 replies; 11+ messages in thread
From: Kim Gybels @ 2018-06-30 13:38 UTC (permalink / raw)
  To: git
  Cc: Kim Gybels, Adam Borowski, Johannes Schindelin, Michael J Gruber,
	SZEDER Gábor, Nguyễn Thái Ngọc Duy,
	Junio C Hamano, Jeff King, Torsten Bögershausen

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gc --auto: release pack files before auto packing
  2018-06-30 13:38 [PATCH] gc --auto: release pack files before auto packing Kim Gybels
@ 2018-06-30 14:58 ` Duy Nguyen
  2018-07-06 16:39   ` Junio C Hamano
  2018-07-04 20:16 ` [PATCH v2] gc --auto: clear repository " Kim Gybels
  1 sibling, 1 reply; 11+ messages in thread
From: Duy Nguyen @ 2018-06-30 14:58 UTC (permalink / raw)
  To: Kim Gybels
  Cc: git, Adam Borowski, Johannes Schindelin, Michael J Gruber,
	SZEDER Gábor, Junio C Hamano, Jeff King,
	Torsten Bögershausen

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
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] gc --auto: clear repository before auto packing
  2018-06-30 13:38 [PATCH] gc --auto: release pack files before auto packing Kim Gybels
  2018-06-30 14:58 ` Duy Nguyen
@ 2018-07-04 20:16 ` " Kim Gybels
  2018-07-07  9:57   ` SZEDER Gábor
  2018-07-09 20:37   ` [PATCH v3] gc --auto: release pack files " Kim Gybels
  1 sibling, 2 replies; 11+ messages in thread
From: Kim Gybels @ 2018-07-04 20:16 UTC (permalink / raw)
  To: git
  Cc: Kim Gybels, Adam Borowski, Johannes Schindelin, Michael J Gruber,
	SZEDER Gábor, Nguyễn Thái Ngọc Duy,
	Junio C Hamano, Jeff King, Torsten Bögershausen

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gc --auto: release pack files before auto packing
  2018-06-30 14:58 ` Duy Nguyen
@ 2018-07-06 16:39   ` Junio C Hamano
  2018-07-07  9:40     ` SZEDER Gábor
  0 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2018-07-06 16:39 UTC (permalink / raw)
  To: Duy Nguyen
  Cc: Kim Gybels, git, Adam Borowski, Johannes Schindelin,
	Michael J Gruber, SZEDER Gábor, Jeff King,
	Torsten Bögershausen

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gc --auto: release pack files before auto packing
  2018-07-06 16:39   ` Junio C Hamano
@ 2018-07-07  9:40     ` SZEDER Gábor
  2018-07-07 23:16       ` Kim Gybels
  0 siblings, 1 reply; 11+ messages in thread
From: SZEDER Gábor @ 2018-07-07  9:40 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Nguyễn Thái Ngọc Duy, kgybels, Git mailing list,
	kilobyte, Johannes Schindelin, Michael J Gruber, Jeff King,
	tboegi

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] gc --auto: clear repository before auto packing
  2018-07-04 20:16 ` [PATCH v2] gc --auto: clear repository " Kim Gybels
@ 2018-07-07  9:57   ` SZEDER Gábor
  2018-07-09 20:37   ` [PATCH v3] gc --auto: release pack files " Kim Gybels
  1 sibling, 0 replies; 11+ messages in thread
From: SZEDER Gábor @ 2018-07-07  9:57 UTC (permalink / raw)
  To: kgybels
  Cc: Git mailing list, kilobyte, Johannes Schindelin,
	Michael J Gruber, Nguyễn Thái Ngọc Duy,
	Junio C Hamano, Jeff King, tboegi

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
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gc --auto: release pack files before auto packing
  2018-07-07  9:40     ` SZEDER Gábor
@ 2018-07-07 23:16       ` Kim Gybels
  2018-07-09 14:33         ` Duy Nguyen
  0 siblings, 1 reply; 11+ messages in thread
From: Kim Gybels @ 2018-07-07 23:16 UTC (permalink / raw)
  To: SZEDER Gábor
  Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy,
	Git mailing list, kilobyte, Johannes Schindelin,
	Michael J Gruber, Jeff King, tboegi

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gc --auto: release pack files before auto packing
  2018-07-07 23:16       ` Kim Gybels
@ 2018-07-09 14:33         ` Duy Nguyen
  2018-07-09 21:10           ` Junio C Hamano
  0 siblings, 1 reply; 11+ messages in thread
From: Duy Nguyen @ 2018-07-09 14:33 UTC (permalink / raw)
  To: Kim Gybels
  Cc: SZEDER Gábor, Junio C Hamano, Git Mailing List,
	Adam Borowski, Johannes Schindelin, Michael J Gruber, Jeff King,
	Torsten Bögershausen

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3] gc --auto: release pack files before auto packing
  2018-07-04 20:16 ` [PATCH v2] gc --auto: clear repository " Kim Gybels
  2018-07-07  9:57   ` SZEDER Gábor
@ 2018-07-09 20:37   ` " Kim Gybels
  1 sibling, 0 replies; 11+ messages in thread
From: Kim Gybels @ 2018-07-09 20:37 UTC (permalink / raw)
  To: git
  Cc: Kim Gybels, Adam Borowski, Johannes Schindelin, Michael J Gruber,
	SZEDER Gábor, Nguyễn Thái Ngọc Duy,
	Junio C Hamano, Jeff King, Torsten Bögershausen

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gc --auto: release pack files before auto packing
  2018-07-09 14:33         ` Duy Nguyen
@ 2018-07-09 21:10           ` Junio C Hamano
  2018-07-11 15:33             ` Duy Nguyen
  0 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2018-07-09 21:10 UTC (permalink / raw)
  To: Duy Nguyen
  Cc: Kim Gybels, SZEDER Gábor, Git Mailing List, Adam Borowski,
	Johannes Schindelin, Michael J Gruber, Jeff King,
	Torsten Bögershausen

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?



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gc --auto: release pack files before auto packing
  2018-07-09 21:10           ` Junio C Hamano
@ 2018-07-11 15:33             ` Duy Nguyen
  0 siblings, 0 replies; 11+ messages in thread
From: Duy Nguyen @ 2018-07-11 15:33 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Kim Gybels, SZEDER Gábor, Git Mailing List, Adam Borowski,
	Johannes Schindelin, Michael J Gruber, Jeff King,
	Torsten Bögershausen

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-30 13:38 [PATCH] gc --auto: release pack files before auto packing Kim Gybels
2018-06-30 14:58 ` Duy Nguyen
2018-07-06 16:39   ` Junio C Hamano
2018-07-07  9:40     ` SZEDER Gábor
2018-07-07 23:16       ` Kim Gybels
2018-07-09 14:33         ` Duy Nguyen
2018-07-09 21:10           ` Junio C Hamano
2018-07-11 15:33             ` Duy Nguyen
2018-07-04 20:16 ` [PATCH v2] gc --auto: clear repository " Kim Gybels
2018-07-07  9:57   ` SZEDER Gábor
2018-07-09 20:37   ` [PATCH v3] gc --auto: release pack files " Kim Gybels

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox