[2/2] pull: release packs before fetching

Commit Message

Johannes Schindelin Sept. 8, 2021, 8:29 a.m. UTC

From: Johannes Schindelin <[e-mail]>

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 <[e-mail]>
---
 builtin/pull.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Junio C Hamano Sept. 8, 2021, 7:16 p.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

> From: Johannes Schindelin <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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;

Johannes Schindelin Sept. 9, 2021, 11:20 a.m. UTC | #2

Hi Junio,

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

> "Johannes Schindelin via GitGitGadget" <[e-mail]>
> writes:
>
> > From: Johannes Schindelin <[e-mail]>
> >
> > 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 <[e-mail]>
> > ---
> >  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

Patch

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;