Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Copy to cliboard fails if the file has changed #21

Closed
akinsho opened this issue Jul 5, 2021 · 4 comments · Fixed by #24
Closed

Copy to cliboard fails if the file has changed #21

akinsho opened this issue Jul 5, 2021 · 4 comments · Fixed by #24
Assignees
Labels
bug Something isn't working

Comments

@akinsho
Copy link

akinsho commented Jul 5, 2021

Describe the bug
A clear and concise description of what the bug is:

  • What were you trying to do?
    Whilst trying to copy a line to the clipboard in a file I had changed but not committed the plugin errors showing.
E5108: Error executing lua ...ite/pack/packer/opt/gitlinker.nvim/lua/gitlinker/git.lua:286: Failed get appropriate revision: '.config/nvim/lua/as/plugins/init.lua' has changed relative to 'HEAD': '.config/nvim/lua/as/plugins/init.lua' has changed relative to '@{u}': could not retrieve revision for 'origin'
  • What was the expected result?
    Error if the line is changed, but if the line is not changed then use the permalink from HEAD, since that part of the code is still unchanged in HEAD and so another developer would be able to use the link.
    :GBrowse does not fail in these situations I believe and uses HEAD as well I think.

*What was the actual result?
If there has been any change at all to a file, any attempts to copy from it fail.

To Reproduce

  • Provide a minimal lua configuration that reproduces the bug
vim.cmd [[packadd! gitlinker.nvim]]
vim.cmd [[packadd! plenary.nvim]]

require('gitlinker').setup {}
  • Describe the steps to reproduce the behavior:
  1. Go to a file in a git repo that has no changes.
  2. Use the default mapping and observe that it works correctly.
  3. Change any line, and jump to another unchanged line
  4. Attempt to use the mapping again -> See error

Expected behavior
I believe that in the case of fugitive it does not throw an error if the file has been changed
I believe it must employ some heuristic to figure out the last sensible commit to base the permalink off presumably HEAD.

Screenshots
If applicable, add screenshots to help explain your problem.

System (please complete the following information):

  • OS: Ubuntu 20.04, macOS Big Sur
  • neovim --version: 0.5
  • git --version: 2.25.1

Additional context
Add any other context about the problem here.

@akinsho akinsho added the bug Something isn't working label Jul 5, 2021
@ruifm
Copy link
Owner

ruifm commented Jul 5, 2021

The issue I see with this is if the some lines were changed before the current position, the resulting line number would be meaningless since the URL would point to some other location in the file.

E.g.:

a
b
c
d

If you changed the file by repeating the first 3 lines:

a
a
a
a
b
c
d

And you have your cursor at c, the URL will not make sense at all since the generated URL would have line number 6 instead of 3 which is wrong.

The actual way to solve this would be to parse the git diff and do some arithmetics to figure out the right line numbers, which is not something trivial.

I'm not sure fugitive's :GBrowse also did not fail but I will take your word. What I am sure of is that they do not parse the diff nor do any line arithmetic computations because I've looked at their code.
I vaguely remember that when I used :GBrowse, sometimes it would generate invalid URLs in these scenarios.

IMO, that is incorrect behavior.

The way I see it, we can either:

  • Fully support this by parsing git diff for the buffer, or leveraging an existing plugin such as gitsigns and compute the remote's equivalent line ranges. That will take sometime to get right, PRs/help welcome (including possibly devolping a public hunk API for gitsigns, see Add function to list/export all hunks lewis6991/gitsigns.nvim#242)
  • Do it halfway: if the buffer has changed, generate an URL without line ranges, aka best-effort solution. This should be very trivial and I can try it sometime this week. I'm wondering what can break in case the file was renamed/moved....

@akinsho
Copy link
Author

akinsho commented Jul 5, 2021

@ruifm thanks for the detailed and timely response. Tbh the proper solution whilst ideal seems quite high effort. IMO a small step in the right direction would be a less red screen of death like error, something maybe unobtrusive that just says "This plugin doesn't work if the file has been changed" rather than the trace etc.

I think in terms of actually solving the issue though, tbh I think a guesstimation is not that bad, since I think a bit of common sense and documentation can overcome some of the difficulty. In a lot of situations the URL might be valid, and if it isn't, I think a user who has been modifying the file knows they've likely done it to themselves, and it's best effort. More often than not I'd rather take my chances in this case since if it returns a URL linking to line 25 instead of 28 at least by this point it's open to almost the correct place in my browser and at this point I can just click on the correct line and proceed 🤷🏾‍♂️ rather than the alternative of stashing my changes or more likely, opening the browser and navigating manually.

EDIT: should have said a URL without line ranges is also perfectly acceptable, since you can then still get something usable you can add the correct range to in the browser if needed.

I think the fix can also be iterative as better solutions become more feasible, rather than an immediate need to parse diffs etc.

@ruifm ruifm changed the title Copy to cliboard fails if the fail has changed at all Copy to cliboard fails if the file has changed Jul 7, 2021
ruifm added a commit that referenced this issue Jul 7, 2021
@ruifm
Copy link
Owner

ruifm commented Jul 7, 2021

@akinsho I will try #24 a little bit more today, if nothing is wrong, it should fix this issue.

Later I will add support for lines in changed files using diff hunks, now I have a grasp on the best way to do it.

@akinsho
Copy link
Author

akinsho commented Jul 8, 2021

@ruifm I'll give it a try as well. Thanks for working on it 👍🏾

@ruifm ruifm closed this as completed in #24 Jul 9, 2021
stevanmilic pushed a commit to stevanmilic/gitlinker.nvim that referenced this issue Nov 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants