[PATCH 0/2] pull: release handles to pack files before potentially gc'ing

[PATCH 0/2] pull: release handles to pack files before potentially gc'ing

[Johannes Schindelin via GitGitGadget]

It was reported in Git for Windows
[https://github.com/git-for-windows/git/issues/3336] that there are still
some code paths where .pack files are still open when a garbage collection
is triggered. This time, the affected command is git pull.

The fix is relatively trivial but uncovers a bug in the commit-graph code,
which we have to fix first.

Johannes Schindelin (2):
  commit-graph: when closing the graph, also release the slab
  pull: release packs before fetching

 builtin/pull.c | 2 ++
 commit-graph.c | 1 +
 2 files changed, 3 insertions(+)


base-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1032%2Fdscho%2Frelease-packs-before-fetching-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1032/dscho/release-packs-before-fetching-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1032
-- 
gitgitgadget

[PATCH 1/2] commit-graph: when closing the graph, also release the slab

[Johannes Schindelin via GitGitGadget]

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

The slab has information about the commit graph. That means that it is
meaningless (and even misleading) when the commit graph was closed.

This seems not to matter currently, but we're about to fix a
Windows-specific bug where `git pull` does not close the object store
before fetching (risking that an implicit auto-gc fails to remove the
now-obsolete pack file(s)), and once we have that bug fix in place, it
does matter: after that bug fix, we will open the object store, do some
stuff with it, then close it, fetch, and then open it again, and do more
stuff. If we close the commit graph without releasing the corresponding
slab, we're hit by a symptom like this in t5520.19:

	BUG: commit-reach.c:85: bad generation skip 9223372036854775807
	> 3 at 5cd378271655d43a3b4477520014f02213ad1546

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

diff --git a/commit-graph.c b/commit-graph.c
index 3860a0d8477..09984455150 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -713,6 +713,7 @@ static void close_commit_graph_one(struct commit_graph *g)
 	if (!g)
 		return;
 
+	clear_commit_graph_data_slab(&commit_graph_data_slab);
 	close_commit_graph_one(g->base_graph);
 	free_commit_graph(g);
 }
-- 
gitgitgadget

[PATCH 2/2] pull: release packs before fetching

[Johannes Schindelin via GitGitGadget]

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 try to release all open
handles to any `.pack` file before e.g. repacking (which would want to
remove the original `.pack` file(s) after it is done).

Since the `read_cache_unmerged()` and/or the `get_oid()` call in `git
pull` can cause `.pack` files to be opened, we need to release the open
handles before calling `git fetch`: the latter process might want to
spawn an auto-gc, which in turn might want to repack the objects.

This commit is similar in spirit to 5bdece0d705 (gc/repack: release
packs when needed, 2018-12-15).

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

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

diff --git a/builtin/pull.c b/builtin/pull.c
index 3e13f810843..d9f0156d969 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -26,6 +26,7 @@
 #include "wt-status.h"
 #include "commit-reach.h"
 #include "sequencer.h"
