[RFC 0/2] push checks for unpushed remotes in submodules

When working with submodules it is easy to end up in a state when submodule
commits required by the super-project only is present locally. This is most
often a human error (although technical errors such as connection failure
can be a reason).

This patch-series tries to prevent pushing a super-project if not all (by
the super-project used) submodules are pushed first. This will prevent the
human error of forgetting to push submodules before pushing the
super-project.

This is a RFC-series, please consider:
* Is this going into the right direction?
* Should we use a new flag similar to REF_STATUS_REJECT_NONFASTFORWARD?

Future improvements could be:
* Make git status aware of unpushed submodules.
* Show a list of unpushed submodules when a push is denied.

Fredrik Gustafsson (1):
  Don't push a repository with unpushed submodules

Heiko Voigt (1):
  test whether push checks for unpushed remotes in submodules

 submodule.c                    |   79 ++++++++++++++++++++++++++++++++++++++++
 submodule.h                    |    1 +
 t/t5531-deep-submodule-push.sh |   68 ++++++++++++++++++++++++++++++++++
 transport.c                    |    8 ++++
 4 files changed, 156 insertions(+), 0 deletions(-)

-- 
1.7.6.rc3.2.g0185a

From: Heiko Voigt <redacted>

These tests are used to extend push to check whether all recorded
submodule commits have also been pushed.

Signed-off-by: Heiko Voigt <redacted>
---
 t/t5531-deep-submodule-push.sh |   46 +++++++++++++++++++++++++++++++++++++++-
 1 files changed, 45 insertions(+), 1 deletions(-)

diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index faa2e96..0b55466 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh

@@ -28,8 +28,52 @@ test_expect_success setup '
 test_expect_success push '
 	(
 		cd work &&
-		git push ../pub.git master
+		git push -f ../pub.git master
 	)
 '
 
+test_expect_failure 'push fails 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" &&
+		test_must_fail git push ../pub.git master
+	)
+'
+
+test_expect_failure '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 ../pub.git master
+	)
+'
+
+test_expect_failure 'push succeeds after commit was pushed to remote' '
+	(
+		cd work/gar/bage &&
+		git push origin master
+	) &&
+	(
+		cd work &&
+		git push ../pub.git master
+	)
+'
 test_done

-- 
1.7.6.rc3.2.g0185a

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 avaliable on the server.

Check that all submodule commits that are about to be pushed are present
on a remote of the submodule and require forcing if that is not the
case.

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 <redacted>
Mentored-by: Jens Lehmann [off-list ref]
Mentored-by: Heiko Voigt [off-list ref]
---
 submodule.c                    |   79 ++++++++++++++++++++++++++++++++++++++++
 submodule.h                    |    1 +
 t/t5531-deep-submodule-push.sh |   30 ++++++++++++++--
 transport.c                    |    8 ++++
 4 files changed, 115 insertions(+), 3 deletions(-)

diff --git a/submodule.c b/submodule.c
index b6dec70..fe1daae 100644
--- a/submodule.c
+++ b/submodule.c

@@ -308,6 +308,85 @@ void set_config_fetch_recurse_submodules(int value)
 	config_fetch_recurse_submodules = value;
 }
 
+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)) {
+		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);
+	}
+	return is_pushed;
+}
+
+static void submodule_collect_unpushed_cb(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;
+		}
+	}
+}
+
+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;
+		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_unpushed_cb;
+			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;
+		}
+	}
+	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 0b55466..0ef943a 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh

@@ -32,7 +32,7 @@ test_expect_success push '
 	)
 '
 
-test_expect_failure 'push fails if submodule has no remote' '
+test_expect_success 'push fails if submodule has no remote' '
 	(
 		cd work/gar/bage &&
 		>junk2 &&

@@ -47,7 +47,7 @@ test_expect_failure 'push fails if submodule has no remote' '
 	)
 '
 
-test_expect_failure 'push fails if submodule commit not on remote' '
+test_expect_success 'push fails if submodule commit not on remote' '
 	(
 		cd work/gar &&
 		git clone --bare bage ../../submodule.git &&

@@ -66,7 +66,7 @@ test_expect_failure 'push fails if submodule commit not on remote' '
 	)
 '
 
-test_expect_failure 'push succeeds after commit was pushed to remote' '
+test_expect_success 'push succeeds after commit was pushed to remote' '
 	(
 		cd work/gar/bage &&
 		git push origin master

@@ -76,4 +76,28 @@ test_expect_failure 'push succeeds after commit was pushed to remote' '
 		git push ../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 ../pub.git
+	)
+'
 test_done

diff --git a/transport.c b/transport.c
index c9c8056..d83595b 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,13 @@ int transport_push(struct transport *transport,
 			flags & TRANSPORT_PUSH_MIRROR,
 			flags & TRANSPORT_PUSH_FORCE);
 
+		if(!(flags & TRANSPORT_PUSH_FORCE) && !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's unpushed submodules, aborting. Use -f to force a push");
+		}
+
 		push_ret = transport->push_refs(transport, remote_refs, flags);
 		err = push_had_errors(remote_refs);
 		ret = push_ret | err;

-- 
1.7.6.rc3.2.g0185a

On 11-06-26 02:29 PM, Fredrik Gustafsson wrote:

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 avaliable on the server.

Check that all submodule commits that are about to be pushed are present
on a remote of the submodule and require forcing if that is not the
case.

I have a few concerns about what this is trying to do.

First, I expect performance will be terrible in repositories with large
and/or many submodules.  I'd go so far as to say that it's pretty much a
show-stopper for our repository.

Second, there are many times where I'm working in a submodule on branch
"TopicA" but not in branch "TopicB".  If I've made submodule changes in
TopicA then try to push up TopicB, won't I have have to tell push to "-f"?
But that turns off other checks that I'd rather keep.

I'd feel a lot better about this patch if the check was *off* by default and
there was a config setting / command-line option to turn it on.

I also agree with Junio that this kind of verification makes more sense in a
hook on the server side.

		M.

Am 29.06.2011 00:06, schrieb Marc Branchaud:

First, I expect performance will be terrible in repositories with large
and/or many submodules.  I'd go so far as to say that it's pretty much a
show-stopper for our repository.

Large submodules won't be the problem here, but many submodules and/or
many commits might cause some performance degradations here. But please
notice that there is no communication with the upstream of the submodules,
we only check the refs present locally. Do you still think doing some
"git rev-list" invocations in your submodules will be a problem? Then
please say so.

Second, there are many times where I'm working in a submodule on branch
"TopicA" but not in branch "TopicB".  If I've made submodule changes in
TopicA then try to push up TopicB, won't I have have to tell push to "-f"?
But that turns off other checks that I'd rather keep.

Nope, this patch only checks the refs to be pushed, not any others. So it
will only check that all submodule commits on branch "TopicB" are pushed.

I'd feel a lot better about this patch if the check was *off* by default and
there was a config setting / command-line option to turn it on.

I have no objections against making that configurable, although I tend
towards making this check default "on". But please feel free to test this
feature and tell us if it really hinders you in your work or does cause
performance degradation, we'll really appreciate the feedback!

I also agree with Junio that this kind of verification makes more sense in a
hook on the server side.

Hmm, but isn't that assuming that the server knows (= hosts?) both the
submodule and the superproject?

On Tue, Jun 28, 2011 at 06:06:35PM -0400, Marc Branchaud wrote:

On 11-06-26 02:29 PM, Fredrik Gustafsson wrote:

quoted

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 avaliable on the server.

Check that all submodule commits that are about to be pushed are present
on a remote of the submodule and require forcing if that is not the
case.

I have a few concerns about what this is trying to do.

First, I expect performance will be terrible in repositories with large
and/or many submodules.  I'd go so far as to say that it's pretty much a
show-stopper for our repository.

This is not acceptable (in my opinion). The point of making this (only) a
client side check was to not have a huge performance impact.

Would you care for testing this in your enviroment or give me enough
data to be able to set up a test enviroment size-wize like yours?

Second, there are many times where I'm working in a submodule on branch
"TopicA" but not in branch "TopicB".  If I've made submodule changes in
TopicA then try to push up TopicB, won't I have have to tell push to "-f"?
But that turns off other checks that I'd rather keep.

I don't quite follow you here. Anyway, only the commits of the
superproject that you are about to push is checked, and only the
submodules that are changed in any of those commits are examined.

So if you're working in TopicA in a submodule and tries to push TopicB
in a superproject that uses TopicC in the submodule, TopicA will not be
required to be pushed. (if so, is it a bug).

