[PATCH v5 0/3] push: submodule support

[PATCH v5 0/3] push: submodule support

[Heiko Voigt]

I have rewritten and cleaned up the recursive submodule on-demand push
patch from Fredrik. Since this involved changing large parts of it I
have taken over the ownership. To pay credit that this is built upon
Fredriks work I have left the signed-off and mentored-by footers the
same. I hope that this is the proper way to handle such cases. If
someone comes up with a better idea I am happy to change things.

Cheers Heiko

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

The second iteration of this patch series can be found here:
http://thread.gmane.org/gmane.comp.version-control.git/177992

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

The fourth iteration of this patch series can be found here:
http://thread.gmane.org/gmane.comp.version-control.git/179731

Heiko Voigt (3):
  Teach revision walking machinery to walk multiple times sequencially
  Refactor submodule push check to use string list instead of integer
  push: teach --recurse-submodules the on-demand option

 .gitignore                                       |    1 +
 Documentation/git-push.txt                       |   14 +++-
 Documentation/technical/api-revision-walking.txt |    5 +
 Makefile                                         |    1 +
 builtin/push.c                                   |    7 ++
 object.c                                         |   11 +++
 object.h                                         |    2 +
 revision.c                                       |    5 +
 revision.h                                       |    1 +
 submodule.c                                      |   70 ++++++++++++++---
 submodule.h                                      |    4 +-
 t/t0062-revision-walking.sh                      |   33 ++++++++
 t/t5531-deep-submodule-push.sh                   |   94 ++++++++++++++++++++++
 test-revision-walking.c                          |   66 +++++++++++++++
 transport.c                                      |   41 +++++++++-
 transport.h                                      |    1 +
 16 files changed, 338 insertions(+), 18 deletions(-)
 create mode 100755 t/t0062-revision-walking.sh
 create mode 100644 test-revision-walking.c

-- 
1.7.9.114.gead08

[PATCH v5 1/3] Teach revision walking machinery to walk multiple times sequencially

[Heiko Voigt]

Previously it was not possible to iterate revisions twice using the
revision walking api. We add a reset_revision_walk() which clears the
used flags. This allows us to do multiple sequencial revision walks.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 .gitignore                                       |    1 +
 Documentation/technical/api-revision-walking.txt |    5 ++
 Makefile                                         |    1 +
 object.c                                         |   11 ++++
 object.h                                         |    2 +
 revision.c                                       |    5 ++
 revision.h                                       |    1 +
 submodule.c                                      |    2 +
 t/t0062-revision-walking.sh                      |   33 +++++++++++
 test-revision-walking.c                          |   66 ++++++++++++++++++++++
 10 files changed, 127 insertions(+), 0 deletions(-)
 create mode 100755 t/t0062-revision-walking.sh
 create mode 100644 test-revision-walking.c

diff --git a/.gitignore b/.gitignore
index 3b7680e..2f09842 100644
--- a/.gitignore
+++ b/.gitignore
@@ -184,6 +184,7 @@
 /test-obj-pool
 /test-parse-options
 /test-path-utils
+/test-revision-walking
 /test-run-command
 /test-sha1
 /test-sigchain
diff --git a/Documentation/technical/api-revision-walking.txt b/Documentation/technical/api-revision-walking.txt
index 996da05..b7d0d9a 100644
--- a/Documentation/technical/api-revision-walking.txt
+++ b/Documentation/technical/api-revision-walking.txt
@@ -56,6 +56,11 @@ function.
 	returning a `struct commit *` each time you call it. The end of the
 	revision list is indicated by returning a NULL pointer.
 
+`reset_revision_walk`::
+
+	Reset the flags used by the revision walking api. You can use
+	this to do multiple sequencial revision walks.
+
 Data structures
 ---------------
 
diff --git a/Makefile b/Makefile
index c457c34..bff686f 100644
--- a/Makefile
+++ b/Makefile
@@ -472,6 +472,7 @@ TEST_PROGRAMS_NEED_X += test-mktemp
 TEST_PROGRAMS_NEED_X += test-obj-pool
 TEST_PROGRAMS_NEED_X += test-parse-options
 TEST_PROGRAMS_NEED_X += test-path-utils
+TEST_PROGRAMS_NEED_X += test-revision-walking
 TEST_PROGRAMS_NEED_X += test-run-command
 TEST_PROGRAMS_NEED_X += test-sha1
 TEST_PROGRAMS_NEED_X += test-sigchain
diff --git a/object.c b/object.c
index 6b06297..6291ce9 100644
--- a/object.c
+++ b/object.c
@@ -275,3 +275,14 @@ void object_array_remove_duplicates(struct object_array *array)
 		array->nr = dst;
 	}
 }
+
+void clear_object_flags(unsigned flags)
+{
+	int i;
+	struct object *obj;
+
+	for (i=0; i < obj_hash_size; i++) {
+		if ((obj = obj_hash[i]) && obj->flags & flags)
+			obj->flags &= ~flags;
+	}
+}
diff --git a/object.h b/object.h
index b6618d9..6a97b6b 100644
--- a/object.h
+++ b/object.h
@@ -76,4 +76,6 @@ void add_object_array(struct object *obj, const char *name, struct object_array
 void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode);
 void object_array_remove_duplicates(struct object_array *);
 
+void clear_object_flags(unsigned flags);
+
 #endif /* OBJECT_H */
diff --git a/revision.c b/revision.c
index c97d834..77ce6bd 100644
--- a/revision.c
+++ b/revision.c
@@ -2061,6 +2061,11 @@ static void set_children(struct rev_info *revs)
 	}
 }
 
+void reset_revision_walk()
+{
+	clear_object_flags(SEEN | ADDED | SHOWN);
+}
+
 int prepare_revision_walk(struct rev_info *revs)
 {
 	int nr = revs->pending.nr;
diff --git a/revision.h b/revision.h
index b8e9223..3535733 100644
--- a/revision.h
+++ b/revision.h
@@ -192,6 +192,7 @@ extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ct
 				 const char * const usagestr[]);
 extern int handle_revision_arg(const char *arg, struct rev_info *revs,int flags,int cant_be_filename);
 
+extern void reset_revision_walk();
 extern int prepare_revision_walk(struct rev_info *revs);
 extern struct commit *get_revision(struct rev_info *revs);
 extern char *get_revision_mark(const struct rev_info *revs, const struct commit *commit);
diff --git a/submodule.c b/submodule.c
index 9a28060..645ff5d 100644
--- a/submodule.c
+++ b/submodule.c
@@ -404,6 +404,7 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remote
 	while ((commit = get_revision(&rev)) && !needs_pushing)
 		commit_need_pushing(commit, &needs_pushing);
 
+	reset_revision_walk();
 	free(sha1_copy);
 	strbuf_release(&remotes_arg);
 
@@ -741,6 +742,7 @@ static int find_first_merges(struct object_array *result, const char *path,
 		if (in_merge_bases(b, &commit, 1))
 			add_object_array(o, NULL, &merges);
 	}
