diff mbox series

[2/8] submodule: store new submodule commits oid_array in a struct

Message ID 20220210044152.78352-3-chooglen@google.com (mailing list archive)
State Superseded
Headers show
Series fetch --recurse-submodules: fetch unpopulated submodules | expand

Commit Message

Glen Choo Feb. 10, 2022, 4:41 a.m. UTC 
This commit prepares for a future commit that will teach `git fetch
--recurse-submodules` how to fetch submodules that are present in
<gitdir>/modules, but are not populated. To do this, we need to store
more information about the changed submodule so that we can read the
submodule configuration from the superproject commit instead of the
filesystem.

Refactor the changed submodules string_list.util to hold a struct
instead of an oid_array. This struct only holds the new_commits
oid_array for now; more information will be added later.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 submodule.c | 60 ++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 19 deletions(-)

Comments

Jonathan Tan Feb. 10, 2022, 7 p.m. UTC | #1 
Glen Choo <chooglen@google.com> writes:
> +/*
> + * Holds relevant information for a changed submodule. Used as the .util
> + * member of the changed submodule string_list_item.
> + */
> +struct changed_submodule_data {
> +	/* The submodule commits that have changed in the rev walk. */
> +	struct oid_array *new_commits;
> +};

Overall this change is straightforward and looks good, except that I
think that the struct oid_array can be embedded directly instead of
through a pointer.
Junio C Hamano Feb. 10, 2022, 10:05 p.m. UTC | #2 
Jonathan Tan <jonathantanmy@google.com> writes:

> Glen Choo <chooglen@google.com> writes:
>> +/*
>> + * Holds relevant information for a changed submodule. Used as the .util
>> + * member of the changed submodule string_list_item.
>> + */
>> +struct changed_submodule_data {
>> +	/* The submodule commits that have changed in the rev walk. */
>> +	struct oid_array *new_commits;
>> +};
>
> Overall this change is straightforward and looks good, except that I
> think that the struct oid_array can be embedded directly instead of
> through a pointer.

True.

I am a bit behind and haven't seen the simplicity 1/8 promised to
bring to us, but hopefully we'll see soon enough why 1/8 is a good
idea.
diff mbox series

Patch

diff --git a/submodule.c b/submodule.c
index 49f9dc5d23..e2405c9f15 100644
--- a/submodule.c
+++ b/submodule.c
@@ -806,6 +806,21 @@  static const char *default_name_or_path(const char *path_or_name)
 	return path_or_name;
 }
 
+/*
+ * Holds relevant information for a changed submodule. Used as the .util
+ * member of the changed submodule string_list_item.
+ */
+struct changed_submodule_data {
+	/* The submodule commits that have changed in the rev walk. */
+	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);
+}
+
 static void collect_changed_submodules_cb(struct diff_queue_struct *q,
 					  struct diff_options *options,
 					  void *data)
@@ -820,6 +835,7 @@  static void collect_changed_submodules_cb(struct diff_queue_struct *q,
 		const struct submodule *submodule;
 		const char *name;
 		struct string_list_item *item;
+		struct changed_submodule_data *cs_data;
 
 		if (!S_ISGITLINK(p->two->mode))
 			continue;
@@ -847,10 +863,15 @@  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()? */
-			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);
 	}
 }
 
@@ -897,11 +918,12 @@  static void collect_changed_submodules(struct repository *r,
 	reset_revision_walk();
 }
 
-static void free_submodules_oids(struct string_list *submodules)
+static void free_submodules_data(struct string_list *submodules)
 {
 	struct string_list_item *item;
-	for_each_string_list_item(item, submodules)
-		oid_array_clear((struct oid_array *) item->util);
+	for_each_string_list_item(item, submodules) {
+		changed_submodule_data_clear(item->util);
+	}
 	string_list_clear(submodules, 1);
 }
 
@@ -1067,7 +1089,7 @@  int find_unpushed_submodules(struct repository *r,
 	collect_changed_submodules(r, &submodules, &argv);
 
 	for_each_string_list_item(name, &submodules) {
-		struct oid_array *commits = name->util;
+		struct changed_submodule_data *cs_data = name->util;
 		const struct submodule *submodule;
 		const char *path = NULL;
 
@@ -1080,11 +1102,11 @@  int find_unpushed_submodules(struct repository *r,
 		if (!path)
 			continue;
 
-		if (submodule_needs_pushing(r, path, commits))
+		if (submodule_needs_pushing(r, path, cs_data->new_commits))
 			string_list_insert(needs_pushing, path);
 	}
 
-	free_submodules_oids(&submodules);
+	free_submodules_data(&submodules);
 	strvec_clear(&argv);
 
 	return needs_pushing->nr;
@@ -1254,7 +1276,7 @@  static void calculate_changed_submodule_paths(struct repository *r,
 	collect_changed_submodules(r, changed_submodule_names, &argv);
 
 	for_each_string_list_item(name, changed_submodule_names) {
-		struct oid_array *commits = name->util;
+		struct changed_submodule_data *cs_data = name->util;
 		const struct submodule *submodule;
 		const char *path = NULL;
 
@@ -1267,8 +1289,8 @@  static void calculate_changed_submodule_paths(struct repository *r,
 		if (!path)
 			continue;
 
-		if (submodule_has_commits(r, path, commits)) {
-			oid_array_clear(commits);
+		if (submodule_has_commits(r, path, cs_data->new_commits)) {
+			oid_array_clear(cs_data->new_commits);
 			*name->string = '\0';
 		}
 	}
@@ -1305,7 +1327,7 @@  int submodule_touches_in_range(struct repository *r,
 
 	strvec_clear(&args);
 
-	free_submodules_oids(&subs);
+	free_submodules_data(&subs);
 	return ret;
 }
 
@@ -1587,7 +1609,7 @@  static int fetch_finish(int retvalue, struct strbuf *err,
 	struct fetch_task *task = task_cb;
 
 	struct string_list_item *it;
-	struct oid_array *commits;
+	struct changed_submodule_data *cs_data;
 
 	if (!task || !task->sub)
 		BUG("callback cookie bogus");
@@ -1615,14 +1637,14 @@  static int fetch_finish(int retvalue, struct strbuf *err,
 		/* Could be an unchanged submodule, not contained in the list */
 		goto out;
 
-	commits = it->util;
-	oid_array_filter(commits,
+	cs_data = it->util;
+	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;
 		ALLOC_GROW(spf->oid_fetch_tasks,
 			   spf->oid_fetch_tasks_nr + 1,
 			   spf->oid_fetch_tasks_alloc);
@@ -1680,7 +1702,7 @@  int fetch_populated_submodules(struct repository *r,
 
 	strvec_clear(&spf.args);
 out:
-	free_submodules_oids(&spf.changed_submodule_names);
+	free_submodules_data(&spf.changed_submodule_names);
 	return spf.result;
 }