-
-
Notifications
You must be signed in to change notification settings - Fork 909
Split get_git_dir() into get_git_dir() and get_common_git_dir() #1973
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
would it maybe make more sense to change the |
|
The root cause of this issue was that get_git_dir() returned the wrong directory in the worktree case. |
|
I'm not asking for another test -- I think that |
|
I have double checked the usage and discovered a minor issue with get_root() not failing in a .git sub dir as desired. In general I think the refactoring is reasonable, because get_git_dir() now uses '--git-dir' and get_git_common_dir() uses '---git-common-dir'. The alternative would be much more confusing if we stick with get_git_dir for '---git-common-dir' and new name like get_git_individual_dir or such for '---git-dir' I have rebased this PR to rename get_common_git_dir() to get_git_common_dir() to match the git parameter. What do you think now? |
|
can you see if just fixing the to assert a failure you can use |
|
The fall back path still exists for git < 2.5. |
|
|
This fixes the conflicted state check when using work trees. pre-commit#1972
I think the tests are failing -- I also really think there's probably a way to rewrite the merge-conflict parts to not use |
|
Which tests are failing? I am a bit lost with the Azure results. If we want to keep the old get_git_dir function to return the git common dir, we need a new function which returns the real git dir. I did not get the point why this solution is problematic. What issues do you expect? This PR fixes a regression since #809 by reverting the changed behaviour of get_git_dir() and introducing a new one with the most obvious name. |
|
if you click through azure pipelines and click on the red things you eventually get to tox output -- they are all failing these tests: |
|
Both text are fixed now. I had basically reverted #1778, Now I use --is-inside-git-dir to make the old and the new test working. |
|
Azure is still red. Is there something missing? |
This fixes the conflicted state check when using work trees. #1972