Skip to content

Conversation

@daschuer
Copy link
Contributor

This fixes the conflicted state check when using work trees. #1972

@asottile
Copy link
Member

asottile commented Aug 6, 2021

would it maybe make more sense to change the is_in_merge_conflict to utilize git rev-parse -q --verify MERGE_HEAD ? and specifically test that in the test?

@daschuer
Copy link
Contributor Author

daschuer commented Aug 6, 2021

The root cause of this issue was that get_git_dir() returned the wrong directory in the worktree case.
The test checks that this is fixed now.
The issue with the merge conflict was only a symptom because is_in_merge_conflict() calls get_git_dir('.')
So I don't think it is necessary to add another test.

@asottile
Copy link
Member

asottile commented Aug 6, 2021

I'm not asking for another test -- I think that get_git_dir should maybe not be used to detect merge conflicts and that a more reliable approach would not require the rest of the refactor

@daschuer
Copy link
Contributor Author

daschuer commented Aug 6, 2021

I have double checked the usage and discovered a minor issue with get_root() not failing in a .git sub dir as desired.
A test is not possible, because the case causes a fatal error or do you know a way to assert a fatal error call?

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?

@asottile
Copy link
Member

asottile commented Aug 6, 2021

--git-common-dir is too new for many git versions so changing to use that unconditionally isn't acceptable

can you see if just fixing the in_merge_conflict function solves your issue? take my suggestion above please

to assert a failure you can use pytest.raises -- there are quite a few examples already

@daschuer
Copy link
Contributor Author

daschuer commented Aug 6, 2021

The fall back path still exists for git < 2.5.

@daschuer
Copy link
Contributor Author

daschuer commented Aug 6, 2021

get_conflicted_files is also broken. All in all we have four usages of git_get_dir() two are broken one is broken for another reason and one is correct.

@daschuer
Copy link
Contributor Author

daschuer commented Aug 7, 2021

This is the breaking commit 3f78487
#809
Before get_git_dir() was using --git-dir as expected and is_in_merge_conflict was working.
This PR restores the original function an provides another function to maintain the capability of worktree installation.

What do you think? Merge?

@asottile
Copy link
Member

asottile commented Aug 9, 2021

What do you think? Merge?

I think the tests are failing -- I also really think there's probably a way to rewrite the merge-conflict parts to not use git_dir at all and then the refactor wouldn't be needed

@daschuer
Copy link
Contributor Author

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.
How should this be called?

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.

@asottile
Copy link
Member

if you click through azure pipelines and click on the red things you eventually get to tox output -- they are all failing these tests:

FAILED tests/git_test.py::test_get_root_bare_worktree - pre_commit.errors.Fat...
FAILED tests/git_test.py::test_get_root_worktree_in_git - pre_commit.errors.F...

@daschuer
Copy link
Contributor Author

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.

@daschuer
Copy link
Contributor Author

Azure is still red. Is there something missing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants