threads / patch / 41182

patch, 4 partsAlways release pack files before calling gc --auto

Subject: [PATCH 0/4] Always release pack files before calling gc --auto

## tl;dr

7 messages between Jan 13, 2016 and Jan 13, 2016. Diffs are folded; open one to read it.

replies: 6people: 2as markdown or json

Johannes Schindelin· Jan 13, 2016, 17:20 UTC · lore

These patches are companions to 786b150 (clone --dissociate: avoid locking pack files, 2015-10-06), triggered by a bug report to the Git for Windows project.

Johannes Schindelin (4):
  fetch: release pack files before garbage-collecting
  am: release pack files before garbage-collecting
  merge: release pack files before garbage-collecting
  receive-pack: release pack files before garbage-collecting

 builtin/am.c           |  1 +
 builtin/fetch.c        |  2 ++
 builtin/merge.c        |  1 +
 builtin/receive-pack.c |  1 +
 t/t5510-fetch.sh       | 13 +++++++++++++
 5 files changed, 18 insertions(+)

-- 
2.6.3.windows.1.300.g1c25e49

Johannes Schindelin· Jan 13, 2016, 17:20 UTC · re: Johannes Schindelin · lore

[PATCH 1/4] fetch: release pack files before garbage-collecting

Before auto-gc'ing, we need to make sure that the pack files are released in case they need to be repacked and garbage-collected.

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/fetch.c  |  2 ++
 t/t5510-fetch.sh | 13 +++++++++++++
 2 files changed, 15 insertions(+)

Show changes to 2 files +15 −0

builtin/fetch.c, t/t5510-fetch.sh

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 33f04c1..8e74213 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1225,6 +1225,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	list.strdup_strings = 1;
 	string_list_clear(&list, 0);
 
+	close_all_packs();
+
 	argv_array_pushl(&argv_gc_auto, "gc", "--auto", NULL);
 	if (verbosity < 0)
 		argv_array_push(&argv_gc_auto, "--quiet");
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 0ba9db0..e3ee4bd 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -708,4 +708,17 @@ test_expect_success 'fetching a one-level ref works' '
 	)
 '
 
+test_expect_success 'fetching with auto-gc does not lock up' '
+	write_script askyesno <<-\EOF &&
+	echo "$*" &&
+	false
+	EOF
+	git clone "file://$D" auto-gc &&
+	test_commit test2 &&
+	cd auto-gc &&
+	git config gc.autoPackLimit 1 &&
+	GIT_ASK_YESNO="$D/askyesno" git fetch >fetch.out 2>&1 &&
+	! grep "Should I try again" fetch.out
+'
+
 test_done

-- 
2.6.3.windows.1.300.g1c25e49

Johannes Schindelin· Jan 13, 2016, 17:20 UTC · re: Johannes Schindelin · lore

[PATCH 2/4] am: release pack files before garbage-collecting

Before auto-gc'ing, we need to make sure that the pack files are released in case they need to be repacked and garbage-collected.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/am.c | 1 +
 1 file changed, 1 insertion(+)

Show changes to builtin/am.c +1 −0

diff --git a/builtin/am.c b/builtin/am.c
index 9fb42fd..de235cf 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1939,6 +1939,7 @@ next:
 	 */
 	if (!state->rebasing) {
 		am_destroy(state);
+		close_all_packs();
 		run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
 	}
 }

-- 
2.6.3.windows.1.300.g1c25e49

Johannes Schindelin· Jan 13, 2016, 17:20 UTC · re: Johannes Schindelin · lore

[PATCH 3/4] merge: release pack files before garbage-collecting

Before auto-gc'ing, we need to make sure that the pack files are released in case they need to be repacked and garbage-collected.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/merge.c | 1 +
 1 file changed, 1 insertion(+)

Show changes to builtin/merge.c +1 −0

