git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH 0/1] gc/repack: release packs when needed
@ 2018-12-15 22:04 Johannes Schindelin via GitGitGadget
  2018-12-15 22:04 ` [PATCH 1/1] " Johannes Schindelin via GitGitGadget
  0 siblings, 1 reply; 5+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-12-15 22:04 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

This fixes more "can't delete files while they are still open" issues on
Windows.

Johannes Schindelin (1):
  gc/repack: release packs when needed

 builtin/gc.c     | 4 +++-
 builtin/repack.c | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)


base-commit: 5d826e972970a784bd7a7bdf587512510097b8c7
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-95%2Fdscho%2Frelease-gc-repack-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-95/dscho/release-gc-repack-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/95
-- 
gitgitgadget

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

* [PATCH 1/1] gc/repack: release packs when needed
  2018-12-15 22:04 [PATCH 0/1] gc/repack: release packs when needed Johannes Schindelin via GitGitGadget
@ 2018-12-15 22:04 ` " Johannes Schindelin via GitGitGadget
  2019-01-10 21:01   ` Junio C Hamano
  0 siblings, 1 reply; 5+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-12-15 22:04 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

On Windows, files cannot be removed nor renamed if there are still
handles held by a process. To remedy that, we introduced the
close_all_packs() function.

Earlier, we made sure that the packs are released just before `git gc`
is spawned, in case that gc wants to remove no-longer needed packs.

But this developer forgot that gc itself also needs to let go of packs,
e.g. when consolidating all packs via the --aggressive option.

Likewise, `git repack -d` wants to delete obsolete packs and therefore
needs to close all pack handles, too.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/gc.c     | 4 +++-
 builtin/repack.c | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 871a56f1c5..df90fd7f51 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -659,8 +659,10 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 
 	report_garbage = report_pack_garbage;
 	reprepare_packed_git(the_repository);
-	if (pack_garbage.nr > 0)
+	if (pack_garbage.nr > 0) {
+		close_all_packs(the_repository->objects);
 		clean_pack_garbage();
+	}
 
 	if (gc_write_commit_graph)
 		write_commit_graph_reachable(get_object_directory(), 0,
diff --git a/builtin/repack.c b/builtin/repack.c
index 45583683ee..f9319defe4 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -419,6 +419,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	if (!names.nr && !po_args.quiet)
 		printf("Nothing new to pack.\n");
 
+	close_all_packs(the_repository->objects);
+
 	/*
 	 * Ok we have prepared all new packfiles.
 	 * First see if there are packs of the same name and if so
-- 
gitgitgadget

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

* Re: [PATCH 1/1] gc/repack: release packs when needed
  2018-12-15 22:04 ` [PATCH 1/1] " Johannes Schindelin via GitGitGadget
@ 2019-01-10 21:01   ` Junio C Hamano
  2019-01-11 16:10     ` Jeff King
  0 siblings, 1 reply; 5+ messages in thread
From: Junio C Hamano @ 2019-01-10 21:01 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> On Windows, files cannot be removed nor renamed if there are still
> handles held by a process. To remedy that, we introduced the
> close_all_packs() function.
>
> Earlier, we made sure that the packs are released just before `git gc`
> is spawned, in case that gc wants to remove no-longer needed packs.
>
> But this developer forgot that gc itself also needs to let go of packs,
> e.g. when consolidating all packs via the --aggressive option.
>
> Likewise, `git repack -d` wants to delete obsolete packs and therefore
> needs to close all pack handles, too.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/gc.c     | 4 +++-
>  builtin/repack.c | 2 ++
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 871a56f1c5..df90fd7f51 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -659,8 +659,10 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>  
>  	report_garbage = report_pack_garbage;
>  	reprepare_packed_git(the_repository);
> -	if (pack_garbage.nr > 0)
> +	if (pack_garbage.nr > 0) {
> +		close_all_packs(the_repository->objects);
>  		clean_pack_garbage();
> +	}

Closing before removing does make sense, but wouldn't we want to
move reprepare_packed_git() after clean_pack_garbage() while at it?
After all, the logical sequence is that we used the current set of
packs to figure out whihch ones are garbage, then now we are about
to discard.  We close the packs in the current set (i.e. the fix
made in this patch), discard the garbage packs.  It would make sense
to start using the new set (i.e. "reprepare") after all that is
done, no?  Especially, given that the next step (write-commit-graph)
still wants to read quite a lot of data from now the latest set of
packfiles...

