Skip to contentSkip to conversation

changed submodules

PATCH Superseded Brandon Williams 3 participants 33 messages Apr 28, 2017 – May 2, 2017

lore
Share & tools
lore
#0 lore raw

cover changed submodules

A little bit ago I was looking for a function in our code base which would be able to give me a list of all the submodules which changed across a number of commits. I stumbled upon some of that logic in both our recurisve fetch and pull code paths. Problem is both of these code paths were performing almost identical tasks, without sharing any code.

This series is meant as a cleanup to submodule.c in order to unify these two code paths as well as make it easier for future callers to be able to query for submodules which have chagned across a number of commits.

Brandon Williams (6):
submodule: rename add_sha1_to_array
submodule: rename free_submodules_sha1s
submodule: remove add_oid_to_argv
submodule: change string_list changed_submodule_paths
submodule: improve submodule_has_commits
submodule: refactor logic to determine changed submodules

 submodule.c | 295 ++++++++++++++++++++++++++++--------------------------------
 1 file changed, 139 insertions(+), 156 deletions(-)
Signature · 1 line
2.13.0.rc0.306.g87b477812d-goog
#1 lore raw

PATCH 1/6 submodule: rename add_sha1_to_array

Rename 'add_sha1_to_array()' to 'append_oid_to_array()' to more accuratly describe what the function does since it handles 'struct object_id' and not sha1 character arrays.

---
 submodule.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
submodule.c +5 -4
index d3299e29c..be0f5d847 100644
@@ -951,17 +951,18 @@ static void submodule_collect_changed_cb(struct diff_queue_struct *q,
 	}
 }
 
-static int add_sha1_to_array(const char *ref, const struct object_id *oid,
-			     int flags, void *data)
+static int append_oid_to_array(const char *ref, const struct object_id *oid,
+			       int flags, void *data)
 {
-	oid_array_append(data, oid);
+	struct oid_array *array = data;
+	oid_array_append(array, oid);
 	return 0;
 }
 
 void check_for_new_submodule_commits(struct object_id *oid)
 {
 	if (!initialized_fetch_ref_tips) {
-		for_each_ref(add_sha1_to_array, &ref_tips_before_fetch);
+		for_each_ref(append_oid_to_array, &ref_tips_before_fetch);
 		initialized_fetch_ref_tips = 1;
 	}
 
Signature · 1 line
2.13.0.rc0.306.g87b477812d-goog
#2 lore raw

Re: [PATCH 1/6] submodule: rename add_sha1_to_array

Quoted 2 lines · Brandon Williams

Rename 'add_sha1_to_array()' to 'append_oid_to_array()' to more accuratly describe what the function does since it handles 'struct

accurately. Will fix while queuing.

Makes sense. Thanks.

Quoted 33 lines

object_id' and not sha1 character arrays.

---
 submodule.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
submodule.c +5 -4
index d3299e29c..be0f5d847 100644
@@ -951,17 +951,18 @@ static void submodule_collect_changed_cb(struct diff_queue_struct *q,
 	}
 }
 
-static int add_sha1_to_array(const char *ref, const struct object_id *oid,
-			     int flags, void *data)
+static int append_oid_to_array(const char *ref, const struct object_id *oid,
+			       int flags, void *data)
 {
-	oid_array_append(data, oid);
+	struct oid_array *array = data;
+	oid_array_append(array, oid);
 	return 0;
 }
 
 void check_for_new_submodule_commits(struct object_id *oid)
 {
 	if (!initialized_fetch_ref_tips) {
-		for_each_ref(add_sha1_to_array, &ref_tips_before_fetch);
+		for_each_ref(append_oid_to_array, &ref_tips_before_fetch);
 		initialized_fetch_ref_tips = 1;
 	}
#3 lore raw

PATCH 2/6 submodule: rename free_submodules_sha1s

In reply to Brandon Williams

Rename 'free_submodules_sha1s()' to 'free_submodules_oids()' since the function frees a 'struct string_list' which has a 'struct oid_array' stored in the 'util' field.

---
 submodule.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
submodule.c +3 -2
index be0f5d847..46abd52b1 100644
@@ -738,7 +738,7 @@ static void find_unpushed_submodule_commits(struct commit *commit,
 	diff_tree_combined_merge(commit, 1, &rev);
 }
 
-static void free_submodules_sha1s(struct string_list *submodules)
+static void free_submodules_oids(struct string_list *submodules)
 {
 	struct string_list_item *item;
 	for_each_string_list_item(item, submodules)
@@ -779,7 +779,8 @@ int find_unpushed_submodules(struct oid_array *commits,
 		if (submodule_needs_pushing(submodule->string, commits))
 			string_list_insert(needs_pushing, submodule->string);
 	}
-	free_submodules_sha1s(&submodules);
+
+	free_submodules_oids(&submodules);
 
 	return needs_pushing->nr;
 }
Signature · 1 line
2.13.0.rc0.306.g87b477812d-goog
#4 lore raw

PATCH 3/6 submodule: remove add_oid_to_argv

In reply to Brandon Williams

The function 'add_oid_to_argv()' provides the same functionality as 'append_oid_to_argv()'. Remove this duplicate function and instead use 'append_oid_to_argv()' where 'add_oid_to_argv()' was previously used.

---
 submodule.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)
submodule.c +2 -8
index 46abd52b1..7baa28ae0 100644
@@ -970,12 +970,6 @@ void check_for_new_submodule_commits(struct object_id *oid)
 	oid_array_append(&ref_tips_after_fetch, oid);
 }
 
-static int add_oid_to_argv(const struct object_id *oid, void *data)
-{
-	argv_array_push(data, oid_to_hex(oid));
-	return 0;
-}
-
 static void calculate_changed_submodule_paths(void)
 {
 	struct rev_info rev;
@@ -989,10 +983,10 @@ static void calculate_changed_submodule_paths(void)
 	init_revisions(&rev, NULL);
 	argv_array_push(&argv, "--"); /* argv[0] program name */
 	oid_array_for_each_unique(&ref_tips_after_fetch,
-				   add_oid_to_argv, &argv);
+				   append_oid_to_argv, &argv);
 	argv_array_push(&argv, "--not");
 	oid_array_for_each_unique(&ref_tips_before_fetch,
-				   add_oid_to_argv, &argv);
+				   append_oid_to_argv, &argv);
 	setup_revisions(argv.argc, argv.argv, &rev, NULL);
 	if (prepare_revision_walk(&rev))
 		die("revision walk setup failed");
Signature · 1 line
2.13.0.rc0.306.g87b477812d-goog
#5 lore raw

PATCH 4/6 submodule: change string_list changed_submodule_paths

In reply to Brandon Williams

Eliminate a call to 'xstrdup()' by changing the string_list 'changed_submodule_paths' to duplicated strings added to it.

---
 submodule.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
submodule.c +2 -2
index 7baa28ae0..3bcf44521 100644
@@ -20,7 +20,7 @@
 static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
 static int config_update_recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static int parallel_jobs = 1;
-static struct string_list changed_submodule_paths = STRING_LIST_INIT_NODUP;
+static struct string_list changed_submodule_paths = STRING_LIST_INIT_DUP;
 static int initialized_fetch_ref_tips;
 static struct oid_array ref_tips_before_fetch;
 static struct oid_array ref_tips_after_fetch;
