{"thread":{"id":"28168","subject":"[PATCH v4 0/2] push: submodule support","startedAt":"2011-08-19T22:08:46Z","lastAt":"2011-12-13T08:48:27Z","messageCount":20,"participants":["Fredrik Gustafsson","Junio C Hamano","Heiko Voigt","Jens Lehmann","Phil Hord"],"isPatch":true,"patchVersion":4,"patchTotal":2},"messages":[{"id":"173888","messageId":"1313791728-11328-1-git-send-email-iveqy@iveqy.com","threadId":"28168","inReplyTo":null,"subject":"[PATCH v4 0/2] push: submodule support","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2011-08-19T22:08:46Z","receivedAt":"2011-08-19T22:08:46Z","isPatch":true,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"The 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\nFredrik Gustafsson (2):\n  push: Don't push a repository with unpushed submodules\n  push: teach --recurse-submodules the on-demand option\n\n Documentation/git-push.txt     |    9 ++\n builtin/push.c                 |   26 +++++++\n combine-diff.c                 |    2 +-\n submodule.c                    |  161 ++++++++++++++++++++++++++++++++++++++++\n submodule.h                    |    2 +\n t/t5531-deep-submodule-push.sh |  111 +++++++++++++++++++++++++++\n transport.c                    |   17 ++++\n transport.h                    |    2 +\n 8 files changed, 329 insertions(+), 1 deletions(-)\n\n-- \n1.7.6.551.gfb18e\n"},{"id":"173890","messageId":"1313791728-11328-2-git-send-email-iveqy@iveqy.com","threadId":"28168","inReplyTo":"1313791728-11328-1-git-send-email-iveqy@iveqy.com","subject":"[PATCH v4 1/2] push: Don't push a repository with unpushed submodules","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2011-08-19T22:08:47Z","receivedAt":"2011-08-19T22:08:47Z","isPatch":true,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"When working with submodules it is easy to forget to push a\nsubmodule to the server but pushing a super-project that\ncontains a commit for that submodule. The result is that the\nsuperproject points at a submodule commit that is not available\non the server.\n\nThis adds the option --recurse-submodules=check to push. When\nusing this option git will check that all submodule commits that\nare about to be pushed are present on a remote of the submodule.\n\nTo be able to use a combined diff, disabling a diff callback has\nbeen removed from combined-diff.c.\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     |    6 ++\n builtin/push.c                 |   19 +++++++\n combine-diff.c                 |    2 +-\n submodule.c                    |  108 ++++++++++++++++++++++++++++++++++++++++\n submodule.h                    |    1 +\n t/t5531-deep-submodule-push.sh |   87 ++++++++++++++++++++++++++++++++\n transport.c                    |    9 +++\n transport.h                    |    1 +\n 8 files changed, 232 insertions(+), 1 deletions(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex 49c6e9f..aede488 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -162,6 +162,12 @@ 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+\n+\n include::urls-remotes.txt[]\n \n OUTPUT\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 9cebf9e..35cce53 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -8,6 +8,7 @@\n #include \"remote.h\"\n #include \"transport.h\"\n #include \"parse-options.h\"\n+#include \"submodule.h\"\n \n static const char * const push_usage[] = {\n \t\"git push [<options>] [<repository> [<refspec>...]]\",\n@@ -219,6 +220,21 @@ static int do_push(const char *repo, int flags)\n \treturn !!errs;\n }\n \n+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+\tif (arg) {\n+\t\tif (!strcmp(arg, \"check\"))\n+\t\t\t*flags |= TRANSPORT_RECURSE_SUBMODULES_CHECK;\n+\t\telse\n+\t\t\tdie(\"bad %s argument: %s\", opt->long_name, arg);\n+\t} else\n+\t\tdie(\"option %s needs an argument (check)\", opt->long_name);\n+\n+\treturn 0;\n+}\n+\n int cmd_push(int argc, const char **argv, const char *prefix)\n {\n \tint flags = 0;\n@@ -236,6 +252,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT('n' , \"dry-run\", &flags, \"dry run\", TRANSPORT_PUSH_DRY_RUN),\n \t\tOPT_BIT( 0,  \"porcelain\", &flags, \"machine-readable output\", TRANSPORT_PUSH_PORCELAIN),\n \t\tOPT_BIT('f', \"force\", &flags, \"force updates\", TRANSPORT_PUSH_FORCE),\n+\t\t{ OPTION_CALLBACK, 0, \"recurse-submodules\", &flags, \"check\",\n+\t\t\t\"controls recursive pushing of submodules\",\n+\t\t\tPARSE_OPT_OPTARG, option_parse_recurse_submodules },\n \t\tOPT_BOOLEAN( 0 , \"thin\", &thin, \"use thin pack\"),\n \t\tOPT_STRING( 0 , \"receive-pack\", &receivepack, \"receive-pack\", \"receive pack program\"),\n \t\tOPT_STRING( 0 , \"exec\", &receivepack, \"receive-pack\", \"receive pack program\"),\ndiff --git a/combine-diff.c b/combine-diff.c\nindex b11eb71..f7a8978 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1074,7 +1074,7 @@ void diff_tree_combined(const unsigned char *sha1,\n \t\t * when doing combined diff.\n \t\t */\n \t\tint stat_opt = (opt->output_format &\n-\t\t\t\t(DIFF_FORMAT_NUMSTAT|DIFF_FORMAT_DIFFSTAT));\n+\t\t\t\t(DIFF_FORMAT_NUMSTAT|DIFF_FORMAT_DIFFSTAT|DIFF_FORMAT_CALLBACK));\n \t\tif (i == 0 && stat_opt)\n \t\t\tdiffopts.output_format = stat_opt;\n \t\telse\ndiff --git a/submodule.c b/submodule.c\nindex 1ba9646..45f508c 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -308,6 +308,114 @@ void set_config_fetch_recurse_submodules(int value)\n \tconfig_fetch_recurse_submodules = value;\n }\n \n+static int has_remote(const char *refname, const unsigned char *sha1, int flags, void *cb_data)\n+{\n+\treturn 1;\n+}\n+\n+static int submodule_needs_pushing(const char *path, const unsigned char sha1[20])\n+{\n+\tif (add_submodule_odb(path) || !lookup_commit_reference(sha1))\n+\t\treturn 0;\n+\n+\tif (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {\n+\t\tstruct child_process cp;\n+\t\tconst char *argv[] = {\"rev-list\", NULL, \"--not\", \"--remotes\", \"-n\", \"1\" , NULL};\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tint needs_pushing = 0;\n+\n+\t\targv[1] = sha1_to_hex(sha1);\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.out = -1;\n+\t\tcp.dir = path;\n+\t\tif (start_command(&cp))\n+\t\t\tdie(\"Could not run 'git rev-list %s --not --remotes -n 1' command in submodule %s\",\n+\t\t\t\tsha1_to_hex(sha1), path);\n+\t\tif (strbuf_read(&buf, cp.out, 41))\n+\t\t\tneeds_pushing = 1;\n+\t\tfinish_command(&cp);\n+\t\tclose(cp.out);\n+\t\tstrbuf_release(&buf);\n+\t\treturn needs_pushing;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static void collect_submodules_from_diff(struct diff_queue_struct *q,\n+\t\t\t\t\t struct diff_options *options,\n+\t\t\t\t\t void *data)\n+{\n+\tint i;\n+\tint *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}\n+\t}\n+}\n+\n+\n+static void commit_need_pushing(struct commit *commit, struct commit_list *parent, int *needs_pushing)\n+{\n+\tconst unsigned char (*parents)[20];\n+\tunsigned int i, n;\n+\tstruct rev_info rev;\n+\n+\tn = commit_list_count(parent);\n+\tparents = xmalloc(n * sizeof(*parents));\n+\n+\tfor (i = 0; i < n; i++) {\n+\t\thashcpy((unsigned char *)(parents + i), parent->item->object.sha1);\n+\t\tparent = parent->next;\n+\t}\n+\n+\tinit_revisions(&rev, NULL);\n+\trev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n+\trev.diffopt.format_callback = collect_submodules_from_diff;\n+\trev.diffopt.format_callback_data = needs_pushing;\n+\tdiff_tree_combined(commit->object.sha1, parents, n, 1, &rev);\n+\n+\tfree(parents);\n+}\n+\n+int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name)\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+\tstruct strbuf remotes_arg = STRBUF_INIT;\n+\n+\tstrbuf_addf(&remotes_arg, \"--remotes=%s\", remotes_name);\n+\tinit_revisions(&rev, NULL);\n+\tsha1_copy = xstrdup(sha1_to_hex(new_sha1));\n+\targv[1] = sha1_copy;\n+\targv[3] = remotes_arg.buf;\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)) && !needs_pushing)\n+\t\tcommit_need_pushing(commit, commit->parents, &needs_pushing);\n+\n+\tfree(sha1_copy);\n+\tstrbuf_release(&remotes_arg);\n+\n+\treturn needs_pushing;\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 5350b0d..799c22d 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -29,5 +29,6 @@ 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]);\n+int check_submodule_needs_pushing(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 faa2e96..30bec4b 100755\n--- a/t/t5531-deep-submodule-push.sh\n+++ b/t/t5531-deep-submodule-push.sh\n@@ -32,4 +32,91 @@ test_expect_success push '\n \t)\n '\n \n+test_expect_success 'push if submodule has no remote' '\n+\t(\n+\t\tcd work/gar/bage &&\n+\t\t>junk2 &&\n+\t\tgit add junk2 &&\n+\t\tgit commit -m \"Second junk\"\n+\t) &&\n+\t(\n+\t\tcd work &&\n+\t\tgit add gar/bage &&\n+\t\tgit commit -m \"Second commit for gar/bage\" &&\n+\t\tgit push --recurse-submodules=check ../pub.git master\n+\t)\n+'\n+\n+test_expect_success 'push fails if submodule commit not on remote' '\n+\t(\n+\t\tcd work/gar &&\n+\t\tgit clone --bare bage ../../submodule.git &&\n+\t\tcd bage &&\n+\t\tgit remote add origin ../../../submodule.git &&\n+\t\tgit fetch &&\n+\t\t>junk3 &&\n+\t\tgit add junk3 &&\n+\t\tgit commit -m \"Third junk\"\n+\t) &&\n+\t(\n+\t\tcd work &&\n+\t\tgit add gar/bage &&\n+\t\tgit commit -m \"Third commit for gar/bage\" &&\n+\t\ttest_must_fail git push --recurse-submodules=check ../pub.git master\n+\t)\n+'\n+\n+test_expect_success 'push succeeds after commit was pushed to remote' '\n+\t(\n+\t\tcd work/gar/bage &&\n+\t\tgit push origin master\n+\t) &&\n+\t(\n+\t\tcd work &&\n+\t\tgit push --recurse-submodules=check ../pub.git master\n+\t)\n+'\n+\n+test_expect_success 'push fails when commit on multiple branches if one branch has no remote' '\n+\t(\n+\t\tcd work/gar/bage &&\n+\t\t>junk4 &&\n+\t\tgit add junk4 &&\n+\t\tgit commit -m \"Fourth junk\"\n+\t) &&\n+\t(\n+\t\tcd work &&\n+\t\tgit branch branch2 &&\n+\t\tgit add gar/bage &&\n+\t\tgit commit -m \"Fourth commit for gar/bage\" &&\n+\t\tgit checkout branch2 &&\n+\t\t(\n+\t\t\tcd gar/bage &&\n+\t\t\tgit checkout HEAD~1\n+\t\t) &&\n+\t\t>junk1 &&\n+\t\tgit add junk1 &&\n+\t\tgit commit -m \"First junk\" &&\n+\t\ttest_must_fail git push --recurse-submodules=check ../pub.git\n+\t)\n+'\n+\n+test_expect_success 'push succeeds if submodule has no remote and is on the first superproject commit' '\n+\tgit init --bare a\n+\tgit clone a a1 &&\n+\t(\n+\t\tcd a1 &&\n+\t\tgit init b\n+\t\t(\n+\t\t\tcd b &&\n+\t\t\t>junk &&\n+\t\t\tgit add junk &&\n+\t\t\tgit commit -m \"initial\"\n+\t\t) &&\n+\t\tgit add b &&\n+\t\tgit commit -m \"added submodule\" &&\n+\t\tgit push --recurse-submodule=check origin master\n+\t)\n+'\n+\n test_done\ndiff --git a/transport.c b/transport.c\nindex 98c5778..d2725e5 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -10,6 +10,7 @@\n #include \"refs.h\"\n #include \"branch.h\"\n #include \"url.h\"\n+#include \"submodule.h\"\n \n /* rsync support */\n \n@@ -1045,6 +1046,14 @@ 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\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    check_submodule_needs_pushing(ref->new_sha1,transport->remote->name))\n+\t\t\t\t\tdie(\"There are unpushed submodules, aborting.\");\n+\t\t}\n+\n \t\tpush_ret = transport->push_refs(transport, remote_refs, flags);\n \t\terr = push_had_errors(remote_refs);\n \t\tret = push_ret | err;\ndiff --git a/transport.h b/transport.h\nindex 161d724..059b330 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -101,6 +101,7 @@ struct transport {\n #define TRANSPORT_PUSH_MIRROR 8\n #define TRANSPORT_PUSH_PORCELAIN 16\n #define TRANSPORT_PUSH_SET_UPSTREAM 32\n+#define TRANSPORT_RECURSE_SUBMODULES_CHECK 64\n \n #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)\n \n-- \n1.7.6.551.gfb18e\n"},{"id":"173889","messageId":"1313791728-11328-3-git-send-email-iveqy@iveqy.com","threadId":"28168","inReplyTo":"1313791728-11328-1-git-send-email-iveqy@iveqy.com","subject":"[PATCH v4 2/2] push: teach --recurse-submodules the on-demand option","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2011-08-19T22:08:48Z","receivedAt":"2011-08-19T22:08:48Z","isPatch":true,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?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     |   13 ++++--\n builtin/push.c                 |    7 +++\n submodule.c                    |   89 ++++++++++++++++++++++++++++++++--------\n submodule.h                    |    1 +\n t/t5531-deep-submodule-push.sh |   24 +++++++++++\n transport.c                    |   10 ++++-\n transport.h                    |    1 +\n 7 files changed, 121 insertions(+), 24 deletions(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex aede488..fe60d28 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -162,11 +162,14 @@ 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-\n+--recurse-submodules=<check|on-demand>::\n+\tCheck whether all submodule commits used by the revisions to be pushed\n+\tare available on a remote tracking branch. If check is used the push\n+\twill be aborted and the command will exit with non-zero status.\n+\tIf on-demand is used all submodules that changed in the\n+\tto 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 include::urls-remotes.txt[]\n \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 45f508c..dc95498 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -8,7 +8,10 @@\n #include \"diffcore.h\"\n #include \"refs.h\"\n #include \"string-list.h\"\n+#include \"transport.h\"\n \n+typedef int (*needs_push_func_t)(const char *path, const unsigned char sha1[20],\n+\t\tvoid *data);\n static struct string_list config_name_for_path;\n static struct string_list config_fetch_recurse_submodules_for_name;\n static struct string_list config_ignore_for_name;\n@@ -308,21 +311,24 @@ void set_config_fetch_recurse_submodules(int value)\n \tconfig_fetch_recurse_submodules = value;\n }\n \n+typedef int (*module_func_t)(const char *path, const unsigned char sha1[20], void *data);\n+\n static int has_remote(const char *refname, const unsigned char *sha1, int flags, void *cb_data)\n {\n \treturn 1;\n }\n \n-static int submodule_needs_pushing(const char *path, const unsigned char sha1[20])\n+int submodule_needs_pushing(const char *path, const unsigned char sha1[20], void *data)\n {\n+\tint *needs_pushing = data;\n+\n \tif (add_submodule_odb(path) || !lookup_commit_reference(sha1))\n-\t\treturn 0;\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[] = {\"rev-list\", NULL, \"--not\", \"--remotes\", \"-n\", \"1\" , NULL};\n \t\tstruct strbuf buf = STRBUF_INIT;\n-\t\tint needs_pushing = 0;\n \n \t\targv[1] = sha1_to_hex(sha1);\n \t\tmemset(&cp, 0, sizeof(cp));\n@@ -336,41 +342,74 @@ static int submodule_needs_pushing(const char *path, const unsigned char sha1[20\n \t\t\tdie(\"Could not run 'git rev-list %s --not --remotes -n 1' command in submodule %s\",\n \t\t\t\tsha1_to_hex(sha1), path);\n \t\tif (strbuf_read(&buf, cp.out, 41))\n-\t\t\tneeds_pushing = 1;\n+\t\t\t*needs_pushing = 1;\n \t\tfinish_command(&cp);\n \t\tclose(cp.out);\n \t\tstrbuf_release(&buf);\n-\t\treturn needs_pushing;\n+\t\treturn !*needs_pushing;\n \t}\n \n-\treturn 0;\n+\treturn 1;\n+}\n+\n+int push_submodule(const char *path, const unsigned char sha1[20], void *data)\n+{\n+\tif (add_submodule_odb(path) || !lookup_commit_reference(sha1))\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.out = -1;\n+\t\tcp.dir = path;\n+\t\tif (run_command(&cp))\n+\t\t\tdie(\"Could not run 'git push' command in submodule %s\", path);\n+\t\tclose(cp.out);\n+\t}\n+\n+\treturn 1;\n }\n \n+struct collect_submodules_data {\n+\tmodule_func_t func;\n+\tvoid *data;\n+\tint ret;\n+};\n+\n static void collect_submodules_from_diff(struct diff_queue_struct *q,\n \t\t\t\t\t struct diff_options *options,\n \t\t\t\t\t void *data)\n {\n \tint i;\n-\tint *needs_pushing = data;\n+\tstruct collect_submodules_data *me = 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\tif (!(me->ret = me->func(p->two->path, p->two->sha1, me->data)))\n \t\t\tbreak;\n-\t\t}\n \t}\n }\n \n-\n-static void commit_need_pushing(struct commit *commit, struct commit_list *parent, int *needs_pushing)\n+static int commit_need_pushing(struct commit *commit, struct commit_list *parent,\n+\tmodule_func_t func, void *data)\n {\n \tconst unsigned char (*parents)[20];\n \tunsigned int i, n;\n \tstruct rev_info rev;\n \n+\tstruct collect_submodules_data cb;\n+\tcb.func = func;\n+\tcb.data = data;\n+\tcb.ret = 1;\n+\n \tn = commit_list_count(parent);\n \tparents = xmalloc(n * sizeof(*parents));\n \n@@ -382,21 +421,23 @@ static void commit_need_pushing(struct commit *commit, struct commit_list *paren\n \tinit_revisions(&rev, NULL);\n \trev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n \trev.diffopt.format_callback = collect_submodules_from_diff;\n-\trev.diffopt.format_callback_data = needs_pushing;\n+\trev.diffopt.format_callback_data = &cb;\n \tdiff_tree_combined(commit->object.sha1, parents, n, 1, &rev);\n \n \tfree(parents);\n+\treturn cb.ret;\n }\n \n-int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name)\n+static int inspect_superproject_commits(unsigned char new_sha1[20], const char *remotes_name,\n+\tmodule_func_t func, void *data)\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 \tstruct strbuf remotes_arg = STRBUF_INIT;\n+\tint do_continue = 1;\n \n \tstrbuf_addf(&remotes_arg, \"--remotes=%s\", remotes_name);\n \tinit_revisions(&rev, NULL);\n@@ -407,13 +448,25 @@ 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, commit->parents, &needs_pushing);\n+\twhile ((commit = get_revision(&rev)) && do_continue)\n+\t\tdo_continue = commit_need_pushing(commit, commit->parents, func, data);\n \n \tfree(sha1_copy);\n \tstrbuf_release(&remotes_arg);\n \n-\treturn needs_pushing;\n+\treturn do_continue;\n+}\n+\n+int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name)\n+{\n+\tint needs_push = 0;\n+\tinspect_superproject_commits(new_sha1, remotes_name, submodule_needs_pushing, &needs_push);\n+\treturn needs_push;\n+}\n+\n+void push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name)\n+{\n+\tinspect_superproject_commits(new_sha1, remotes_name, push_submodule, NULL);\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 799c22d..a0074aa 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -30,5 +30,6 @@ 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]);\n int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name);\n+void 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..35820ec 100755\n--- a/t/t5531-deep-submodule-push.sh\n+++ b/t/t5531-deep-submodule-push.sh\n@@ -119,4 +119,28 @@ 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' '\n+\t(\n+\t\tcd work &&\n+\t\tgit checkout master &&\n+\t\tgit push --recurse-submodules=on-demand ../pub.git master\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\t>junk4 &&\n+\t\t\tgit add junk4 &&\n+\t\t\tgit commit -m \"junk4\" &&\n+\t\t\tgit push\n+\t\t) &&\n+\t\tgit add gar/bage &&\n+\t\tgit commit -m \"updated submodule\" &&\n+\t\tgit push --recurse-submodules=on-demand ../pub.git master\n+\t)\n+'\n+\n test_done\ndiff --git a/transport.c b/transport.c\nindex d2725e5..59c90c7 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1046,7 +1046,15 @@ 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,transport->remote->name);\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\tfor (; ref; ref = ref->next)\n \t\t\t\tif (!is_null_sha1(ref->new_sha1) &&\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.6.551.gfb18e\n"},{"id":"173903","messageId":"7vwre9yodc.fsf@alter.siamese.dyndns.org","threadId":"28168","inReplyTo":"1313791728-11328-2-git-send-email-iveqy@iveqy.com","subject":"Re: [PATCH v4 1/2] push: Don't push a repository with unpushed submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-19T23:26:23Z","receivedAt":"2011-08-19T23:26:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fredrik Gustafsson <iveqy@iveqy.com> writes:\n\n> diff --git a/combine-diff.c b/combine-diff.c\n> index b11eb71..f7a8978 100644\n> --- a/combine-diff.c\n> +++ b/combine-diff.c\n> @@ -1074,7 +1074,7 @@ void diff_tree_combined(const unsigned char *sha1,\n>  \t\t * when doing combined diff.\n>  \t\t */\n>  \t\tint stat_opt = (opt->output_format &\n> -\t\t\t\t(DIFF_FORMAT_NUMSTAT|DIFF_FORMAT_DIFFSTAT));\n> +\t\t\t\t(DIFF_FORMAT_NUMSTAT|DIFF_FORMAT_DIFFSTAT|DIFF_FORMAT_CALLBACK));\n>  \t\tif (i == 0 && stat_opt)\n>  \t\t\tdiffopts.output_format = stat_opt;\n>  \t\telse\n\nSorry, but this is not what I meant. With this change, you are running N\n(= number of parents) diffs with the end result, but only making a\ncallback while running a diff with the first parent, and not getting\nanything from comparison with other parents.\n\nThe existing NUMSTAT/STAT exception is only justified because that is how\n\"diff --stat\" shows merges (i.e. showing the extent of damage to the\nmainline, assuming you are viewing a merge to the mainline from a side\nbranch).\n\nWhat I meant was more along the lines of the following, but I think we\nwould need a new kind of callback that can take N-way parents (which is\nnot depicted here).\n\nLet me cook up something and get back to you later tonight.\n\n combine-diff.c |    6 ++++++\n 1 files changed, 6 insertions(+), 0 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 655fa89..51ebd31 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1017,6 +1017,12 @@ void diff_tree_combined(const unsigned char *sha1,\n \t\t\tnum_paths++;\n \t}\n \tif (num_paths) {\n+\t\tif (opt->output_format & DIFF_FORMAT_CALLBACK) {\n+\t\t\tfor (p = paths; p; p = p->next) {\n+\t\t\t\tif (p->len)\n+\t\t\t\t\t... make callback here ...\n+\t\t\t}\n+\t\t}\n \t\tif (opt->output_format & (DIFF_FORMAT_RAW |\n \t\t\t\t\t  DIFF_FORMAT_NAME |\n \t\t\t\t\t  DIFF_FORMAT_NAME_STATUS)) {\n"},{"id":"173910","messageId":"7vippszj70.fsf@alter.siamese.dyndns.org","threadId":"28168","inReplyTo":"7vwre9yodc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4 1/2] push: Don't push a repository with unpushed submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-20T06:32:51Z","receivedAt":"2011-08-20T06:32:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> What I meant was more along the lines of the following, but I think we\n> would need a new kind of callback that can take N-way parents (which is\n> not depicted here).\n>\n> Let me cook up something and get back to you later tonight.\n\nAnd here is a two-patch series to do just that.\n\nThe first one is meant for you to use, and the second one is a sample\napplication of the new machinery.\n\n-- >8 --\nSubject: [PATCH 1/2] combine-diff: support format_callback\n\nThis teaches combine-diff machinery to feed a combined merge to a callback\nfunction when DIFF_FORMAT_CALLBACK is specified.\n\nSo far, format callback functions are not used for anything but 2-way\ndiffs. A callback is given a diff_queue_struct, which is an array of\ndiff_filepair. As its name suggests, a diff_filepair is a _pair_ of\ndiff_filespec that represents a single preimage and a single postimage.\n\nSince \"diff -c\" is to compare N parents with a single merge result and\nfilter out any paths whose result match one (or more) of the parent(s),\nits output has to be able to represent N preimages and 1 postimage. For\nthis reason, a callback function that inspects a diff_filepair that\nresults from this new infrastructure can and is expected to view the\npreimage side (i.e. pair->one) as an array of diff_filespec. Each element\nin the array, except for the last one, is marked with \"has_more_entries\"\nbit, so that the same callback function can be used for 2-way diffs and\ncombined diffs.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n combine-diff.c |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n diffcore.h     |    2 +-\n 2 files changed, 70 insertions(+), 1 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 655fa89..de88186 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -970,6 +970,72 @@ void show_combined_diff(struct combine_diff_path *p,\n \t\tshow_patch_diff(p, num_parent, dense, rev);\n }\n \n+static void free_combined_pair(struct diff_filepair *pair)\n+{\n+\tfree(pair->two);\n+\tfree(pair);\n+}\n+\n+/*\n+ * A combine_diff_path expresses N parents on the LHS against 1 merge\n+ * result. Synthesize a diff_filepair that has N entries on the \"one\"\n+ * side and 1 entry on the \"two\" side.\n+ *\n+ * In the future, we might want to add more data to combine_diff_path\n+ * so that we can fill fields we are ignoring (most notably, size) here,\n+ * but currently nobody uses it, so this should suffice for now.\n+ */\n+static struct diff_filepair *combined_pair(struct combine_diff_path *p,\n+\t\t\t\t\t   int num_parent)\n+{\n+\tint i;\n+\tstruct diff_filepair *pair;\n+\tstruct diff_filespec *pool;\n+\n+\tpair = xmalloc(sizeof(*pair));\n+\tpool = xcalloc(num_parent + 1, sizeof(struct diff_filespec));\n+\tpair->one = pool + 1;\n+\tpair->two = pool;\n+\n+\tfor (i = 0; i < num_parent; i++) {\n+\t\tpair->one[i].path = p->path;\n+\t\tpair->one[i].mode = p->parent[i].mode;\n+\t\thashcpy(pair->one[i].sha1, p->parent[i].sha1);\n+\t\tpair->one[i].sha1_valid = !is_null_sha1(p->parent[i].sha1);\n+\t\tpair->one[i].has_more_entries = 1;\n+\t}\n+\tpair->one[num_parent - 1].has_more_entries = 0;\n+\n+\tpair->two->path = p->path;\n+\tpair->two->mode = p->mode;\n+\thashcpy(pair->two->sha1, p->sha1);\n+\tpair->two->sha1_valid = !is_null_sha1(p->sha1);\n+\treturn pair;\n+}\n+\n+static void handle_combined_callback(struct diff_options *opt,\n+\t\t\t\t     struct combine_diff_path *paths,\n+\t\t\t\t     int num_parent,\n+\t\t\t\t     int num_paths)\n+{\n+\tstruct combine_diff_path *p;\n+\tstruct diff_queue_struct q;\n+\tint i;\n+\n+\tq.queue = xcalloc(num_paths, sizeof(struct diff_filepair *));\n+\tq.alloc = num_paths;\n+\tq.nr = num_paths;\n+\tfor (i = 0, p = paths; p; p = p->next) {\n+\t\tif (!p->len)\n+\t\t\tcontinue;\n+\t\tq.queue[i++] = combined_pair(p, num_parent);\n+\t}\n+\topt->format_callback(&q, opt, opt->format_callback_data);\n+\tfor (i = 0; i < num_paths; i++)\n+\t\tfree_combined_pair(q.queue[i]);\n+\tfree(q.queue);\n+}\n+\n void diff_tree_combined(const unsigned char *sha1,\n \t\t\tconst unsigned char parent[][20],\n \t\t\tint num_parent,\n@@ -1029,6 +1095,9 @@ void diff_tree_combined(const unsigned char *sha1,\n \t\telse if (opt->output_format &\n \t\t\t (DIFF_FORMAT_NUMSTAT|DIFF_FORMAT_DIFFSTAT))\n \t\t\tneedsep = 1;\n+\t\telse if (opt->output_format & DIFF_FORMAT_CALLBACK)\n+\t\t\thandle_combined_callback(opt, paths, num_parent, num_paths);\n+\n \t\tif (opt->output_format & DIFF_FORMAT_PATCH) {\n \t\t\tif (needsep)\n \t\t\t\tputchar(opt->line_termination);\ndiff --git a/diffcore.h b/diffcore.h\nindex b8f1fde..8f32b82 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -45,7 +45,7 @@ struct diff_filespec {\n \tunsigned dirty_submodule : 2;  /* For submodules: its work tree is dirty */\n #define DIRTY_SUBMODULE_UNTRACKED 1\n #define DIRTY_SUBMODULE_MODIFIED  2\n-\n+\tunsigned has_more_entries : 1; /* only appear in combined diff */\n \tstruct userdiff_driver *driver;\n \t/* data should be considered \"binary\"; -1 means \"don't know yet\" */\n \tint is_binary;\n-- \n1.7.6.557.gcee42\n\n \n"},{"id":"173911","messageId":"7vd3g0zj3l.fsf_-_@alter.siamese.dyndns.org","threadId":"28168","inReplyTo":"7vwre9yodc.fsf@alter.siamese.dyndns.org","subject":"[PATCH 2/2] demonstrate format-callback used in combined diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-20T06:34:54Z","receivedAt":"2011-08-20T06:34:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This demonstrates how to use the format-callback machinery added\nto combined diff.\n\n $ ./git demo v1.7.6..maint\n\nworks like \"git log\" with the same revision-range arguments, but shows\nlist of paths that have contents in the child commit different from any of\nits parent commit(s). As a consequence, when a trivial merge takes the\ncontents of a path as a whole from one parent, such a path is not shown.\n\nNotice how the same function can be used to be called back for a two-way\ndiff (i.e. there is only one entry on the preimage \"one\" side) and also\nfor a combined diff.\n\nObviously not meant for inclusion.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin.h     |    1 +\n builtin/log.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++++\n git.c         |    1 +\n 3 files changed, 50 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin.h b/builtin.h\nindex 0e9da90..aef8917 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -59,6 +59,7 @@ extern int cmd_commit(int argc, const char **argv, const char *prefix);\n extern int cmd_commit_tree(int argc, const char **argv, const char *prefix);\n extern int cmd_config(int argc, const char **argv, const char *prefix);\n extern int cmd_count_objects(int argc, const char **argv, const char *prefix);\n+extern int cmd_demo(int argc, const char **argv, const char *prefix);\n extern int cmd_describe(int argc, const char **argv, const char *prefix);\n extern int cmd_diff_files(int argc, const char **argv, const char *prefix);\n extern int cmd_diff_index(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 5c2af59..cc222c8 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -8,6 +8,7 @@\n #include \"color.h\"\n #include \"commit.h\"\n #include \"diff.h\"\n+#include \"diffcore.h\"\n #include \"revision.h\"\n #include \"log-tree.h\"\n #include \"builtin.h\"\n@@ -436,6 +437,53 @@ static void show_rev_tweak_rev(struct rev_info *rev, struct setup_revision_opt *\n \t\trev->diffopt.output_format = DIFF_FORMAT_PATCH;\n }\n \n+static void show_paths_callback(struct diff_queue_struct *q,\n+\t\t\t\tstruct diff_options *options,\n+\t\t\t\tvoid *data)\n+{\n+\tint i;\n+\tfor (i = 0; i < q->nr; i++) {\n+\t\tstruct diff_filepair *pair = q->queue[i];\n+\t\tstruct diff_filespec *spec;\n+\t\tint j;\n+\n+\t\tj = 0;\n+\t\tspec = pair->one;\n+\t\twhile (1) {\n+\t\t\tprintf(\"Parent[%d] %s (%s)\\n\",\n+\t\t\t       j, spec->path, sha1_to_hex(spec->sha1));\n+\t\t\tif (!spec->has_more_entries)\n+\t\t\t\tbreak;\n+\t\t\tj++;\n+\t\t\tspec++;\n+\t\t}\n+\t\tprintf(\"Result    %s (%s)\\n\",\n+\t\t       pair->two->path, sha1_to_hex(pair->two->sha1));\n+\t}\n+}\n+\n+int cmd_demo(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct rev_info rev;\n+\tstruct setup_revision_opt opt;\n+\n+\tinit_revisions(&rev, prefix);\n+\trev.diff = 1;\n+\tmemset(&opt, 0, sizeof(opt));\n+\topt.def = \"HEAD\";\n+\tcmd_log_init(argc, argv, prefix, &rev, &opt);\n+\n+\trev.diffopt.output_format = DIFF_FORMAT_CALLBACK;\n+\trev.diffopt.format_callback = show_paths_callback;\n+\trev.diffopt.format_callback_data = NULL;\n+\trev.diff = 1;\n+\trev.combine_merges = 1;\n+\trev.dense_combined_merges = 0;\n+\trev.ignore_merges = 0;\n+\n+\treturn cmd_log_walk(&rev);\n+}\n+\n int cmd_show(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info rev;\ndiff --git a/git.c b/git.c\nindex 89721d4..34d2381 100644\n--- a/git.c\n+++ b/git.c\n@@ -346,6 +346,7 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"commit-tree\", cmd_commit_tree, RUN_SETUP },\n \t\t{ \"config\", cmd_config, RUN_SETUP_GENTLY },\n \t\t{ \"count-objects\", cmd_count_objects, RUN_SETUP },\n+\t\t{ \"demo\", cmd_demo, RUN_SETUP },\n \t\t{ \"describe\", cmd_describe, RUN_SETUP },\n \t\t{ \"diff\", cmd_diff },\n \t\t{ \"diff-files\", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },\n-- \n1.7.6.557.gcee42\n"},{"id":"173952","messageId":"7vmxf3xnsf.fsf@alter.siamese.dyndns.org","threadId":"28168","inReplyTo":"7vippszj70.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4 1/2] push: Don't push a repository with unpushed submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-21T06:48:48Z","receivedAt":"2011-08-21T06:48:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> What I meant was more along the lines of the following, but I think we\n>> would need a new kind of callback that can take N-way parents (which is\n>> not depicted here).\n>>\n>> Let me cook up something and get back to you later tonight.\n>\n> And here is a two-patch series to do just that.\n>\n> The first one is meant for you to use, and the second one is a sample\n> application of the new machinery.\n>\n> -- >8 --\n> Subject: [PATCH 1/2] combine-diff: support format_callback\n>\n> This teaches combine-diff machinery to feed a combined merge to a callback\n> function when DIFF_FORMAT_CALLBACK is specified.\n\nAfter removing the change to combine-diff.c from your two-patch series, I\napplied them on top of this one, and queued the result in 'pu'.\n\nWhile I tried to be careful while doing this callback-for-combine-diff\npatch so that a callback function written for two-way diff can be used\nwithout any change as long as it does not care about the LHS (i.e. \"one\")\nof the filepair, please double check. I didn't read your change to\nsubmodule.c very carefully (and I didn't have to change it).\n\nThe result seems to pass your new tests ;-).\n"},{"id":"173974","messageId":"20110821215556.GA2370@kolya","threadId":"28168","inReplyTo":"7vd3g0zj3l.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] demonstrate format-callback used in combined diff","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2011-08-21T21:55:57Z","receivedAt":"2011-08-21T21:55:57Z","isPatch":true,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"Thank you. GSoC is now finished and I have examans next week that needs\nmy attention. I will continue finish what I started but won't be able to\ndo so until the end of the weak. Just so you know...\n\n/Fredrik Gustafsson\n"},{"id":"174037","messageId":"20110822194728.GA11745@sandbox-rc","threadId":"28168","inReplyTo":"7vmxf3xnsf.fsf@alter.siamese.dyndns.org","subject":"Re: Re: [PATCH v4 1/2] push: Don't push a repository with unpushed submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2011-08-22T19:47:29Z","receivedAt":"2011-08-22T19:47:29Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nOn Sat, Aug 20, 2011 at 11:48:48PM -0700, Junio C Hamano wrote:\n> After removing the change to combine-diff.c from your two-patch series, I\n> applied them on top of this one, and queued the result in 'pu'.\n> \n> While I tried to be careful while doing this callback-for-combine-diff\n> patch so that a callback function written for two-way diff can be used\n> without any change as long as it does not care about the LHS (i.e. \"one\")\n> of the filepair, please double check. I didn't read your change to\n> submodule.c very carefully (and I didn't have to change it).\n> \n> The result seems to pass your new tests ;-).\n\nVery nice. Today I had a deeper look into the current tests for\non-demand and found a bug in them. Cleaning them up also revealed a bug\nin the current code. Junio could you please squash this[1] in the last\npatch (on-demand option).\n\nI analysed the cause of this bug and it seems that we are not allowed to\niterate revisions using init_revisions() and setup_revisions() more\nthan once. I tracked this down to the SEEN flag in the struct object.\nJunio since you are one person listed in the api docs could you maybe\nquickly explain to me what this flag is used for?\n\nI quickly tried to implement a reset_revision_walk function which will\nreset this flag but it seems that this breaks some expectations in the\ncode since I got a segfault.\n\nCheers Heiko\n\n[1]\n\ndiff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\nindex 35820ec..b0e94f7 100755\n--- a/t/t5531-deep-submodule-push.sh\n+++ b/t/t5531-deep-submodule-push.sh\n@@ -124,6 +124,10 @@ test_expect_success 'push unpushed submodules' '\n \t\tcd work &&\n \t\tgit checkout master &&\n \t\tgit push --recurse-submodules=on-demand ../pub.git master\n+\t\tcd gar/bage &&\n+\t\tgit rev-parse master >expected &&\n+\t\tgit rev-parse origin/master >actual &&\n+\t\ttest_cmp expected actual\n \t)\n '\n \n@@ -132,10 +136,14 @@ test_expect_success 'push unpushed submodules when not needed' '\n \t\tcd work &&\n \t\t(\n \t\t\tcd gar/bage &&\n-\t\t\t>junk4 &&\n-\t\t\tgit add junk4 &&\n-\t\t\tgit commit -m \"junk4\" &&\n-\t\t\tgit push\n+\t\t\tgit checkout master &&\n+\t\t\t>junk5 &&\n+\t\t\tgit add junk5 &&\n+\t\t\tgit commit -m \"junk5\" &&\n+\t\t\tgit push &&\n+\t\t\tgit rev-parse master >expected &&\n+\t\t\tgit rev-parse origin/master >actual &&\n+\t\t\ttest_cmp expected actual\n \t\t) &&\n \t\tgit add gar/bage &&\n \t\tgit commit -m \"updated submodule\" &&\n@@ -143,4 +151,20 @@ test_expect_success 'push unpushed submodules when not needed' '\n \t)\n '\n \n+test_expect_failure 'push unpushed submodules on-demand fails when submodule not pushable' '\n+\t(\n+\t\tcd work &&\n+\t\t(\n+\t\t\tcd gar/bage &&\n+\t\t\tgit checkout HEAD~0 &&\n+\t\t\t>junk6 &&\n+\t\t\tgit add junk6 &&\n+\t\t\tgit commit -m \"junk6\"\n+\t\t) &&\n+\t\tgit add gar/bage &&\n+\t\tgit commit -m \"updated submodule\" &&\n+\t\ttest_must_fail git push --recurse-submodules=on-demand ../pub.git master\n+\t)\n+'\n+\n test_done\n-- \n1.7.6.46.g0f058\n"},{"id":"174050","messageId":"7vd3fxulw8.fsf@alter.siamese.dyndns.org","threadId":"28168","inReplyTo":"20110822194728.GA11745@sandbox-rc","subject":"Re: [PATCH v4 1/2] push: Don't push a repository with unpushed submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-22T22:22:31Z","receivedAt":"2011-08-22T22:22:31Z","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> Junio since you are one person listed in the api docs could you maybe\n> quickly explain to me what this flag is used for?\n\nIt is used in order to avoid walking the object we have walked already.\n\nWhich in turn means that once you walk chain of objects, unless you\nremember the ones you walked and clear the marks after you are done, you\ncannot walk the object chain for unrelated purposes.  See how functions\nlike get_merge_bases_many() walk portions of graph for their own purpose\nand then avoid disrupting others by calling clear_commit_marks(). The use\nof TMP_MARK (and its clearing after the function is done with the marked\nobjects) in remove_duplicate_parents() serve the same purpose.\n"},{"id":"174115","messageId":"20110823194521.GB57187@book.hvoigt.net","threadId":"28168","inReplyTo":"7vd3fxulw8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4 1/2] push: Don't push a repository with unpushed submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2011-08-23T19:45:21Z","receivedAt":"2011-08-23T19:45:21Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Mon, Aug 22, 2011 at 03:22:31PM -0700, Junio C Hamano wrote:\n> Heiko Voigt <hvoigt@hvoigt.net> writes:\n> \n> > Junio since you are one person listed in the api docs could you maybe\n> > quickly explain to me what this flag is used for?\n> \n> It is used in order to avoid walking the object we have walked already.\n> \n> Which in turn means that once you walk chain of objects, unless you\n> remember the ones you walked and clear the marks after you are done, you\n> cannot walk the object chain for unrelated purposes.  See how functions\n> like get_merge_bases_many() walk portions of graph for their own purpose\n> and then avoid disrupting others by calling clear_commit_marks(). The use\n> of TMP_MARK (and its clearing after the function is done with the marked\n> objects) in remove_duplicate_parents() serve the same purpose.\n\nThanks I will have look at those places and try to cook up something.\n\nCheers Heiko\n"},{"id":"174191","messageId":"20110824211431.GH45292@book.hvoigt.net","threadId":"28168","inReplyTo":"7vd3fxulw8.fsf@alter.siamese.dyndns.org","subject":"[WIP PATCH] revision-walking: allow iterating revisions multiple times","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2011-08-24T21:14:31Z","receivedAt":"2011-08-24T21:14:31Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"---\nHi,\n\nOn Mon, Aug 22, 2011 at 03:22:31PM -0700, Junio C Hamano wrote:\n> Heiko Voigt <hvoigt@hvoigt.net> writes:\n> \n> > Junio since you are one person listed in the api docs could you maybe\n> > quickly explain to me what this flag is used for?\n> \n> It is used in order to avoid walking the object we have walked already.\n> \n> Which in turn means that once you walk chain of objects, unless you\n> remember the ones you walked and clear the marks after you are done, you\n> cannot walk the object chain for unrelated purposes.  See how functions\n> like get_merge_bases_many() walk portions of graph for their own purpose\n> and then avoid disrupting others by calling clear_commit_marks(). The use\n> of TMP_MARK (and its clearing after the function is done with the marked\n> objects) in remove_duplicate_parents() serve the same purpose.\n\nWhat do you think about this approach ? Its not yet correctly collecting\nrevisions for all situations but it fixes the demonstrated test failure.\n\n revision.c  |   24 ++++++++++++++++++++++++\n revision.h  |    3 +++\n submodule.c |    3 +++\n 3 files changed, 30 insertions(+), 0 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex c46cfaa..e374c4a 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -500,6 +500,8 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit,\n \t\t\t\tcontinue;\n \t\t\tp->object.flags |= SEEN;\n \t\t\tcommit_list_insert_by_date_cached(p, list, cached_base, cache_ptr);\n+\t\t\tif (revs->fill_reset_list)\n+\t\t\t\tadd_object_array(&p->object, NULL, &revs->walked);\n \t\t}\n \t\treturn 0;\n \t}\n@@ -527,6 +529,8 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit,\n \t\tif (!(p->object.flags & SEEN)) {\n \t\t\tp->object.flags |= SEEN;\n \t\t\tcommit_list_insert_by_date_cached(p, list, cached_base, cache_ptr);\n+\t\t\tif (revs->fill_reset_list)\n+\t\t\t\tadd_object_array(&p->object, NULL, &revs->walked);\n \t\t}\n \t\tif (revs->first_parent_only)\n \t\t\tbreak;\n@@ -1950,6 +1954,23 @@ static void set_children(struct rev_info *revs)\n \t}\n }\n \n+void reset_revision_walk(struct rev_info *revs)\n+{\n+\tint nr = revs->walked.nr;\n+\tstruct object_array_entry *e = revs->walked.objects;\n+\n+\t/* reset the seen flags set by prepare_revision_walk */\n+\twhile (--nr >= 0) {\n+\t\tstruct object *o = e->item;\n+\t\to->flags &= ~(ALL_REV_FLAGS);\n+\t\te++;\n+\t}\n+\tfree(revs->walked.objects);\n+\trevs->walked.nr = 0;\n+\trevs->walked.alloc = 0;\n+\trevs->walked.objects = NULL;\n+}\n+\n int prepare_revision_walk(struct rev_info *revs)\n {\n \tint nr = revs->pending.nr;\n@@ -1964,6 +1985,9 @@ int prepare_revision_walk(struct rev_info *revs)\n \t\tif (commit) {\n \t\t\tif (!(commit->object.flags & SEEN)) {\n \t\t\t\tcommit->object.flags |= SEEN;\n+\t\t\t\tif (revs->fill_reset_list)\n+\t\t\t\t\tadd_object_array(&commit->object, NULL,\n+\t\t\t\t\t\t\t &revs->walked);\n \t\t\t\tcommit_list_insert_by_date(commit, &revs->commits);\n \t\t\t}\n \t\t}\ndiff --git a/revision.h b/revision.h\nindex 3d64ada..6a0fa99 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -28,6 +28,7 @@ struct rev_info {\n \t/* Starting list */\n \tstruct commit_list *commits;\n \tstruct object_array pending;\n+\tstruct object_array walked;\n \n \t/* Parents of shown commits */\n \tstruct object_array boundary_commits;\n@@ -72,6 +73,7 @@ struct rev_info {\n \t\t\tbisect:1,\n \t\t\tancestry_path:1,\n \t\t\tfirst_parent_only:1;\n+\tunsigned int\tfill_reset_list:1;\n \n \t/* Diff flags */\n \tunsigned int\tdiff:1,\n@@ -169,6 +171,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(struct rev_info *revs);\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 dc95498..410d8e4 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -441,6 +441,7 @@ static int inspect_superproject_commits(unsigned char new_sha1[20], const char *\n \n \tstrbuf_addf(&remotes_arg, \"--remotes=%s\", remotes_name);\n \tinit_revisions(&rev, NULL);\n+\trev.fill_reset_list = 1;\n \tsha1_copy = xstrdup(sha1_to_hex(new_sha1));\n \targv[1] = sha1_copy;\n \targv[3] = remotes_arg.buf;\n@@ -451,6 +452,8 @@ static int inspect_superproject_commits(unsigned char new_sha1[20], const char *\n \twhile ((commit = get_revision(&rev)) && do_continue)\n \t\tdo_continue = commit_need_pushing(commit, commit->parents, func, data);\n \n+\n+\treset_revision_walk(&rev);\n \tfree(sha1_copy);\n \tstrbuf_release(&remotes_arg);\n \n-- \n1.7.6.553.g84dc\n"},{"id":"174196","messageId":"7vhb56o56h.fsf@alter.siamese.dyndns.org","threadId":"28168","inReplyTo":"20110824211431.GH45292@book.hvoigt.net","subject":"Re: [WIP PATCH] revision-walking: allow iterating revisions multiple times","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-24T21:44:38Z","receivedAt":"2011-08-24T21:44:38Z","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> +void reset_revision_walk(struct rev_info *revs)\n> +{\n> +\tint nr = revs->walked.nr;\n> +\tstruct object_array_entry *e = revs->walked.objects;\n> +\n> +\t/* reset the seen flags set by prepare_revision_walk */\n> +\twhile (--nr >= 0) {\n> +\t\tstruct object *o = e->item;\n> +\t\to->flags &= ~(ALL_REV_FLAGS);\n> +\t\te++;\n> +\t}\n> +\tfree(revs->walked.objects);\n> +\trevs->walked.nr = 0;\n> +\trevs->walked.alloc = 0;\n> +\trevs->walked.objects = NULL;\n> +}\n\nI am afraid that this is not good enough for general purpose.  The object\nyou walk in the middle of doing something may have been marked for reasons\nother than your extra walking before you started your walk. Imagine\n\n * The command takes arguments like rev-list does;\n\n * It calls setup_revisions(), which marks commits given from the command\n   line with marks like UNINTERESTING, and then prepare_revision_walk();\n\n * It walks the commit graph and does interesting things on commits that\n   it discovers, by repeatedly calling get_revision(), e.g.:\n\n   \twhile ((commit = get_revision()) != NULL) {\n\t\tdo_something_interesting(commit);\n        }\n\nNow, you add a new caller that walks the commit graph for a different\nreason from the primary revision walking done by the command somewhere\ndown in the callchain of do_something_interesting()---obviously you cannot\nuse the above reset_revision_walk() to clean things up, as it will break\nthe outer revision walk.\n\nIf on the other hand you will _never_ have more than one revision walk\ngoing on, it may amount to the same thing to iterate over the object array\nand clear all the flags.\n\nTraditionally the way to do nested revision walk that can potentially be\ndone more than once (but never having such a sub-walk in parallel) was to\nremember the start points of the subwalk, use private marks that are not\nused in the outer walk during the subwalk, and call clear_commit_marks()\non these start points when a subwalk is done to clear only the marks the\nsubwalk used.\n"},{"id":"174763","messageId":"7vmxemls8z.fsf@alter.siamese.dyndns.org","threadId":"28168","inReplyTo":"1313791728-11328-3-git-send-email-iveqy@iveqy.com","subject":"Re: [PATCH v4 2/2] push: teach --recurse-submodules the on-demand option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-02T18:21:48Z","receivedAt":"2011-09-02T18:21:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fredrik Gustafsson <iveqy@iveqy.com> writes:\n\n> diff --git a/submodule.c b/submodule.c\n> index 45f508c..dc95498 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -8,7 +8,10 @@\n>  #include \"diffcore.h\"\n>  #include \"refs.h\"\n>  #include \"string-list.h\"\n> +#include \"transport.h\"\n>  \n> +typedef int (*needs_push_func_t)(const char *path, const unsigned char sha1[20],\n> +\t\tvoid *data);\n>  static struct string_list config_name_for_path;\n>  static struct string_list config_fetch_recurse_submodules_for_name;\n>  static struct string_list config_ignore_for_name;\n> @@ -308,21 +311,24 @@ void set_config_fetch_recurse_submodules(int value)\n>  \tconfig_fetch_recurse_submodules = value;\n>  }\n>  \n> +typedef int (*module_func_t)(const char *path, const unsigned char sha1[20], void *data);\n> +\n>  static int has_remote(const char *refname, const unsigned char *sha1, int flags, void *cb_data)\n>  {\n>  \treturn 1;\n>  }\n>  \n> -static int submodule_needs_pushing(const char *path, const unsigned char sha1[20])\n> +int submodule_needs_pushing(const char *path, const unsigned char sha1[20], void *data)\n>  {\n> +\tint *needs_pushing = data;\n> +\n>  \tif (add_submodule_odb(path) || !lookup_commit_reference(sha1))\n> -\t\treturn 0;\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[] = {\"rev-list\", NULL, \"--not\", \"--remotes\", \"-n\", \"1\" , NULL};\n>  \t\tstruct strbuf buf = STRBUF_INIT;\n> -\t\tint needs_pushing = 0;\n>  \n>  \t\targv[1] = sha1_to_hex(sha1);\n>  \t\tmemset(&cp, 0, sizeof(cp));\n> @@ -336,41 +342,74 @@ static int submodule_needs_pushing(const char *path, const unsigned char sha1[20\n>  \t\t\tdie(\"Could not run 'git rev-list %s --not --remotes -n 1' command in submodule %s\",\n>  \t\t\t\tsha1_to_hex(sha1), path);\n>  \t\tif (strbuf_read(&buf, cp.out, 41))\n> -\t\t\tneeds_pushing = 1;\n> +\t\t\t*needs_pushing = 1;\n>  \t\tfinish_command(&cp);\n>  \t\tclose(cp.out);\n>  \t\tstrbuf_release(&buf);\n> -\t\treturn needs_pushing;\n> +\t\treturn !*needs_pushing;\n>  \t}\n> -\treturn 0;\n> +\treturn 1;\n> +}\n\nIt appears to me that this patch is flipping the meaning of the function,\nand the returned value from here is no longer \"do we know that this\nsubmodule needs to be pushed (yes/no)?\".  The function needs to be renamed\nto describe what it does better.\n\nAlso you would need to give a comment before the function to describe the\nsemantics of these two return values (one from the function, the other\nfrom the value placed via the callback data pointer).\n\nThe latter is especially important because the caller that gets 1 from\nthis function would not be able to tell if the value in the callback data\npointer is valid (only happens if \"rev-list\" said something) or undefined\n(no assignment is ever done via *needs_pushing pointer to zero it when\n\"rev-list\" is silent, or if no submodule is checked out at path).\n\n> +int push_submodule(const char *path, const unsigned char sha1[20], void *data)\n> +{\n> +\tif (add_submodule_odb(path) || !lookup_commit_reference(sha1))\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.out = -1;\n\nIs this correct? Nobody seems to read from this pipe from the \"git push\"\noutput. Don't you either want to send it to the end user, or squelch it by\nsending it to /dev/null? You could of course read its output between the\nfollowing run_command() and close() and do something intelligent depending\non what the command tells you, if you wanted to, but I somehow doubt that\nis what you had in mind here...\n\n> +\t\tcp.dir = path;\n> +\t\tif (run_command(&cp))\n> +\t\t\tdie(\"Could not run 'git push' command in submodule %s\", path);\n> +\t\tclose(cp.out);\n> +\t}\n> +\n> +\treturn 1;\n>  }\n\nDo you really want to \"die\" here? You would definitely do if the failure\nwas due to corruption of your submodule repository, but wouldn't you want\nto continue pushing other submodules if you couldn't push this submodule\ndue to non-fast-forward (i.e. somebody else pushed there first), for\nexample?\n\n> +struct collect_submodules_data {\n> +\tmodule_func_t func;\n> +\tvoid *data;\n> +\tint ret;\n> +};\n\nWhat are the meaning of these fields? Document them.\n\nDo you really need a double indirection like this, I wonder...\n\n>  static void collect_submodules_from_diff(struct diff_queue_struct *q,\n>  \t\t\t\t\t struct diff_options *options,\n>  \t\t\t\t\t void *data)\n>  {\n>  \tint i;\n> -\tint *needs_pushing = data;\n> +\tstruct collect_submodules_data *me = 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\tif (!(me->ret = me->func(p->two->path, p->two->sha1, me->data)))\n>  \t\t\tbreak;\n> -\t\t}\n>  \t}\n>  }\n>  \n> -\n> -static void commit_need_pushing(struct commit *commit, struct commit_list *parent, int *needs_pushing)\n> +static int commit_need_pushing(struct commit *commit, struct commit_list *parent,\n> +\tmodule_func_t func, void *data)\n>  {\n>  \tconst unsigned char (*parents)[20];\n>  \tunsigned int i, n;\n>  \tstruct rev_info rev;\n>  \n> +\tstruct collect_submodules_data cb;\n> +\tcb.func = func;\n\nJust a style thing, but because we do not allow decl-after-statement, it\nis customary to have the blank line _after_ the last decl, not in between\nthe declarations.\n\n> +\tcb.data = data;\n> +\tcb.ret = 1;\n> +\n>  \tn = commit_list_count(parent);\n>  \tparents = xmalloc(n * sizeof(*parents));\n>  \n> @@ -382,21 +421,23 @@ static void commit_need_pushing(struct commit *commit, struct commit_list *paren\n>  \tinit_revisions(&rev, NULL);\n>  \trev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n>  \trev.diffopt.format_callback = collect_submodules_from_diff;\n> -\trev.diffopt.format_callback_data = needs_pushing;\n> +\trev.diffopt.format_callback_data = &cb;\n>  \tdiff_tree_combined(commit->object.sha1, parents, n, 1, &rev);\n>  \n>  \tfree(parents);\n> +\treturn cb.ret;\n>  }\n>  \n> -int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name)\n> +static int inspect_superproject_commits(unsigned char new_sha1[20], const char *remotes_name,\n> +\tmodule_func_t func, void *data)\n\nContrast your new name with \"check-submodule-needs-pushing\".  \"inspect\"\n(or \"check\" for that matter) is a poor word to use in function names, as\nthe word by itself does not convey what aspect of the object of the verb\nis being inspected or checked. The old name was fine because other words\nin the name described what it was checking. The new name does not tell us\nanything useful. First try to explain to yourself at high level what the\nfunction does in a few lines, and then a more appropriate name would come\nto you.\n\n>  {\n>  \tstruct rev_info rev;\n>  \tstruct commit *commit;\n>  \tconst char *argv[] = {NULL, NULL, \"--not\", \"NULL\", NULL};\n\nWhat is this string \"NULL\" doing here???\n\n>  \tint argc = ARRAY_SIZE(argv) - 1;\n>  \tchar *sha1_copy;\n> -\tint needs_pushing = 0;\n>  \tstruct strbuf remotes_arg = STRBUF_INIT;\n> +\tint do_continue = 1;\n>  \n>  \tstrbuf_addf(&remotes_arg, \"--remotes=%s\", remotes_name);\n>  \tinit_revisions(&rev, NULL);\n> @@ -407,13 +448,25 @@ 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, commit->parents, &needs_pushing);\n> +\twhile ((commit = get_revision(&rev)) && do_continue)\n> +\t\tdo_continue = commit_need_pushing(commit, commit->parents, func, data);\n\nA funny way to write\n\n\twhite ((commit = get_revision(&rev)) != NULL) {\n               if (!commit_need_pushing(commit, commit->parents, func, data))\n\t\t\tbreak;\n\t}\n\nor even:\n\n\twhite ((commit = get_revision(&rev)) != NULL &&\n        \tcommit_need_pushing(commit, commit->parents, func, data))\n\t\t ; /* nothing */\n\nNo caller of this function uses its return value (one caller uses its\nreturn value left in \"data\" pointer), so I do not think you would need the\n\"do_continue\" variable, which is misnamed (the name makes sense only as\nthe loop control inside this function, but does not make any sense as the\nreturn value from this function---it does not tell the caller to continue).\n\nAs there is only this calling site of commit_need_pushing(), I wonder why\nthe function needs to be able to take commit and commit->parents as\nseparate parameters. Does it even make sense in other contexts to compare\na commit with list of commits that are not its parents and decide if the\ncommit needs pushing based on that comparison?\n\n>  \n>  \tfree(sha1_copy);\n>  \tstrbuf_release(&remotes_arg);\n>  \n> -\treturn needs_pushing;\n> +\treturn do_continue;\n> +}\n> +\n> +int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name)\n> +{\n> +\tint needs_push = 0;\n> +\tinspect_superproject_commits(new_sha1, remotes_name, submodule_needs_pushing, &needs_push);\n> +\treturn needs_push;\n> +}\n> +\n> +void push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name)\n> +{\n> +\tinspect_superproject_commits(new_sha1, remotes_name, push_submodule, NULL);\n>  }\n\nAgain the called function is misnamed as the primary purpose it is used is not\nto inspect, but to cause effects. But more importantly...\n\nWhat does this do, given the loop structure of inspect_superproject_commits()? \nIf your superproject is three commits ahead of the remote, the get_revision()\nloop may run three times, calling commit_need_pushing() and have it inspect\nthese superproject commits, and may find that you bound different commits\nfrom the same submodule multiple times during these three superproject commits.\nDon't you end up running \"git push\" multiple times?\n\nI have to say the overall code struction of this patch is simply broken.\n\nHow about doing it this way instead?\n\n - Update check-submodule-needs-pushing that used to stop at the first\n   submodule that are not up-to-date not to do that. Instead, loop over\n   all the submodules, find and collect which ones needs pushing, and\n   return it as a list of submodules. Make sure you have the same\n   submodule appear at most once in the result.  You may want to rename it\n   to reflect the new role of the function (i.e. collecting submodules\n   that needs to be pushed). Perhaps collect_stale_submodules() or\n   something.\n\n   For this, I do not think you need to touch the implementation of the\n   submodule_needs_pushing() function at all. You do not need to introduce\n   the indirection such as module_func_t and collect_submodules_data\n   either. The only change needed is to collect_submodules_from_diff()\n   that would treat the callback data not as a pointer to int\n   (needs-pushing), but as a pointer to the structure to collect the names\n   of submodules that need to be pushed (e.g. \"struct string_list\"), and\n   make it not break the loop upon the first submodule that is stale.\n\n - Instead of adding a call to push-unpushed-submodules before\n   check-submodule-needs-pushing in transport_push(), first call\n   check-submodule-needs-pushing when on-demand or check is in effect.\n\n   When in check mode, if check-submodule-needs-pushing returned a\n   non-empty list, report which ones are stale and die.\n\n   If in on-demand mode, you have a list of submodules you need to run\n   \"git push\" in. Iterate over that list and do your push_submodule().\n   You may want to reconsider your \"die()\" there, though.\n\nHmm?\n"},{"id":"177904","messageId":"7vr52bjljd.fsf@alter.siamese.dyndns.org","threadId":"28168","inReplyTo":"20111017190749.GA3126@sandbox-rc","subject":"Re: [PATCH v4 2/2] push: teach --recurse-submodules the on-demand option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-17T22:33:26Z","receivedAt":"2011-10-17T22:33:26Z","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> since we have not heard anything from Fredrik I will probably look into\n> cleaning this up. Should I do that with follow-up patches since this\n> patch is already in next?\n\nI thought we kicked it back to 'pu' after 1.7.7 cycle.\n\nI would personally want to put a freeze on \"recursively do anything to\nsubmodule\" topic (including but not limited to \"checkout\") for now, until\nwe know how we would want to support \"floating submodule\" model. For\nexisting code in-flight, I would like to see us at least have a warm and\nfuzzy feeling that we know which part of the code such a support would\nneed to undo and how the update would look like before moving forward.\n\nThere are two camps that use submodules in their large-ish projects.\n\nOne is mostly happy with the traditional \"submodule trees checked out must\nmatch what the superproject says, otherwise you have local changes and the\nbuild product cannot be called to have emerged from that particular\nsuperproject commit\" model. Let's call this \"exact submodules\" model.\n\nThe other prefers \"submodule trees checked out are whatever submodule\ncommits that happen to sit at the tips of the designated branches the\nsuperproject wants to use\" model. The superproject tree does not exactly\nknow or care what commit to use from each of its submodules, and I would\nimagine that it may be more convenient for developers. They do not have to\ncare the entire build product while they commit---only the integration\nprocess that could be separate and perhaps automated needs to know.\n\nWe haven't given any explicit support to the latter \"floating submodules\"\nmodel so far. There may be easy workarounds to many of the potential\nissues, (e.g. at \"git diff/status\" level, there may be some configuration\nvariables to tell the tools to ignore differences between the commit the\nsuperproject records for the submodule path and the HEAD in the\nsubmodule), but with recent work on submodules such as \"allow pushing\nsuperproject only after submodule commits are pushed out\", I am afraid\nthat we seem to be piling random new things with the assumption that we\nwould never support anything but \"exact submodules\" model. Continuing the\ndevelopment that way would require retrofitting support for \"floating\nsubmodules\" model to largely undo the unwarranted assumptions existing\ncode makes. That is the reason why I would like to see people think about\nthe need to support the other \"floating submodules\" model, before making\nthe existing mess even worse.\n\nThe very first step for floating submodules support would be relatively\nsimple. We could declare that an entry in the .gitmodules file in the\nsuperproject can optionally specify which branch needs to be checked out\nwith something like:\n\n\t[submodule \"libfoo\"]\n\t\tbranch = master\n                path = include/foo\n                url = git://foo.com/git/lib.git\n                \nand when such an entry is defined, a command at the superproject level\nwould largely ignore what is at include/foo in the tree object recorded in\nthe superproject commit and in the index. When we show \"git status\" in the\nsuperproject, instead of using the commit bound to the superproject, we\nwould use include/foo/.git/HEAD as the basis for detecting \"local\" changes\nto the submodule. We could even declare that the gitlink for such a\nsubmodule should record 0{40} SHA-1 in the superproject, but I do not\nthink that is necessary.\n"},{"id":"177969","messageId":"4E9DE883.9050105@web.de","threadId":"28168","inReplyTo":"7vr52bjljd.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4 2/2] push: teach --recurse-submodules the on-demand option","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2011-10-18T20:58:43Z","receivedAt":"2011-10-18T20:58:43Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 18.10.2011 00:33, schrieb Junio C Hamano:\n> I would personally want to put a freeze on \"recursively do anything to\n> submodule\" topic (including but not limited to \"checkout\") for now, until\n> we know how we would want to support \"floating submodule\" model. For\n> existing code in-flight, I would like to see us at least have a warm and\n> fuzzy feeling that we know which part of the code such a support would\n> need to undo and how the update would look like before moving forward.\n\nMakes sense.\n\n> There are two camps that use submodules in their large-ish projects.\n> \n> One is mostly happy with the traditional \"submodule trees checked out must\n> match what the superproject says, otherwise you have local changes and the\n> build product cannot be called to have emerged from that particular\n> superproject commit\" model. Let's call this \"exact submodules\" model.\n> \n> The other prefers \"submodule trees checked out are whatever submodule\n> commits that happen to sit at the tips of the designated branches the\n> superproject wants to use\" model. The superproject tree does not exactly\n> know or care what commit to use from each of its submodules, and I would\n> imagine that it may be more convenient for developers. They do not have to\n> care the entire build product while they commit---only the integration\n> process that could be separate and perhaps automated needs to know.\n>\n> We haven't given any explicit support to the latter \"floating submodules\"\n> model so far. There may be easy workarounds to many of the potential\n> issues, (e.g. at \"git diff/status\" level, there may be some configuration\n> variables to tell the tools to ignore differences between the commit the\n> superproject records for the submodule path and the HEAD in the\n> submodule), but with recent work on submodules such as \"allow pushing\n> superproject only after submodule commits are pushed out\", I am afraid\n> that we seem to be piling random new things with the assumption that we\n> would never support anything but \"exact submodules\" model.\n\nIt's not about never supporting anything else, but right now we are\nscratching our own itch ;-)\n\n> Continuing the\n> development that way would require retrofitting support for \"floating\n> submodules\" model to largely undo the unwarranted assumptions existing\n> code makes. That is the reason why I would like to see people think about\n> the need to support the other \"floating submodules\" model, before making\n> the existing mess even worse.\n\nIf you configure diff.ignoreSubmodules=all and fetch.recurseSubmodules=false\nand write a script fetching and checking out the branch(es) of your choice\nin the submodule(s) you run each time you want to update the branch tip\nthere, you should be almost there with current Git. But yes, we could do\nbetter.\n\n> The very first step for floating submodules support would be relatively\n> simple. We could declare that an entry in the .gitmodules file in the\n> superproject can optionally specify which branch needs to be checked out\n> with something like:\n> \n> \t[submodule \"libfoo\"]\n> \t\tbranch = master\n>                 path = include/foo\n>                 url = git://foo.com/git/lib.git\n>                 \n> and when such an entry is defined, a command at the superproject level\n> would largely ignore what is at include/foo in the tree object recorded in\n> the superproject commit and in the index. When we show \"git status\" in the\n> superproject, instead of using the commit bound to the superproject, we\n> would use include/foo/.git/HEAD as the basis for detecting \"local\" changes\n> to the submodule.\n\nYup. And the presence of the \"branch\" config could tell \"git submodule\nupdate\" to fetch and advance that branch to the tip every time it is run.\nAnd it could tell the diff machinery (which is also used by status) to\nignore the differences between a submodule's HEAD and the SHA-1 in the\nsuperproject (while still allowing to silence the presence of untracked\nand/or modified files by using the diff.ignoreSubmodules option) and\nfetch would just stop doing any on-demand action for such submodules.\nAnything I missed?\n\n> We could even declare that the gitlink for such a\n> submodule should record 0{40} SHA-1 in the superproject, but I do not\n> think that is necessary.\n\nMe neither, e.g. the SHA-1 which was the submodules HEAD when it was added\nshould do nicely. And that would avoid referencing a non-existing commit\nin case you later want to turn a floating submodule into an exact one.\n"},{"id":"180971","messageId":"CABURp0okOmsk4JV9Ku5pHJb5vT-kr_fmweNNBKZ_OoRyfZan=Q@mail.gmail.com","threadId":"28168","inReplyTo":"4E9DE883.9050105@web.de","subject":"Re: [PATCH v4 2/2] push: teach --recurse-submodules the on-demand option","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2011-12-12T21:16:19Z","receivedAt":"2011-12-12T21:16:19Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"On Tue, Oct 18, 2011 at 4:58 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:\n> Am 18.10.2011 00:33, schrieb Junio C Hamano:\n>> The very first step for floating submodules support would be relatively\n>> simple. We could declare that an entry in the .gitmodules file in the\n>> superproject can optionally specify which branch needs to be checked out\n>> with something like:\n>>\n>>       [submodule \"libfoo\"]\n>>               branch = master\n>>                 path = include/foo\n>>                 url = git://foo.com/git/lib.git\n>>\n>> and when such an entry is defined, a command at the superproject level\n>> would largely ignore what is at include/foo in the tree object recorded in\n>> the superproject commit and in the index. When we show \"git status\" in the\n>> superproject, instead of using the commit bound to the superproject, we\n>> would use include/foo/.git/HEAD as the basis for detecting \"local\" changes\n>> to the submodule.\n>\n> Yup. And the presence of the \"branch\" config could tell \"git submodule\n> update\" to fetch and advance that branch to the tip every time it is run.\n> And it could tell the diff machinery (which is also used by status) to\n> ignore the differences between a submodule's HEAD and the SHA-1 in the\n> superproject (while still allowing to silence the presence of untracked\n> and/or modified files by using the diff.ignoreSubmodules option) and\n> fetch would just stop doing any on-demand action for such submodules.\n> Anything I missed?\n>\n>> We could even declare that the gitlink for such a\n>> submodule should record 0{40} SHA-1 in the superproject, but I do not\n>> think that is necessary.\n>\n> Me neither, e.g. the SHA-1 which was the submodules HEAD when it was added\n> should do nicely. And that would avoid referencing a non-existing commit\n> in case you later want to turn a floating submodule into an exact one.\n\n\nI'm sorry I missed this comment before.\n\nI hope we can allow storing the actual gitlink in the superproject for\neach commit even when we're using floating submodules.  I\nthought-experimented with this a bit last year and came to the\nconclusion that I should be able to 'float' to tips (developer\nconvenience) and also to store the SHA-1 of each gitlink through\nhistory (automated maybe; as-needed).\n\nThe problem with \"float-only\" is that it loses history so, for\nexample, git-bisect doesn't work.\n\nThe problem with \"float + gitlinks\", of course, is that it looks like\n\"not floating\" to the developers (git-status is dirty unless\noverridden, etc.)\n\nIs there a deeper reason this wouldn't be possible?\n\nPhil\n"},{"id":"180983","messageId":"4EE6805D.7020708@web.de","threadId":"28168","inReplyTo":"CABURp0okOmsk4JV9Ku5pHJb5vT-kr_fmweNNBKZ_OoRyfZan=Q@mail.gmail.com","subject":"Re: [PATCH v4 2/2] push: teach --recurse-submodules the on-demand option","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2011-12-12T22:29:49Z","receivedAt":"2011-12-12T22:29:49Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 12.12.2011 22:16, schrieb Phil Hord:\n> On Tue, Oct 18, 2011 at 4:58 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:\n>> Am 18.10.2011 00:33, schrieb Junio C Hamano:\n>>> We could even declare that the gitlink for such a\n>>> submodule should record 0{40} SHA-1 in the superproject, but I do not\n>>> think that is necessary.\n>>\n>> Me neither, e.g. the SHA-1 which was the submodules HEAD when it was added\n>> should do nicely. And that would avoid referencing a non-existing commit\n>> in case you later want to turn a floating submodule into an exact one.\n> \n> \n> I'm sorry I missed this comment before.\n> \n> I hope we can allow storing the actual gitlink in the superproject for\n> each commit even when we're using floating submodules.\n\nI think you misread my statement, I was just talking about the initial\ncommit containing the newly added submodule, not any subsequent ones.\nFloating makes differences between the original SHA-1 and the current\ntip of the branch invisible, so there is nothing to commit.\n\n>  I thought-experimented with this a bit last year and came to the\n> conclusion that I should be able to 'float' to tips (developer\n> convenience) and also to store the SHA-1 of each gitlink through\n> history (automated maybe; as-needed).\n\nWhich means that after \"git submodule update\" floated a submodule branch\nfurther, you would have to commit that in the superproject.\n\n> The problem with \"float-only\" is that it loses history so, for\n> example, git-bisect doesn't work.\n\nYep. And different developers can have the same superproject commit\nchecked out but their submodules can be quite different.\n\n> The problem with \"float + gitlinks\", of course, is that it looks like\n> \"not floating\" to the developers (git-status is dirty unless\n> overridden, etc.)\n\nYeah. But what if each \"git submodule update\" would update the tip of\nthe submodule branch and add that to the superproject? You could follow\na tip but still produce reproducible trees.\n"},{"id":"180995","messageId":"CABURp0qkKXCW-U=78OpnejdtdpphhJtOoDubz77m7Gt3o5sC=Q@mail.gmail.com","threadId":"28168","inReplyTo":"4EE6805D.7020708@web.de","subject":"Re: [PATCH v4 2/2] push: teach --recurse-submodules the on-demand option","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2011-12-12T23:50:34Z","receivedAt":"2011-12-12T23:50:34Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"On Mon, Dec 12, 2011 at 5:29 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:\n> Am 12.12.2011 22:16, schrieb Phil Hord:\n>> On Tue, Oct 18, 2011 at 4:58 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:\n>>> Am 18.10.2011 00:33, schrieb Junio C Hamano:\n>>>> We could even declare that the gitlink for such a\n>>>> submodule should record 0{40} SHA-1 in the superproject, but I do not\n>>>> think that is necessary.\n>>>\n>>> Me neither, e.g. the SHA-1 which was the submodules HEAD when it was added\n>>> should do nicely. And that would avoid referencing a non-existing commit\n>>> in case you later want to turn a floating submodule into an exact one.\n>>\n>>\n>> I'm sorry I missed this comment before.\n>>\n>> I hope we can allow storing the actual gitlink in the superproject for\n>> each commit even when we're using floating submodules.\n>\n> I think you misread my statement, I was just talking about the initial\n> commit containing the newly added submodule, not any subsequent ones.\n> Floating makes differences between the original SHA-1 and the current\n> tip of the branch invisible, so there is nothing to commit.\n>\n>>  I thought-experimented with this a bit last year and came to the\n>> conclusion that I should be able to 'float' to tips (developer\n>> convenience) and also to store the SHA-1 of each gitlink through\n>> history (automated maybe; as-needed).\n>\n> Which means that after \"git submodule update\" floated a submodule branch\n> further, you would have to commit that in the superproject.\n\nSadly, yes.  Currently I have my CI-server do this for me after it\nverifies each new submodule commit is able to build successfully.\n\n>> The problem with \"float-only\" is that it loses history so, for\n>> example, git-bisect doesn't work.\n>\n> Yep. And different developers can have the same superproject commit\n> checked out but their submodules can be quite different.\n\n>> The problem with \"float + gitlinks\", of course, is that it looks like\n>> \"not floating\" to the developers (git-status is dirty unless\n>> overridden, etc.)\n>\n> Yeah. But what if each \"git submodule update\" would update the tip of\n> the submodule branch and add that to the superproject? You could follow\n> a tip but still produce reproducible trees.\n\nYes, and that's what I want.\n\nNot what it sounded like was being suggested before, which (to my\neyes) implied that the submodule gitlinks were useless noise.\n\nPhil\n"},{"id":"181028","messageId":"4EE7115B.8040000@web.de","threadId":"28168","inReplyTo":"CABURp0qkKXCW-U=78OpnejdtdpphhJtOoDubz77m7Gt3o5sC=Q@mail.gmail.com","subject":"Re: [PATCH v4 2/2] push: teach --recurse-submodules the on-demand option","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2011-12-13T08:48:27Z","receivedAt":"2011-12-13T08:48:27Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 13.12.2011 00:50, schrieb Phil Hord:\n> On Mon, Dec 12, 2011 at 5:29 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:\n>> Am 12.12.2011 22:16, schrieb Phil Hord:\n>>>  I thought-experimented with this a bit last year and came to the\n>>> conclusion that I should be able to 'float' to tips (developer\n>>> convenience) and also to store the SHA-1 of each gitlink through\n>>> history (automated maybe; as-needed).\n>>\n>> Which means that after \"git submodule update\" floated a submodule branch\n>> further, you would have to commit that in the superproject.\n> \n> Sadly, yes.  Currently I have my CI-server do this for me after it\n> verifies each new submodule commit is able to build successfully.\n\nWhich I think is a good thing to do, as you have a good chance of\ncatching breakage introduced by the submodule updates. \"float-only\"\nsubmodules won't always be a pleasant experience, as they can (and\nsometimes will) get you into trouble when advancing them introduces\nbugs (and then you can't even bisect that breakage).\n\n>>> The problem with \"float + gitlinks\", of course, is that it looks like\n>>> \"not floating\" to the developers (git-status is dirty unless\n>>> overridden, etc.)\n>>\n>> Yeah. But what if each \"git submodule update\" would update the tip of\n>> the submodule branch and add that to the superproject? You could follow\n>> a tip but still produce reproducible trees.\n> \n> Yes, and that's what I want.\n> \n> Not what it sounded like was being suggested before, which (to my\n> eyes) implied that the submodule gitlinks were useless noise.\n\nIt was suggested in other threads in the past. For a start, you could\nwrite a script doing that and play around with it. And if that works\nwell for you, we can discuss if implementing that functionality into\n\"git submodule update\" makes sense.\n"}]}