+	reset_revision_walk();
 
 	/* Now we've got all merges that contain a and b. Prune all
 	 * merges that contain another found merge and save them in
diff --git a/t/t0062-revision-walking.sh b/t/t0062-revision-walking.sh
new file mode 100755
index 0000000..3d98eb8
--- /dev/null
+++ b/t/t0062-revision-walking.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+#
+# Copyright (c) 2012 Heiko Voigt
+#
+
+test_description='Test revision walking api'
+
+. ./test-lib.sh
+
+cat >run_twice_expected <<-EOF
+1st
+ > add b
+ > add a
+2nd
+ > add b
+ > add a
+EOF
+
+test_expect_success 'setup' '
+	echo a > a &&
+	git add a &&
+	git commit -m "add a" &&
+	echo b > b &&
+	git add b &&
+	git commit -m "add b"
+'
+
+test_expect_success 'revision walking can be done twice' '
+	test-revision-walking run-twice > run_twice_actual
+	test_cmp run_twice_expected run_twice_actual
+'
+
+test_done
diff --git a/test-revision-walking.c b/test-revision-walking.c
new file mode 100644
index 0000000..27ad597
--- /dev/null
+++ b/test-revision-walking.c
@@ -0,0 +1,66 @@
+/*
+ * test-revision-walking.c: test revision walking API.
+ *
+ * (C) 2012 Heiko Voigt <hvoigt@hvoigt.net>
+ *
+ * This code is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include "cache.h"
+#include "commit.h"
+#include "diff.h"
+#include "revision.h"
+
+static void print_commit(struct commit *commit)
+{
+	struct strbuf sb = STRBUF_INIT;
+	struct pretty_print_context ctx = {0};
+	ctx.date_mode = DATE_NORMAL;
+	format_commit_message(commit, " %m %s", &sb, &ctx);
+	printf("%s\n", sb.buf);
+	strbuf_release(&sb);
+}
+
+static int run_revision_walk()
+{
+	struct rev_info rev;
+	struct commit *commit;
+	const char *argv[] = {NULL, "--all", NULL};
+	int argc = ARRAY_SIZE(argv) - 1;
+	int got_revision = 0;
+
+	init_revisions(&rev, NULL);
+	setup_revisions(argc, argv, &rev, NULL);
+	if (prepare_revision_walk(&rev))
+		die("revision walk setup failed");
+
+	while ((commit = get_revision(&rev)) != NULL) {
+		print_commit(commit);
+		got_revision = 1;
+	}
+
+	reset_revision_walk();
+	return got_revision;
+}
+
+int main(int argc, char **argv)
+{
+	if (argc < 2)
+		return 1;
+
+	if (!strcmp(argv[1], "run-twice")) {
+		printf("1st\n");
+		if (!run_revision_walk())
+			return 1;
+		printf("2nd\n");
+		if (!run_revision_walk())
+			return 1;
+
+		return 0;
+	}
+
+	fprintf(stderr, "check usage\n");
+	return 1;
+}
-- 
1.7.9.114.gead08

[PATCH v5 2/3] Refactor submodule push check to use string list instead of integer

[Heiko Voigt]

This allows us to tell the user which submodules have not been pushed.
Additionally this is helpful when we want to automatically try to push
submodules that have not been pushed.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 submodule.c |   20 ++++++++++----------
 submodule.h |    3 ++-
 transport.c |   24 ++++++++++++++++++++++--
 3 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/submodule.c b/submodule.c
index 645ff5d..3c714c2 100644
--- a/submodule.c
+++ b/submodule.c
@@ -357,21 +357,20 @@ static void collect_submodules_from_diff(struct diff_queue_struct *q,
 					 void *data)
 {
 	int i;
-	int *needs_pushing = data;
+	struct string_list *needs_pushing = data;
 
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filepair *p = q->queue[i];
 		if (!S_ISGITLINK(p->two->mode))
 			continue;
 		if (submodule_needs_pushing(p->two->path, p->two->sha1)) {
-			*needs_pushing = 1;
-			break;
+			if (!string_list_has_string(needs_pushing, p->two->path))
+				string_list_insert(needs_pushing, p->two->path);
 		}
 	}
 }
 
-
-static void commit_need_pushing(struct commit *commit, int *needs_pushing)
+static void commit_need_pushing(struct commit *commit, struct string_list *needs_pushing)
 {
 	struct rev_info rev;
 
@@ -382,14 +381,15 @@ static void commit_need_pushing(struct commit *commit, int *needs_pushing)
 	diff_tree_combined_merge(commit, 1, &rev);
 }
 
-int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name)
+int check_submodule_needs_pushing(unsigned char new_sha1[20],
+		const char *remotes_name, struct string_list *needs_pushing)
 {
 	struct rev_info rev;
 	struct commit *commit;
 	const char *argv[] = {NULL, NULL, "--not", "NULL", NULL};
 	int argc = ARRAY_SIZE(argv) - 1;
 	char *sha1_copy;
-	int needs_pushing = 0;
+
 	struct strbuf remotes_arg = STRBUF_INIT;
 
 	strbuf_addf(&remotes_arg, "--remotes=%s", remotes_name);
@@ -401,14 +401,14 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remote
 	if (prepare_revision_walk(&rev))
 		die("revision walk setup failed");
 
-	while ((commit = get_revision(&rev)) && !needs_pushing)
-		commit_need_pushing(commit, &needs_pushing);
+	while ((commit = get_revision(&rev)) != NULL)
+		commit_need_pushing(commit, needs_pushing);
 
 	reset_revision_walk();
 	free(sha1_copy);
 	strbuf_release(&remotes_arg);
 
-	return needs_pushing;
+	return needs_pushing->nr;
 }
 
 static int is_submodule_commit_present(const char *path, unsigned char sha1[20])
diff --git a/submodule.h b/submodule.h
index 80e04f3..ddd1941 100644
--- a/submodule.h
+++ b/submodule.h
@@ -29,6 +29,7 @@ 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 search);
-int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name);
+int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name,
+		struct string_list *needs_pushing);
 
 #endif
diff --git a/transport.c b/transport.c
index cac0c06..d13bd4a 100644
--- a/transport.c
+++ b/transport.c
@@ -11,6 +11,7 @@
 #include "branch.h"
 #include "url.h"
 #include "submodule.h"
+#include "string-list.h"
 
 /* rsync support */
 
@@ -1000,6 +1001,20 @@ void transport_set_verbosity(struct transport *transport, int verbosity,
 	transport->progress = force_progress || (verbosity >= 0 && isatty(2));
 }
 
+static void die_with_unpushed_submodules(struct string_list *needs_pushing)
+{
+	int i;
+
+	fprintf(stderr, "The following submodule paths contain changes that can\n"
+			"not be found on any remote:\n");
+	for (i = 0; i < needs_pushing->nr; i++)
+		printf("  %s\n", needs_pushing->items[i].string);
+
+	string_list_clear(needs_pushing, 0);
+
+	die("Aborting.");
+}
+
 int transport_push(struct transport *transport,
 		   int refspec_nr, const char **refspec, int flags,
 		   int *nonfastforward)
