mini.nvim icon indicating copy to clipboard operation
mini.nvim copied to clipboard

Beta-testing 'mini.diff'

Open echasnovski opened this issue 1 year ago • 51 comments

Please leave your feedback about new mini.diff module here. Feel free to either add new comment or positively upvote existing one.

Some things I am interested to find out (obviously, besides bugs):

  • Does it feel to work smoothly (both with regular visualization and with overlay)?
  • Is configuration intuitive enough?
  • Are default values of settings convenient for you?
  • Is documentation and examples clear enough?

Thanks!

echasnovski avatar Mar 28 '24 16:03 echasnovski

Seems good and works well... config for 'signs' or 'numbers' is a bit confusing on what the syntax is supposed to look like when combined with the vim.o.number text already there...

bdillahu avatar Mar 28 '24 19:03 bdillahu

Seems good and works well... config for 'signs' or 'numbers' is a bit confusing on what the syntax is supposed to look like when combined with the vim.o.number text already there...

I am not sure I understand the issue. Currently default style is "number' if line number are enabled at the time of require('mini.diff').setup() and 'sign' otherwise. If style is 'sign', actual sign text can be configured in view.signs. Hope it helps.

echasnovski avatar Mar 28 '24 19:03 echasnovski

I understand I believe, but what syntax to use is unclear to me.

This is in the config information:

    -- Visualization style. Possible values are 'sign' and 'number'.
    style = vim.o.number and 'number' or 'sign',

do I put:

    style = vim.o.number and 'sign',

or

    style = 'sign',

