{"thread":{"id":"54266","subject":"[PATCH 0/1] Test case for checkout of added/deleted submodules in clones","startedAt":"2020-09-21T08:15:58Z","lastAt":"2020-09-22T14:04:26Z","messageCount":4,"participants":["Luke Diamand","Kaartic Sivaraam","Philippe Blain"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"406014","messageId":"20200921081537.15300-1-luke@diamand.org","threadId":"54266","inReplyTo":null,"subject":"[PATCH 0/1] Test case for checkout of added/deleted submodules in clones","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2020-09-21T08:15:36Z","receivedAt":"2020-09-21T08:15:58Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"I hit some quite confusing (to me anyway) corner cases in `git\nsubmodule` handling of checking out revisions where submodules have been\nadded or removed.\n\nIf you clone a repo with an \"old\" submodule which is missing from HEAD,\nand then try to checkout an older revision, you can sometimes get:\n\n    fatal: not a git repository: ../.git/modules/old\n    fatal: could not reset submodule index\n\nIf you clone a repo with a \"new\" submodule, and then checkout an older\nrevision where it is deleted, you can sometimes find that new submodule\nis left lying around.\n\nLuke Diamand (1):\n  Test case for checkout of added/deleted submodules in clones\n\n t/t5619-submodules-missing.sh | 104 ++++++++++++++++++++++++++++++++++\n 1 file changed, 104 insertions(+)\n create mode 100755 t/t5619-submodules-missing.sh\n\n-- \n2.28.0.762.g324f61785e\n\n"},{"id":"406015","messageId":"20200921081537.15300-2-luke@diamand.org","threadId":"54266","inReplyTo":"20200921081537.15300-1-luke@diamand.org","subject":"[PATCH 1/1] Test case for checkout of added/deleted submodules in clones","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2020-09-21T08:15:37Z","receivedAt":"2020-09-21T08:15:59Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Test case for various cases of deleted submodules:\n\n1. Clone a repo where an `old` submodule has been removed in HEAD.\nTry to checkout a revision from before `old` was deleted.\n\nThis can fail in a rather ugly way depending on how the `git checkout`\nand `git submodule` commands are used.\n\n2. Clone a repo where a `new` submodule has been added. Try to\ncheckout a revision from before `new` was added.\n\nThis can leave `new` lying around in some circumstances, and not in\nothers, in a way which is confusing (at least to me).\n\nSigned-off-by: Luke Diamand <luke@diamand.org>\n---\n t/t5619-submodules-missing.sh | 104 ++++++++++++++++++++++++++++++++++\n 1 file changed, 104 insertions(+)\n create mode 100755 t/t5619-submodules-missing.sh\n\ndiff --git a/t/t5619-submodules-missing.sh b/t/t5619-submodules-missing.sh\nnew file mode 100755\nindex 0000000000..d7878c52fc\n--- /dev/null\n+++ b/t/t5619-submodules-missing.sh\n@@ -0,0 +1,104 @@\n+#!/bin/sh\n+\n+test_description='Clone a repo containing submodules. Sync to a revision where the submodule is missing or added'\n+\n+. ./test-lib.sh\n+\n+pwd=$(pwd)\n+\n+# Setup a super project with a submodule called `old`, which gets deleted, and\n+# a submodule `new` which is added later on.\n+\n+test_expect_success 'setup' '\n+\tmkdir super old new &&\n+\tgit -C old init &&\n+\ttest_commit -C old commit_old &&\n+\t(cd super &&\n+\t\tgit init . &&\n+\t\tgit submodule add ../old old &&\n+\t\tgit commit -m \"adding submodule old\" &&\n+\t\ttest_commit commit2 &&\n+\t\tgit tag OLD &&\n+\t\ttest_path_is_file old/commit_old.t &&\n+\t\tgit rm old &&\n+\t\tgit commit -m \"Remove old submodule\" &&\n+\t\ttest_commit commit3\n+\t) &&\n+\tgit -C new init &&\n+\ttest_commit -C new commit_new &&\n+\t(cd super &&\n+\t\tgit tag BEFORE_NEW &&\n+\t\tgit submodule add ../new new &&\n+\t\tgit commit -m \"adding submodule new\" &&\n+\t\ttest_commit commit4\n+\t)\n+'\n+\n+# Checkout the OLD tag inside the original repo. This works fine since all of\n+# the submodules are present in .git/modules.\n+test_expect_success 'checkout old inside original repo' '\n+\t(cd super &&\n+\t\tgit config advice.detachedHead false &&\n+\t\tgit tag LATEST &&\n+\t\tgit checkout --recurse-submodules OLD &&\n+\t\tgit submodule update --checkout --remote --force &&\n+\t\ttest_path_is_file old/commit_old.t &&\n+\t\ttest_path_is_missing new/commit_new.t &&\n+\t\tgit checkout --recurse-submodules LATEST &&\n+\t\ttest_path_is_file new/commit_new.t\n+\t)\n+'\n+\n+# Clone the repo, and then checkout the OLD tag inside the clone.\n+# The `old` submodule does not get updated. Instead we get:\n+#\n+# fatal: not a git repository: ../.git/modules/old\n+# fatal: could not reset submodule index\n+#\n+# That's because `old` is missing from .git/modules since it\n+# was not cloned originally and `checkout` does not know how to\n+# fetch the remote submodules, whereas `submodule update --remote` does.\n+\n+test_expect_failure 'checkout old with --recurse-submodules' '\n+\ttest_when_finished \"rm -fr super-clone\" &&\n+\tgit clone --recurse-submodules super super-clone &&\n+\t(cd super-clone &&\n+\t\tgit config advice.detachedHead false &&\n+\t\ttest_path_is_file commit3.t &&\n+\t\ttest_path_is_file commit2.t &&\n+\t\ttest_path_is_missing old &&\n+\t\ttest_path_is_file new/commit_new.t &&\n+\t\tgit checkout --recurse-submodules OLD &&\n+\t\tgit submodule update --checkout --remote --force &&\n+\t\ttest_path_is_file commit2.t &&\n+\t\ttest_path_is_missing commit3.t &&\n+\t\ttest_path_is_dir old &&\n+\t\ttest_path_is_file old/commit_old.t\n+\t)\n+'\n+\n+# As above, but this time, instead of using \"checkout --recurse-submodules\" we just\n+# use \"checkout\" to avoid the missing submodule error.\n+#\n+# The checkout of `old` now works fine, but instead `new` is left lying\n+# around with seemingly no way to clean it up. Even a later invocation of\n+# `git checkout --recurse-submodules` does not get rid of it.\n+\n+test_expect_failure 'checkout old without --recurse-submodules' '\n+\ttest_when_finished \"rm -fr super-clone\" &&\n+\tgit clone --recurse-submodules super super-clone &&\n+\t(cd super-clone &&\n+\t\tgit config advice.detachedHead false &&\n+\t\ttest_path_is_file new/commit_new.t &&\n+\t\tgit checkout OLD &&\n+\t\tgit submodule update --checkout --remote --force &&\n+\t\tgit checkout --recurse-submodules OLD &&\n+\t\ttest_path_is_file commit2.t &&\n+\t\ttest_path_is_missing commit3.t &&\n+\t\ttest_path_is_dir old &&\n+\t\ttest_path_is_file old/commit_old.t &&\n+\t\ttest_path_is_missing new/commit_new.t\n+\t)\n+'\n+\n+test_done\n-- \n2.28.0.762.g324f61785e\n\n"},{"id":"406118","messageId":"d1b6df73-e945-2ccf-129c-62add58e5747@gmail.com","threadId":"54266","inReplyTo":"20200921081537.15300-2-luke@diamand.org","subject":"Re: [PATCH 1/1] Test case for checkout of added/deleted submodules in clones","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2020-09-22T09:35:32Z","receivedAt":"2020-09-22T09:35:38Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On 21-09-2020 13:45, Luke Diamand wrote:\n> +\n> +# Checkout the OLD tag inside the original repo. This works fine since all of\n> +# the submodules are present in .git/modules.\n> +test_expect_success 'checkout old inside original repo' '\n> +\t(cd super &&\n> +\t\tgit config advice.detachedHead false &&\n> +\t\tgit tag LATEST &&\n> +\t\tgit checkout --recurse-submodules OLD &&\n> +\t\tgit submodule update --checkout --remote --force &&\n> +\t\ttest_path_is_file old/commit_old.t &&\n> +\t\ttest_path_is_missing new/commit_new.t &&\n> +\t\tgit checkout --recurse-submodules LATEST &&\n> +\t\ttest_path_is_file new/commit_new.t\n> +\t)\n> +'\n> +\n\nPhilippe pointed out the reason behind this behavriour [1] for this in\nanother thread.\n\n> +# Clone the repo, and then checkout the OLD tag inside the clone.\n> +# The `old` submodule does not get updated. Instead we get:\n> +#\n> +# fatal: not a git repository: ../.git/modules/old\n> +# fatal: could not reset submodule index\n> +#\n> +# That's because `old` is missing from .git/modules since it\n> +# was not cloned originally and `checkout` does not know how to\n> +# fetch the remote submodules, whereas `submodule update --remote` does.\n> +\n> +test_expect_failure 'checkout old with --recurse-submodules' '\n> +\ttest_when_finished \"rm -fr super-clone\" &&\n> +\tgit clone --recurse-submodules super super-clone &&\n> +\t(cd super-clone &&\n> +\t\tgit config advice.detachedHead false &&\n> +\t\ttest_path_is_file commit3.t &&\n> +\t\ttest_path_is_file commit2.t &&\n> +\t\ttest_path_is_missing old &&\n> +\t\ttest_path_is_file new/commit_new.t &&\n> +\t\tgit checkout --recurse-submodules OLD &&\n> +\t\tgit submodule update --checkout --remote --force &&\n> +\t\ttest_path_is_file commit2.t &&\n> +\t\ttest_path_is_missing commit3.t &&\n> +\t\ttest_path_is_dir old &&\n> +\t\ttest_path_is_file old/commit_old.t\n> +\t)\n> +'\n> +\n> +# As above, but this time, instead of using \"checkout --recurse-submodules\" we just\n> +# use \"checkout\" to avoid the missing submodule error.\n> +#\n> +# The checkout of `old` now works fine, but instead `new` is left lying\n> +# around with seemingly no way to clean it up. Even a later invocation of\n> +# `git checkout --recurse-submodules` does not get rid of it.\n> +\n> +test_expect_failure 'checkout old without --recurse-submodules' '\n> +\ttest_when_finished \"rm -fr super-clone\" &&\n> +\tgit clone --recurse-submodules super super-clone &&\n> +\t(cd super-clone &&\n> +\t\tgit config advice.detachedHead false &&\n> +\t\ttest_path_is_file new/commit_new.t &&\n> +\t\tgit checkout OLD &&\n> +\t\tgit submodule update --checkout --remote --force &&\n> +\t\tgit checkout --recurse-submodules OLD &&\n> +\t\ttest_path_is_file commit2.t &&\n> +\t\ttest_path_is_missing commit3.t &&\n> +\t\ttest_path_is_dir old &&\n> +\t\ttest_path_is_file old/commit_old.t &&\n> +\t\ttest_path_is_missing new/commit_new.t\n> +\t)\n> +'\n> +\n> +test_done\n> \n\nI think this isn't the complete story. The submodule removal is done\nproperly if `git checkout --recurse-submodules` is called when the\nrevision is not checked out (modulo the checkout failing issue detailed\nin the previous test case). For some reason, it doesn't work when the\nrevisions is already checked out. i.e., the following passes\n\n  test_expect_success 'checkout old without --recurse-submodules' '\n        test_when_finished \"rm -fr super-clone\" &&\n        git clone --recurse-submodules super super-clone &&\n        (cd super-clone &&\n                git config advice.detachedHead false &&\n                test_path_is_file new/commit_new.t &&\n                git checkout OLD &&\n                git submodule update --checkout --remote --force &&\n                git checkout --recurse-submodules LATEST &&\n                test_path_is_file commit2.t &&\n                test_path_is_file commit3.t &&\n                test_path_is_missing old &&\n                test_path_is_missing old/commit_old.t &&\n                test_path_is_file new/commit_new.t\n        )\n  '\n\nAlso, this is the exact case that seems to be explained in commit\nbbad9f9314 (rm: better document side effects when removing a submodule,\n2014-01-07) which adds the following BUGS part to the documentation.\n\n    BUGS\n    ----\n    Each time a superproject update removes a populated submodule\n    e.g. when switching between commits before and after the removal) a\n    stale submodule checkout will remain in the old location. Removing\n    the old directory is only safe when it uses a gitfile, as otherwise\n    the history of the submodule will be deleted too. This step will be\n    obsolete when recursive submodule update has been implemented.\n\nAs Phillipe points out in [2], I do wonder if this part has now become\nstale.\n\n[ References ]\n\n[1]:\nhttps://lore.kernel.org/git/20200501005432.h62dnpkx7feb7rto@glandium.org/T/#u\n\n[2]:\nhttps://public-inbox.org/git/0B191753-C1AD-499C-B8B2-122F49CF6F14@gmail.com/\n\n-- \nSivaraam\n"},{"id":"406127","messageId":"8F8BD84C-ADBB-4A58-B3D0-87F9A428E5B4@gmail.com","threadId":"54266","inReplyTo":"20200921081537.15300-2-luke@diamand.org","subject":"Re: [PATCH 1/1] Test case for checkout of added/deleted submodules in clones","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-09-22T14:04:16Z","receivedAt":"2020-09-22T14:04:26Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Luke,\n\n> Le 21 sept. 2020 à 04:15, Luke Diamand <luke@diamand.org> a écrit :\n> \n> Test case for various cases of deleted submodules:\n> \n> 1. Clone a repo where an `old` submodule has been removed in HEAD.\n> Try to checkout a revision from before `old` was deleted.\n> \n> This can fail in a rather ugly way depending on how the `git checkout`\n> and `git submodule` commands are used.\n\nAs I wrote in my previous email [1], this fails if '--recurse-submodules' \nwas passed to 'git clone', which writes 'submodule.active=.' to the config.\nSo I would phrase it as:\n\nThis fails if the 'checkout' uses '--recurse-submodules' and \na pathspec in the configuration variable 'submodule.active' matches\nthe name of the submodule, which is the case for the match-all value '.' written to \nthis variable by 'git clone --recurse-submodules'.\n\n> 2. Clone a repo where a `new` submodule has been added. Try to\n> checkout a revision from before `new` was added.\n> \n> This can leave `new` lying around in some circumstances, and not in\n> others, in a way which is confusing (at least to me).\n> \n> Signed-off-by: Luke Diamand <luke@diamand.org>\n> ---\n> t/t5619-submodules-missing.sh | 104 ++++++++++++++++++++++++++++++++++\n\nI'm a little torn about where to put this test case. The heart of the matter\nis that clone should not write '.' to 'submodule.active' if it does not clone all submodules,\neven deleted ones, since 'checkout' cant (yet?) cope with an active submodule for which\nit does not find the Git repository. So the \"bug\" is indeed in clone, thus t56** is a good place.\nHowever, I would say that from a user experience perspective,\nthis test case should be in t2013-checkout-submodules, because if someone clones\nwith '--recurse-submodules', and then checks out the old revision with '--recurse-submodules',\nthen clearly their expectation is that the checkout should work.\n\nIn any case, if we leave it in t56** I think the name of the test file should maybe contain \"clone\", \nsince that seems to be the case for almost all test files in this series.\n\n> 1 file changed, 104 insertions(+)\n> create mode 100755 t/t5619-submodules-missing.sh\n> \n> diff --git a/t/t5619-submodules-missing.sh b/t/t5619-submodules-missing.sh\n> new file mode 100755\n> index 0000000000..d7878c52fc\n> --- /dev/null\n> +++ b/t/t5619-submodules-missing.sh\n> @@ -0,0 +1,104 @@\n> +#!/bin/sh\n> +\n> +test_description='Clone a repo containing submodules. Sync to a revision where the submodule is missing or added'\n\n\"Sync\" is confusing here ('git submodule sync\" exists and does something completely different). \nWhat this test is doing is a (recursive) *checkout* of a revision where the submodule exists,\nstarting from a revision where it's absent, with 'submodule.active' set to a pathspec that matches\nthat submodule.\n\n> +\n> +. ./test-lib.sh\n> +\n> +pwd=$(pwd)\n> +\n> +# Setup a super project with a submodule called `old`, which gets deleted, and\n> +# a submodule `new` which is added later on.\n\nAs I showed in [1], we don't need the 'new' submodule to demonstrate the faulty\nbehaviour, so I would not include it in this test case, \nit's adding unnecessary noise in my opinion.\n\n> +\n> +test_expect_success 'setup' '\n> +\tmkdir super old new &&\n> +\tgit -C old init &&\n> +\ttest_commit -C old commit_old &&\n> +\t(cd super &&\n> +\t\tgit init . &&\n> +\t\tgit submodule add ../old old &&\n> +\t\tgit commit -m \"adding submodule old\" &&\n> +\t\ttest_commit commit2 &&\n> +\t\tgit tag OLD &&\n> +\t\ttest_path_is_file old/commit_old.t &&\n> +\t\tgit rm old &&\n> +\t\tgit commit -m \"Remove old submodule\" &&\n> +\t\ttest_commit commit3\n> +\t) &&\n> +\tgit -C new init &&\n> +\ttest_commit -C new commit_new &&\n> +\t(cd super &&\n> +\t\tgit tag BEFORE_NEW &&\n> +\t\tgit submodule add ../new new &&\n> +\t\tgit commit -m \"adding submodule new\" &&\n> +\t\ttest_commit commit4\n> +\t)\n> +'\n> +\n> +# Checkout the OLD tag inside the original repo. This works fine since all of\n> +# the submodules are present in .git/modules.\n> +test_expect_success 'checkout old inside original repo' '\n> +\t(cd super &&\n> +\t\tgit config advice.detachedHead false &&\n> +\t\tgit tag LATEST &&\n\nminor point: this 'tag' command should be in the \"setup\" test above.\n\n> +\t\tgit checkout --recurse-submodules OLD &&\n> +\t\tgit submodule update --checkout --remote --force &&\n\nThis invocation of 'git submodule update' does nothing here (the \nsubmodule is already correctly checked out at the revision\nregistered in the superproject as a result of `git checkout --recurse-submodules OLD`)\nso I would remove it.\n\n> +\n> +# Clone the repo, and then checkout the OLD tag inside the clone.\n> +# The `old` submodule does not get updated\n\nMinor point, but I would write \"checked out\" instead of \"updated\".\n\n> . Instead we get:\n> +#\n> +# fatal: not a git repository: ../.git/modules/old\n> +# fatal: could not reset submodule index\n> +#\n> +# That's because `old` is missing from .git/modules since it\n> +# was not cloned originally\n\nI would add a little bit of info here:\n\n...since it was not clone originally, 'checkout' wants to recurse \ninto it because 'submodule.active' was set to '.' by 'clone --recurse-submodules',\nand 'checkout' does not know how to....\n\n> and `checkout` does not know how to\n> +# fetch the remote submodules,\n\nI think \"clone\" would be more appropriate then 'fetch\" here. \n\n> whereas `submodule update --remote` does.\n\nI don't think you want to add '--remote' here.\nWhat we want is to checkout the submodule at the revision\nspecified by the superproject at tag OLD, which is what a \nplain 'git submodule update' should do. Adding '--remote'\nwould fetch the *submodule's remote* and checkout \nthe submodule at the commit at the tip of the HEAD branch of that\nremote [2] (which in this specific case would be the same commit, \nsince no commit were done in the 'old' repo after it was added \nas a submodule to 'super', but in general this does not have to \nbe the case).\n\n> +\n> +test_expect_failure 'checkout old with --recurse-submodules' '\n> +\ttest_when_finished \"rm -fr super-clone\" &&\n> +\tgit clone --recurse-submodules super super-clone &&\n> +\t(cd super-clone &&\n> +\t\tgit config advice.detachedHead false &&\n> +\t\ttest_path_is_file commit3.t &&\n> +\t\ttest_path_is_file commit2.t &&\n> +\t\ttest_path_is_missing old &&\n> +\t\ttest_path_is_file new/commit_new.t &&\n> +\t\tgit checkout --recurse-submodules OLD &&\n\nThe test case will fail here as the exit code of 'checkout' is 128,\nso any further command don't affect the outcome of the test.\nI think it's a good reflex to try 'git submodule update' next at this point, \nand I agree that in an ideal world it should be able to help and clone\nthe missing submodule, but I think it should be tested in a new test,\n following this one.\n\n> +\t\tgit submodule update --checkout --remote --force &&\n\nAs I wrote above '--remote' is not what we want here, and '--checkout'\nis the default behaviour so unnecessary. Also, '--force' should not be necessary \nto clone the submodule here since there is no revision checked out in it yet (see [3], [4]).\n\nAs I detailed in [5], \"git submodule update\" can't clone the submodule\nuntil the whole .git/modules/old directory, which is written by the failing\n'checkout --recurse-submodules', is manually deleted, so this would be \nan additional 'test_expect_failure' case.\n\n> +# As above, but this time, instead of using \"checkout --recurse-submodules\" we just\n> +# use \"checkout\" to avoid the missing submodule error.\n> +#\n> +# The checkout of `old` now works fine, but instead `new` is left lying\n> +# around with seemingly no way to clean it up.\n\n`git clean -dff` cleans it up.\n\n> Even a later invocation of\n> +# `git checkout --recurse-submodules` does not get rid of it.\n> +\n> +test_expect_failure 'checkout old without --recurse-submodules' '\n> +\ttest_when_finished \"rm -fr super-clone\" &&\n> +\tgit clone --recurse-submodules super super-clone &&\n> +\t(cd super-clone &&\n> +\t\tgit config advice.detachedHead false &&\n> +\t\ttest_path_is_file new/commit_new.t &&\n> +\t\tgit checkout OLD &&\n> +\t\tgit submodule update --checkout --remote --force &&\n> +\t\tgit checkout --recurse-submodules OLD &&\n> +\t\ttest_path_is_file commit2.t &&\n> +\t\ttest_path_is_missing commit3.t &&\n> +\t\ttest_path_is_dir old &&\n> +\t\ttest_path_is_file old/commit_old.t &&\n> +\t\ttest_path_is_missing new/commit_new.t\n> +\t)\n\nI would simply remove this test case, it does not add new information.\nAfter 'git checkout OLD', since '--recurse-submodules was not used,\nthe \"new\" submodules appears as \"untracked\",\nso the command to remove it is \"git clean\", and since it contains a '.git'\ngitfile, two '-f' are needed [6]. Since it is untracked, it makes sense that no\nfurther Git command should touch it.\n\nThanks for working on this!\n\nPhilippe.\n\n[1] https://lore.kernel.org/git/0B191753-C1AD-499C-B8B2-122F49CF6F14@gmail.com/T/#m85fe0b90231033c96d3d75bac6e8ea9b2ae6d467\n[2] https://git-scm.com/docs/git-submodule#Documentation/git-submodule.txt---remote\n[3] https://git-scm.com/docs/git-submodule#Documentation/git-submodule.txt--f\n[4] https://git-scm.com/docs/git-submodule#Documentation/git-submodule.txt-update--init--remote-N--no-fetch--no-recommend-shallow-f--force--checkout--rebase--merge--referenceltrepositorygt--depthltdepthgt--recursive--jobsltngt--no-single-branch--ltpathgt82308203\n[5] https://lore.kernel.org/git/20200501005432.h62dnpkx7feb7rto@glandium.org/T/#u\n[6] https://git-scm.com/docs/git-clean#Documentation/git-clean.txt--f"}]}