threads / patch / 28055

patchpush: Don't push a repository with unpushed submodules

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

## tl;dr

2 messages between Aug 9, 2011 and Aug 9, 2011. Diffs are folded; open one to read it.

replies: 1people: 2as markdown or json

Fredrik Gustafsson· Aug 9, 2011, 18:15 UTC · lore

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(-)
Show changes to 7 files +262 −0

Documentation/git-push.txt, builtin/push.c, submodule.c, submodule.h, t/t5531-deep-submodule-push.sh, transport.c, transport.h

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
Junio C Hamano· Aug 9, 2011, 21:12 UTC · re: Fredrik Gustafsson · lore

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

Fredrik Gustafsson <iveqy@iveqy.com> writes:
Show 9 quoted lines
> 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
Show 54 quoted lines
>  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?

Show 36 quoted lines
>  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?

Show 16 quoted lines
> +			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.

Show 17 quoted lines
> +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...

Show 18 quoted lines
> +		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.

Show 9 quoted lines
> +	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.

Show 15 quoted lines
> +	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?

Show 6 quoted lines
> +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?

← back to recent threads