[PATCH v2 0/9] fetch --recurse-submodules: fetch unpopulated submodules
- From
Glen Choo <chooglen@google.com>
- Date
- Feb 15, 2022, 17:23 UTC
- Message-ID
- <20220215172318.73533-1-chooglen@google.com>
- In-Reply-To
- <20220210044152.78352-1-chooglen@google.com>
Original cover letter: https://lore.kernel.org/git/20220210044152.78352-1-chooglen@google.com
This series is based on gc/branch-recurse-submodules.
Thanks for the kind feedback :) I believe this version addresses all of the feedback, though I suspect that I might not have given clear-enough answers to all of the questions. Let me know if more clarification is needed.
= Patch organization
The patches are organized somewhat differently vs v1. During review of v1, I realized that it didn't make sense to read submodules from the superproject commit until we actually fetch unpopulated submodules. So this version just has all of the C patches together, instead of putting the test suite patches after "read submodules from the superproject commit".
- Patches 1-2 are quality-of-life improvements to the test suite that make it easier to write the tests in patch 7. - Patches 3-5 are preparation for "git fetch" to read .gitmodules from the superproject commit in patch 7. - Patch 6 separates the steps of "finding which submodules to fetch" and "fetching the submodules", making it easier to tell "git fetch" to fetch unpopulated submodules. - Patch 7 teaches "git fetch" to fetch changed, unpopulated submodules in addition to populated submodules. - Patch 8 is an optional bugfix that fixes a bug with "git fetch --update-shallow" in a repository with submodules. This was discovered in v1 but is no longer necessary for tests to pass. - Patch 9 is an optional bugfix + cleanup of the "git fetch" code that removes the last caller of the deprecated "add_submodule_odb()".
= Changes
Since v1:
- Numerous style fixes suggested by Jonathan (thanks!)
- In patch 3, don't prematurely read submodules from the superproject
commit (see:
<kl6l5yplyat6.fsf@chooglen-macbookpro.roam.corp.google.com>).
- In patch 7, stop using "git checkout" and "! grep" in tests.
- In patch 7, stop doing the "find changed submodules" rev walk
unconditionally. Instead, continue to check for .gitmodules, but also
check for submodules in $GIT_DIR/modules.
- I'm not entirely happy with the helper function name, see "---" for
details.
- Move "git fetch --update-shallow" bugfix to patch 8.
- Because the "find changed submodules" rev walk is no longer
unconditional, this fix is no longer needed for tests to pass.
- Rename fetch_populated_submodules() to fetch_submodules().Glen Choo (9): t5526: introduce test helper to assert on fetches t5526: use grep to assert on fetches submodule: make static functions read submodules from commits submodule: inline submodule_commits() into caller submodule: store new submodule commits oid_array in a struct submodule: extract get_fetch_task() fetch: fetch unpopulated, changed submodules submodule: read shallows when finding changed submodules submodule: fix latent check_has_commit() bug
Documentation/fetch-options.txt | 26 ++- Documentation/git-fetch.txt | 10 +- builtin/fetch.c | 14 +- submodule.c | 321 +++++++++++++++++---------- submodule.h | 21 +- t/t5526-fetch-submodules.sh | 374 ++++++++++++++++++++++++-------- 6 files changed, 534 insertions(+), 232 deletions(-)
Range-diff against v1:
4: 432aa60296 = 1: a159cdaabb t5526: introduce test helper to assert on fetches
5: 9515d22804 = 2: 48894c6c43 t5526: use grep to assert on fetches
3: 3c28ea743c ! 3: 6cf5e76d62 submodule: make static functions read submodules from commits
@@ Commit message
submodule: make static functions read submodules from commits
A future commit will teach "fetch --recurse-submodules" to fetch
- unpopulated submodules. Prepare for this by teaching the necessary
- static functions to read submodules from superproject commits instead of
- the index and filesystem. Then, store the necessary fields (path and
- super_oid), and use them in "fetch --recurse-submodules" where possible.
+ unpopulated submodules. To prepare for this, teach the necessary static
+ functions how to read submodules from superproject commits using a
+ "treeish_name" argument (instead of always reading from the index and
+ filesystem) but do not actually change where submodules are read from.
+ Submodules will be read from commits when we fetch unpopulated
+ submodules.
- As a result, "git fetch" now reads changed submodules using the
- `.gitmodules` and path from super_oid's tree (which is where "git fetch"
- actually noticed the changed submodule) instead of the filesystem.
+ The changed function signatures follow repo_submodule_init()'s argument
+ order, i.e. "path" then "treeish_name". Where needed, reorder the
+ arguments of functions that already take "path" and "treeish_name" to be
+ consistent with this convention.
Signed-off-by: Glen Choo <chooglen@google.com>
## submodule.c ##
-@@ submodule.c: static const char *default_name_or_path(const char *path_or_name)
- * member of the changed submodule string_list_item.
- */
- struct changed_submodule_data {
-+ /*
-+ * The first superproject commit in the rev walk that points to the
-+ * submodule.
-+ */
-+ const struct object_id *super_oid;
-+ /*
-+ * Path to the submodule in the superproject commit referenced
-+ * by 'super_oid'.
-+ */
-+ char *path;
- /* The submodule commits that have changed in the rev walk. */
- struct oid_array *new_commits;
- };
-@@ submodule.c: static void changed_submodule_data_clear(struct changed_submodule_data *cs_data)
- {
- oid_array_clear(cs_data->new_commits);
- free(cs_data->new_commits);
-+ free(cs_data->path);
- }
-
- static void collect_changed_submodules_cb(struct diff_queue_struct *q,
-@@ submodule.c: static void collect_changed_submodules_cb(struct diff_queue_struct *q,
- cs_data = xcalloc(1, sizeof(struct changed_submodule_data));
- /* NEEDSWORK: should we have oid_array_init()? */
- cs_data->new_commits = xcalloc(1, sizeof(struct oid_array));
-+ cs_data->super_oid = commit_oid;
-+ cs_data->path = xstrdup(p->two->path);
- item->util = cs_data;
- }
- oid_array_append(cs_data->new_commits, &p->two->oid);
@@ submodule.c: struct has_commit_data {
struct repository *repo;
int result;
@@ submodule.c: static int submodule_needs_pushing(struct repository *r,
/*
* NOTE: We do consider it safe to return "no" here. The
* correct answer would be "We do not know" instead of
-@@ submodule.c: static void calculate_changed_submodule_paths(struct repository *r,
- const struct submodule *submodule;
- const char *path = NULL;
-
-- submodule = submodule_from_name(r, null_oid(), name->string);
-+ submodule = submodule_from_name(r, cs_data->super_oid, name->string);
- if (submodule)
- path = submodule->path;
- else
@@ submodule.c: static void calculate_changed_submodule_paths(struct repository *r,
if (!path)
continue;
-- if (submodule_has_commits(r, path, cs_data->new_commits)) {
-+ if (submodule_has_commits(r, path, cs_data->super_oid, cs_data->new_commits)) {
- oid_array_clear(cs_data->new_commits);
+- if (submodule_has_commits(r, path, commits)) {
++ if (submodule_has_commits(r, path, null_oid(), commits)) {
+ oid_array_clear(commits);
*name->string = '\0';
}
@@ submodule.c: static const struct submodule *get_non_gitmodules_submodule(const char *path)
1: a8ef64d16c ! 4: 07fd4ff0a9 submodule: inline submodule_commits() into caller
@@ Commit message
Prepare for this change by inlining submodule_commits() (which inserts
into the string_list and initializes the string_list_item.util) into its
- only caller. This simplifies the code and makes it easier for the caller
- to add information to the string_list_item.util.
+ only caller so that the code is easier to refactor later.
Signed-off-by: Glen Choo <chooglen@google.com>
2: 11e48fbd41 ! 5: f049cb231b submodule: store new submodule commits oid_array in a struct
@@ submodule.c: static const char *default_name_or_path(const char *path_or_name)
+ */
+struct changed_submodule_data {
+ /* The submodule commits that have changed in the rev walk. */
-+ struct oid_array *new_commits;
++ struct oid_array new_commits;
+};
+
+static void changed_submodule_data_clear(struct changed_submodule_data *cs_data)
+{
-+ oid_array_clear(cs_data->new_commits);
-+ free(cs_data->new_commits);
++ oid_array_clear(&cs_data->new_commits);
+}
+
static void collect_changed_submodules_cb(struct diff_queue_struct *q,
@@ submodule.c: static void collect_changed_submodules_cb(struct diff_queue_struct
if (!S_ISGITLINK(p->two->mode))
continue;
@@ submodule.c: static void collect_changed_submodules_cb(struct diff_queue_struct *q,
- continue;
item = string_list_insert(changed, name);
-- if (!item->util)
-+ if (item->util)
-+ cs_data = item->util;
-+ else {
-+ cs_data = xcalloc(1, sizeof(struct changed_submodule_data));
- /* NEEDSWORK: should we have oid_array_init()? */
+ if (!item->util)
+- /* NEEDSWORK: should we have oid_array_init()? */
- item->util = xcalloc(1, sizeof(struct oid_array));
- oid_array_append(item->util, &p->two->oid);
-+ cs_data->new_commits = xcalloc(1, sizeof(struct oid_array));
-+ item->util = cs_data;
-+ }
-+ oid_array_append(cs_data->new_commits, &p->two->oid);
++ item->util = xcalloc(1, sizeof(struct changed_submodule_data));
++ cs_data = item->util;
++ oid_array_append(&cs_data->new_commits, &p->two->oid);
}
}
@@ submodule.c: int find_unpushed_submodules(struct repository *r,
continue;
- if (submodule_needs_pushing(r, path, commits))
-+ if (submodule_needs_pushing(r, path, cs_data->new_commits))
++ if (submodule_needs_pushing(r, path, &cs_data->new_commits))
string_list_insert(needs_pushing, path);
}
@@ submodule.c: static void calculate_changed_submodule_paths(struct repository *r,
if (!path)
continue;
-- if (submodule_has_commits(r, path, commits)) {
+- if (submodule_has_commits(r, path, null_oid(), commits)) {
- oid_array_clear(commits);
-+ if (submodule_has_commits(r, path, cs_data->new_commits)) {
-+ oid_array_clear(cs_data->new_commits);
++ if (submodule_has_commits(r, path, null_oid(), &cs_data->new_commits)) {
++ changed_submodule_data_clear(cs_data);
*name->string = '\0';
}
}
@@ submodule.c: static int fetch_finish(int retvalue, struct strbuf *err,
- commits = it->util;
- oid_array_filter(commits,
+ cs_data = it->util;
-+ oid_array_filter(cs_data->new_commits,
++ oid_array_filter(&cs_data->new_commits,
commit_missing_in_sub,
task->repo);
/* Are there commits we want, but do not exist? */
- if (commits->nr) {
- task->commits = commits;
-+ if (cs_data->new_commits->nr) {
-+ task->commits = cs_data->new_commits;
++ if (cs_data->new_commits.nr) {
++ task->commits = &cs_data->new_commits;
ALLOC_GROW(spf->oid_fetch_tasks,
spf->oid_fetch_tasks_nr + 1,
spf->oid_fetch_tasks_alloc);
6: db6de6617b ! 6: 814073eecc submodule: extract get_fetch_task()
@@ submodule.c: static int get_next_submodule(struct child_process *cp,
break;
case RECURSE_SUBMODULES_OFF:
continue;
- }
+@@ submodule.c: static int get_next_submodule(struct child_process *cp,
task->repo = get_submodule_repo_for(spf->r, task->sub->path, null_oid());
-- if (task->repo) {
+ if (task->repo) {
- struct strbuf submodule_prefix = STRBUF_INIT;
- child_process_init(cp);
- cp->dir = task->repo->gitdir;
- prepare_submodule_repo_env_in_gitdir(&cp->env_array);
- cp->git_cmd = 1;
-- if (!spf->quiet)
-- strbuf_addf(err, _("Fetching submodule %s%s\n"),
-- spf->prefix, ce->name);
+ if (!spf->quiet)
+ strbuf_addf(err, _("Fetching submodule %s%s\n"),
+ spf->prefix, ce->name);
- strvec_init(&cp->args);
- strvec_pushv(&cp->args, spf->args.v);
- strvec_push(&cp->args, default_argv);
@@ submodule.c: static int get_next_submodule(struct child_process *cp,
- spf->prefix,
- task->sub->path);
- strvec_push(&cp->args, submodule_prefix.buf);
--
-- spf->count++;
+
+ spf->count++;
- *task_cb = task;
-
- strbuf_release(&submodule_prefix);
- return 1;
-- } else {
-+ if (!task->repo) {
++ return task;
+ } else {
struct strbuf empty_submodule_path = STRBUF_INIT;
- fetch_task_release(task);
@@ submodule.c: static int get_next_submodule(struct child_process *cp,
- ce->name);
- }
strbuf_release(&empty_submodule_path);
-+ continue;
}
-+ if (!spf->quiet)
-+ strbuf_addf(err, _("Fetching submodule %s%s\n"),
-+ spf->prefix, ce->name);
-+
-+ spf->count++;
-+ return task;
-+ }
+ }
+ return NULL;
+}
+
@@ submodule.c: static int get_next_submodule(struct child_process *cp,
+ strvec_push(&cp->args, default_argv);
+ strvec_push(&cp->args, "--submodule-prefix");
+
-+ strbuf_addf(&submodule_prefix, "%s%s/", spf->prefix,
-+ task->sub->path);
++ strbuf_addf(&submodule_prefix, "%s%s/",
++ spf->prefix,
++ task->sub->path);
+ strvec_push(&cp->args, submodule_prefix.buf);
+ *task_cb = task;
+
+ strbuf_release(&submodule_prefix);
+ return 1;
- }
++ }
if (spf->oid_fetch_tasks_nr) {
+ struct fetch_task *task =
7: 1737338380 ! 7: 10fd5bf921 fetch: fetch unpopulated, changed submodules
@@ Documentation/git-fetch.txt: include::transfer-data-leaks.txt[]
SEE ALSO
--------
+ ## builtin/fetch.c ##
+@@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)
+ max_children = fetch_parallel_config;
+
+ add_options_to_argv(&options);
+- result = fetch_populated_submodules(the_repository,
+- &options,
+- submodule_prefix,
+- recurse_submodules,
+- recurse_submodules_default,
+- verbosity < 0,
+- max_children);
++ result = fetch_submodules(the_repository,
++ &options,
++ submodule_prefix,
++ recurse_submodules,
++ recurse_submodules_default,
++ verbosity < 0,
++ max_children);
+ strvec_clear(&options);
+ }
+
+
## submodule.c ##
-@@
- #include "parse-options.h"
- #include "object-store.h"
- #include "commit-reach.h"
-+#include "shallow.h"
+@@ submodule.c: static const char *default_name_or_path(const char *path_or_name)
+ * member of the changed submodule string_list_item.
+ */
+ struct changed_submodule_data {
++ /*
++ * The first superproject commit in the rev walk that points to the
++ * submodule.
++ */
++ const struct object_id *super_oid;
++ /*
++ * Path to the submodule in the superproject commit referenced
++ * by 'super_oid'.
++ */
++ char *path;
+ /* The submodule commits that have changed in the rev walk. */
+ struct oid_array new_commits;
+ };
+@@ submodule.c: struct changed_submodule_data {
+ static void changed_submodule_data_clear(struct changed_submodule_data *cs_data)
+ {
+ oid_array_clear(&cs_data->new_commits);
++ free(cs_data->path);
+ }
- static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
- static int initialized_fetch_ref_tips;
-@@ submodule.c: static void collect_changed_submodules(struct repository *r,
+ static void collect_changed_submodules_cb(struct diff_queue_struct *q,
+@@ submodule.c: static void collect_changed_submodules_cb(struct diff_queue_struct *q,
+ if (!item->util)
+ item->util = xcalloc(1, sizeof(struct changed_submodule_data));
+ cs_data = item->util;
++ cs_data->super_oid = commit_oid;
++ cs_data->path = xstrdup(p->two->path);
+ oid_array_append(&cs_data->new_commits, &p->two->oid);
+ }
+ }
+@@ submodule.c: void check_for_new_submodule_commits(struct object_id *oid)
+ oid_array_append(&ref_tips_after_fetch, oid);
+ }
- save_warning = warn_on_object_refname_ambiguity;
- warn_on_object_refname_ambiguity = 0;
-+ /* make sure shallows are read */
-+ is_repository_shallow(the_repository);
++/*
++ * Returns 1 if the repo has absorbed submodule gitdirs, and 0
++ * otherwise. Like submodule_name_to_gitdir(), this checks
++ * $GIT_DIR/modules, not $GIT_COMMON_DIR.
++ */
++static int repo_has_absorbed_submodules(struct repository *r)
++{
++ struct strbuf buf = STRBUF_INIT;
+
- repo_init_revisions(r, &rev, NULL);
- setup_revisions(argv->nr, argv->v, &rev, &s_r_opt);
- warn_on_object_refname_ambiguity = save_warning;
-@@ submodule.c: static void calculate_changed_submodule_paths(struct repository *r,
++ strbuf_repo_git_path(&buf, r, "modules/");
++ return file_exists(buf.buf) && !is_empty_dir(buf.buf);
++}
++
+ static void calculate_changed_submodule_paths(struct repository *r,
+ struct string_list *changed_submodule_names)
+ {
struct strvec argv = STRVEC_INIT;
struct string_list_item *name;
- /* No need to check if there are no submodules configured */
- if (!submodule_from_path(r, NULL, NULL))
-- return;
--
++ /* No need to check if no submodules would be fetched */
++ if (!submodule_from_path(r, NULL, NULL) &&
++ !repo_has_absorbed_submodules(r))
+ return;
+
strvec_push(&argv, "--"); /* argv[0] program name */
- oid_array_for_each_unique(&ref_tips_after_fetch,
- append_oid_to_argv, &argv);
@@ submodule.c: int submodule_touches_in_range(struct repository *r,
}
@@ submodule.c: get_fetch_task(struct submodule_parallel_fetch *spf,
{
default:
@@ submodule.c: get_fetch_task(struct submodule_parallel_fetch *spf,
- strbuf_addf(err, _("Fetching submodule %s%s\n"),
- spf->prefix, ce->name);
+ strbuf_addf(err, _("Fetching submodule %s%s\n"),
+ spf->prefix, ce->name);
+
+- spf->count++;
++ spf->index_count++;
+ return task;
+ } else {
+ struct strbuf empty_submodule_path = STRBUF_INIT;
+@@ submodule.c: get_fetch_task(struct submodule_parallel_fetch *spf,
+ return NULL;
+ }
-- spf->count++;
-+ spf->index_count++;
-+ return task;
-+ }
-+ return NULL;
-+}
-+
+static struct fetch_task *
+get_fetch_task_from_changed(struct submodule_parallel_fetch *spf,
+ const char **default_argv, struct strbuf *err)
@@ submodule.c: get_fetch_task(struct submodule_parallel_fetch *spf,
+ find_unique_abbrev(cs_data->super_oid,
+ DEFAULT_ABBREV));
+ spf->changed_count++;
- return task;
- }
- return NULL;
-@@ submodule.c: static int get_next_submodule(struct child_process *cp, struct strbuf *err,
++ return task;
++ }
++ return NULL;
++}
++
+ static int get_next_submodule(struct child_process *cp, struct strbuf *err,
+ void *data, void **task_cb)
{
struct submodule_parallel_fetch *spf = data;
const char *default_argv = NULL;
@@ submodule.c: static int get_next_submodule(struct child_process *cp, struct strb
return 1;
}
+@@ submodule.c: static int fetch_finish(int retvalue, struct strbuf *err,
+ return 0;
+ }
+
+-int fetch_populated_submodules(struct repository *r,
+- const struct strvec *options,
+- const char *prefix, int command_line_option,
+- int default_option,
+- int quiet, int max_parallel_jobs)
++int fetch_submodules(struct repository *r,
++ const struct strvec *options,
++ const char *prefix, int command_line_option,
++ int default_option,
++ int quiet, int max_parallel_jobs)
+ {
+ int i;
+ struct submodule_parallel_fetch spf = SPF_INIT;
+
+ ## submodule.h ##
+@@ submodule.h: int should_update_submodules(void);
+ */
+ const struct submodule *submodule_from_ce(const struct cache_entry *ce);
+ void check_for_new_submodule_commits(struct object_id *oid);
+-int fetch_populated_submodules(struct repository *r,
+- const struct strvec *options,
+- const char *prefix,
+- int command_line_option,
+- int default_option,
+- int quiet, int max_parallel_jobs);
++int fetch_submodules(struct repository *r,
++ const struct strvec *options,
++ const char *prefix,
++ int command_line_option,
++ int default_option,
++ int quiet, int max_parallel_jobs);
+ unsigned is_submodule_modified(const char *path, int ignore_untracked);
+ int submodule_uses_gitfile(const char *path);
+
## t/t5526-fetch-submodules.sh ##
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-demand' recurses as deep as necess
verify_fetch_result actual.err
'
-+# Cleans up after tests that checkout branches other than the main ones
-+# in the tests.
-+checkout_main_branches() {
-+ git -C downstream checkout --recurse-submodules super &&
-+ git -C downstream/submodule checkout --recurse-submodules sub &&
-+ git -C downstream/submodule/subdir/deepsubmodule checkout --recurse-submodules deep
-+}
-+
+# Test that we can fetch submodules in other branches by running fetch
-+# in a branch that has no submodules.
++# in a commit that has no submodules.
+test_expect_success 'setup downstream branch without submodules' '
+ (
+ cd downstream &&
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-deman
+'
+
+test_expect_success "'--recurse-submodules=on-demand' should fetch submodule commits if the submodule is changed but the index has no submodules" '
-+ test_when_finished "checkout_main_branches" &&
+ git -C downstream fetch --recurse-submodules &&
+ # Create new superproject commit with updated submodules
+ add_upstream_commit &&
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-deman
+ (
+ cd downstream &&
+ git switch --recurse-submodules no-submodules &&
-+ git fetch --recurse-submodules=on-demand >../actual.out 2>../actual.err &&
-+ git checkout --recurse-submodules origin/super 2>../actual-checkout.err
++ git fetch --recurse-submodules=on-demand >../actual.out 2>../actual.err
+ ) &&
+ test_must_be_empty actual.out &&
+ git rev-parse --short HEAD >superhead &&
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-deman
+
+ # Assert that the fetch happened at the non-HEAD commits
+ grep "Fetching submodule submodule at commit $superhead" actual.err &&
-+ grep "Fetching submodule submodule/subdir/deepsubmodule at commit $subhead" actual.err &&
-+
-+ # Assert that we can checkout the superproject commit with --recurse-submodules
-+ ! grep -E "error: Submodule .+ could not be updated" actual-checkout.err
++ grep "Fetching submodule submodule/subdir/deepsubmodule at commit $subhead" actual.err
+'
+
+test_expect_success "'--recurse-submodules' should fetch submodule commits if the submodule is changed but the index has no submodules" '
-+ test_when_finished "checkout_main_branches" &&
+ # Fetch any leftover commits from other tests.
+ git -C downstream fetch --recurse-submodules &&
+ # Create new superproject commit with updated submodules
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-deman
+ (
+ cd downstream &&
+ git switch --recurse-submodules no-submodules &&
-+ git fetch --recurse-submodules >../actual.out 2>../actual.err &&
-+ git checkout --recurse-submodules origin/super 2>../actual-checkout.err
++ git fetch --recurse-submodules >../actual.out 2>../actual.err
+ ) &&
+ test_must_be_empty actual.out &&
+ git rev-parse --short HEAD >superhead &&
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-deman
+
+ # Assert that the fetch happened at the non-HEAD commits
+ grep "Fetching submodule submodule at commit $superhead" actual.err &&
-+ grep "Fetching submodule submodule/subdir/deepsubmodule at commit $subhead" actual.err &&
-+
-+ # Assert that we can checkout the superproject commit with --recurse-submodules
-+ ! grep -E "error: Submodule .+ could not be updated" actual-checkout.err
++ grep "Fetching submodule submodule/subdir/deepsubmodule at commit $subhead" actual.err
+'
+
+test_expect_success "'--recurse-submodules' should ignore changed, inactive submodules" '
-+ test_when_finished "checkout_main_branches" &&
+ # Fetch any leftover commits from other tests.
+ git -C downstream fetch --recurse-submodules &&
+ # Create new superproject commit with updated submodules
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-deman
+ verify_fetch_result actual.err
+'
+
-+# Test that we properly fetch the submodules in the index as well as
-+# submodules in other branches.
++# In downstream, init "submodule2", but do not check it out while
++# fetching. This lets us assert that unpopulated submodules can be
++# fetched.
+test_expect_success 'setup downstream branch with other submodule' '
+ mkdir submodule2 &&
+ (
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-deman
+'
+
+test_expect_success "'--recurse-submodules' should fetch submodule commits in changed submodules and the index" '
-+ test_when_finished "checkout_main_branches" &&
+ # Fetch any leftover commits from other tests.
+ git -C downstream fetch --recurse-submodules &&
+ # Create new commit in origin/super
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-deman
+ git checkout super &&
+ (
+ cd downstream &&
-+ git fetch --recurse-submodules >../actual.out 2>../actual.err &&
-+ git checkout --recurse-submodules origin/super-sub2-only 2>../actual-checkout.err
++ git fetch --recurse-submodules >../actual.out 2>../actual.err
+ ) &&
+ test_must_be_empty actual.out &&
+
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-deman
+ git -C submodule rev-parse --short HEAD >subhead &&
+ git -C deepsubmodule rev-parse --short HEAD >deephead &&
+ verify_fetch_result actual.err &&
-+ # Assert that submodule is read from the index, not from a commit
-+ ! grep "Fetching submodule submodule at commit" actual.err &&
++ # grep for the exact line to check that the submodule is read
++ # from the index, not from a commit
++ grep "^Fetching submodule submodule\$" actual.err &&
+
+ # Assert that super-sub2-only and submodule2 were fetched even
+ # though another branch is checked out
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-deman
+ grep -E "\.\.${super_sub2_only_head}\s+super-sub2-only\s+-> origin/super-sub2-only" actual.err &&
+ grep "Fetching submodule submodule2 at commit $super_sub2_only_head" actual.err &&
+ sub2head=$(git -C submodule2 rev-parse --short HEAD) &&
-+ grep -E "\.\.${sub2head}\s+sub2\s+-> origin/sub2" actual.err &&
-+
-+ # Assert that we can checkout the superproject commit with --recurse-submodules
-+ ! grep -E "error: Submodule .+ could not be updated" actual-checkout.err
++ grep -E "\.\.${sub2head}\s+sub2\s+-> origin/sub2" actual.err
+'
+
test_expect_success "'--recurse-submodules=on-demand' stops when no new submodule commits are found in the superproject (and ignores config)" '
-: ---------- > 8: 8aa68111b0 submodule: read shallows when finding changed submodules
8: e44bb1560e ! 9: 05a8b93154 submodule: fix bug and remove add_submodule_odb()
@@ Metadata
Author: Glen Choo <chooglen@google.com>
## Commit message ##
- submodule: fix bug and remove add_submodule_odb()
+ submodule: fix latent check_has_commit() bug
- add_submodule_odb() is a hack - it adds a submodule's odb as an
- alternate, allowing the submodule's objects to be read via
- the_repository. Its last caller is submodule_has_commits(), which calls
- add_submodule_odb() to prepare for check_has_commit(). This used to be
- necessary because check_has_commit() used the_repository's odb, but this
- is longer true as of 13a2f620b2 (submodule: pass repo to
- check_has_commit(), 2021-10-08).
+ When check_has_commit() is called on a missing submodule, initialization
+ of the struct repository fails, but it attempts to clear the struct
+ anyway (which is a fatal error). This bug is masked by its only caller,
+ submodule_has_commits(), first calling add_submodule_odb(). The latter
+ fails if the submodule does not exist, making submodule_has_commits()
+ exit early and not invoke check_has_commit().
- Removing add_submodule_odb() reveals a bug in check_has_commit(), where
- check_has_commit() will segfault if the submodule is missing (e.g. the
- user has not init-ed the submodule). This happens because the submodule
- struct cannot be initialized, but check_has_commit() tries to cleanup
- the uninitialized struct anyway. This was masked by add_submodule_odb(),
- because add_submodule_odb() fails when the submodule is missing, causing
- the caller to return early and avoid calling check_has_commit().
+ Fix this bug, and because calling add_submodule_odb() is no longer
+ necessary as of 13a2f620b2 (submodule: pass repo to
+ check_has_commit(), 2021-10-08), remove that call too.
- Fix the bug and remove the call to add_submodule_odb(). Since
- add_submodule_odb() has no more callers, remove it too.
-
- Note that submodule odbs can still by added as alternates via
- add_submodule_odb_by_path().
+ This is the last caller of add_submodule_odb(), so remove that
+ function. (Submodule ODBs are still added as alternates via
+ add_submodule_odb_by_path().)
Signed-off-by: Glen Choo <chooglen@google.com>
base-commit: 679e3693aba0c17af60c031f7eef68f2296b8dad
-- 2.33.GIT