@@ -1040,10 +1055,15 @@ int transport_push(struct transport *transport,
 
 		if ((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) && !is_bare_repository()) {
 			struct ref *ref = remote_refs;
+			struct string_list needs_pushing;
+
+			memset(&needs_pushing, 0, sizeof(struct string_list));
+			needs_pushing.strdup_strings = 1;
 			for (; ref; ref = ref->next)
 				if (!is_null_sha1(ref->new_sha1) &&
-				    check_submodule_needs_pushing(ref->new_sha1,transport->remote->name))
-					die("There are unpushed submodules, aborting.");
+				    check_submodule_needs_pushing(ref->new_sha1,
+					    transport->remote->name, &needs_pushing))
+					die_with_unpushed_submodules(&needs_pushing);
 		}
 
 		push_ret = transport->push_refs(transport, remote_refs, flags);
-- 
1.7.9.114.gead08

[PATCH v5 3/3] push: teach --recurse-submodules the on-demand option

[Heiko Voigt]

When using this option git will search for all submodules that
have changed in the revisions to be send. It will then try to
push the currently checked out branch of each submodule.

This helps when a user has finished working on a change which
involves submodules and just wants to push everything in one go.

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     |   14 ++++--
 builtin/push.c                 |    7 +++
 submodule.c                    |   48 ++++++++++++++++++++
 submodule.h                    |    1 +
 t/t5531-deep-submodule-push.sh |   94 ++++++++++++++++++++++++++++++++++++++++
 transport.c                    |   17 +++++++-
 transport.h                    |    1 +
 7 files changed, 177 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index aede488..649ee3a 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -162,10 +162,16 @@ 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.
+--recurse-submodules=<check|on-demand>::
+	Make sure all submodule commits used by the revisions to be
+	pushed are available on a remote tracking branch. If check is
+	used it will be checked that all submodule commits that changed
+	in the revisions to be pushed are available on a remote.
+	Otherwise the push will be aborted and exit with non-zero
+	status. If on-demand is used all submodules that changed in the
+	revisions to be pushed will be pushed. If on-demand was not able
+	to push all necessary revisions it will also be aborted and exit
+	with non-zero status.
 
 
 include::urls-remotes.txt[]
diff --git a/builtin/push.c b/builtin/push.c
index 35cce53..f2ef8dd 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -224,9 +224,16 @@ static int option_parse_recurse_submodules(const struct option *opt,
 				   const char *arg, int unset)
 {
 	int *flags = opt->value;
+
+	if (*flags & (TRANSPORT_RECURSE_SUBMODULES_CHECK |
+		      TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND))
+		die("%s can only be used once.", opt->long_name);
+
 	if (arg) {
 		if (!strcmp(arg, "check"))
 			*flags |= TRANSPORT_RECURSE_SUBMODULES_CHECK;
+		else if (!strcmp(arg, "on-demand"))
+			*flags |= TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND;
 		else
 			die("bad %s argument: %s", opt->long_name, arg);
 	} else
diff --git a/submodule.c b/submodule.c
index 3c714c2..ff0cfd8 100644
--- a/submodule.c
+++ b/submodule.c
@@ -411,6 +411,54 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20],
 	return needs_pushing->nr;
 }
 
+static int push_submodule(const char *path)
+{
+	if (add_submodule_odb(path))
+		return 1;
+
+	if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
+		struct child_process cp;
+		const char *argv[] = {"push", NULL};
+
+		memset(&cp, 0, sizeof(cp));
+		cp.argv = argv;
+		cp.env = local_repo_env;
+		cp.git_cmd = 1;
+		cp.no_stdin = 1;
+		cp.dir = path;
+		if (run_command(&cp))
+			return 0;
+		close(cp.out);
+	}
+
+	return 1;
+}
+
+int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name)
+{
+	int i, ret = 1;
+	struct string_list needs_pushing;
+
+	memset(&needs_pushing, 0, sizeof(struct string_list));
+	needs_pushing.strdup_strings = 1;
+
+	if (!check_submodule_needs_pushing(new_sha1, remotes_name, &needs_pushing))
+		return 1;
+
+	for (i = 0; i < needs_pushing.nr; i++) {
+		const char *path = needs_pushing.items[i].string;
+		fprintf(stderr, "Pushing submodule '%s'\n", path);
+		if (!push_submodule(path)) {
+			fprintf(stderr, "Unable to push submodule '%s'\n", path);
+			ret = 0;
+		}
+	}
+
+	string_list_clear(&needs_pushing, 0);
+
+	return ret;
+}
+
 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 ddd1941..af74941 100644
--- a/submodule.h
+++ b/submodule.h
@@ -31,5 +31,6 @@ int merge_submodule(unsigned char result[20], const char *path, const unsigned c
 		    const unsigned char a[20], const unsigned char b[20], int search);
 int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name,
 		struct string_list *needs_pushing);
+int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);
 
 #endif
diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index 30bec4b..1947c28 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -119,4 +119,98 @@ test_expect_success 'push succeeds if submodule has no remote and is on the firs
 	)
 '
 
+test_expect_success 'push unpushed submodules when not needed' '
+	(
+		cd work &&
+		(
+			cd gar/bage &&
+			git checkout master &&
+			>junk5 &&
+			git add junk5 &&
+			git commit -m "Fifth junk" &&
+			git push &&
+			git rev-parse origin/master >../../../expected
+		) &&
+		git checkout master &&
+		git add gar/bage &&
+		git commit -m "Fifth commit for gar/bage" &&
+		git push --recurse-submodules=on-demand ../pub.git master
+	) &&
+	(
+		cd submodule.git &&
+		git rev-parse master >../actual
+	) &&
+	test_cmp expected actual
+'
+
+test_expect_success 'push unpushed submodules when not needed 2' '
+	(
+		cd submodule.git &&
+		git rev-parse master >../expected
+	) &&
+	(
+		cd work &&
+		(
+			cd gar/bage &&
+			>junk6 &&
+			git add junk6 &&
+			git commit -m "Sixth junk"
+		) &&
+		>junk2 &&
+		git add junk2 &&
+		git commit -m "Second junk for work" &&
+		git push --recurse-submodules=on-demand ../pub.git master
+	) &&
+	(
+		cd submodule.git &&
+		git rev-parse master >../actual
+	) &&
+	test_cmp expected actual
+'
+
+test_expect_success 'push unpushed submodules recursively' '
+	(
+		cd work &&
+		(
+			cd gar/bage &&
+			git checkout master &&
+			> junk7 &&
+			git add junk7 &&
+			git commit -m "Seventh junk" &&
+			git rev-parse master >../../../expected
+		) &&
+		git checkout master &&
+		git add gar/bage &&
+		git commit -m "Seventh commit for gar/bage" &&
+		git push --recurse-submodules=on-demand ../pub.git master
+	) &&
+	(
+		cd submodule.git &&
+		git rev-parse master >../actual
+	) &&
+	test_cmp expected actual
+'
+
+test_expect_success 'push unpushable submodule recursively fails' '
+	(
+		cd work &&
+		(
+			cd gar/bage &&
+			git rev-parse origin/master >../../../expected &&
+			git checkout master~0 &&
+			> junk8 &&
+			git add junk8 &&
+			git commit -m "Eighth junk"
+		) &&
+		git add gar/bage &&
+		git commit -m "Eighth commit for gar/bage" &&
+		test_must_fail git push --recurse-submodules=on-demand ../pub.git master
+	) &&
+	(
+		cd submodule.git &&
+		git rev-parse master >../actual
+	) &&
+	test_cmp expected actual
+'
+
 test_done