diff --git a/builtin/merge.c b/builtin/merge.c
index 15bf95b..b98a348 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -404,6 +404,7 @@ static void finish(struct commit *head_commit,
 			 * We ignore errors in 'gc --auto', since the
 			 * user should see them.
 			 */
+			close_all_packs();
 			run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
 		}
 	}

-- 
2.6.3.windows.1.300.g1c25e49

Johannes Schindelin· Jan 13, 2016, 17:20 UTC · re: Johannes Schindelin · lore

[PATCH 4/4] receive-pack: release pack files before garbage-collecting

Before auto-gc'ing, we need to make sure that the pack files are released in case they need to be repacked and garbage-collected.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/receive-pack.c | 1 +
 1 file changed, 1 insertion(+)

Show changes to builtin/receive-pack.c +1 −0

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 2b3b746..f2d6761 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1796,6 +1796,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 				"gc", "--auto", "--quiet", NULL,
 			};
 			int opt = RUN_GIT_CMD | RUN_COMMAND_STDOUT_TO_STDERR;
+			close_all_packs();
 			run_command_v_opt(argv_gc_auto, opt);
 		}
 		if (auto_update_server_info)

-- 
2.6.3.windows.1.300.g1c25e49

Jeff King· Jan 13, 2016, 17:52 UTC · re: Johannes Schindelin · lore

Re: [PATCH 0/4] Always release pack files before calling gc --auto

On Wed, Jan 13, 2016 at 06:20:02PM +0100, Johannes Schindelin wrote:

Show 9 quoted lines

> These patches are companions to 786b150 (clone --dissociate: avoid
> locking pack files, 2015-10-06), triggered by a bug report to the Git
> for Windows project.
> 
> Johannes Schindelin (4):
>   fetch: release pack files before garbage-collecting
>   am: release pack files before garbage-collecting
>   merge: release pack files before garbage-collecting
>   receive-pack: release pack files before garbage-collecting

I think this is fine, but I noticed an almost-problem that I think is worth mentioning.

Closing packs can open up race conditions in some cases. I.e., we think we have an object in a particular pack, do some work on that basis, and then later find the pack to be inaccessible (due to somebody else running gc). If we keep the fd open, there's no race, but if we close it, we can't reopen it.

I think all of the cases here are fine, for two reasons:

  1. The new closing is right before the program would exit anyway, so
     there's no interesting work left to do (and obviously that's where
     we generally want to call "gc --auto", too)

  2. This type of race might only be an issue for pack-objects (or at least that's
     the only place I've seen it).  Most normal read_sha1_file() callers
     will happily re-scan the pack directory for the new packs upon
     finding that the old pack went away. But pack-objects peeks in the
     packfiles a little more intimately, and will record the actual pack
     and offset (for delta reuse).

     It _might_ also be possible to trigger this race using bitmaps,
     which are also pretty intimate with the packfile code. I'm not
     sure.

So I think this series is OK.

-Peff

Johannes Schindelin· Jan 13, 2016, 18:52 UTC · re: Jeff King · lore

Re: [PATCH 0/4] Always release pack files before calling gc --auto

Hi Peff,

On Wed, 13 Jan 2016, Jeff King wrote:

Show 19 quoted lines

> On Wed, Jan 13, 2016 at 06:20:02PM +0100, Johannes Schindelin wrote:
> 
> > These patches are companions to 786b150 (clone --dissociate: avoid
> > locking pack files, 2015-10-06), triggered by a bug report to the Git
> > for Windows project.
> > 
> > Johannes Schindelin (4):
> >   fetch: release pack files before garbage-collecting
> >   am: release pack files before garbage-collecting
> >   merge: release pack files before garbage-collecting
> >   receive-pack: release pack files before garbage-collecting
> 
> [...]
>
> I think all of the cases here are fine, for two reasons:
> 
>   1. The new closing is right before the program would exit anyway, so
>      there's no interesting work left to do (and obviously that's where
>      we generally want to call "gc --auto", too)

This is actually what I checked, but of course I managed to forget to mention it in the cover letter :-(

Thanks for verifying, and for spelling it out. Dscho