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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconvenient Lua API #1030

Open
Iskustvo opened this issue Jun 4, 2024 · 4 comments
Open

Inconvenient Lua API #1030

Iskustvo opened this issue Jun 4, 2024 · 4 comments
Labels
enhancement New feature or request

Comments

@Iskustvo
Copy link

Iskustvo commented Jun 4, 2024

Description

Thank you for this plugin and sorry for the long post (I wrote it locally, before seeing the template 馃槥 )!
I'm well aware that this contains a mix of bugs, feature requests and questions for help, but I honestly believe
that as a whole, it should help you better understand struggles and frustration of everyday users with current API.
Please bear with me on this one, and if you prefer, I'll later split it into smaller issues (whichever you consider "in scope").

Goal

I want to create <Enter> mapping which will:

  • In case when line under cursor isn't commited, show popup with such message.
  • Otherwise, open new Vim tab and show commit's changes (diff) which introduced line under cursor.
  • Work "recursively", i.e., be able to use this keymap on a line in parent's snapshot to go further down the blame history.

Check existing support (technical breakdown)

  • In case when line under cursor isn't commited, show window with such message -> gitsigns.blame_line
  • Blame the line under cursor and get commit SHA -> gitsigns.blame_line ?
  • Open snapshot of a file in blamed commit -> gitsigns.show
  • Turn that snapshot into before/after diff -> gitsigns.diffthis

It seems quite reassuring that this is "in scope" of the plugin and should be easy to implement, right?

Implementation

Blame the line under cursor and get commit SHA

Unfortunately, gitsigns.blame_line seems to be the only API available for the job.
Issues:

  • It doesn't just return blamed line as string, but it opens a popup window which has it.
  • It's asynchronous (but at least accepts "then" callback for chaining operations).

Because of these, the only way to extract blamed commit SHA via Gitsigns is the following:

-- Open popup window with commit of blamed line.
gitsigns.blame_line(
    { full = false },
    function()
        -- In order to focus opened popup window, blame_line needs to be called again.
        gitsigns.blame_line(
            {},
            function()
                -- Now that popup is focused, extract commit SHA from the start of it.
                local blamed_commit = vim.fn.getline(1):match("^(%x+)")

                -- Close the focused popup.
                vim.cmd(":quit")

                -- If commit is unavailable (i.e. local changes), reopen the popup window, but without focus.
                if blamed_commit == nil then
                    gitsigns.blame_line()
                    return
                end

                -- Rest of the code that has to be executed synchronously after blamed commit is extracted.
            end)
    end)

Preferred solution: Having something like gitsigns.get_blame_line which will synchronously return blamed line for opened file snapshot.

Open snapshot of a file in blamed commit

Here, gitsigns.show does just what we want, right?
Issues:

  • It's also asynchronous, but unlike other functions, it doesn't accept callback which will be executed upon completion.
  • Even if it did accept callback, it's hard to define when it should be invoked:
    • After buffer with snapshot was opened and focused?
    • After Gitsigns attaches to it?
    • After Gitsigns correctly sets the base to snapshot's parent?

Because of this, inconvenient code is forced on us again:

-- Open popup window with commit SHA of blamed line.
gitsigns.blame_line(
    { full = false },
    function()
        -- In order to focus opened popup window, blame_line needs to be called again.
        gitsigns.blame_line(
            {},
            function()
                -- Now that popup is focused, extract commit SHA from the start of it.
                local blamed_commit = vim.fn.getline(1):match("^(%x+)")

                -- Close the focused popup.
                vim.cmd(":quit")

                -- If commit is unavailable (i.e. local changes), reopen the popup window, but without focus.
                if blamed_commit == nil then
                    gitsigns.blame_line()
                    return
                end

                -- Create new tab which is viewing current version of the file.
                vim.cmd(":tabnew %")

                -- React to opening of the gitsigns.show (below).
                vim.api.nvim_create_autocmd("User", {
                    pattern = "GitSignsUpdate",
                    callback = function()
                        -- Rest of the code that has to be executed synchronously after gitsigns.show.
                    end,
                    once = true
                })

                -- Open and focus blamed version of the file in new tab.
                gitsigns.show(blamed_commit)
            end)
    end)

Turn that snapshot into before/after diff

Again, this also looks like a thing that can't go wrong. We just have to diffthis, right?

Issues:

  • In order to successfully perform diffthis, "Gitsigns" had to already finish all of its configuration magic, and there's no specific event for that.
  • You may have noticed GitSignsUpdate event in last code snippet, but that one is fired multiple times (at least twice) for every new opened file, either by a bug or design.
  • Reacting only on N-th occurrence of this event is just asking for trouble down the road, so that's why we're waiting only for first one, to know that at least Gitsigns is available in that buffer.
  • Because we are reacting only to first occurrence (which might not have its base correctly setup), we have to specify revision manually - gitsigns.diffthis(blamed_commit .. "^")
  • Unfortunately, for reasons unknown to me, this doesn't work recursively, i.e., it misbehaves on second attempt to go deeper in blame history.

