Show diff for renamed file when filtering by path #4750
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ile with renames The test shows that selecting a commit before the rename shows an empty diff, and selecting the rename itself shows an added file rather than a rename.
The combination of --abbrev=40 and %h can be shortened to %H.
I can only guess what happened here: in aa750c0, this code to manually load the reflog commits was added, to make sorting branches by recency work when the reflog is filtered by path. At that time we didn't have separate ReflogCommits and FilteredReflogCommits models yet. Then, FilteredReflogCommits was introduced (in 8822c40), probably for the very purpose of being able to get rid of this again; but then it was forgotton to actually get rid of it. The funny thing is that the introduction of FilteredReflogCommits happened in the very next commit, 15 minutes later.
Using the filtered one is probably not a good idea. It didn't do much harm because the split of ReflogCommits and FilteredReflogCommits doesn't really work right now (FilteredReflogCommits is always the same as ReflogCommits, even in filtering mode), but we'll fix this in the next commit.
Recycle reflog commits only for the non-filtered ones. If we're not filtering, FilteredReflogCommits and ReflogCommits are the same. If we then enter filtering and get reflog entries again, and pass a lastReflogCommit, we'd recycle the previous FilteredReflogCommits, which are unfiltered, so that's no good. Work around this by simply never using the recycle mechanism when getting the filtered reflog commits. We could do better by remembering what the last filter path or author was, and only suppressing the recycling when it changed; but that's more complexity than I want to add, so hopefully this is good enough.
I don't know why this function argument was added, but I don't like unnecessary indirections, so I'm removing it as SubCommitsHelper has access to everything it needs to do it itself.
It is good practice to use a -- argument even if no further arguments follow. Doesn't really make a difference for this particular command though, I think.
These are very similar in that they call RunAndProcessLines on a git log command and then process lines to create commits. Extract this into a common helper function. At the moment this doesn't gain us much, but in the next commit we will extend this helper function considerably with filter path logic, which would otherwise have to be duplicated in both places.
When filtering for a file path we use the --follow option for "git log", so it will follow renames of the file, which is great. However, if you then selected one of the commits before a rename, you didn't see a diff, because we would pass the original filter path to the "git show" call. To fix this, call git log with the --name-status option when filtering by path, so that each commit reports which file paths are touched in this commit; remember these in the commit object, so that we can pass them to the "git show" call later. Be careful not to store too many such paths unnecessarily. When filtering by folder rather than file, all these paths will necessarily be inside the original filter path, so detect this and don't store them in this case. There is some unfortunate code duplication between loading commits and loading reflog commits, which I am too lazy to clean up right now.
When shift-selecting a range of commits across a file rename in filtering-by-path mode, the diff currently shows an added file rather than a renamed file. Add a test that demonstrates this, we'll fix this in the next commit.
We need to pass the union of filter paths at both ends of the range.
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When filtering by file path for a file that was renamed at some point, we show all commits even before the rename, which is great; however when you selected a commit before the rename, the diff was empty. The commit for the rename itself would show the file as added rather than renamed.
This PR fixes that; along the way, we fix the filtered reflog display, which is supposed to filter by path like the commits view does, but this didn't work.
Fixes #4510.