{"thread":{"id":"27704","subject":"[RFC 1/2] test whether push checks for unpushed remotes in submodules","startedAt":"2011-06-26T18:29:45Z","lastAt":"2011-07-04T20:05:09Z","messageCount":14,"participants":["Fredrik Gustafsson","Junio C Hamano","Heiko Voigt","Marc Branchaud","Jens Lehmann"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"170553","messageId":"1309112987-3185-1-git-send-email-iveqy@iveqy.com","threadId":"27704","inReplyTo":null,"subject":"[RFC 0/2] push checks for unpushed remotes in submodules","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2011-06-26T18:29:45Z","receivedAt":"2011-06-26T18:29:45Z","isPatch":false,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"When working with submodules it is easy to end up in a state when submodule\ncommits required by the super-project only is present locally. This is most\noften a human error (although technical errors such as connection failure\ncan be a reason).\n\nThis patch-series tries to prevent pushing a super-project if not all (by\nthe super-project used) submodules are pushed first. This will prevent the\nhuman error of forgetting to push submodules before pushing the\nsuper-project.\n\nThis is a RFC-series, please consider:\n* Is this going into the right direction?\n* Should we use a new flag similar to REF_STATUS_REJECT_NONFASTFORWARD?\n\nFuture improvements could be:\n* Make git status aware of unpushed submodules.\n* Show a list of unpushed submodules when a push is denied.\n\nFredrik Gustafsson (1):\n  Don't push a repository with unpushed submodules\n\nHeiko Voigt (1):\n  test whether push checks for unpushed remotes in submodules\n\n submodule.c                    |   79 ++++++++++++++++++++++++++++++++++++++++\n submodule.h                    |    1 +\n t/t5531-deep-submodule-push.sh |   68 ++++++++++++++++++++++++++++++++++\n transport.c                    |    8 ++++\n 4 files changed, 156 insertions(+), 0 deletions(-)\n\n-- \n1.7.6.rc3.2.g0185a\n"},{"id":"170551","messageId":"1309112987-3185-2-git-send-email-iveqy@iveqy.com","threadId":"27704","inReplyTo":"1309112987-3185-1-git-send-email-iveqy@iveqy.com","subject":"[RFC 1/2] test whether push checks for unpushed remotes in submodules","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2011-06-26T18:29:46Z","receivedAt":"2011-06-26T18:29:46Z","isPatch":false,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"From: Heiko Voigt <hvoigt@hvoigt.net>\n\nThese tests are used to extend push to check whether all recorded\nsubmodule commits have also been pushed.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\n t/t5531-deep-submodule-push.sh |   46 +++++++++++++++++++++++++++++++++++++++-\n 1 files changed, 45 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\nindex faa2e96..0b55466 100755\n--- a/t/t5531-deep-submodule-push.sh\n+++ b/t/t5531-deep-submodule-push.sh\n@@ -28,8 +28,52 @@ test_expect_success setup '\n test_expect_success push '\n \t(\n \t\tcd work &&\n-\t\tgit push ../pub.git master\n+\t\tgit push -f ../pub.git master\n \t)\n '\n \n+test_expect_failure 'push fails 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\ttest_must_fail git push ../pub.git master\n+\t)\n+'\n+\n+test_expect_failure '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 ../pub.git master\n+\t)\n+'\n+\n+test_expect_failure '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 ../pub.git master\n+\t)\n+'\n test_done\n-- \n1.7.6.rc3.2.g0185a\n"},{"id":"170552","messageId":"1309112987-3185-3-git-send-email-iveqy@iveqy.com","threadId":"27704","inReplyTo":"1309112987-3185-1-git-send-email-iveqy@iveqy.com","subject":"[RFC 2/2] Don't push a repository with unpushed submodules","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2011-06-26T18:29:47Z","receivedAt":"2011-06-26T18:29:47Z","isPatch":false,"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 submodule\nto the server but pushing a super-project that contains a commit for\nthat submodule. The result is that the superproject points at a\nsubmodule commit that is not avaliable on the server.\n\nCheck that all submodule commits that are about to be pushed are present\non a remote of the submodule and require forcing if that is not the\ncase.\n\nThis does not guarantee that all submodules a super-project needs will be\navailable on the server. In that case both the super-project and the\nsubmodules would need an atomic push. This does however prevent the\nhuman error of forgetting to push a submodule.\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 submodule.c                    |   79 ++++++++++++++++++++++++++++++++++++++++\n submodule.h                    |    1 +\n t/t5531-deep-submodule-push.sh |   30 ++++++++++++++--\n transport.c                    |    8 ++++\n 4 files changed, 115 insertions(+), 3 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex b6dec70..fe1daae 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -308,6 +308,85 @@ void set_config_fetch_recurse_submodules(int value)\n \tconfig_fetch_recurse_submodules = value;\n }\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\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+\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 (!run_command(&cp) && !strbuf_read(&buf, cp.out, 41))\n+\t\t\tis_pushed = 1;\n+\t\tclose(cp.out);\n+\t\tstrbuf_release(&buf);\n+\t}\n+\treturn is_pushed;\n+}\n+\n+static void submodule_collect_unpushed_cb(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+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\twhile (parent) {\n+\t\t\tstruct diff_options diff_opts;\n+\t\t\tdiff_setup(&diff_opts);\n+\t\t\tDIFF_OPT_SET(&diff_opts, RECURSIVE);\n+\t\t\tdiff_opts.output_format |= DIFF_FORMAT_CALLBACK;\n+\t\t\tdiff_opts.format_callback = submodule_collect_unpushed_cb;\n+\t\t\tdiff_opts.format_callback_data = &found_unpushed_submodule;\n+\t\t\tif (diff_setup_done(&diff_opts) < 0)\n+\t\t\t\tdie(\"diff_setup_done failed\");\n+\t\t\tdiff_tree_sha1(parent->item->object.sha1, commit->object.sha1, \"\", &diff_opts);\n+\t\t\tdiffcore_std(&diff_opts);\n+\t\t\tdiff_flush(&diff_opts);\n+\t\t\tparent = parent->next;\n+\t\t}\n+\t}\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 0b55466..0ef943a 100755\n--- a/t/t5531-deep-submodule-push.sh\n+++ b/t/t5531-deep-submodule-push.sh\n@@ -32,7 +32,7 @@ test_expect_success push '\n \t)\n '\n \n-test_expect_failure 'push fails if submodule has no remote' '\n+test_expect_success 'push fails if submodule has no remote' '\n \t(\n \t\tcd work/gar/bage &&\n \t\t>junk2 &&\n@@ -47,7 +47,7 @@ test_expect_failure 'push fails if submodule has no remote' '\n \t)\n '\n \n-test_expect_failure 'push fails if submodule commit not on remote' '\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@@ -66,7 +66,7 @@ test_expect_failure 'push fails if submodule commit not on remote' '\n \t)\n '\n \n-test_expect_failure 'push succeeds after commit was pushed to remote' '\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@@ -76,4 +76,28 @@ test_expect_failure 'push succeeds after commit was pushed to remote' '\n \t\tgit push ../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 ../pub.git\n+\t)\n+'\n test_done\ndiff --git a/transport.c b/transport.c\nindex c9c8056..d83595b 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,13 @@ 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_PUSH_FORCE) && !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) && check_for_unpushed_submodule_commits(ref->new_sha1))\n+\t\t\t\t\tdie(\"There's unpushed submodules, aborting. Use -f to force a push\");\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;\n-- \n1.7.6.rc3.2.g0185a\n"},{"id":"170637","messageId":"7v1uydvmh0.fsf@alter.siamese.dyndns.org","threadId":"27704","inReplyTo":"1309112987-3185-3-git-send-email-iveqy@iveqy.com","subject":"Re: [RFC 2/2] Don't push a repository with unpushed submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-28T18:29:15Z","receivedAt":"2011-06-28T18:29:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fredrik Gustafsson <iveqy@iveqy.com> writes:\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\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> +\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 (!run_command(&cp) && !strbuf_read(&buf, cp.out, 41))\n> +\t\t\tis_pushed = 1;\n> +\t\tclose(cp.out);\n> +\t\tstrbuf_release(&buf);\n> +\t}\n\nWhat if\n\n (1) you are binding somebody else's project as your own submodule, you do\n     not make any local changes (you won't be pushing them out anyway),\n     and you do not have remote tracking branches in that submodule\n     project?\n\n (2) you have a project you can push to that is bound as a submodule, you\n     have pushed the commit that is bound in the superproject's tree to\n     that submodule project, but you do not have remote tracking branches\n     in that submodule project?\n\n (3) you have a project you can push to that is bound as a submodule, you\n     have multiple remotes defined (e.g. one for the public server you\n     intend for this check to be in effect, the other is just for your\n     private backup), and you have pushed the necessary commit to your\n     backup but not to the public one?\n\nThe above check would fail in cases (1) and (2) even though it does not\nhave to. It would succeed in case (3), but people would not be able to use\nthe superproject as the necessary commit is not there but only on your\nwork and backup repositories.\n\nWhat am I missing?\n\nI am not sure if the check imposed on the client end is such a great\nidea. I suspect that it would depend on the superproject which submodule\ncommit must exist \"out there\" for others to fetch. If you assume a closed\nenvironment where all the superprojects and necessary submodule projects\nare housed at a central location everybody pushes into and have tight\ncontrol over how project participants clone and check out the\nsuperprojects and submodules, you do not have to worry about any of the\nabove, but then the server-side can perform the check when it accepts a\npush (and you would already be doing other checks there in your hooks\nanyway in the industrial setup, I would guess).\n"},{"id":"170641","messageId":"20110628193034.GB3700@book.hvoigt.net","threadId":"27704","inReplyTo":"7v1uydvmh0.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC 2/2] Don't push a repository with unpushed submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2011-06-28T19:30:35Z","receivedAt":"2011-06-28T19:30:35Z","isPatch":false,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nOn Tue, Jun 28, 2011 at 11:29:15AM -0700, Junio C Hamano wrote:\n> Fredrik Gustafsson <iveqy@iveqy.com> writes:\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\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> > +\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 (!run_command(&cp) && !strbuf_read(&buf, cp.out, 41))\n> > +\t\t\tis_pushed = 1;\n> > +\t\tclose(cp.out);\n> > +\t\tstrbuf_release(&buf);\n> > +\t}\n> \n> What if\n> \n>  (1) you are binding somebody else's project as your own submodule, you do\n>      not make any local changes (you won't be pushing them out anyway),\n>      and you do not have remote tracking branches in that submodule\n>      project?\n\nIn this scenario the superproject can not be cloned that way that it\nwould contain the submodule right? I would consider this a rather exotic\nway to work since pushing means to share your work somehow. This way you\nwould share your work in the superproject but not from the submodule?\nHow about not doing the is-pushed check when the submodule has no\nremotes? If we assume that only people having remote tracking branches\nwant to share them via push this would allow your usecase.\n\n>  (2) you have a project you can push to that is bound as a submodule, you\n>      have pushed the commit that is bound in the superproject's tree to\n>      that submodule project, but you do not have remote tracking branches\n>      in that submodule project?\n\nThis could also be solved with the above proposal.\n\n>  (3) you have a project you can push to that is bound as a submodule, you\n>      have multiple remotes defined (e.g. one for the public server you\n>      intend for this check to be in effect, the other is just for your\n>      private backup), and you have pushed the necessary commit to your\n>      backup but not to the public one?\n> \n> The above check would fail in cases (1) and (2) even though it does not\n> have to. It would succeed in case (3), but people would not be able to use\n> the superproject as the necessary commit is not there but only on your\n> work and backup repositories.\n> \n> What am I missing?\n> \n> I am not sure if the check imposed on the client end is such a great\n> idea.\n\nThis check is solely meant as a convenience security measure. It should\nand can not enforce a tight check whether a superproject (including its\nsubmodules) can be cloned/checked out at all times. But it ensures that\na developer has pushed his submodule commits \"somewhere\" which is enough\nin practice.\n\nOne typical scenario is that people are working together using shared\nremotes. In this scenario this patch provides a consistency check which\ncatches typical mistakes.\n\nIf you fork a project you might change or add a new url for a submodule\nlocally since you cannot directly push to upstream. This is situation 3\nin your above description. All people working with you know which url\nyou are using for the submodule. In this situation the check helps you\nand can not be enforced on the remote side since only the client knows\nwhich remotes the submodule has.\n\nMaybe we should provide a configuration option for this check so that\npeople who know what they are doing could switch it off?\n\n> I suspect that it would depend on the superproject which submodule\n> commit must exist \"out there\" for others to fetch. If you assume a closed\n> environment where all the superprojects and necessary submodule projects\n> are housed at a central location everybody pushes into and have tight\n> control over how project participants clone and check out the\n> superprojects and submodules, you do not have to worry about any of the\n> above, but then the server-side can perform the check when it accepts a\n> push (and you would already be doing other checks there in your hooks\n> anyway in the industrial setup, I would guess).\n\nAs mentioned above a check on the remote end is only applicable if you\nhave a certain defined remote for the submodule in a superproject. This\nalso has to be in an environment which has control over all\nprojects/submodules. The presented solution does not just cover that but\nalso the case where you fork and use different remotes than upstream.\n\nCheers Heiko\n"},{"id":"170642","messageId":"7viprpu1p5.fsf@alter.siamese.dyndns.org","threadId":"27704","inReplyTo":"20110628193034.GB3700@book.hvoigt.net","subject":"Re: [RFC 2/2] Don't push a repository with unpushed submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-28T20:43:18Z","receivedAt":"2011-06-28T20:43:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt <hvoigt@hvoigt.net> writes:\n\n>> What if\n>> \n>>  (1) you are binding somebody else's project as your own submodule, you do\n>>      not make any local changes (you won't be pushing them out anyway),\n>>      and you do not have remote tracking branches in that submodule\n>>      project?\n>\n> In this scenario the superproject can not be cloned that way that it\n> would contain the submodule right? I would consider this a rather exotic\n> way to work since pushing means to share your work somehow.\n\nSorry, I don't follow. Isn't this the classical example of an el-cheapo\nrouter firmware project (i.e. superproject) binding unmodified Linux\nkernel project as one of its submodules without you having any push\nprivilege to Linus's repository, which was one of the original examples\nused in the very initial submodule discussion?\n\n> How about not doing the is-pushed check when the submodule has no\n> remotes? If we assume that only people having remote tracking branches\n> want to share them via push this would allow your usecase.\n\nYes, that would reduce the false positive; same for (2).\n\n> This check is solely meant as a convenience security measure. It should\n> and can not enforce a tight check whether a superproject (including its\n> submodules) can be cloned/checked out at all times. But it ensures that\n> a developer has pushed his submodule commits \"somewhere\" which is enough\n> in practice.\n\nI am not entirely convinced but if this would catch more than 80% of\ncasual mistakes, it would be good enough.  I was hoping that somebody may\ncome up with an idea that would work even in case (3), though.\n"},{"id":"170644","messageId":"20110628215931.GA3974@paksenarrion.iveqy.com","threadId":"27704","inReplyTo":"7viprpu1p5.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC 2/2] Don't push a repository with unpushed submodules","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2011-06-28T21:59:31Z","receivedAt":"2011-06-28T21:59:31Z","isPatch":false,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"On Tue, Jun 28, 2011 at 01:43:18PM -0700, Junio C Hamano wrote:\n> > This check is solely meant as a convenience security measure. It should\n> > and can not enforce a tight check whether a superproject (including its\n> > submodules) can be cloned/checked out at all times. But it ensures that\n> > a developer has pushed his submodule commits \"somewhere\" which is enough\n> > in practice.\n> \n> I am not entirely convinced but if this would catch more than 80% of\n> casual mistakes, it would be good enough.  I was hoping that somebody may\n> come up with an idea that would work even in case (3), though.\n> \n\nThere's ways to do a \"better\" check, but only(*) if the client communicates\nwith the server. This is expensive and doesn't make any sense to do for\nthe error we're trying to prevent here, forgetful developers that\nforgotten to push a submodule.\n\nA design goal for this check has been to make it just a client side\ncheck.\n\nI do not have a % value of how usual this fault is. I do know that\ndevelopers being introduced to submodules that I know of tends to\nforget this (and so do I occasionally).\n\n* According to what I found out. If there's a better solution I would of\n  course be very happy.\n\n-- \nMed vänliga hälsningar\nFredrik Gustafsson\n\ntel: 0733-608274\ne-post: iveqy@iveqy.com\n"},{"id":"170646","messageId":"4E0A506B.6040407@xiplink.com","threadId":"27704","inReplyTo":"1309112987-3185-3-git-send-email-iveqy@iveqy.com","subject":"Re: [RFC 2/2] Don't push a repository with unpushed submodules","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2011-06-28T22:06:35Z","receivedAt":"2011-06-28T22:06:35Z","isPatch":false,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"On 11-06-26 02:29 PM, Fredrik Gustafsson wrote:\n> When working with submodules it is easy to forget to push a submodule\n> to the server but pushing a super-project that contains a commit for\n> that submodule. The result is that the superproject points at a\n> submodule commit that is not avaliable on the server.\n> \n> Check that all submodule commits that are about to be pushed are present\n> on a remote of the submodule and require forcing if that is not the\n> case.\n\nI have a few concerns about what this is trying to do.\n\nFirst, I expect performance will be terrible in repositories with large\nand/or many submodules.  I'd go so far as to say that it's pretty much a\nshow-stopper for our repository.\n\nSecond, there are many times where I'm working in a submodule on branch\n\"TopicA\" but not in branch \"TopicB\".  If I've made submodule changes in\nTopicA then try to push up TopicB, won't I have have to tell push to \"-f\"?\nBut that turns off other checks that I'd rather keep.\n\nI'd feel a lot better about this patch if the check was *off* by default and\nthere was a config setting / command-line option to turn it on.\n\nI also agree with Junio that this kind of verification makes more sense in a\nhook on the server side.\n\n\t\tM.\n"},{"id":"170649","messageId":"4E0A54B8.6050904@web.de","threadId":"27704","inReplyTo":"7viprpu1p5.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC 2/2] Don't push a repository with unpushed submodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2011-06-28T22:24:56Z","receivedAt":"2011-06-28T22:24:56Z","isPatch":false,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 28.06.2011 22:43, schrieb Junio C Hamano:\n> Heiko Voigt <hvoigt@hvoigt.net> writes:\n> \n>>> What if\n>>>\n>>>  (1) you are binding somebody else's project as your own submodule, you do\n>>>      not make any local changes (you won't be pushing them out anyway),\n>>>      and you do not have remote tracking branches in that submodule\n>>>      project?\n>>\n>> In this scenario the superproject can not be cloned that way that it\n>> would contain the submodule right? I would consider this a rather exotic\n>> way to work since pushing means to share your work somehow.\n> \n> Sorry, I don't follow. Isn't this the classical example of an el-cheapo\n> router firmware project (i.e. superproject) binding unmodified Linux\n> kernel project as one of its submodules without you having any push\n> privilege to Linus's repository, which was one of the original examples\n> used in the very initial submodule discussion?\n\nYes, but if you push a version to el-cheapo upstream containing a Linux\nsubmodule commit not contained in Linus' repository (because you made some\nchanges yourself), that won't be cloneable from upstream el-cheapo by\nanyone else. This is what this check makes you aware of, and the best\npractice here IMO is: make your own fork of Linus' repo (on github or\nsomeplace similar) and then you do have the push rights. (This is what we\ndo where I work for every repo we don't have push access to and it solves\nthis problem nicely. And in fact this is pretty much the same I do for a\nstand alone repo - like Git - I don't have push access to but want to\npublish my changes for: I make a fork that gives me push rights and allows\nme to share my work)\n\n>> This check is solely meant as a convenience security measure. It should\n>> and can not enforce a tight check whether a superproject (including its\n>> submodules) can be cloned/checked out at all times. But it ensures that\n>> a developer has pushed his submodule commits \"somewhere\" which is enough\n>> in practice.\n> \n> I am not entirely convinced but if this would catch more than 80% of\n> casual mistakes, it would be good enough.  I was hoping that somebody may\n> come up with an idea that would work even in case (3), though.\n\nMy impression is that we would catch more than 80% (but I admit that I\nmight be influenced by the way we use submodules). Anyways, maybe the\nsolution for (3) is to only take the default remote into account and to\nignore the others? (Because that's the one most users will initialize from\nthe .gitmodules of the superproject)\n"},{"id":"170650","messageId":"4E0A568E.3040202@web.de","threadId":"27704","inReplyTo":"4E0A506B.6040407@xiplink.com","subject":"Re: [RFC 2/2] Don't push a repository with unpushed submodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2011-06-28T22:32:46Z","receivedAt":"2011-06-28T22:32:46Z","isPatch":false,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 29.06.2011 00:06, schrieb Marc Branchaud:\n> First, I expect performance will be terrible in repositories with large\n> and/or many submodules.  I'd go so far as to say that it's pretty much a\n> show-stopper for our repository.\n\nLarge submodules won't be the problem here, but many submodules and/or\nmany commits might cause some performance degradations here. But please\nnotice that there is no communication with the upstream of the submodules,\nwe only check the refs present locally. Do you still think doing some\n\"git rev-list\" invocations in your submodules will be a problem? Then\nplease say so.\n\n> Second, there are many times where I'm working in a submodule on branch\n> \"TopicA\" but not in branch \"TopicB\".  If I've made submodule changes in\n> TopicA then try to push up TopicB, won't I have have to tell push to \"-f\"?\n> But that turns off other checks that I'd rather keep.\n\nNope, this patch only checks the refs to be pushed, not any others. So it\nwill only check that all submodule commits on branch \"TopicB\" are pushed.\n\n> I'd feel a lot better about this patch if the check was *off* by default and\n> there was a config setting / command-line option to turn it on.\n\nI have no objections against making that configurable, although I tend\ntowards making this check default \"on\". But please feel free to test this\nfeature and tell us if it really hinders you in your work or does cause\nperformance degradation, we'll really appreciate the feedback!\n\n> I also agree with Junio that this kind of verification makes more sense in a\n> hook on the server side.\n\nHmm, but isn't that assuming that the server knows (= hosts?) both the\nsubmodule and the superproject?\n"},{"id":"170654","messageId":"20110628230248.GA4436@paksenarrion.iveqy.com","threadId":"27704","inReplyTo":"4E0A506B.6040407@xiplink.com","subject":"Re: [RFC 2/2] Don't push a repository with unpushed submodules","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2011-06-28T23:02:49Z","receivedAt":"2011-06-28T23:02:49Z","isPatch":false,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"On Tue, Jun 28, 2011 at 06:06:35PM -0400, Marc Branchaud wrote:\n> On 11-06-26 02:29 PM, Fredrik Gustafsson wrote:\n> > When working with submodules it is easy to forget to push a submodule\n> > to the server but pushing a super-project that contains a commit for\n> > that submodule. The result is that the superproject points at a\n> > submodule commit that is not avaliable on the server.\n> > \n> > Check that all submodule commits that are about to be pushed are present\n> > on a remote of the submodule and require forcing if that is not the\n> > case.\n> \n> I have a few concerns about what this is trying to do.\n> \n> First, I expect performance will be terrible in repositories with large\n> and/or many submodules.  I'd go so far as to say that it's pretty much a\n> show-stopper for our repository.\n\nThis is not acceptable (in my opinion). The point of making this (only) a\nclient side check was to not have a huge performance impact.\n\nWould you care for testing this in your enviroment or give me enough\ndata to be able to set up a test enviroment size-wize like yours?\n\n> Second, there are many times where I'm working in a submodule on branch\n> \"TopicA\" but not in branch \"TopicB\".  If I've made submodule changes in\n> TopicA then try to push up TopicB, won't I have have to tell push to \"-f\"?\n> But that turns off other checks that I'd rather keep.\n\nI don't quite follow you here. Anyway, only the commits of the\nsuperproject that you are about to push is checked, and only the\nsubmodules that are changed in any of those commits are examined.\n\nSo if you're working in TopicA in a submodule and tries to push TopicB\nin a superproject that uses TopicC in the submodule, TopicA will not be\nrequired to be pushed. (if so, is it a bug).\n\n> I'd feel a lot better about this patch if the check was *off* by default and\n> there was a config setting / command-line option to turn it on.\n> \n> I also agree with Junio that this kind of verification makes more sense in a\n> hook on the server side.\n\nServerside, we cannot guarantee that all submodules are reachable, they\nmight be on different servers, maybe not even connected to eachothers. Even\nif they are connected this would requiring network traffic. Making this check \nan even bigger performance killer. This check is not supposed to guarantee a \nsane server-repo (that would be much harder) and therefore this check is \n\"overkill\" to have on the server-side. Client side we always have all\ninformation needed for this.\n\nNote the problem:\n\"Prevent the developer of pushing a superrepo that has submodule\n(commits) only locally avaliable\"\n\nThat's the problem we're trying to solve, NOT:\n\n\"Prevent the developer of pushing a superrepo that has submodule\n(commits) not avaliable for an other developer\"\n\nThe second problem is just too complex and too slow to solve in a generic\nway.\n\n-- \nMed vänliga hälsningar\nFredrik Gustafsson\n\ntel: 0733-608274\ne-post: iveqy@iveqy.com\n"},{"id":"170674","messageId":"4E0B60FE.1050000@xiplink.com","threadId":"27704","inReplyTo":"4E0A568E.3040202@web.de","subject":"Re: [RFC 2/2] Don't push a repository with unpushed submodules","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2011-06-29T17:29:34Z","receivedAt":"2011-06-29T17:29:34Z","isPatch":false,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"On 11-06-28 06:32 PM, Jens Lehmann wrote:\n> Am 29.06.2011 00:06, schrieb Marc Branchaud:\n>> First, I expect performance will be terrible in repositories with large\n>> and/or many submodules.  I'd go so far as to say that it's pretty much a\n>> show-stopper for our repository.\n> \n> Large submodules won't be the problem here, but many submodules and/or\n> many commits might cause some performance degradations here. But please\n> notice that there is no communication with the upstream of the submodules,\n> we only check the refs present locally. Do you still think doing some\n> \"git rev-list\" invocations in your submodules will be a problem? Then\n> please say so.\n\nYes, that's exactly what I'm saying.  I'm concerned about rev-list's\nperformance, particularly when my system's disk cache is empty (or already\nfull with something else).  When I saw the patch I tried a quick\n\tgit rev-list X --not --remotes -n 1\nin a Linux kernel submodule where X was a non-existent ref, and watched my\ndisk whirl for a few minutes.\n\nAdmittedly that's nothing like what the patch does -- mea culpa.  I see now\nthat the check looks for refs in the submodule's recent history, so the scan\nis likely to be pretty short.\n\nAlso, as you say below and Fredrik said in his reply, the patch only checks\nsubmodules affected by a super-repo ref that is being pushed.  So I agree\nthat the performance hit is likely to be minimal even with large submodules.\n\n(Fredrik, FYI my super-repo itself is large enough to make \"git status\" take\nup a good part of the disk cache.  This repo has several submodules,\nincluding a few (different) Linux kernel repos.  I already get bogged down a\nbit when I have to status the main repo and one of the Linux submodules.  So\nanything that adds extra submodule inspection makes me nervous.)\n\n>> Second, there are many times where I'm working in a submodule on branch\n>> \"TopicA\" but not in branch \"TopicB\".  If I've made submodule changes in\n>> TopicA then try to push up TopicB, won't I have have to tell push to \"-f\"?\n>> But that turns off other checks that I'd rather keep.\n> \n> Nope, this patch only checks the refs to be pushed, not any others. So it\n> will only check that all submodule commits on branch \"TopicB\" are pushed.\n\nThanks for pointing that out!\n\n>> I'd feel a lot better about this patch if the check was *off* by default and\n>> there was a config setting / command-line option to turn it on.\n> \n> I have no objections against making that configurable, although I tend\n> towards making this check default \"on\". But please feel free to test this\n> feature and tell us if it really hinders you in your work or does cause\n> performance degradation, we'll really appreciate the feedback!\n\nWell I'm less concerned about it being configurable now.  I did a few more\ntests and the rev-list performance looked fine.\n\n\t\tM.\n"},{"id":"170675","messageId":"4E0B623D.1060507@xiplink.com","threadId":"27704","inReplyTo":"20110628230248.GA4436@paksenarrion.iveqy.com","subject":"Re: [RFC 2/2] Don't push a repository with unpushed submodules","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2011-06-29T17:34:53Z","receivedAt":"2011-06-29T17:34:53Z","isPatch":false,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"On 11-06-28 07:02 PM, Fredrik Gustafsson wrote:\n> \n>  [ ... ]\n> \n> Serverside, we cannot guarantee that all submodules are reachable, they\n> might be on different servers, maybe not even connected to eachothers. Even\n> if they are connected this would requiring network traffic. Making this check \n> an even bigger performance killer. This check is not supposed to guarantee a \n> sane server-repo (that would be much harder) and therefore this check is \n> \"overkill\" to have on the server-side. Client side we always have all\n> information needed for this.\n> \n> Note the problem:\n> \"Prevent the developer of pushing a superrepo that has submodule\n> (commits) only locally avaliable\"\n> \n> That's the problem we're trying to solve, NOT:\n> \n> \"Prevent the developer of pushing a superrepo that has submodule\n> (commits) not avaliable for an other developer\"\n> \n> The second problem is just too complex and too slow to solve in a generic\n> way.\n\nFair enough.\n\nSo my only remaining concern is that using \"push -f\" to override the check is\ntoo much.  I'd rather see a different option control this.  Maybe\n--ignore-submodules?  (I think it'd be fine if -f implied --ignore-submodules.)\n\n\t\tM.\n"},{"id":"170821","messageId":"20110704200509.GA783@book.hvoigt.net","threadId":"27704","inReplyTo":"7viprpu1p5.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC 2/2] Don't push a repository with unpushed submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2011-07-04T20:05:09Z","receivedAt":"2011-07-04T20:05:09Z","isPatch":false,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nOn Tue, Jun 28, 2011 at 01:43:18PM -0700, Junio C Hamano wrote:\n> Heiko Voigt <hvoigt@hvoigt.net> writes:\n> \n> >> What if\n> >> \n> >>  (1) you are binding somebody else's project as your own submodule, you do\n> >>      not make any local changes (you won't be pushing them out anyway),\n> >>      and you do not have remote tracking branches in that submodule\n> >>      project?\n> >\n> > In this scenario the superproject can not be cloned that way that it\n> > would contain the submodule right? I would consider this a rather exotic\n> > way to work since pushing means to share your work somehow.\n> \n> Sorry, I don't follow. Isn't this the classical example of an el-cheapo\n> router firmware project (i.e. superproject) binding unmodified Linux\n> kernel project as one of its submodules without you having any push\n> privilege to Linus's repository, which was one of the original examples\n> used in the very initial submodule discussion?\n\nBut in such an example the Linux submodule (if used with git submodule)\nwould have remote tracking branches even though they are not directly\npushable.\n\nCheers Heiko\n"}]}