@@ -939,7 +939,7 @@ static void submodule_collect_changed_cb(struct diff_queue_struct *q,
 			struct string_list_item *path;
 			path = unsorted_string_list_lookup(&changed_submodule_paths, p->two->path);
 			if (!path && !is_submodule_commit_present(p->two->path, p->two->oid.hash))
-				string_list_append(&changed_submodule_paths, xstrdup(p->two->path));
+				string_list_append(&changed_submodule_paths, p->two->path);
 		} else {
 			/* Submodule is new or was moved here */
 			/* NEEDSWORK: When the .git directories of submodules
Signature · 1 line
2.13.0.rc0.306.g87b477812d-goog
#6 lore raw

Re: [PATCH 4/6] submodule: change string_list changed_submodule_paths

Quoted 22 lines · Brandon Williams

Eliminate a call to 'xstrdup()' by changing the string_list 'changed_submodule_paths' to duplicated strings added to it.

---
 submodule.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
submodule.c +1 -1
index 7baa28ae0..3bcf44521 100644
@@ -20,7 +20,7 @@
 static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
 static int config_update_recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static int parallel_jobs = 1;
-static struct string_list changed_submodule_paths = STRING_LIST_INIT_NODUP;
+static struct string_list changed_submodule_paths = STRING_LIST_INIT_DUP;
 static int initialized_fetch_ref_tips;
 static struct oid_array ref_tips_before_fetch;
 static struct oid_array ref_tips_after_fetch;

submodule.c static void submodule_collect_changed_cb(struct diff_queue_struct *q, · line 942

Quoted 5 lines
submodule.c +1 -1
@@ -939,7 +939,7 @@ static void submodule_collect_changed_cb(struct diff_queue_struct *q,
 			struct string_list_item *path;
 			path = unsorted_string_list_lookup(&changed_submodule_paths, p->two->path);
 			if (!path && !is_submodule_commit_present(p->two->path, p->two->oid.hash))
-				string_list_append(&changed_submodule_paths, xstrdup(p->two->path));
+				string_list_append(&changed_submodule_paths, p->two->path);

I notice that "path" is not used at all, and other users of this string list do not even bother using a variable, i.e.

if (!unsorted_string_list_lookup(&changed_submodule_paths, ...))

In fact, it might be even better to use a hashmap for this instead?

The call to string_list_clear() onthis list tells it to free the util field, and I went to see what we are storing in the util field, but it seems that it is freeing NULLs, which is somewhat misleading and time-wasting on the code readers. Using hashmap may also clear this up.

But all of the above are not within the scope of this topic ;-)

#7 lore raw

Re: [PATCH 4/6] submodule: change string_list changed_submodule_paths

Quoted 37 lines · Junio C Hamano
Quoted 22 lines · Brandon Williams

Eliminate a call to 'xstrdup()' by changing the string_list 'changed_submodule_paths' to duplicated strings added to it.

---
 submodule.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
submodule.c +1 -1
index 7baa28ae0..3bcf44521 100644
@@ -20,7 +20,7 @@
 static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
 static int config_update_recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static int parallel_jobs = 1;
-static struct string_list changed_submodule_paths = STRING_LIST_INIT_NODUP;
+static struct string_list changed_submodule_paths = STRING_LIST_INIT_DUP;
 static int initialized_fetch_ref_tips;
 static struct oid_array ref_tips_before_fetch;
 static struct oid_array ref_tips_after_fetch;

submodule.c static void submodule_collect_changed_cb(struct diff_queue_struct *q, · line 942

Quoted 5 lines
submodule.c +1 -1
@@ -939,7 +939,7 @@ static void submodule_collect_changed_cb(struct diff_queue_struct *q,
 			struct string_list_item *path;
 			path = unsorted_string_list_lookup(&changed_submodule_paths, p->two->path);
 			if (!path && !is_submodule_commit_present(p->two->path, p->two->oid.hash))
-				string_list_append(&changed_submodule_paths, xstrdup(p->two->path));
+				string_list_append(&changed_submodule_paths, p->two->path);

I notice that "path" is not used at all, and other users of this string list do not even bother using a variable, i.e.

if (!unsorted_string_list_lookup(&changed_submodule_paths, ...))

In fact, it might be even better to use a hashmap for this instead?

Quoted 7 lines

The call to string_list_clear() onthis list tells it to free the util field, and I went to see what we are storing in the util field, but it seems that it is freeing NULLs, which is somewhat misleading and time-wasting on the code readers. Using hashmap may also clear this up.

But all of the above are not within the scope of this topic ;-)

All good good points which hopefully are resolved in a later patch. This patch exists mainly to factor out the change to make the string_list duplicate the strings added to it as opposed to have that change seem random and out of place in a later patch.

Most of this logic itself is removed entirely later once it is unified with the other code path which checked for changed submodules. This may be out of context on this particular patch, but eventually an oid_array is stored in the 'util' field of the string list items. This oid_array holds all of the changes to the submodule pointers. This allows us to then batch check for the existence of all the submodule commits instead of checking each change individually (which is what this code path currently does).

Signature · 1 line
Brandon Williams
#8 lore raw

PATCH 5/6 submodule: improve submodule_has_commits

In reply to Brandon Williams

Teach 'submodule_has_commits()' to ensure that if a commit exists in a submodule, that it is also reachable from a ref.

This is a prepritory step prior to merging the logic which checkes for changed submodules when fetching or pushing.

---
 submodule.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
submodule.c +24 -0
index 3bcf44521..100d31d39 100644
@@ -648,6 +648,30 @@ static int submodule_has_commits(const char *path, struct oid_array *commits)
 		return 0;
 
 	oid_array_for_each_unique(commits, check_has_commit, &has_commit);
+
+	if (has_commit) {
+		/*
+		 * Even if the submodule is checked out and the commit is
+		 * present, make sure it is reachable from a ref.
+		 */
+		struct child_process cp = CHILD_PROCESS_INIT;
+		struct strbuf out = STRBUF_INIT;
+
+		argv_array_pushl(&cp.args, "rev-list", "-n", "1", NULL);
+		oid_array_for_each_unique(commits, append_oid_to_argv, &cp.args);
+		argv_array_pushl(&cp.args, "--not", "--all", NULL);
+
+		prepare_submodule_repo_env(&cp.env_array);
+		cp.git_cmd = 1;
+		cp.no_stdin = 1;
+		cp.dir = path;
+
+		if (capture_command(&cp, &out, 1024) || out.len)
+			has_commit = 0;
+
+		strbuf_release(&out);
+	}
+
 	return has_commit;
 }
 
Signature · 1 line
2.13.0.rc0.306.g87b477812d-goog
#9 lore raw

Re: [PATCH 5/6] submodule: improve submodule_has_commits

Quoted 4 lines · Brandon Williams

Teach 'submodule_has_commits()' to ensure that if a commit exists in a submodule, that it is also reachable from a ref.

This is a prepritory step prior to merging the logic which checkes for

s/prepritory/preparatory/
s/checkes/checks/

This is the first commit in the series that changes user observable behavior, I guess there will be tests in a later patch? Can you elaborate in this commit message more about why it is useful (or at least harmless for performing this check in the case of fetch/push)?

Quoted 18 lines
submodule.c +10 -0
index 3bcf44521..100d31d39 100644
@@ -648,6 +648,30 @@ static int submodule_has_commits(const char *path, struct oid_array *commits)
                return 0;
 
        oid_array_for_each_unique(commits, check_has_commit, &has_commit);
