Skip to content

Conversation

refractalize
Copy link
Contributor

I noticed that the :DiffviewOpen ..commit --imply-local didn't show all the diffs I was expecting, and only shows diffs for files that were modified in the working copy - or indeed no diffs if the working copy is clean.

:DiffviewOpen commit --imply-local does work, the only difference being that it puts the commit files on the left, not the right.
Also, :DiffviewOpen origin/master..commit --imply-local fails in the same way as :DiffviewOpen ..commit --imply-local, but only when your checked out commit is the same as origin/master (this is what confused me originally!)

Anyway, it turns out we were just missing a case where the left-hand-side of the diff is local and the right-hand-side is a commit, so I've added that in this PR.

Copy link
Owner

@sindrets sindrets left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

You have identified an issue here, but this solution is not quite right. The rev range arg ..{REV} is a shorthand for HEAD..{REV} (= HEAD^ {REV}). This will not produce the correct diff. In fact, given that Git has no way of describing the current state of the working tree (what we call LOCAL), there is no way in Git's rev syntax to describe the rev range you want (LOCAL as the old state, compared against {REV} as the new state).

I don't think it's particularly meaningful to support specifying these ranges in this specific way, given that you get the exact same diff by just doing {REV}, except the two states will be swapped.

But on the other hand, granted that I added this non-standard --imply-local behavior, it should certainly behave correctly according to how it's documented. So I agree that we should support the type of rev range you're trying to use here.

But it's not as straight forward as just changing this rev_to_args() method. Like I mentioned, Git has no way of expressing LOCAL..{REV}. So I think we need to do something a little nasty here. In the case where we have one of these non-standard ranges, we need to run a diff against {REV}, and then swap the left and right side.

@sindrets
Copy link
Owner

Let me have a look at it actually. I think we can fix this quite easily.

@sindrets sindrets changed the title fix :DiffviewOpen ..commit --imply-local=true fix: Handle rev ranges that resolve to LOCAL..{REV} Nov 20, 2023
@sindrets sindrets merged commit 3dc498c into sindrets:main Nov 20, 2023
@sindrets
Copy link
Owner

Thanks!

@refractalize
Copy link
Contributor Author

Thanks @sindrets !

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants