feat: allow diff to take a branch name as origin using worktree #655
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #655 +/- ##
==========================================
- Coverage 93.54% 93.50% -0.04%
==========================================
Files 20 20
Lines 2122 2125 +3
==========================================
+ Hits 1985 1987 +2
- Misses 137 138 +1 ☔ View full report in Codecov by Sentry. |
flux_local/tool/diff.py
Outdated
| @@ -213,8 +222,8 @@ def add_diff_flags(args: ArgumentParser) -> None: | |||
|
|
|||
| @contextmanager | |||
| def create_diff_path( | |||
| selector: git_repo.PathSelector, | |||
| **kwargs: Any, | |||
| selector: git_repo.PathSelector, | |||
There was a problem hiding this comment.
Can you remove whitespace changes? This is not the same format used by ruff so consider running the pre-commit also.
Also revert the other vertical whitespace changes if you don't mind.
There was a problem hiding this comment.
Running pre-commit seems to apply changes to things not modified by myself. I've removed all the space and spacing that I added.
flux_local/tool/diff.py
Outdated
| @@ -225,9 +234,10 @@ def create_diff_path( | |||
| yield git_repo.PathSelector(path_orig, sources=kwargs.get("sources")) | |||
| return | |||
|
|
|||
| with git_repo.create_worktree(selector.repo) as worktree: | |||
| yield git_repo.PathSelector(pathlib.Path(worktree) / selector.relative_path) | |||
| branch_orig = kwargs.get('branch_orig', None) | |||
There was a problem hiding this comment.
The None is not needed here as it is already the default value so prefer to write without it. While it makes it more explicit, this is a common enough paradigm that it its OK to be implicit.
| branch_orig = kwargs.get('branch_orig', None) | |
| branch_orig = kwargs.get('branch_orig') |
There was a problem hiding this comment.
Consider inlining the whole thing in the create_worktree branch if it can fit since its a trivial expression and the variable isn't used again.
flux_local/git_repo.py
Outdated
| """Create a ContextManager for a new git worktree in the current repo. | ||
|
|
||
| This is used to get a fork of the current repo without any local changes | ||
| in order to produce a diff. | ||
| """ | ||
| orig = os.getcwd() | ||
|
|
||
| with tempfile.TemporaryDirectory() as tmp_dir: | ||
| _LOGGER.debug("Creating worktree in %s", tmp_dir) | ||
| # Add --detach to avoid creating a branch since we will not make modifications |
There was a problem hiding this comment.
Push this comment inside the "branch is None" case
flux_local/git_repo.py
Outdated
| @@ -772,20 +772,26 @@ async def update_kustomization(cluster: Cluster) -> None: | |||
|
|
|||
|
|
|||
| @contextlib.contextmanager | |||
| def create_worktree(repo: git.repo.Repo) -> Generator[Path, None, None]: | |||
| def create_worktree(repo: git.repo.Repo, branch: str | None = None) -> Generator[Path, None, None]: | |||
There was a problem hiding this comment.
Can you add a comment in the pydoc that branch is an existing branch? (or variable could be named existing_branch). I just want to clarify here that it is not creating a new branch since much of the code below is about avoiding creating a new branch, implying that it may be creating a branch when passed in.
There was a problem hiding this comment.
I renamed the argument to existing_branch like you suggested and added a comment.
Following the discussion on discord, I've extended the behaviour of
create_worktreeto use a branch and not be detached. This allows us to compare two branch, the current andmainfor example easily.