+#include "packfile.h"
 
 /**
  * Parses the value of --rebase. If value is a false value, returns
@@ -998,6 +999,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			oidclr(&rebase_fork_point);
 	}
 
+	close_object_store(the_repository->objects);
 	if (run_fetch(repo, refspecs))
 		return 1;
 
-- 
gitgitgadget

Re: [PATCH 2/2] pull: release packs before fetching

[Junio C Hamano]

"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 try to release all open
> handles to any `.pack` file before e.g. repacking (which would want to
> remove the original `.pack` file(s) after it is done).
>
> Since the `read_cache_unmerged()` and/or the `get_oid()` call in `git
> pull` can cause `.pack` files to be opened, we need to release the open
> handles before calling `git fetch`: the latter process might want to
> spawn an auto-gc, which in turn might want to repack the objects.
>
> This commit is similar in spirit to 5bdece0d705 (gc/repack: release
> packs when needed, 2018-12-15).
>
> This fixes https://github.com/git-for-windows/git/issues/3336.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/pull.c | 2 ++
>  1 file changed, 2 insertions(+)

After run_fetch() returns, we then go on to access objects from our
object store (that's natural---after all, we fetched because we
wanted to access the objects we have plus objects they have to offer
to us) and the object store is transparently reopened for us.  Which
may make a bit confusing API to newbies, but is an easy one to use,
once we get used to it.

A few general comments.

 * Right now, run_fetch() does not do anything that needs to access
   objects, but there is no reason to expect that will continue to
   be the case, and once we added an call to an innocuous helper
   function that happens to access objects, the close_object_store()
   call made by the caller before run_fetch() was called becomes
   moot.  The more we can delay the call to close_object_store(),
   the better.  And the absolute last point we can defer the call to
   close_object_store() is where immediately before run_fetch() calls
   run_command_v_opt() to spawn "git fetch".

 * Which makes me wonder if we may be better off having a bit in the
   flags word the run_command() API takes to make a call to
   close_object_store() for us.  run_fetch() that uses the
   run_command API can use that bit without having to worry about
   making a call to close_object_store() itself and when.

 * Hits from "git grep -A2 close_object_store()" shows a notable
   pattern.  Before run_auto_maintenance(), we often see a call to
   it.  It almost feels (but I didn't dig it deeper) that a call to
   run_auto_maintenance() that does not call close_object_store()
   before doing so is a bug (there is one in builtin/commit.c).

 * Which in turn makes me wonder if these many calls to close before
   run_auto_maintenance() should be moved to run_auto_maintenance()
   itself (which in turn can use the new flags bit in the
   run_command() API).

Sprinkling yet another call to close_object_store() as we discover
need for doing so like this patch does is certainly OK, but as we
add new hooks and higher-level commands, it will get messier and
messier.  It probably may make sense to go in and clean it up,
hopefully guided by the above observations, either before this
"fix", or soon after it graduates before we forget.

Will queue, but will not merge down to 'next' until I hear an Ack on
the commit-graph stuff.

Thanks.



> diff --git a/builtin/pull.c b/builtin/pull.c
> index 3e13f810843..d9f0156d969 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -26,6 +26,7 @@
>  #include "wt-status.h"
>  #include "commit-reach.h"
>  #include "sequencer.h"
> +#include "packfile.h"
>  
>  /**
>   * Parses the value of --rebase. If value is a false value, returns
> @@ -998,6 +999,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>  			oidclr(&rebase_fork_point);
>  	}
>  
> +	close_object_store(the_repository->objects);
>  	if (run_fetch(repo, refspecs))
>  		return 1;

Re: [PATCH 2/2] pull: release packs before fetching

[Johannes Schindelin]

Hi Junio,

On Wed, 8 Sep 2021, Junio C Hamano wrote:

> "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 try to release all open
> > handles to any `.pack` file before e.g. repacking (which would want to
> > remove the original `.pack` file(s) after it is done).
> >
> > Since the `read_cache_unmerged()` and/or the `get_oid()` call in `git
> > pull` can cause `.pack` files to be opened, we need to release the open
> > handles before calling `git fetch`: the latter process might want to
> > spawn an auto-gc, which in turn might want to repack the objects.
> >
> > This commit is similar in spirit to 5bdece0d705 (gc/repack: release
> > packs when needed, 2018-12-15).
> >
> > This fixes https://github.com/git-for-windows/git/issues/3336.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  builtin/pull.c | 2 ++
> >  1 file changed, 2 insertions(+)
>
> After run_fetch() returns, we then go on to access objects from our
> object store (that's natural---after all, we fetched because we
> wanted to access the objects we have plus objects they have to offer
> to us) and the object store is transparently reopened for us.  Which
> may make a bit confusing API to newbies, but is an easy one to use,
> once we get used to it.
>
> A few general comments.
>
>  * Right now, run_fetch() does not do anything that needs to access
>    objects, but there is no reason to expect that will continue to
>    be the case, and once we added an call to an innocuous helper
>    function that happens to access objects, the close_object_store()
>    call made by the caller before run_fetch() was called becomes
>    moot.  The more we can delay the call to close_object_store(),
>    the better.  And the absolute last point we can defer the call to
>    close_object_store() is where immediately before run_fetch() calls
>    run_command_v_opt() to spawn "git fetch".
>
>  * Which makes me wonder if we may be better off having a bit in the
>    flags word the run_command() API takes to make a call to
>    close_object_store() for us.  run_fetch() that uses the
>    run_command API can use that bit without having to worry about
>    making a call to close_object_store() itself and when.
>
>  * Hits from "git grep -A2 close_object_store()" shows a notable
>    pattern.  Before run_auto_maintenance(), we often see a call to
>    it.  It almost feels (but I didn't dig it deeper) that a call to
>    run_auto_maintenance() that does not call close_object_store()
>    before doing so is a bug (there is one in builtin/commit.c).
>
>  * Which in turn makes me wonder if these many calls to close before
>    run_auto_maintenance() should be moved to run_auto_maintenance()
>    itself (which in turn can use the new flags bit in the
>    run_command() API).
>
> Sprinkling yet another call to close_object_store() as we discover
> need for doing so like this patch does is certainly OK, but as we
> add new hooks and higher-level commands, it will get messier and
> messier.  It probably may make sense to go in and clean it up,
> hopefully guided by the above observations, either before this
> "fix", or soon after it graduates before we forget.

I like those ideas, and submitted a follow-up patch series.

> Will queue, but will not merge down to 'next' until I hear an Ack on
> the commit-graph stuff.

Thank you.

For procedural reasons, I would like to keep the current patch series
as-is, because that will free some mental space for me maintaining Git for
Windows (where I already merged them, after a contributor verified the
fix).

Thanks,
Dscho