+
+       if (has_commit) {
+               /*
+                * Even if the submodule is checked out and the commit is
+                * present, make sure it is reachable from a ref.
+                */
+               struct child_process cp = CHILD_PROCESS_INIT;
+               struct strbuf out = STRBUF_INIT;
+
+               argv_array_pushl(&cp.args, "rev-list", "-n", "1", NULL);

submodule.c static int submodule_has_commits(const char *path, struct oid_array *commits) · line 670

Quoted 10 lines
submodule.c +10 -0
+               oid_array_for_each_unique(commits, append_oid_to_argv, &cp.args);
+               argv_array_pushl(&cp.args, "--not", "--all", NULL);
+
+               prepare_submodule_repo_env(&cp.env_array);
+               cp.git_cmd = 1;
+               cp.no_stdin = 1;
+               cp.dir = path;
+
+               if (capture_command(&cp, &out, 1024) || out.len)
+                       has_commit = 0;

So if we fail to launch capture_command, we assume we do not have the commits?

capture_command can fail for reasons that are hard to track down or even spurious (OOM due to excessive output, disk failure, corrupt repo, error in executing the process, getting a signal and so on), some of them are ok to ignore, others should never be ignored.

So I'd rather die on capture_command, and inspect out.len only in case of successful capturing.

In addition to that we're only interested if there is any output, such that we can optimize further:
c.f. http://public-inbox.org/git/[email protected]/

    if (start_command(&cp))
        die("cannot start git-rev-list in submodule'%s', sub->path);

    /* read only one character, if any */
    if (xread(cp.out, &tmp, 1);
        has_commit = 0;

    /*
     * close cp.out, such that the child may get SIGPIPE, upon which
     * it dies (silently, maybe we need to suppress cp.err ?)
     */
    close(cp.out);

    /*
     * Though we need to be nitpicky about finish_command IIUC by now:
     * TODO(sbeller): if this turns out to be true, fixup is_submodule_modified
    */
    int code = finish_command(&cp);
    if (!code && code != 128 + SIGPIPE)
        die("git rev-list failed in submodule'%s'", sub->path);

Upon rereading the patch, I notice the '-n 1', which would make the optimized code above useless, so just consider it food for thought.

Thanks,
Stefan

#10 lore raw

Re: [PATCH 5/6] submodule: improve submodule_has_commits

Quoted 6 lines · Stefan Beller
Quoted 4 lines · Brandon Williams

Teach 'submodule_has_commits()' to ensure that if a commit exists in a submodule, that it is also reachable from a ref.

This is a prepritory step prior to merging the logic which checkes for

Quoted 7 lines

s/prepritory/preparatory/
s/checkes/checks/

This is the first commit in the series that changes user observable behavior, I guess there will be tests in a later patch? Can you elaborate in this commit message more about why it is useful (or at least harmless for performing this check in the case of fetch/push)?

This hunk of logic is essentially a copy and paste from elsewhere in the file. Essentially both code paths were essentially doing the same thing (checking if a submodule has a commit) but one of the code paths included this logic to ensure that it was reachable from a ref in the submodule. In order to merge the two code paths, I included that logic here.

Maybe it would make sense to squash this into the next patch as the place where this was copied from is removed?

Signature · 1 line
Brandon Williams
#11 lore raw

Re: [PATCH 5/6] submodule: improve submodule_has_commits

Quoted 5 lines · Brandon Williams

This hunk of logic is essentially a copy and paste from elsewhere in the file. Essentially both code paths were essentially doing the same thing (checking if a submodule has a commit) but one of the code paths included this logic to ensure that it was reachable from a ref in the submodule. In order to merge the two code paths, I included that logic here.

yes, I get that. The question is whether the other code path omitted the check intentionally or just missed it (Is it a bug or feature to have this check in that code
path now)?

#12 lore raw

Re: [PATCH 5/6] submodule: improve submodule_has_commits

Quoted 8 lines · Stefan Beller
Quoted 5 lines · Brandon Williams

This hunk of logic is essentially a copy and paste from elsewhere in the file. Essentially both code paths were essentially doing the same thing (checking if a submodule has a commit) but one of the code paths included this logic to ensure that it was reachable from a ref in the submodule. In order to merge the two code paths, I included that logic here.

Quoted 4 lines

yes, I get that. The question is whether the other code path omitted the check intentionally or just missed it (Is it a bug or feature to have this check in that code
path now)?

I'd take a look at Junio's response. This check more than likely needs to be included.

Signature · 1 line
Brandon Williams
#13 lore raw

Re: [PATCH 5/6] submodule: improve submodule_has_commits

In reply to Brandon Williams
Quoted 17 lines · Brandon Williams
submodule.c +16 -0
 	oid_array_for_each_unique(commits, check_has_commit, &has_commit);
+
+	if (has_commit) {
+		/*
+		 * Even if the submodule is checked out and the commit is
+		 * present, make sure it is reachable from a ref.
+		 */
+		struct child_process cp = CHILD_PROCESS_INIT;
+		struct strbuf out = STRBUF_INIT;
+
+		argv_array_pushl(&cp.args, "rev-list", "-n", "1", NULL);
+		oid_array_for_each_unique(commits, append_oid_to_argv, &cp.args);
+		argv_array_pushl(&cp.args, "--not", "--all", NULL);
+
+		prepare_submodule_repo_env(&cp.env_array);
+		cp.git_cmd = 1;
+		cp.no_stdin = 1;

submodule.c static int submodule_has_commits(const char *path, struct oid_array *commits) · line 676

Quoted 10 lines
submodule.c +8 -0
+		cp.dir = path;
+
+		if (capture_command(&cp, &out, 1024) || out.len)
+			has_commit = 0;
+
+		strbuf_release(&out);
+	}
+
 	return has_commit;
 }

The "check-has-commit" we see in the pre-context is "we contaminated our in-core object store by tentatively borrowing from submodule's object store---now do we see these commits in our in-core view?" Which is a wrong thing to do from two separate point of view. Even though the commit in question may be visible in our contaminated view, there is no guarantee that the commit exists in the object store of the correct submodule. And of course the commit may exist but may not be anchored by any ref.

This patch fixes the latter, and if we remove that check-has-commit call before it, we can fix the former at the same time.

There is value in leaving the check-has-commit code if we anticipate that we would very often have to say "no, the submodule does not have these commits"---a cheap but wrong check it does can be used as an optimization. If we do not have the commit object anywhere, there is no chance we have it in the object store of the correct submodule and have it reachable from a ref, so we can fail without spawning rev-list which is expensive.

#14 lore raw

Re: [PATCH 5/6] submodule: improve submodule_has_commits

Quoted 39 lines · Junio C Hamano
Quoted 17 lines · Brandon Williams
submodule.c +16 -0
 	oid_array_for_each_unique(commits, check_has_commit, &has_commit);
+
+	if (has_commit) {
+		/*
+		 * Even if the submodule is checked out and the commit is
+		 * present, make sure it is reachable from a ref.
+		 */
+		struct child_process cp = CHILD_PROCESS_INIT;
+		struct strbuf out = STRBUF_INIT;
+
+		argv_array_pushl(&cp.args, "rev-list", "-n", "1", NULL);
+		oid_array_for_each_unique(commits, append_oid_to_argv, &cp.args);
+		argv_array_pushl(&cp.args, "--not", "--all", NULL);
+
+		prepare_submodule_repo_env(&cp.env_array);
+		cp.git_cmd = 1;
+		cp.no_stdin = 1;

submodule.c static int submodule_has_commits(const char *path, struct oid_array *commits) · line 676

Quoted 10 lines
submodule.c +8 -0
+		cp.dir = path;
+
+		if (capture_command(&cp, &out, 1024) || out.len)
+			has_commit = 0;
+
+		strbuf_release(&out);
+	}
+
 	return has_commit;
 }

The "check-has-commit" we see in the pre-context is "we contaminated our in-core object store by tentatively borrowing from submodule's object store---now do we see these commits in our in-core view?" Which is a wrong thing to do from two separate point of view. Even though the commit in question may be visible in our contaminated view, there is no guarantee that the commit exists in the object store of the correct submodule. And of course the commit may exist but may not be anchored by any ref.

Quoted 2 lines

This patch fixes the latter, and if we remove that check-has-commit call before it, we can fix the former at the same time.

I noticed this when cleaning up this code but was unsure if I should drop the "check-has-commit" bit.

Quoted 8 lines

There is value in leaving the check-has-commit code if we anticipate that we would very often have to say "no, the submodule does not have these commits"---a cheap but wrong check it does can be used as an optimization. If we do not have the commit object anywhere, there is no chance we have it in the object store of the correct submodule and have it reachable from a ref, so we can fail without spawning rev-list which is expensive.

Mostly because it gave the code a way to fail quickly, of course I'm making the assumption that polluting the object store than then checking it is quicker than launching a child process (though I guess most things are cheaper than launching a process ;)

Signature · 1 line
Brandon Williams
#15 lore raw

PATCH 6/6 submodule: refactor logic to determine changed submodules

In reply to Brandon Williams

There are currently two instances (fetch and push) where we want to determine if submodules have changed given some revision specification. These two instances don't use the same logic to generate a list of changed submodules and as a result there is a fair amount of code duplication.

This patch refactors these two code paths such that they both use the same logic to generate a list of changed submodules. This also makes it easier for future callers to be able to reuse this logic as they only need to create an argv_array with the revision specification to be using during the revision walk.

---
 submodule.c | 247 ++++++++++++++++++++++++++----------------------------------
 1 file changed, 105 insertions(+), 142 deletions(-)
submodule.c +105 -142
index 100d31d39..88093779e 100644
@@ -617,6 +617,94 @@ const struct submodule *submodule_from_ce(const struct cache_entry *ce)
 	return submodule_from_path(null_sha1, ce->name);
 }
 
+static struct oid_array *submodule_commits(struct string_list *submodules,
+					   const char *path)
+{
+	struct string_list_item *item;
+
+	item = string_list_insert(submodules, path);
+	if (item->util)
+		return (struct oid_array *) item->util;
+
+	/* NEEDSWORK: should we have oid_array_init()? */
+	item->util = xcalloc(1, sizeof(struct oid_array));
+	return (struct oid_array *) item->util;
+}
+
+static void collect_changed_submodules_cb(struct diff_queue_struct *q,
+					  struct diff_options *options,
+					  void *data)
+{
+	int i;
+	struct string_list *changed = data;
+
+	for (i = 0; i < q->nr; i++) {
+		struct diff_filepair *p = q->queue[i];
+		struct oid_array *commits;
+		if (!S_ISGITLINK(p->two->mode))
+			continue;
+
+		if (S_ISGITLINK(p->one->mode)) {
+			/*
+			 * NEEDSWORK: We should honor the name configured in
+			 * the .gitmodules file of the commit we are examining
+			 * here to be able to correctly follow submodules
+			 * being moved around.
+			 */
+			commits = submodule_commits(changed, p->two->path);
+			oid_array_append(commits, &p->two->oid);
+		} else {
+			/* Submodule is new or was moved here */
+			/*
+			 * NEEDSWORK: When the .git directories of submodules
+			 * live inside the superprojects .git directory some
+			 * day we should fetch new submodules directly into
+			 * that location too when config or options request
+			 * that so they can be checked out from there.
+			 */
+			continue;
+		}
+	}
+}
+
+/*
+ * Collect the paths of submodules in 'changed' which have changed based on
+ * the revisions as specified in 'argv'.  Each entry in 'changed' will also
+ * have a corresponding 'struct oid_array' (in the 'util' field) which lists
+ * what the submodule pointers were updated to during the change.
+ */
+static void collect_changed_submodules(struct string_list *changed,
+				       struct argv_array *argv)
+{
+	struct rev_info rev;
+	const struct commit *commit;
+
+	init_revisions(&rev, NULL);
+	setup_revisions(argv->argc, argv->argv, &rev, NULL);
+	if (prepare_revision_walk(&rev))
+		die("revision walk setup failed");
+
+	while ((commit = get_revision(&rev))) {
+		struct rev_info diff_rev;
+
+		init_revisions(&diff_rev, NULL);
+		diff_rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
+		diff_rev.diffopt.format_callback = collect_changed_submodules_cb;
+		diff_rev.diffopt.format_callback_data = changed;
+		diff_tree_combined_merge(commit, 1, &diff_rev);
+	}
+
+	reset_revision_walk();
+}
+
+static void free_submodules_oids(struct string_list *submodules)
+{
+	struct string_list_item *item;
+	for_each_string_list_item(item, submodules)
+		oid_array_clear((struct oid_array *) item->util);
+	string_list_clear(submodules, 1);
+}
+
 static int has_remote(const char *refname, const struct object_id *oid,
 		      int flags, void *cb_data)
 {
@@ -719,92 +807,31 @@ static int submodule_needs_pushing(const char *path, struct oid_array *commits)
 	return 0;
 }
 
