Detect if this is a merge or rebase in rebase_migration command #260
Conversation
|
I don't think this is the right approach. It would be better to fix django-linear-migrations to handle both cases. It's possible to detect if the current directory is a Git repository and whether there's a rebase or merge in progress with a few Git commands. |
|
Agree, it's not the fix for the command, it's just the less risky change that allows us to modify the command. I thought it would be simpler to add changes that don't change the logic. But I can make the existing command work with both rebase and merge. I think we can check the existence of the MERGE_HEAD file in the .git directory (in the same way as the git client does it. The question for me is how far the library should go to find out the path to the .git directory?
And in this case, there is also a question about what we should do if there is no Git repository? I think command should just fail with an error |
|
Option 2: use |
dmitrysleptsov
changed the title
| text=True, | ||
| ) | ||
| except (FileNotFoundError, subprocess.SubprocessError): | ||
| # Either `git` is not available or there is no git repository |
There was a problem hiding this comment.
If some error should be shown to user, this function can be moved under Command class
|
It's ready for review when you have time @adamchainz . I tested it, works as expected for both |
|
Great, thanks 👍. Can you update the docs to mention the automatic detection , and add a changelog note? I’ll properly review a bit later, if you can do those tasks it would help. |
| 2.8.0 (2023-05-28) | ||
| ------------------ | ||
|
|
||
| * Improve `rebase_migration` command to handle both rebasing and merging of the base branch |
There was a problem hiding this comment.
reStructuredText uses two backticks for code
| 2.8.0 (2023-05-28) | ||
| ------------------ |
There was a problem hiding this comment.
headers are inserted at release time... I should add a comment to this file to clarify that.
| def setUp(self) -> None: | ||
| subprocess_run_patch = mock.patch( | ||
| "django_linear_migrations.management.commands.rebase_migration" | ||
| ".subprocess.run" | ||
| ) | ||
| self.mock_subprocess_run = subprocess_run_patch.start() | ||
|
|
||
| self.addCleanup(subprocess_run_patch.stop) |
There was a problem hiding this comment.
I'm gonna rewrite these tests to not use mocking
| def is_merge_in_progress() -> bool: | ||
| try: | ||
| result = subprocess.run( | ||
| ["git", "rev-parse", "--git-dir"], | ||
| capture_output=True, | ||
| check=True, | ||
| text=True, | ||
| ) | ||
| except (FileNotFoundError, subprocess.SubprocessError): | ||
| # Either `git` is not available or there is no git repository, fall back to | ||
| # default behaviour | ||
| return False | ||
|
|
||
| git_dir = result.stdout.strip() | ||
| return Path(git_dir).joinpath("MERGE_HEAD").exists() |
There was a problem hiding this comment.
we can do better with git rev-parse --verify MERGE_HEAD, skipping getting the git dir and reading the internal files
|
I think your pull request did not allow me to push changes, so I have had to create a new one in #261. For future ref, please ensure the box is checked: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork |
Resolve #80. Adds logic to check if there is a merge request in progress. If so, handle it by swapping the migration files.