diff --git a/transport.c b/transport.c
index d13bd4a..8c0fec0 100644
--- a/transport.c
+++ b/transport.c
@@ -1009,6 +1009,11 @@ static void die_with_unpushed_submodules(struct string_list *needs_pushing)
 			"not be found on any remote:\n");
 	for (i = 0; i < needs_pushing->nr; i++)
 		printf("  %s\n", needs_pushing->items[i].string);
+	fprintf(stderr, "\nPlease try\n\n"
+			"	git push --recurse-submodules=on-demand\n\n"
+			"or cd to the path and use\n\n"
+			"	git push\n\n"
+			"to push them to a remote.\n\n");
 
 	string_list_clear(needs_pushing, 0);
 
@@ -1053,7 +1058,17 @@ int transport_push(struct transport *transport,
 			flags & TRANSPORT_PUSH_MIRROR,
 			flags & TRANSPORT_PUSH_FORCE);
 
-		if ((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) && !is_bare_repository()) {
+		if ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) && !is_bare_repository()) {
+			struct ref *ref = remote_refs;
+			for (; ref; ref = ref->next)
+				if (!is_null_sha1(ref->new_sha1) &&
+				    !push_unpushed_submodules(ref->new_sha1,
+					    transport->remote->name))
+				    die ("Failed to push all needed submodules!");
+		}
+
+		if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
+			      TRANSPORT_RECURSE_SUBMODULES_CHECK)) && !is_bare_repository()) {
 			struct ref *ref = remote_refs;
 			struct string_list needs_pushing;
 
diff --git a/transport.h b/transport.h
index 059b330..9d19c78 100644
--- a/transport.h
+++ b/transport.h
@@ -102,6 +102,7 @@ struct transport {
 #define TRANSPORT_PUSH_PORCELAIN 16
 #define TRANSPORT_PUSH_SET_UPSTREAM 32
 #define TRANSPORT_RECURSE_SUBMODULES_CHECK 64
+#define TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND 128
 
 #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
 
-- 
1.7.9.114.gead08

Re: [PATCH v5 1/3] Teach revision walking machinery to walk multiple times sequencially

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> Previously it was not possible to iterate revisions twice using the
> revision walking api. We add a reset_revision_walk() which clears the
> used flags. This allows us to do multiple sequencial revision walks.
>
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>

I am kind of surprised that this is already its 5th round.

> diff --git a/object.c b/object.c
> index 6b06297..6291ce9 100644
> --- a/object.c
> +++ b/object.c
> @@ -275,3 +275,14 @@ void object_array_remove_duplicates(struct object_array *array)
>  		array->nr = dst;
>  	}
>  }
> +
> +void clear_object_flags(unsigned flags)
> +{
> +	int i;
> +	struct object *obj;
> +
> +	for (i=0; i < obj_hash_size; i++) {
> +		if ((obj = obj_hash[i]) && obj->flags & flags)
> +			obj->flags &= ~flags;
> +	}
> +}

Minimally,

        void clear_object_flags(unsigned flags)
        {
                int i;

                for (i = 0; i < obj_hash_size; i++) {
                        struct object *obj = obj_hash[i];
                        if (obj && (obj->flags & flags))
                                obj->flags &= ~flags;
                }
        }

I am not sure if the "If there is any bit set we care about, drop them"
buys us anything, though.

> diff --git a/revision.c b/revision.c
> index c97d834..77ce6bd 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2061,6 +2061,11 @@ static void set_children(struct rev_info *revs)
>  	}
>  }
>  
> +void reset_revision_walk()

	void reset_revision_walk(void)

> +{
> +	clear_object_flags(SEEN | ADDED | SHOWN);
> +}

But is this really the right API?  After a particular program finishes
using the revision walker, wouldn't it want to clear both the set of these
standard flag bits used by the traversal machinery, as well as whatever
program specific bits it used to mark the objects with?

> diff --git a/revision.h b/revision.h
> index b8e9223..3535733 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -192,6 +192,7 @@ extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ct
>  ...
> +extern void reset_revision_walk();

Likewise, "extern void reset_revision_walk(void);".

> diff --git a/submodule.c b/submodule.c
> index 9a28060..645ff5d 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -404,6 +404,7 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remote
>  	while ((commit = get_revision(&rev)) && !needs_pushing)
>  		commit_need_pushing(commit, &needs_pushing);
>  
> +	reset_revision_walk();
>  	free(sha1_copy);
>  	strbuf_release(&remotes_arg);
>  
> @@ -741,6 +742,7 @@ static int find_first_merges(struct object_array *result, const char *path,
>  		if (in_merge_bases(b, &commit, 1))
>  			add_object_array(o, NULL, &merges);
>  	}
> +	reset_revision_walk();
>  
>  	/* Now we've got all merges that contain a and b. Prune all
>  	 * merges that contain another found merge and save them in

These two hunk look like a *BUGFIX* to me (certainly it does not look like
this is an addition of any new feature).

What bug does this fix, and how is the current submodule code broken
without this patch?  Can you describe the problem in the log message, and
add a test to demonstrate the existing breakage?

Re: [PATCH v5 2/3] Refactor submodule push check to use string list instead of integer

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> This allows us to tell the user which submodules have not been pushed.
> Additionally this is helpful when we want to automatically try to push
> submodules that have not been pushed.

Makes sense.

> diff --git a/submodule.c b/submodule.c
> index 645ff5d..3c714c2 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -357,21 +357,20 @@ static void collect_submodules_from_diff(struct diff_queue_struct *q,
>  					 void *data)
>  {
>  	int i;
> -	int *needs_pushing = data;
> +	struct string_list *needs_pushing = data;
>  
>  	for (i = 0; i < q->nr; i++) {
>  		struct diff_filepair *p = q->queue[i];
>  		if (!S_ISGITLINK(p->two->mode))
>  			continue;
>  		if (submodule_needs_pushing(p->two->path, p->two->sha1)) {
> -			*needs_pushing = 1;
> -			break;
> +			if (!string_list_has_string(needs_pushing, p->two->path))
> +				string_list_insert(needs_pushing, p->two->path);

Does string_list API have "look for this and insert if it doesn't exist
but otherwise don't do anything"?  Running get_entry_index() to answer
has_string() once and then calling it again to find where to insert to
respond to insert() looks a bit wasteful.

Just wondering.

>  		}
>  	}
>  }
>  
> -
> -static void commit_need_pushing(struct commit *commit, int *needs_pushing)
> +static void commit_need_pushing(struct commit *commit, struct string_list *needs_pushing)
>  {
>  	struct rev_info rev;
>  
> @@ -382,14 +381,15 @@ static void commit_need_pushing(struct commit *commit, int *needs_pushing)
>  	diff_tree_combined_merge(commit, 1, &rev);
>  }
>  
> -int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name)
> +int check_submodule_needs_pushing(unsigned char new_sha1[20],
> +		const char *remotes_name, struct string_list *needs_pushing)
>  {
>  	struct rev_info rev;
>  	struct commit *commit;
>  	const char *argv[] = {NULL, NULL, "--not", "NULL", NULL};
>  	int argc = ARRAY_SIZE(argv) - 1;
>  	char *sha1_copy;
> -	int needs_pushing = 0;
> +
>  	struct strbuf remotes_arg = STRBUF_INIT;
>  
>  	strbuf_addf(&remotes_arg, "--remotes=%s", remotes_name);
> @@ -401,14 +401,14 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remote
>  	if (prepare_revision_walk(&rev))
>  		die("revision walk setup failed");
>  
> -	while ((commit = get_revision(&rev)) && !needs_pushing)
> -		commit_need_pushing(commit, &needs_pushing);
> +	while ((commit = get_revision(&rev)) != NULL)
> +		commit_need_pushing(commit, needs_pushing);

Now the helper function to find list of submodules that need pushing given
one commit starting to look more and more misnamed.  It used to be "learn
if something needs pushing", but now it is "find what needs pushing".

Can somebody think of a good adjective to describe a submodule (or a set
of submodules) in this state, so that we can name this helper function
find_blue_submodules(), if the adjective were "blue"?

"Unpushed" submodule is the word used in the later part of the patch.

Re: [PATCH v5 3/3] push: teach --recurse-submodules the on-demand option

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> diff --git a/submodule.c b/submodule.c
> index 3c714c2..ff0cfd8 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -411,6 +411,54 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20],
>  	return needs_pushing->nr;
>  }
>  
> +static int push_submodule(const char *path)
> +{
> +	if (add_submodule_odb(path))
> +		return 1;
> +
> +	if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
> +		struct child_process cp;
> +		const char *argv[] = {"push", NULL};
> +
> +		memset(&cp, 0, sizeof(cp));
> +		cp.argv = argv;
> +		cp.env = local_repo_env;
> +		cp.git_cmd = 1;
> +		cp.no_stdin = 1;
> +		cp.dir = path;
> +		if (run_command(&cp))
> +			return 0;
> +		close(cp.out);
> +	}
> +
> +	return 1;
> +}

Hmm, this makes me wonder if we fire subprocesses and have them run in
parallel (to a reasonably limited parallelism), it might make the overall
user experience more pleasant, and if we did the same on the fetching
side, it would be even nicer.

We would need to keep track of children and after firing a handful of them
we would need to start waiting for some to finish and collect their exit
status before firing more, and at the end we would need to wait for the
remaining ones and find how each one of them did before returning from
push_unpushed_submodules().  If we were to do so, what are the missing
support we would need from the run_command() subsystem?

> +int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name)
> +{
> +	int i, ret = 1;
> +	struct string_list needs_pushing;
> +
> +	memset(&needs_pushing, 0, sizeof(struct string_list));
> +	needs_pushing.strdup_strings = 1;
> +
> +	if (!check_submodule_needs_pushing(new_sha1, remotes_name, &needs_pushing))
> +		return 1;
> +
> +	for (i = 0; i < needs_pushing.nr; i++) {
> +		const char *path = needs_pushing.items[i].string;
> +		fprintf(stderr, "Pushing submodule '%s'\n", path);
> +		if (!push_submodule(path)) {
> +			fprintf(stderr, "Unable to push submodule '%s'\n", path);
> +			ret = 0;
> +		}
> +	}

Re: [PATCH v5 3/3] push: teach --recurse-submodules the on-demand option

[Jens Lehmann]

Am 14.02.2012 04:34, schrieb Junio C Hamano:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
> 
>> diff --git a/submodule.c b/submodule.c
>> index 3c714c2..ff0cfd8 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -411,6 +411,54 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20],
>>  	return needs_pushing->nr;
>>  }
>>  
>> +static int push_submodule(const char *path)
>> +{
>> +	if (add_submodule_odb(path))
>> +		return 1;
>> +
>> +	if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
>> +		struct child_process cp;
>> +		const char *argv[] = {"push", NULL};
>> +
>> +		memset(&cp, 0, sizeof(cp));
>> +		cp.argv = argv;
>> +		cp.env = local_repo_env;
>> +		cp.git_cmd = 1;
>> +		cp.no_stdin = 1;
>> +		cp.dir = path;
>> +		if (run_command(&cp))
>> +			return 0;
>> +		close(cp.out);
>> +	}
>> +
>> +	return 1;
>> +}
> 
> Hmm, this makes me wonder if we fire subprocesses and have them run in
> parallel (to a reasonably limited parallelism), it might make the overall
> user experience more pleasant, and if we did the same on the fetching
> side, it would be even nicer.

Yeah, I had the same idea and did some experiments when working on
fetch some time ago.

> We would need to keep track of children and after firing a handful of them
> we would need to start waiting for some to finish and collect their exit
> status before firing more, and at the end we would need to wait for the
> remaining ones and find how each one of them did before returning from
> push_unpushed_submodules().  If we were to do so, what are the missing
> support we would need from the run_command() subsystem?

We would not only have to collect the exit status but also the output
lines. You don't want to see the output of multiple fetches or pushes
mixed together, so it makes sense to just defer that until the command
exited and then print everything at once. The interesting part I couldn't
come up with an easy solution for is to preserve the output order between
the stdout and stdin lines, as they contain different parts of the
progress which would look strange when shuffled around.

And I saw that sometimes parallel fetches took way longer than doing them
sequentially (in my case because of strange DNS behavior of my DSL router),
so we would definitely want a config option for that (maybe setting the
maximum number of simultaneous threads to be used).

But don't get me wrong, I'm all for having that feature! :-)

Re: Re: [PATCH v5 1/3] Teach revision walking machinery to walk multiple times sequencially

[Heiko Voigt]

Hi,

sorry that my reply to this took quite some time.

On Mon, Feb 13, 2012 at 05:33:34PM -0800, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
> 
> > Previously it was not possible to iterate revisions twice using the
> > revision walking api. We add a reset_revision_walk() which clears the
> > used flags. This allows us to do multiple sequencial revision walks.
> >
> > Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> 
> I am kind of surprised that this is already its 5th round.

Well, not this patch in particular but the series so I chose to it for
the whole series.

> Minimally,
> 
>         void clear_object_flags(unsigned flags)
>         {
>                 int i;
> 
>                 for (i = 0; i < obj_hash_size; i++) {
>                         struct object *obj = obj_hash[i];
>                         if (obj && (obj->flags & flags))
>                                 obj->flags &= ~flags;
>                 }
>         }
> 
> I am not sure if the "If there is any bit set we care about, drop them"
> buys us anything, though.

Ok done. And also dropped the "if there is any bit set" condition.

> > +void reset_revision_walk()
> 
> 	void reset_revision_walk(void)

Done.

> > +{
> > +	clear_object_flags(SEEN | ADDED | SHOWN);
> > +}
> 
> But is this really the right API?  After a particular program finishes
> using the revision walker, wouldn't it want to clear both the set of these
> standard flag bits used by the traversal machinery, as well as whatever
> program specific bits it used to mark the objects with?

Well if a program uses extra flags on objects it should clear the flags
it set by using the clear_objects_flags() function itself. For example if
the program wants to reuse those extra flags in a second revision walk
it would not be possible if reset_revision_walk() would clear all flags.

> > diff --git a/revision.h b/revision.h
> > index b8e9223..3535733 100644
> > --- a/revision.h
> > +++ b/revision.h
> > @@ -192,6 +192,7 @@ extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ct
> >  ...
> > +extern void reset_revision_walk();
> 
> Likewise, "extern void reset_revision_walk(void);".

Done.

> 
> > diff --git a/submodule.c b/submodule.c
> > index 9a28060..645ff5d 100644
> > --- a/submodule.c
> > +++ b/submodule.c
> > @@ -404,6 +404,7 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remote
> >  	while ((commit = get_revision(&rev)) && !needs_pushing)
> >  		commit_need_pushing(commit, &needs_pushing);
> >  
> > +	reset_revision_walk();
> >  	free(sha1_copy);
> >  	strbuf_release(&remotes_arg);
> >  
> > @@ -741,6 +742,7 @@ static int find_first_merges(struct object_array *result, const char *path,
> >  		if (in_merge_bases(b, &commit, 1))
> >  			add_object_array(o, NULL, &merges);
> >  	}
> > +	reset_revision_walk();
> >  
> >  	/* Now we've got all merges that contain a and b. Prune all
> >  	 * merges that contain another found merge and save them in
> 
> These two hunk look like a *BUGFIX* to me (certainly it does not look like
> this is an addition of any new feature).
> 
> What bug does this fix, and how is the current submodule code broken
> without this patch?  Can you describe the problem in the log message, and
> add a test to demonstrate the existing breakage?

There is no breakage I know of. Its rather a cleanup which allows to
call these functions multiple times. I did this to avoid surprises.
Should I put these in a separate patch or just add an explanation into
the current one?

Cheers Heiko

Re: [PATCH v5 2/3] Refactor submodule push check to use string list instead of integer

[Heiko Voigt]

On Mon, Feb 13, 2012 at 07:28:24PM -0800, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
> > diff --git a/submodule.c b/submodule.c
> > index 645ff5d..3c714c2 100644
> > --- a/submodule.c
> > +++ b/submodule.c
> > @@ -357,21 +357,20 @@ static void collect_submodules_from_diff(struct diff_queue_struct *q,
> >  					 void *data)
> >  {
> >  	int i;
> > -	int *needs_pushing = data;
> > +	struct string_list *needs_pushing = data;
> >  
> >  	for (i = 0; i < q->nr; i++) {
> >  		struct diff_filepair *p = q->queue[i];
> >  		if (!S_ISGITLINK(p->two->mode))
> >  			continue;
> >  		if (submodule_needs_pushing(p->two->path, p->two->sha1)) {
> > -			*needs_pushing = 1;
> > -			break;
> > +			if (!string_list_has_string(needs_pushing, p->two->path))
> > +				string_list_insert(needs_pushing, p->two->path);
> 
> Does string_list API have "look for this and insert if it doesn't exist
> but otherwise don't do anything"?  Running get_entry_index() to answer
> has_string() once and then calling it again to find where to insert to
> respond to insert() looks a bit wasteful.
> 
> Just wondering.

If I see correctly currently it has no such functionality. I can have a
look at adding another patch implementing this.


> > -
> > -static void commit_need_pushing(struct commit *commit, int *needs_pushing)
> > +static void commit_need_pushing(struct commit *commit, struct string_list *needs_pushing)
> >  {
> >  	struct rev_info rev;
> >  
> > @@ -382,14 +381,15 @@ static void commit_need_pushing(struct commit *commit, int *needs_pushing)
> >  	diff_tree_combined_merge(commit, 1, &rev);
> >  }
> >  
> > -int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name)
> > +int check_submodule_needs_pushing(unsigned char new_sha1[20],
> > +		const char *remotes_name, struct string_list *needs_pushing)
> >  {
> >  	struct rev_info rev;
> >  	struct commit *commit;
> >  	const char *argv[] = {NULL, NULL, "--not", "NULL", NULL};
> >  	int argc = ARRAY_SIZE(argv) - 1;
> >  	char *sha1_copy;
> > -	int needs_pushing = 0;
> > +
> >  	struct strbuf remotes_arg = STRBUF_INIT;
> >  
> >  	strbuf_addf(&remotes_arg, "--remotes=%s", remotes_name);
> > @@ -401,14 +401,14 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remote
> >  	if (prepare_revision_walk(&rev))
> >  		die("revision walk setup failed");
> >  
> > -	while ((commit = get_revision(&rev)) && !needs_pushing)
> > -		commit_need_pushing(commit, &needs_pushing);
> > +	while ((commit = get_revision(&rev)) != NULL)
> > +		commit_need_pushing(commit, needs_pushing);
> 
> Now the helper function to find list of submodules that need pushing given
> one commit starting to look more and more misnamed.  It used to be "learn
> if something needs pushing", but now it is "find what needs pushing".
> 
> Can somebody think of a good adjective to describe a submodule (or a set
> of submodules) in this state, so that we can name this helper function
> find_blue_submodules(), if the adjective were "blue"?
> 
> "Unpushed" submodule is the word used in the later part of the patch.

So why not use unpushed as the adjective? find_unpushed_submodules()
sound good to me. Will change the patch accordingly. Since we are
already discussing renaming should I also rename commit_need_pushing()
to find_unpushed_submodule_commits()? I think that would make the whole
naming more consistent.

Cheers Heiko

Re: [PATCH v5 3/3] push: teach --recurse-submodules the on-demand option

[Heiko Voigt]

On Wed, Feb 15, 2012 at 11:28:02PM +0100, Jens Lehmann wrote:
> Am 14.02.2012 04:34, schrieb Junio C Hamano:
> > Heiko Voigt <hvoigt@hvoigt.net> writes:
> > 
> >> diff --git a/submodule.c b/submodule.c
> >> index 3c714c2..ff0cfd8 100644
> >> --- a/submodule.c
> >> +++ b/submodule.c
> >> @@ -411,6 +411,54 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20],
> >>  	return needs_pushing->nr;
> >>  }
> >>  
> >> +static int push_submodule(const char *path)
> >> +{
> >> +	if (add_submodule_odb(path))
> >> +		return 1;
> >> +
> >> +	if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
> >> +		struct child_process cp;
> >> +		const char *argv[] = {"push", NULL};
> >> +
> >> +		memset(&cp, 0, sizeof(cp));
> >> +		cp.argv = argv;
> >> +		cp.env = local_repo_env;
> >> +		cp.git_cmd = 1;
> >> +		cp.no_stdin = 1;
> >> +		cp.dir = path;
> >> +		if (run_command(&cp))
> >> +			return 0;
> >> +		close(cp.out);
> >> +	}
> >> +
> >> +	return 1;
> >> +}
> > 
> > Hmm, this makes me wonder if we fire subprocesses and have them run in
> > parallel (to a reasonably limited parallelism), it might make the overall
> > user experience more pleasant, and if we did the same on the fetching
> > side, it would be even nicer.
> 
> Yeah, I had the same idea and did some experiments when working on
> fetch some time ago.
> 
> > We would need to keep track of children and after firing a handful of them
> > we would need to start waiting for some to finish and collect their exit
> > status before firing more, and at the end we would need to wait for the
> > remaining ones and find how each one of them did before returning from
> > push_unpushed_submodules().  If we were to do so, what are the missing
> > support we would need from the run_command() subsystem?
> 
> We would not only have to collect the exit status but also the output
> lines. You don't want to see the output of multiple fetches or pushes
> mixed together, so it makes sense to just defer that until the command
> exited and then print everything at once. The interesting part I couldn't
> come up with an easy solution for is to preserve the output order between
> the stdout and stdin lines, as they contain different parts of the
> progress which would look strange when shuffled around.
> 
> And I saw that sometimes parallel fetches took way longer than doing them
> sequentially (in my case because of strange DNS behavior of my DSL router),
> so we would definitely want a config option for that (maybe setting the
> maximum number of simultaneous threads to be used).
> 
> But don't get me wrong, I'm all for having that feature! :-)

Me too, but I would suggest that we first implement the basic recursive
push functionality and then start to do optimizations like this. I think
we will definitely be interested in parallel pushing once we start using it.

Cheers Heiko

Re: [PATCH v5 2/3] Refactor submodule push check to use string list instead of integer

[Heiko Voigt]

On Mon, Mar 26, 2012 at 09:33:17PM +0200, Heiko Voigt wrote:
> On Mon, Feb 13, 2012 at 07:28:24PM -0800, Junio C Hamano wrote:
> > Heiko Voigt <hvoigt@hvoigt.net> writes:
> > > -			*needs_pushing = 1;
> > > -			break;
> > > +			if (!string_list_has_string(needs_pushing, p->two->path))
> > > +				string_list_insert(needs_pushing, p->two->path);
> > 
> > Does string_list API have "look for this and insert if it doesn't exist
> > but otherwise don't do anything"?  Running get_entry_index() to answer
> > has_string() once and then calling it again to find where to insert to
> > respond to insert() looks a bit wasteful.
> > 
> > Just wondering.
> 
> If I see correctly currently it has no such functionality. I can have a
> look at adding another patch implementing this.

It seems my guess was wrong. As far as I read in the code
string_list_insert() already skips inserting existing strings and just
returns the existing items in the list. So it should be fine to remove
the string_list_has_string() call.

Cheers Heiko

Re: [PATCH v5 3/3] push: teach --recurse-submodules the on-demand option

[Zbigniew Jędrzejewski-Szmek]

On 02/13/2012 10:30 AM, Heiko Voigt wrote:
> When using this option git will search for all submodules that
> have changed in the revisions to be send. It will then try to
> push the currently checked out branch of each submodule.
>
> This helps when a user has finished working on a change which
> involves submodules and just wants to push everything in one go.

> ---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.
> +--recurse-submodules=<check|on-demand>::
> +	Make sure all submodule commits used by the revisions to be
> +	pushed are available on a remote tracking branch. If check is
> +	used it will be checked that all submodule commits that changed
> +	in the revisions to be pushed are available on a remote.
> +	Otherwise the push will be aborted and exit with non-zero
> +	status. If on-demand is used all submodules that changed in the
> +	revisions to be pushed will be pushed. If on-demand was not able
> +	to push all necessary revisions it will also be aborted and exit
> +	with non-zero status.
Hi,
this desciption seems awkward. Not sure how to improve it, but:
- the argument 'check' is changed to '<check|on-demand>', i.e. brackets 
are added. This changes the meaning, because brackets are used around a 
name for a value provided by the user. So here brackets shouldn't be 
used, because 'check' and 'on-demand' are literals.
- s/if check is used it will be checked/if check is used git will verify 
that/
- s/a remote/the remote/
- s/Otherwise the push/If any commits are missing the push/ (because 
'Otherwise' could refer to 'If check is used'.)

Zbyszek

Re: [PATCH v5 1/3] Teach revision walking machinery to walk multiple times sequencially

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

>> > +{
>> > +	clear_object_flags(SEEN | ADDED | SHOWN);
>> > +}
>> 
>> But is this really the right API?  After a particular program finishes
>> using the revision walker, wouldn't it want to clear both the set of these
>> standard flag bits used by the traversal machinery, as well as whatever
>> program specific bits it used to mark the objects with?
>
> Well if a program uses extra flags on objects it should clear the flags
> it set by using the clear_objects_flags() function itself. For example if
> the program wants to reuse those extra flags in a second revision walk
> it would not be possible if reset_revision_walk() would clear all flags.

OK.

>> These two hunk look like a *BUGFIX* to me (certainly it does not look like
>> this is an addition of any new feature).
>> 
>> What bug does this fix, and how is the current submodule code broken
>> without this patch?  Can you describe the problem in the log message, and
>> add a test to demonstrate the existing breakage?
>
> There is no breakage I know of. Its rather a cleanup which allows to
> call these functions multiple times. I did this to avoid surprises.

OK.

Thanks.

Re: [PATCH v5 2/3] Refactor submodule push check to use string list instead of integer

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> It seems my guess was wrong. As far as I read in the code
> string_list_insert() already skips inserting existing strings and just
> returns the existing items in the list. So it should be fine to remove
> the string_list_has_string() call.

Sounds sane.  Thanks.

Re: [PATCH v5 3/3] push: teach --recurse-submodules the on-demand option

[Heiko Voigt]

On Mon, Mar 26, 2012 at 11:22:49PM +0200, Zbigniew J??drzejewski-Szmek wrote:
> On 02/13/2012 10:30 AM, Heiko Voigt wrote:
>> When using this option git will search for all submodules that
>> have changed in the revisions to be send. It will then try to
>> push the currently checked out branch of each submodule.
>>
>> This helps when a user has finished working on a change which
>> involves submodules and just wants to push everything in one go.
>
>> ---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.
>> +--recurse-submodules=<check|on-demand>::
>> +	Make sure all submodule commits used by the revisions to be
>> +	pushed are available on a remote tracking branch. If check is
>> +	used it will be checked that all submodule commits that changed
>> +	in the revisions to be pushed are available on a remote.
>> +	Otherwise the push will be aborted and exit with non-zero
>> +	status. If on-demand is used all submodules that changed in the
>> +	revisions to be pushed will be pushed. If on-demand was not able
>> +	to push all necessary revisions it will also be aborted and exit
>> +	with non-zero status.
> Hi,
> this desciption seems awkward. Not sure how to improve it, but:
> - the argument 'check' is changed to '<check|on-demand>', i.e. brackets  
> are added. This changes the meaning, because brackets are used around a  
> name for a value provided by the user. So here brackets shouldn't be  
> used, because 'check' and 'on-demand' are literals.
> - s/if check is used it will be checked/if check is used git will verify  
> that/
> - s/a remote/the remote/
> - s/Otherwise the push/If any commits are missing the push/ (because  
> 'Otherwise' could refer to 'If check is used'.)

I agree on all of these except

	s/a remote/the remote/

which I changed to

	s/a remote/at least one remote of the submodule/

since it really just checks whether the commits are on some remote. It
it not strictly the one defined in .gitmodules. Doing it this way allows
to work with forks without the need to change .gitmodules.

Cheers Heiko

[RFC/PATCH] read from 2 filedescriptors simultaneously into one strbuf

[Heiko Voigt]

Hi,

On Wed, Feb 15, 2012 at 11:28:02PM +0100, Jens Lehmann wrote:
> Am 14.02.2012 04:34, schrieb Junio C Hamano:
> > Hmm, this makes me wonder if we fire subprocesses and have them run in
> > parallel (to a reasonably limited parallelism), it might make the overall
> > user experience more pleasant, and if we did the same on the fetching
> > side, it would be even nicer.
> 
> Yeah, I had the same idea and did some experiments when working on
> fetch some time ago.
> 
> > We would need to keep track of children and after firing a handful of them
> > we would need to start waiting for some to finish and collect their exit
> > status before firing more, and at the end we would need to wait for the
> > remaining ones and find how each one of them did before returning from
> > push_unpushed_submodules().  If we were to do so, what are the missing
> > support we would need from the run_command() subsystem?
> 
> We would not only have to collect the exit status but also the output
> lines. You don't want to see the output of multiple fetches or pushes
> mixed together, so it makes sense to just defer that until the command
> exited and then print everything at once. The interesting part I couldn't
> come up with an easy solution for is to preserve the output order between
> the stdout and stdin lines, as they contain different parts of the
> progress which would look strange when shuffled around.

I had a go at a possible implementation for deferred output of parallel
subprocesses today. The idea is to use two concurrent threads
simultaneously reading from the two filedescriptors and writing the
result into one strbuf. Below is the diff for the current code I have[1].

One current limitation is that it only works with pthreads enabled. I
first tried to use the same pipe for the two threads to avoid the need
for locking and shared memory. I am not sure but it seems that
concurrents writes into the same pipe are not allowed.

What do you think of this approach? Is it the correct place or should it
go into another module? Also the function naming might need some polishing.

Cheers Heiko

P.S.: We can of course get rid of the second thread and just read in the
main thread for one of the filedescriptors but this was easier while
thinking about it.

[1]
diff --git a/run-command.c b/run-command.c
index 2a1041e..48c9e07 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1,6 +1,8 @@
 #include "cache.h"
 #include "run-command.h"
 #include "exec_cmd.h"
+#include "strbuf.h"
+#include <pthread.h>
 
 static inline void close_pair(int fd[2])
 {
@@ -407,6 +409,60 @@ fail_pipe:
 	return 0;
 }
 
+#ifndef NO_PTHREADS
+struct fd2fd_data {
+	pthread_mutex_t mutex;
+	struct strbuf *output;
+};
+
+static int fd2fd(int in, int out, void *data)
+{
+	struct fd2fd_data *me = data;
+	struct strbuf line;
+
+	FILE *fin = xfdopen(in, "r");
+
+	while (strbuf_getwholeline(&line, fin, '\n') != EOF) {
+		pthread_mutex_lock(&me->mutex);
+		strbuf_addbuf(me->output, &line);
+		pthread_mutex_unlock(&me->mutex);
+	}
+
+	strbuf_release(&line);
+	return 0;
+}
+
+void read_2_fds_into_strbuf(int fd1, int fd2, struct strbuf *output)
+{
+	struct async err_async;
+	struct async out_async;
+	struct fd2fd_data async_data;
+
+	pthread_mutex_init(&async_data.mutex, NULL);
+	async_data.output = output;
+
+	/* start two threads to read fd1 and fd2 simultaneously
+	 * into one strbuf */
+	memset(&out_async, 0, sizeof(out_async));
+	out_async.proc = fd2fd;
+	out_async.data = &async_data;
+	out_async.in = fd1;
+
+	memset(&err_async, 0, sizeof(err_async));
+	err_async.proc = fd2fd;
+	err_async.data = &async_data;
+	err_async.in = fd2;
+
+	start_async(&out_async);
+	start_async(&err_async);
+
+	finish_async(&out_async);
+	finish_async(&err_async);
+
+	pthread_mutex_destroy(&async_data.mutex);
+}
+#endif /* NO_PTHREADS */
+
 int finish_command(struct child_process *cmd)
 {
 	return wait_or_whine(cmd->pid, cmd->argv[0], cmd->silent_exec_failure);
diff --git a/run-command.h b/run-command.h
index 56491b9..a65debb 100644
--- a/run-command.h
+++ b/run-command.h
@@ -5,6 +5,8 @@
 #include <pthread.h>
 #endif
 
+struct strbuf;
+
 struct child_process {
 	const char **argv;
 	pid_t pid;
@@ -90,4 +92,8 @@ struct async {
 int start_async(struct async *async);
 int finish_async(struct async *async);
 
+#ifndef NO_PTHREADS
+void read_2_fds_into_strbuf(int fd1, int fd2, struct strbuf *output);
+#endif
+
 #endif
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 10b26e4..774807b 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -11,4 +11,10 @@ test_expect_success 'start_command reports ENOENT' '
 	test-run-command start-command-ENOENT ./does-not-exist
 '
 
+test_expect_success 'read_2_fds_into_strbuf basic behavior' '
+	test-run-command out2strbuf >actual &&
+	grep "^Hallo Stdout$" actual &&
+	grep "^Hallo Stderr$" actual
+'
+
 test_done
diff --git a/test-run-command.c b/test-run-command.c
index 0612bfa..f59485d 100644
--- a/test-run-command.c
+++ b/test-run-command.c
@@ -10,26 +10,72 @@
 
 #include "git-compat-util.h"
 #include "run-command.h"
+#include "strbuf.h"
 #include <string.h>
 #include <errno.h>
 
+static int run_write_async(int in, int out, void *data)
+{
+	const char *msg = data;
+
+	FILE *fout = xfdopen(out, "w");
+	fprintf(fout, "%s\n", msg);
+
+	fclose(fout);
+	return 0;
+}
+
 int main(int argc, char **argv)
 {
 	struct child_process proc;
+	struct async write_out;
+	struct async write_err;
 
 	memset(&proc, 0, sizeof(proc));
 
-	if (argc < 3)
+	if (argc < 2)
 		return 1;
-	proc.argv = (const char **)argv+2;
 
 	if (!strcmp(argv[1], "start-command-ENOENT")) {
+		if (argc < 3)
+			return 1;
+		proc.argv = (const char **)argv+2;
+
 		if (start_command(&proc) < 0 && errno == ENOENT)
 			return 0;
 		fprintf(stderr, "FAIL %s\n", argv[1]);
 		return 1;
 	}
 
+	if (!strcmp(argv[1], "out2strbuf")) {
+#ifndef NO_PTHREADS
+		struct strbuf output = STRBUF_INIT;
+
+		memset(&write_out, 0, sizeof(write_out));
+		write_out.data = "Hallo Stdout";
+		write_out.proc = run_write_async;
+		write_out.out = -1;
+
+		memset(&write_err, 0, sizeof(write_err));
+		write_err.data = "Hallo Stderr";
+		write_err.proc = run_write_async;
+		write_err.out = -1;
+
+		start_async(&write_out);
+		start_async(&write_err);
+
+		read_2_fds_into_strbuf(write_out.out, write_err.out, &output);
+
+		finish_async(&write_out);
+		finish_async(&write_err);
+
+		printf("%s", output.buf);
+		strbuf_release(&output);
+#endif /* NO_PTHREADS */
+
+		return 0;
+	}
+
 	fprintf(stderr, "check usage\n");
 	return 1;
 }