[3/9] submodule.c: sort changed_submodule_names before searching it
diff mbox series

Message ID 20181016181327.107186-4-sbeller@google.com
State New
Headers show
Series
  • Resending sb/submodule-recursive-fetch-gets-the-tip
Related show

Commit Message

Stefan Beller Oct. 16, 2018, 6:13 p.m. UTC
We can string_list_insert() to maintain sorted-ness of the
list as we find new items, or we can string_list_append() to
build an unsorted list and sort it at the end just once.

As we do not rely on the sortedness while building the
list, we pick the "append and sort at the end" as it
has better worst case execution times.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 submodule.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Jonathan Tan Oct. 17, 2018, 9:21 p.m. UTC | #1
> We can string_list_insert() to maintain sorted-ness of the
> list as we find new items, or we can string_list_append() to
> build an unsorted list and sort it at the end just once.

This confused me at first, because neither function is mentioned in the
patch.

> As we do not rely on the sortedness while building the
> list, we pick the "append and sort at the end" as it
> has better worst case execution times.

It took me some time to see that you were rejecting the two solutions
you listed in the first paragraph, and are instead using a third (that
you describe in this paragraph).

The code itself looks fine.

In the future, I think that it's better if this type of patch went into
its own patch set - this seems independent of the concerns of this patch
set, so splitting up keeps patch sets small.

Patch
diff mbox series

diff --git a/submodule.c b/submodule.c
index e145ebbb16..9fbfcfcfe1 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1270,7 +1270,7 @@  static int get_next_submodule(struct child_process *cp,
 		case RECURSE_SUBMODULES_DEFAULT:
 		case RECURSE_SUBMODULES_ON_DEMAND:
 			if (!submodule ||
-			    !unsorted_string_list_lookup(
+			    !string_list_lookup(
 					&changed_submodule_names,
 					submodule->name))
 				continue;
@@ -1364,6 +1364,7 @@  int fetch_populated_submodules(struct repository *r,
 	/* default value, "--submodule-prefix" and its value are added later */
 
 	calculate_changed_submodule_paths();
+	string_list_sort(&changed_submodule_names);
 	run_processes_parallel(max_parallel_jobs,
 			       get_next_submodule,
 			       fetch_start_failure,