{"thread":{"id":"29611","subject":"[PATCH v5 0/3] push: submodule support","startedAt":"2012-02-13T09:25:41Z","lastAt":"2012-05-13T14:47:34Z","messageCount":17,"participants":["Heiko Voigt","Junio C Hamano","Jens Lehmann","Zbigniew Jędrzejewski-Szmek"],"isPatch":true,"patchVersion":5,"patchTotal":3},"messages":[{"id":"184556","messageId":"20120213092541.GA15585@t1405.greatnet.de","threadId":"29611","inReplyTo":null,"subject":"[PATCH v5 0/3] push: submodule support","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-02-13T09:25:41Z","receivedAt":"2012-02-13T09:25:41Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"I have rewritten and cleaned up the recursive submodule on-demand push\npatch from Fredrik. Since this involved changing large parts of it I\nhave taken over the ownership. To pay credit that this is built upon\nFredriks work I have left the signed-off and mentored-by footers the\nsame. I hope that this is the proper way to handle such cases. If\nsomeone comes up with a better idea I am happy to change things.\n\nCheers Heiko\n\nThe first iteration of this patch series can be found here:\nhttp://thread.gmane.org/gmane.comp.version-control.git/176328/focus=176327\n\nThe second iteration of this patch series can be found here:\nhttp://thread.gmane.org/gmane.comp.version-control.git/177992\n\nThe third iteration of this patch series can be found here:\nhttp://thread.gmane.org/gmane.comp.version-control.git/179037/focus=179048\n\nThe fourth iteration of this patch series can be found here:\nhttp://thread.gmane.org/gmane.comp.version-control.git/179731\n\nHeiko Voigt (3):\n  Teach revision walking machinery to walk multiple times sequencially\n  Refactor submodule push check to use string list instead of integer\n  push: teach --recurse-submodules the on-demand option\n\n .gitignore                                       |    1 +\n Documentation/git-push.txt                       |   14 +++-\n Documentation/technical/api-revision-walking.txt |    5 +\n Makefile                                         |    1 +\n builtin/push.c                                   |    7 ++\n object.c                                         |   11 +++\n object.h                                         |    2 +\n revision.c                                       |    5 +\n revision.h                                       |    1 +\n submodule.c                                      |   70 ++++++++++++++---\n submodule.h                                      |    4 +-\n t/t0062-revision-walking.sh                      |   33 ++++++++\n t/t5531-deep-submodule-push.sh                   |   94 ++++++++++++++++++++++\n test-revision-walking.c                          |   66 +++++++++++++++\n transport.c                                      |   41 +++++++++-\n transport.h                                      |    1 +\n 16 files changed, 338 insertions(+), 18 deletions(-)\n create mode 100755 t/t0062-revision-walking.sh\n create mode 100644 test-revision-walking.c\n\n-- \n1.7.9.114.gead08\n"},{"id":"184557","messageId":"20120213092730.GB15585@t1405.greatnet.de","threadId":"29611","inReplyTo":"20120213092541.GA15585@t1405.greatnet.de","subject":"[PATCH v5 1/3] Teach revision walking machinery to walk multiple times sequencially","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-02-13T09:27:30Z","receivedAt":"2012-02-13T09:27:30Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Previously it was not possible to iterate revisions twice using the\nrevision walking api. We add a reset_revision_walk() which clears the\nused flags. This allows us to do multiple sequencial revision walks.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\n .gitignore                                       |    1 +\n Documentation/technical/api-revision-walking.txt |    5 ++\n Makefile                                         |    1 +\n object.c                                         |   11 ++++\n object.h                                         |    2 +\n revision.c                                       |    5 ++\n revision.h                                       |    1 +\n submodule.c                                      |    2 +\n t/t0062-revision-walking.sh                      |   33 +++++++++++\n test-revision-walking.c                          |   66 ++++++++++++++++++++++\n 10 files changed, 127 insertions(+), 0 deletions(-)\n create mode 100755 t/t0062-revision-walking.sh\n create mode 100644 test-revision-walking.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 3b7680e..2f09842 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -184,6 +184,7 @@\n /test-obj-pool\n /test-parse-options\n /test-path-utils\n+/test-revision-walking\n /test-run-command\n /test-sha1\n /test-sigchain\ndiff --git a/Documentation/technical/api-revision-walking.txt b/Documentation/technical/api-revision-walking.txt\nindex 996da05..b7d0d9a 100644\n--- a/Documentation/technical/api-revision-walking.txt\n+++ b/Documentation/technical/api-revision-walking.txt\n@@ -56,6 +56,11 @@ function.\n \treturning a `struct commit *` each time you call it. The end of the\n \trevision list is indicated by returning a NULL pointer.\n \n+`reset_revision_walk`::\n+\n+\tReset the flags used by the revision walking api. You can use\n+\tthis to do multiple sequencial revision walks.\n+\n Data structures\n ---------------\n \ndiff --git a/Makefile b/Makefile\nindex c457c34..bff686f 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -472,6 +472,7 @@ TEST_PROGRAMS_NEED_X += test-mktemp\n TEST_PROGRAMS_NEED_X += test-obj-pool\n TEST_PROGRAMS_NEED_X += test-parse-options\n TEST_PROGRAMS_NEED_X += test-path-utils\n+TEST_PROGRAMS_NEED_X += test-revision-walking\n TEST_PROGRAMS_NEED_X += test-run-command\n TEST_PROGRAMS_NEED_X += test-sha1\n TEST_PROGRAMS_NEED_X += test-sigchain\ndiff --git a/object.c b/object.c\nindex 6b06297..6291ce9 100644\n--- a/object.c\n+++ b/object.c\n@@ -275,3 +275,14 @@ void object_array_remove_duplicates(struct object_array *array)\n \t\tarray->nr = dst;\n \t}\n }\n+\n+void clear_object_flags(unsigned flags)\n+{\n+\tint i;\n+\tstruct object *obj;\n+\n+\tfor (i=0; i < obj_hash_size; i++) {\n+\t\tif ((obj = obj_hash[i]) && obj->flags & flags)\n+\t\t\tobj->flags &= ~flags;\n+\t}\n+}\ndiff --git a/object.h b/object.h\nindex b6618d9..6a97b6b 100644\n--- a/object.h\n+++ b/object.h\n@@ -76,4 +76,6 @@ void add_object_array(struct object *obj, const char *name, struct object_array\n void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode);\n void object_array_remove_duplicates(struct object_array *);\n \n+void clear_object_flags(unsigned flags);\n+\n #endif /* OBJECT_H */\ndiff --git a/revision.c b/revision.c\nindex c97d834..77ce6bd 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2061,6 +2061,11 @@ static void set_children(struct rev_info *revs)\n \t}\n }\n \n+void reset_revision_walk()\n+{\n+\tclear_object_flags(SEEN | ADDED | SHOWN);\n+}\n+\n int prepare_revision_walk(struct rev_info *revs)\n {\n \tint nr = revs->pending.nr;\ndiff --git a/revision.h b/revision.h\nindex b8e9223..3535733 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -192,6 +192,7 @@ extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ct\n \t\t\t\t const char * const usagestr[]);\n extern int handle_revision_arg(const char *arg, struct rev_info *revs,int flags,int cant_be_filename);\n \n+extern void reset_revision_walk();\n extern int prepare_revision_walk(struct rev_info *revs);\n extern struct commit *get_revision(struct rev_info *revs);\n extern char *get_revision_mark(const struct rev_info *revs, const struct commit *commit);\ndiff --git a/submodule.c b/submodule.c\nindex 9a28060..645ff5d 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -404,6 +404,7 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remote\n \twhile ((commit = get_revision(&rev)) && !needs_pushing)\n \t\tcommit_need_pushing(commit, &needs_pushing);\n \n+\treset_revision_walk();\n \tfree(sha1_copy);\n \tstrbuf_release(&remotes_arg);\n \n@@ -741,6 +742,7 @@ static int find_first_merges(struct object_array *result, const char *path,\n \t\tif (in_merge_bases(b, &commit, 1))\n \t\t\tadd_object_array(o, NULL, &merges);\n \t}\n+\treset_revision_walk();\n \n \t/* Now we've got all merges that contain a and b. Prune all\n \t * merges that contain another found merge and save them in\ndiff --git a/t/t0062-revision-walking.sh b/t/t0062-revision-walking.sh\nnew file mode 100755\nindex 0000000..3d98eb8\n--- /dev/null\n+++ b/t/t0062-revision-walking.sh\n@@ -0,0 +1,33 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2012 Heiko Voigt\n+#\n+\n+test_description='Test revision walking api'\n+\n+. ./test-lib.sh\n+\n+cat >run_twice_expected <<-EOF\n+1st\n+ > add b\n+ > add a\n+2nd\n+ > add b\n+ > add a\n+EOF\n+\n+test_expect_success 'setup' '\n+\techo a > a &&\n+\tgit add a &&\n+\tgit commit -m \"add a\" &&\n+\techo b > b &&\n+\tgit add b &&\n+\tgit commit -m \"add b\"\n+'\n+\n+test_expect_success 'revision walking can be done twice' '\n+\ttest-revision-walking run-twice > run_twice_actual\n+\ttest_cmp run_twice_expected run_twice_actual\n+'\n+\n+test_done\ndiff --git a/test-revision-walking.c b/test-revision-walking.c\nnew file mode 100644\nindex 0000000..27ad597\n--- /dev/null\n+++ b/test-revision-walking.c\n@@ -0,0 +1,66 @@\n+/*\n+ * test-revision-walking.c: test revision walking API.\n+ *\n+ * (C) 2012 Heiko Voigt <hvoigt@hvoigt.net>\n+ *\n+ * This code is free software; you can redistribute it and/or modify\n+ * it under the terms of the GNU General Public License version 2 as\n+ * published by the Free Software Foundation.\n+ */\n+\n+#include \"cache.h\"\n+#include \"commit.h\"\n+#include \"diff.h\"\n+#include \"revision.h\"\n+\n+static void print_commit(struct commit *commit)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct pretty_print_context ctx = {0};\n+\tctx.date_mode = DATE_NORMAL;\n+\tformat_commit_message(commit, \" %m %s\", &sb, &ctx);\n+\tprintf(\"%s\\n\", sb.buf);\n+\tstrbuf_release(&sb);\n+}\n+\n+static int run_revision_walk()\n+{\n+\tstruct rev_info rev;\n+\tstruct commit *commit;\n+\tconst char *argv[] = {NULL, \"--all\", NULL};\n+\tint argc = ARRAY_SIZE(argv) - 1;\n+\tint got_revision = 0;\n+\n+\tinit_revisions(&rev, NULL);\n+\tsetup_revisions(argc, argv, &rev, NULL);\n+\tif (prepare_revision_walk(&rev))\n+\t\tdie(\"revision walk setup failed\");\n+\n+\twhile ((commit = get_revision(&rev)) != NULL) {\n+\t\tprint_commit(commit);\n+\t\tgot_revision = 1;\n+\t}\n+\n+\treset_revision_walk();\n+\treturn got_revision;\n+}\n+\n+int main(int argc, char **argv)\n+{\n+\tif (argc < 2)\n+\t\treturn 1;\n+\n+\tif (!strcmp(argv[1], \"run-twice\")) {\n+\t\tprintf(\"1st\\n\");\n+\t\tif (!run_revision_walk())\n+\t\t\treturn 1;\n+\t\tprintf(\"2nd\\n\");\n+\t\tif (!run_revision_walk())\n+\t\t\treturn 1;\n+\n+\t\treturn 0;\n+\t}\n+\n+\tfprintf(stderr, \"check usage\\n\");\n+\treturn 1;\n+}\n-- \n1.7.9.114.gead08\n"},{"id":"184558","messageId":"20120213092900.GC15585@t1405.greatnet.de","threadId":"29611","inReplyTo":"20120213092541.GA15585@t1405.greatnet.de","subject":"[PATCH v5 2/3] Refactor submodule push check to use string list instead of integer","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-02-13T09:29:00Z","receivedAt":"2012-02-13T09:29:00Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"This allows us to tell the user which submodules have not been pushed.\nAdditionally this is helpful when we want to automatically try to push\nsubmodules that have not been pushed.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\n submodule.c |   20 ++++++++++----------\n submodule.h |    3 ++-\n transport.c |   24 ++++++++++++++++++++++--\n 3 files changed, 34 insertions(+), 13 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 645ff5d..3c714c2 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -357,21 +357,20 @@ static void collect_submodules_from_diff(struct diff_queue_struct *q,\n \t\t\t\t\t void *data)\n {\n \tint i;\n-\tint *needs_pushing = data;\n+\tstruct string_list *needs_pushing = data;\n \n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filepair *p = q->queue[i];\n \t\tif (!S_ISGITLINK(p->two->mode))\n \t\t\tcontinue;\n \t\tif (submodule_needs_pushing(p->two->path, p->two->sha1)) {\n-\t\t\t*needs_pushing = 1;\n-\t\t\tbreak;\n+\t\t\tif (!string_list_has_string(needs_pushing, p->two->path))\n+\t\t\t\tstring_list_insert(needs_pushing, p->two->path);\n \t\t}\n \t}\n }\n \n-\n-static void commit_need_pushing(struct commit *commit, int *needs_pushing)\n+static void commit_need_pushing(struct commit *commit, struct string_list *needs_pushing)\n {\n \tstruct rev_info rev;\n \n@@ -382,14 +381,15 @@ static void commit_need_pushing(struct commit *commit, int *needs_pushing)\n \tdiff_tree_combined_merge(commit, 1, &rev);\n }\n \n-int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name)\n+int check_submodule_needs_pushing(unsigned char new_sha1[20],\n+\t\tconst char *remotes_name, struct string_list *needs_pushing)\n {\n \tstruct rev_info rev;\n \tstruct commit *commit;\n \tconst char *argv[] = {NULL, NULL, \"--not\", \"NULL\", NULL};\n \tint argc = ARRAY_SIZE(argv) - 1;\n \tchar *sha1_copy;\n-\tint needs_pushing = 0;\n+\n \tstruct strbuf remotes_arg = STRBUF_INIT;\n \n \tstrbuf_addf(&remotes_arg, \"--remotes=%s\", remotes_name);\n@@ -401,14 +401,14 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remote\n \tif (prepare_revision_walk(&rev))\n \t\tdie(\"revision walk setup failed\");\n \n-\twhile ((commit = get_revision(&rev)) && !needs_pushing)\n-\t\tcommit_need_pushing(commit, &needs_pushing);\n+\twhile ((commit = get_revision(&rev)) != NULL)\n+\t\tcommit_need_pushing(commit, needs_pushing);\n \n \treset_revision_walk();\n \tfree(sha1_copy);\n \tstrbuf_release(&remotes_arg);\n \n-\treturn needs_pushing;\n+\treturn needs_pushing->nr;\n }\n \n static int is_submodule_commit_present(const char *path, unsigned char sha1[20])\ndiff --git a/submodule.h b/submodule.h\nindex 80e04f3..ddd1941 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -29,6 +29,7 @@ int fetch_populated_submodules(int num_options, const char **options,\n unsigned is_submodule_modified(const char *path, int ignore_untracked);\n int merge_submodule(unsigned char result[20], const char *path, const unsigned char base[20],\n \t\t    const unsigned char a[20], const unsigned char b[20], int search);\n-int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name);\n+int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name,\n+\t\tstruct string_list *needs_pushing);\n \n #endif\ndiff --git a/transport.c b/transport.c\nindex cac0c06..d13bd4a 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -11,6 +11,7 @@\n #include \"branch.h\"\n #include \"url.h\"\n #include \"submodule.h\"\n+#include \"string-list.h\"\n \n /* rsync support */\n \n@@ -1000,6 +1001,20 @@ void transport_set_verbosity(struct transport *transport, int verbosity,\n \ttransport->progress = force_progress || (verbosity >= 0 && isatty(2));\n }\n \n+static void die_with_unpushed_submodules(struct string_list *needs_pushing)\n+{\n+\tint i;\n+\n+\tfprintf(stderr, \"The following submodule paths contain changes that can\\n\"\n+\t\t\t\"not be found on any remote:\\n\");\n+\tfor (i = 0; i < needs_pushing->nr; i++)\n+\t\tprintf(\"  %s\\n\", needs_pushing->items[i].string);\n+\n+\tstring_list_clear(needs_pushing, 0);\n+\n+\tdie(\"Aborting.\");\n+}\n+\n int transport_push(struct transport *transport,\n \t\t   int refspec_nr, const char **refspec, int flags,\n \t\t   int *nonfastforward)\n@@ -1040,10 +1055,15 @@ int transport_push(struct transport *transport,\n \n \t\tif ((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) && !is_bare_repository()) {\n \t\t\tstruct ref *ref = remote_refs;\n+\t\t\tstruct string_list needs_pushing;\n+\n+\t\t\tmemset(&needs_pushing, 0, sizeof(struct string_list));\n+\t\t\tneeds_pushing.strdup_strings = 1;\n \t\t\tfor (; ref; ref = ref->next)\n \t\t\t\tif (!is_null_sha1(ref->new_sha1) &&\n-\t\t\t\t    check_submodule_needs_pushing(ref->new_sha1,transport->remote->name))\n-\t\t\t\t\tdie(\"There are unpushed submodules, aborting.\");\n+\t\t\t\t    check_submodule_needs_pushing(ref->new_sha1,\n+\t\t\t\t\t    transport->remote->name, &needs_pushing))\n+\t\t\t\t\tdie_with_unpushed_submodules(&needs_pushing);\n \t\t}\n \n \t\tpush_ret = transport->push_refs(transport, remote_refs, flags);\n-- \n1.7.9.114.gead08\n"},{"id":"184559","messageId":"20120213093008.GD15585@t1405.greatnet.de","threadId":"29611","inReplyTo":"20120213092541.GA15585@t1405.greatnet.de","subject":"[PATCH v5 3/3] push: teach --recurse-submodules the on-demand option","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-02-13T09:30:08Z","receivedAt":"2012-02-13T09:30:08Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"When using this option git will search for all submodules that\nhave changed in the revisions to be send. It will then try to\npush the currently checked out branch of each submodule.\n\nThis helps when a user has finished working on a change which\ninvolves submodules and just wants to push everything in one go.\n\nSigned-off-by: Fredrik Gustafsson <iveqy@iveqy.com>\nMentored-by: Jens Lehmann <Jens.Lehmann@web.de>\nMentored-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\n Documentation/git-push.txt     |   14 ++++--\n builtin/push.c                 |    7 +++\n submodule.c                    |   48 ++++++++++++++++++++\n submodule.h                    |    1 +\n t/t5531-deep-submodule-push.sh |   94 ++++++++++++++++++++++++++++++++++++++++\n transport.c                    |   17 +++++++-\n transport.h                    |    1 +\n 7 files changed, 177 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex aede488..649ee3a 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -162,10 +162,16 @@ useful if you write an alias or script around 'git push'.\n \tis specified. This flag forces progress status even if the\n \tstandard error stream is not directed to a terminal.\n \n---recurse-submodules=check::\n-\tCheck whether all submodule commits used by the revisions to be\n-\tpushed are available on a remote tracking branch. Otherwise the\n-\tpush will be aborted and the command will exit with non-zero status.\n+--recurse-submodules=<check|on-demand>::\n+\tMake sure all submodule commits used by the revisions to be\n+\tpushed are available on a remote tracking branch. If check is\n+\tused it will be checked that all submodule commits that changed\n+\tin the revisions to be pushed are available on a remote.\n+\tOtherwise the push will be aborted and exit with non-zero\n+\tstatus. If on-demand is used all submodules that changed in the\n+\trevisions to be pushed will be pushed. If on-demand was not able\n+\tto push all necessary revisions it will also be aborted and exit\n+\twith non-zero status.\n \n \n include::urls-remotes.txt[]\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 35cce53..f2ef8dd 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -224,9 +224,16 @@ static int option_parse_recurse_submodules(const struct option *opt,\n \t\t\t\t   const char *arg, int unset)\n {\n \tint *flags = opt->value;\n+\n+\tif (*flags & (TRANSPORT_RECURSE_SUBMODULES_CHECK |\n+\t\t      TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND))\n+\t\tdie(\"%s can only be used once.\", opt->long_name);\n+\n \tif (arg) {\n \t\tif (!strcmp(arg, \"check\"))\n \t\t\t*flags |= TRANSPORT_RECURSE_SUBMODULES_CHECK;\n+\t\telse if (!strcmp(arg, \"on-demand\"))\n+\t\t\t*flags |= TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND;\n \t\telse\n \t\t\tdie(\"bad %s argument: %s\", opt->long_name, arg);\n \t} else\ndiff --git a/submodule.c b/submodule.c\nindex 3c714c2..ff0cfd8 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -411,6 +411,54 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20],\n \treturn needs_pushing->nr;\n }\n \n+static int push_submodule(const char *path)\n+{\n+\tif (add_submodule_odb(path))\n+\t\treturn 1;\n+\n+\tif (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {\n+\t\tstruct child_process cp;\n+\t\tconst char *argv[] = {\"push\", NULL};\n+\n+\t\tmemset(&cp, 0, sizeof(cp));\n+\t\tcp.argv = argv;\n+\t\tcp.env = local_repo_env;\n+\t\tcp.git_cmd = 1;\n+\t\tcp.no_stdin = 1;\n+\t\tcp.dir = path;\n+\t\tif (run_command(&cp))\n+\t\t\treturn 0;\n+\t\tclose(cp.out);\n+\t}\n+\n+\treturn 1;\n+}\n+\n+int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name)\n+{\n+\tint i, ret = 1;\n+\tstruct string_list needs_pushing;\n+\n+\tmemset(&needs_pushing, 0, sizeof(struct string_list));\n+\tneeds_pushing.strdup_strings = 1;\n+\n+\tif (!check_submodule_needs_pushing(new_sha1, remotes_name, &needs_pushing))\n+\t\treturn 1;\n+\n+\tfor (i = 0; i < needs_pushing.nr; i++) {\n+\t\tconst char *path = needs_pushing.items[i].string;\n+\t\tfprintf(stderr, \"Pushing submodule '%s'\\n\", path);\n+\t\tif (!push_submodule(path)) {\n+\t\t\tfprintf(stderr, \"Unable to push submodule '%s'\\n\", path);\n+\t\t\tret = 0;\n+\t\t}\n+\t}\n+\n+\tstring_list_clear(&needs_pushing, 0);\n+\n+\treturn ret;\n+}\n+\n static int is_submodule_commit_present(const char *path, unsigned char sha1[20])\n {\n \tint is_present = 0;\ndiff --git a/submodule.h b/submodule.h\nindex ddd1941..af74941 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -31,5 +31,6 @@ int merge_submodule(unsigned char result[20], const char *path, const unsigned c\n \t\t    const unsigned char a[20], const unsigned char b[20], int search);\n int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name,\n \t\tstruct string_list *needs_pushing);\n+int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);\n \n #endif\ndiff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\nindex 30bec4b..1947c28 100755\n--- a/t/t5531-deep-submodule-push.sh\n+++ b/t/t5531-deep-submodule-push.sh\n@@ -119,4 +119,98 @@ test_expect_success 'push succeeds if submodule has no remote and is on the firs\n \t)\n '\n \n+test_expect_success 'push unpushed submodules when not needed' '\n+\t(\n+\t\tcd work &&\n+\t\t(\n+\t\t\tcd gar/bage &&\n+\t\t\tgit checkout master &&\n+\t\t\t>junk5 &&\n+\t\t\tgit add junk5 &&\n+\t\t\tgit commit -m \"Fifth junk\" &&\n+\t\t\tgit push &&\n+\t\t\tgit rev-parse origin/master >../../../expected\n+\t\t) &&\n+\t\tgit checkout master &&\n+\t\tgit add gar/bage &&\n+\t\tgit commit -m \"Fifth commit for gar/bage\" &&\n+\t\tgit push --recurse-submodules=on-demand ../pub.git master\n+\t) &&\n+\t(\n+\t\tcd submodule.git &&\n+\t\tgit rev-parse master >../actual\n+\t) &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'push unpushed submodules when not needed 2' '\n+\t(\n+\t\tcd submodule.git &&\n+\t\tgit rev-parse master >../expected\n+\t) &&\n+\t(\n+\t\tcd work &&\n+\t\t(\n+\t\t\tcd gar/bage &&\n+\t\t\t>junk6 &&\n+\t\t\tgit add junk6 &&\n+\t\t\tgit commit -m \"Sixth junk\"\n+\t\t) &&\n+\t\t>junk2 &&\n+\t\tgit add junk2 &&\n+\t\tgit commit -m \"Second junk for work\" &&\n+\t\tgit push --recurse-submodules=on-demand ../pub.git master\n+\t) &&\n+\t(\n+\t\tcd submodule.git &&\n+\t\tgit rev-parse master >../actual\n+\t) &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'push unpushed submodules recursively' '\n+\t(\n+\t\tcd work &&\n+\t\t(\n+\t\t\tcd gar/bage &&\n+\t\t\tgit checkout master &&\n+\t\t\t> junk7 &&\n+\t\t\tgit add junk7 &&\n+\t\t\tgit commit -m \"Seventh junk\" &&\n+\t\t\tgit rev-parse master >../../../expected\n+\t\t) &&\n+\t\tgit checkout master &&\n+\t\tgit add gar/bage &&\n+\t\tgit commit -m \"Seventh commit for gar/bage\" &&\n+\t\tgit push --recurse-submodules=on-demand ../pub.git master\n+\t) &&\n+\t(\n+\t\tcd submodule.git &&\n+\t\tgit rev-parse master >../actual\n+\t) &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'push unpushable submodule recursively fails' '\n+\t(\n+\t\tcd work &&\n+\t\t(\n+\t\t\tcd gar/bage &&\n+\t\t\tgit rev-parse origin/master >../../../expected &&\n+\t\t\tgit checkout master~0 &&\n+\t\t\t> junk8 &&\n+\t\t\tgit add junk8 &&\n+\t\t\tgit commit -m \"Eighth junk\"\n+\t\t) &&\n+\t\tgit add gar/bage &&\n+\t\tgit commit -m \"Eighth commit for gar/bage\" &&\n+\t\ttest_must_fail git push --recurse-submodules=on-demand ../pub.git master\n+\t) &&\n+\t(\n+\t\tcd submodule.git &&\n+\t\tgit rev-parse master >../actual\n+\t) &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\ndiff --git a/transport.c b/transport.c\nindex d13bd4a..8c0fec0 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1009,6 +1009,11 @@ static void die_with_unpushed_submodules(struct string_list *needs_pushing)\n \t\t\t\"not be found on any remote:\\n\");\n \tfor (i = 0; i < needs_pushing->nr; i++)\n \t\tprintf(\"  %s\\n\", needs_pushing->items[i].string);\n+\tfprintf(stderr, \"\\nPlease try\\n\\n\"\n+\t\t\t\"\tgit push --recurse-submodules=on-demand\\n\\n\"\n+\t\t\t\"or cd to the path and use\\n\\n\"\n+\t\t\t\"\tgit push\\n\\n\"\n+\t\t\t\"to push them to a remote.\\n\\n\");\n \n \tstring_list_clear(needs_pushing, 0);\n \n@@ -1053,7 +1058,17 @@ int transport_push(struct transport *transport,\n \t\t\tflags & TRANSPORT_PUSH_MIRROR,\n \t\t\tflags & TRANSPORT_PUSH_FORCE);\n \n-\t\tif ((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) && !is_bare_repository()) {\n+\t\tif ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) && !is_bare_repository()) {\n+\t\t\tstruct ref *ref = remote_refs;\n+\t\t\tfor (; ref; ref = ref->next)\n+\t\t\t\tif (!is_null_sha1(ref->new_sha1) &&\n+\t\t\t\t    !push_unpushed_submodules(ref->new_sha1,\n+\t\t\t\t\t    transport->remote->name))\n+\t\t\t\t    die (\"Failed to push all needed submodules!\");\n+\t\t}\n+\n+\t\tif ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n+\t\t\t      TRANSPORT_RECURSE_SUBMODULES_CHECK)) && !is_bare_repository()) {\n \t\t\tstruct ref *ref = remote_refs;\n \t\t\tstruct string_list needs_pushing;\n \ndiff --git a/transport.h b/transport.h\nindex 059b330..9d19c78 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -102,6 +102,7 @@ struct transport {\n #define TRANSPORT_PUSH_PORCELAIN 16\n #define TRANSPORT_PUSH_SET_UPSTREAM 32\n #define TRANSPORT_RECURSE_SUBMODULES_CHECK 64\n+#define TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND 128\n \n #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)\n \n-- \n1.7.9.114.gead08\n"},{"id":"184632","messageId":"7vfwee9p1t.fsf@alter.siamese.dyndns.org","threadId":"29611","inReplyTo":"20120213092730.GB15585@t1405.greatnet.de","subject":"Re: [PATCH v5 1/3] Teach revision walking machinery to walk multiple times sequencially","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-14T01:33:34Z","receivedAt":"2012-02-14T01:33:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt <hvoigt@hvoigt.net> writes:\n\n> Previously it was not possible to iterate revisions twice using the\n> revision walking api. We add a reset_revision_walk() which clears the\n> used flags. This allows us to do multiple sequencial revision walks.\n>\n> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n\nI am kind of surprised that this is already its 5th round.\n\n> diff --git a/object.c b/object.c\n> index 6b06297..6291ce9 100644\n> --- a/object.c\n> +++ b/object.c\n> @@ -275,3 +275,14 @@ void object_array_remove_duplicates(struct object_array *array)\n>  \t\tarray->nr = dst;\n>  \t}\n>  }\n> +\n> +void clear_object_flags(unsigned flags)\n> +{\n> +\tint i;\n> +\tstruct object *obj;\n> +\n> +\tfor (i=0; i < obj_hash_size; i++) {\n> +\t\tif ((obj = obj_hash[i]) && obj->flags & flags)\n> +\t\t\tobj->flags &= ~flags;\n> +\t}\n> +}\n\nMinimally,\n\n        void clear_object_flags(unsigned flags)\n        {\n                int i;\n\n                for (i = 0; i < obj_hash_size; i++) {\n                        struct object *obj = obj_hash[i];\n                        if (obj && (obj->flags & flags))\n                                obj->flags &= ~flags;\n                }\n        }\n\nI am not sure if the \"If there is any bit set we care about, drop them\"\nbuys us anything, though.\n\n> diff --git a/revision.c b/revision.c\n> index c97d834..77ce6bd 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2061,6 +2061,11 @@ static void set_children(struct rev_info *revs)\n>  \t}\n>  }\n>  \n> +void reset_revision_walk()\n\n\tvoid reset_revision_walk(void)\n\n> +{\n> +\tclear_object_flags(SEEN | ADDED | SHOWN);\n> +}\n\nBut is this really the right API?  After a particular program finishes\nusing the revision walker, wouldn't it want to clear both the set of these\nstandard flag bits used by the traversal machinery, as well as whatever\nprogram specific bits it used to mark the objects with?\n\n> diff --git a/revision.h b/revision.h\n> index b8e9223..3535733 100644\n> --- a/revision.h\n> +++ b/revision.h\n> @@ -192,6 +192,7 @@ extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ct\n>  ...\n> +extern void reset_revision_walk();\n\nLikewise, \"extern void reset_revision_walk(void);\".\n\n> diff --git a/submodule.c b/submodule.c\n> index 9a28060..645ff5d 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -404,6 +404,7 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remote\n>  \twhile ((commit = get_revision(&rev)) && !needs_pushing)\n>  \t\tcommit_need_pushing(commit, &needs_pushing);\n>  \n> +\treset_revision_walk();\n>  \tfree(sha1_copy);\n>  \tstrbuf_release(&remotes_arg);\n>  \n> @@ -741,6 +742,7 @@ static int find_first_merges(struct object_array *result, const char *path,\n>  \t\tif (in_merge_bases(b, &commit, 1))\n>  \t\t\tadd_object_array(o, NULL, &merges);\n>  \t}\n> +\treset_revision_walk();\n>  \n>  \t/* Now we've got all merges that contain a and b. Prune all\n>  \t * merges that contain another found merge and save them in\n\nThese two hunk look like a *BUGFIX* to me (certainly it does not look like\nthis is an addition of any new feature).\n\nWhat bug does this fix, and how is the current submodule code broken\nwithout this patch?  Can you describe the problem in the log message, and\nadd a test to demonstrate the existing breakage?\n"},{"id":"184633","messageId":"7vbop29jqf.fsf@alter.siamese.dyndns.org","threadId":"29611","inReplyTo":"20120213092900.GC15585@t1405.greatnet.de","subject":"Re: [PATCH v5 2/3] Refactor submodule push check to use string list instead of integer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-14T03:28:24Z","receivedAt":"2012-02-14T03:28:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt <hvoigt@hvoigt.net> writes:\n\n> This allows us to tell the user which submodules have not been pushed.\n> Additionally this is helpful when we want to automatically try to push\n> submodules that have not been pushed.\n\nMakes sense.\n\n> diff --git a/submodule.c b/submodule.c\n> index 645ff5d..3c714c2 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -357,21 +357,20 @@ static void collect_submodules_from_diff(struct diff_queue_struct *q,\n>  \t\t\t\t\t void *data)\n>  {\n>  \tint i;\n> -\tint *needs_pushing = data;\n> +\tstruct string_list *needs_pushing = data;\n>  \n>  \tfor (i = 0; i < q->nr; i++) {\n>  \t\tstruct diff_filepair *p = q->queue[i];\n>  \t\tif (!S_ISGITLINK(p->two->mode))\n>  \t\t\tcontinue;\n>  \t\tif (submodule_needs_pushing(p->two->path, p->two->sha1)) {\n> -\t\t\t*needs_pushing = 1;\n> -\t\t\tbreak;\n> +\t\t\tif (!string_list_has_string(needs_pushing, p->two->path))\n> +\t\t\t\tstring_list_insert(needs_pushing, p->two->path);\n\nDoes string_list API have \"look for this and insert if it doesn't exist\nbut otherwise don't do anything\"?  Running get_entry_index() to answer\nhas_string() once and then calling it again to find where to insert to\nrespond to insert() looks a bit wasteful.\n\nJust wondering.\n\n>  \t\t}\n>  \t}\n>  }\n>  \n> -\n> -static void commit_need_pushing(struct commit *commit, int *needs_pushing)\n> +static void commit_need_pushing(struct commit *commit, struct string_list *needs_pushing)\n>  {\n>  \tstruct rev_info rev;\n>  \n> @@ -382,14 +381,15 @@ static void commit_need_pushing(struct commit *commit, int *needs_pushing)\n>  \tdiff_tree_combined_merge(commit, 1, &rev);\n>  }\n>  \n> -int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name)\n> +int check_submodule_needs_pushing(unsigned char new_sha1[20],\n> +\t\tconst char *remotes_name, struct string_list *needs_pushing)\n>  {\n>  \tstruct rev_info rev;\n>  \tstruct commit *commit;\n>  \tconst char *argv[] = {NULL, NULL, \"--not\", \"NULL\", NULL};\n>  \tint argc = ARRAY_SIZE(argv) - 1;\n>  \tchar *sha1_copy;\n> -\tint needs_pushing = 0;\n> +\n>  \tstruct strbuf remotes_arg = STRBUF_INIT;\n>  \n>  \tstrbuf_addf(&remotes_arg, \"--remotes=%s\", remotes_name);\n> @@ -401,14 +401,14 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remote\n>  \tif (prepare_revision_walk(&rev))\n>  \t\tdie(\"revision walk setup failed\");\n>  \n> -\twhile ((commit = get_revision(&rev)) && !needs_pushing)\n> -\t\tcommit_need_pushing(commit, &needs_pushing);\n> +\twhile ((commit = get_revision(&rev)) != NULL)\n> +\t\tcommit_need_pushing(commit, needs_pushing);\n\nNow the helper function to find list of submodules that need pushing given\none commit starting to look more and more misnamed.  It used to be \"learn\nif something needs pushing\", but now it is \"find what needs pushing\".\n\nCan somebody think of a good adjective to describe a submodule (or a set\nof submodules) in this state, so that we can name this helper function\nfind_blue_submodules(), if the adjective were \"blue\"?\n\n\"Unpushed\" submodule is the word used in the later part of the patch.\n"},{"id":"184634","messageId":"7v7gzq9jg2.fsf@alter.siamese.dyndns.org","threadId":"29611","inReplyTo":"20120213093008.GD15585@t1405.greatnet.de","subject":"Re: [PATCH v5 3/3] push: teach --recurse-submodules the on-demand option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-14T03:34:37Z","receivedAt":"2012-02-14T03:34:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt <hvoigt@hvoigt.net> writes:\n\n> diff --git a/submodule.c b/submodule.c\n> index 3c714c2..ff0cfd8 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -411,6 +411,54 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20],\n>  \treturn needs_pushing->nr;\n>  }\n>  \n> +static int push_submodule(const char *path)\n> +{\n> +\tif (add_submodule_odb(path))\n> +\t\treturn 1;\n> +\n> +\tif (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {\n> +\t\tstruct child_process cp;\n> +\t\tconst char *argv[] = {\"push\", NULL};\n> +\n> +\t\tmemset(&cp, 0, sizeof(cp));\n> +\t\tcp.argv = argv;\n> +\t\tcp.env = local_repo_env;\n> +\t\tcp.git_cmd = 1;\n> +\t\tcp.no_stdin = 1;\n> +\t\tcp.dir = path;\n> +\t\tif (run_command(&cp))\n> +\t\t\treturn 0;\n> +\t\tclose(cp.out);\n> +\t}\n> +\n> +\treturn 1;\n> +}\n\nHmm, this makes me wonder if we fire subprocesses and have them run in\nparallel (to a reasonably limited parallelism), it might make the overall\nuser experience more pleasant, and if we did the same on the fetching\nside, it would be even nicer.\n\nWe would need to keep track of children and after firing a handful of them\nwe would need to start waiting for some to finish and collect their exit\nstatus before firing more, and at the end we would need to wait for the\nremaining ones and find how each one of them did before returning from\npush_unpushed_submodules().  If we were to do so, what are the missing\nsupport we would need from the run_command() subsystem?\n\n> +int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name)\n> +{\n> +\tint i, ret = 1;\n> +\tstruct string_list needs_pushing;\n> +\n> +\tmemset(&needs_pushing, 0, sizeof(struct string_list));\n> +\tneeds_pushing.strdup_strings = 1;\n> +\n> +\tif (!check_submodule_needs_pushing(new_sha1, remotes_name, &needs_pushing))\n> +\t\treturn 1;\n> +\n> +\tfor (i = 0; i < needs_pushing.nr; i++) {\n> +\t\tconst char *path = needs_pushing.items[i].string;\n> +\t\tfprintf(stderr, \"Pushing submodule '%s'\\n\", path);\n> +\t\tif (!push_submodule(path)) {\n> +\t\t\tfprintf(stderr, \"Unable to push submodule '%s'\\n\", path);\n> +\t\t\tret = 0;\n> +\t\t}\n> +\t}\n"},{"id":"184795","messageId":"4F3C3172.8030505@web.de","threadId":"29611","inReplyTo":"7v7gzq9jg2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v5 3/3] push: teach --recurse-submodules the on-demand option","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2012-02-15T22:28:02Z","receivedAt":"2012-02-15T22:28:02Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 14.02.2012 04:34, schrieb Junio C Hamano:\n> Heiko Voigt <hvoigt@hvoigt.net> writes:\n> \n>> diff --git a/submodule.c b/submodule.c\n>> index 3c714c2..ff0cfd8 100644\n>> --- a/submodule.c\n>> +++ b/submodule.c\n>> @@ -411,6 +411,54 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20],\n>>  \treturn needs_pushing->nr;\n>>  }\n>>  \n>> +static int push_submodule(const char *path)\n>> +{\n>> +\tif (add_submodule_odb(path))\n>> +\t\treturn 1;\n>> +\n>> +\tif (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {\n>> +\t\tstruct child_process cp;\n>> +\t\tconst char *argv[] = {\"push\", NULL};\n>> +\n>> +\t\tmemset(&cp, 0, sizeof(cp));\n>> +\t\tcp.argv = argv;\n>> +\t\tcp.env = local_repo_env;\n>> +\t\tcp.git_cmd = 1;\n>> +\t\tcp.no_stdin = 1;\n>> +\t\tcp.dir = path;\n>> +\t\tif (run_command(&cp))\n>> +\t\t\treturn 0;\n>> +\t\tclose(cp.out);\n>> +\t}\n>> +\n>> +\treturn 1;\n>> +}\n> \n> Hmm, this makes me wonder if we fire subprocesses and have them run in\n> parallel (to a reasonably limited parallelism), it might make the overall\n> user experience more pleasant, and if we did the same on the fetching\n> side, it would be even nicer.\n\nYeah, I had the same idea and did some experiments when working on\nfetch some time ago.\n\n> We would need to keep track of children and after firing a handful of them\n> we would need to start waiting for some to finish and collect their exit\n> status before firing more, and at the end we would need to wait for the\n> remaining ones and find how each one of them did before returning from\n> push_unpushed_submodules().  If we were to do so, what are the missing\n> support we would need from the run_command() subsystem?\n\nWe would not only have to collect the exit status but also the output\nlines. You don't want to see the output of multiple fetches or pushes\nmixed together, so it makes sense to just defer that until the command\nexited and then print everything at once. The interesting part I couldn't\ncome up with an easy solution for is to preserve the output order between\nthe stdout and stdin lines, as they contain different parts of the\nprogress which would look strange when shuffled around.\n\nAnd I saw that sometimes parallel fetches took way longer than doing them\nsequentially (in my case because of strange DNS behavior of my DSL router),\nso we would definitely want a config option for that (maybe setting the\nmaximum number of simultaneous threads to be used).\n\nBut don't get me wrong, I'm all for having that feature! :-)\n"},{"id":"187770","messageId":"20120326193241.GA41087@book.hvoigt.net","threadId":"29611","inReplyTo":"7vfwee9p1t.fsf@alter.siamese.dyndns.org","subject":"Re: Re: [PATCH v5 1/3] Teach revision walking machinery to walk multiple times sequencially","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-03-26T19:32:52Z","receivedAt":"2012-03-26T19:32:52Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nsorry that my reply to this took quite some time.\n\nOn Mon, Feb 13, 2012 at 05:33:34PM -0800, Junio C Hamano wrote:\n> Heiko Voigt <hvoigt@hvoigt.net> writes:\n> \n> > Previously it was not possible to iterate revisions twice using the\n> > revision walking api. We add a reset_revision_walk() which clears the\n> > used flags. This allows us to do multiple sequencial revision walks.\n> >\n> > Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n> \n> I am kind of surprised that this is already its 5th round.\n\nWell, not this patch in particular but the series so I chose to it for\nthe whole series.\n\n> Minimally,\n> \n>         void clear_object_flags(unsigned flags)\n>         {\n>                 int i;\n> \n>                 for (i = 0; i < obj_hash_size; i++) {\n>                         struct object *obj = obj_hash[i];\n>                         if (obj && (obj->flags & flags))\n>                                 obj->flags &= ~flags;\n>                 }\n>         }\n> \n> I am not sure if the \"If there is any bit set we care about, drop them\"\n> buys us anything, though.\n\nOk done. And also dropped the \"if there is any bit set\" condition.\n\n> > +void reset_revision_walk()\n> \n> \tvoid reset_revision_walk(void)\n\nDone.\n\n> > +{\n> > +\tclear_object_flags(SEEN | ADDED | SHOWN);\n> > +}\n> \n> But is this really the right API?  After a particular program finishes\n> using the revision walker, wouldn't it want to clear both the set of these\n> standard flag bits used by the traversal machinery, as well as whatever\n> program specific bits it used to mark the objects with?\n\nWell if a program uses extra flags on objects it should clear the flags\nit set by using the clear_objects_flags() function itself. For example if\nthe program wants to reuse those extra flags in a second revision walk\nit would not be possible if reset_revision_walk() would clear all flags.\n\n> > diff --git a/revision.h b/revision.h\n> > index b8e9223..3535733 100644\n> > --- a/revision.h\n> > +++ b/revision.h\n> > @@ -192,6 +192,7 @@ extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ct\n> >  ...\n> > +extern void reset_revision_walk();\n> \n> Likewise, \"extern void reset_revision_walk(void);\".\n\nDone.\n\n> \n> > diff --git a/submodule.c b/submodule.c\n> > index 9a28060..645ff5d 100644\n> > --- a/submodule.c\n> > +++ b/submodule.c\n> > @@ -404,6 +404,7 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remote\n> >  \twhile ((commit = get_revision(&rev)) && !needs_pushing)\n> >  \t\tcommit_need_pushing(commit, &needs_pushing);\n> >  \n> > +\treset_revision_walk();\n> >  \tfree(sha1_copy);\n> >  \tstrbuf_release(&remotes_arg);\n> >  \n> > @@ -741,6 +742,7 @@ static int find_first_merges(struct object_array *result, const char *path,\n> >  \t\tif (in_merge_bases(b, &commit, 1))\n> >  \t\t\tadd_object_array(o, NULL, &merges);\n> >  \t}\n> > +\treset_revision_walk();\n> >  \n> >  \t/* Now we've got all merges that contain a and b. Prune all\n> >  \t * merges that contain another found merge and save them in\n> \n> These two hunk look like a *BUGFIX* to me (certainly it does not look like\n> this is an addition of any new feature).\n> \n> What bug does this fix, and how is the current submodule code broken\n> without this patch?  Can you describe the problem in the log message, and\n> add a test to demonstrate the existing breakage?\n\nThere is no breakage I know of. Its rather a cleanup which allows to\ncall these functions multiple times. I did this to avoid surprises.\nShould I put these in a separate patch or just add an explanation into\nthe current one?\n\nCheers Heiko\n"},{"id":"187771","messageId":"20120326193317.GB41087@book.hvoigt.net","threadId":"29611","inReplyTo":"7vbop29jqf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v5 2/3] Refactor submodule push check to use string list instead of integer","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-03-26T19:33:18Z","receivedAt":"2012-03-26T19:33:18Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Mon, Feb 13, 2012 at 07:28:24PM -0800, Junio C Hamano wrote:\n> Heiko Voigt <hvoigt@hvoigt.net> writes:\n> > diff --git a/submodule.c b/submodule.c\n> > index 645ff5d..3c714c2 100644\n> > --- a/submodule.c\n> > +++ b/submodule.c\n> > @@ -357,21 +357,20 @@ static void collect_submodules_from_diff(struct diff_queue_struct *q,\n> >  \t\t\t\t\t void *data)\n> >  {\n> >  \tint i;\n> > -\tint *needs_pushing = data;\n> > +\tstruct string_list *needs_pushing = data;\n> >  \n> >  \tfor (i = 0; i < q->nr; i++) {\n> >  \t\tstruct diff_filepair *p = q->queue[i];\n> >  \t\tif (!S_ISGITLINK(p->two->mode))\n> >  \t\t\tcontinue;\n> >  \t\tif (submodule_needs_pushing(p->two->path, p->two->sha1)) {\n> > -\t\t\t*needs_pushing = 1;\n> > -\t\t\tbreak;\n> > +\t\t\tif (!string_list_has_string(needs_pushing, p->two->path))\n> > +\t\t\t\tstring_list_insert(needs_pushing, p->two->path);\n> \n> Does string_list API have \"look for this and insert if it doesn't exist\n> but otherwise don't do anything\"?  Running get_entry_index() to answer\n> has_string() once and then calling it again to find where to insert to\n> respond to insert() looks a bit wasteful.\n> \n> Just wondering.\n\nIf I see correctly currently it has no such functionality. I can have a\nlook at adding another patch implementing this.\n\n\n> > -\n> > -static void commit_need_pushing(struct commit *commit, int *needs_pushing)\n> > +static void commit_need_pushing(struct commit *commit, struct string_list *needs_pushing)\n> >  {\n> >  \tstruct rev_info rev;\n> >  \n> > @@ -382,14 +381,15 @@ static void commit_need_pushing(struct commit *commit, int *needs_pushing)\n> >  \tdiff_tree_combined_merge(commit, 1, &rev);\n> >  }\n> >  \n> > -int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name)\n> > +int check_submodule_needs_pushing(unsigned char new_sha1[20],\n> > +\t\tconst char *remotes_name, struct string_list *needs_pushing)\n> >  {\n> >  \tstruct rev_info rev;\n> >  \tstruct commit *commit;\n> >  \tconst char *argv[] = {NULL, NULL, \"--not\", \"NULL\", NULL};\n> >  \tint argc = ARRAY_SIZE(argv) - 1;\n> >  \tchar *sha1_copy;\n> > -\tint needs_pushing = 0;\n> > +\n> >  \tstruct strbuf remotes_arg = STRBUF_INIT;\n> >  \n> >  \tstrbuf_addf(&remotes_arg, \"--remotes=%s\", remotes_name);\n> > @@ -401,14 +401,14 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remote\n> >  \tif (prepare_revision_walk(&rev))\n> >  \t\tdie(\"revision walk setup failed\");\n> >  \n> > -\twhile ((commit = get_revision(&rev)) && !needs_pushing)\n> > -\t\tcommit_need_pushing(commit, &needs_pushing);\n> > +\twhile ((commit = get_revision(&rev)) != NULL)\n> > +\t\tcommit_need_pushing(commit, needs_pushing);\n> \n> Now the helper function to find list of submodules that need pushing given\n> one commit starting to look more and more misnamed.  It used to be \"learn\n> if something needs pushing\", but now it is \"find what needs pushing\".\n> \n> Can somebody think of a good adjective to describe a submodule (or a set\n> of submodules) in this state, so that we can name this helper function\n> find_blue_submodules(), if the adjective were \"blue\"?\n> \n> \"Unpushed\" submodule is the word used in the later part of the patch.\n\nSo why not use unpushed as the adjective? find_unpushed_submodules()\nsound good to me. Will change the patch accordingly. Since we are\nalready discussing renaming should I also rename commit_need_pushing()\nto find_unpushed_submodule_commits()? I think that would make the whole\nnaming more consistent.\n\nCheers Heiko\n"},{"id":"187772","messageId":"20120326193334.GC41087@book.hvoigt.net","threadId":"29611","inReplyTo":"4F3C3172.8030505@web.de","subject":"Re: [PATCH v5 3/3] push: teach --recurse-submodules the on-demand option","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-03-26T19:33:34Z","receivedAt":"2012-03-26T19:33:34Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Wed, Feb 15, 2012 at 11:28:02PM +0100, Jens Lehmann wrote:\n> Am 14.02.2012 04:34, schrieb Junio C Hamano:\n> > Heiko Voigt <hvoigt@hvoigt.net> writes:\n> > \n> >> diff --git a/submodule.c b/submodule.c\n> >> index 3c714c2..ff0cfd8 100644\n> >> --- a/submodule.c\n> >> +++ b/submodule.c\n> >> @@ -411,6 +411,54 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20],\n> >>  \treturn needs_pushing->nr;\n> >>  }\n> >>  \n> >> +static int push_submodule(const char *path)\n> >> +{\n> >> +\tif (add_submodule_odb(path))\n> >> +\t\treturn 1;\n> >> +\n> >> +\tif (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {\n> >> +\t\tstruct child_process cp;\n> >> +\t\tconst char *argv[] = {\"push\", NULL};\n> >> +\n> >> +\t\tmemset(&cp, 0, sizeof(cp));\n> >> +\t\tcp.argv = argv;\n> >> +\t\tcp.env = local_repo_env;\n> >> +\t\tcp.git_cmd = 1;\n> >> +\t\tcp.no_stdin = 1;\n> >> +\t\tcp.dir = path;\n> >> +\t\tif (run_command(&cp))\n> >> +\t\t\treturn 0;\n> >> +\t\tclose(cp.out);\n> >> +\t}\n> >> +\n> >> +\treturn 1;\n> >> +}\n> > \n> > Hmm, this makes me wonder if we fire subprocesses and have them run in\n> > parallel (to a reasonably limited parallelism), it might make the overall\n> > user experience more pleasant, and if we did the same on the fetching\n> > side, it would be even nicer.\n> \n> Yeah, I had the same idea and did some experiments when working on\n> fetch some time ago.\n> \n> > We would need to keep track of children and after firing a handful of them\n> > we would need to start waiting for some to finish and collect their exit\n> > status before firing more, and at the end we would need to wait for the\n> > remaining ones and find how each one of them did before returning from\n> > push_unpushed_submodules().  If we were to do so, what are the missing\n> > support we would need from the run_command() subsystem?\n> \n> We would not only have to collect the exit status but also the output\n> lines. You don't want to see the output of multiple fetches or pushes\n> mixed together, so it makes sense to just defer that until the command\n> exited and then print everything at once. The interesting part I couldn't\n> come up with an easy solution for is to preserve the output order between\n> the stdout and stdin lines, as they contain different parts of the\n> progress which would look strange when shuffled around.\n> \n> And I saw that sometimes parallel fetches took way longer than doing them\n> sequentially (in my case because of strange DNS behavior of my DSL router),\n> so we would definitely want a config option for that (maybe setting the\n> maximum number of simultaneous threads to be used).\n> \n> But don't get me wrong, I'm all for having that feature! :-)\n\nMe too, but I would suggest that we first implement the basic recursive\npush functionality and then start to do optimizations like this. I think\nwe will definitely be interested in parallel pushing once we start using it.\n\nCheers Heiko\n"},{"id":"187775","messageId":"20120326195535.GA41511@book.hvoigt.net","threadId":"29611","inReplyTo":"20120326193317.GB41087@book.hvoigt.net","subject":"Re: [PATCH v5 2/3] Refactor submodule push check to use string list instead of integer","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-03-26T19:55:45Z","receivedAt":"2012-03-26T19:55:45Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Mon, Mar 26, 2012 at 09:33:17PM +0200, Heiko Voigt wrote:\n> On Mon, Feb 13, 2012 at 07:28:24PM -0800, Junio C Hamano wrote:\n> > Heiko Voigt <hvoigt@hvoigt.net> writes:\n> > > -\t\t\t*needs_pushing = 1;\n> > > -\t\t\tbreak;\n> > > +\t\t\tif (!string_list_has_string(needs_pushing, p->two->path))\n> > > +\t\t\t\tstring_list_insert(needs_pushing, p->two->path);\n> > \n> > Does string_list API have \"look for this and insert if it doesn't exist\n> > but otherwise don't do anything\"?  Running get_entry_index() to answer\n> > has_string() once and then calling it again to find where to insert to\n> > respond to insert() looks a bit wasteful.\n> > \n> > Just wondering.\n> \n> If I see correctly currently it has no such functionality. I can have a\n> look at adding another patch implementing this.\n\nIt seems my guess was wrong. As far as I read in the code\nstring_list_insert() already skips inserting existing strings and just\nreturns the existing items in the list. So it should be fine to remove\nthe string_list_has_string() call.\n\nCheers Heiko\n"},{"id":"187788","messageId":"4F70DE29.6050808@in.waw.pl","threadId":"29611","inReplyTo":"20120213093008.GD15585@t1405.greatnet.de","subject":"Re: [PATCH v5 3/3] push: teach --recurse-submodules the on-demand option","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-03-26T21:22:49Z","receivedAt":"2012-03-26T21:22:49Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"On 02/13/2012 10:30 AM, Heiko Voigt wrote:\n> When using this option git will search for all submodules that\n> have changed in the revisions to be send. It will then try to\n> push the currently checked out branch of each submodule.\n>\n> This helps when a user has finished working on a change which\n> involves submodules and just wants to push everything in one go.\n\n> ---recurse-submodules=check::\n> -\tCheck whether all submodule commits used by the revisions to be\n> -\tpushed are available on a remote tracking branch. Otherwise the\n> -\tpush will be aborted and the command will exit with non-zero status.\n> +--recurse-submodules=<check|on-demand>::\n> +\tMake sure all submodule commits used by the revisions to be\n> +\tpushed are available on a remote tracking branch. If check is\n> +\tused it will be checked that all submodule commits that changed\n> +\tin the revisions to be pushed are available on a remote.\n> +\tOtherwise the push will be aborted and exit with non-zero\n> +\tstatus. If on-demand is used all submodules that changed in the\n> +\trevisions to be pushed will be pushed. If on-demand was not able\n> +\tto push all necessary revisions it will also be aborted and exit\n> +\twith non-zero status.\nHi,\nthis desciption seems awkward. Not sure how to improve it, but:\n- the argument 'check' is changed to '<check|on-demand>', i.e. brackets \nare added. This changes the meaning, because brackets are used around a \nname for a value provided by the user. So here brackets shouldn't be \nused, because 'check' and 'on-demand' are literals.\n- s/if check is used it will be checked/if check is used git will verify \nthat/\n- s/a remote/the remote/\n- s/Otherwise the push/If any commits are missing the push/ (because \n'Otherwise' could refer to 'If check is used'.)\n\nZbyszek\n"},{"id":"187789","messageId":"7v1uofnjam.fsf@alter.siamese.dyndns.org","threadId":"29611","inReplyTo":"20120326193241.GA41087@book.hvoigt.net","subject":"Re: [PATCH v5 1/3] Teach revision walking machinery to walk multiple times sequencially","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-26T21:28:49Z","receivedAt":"2012-03-26T21:28:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt <hvoigt@hvoigt.net> writes:\n\n>> > +{\n>> > +\tclear_object_flags(SEEN | ADDED | SHOWN);\n>> > +}\n>> \n>> But is this really the right API?  After a particular program finishes\n>> using the revision walker, wouldn't it want to clear both the set of these\n>> standard flag bits used by the traversal machinery, as well as whatever\n>> program specific bits it used to mark the objects with?\n>\n> Well if a program uses extra flags on objects it should clear the flags\n> it set by using the clear_objects_flags() function itself. For example if\n> the program wants to reuse those extra flags in a second revision walk\n> it would not be possible if reset_revision_walk() would clear all flags.\n\nOK.\n\n>> These two hunk look like a *BUGFIX* to me (certainly it does not look like\n>> this is an addition of any new feature).\n>> \n>> What bug does this fix, and how is the current submodule code broken\n>> without this patch?  Can you describe the problem in the log message, and\n>> add a test to demonstrate the existing breakage?\n>\n> There is no breakage I know of. Its rather a cleanup which allows to\n> call these functions multiple times. I did this to avoid surprises.\n\nOK.\n\nThanks.\n"},{"id":"187790","messageId":"7vwr67m4pc.fsf@alter.siamese.dyndns.org","threadId":"29611","inReplyTo":"20120326195535.GA41511@book.hvoigt.net","subject":"Re: [PATCH v5 2/3] Refactor submodule push check to use string list instead of integer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-26T21:29:19Z","receivedAt":"2012-03-26T21:29:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt <hvoigt@hvoigt.net> writes:\n\n> It seems my guess was wrong. As far as I read in the code\n> string_list_insert() already skips inserting existing strings and just\n> returns the existing items in the list. So it should be fine to remove\n> the string_list_has_string() call.\n\nSounds sane.  Thanks.\n"},{"id":"187937","messageId":"20120328153010.GA46920@book.hvoigt.net","threadId":"29611","inReplyTo":"4F70DE29.6050808@in.waw.pl","subject":"Re: [PATCH v5 3/3] push: teach --recurse-submodules the on-demand option","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-03-28T15:30:11Z","receivedAt":"2012-03-28T15:30:11Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Mon, Mar 26, 2012 at 11:22:49PM +0200, Zbigniew J??drzejewski-Szmek wrote:\n> On 02/13/2012 10:30 AM, Heiko Voigt wrote:\n>> When using this option git will search for all submodules that\n>> have changed in the revisions to be send. It will then try to\n>> push the currently checked out branch of each submodule.\n>>\n>> This helps when a user has finished working on a change which\n>> involves submodules and just wants to push everything in one go.\n>\n>> ---recurse-submodules=check::\n>> -\tCheck whether all submodule commits used by the revisions to be\n>> -\tpushed are available on a remote tracking branch. Otherwise the\n>> -\tpush will be aborted and the command will exit with non-zero status.\n>> +--recurse-submodules=<check|on-demand>::\n>> +\tMake sure all submodule commits used by the revisions to be\n>> +\tpushed are available on a remote tracking branch. If check is\n>> +\tused it will be checked that all submodule commits that changed\n>> +\tin the revisions to be pushed are available on a remote.\n>> +\tOtherwise the push will be aborted and exit with non-zero\n>> +\tstatus. If on-demand is used all submodules that changed in the\n>> +\trevisions to be pushed will be pushed. If on-demand was not able\n>> +\tto push all necessary revisions it will also be aborted and exit\n>> +\twith non-zero status.\n> Hi,\n> this desciption seems awkward. Not sure how to improve it, but:\n> - the argument 'check' is changed to '<check|on-demand>', i.e. brackets  \n> are added. This changes the meaning, because brackets are used around a  \n> name for a value provided by the user. So here brackets shouldn't be  \n> used, because 'check' and 'on-demand' are literals.\n> - s/if check is used it will be checked/if check is used git will verify  \n> that/\n> - s/a remote/the remote/\n> - s/Otherwise the push/If any commits are missing the push/ (because  \n> 'Otherwise' could refer to 'If check is used'.)\n\nI agree on all of these except\n\n\ts/a remote/the remote/\n\nwhich I changed to\n\n\ts/a remote/at least one remote of the submodule/\n\nsince it really just checks whether the commits are on some remote. It\nit not strictly the one defined in .gitmodules. Doing it this way allows\nto work with forks without the need to change .gitmodules.\n\nCheers Heiko\n"},{"id":"191466","messageId":"20120513144733.GA23408@book.hvoigt.net","threadId":"29611","inReplyTo":"4F3C3172.8030505@web.de","subject":"[RFC/PATCH] read from 2 filedescriptors simultaneously into one strbuf","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-05-13T14:47:34Z","receivedAt":"2012-05-13T14:47:34Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nOn Wed, Feb 15, 2012 at 11:28:02PM +0100, Jens Lehmann wrote:\n> Am 14.02.2012 04:34, schrieb Junio C Hamano:\n> > Hmm, this makes me wonder if we fire subprocesses and have them run in\n> > parallel (to a reasonably limited parallelism), it might make the overall\n> > user experience more pleasant, and if we did the same on the fetching\n> > side, it would be even nicer.\n> \n> Yeah, I had the same idea and did some experiments when working on\n> fetch some time ago.\n> \n> > We would need to keep track of children and after firing a handful of them\n> > we would need to start waiting for some to finish and collect their exit\n> > status before firing more, and at the end we would need to wait for the\n> > remaining ones and find how each one of them did before returning from\n> > push_unpushed_submodules().  If we were to do so, what are the missing\n> > support we would need from the run_command() subsystem?\n> \n> We would not only have to collect the exit status but also the output\n> lines. You don't want to see the output of multiple fetches or pushes\n> mixed together, so it makes sense to just defer that until the command\n> exited and then print everything at once. The interesting part I couldn't\n> come up with an easy solution for is to preserve the output order between\n> the stdout and stdin lines, as they contain different parts of the\n> progress which would look strange when shuffled around.\n\nI had a go at a possible implementation for deferred output of parallel\nsubprocesses today. The idea is to use two concurrent threads\nsimultaneously reading from the two filedescriptors and writing the\nresult into one strbuf. Below is the diff for the current code I have[1].\n\nOne current limitation is that it only works with pthreads enabled. I\nfirst tried to use the same pipe for the two threads to avoid the need\nfor locking and shared memory. I am not sure but it seems that\nconcurrents writes into the same pipe are not allowed.\n\nWhat do you think of this approach? Is it the correct place or should it\ngo into another module? Also the function naming might need some polishing.\n\nCheers Heiko\n\nP.S.: We can of course get rid of the second thread and just read in the\nmain thread for one of the filedescriptors but this was easier while\nthinking about it.\n\n[1]\ndiff --git a/run-command.c b/run-command.c\nindex 2a1041e..48c9e07 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1,6 +1,8 @@\n #include \"cache.h\"\n #include \"run-command.h\"\n #include \"exec_cmd.h\"\n+#include \"strbuf.h\"\n+#include <pthread.h>\n \n static inline void close_pair(int fd[2])\n {\n@@ -407,6 +409,60 @@ fail_pipe:\n \treturn 0;\n }\n \n+#ifndef NO_PTHREADS\n+struct fd2fd_data {\n+\tpthread_mutex_t mutex;\n+\tstruct strbuf *output;\n+};\n+\n+static int fd2fd(int in, int out, void *data)\n+{\n+\tstruct fd2fd_data *me = data;\n+\tstruct strbuf line;\n+\n+\tFILE *fin = xfdopen(in, \"r\");\n+\n+\twhile (strbuf_getwholeline(&line, fin, '\\n') != EOF) {\n+\t\tpthread_mutex_lock(&me->mutex);\n+\t\tstrbuf_addbuf(me->output, &line);\n+\t\tpthread_mutex_unlock(&me->mutex);\n+\t}\n+\n+\tstrbuf_release(&line);\n+\treturn 0;\n+}\n+\n+void read_2_fds_into_strbuf(int fd1, int fd2, struct strbuf *output)\n+{\n+\tstruct async err_async;\n+\tstruct async out_async;\n+\tstruct fd2fd_data async_data;\n+\n+\tpthread_mutex_init(&async_data.mutex, NULL);\n+\tasync_data.output = output;\n+\n+\t/* start two threads to read fd1 and fd2 simultaneously\n+\t * into one strbuf */\n+\tmemset(&out_async, 0, sizeof(out_async));\n+\tout_async.proc = fd2fd;\n+\tout_async.data = &async_data;\n+\tout_async.in = fd1;\n+\n+\tmemset(&err_async, 0, sizeof(err_async));\n+\terr_async.proc = fd2fd;\n+\terr_async.data = &async_data;\n+\terr_async.in = fd2;\n+\n+\tstart_async(&out_async);\n+\tstart_async(&err_async);\n+\n+\tfinish_async(&out_async);\n+\tfinish_async(&err_async);\n+\n+\tpthread_mutex_destroy(&async_data.mutex);\n+}\n+#endif /* NO_PTHREADS */\n+\n int finish_command(struct child_process *cmd)\n {\n \treturn wait_or_whine(cmd->pid, cmd->argv[0], cmd->silent_exec_failure);\ndiff --git a/run-command.h b/run-command.h\nindex 56491b9..a65debb 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -5,6 +5,8 @@\n #include <pthread.h>\n #endif\n \n+struct strbuf;\n+\n struct child_process {\n \tconst char **argv;\n \tpid_t pid;\n@@ -90,4 +92,8 @@ struct async {\n int start_async(struct async *async);\n int finish_async(struct async *async);\n \n+#ifndef NO_PTHREADS\n+void read_2_fds_into_strbuf(int fd1, int fd2, struct strbuf *output);\n+#endif\n+\n #endif\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex 10b26e4..774807b 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -11,4 +11,10 @@ test_expect_success 'start_command reports ENOENT' '\n \ttest-run-command start-command-ENOENT ./does-not-exist\n '\n \n+test_expect_success 'read_2_fds_into_strbuf basic behavior' '\n+\ttest-run-command out2strbuf >actual &&\n+\tgrep \"^Hallo Stdout$\" actual &&\n+\tgrep \"^Hallo Stderr$\" actual\n+'\n+\n test_done\ndiff --git a/test-run-command.c b/test-run-command.c\nindex 0612bfa..f59485d 100644\n--- a/test-run-command.c\n+++ b/test-run-command.c\n@@ -10,26 +10,72 @@\n \n #include \"git-compat-util.h\"\n #include \"run-command.h\"\n+#include \"strbuf.h\"\n #include <string.h>\n #include <errno.h>\n \n+static int run_write_async(int in, int out, void *data)\n+{\n+\tconst char *msg = data;\n+\n+\tFILE *fout = xfdopen(out, \"w\");\n+\tfprintf(fout, \"%s\\n\", msg);\n+\n+\tfclose(fout);\n+\treturn 0;\n+}\n+\n int main(int argc, char **argv)\n {\n \tstruct child_process proc;\n+\tstruct async write_out;\n+\tstruct async write_err;\n \n \tmemset(&proc, 0, sizeof(proc));\n \n-\tif (argc < 3)\n+\tif (argc < 2)\n \t\treturn 1;\n-\tproc.argv = (const char **)argv+2;\n \n \tif (!strcmp(argv[1], \"start-command-ENOENT\")) {\n+\t\tif (argc < 3)\n+\t\t\treturn 1;\n+\t\tproc.argv = (const char **)argv+2;\n+\n \t\tif (start_command(&proc) < 0 && errno == ENOENT)\n \t\t\treturn 0;\n \t\tfprintf(stderr, \"FAIL %s\\n\", argv[1]);\n \t\treturn 1;\n \t}\n \n+\tif (!strcmp(argv[1], \"out2strbuf\")) {\n+#ifndef NO_PTHREADS\n+\t\tstruct strbuf output = STRBUF_INIT;\n+\n+\t\tmemset(&write_out, 0, sizeof(write_out));\n+\t\twrite_out.data = \"Hallo Stdout\";\n+\t\twrite_out.proc = run_write_async;\n+\t\twrite_out.out = -1;\n+\n+\t\tmemset(&write_err, 0, sizeof(write_err));\n+\t\twrite_err.data = \"Hallo Stderr\";\n+\t\twrite_err.proc = run_write_async;\n+\t\twrite_err.out = -1;\n+\n+\t\tstart_async(&write_out);\n+\t\tstart_async(&write_err);\n+\n+\t\tread_2_fds_into_strbuf(write_out.out, write_err.out, &output);\n+\n+\t\tfinish_async(&write_out);\n+\t\tfinish_async(&write_err);\n+\n+\t\tprintf(\"%s\", output.buf);\n+\t\tstrbuf_release(&output);\n+#endif /* NO_PTHREADS */\n+\n+\t\treturn 0;\n+\t}\n+\n \tfprintf(stderr, \"check usage\\n\");\n \treturn 1;\n }\n"}]}