[PATCH] push: Don't push a repository with unpushed submodules

[PATCH] push: Don't push a repository with unpushed submodules

[Fredrik Gustafsson]

When working with submodules it is easy to forget to push a
submodule to the server but pushing a super-project that
contains a commit for that submodule. The result is that the
superproject points at a submodule commit that is not available
on the server.

This adds the option --recurse-submodules=check to push. When
using this option git will check that all submodule commits that
are about to be pushed are present on a remote of the submodule.

This does not guarantee that all submodules a super-project
needs will be available on the server. In that case both the
super-project and the submodules would need an atomic push. This
does however prevent the human error of forgetting to push a
submodule.

Signed-off-by: Fredrik Gustafsson <iveqy@iveqy.com>
Mentored-by: Jens Lehmann <Jens.Lehmann@web.de>
Mentored-by: Heiko Voigt <hvoigt@hvoigt.net>
---
We decided to make this a one patch series since by the default
the check is turned off.

The first iteration of this patch series can be found here:
http://thread.gmane.org/gmane.comp.version-control.git/176328/focus=176327

A new test is added in this iteration to show a bug that now is fixed.

 Documentation/git-push.txt     |    6 ++
 builtin/push.c                 |   17 +++++
 submodule.c                    |  129 ++++++++++++++++++++++++++++++++++++++++
 submodule.h                    |    1 +
 t/t5531-deep-submodule-push.sh |   99 ++++++++++++++++++++++++++++++
 transport.c                    |    9 +++
 transport.h                    |    1 +
 7 files changed, 262 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 88acfcd..6ae6ba3 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -162,6 +162,12 @@ useful if you write an alias or script around 'git push'.
 	is specified. This flag forces progress status even if the
 	standard error stream is not directed to a terminal.
 
+--recurse-submodules=check::
+	Check whether all submodule commits used by the revisions to be
+	pushed are available on a remote tracking branch. Otherwise the
+	push will be aborted and the command will exit with non-zero status.
+
+
 include::urls-remotes.txt[]
 
 OUTPUT
diff --git a/builtin/push.c b/builtin/push.c
index 9cebf9e..23ba365 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -8,6 +8,7 @@
 #include "remote.h"
 #include "transport.h"
 #include "parse-options.h"
+#include "submodule.h"
 
 static const char * const push_usage[] = {
 	"git push [<options>] [<repository> [<refspec>...]]",
@@ -219,6 +220,19 @@ static int do_push(const char *repo, int flags)
 	return !!errs;
 }
 
+static int option_parse_recurse_submodules(const struct option *opt,
+				   const char *arg, int unset)
+{
+	int *flags = opt->value;
+	if (arg) {
+		if (!strcmp(arg, "check"))
+			*flags |= TRANSPORT_RECURSE_SUBMODULES_CHECK;
+		else
+			die("bad %s argument: %s", opt->long_name, arg);
+	}
+	return 0;
+}
+
 int cmd_push(int argc, const char **argv, const char *prefix)
 {
 	int flags = 0;
@@ -236,6 +250,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BIT('n' , "dry-run", &flags, "dry run", TRANSPORT_PUSH_DRY_RUN),
 		OPT_BIT( 0,  "porcelain", &flags, "machine-readable output", TRANSPORT_PUSH_PORCELAIN),
 		OPT_BIT('f', "force", &flags, "force updates", TRANSPORT_PUSH_FORCE),
+		{ OPTION_CALLBACK, 0, "recurse-submodules", &flags, "check",
+			"controls recursive pushing of submodules",
+			PARSE_OPT_OPTARG, option_parse_recurse_submodules },
 		OPT_BOOLEAN( 0 , "thin", &thin, "use thin pack"),
 		OPT_STRING( 0 , "receive-pack", &receivepack, "receive-pack", "receive pack program"),
 		OPT_STRING( 0 , "exec", &receivepack, "receive-pack", "receive pack program"),
diff --git a/submodule.c b/submodule.c
index 1ba9646..c44472f 100644
--- a/submodule.c
+++ b/submodule.c
@@ -308,6 +308,135 @@ void set_config_fetch_recurse_submodules(int value)
 	config_fetch_recurse_submodules = value;
 }
 
+static int has_remote(const char *refname, const unsigned char *sha1, int flags, void *cb_data)
+{
+	return 1;
+}
+
+/*
+ * checks whether a certain submodule commit is pushed based on the
+ * submodules remote tracking branches. returns 1 in case no remote
+ * tracking branches are found.
+ */
+static int is_submodule_commit_pushed(const char *path, const unsigned char sha1[20])
+{
+	int is_pushed = 0;
+	if (!add_submodule_odb(path) && lookup_commit_reference(sha1)) {
+		if (for_each_remote_ref_submodule(path, has_remote, NULL)) {
+			struct child_process cp;
+			const char *argv[] = {"rev-list", NULL, "--not", "--remotes", "-n", "1" , NULL};
+			struct strbuf buf = STRBUF_INIT;
+
+			argv[1] = sha1_to_hex(sha1);
+			memset(&cp, 0, sizeof(cp));
+			cp.argv = argv;
+			cp.env = local_repo_env;
+			cp.git_cmd = 1;
+			cp.no_stdin = 1;
+			cp.out = -1;
+			cp.dir = path;
+			if (!run_command(&cp) && !strbuf_read(&buf, cp.out, 41))
+				is_pushed = 1;
+			close(cp.out);
+			strbuf_release(&buf);
+		} else
+			is_pushed = 1;
+	}
+	return is_pushed;
+}
+
+static void collect_unpushed_submodules_from_revs(struct diff_queue_struct *q,
+					 struct diff_options *options,
+					 void *data)
+{
+	int i;
+	int *found_unpushed_submodule = data;
+
+	for (i = 0; i < q->nr; i++) {
+		struct diff_filepair *p = q->queue[i];
+		if (!S_ISGITLINK(p->two->mode))
+			continue;
+		if (!is_submodule_commit_pushed(p->two->path, p->two->sha1)) {
+			*found_unpushed_submodule = 1;
+			break;
+		}
+	}
+}
+
+static int collect_unpushed_submodules_in_tree(const unsigned char *sha1,
+					const char *base, int baselen,
+					const char *pathname, unsigned mode,
+					int stage, void *context)
+{
+	int *found_unpushed_submodules = context;
+	struct strbuf path = STRBUF_INIT;
+
+	strbuf_add(&path, base, strlen(base));
+	strbuf_add(&path, pathname, strlen(pathname));
+
+	if (S_ISGITLINK(mode) && !is_submodule_commit_pushed(path.buf, sha1)) {
+		*found_unpushed_submodules = 1;
+		return 0;
+	}
+	return READ_TREE_RECURSIVE;
+}
+
+static void parent_commits_pushed(struct commit *commit, struct commit_list *parent, int *found_unpushed_submodule)
+{
+	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 = collect_unpushed_submodules_from_revs;
+		diff_opts.format_callback_data = found_unpushed_submodule;
+		if (diff_setup_done(&diff_opts) < 0)
+			die("diff_setup_done failed");
+		diff_tree_sha1(parent->item->object.sha1, commit->object.sha1, "", &diff_opts);
+		diffcore_std(&diff_opts);
+		diff_flush(&diff_opts);
+		parent = parent->next;
+	}
+}
+
+static void tree_commits_pushed(struct commit *commit, int *found_unpushed_submodule)
+{
+	struct tree * tree;
+	struct pathspec pathspec;
+	tree = parse_tree_indirect(commit->object.sha1);
+	init_pathspec(&pathspec,NULL);
+	read_tree_recursive(tree, "", 0, 0, &pathspec, collect_unpushed_submodules_in_tree,
+			    found_unpushed_submodule);
+}
+
+int check_for_unpushed_submodule_commits(unsigned char new_sha1[20])
+{
+	struct rev_info rev;
+	struct commit *commit;
+	const char *argv[] = {NULL, NULL, "--not", "--remotes", NULL};
+	int argc = ARRAY_SIZE(argv) - 1;
+	char *sha1_copy;
+	int found_unpushed_submodule = 0;
+
+	init_revisions(&rev, NULL);
+	sha1_copy = xstrdup(sha1_to_hex(new_sha1));
+	argv[1] = sha1_copy;
+	setup_revisions(argc, argv, &rev, NULL);
+	if (prepare_revision_walk(&rev))
+		die("revision walk setup failed");
+
+	while ((commit = get_revision(&rev)) && !found_unpushed_submodule) {
+		struct commit_list *parent = commit->parents;
+		if (parent)
+			parent_commits_pushed(commit, parent, &found_unpushed_submodule);
+		else
+			tree_commits_pushed(commit, &found_unpushed_submodule);
+	}
+
+	free(sha1_copy);
+	return found_unpushed_submodule;
+}
+
 static int is_submodule_commit_present(const char *path, unsigned char sha1[20])
 {
 	int is_present = 0;
diff --git a/submodule.h b/submodule.h
index 5350b0d..0a4d395 100644
--- a/submodule.h
+++ b/submodule.h
@@ -29,5 +29,6 @@ int fetch_populated_submodules(int num_options, const char **options,
 unsigned is_submodule_modified(const char *path, int ignore_untracked);
 int merge_submodule(unsigned char result[20], const char *path, const unsigned char base[20],
 		    const unsigned char a[20], const unsigned char b[20]);
+int check_for_unpushed_submodule_commits(unsigned char sha1[20]);
 
 #endif
diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index faa2e96..d3a1789 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -32,4 +32,103 @@ test_expect_success push '
 	)
 '
 
+test_expect_success 'push if submodule has no remote' '
+	(
+		cd work/gar/bage &&
+		>junk2 &&
+		git add junk2 &&
+		git commit -m "Second junk"
+	) &&
+	(
+		cd work &&
+		git add gar/bage &&
+		git commit -m "Second commit for gar/bage" &&
+		git push --recurse-submodules=check ../pub.git master
+	)
+'
+
+test_expect_success 'push fails if submodule commit not on remote' '
+	(
+		cd work/gar &&
+		git clone --bare bage ../../submodule.git &&
+		cd bage &&
+		git remote add origin ../../../submodule.git &&
+		git fetch &&
+		>junk3 &&
+		git add junk3 &&
+		git commit -m "Third junk"
+	) &&
+	(
+		cd work &&
+		git add gar/bage &&
+		git commit -m "Third commit for gar/bage" &&
+		test_must_fail git push --recurse-submodules=check ../pub.git master
+	)
+'
+
+test_expect_success 'push succeeds after commit was pushed to remote' '
+	(
+		cd work/gar/bage &&
+		git push origin master
+	) &&
+	(
+		cd work &&
+		git push --recurse-submodules=check ../pub.git master
+	)
+'
+
+test_expect_success 'push fails when commit on multiple branches if one branch has no remote' '
+	(
+		cd work/gar/bage &&
+		>junk4 &&
+		git add junk4 &&
+		git commit -m "Fourth junk"
+	) &&
+	(
+		cd work &&
+		git branch branch2 &&
+		git add gar/bage &&
+		git commit -m "Fourth commit for gar/bage" &&
+		git checkout branch2 &&
+		(
+			cd gar/bage &&
+			git checkout HEAD~1
+		) &&
+		>junk1 &&
+		git add junk1 &&
+		git commit -m "First junk" &&
+		test_must_fail git push --recurse-submodules=check ../pub.git
+	)
+'
+
+test_expect_success 'push succeeds if submodule has no remote and is on the first superproject commit' '
+	mkdir a &&
+	(
+		cd a &&
+		git init --bare
+	) &&
+	git clone a a1 &&
+	(
+		cd a1 &&
+		mkdir b &&
+		(
+			cd b &&
+			git init &&
+			>junk &&
+			git add junk &&
+			git commit -m "initial"
+		) &&
+		git add b &&
+		git commit -m "added submodule" &&
+		git push origin master
+	)
+'
+
+test_expect_success 'push succeeds when --no-recurse-submodules is used' '
+	(
+		cd work &&
+		git push ../pub.git --no-recurse-submodules
+	)
+'
+
 test_done
diff --git a/transport.c b/transport.c
index c9c8056..9c100c9 100644
--- a/transport.c
+++ b/transport.c
@@ -10,6 +10,7 @@
 #include "refs.h"
 #include "branch.h"
 #include "url.h"
+#include "submodule.h"
 
 /* rsync support */
 
@@ -1041,6 +1042,14 @@ int transport_push(struct transport *transport,
 			flags & TRANSPORT_PUSH_MIRROR,
 			flags & TRANSPORT_PUSH_FORCE);
 
+		if ((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) && !is_bare_repository()) {
+			struct ref *ref = remote_refs;
+			for (; ref; ref = ref->next)
+				if (!is_null_sha1(ref->new_sha1) &&
+				    check_for_unpushed_submodule_commits(ref->new_sha1))
+					die("There are unpushed submodules, aborting.");
+		}
+
 		push_ret = transport->push_refs(transport, remote_refs, flags);
 		err = push_had_errors(remote_refs);
 		ret = push_ret | err;
diff --git a/transport.h b/transport.h
index 161d724..059b330 100644
--- a/transport.h
+++ b/transport.h
@@ -101,6 +101,7 @@ struct transport {
 #define TRANSPORT_PUSH_MIRROR 8
 #define TRANSPORT_PUSH_PORCELAIN 16
 #define TRANSPORT_PUSH_SET_UPSTREAM 32
+#define TRANSPORT_RECURSE_SUBMODULES_CHECK 64
 
 #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
 
-- 
1.7.6.397.g23ae2.dirty

Re: [PATCH] push: Don't push a repository with unpushed submodules

[Junio C Hamano]

Fredrik Gustafsson <iveqy@iveqy.com> writes:

> When working with submodules it is easy to forget to push a
> submodule to the server but pushing a super-project that
> contains a commit for that submodule. The result is that the
> superproject points at a submodule commit that is not available
> on the server.
>
> This adds the option --recurse-submodules=check to push. When
> using this option git will check that all submodule commits that
> are about to be pushed are present on a remote of the submodule.

Looking better.

> This does not guarantee that all submodules a super-project
> needs will be available on the server.

Hmm, why not?

You do not, when this option is in effect, push a new commit of the
superproject to the remote unless you know all the submodule commits
pointed at by the superproject commit you are pushing out are already
pushed out, right? If so how could some submodules become unavailable on
the other end?

Are you thinking about some submodules that you did not touch may have
been updated by somebody else, rewinding the branch that contained the
commit that you did not modify and that you have in your superproject
commit, or something? But ...

> In that case both the
> super-project and the submodules would need an atomic push.

... such a case won't be helped with any atomic push, as you didn't even
update that submodule that somebody else rewound.

So you must be thinking about something else. Can you clarify the failure
mode you are worried about a bit better here?

You may also drop the "atomic push might help something" from the
description; as it is unclear what it could help. Then what remains would
be...

> This
> does however prevent the human error of forgetting to push a
> submodule.

...this one, which is correct. This change is primarily to help prevent a
human error that was described in the first paragraph of the message (I do
not think it prevents any other kind of error). So while it is correct, it
may not have to be said again here.

> The first iteration of this patch series can be found here:
> http://thread.gmane.org/gmane.comp.version-control.git/176328/focus=176327

And the second iteration was here:

  http://thread.gmane.org/gmane.comp.version-control.git/177992

>  Documentation/git-push.txt     |    6 ++
>  builtin/push.c                 |   17 +++++
>  submodule.c                    |  129 ++++++++++++++++++++++++++++++++++++++++
>  submodule.h                    |    1 +
>  t/t5531-deep-submodule-push.sh |   99 ++++++++++++++++++++++++++++++
>  transport.c                    |    9 +++
>  transport.h                    |    1 +
>  7 files changed, 262 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index 88acfcd..6ae6ba3 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -162,6 +162,12 @@ useful if you write an alias or script around 'git push'.
>  	is specified. This flag forces progress status even if the
>  	standard error stream is not directed to a terminal.
>  
> +--recurse-submodules=check::
> +	Check whether all submodule commits used by the revisions to be
> +	pushed are available on a remote tracking branch. Otherwise the
> +	push will be aborted and the command will exit with non-zero status.
> +
> +
>  include::urls-remotes.txt[]
>  
>  OUTPUT
> diff --git a/builtin/push.c b/builtin/push.c
> index 9cebf9e..23ba365 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -8,6 +8,7 @@
>  #include "remote.h"
>  #include "transport.h"
>  #include "parse-options.h"
> +#include "submodule.h"
>  
>  static const char * const push_usage[] = {
>  	"git push [<options>] [<repository> [<refspec>...]]",
> @@ -219,6 +220,19 @@ static int do_push(const char *repo, int flags)
>  	return !!errs;
>  }
>  
> +static int option_parse_recurse_submodules(const struct option *opt,
> +				   const char *arg, int unset)
> +{
> +	int *flags = opt->value;
> +	if (arg) {
> +		if (!strcmp(arg, "check"))
> +			*flags |= TRANSPORT_RECURSE_SUBMODULES_CHECK;
> +		else
> +			die("bad %s argument: %s", opt->long_name, arg);
> +	}
> +	return 0;
> +}

Later you may want to add a configuration variable to flip the default,
and _before_ that happens (e.g. like "now"), you would need to be able to
accept --recurse-submodules=no-check or something that defeats the
configured default from the command line during the transition period.

Why not use a simple string-valued variable to keep the last value of this
option you saw instead of using a callback?  The overall control flow in
cmd_push() would look like this:

	const char *submodules = NULL; /* default */
        ...
	OPT_STRING(0, "recurse-submodules", &submodules, ...),
	...
	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
	parse_recurse_submodules_option(submodules, &flags);

and you set or unset the necessary bits in flags in the helper function.
That would also help people who use an alias with --recurse-submodules=
early on the command line to force his own default before configuration is
introduced by naturally implementing the usual "last one on the command
line wins" rule, no?

>  int cmd_push(int argc, const char **argv, const char *prefix)
>  {
>  	int flags = 0;
> @@ -236,6 +250,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)
>  		OPT_BIT('n' , "dry-run", &flags, "dry run", TRANSPORT_PUSH_DRY_RUN),
>  		OPT_BIT( 0,  "porcelain", &flags, "machine-readable output", TRANSPORT_PUSH_PORCELAIN),
>  		OPT_BIT('f', "force", &flags, "force updates", TRANSPORT_PUSH_FORCE),
> +		{ OPTION_CALLBACK, 0, "recurse-submodules", &flags, "check",
> +			"controls recursive pushing of submodules",
> +			PARSE_OPT_OPTARG, option_parse_recurse_submodules },
>  		OPT_BOOLEAN( 0 , "thin", &thin, "use thin pack"),
>  		OPT_STRING( 0 , "receive-pack", &receivepack, "receive-pack", "receive pack program"),
>  		OPT_STRING( 0 , "exec", &receivepack, "receive-pack", "receive pack program"),
> diff --git a/submodule.c b/submodule.c
> index 1ba9646..c44472f 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -308,6 +308,135 @@ void set_config_fetch_recurse_submodules(int value)
>  	config_fetch_recurse_submodules = value;
>  }
>  
> +static int has_remote(const char *refname, const unsigned char *sha1, int flags, void *cb_data)
> +{
> +	return 1;
> +}
> +
> +/*
> + * checks whether a certain submodule commit is pushed based on the
> + * submodules remote tracking branches. returns 1 in case no remote
> + * tracking branches are found.
> + */
> +static int is_submodule_commit_pushed(const char *path, const unsigned char sha1[20])
> +{
> +	int is_pushed = 0;
> +	if (!add_submodule_odb(path) && lookup_commit_reference(sha1)) {
> +		if (for_each_remote_ref_submodule(path, has_remote, NULL)) {

This code assumes that this for_each_remote_ref_submodule() call will
return non-zero only when has_remote() is called even once. Is that really
true? Doesn't the function (not has_remote() but the iterator) want to
signal errors in any way, and if so doesn't it return a negative value or
something?

> +			struct child_process cp;
> +			const char *argv[] = {"rev-list", NULL, "--not", "--remotes", "-n", "1" , NULL};
> +			struct strbuf buf = STRBUF_INIT;
> +
> +			argv[1] = sha1_to_hex(sha1);
> +			memset(&cp, 0, sizeof(cp));
> +			cp.argv = argv;
> +			cp.env = local_repo_env;
> +			cp.git_cmd = 1;
> +			cp.no_stdin = 1;
> +			cp.out = -1;
> +			cp.dir = path;
> +			if (!run_command(&cp) && !strbuf_read(&buf, cp.out, 41))
> +				is_pushed = 1;
> +			close(cp.out);
> +			strbuf_release(&buf);

Hmm, this run_command(), while it may not be a bug, feels somewhat fishy.

It calls start and then calls finish. What if the subprocess is really
fast, writes everything it needs to write (which is presumably 41 bytes at
most), closes its end of the pipe? And then you read from the other end of
the pipe. Am I being too paranoid to think that this sequence is ...

	start_command();
        strbuf_read();
        finish_command();
        close();
        strbuf_release();

... a more natural way of writing this logic?

> +		} else
> +			is_pushed = 1;

This case is not "we know it is pushed" but "we do not know (and we do not
care) if it was pushed, because the submodule does not have a tracking",
no?

Probably the function should be named "submodule_needs_pushing()" and
return the value accordingly. Then "we do not have to prevent the user
from pushing the superproject based on the state of this submodule" case
can say "No, this submodule repository does not need pushing", which reads
a lot more naturally. The overall code structure would also become
simpler:

	static int submodule_needs_pushing(...)
        {
		int needs_pushing = 0;
                if (!(we can inspect the objects in this submodule))
			return needs_pushing;

               	if (for_each_remote_ref_in_submodule(...)) {
                	... run rev-list ...
                       	if (found commit not in remotes)
				needs_pushing = 1;
			... clean-up rev-list ...                                
		}
		return needs_pushing;
	}

What happens in a submodule repository that you do not have a checkout?
the first if() statement in the function would be false, and you would end
up returning "is-pushed? no" and collect_unpushed_submodules_from_revs()
(which also is a misnomer; you do not care if commits in certain
submodules haven't been pushed---it should collect submodules that "needs
pushing", which is subtly different from "have not been pushed") that
called this function will say "Ah I found one, I found one, that is an
error!".

And I think that is wrong.

Imagine a merge in a superproject between two branches, among which only
one of them updated the submodule while the other one did not touch. As
far as that submodule path is concerned, the merge should resolve cleanly
without a checkout (or clone) of the submodule repository to take the
update from the branch that updated the submodule commit.

So the "submodule-needs-pushing()" function should return "no need to push
(there is nothing to push to begin with)" for a submodule that is not even
"submodule init"ed.

> +static int collect_unpushed_submodules_in_tree(const unsigned char *sha1,
> +					const char *base, int baselen,
> +					const char *pathname, unsigned mode,
> +					int stage, void *context)
> +{
> +	int *found_unpushed_submodules = context;
> +	struct strbuf path = STRBUF_INIT;
> +
> +	strbuf_add(&path, base, strlen(base));
> +	strbuf_add(&path, pathname, strlen(pathname));
> +
> +	if (S_ISGITLINK(mode) && !is_submodule_commit_pushed(path.buf, sha1)) {
> +		*found_unpushed_submodules = 1;
> +		return 0;
> +	}
> +	return READ_TREE_RECURSIVE;
> +}

I find it somewhat distasteful to have this separate codepath (and
tree_commits_pushed() function) only for the root commit. Why not use the
same diff-tree machinery?

> +static void parent_commits_pushed(struct commit *commit, struct commit_list *parent, int *found_unpushed_submodule)
> +{
> +	while (parent) {

Also I wonder if you really want to do pair-wise diff here, which would
mean that for a typical two-parent merge you will potentially have to
check the same submodule twice (no I am not talking about the cost of
running diff the same number of times as you have parents, but I am
talking about the cost of running submodule_needs_pushing()).

Would it make sense to run the equivalent of "show -c --name-only" here
internally ("diff-tree --root -c --name-only" to be more exact)? Then any
submodule path that match one of the parents you would not have to check
at all. Just thinking aloud...

> +		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 = collect_unpushed_submodules_from_revs;
> +		diff_opts.format_callback_data = found_unpushed_submodule;
> +		if (diff_setup_done(&diff_opts) < 0)
> +			die("diff_setup_done failed");
> +		diff_tree_sha1(parent->item->object.sha1, commit->object.sha1, "", &diff_opts);
> +		diffcore_std(&diff_opts);
> +		diff_flush(&diff_opts);
> +		parent = parent->next;
> +	}
> +}
> +
> +static void tree_commits_pushed(struct commit *commit, int *found_unpushed_submodule)
> +{
> +	struct tree * tree;

(style) struct tree *tree;

But I already suggested to get rid of this separate codepath, so it is not
such a big deal.

> +	struct pathspec pathspec;
> +	tree = parse_tree_indirect(commit->object.sha1);
> +	init_pathspec(&pathspec,NULL);
> +	read_tree_recursive(tree, "", 0, 0, &pathspec, collect_unpushed_submodules_in_tree,
> +			    found_unpushed_submodule);
> +}
> +
> +int check_for_unpushed_submodule_commits(unsigned char new_sha1[20])
> +{

The same "we are not looking for 'unpushed' ones, but for 'needs pushing'
ones" comment on naming applies here.

> +	struct rev_info rev;
> +	struct commit *commit;
> +	const char *argv[] = {NULL, NULL, "--not", "--remotes", NULL};
> +	int argc = ARRAY_SIZE(argv) - 1;
> +	char *sha1_copy;
> +	int found_unpushed_submodule = 0;
> +
> +	init_revisions(&rev, NULL);
> +	sha1_copy = xstrdup(sha1_to_hex(new_sha1));
> +	argv[1] = sha1_copy;
> +	setup_revisions(argc, argv, &rev, NULL);
> +	if (prepare_revision_walk(&rev))
> +		die("revision walk setup failed");
> +
> +	while ((commit = get_revision(&rev)) && !found_unpushed_submodule) {

Why are we checking all commits that are not in "any of the remotes" (I am
referring to "--not --remotes" in the argv[]) here, instead of all commits
that are not in "the remote we are pushing this superproject commit to"?
At this point in the codepath don't you (or you may not---I didn't check)
already know where you are pushing?

> +test_expect_success 'push succeeds if submodule has no remote and is on the first superproject commit' '
> +	mkdir a &&
> +	(
> +		cd a &&
> +		git init --bare
> +	) &&

Perhaps

	git init --bare a

would do?