Adjust git dir when checking for merge in worktree #662
Conversation
errsyn
force-pushed
the
worktree_adjust_gitdir
branch
2 times, most recently
from
04102c2 to
c747973
Compare
| @@ -10,15 +10,25 @@ | |||
| b'=======\n', | |||
| b'>>>>>>> ', | |||
| ] | |||
| GITDIR_PREFIX_LEN = len('gitdir: ') | |||
There was a problem hiding this comment.
I would prefer to use git's mechanisms for discovering this directory -- using implementation detail like reading the file is going to be fragile
tests/check_merge_conflict_test.py
Outdated
| @pytest.fixture(scope='function', params=['repo', 'worktree']) | ||
| def f1_is_a_conflict_file(tmpdir, request): |
There was a problem hiding this comment.
I would prefer not to use parametrized fixtures -- they're really difficult to understand and maintain
what you can actually do here instead is write a separate test which consumes this fixture and the makes a worktree from that and call the original test -- it's going to be a lot easier to follow than this action-at-a-distance
There was a problem hiding this comment.
Good call! Initially I thought the parameterized fixtures could be a nice approach to consolidate the variants for all dependent tests, but I can definitely see how they can negatively impact maintainability in the long run.
errsyn
force-pushed
the
worktree_adjust_gitdir
branch
2 times, most recently
from
214e217 to
751ab24
Compare
tests/check_merge_conflict_test.py
Outdated
| worktree = f1_is_a_conflict_file.dirpath().join('worktree') | ||
| cmd_output('git', 'worktree', 'add', str(worktree)) | ||
| with worktree.as_cwd(): | ||
| cmd_output( | ||
| 'git', 'pull', '--no-rebase', 'origin', 'master', retcode=None, | ||
| ) | ||
| assert os.path.exists( | ||
| f1_is_a_conflict_file.join( | ||
| '.git', 'worktrees', 'worktree', 'MERGE_MSG', | ||
| ), | ||
| ) | ||
| yield worktree |
There was a problem hiding this comment.
I meant that you would inline this directly in your test above the call to the other test
no need to make a fixture if only one thing is using it
tests/check_merge_conflict_test.py
Outdated
| def test_not_in_a_repository(tmpdir): | ||
| with tmpdir.as_cwd(): | ||
| f1 = tmpdir.join('f1') | ||
| f1.write('foo\n') | ||
| assert main(['f1']) == 0 | ||
|
|
||
|
|
There was a problem hiding this comment.
I don't think this is a realistic test -- I expect these to always run inside a git repo
| @@ -13,12 +16,17 @@ | |||
|
|
|||
|
|
|||
| def is_in_merge() -> int: | |||
There was a problem hiding this comment.
this is probably my fault -- but can you change this to -> bool
errsyn
force-pushed
the
worktree_adjust_gitdir
branch
from
751ab24 to
f1a60ea
Compare
asottile
force-pushed
the
worktree_adjust_gitdir
branch
from
f1a60ea to
07af540
Compare
Fixes #638
Right now ongoing merges aren't detected in linked working trees, as it is assumed that
.gitis a directory. However, this is not the case for linked working trees. Working trees instead have a.gitfile, which references the path to the git directory like this:So when we are dealing with a linked working tree, we just have to readjust the git directory to the one referenced in the
.gitfile.