to use the sign? (trial and error has it working with the first of my options, but in the spirit of beta feedback on the docs, I thought I would point out where I found it confusing.

bdillahu avatar Mar 28 '24 19:03 bdillahu

I understand I believe, but what syntax to use is unclear to me.

If you want to always use 'sign', then:

require('mini.diff').setup({ view = { style = 'sign' } })

Conversely, if you want to always use 'number', then:

require('mini.diff').setup({ view = { style = 'number' } })

echasnovski avatar Mar 28 '24 20:03 echasnovski

Hi, and as always i love your plugins. I just tried it out and Hunk staging is not working for me. I am able to reset hunks though. Is there anything specific i have to look out for. Also resetting hunks saves my buffer and writes to it after resetting. Is there anyway i can prevent that by only resetting and not saving the file.

231tr0n avatar Mar 29 '24 07:03 231tr0n

Hi, and as always i love your plugins.

:heart:

I just tried it out and Hunk staging is not working for me. I am able to reset hunks though. Is there anything specific i have to look out for.

Sigh... If visualization is done properly, then it will be not easy to debug. If that is the case, my guess is that there is an error during computing or applying patch which gets silently ignored (because I assume/hope that the code will always work). So chances are, there is an error somewhere here. Do you have time/energy/enthusiasm to see where the issue occurs exactly?

First, I'd start with asking what Git version do you have? Second, I'll add some logging behavior here and possibly here to see if there is a non-zero exit code (signalling that there were some error).

Also resetting hunks saves my buffer and writes to it after resetting. Is there anyway i can prevent that by only resetting and not saving the file.

Initially I did not like that it makes the buffer modified and prompts a save. Besides, 'gitsigns.nvim' seems to also not make buffer modified when resetting buffers. But you know what... After you mentioned it, it is a text change which should make buffer be modified. I'll run locally with it reverted to see if there are any other issues.

echasnovski avatar Mar 29 '24 09:03 echasnovski

Hi, and as always i love your plugins.

❤️

I just tried it out and Hunk staging is not working for me. I am able to reset hunks though. Is there anything specific i have to look out for.

Sigh... If visualization is done properly, then it will be not easy to debug. If that is the case, my guess is that there is an error during computing or applying patch which gets silently ignored (because I assume/hope that the code will always work). So chances are, there is an error somewhere here. Do you have time/energy/enthusiasm to see where the issue occurs exactly?

First, I'd start with asking what Git version do you have? Second, I'll add some logging behavior here and possibly here to see if there is a non-zero exit code (signalling that there were some error).

Also resetting hunks saves my buffer and writes to it after resetting. Is there anyway i can prevent that by only resetting and not saving the file.

Initially I did not like that it makes the buffer modified and prompts a save. Besides, 'gitsigns.nvim' seems to also not make buffer modified when resetting buffers. But you know what... After you mentioned it, it is a text change which should make buffer be modified. I'll run locally with it reverted to see if there are any other issues.

Hi yeah i am intrested in debugging this out. This is my git version git version 2.34.1 Let me know what else i should be doing Also just to mention i use delta as my pager to view git diffs if that matters here.

231tr0n avatar Mar 29 '24 11:03 231tr0n

git version 2.34.1

That should be enough. It looks like it does not support --format option in git ls-files. To double check, would you mind executing the following command from the root of some repo (replace path/to/some/file with actual path to some file inside a repo):

git ls-files --full-name --format="%(objectmode) %(path)" -- path/to/some/file

According to Git documentation, the --format option appeared in 2.38.0, which was released in October 2022. So this command should show an error for you, correct?

I presume you use the one which comes in Ubuntu 22.04? For the time being, updating version of Git should fix things.

echasnovski avatar Mar 29 '24 12:03 echasnovski

git version 2.34.1

That should be enough. It looks like it does not support --format option in git ls-files. To double check, would you mind executing the following command from the root of some repo (replace path/to/some/file with actual path to some file inside a repo):

git ls-files --full-name --format="%(objectmode) %(path)" -- path/to/some/file

According to Git documentation, the --format option appeared in 2.38.0, which was released in October 2022. So this command should show an error for you, correct?

I presume you use the one which comes in Ubuntu 22.04? For the time being, updating version of Git should fix things.

yes you are right i am using ubuntu 22.04 in wsl2. Will try updating git first and try staging hunks again.

231tr0n avatar Mar 29 '24 12:03 231tr0n

git version 2.34.1

That should be enough. It looks like it does not support --format option in git ls-files. To double check, would you mind executing the following command from the root of some repo (replace path/to/some/file with actual path to some file inside a repo):

git ls-files --full-name --format="%(objectmode) %(path)" -- path/to/some/file

According to Git documentation, the --format option appeared in 2.38.0, which was released in October 2022. So this command should show an error for you, correct?

I presume you use the one which comes in Ubuntu 22.04? For the time being, updating version of Git should fix things.

Yup updating git version fixes the issue. You were right. I added this ppa:git-core/ppa and updated git and staging works now.

But how do i unstage a staged hunk. Like i tried gH and it does not work. It throws out an error saying no hunks found. Also no special colors are shown for CursorLineNr for a staged hunk to identify and unstage it.

231tr0n avatar Mar 29 '24 12:03 231tr0n

Yup updating git version fixes the issue. You were right. I added this ppa:git-core/ppa and updated git and staging works now.

Glad it works now!

But how do i unstage a staged hunk. Like i tried gH and it does not work. It throws out an error saying no hunks found. Also no special colors are shown for CursorLineNr for a staged hunk to identify and unstage it.

This is not supported. Mostly because I could not find a reasonable abstraction for this action which can be applied for any source of reference text.

echasnovski avatar Mar 29 '24 12:03 echasnovski

Yup updating git version fixes the issue. You were right. I added this ppa:git-core/ppa and updated git and staging works now.

Glad it works now!

But how do i unstage a staged hunk. Like i tried gH and it does not work. It throws out an error saying no hunks found. Also no special colors are shown for CursorLineNr for a staged hunk to identify and unstage it.

This is not supported. Mostly because I could not find a reasonable abstraction for this action which can be applied for any source of reference text.

Yup cool thank you for this wonderful plugin once again.

231tr0n avatar Mar 29 '24 12:03 231tr0n

Great plugin as always!

Couple of questions.

Your plugins has the vim.b.minidiff_summary_string which is the equivalent of vim.b.gitsigns_status in gitsigns.nvim. Is there a equivalent for vim.b.gitsigns_head to display current branch in the statusline?

Is the current line blame functionality out of the scope of this plugin?

Ernest1338 avatar Mar 29 '24 16:03 Ernest1338

Your plugins has the vim.b.minidiff_summary_string which is the equivalent of vim.b.gitsigns_status in gitsigns.nvim. Is there a equivalent for vim.b.gitsigns_head to display current branch in the statusline?

Is the current line blame functionality out of the scope of this plugin?

The answer to both of these is that it is planned as a part of 'mini.git'.

Having current head information provided by 'mini.diff' does not really align with its whole design of having configurable source. There is no reasonable abstraction for Git's HEAD in generic source of reference text.

echasnovski avatar Mar 29 '24 16:03 echasnovski

Continuing the discussion from Reddit since the code formatting here is much better:

Here's what I came up with for the branch name (I added a simple caching mechanism for performance improvements). However, I couldn't implement your second suggestion of using vim.schedule_wrap beforehand because it caused some issues, and I'm not entirely sure why. Here's a breakdown of the code and steps:

  1. Check if it's a valid file and if it's within a git repository before proceeding.
  2. Retrieve the branch name for the buffer and cache it.
  3. If the branch name is already cached, there's no need to call git again.
  4. Clear the cache if the directory changes.

I didn't expect to end up writing around 170 lines of code just because I switched from one plugin to another, but it was quite an interesting experience.

I have another question regarding mini.map, will MiniMap have something like gen_integration.minidiff similar to the functionality of gitsigns ?https://github.com/echasnovski/mini.nvim/blob/0ec3216d3224ebd6828e5637fa9a10a00ba67113/lua/mini/map.lua#L864

Code:

local function is_valid_git_repo(buf_id)
	-- Check if it's a valid buffer
	local path = vim.api.nvim_buf_get_name(buf_id)
	if path == "" or vim.fn.filereadable(path) ~= 1 then
		return false
	end

	-- Check if the current directory is a Git repository
	if vim.fn.isdirectory(".git") == 0 then
		return false
	end

	return true
end

local branch_cache = {}

-- Function to clear the Git branch cache
local function clear_git_branch_cache()
	-- Clear by doing an empty table :)
	branch_cache = {}