I'd feel a lot better about this patch if the check was *off* by default and
there was a config setting / command-line option to turn it on.

I also agree with Junio that this kind of verification makes more sense in a
hook on the server side.

Serverside, we cannot guarantee that all submodules are reachable, they
might be on different servers, maybe not even connected to eachothers. Even
if they are connected this would requiring network traffic. Making this check 
an even bigger performance killer. This check is not supposed to guarantee a 
sane server-repo (that would be much harder) and therefore this check is 
"overkill" to have on the server-side. Client side we always have all
information needed for this.

Note the problem:
"Prevent the developer of pushing a superrepo that has submodule
(commits) only locally avaliable"

That's the problem we're trying to solve, NOT:

"Prevent the developer of pushing a superrepo that has submodule
(commits) not avaliable for an other developer"

The second problem is just too complex and too slow to solve in a generic
way.

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iveqy@iveqy.com

On 11-06-28 06:32 PM, Jens Lehmann wrote:

Am 29.06.2011 00:06, schrieb Marc Branchaud:

quoted

First, I expect performance will be terrible in repositories with large
and/or many submodules.  I'd go so far as to say that it's pretty much a
show-stopper for our repository.

Large submodules won't be the problem here, but many submodules and/or
many commits might cause some performance degradations here. But please
notice that there is no communication with the upstream of the submodules,
we only check the refs present locally. Do you still think doing some
"git rev-list" invocations in your submodules will be a problem? Then
please say so.

Yes, that's exactly what I'm saying.  I'm concerned about rev-list's
performance, particularly when my system's disk cache is empty (or already
full with something else).  When I saw the patch I tried a quick
	git rev-list X --not --remotes -n 1
in a Linux kernel submodule where X was a non-existent ref, and watched my
disk whirl for a few minutes.

Admittedly that's nothing like what the patch does -- mea culpa.  I see now
that the check looks for refs in the submodule's recent history, so the scan
is likely to be pretty short.

Also, as you say below and Fredrik said in his reply, the patch only checks
submodules affected by a super-repo ref that is being pushed.  So I agree
that the performance hit is likely to be minimal even with large submodules.

(Fredrik, FYI my super-repo itself is large enough to make "git status" take
up a good part of the disk cache.  This repo has several submodules,
including a few (different) Linux kernel repos.  I already get bogged down a
bit when I have to status the main repo and one of the Linux submodules.  So
anything that adds extra submodule inspection makes me nervous.)

quoted

Second, there are many times where I'm working in a submodule on branch
"TopicA" but not in branch "TopicB".  If I've made submodule changes in
TopicA then try to push up TopicB, won't I have have to tell push to "-f"?
But that turns off other checks that I'd rather keep.

Nope, this patch only checks the refs to be pushed, not any others. So it
will only check that all submodule commits on branch "TopicB" are pushed.

Thanks for pointing that out!

quoted

I'd feel a lot better about this patch if the check was *off* by default and
there was a config setting / command-line option to turn it on.

I have no objections against making that configurable, although I tend
towards making this check default "on". But please feel free to test this
feature and tell us if it really hinders you in your work or does cause
performance degradation, we'll really appreciate the feedback!

Well I'm less concerned about it being configurable now.  I did a few more
tests and the rev-list performance looked fine.

		M.

On 11-06-28 07:02 PM, Fredrik Gustafsson wrote:

 [ ... ]

Serverside, we cannot guarantee that all submodules are reachable, they
might be on different servers, maybe not even connected to eachothers. Even
if they are connected this would requiring network traffic. Making this check 
an even bigger performance killer. This check is not supposed to guarantee a 
sane server-repo (that would be much harder) and therefore this check is 
"overkill" to have on the server-side. Client side we always have all
information needed for this.

Note the problem:
"Prevent the developer of pushing a superrepo that has submodule
(commits) only locally avaliable"

That's the problem we're trying to solve, NOT:

"Prevent the developer of pushing a superrepo that has submodule
(commits) not avaliable for an other developer"

The second problem is just too complex and too slow to solve in a generic
way.

Fair enough.

So my only remaining concern is that using "push -f" to override the check is
too much.  I'd rather see a different option control this.  Maybe
--ignore-submodules?  (I think it'd be fine if -f implied --ignore-submodules.)

		M.