Support for nested git repositories #37947 #157812
Conversation
- Subdirectories with their own .git folders will no longer be considered part of an outer repository - This allows vscode to correctly detect repositories in subdirectories of an outer repository
|
@nteymory, thank you very much for your contribution. I will pull down your changes next week and I will try to do some manual testing to see if there are any other issues that have to be addressed other than enabling the discovery of nested git repositories. I would highly appreciate if you could do some tests on your end as well. Thank you! |
| // If this path is not the root folder of this repo | ||
| // and it contains it's own .git folder, then consider it a separate repo | ||
| if (!pathEquals(liveRepository.repository.root, resourcePath)) { | ||
| if (fs.existsSync(path.join(resourcePath, '.git'))) { |
There was a problem hiding this comment.
Could you instead compare the outputs of git rev-parse HEAD for the root and the nested resource? I find that checking for .git is usually a code smell.
There was a problem hiding this comment.
Sure, that seems like a better idea, let me whip something up..
There was a problem hiding this comment.
Actually this might be better.
git rev-parse --show-toplevel
You might need to see what the minimum version of git that VSCode supports is
There was a problem hiding this comment.
Right.. this gets more complicated because getRepository -> getOpenRepository is synchronous. This is what is called from openRepository and ultimately the cause of the problem. So, there is no straight forward way to insert a this.git.exec directly into getOpenRepository.
An alternate way to solve this might be to replace the this.getRepository(repoPath) check inside openRepository? This would also fix the problem, but then getOpenRepository is still arguably not implemented correctly. It will continue to report subdirectories that contain their own repositories as belonging to the outer repository, which is the root of this problem.
What are your thoughts about this?
There was a problem hiding this comment.
Actually this might be better.
git rev-parse --show-toplevelYou might need to see what the minimum version of git that VSCode supports is
Yes, in fact, it looks like Git.getRepositoryRoot does exactly this already. It would be ideal to compare the repository root from this call for the subdirectory to the root of the outer repository. Alas, the async problem.
Maybe I can try to write a specialized synchronous version of Git.exec for this specific problem..
Or maybe we don't care that getOpenRepository is wrong? Honestly, I was a bit surprised getOpenRepository works the way it does, I thought it would only match the root directory for the repo. I'm not entirely sure why it's like this or what the intent was.
There was a problem hiding this comment.
Model.getOpenRepository has two callsites that aren't async. One might be possible to change, but the other is Model.getRepository, which is exported and has more callsites that aren't async. At a cursory glance, it doesn't look straight forward to simply propagate async until it works. The effects will be cascading.
There was a problem hiding this comment.
@nteymory, @tristan957, thank you for keeping the conversation going. As I was looking at the code, I pretty much went down the same path as the two of you, and I have to admit that at the moment I am missing some historical context around the implementation of getOpenRepository() and getRepository(). I am hoping to get that context next week and then I will circle back to this pull request.
There was a problem hiding this comment.
@lszomoru is there anything I can do to move this pull request forward? I would love to see this problem fixed.
There was a problem hiding this comment.
This week, the team was heads down preparing the next stable release. Having said that, I did look at the code and I have created a new pull request (#159291) that adds a new setting as well as some new code to support the new setting while keeping the existing code path in case the setting is disabled. I will try to get this pull request in next week but if you want please feel free to check out the branch, build, and test the new setting. Thank you!
There was a problem hiding this comment.
It looks like your changes work. I will close this pull request
|
Addressed by #159291 |
Based on the discussion here: #37947