On Thu, Mar 12, 2020 at 01:42:58PM -0700, Junio C Hamano wrote:
> > Placing a reprepare_packed_git() call inside chck_connected() before
> > looping through the packed_git list seems like the safest way to
> > avoid this issue in the future.
>
> Hmmm. I am not sure if I am convinced that check_connected() is the
> best place to do this. Do we know the place that adds a new pack to
> the repository, yet forgets to add it to the packed-git list, that
> caused the failure you were observing? Doing this change, without
> describing the answer to the question in the log message, makes it
> smell rather like a random hack than a designed solution to me.
Thanks, I was just writing a very similar message. :)
> If lazy fetching of objects happen in multiple fetches before a
> single check_connected() sweeps them to check for connectivity, then
> perhaps the lazy fetching codepath needs to remember the fact that
> it added a new pack that is still not known to the packed-git list
> (or just add it immediately, without having to scan at all), and
> check_connected() would need to rescan only when there is at least
> one such new pack? That way, you do not have to penalize normal
> callers of check_connected() that do not use lazy fetches at all,
> right?
I share your concern that it's not the best place for this. In practice,
I do doubt that callers of check_connected() would notice, as it's a
pretty heavy-weight operation by itself. But I'd be concerned about
going the other way: we know that calling it in check_connected() fixes
_this_ problem, but we don't know if there's other code that would need
a similar fix.
I was able to reproduce easily from Stolee's instructions, but I did
notice one interesting thing: the problem occurs only with the http
protocol, not with git:// or ssh. The obvious difference between them is
that http code is mostly running in the remote-helper program and in a
spawned git-fetch-pack. But the check_connected() we hit is in the
parent git-fetch process. So I _suspect_ that there's some low-level
code which calls reprepare() that happens in-process in most cases, but
not for the http case.
If I instrument Git like this:
diff --git a/common-main.c b/common-main.c
index 71e21dd20a..bf6ffe42b7 100644
--- a/common-main.c
+++ b/common-main.c
@@ -49,6 +49,11 @@ int main(int argc, const char **argv)
trace2_cmd_start(argv);
trace2_collect_process_info(TRACE2_PROCESS_INFO_STARTUP);
+ trace_printf("pid=%d, cmd=%s",
+ (int)getpid(),
+ !ends_with(argv[0], "git") ? argv[0] :
+ argv[1] ? argv[1] : argv[0]);
+
result = cmd_main(argc, argv);
trace2_cmd_exit(result);
diff --git a/packfile.c b/packfile.c
index f4e752996d..832d020e6e 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1004,6 +1004,8 @@ void reprepare_packed_git(struct repository *r)
{
struct object_directory *odb;
+ trace_printf("repreparing in pid %d", (int)getpid());
+
obj_read_lock();
for (odb = r->objects->odb; odb; odb = odb->next)
odb_clear_loose_cache(odb);
then running over the git protocol gives me:
17:13:52.642337 common-main.c:52 pid=882782, cmd=fetch
17:13:52.642871 git.c:439 trace: built-in: git fetch origin
17:13:52.725971 packfile.c:1007 repreparing in pid 882782
remote: Enumerating objects: 49, done.
17:13:52.793355 run-command.c:663 trace: run_command: git index-pack --stdin -v --fix-thin --promisor --pack_header=2,49
remote: Total 49 (delta 0), reused 0 (delta 0), pack-reused 49
17:13:52.796106 common-main.c:52 pid=882784, cmd=index-pack
17:13:52.796655 git.c:439 trace: built-in: git index-pack --stdin -v --fix-thin --promisor --pack_header=2,49
Receiving objects: 100% (49/49), 6.87 KiB | 6.87 MiB/s, done.
Resolving deltas: 100% (17/17), done.
17:13:52.811302 packfile.c:1007 repreparing in pid 882782
From git://github.com/derrickstolee/TreeSearch
* [new branch] master -> origin/master
17:13:52.812955 run-command.c:1616 run_processes_parallel: preparing to run up to 1 tasks
17:13:52.812986 run-command.c:1648 run_processes_parallel: done
17:13:52.813034 run-command.c:663 trace: run_command: git gc --auto
17:13:52.815435 common-main.c:52 pid=882788, cmd=gc
17:13:52.815941 git.c:439 trace: built-in: git gc --auto
There's an early reprepare, but the one after index-pack finishes
resolving deltas is probably the interesting one. And it happens inside
the fetch process. But if we go back to http, I get:
17:15:28.234866 common-main.c:52 pid=882836, cmd=fetch
17:15:28.235403 git.c:439 trace: built-in: git fetch origin
17:15:28.236329 run-command.c:663 trace: run_command: GIT_DIR=.git git-remote-https origin https://github.com/derrickstolee/TreeSearch
17:15:28.244049 common-main.c:52 pid=882837, cmd=/home/peff/compile/git/git-remote-https
17:15:28.382586 packfile.c:1007 repreparing in pid 882836
17:15:28.382996 run-command.c:663 trace: run_command: git fetch-pack --stateless-rpc --stdin --lock-pack --include-tag --thin --from-promisor --filter=blob:none https://github.com/derrickstolee/TreeSearch/
17:15:28.386284 common-main.c:52 pid=882839, cmd=fetch-pack
17:15:28.386807 git.c:439 trace: built-in: git fetch-pack --stateless-rpc --stdin --lock-pack --include-tag --thin --from-promisor --filter=blob:none https://github.com/derrickstolee/TreeSearch/
remote: Enumerating objects: 49, done.
remote: Total 49 (delta 0), reused 0 (delta 0), pack-reused 49
17:15:28.444799 run-command.c:663 trace: run_command: git index-pack --stdin -v --fix-thin --promisor --pack_header=2,49
17:15:28.447441 common-main.c:52 pid=882841, cmd=index-pack
17:15:28.447944 git.c:439 trace: built-in: git index-pack --stdin -v --fix-thin --promisor --pack_header=2,49
Receiving objects: 100% (49/49), 6.87 KiB | 3.44 MiB/s, done.
Resolving deltas: 100% (17/17), done.
17:15:28.461871 packfile.c:1007 repreparing in pid 882839
error: https://github.com/derrickstolee/TreeSearch did not send all necessary objects
17:15:28.463446 run-command.c:663 trace: run_command: git gc --auto
17:15:28.463949 common-main.c:52 pid=882845, cmd=gc
17:15:28.464056 git.c:439 trace: built-in: git gc --auto
There we see that same reprepare happen in 882839, which is the child
fetch-pack. The parent fetch probably needs to reprepare itself after
fetch-pack completes.
-Peff