-static struct oid_array *submodule_commits(struct string_list *submodules,
-					    const char *path)
-{
-	struct string_list_item *item;
-
-	item = string_list_insert(submodules, path);
-	if (item->util)
-		return (struct oid_array *) item->util;
-
-	/* NEEDSWORK: should we have oid_array_init()? */
-	item->util = xcalloc(1, sizeof(struct oid_array));
-	return (struct oid_array *) item->util;
-}
-
-static void collect_submodules_from_diff(struct diff_queue_struct *q,
-					 struct diff_options *options,
-					 void *data)
-{
-	int i;
-	struct string_list *submodules = data;
-
-	for (i = 0; i < q->nr; i++) {
-		struct diff_filepair *p = q->queue[i];
-		struct oid_array *commits;
-		if (!S_ISGITLINK(p->two->mode))
-			continue;
-		commits = submodule_commits(submodules, p->two->path);
-		oid_array_append(commits, &p->two->oid);
-	}
-}
-
-static void find_unpushed_submodule_commits(struct commit *commit,
-		struct string_list *needs_pushing)
-{
-	struct rev_info rev;
-
-	init_revisions(&rev, NULL);
-	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
-	rev.diffopt.format_callback = collect_submodules_from_diff;
-	rev.diffopt.format_callback_data = needs_pushing;
-	diff_tree_combined_merge(commit, 1, &rev);
-}
-
-static void free_submodules_oids(struct string_list *submodules)
-{
-	struct string_list_item *item;
-	for_each_string_list_item(item, submodules)
-		oid_array_clear((struct oid_array *) item->util);
-	string_list_clear(submodules, 1);
-}
-
 int find_unpushed_submodules(struct oid_array *commits,
 		const char *remotes_name, struct string_list *needs_pushing)
 {
-	struct rev_info rev;
-	struct commit *commit;
 	struct string_list submodules = STRING_LIST_INIT_DUP;
 	struct string_list_item *submodule;
 	struct argv_array argv = ARGV_ARRAY_INIT;
 
-	init_revisions(&rev, NULL);
-
 	/* argv.argv[0] will be ignored by setup_revisions */
 	argv_array_push(&argv, "find_unpushed_submodules");
 	oid_array_for_each_unique(commits, append_oid_to_argv, &argv);
 	argv_array_push(&argv, "--not");
 	argv_array_pushf(&argv, "--remotes=%s", remotes_name);
 
-	setup_revisions(argv.argc, argv.argv, &rev, NULL);
-	if (prepare_revision_walk(&rev))
-		die("revision walk setup failed");
-
-	while ((commit = get_revision(&rev)) != NULL)
-		find_unpushed_submodule_commits(commit, &submodules);
-
-	reset_revision_walk();
-	argv_array_clear(&argv);
+	collect_changed_submodules(&submodules, &argv);
 
 	for_each_string_list_item(submodule, &submodules) {
-		struct oid_array *commits = (struct oid_array *) submodule->util;
+		struct oid_array *commits = submodule->util;
+		const char *path = submodule->string;
 
-		if (submodule_needs_pushing(submodule->string, commits))
-			string_list_insert(needs_pushing, submodule->string);
+		if (submodule_needs_pushing(path, commits))
+			string_list_insert(needs_pushing, path);
 	}
 
 	free_submodules_oids(&submodules);
+	argv_array_clear(&argv);
 
 	return needs_pushing->nr;
 }
@@ -921,61 +948,6 @@ int push_unpushed_submodules(struct oid_array *commits,
 	return ret;
 }
 
-static int is_submodule_commit_present(const char *path, unsigned char sha1[20])
-{
-	int is_present = 0;
-	if (!add_submodule_odb(path) && lookup_commit_reference(sha1)) {
-		/* Even if the submodule is checked out and the commit is
-		 * present, make sure it is reachable from a ref. */
-		struct child_process cp = CHILD_PROCESS_INIT;
-		const char *argv[] = {"rev-list", "-n", "1", NULL, "--not", "--all", NULL};
-		struct strbuf buf = STRBUF_INIT;
-
-		argv[3] = sha1_to_hex(sha1);
-		cp.argv = argv;
-		prepare_submodule_repo_env(&cp.env_array);
-		cp.git_cmd = 1;
-		cp.no_stdin = 1;
-		cp.dir = path;
-		if (!capture_command(&cp, &buf, 1024) && !buf.len)
-			is_present = 1;
-
-		strbuf_release(&buf);
-	}
-	return is_present;
-}
-
-static void submodule_collect_changed_cb(struct diff_queue_struct *q,
-					 struct diff_options *options,
-					 void *data)
-{
-	int i;
-	for (i = 0; i < q->nr; i++) {
-		struct diff_filepair *p = q->queue[i];
-		if (!S_ISGITLINK(p->two->mode))
-			continue;
-
-		if (S_ISGITLINK(p->one->mode)) {
-			/* NEEDSWORK: We should honor the name configured in
-			 * the .gitmodules file of the commit we are examining
-			 * here to be able to correctly follow submodules
-			 * being moved around. */
-			struct string_list_item *path;
-			path = unsorted_string_list_lookup(&changed_submodule_paths, p->two->path);
-			if (!path && !is_submodule_commit_present(p->two->path, p->two->oid.hash))
-				string_list_append(&changed_submodule_paths, p->two->path);
-		} else {
-			/* Submodule is new or was moved here */
-			/* NEEDSWORK: When the .git directories of submodules
-			 * live inside the superprojects .git directory some
-			 * day we should fetch new submodules directly into
-			 * that location too when config or options request
-			 * that so they can be checked out from there. */
-			continue;
-		}
-	}
-}
-
 static int append_oid_to_array(const char *ref, const struct object_id *oid,
 			       int flags, void *data)
 {
@@ -996,45 +968,36 @@ void check_for_new_submodule_commits(struct object_id *oid)
 
 static void calculate_changed_submodule_paths(void)
 {
-	struct rev_info rev;
-	struct commit *commit;
 	struct argv_array argv = ARGV_ARRAY_INIT;
+	struct string_list changed_submodules = STRING_LIST_INIT_DUP;
+	const struct string_list_item *item;
 
 	/* No need to check if there are no submodules configured */
 	if (!submodule_from_path(NULL, NULL))
 		return;
 
-	init_revisions(&rev, NULL);
 	argv_array_push(&argv, "--"); /* argv[0] program name */
 	oid_array_for_each_unique(&ref_tips_after_fetch,
 				   append_oid_to_argv, &argv);
 	argv_array_push(&argv, "--not");
 	oid_array_for_each_unique(&ref_tips_before_fetch,
 				   append_oid_to_argv, &argv);
-	setup_revisions(argv.argc, argv.argv, &rev, NULL);
-	if (prepare_revision_walk(&rev))
-		die("revision walk setup failed");
 
 	/*
 	 * Collect all submodules (whether checked out or not) for which new
 	 * commits have been recorded upstream in "changed_submodule_paths".
 	 */
-	while ((commit = get_revision(&rev))) {
-		struct commit_list *parent = commit->parents;
-		while (parent) {
-			struct diff_options diff_opts;
-			diff_setup(&diff_opts);
-			DIFF_OPT_SET(&diff_opts, RECURSIVE);
-			diff_opts.output_format |= DIFF_FORMAT_CALLBACK;
-			diff_opts.format_callback = submodule_collect_changed_cb;
-			diff_setup_done(&diff_opts);
-			diff_tree_sha1(parent->item->object.oid.hash, commit->object.oid.hash, "", &diff_opts);
-			diffcore_std(&diff_opts);
-			diff_flush(&diff_opts);
-			parent = parent->next;
-		}
+	collect_changed_submodules(&changed_submodules, &argv);
+
+	for_each_string_list_item(item, &changed_submodules) {
+		struct oid_array *commits = item->util;
+		const char *path = item->string;
+
+		if (!submodule_has_commits(path, commits))
+			string_list_append(&changed_submodule_paths, path);
 	}
 
+	free_submodules_oids(&changed_submodules);
 	argv_array_clear(&argv);
 	oid_array_clear(&ref_tips_before_fetch);
 	oid_array_clear(&ref_tips_after_fetch);
Signature · 1 line
2.13.0.rc0.306.g87b477812d-goog
#16 lore raw

Re: [PATCH 6/6] submodule: refactor logic to determine changed submodules

+ Heiko, who touched the pushing code end of last year.

Quoted 11 lines · Brandon Williams

There are currently two instances (fetch and push) where we want to determine if submodules have changed given some revision specification. These two instances don't use the same logic to generate a list of changed submodules and as a result there is a fair amount of code duplication.

This patch refactors these two code paths such that they both use the same logic to generate a list of changed submodules. This also makes it easier for future callers to be able to reuse this logic as they only need to create an argv_array with the revision specification to be using during the revision walk.

Thanks for doing some refactoring. :)

submodule.c static int submodule_needs_pushing(const char *path, struct oid_array *commits)

Quoted 7 lines
submodule.c +0 -4
-static struct oid_array *submodule_commits(struct string_list *submodules,
-                                           const char *path)
...
 
-static void free_submodules_oids(struct string_list *submodules)
-{
...

These are just moved north, no change in code.
If you want to be extra nice to reviewers this could have been an extra patch, as it makes reviewing easier if you just have to look at the lines of code with one goal ("shuffling code around, no change intended" vs "make a change to improve things with no bad side effects")

submodule.c const struct submodule *submodule_from_ce(const struct cache_entry *ce) · line 637

Quoted 5 lines
submodule.c +5 -0
+
+static void collect_changed_submodules_cb(struct diff_queue_struct *q,
+                                         struct diff_options *options,
+                                         void *data)
+{

This one combines both collect_submodules_from_diff and submodule_collect_changed_cb ?

Quoted 12 lines
submodule.c +0 -8
@@ -921,61 +948,6 @@ int push_unpushed_submodules(struct oid_array *commits,
        return ret;
 }
 
-static int is_submodule_commit_present(const char *path, unsigned char sha1[20])
-{
-       int is_present = 0;
-       if (!add_submodule_odb(path) && lookup_commit_reference(sha1)) {
-               /* Even if the submodule is checked out and the commit is
-                * present, make sure it is reachable from a ref. */
-               struct child_process cp = CHILD_PROCESS_INIT;
-               const char *argv[] = {"rev-list", "-n", "1", NULL, "--not", "--all", NULL};

submodule.c int push_unpushed_submodules(struct oid_array *commits, · line 941

Quoted 10 lines
submodule.c +0 -10
-               struct strbuf buf = STRBUF_INIT;
-
-               argv[3] = sha1_to_hex(sha1);
-               cp.argv = argv;
-               prepare_submodule_repo_env(&cp.env_array);
-               cp.git_cmd = 1;
-               cp.no_stdin = 1;
-               cp.dir = path;
-               if (!capture_command(&cp, &buf, 1024) && !buf.len)
-                       is_present = 1;

Oh, I see where the nit in patch 5/6 is coming from. Another note on that: The hint is way off. The hint should be on the order of GIT_SHA1_HEXSZ

submodule.c static int submodule_needs_pushing(const char *path, struct oid_array *commits)

Quoted 6 lines
submodule.c +0 -0
 int find_unpushed_submodules(struct oid_array *commits,
                const char *remotes_name, struct string_list *needs_pushing)
...
 
 static void calculate_changed_submodule_paths(void)
...

These are both nicely clean now.

Thanks,
Stefan

#17 lore raw

Re: [PATCH 6/6] submodule: refactor logic to determine changed submodules

Quoted 21 lines · Stefan Beller

+ Heiko, who touched the pushing code end of last year.

Quoted 11 lines · Brandon Williams

There are currently two instances (fetch and push) where we want to determine if submodules have changed given some revision specification. These two instances don't use the same logic to generate a list of changed submodules and as a result there is a fair amount of code duplication.

This patch refactors these two code paths such that they both use the same logic to generate a list of changed submodules. This also makes it easier for future callers to be able to reuse this logic as they only need to create an argv_array with the revision specification to be using during the revision walk.

Thanks for doing some refactoring. :)

Quoted 3 lines
submodule.c +0 -2
-static struct oid_array *submodule_commits(struct string_list *submodules,
-                                           const char *path)
...
Quoted 9 lines

submodule.c static int submodule_needs_pushing(const char *path, struct oid_array *commits) · line 766

Quoted 3 lines
submodule.c +0 -2
-static void free_submodules_oids(struct string_list *submodules)
-{
...

These are just moved north, no change in code.
If you want to be extra nice to reviewers this could have been an extra patch, as it makes reviewing easier if you just have to look at the lines of code with one goal ("shuffling code around, no change intended" vs "make a change to improve things with no bad side effects")

Yeah I suppose, I was having a difficult time breaking this largest patch into multiple.

Quoted 9 lines

submodule.c const struct submodule *submodule_from_ce(const struct cache_entry *ce) · line 637

Quoted 6 lines
submodule.c +5 -0
+
+static void collect_changed_submodules_cb(struct diff_queue_struct *q,
+                                         struct diff_options *options,
+                                         void *data)
+{

This one combines both collect_submodules_from_diff and submodule_collect_changed_cb ?

Yep.

Quoted 19 lines
Quoted 19 lines
submodule.c +0 -14
@@ -921,61 +948,6 @@ int push_unpushed_submodules(struct oid_array *commits,
        return ret;
 }
 
-static int is_submodule_commit_present(const char *path, unsigned char sha1[20])
-{
-       int is_present = 0;
-       if (!add_submodule_odb(path) && lookup_commit_reference(sha1)) {
-               /* Even if the submodule is checked out and the commit is
-                * present, make sure it is reachable from a ref. */
-               struct child_process cp = CHILD_PROCESS_INIT;
-               const char *argv[] = {"rev-list", "-n", "1", NULL, "--not", "--all", NULL};
-               struct strbuf buf = STRBUF_INIT;
-
-               argv[3] = sha1_to_hex(sha1);
-               cp.argv = argv;
-               prepare_submodule_repo_env(&cp.env_array);
-               cp.git_cmd = 1;
Quoted 8 lines

submodule.c int push_unpushed_submodules(struct oid_array *commits, · line 941

Quoted 4 lines
submodule.c +0 -4
-               cp.no_stdin = 1;
-               cp.dir = path;
-               if (!capture_command(&cp, &buf, 1024) && !buf.len)
-                       is_present = 1;

Oh, I see where the nit in patch 5/6 is coming from. Another note on that: The hint is way off. The hint should be on the order of GIT_SHA1_HEXSZ

Yeah agreed. I make this change in the earlier patch.

Quoted 12 lines

submodule.c static int submodule_needs_pushing(const char *path, struct oid_array *commits)

Quoted 7 lines
submodule.c +0 -0
 int find_unpushed_submodules(struct oid_array *commits,
                const char *remotes_name, struct string_list *needs_pushing)
...
 
 static void calculate_changed_submodule_paths(void)
...

These are both nicely clean now.

Thanks,
Stefan

Signature · 1 line
Brandon Williams
#18 lore raw
In reply to Brandon Williams
Quoted 9 lines · Brandon Williams

A little bit ago I was looking for a function in our code base which would be able to give me a list of all the submodules which changed across a number of commits. I stumbled upon some of that logic in both our recurisve fetch and pull code paths. Problem is both of these code paths were performing almost identical tasks, without sharing any code.

This series is meant as a cleanup to submodule.c in order to unify these two code paths as well as make it easier for future callers to be able to query for submodules which have chagned across a number of commits.

Yay. Nice to see code reduction.

Quoted 11 lines

Brandon Williams (6):
submodule: rename add_sha1_to_array
submodule: rename free_submodules_sha1s
submodule: remove add_oid_to_argv
submodule: change string_list changed_submodule_paths
submodule: improve submodule_has_commits
submodule: refactor logic to determine changed submodules

 submodule.c | 295 ++++++++++++++++++++++++++++--------------------------------
 1 file changed, 139 insertions(+), 156 deletions(-)
#19 lore raw

v2 cover changed submodules

In reply to Brandon Williams

Changes in v2:
- few tweaks in the commit messages of a couple of the commits
- use 'GIT_MAX_HEXSZ + 1' as the hint size in [5/6]
- added a comment in [5/6] better explaining the rational for having a quick, incorrect check for the existence of a commit in a submodule.

Brandon Williams (6):
submodule: rename add_sha1_to_array
submodule: rename free_submodules_sha1s
submodule: remove add_oid_to_argv
submodule: change string_list changed_submodule_paths
submodule: improve submodule_has_commits
submodule: refactor logic to determine changed submodules

 submodule.c | 305 +++++++++++++++++++++++++++++-------------------------------
 1 file changed, 149 insertions(+), 156 deletions(-)
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog
#20 lore raw

PATCH v2 1/6 submodule: rename add_sha1_to_array

Rename 'add_sha1_to_array()' to 'append_oid_to_array()' to more accurately describe what the function does since it handles 'struct object_id' and not sha1 character arrays.

Change-Id Ia6d15f34cee4d0dc32f7a475c69f4cb3aa8ce5bf
---
 submodule.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
submodule.c +5 -4
index d3299e29c..be0f5d847 100644
@@ -951,17 +951,18 @@ static void submodule_collect_changed_cb(struct diff_queue_struct *q,
 	}
 }
 
-static int add_sha1_to_array(const char *ref, const struct object_id *oid,
-			     int flags, void *data)
+static int append_oid_to_array(const char *ref, const struct object_id *oid,
+			       int flags, void *data)
 {
-	oid_array_append(data, oid);
+	struct oid_array *array = data;
+	oid_array_append(array, oid);
 	return 0;
 }
 
 void check_for_new_submodule_commits(struct object_id *oid)
 {
 	if (!initialized_fetch_ref_tips) {
-		for_each_ref(add_sha1_to_array, &ref_tips_before_fetch);
+		for_each_ref(append_oid_to_array, &ref_tips_before_fetch);
 		initialized_fetch_ref_tips = 1;
 	}
 
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog
#21 lore raw

Re: [PATCH v2 1/6] submodule: rename add_sha1_to_array

Quoted 1 line · Brandon Williams

Change-Id: Ia6d15f34cee4d0dc32f7a475c69f4cb3aa8ce5bf

Uh?

Maybe another side project for the long todo list: get git-trailers into shape, such that it can be configured to yell at you upon formatting the patch or sending email when a given trailer occurs.

Thanks,
Stefan

#22 lore raw

Re: [PATCH v2 1/6] submodule: rename add_sha1_to_array

Quoted 5 lines · Stefan Beller
Quoted 1 line · Brandon Williams

Change-Id: Ia6d15f34cee4d0dc32f7a475c69f4cb3aa8ce5bf

Uh?

Oops! I forgot to run it through my scrubbing script...

Quoted 7 lines

Maybe another side project for the long todo list: get git-trailers into shape, such that it can be configured to yell at you upon formatting the patch or sending email when a given trailer occurs.

Thanks,
Stefan

Signature · 1 line
Brandon Williams
#23 lore raw

PATCH v2 2/6 submodule: rename free_submodules_sha1s

In reply to Brandon Williams

Rename 'free_submodules_sha1s()' to 'free_submodules_oids()' since the function frees a 'struct string_list' which has a 'struct oid_array' stored in the 'util' field.

Change-Id I0c52fa3af1b1492b196bcf52f8b8cc8f5daf085d
---
 submodule.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
submodule.c +3 -2
index be0f5d847..46abd52b1 100644
@@ -738,7 +738,7 @@ static void find_unpushed_submodule_commits(struct commit *commit,
 	diff_tree_combined_merge(commit, 1, &rev);
 }
 
-static void free_submodules_sha1s(struct string_list *submodules)
+static void free_submodules_oids(struct string_list *submodules)
 {
 	struct string_list_item *item;
 	for_each_string_list_item(item, submodules)
@@ -779,7 +779,8 @@ int find_unpushed_submodules(struct oid_array *commits,
 		if (submodule_needs_pushing(submodule->string, commits))
 			string_list_insert(needs_pushing, submodule->string);
 	}
-	free_submodules_sha1s(&submodules);
+
+	free_submodules_oids(&submodules);
 
 	return needs_pushing->nr;
 }
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog
#24 lore raw

PATCH v2 3/6 submodule: remove add_oid_to_argv

In reply to Brandon Williams

The function 'add_oid_to_argv()' provides the same functionality as 'append_oid_to_argv()'. Remove this duplicate function and instead use 'append_oid_to_argv()' where 'add_oid_to_argv()' was previously used.

Change-Id Id0abea012707460cb7000df761e6557ba5cd88d9
---
 submodule.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)
submodule.c +2 -8
index 46abd52b1..7baa28ae0 100644
@@ -970,12 +970,6 @@ void check_for_new_submodule_commits(struct object_id *oid)
 	oid_array_append(&ref_tips_after_fetch, oid);
 }
 
-static int add_oid_to_argv(const struct object_id *oid, void *data)
-{
-	argv_array_push(data, oid_to_hex(oid));
-	return 0;
-}
-
 static void calculate_changed_submodule_paths(void)
 {
 	struct rev_info rev;
@@ -989,10 +983,10 @@ static void calculate_changed_submodule_paths(void)
 	init_revisions(&rev, NULL);
 	argv_array_push(&argv, "--"); /* argv[0] program name */
 	oid_array_for_each_unique(&ref_tips_after_fetch,
-				   add_oid_to_argv, &argv);
+				   append_oid_to_argv, &argv);
 	argv_array_push(&argv, "--not");
 	oid_array_for_each_unique(&ref_tips_before_fetch,
-				   add_oid_to_argv, &argv);
+				   append_oid_to_argv, &argv);
 	setup_revisions(argv.argc, argv.argv, &rev, NULL);
 	if (prepare_revision_walk(&rev))
 		die("revision walk setup failed");
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog
#25 lore raw

PATCH v2 4/6 submodule: change string_list changed_submodule_paths

In reply to Brandon Williams

Eliminate a call to 'xstrdup()' by changing the string_list 'changed_submodule_paths' to duplicated strings added to it.

Change-Id Id4b53837a6e209c0c0837c9f5ba06c70df2ffe06
---
 submodule.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
submodule.c +2 -2
index 7baa28ae0..3bcf44521 100644
@@ -20,7 +20,7 @@
 static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
 static int config_update_recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static int parallel_jobs = 1;
-static struct string_list changed_submodule_paths = STRING_LIST_INIT_NODUP;
+static struct string_list changed_submodule_paths = STRING_LIST_INIT_DUP;
 static int initialized_fetch_ref_tips;
 static struct oid_array ref_tips_before_fetch;
 static struct oid_array ref_tips_after_fetch;
@@ -939,7 +939,7 @@ static void submodule_collect_changed_cb(struct diff_queue_struct *q,
 			struct string_list_item *path;
 			path = unsorted_string_list_lookup(&changed_submodule_paths, p->two->path);
 			if (!path && !is_submodule_commit_present(p->two->path, p->two->oid.hash))
-				string_list_append(&changed_submodule_paths, xstrdup(p->two->path));
+				string_list_append(&changed_submodule_paths, p->two->path);
 		} else {
 			/* Submodule is new or was moved here */
 			/* NEEDSWORK: When the .git directories of submodules
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog
#26 lore raw

PATCH v2 5/6 submodule: improve submodule_has_commits

In reply to Brandon Williams

Teach 'submodule_has_commits()' to ensure that if a commit exists in a submodule, that it is also reachable from a ref.

This is a preparatory step prior to merging the logic which checks for changed submodules when fetching or pushing.

Change-Id I4fed2acfa7e69a5fbbca534df165671e77a90f22
---
 submodule.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)
submodule.c +34 -0
index 3bcf44521..057695e64 100644
@@ -644,10 +644,44 @@ static int submodule_has_commits(const char *path, struct oid_array *commits)
 {
 	int has_commit = 1;
 
+	/*
+	 * Perform a cheap, but incorrect check for the existance of 'commits'.
+	 * This is done by adding the submodule's object store to the in-core
+	 * object store, and then querying for each commit's existance.  If we
+	 * do not have the commit object anywhere, there is no chance we have
+	 * it in the object store of the correct submodule and have it
+	 * reachable from a ref, so we can fail early without spawning rev-list
+	 * which is expensive.
+	 */
 	if (add_submodule_odb(path))
 		return 0;
 
 	oid_array_for_each_unique(commits, check_has_commit, &has_commit);
+
+	if (has_commit) {
+		/*
+		 * Even if the submodule is checked out and the commit is
+		 * present, make sure it exists in the submodule's object store
+		 * and that it is reachable from a ref.
+		 */
+		struct child_process cp = CHILD_PROCESS_INIT;
+		struct strbuf out = STRBUF_INIT;
+
+		argv_array_pushl(&cp.args, "rev-list", "-n", "1", NULL);
+		oid_array_for_each_unique(commits, append_oid_to_argv, &cp.args);
+		argv_array_pushl(&cp.args, "--not", "--all", NULL);
+
+		prepare_submodule_repo_env(&cp.env_array);
+		cp.git_cmd = 1;
+		cp.no_stdin = 1;
+		cp.dir = path;
+
+		if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len)
+			has_commit = 0;
+
+		strbuf_release(&out);
+	}
+
 	return has_commit;
 }
 
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog
#27 lore raw

Re: [PATCH v2 5/6] submodule: improve submodule_has_commits

Quoted 21 lines · Brandon Williams

Teach 'submodule_has_commits()' to ensure that if a commit exists in a submodule, that it is also reachable from a ref.

This is a preparatory step prior to merging the logic which checks for changed submodules when fetching or pushing.

Change-Id I4fed2acfa7e69a5fbbca534df165671e77a90f22
---
 submodule.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)
submodule.c +1 -0
index 3bcf44521..057695e64 100644
@@ -644,10 +644,44 @@ static int submodule_has_commits(const char *path, struct oid_array *commits)
 {
        int has_commit = 1;
 
+       /*

submodule.c static int submodule_has_commits(const char *path, struct oid_array *commits) · line 657

Quoted 10 lines
submodule.c +8 -0
+        * Perform a cheap, but incorrect check for the existance of 'commits'.
+        * This is done by adding the submodule's object store to the in-core
+        * object store, and then querying for each commit's existance.  If we
+        * do not have the commit object anywhere, there is no chance we have
+        * it in the object store of the correct submodule and have it
+        * reachable from a ref, so we can fail early without spawning rev-list
+        * which is expensive.
+        */
        if (add_submodule_odb(path))
                return 0;

Thanks for the comment!

Quoted 12 lines
submodule.c +10 -0
        oid_array_for_each_unique(commits, check_has_commit, &has_commit);
+
+       if (has_commit) {
+               /*
+                * Even if the submodule is checked out and the commit is
+                * present, make sure it exists in the submodule's object store
+                * and that it is reachable from a ref.
+                */
+               struct child_process cp = CHILD_PROCESS_INIT;
+               struct strbuf out = STRBUF_INIT;
+

submodule.c static int submodule_has_commits(const char *path, struct oid_array *commits) · line 679

Quoted 10 lines
submodule.c +10 -0
+               argv_array_pushl(&cp.args, "rev-list", "-n", "1", NULL);
+               oid_array_for_each_unique(commits, append_oid_to_argv, &cp.args);
+               argv_array_pushl(&cp.args, "--not", "--all", NULL);
+
+               prepare_submodule_repo_env(&cp.env_array);
+               cp.git_cmd = 1;
+               cp.no_stdin = 1;
+               cp.dir = path;
+
+               if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len)

eh, I gave too much and self-contradicting feedback here earlier, ideally I'd like to review this to be similar as:

    if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1)
        die("cannot capture git-rev-list in submodule '%s', sub->path);

    if (out.len)
        has_commit = 0;

instead as that does not have a silent error. (though it errs on the safe side, so maybe it is not to bad.)

I could understand if the callers do not want to have submodule_has_commits die()-ing on them, so maybe

    if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) {
        warning("cannot capture git-rev-list in submodule '%s', sub->path);
        has_commit = -1;
        /* this would require auditing all callers and handling -1 though */
    }

    if (out.len)
        has_commit = 0;

As the comment eludes, we'd then have
0 -> has no commits
1 -> has commits
-1 -> error

So to group (error || has_no_commits), we could write

    if (submodule_has_commits(..) <= 0)

which is awkward. So maybe we can rename the function to misses_submodule_commits instead, as then we could flip the return value as well and have

0 -> has commits
1 -> has no commits
-1 -> error

and the lazy invoker could just go with

    if (!misses_submodule_commits(..))
        proceed();
    else
        die("missing submodule commits or errors; I don't care");

whereas the careful invoker could go with

    switch (misses_submodule_commits(..)) {
    case 0:
        proceed(); break;
    case 1:
        pull_magic_trick(); break;
    case -1:
        make_errors_go_away_and_retry(); break;
    }

---
On the longer term plan:
As you wrote about costs. Maybe instead of invoking rev-list, we could try to have this in-core as a first try-out for "classified-repos", looking at refs.h there is e.g.

int for_each_ref_submodule(const char *submodule_path,
each_ref_fn fn, void *cb_data);

which we could use to obtain all submodule refs and then use the revision walking machinery to find out ourselves if we have or do not have the commits. (As we loaded the odb of the submodule, this would *just work*, building one kludgy hack upon the next.)

Thanks,
Stefan

#28 lore raw

Re: [PATCH v2 5/6] submodule: improve submodule_has_commits

Quoted 9 lines · Stefan Beller

Review

Quoted 2 lines · Brandon Williams
(quoted diff) +2 -0
+
+               if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len)

eh, I gave too much and self-contradicting feedback here earlier, ideally I'd like to review this to be similar as:

if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1)
die("cannot capture git-rev-list in submodule '%s', sub->path);

This wouldn't really work because if you provide a SHA1 to rev-list which it isn't able to find then it returns a non-zero exit code which would cause this to die, which isn't the desired behavior.

Quoted 45 lines
    if (out.len)
        has_commit = 0;

instead as that does not have a silent error. (though it errs on the safe side, so maybe it is not to bad.)

I could understand if the callers do not want to have submodule_has_commits die()-ing on them, so maybe

    if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) {
        warning("cannot capture git-rev-list in submodule '%s', sub->path);
        has_commit = -1;
        /* this would require auditing all callers and handling -1 though */
    }

    if (out.len)
        has_commit = 0;

As the comment eludes, we'd then have
0 -> has no commits
1 -> has commits
-1 -> error

So to group (error || has_no_commits), we could write

    if (submodule_has_commits(..) <= 0)

which is awkward. So maybe we can rename the function to misses_submodule_commits instead, as then we could flip the return value as well and have

0 -> has commits
1 -> has no commits
-1 -> error

and the lazy invoker could just go with

    if (!misses_submodule_commits(..))
        proceed();
    else
        die("missing submodule commits or errors; I don't care");

whereas the careful invoker could go with

Quoted 8 lines
    switch (misses_submodule_commits(..)) {
    case 0:
        proceed(); break;
    case 1:
        pull_magic_trick(); break;
    case -1:
        make_errors_go_away_and_retry(); break;
    }

I feel like you're making this a little too complicated, as all I'm doing is shuffling around already existing logic. I understand the want to make things more robust but this seems unnecessarily complex.

Quoted 17 lines

---
On the longer term plan:
As you wrote about costs. Maybe instead of invoking rev-list, we could try to have this in-core as a first try-out for "classified-repos", looking at refs.h there is e.g.

int for_each_ref_submodule(const char *submodule_path,
each_ref_fn fn, void *cb_data);

which we could use to obtain all submodule refs and then use the revision walking machinery to find out ourselves if we have or do not have the commits. (As we loaded the odb of the submodule, this would *just work*, building one kludgy hack upon the next.)

Thanks,
Stefan

Signature · 1 line
Brandon Williams
#29 lore raw

Re: [PATCH v2 5/6] submodule: improve submodule_has_commits

Quoted 8 lines · Brandon Williams
Quoted 7 lines · Stefan Beller

Review

Quoted 2 lines · Brandon Williams
(quoted diff) +2 -0
+
+               if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len)

eh, I gave too much and self-contradicting feedback here earlier, ideally I'd like to review this to be similar as:

Quoted 6 lines
Quoted 2 lines

if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1)
die("cannot capture git-rev-list in submodule '%s', sub->path);

This wouldn't really work because if you provide a SHA1 to rev-list which it isn't able to find then it returns a non-zero exit code which would cause this to die, which isn't the desired behavior.

Oh. In that case, why do we even check for its stdout?
(from rev-lists man page)
--quiet
Don’t print anything to standard output. This form is primarily meant to allow the caller to test the exit status to see if a range of objects is fully connected (or not). It is faster than redirecting stdout to /dev/null as the output does not have to be formatted.

Quoted 4 lines

I feel like you're making this a little too complicated, as all I'm doing is shuffling around already existing logic. I understand the want to make things more robust but this seems unnecessarily complex.

ok. I was just giving my thoughts on how I would approach it.

Thanks,
Stefan

#30 lore raw

Re: [PATCH v2 5/6] submodule: improve submodule_has_commits

Quoted 16 lines · Stefan Beller
Quoted 8 lines · Brandon Williams
Quoted 7 lines · Stefan Beller

Review

Quoted 2 lines · Brandon Williams
(quoted diff) +2 -0
+
+               if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len)

eh, I gave too much and self-contradicting feedback here earlier, ideally I'd like to review this to be similar as:

Quoted 6 lines
Quoted 2 lines

if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1)
die("cannot capture git-rev-list in submodule '%s', sub->path);

This wouldn't really work because if you provide a SHA1 to rev-list which it isn't able to find then it returns a non-zero exit code which would cause this to die, which isn't the desired behavior.

Quoted 9 lines

Oh. In that case, why do we even check for its stdout?
(from rev-lists man page)
--quiet
Don’t print anything to standard output. This form is primarily meant to allow the caller to test the exit status to see if a range of objects is fully connected (or not). It is faster than redirecting stdout to /dev/null as the output does not have to be formatted.

Sounds good, Let me take a look at using --quiet

Quoted 9 lines
Quoted 4 lines

I feel like you're making this a little too complicated, as all I'm doing is shuffling around already existing logic. I understand the want to make things more robust but this seems unnecessarily complex.

ok. I was just giving my thoughts on how I would approach it.

Thanks,
Stefan

Signature · 1 line
Brandon Williams
#31 lore raw

Re: [PATCH v2 5/6] submodule: improve submodule_has_commits

Quoted 27 lines · Brandon Williams
Quoted 16 lines · Stefan Beller
Quoted 8 lines · Brandon Williams
Quoted 7 lines · Stefan Beller

Review

Quoted 2 lines · Brandon Williams
(quoted diff) +2 -0
+
+               if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len)

eh, I gave too much and self-contradicting feedback here earlier, ideally I'd like to review this to be similar as:

Quoted 6 lines
Quoted 2 lines

if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1)
die("cannot capture git-rev-list in submodule '%s', sub->path);

This wouldn't really work because if you provide a SHA1 to rev-list which it isn't able to find then it returns a non-zero exit code which would cause this to die, which isn't the desired behavior.

Quoted 9 lines

Oh. In that case, why do we even check for its stdout?
(from rev-lists man page)
--quiet
Don’t print anything to standard output. This form is primarily meant to allow the caller to test the exit status to see if a range of objects is fully connected (or not). It is faster than redirecting stdout to /dev/null as the output does not have to be formatted.

Quoted 1 line

Sounds good, Let me take a look at using --quiet

From our offline discussion --quiet doesn't do what we want here. We are checking (1) if the commit exists and (2) if it is reachable. Using --quiet would only satisfy (1)

Quoted 13 lines
Quoted 10 lines
Quoted 5 lines

I feel like you're making this a little too complicated, as all I'm doing is shuffling around already existing logic. I understand the want to make things more robust but this seems unnecessarily complex.

ok. I was just giving my thoughts on how I would approach it.

Thanks,
Stefan

Signature · 1 line
Brandon Williams
Signature · 1 line
Brandon Williams
#32 lore raw

PATCH v2 6/6 submodule: refactor logic to determine changed submodules

In reply to Brandon Williams

There are currently two instances (fetch and push) where we want to determine if submodules have changed given some revision specification. These two instances don't use the same logic to generate a list of changed submodules and as a result there is a fair amount of code duplication.

This patch refactors these two code paths such that they both use the same logic to generate a list of changed submodules. This also makes it easier for future callers to be able to reuse this logic as they only need to create an argv_array with the revision specification to be using during the revision walk.

Change-Id Ie2381bd3eaecf5cec62e127df470f27a44ea47da
---
 submodule.c | 247 ++++++++++++++++++++++++++----------------------------------
 1 file changed, 105 insertions(+), 142 deletions(-)
submodule.c +105 -142
index 057695e64..7eaa3d384 100644
@@ -617,6 +617,94 @@ const struct submodule *submodule_from_ce(const struct cache_entry *ce)
 	return submodule_from_path(null_sha1, ce->name);
 }
 
+static struct oid_array *submodule_commits(struct string_list *submodules,
+					   const char *path)
+{
+	struct string_list_item *item;
+
+	item = string_list_insert(submodules, path);
+	if (item->util)
+		return (struct oid_array *) item->util;
+
+	/* NEEDSWORK: should we have oid_array_init()? */
+	item->util = xcalloc(1, sizeof(struct oid_array));
+	return (struct oid_array *) item->util;
+}
+
+static void collect_changed_submodules_cb(struct diff_queue_struct *q,
+					  struct diff_options *options,
+					  void *data)
+{
+	int i;
+	struct string_list *changed = data;
+
+	for (i = 0; i < q->nr; i++) {
+		struct diff_filepair *p = q->queue[i];
+		struct oid_array *commits;
+		if (!S_ISGITLINK(p->two->mode))
+			continue;
+
+		if (S_ISGITLINK(p->one->mode)) {
+			/*
+			 * NEEDSWORK: We should honor the name configured in
+			 * the .gitmodules file of the commit we are examining
+			 * here to be able to correctly follow submodules
+			 * being moved around.
+			 */
+			commits = submodule_commits(changed, p->two->path);
+			oid_array_append(commits, &p->two->oid);
+		} else {
+			/* Submodule is new or was moved here */
+			/*
+			 * NEEDSWORK: When the .git directories of submodules
+			 * live inside the superprojects .git directory some
+			 * day we should fetch new submodules directly into
+			 * that location too when config or options request
+			 * that so they can be checked out from there.
+			 */
+			continue;
+		}
+	}
+}
+
+/*
+ * Collect the paths of submodules in 'changed' which have changed based on
+ * the revisions as specified in 'argv'.  Each entry in 'changed' will also
+ * have a corresponding 'struct oid_array' (in the 'util' field) which lists
+ * what the submodule pointers were updated to during the change.
+ */
+static void collect_changed_submodules(struct string_list *changed,
+				       struct argv_array *argv)
+{
+	struct rev_info rev;
+	const struct commit *commit;
+
+	init_revisions(&rev, NULL);
+	setup_revisions(argv->argc, argv->argv, &rev, NULL);
+	if (prepare_revision_walk(&rev))
+		die("revision walk setup failed");
+
+	while ((commit = get_revision(&rev))) {
+		struct rev_info diff_rev;
+
+		init_revisions(&diff_rev, NULL);
+		diff_rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
+		diff_rev.diffopt.format_callback = collect_changed_submodules_cb;
+		diff_rev.diffopt.format_callback_data = changed;
+		diff_tree_combined_merge(commit, 1, &diff_rev);
+	}
+
+	reset_revision_walk();
+}
+
+static void free_submodules_oids(struct string_list *submodules)
+{
+	struct string_list_item *item;
+	for_each_string_list_item(item, submodules)
+		oid_array_clear((struct oid_array *) item->util);
+	string_list_clear(submodules, 1);
+}
+
 static int has_remote(const char *refname, const struct object_id *oid,
 		      int flags, void *cb_data)
 {
@@ -729,92 +817,31 @@ static int submodule_needs_pushing(const char *path, struct oid_array *commits)
 	return 0;
 }
 
-static struct oid_array *submodule_commits(struct string_list *submodules,
-					    const char *path)
-{
-	struct string_list_item *item;
-
-	item = string_list_insert(submodules, path);
-	if (item->util)
-		return (struct oid_array *) item->util;
-
-	/* NEEDSWORK: should we have oid_array_init()? */
-	item->util = xcalloc(1, sizeof(struct oid_array));
-	return (struct oid_array *) item->util;
-}
-
-static void collect_submodules_from_diff(struct diff_queue_struct *q,
-					 struct diff_options *options,
-					 void *data)
-{
-	int i;
-	struct string_list *submodules = data;
-
-	for (i = 0; i < q->nr; i++) {
-		struct diff_filepair *p = q->queue[i];
-		struct oid_array *commits;
-		if (!S_ISGITLINK(p->two->mode))
-			continue;
-		commits = submodule_commits(submodules, p->two->path);
-		oid_array_append(commits, &p->two->oid);
-	}
-}
-
-static void find_unpushed_submodule_commits(struct commit *commit,
-		struct string_list *needs_pushing)
-{
-	struct rev_info rev;
-
-	init_revisions(&rev, NULL);
-	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
-	rev.diffopt.format_callback = collect_submodules_from_diff;
-	rev.diffopt.format_callback_data = needs_pushing;
-	diff_tree_combined_merge(commit, 1, &rev);
-}
-
-static void free_submodules_oids(struct string_list *submodules)
-{
-	struct string_list_item *item;
-	for_each_string_list_item(item, submodules)
-		oid_array_clear((struct oid_array *) item->util);
-	string_list_clear(submodules, 1);
-}
-
 int find_unpushed_submodules(struct oid_array *commits,
 		const char *remotes_name, struct string_list *needs_pushing)
 {
-	struct rev_info rev;
-	struct commit *commit;
 	struct string_list submodules = STRING_LIST_INIT_DUP;
 	struct string_list_item *submodule;
 	struct argv_array argv = ARGV_ARRAY_INIT;
 
-	init_revisions(&rev, NULL);
-
 	/* argv.argv[0] will be ignored by setup_revisions */
 	argv_array_push(&argv, "find_unpushed_submodules");
 	oid_array_for_each_unique(commits, append_oid_to_argv, &argv);
 	argv_array_push(&argv, "--not");
 	argv_array_pushf(&argv, "--remotes=%s", remotes_name);
 
-	setup_revisions(argv.argc, argv.argv, &rev, NULL);
-	if (prepare_revision_walk(&rev))
-		die("revision walk setup failed");
-
-	while ((commit = get_revision(&rev)) != NULL)
-		find_unpushed_submodule_commits(commit, &submodules);
-
-	reset_revision_walk();
-	argv_array_clear(&argv);
+	collect_changed_submodules(&submodules, &argv);
 
 	for_each_string_list_item(submodule, &submodules) {
-		struct oid_array *commits = (struct oid_array *) submodule->util;
+		struct oid_array *commits = submodule->util;
+		const char *path = submodule->string;
 
-		if (submodule_needs_pushing(submodule->string, commits))
-			string_list_insert(needs_pushing, submodule->string);
+		if (submodule_needs_pushing(path, commits))
+			string_list_insert(needs_pushing, path);
 	}
 
 	free_submodules_oids(&submodules);
+	argv_array_clear(&argv);
 
 	return needs_pushing->nr;
 }
@@ -931,61 +958,6 @@ int push_unpushed_submodules(struct oid_array *commits,
 	return ret;
 }
 
-static int is_submodule_commit_present(const char *path, unsigned char sha1[20])
-{
-	int is_present = 0;
-	if (!add_submodule_odb(path) && lookup_commit_reference(sha1)) {
-		/* Even if the submodule is checked out and the commit is
-		 * present, make sure it is reachable from a ref. */
-		struct child_process cp = CHILD_PROCESS_INIT;
-		const char *argv[] = {"rev-list", "-n", "1", NULL, "--not", "--all", NULL};
-		struct strbuf buf = STRBUF_INIT;
-
-		argv[3] = sha1_to_hex(sha1);
-		cp.argv = argv;
-		prepare_submodule_repo_env(&cp.env_array);
-		cp.git_cmd = 1;
-		cp.no_stdin = 1;
-		cp.dir = path;
-		if (!capture_command(&cp, &buf, 1024) && !buf.len)
-			is_present = 1;
-
-		strbuf_release(&buf);
-	}
-	return is_present;
-}
-
-static void submodule_collect_changed_cb(struct diff_queue_struct *q,
-					 struct diff_options *options,
-					 void *data)
-{
-	int i;
-	for (i = 0; i < q->nr; i++) {
-		struct diff_filepair *p = q->queue[i];
-		if (!S_ISGITLINK(p->two->mode))
-			continue;
-
-		if (S_ISGITLINK(p->one->mode)) {
-			/* NEEDSWORK: We should honor the name configured in
-			 * the .gitmodules file of the commit we are examining
-			 * here to be able to correctly follow submodules
-			 * being moved around. */
-			struct string_list_item *path;
-			path = unsorted_string_list_lookup(&changed_submodule_paths, p->two->path);
-			if (!path && !is_submodule_commit_present(p->two->path, p->two->oid.hash))
-				string_list_append(&changed_submodule_paths, p->two->path);
-		} else {
-			/* Submodule is new or was moved here */
-			/* NEEDSWORK: When the .git directories of submodules
-			 * live inside the superprojects .git directory some
-			 * day we should fetch new submodules directly into
-			 * that location too when config or options request
-			 * that so they can be checked out from there. */
-			continue;
-		}
-	}
-}
-
 static int append_oid_to_array(const char *ref, const struct object_id *oid,
 			       int flags, void *data)
 {
@@ -1006,45 +978,36 @@ void check_for_new_submodule_commits(struct object_id *oid)
 
 static void calculate_changed_submodule_paths(void)
 {
-	struct rev_info rev;
-	struct commit *commit;
 	struct argv_array argv = ARGV_ARRAY_INIT;
+	struct string_list changed_submodules = STRING_LIST_INIT_DUP;
+	const struct string_list_item *item;
 
 	/* No need to check if there are no submodules configured */
 	if (!submodule_from_path(NULL, NULL))
 		return;
 
-	init_revisions(&rev, NULL);
 	argv_array_push(&argv, "--"); /* argv[0] program name */
 	oid_array_for_each_unique(&ref_tips_after_fetch,
 				   append_oid_to_argv, &argv);
 	argv_array_push(&argv, "--not");
 	oid_array_for_each_unique(&ref_tips_before_fetch,
 				   append_oid_to_argv, &argv);
-	setup_revisions(argv.argc, argv.argv, &rev, NULL);
-	if (prepare_revision_walk(&rev))
-		die("revision walk setup failed");
 
 	/*
 	 * Collect all submodules (whether checked out or not) for which new
 	 * commits have been recorded upstream in "changed_submodule_paths".
 	 */
-	while ((commit = get_revision(&rev))) {
-		struct commit_list *parent = commit->parents;
-		while (parent) {
-			struct diff_options diff_opts;
-			diff_setup(&diff_opts);
-			DIFF_OPT_SET(&diff_opts, RECURSIVE);
-			diff_opts.output_format |= DIFF_FORMAT_CALLBACK;
-			diff_opts.format_callback = submodule_collect_changed_cb;
-			diff_setup_done(&diff_opts);
-			diff_tree_sha1(parent->item->object.oid.hash, commit->object.oid.hash, "", &diff_opts);
-			diffcore_std(&diff_opts);
-			diff_flush(&diff_opts);
-			parent = parent->next;
-		}
+	collect_changed_submodules(&changed_submodules, &argv);
+
+	for_each_string_list_item(item, &changed_submodules) {
+		struct oid_array *commits = item->util;
+		const char *path = item->string;
+
+		if (!submodule_has_commits(path, commits))
+			string_list_append(&changed_submodule_paths, path);
 	}
 
+	free_submodules_oids(&changed_submodules);
 	argv_array_clear(&argv);
 	oid_array_clear(&ref_tips_before_fetch);
 	oid_array_clear(&ref_tips_after_fetch);
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog