[PATCH v2 0/3] check for unpushed remotes in submodules

[PATCH v2 0/3] check for unpushed remotes in submodules

[Fredrik Gustafsson]

When working with submodules it is easy to end up in a state when submodule
commits required by the superproject 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 superproject if not all (by
the superproject used) submodules are pushed first. This will prevent the
human error of forgetting to push submodules before pushing the
superproject.

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

The changes from the previous iteration are in patch 0003.

Regarding the discussion of superprojects with submodules that have no
remote tracking branches: A push will still be denied.  After some
discussion we did not consider this a drawback since all submodules that
are added/handled using the submodule script will have remote tracking
branches. Even if the submodules are not pushable when the superproject
records a commit that is reachable from a remote branch the push will not
be denied.

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

Fredrik Gustafsson (2):
  push: Don't push a repository with unpushed submodules
  push: Add the --no-recurse-submodules option

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

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

-- 
1.7.6.236.g7ad21

[PATCH v2 1/3] test whether push checks for unpushed remotes in submodules

[Fredrik Gustafsson]

From: Heiko Voigt <hvoigt@hvoigt.net>

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

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 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.236.g7ad21

[PATCH v2 2/3] 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 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 <iveqy@iveqy.com>
Mentored-by: Jens Lehmann <Jens.Lehmann@web.de>
Mentored-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 submodule.c                    |  115 ++++++++++++++++++++++++++++++++++++++++
 submodule.h                    |    1 +
 t/t5531-deep-submodule-push.sh |   54 ++++++++++++++++++-
 transport.c                    |    8 +++
 4 files changed, 175 insertions(+), 3 deletions(-)

diff --git a/submodule.c b/submodule.c
index 1ba9646..3820f1b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -308,6 +308,121 @@ 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 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)) {
+		if(!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 0b55466..15474c1 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,52 @@ 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_expect_success 'push fails 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" &&
+		test_must_fail git push origin master
+	)
+'
+
 test_done
diff --git a/transport.c b/transport.c
index c9c8056..e0fd435 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 are 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.236.g7ad21

[PATCH v2 3/3] push: Add the --no-recurse-submodules option

[Fredrik Gustafsson]

This adds the option --no-recurse-submodules to push. That is, git
will not check if the submodules are pushed. -f or --force still also
disables this check. Documentation is also updated.

Signed-off-by: Fredrik Gustafsson <iveqy@iveqy.com>
Mentored-by: Jens Lehmann <Jens.Lehmann@web.de>
Mentored-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 Documentation/git-push.txt     |    6 ++++++
 builtin/push.c                 |    1 +
 t/t5531-deep-submodule-push.sh |    7 +++++++
 transport.c                    |    4 +++-
 transport.h                    |    1 +
 5 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 88acfcd..d63a57c 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -113,6 +113,8 @@ nor in any Push line of the corresponding remotes file---see below).
 	not an ancestor of the local ref used to overwrite it.
 	This flag disables the check.  This can cause the
 	remote repository to lose commits; use it with care.
++
+	This also enforces --no-recurse-submodules
 
 --repo=<repository>::
 	This option is only relevant if no <repository> argument is
@@ -162,6 +164,10 @@ 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.
 
+--no-recurse-submodules::
+	Don't check if all submodule commits this repository refers to are
+	pushed to their remotes.
+
 include::urls-remotes.txt[]
 
 OUTPUT
diff --git a/builtin/push.c b/builtin/push.c
index 9cebf9e..07a8b11 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -236,6 +236,7 @@ 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),
+		OPT_BIT(0, "no-recurse-submodules", &flags, "do not recurse submodules", TRANSPORT_PUSH_NO_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/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index 15474c1..74c615c 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -124,4 +124,11 @@ test_expect_success 'push fails if submodule has no remote and is on the first s
 	)
 '
 
+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 e0fd435..9681560 100644
--- a/transport.c
+++ b/transport.c
@@ -1042,7 +1042,9 @@ int transport_push(struct transport *transport,
 			flags & TRANSPORT_PUSH_MIRROR,
 			flags & TRANSPORT_PUSH_FORCE);
 
-		if(!(flags & TRANSPORT_PUSH_FORCE) && !is_bare_repository()) {
+		if(!(flags & TRANSPORT_PUSH_NO_RECURSE_SUBMODULES) &&
+		   !(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))
diff --git a/transport.h b/transport.h
index 161d724..c6ccf8c 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_PUSH_NO_RECURSE_SUBMODULES 64
 
 #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
 
-- 
1.7.6.236.g7ad21

Re: [PATCH v2 3/3] push: Add the --no-recurse-submodules option

[Junio C Hamano]

Fredrik Gustafsson <iveqy@iveqy.com> writes:

> This adds the option --no-recurse-submodules to push. That is, git

I think this needs to be renamed at least for two reasons.

The name makes it sound as if "push --recurse-submodules" would
recursively visit the submodules and runs "push" there, but I do not think
that is what this flag does.

> diff --git a/transport.h b/transport.h
> index 161d724..c6ccf8c 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_PUSH_NO_RECURSE_SUBMODULES 64

Also naming the constant with NO will invite an unnecessary double
negation, like this:

	if (!(flags & FROTZ_NO_NITFOL))
        	do_nitfol_to_frotz();

Besides, I would be moderately annoyed if this check were the default.

Re: [PATCH v2 3/3] push: Add the --no-recurse-submodules option

[Jens Lehmann]

Am 28.07.2011 22:05, schrieb Junio C Hamano:
> Fredrik Gustafsson <iveqy@iveqy.com> writes:
> 
>> This adds the option --no-recurse-submodules to push. That is, git
> 
> I think this needs to be renamed at least for two reasons.
> 
> The name makes it sound as if "push --recurse-submodules" would
> recursively visit the submodules and runs "push" there, but I do not think
> that is what this flag does.

That is because the patch that does this is still in the making ;-)