end

-- Autocommand to clear the Git branch cache when the directory changes
vim.api.nvim_create_autocmd("DirChanged", {
	callback = clear_git_branch_cache,
})

local function update_git_branch(data)
	if not is_valid_git_repo(data.buf) then
		return
	end

	-- Check if branch is already cached
	local cached_branch = branch_cache[data.buf]
	if cached_branch then
		vim.b.git_branch = cached_branch
		return
	end

	local stdout = vim.uv.new_pipe(false)
	local handle, pid
	handle, pid = vim.uv.spawn(
		"git",
		{
			args = { "-C", vim.fn.expand("%:p:h"), "branch", "--show-current" },
			stdio = { nil, stdout, nil },
		},
		vim.schedule_wrap(function(code, signal)
			if code == 0 then
				stdout:read_start(function(err, content)
					if content then
						vim.b.git_branch = content:gsub("\n", "") -- Remove newline character
						branch_cache[data.buf] = vim.b.git_branch -- Cache the branch name
						stdout:close()
					end
				end)
			else
				stdout:close()
			end
		end)
	)
end

-- Call this function when the buffer is opened in a window
vim.api.nvim_create_autocmd("BufWinEnter", {
	callback = update_git_branch,
})

thank you

bassamsdata avatar Mar 29 '24 17:03 bassamsdata

Here's what I came up with for the branch name (I added a simple caching mechanism for performance improvements). However, I couldn't implement your second suggestion of using vim.schedule_wrap beforehand because it caused some issues, and I'm not entirely sure why.

Thanks for posting it here!

I was wrong in suggesting that that function is better be extracted out of the call because it uses stdout, which is created on every vim.loop.spawn() call. Better formatting made me see this :)

I have another question regarding mini.map, will MiniMap have something like gen_integration.minidiff similar to the functionality of gitsigns ?

Yes, it is planned. Along with similar 'mini.statusline' updates.

echasnovski avatar Mar 29 '24 17:03 echasnovski

@231tr0n, the latest main now treats resetting hunks as a regular text edit, i.e. it does not save file to preserve 'modified' (as was before). Also there is a clarification about Git minimum version 2.38.0 for applying/staging hunks. Thanks for the feedback!

echasnovski avatar Mar 29 '24 18:03 echasnovski

Seems good and works well... config for 'signs' or 'numbers' is a bit confusing on what the syntax is supposed to look like when combined with the vim.o.number text already there...

For the record, I agree this was a bit confusing for me too. But I think one of the confusing parts was my own assumption that you could choose to do both sign and number at same time. So I was trying to understand how I could do that.

So my comment here is to 1) agree that it was a little confusing and 2) to request the ability to choose both line number coloring and signs at the same time (I like it to be super obvious haha).

Also as one more point of feedback on the confusion part... while I know a bit of Lua from hacking around, non technical people / non lua folks may be confused by the vim.go.number and 'number' or 'sign'.

GitMurf avatar Mar 29 '24 19:03 GitMurf

@echasnovski initial feedback:

Overall, Awesome! I have been able to disable gitsigns because mini.diff can do most of the important things I cared about with gitsigns. The only 3 remaining items that I would love to see to make my "transition" complete are:

  1. As stated in my previous comment, be able to turn on both number and signs for the view style.
  2. Have a "hover" preview option to show a single hunk diff. Essentially show what the overlay shows but in a hover that I can quickly view without turning on the entire overlay feature on/off.
  3. A "rolling" goto meaning if I goto "next" and it hits the last hunk in my buffer, if i do next again it brings me back to the top to continue to cycle through. And same with reverse if I use previous to go to the very top and then have it cycle to the bottom in a looping fashion. Currently it sends a notification saying "no more hunks in that direction".

Again, awesome work! Everything else seems to be working very nicely.

GitMurf avatar Mar 29 '24 19:03 GitMurf

@GitMurf, thanks for the feedback!

Seems good and works well... config for 'signs' or 'numbers' is a bit confusing on what the syntax is supposed to look like when combined with the vim.o.number text already there...

For the record, I agree this was a bit confusing for me too. But I think one of the confusing parts was my own assumption that you could choose to do both sign and number at same time. So I was trying to understand how I could do that.

So my comment here is to 1) agree that it was a little confusing and 2) to request the ability to choose both line number coloring and signs at the same time (I like it to be super obvious haha).

No, I don't think this is a reasonable feature. Although actual implementation is pretty straightforward, actual design of how users enable that will be somewhat confusing. It would have to be a set of booleans like "view.signs = true/falseandview.line_number = true/false`, which I don't like that much.

Also as one more point of feedback on the confusion part... while I know a bit of Lua from hacking around, non technical people / non lua folks may be confused by the vim.go.number and 'number' or 'sign'.

I can see that. That's why there is a comment above it: "Default: 'number' if line numbers are enabled, 'sign' otherwise.".

  1. Have a "hover" preview option to show a single hunk diff. Essentially show what the overlay shows but in a hover that I can quickly view without turning on the entire overlay feature on/off.

