{"thread":{"id":"28095","subject":"[PATCH v4 2/2] push: Don't push a repository with unpushed submodules","startedAt":"2011-08-14T09:03:33Z","lastAt":"2011-08-19T20:44:33Z","messageCount":4,"participants":["Fredrik Gustafsson","Heiko Voigt","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":2},"messages":[{"id":"173488","messageId":"1313312613-2647-1-git-send-email-iveqy@iveqy.com","threadId":"28095","inReplyTo":null,"subject":"[PATCH v4 2/2] push: Don't push a repository with unpushed submodules","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2011-08-14T09:03:33Z","receivedAt":"2011-08-14T09:03:33Z","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---\nI am not sure whether the change required for the diff callback\nhas other side effects. Another pair of eyes appreciated.\n\n Documentation/git-push.txt     |    6 ++\n builtin/push.c                 |   17 ++++++\n combine-diff.c                 |    2 +-\n submodule.c                    |  108 ++++++++++++++++++++++++++++++++++++++++\n submodule.h                    |    1 +\n t/t5531-deep-submodule-push.sh |   94 ++++++++++++++++++++++++++++++++++\n transport.c                    |    9 +++\n transport.h                    |    1 +\n 8 files changed, 237 insertions(+), 1 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/combine-diff.c b/combine-diff.c\nindex be67cfc..e5a0d84 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1068,7 +1068,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..4dda8b8 100755\n--- a/t/t5531-deep-submodule-push.sh\n+++ b/t/t5531-deep-submodule-push.sh\n@@ -32,4 +32,98 @@ 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_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..fa279d5 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_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.398.g64865.dirty\n"},{"id":"173772","messageId":"20110818192157.GA10598@book.hvoigt.net","threadId":"28095","inReplyTo":"1313312613-2647-1-git-send-email-iveqy@iveqy.com","subject":"Re: [PATCH v4 2/2] push: Don't push a repository with unpushed submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2011-08-18T19:21:58Z","receivedAt":"2011-08-18T19:21:58Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi Junio,\n\nOn Sun, Aug 14, 2011 at 11:03:33AM +0200, Fredrik Gustafsson wrote:\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\nSince I do not see thisi in pu maybe you missed this? There was a series\ncontaining the same patch send before which Jens asked to ignore but\nthis was the one which was meant to be included.\n\nCheers Heiko\n"},{"id":"173783","messageId":"7v4o1ea2at.fsf@alter.siamese.dyndns.org","threadId":"28095","inReplyTo":"20110818192157.GA10598@book.hvoigt.net","subject":"Re: [PATCH v4 2/2] push: Don't push a repository with unpushed submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-18T20:32:26Z","receivedAt":"2011-08-18T20:32: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> On Sun, Aug 14, 2011 at 11:03:33AM +0200, Fredrik Gustafsson wrote:\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> Since I do not see thisi in pu maybe you missed this? There was a series\n> containing the same patch send before which Jens asked to ignore but\n> this was the one which was meant to be included.\n\nHmm, this is how the messages look like in my threaded MUA:\n\n  [  34: Fredrik Gustafsson     ] [PATCH v4 0/2] push limitations\n   [ 377: Fredrik Gustafsson     ] [PATCH v4 2/2] push: Don't push a reposi...\n   [  35: Jens Lehmann           ] Re: [PATCH v4 0/2] push limitations\n  [ 377: Fredrik Gustafsson     ] [PATCH v4 2/2] push: Don't push a reposit...\n   [  14: Heiko Voigt            ] \n\nwhere Jens says \"Please ignore this series, it slipped by accident\" in the\nthird one. And the second [PATCH v4 2/2] message that you are reminding me\nof (thanks!) has the identical proposed commit log message and patch text\nas the other [PATCH v4 2/2] message.\n\nSo I am not sure what is going on here.\n\nAlso I vaguely recall that I said something about the command line parser\nfor this new option during the review of the previous round. Has that been\nresolved/corrected?\n"},{"id":"173880","messageId":"20110819204433.GD27639@book.hvoigt.net","threadId":"28095","inReplyTo":"7v4o1ea2at.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4 2/2] push: Don't push a repository with unpushed submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2011-08-19T20:44:33Z","receivedAt":"2011-08-19T20:44:33Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nOn Thu, Aug 18, 2011 at 01:32:26PM -0700, Junio C Hamano wrote:\n> Heiko Voigt <hvoigt@hvoigt.net> writes:\n> > Since I do not see thisi in pu maybe you missed this? There was a series\n> > containing the same patch send before which Jens asked to ignore but\n> > this was the one which was meant to be included.\n> \n> Hmm, this is how the messages look like in my threaded MUA:\n> \n>   [  34: Fredrik Gustafsson     ] [PATCH v4 0/2] push limitations\n>    [ 377: Fredrik Gustafsson     ] [PATCH v4 2/2] push: Don't push a reposi...\n>    [  35: Jens Lehmann           ] Re: [PATCH v4 0/2] push limitations\n>   [ 377: Fredrik Gustafsson     ] [PATCH v4 2/2] push: Don't push a reposit...\n>    [  14: Heiko Voigt            ] \n> \n> where Jens says \"Please ignore this series, it slipped by accident\" in the\n> third one. And the second [PATCH v4 2/2] message that you are reminding me\n> of (thanks!) has the identical proposed commit log message and patch text\n> as the other [PATCH v4 2/2] message.\n> \n> So I am not sure what is going on here.\n\nSorry for confusing you.\n\nThere was a patch series which included a patch which was the same as\nthe patch I replied to. This was sent by accident since it also included\nan old patch which was originally part of the 'fg/submodule-git-file-git-dir'\nseries.\n\nAfterwards Fredrik also sent the same patch on its own again but forgot\nto edit the subject. That was the patch to be included. But we already\ndiscovered a small bug in the option parsing and will probably add\nanother patch implementing the on-demand option. So its probably simpler\nif you just wait for our next iteration.\n\n> Also I vaguely recall that I said something about the command line parser\n> for this new option during the review of the previous round. Has that been\n> resolved/corrected?\n\nWe plan to make the --recurse-submodules option symmetric to the one\nalready implemented in fetch. AFAIK, having a mixed boolean and other\nvalues option is only possible by using a callback. To make extending it\nsimpler we decided to put the necessary infrastructure already in place.\n\nCheers Heiko\n"}]}