The cover letter should have mentioned it, but we talked about making push
pretty symmetric to fetch:

- Use "--no-recurse-submodules" if you don't want submodules to be pushed
  (we added that right now so users can disable the behavior the second
  commit introduces)
- Use "--recurse-submodules=on-demand" to push only those submodules where
  new commits have been recorded in the superproject's refs to be pushed
- Use "--recurse-submodules" to unconditionally push everything in the
  submodules too
- Make the default configurable by a "push.recurseSubmodules" option

We'll need another round to discuss how to handle private submodules which
were never intended to be pushed, but I think the general idea of having
fetch and push use similar options makes sense, no?

Re: [PATCH v2 3/3] push: Add the --no-recurse-submodules option

[Jens Lehmann]

Am 28.07.2011 22:05, schrieb Junio C Hamano:
> Fredrik Gustafsson <iveqy@iveqy.com> writes:
>> diff --git a/transport.h b/transport.h
>> index 161d724..c6ccf8c 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_PUSH_NO_RECURSE_SUBMODULES 64
> 
> Also naming the constant with NO will invite an unnecessary double
> negation, like this:
> 
> 	if (!(flags & FROTZ_NO_NITFOL))
>         	do_nitfol_to_frotz();

Right, we'll drop the "NO_" from that constant.

> Besides, I would be moderately annoyed if this check were the default.

We will skip the check for submodules without remotes, does that lessen
your annoyance? We still think it is a good idea to have that test enabled
by default, but it might be a good idea to wait with that until we provide
a central config option to enable users to turn that off with a simple
"git config push.recurseSubmodules off". What do you think?

Re: [PATCH v2 3/3] push: Add the --no-recurse-submodules option

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> We will skip the check for submodules without remotes, does that lessen
> your annoyance? We still think it is a good idea to have that test enabled
> by default, but it might be a good idea to wait with that until we provide
> a central config option to enable users to turn that off with a simple
> "git config push.recurseSubmodules off". What do you think?

I would rather see a questionable (in the sense that it can trigger false
positive and annoy users of workflows you didn't anticipate) new feature
turned off by default, so that interested people who _do_ want to can use
it knowing what they are getting into, to find out as much unexpected
fallout as possible. So if we want to give this topic early graduation, I
would suggest making it a command line option that defaults to off first,
then in a later commit add a configuration option to enable people to turn
it on (and allow those who do not see the point of the new feature to keep
it turned off permanently). Enabling the feature by default can be done
in a later version after the feature proves to be truly useful for people
who opted into the experiment.

Re: [PATCH v2 0/3] check for unpushed remotes in submodules

[Junio C Hamano]

Fredrik Gustafsson <iveqy@iveqy.com> writes:

> Regarding the discussion of superprojects with submodules that have no
> remote tracking branches: A push will still be denied.

I have marked one part of the data synchronized across machines as
"private" submodule (which contains my gpg keychains, encrypted password
files, personaly memos, etc.) and push only the outer "shell" superproject
(which has tools that I use everywhere to go to $HOME/bin among other
things) to certain machines without the private parts, and the
superproject is designed to work without a checkout (nor clone) of a
submodule.

With this patch series, it sounds like I cannot use this repository
structure anymore, which is sad.

Re: [PATCH v2 0/3] check for unpushed remotes in submodules

[Jens Lehmann]

Am 28.07.2011 21:58, schrieb Junio C Hamano:
> Fredrik Gustafsson <iveqy@iveqy.com> writes:
> 
>> Regarding the discussion of superprojects with submodules that have no
>> remote tracking branches: A push will still be denied.
> 
> I have marked one part of the data synchronized across machines as
> "private" submodule (which contains my gpg keychains, encrypted password
> files, personaly memos, etc.) and push only the outer "shell" superproject
> (which has tools that I use everywhere to go to $HOME/bin among other
> things) to certain machines without the private parts, and the
> superproject is designed to work without a checkout (nor clone) of a
> submodule.
> 
> With this patch series, it sounds like I cannot use this repository
> structure anymore, which is sad.

Thanks for bringing this use case up. Now I understand why you asked if
submodules without remote tracking branches should be checked too. We
discussed that and couldn't think of a scenario where the user doesn't
want to have remote tracking branches in a submodule, but we missed the
use case you described here. Back to the drawing board ...