NeogitOrg / neogit

An interactive and powerful Git interface for Neovim, inspired by Magit
MIT License
4.04k stars 239 forks source link

(Git) Split Diff viewer #57

Closed mjlbach closed 3 years ago

mjlbach commented 3 years ago

90% of my fugitive use is :Gdiffsplit, it's the most useful git command (for me) by far.

I would love to see a method for viewing (and stepping through) diffs. Happy to contribute to this if you think it is in scope.

Unrelated but @RianFuro @TimUntersberger do you all actively lurk on our gitter/IRC? I'm a huge fan of this plugin and would like to make sure we're doing whatever we can in core to support your work.

TimUntersberger commented 3 years ago

What are the benefits of doing it in the proposed way instead of the existing solution?

mjlbach commented 3 years ago

What do you mean the existing way?

TimUntersberger commented 3 years ago

You can press tab when above a modified file to see the diff of the file like in magit. You can then also stage/unstage/discard a selected part, the hovered hunk or the whole file depending on the position of the cursor (Basically just like in magit).

mjlbach commented 3 years ago

Ah yes (sorry, I am familiar with this feature). Here is the difference:

Neogit + tab

image

:Gdiffsplit master

image

I use the second more for comparing across branches/making edits/transferring hunks across branches. No worries if it's not in scope, it's just the only reason I have fugitive installed alongside Neogit 😆

RianFuro commented 3 years ago

I do think there's definitely a place for a split diff view in neogit. I don't use it nearly as often as I should, but when I'm inside webstorm I really appreciate it.

This is also the part where we can't really stick to magit since it's extensively using ediff for that, so if fugitive's offering is worth being inspired by, I'm totally game.

TimUntersberger commented 3 years ago

Unrelated but @RianFuro @TimUntersberger do you all actively lurk on our gitter/IRC?

No.

I'm a huge fan of this plugin and would like to make sure we're doing whatever we can in core to support your work.

One thing that still annoys me a bit is that folded text is not highlighted like normal text. More on this can be found in the issue I created.

TimUntersberger commented 3 years ago

This feature needs some planning in my opinion. I feel like fugitive's version can be improved (though I don't have any concrete prove)

TimUntersberger commented 3 years ago

I played with the native diff viewer a bit and found :diffpatch which could make implementing this a lot easier, but this requires patch to be installed and it looks like windows doesn't have this by default. Does anyone have this exe on windows (Maybe I just deleted it accidently)?

TimUntersberger commented 3 years ago

@mjlbach I am still a bit confused about the workflow. Could you please write about what you would expect the split diff view to support and how you would like to see them realized?

akinsho commented 3 years ago

@TimUntersberger this is also something that keeps me using both fugitive and neogit so thought I'd help regarding a/my workflow if it helps with the implementation.

I personally use it primarily for reviewing PRs or reviewing my current branch against another. The way it works with fugitive is you can run GdiffSplit <target-commit-or-branch> and this will open a full screen 2 way diff of the current file.

For me it's the only way to review changes to a whole file beside another version outside of the browser. The ability to specify a target is also essential IMO since I often need to check the changes against a branch or commit not just vs. the branch I'm on.

It also uses scrollbind which I think is just how vim diff behaves anyway. If it has anymore advanced features than this I don't really use them tbh so maybe someone else can chime in on those. I just think as far as neogit is concerned there's no good way currently to see all the changes in one file in fullscreen given hunks in the status buffer don't apply and are good for a short overview but can't really be used for PR reviews.

One thing which I think has been raised many times in fugitive is the ability to review all changed files i.e. cycle through the diffs natively in fugitive but that was never implemented but would be to me the next logical step since ideally you want to review all the changes in the branch.

Hope this helps.

TimUntersberger commented 3 years ago

One thing which I think has been raised many times in fugitive is the ability to review all changed files i.e. cycle through the diffs natively in fugitive but that was never implemented but would be to me the next logical step since ideally you want to review all the changes in the branch.

This should be easy to add once we have the basic functionality for 1 file.

mjlbach commented 3 years ago

This is how I typically use :Gdiffsplit. The main utility is viewing file diffs against upstream repos, especially when reviewing a PR, where I frequently need to merge changes from the upstream repo before reviewing (or see the difference to know what changed). I made a video to illustrate this (Important part starts around :25):

https://user-images.githubusercontent.com/13316262/114320314-46fd2a80-9aca-11eb-9659-edd0fa24a095.mov

gennaro-tedesco commented 3 years ago

I too fully support the idea of having Gvdiff of some sort. At the moment this is the only reason I have not yet fully replaced fugitive with neogit (and I guess many other users do make use of the diff view, it is in general what git is for, after all).

I understand this is a big shot (and probably available for second iterations) but what could be also ideal would be the possibility to list files that are different between two reviews (perhaps in the quickfix window, for easy navigation and access)?

TimUntersberger commented 3 years ago

I think the first step is for me to complete the diff view of 1 file with sane default keybindings. Afterwards we can easily expand this to multiple files and changing the way the diff is displayed (ex.: instead of floating windows a tab)

TimUntersberger commented 3 years ago

@mjlbach have you checked out the PR yet? The diff view currently is just an alternative to <tab>. The next step would be using the diff to look at a diff between branches. (Maybe this should be a separate popup like in magit)

Shatur commented 3 years ago

There is an awesome diffview.nvim. Maybe just integrate this plugin?

akinsho commented 3 years ago

I think having a native solution that integrates with the rest of neogit is valuable despite there being another external solution, potentially a neogit native solution can also integrate with the rest of neogit.

Rather having to handle these two things separately if you can then open full screen diffs from neogit hunks or it could integrate with a commit viewer or the log viewer etc. 🤷🏾‍♂️ Also a lot of this work has already been done in #100

Shatur commented 3 years ago

I think having a native solution that integrates with the rest of neogit is valuable despite there being another external solution, potentially a neogit native solution can also integrate with the rest of neogit.

External plugin could be integrated in the same way. Such integration is much easier to maintain. Personally, I don't think it makes sense to reinvent the wheel. So I decided to suggest the solution.

TimUntersberger commented 3 years ago

I am trying to talk to @sindrets about a few questions I have to see whether integrating the plugin is possible.

I am also in favor of integrating the plugin instead of writing our own version.

Issue in diffview: #20

TimUntersberger commented 3 years ago

I created a PR for tracking the process here: #124

It looks like this should be soon ready to merge. The integration works, but it is still a bit slow, because diffview has to redo our calculations ,because it doesn't know about us. We are working on an API that supports providing the diff content ourselves, which will greatly reduce the load time.

mjlbach commented 3 years ago

I've been following diffview closely, I really like the interface! Looking forward to seeing this in neogit :)

akinsho commented 3 years ago

Just tried this out and it works really nicely @TimUntersberger thanks for all your hard work on this also @sindrets 🙌🏾

Shatur commented 3 years ago

When no item in NeogitStatus is selected then I get an error: E5108: Error executing lua ...onfig/nvim/pack/plugins/opt/neogit/lua/neogit/status.lua:730: attempt to index local 'item' (a nil value)