Only thing that fixed "recursive" behavior was doing diffthis synchronously after manually changing the base:

-- Open popup window with commit SHA of blamed line.
gitsigns.blame_line(
    { full = false },
    function()
        -- In order to focus opened popup window, blame_line needs to be called again.
        gitsigns.blame_line(
            {},
            function()
                -- Now that popup is focused, extract commit SHA from the start of it.
                local blamed_commit = vim.fn.getline(1):match("^(%x+)")

                -- Close the focused popup.
                vim.cmd(":quit")

                -- If commit is unavailable (i.e. local changes), reopen the popup window, but without focus.
                if blamed_commit == nil then
                    gitsigns.blame_line()
                    return
                end

                -- Create new tab which is viewing current version of the file.
                vim.cmd(":tabnew %")

                -- React to opening of the gitsigns.show (below).
                vim.api.nvim_create_autocmd("User", {
                    pattern = "GitSignsUpdate",
                    callback = function()
                        -- Diff blamed version of the file with its parent's version of the same file.
                        gitsigns.change_base(blamed_commit .. "^", false, gitsigns.diffthis)
                    end,
                    once = true
                })

                -- Open and focus blamed version of the file in new tab.
                gitsigns.show(blamed_commit)
            end)
    end)

Test environment

  • Execute the following bash script to create dummy git repo for testing in /tmp:
mkdir /tmp/gitsigns_test
cd /tmp/gitsigns_test
git init
echo -e "aaa\nbbb\nccc" > file
git add file
git commit -m "add 3 lines"
echo -e "aaaa\nbbbb\nccc" > file
git commit -am "modify first two lines"
echo -e "aaaa\nbbbbb\ncccc" > file
git commit -am "modify last two lines"
  • Use minimal.lua when testing.

Outstanding bugs

Wrong caching (I assume)

Steps to reproduce:

  • Open dummy file from testing directory in Neovim.

  • Navigate to last line (cccc).

  • Press key that will execute our mapping (<Enter> for me).

  • Observe that newly created Vim tab has correct diff and that each buffer has correct "signs".
    2024-06-04-193449_1920x1062_scrot

  • Close the tab with diff: :tabclose

  • Observe that "signs" in the original file are "copied" from our showed snapshot and don't represent the correct state.
    2024-06-04-194209_1920x1062_scrot

  • Execute :Gitsigns blame_line on marked lines.

  • Observe that Gitsigns shows "Not Commited Yet" message, even though there are no local (nor staged) changes.
    2024-06-04-194346_1920x1062_scrot

Initial commits are missing signs and cannot be blamed.

Steps to reproduce:

  • Open dummy file from testing directory in Neovim.
  • Navigate to first line (aaaa).
  • Press key that should execute our mapping (<Enter> for me).
  • Observe that newly created Vim tab has correct diff, but buffer with initial commit (left window) is missing "signs".
    2024-06-04-195005_1920x1062_scrot
  • Navigate to snapshot of the initial commit.
  • Execute :Gitsigns blame_line on any line.
  • Observe that command was just ignored, it didn't spawn popup window, even though all lines were added in that commit.

NOTE: After retesting with minimal.lua, I see the following error, which doesn't popup otherwise.

Error executing luv callback:
...fig/nvim/gitsigns_issue/gitsigns//lua/gitsigns/async.lua:85: The coroutine failed with this message: ...onfig/nvim/gitsigns_issue/gitsigns//lua/gitsigns/git.lua:516: (ERROR) ...onfig/nvim/gitsigns_issue/gitsigns//lua/gitsigns/git.lua(516
): fatal: Not a valid object name fff6a8ba^^

This makes perfect sense given that fff6a8ba^ is initial commit and fff6a8ba^^ is therefore invalid.
But it again makes me stuck because the only way to avoid this is to manually invoke git and check this edge case.
Gitsigns doesn't provide me any way to handle it gracefully.

Neovim version

NVIM v0.10.0

Operating system and version

Arch Linux (x86_64) Kernel: 6.9.2-arch1-1

Expected behavior

No response

Actual behavior

Everything is in description.

Minimal config

for name, url in pairs {
    gitsigns = 'https://github.com/lewis6991/gitsigns.nvim',
} do
    local install_path = vim.fn.fnamemodify('gitsigns_issue/' .. name, ':p')
    if vim.fn.isdirectory(install_path) == 0 then
        vim.fn.system { 'git', 'clone', '--depth=1', url, install_path }
    end
    vim.opt.runtimepath:append(install_path)
end

