git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] connected.c: reprepare packs for corner cases

From
Jeff King <peff@peff.net>
Date
Mar 12, 2020, 21:16 UTC
Message-ID
<20200312211638.GA872402@coredump.intra.peff.net>
In-Reply-To
<xmqqzhcle23x.fsf@gitster.c.googlers.com>
On Thu, Mar 12, 2020 at 01:42:58PM -0700, Junio C Hamano wrote:
Show 10 quoted lines
> > 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. :)
Show 9 quoted lines
> 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
Previous: Junio C HamanoNext: Jeff King
Message 5 of 12 in “connected.c: reprepare packs for corner cases”
  1. connected.c: reprepare packs for corner casesDerrick Stolee via GitGitGadget, Mar 12, 2020
  2. Jonathan TanMar 12, 2020
  3. Derrick StoleeMar 12, 2020
  4. Junio C HamanoMar 12, 2020
  5. Jeff KingMar 12, 2020
  6. Jeff KingMar 12, 2020
  7. Derrick StoleeMar 13, 2020
  8. Junio C HamanoMar 13, 2020
  9. Jeff KingMar 13, 2020
  10. Jeff KingMar 13, 2020
  11. Derrick StoleeMar 13, 2020
  12. connected.c: reprepare packs for corner casesDerrick Stolee via GitGitGadget, Mar 13, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.