{"thread":{"id":"49230","subject":"[PATCH 1/2] t2013: add test for missing but active submodule","startedAt":"2018-08-27T22:13:04Z","lastAt":"2018-09-07T19:53:55Z","messageCount":10,"participants":["Stefan Beller","Junio C Hamano","SZEDER Gábor","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"356631","messageId":"20180827221257.149257-1-sbeller@google.com","threadId":"49230","inReplyTo":null,"subject":"[PATCH 1/2] t2013: add test for missing but active submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-27T22:12:56Z","receivedAt":"2018-08-27T22:13:04Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When cloning a superproject with the option\n --recurse-submodules='.', it is easy to find yourself wanting\na submodule active, but not having that submodule present in\nthe modules directory.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n t/t2013-checkout-submodule.sh | 24 ++++++++++++++++++++++++\n 1 file changed, 24 insertions(+)\n\ndiff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh\nindex 6ef15738e44..c69640fc341 100755\n--- a/t/t2013-checkout-submodule.sh\n+++ b/t/t2013-checkout-submodule.sh\n@@ -63,6 +63,30 @@ test_expect_success '\"checkout <submodule>\" honors submodule.*.ignore from .git/\n \t! test -s actual\n '\n \n+test_expect_success 'setup superproject with historic submodule' '\n+\ttest_create_repo super1 &&\n+\ttest_create_repo sub1 &&\n+\ttest_commit -C sub1 sub_content &&\n+\tgit -C super1 submodule add ../sub1 &&\n+\tgit -C super1 commit -a -m \"sub1 added\" &&\n+\ttest_commit -C super1 historic_state &&\n+\tgit -C super1 rm sub1 &&\n+\tgit -C super1 commit -a -m \"deleted sub\" &&\n+\ttest_commit -C super1 new_state &&\n+\ttest_path_is_missing super1/sub &&\n+\n+\t# The important part is to ensure sub1 is not in there any more.\n+\t# There is another series in flight, that may remove an\n+\t# empty .gitmodules file entirely.\n+\ttest_must_be_empty super1/.gitmodules\n+'\n+\n+test_expect_failure 'checkout old state with deleted submodule' '\n+\ttest_when_finished \"rm -rf super1 sub1 super1_clone\" &&\n+\tgit clone --recurse-submodules super1 super1_clone &&\n+\tgit -C super1_clone checkout --recurse-submodules historic_state\n+'\n+\n KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1\n test_submodule_switch_recursing_with_args \"checkout\"\n \n-- \n2.18.0\n\n"},{"id":"356632","messageId":"20180827221257.149257-2-sbeller@google.com","threadId":"49230","inReplyTo":"20180827221257.149257-1-sbeller@google.com","subject":"[PATCH 2/2] submodule.c: warn about missing submodule git directories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-27T22:12:57Z","receivedAt":"2018-08-27T22:13:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This is the continuation of f2d48994dc1 (submodule.c: submodule_move_head\nworks with broken submodules, 2017-04-18), which tones down the case of\n\"broken submodule\" in case of a missing git directory of the submodule to\nbe only a warning.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n submodule.c                   | 16 ++++++++++++++++\n t/t2013-checkout-submodule.sh |  2 +-\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 50cbf5f13ed..689439a3d0c 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1641,6 +1641,22 @@ int submodule_move_head(const char *path,\n \t\t} else {\n \t\t\tchar *gitdir = xstrfmt(\"%s/modules/%s\",\n \t\t\t\t    get_git_common_dir(), sub->name);\n+\n+\t\t\tif (!is_git_directory(gitdir)) {\n+\t\t\t\t/*\n+\t\t\t\t * It is safe to assume we could just clone\n+\t\t\t\t * the submodule here, as we passed the\n+\t\t\t\t * is_submodule_active test above (i.e. the\n+\t\t\t\t * user is interested in this submodule.\n+\t\t\t\t *\n+\t\t\t\t * However as this code path is exercised\n+\t\t\t\t * for operations that typically do not involve\n+\t\t\t\t * network operations, let's not do that for now.\n+\t\t\t\t */\n+\t\t\t\twarning(_(\"Submodule '%s' missing\"), path);\n+\t\t\t\tfree(gitdir);\n+\t\t\t\treturn 0;\n+\t\t\t}\n \t\t\tconnect_work_tree_and_git_dir(path, gitdir, 0);\n \t\t\tfree(gitdir);\n \ndiff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh\nindex c69640fc341..82ef4576b91 100755\n--- a/t/t2013-checkout-submodule.sh\n+++ b/t/t2013-checkout-submodule.sh\n@@ -81,7 +81,7 @@ test_expect_success 'setup superproject with historic submodule' '\n \ttest_must_be_empty super1/.gitmodules\n '\n \n-test_expect_failure 'checkout old state with deleted submodule' '\n+test_expect_success 'checkout old state with deleted submodule' '\n \ttest_when_finished \"rm -rf super1 sub1 super1_clone\" &&\n \tgit clone --recurse-submodules super1 super1_clone &&\n \tgit -C super1_clone checkout --recurse-submodules historic_state\n-- \n2.18.0\n\n"},{"id":"356717","messageId":"xmqqtvnefhgx.fsf@gitster-ct.c.googlers.com","threadId":"49230","inReplyTo":"20180827221257.149257-2-sbeller@google.com","subject":"Re: [PATCH 2/2] submodule.c: warn about missing submodule git directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-28T18:56:14Z","receivedAt":"2018-08-28T18:56:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> This is the continuation of f2d48994dc1 (submodule.c: submodule_move_head\n> works with broken submodules, 2017-04-18), which tones down the case of\n> \"broken submodule\" in case of a missing git directory of the submodule to\n> be only a warning.\n\nAfter seeing this warning, as we do not do any remedial action in\nthis codepath, the user with a repository in this state will keep\nseeing the 'missing' message.  Wouldn't we want to give a hint in\naddition (e.g. 'you can run \"git submodule update $name\" to\nrecover', or something like that)?\n\nThe MOVE_HEAD_FORCE codepath that follows this hunk is, eh, already\nforcing to correct the situation, so there is no need to touch that,\nwhich makes sense, if I understand correctly.\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  submodule.c                   | 16 ++++++++++++++++\n>  t/t2013-checkout-submodule.sh |  2 +-\n>  2 files changed, 17 insertions(+), 1 deletion(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 50cbf5f13ed..689439a3d0c 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1641,6 +1641,22 @@ int submodule_move_head(const char *path,\n>  \t\t} else {\n>  \t\t\tchar *gitdir = xstrfmt(\"%s/modules/%s\",\n>  \t\t\t\t    get_git_common_dir(), sub->name);\n> +\n> +\t\t\tif (!is_git_directory(gitdir)) {\n> +\t\t\t\t/*\n> +\t\t\t\t * It is safe to assume we could just clone\n> +\t\t\t\t * the submodule here, as we passed the\n> +\t\t\t\t * is_submodule_active test above (i.e. the\n> +\t\t\t\t * user is interested in this submodule.\n> +\t\t\t\t *\n> +\t\t\t\t * However as this code path is exercised\n> +\t\t\t\t * for operations that typically do not involve\n> +\t\t\t\t * network operations, let's not do that for now.\n> +\t\t\t\t */\n> +\t\t\t\twarning(_(\"Submodule '%s' missing\"), path);\n> +\t\t\t\tfree(gitdir);\n> +\t\t\t\treturn 0;\n> +\t\t\t}\n>  \t\t\tconnect_work_tree_and_git_dir(path, gitdir, 0);\n>  \t\t\tfree(gitdir);\n>  \n> diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh\n> index c69640fc341..82ef4576b91 100755\n> --- a/t/t2013-checkout-submodule.sh\n> +++ b/t/t2013-checkout-submodule.sh\n> @@ -81,7 +81,7 @@ test_expect_success 'setup superproject with historic submodule' '\n>  \ttest_must_be_empty super1/.gitmodules\n>  '\n>  \n> -test_expect_failure 'checkout old state with deleted submodule' '\n> +test_expect_success 'checkout old state with deleted submodule' '\n>  \ttest_when_finished \"rm -rf super1 sub1 super1_clone\" &&\n>  \tgit clone --recurse-submodules super1 super1_clone &&\n>  \tgit -C super1_clone checkout --recurse-submodules historic_state\n"},{"id":"356774","messageId":"CAGZ79kYApAdFzC82LZNjYDzL5uWxtXHbvqUyjZ9OF+b=pGNbUA@mail.gmail.com","threadId":"49230","inReplyTo":"xmqqtvnefhgx.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] submodule.c: warn about missing submodule git directories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-28T21:49:19Z","receivedAt":"2018-08-28T21:49:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 28, 2018 at 11:56 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Stefan Beller <sbeller@google.com> writes:\n>\n> > This is the continuation of f2d48994dc1 (submodule.c: submodule_move_head\n> > works with broken submodules, 2017-04-18), which tones down the case of\n> > \"broken submodule\" in case of a missing git directory of the submodule to\n> > be only a warning.\n>\n> After seeing this warning, as we do not do any remedial action in\n> this codepath, the user with a repository in this state will keep\n> seeing the 'missing' message.  Wouldn't we want to give a hint in\n> addition (e.g. 'you can run \"git submodule update $name\" to\n> recover', or something like that)?\n\nNot quite, as this is only triggered in the case of 'old_head = NULL', which\nis when you have a superproject that is missing the submodule in the\nworking tree before and wants to have it afterwards.\n\nLooking at the test in the previous patch, I would think a reasonable workflow\nin the test is\n\n    git clone --recurse-submodules super1 super1_clone && cd super1_clone\n    git checkout --recurse-submodules historic_state\n    # see warning, but checkout proceeds\n\n    git fetch --recurse-submodules\n    # clones the submodule as it was configured active via the clone\n\n    git checkout --recurse-submodules historic_state\n    # this checkout will put the submodule in place\n    #  not sure if -f is needed here, though.\n\n\nI am currently working on the cloning of submodules that are not currently\nin the working tree while fetching in the superproject, which would address\nthe latter part.\n\n> The MOVE_HEAD_FORCE codepath that follows this hunk is, eh, already\n> forcing to correct the situation, so there is no need to touch that,\n> which makes sense, if I understand correctly.\n\nNo, that is not executed for now as it depends on 'old_head'.\n\nIn case of FORCE we might want to die instead of just warn about that submodule.\n\nStefan\n"},{"id":"356860","messageId":"xmqq1sahcctx.fsf@gitster-ct.c.googlers.com","threadId":"49230","inReplyTo":"CAGZ79kYApAdFzC82LZNjYDzL5uWxtXHbvqUyjZ9OF+b=pGNbUA@mail.gmail.com","subject":"Re: [PATCH 2/2] submodule.c: warn about missing submodule git directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-29T17:16:58Z","receivedAt":"2018-08-29T17:17:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Not quite, as this is ...\n>\n> Looking at the test in the previous patch, I would think a reasonable workflow\n> in the test is ...\n> \n>> The MOVE_HEAD_FORCE codepath that follows this hunk is, eh, already\n>> forcing to correct the situation, so there is no need to touch that,\n>> which makes sense, if I understand correctly.\n>\n> No, that is not executed for now as it depends on 'old_head'.\n\nAll explanation worth having in the log message to help future\nreaders, don't you think?\n\nThanks.\n"},{"id":"356876","messageId":"CAGZ79kaq2oBBfFExCTKoNMhTx7iKRto0zddPknWn5qRjw0fKkw@mail.gmail.com","threadId":"49230","inReplyTo":"xmqq1sahcctx.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] submodule.c: warn about missing submodule git directories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-29T20:32:55Z","receivedAt":"2018-08-29T20:33:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Aug 29, 2018 at 10:17 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Stefan Beller <sbeller@google.com> writes:\n>\n> > Not quite, as this is ...\n> >\n> > Looking at the test in the previous patch, I would think a reasonable workflow\n> > in the test is ...\n> >\n> >> The MOVE_HEAD_FORCE codepath that follows this hunk is, eh, already\n> >> forcing to correct the situation, so there is no need to touch that,\n> >> which makes sense, if I understand correctly.\n> >\n> > No, that is not executed for now as it depends on 'old_head'.\n>\n> All explanation worth having in the log message to help future\n> readers, don't you think?\n\nok, will do.\n\nThanks,\nStefan\n"},{"id":"356884","messageId":"20180829210436.GA31707@localhost","threadId":"49230","inReplyTo":"20180827221257.149257-1-sbeller@google.com","subject":"Re: [PATCH 1/2] t2013: add test for missing but active submodule","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-08-29T21:04:36Z","receivedAt":"2018-08-29T21:04:45Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Aug 27, 2018 at 03:12:56PM -0700, Stefan Beller wrote:\n> When cloning a superproject with the option\n>  --recurse-submodules='.', it is easy to find yourself wanting\n> a submodule active, but not having that submodule present in\n> the modules directory.\n> \n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  t/t2013-checkout-submodule.sh | 24 ++++++++++++++++++++++++\n>  1 file changed, 24 insertions(+)\n> \n> diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh\n> index 6ef15738e44..c69640fc341 100755\n> --- a/t/t2013-checkout-submodule.sh\n> +++ b/t/t2013-checkout-submodule.sh\n> @@ -63,6 +63,30 @@ test_expect_success '\"checkout <submodule>\" honors submodule.*.ignore from .git/\n>  \t! test -s actual\n>  '\n>  \n> +test_expect_success 'setup superproject with historic submodule' '\n> +\ttest_create_repo super1 &&\n> +\ttest_create_repo sub1 &&\n> +\ttest_commit -C sub1 sub_content &&\n> +\tgit -C super1 submodule add ../sub1 &&\n> +\tgit -C super1 commit -a -m \"sub1 added\" &&\n> +\ttest_commit -C super1 historic_state &&\n> +\tgit -C super1 rm sub1 &&\n> +\tgit -C super1 commit -a -m \"deleted sub\" &&\n> +\ttest_commit -C super1 new_state &&\n\nThese six consecutive commands above all specify the '-C super1'\noptions ...\n\n> +\ttest_path_is_missing super1/sub &&\n> +\n> +\t# The important part is to ensure sub1 is not in there any more.\n> +\t# There is another series in flight, that may remove an\n> +\t# empty .gitmodules file entirely.\n> +\ttest_must_be_empty super1/.gitmodules\n\n... and both of these two checks use the 'super1/' path prefix.  I\nthink it would be more readable to simply 'cd super1' first.\n\n> +'\n> +\n> +test_expect_failure 'checkout old state with deleted submodule' '\n> +\ttest_when_finished \"rm -rf super1 sub1 super1_clone\" &&\n> +\tgit clone --recurse-submodules super1 super1_clone &&\n> +\tgit -C super1_clone checkout --recurse-submodules historic_state\n> +'\n> +\n>  KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1\n>  test_submodule_switch_recursing_with_args \"checkout\"\n>  \n> -- \n> 2.18.0\n> \n"},{"id":"357474","messageId":"20180905191849.GB120842@aiede.svl.corp.google.com","threadId":"49230","inReplyTo":"20180827221257.149257-2-sbeller@google.com","subject":"Re: [PATCH 2/2] submodule.c: warn about missing submodule git directories","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-05T19:18:49Z","receivedAt":"2018-09-05T19:18:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> This is the continuation of f2d48994dc1 (submodule.c: submodule_move_head\n> works with broken submodules, 2017-04-18), which tones down the case of\n> \"broken submodule\" in case of a missing git directory of the submodule to\n> be only a warning.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  submodule.c                   | 16 ++++++++++++++++\n>  t/t2013-checkout-submodule.sh |  2 +-\n>  2 files changed, 17 insertions(+), 1 deletion(-)\n\nI don't understand what workflow this is a part of.\n\nIf the submodule is missing, shouldn't we make it non-missing instead\nof producing a partial checkout that doesn't build?\n\nThanks,\nJonathan\n"},{"id":"357649","messageId":"CAGZ79kb0VEDxe0TAMf66nWk3pYMPxrpb_zFmoU4gezzswM5XSQ@mail.gmail.com","threadId":"49230","inReplyTo":"20180905191849.GB120842@aiede.svl.corp.google.com","subject":"Re: [PATCH 2/2] submodule.c: warn about missing submodule git directories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-07T18:49:02Z","receivedAt":"2018-09-07T18:49:15Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Sep 5, 2018 at 12:18 PM Jonathan Nieder <jrnieder@gmail.com> wrote:\n>\n> Hi,\n>\n> Stefan Beller wrote:\n>\n> > This is the continuation of f2d48994dc1 (submodule.c: submodule_move_head\n> > works with broken submodules, 2017-04-18), which tones down the case of\n> > \"broken submodule\" in case of a missing git directory of the submodule to\n> > be only a warning.\n> >\n> > Signed-off-by: Stefan Beller <sbeller@google.com>\n> > ---\n> >  submodule.c                   | 16 ++++++++++++++++\n> >  t/t2013-checkout-submodule.sh |  2 +-\n> >  2 files changed, 17 insertions(+), 1 deletion(-)\n>\n> I don't understand what workflow this is a part of.\n>\n> If the submodule is missing, shouldn't we make it non-missing instead\n> of producing a partial checkout that doesn't build?\n\nNo. checkout and friends do not want to touch the network\n(unless we are in a partial clone world; that is the user is fully\naware that commands can use the network at totally unexpected\ntimes)\n\nSo for that, all we can do is better error messages.\n\nStefan\n"},{"id":"357650","messageId":"20180907195349.GA103699@aiede.svl.corp.google.com","threadId":"49230","inReplyTo":"CAGZ79kb0VEDxe0TAMf66nWk3pYMPxrpb_zFmoU4gezzswM5XSQ@mail.gmail.com","subject":"Re: [PATCH 2/2] submodule.c: warn about missing submodule git directories","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-07T19:53:49Z","receivedAt":"2018-09-07T19:53:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n> On Wed, Sep 5, 2018 at 12:18 PM Jonathan Nieder <jrnieder@gmail.com> wrote:\n>> Stefan Beller wrote:\n\n>>> This is the continuation of f2d48994dc1 (submodule.c: submodule_move_head\n>>> works with broken submodules, 2017-04-18), which tones down the case of\n>>> \"broken submodule\" in case of a missing git directory of the submodule to\n>>> be only a warning.\n[...]\n>> I don't understand what workflow this is a part of.\n>>\n>> If the submodule is missing, shouldn't we make it non-missing instead\n>> of producing a partial checkout that doesn't build?\n>\n> No. checkout and friends do not want to touch the network\n> (unless we are in a partial clone world; that is the user is fully\n> aware that commands can use the network at totally unexpected\n> times)\n>\n> So for that, all we can do is better error messages.\n\nThanks.  This patch doesn't just improve error messages, though, but\nit makes the operation report success instead of failing.\n\nIsn't that likely to produce more confusion when I run additional\ncommands afterward?  In other words, instead of\n\n\t$ git checkout --recurse-submodules -B master origin/new-fancy-branch\n\tBranch 'master' set up to track remote branch 'new-fancy-branch' from 'origin'.\n\tSwitched to a new branch 'master'\n\twarning: Submodule 'new-fancy-submodule' is missing\n\t$ git status\n[some unclean state]\n\nI would prefer to experience\n\n\t$ git checkout --recurse-submodules -B master origin/new-fancy-branch\n\tfatal: missing submodule 'new-fancy-submodule'\n\thint: run \"git fetch --recurse-submodules\" to fetch it\n\t$ git status\n[clean state]\n\t$ git fetch --recurse-submodules\n[...]\n\t$ git checkout --recurse-submodules -B master origin/new-fancy-branch\n\tBranch 'master' set up to track remote branch 'new-fancy-branch' from 'origin'.\n\tSwitched to a new branch 'master'\n\t$ git status\n[clean state]\n\nThanks,\nJonathan\n"}]}