{"thread":{"id":"53363","subject":"git checkout --recurse-submodules doesn't like checking out an older commit after a submodule was removed","startedAt":"2020-05-01T00:54:44Z","lastAt":"2020-05-01T14:40:18Z","messageCount":2,"participants":["Mike Hommey","Philippe Blain"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"396707","messageId":"20200501005432.h62dnpkx7feb7rto@glandium.org","threadId":"53363","inReplyTo":null,"subject":"git checkout --recurse-submodules doesn't like checking out an older commit after a submodule was removed","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2020-05-01T00:54:32Z","receivedAt":"2020-05-01T00:54:44Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Hi,\n\nDoing the following fails:\n\n$ git clone --recurse-submodules https://github.com/trailofbits/winchecksec/\n$ cd winchecksec\n$ git checkout --recurse-submodules 93ffe67dbfc757bf6f440d80b8acf88e652ed60a\nfatal: not a git repository: ../.git/modules/pe-parse\nfatal: could not reset submodule index\n\nThe main reason is that the pe-parse directory/submodule was removed in\nthe commit that follows 93ffe67dbfc757bf6f440d80b8acf88e652ed60a (which\nis current master).\n\nThis also leaves the tree in a bad state, and it's hard to fix anything\nfrom that state.\n\nMike\n"},{"id":"396753","messageId":"7C3FF354-F10E-4822-ACF8-31417B05C099@gmail.com","threadId":"53363","inReplyTo":"20200501005432.h62dnpkx7feb7rto@glandium.org","subject":"Re: git checkout --recurse-submodules doesn't like checking out an older commit after a submodule was removed","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-05-01T14:40:03Z","receivedAt":"2020-05-01T14:40:18Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Mike,\n\n> Le 30 avr. 2020 à 20:54, Mike Hommey <mh@glandium.org> a écrit :\n> \n> Hi,\n> \n> Doing the following fails:\n> \n> $ git clone --recurse-submodules https://github.com/trailofbits/winchecksec/\n> $ cd winchecksec\n> $ git checkout --recurse-submodules 93ffe67dbfc757bf6f440d80b8acf88e652ed60a\n> fatal: not a git repository: ../.git/modules/pe-parse\n> fatal: could not reset submodule index\n\nThanks for the report, I can reproduce that with Git 2.26.0.\n\n> The main reason is that the pe-parse directory/submodule was removed in\n> the commit that follows 93ffe67dbfc757bf6f440d80b8acf88e652ed60a (which\n> is current master).\n\nYes, indeed. If you look at the code for `git clone`, you will see that the `--recurse-submodules`\nflag causes `git submodule update --init` to be called in the \"checkout\" phase of the clone [1].\nThis means that only submodules that are present in the HEAD commit of the default branch (or \nthe branch to be checked out if using the `--branch` flag of `git clone`) will be cloned.\n\nThis also means that they won't be cloned if using `git clone --no-checkout`, and similarly they won't be \ncloned if they are present in a different branch then the one being checked out by `git clone`.\n\nHowever, the `submodule.active=.` config value will still be written to the local configuration [2].\nThis is why `git checkout --recurse-submodules` tries to checkout the submodule when checking\nout commit 93ffe67db.\n\n> This also leaves the tree in a bad state, and it's hard to fix anything\n> from that state.\n\nIndeed, the files are written to disk as in commit 93ffe67dbfc757bf6f440d80b8acf88e652ed60a, \nbut HEAD is not updated, and so files appear modified and .gitmodules appears untracked.\nFor me,\n\n    git reset --hard && git clean -df\n\nreturned me to the initial state (master checked out) without errors.\n\nHowever, trying to get to the intended state (93ffe67dbfc757bf6f440d80b8acf88e652ed60a\nchecked out with its submodule) is indeed pretty hard. The first thing I tried \n(after the reset and clean above) was to checkout 93ffe67 without the `--recurse-submodules` flag. \n\n    git checkout 93ffe67\n\nThis succeeds. However, note that if  `git clean -f` is used instead of `git clean -df` above, \nthen the pe-parse directory (with only its gitfile inside) is left untouched and the checkout fails with \n\n    fatal: not a git repository: pe-parse/../.git/modules/pe-parse\n\nand HEAD stays at master. A subsequent `git status` fails the same way. However \n`git status --ignore-submodules` does not error (and shows that the files are *staged*\nas in 93ffe67, which is very confusing!)\n\nAnyway assuming we used `git clean -df`, then the checkout of 93ffe67 succeeds, but\nthe submodule is not checked out. Trying `git submodule update` (with or without `--init) fails:\n\n    fatal: not a git repository: /private/var/folders/lr/r6n2057j0dzd4gdb614fp0740000gp/T/winchecksec/pe-parse/../.git/modules/pe-parse\n    Failed to clone 'pe-parse'. Retry scheduled\n    BUG: submodule considered for cloning, doesn't need cloning any more?\n    fatal: could not get a repository handle for submodule 'pe-parse'\n\nI noticed here that the submodule config file `.git/modules/pe-parse/config` was written by the \nfirst failed recursive checkout with this content:\n\n    $ cat .git/modules/pe-parse/config\n    [core]\n        worktree = ../../../pe-parse\n\nI next tried `git sub deinit --all -f` and retried `git submodule update --init`, but still got the\nsame error. This is in part because the deinit leaves and empty file `.git/modules/pe-parse/config`.\nRemoving this file and retrying gives the same error. Removing the directory  `.git/modules/pe-parse/` \nand retrying finally succeeds in cloning the submodule.\n\nAfterwards, alternating `git checkout --r master` and `git checkout --r 93ffe67` both succeeds.\n\nIn my opinion there are several things that are going wrong here:\n\n1. The most user-friendly approach, I think, would be that  `git clone --recurse-submodules` would\nlook at the history of all branches and clone any submodules it finds. This would require the most\ncode modification, and might not be desired in some cases, so maybe it should be opt-in \n(i.e. adding supplementary flag).\n\n2. Maybe it would be safer to *not* write `submodule.active=.` to the config if no submodules \nare present in the HEAD commit of the branch checked out by `git clone`. This would alleviate \nthe situation you got (the recursive checkout would succeed, the submodule would not be \nchecked out, but `git submodule update --init` would clone it and check it out.)\n\n3. I think `git checkout --recurse-submodules` should not leave the tree and the submodule config file\nin a hard-to-recover-from state if it can't correctly checkout the submodules. It should abort completely \nand cleanly and say it can't find the submodule repository, and tell the user how to clone it.\n\n4. I think `git submodule update --init` should succeed in cloning the submodule if the \n`.git/modules/pe-parse/` directory exists but is empty. I also think it should succeeds \nif the only file in it is an empty `config` file. Probably it should also succeed if\nthe config file contains the `core.worktree` config as above.\n\n5. I think `git submodule deinit --all -f` should not let an empty config file in the submodule repository.\n\nI'm CC-ing some people that I *think* are working on some aspects of submodules \n(Jonathan and Emily, forgive me if that's not the case!) in case they want to add to the analysis above.\n\nCheers,\n\nPhilippe.\n\n[1] https://github.com/git/git/blob/d61d20c9b413225793f8a0b491bbbec61c184e26/builtin/clone.c#L821-L847\n[2] https://github.com/git/git/blob/d61d20c9b413225793f8a0b491bbbec61c184e26/builtin/clone.c#L1087-L1092"}]}