search

f-person / git-blame.nvim

Git Blame plugin for Neovim written in Lua
GNU General Public License v3.0
839 stars 41 forks source link

fix: Modify file URL commands to open relative latest commits #107

Closed bossley9 closed 8 months ago

bossley9 commented 9 months ago

Based on the conversation in https://github.com/f-person/git-blame.nvim/issues/103, we have decided to open file URLs using the SHA of the blame commit rather than the most recent upstream commit to prevent 404s when the upstream is out of date. I accomplished this by modifying get_blame_info to accept a range of lines and fetch the most recent blame from the lines provided, and modifying get_sha to accept an optional range of lines. Then I removed get_latest_sha.

bossley9 commented 9 months ago

After implementing this, I'm a bit hesitant for this to be the solution. Because we open the file URLs using the latest commit from the blame, the line numbers do not end up matching most of the time... which means you might see a blame for line X but in that commit, the same lines were located at line Y. It's a bit hit or miss because Git blame doesn't provide the original line the blame came from, so the best we can do is select the line(s) of the visual range, which is most often wrong :disappointed:

Example: If I try to open the blame file URL for these two lines of code, it correctly takes me to https://github.com/f-person/git-blame.nvim/commit/53562e0b18a077f55bd2afd64418ed34765c8c37, but the wrong lines are highlighted because the file has changed since then. These are the resulting highlighted lines.

I think we might actually want to keep using the latest SHA for these commands. Thoughts @f-person and @nrako?

f-person commented 9 months ago

Hey! Thanks for the PR; I'll try to check it soon!

Yeah, it makes sense; let's use the latest (not necessarily the latest remote) commit then; I think this behavior would be more expected.

This got me thinking… If we open the URL on line SHA, this wouldn't really match most use cases since a commit could be from 2 years ago, so the rest of the code won't be visible :/

What if we use the latest commit for everything but provide an option for using the blame commit instead (either through setup or an additional argument to commands, or both)

bossley9 commented 9 months ago

Yeah, I think it would be a good idea to default to the latest commit SHA for these commands but have the option to choose the line commit SHA if someone wanted that behavior 👍

bossley9 commented 8 months ago

@nrako are you ok with this solution? I also updated the documentation :slightly_smiling_face:

nrako commented 8 months ago

@bossley9 – this looks good to me 👍🏽