No, I don't see it be valuable enough to warrant separate functionality. Besides, going away from "preview single hunk" was one of the goals of 'mini.diff' for me: I didn't really like its workflow. So toggling overlay is the method to preview hunks in 'mini.diff'.

  1. A "rolling" goto meaning if I goto "next" and it hits the last hunk in my buffer, if i do next again it brings me back to the top to continue to cycle through. And same with reverse if I use previous to go to the very top and then have it cycle to the bottom in a looping fashion. Currently it sends a notification saying "no more hunks in that direction".

The wrapping around edges is a tough one. I'll duplicate my answer from Reddit:

I spent too much time weighing pros and cons of either wrap should be enabled by default and/or should it be configurable.

Recently I've changed my mind from "wrap is always better" to "wrap sometimes harms the workflow". In this particular instance I do believe that wrapping brings more harm than good. Mostly because it is usually a bit disorienting having to spend fraction of a second to deduce whether it was the last hunk or not. So there is no wrap by default.

Making it configurable is certainly possible, but it would need a slightly more complex and not as straightforward code as there is now.

So at least for the time being, there will be no configurable wrapping, sorry.

echasnovski avatar Mar 29 '24 19:03 echasnovski

Thanks for your responses to my feedback.

As for the last item, wrapping around edges... would it be reasonable enough to have MiniDiff.goto() return a value? either true if successfull or false if "edge of file"? because then I could have my keymap be a function that when doing goto next, if it returns false, then I can do a goto "first" following it because I know I am at the bottom of the file and therefore rolling over would just be jumping to the first hunk. Does that make sense? thoughts?

GitMurf avatar Mar 29 '24 20:03 GitMurf

As for the last item, wrapping around edges... would it be reasonable enough to have MiniDiff.goto() return a value? either true if successfull or false if "edge of file"? because then I could have my keymap be a function that when doing goto next, if it returns false, then I can do a goto "first" following it because I know I am at the bottom of the file and therefore rolling over would just be jumping to the first hunk. Does that make sense? thoughts?

It is already kind of possible with a more straightforward approach. It can be checked if cursor actually moved after goto_hunk('next'), i.e. 1) cache cursor position; 2) execute goto_hunk('next'); 3) check if cursor has changed and if not - do goto_hunk('first'). This will still produce a notification about no hunks in direction "next", but this might be a good thing.

echasnovski avatar Mar 29 '24 20:03 echasnovski

@231tr0n, the latest main now treats resetting hunks as a regular text edit, i.e. it does not save file to preserve 'modified' (as was before). Also there is a clarification about Git minimum version 2.38.0 for applying/staging hunks. Thanks for the feedback!

Awesome thank you for the change.

231tr0n avatar Mar 29 '24 20:03 231tr0n

Love the plug-in, specifically the overlay toggle. Two comments / questions for you:

  • Because the overlay is one of the coolest features of the plugin, I'm curious why you chose not to provide a mapping. The plug-in provides several other mappings. I mapped mine to \D as I use mini.basics and it fits with the rest of the other user toggle-able mappings defined there.
  • I rely on CursorLineNr (I bold mine) solely to identify the line number my cursor is currently on (cursorlineoptr = "number"), but when I use view = { style = "number" } }, there is no means to identify what line number I'm on anymore. Is there a way to prioritize CursorLineNr over MiniDiffSign* highlight groups? Or, even better, any chance of getting MiniDiffSignAddNr, MiniDiffSignChangeNr, and MiniDiffSignDeleteNr highlight groups?

pkazmier avatar Mar 30 '24 15:03 pkazmier

  • Because the overlay is one of the coolest features of the plugin, I'm curious why you chose not to provide a mapping. The plug-in provides several other mappings. I mapped mine to \D as I use mini.basics and it fits with the rest of the other user toggle-able mappings defined there.

No objective reason per se. Just that this type of mapping for me fits more as the <Leader> mapping (I use <Leader>go), which is a no go as default mapping.

  • I rely on CursorLineNr (I bold mine) solely to identify the line number my cursor is currently on (cursorlineoptr = "number"), but when I use view = { style = "number" } }, there is no means to identify what line number I'm on anymore. Is there a way to prioritize CursorLineNr over MiniDiffSign* highlight groups?

Me too ('mini.hues' also uses bold), but I don't think there is a way to do this with current Neovim functionality. I'd expect setting the lowest priority for extmarks (in 'mini.diff' via config.view.priority) would solve the issue, but it does not. Integrating Vim's UI highlighting and extmark highlighting is hard.

Also, both cursor and cursorline should be enough to quickly identify where cursor is at.

That said, I think overriding is even not the best outcome here, as those should combine. I.e. line number of the current and added line should be bold green in this case. I have opened an issue in Neovim for that (exactly as the result of what we discuss here): neovim/neovim#27555.

Or, even better, any chance of getting MiniDiffSignAddNr, MiniDiffSignChangeNr, and MiniDiffSignDeleteNr highlight groups?

I am not sure why this is better or even that useful at all. Using MiniDiffSignXXX groups for both "sign" and "number" styles seems reasonable because in both cases the target is in the gutter (right hand are with line information) and mostly need to highlight the text.

echasnovski avatar Mar 30 '24 15:03 echasnovski

Thanks for this plugin. I never used gitsigns or similar plugins before (didn't really think I needed them). But for some reason, I just tried this and I realize it is actually quite useful.

I've customized mini.diff to use these signs:

  view = {
    signs = {
      add = "▕▏",
      change = "▕▏",
      delete = "▁▁"
    },
  },

One thing I noticed: gitsigns.nvim have some additional signs that you did not implement yet. Specifically, the topdelete sign comes to mind. I think perhaps it makes sense to add that one as well to mini.diff, e.g.:

  view = {
    signs = {
      add = "▕▏",
      change = "▕▏",
      delete = "▁▁"
      topdelete = "▔▔"
    },
  },

lervag avatar Mar 30 '24 16:03 lervag

One thing I noticed: gitsigns.nvim have some additional signs that you did not implement yet. Specifically, the topdelete sign comes to mind. I think perhaps it makes sense to add that one as well to mini.diff

Thanks for the suggestion!

This sign is only useful when first lines are deleted in order to be able to distinguish whether deleted lines were above the current first line or below. Other than that, "delete" sign always indicates that some text was deleted below the line with the visualization.

I think this is not that useful feature as it is usually either obvious from the current text or can be confirmed via overlay. So it was intentionally not added to reduce config options.

echasnovski avatar Mar 30 '24 17:03 echasnovski

This sign is only useful when first lines are deleted in order to be able to distinguish whether deleted lines were above the current first line or below. Other than that, "delete" sign always indicates that some text was deleted below the line with the visualization.

I think this is not that useful feature as it is usually either obvious from the current text or can be confirmed via overlay. So it was intentionally not added to reduce config options.

I agree it's not very useful. But during testing, I realized it was quite annoying in the edge case where it is useful. I'll respect if you choose to not implement it, of course. But I thought it was useful to lift this as something I noticed as a beta tester. :)

lervag avatar Mar 30 '24 17:03 lervag

I'm experiencing an issue with the overlay feature where it doesn't work if I delete the first line of text in the buffer alone or with a paragraph below it. However, if I exclude the first line, then it works as expected. the video demonstrating the issue is below:

https://github.com/echasnovski/mini.nvim/assets/105807570/60722576-5688-4e18-bf49-29b9b5c58ed6

bassamsdata avatar Mar 30 '24 18:03 bassamsdata

I'm experiencing an issue with the overlay feature where it doesn't work if I delete the first line of text in the buffer alone or with a paragraph below it. However, if I exclude the first line, then it works as expected. the video demonstrating the issue is below:

This is a current Neovim behavior of virtual lines being placed above the first line. This is documented and needs manual scroll with <C-y> for them to become visible. Unfortunately, there is no proper approach to handle this at the moment.

echasnovski avatar Mar 30 '24 18:03 echasnovski