require('gitsigns').setup {
    debug_mode = true, -- You must add this to enable debug messages
    on_attach = function(buffer_number)
        local gitsigns = require('gitsigns')
        vim.keymap.set(
            'n',
            '<Enter>',
            function()
                -- Open popup window with commit SHA of blamed line.
                gitsigns.blame_line(
                    { full = false },
                    function()
                        -- In order to focus opened popup window, blame_line needs to be called again.
                        gitsigns.blame_line({},
                            function()
                                -- Now that popup is focused, extract commit SHA from the start of it.
                                local blamed_commit = vim.fn.getline(1):match("^(%x+)")

                                -- Close the focused popup.
                                vim.cmd(":quit")

                                -- If commit is unavailable (i.e. local changes), reopen the popup window, but without focus.
                                if blamed_commit == nil then
                                    gitsigns.blame_line()
                                    return
                                end

                                -- Create new tab which is viewing current version of the file.
                                vim.cmd(":tabnew %")

                                -- React to opening of the gitsigns.show (below) and diff it.
                                vim.api.nvim_create_autocmd("User", {
                                    pattern = "GitSignsUpdate",
                                    callback = function()
                                        -- Diff blamed version of the file with its parent's version of the same file.
                                        gitsigns.change_base(blamed_commit .. "^", false, gitsigns.diffthis)
                                        -- gitsigns.diffthis(blamed_commit .. "^")
                                    end,
                                    once = true
                                })

                                -- Open and focus blamed version of the file in new tab.
                                gitsigns.show(blamed_commit)
                            end)
                    end)
            end,
            { buffer = buffer_number, desc = "[Enter] the diff mode between current's line commit and its parent" })
    end
}

Steps to reproduce

Everything is in description.

Gitsigns debug messages

No response

@Iskustvo Iskustvo added the bug Something isn't working label Jun 4, 2024
@lewis6991 lewis6991 added enhancement New feature or request and removed bug Something isn't working labels Jun 7, 2024
@lewis6991
Copy link
Owner

Looks like you are trying to use the functions outside their intended use.

Consider using vim.system({'git', ...}):wait() directly.

@Iskustvo
Copy link
Author

Iskustvo commented Jun 13, 2024

Well, yeah, since Lua API is designed for convenient mapping, not programming :(

While I can use git directly, given that I still like and want to have this plugin, it feels like a bad idea:

  • I would have to repeat same git commands which Gitsigns will also do independently.
  • It's quite inconvenient to implement caching which already exists in Gitsings (and it would be bad to duplicate it).
  • Maintaining and debugging edge cases (e.g. like initial commit) in user config also doesn't seem too appealing.

I would like to split this into following more concrete issues:

  • [Enhancement] Expose Lua API with gitsigns.get_blame_line() which will synchronously return blamed line for opened file snapshot
  • [Enhancement] Refine Gitsigns events so that user can accurately react on Gitsigns commands/functions (e.g. after gitsigns.show() has opened a file snapshot, attached itself to it and set appropriate base)
  • [Enhancement] Extend {base}/{revision} parameters to also accept special value "parent" which will correctly handle plugin behavior for initial commits, i.e. in diffthis() it should:
    • In case of normal file -> Open diff between current version of the file and version of parent's commit (if it exists).
    • In case of Gitsigns's file snapshot from different commit -> Open diff between that snapshot and the one from its parent's commit (if it exists).
    • In case of initial commit (detected like this and hopefuly cached) -> Open popup with message like "Initial commit - Cannot diff parent".
  • [Bug] gitsigns.blame_line() incorrectly shows "Not Committed Yet" for lines in showed file snapshots that were modified by parent commit
  • [Bug] When Gitsigns attaches to a snapshot of a file in initial commit, "signs" don't indicate that all lines were added in that commit, as they do for all other commits (compared to their parent's snapshot)

Of course, this whole "feature" (minus the new Vim tab) can be internally implemented inside of plugin and exposed as a function, but I'm not sure how many people would use it, so it's questionable.
On the other hand, I really feel like Gitsings should have building blocks to allow anyone to implement this in efficient/convenient way, given the scope that it already has.

What do you think, should I proceed and create proposed issues?

@lewis6991
Copy link
Owner

lewis6991 commented Jun 13, 2024

You can create issues, but I'll consider each separately, in conjunction with all other issues and feature requests.

There would also be no timeline for these either since I use this plugin mostly for myself and have limited time.

@Iskustvo
Copy link
Author

No problem, I fully understand the situation you're in.
Regardless, thank you for taking the time for this!

I guess I'll just spawn smaller tickets aiming to achieve this mapping as I go, given that behavior is constantly changed by your work. At the very least, your last commit solved this one already:

[Bug] When Gitsigns attaches to a snapshot of a file in initial commit, "signs" don't indicate that all lines were added in that commit, as they do for all other commits (compared to their parent's snapshot)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants