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

Subsystems: the rest

STALE3012d

5 messages, 3 authors, 2018-07-09 · open the first message on its own page

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

From: Kim Gybels <hidden>
Date: 2018-06-30 13:46:35

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

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

From: Duy Nguyen <hidden>
Date: 2018-06-30 14:58:58

On Sat, Jun 30, 2018 at 03:38:21PM +0200, Kim Gybels wrote:

quoted hunk

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

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

quoted hunk

 		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

From: Kim Gybels <hidden>
Date: 2018-07-04 20:16:29

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

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 v2] gc --auto: clear repository before auto packing

From: SZEDER Gábor <hidden>
Date: 2018-07-07 09:58:10

On Wed, Jul 4, 2018 at 10:16 PM Kim Gybels [off-list ref] wrote:

quoted hunk

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

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

From: Kim Gybels <hidden>
Date: 2018-07-09 20:38:07

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

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help