mbox series

[v2,0/9] fetch --recurse-submodules: fetch unpopulated submodules

Message ID 20220215172318.73533-1-chooglen@google.com (mailing list archive)
Headers show
Series fetch --recurse-submodules: fetch unpopulated submodules | expand

Message

Glen Choo Feb. 15, 2022, 5:23 p.m. UTC 
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