Support a three-way diff for regular code review

[upbqdn]

When I have staged changes, and I open Ediff by pressing E, and then s for staging, I get a three-window ediff view. The right-most window is then a regular buffer with line numbers, and LSP enabled, but it's still synced with the other two windows when I jump between edits.

This is great for reviewing Rust code and probably code in many other languages because it allows me to display docs, jump to definitions, and all do sorts of other shenanigans.

However, I'm not able to get the three-window ediff when I want to do a regular review.

What I've been doing for about a year when reviewing code is that I open the diff (either magit-diff or ediff), and when I need to jump to a certain definition, I either close the whole diff to look it up, or I open a new window which is independent of the diff, so the process is always clumsy.

How do others look up definitions when they review code?

[upbqdn]

If anyone would be able to describe how this could be done, I might give it a try and implement it.

[tarsius]

This is great for reviewing Rust code and probably code in many other languages because it allows me to display docs, jump to definitions, and all do sorts of other shenanigans.

Do you know why you can do that in the buffer in which you can do it but not in others?

However, I'm not able to get the three-window ediff when I want to do a regular review.

Which commands perform "regular review"?

[upbqdn]

Which commands perform "regular review"?

I usually open magit-diff by pressing dd on a commit or a whole branch, and when the code change is complicated, I press e inside magit-diff to open ediff.

Do you know why you can do that in the buffer in which you can do it but not in others?

I don't think I do, unfortunately. I guess the buffers in both magit-diff and ediff don't represent actual files on the disk that LSP could take and analyze, but the third extra buffer in ediff represents a real file that is on the disk under the branch that is currently checked-out.

I've been using Magit for about a year, and I always missed that I couldn't interact with the code in diffs. I only recently noticed that when I press Es on an unstaged change and select a file, I get one buffer that represents git's HEAD, one that represents git's index, and one with the current file in the working tree. Also, I get a buffer that represents the current file when I only press e on an unstaged change --- this brings up two buffers --- git's index, and the current file. Would it be possible to do this when I'm diffing a file from magit where one part is identical to the file in the working tree?

[tarsius]

That's the dwim command and in this context it will run magit-ediff-show-commit.

For a regular commit (with just one parent) there are only two sides; the commit and its parent. There is no third side that would make sense here.

In this case the two ediff buffers don't visit files that can be found on the file system. Instead they are populated with content retrieved from history. buffer-file-name is nil in these buffers and the commands that fail probably use the value of that variable. Some of these commands could probably be changed to feed the displayed version of the file to the underlying tool on stdin instead of trying to give them a file that doesn't actually exist.

Doing this for every command would of course be painful, but in the short term that is likely your only option and if you only have to do it for one or two commands it would still be feasible.

I don't intend to do anything in the short to mid term to make this easier. I do however still plan to implement git-handler (see #2958). But that too would only be a step in the right direction. It wouldn't magically enable this without extra work. But once I have implemented that I would be motivated again to work on things like this. Currently I am not because it would all have to be thrown out and reimplemented once I start work on git-handler.

[upbqdn]

Some of these commands could probably be changed to feed the displayed version of the file to the underlying tool on stdin instead of trying to give them a file that doesn't actually exist.

Ok, thank you.

I have one more unrelated question though: I noticed I'm not able to stage unstaged changes via magit-ediff-stage. All changes are in buffer C which represents a file in git's working tree, and I'm not able to move them to buffer B, which represents git's index. This doesn't seem like the intended behavior. I'm able to stage changes in magit-diff-dwim as usual.

Btw, I added some details to my comment above about half an hour after posting it.

[kyleam]

I'm not able to stage unstaged changes via magit-ediff-stage

Trying quickly with the tip of master (3e833f3), I don't notice any issues. It's probably worth filing an issue with detailed steps to reproduce.

[upbqdn]

I'll try it again properly, and open an issue if it still doesn't work. I'm using Doom atm.