{"thread":{"id":"56467","subject":"[PATCH 0/3] More add_submodule_odb() cleanup in merge code","startedAt":"2021-09-08T18:18:30Z","lastAt":"2021-09-14T01:31:19Z","messageCount":14,"participants":["Jonathan Tan","Junio C Hamano","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"435131","messageId":"cover.1631123754.git.jonathantanmy@google.com","threadId":"56467","inReplyTo":null,"subject":"[PATCH 0/3] More add_submodule_odb() cleanup in merge code","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-09-08T18:18:12Z","receivedAt":"2021-09-08T18:18:30Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"(CC-ing Elijah in case he has insight into the merge-ort part.)\n\nWhile continuing the effort into removing add_submodule_odb() (as part\nof the submodule partial clone effort) I came across this part of the\nmerge code that specifies the repository being operated on in two ways:\none as a struct repository pointer and the other as a path. This patch\nset unifies the two.\n\nI normally would not send refactoring patches unless I also have a\nfeature patch that uses the results of said refactoring, but in this\ncase, I think that these patches are worth having since they clarify a\npotentially unclear part of the API.\n\nNote that these patches mean that the merging code no longer supports\nsubmodules that have their .git dirs in the worktree, but from what I\ncan tell, this seems to be the direction we're going in\n(repo_submodule_init() does not support such submodules).\n\nPatch 3 is included to show how I'm verifying some things. Including\nsomething like that in the master branch would probably require\nconditional compilation (to exclude the additional field in struct\nobject used for checking, among other things), so I'm just including it\nhere for informational purposes.\n\nAll these patches work under GIT_TEST_MERGE_ALGORITHM=recursive and\nGIT_TEST_MERGE_ALGORITHM=ort (and when that envvar is unset, for good\nmeasure).\n\nJonathan Tan (3):\n t6437: run absorbgitdirs on repos\n revision: remove \"submodule\" from opt struct\n DO NOT SUBMIT commit-reach,revision: verify non-mixing\n\n alloc.c | 2 +\n commit-reach.c | 60 +++++++++++++++++-------\n merge-ort.c | 55 +++++++++++++++-------\n merge-recursive.c | 51 +++++++++++++-------\n object.h | 1 +\n revision.c | 96 ++++++++++++++++++++++----------------\n revision.h | 1 -\n t/t6437-submodule-merge.sh | 9 ++--\n 8 files changed, 179 insertions(+), 96 deletions(-)\n\n-- \n2.33.0.309.g3052b89438-goog\n\n"},{"id":"435132","messageId":"942d3ce2d3cf96192c7e9d5860a18c333dd08acf.1631123754.git.jonathantanmy@google.com","threadId":"56467","inReplyTo":"cover.1631123754.git.jonathantanmy@google.com","subject":"[PATCH 1/3] t6437: run absorbgitdirs on repos","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-09-08T18:18:13Z","receivedAt":"2021-09-08T18:18:33Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The submodule merge code is being transitioned away from\nadd_submodule_odb() to repo_submodule_init(), and the latter does not\nsupport submodules that have their .git directories in the worktree\n(instead of in .git/modules). Migrate the test code by calling\nabsorbgitdirs wherever necessary to place the .git directories of\nsubmodules in .git/modules of the superproject.\n\nSigned-off-by: Jonathan Tan \n---\n t/t6437-submodule-merge.sh | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh\nindex e5e89c2045..8efce86b42 100755\n--- a/t/t6437-submodule-merge.sh\n+++ b/t/t6437-submodule-merge.sh\n@@ -27,7 +27,8 @@ test_expect_success setup '\n \t git add file &&\n \t test_tick &&\n \t git commit -m sub-root) &&\n-\tgit add sub &&\n+\tgit submodule add ./sub sub &&\n+\tgit submodule absorbgitdirs &&\n \ttest_tick &&\n \tgit commit -m root &&\n \n@@ -82,7 +83,8 @@ test_expect_success 'setup for merge search' '\n \t git branch sub-a) &&\n \tgit commit --allow-empty -m init &&\n \tgit branch init &&\n-\tgit add sub &&\n+\tgit submodule add ./sub sub &&\n+\tgit submodule absorbgitdirs &&\n \tgit commit -m \"a\" &&\n \tgit branch a &&\n \n@@ -112,7 +114,8 @@ test_expect_success 'setup for merge search' '\n \tgit checkout -b g init &&\n \t(cd sub &&\n \t git checkout -b sub-g sub-c) &&\n-\tgit add sub &&\n+\tgit submodule add ./sub sub &&\n+\tgit submodule absorbgitdirs &&\n \tgit commit -a -m \"g\")\n '\n \n-- \n2.33.0.309.g3052b89438-goog\n\n"},{"id":"435133","messageId":"7ab764f27061c04c589262e621ef664357682ca9.1631123754.git.jonathantanmy@google.com","threadId":"56467","inReplyTo":"cover.1631123754.git.jonathantanmy@google.com","subject":"[PATCH 2/3] revision: remove \"submodule\" from opt struct","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-09-08T18:18:14Z","receivedAt":"2021-09-08T18:19:08Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Clean up a TODO in revision.h by removing the \"submodule\" field from\nstruct setup_revision_opt. This field is only used to specify the ref\nstore to use, so use rev_info->repo to determine the ref store instead.\n\nThe only users of this field are merge-ort.c and merge-recursive.c.\nHowever, both these files specify the superproject as rev_info->repo and\nthe submodule as setup_revision_opt->submodule. In order to be able to\npass the submodule as rev_info->repo, all commits must be parsed with\nthe submodule explicitly specified; this patch does that as well. (An\nincremental solution in which only some commits are parsed with explicit\nsubmodule will not work, because if the same commit is parsed twice in\ndifferent repositories, there will be 2 heap-allocated object structs\ncorresponding to that commit, and any flag set by the revision walking\nmechanism on one of them will not be reflected onto the other.)\n\nSigned-off-by: Jonathan Tan \n---\n merge-ort.c | 55 +++++++++++++++++++++++++++++++----------------\n merge-recursive.c | 51 ++++++++++++++++++++++++++++---------------\n revision.c | 16 +++++---------\n revision.h | 1 -\n 4 files changed, 76 insertions(+), 47 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 515dc39b7f..419ac0f396 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -32,6 +32,7 @@\n #include \"promisor-remote.h\"\n #include \"revision.h\"\n #include \"strmap.h\"\n+#include \"submodule-config.h\"\n #include \"submodule.h\"\n #include \"tree.h\"\n #include \"unpack-trees.h\"\n@@ -1511,7 +1512,6 @@ static int find_first_merges(struct repository *repo,\n \txsnprintf(merged_revision, sizeof(merged_revision), \"^%s\",\n \t\t oid_to_hex(&a->object.oid));\n \trepo_init_revisions(repo, &revs, NULL);\n-\trev_opts.submodule = path;\n \t/* FIXME: can't handle linked worktrees in submodules yet */\n \trevs.single_worktree = path != NULL;\n \tsetup_revisions(ARRAY_SIZE(rev_args)-1, rev_args, &revs, &rev_opts);\n@@ -1521,7 +1521,7 @@ static int find_first_merges(struct repository *repo,\n \t\tdie(\"revision walk setup failed\");\n \twhile ((commit = get_revision(&revs)) != NULL) {\n \t\tstruct object *o = &(commit->object);\n-\t\tif (in_merge_bases(b, commit))\n+\t\tif (repo_in_merge_bases(repo, b, commit))\n \t\t\tadd_object_array(o, NULL, &merges);\n \t}\n \treset_revision_walk();\n@@ -1536,7 +1536,7 @@ static int find_first_merges(struct repository *repo,\n \t\tcontains_another = 0;\n \t\tfor (j = 0; j < merges.nr; j++) {\n \t\t\tstruct commit *m2 = (struct commit *) merges.objects[j].item;\n-\t\t\tif (i != j && in_merge_bases(m2, m1)) {\n+\t\t\tif (i != j && repo_in_merge_bases(repo, m2, m1)) {\n \t\t\t\tcontains_another = 1;\n \t\t\t\tbreak;\n \t\t\t}\n@@ -1557,10 +1557,13 @@ static int merge_submodule(struct merge_options *opt,\n \t\t\t const struct object_id *b,\n \t\t\t struct object_id *result)\n {\n+\tconst struct submodule *submodule;\n+\tstruct repository subrepo;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tint ret = 0;\n \tstruct commit *commit_o, *commit_a, *commit_b;\n \tint parent_count;\n \tstruct object_array merges;\n-\tstruct strbuf sb = STRBUF_INIT;\n \n \tint i;\n \tint search = !opt->priv->call_depth;\n@@ -1576,6 +1579,10 @@ static int merge_submodule(struct merge_options *opt,\n \tif (is_null_oid(b))\n \t\treturn 0;\n \n+\t/*\n+\t * NEEDSWORK: Remove this when all submodule object accesses are\n+\t * through explicitly specified repositores.\n+\t */\n \tif (add_submodule_odb(path)) {\n \t\tpath_msg(opt, path, 0,\n \t\t\t _(\"Failed to merge submodule %s (not checked out)\"),\n@@ -1583,39 +1590,49 @@ static int merge_submodule(struct merge_options *opt,\n \t\treturn 0;\n \t}\n \n-\tif (!(commit_o = lookup_commit_reference(opt->repo, o)) ||\n-\t !(commit_a = lookup_commit_reference(opt->repo, a)) ||\n-\t !(commit_b = lookup_commit_reference(opt->repo, b))) {\n+\tsubmodule = submodule_from_path(opt->repo, null_oid(), path);\n+\tif (repo_submodule_init(&subrepo, opt->repo, submodule)) {\n+\t\tpath_msg(opt, path, 0,\n+\t\t\t\t_(\"Failed to merge submodule %s (not checked out)\"),\n+\t\t\t\tpath);\n+\t\treturn 0;\n+\t}\n+\n+\tif (!(commit_o = lookup_commit_reference(&subrepo, o)) ||\n+\t !(commit_a = lookup_commit_reference(&subrepo, a)) ||\n+\t !(commit_b = lookup_commit_reference(&subrepo, b))) {\n \t\tpath_msg(opt, path, 0,\n \t\t\t _(\"Failed to merge submodule %s (commits not present)\"),\n \t\t\t path);\n-\t\treturn 0;\n+\t\tgoto cleanup;\n \t}\n \n \t/* check whether both changes are forward */\n-\tif (!in_merge_bases(commit_o, commit_a) ||\n-\t !in_merge_bases(commit_o, commit_b)) {\n+\tif (!repo_in_merge_bases(&subrepo, commit_o, commit_a) ||\n+\t !repo_in_merge_bases(&subrepo, commit_o, commit_b)) {\n \t\tpath_msg(opt, path, 0,\n \t\t\t _(\"Failed to merge submodule %s \"\n \t\t\t \"(commits don't follow merge-base)\"),\n \t\t\t path);\n-\t\treturn 0;\n+\t\tgoto cleanup;\n \t}\n \n \t/* Case #1: a is contained in b or vice versa */\n-\tif (in_merge_bases(commit_a, commit_b)) {\n+\tif (repo_in_merge_bases(&subrepo, commit_a, commit_b)) {\n \t\toidcpy(result, b);\n \t\tpath_msg(opt, path, 1,\n \t\t\t _(\"Note: Fast-forwarding submodule %s to %s\"),\n \t\t\t path, oid_to_hex(b));\n-\t\treturn 1;\n+\t\tret = 1;\n+\t\tgoto cleanup;\n \t}\n-\tif (in_merge_bases(commit_b, commit_a)) {\n+\tif (repo_in_merge_bases(&subrepo, commit_b, commit_a)) {\n \t\toidcpy(result, a);\n \t\tpath_msg(opt, path, 1,\n \t\t\t _(\"Note: Fast-forwarding submodule %s to %s\"),\n \t\t\t path, oid_to_hex(a));\n-\t\treturn 1;\n+\t\tret = 1;\n+\t\tgoto cleanup;\n \t}\n \n \t/*\n@@ -1627,10 +1644,10 @@ static int merge_submodule(struct merge_options *opt,\n \n \t/* Skip the search if makes no sense to the calling context. */\n \tif (!search)\n-\t\treturn 0;\n+\t\tgoto cleanup;\n \n \t/* find commit which merges them */\n-\tparent_count = find_first_merges(opt->repo, path, commit_a, commit_b,\n+\tparent_count = find_first_merges(&subrepo, path, commit_a, commit_b,\n \t\t\t\t\t &merges);\n \tswitch (parent_count) {\n \tcase 0:\n@@ -1664,7 +1681,9 @@ static int merge_submodule(struct merge_options *opt,\n \t}\n \n \tobject_array_clear(&merges);\n-\treturn 0;\n+cleanup:\n+\trepo_clear(&subrepo);\n+\treturn ret;\n }\n \n static void initialize_attr_index(struct merge_options *opt)\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 3355d50e8a..bc5b34d56e 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -24,6 +24,7 @@\n #include \"repository.h\"\n #include \"revision.h\"\n #include \"string-list.h\"\n+#include \"submodule-config.h\"\n #include \"submodule.h\"\n #include \"tag.h\"\n #include \"tree-walk.h\"\n@@ -1113,7 +1114,6 @@ static int find_first_merges(struct repository *repo,\n \txsnprintf(merged_revision, sizeof(merged_revision), \"^%s\",\n \t\t oid_to_hex(&a->object.oid));\n \trepo_init_revisions(repo, &revs, NULL);\n-\trev_opts.submodule = path;\n \t/* FIXME: can't handle linked worktrees in submodules yet */\n \trevs.single_worktree = path != NULL;\n \tsetup_revisions(ARRAY_SIZE(rev_args)-1, rev_args, &revs, &rev_opts);\n@@ -1123,7 +1123,7 @@ static int find_first_merges(struct repository *repo,\n \t\tdie(\"revision walk setup failed\");\n \twhile ((commit = get_revision(&revs)) != NULL) {\n \t\tstruct object *o = &(commit->object);\n-\t\tif (in_merge_bases(b, commit))\n+\t\tif (repo_in_merge_bases(repo, b, commit))\n \t\t\tadd_object_array(o, NULL, &merges);\n \t}\n \treset_revision_walk();\n@@ -1138,7 +1138,7 @@ static int find_first_merges(struct repository *repo,\n \t\tcontains_another = 0;\n \t\tfor (j = 0; j < merges.nr; j++) {\n \t\t\tstruct commit *m2 = (struct commit *) merges.objects[j].item;\n-\t\t\tif (i != j && in_merge_bases(m2, m1)) {\n+\t\t\tif (i != j && repo_in_merge_bases(repo, m2, m1)) {\n \t\t\t\tcontains_another = 1;\n \t\t\t\tbreak;\n \t\t\t}\n@@ -1174,6 +1174,9 @@ static int merge_submodule(struct merge_options *opt,\n \t\t\t const struct object_id *base, const struct object_id *a,\n \t\t\t const struct object_id *b)\n {\n+\tconst struct submodule *submodule;\n+\tstruct repository subrepo;\n+\tint ret = 0;\n \tstruct commit *commit_base, *commit_a, *commit_b;\n \tint parent_count;\n \tstruct object_array merges;\n@@ -1197,27 +1200,37 @@ static int merge_submodule(struct merge_options *opt,\n \tif (is_null_oid(b))\n \t\treturn 0;\n \n+\t/*\n+\t * NEEDSWORK: Remove this when all submodule object accesses are\n+\t * through explicitly specified repositores.\n+\t */\n \tif (add_submodule_odb(path)) {\n \t\toutput(opt, 1, _(\"Failed to merge submodule %s (not checked out)\"), path);\n \t\treturn 0;\n \t}\n \n-\tif (!(commit_base = lookup_commit_reference(opt->repo, base)) ||\n-\t !(commit_a = lookup_commit_reference(opt->repo, a)) ||\n-\t !(commit_b = lookup_commit_reference(opt->repo, b))) {\n-\t\toutput(opt, 1, _(\"Failed to merge submodule %s (commits not present)\"), path);\n+\tsubmodule = submodule_from_path(opt->repo, null_oid(), path);\n+\tif (repo_submodule_init(&subrepo, opt->repo, submodule)) {\n+\t\toutput(opt, 1, _(\"Failed to merge submodule %s (not checked out)\"), path);\n \t\treturn 0;\n \t}\n \n+\tif (!(commit_base = lookup_commit_reference(&subrepo, base)) ||\n+\t !(commit_a = lookup_commit_reference(&subrepo, a)) ||\n+\t !(commit_b = lookup_commit_reference(&subrepo, b))) {\n+\t\toutput(opt, 1, _(\"Failed to merge submodule %s (commits not present)\"), path);\n+\t\tgoto cleanup;\n+\t}\n+\n \t/* check whether both changes are forward */\n-\tif (!in_merge_bases(commit_base, commit_a) ||\n-\t !in_merge_bases(commit_base, commit_b)) {\n+\tif (!repo_in_merge_bases(&subrepo, commit_base, commit_a) ||\n+\t !repo_in_merge_bases(&subrepo, commit_base, commit_b)) {\n \t\toutput(opt, 1, _(\"Failed to merge submodule %s (commits don't follow merge-base)\"), path);\n-\t\treturn 0;\n+\t\tgoto cleanup;\n \t}\n \n \t/* Case #1: a is contained in b or vice versa */\n-\tif (in_merge_bases(commit_a, commit_b)) {\n+\tif (repo_in_merge_bases(&subrepo, commit_a, commit_b)) {\n \t\toidcpy(result, b);\n \t\tif (show(opt, 3)) {\n \t\t\toutput(opt, 3, _(\"Fast-forwarding submodule %s to the following commit:\"), path);\n@@ -1227,9 +1240,10 @@ static int merge_submodule(struct merge_options *opt,\n \t\telse\n \t\t\t; /* no output */\n \n-\t\treturn 1;\n+\t\tret = 1;\n+\t\tgoto cleanup;\n \t}\n-\tif (in_merge_bases(commit_b, commit_a)) {\n+\tif (repo_in_merge_bases(&subrepo, commit_b, commit_a)) {\n \t\toidcpy(result, a);\n \t\tif (show(opt, 3)) {\n \t\t\toutput(opt, 3, _(\"Fast-forwarding submodule %s to the following commit:\"), path);\n@@ -1239,7 +1253,8 @@ static int merge_submodule(struct merge_options *opt,\n \t\telse\n \t\t\t; /* no output */\n \n-\t\treturn 1;\n+\t\tret = 1;\n+\t\tgoto cleanup;\n \t}\n \n \t/*\n@@ -1251,10 +1266,10 @@ static int merge_submodule(struct merge_options *opt,\n \n \t/* Skip the search if makes no sense to the calling context. */\n \tif (!search)\n-\t\treturn 0;\n+\t\tgoto cleanup;\n \n \t/* find commit which merges them */\n-\tparent_count = find_first_merges(opt->repo, &merges, path,\n+\tparent_count = find_first_merges(&subrepo, &merges, path,\n \t\t\t\t\t commit_a, commit_b);\n \tswitch (parent_count) {\n \tcase 0:\n@@ -1281,7 +1296,9 @@ static int merge_submodule(struct merge_options *opt,\n \t}\n \n \tobject_array_clear(&merges);\n-\treturn 0;\n+cleanup:\n+\trepo_clear(&subrepo);\n+\treturn ret;\n }\n \n static int merge_mode_and_contents(struct merge_options *opt,\ndiff --git a/revision.c b/revision.c\nindex 0dabb5a0bc..7e7c41fedd 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2563,8 +2563,7 @@ static int for_each_good_bisect_ref(struct ref_store *refs, each_ref_fn fn, void\n \treturn for_each_bisect_ref(refs, fn, cb_data, term_good);\n }\n \n-static int handle_revision_pseudo_opt(const char *submodule,\n-\t\t\t\t struct rev_info *revs,\n+static int handle_revision_pseudo_opt(struct rev_info *revs,\n \t\t\t\t const char **argv, int *flags)\n {\n \tconst char *arg = argv[0];\n@@ -2572,7 +2571,7 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \tstruct ref_store *refs;\n \tint argcount;\n \n-\tif (submodule) {\n+\tif (revs->repo != the_repository) {\n \t\t/*\n \t\t * We need some something like get_submodule_worktrees()\n \t\t * before we can go through all worktrees of a submodule,\n@@ -2581,9 +2580,8 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \t\t */\n \t\tif (!revs->single_worktree)\n \t\t\tBUG(\"--single-worktree cannot be used together with submodule\");\n-\t\trefs = get_submodule_ref_store(submodule);\n-\t} else\n-\t\trefs = get_main_ref_store(revs->repo);\n+\t}\n+\trefs = get_main_ref_store(revs->repo);\n \n \t/*\n \t * NOTE!\n@@ -2707,12 +2705,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n {\n \tint i, flags, left, seen_dashdash, revarg_opt;\n \tstruct strvec prune_data = STRVEC_INIT;\n-\tconst char *submodule = NULL;\n \tint seen_end_of_options = 0;\n \n-\tif (opt)\n-\t\tsubmodule = opt->submodule;\n-\n \t/* First, search for \"--\" */\n \tif (opt && opt->assume_dashdash) {\n \t\tseen_dashdash = 1;\n@@ -2741,7 +2735,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\tif (!seen_end_of_options && *arg == '-') {\n \t\t\tint opts;\n \n-\t\t\topts = handle_revision_pseudo_opt(submodule,\n+\t\t\topts = handle_revision_pseudo_opt(\n \t\t\t\t\t\trevs, argv + i,\n \t\t\t\t\t\t&flags);\n \t\t\tif (opts > 0) {\ndiff --git a/revision.h b/revision.h\nindex 0c65a760ee..5578bb4720 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -336,7 +336,6 @@ extern volatile show_early_output_fn_t show_early_output;\n struct setup_revision_opt {\n \tconst char *def;\n \tvoid (*tweak)(struct rev_info *, struct setup_revision_opt *);\n-\tconst char *submodule;\t/* TODO: drop this and use rev_info->repo */\n \tunsigned int\tassume_dashdash:1,\n \t\t\tallow_exclude_promisor_objects:1;\n \tunsigned revarg_opt;\n-- \n2.33.0.309.g3052b89438-goog\n\n"},{"id":"435134","messageId":"f149dee7afffc9de72d3b81dfb1afbd2b027c9b4.1631123754.git.jonathantanmy@google.com","threadId":"56467","inReplyTo":"cover.1631123754.git.jonathantanmy@google.com","subject":"[PATCH 3/3] DO NOT SUBMIT commit-reach,revision: verify non-mixing","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-09-08T18:18:15Z","receivedAt":"2021-09-08T18:19:13Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This patch is provided to show reviewers how I'm verifying that there is\nno mixing of object structs in the revision walking mechanisms. I've\nintercepted any creation of commit nodes in alloc.c (whether through\nalloc_commit_node() directly or through alloc_object_node()) to add the\nrepository in which that node is allocated, and checked all additions of\ncommits to data structures (as far as I can tell) in commit-reach.c and\nrevision.c to make sure that the repository in the added objects is what\nwe expect. This ensures that any flags set are set on the correct nodes\n(for example, it won't be set on an object allocated in the_repository\nand then checked from an object allocated in a submodule).\n\nI'll leave out this patch in subsequent revisions of this patch set.\n\nSigned-off-by: Jonathan Tan \n---\n alloc.c | 2 ++\n commit-reach.c | 60 +++++++++++++++++++++++++------------\n object.h | 1 +\n revision.c | 80 ++++++++++++++++++++++++++++++++------------------\n 4 files changed, 97 insertions(+), 46 deletions(-)\n\ndiff --git a/alloc.c b/alloc.c\nindex 957a0af362..98b8c06834 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -96,6 +96,7 @@ void *alloc_object_node(struct repository *r)\n {\n \tstruct object *obj = alloc_node(r->parsed_objects->object_state, sizeof(union any_object));\n \tobj->type = OBJ_NONE;\n+\tobj->repo = r;\n \treturn obj;\n }\n \n@@ -120,6 +121,7 @@ void *alloc_commit_node(struct repository *r)\n {\n \tstruct commit *c = alloc_node(r->parsed_objects->commit_state, sizeof(struct commit));\n \tinit_commit_node(c);\n+\tc->object.repo = r;\n \treturn c;\n }\n \ndiff --git a/commit-reach.c b/commit-reach.c\nindex c226ee3da4..eb868c3837 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -9,6 +9,30 @@\n #include \"tag.h\"\n #include \"commit-reach.h\"\n \n+static void checked_put(struct repository *repo, struct prio_queue *queue, struct commit *commit)\n+{\n+\tif (repo != commit->object.repo)\n+\t\t*(int *)0=1;\n+\tprio_queue_put(queue, commit);\n+}\n+#define prio_queue_put DO_NOT_USE\n+\n+static struct commit_list *checked_insert(struct repository *repo, struct commit *item, struct commit_list **list)\n+{\n+\tif (repo != item->object.repo)\n+\t\t*(int *)0=1;\n+\treturn commit_list_insert(item, list);\n+}\n+#define commit_list_insert DO_NOT_USE\n+\n+static struct commit_list *checked_insert_by_date(struct repository *repo, struct commit *item, struct commit_list **list)\n+{\n+\tif (repo != item->object.repo)\n+\t\t*(int *)0=1;\n+\treturn commit_list_insert_by_date(item, list);\n+}\n+#define commit_list_insert_by_date DO_NOT_USE\n+\n /* Remember to update object flag allocation in object.h */\n #define PARENT1\t\t(1u<<16)\n #define PARENT2\t\t(1u<<17)\n@@ -66,11 +90,11 @@ static struct commit_list *paint_down_to_common(struct repository *r,\n \t\tcommit_list_append(one, &result);\n \t\treturn result;\n \t}\n-\tprio_queue_put(&queue, one);\n+\tchecked_put(r, &queue, one);\n \n \tfor (i = 0; i < n; i++) {\n \t\ttwos[i]->object.flags |= PARENT2;\n-\t\tprio_queue_put(&queue, twos[i]);\n+\t\tchecked_put(r, &queue, twos[i]);\n \t}\n \n \twhile (queue_has_nonstale(&queue)) {\n@@ -92,7 +116,7 @@ static struct commit_list *paint_down_to_common(struct repository *r,\n \t\tif (flags == (PARENT1 | PARENT2)) {\n \t\t\tif (!(commit->object.flags & RESULT)) {\n \t\t\t\tcommit->object.flags |= RESULT;\n-\t\t\t\tcommit_list_insert_by_date(commit, &result);\n+\t\t\t\tchecked_insert_by_date(r, commit, &result);\n \t\t\t}\n \t\t\t/* Mark parents of a found merge stale */\n \t\t\tflags |= STALE;\n@@ -106,7 +130,7 @@ static struct commit_list *paint_down_to_common(struct repository *r,\n \t\t\tif (repo_parse_commit(r, p))\n \t\t\t\treturn NULL;\n \t\t\tp->object.flags |= flags;\n-\t\t\tprio_queue_put(&queue, p);\n+\t\t\tchecked_put(r, &queue, p);\n \t\t}\n \t}\n \n@@ -128,7 +152,7 @@ static struct commit_list *merge_bases_many(struct repository *r,\n \t\t\t * We do not mark this even with RESULT so we do not\n \t\t\t * have to clean it up.\n \t\t\t */\n-\t\t\treturn commit_list_insert(one, &result);\n+\t\t\treturn checked_insert(r, one, &result);\n \t}\n \n \tif (repo_parse_commit(r, one))\n@@ -143,7 +167,7 @@ static struct commit_list *merge_bases_many(struct repository *r,\n \twhile (list) {\n \t\tstruct commit *commit = pop_commit(&list);\n \t\tif (!(commit->object.flags & STALE))\n-\t\t\tcommit_list_insert_by_date(commit, &result);\n+\t\t\tchecked_insert_by_date(r, commit, &result);\n \t}\n \treturn result;\n }\n@@ -155,7 +179,7 @@ struct commit_list *get_octopus_merge_bases(struct commit_list *in)\n \tif (!in)\n \t\treturn ret;\n \n-\tcommit_list_insert(in->item, &ret);\n+\tchecked_insert(the_repository, in->item, &ret);\n \n \tfor (i = in->next; i; i = i->next) {\n \t\tstruct commit_list *new_commits = NULL, *end = NULL;\n@@ -287,7 +311,7 @@ static int remove_redundant_with_gen(struct repository *r,\n \t\t/* push the STALE bits up to min generation */\n \t\tstruct commit_list *stack = NULL;\n \n-\t\tcommit_list_insert(walk_start[i], &stack);\n+\t\tchecked_insert(r, walk_start[i], &stack);\n \t\twalk_start[i]->object.flags |= STALE;\n \n \t\twhile (stack) {\n@@ -317,7 +341,7 @@ static int remove_redundant_with_gen(struct repository *r,\n \t\t\twhile (parents) {\n \t\t\t\tif (!(parents->item->object.flags & STALE)) {\n \t\t\t\t\tparents->item->object.flags |= STALE;\n-\t\t\t\t\tcommit_list_insert(parents->item, &stack);\n+\t\t\t\t\tchecked_insert(r, parents->item, &stack);\n \t\t\t\t\tbreak;\n \t\t\t\t}\n \t\t\t\tparents = parents->next;\n@@ -410,7 +434,7 @@ static struct commit_list *get_merge_bases_many_0(struct repository *r,\n \tcnt = remove_redundant(r, rslt, cnt);\n \tresult = NULL;\n \tfor (i = 0; i < cnt; i++)\n-\t\tcommit_list_insert_by_date(rslt[i], &result);\n+\t\tchecked_insert_by_date(r, rslt[i], &result);\n \tfree(rslt);\n \treturn result;\n }\n@@ -451,7 +475,7 @@ int repo_is_descendant_of(struct repository *r,\n \tif (generation_numbers_enabled(the_repository)) {\n \t\tstruct commit_list *from_list = NULL;\n \t\tint result;\n-\t\tcommit_list_insert(commit, &from_list);\n+\t\tchecked_insert(r, commit, &from_list);\n \t\tresult = can_all_from_reach(from_list, with_commit, 0);\n \t\tfree_commit_list(from_list);\n \t\treturn result;\n@@ -550,7 +574,7 @@ struct commit_list *reduce_heads(struct commit_list *heads)\n \t}\n \tnum_head = remove_redundant(the_repository, array, num_head);\n \tfor (i = 0; i < num_head; i++)\n-\t\ttail = &commit_list_insert(array[i], tail)->next;\n+\t\ttail = &checked_insert(the_repository, array[i], tail)->next;\n \tfree(array);\n \treturn result;\n }\n@@ -588,7 +612,7 @@ int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid)\n \tif (parse_commit(new_commit) < 0)\n \t\treturn 0;\n \n-\tcommit_list_insert(old_commit, &old_commit_list);\n+\tchecked_insert(the_repository, old_commit, &old_commit_list);\n \tret = repo_is_descendant_of(the_repository,\n \t\t\t\t new_commit, old_commit_list);\n \tfree_commit_list(old_commit_list);\n@@ -765,7 +789,7 @@ int can_all_from_reach_with_flag(struct object_array *from,\n \t\tstruct commit_list *stack = NULL;\n \n \t\tlist[i]->object.flags |= assign_flag;\n-\t\tcommit_list_insert(list[i], &stack);\n+\t\tchecked_insert(the_repository, list[i], &stack);\n \n \t\twhile (stack) {\n \t\t\tstruct commit_list *parent;\n@@ -789,7 +813,7 @@ int can_all_from_reach_with_flag(struct object_array *from,\n \t\t\t\t\t commit_graph_generation(parent->item) < min_generation)\n \t\t\t\t\t\tcontinue;\n \n-\t\t\t\t\tcommit_list_insert(parent->item, &stack);\n+\t\t\t\t\tchecked_insert(the_repository, parent->item, &stack);\n \t\t\t\t\tbreak;\n \t\t\t\t}\n \t\t\t}\n@@ -907,7 +931,7 @@ struct commit_list *get_reachable_subset(struct commit **from, int nr_from,\n \t\t\tc->object.flags |= PARENT2;\n \t\t\tparse_commit(c);\n \n-\t\t\tprio_queue_put(&queue, *item);\n+\t\t\tchecked_put(the_repository, &queue, *item);\n \t\t}\n \t}\n \n@@ -917,7 +941,7 @@ struct commit_list *get_reachable_subset(struct commit **from, int nr_from,\n \t\tif (current->object.flags & PARENT1) {\n \t\t\tcurrent->object.flags &= ~PARENT1;\n \t\t\tcurrent->object.flags |= reachable_flag;\n-\t\t\tcommit_list_insert(current, &found_commits);\n+\t\t\tchecked_insert(the_repository, current, &found_commits);\n \t\t\tnum_to_find--;\n \t\t}\n \n@@ -933,7 +957,7 @@ struct commit_list *get_reachable_subset(struct commit **from, int nr_from,\n \t\t\t\tcontinue;\n \n \t\t\tp->object.flags |= PARENT2;\n-\t\t\tprio_queue_put(&queue, p);\n+\t\t\tchecked_put(the_repository, &queue, p);\n \t\t}\n \t}\n \ndiff --git a/object.h b/object.h\nindex 3b38c9cc98..22e9d2a52a 100644\n--- a/object.h\n+++ b/object.h\n@@ -90,6 +90,7 @@ struct object {\n \tunsigned type : TYPE_BITS;\n \tunsigned flags : FLAG_BITS;\n \tstruct object_id oid;\n+\tstruct repository *repo;\n };\n \n const char *type_name(unsigned int type);\ndiff --git a/revision.c b/revision.c\nindex 7e7c41fedd..ba30e58921 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -33,6 +33,30 @@\n #include \"bloom.h\"\n #include \"json-writer.h\"\n \n+static void checked_put(struct repository *repo, struct prio_queue *queue, struct commit *commit)\n+{\n+\tif (repo != commit->object.repo)\n+\t\t*(int *)0=1;\n+\tprio_queue_put(queue, commit);\n+}\n+#define prio_queue_put DO_NOT_USE\n+\n+static struct commit_list *checked_insert(struct repository *repo, struct commit *item, struct commit_list **list)\n+{\n+\tif (repo != item->object.repo)\n+\t\t*(int *)0=1;\n+\treturn commit_list_insert(item, list);\n+}\n+#define commit_list_insert DO_NOT_USE\n+\n+static struct commit_list *checked_insert_by_date(struct repository *repo, struct commit *item, struct commit_list **list)\n+{\n+\tif (repo != item->object.repo)\n+\t\t*(int *)0=1;\n+\treturn commit_list_insert_by_date(item, list);\n+}\n+#define commit_list_insert_by_date DO_NOT_USE\n+\n volatile show_early_output_fn_t show_early_output;\n \n static const char *term_bad;\n@@ -1124,9 +1148,9 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \t\t\t\tcontinue;\n \t\t\tp->object.flags |= (SEEN | NOT_USER_GIVEN);\n \t\t\tif (list)\n-\t\t\t\tcommit_list_insert_by_date(p, list);\n+\t\t\t\tchecked_insert_by_date(revs->repo, p, list);\n \t\t\tif (queue)\n-\t\t\t\tprio_queue_put(queue, p);\n+\t\t\t\tchecked_put(revs->repo, queue, p);\n \t\t}\n \t\treturn 0;\n \t}\n@@ -1166,9 +1190,9 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \t\tif (!(p->object.flags & SEEN)) {\n \t\t\tp->object.flags |= (SEEN | NOT_USER_GIVEN);\n \t\t\tif (list)\n-\t\t\t\tcommit_list_insert_by_date(p, list);\n+\t\t\t\tchecked_insert_by_date(revs->repo, p, list);\n \t\t\tif (queue)\n-\t\t\t\tprio_queue_put(queue, p);\n+\t\t\t\tchecked_put(revs->repo, queue, p);\n \t\t}\n \t\tif (revs->first_parent_only)\n \t\t\tbreak;\n@@ -1291,7 +1315,7 @@ static int still_interesting(struct commit_list *src, timestamp_t date, int slop\n * the result of \"A..B\" without --ancestry-path, and limits the latter\n * further to the ones that can reach one of the commits in \"bottom\".\n */\n-static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *list)\n+static void limit_to_ancestry(struct repository *repo, struct commit_list *bottom, struct commit_list *list)\n {\n \tstruct commit_list *p;\n \tstruct commit_list *rlist = NULL;\n@@ -1302,7 +1326,7 @@ static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *li\n \t * process parents before children.\n \t */\n \tfor (p = list; p; p = p->next)\n-\t\tcommit_list_insert(p->item, &rlist);\n+\t\tchecked_insert(repo, p->item, &rlist);\n \n \tfor (p = bottom; p; p = p->next)\n \t\tp->item->object.flags |= TMP_MARK;\n@@ -1362,12 +1386,12 @@ static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *li\n * to filter the result of \"A..B\" further to the ones that can actually\n * reach A.\n */\n-static struct commit_list *collect_bottom_commits(struct commit_list *list)\n+static struct commit_list *collect_bottom_commits(struct repository *repo, struct commit_list *list)\n {\n \tstruct commit_list *elem, *bottom = NULL;\n \tfor (elem = list; elem; elem = elem->next)\n \t\tif (elem->item->object.flags & BOTTOM)\n-\t\t\tcommit_list_insert(elem->item, &bottom);\n+\t\t\tchecked_insert(repo, elem->item, &bottom);\n \treturn bottom;\n }\n \n@@ -1399,7 +1423,7 @@ static int limit_list(struct rev_info *revs)\n \tstruct commit *interesting_cache = NULL;\n \n \tif (revs->ancestry_path) {\n-\t\tbottom = collect_bottom_commits(original_list);\n+\t\tbottom = collect_bottom_commits(revs->repo, original_list);\n \t\tif (!bottom)\n \t\t\tdie(\"--ancestry-path given but there are no bottom commits\");\n \t}\n@@ -1427,7 +1451,7 @@ static int limit_list(struct rev_info *revs)\n \t\t !revs->line_level_traverse)\n \t\t\tcontinue;\n \t\tdate = commit->date;\n-\t\tp = &commit_list_insert(commit, p)->next;\n+\t\tp = &checked_insert(revs->repo, commit, p)->next;\n \n \t\tshow = show_early_output;\n \t\tif (!show)\n@@ -1443,7 +1467,7 @@ static int limit_list(struct rev_info *revs)\n \t\tlimit_left_right(newlist, revs);\n \n \tif (bottom) {\n-\t\tlimit_to_ancestry(bottom, newlist);\n+\t\tlimit_to_ancestry(revs->repo, bottom, newlist);\n \t\tfree_commit_list(bottom);\n \t}\n \n@@ -3133,14 +3157,14 @@ static struct commit_list **simplify_one(struct rev_info *revs, struct commit *c\n \tfor (cnt = 0, p = commit->parents; p; p = p->next) {\n \t\tpst = locate_simplify_state(revs, p->item);\n \t\tif (!pst->simplified) {\n-\t\t\ttail = &commit_list_insert(p->item, tail)->next;\n+\t\t\ttail = &checked_insert(revs->repo, p->item, tail)->next;\n \t\t\tcnt++;\n \t\t}\n \t\tif (revs->first_parent_only)\n \t\t\tbreak;\n \t}\n \tif (cnt) {\n-\t\ttail = &commit_list_insert(commit, tail)->next;\n+\t\ttail = &checked_insert(revs->repo, commit, tail)->next;\n \t\treturn tail;\n \t}\n \n@@ -3230,7 +3254,7 @@ static void simplify_merges(struct rev_info *revs)\n \t\t * Do not free(list) here yet; the original list\n \t\t * is used later in this function.\n \t\t */\n-\t\tcommit_list_insert(commit, &yet_to_do);\n+\t\tchecked_insert(revs->repo, commit, &yet_to_do);\n \t}\n \twhile (yet_to_do) {\n \t\tlist = yet_to_do;\n@@ -3252,7 +3276,7 @@ static void simplify_merges(struct rev_info *revs)\n \t\tcommit = pop_commit(&list);\n \t\tst = locate_simplify_state(revs, commit);\n \t\tif (st->simplified == commit)\n-\t\t\ttail = &commit_list_insert(commit, tail)->next;\n+\t\t\ttail = &checked_insert(revs->repo, commit, tail)->next;\n \t}\n }\n \n@@ -3316,13 +3340,13 @@ static void trace2_topo_walk_statistics_atexit(void)\n \tjw_release(&jw);\n }\n \n-static inline void test_flag_and_insert(struct prio_queue *q, struct commit *c, int flag)\n+static inline void test_flag_and_insert(struct repository *repo, struct prio_queue *q, struct commit *c, int flag)\n {\n \tif (c->object.flags & flag)\n \t\treturn;\n \n \tc->object.flags |= flag;\n-\tprio_queue_put(q, c);\n+\tchecked_put(repo, q, c);\n }\n \n static void explore_walk_step(struct rev_info *revs)\n@@ -3352,7 +3376,7 @@ static void explore_walk_step(struct rev_info *revs)\n \t\tmark_parents_uninteresting(c);\n \n \tfor (p = c->parents; p; p = p->next)\n-\t\ttest_flag_and_insert(&info->explore_queue, p->item, TOPO_WALK_EXPLORED);\n+\t\ttest_flag_and_insert(revs->repo, &info->explore_queue, p->item, TOPO_WALK_EXPLORED);\n }\n \n static void explore_to_depth(struct rev_info *revs,\n@@ -3393,7 +3417,7 @@ static void indegree_walk_step(struct rev_info *revs)\n \t\telse\n \t\t\t*pi = 2;\n \n-\t\ttest_flag_and_insert(&info->indegree_queue, parent, TOPO_WALK_INDEGREE);\n+\t\ttest_flag_and_insert(revs->repo, &info->indegree_queue, parent, TOPO_WALK_INDEGREE);\n \n \t\tif (revs->first_parent_only)\n \t\t\treturn;\n@@ -3464,8 +3488,8 @@ static void init_topo_walk(struct rev_info *revs)\n \t\tif (repo_parse_commit_gently(revs->repo, c, 1))\n \t\t\tcontinue;\n \n-\t\ttest_flag_and_insert(&info->explore_queue, c, TOPO_WALK_EXPLORED);\n-\t\ttest_flag_and_insert(&info->indegree_queue, c, TOPO_WALK_INDEGREE);\n+\t\ttest_flag_and_insert(revs->repo, &info->explore_queue, c, TOPO_WALK_EXPLORED);\n+\t\ttest_flag_and_insert(revs->repo, &info->indegree_queue, c, TOPO_WALK_INDEGREE);\n \n \t\tgeneration = commit_graph_generation(c);\n \t\tif (generation < info->min_generation)\n@@ -3482,7 +3506,7 @@ static void init_topo_walk(struct rev_info *revs)\n \t\tstruct commit *c = list->item;\n \n \t\tif (*(indegree_slab_at(&info->indegree, c)) == 1)\n-\t\t\tprio_queue_put(&info->topo_queue, c);\n+\t\t\tchecked_put(revs->repo, &info->topo_queue, c);\n \t}\n \n \t/*\n@@ -3545,7 +3569,7 @@ static void expand_topo_walk(struct rev_info *revs, struct commit *commit)\n \n \t\t(*pi)--;\n \t\tif (*pi == 1)\n-\t\t\tprio_queue_put(&info->topo_queue, parent);\n+\t\t\tchecked_put(revs->repo, &info->topo_queue, parent);\n \n \t\tif (revs->first_parent_only)\n \t\t\treturn;\n@@ -3635,7 +3659,7 @@ static enum rewrite_result rewrite_one_1(struct rev_info *revs,\n \t}\n }\n \n-static void merge_queue_into_list(struct prio_queue *q, struct commit_list **list)\n+static void merge_queue_into_list(struct repository *repo, struct prio_queue *q, struct commit_list **list)\n {\n \twhile (q->nr) {\n \t\tstruct commit *item = prio_queue_peek(q);\n@@ -3644,7 +3668,7 @@ static void merge_queue_into_list(struct prio_queue *q, struct commit_list **lis\n \t\tif (p && p->item->date >= item->date)\n \t\t\tlist = &p->next;\n \t\telse {\n-\t\t\tp = commit_list_insert(item, list);\n+\t\t\tp = checked_insert(repo, item, list);\n \t\t\tlist = &p->next; /* skip newly added item */\n \t\t\tprio_queue_get(q); /* pop item */\n \t\t}\n@@ -3655,7 +3679,7 @@ static enum rewrite_result rewrite_one(struct rev_info *revs, struct commit **pp\n {\n \tstruct prio_queue queue = { compare_commits_by_commit_date };\n \tenum rewrite_result ret = rewrite_one_1(revs, pp, &queue);\n-\tmerge_queue_into_list(&queue, &revs->commits);\n+\tmerge_queue_into_list(revs->repo, &queue, &revs->commits);\n \tclear_prio_queue(&queue);\n \treturn ret;\n }\n@@ -4075,7 +4099,7 @@ static void create_boundary_commit_list(struct rev_info *revs)\n \t\tif (c->object.flags & (SHOWN | BOUNDARY))\n \t\t\tcontinue;\n \t\tc->object.flags |= BOUNDARY;\n-\t\tcommit_list_insert(c, &revs->commits);\n+\t\tchecked_insert(revs->repo, c, &revs->commits);\n \t}\n \n \t/*\n@@ -4179,7 +4203,7 @@ struct commit *get_revision(struct rev_info *revs)\n \tif (revs->reverse) {\n \t\treversed = NULL;\n \t\twhile ((c = get_revision_internal(revs)))\n-\t\t\tcommit_list_insert(c, &reversed);\n+\t\t\tchecked_insert(revs->repo, c, &reversed);\n \t\trevs->commits = reversed;\n \t\trevs->reverse = 0;\n \t\trevs->reverse_output_stage = 1;\n-- \n2.33.0.309.g3052b89438-goog\n\n"},{"id":"435183","messageId":"20210908220252.3799351-1-jonathantanmy@google.com","threadId":"56467","inReplyTo":"942d3ce2d3cf96192c7e9d5860a18c333dd08acf.1631123754.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/3] t6437: run absorbgitdirs on repos","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-09-08T22:02:52Z","receivedAt":"2021-09-08T22:02:59Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> The submodule merge code is being transitioned away from\n> add_submodule_odb() to repo_submodule_init(), and the latter does not\n> support submodules that have their .git directories in the worktree\n> (instead of in .git/modules). Migrate the test code by calling\n> absorbgitdirs wherever necessary to place the .git directories of\n> submodules in .git/modules of the superproject.\n\nUpon further work, there seems to be quite a lot of test code to\nmigrate, and it might be easier instead to add support for unabsorbed\nsubmodules to repo_submodule_init() (say, following what\nopen_submodule() in submodule.c does). Feel free to comment on this or\nother aspects of the patch set, and in the meantime, I'll investigate\nthat.\n"},{"id":"435188","messageId":"xmqqwnnq1zh9.fsf@gitster.g","threadId":"56467","inReplyTo":"942d3ce2d3cf96192c7e9d5860a18c333dd08acf.1631123754.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/3] t6437: run absorbgitdirs on repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-08T22:20:18Z","receivedAt":"2021-09-08T22:20:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> The submodule merge code is being transitioned away from\n> add_submodule_odb() to repo_submodule_init(), and the latter does not\n> support submodules that have their .git directories in the worktree\n> (instead of in .git/modules). Migrate the test code by calling\n> absorbgitdirs wherever necessary to place the .git directories of\n> submodules in .git/modules of the superproject.\n\nIt is wonderful that we can work with the \"absorbed\" form of the\nsubmodule repositories, and having all the submodules in \"absorbed\"\nform may be ideal in the longer term, but I fail to convince myself\nthat this particular change to test script is a good idea.\n\nAfter all, if you are not cloning from somebody else but creating\na new submodule in a superproject you have (whether that was locally\ncreated or cloned from elsewhere), wouldn't it more natural to have\na submodule that is not \"absorbed\" there? Is it easy to create an\n\"absorbed\" submodule locally? \n\nThe change to tests in this patch may make things work with the\nrepo_submodule_init() that is incapable of work with submodules with\nin-worktree .git repository. In theory, the users can do the same,\ni.e. have an extra \"git submodule absorbgitdirs\" call, when adding a\nnew submodule, to make things work with repo_submodule_init(), too.\n\nBut that sounds like a workaround, not a feature.\n\nQuite honestly, if repo_submodule_init() can only work with the\n\"absorbed\" form, isn't it simply a bug?\n\nTwo independent (i.e. we can do either or both) things may improve\nthe situation:\n\n - allow repo_submodule_init() to work on both forms (in place---no\n cheating by calling \"absorb\" behind user's back).\n\n - teach \"git submodule add\" to transition an old-style submodule\n into the \"absorbed\" form (either with an option, or do so by\n default with an escape hatch to disable).\n\nThe latter indirectly attacks the \"repo_submodule_init() can only\nwork with absorbed form\" issue by making it harder to create a\nnon-absorbed submodule to begin with.\n\nHmm?\n"},{"id":"435275","messageId":"CABPp-BGKBMtue-T1ah-+RQHJ+ceYBVsqs54Vx0p-Hs+UkLXPtw@mail.gmail.com","threadId":"56467","inReplyTo":"cover.1631123754.git.jonathantanmy@google.com","subject":"Re: [PATCH 0/3] More add_submodule_odb() cleanup in merge code","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-09-09T15:26:58Z","receivedAt":"2021-09-09T15:27:20Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Sep 8, 2021 at 11:18 AM Jonathan Tan wrote:\n>\n> (CC-ing Elijah in case he has insight into the merge-ort part.)\n\nAll the submodule merging related functions were lifted from\nmerge-recursive.c and minimally modified to fit the new structure.\nThe only substantive change I made was to fix the merge result for\nvirtual merge bases, but that's like one or two lines of code. In\nparticular, everything relating to submodule objects was totally\nuntouched...and I think that's reflected in the fact that your PATCH 2\nbasically is the same patch twice, once for merge-recursive and once\nfor merge-ort.\n\nI read over PATCH 2 and I didn't find anything that looked\nproblematic, but I'm not up-to-speed on the add_submodule_odb and repo\nhandling bits of the codebase so I'm not sure I would catch anything.\nBut I am encouraged by the fact that it looks like you did the same\nstuff to merge-recursive and merge-ort; I'd be worried you missed\nsomething if that weren't the case.\n\n\nAs a sidenote, though...\n\nThis does remind me that I noticed that the following functions from\nobject-store.h do not take an explicit repository:\n\nwrite_object_file()\nhash_object_file()\nhash_object_file_literally()\nforce_object_loose()\n\nI have a patch sitting around somewhere (possibly only still\nreferenced in my 'temp' branch) to make repo_*() variants of the above\nfunctions, and turn the above into simple wrappers of the repo_*()\nvariants which just pass the_repository (much like someone else did\nwith read_object_file() and repo_read_object_file()). It also updates\nmerge-ort to use the new repo_*() functions. However, I ended up\nexcluding it from my merge-ort submissions since it wasn't necessary.\nWould this be of interest in your submodule work, though? I guess\nit'd only matter if we started doing real merges of submodules as part\nof a parent repo merge. (As opposed to the fast-forward-only merges\nthat merge-recursive/merge-ort do right now for submodules.)\n\n> While continuing the effort into removing add_submodule_odb() (as part\n> of the submodule partial clone effort) I came across this part of the\n> merge code that specifies the repository being operated on in two ways:\n> one as a struct repository pointer and the other as a path. This patch\n> set unifies the two.\n>\n> I normally would not send refactoring patches unless I also have a\n> feature patch that uses the results of said refactoring, but in this\n> case, I think that these patches are worth having since they clarify a\n> potentially unclear part of the API.\n>\n> Note that these patches mean that the merging code no longer supports\n> submodules that have their .git dirs in the worktree, but from what I\n> can tell, this seems to be the direction we're going in\n> (repo_submodule_init() does not support such submodules).\n>\n> Patch 3 is included to show how I'm verifying some things. Including\n> something like that in the master branch would probably require\n> conditional compilation (to exclude the additional field in struct\n> object used for checking, among other things), so I'm just including it\n> here for informational purposes.\n>\n> All these patches work under GIT_TEST_MERGE_ALGORITHM=recursive and\n> GIT_TEST_MERGE_ALGORITHM=ort (and when that envvar is unset, for good\n> measure).\n>\n> Jonathan Tan (3):\n> t6437: run absorbgitdirs on repos\n> revision: remove \"submodule\" from opt struct\n> DO NOT SUBMIT commit-reach,revision: verify non-mixing\n>\n> alloc.c | 2 +\n> commit-reach.c | 60 +++++++++++++++++-------\n> merge-ort.c | 55 +++++++++++++++-------\n> merge-recursive.c | 51 +++++++++++++-------\n> object.h | 1 +\n> revision.c | 96 ++++++++++++++++++++++----------------\n> revision.h | 1 -\n> t/t6437-submodule-merge.sh | 9 ++--\n> 8 files changed, 179 insertions(+), 96 deletions(-)\n>\n> --\n> 2.33.0.309.g3052b89438-goog\n>\n"},{"id":"435285","messageId":"20210909174758.2550040-1-jonathantanmy@google.com","threadId":"56467","inReplyTo":"xmqqwnnq1zh9.fsf@gitster.g","subject":"Re: [PATCH 1/3] t6437: run absorbgitdirs on repos","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-09-09T17:47:58Z","receivedAt":"2021-09-09T17:48:06Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Quite honestly, if repo_submodule_init() can only work with the\n> \"absorbed\" form, isn't it simply a bug?\n> \n> Two independent (i.e. we can do either or both) things may improve\n> the situation:\n> \n> - allow repo_submodule_init() to work on both forms (in place---no\n> cheating by calling \"absorb\" behind user's back).\n> \n> - teach \"git submodule add\" to transition an old-style submodule\n> into the \"absorbed\" form (either with an option, or do so by\n> default with an escape hatch to disable).\n> \n> The latter indirectly attacks the \"repo_submodule_init() can only\n> work with absorbed form\" issue by making it harder to create a\n> non-absorbed submodule to begin with.\n\nThanks for taking a look.\n\nI took a further look and it doesn't look too difficult to teach\nrepo_submodule_init() to work on both forms (without calling\n\"absorbgitdirs\") - I'll send an updated patch set soon. As for \"git\nsubmodule add\" transitioning it into the absorbed form, I'll leave that\nfor someone else to do - although if repo_submodule_init() supports the\nunabsorbed form now, that is not so urgent to do, I think.\n"},{"id":"435287","messageId":"20210909175152.2550626-1-jonathantanmy@google.com","threadId":"56467","inReplyTo":"CABPp-BGKBMtue-T1ah-+RQHJ+ceYBVsqs54Vx0p-Hs+UkLXPtw@mail.gmail.com","subject":"Re: [PATCH 0/3] More add_submodule_odb() cleanup in merge code","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-09-09T17:51:52Z","receivedAt":"2021-09-09T17:51:56Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Wed, Sep 8, 2021 at 11:18 AM Jonathan Tan wrote:\n> >\n> > (CC-ing Elijah in case he has insight into the merge-ort part.)\n> \n> All the submodule merging related functions were lifted from\n> merge-recursive.c and minimally modified to fit the new structure.\n> The only substantive change I made was to fix the merge result for\n> virtual merge bases, but that's like one or two lines of code. In\n> particular, everything relating to submodule objects was totally\n> untouched...and I think that's reflected in the fact that your PATCH 2\n> basically is the same patch twice, once for merge-recursive and once\n> for merge-ort.\n> \n> I read over PATCH 2 and I didn't find anything that looked\n> problematic, but I'm not up-to-speed on the add_submodule_odb and repo\n> handling bits of the codebase so I'm not sure I would catch anything.\n> But I am encouraged by the fact that it looks like you did the same\n> stuff to merge-recursive and merge-ort; I'd be worried you missed\n> something if that weren't the case.\n\nThanks for taking a look.\n\n> As a sidenote, though...\n> \n> This does remind me that I noticed that the following functions from\n> object-store.h do not take an explicit repository:\n> \n> write_object_file()\n> hash_object_file()\n> hash_object_file_literally()\n> force_object_loose()\n> \n> I have a patch sitting around somewhere (possibly only still\n> referenced in my 'temp' branch) to make repo_*() variants of the above\n> functions, and turn the above into simple wrappers of the repo_*()\n> variants which just pass the_repository (much like someone else did\n> with read_object_file() and repo_read_object_file()). It also updates\n> merge-ort to use the new repo_*() functions. However, I ended up\n> excluding it from my merge-ort submissions since it wasn't necessary.\n> Would this be of interest in your submodule work, though? I guess\n> it'd only matter if we started doing real merges of submodules as part\n> of a parent repo merge. (As opposed to the fast-forward-only merges\n> that merge-recursive/merge-ort do right now for submodules.)\n\nI think that these functions would be useful if we were to write into\nsubmodules (for example, in a real merge of submodules, as you said).\nRight now my submodule work is just making it support partial clones, so\nI'm not planning to introduce any new submodule writes. (Well, besides\nthe fact that a lazy-fetch in a submodule would end up with new objects,\nbut that's already handled because we do the fetch in a separate process\nin the submodule's gitdir.)\n"},{"id":"435290","messageId":"cover.1631212893.git.jonathantanmy@google.com","threadId":"56467","inReplyTo":"cover.1631123754.git.jonathantanmy@google.com","subject":"[PATCH v2 0/3] More add_submodule_odb() cleanup in merge code","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-09-09T18:47:26Z","receivedAt":"2021-09-09T18:47:44Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This is on jt/grep-wo-submodule-odb-as-alternate (unlike version 1)\nbecause that branch introduces another usage of repo_submodule_init(),\nand I need to update it.\n\nThe main change from version 1 is that I have introduced a patch (patch\n2) that teaches repo_submodule_init() to support submodules with\nunabsorbed gitdirs, which means that I no longer need the original patch\n1 that updated t6437 to absorb all git dirs.\n\nOnce again, all these patches work under\nGIT_TEST_MERGE_ALGORITHM=recursive and GIT_TEST_MERGE_ALGORITHM=ort (and\nwhen that envvar is unset, for good measure).\n\nI have also tested it with the \"DO NOT SUBMIT\" patch 3 from version 1.\n\nJonathan Tan (3):\n submodule: remove unnecessary unabsorbed fallback\n repository: support unabsorbed in repo_submodule_init\n revision: remove \"submodule\" from opt struct\n\n builtin/grep.c | 5 +-\n builtin/ls-files.c | 4 +-\n builtin/submodule--helper.c | 7 +--\n merge-ort.c | 53 +++++++++++++-------\n merge-recursive.c | 49 +++++++++++-------\n repository.c | 21 ++++----\n repository.h | 15 +++---\n revision.c | 16 ++----\n revision.h | 1 -\n submodule.c | 24 ++-------\n t/helper/test-submodule-nested-repo-config.c | 4 +-\n 11 files changed, 102 insertions(+), 97 deletions(-)\n\n-- \n2.33.0.309.g3052b89438-goog\n\n"},{"id":"435291","messageId":"a71965545d940f57cb82cac925d5c937959ddf64.1631212893.git.jonathantanmy@google.com","threadId":"56467","inReplyTo":"cover.1631212893.git.jonathantanmy@google.com","subject":"[PATCH v2 1/3] submodule: remove unnecessary unabsorbed fallback","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-09-09T18:47:27Z","receivedAt":"2021-09-09T18:47:48Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In get_submodule_repo_for(), there is a fallback code path for the case\nin which a submodule has an unabsorbed gitdir. (See the documentation\nfor \"git submodule absorbgitdirs\" for more information about absorbed\nand unabsorbed gitdirs.) However, this code path is unnecessary, because\nsuch submodules are already handled: when the fetch_task is created in\nfetch_task_create(), it will create its own struct submodule with a path\nand name, and repo_submodule_init() can handle such a struct.\n\nThis fallback was introduced in 26f80ccfc1 (\"submodule: migrate\nget_next_submodule to use repository structs\", 2018-12-05). It was\nunnecessary even then, but perhaps it escaped notice because its parent\ncommit d5498e0871 (\"repository: repo_submodule_init to take a submodule\nstruct\", 2018-12-05) was the one that taught repo_submodule_init() to\nhandle such created structs. Before, it took a path and always checked\n.gitmodules, so it truly would have failed if there were no entry in\n.gitmodules.\n\n(Note to reviewers: in 26f80ccfc1, the \"own struct submodule\" I\nmentioned is in get_next_submodule(), not fetch_task_create().)\n\nSigned-off-by: Jonathan Tan \n---\n submodule.c | 15 ++-------------\n 1 file changed, 2 insertions(+), 13 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 8de1aecaeb..3af3da5b5e 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1409,19 +1409,8 @@ static struct repository *get_submodule_repo_for(struct repository *r,\n \tstruct repository *ret = xmalloc(sizeof(*ret));\n \n \tif (repo_submodule_init(ret, r, sub)) {\n-\t\t/*\n-\t\t * No entry in .gitmodules? Technically not a submodule,\n-\t\t * but historically we supported repositories that happen to be\n-\t\t * in-place where a gitlink is. Keep supporting them.\n-\t\t */\n-\t\tstruct strbuf gitdir = STRBUF_INIT;\n-\t\tstrbuf_repo_worktree_path(&gitdir, r, \"%s/.git\", sub->path);\n-\t\tif (repo_init(ret, gitdir.buf, NULL)) {\n-\t\t\tstrbuf_release(&gitdir);\n-\t\t\tfree(ret);\n-\t\t\treturn NULL;\n-\t\t}\n-\t\tstrbuf_release(&gitdir);\n+\t\tfree(ret);\n+\t\treturn NULL;\n \t}\n \n \treturn ret;\n-- \n2.33.0.309.g3052b89438-goog\n\n"},{"id":"435292","messageId":"39aca057cc88d1255d5b166cbf0630f8e9a3a5b0.1631212893.git.jonathantanmy@google.com","threadId":"56467","inReplyTo":"cover.1631212893.git.jonathantanmy@google.com","subject":"[PATCH v2 2/3] repository: support unabsorbed in repo_submodule_init","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-09-09T18:47:28Z","receivedAt":"2021-09-09T18:47:49Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In preparation for a subsequent commit that migrates code using\nadd_submodule_odb() to repo_submodule_init(), teach\nrepo_submodule_init() to support submodules with unabsorbed gitdirs.\n(See the documentation for \"git submodule absorbgitdirs\" for more\ninformation about absorbed and unabsorbed gitdirs.)\n\nSigned-off-by: Jonathan Tan \n---\n builtin/grep.c | 5 +----\n builtin/ls-files.c | 4 +---\n builtin/submodule--helper.c | 7 +------\n repository.c | 21 +++++++++++---------\n repository.h | 15 ++++++++------\n submodule.c | 9 +++------\n t/helper/test-submodule-nested-repo-config.c | 4 +---\n 7 files changed, 28 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 51278b01fa..8af5249a7b 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -433,17 +433,14 @@ static int grep_submodule(struct grep_opt *opt,\n {\n \tstruct repository *subrepo;\n \tstruct repository *superproject = opt->repo;\n-\tconst struct submodule *sub;\n \tstruct grep_opt subopt;\n \tint hit = 0;\n \n-\tsub = submodule_from_path(superproject, null_oid(), path);\n-\n \tif (!is_submodule_active(superproject, path))\n \t\treturn 0;\n \n \tsubrepo = xmalloc(sizeof(*subrepo));\n-\tif (repo_submodule_init(subrepo, superproject, sub)) {\n+\tif (repo_submodule_init(subrepo, superproject, path, null_oid())) {\n \t\tfree(subrepo);\n \t\treturn 0;\n \t}\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 29a26ad8ae..ec19bf54b2 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -209,10 +209,8 @@ static void show_submodule(struct repository *superproject,\n \t\t\t struct dir_struct *dir, const char *path)\n {\n \tstruct repository subrepo;\n-\tconst struct submodule *sub = submodule_from_path(superproject,\n-\t\t\t\t\t\t\t null_oid(), path);\n \n-\tif (repo_submodule_init(&subrepo, superproject, sub))\n+\tif (repo_submodule_init(&subrepo, superproject, path, null_oid()))\n \t\treturn;\n \n \tif (repo_read_index(&subrepo) < 0)\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ef2776a9e4..6718f202db 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2540,7 +2540,6 @@ static int push_check(int argc, const char **argv, const char *prefix)\n \n static int ensure_core_worktree(int argc, const char **argv, const char *prefix)\n {\n-\tconst struct submodule *sub;\n \tconst char *path;\n \tconst char *cw;\n \tstruct repository subrepo;\n@@ -2550,11 +2549,7 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix)\n \n \tpath = argv[1];\n \n-\tsub = submodule_from_path(the_repository, null_oid(), path);\n-\tif (!sub)\n-\t\tBUG(\"We could get the submodule handle before?\");\n-\n-\tif (repo_submodule_init(&subrepo, the_repository, sub))\n+\tif (repo_submodule_init(&subrepo, the_repository, path, null_oid()))\n \t\tdie(_(\"could not get a repository handle for submodule '%s'\"), path);\n \n \tif (!repo_config_get_string_tmp(&subrepo, \"core.worktree\", &cw)) {\ndiff --git a/repository.c b/repository.c\nindex b2bf44c6fa..e4a1afb0ac 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -190,19 +190,15 @@ int repo_init(struct repository *repo,\n \n int repo_submodule_init(struct repository *subrepo,\n \t\t\tstruct repository *superproject,\n-\t\t\tconst struct submodule *sub)\n+\t\t\tconst char *path,\n+\t\t\tconst struct object_id *treeish_name)\n {\n \tstruct strbuf gitdir = STRBUF_INIT;\n \tstruct strbuf worktree = STRBUF_INIT;\n \tint ret = 0;\n \n-\tif (!sub) {\n-\t\tret = -1;\n-\t\tgoto out;\n-\t}\n-\n-\tstrbuf_repo_worktree_path(&gitdir, superproject, \"%s/.git\", sub->path);\n-\tstrbuf_repo_worktree_path(&worktree, superproject, \"%s\", sub->path);\n+\tstrbuf_repo_worktree_path(&gitdir, superproject, \"%s/.git\", path);\n+\tstrbuf_repo_worktree_path(&worktree, superproject, \"%s\", path);\n \n \tif (repo_init(subrepo, gitdir.buf, worktree.buf)) {\n \t\t/*\n@@ -212,6 +208,13 @@ int repo_submodule_init(struct repository *subrepo,\n \t\t * in the superproject's 'modules' directory. In this case the\n \t\t * submodule would not have a worktree.\n \t\t */\n+\t\tconst struct submodule *sub =\n+\t\t\tsubmodule_from_path(superproject, treeish_name, path);\n+\t\tif (!sub) {\n+\t\t\tret = -1;\n+\t\t\tgoto out;\n+\t\t}\n+\n \t\tstrbuf_reset(&gitdir);\n \t\tstrbuf_repo_git_path(&gitdir, superproject,\n \t\t\t\t \"modules/%s\", sub->name);\n@@ -225,7 +228,7 @@ int repo_submodule_init(struct repository *subrepo,\n \tsubrepo->submodule_prefix = xstrfmt(\"%s%s/\",\n \t\t\t\t\t superproject->submodule_prefix ?\n \t\t\t\t\t superproject->submodule_prefix :\n-\t\t\t\t\t \"\", sub->path);\n+\t\t\t\t\t \"\", path);\n \n out:\n \tstrbuf_release(&gitdir);\ndiff --git a/repository.h b/repository.h\nindex 3740c93bc0..c24e177c7e 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -172,15 +172,18 @@ void initialize_the_repository(void);\n int repo_init(struct repository *r, const char *gitdir, const char *worktree);\n \n /*\n- * Initialize the repository 'subrepo' as the submodule given by the\n- * struct submodule 'sub' in parent repository 'superproject'.\n- * Return 0 upon success and a non-zero value upon failure, which may happen\n- * if the submodule is not found, or 'sub' is NULL.\n+ * Initialize the repository 'subrepo' as the submodule at the given path. If\n+ * the submodule's gitdir cannot be found at /.git, this function calls\n+ * submodule_from_path() to try to find it. treeish_name is only used if\n+ * submodule_from_path() needs to be called; see its documentation for more\n+ * information.\n+ * Return 0 upon success and a non-zero value upon failure.\n */\n-struct submodule;\n+struct object_id;\n int repo_submodule_init(struct repository *subrepo,\n \t\t\tstruct repository *superproject,\n-\t\t\tconst struct submodule *sub);\n+\t\t\tconst char *path,\n+\t\t\tconst struct object_id *treeish_name);\n void repo_clear(struct repository *repo);\n \n /*\ndiff --git a/submodule.c b/submodule.c\nindex 3af3da5b5e..ecda0229af 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -520,9 +520,6 @@ static void prepare_submodule_repo_env_in_gitdir(struct strvec *out)\n /*\n * Initialize a repository struct for a submodule based on the provided 'path'.\n *\n- * Unlike repo_submodule_init, this tolerates submodules not present\n- * in .gitmodules. This function exists only to preserve historical behavior,\n- *\n * Returns the repository struct on success,\n * NULL when the submodule is not present.\n */\n@@ -1404,11 +1401,11 @@ static void fetch_task_release(struct fetch_task *p)\n }\n \n static struct repository *get_submodule_repo_for(struct repository *r,\n-\t\t\t\t\t\t const struct submodule *sub)\n+\t\t\t\t\t\t const char *path)\n {\n \tstruct repository *ret = xmalloc(sizeof(*ret));\n \n-\tif (repo_submodule_init(ret, r, sub)) {\n+\tif (repo_submodule_init(ret, r, path, null_oid())) {\n \t\tfree(ret);\n \t\treturn NULL;\n \t}\n@@ -1452,7 +1449,7 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\tcontinue;\n \t\t}\n \n-\t\ttask->repo = get_submodule_repo_for(spf->r, task->sub);\n+\t\ttask->repo = get_submodule_repo_for(spf->r, task->sub->path);\n \t\tif (task->repo) {\n \t\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n \t\t\tchild_process_init(cp);\ndiff --git a/t/helper/test-submodule-nested-repo-config.c b/t/helper/test-submodule-nested-repo-config.c\nindex e3f11ff5a7..dc1c14bde3 100644\n--- a/t/helper/test-submodule-nested-repo-config.c\n+++ b/t/helper/test-submodule-nested-repo-config.c\n@@ -11,15 +11,13 @@ static void die_usage(const char **argv, const char *msg)\n int cmd__submodule_nested_repo_config(int argc, const char **argv)\n {\n \tstruct repository subrepo;\n-\tconst struct submodule *sub;\n \n \tif (argc < 3)\n \t\tdie_usage(argv, \"Wrong number of arguments.\");\n \n \tsetup_git_directory();\n \n-\tsub = submodule_from_path(the_repository, null_oid(), argv[1]);\n-\tif (repo_submodule_init(&subrepo, the_repository, sub)) {\n+\tif (repo_submodule_init(&subrepo, the_repository, argv[1], null_oid())) {\n \t\tdie_usage(argv, \"Submodule not found.\");\n \t}\n \n-- \n2.33.0.309.g3052b89438-goog\n\n"},{"id":"435293","messageId":"978174fdc25cdb7ee766a9e6a056145e127f52a4.1631212893.git.jonathantanmy@google.com","threadId":"56467","inReplyTo":"cover.1631212893.git.jonathantanmy@google.com","subject":"[PATCH v2 3/3] revision: remove \"submodule\" from opt struct","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-09-09T18:47:29Z","receivedAt":"2021-09-09T18:47:49Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Clean up a TODO in revision.h by removing the \"submodule\" field from\nstruct setup_revision_opt. This field is only used to specify the ref\nstore to use, so use rev_info->repo to determine the ref store instead.\n\nThe only users of this field are merge-ort.c and merge-recursive.c.\nHowever, both these files specify the superproject as rev_info->repo and\nthe submodule as setup_revision_opt->submodule. In order to be able to\npass the submodule as rev_info->repo, all commits must be parsed with\nthe submodule explicitly specified; this patch does that as well. (An\nincremental solution in which only some commits are parsed with explicit\nsubmodule will not work, because if the same commit is parsed twice in\ndifferent repositories, there will be 2 heap-allocated object structs\ncorresponding to that commit, and any flag set by the revision walking\nmechanism on one of them will not be reflected onto the other.)\n\nSigned-off-by: Jonathan Tan \n---\n merge-ort.c | 53 +++++++++++++++++++++++++++++++----------------\n merge-recursive.c | 49 ++++++++++++++++++++++++++++---------------\n revision.c | 16 +++++---------\n revision.h | 1 -\n 4 files changed, 72 insertions(+), 47 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 6eb910d6f0..b8efaee8e0 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -32,6 +32,7 @@\n #include \"promisor-remote.h\"\n #include \"revision.h\"\n #include \"strmap.h\"\n+#include \"submodule-config.h\"\n #include \"submodule.h\"\n #include \"tree.h\"\n #include \"unpack-trees.h\"\n@@ -1499,7 +1500,6 @@ static int find_first_merges(struct repository *repo,\n \txsnprintf(merged_revision, sizeof(merged_revision), \"^%s\",\n \t\t oid_to_hex(&a->object.oid));\n \trepo_init_revisions(repo, &revs, NULL);\n-\trev_opts.submodule = path;\n \t/* FIXME: can't handle linked worktrees in submodules yet */\n \trevs.single_worktree = path != NULL;\n \tsetup_revisions(ARRAY_SIZE(rev_args)-1, rev_args, &revs, &rev_opts);\n@@ -1509,7 +1509,7 @@ static int find_first_merges(struct repository *repo,\n \t\tdie(\"revision walk setup failed\");\n \twhile ((commit = get_revision(&revs)) != NULL) {\n \t\tstruct object *o = &(commit->object);\n-\t\tif (in_merge_bases(b, commit))\n+\t\tif (repo_in_merge_bases(repo, b, commit))\n \t\t\tadd_object_array(o, NULL, &merges);\n \t}\n \treset_revision_walk();\n@@ -1524,7 +1524,7 @@ static int find_first_merges(struct repository *repo,\n \t\tcontains_another = 0;\n \t\tfor (j = 0; j < merges.nr; j++) {\n \t\t\tstruct commit *m2 = (struct commit *) merges.objects[j].item;\n-\t\t\tif (i != j && in_merge_bases(m2, m1)) {\n+\t\t\tif (i != j && repo_in_merge_bases(repo, m2, m1)) {\n \t\t\t\tcontains_another = 1;\n \t\t\t\tbreak;\n \t\t\t}\n@@ -1545,10 +1545,12 @@ static int merge_submodule(struct merge_options *opt,\n \t\t\t const struct object_id *b,\n \t\t\t struct object_id *result)\n {\n+\tstruct repository subrepo;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tint ret = 0;\n \tstruct commit *commit_o, *commit_a, *commit_b;\n \tint parent_count;\n \tstruct object_array merges;\n-\tstruct strbuf sb = STRBUF_INIT;\n \n \tint i;\n \tint search = !opt->priv->call_depth;\n@@ -1564,6 +1566,10 @@ static int merge_submodule(struct merge_options *opt,\n \tif (is_null_oid(b))\n \t\treturn 0;\n \n+\t/*\n+\t * NEEDSWORK: Remove this when all submodule object accesses are\n+\t * through explicitly specified repositores.\n+\t */\n \tif (add_submodule_odb(path)) {\n \t\tpath_msg(opt, path, 0,\n \t\t\t _(\"Failed to merge submodule %s (not checked out)\"),\n@@ -1571,39 +1577,48 @@ static int merge_submodule(struct merge_options *opt,\n \t\treturn 0;\n \t}\n \n-\tif (!(commit_o = lookup_commit_reference(opt->repo, o)) ||\n-\t !(commit_a = lookup_commit_reference(opt->repo, a)) ||\n-\t !(commit_b = lookup_commit_reference(opt->repo, b))) {\n+\tif (repo_submodule_init(&subrepo, opt->repo, path, null_oid())) {\n+\t\tpath_msg(opt, path, 0,\n+\t\t\t\t_(\"Failed to merge submodule %s (not checked out)\"),\n+\t\t\t\tpath);\n+\t\treturn 0;\n+\t}\n+\n+\tif (!(commit_o = lookup_commit_reference(&subrepo, o)) ||\n+\t !(commit_a = lookup_commit_reference(&subrepo, a)) ||\n+\t !(commit_b = lookup_commit_reference(&subrepo, b))) {\n \t\tpath_msg(opt, path, 0,\n \t\t\t _(\"Failed to merge submodule %s (commits not present)\"),\n \t\t\t path);\n-\t\treturn 0;\n+\t\tgoto cleanup;\n \t}\n \n \t/* check whether both changes are forward */\n-\tif (!in_merge_bases(commit_o, commit_a) ||\n-\t !in_merge_bases(commit_o, commit_b)) {\n+\tif (!repo_in_merge_bases(&subrepo, commit_o, commit_a) ||\n+\t !repo_in_merge_bases(&subrepo, commit_o, commit_b)) {\n \t\tpath_msg(opt, path, 0,\n \t\t\t _(\"Failed to merge submodule %s \"\n \t\t\t \"(commits don't follow merge-base)\"),\n \t\t\t path);\n-\t\treturn 0;\n+\t\tgoto cleanup;\n \t}\n \n \t/* Case #1: a is contained in b or vice versa */\n-\tif (in_merge_bases(commit_a, commit_b)) {\n+\tif (repo_in_merge_bases(&subrepo, commit_a, commit_b)) {\n \t\toidcpy(result, b);\n \t\tpath_msg(opt, path, 1,\n \t\t\t _(\"Note: Fast-forwarding submodule %s to %s\"),\n \t\t\t path, oid_to_hex(b));\n-\t\treturn 1;\n+\t\tret = 1;\n+\t\tgoto cleanup;\n \t}\n-\tif (in_merge_bases(commit_b, commit_a)) {\n+\tif (repo_in_merge_bases(&subrepo, commit_b, commit_a)) {\n \t\toidcpy(result, a);\n \t\tpath_msg(opt, path, 1,\n \t\t\t _(\"Note: Fast-forwarding submodule %s to %s\"),\n \t\t\t path, oid_to_hex(a));\n-\t\treturn 1;\n+\t\tret = 1;\n+\t\tgoto cleanup;\n \t}\n \n \t/*\n@@ -1615,10 +1630,10 @@ static int merge_submodule(struct merge_options *opt,\n \n \t/* Skip the search if makes no sense to the calling context. */\n \tif (!search)\n-\t\treturn 0;\n+\t\tgoto cleanup;\n \n \t/* find commit which merges them */\n-\tparent_count = find_first_merges(opt->repo, path, commit_a, commit_b,\n+\tparent_count = find_first_merges(&subrepo, path, commit_a, commit_b,\n \t\t\t\t\t &merges);\n \tswitch (parent_count) {\n \tcase 0:\n@@ -1652,7 +1667,9 @@ static int merge_submodule(struct merge_options *opt,\n \t}\n \n \tobject_array_clear(&merges);\n-\treturn 0;\n+cleanup:\n+\trepo_clear(&subrepo);\n+\treturn ret;\n }\n \n static void initialize_attr_index(struct merge_options *opt)\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 3355d50e8a..fc8ac39d8c 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -24,6 +24,7 @@\n #include \"repository.h\"\n #include \"revision.h\"\n #include \"string-list.h\"\n+#include \"submodule-config.h\"\n #include \"submodule.h\"\n #include \"tag.h\"\n #include \"tree-walk.h\"\n@@ -1113,7 +1114,6 @@ static int find_first_merges(struct repository *repo,\n \txsnprintf(merged_revision, sizeof(merged_revision), \"^%s\",\n \t\t oid_to_hex(&a->object.oid));\n \trepo_init_revisions(repo, &revs, NULL);\n-\trev_opts.submodule = path;\n \t/* FIXME: can't handle linked worktrees in submodules yet */\n \trevs.single_worktree = path != NULL;\n \tsetup_revisions(ARRAY_SIZE(rev_args)-1, rev_args, &revs, &rev_opts);\n@@ -1123,7 +1123,7 @@ static int find_first_merges(struct repository *repo,\n \t\tdie(\"revision walk setup failed\");\n \twhile ((commit = get_revision(&revs)) != NULL) {\n \t\tstruct object *o = &(commit->object);\n-\t\tif (in_merge_bases(b, commit))\n+\t\tif (repo_in_merge_bases(repo, b, commit))\n \t\t\tadd_object_array(o, NULL, &merges);\n \t}\n \treset_revision_walk();\n@@ -1138,7 +1138,7 @@ static int find_first_merges(struct repository *repo,\n \t\tcontains_another = 0;\n \t\tfor (j = 0; j < merges.nr; j++) {\n \t\t\tstruct commit *m2 = (struct commit *) merges.objects[j].item;\n-\t\t\tif (i != j && in_merge_bases(m2, m1)) {\n+\t\t\tif (i != j && repo_in_merge_bases(repo, m2, m1)) {\n \t\t\t\tcontains_another = 1;\n \t\t\t\tbreak;\n \t\t\t}\n@@ -1174,6 +1174,8 @@ static int merge_submodule(struct merge_options *opt,\n \t\t\t const struct object_id *base, const struct object_id *a,\n \t\t\t const struct object_id *b)\n {\n+\tstruct repository subrepo;\n+\tint ret = 0;\n \tstruct commit *commit_base, *commit_a, *commit_b;\n \tint parent_count;\n \tstruct object_array merges;\n@@ -1197,27 +1199,36 @@ static int merge_submodule(struct merge_options *opt,\n \tif (is_null_oid(b))\n \t\treturn 0;\n \n+\t/*\n+\t * NEEDSWORK: Remove this when all submodule object accesses are\n+\t * through explicitly specified repositores.\n+\t */\n \tif (add_submodule_odb(path)) {\n \t\toutput(opt, 1, _(\"Failed to merge submodule %s (not checked out)\"), path);\n \t\treturn 0;\n \t}\n \n-\tif (!(commit_base = lookup_commit_reference(opt->repo, base)) ||\n-\t !(commit_a = lookup_commit_reference(opt->repo, a)) ||\n-\t !(commit_b = lookup_commit_reference(opt->repo, b))) {\n-\t\toutput(opt, 1, _(\"Failed to merge submodule %s (commits not present)\"), path);\n+\tif (repo_submodule_init(&subrepo, opt->repo, path, null_oid())) {\n+\t\toutput(opt, 1, _(\"Failed to merge submodule %s (not checked out)\"), path);\n \t\treturn 0;\n \t}\n \n+\tif (!(commit_base = lookup_commit_reference(&subrepo, base)) ||\n+\t !(commit_a = lookup_commit_reference(&subrepo, a)) ||\n+\t !(commit_b = lookup_commit_reference(&subrepo, b))) {\n+\t\toutput(opt, 1, _(\"Failed to merge submodule %s (commits not present)\"), path);\n+\t\tgoto cleanup;\n+\t}\n+\n \t/* check whether both changes are forward */\n-\tif (!in_merge_bases(commit_base, commit_a) ||\n-\t !in_merge_bases(commit_base, commit_b)) {\n+\tif (!repo_in_merge_bases(&subrepo, commit_base, commit_a) ||\n+\t !repo_in_merge_bases(&subrepo, commit_base, commit_b)) {\n \t\toutput(opt, 1, _(\"Failed to merge submodule %s (commits don't follow merge-base)\"), path);\n-\t\treturn 0;\n+\t\tgoto cleanup;\n \t}\n \n \t/* Case #1: a is contained in b or vice versa */\n-\tif (in_merge_bases(commit_a, commit_b)) {\n+\tif (repo_in_merge_bases(&subrepo, commit_a, commit_b)) {\n \t\toidcpy(result, b);\n \t\tif (show(opt, 3)) {\n \t\t\toutput(opt, 3, _(\"Fast-forwarding submodule %s to the following commit:\"), path);\n@@ -1227,9 +1238,10 @@ static int merge_submodule(struct merge_options *opt,\n \t\telse\n \t\t\t; /* no output */\n \n-\t\treturn 1;\n+\t\tret = 1;\n+\t\tgoto cleanup;\n \t}\n-\tif (in_merge_bases(commit_b, commit_a)) {\n+\tif (repo_in_merge_bases(&subrepo, commit_b, commit_a)) {\n \t\toidcpy(result, a);\n \t\tif (show(opt, 3)) {\n \t\t\toutput(opt, 3, _(\"Fast-forwarding submodule %s to the following commit:\"), path);\n@@ -1239,7 +1251,8 @@ static int merge_submodule(struct merge_options *opt,\n \t\telse\n \t\t\t; /* no output */\n \n-\t\treturn 1;\n+\t\tret = 1;\n+\t\tgoto cleanup;\n \t}\n \n \t/*\n@@ -1251,10 +1264,10 @@ static int merge_submodule(struct merge_options *opt,\n \n \t/* Skip the search if makes no sense to the calling context. */\n \tif (!search)\n-\t\treturn 0;\n+\t\tgoto cleanup;\n \n \t/* find commit which merges them */\n-\tparent_count = find_first_merges(opt->repo, &merges, path,\n+\tparent_count = find_first_merges(&subrepo, &merges, path,\n \t\t\t\t\t commit_a, commit_b);\n \tswitch (parent_count) {\n \tcase 0:\n@@ -1281,7 +1294,9 @@ static int merge_submodule(struct merge_options *opt,\n \t}\n \n \tobject_array_clear(&merges);\n-\treturn 0;\n+cleanup:\n+\trepo_clear(&subrepo);\n+\treturn ret;\n }\n \n static int merge_mode_and_contents(struct merge_options *opt,\ndiff --git a/revision.c b/revision.c\nindex cddd0542a6..31fc1884d2 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2561,8 +2561,7 @@ static int for_each_good_bisect_ref(struct ref_store *refs, each_ref_fn fn, void\n \treturn for_each_bisect_ref(refs, fn, cb_data, term_good);\n }\n \n-static int handle_revision_pseudo_opt(const char *submodule,\n-\t\t\t\t struct rev_info *revs,\n+static int handle_revision_pseudo_opt(struct rev_info *revs,\n \t\t\t\t const char **argv, int *flags)\n {\n \tconst char *arg = argv[0];\n@@ -2570,7 +2569,7 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \tstruct ref_store *refs;\n \tint argcount;\n \n-\tif (submodule) {\n+\tif (revs->repo != the_repository) {\n \t\t/*\n \t\t * We need some something like get_submodule_worktrees()\n \t\t * before we can go through all worktrees of a submodule,\n@@ -2579,9 +2578,8 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \t\t */\n \t\tif (!revs->single_worktree)\n \t\t\tBUG(\"--single-worktree cannot be used together with submodule\");\n-\t\trefs = get_submodule_ref_store(submodule);\n-\t} else\n-\t\trefs = get_main_ref_store(revs->repo);\n+\t}\n+\trefs = get_main_ref_store(revs->repo);\n \n \t/*\n \t * NOTE!\n@@ -2699,12 +2697,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n {\n \tint i, flags, left, seen_dashdash, revarg_opt;\n \tstruct strvec prune_data = STRVEC_INIT;\n-\tconst char *submodule = NULL;\n \tint seen_end_of_options = 0;\n \n-\tif (opt)\n-\t\tsubmodule = opt->submodule;\n-\n \t/* First, search for \"--\" */\n \tif (opt && opt->assume_dashdash) {\n \t\tseen_dashdash = 1;\n@@ -2733,7 +2727,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\tif (!seen_end_of_options && *arg == '-') {\n \t\t\tint opts;\n \n-\t\t\topts = handle_revision_pseudo_opt(submodule,\n+\t\t\topts = handle_revision_pseudo_opt(\n \t\t\t\t\t\trevs, argv + i,\n \t\t\t\t\t\t&flags);\n \t\t\tif (opts > 0) {\ndiff --git a/revision.h b/revision.h\nindex fbb068da9f..d8e0e93026 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -339,7 +339,6 @@ extern volatile show_early_output_fn_t show_early_output;\n struct setup_revision_opt {\n \tconst char *def;\n \tvoid (*tweak)(struct rev_info *, struct setup_revision_opt *);\n-\tconst char *submodule;\t/* TODO: drop this and use rev_info->repo */\n \tunsigned int\tassume_dashdash:1,\n \t\t\tallow_exclude_promisor_objects:1;\n \tunsigned revarg_opt;\n-- \n2.33.0.309.g3052b89438-goog\n\n"},{"id":"435803","messageId":"CABPp-BFrDgh02gOmX9SPrF8nXkQ8uyBkvt-cKyLUGtrfsx4HKg@mail.gmail.com","threadId":"56467","inReplyTo":"cover.1631212893.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 0/3] More add_submodule_odb() cleanup in merge code","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-09-14T01:31:04Z","receivedAt":"2021-09-14T01:31:19Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Sep 9, 2021 at 11:50 AM Jonathan Tan wrote:\n>\n> This is on jt/grep-wo-submodule-odb-as-alternate (unlike version 1)\n> because that branch introduces another usage of repo_submodule_init(),\n> and I need to update it.\n>\n> The main change from version 1 is that I have introduced a patch (patch\n> 2) that teaches repo_submodule_init() to support submodules with\n> unabsorbed gitdirs, which means that I no longer need the original patch\n> 1 that updated t6437 to absorb all git dirs.\n>\n> Once again, all these patches work under\n> GIT_TEST_MERGE_ALGORITHM=recursive and GIT_TEST_MERGE_ALGORITHM=ort (and\n> when that envvar is unset, for good measure).\n>\n> I have also tested it with the \"DO NOT SUBMIT\" patch 3 from version 1.\n\nWhile I don't yet know the details of repositories and submodules, I\nknow the merge-recursive and merge-ort side of things and those\nchanges all look reasonable to me. So here's an Ack for that side of\npatch 3.\n\nDespite not being familiar with the repository and submodule storage\nparts of the code, I took a look over the remainder of the 3 patches\nto look for anything that might stand out and look amiss. I didn't\nspot anything; it all looks reasonable to me.\n"}]}