[PATCH 0/6] changed submodules

[PATCH 0/6] changed submodules

[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.

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(-)

-- 
2.13.0.rc0.306.g87b477812d-goog

[PATCH 1/6] submodule: rename add_sha1_to_array

[Brandon Williams]

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.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 submodule.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/submodule.c b/submodule.c
index d3299e29c..be0f5d847 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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;
 	}
 
-- 
2.13.0.rc0.306.g87b477812d-goog

[PATCH 2/6] submodule: rename free_submodules_sha1s

[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.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 submodule.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index be0f5d847..46abd52b1 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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;
 }
-- 
2.13.0.rc0.306.g87b477812d-goog

[PATCH 3/6] submodule: remove add_oid_to_argv

[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.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 submodule.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/submodule.c b/submodule.c
index 46abd52b1..7baa28ae0 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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");
-- 
2.13.0.rc0.306.g87b477812d-goog

[PATCH 4/6] submodule: change string_list changed_submodule_paths

[Brandon Williams]

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

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 submodule.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index 7baa28ae0..3bcf44521 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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
-- 
2.13.0.rc0.306.g87b477812d-goog

[PATCH 5/6] submodule: improve submodule_has_commits

[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.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 submodule.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/submodule.c b/submodule.c
index 3bcf44521..100d31d39 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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;
 }
 
-- 
2.13.0.rc0.306.g87b477812d-goog

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

[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.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 submodule.c | 247 ++++++++++++++++++++++++++----------------------------------
 1 file changed, 105 insertions(+), 142 deletions(-)

diff --git a/submodule.c b/submodule.c
index 100d31d39..88093779e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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);
-- 
2.13.0.rc0.306.g87b477812d-goog

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

[Stefan Beller]

On Fri, Apr 28, 2017 at 4:54 PM, Brandon Williams <bmwill@google.com> wrote:
> 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)?

> diff --git a/submodule.c b/submodule.c
> index 3bcf44521..100d31d39 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -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;

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/20170324223848.GH31294@aiede.mtv.corp.google.com/

    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

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

[Stefan Beller]

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

On Fri, Apr 28, 2017 at 4:54 PM, Brandon Williams <bmwill@google.com> wrote:
> 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. :)

> -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")



> +
> +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 ?

> @@ -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;

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

>  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

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

[Brandon Williams]

On 04/28, Stefan Beller wrote:
> On Fri, Apr 28, 2017 at 4:54 PM, Brandon Williams <bmwill@google.com> wrote:
> > 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)?

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?

-- 
Brandon Williams

Re: [PATCH 0/6] changed submodules

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> 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.

>
> 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(-)

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

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> 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.

> object_id' and not sha1 character arrays.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  submodule.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index d3299e29c..be0f5d847 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -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;
>  	}

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

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> Eliminate a call to 'xstrdup()' by changing the string_list
> 'changed_submodule_paths' to duplicated strings added to it.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  submodule.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 7baa28ae0..3bcf44521 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -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);

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 ;-)

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

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

>  	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;
>  }

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.

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

[Brandon Williams]

On 04/30, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > Eliminate a call to 'xstrdup()' by changing the string_list
> > 'changed_submodule_paths' to duplicated strings added to it.
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> >  submodule.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/submodule.c b/submodule.c
> > index 7baa28ae0..3bcf44521 100644
> > --- a/submodule.c
> > +++ b/submodule.c
> > @@ -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);
> 
> 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 ;-)

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).

-- 
Brandon Williams

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

[Brandon Williams]

On 04/30, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> >  	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;
> >  }
> 
> 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.

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

> 
> 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 ;)

-- 
Brandon Williams

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

[Brandon Williams]

On 04/28, Stefan Beller wrote:
> + Heiko, who touched the pushing code end of last year.
> 
> On Fri, Apr 28, 2017 at 4:54 PM, Brandon Williams <bmwill@google.com> wrote:
> > 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. :)
> 
> > -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")

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

> 
> > +
> > +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.

> 
> > @@ -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;
> 
> 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.

> 
> >  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

-- 
Brandon Williams

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

[Stefan Beller]

On Sun, Apr 30, 2017 at 4:14 PM, Brandon Williams <bmwill@google.com> wrote:

> 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)?

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

[Brandon Williams]

On 05/01, Stefan Beller wrote:
> On Sun, Apr 30, 2017 at 4:14 PM, Brandon Williams <bmwill@google.com> wrote:
> 
> > 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)?

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

-- 
Brandon Williams

[PATCH v2 0/6] changed submodules

[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(-)

-- 
2.13.0.rc1.294.g07d810a77f-goog

[PATCH v2 1/6] submodule: rename add_sha1_to_array

[Brandon Williams]

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
Signed-off-by: Brandon Williams <bmwill@google.com>
---
 submodule.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/submodule.c b/submodule.c
index d3299e29c..be0f5d847 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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;
 	}
 
-- 
2.13.0.rc1.294.g07d810a77f-goog

[PATCH v2 2/6] submodule: rename free_submodules_sha1s

[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
Signed-off-by: Brandon Williams <bmwill@google.com>
---
 submodule.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index be0f5d847..46abd52b1 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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;
 }
-- 
2.13.0.rc1.294.g07d810a77f-goog

[PATCH v2 3/6] submodule: remove add_oid_to_argv

[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
Signed-off-by: Brandon Williams <bmwill@google.com>
---
 submodule.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/submodule.c b/submodule.c
index 46abd52b1..7baa28ae0 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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");
-- 
2.13.0.rc1.294.g07d810a77f-goog

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

[Brandon Williams]

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

Change-Id: Id4b53837a6e209c0c0837c9f5ba06c70df2ffe06
Signed-off-by: Brandon Williams <bmwill@google.com>
---
 submodule.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index 7baa28ae0..3bcf44521 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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
-- 
2.13.0.rc1.294.g07d810a77f-goog

[PATCH v2 5/6] submodule: improve submodule_has_commits

[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
Signed-off-by: Brandon Williams <bmwill@google.com>
---
 submodule.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/submodule.c b/submodule.c
index 3bcf44521..057695e64 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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;
 }
 
-- 
2.13.0.rc1.294.g07d810a77f-goog

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

[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
Signed-off-by: Brandon Williams <bmwill@google.com>
---
 submodule.c | 247 ++++++++++++++++++++++++++----------------------------------
 1 file changed, 105 insertions(+), 142 deletions(-)

diff --git a/submodule.c b/submodule.c
index 057695e64..7eaa3d384 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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);
-- 
2.13.0.rc1.294.g07d810a77f-goog

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

[Stefan Beller]

On Mon, May 1, 2017 at 6:02 PM, Brandon Williams <bmwill@google.com> wrote:

> 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

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

[Brandon Williams]

On 05/01, Stefan Beller wrote:
> On Mon, May 1, 2017 at 6:02 PM, Brandon Williams <bmwill@google.com> wrote:
> 
> > Change-Id: Ia6d15f34cee4d0dc32f7a475c69f4cb3aa8ce5bf
> 
> Uh?

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

> 
> 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

-- 
Brandon Williams

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

[Stefan Beller]

On Mon, May 1, 2017 at 6:02 PM, Brandon Williams <bmwill@google.com> wrote:
> 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
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  submodule.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/submodule.c b/submodule.c
> index 3bcf44521..057695e64 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -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;

Thanks for the comment!

>
>         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)

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

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

[Brandon Williams]

On 05/01, Stefan Beller wrote:
> On Mon, May 1, 2017 at 6:02 PM, Brandon Williams <bmwill@google.com> wrote:
> > +
> > +               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.

> 
>     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;
>     }

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.

> ---
> 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

-- 
Brandon Williams

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

[Stefan Beller]

On Tue, May 2, 2017 at 10:25 AM, Brandon Williams <bmwill@google.com> wrote:
> On 05/01, Stefan Beller wrote:
>> On Mon, May 1, 2017 at 6:02 PM, Brandon Williams <bmwill@google.com> wrote:
>> > +
>> > +               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.

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.

>
> 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

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

[Brandon Williams]

On 05/02, Stefan Beller wrote:
> On Tue, May 2, 2017 at 10:25 AM, Brandon Williams <bmwill@google.com> wrote:
> > On 05/01, Stefan Beller wrote:
> >> On Mon, May 1, 2017 at 6:02 PM, Brandon Williams <bmwill@google.com> wrote:
> >> > +
> >> > +               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.
> 
> 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

> >
> > 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

-- 
Brandon Williams

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

[Brandon Williams]

On 05/02, Brandon Williams wrote:
> On 05/02, Stefan Beller wrote:
> > On Tue, May 2, 2017 at 10:25 AM, Brandon Williams <bmwill@google.com> wrote:
> > > On 05/01, Stefan Beller wrote:
> > >> On Mon, May 1, 2017 at 6:02 PM, Brandon Williams <bmwill@google.com> wrote:
> > >> > +
> > >> > +               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.
> > 
> > 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

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)

> 
> > >
> > > 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
> 
> -- 
> Brandon Williams

-- 
Brandon Williams