Re: [PATCH 1/1] commit-graph: fix writing first commit-graph during fetch

[Jeff King <peff@peff.net>]

On Tue, Oct 22, 2019 at 08:48:20PM -0400, Jeff King wrote:

> I admit I am puzzled, though, _why_ the presence of the submodule
> matters. That is, from your explanation, I thought the issue was simply
> that `fetch` walked (and marked) some commits, and the flags overlapped
> with what the commit-graph code expected.
> 
> I could guess that the presence of the submodule triggers some analysis
> for --recurse-submodules. But then we don't actually recurse (maybe
> because they're not activated? In which case maybe we shouldn't be doing
> that extra walk to look for submodules if there aren't any activated
> ones in our local repo).

Indeed, that seems to be it. If I do this:

  git init repo
  cd repo
  cat >.gitmodules <<\EOF
  [submodule "foo"]
  path = foo
  url = https://example.com
  EOF
  time git fetch /path/to/git.git

then we end up traversing the whole git.git history a second time, even
though we should know off the bat that there are no active submodules
that we would recurse to.

Doing this makes the problem go away:

diff --git a/submodule.c b/submodule.c
index 0f199c5137..0db2f18b93 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1193,7 +1193,7 @@ static void calculate_changed_submodule_paths(struct repository *r,
 	struct string_list_item *name;
 
 	/* No need to check if there are no submodules configured */
-	if (!submodule_from_path(r, NULL, NULL))
+	if (!is_submodule_active(r, NULL))
 		return;
 
 	argv_array_push(&argv, "--"); /* argv[0] program name */

but causes some tests to fail (I think that in some cases we're supposed
to auto-initialize, and we'd probably need to cover that case, too).

All of this is outside of your fix, of course, but:

  1. I'm satisfied now that I understand why the test triggers the
     problem.

  2. You may want have a real activated submodule in your test. Right
     now we'll trigger the submodule-recursion check even without that,
     but in the future we might do something like the hunk above. In
     which case your test wouldn't be checking anything interesting
     anymore.

-Peff