Show diff or file content in fzf_search_git_status #124
Conversation
functions/__fzf_git_diff.fish
Outdated
| # Other files, show the diff compared to last commit (unstaged & staged changes) | ||
| case '*' | ||
| git diff --color HEAD $argv[2] | ||
| end |
There was a problem hiding this comment.
My main concern now is this doesn't cover all the cases. Can you help me understand how this correctly covers all the cases?
There was a problem hiding this comment.
I'm not 100% sure it covers all the cases. It covers the one I found.
The switch match on a regex-like syntax:
case '\?\?': match the untracked files. Those can't be diff bygit diffbecause there are new files so there isn't any previous case to compare them to. Fallback on displaying the whole file.case '?D' 'D?': match files that are deleted, either staged or in the working tree.git diffdoesn't support those so we also fall-back on displaying the whole file as it is in the last commit (bat won't work because the file is no longer there).case '*': match any other file status. We display the diff compared with the last commit. This effectively combine the change in the staging area and the working tree in the preview.
There might be other statuses that a git diff can't properly display, I'm happy to add them if you know some. Otherwise we can add cases whenever we encounter new statuses.
There was a problem hiding this comment.
Actually I just found a new case when someone renames a file
There was a problem hiding this comment.
Hmm yeah I'd rather not introduce a feature that's useful 90% of the time but confuses or misleads the user the other 10%. Another problem related to covering all the cases is that it's not obvious to the user unless they analyze the code in depth whether the diff being shown is the diff against HEAD or against the staging area.
There was a problem hiding this comment.
the diff being shown is the diff against HEAD or against the staging area.
The diff is always against HEAD
I think this is quite a useful feature, I'm not sure what you mean by confuse or mislead in the other 10%. The actual git status of the file is displayed in the main fzf window.
There was a problem hiding this comment.
Sorry, let me think about it some more. In the meantime, would you mind figuring out the rename case?
There was a problem hiding this comment.
Yeah I already pushed a fix for that yesterday :)
There was a problem hiding this comment.
Full disclosure to be transparent and set expectations. Unfortunately, I am still against merging this as is. The code you wrote is great. Your idea is innovative. I really appreciate how much you've really owned this code and have been receptive to my feedback. This would be helpful for many users for many use cases. What I can't stomach, however, is that the preview works inconsistently, ambiguously, in a way that might be misunderstood.
If I were using it and had modified file F on staging and the work tree, and now was preparing to commit my staging area, I'd want to see the diff of F on staging vs F on HEAD. However, if it instead is showing me F on the work tree vs on HEAD, I might be misled to make the wrong decisions. And vice-versa. Basically, it can be confusing, not intuitive to use, even be misunderstood. So I'm going to think about this for another day, but unless there's a way we can show both staging diff and work tree diff and make it clear which one they're seeing, I'm probably not going to merge this.
There was a problem hiding this comment.
Yeah I understand, I agree the behaviour could be better. On the other hand some context is better than no context.
Feel free to close if you think it's not worth it.
There was a problem hiding this comment.
Thanks for understanding Quentin. I'm also disappointed by the technical limitations here and hope this doesn't discourage you from contributing to more OSS in the future. I know you and I would have been able to use it fine, but there are many other users who don't read the docs or aren't as familiar with git and as the author, I must account for their needs as well. If you figure out a way to show both staging and work tree diff, feel free to reopen!
505511b to
5a7d040
Compare
|
Hey @ovv I have an idea now! |
No description provided.