{"thread":{"id":"28055","subject":"[PATCH] push: Don't push a repository with unpushed submodules","startedAt":"2011-08-09T18:15:52Z","lastAt":"2011-08-09T21:12:05Z","messageCount":2,"participants":["Fredrik Gustafsson","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"173212","messageId":"1312913752-21555-1-git-send-email-iveqy@iveqy.com","threadId":"28055","inReplyTo":null,"subject":"[PATCH] push: Don't push a repository with unpushed submodules","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2011-08-09T18:15:52Z","receivedAt":"2011-08-09T18:15:52Z","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\nThis does not guarantee that all submodules a super-project\nneeds will be available on the server. In that case both the\nsuper-project and the submodules would need an atomic push. This\ndoes however prevent the human error of forgetting to push a\nsubmodule.\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---\nWe decided to make this a one patch series since by the default\nthe check is turned off.\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\nA new test is added in this iteration to show a bug that now is fixed.\n\n Documentation/git-push.txt     |    6 ++\n builtin/push.c                 |   17 +++++\n submodule.c                    |  129 ++++++++++++++++++++++++++++++++++++++++\n submodule.h                    |    1 +\n t/t5531-deep-submodule-push.sh |   99 ++++++++++++++++++++++++++++++\n transport.c                    |    9 +++\n transport.h                    |    1 +\n 7 files changed, 262 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex 88acfcd..6ae6ba3 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..23ba365 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,19 @@ 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}\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 +250,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/submodule.c b/submodule.c\nindex 1ba9646..c44472f 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -308,6 +308,135 @@ 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+/*\n+ * checks whether a certain submodule commit is pushed based on the\n+ * submodules remote tracking branches. returns 1 in case no remote\n+ * tracking branches are found.\n+ */\n+static int is_submodule_commit_pushed(const char *path, const unsigned char sha1[20])\n+{\n+\tint is_pushed = 0;\n+\tif (!add_submodule_odb(path) && lookup_commit_reference(sha1)) {\n+\t\tif (for_each_remote_ref_submodule(path, has_remote, NULL)) {\n+\t\t\tstruct child_process cp;\n+\t\t\tconst char *argv[] = {\"rev-list\", NULL, \"--not\", \"--remotes\", \"-n\", \"1\" , NULL};\n+\t\t\tstruct strbuf buf = STRBUF_INIT;\n+\n+\t\t\targv[1] = sha1_to_hex(sha1);\n+\t\t\tmemset(&cp, 0, sizeof(cp));\n+\t\t\tcp.argv = argv;\n+\t\t\tcp.env = local_repo_env;\n+\t\t\tcp.git_cmd = 1;\n+\t\t\tcp.no_stdin = 1;\n+\t\t\tcp.out = -1;\n+\t\t\tcp.dir = path;\n+\t\t\tif (!run_command(&cp) && !strbuf_read(&buf, cp.out, 41))\n+\t\t\t\tis_pushed = 1;\n+\t\t\tclose(cp.out);\n+\t\t\tstrbuf_release(&buf);\n+\t\t} else\n+\t\t\tis_pushed = 1;\n+\t}\n+\treturn is_pushed;\n+}\n+\n+static void collect_unpushed_submodules_from_revs(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 *found_unpushed_submodule = 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 (!is_submodule_commit_pushed(p->two->path, p->two->sha1)) {\n+\t\t\t*found_unpushed_submodule = 1;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+}\n+\n+static int collect_unpushed_submodules_in_tree(const unsigned char *sha1,\n+\t\t\t\t\tconst char *base, int baselen,\n+\t\t\t\t\tconst char *pathname, unsigned mode,\n+\t\t\t\t\tint stage, void *context)\n+{\n+\tint *found_unpushed_submodules = context;\n+\tstruct strbuf path = STRBUF_INIT;\n+\n+\tstrbuf_add(&path, base, strlen(base));\n+\tstrbuf_add(&path, pathname, strlen(pathname));\n+\n+\tif (S_ISGITLINK(mode) && !is_submodule_commit_pushed(path.buf, sha1)) {\n+\t\t*found_unpushed_submodules = 1;\n+\t\treturn 0;\n+\t}\n+\treturn READ_TREE_RECURSIVE;\n+}\n+\n+static void parent_commits_pushed(struct commit *commit, struct commit_list *parent, int *found_unpushed_submodule)\n+{\n+\twhile (parent) {\n+\t\tstruct diff_options diff_opts;\n+\t\tdiff_setup(&diff_opts);\n+\t\tDIFF_OPT_SET(&diff_opts, RECURSIVE);\n+\t\tdiff_opts.output_format |= DIFF_FORMAT_CALLBACK;\n+\t\tdiff_opts.format_callback = collect_unpushed_submodules_from_revs;\n+\t\tdiff_opts.format_callback_data = found_unpushed_submodule;\n+\t\tif (diff_setup_done(&diff_opts) < 0)\n+\t\t\tdie(\"diff_setup_done failed\");\n+\t\tdiff_tree_sha1(parent->item->object.sha1, commit->object.sha1, \"\", &diff_opts);\n+\t\tdiffcore_std(&diff_opts);\n+\t\tdiff_flush(&diff_opts);\n+\t\tparent = parent->next;\n+\t}\n+}\n+\n+static void tree_commits_pushed(struct commit *commit, int *found_unpushed_submodule)\n+{\n+\tstruct tree * tree;\n+\tstruct pathspec pathspec;\n+\ttree = parse_tree_indirect(commit->object.sha1);\n+\tinit_pathspec(&pathspec,NULL);\n+\tread_tree_recursive(tree, \"\", 0, 0, &pathspec, collect_unpushed_submodules_in_tree,\n+\t\t\t    found_unpushed_submodule);\n+}\n+\n+int check_for_unpushed_submodule_commits(unsigned char new_sha1[20])\n+{\n+\tstruct rev_info rev;\n+\tstruct commit *commit;\n+\tconst char *argv[] = {NULL, NULL, \"--not\", \"--remotes\", NULL};\n+\tint argc = ARRAY_SIZE(argv) - 1;\n+\tchar *sha1_copy;\n+\tint found_unpushed_submodule = 0;\n+\n+\tinit_revisions(&rev, NULL);\n+\tsha1_copy = xstrdup(sha1_to_hex(new_sha1));\n+\targv[1] = sha1_copy;\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)) && !found_unpushed_submodule) {\n+\t\tstruct commit_list *parent = commit->parents;\n+\t\tif (parent)\n+\t\t\tparent_commits_pushed(commit, parent, &found_unpushed_submodule);\n+\t\telse\n+\t\t\ttree_commits_pushed(commit, &found_unpushed_submodule);\n+\t}\n+\n+\tfree(sha1_copy);\n+\treturn found_unpushed_submodule;\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..0a4d395 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_for_unpushed_submodule_commits(unsigned char sha1[20]);\n \n #endif\ndiff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\nindex faa2e96..d3a1789 100755\n--- a/t/t5531-deep-submodule-push.sh\n+++ b/t/t5531-deep-submodule-push.sh\n@@ -32,4 +32,103 @@ 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+\tmkdir a &&\n+\t(\n+\t\tcd a &&\n+\t\tgit init --bare\n+\t) &&\n+\tgit clone a a1 &&\n+\t(\n+\t\tcd a1 &&\n+\t\tmkdir b &&\n+\t\t(\n+\t\t\tcd b &&\n+\t\t\tgit init &&\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 origin master\n+\t)\n+'\n+\n+test_expect_success 'push succeeds when --no-recurse-submodules is used' '\n+\t(\n+\t\tcd work &&\n+\t\tgit push ../pub.git --no-recurse-submodules\n+\t)\n+'\n+\n test_done\ndiff --git a/transport.c b/transport.c\nindex c9c8056..9c100c9 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@@ -1041,6 +1042,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_for_unpushed_submodule_commits(ref->new_sha1))\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.397.g23ae2.dirty\n"},{"id":"173223","messageId":"7vobzys316.fsf@alter.siamese.dyndns.org","threadId":"28055","inReplyTo":"1312913752-21555-1-git-send-email-iveqy@iveqy.com","subject":"Re: [PATCH] push: Don't push a repository with unpushed submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-09T21:12:05Z","receivedAt":"2011-08-09T21:12:05Z","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> When working with submodules it is easy to forget to push a\n> submodule to the server but pushing a super-project that\n> contains a commit for that submodule. The result is that the\n> superproject points at a submodule commit that is not available\n> on the server.\n>\n> This adds the option --recurse-submodules=check to push. When\n> using this option git will check that all submodule commits that\n> are about to be pushed are present on a remote of the submodule.\n\nLooking better.\n\n> This does not guarantee that all submodules a super-project\n> needs will be available on the server.\n\nHmm, why not?\n\nYou do not, when this option is in effect, push a new commit of the\nsuperproject to the remote unless you know all the submodule commits\npointed at by the superproject commit you are pushing out are already\npushed out, right? If so how could some submodules become unavailable on\nthe other end?\n\nAre you thinking about some submodules that you did not touch may have\nbeen updated by somebody else, rewinding the branch that contained the\ncommit that you did not modify and that you have in your superproject\ncommit, or something? But ...\n\n> In that case both the\n> super-project and the submodules would need an atomic push.\n\n... such a case won't be helped with any atomic push, as you didn't even\nupdate that submodule that somebody else rewound.\n\nSo you must be thinking about something else. Can you clarify the failure\nmode you are worried about a bit better here?\n\nYou may also drop the \"atomic push might help something\" from the\ndescription; as it is unclear what it could help. Then what remains would\nbe...\n\n> This\n> does however prevent the human error of forgetting to push a\n> submodule.\n\n...this one, which is correct. This change is primarily to help prevent a\nhuman error that was described in the first paragraph of the message (I do\nnot think it prevents any other kind of error). So while it is correct, it\nmay not have to be said again here.\n\n> The first iteration of this patch series can be found here:\n> http://thread.gmane.org/gmane.comp.version-control.git/176328/focus=176327\n\nAnd the second iteration was here:\n\n  http://thread.gmane.org/gmane.comp.version-control.git/177992\n\n>  Documentation/git-push.txt     |    6 ++\n>  builtin/push.c                 |   17 +++++\n>  submodule.c                    |  129 ++++++++++++++++++++++++++++++++++++++++\n>  submodule.h                    |    1 +\n>  t/t5531-deep-submodule-push.sh |   99 ++++++++++++++++++++++++++++++\n>  transport.c                    |    9 +++\n>  transport.h                    |    1 +\n>  7 files changed, 262 insertions(+), 0 deletions(-)\n>\n> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\n> index 88acfcd..6ae6ba3 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\n> diff --git a/builtin/push.c b/builtin/push.c\n> index 9cebf9e..23ba365 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,19 @@ 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}\n> +\treturn 0;\n> +}\n\nLater you may want to add a configuration variable to flip the default,\nand _before_ that happens (e.g. like \"now\"), you would need to be able to\naccept --recurse-submodules=no-check or something that defeats the\nconfigured default from the command line during the transition period.\n\nWhy not use a simple string-valued variable to keep the last value of this\noption you saw instead of using a callback?  The overall control flow in\ncmd_push() would look like this:\n\n\tconst char *submodules = NULL; /* default */\n        ...\n\tOPT_STRING(0, \"recurse-submodules\", &submodules, ...),\n\t...\n\targc = parse_options(argc, argv, prefix, options, push_usage, 0);\n\tparse_recurse_submodules_option(submodules, &flags);\n\nand you set or unset the necessary bits in flags in the helper function.\nThat would also help people who use an alias with --recurse-submodules=\nearly on the command line to force his own default before configuration is\nintroduced by naturally implementing the usual \"last one on the command\nline wins\" rule, no?\n\n>  int cmd_push(int argc, const char **argv, const char *prefix)\n>  {\n>  \tint flags = 0;\n> @@ -236,6 +250,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\"),\n> diff --git a/submodule.c b/submodule.c\n> index 1ba9646..c44472f 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -308,6 +308,135 @@ 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> +/*\n> + * checks whether a certain submodule commit is pushed based on the\n> + * submodules remote tracking branches. returns 1 in case no remote\n> + * tracking branches are found.\n> + */\n> +static int is_submodule_commit_pushed(const char *path, const unsigned char sha1[20])\n> +{\n> +\tint is_pushed = 0;\n> +\tif (!add_submodule_odb(path) && lookup_commit_reference(sha1)) {\n> +\t\tif (for_each_remote_ref_submodule(path, has_remote, NULL)) {\n\nThis code assumes that this for_each_remote_ref_submodule() call will\nreturn non-zero only when has_remote() is called even once. Is that really\ntrue? Doesn't the function (not has_remote() but the iterator) want to\nsignal errors in any way, and if so doesn't it return a negative value or\nsomething?\n\n> +\t\t\tstruct child_process cp;\n> +\t\t\tconst char *argv[] = {\"rev-list\", NULL, \"--not\", \"--remotes\", \"-n\", \"1\" , NULL};\n> +\t\t\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\t\t\targv[1] = sha1_to_hex(sha1);\n> +\t\t\tmemset(&cp, 0, sizeof(cp));\n> +\t\t\tcp.argv = argv;\n> +\t\t\tcp.env = local_repo_env;\n> +\t\t\tcp.git_cmd = 1;\n> +\t\t\tcp.no_stdin = 1;\n> +\t\t\tcp.out = -1;\n> +\t\t\tcp.dir = path;\n> +\t\t\tif (!run_command(&cp) && !strbuf_read(&buf, cp.out, 41))\n> +\t\t\t\tis_pushed = 1;\n> +\t\t\tclose(cp.out);\n> +\t\t\tstrbuf_release(&buf);\n\nHmm, this run_command(), while it may not be a bug, feels somewhat fishy.\n\nIt calls start and then calls finish. What if the subprocess is really\nfast, writes everything it needs to write (which is presumably 41 bytes at\nmost), closes its end of the pipe? And then you read from the other end of\nthe pipe. Am I being too paranoid to think that this sequence is ...\n\n\tstart_command();\n        strbuf_read();\n        finish_command();\n        close();\n        strbuf_release();\n\n... a more natural way of writing this logic?\n\n> +\t\t} else\n> +\t\t\tis_pushed = 1;\n\nThis case is not \"we know it is pushed\" but \"we do not know (and we do not\ncare) if it was pushed, because the submodule does not have a tracking\",\nno?\n\nProbably the function should be named \"submodule_needs_pushing()\" and\nreturn the value accordingly. Then \"we do not have to prevent the user\nfrom pushing the superproject based on the state of this submodule\" case\ncan say \"No, this submodule repository does not need pushing\", which reads\na lot more naturally. The overall code structure would also become\nsimpler:\n\n\tstatic int submodule_needs_pushing(...)\n        {\n\t\tint needs_pushing = 0;\n                if (!(we can inspect the objects in this submodule))\n\t\t\treturn needs_pushing;\n\n               \tif (for_each_remote_ref_in_submodule(...)) {\n                \t... run rev-list ...\n                       \tif (found commit not in remotes)\n\t\t\t\tneeds_pushing = 1;\n\t\t\t... clean-up rev-list ...                                \n\t\t}\n\t\treturn needs_pushing;\n\t}\n\nWhat happens in a submodule repository that you do not have a checkout?\nthe first if() statement in the function would be false, and you would end\nup returning \"is-pushed? no\" and collect_unpushed_submodules_from_revs()\n(which also is a misnomer; you do not care if commits in certain\nsubmodules haven't been pushed---it should collect submodules that \"needs\npushing\", which is subtly different from \"have not been pushed\") that\ncalled this function will say \"Ah I found one, I found one, that is an\nerror!\".\n\nAnd I think that is wrong.\n\nImagine a merge in a superproject between two branches, among which only\none of them updated the submodule while the other one did not touch. As\nfar as that submodule path is concerned, the merge should resolve cleanly\nwithout a checkout (or clone) of the submodule repository to take the\nupdate from the branch that updated the submodule commit.\n\nSo the \"submodule-needs-pushing()\" function should return \"no need to push\n(there is nothing to push to begin with)\" for a submodule that is not even\n\"submodule init\"ed.\n\n> +static int collect_unpushed_submodules_in_tree(const unsigned char *sha1,\n> +\t\t\t\t\tconst char *base, int baselen,\n> +\t\t\t\t\tconst char *pathname, unsigned mode,\n> +\t\t\t\t\tint stage, void *context)\n> +{\n> +\tint *found_unpushed_submodules = context;\n> +\tstruct strbuf path = STRBUF_INIT;\n> +\n> +\tstrbuf_add(&path, base, strlen(base));\n> +\tstrbuf_add(&path, pathname, strlen(pathname));\n> +\n> +\tif (S_ISGITLINK(mode) && !is_submodule_commit_pushed(path.buf, sha1)) {\n> +\t\t*found_unpushed_submodules = 1;\n> +\t\treturn 0;\n> +\t}\n> +\treturn READ_TREE_RECURSIVE;\n> +}\n\nI find it somewhat distasteful to have this separate codepath (and\ntree_commits_pushed() function) only for the root commit. Why not use the\nsame diff-tree machinery?\n\n> +static void parent_commits_pushed(struct commit *commit, struct commit_list *parent, int *found_unpushed_submodule)\n> +{\n> +\twhile (parent) {\n\nAlso I wonder if you really want to do pair-wise diff here, which would\nmean that for a typical two-parent merge you will potentially have to\ncheck the same submodule twice (no I am not talking about the cost of\nrunning diff the same number of times as you have parents, but I am\ntalking about the cost of running submodule_needs_pushing()).\n\nWould it make sense to run the equivalent of \"show -c --name-only\" here\ninternally (\"diff-tree --root -c --name-only\" to be more exact)? Then any\nsubmodule path that match one of the parents you would not have to check\nat all. Just thinking aloud...\n\n> +\t\tstruct diff_options diff_opts;\n> +\t\tdiff_setup(&diff_opts);\n> +\t\tDIFF_OPT_SET(&diff_opts, RECURSIVE);\n> +\t\tdiff_opts.output_format |= DIFF_FORMAT_CALLBACK;\n> +\t\tdiff_opts.format_callback = collect_unpushed_submodules_from_revs;\n> +\t\tdiff_opts.format_callback_data = found_unpushed_submodule;\n> +\t\tif (diff_setup_done(&diff_opts) < 0)\n> +\t\t\tdie(\"diff_setup_done failed\");\n> +\t\tdiff_tree_sha1(parent->item->object.sha1, commit->object.sha1, \"\", &diff_opts);\n> +\t\tdiffcore_std(&diff_opts);\n> +\t\tdiff_flush(&diff_opts);\n> +\t\tparent = parent->next;\n> +\t}\n> +}\n> +\n> +static void tree_commits_pushed(struct commit *commit, int *found_unpushed_submodule)\n> +{\n> +\tstruct tree * tree;\n\n(style) struct tree *tree;\n\nBut I already suggested to get rid of this separate codepath, so it is not\nsuch a big deal.\n\n> +\tstruct pathspec pathspec;\n> +\ttree = parse_tree_indirect(commit->object.sha1);\n> +\tinit_pathspec(&pathspec,NULL);\n> +\tread_tree_recursive(tree, \"\", 0, 0, &pathspec, collect_unpushed_submodules_in_tree,\n> +\t\t\t    found_unpushed_submodule);\n> +}\n> +\n> +int check_for_unpushed_submodule_commits(unsigned char new_sha1[20])\n> +{\n\nThe same \"we are not looking for 'unpushed' ones, but for 'needs pushing'\nones\" comment on naming applies here.\n\n> +\tstruct rev_info rev;\n> +\tstruct commit *commit;\n> +\tconst char *argv[] = {NULL, NULL, \"--not\", \"--remotes\", NULL};\n> +\tint argc = ARRAY_SIZE(argv) - 1;\n> +\tchar *sha1_copy;\n> +\tint found_unpushed_submodule = 0;\n> +\n> +\tinit_revisions(&rev, NULL);\n> +\tsha1_copy = xstrdup(sha1_to_hex(new_sha1));\n> +\targv[1] = sha1_copy;\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)) && !found_unpushed_submodule) {\n\nWhy are we checking all commits that are not in \"any of the remotes\" (I am\nreferring to \"--not --remotes\" in the argv[]) here, instead of all commits\nthat are not in \"the remote we are pushing this superproject commit to\"?\nAt this point in the codepath don't you (or you may not---I didn't check)\nalready know where you are pushing?\n\n> +test_expect_success 'push succeeds if submodule has no remote and is on the first superproject commit' '\n> +\tmkdir a &&\n> +\t(\n> +\t\tcd a &&\n> +\t\tgit init --bare\n> +\t) &&\n\nPerhaps\n\n\tgit init --bare a\n\nwould do?\n"}]}