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

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

[Johannes Schindelin]

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

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

[Johannes Schindelin]

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

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

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

[Johannes Schindelin]

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

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

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

[Johannes Schindelin]

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

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

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

[Johannes Schindelin]

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

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

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

[Jeff King]

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

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

[Johannes Schindelin]

Hi Peff,

On Wed, 13 Jan 2016, Jeff King wrote:

> 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