>  	if (gc_write_commit_graph)
>  		write_commit_graph_reachable(get_object_directory(), 0,
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 45583683ee..f9319defe4 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -419,6 +419,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  	if (!names.nr && !po_args.quiet)
>  		printf("Nothing new to pack.\n");
>  
> +	close_all_packs(the_repository->objects);
> +

On the other hand, This one is added to the ideal and perfect
location, I think.

Thanks.

>  	/*
>  	 * Ok we have prepared all new packfiles.
>  	 * First see if there are packs of the same name and if so

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

* Re: [PATCH 1/1] gc/repack: release packs when needed
  2019-01-10 21:01   ` Junio C Hamano
@ 2019-01-11 16:10     ` Jeff King
  2019-01-11 17:24       ` Junio C Hamano
  0 siblings, 1 reply; 5+ messages in thread
From: Jeff King @ 2019-01-11 16:10 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin

On Thu, Jan 10, 2019 at 01:01:36PM -0800, Junio C Hamano wrote:

> > diff --git a/builtin/gc.c b/builtin/gc.c
> > index 871a56f1c5..df90fd7f51 100644
> > --- a/builtin/gc.c
> > +++ b/builtin/gc.c
> > @@ -659,8 +659,10 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
> >  
> >  	report_garbage = report_pack_garbage;
> >  	reprepare_packed_git(the_repository);
> > -	if (pack_garbage.nr > 0)
> > +	if (pack_garbage.nr > 0) {
> > +		close_all_packs(the_repository->objects);
> >  		clean_pack_garbage();
> > +	}
> 
> Closing before removing does make sense, but wouldn't we want to
> move reprepare_packed_git() after clean_pack_garbage() while at it?
> After all, the logical sequence is that we used the current set of
> packs to figure out whihch ones are garbage, then now we are about
> to discard.  We close the packs in the current set (i.e. the fix
> made in this patch), discard the garbage packs.  It would make sense
> to start using the new set (i.e. "reprepare") after all that is
> done, no?  Especially, given that the next step (write-commit-graph)
> still wants to read quite a lot of data from now the latest set of
> packfiles...

I agree that your suggested ordering makes more sense, but I don't think
it matters in practice with the current code. reprepare_packed_git()
never throws away old pack entries (and if they're mmap'd, we might even
continue to use them). So the end result is the same either way.

-Peff

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

* Re: [PATCH 1/1] gc/repack: release packs when needed
  2019-01-11 16:10     ` Jeff King
@ 2019-01-11 17:24       ` Junio C Hamano
  0 siblings, 0 replies; 5+ messages in thread
From: Junio C Hamano @ 2019-01-11 17:24 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin

Jeff King <peff@peff.net> writes:

> On Thu, Jan 10, 2019 at 01:01:36PM -0800, Junio C Hamano wrote:
>
>> > diff --git a/builtin/gc.c b/builtin/gc.c
>> > index 871a56f1c5..df90fd7f51 100644
>> > --- a/builtin/gc.c
>> > +++ b/builtin/gc.c
>> > @@ -659,8 +659,10 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>> >  
>> >  	report_garbage = report_pack_garbage;
>> >  	reprepare_packed_git(the_repository);
>> > -	if (pack_garbage.nr > 0)
>> > +	if (pack_garbage.nr > 0) {
>> > +		close_all_packs(the_repository->objects);
>> >  		clean_pack_garbage();
>> > +	}
>> 
>> Closing before removing does make sense, but wouldn't we want to
>> move reprepare_packed_git() after clean_pack_garbage() while at it?
>> After all, the logical sequence is that we used the current set of
>> packs to figure out whihch ones are garbage, then now we are about
>> to discard.  We close the packs in the current set (i.e. the fix
>> made in this patch), discard the garbage packs.  It would make sense
>> to start using the new set (i.e. "reprepare") after all that is
>> done, no?  Especially, given that the next step (write-commit-graph)
>> still wants to read quite a lot of data from now the latest set of
>> packfiles...
>
> I agree that your suggested ordering makes more sense, but I don't think
> it matters in practice with the current code. reprepare_packed_git()
> never throws away old pack entries (and if they're mmap'd, we might even
> continue to use them). So the end result is the same either way.

Yeah, it would not make difference to the machine.  I was trying to
be more helpful to human readers.

In any case, this patch from Dec 15 last year is where my backlog
sweeping is at right now X-<.

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

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-15 22:04 [PATCH 0/1] gc/repack: release packs when needed Johannes Schindelin via GitGitGadget
2018-12-15 22:04 ` [PATCH 1/1] " Johannes Schindelin via GitGitGadget
2019-01-10 21:01   ` Junio C Hamano
2019-01-11 16:10     ` Jeff King
2019-01-11 17:24       ` Junio C Hamano

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