When doing worktree diff with a commit, apply/reverse doesn't work in the diff buffer

[geza-herman]

When doing a worktree diff with a commit (d C-u w), then neither magit-apply (a) nor magit-reverse (v) works in the resulting diff buffer on a hunk. Error messages are "Change is already in the working tree" and "Cannot reverse unstaged changes". I think that the problem is that magit thinks that the diff buffer is a diff between the current worktree and HEAD. But it's not, it's a diff between the current worktree and some other commit, so applying and reverse'ing make sense. I remember using this feature before, it used to work.

Magit version is 20240211.1712.

[kyleam]

I remember using this feature before, it used to work.

Looks like that change came with e94b6eb (Record diff-type in magit-diff-mode buffers, 2023-03-18). To confirm, you can set magit--diff-use-recorded-type-p to nil.

I haven't reviewed the commit or the linked issue (related to performance) closely but perhaps something like this

diff --git a/lisp/magit-diff.el b/lisp/magit-diff.el
index f14288de..213d136a 100644
--- a/lisp/magit-diff.el
+++ b/lisp/magit-diff.el
@@ -1253,7 +1253,9 @@ (defun magit-diff-working-tree (&optional rev args files)
    (cons (and current-prefix-arg
               (magit-read-branch-or-commit "Diff working tree and commit"))
          (magit-diff-arguments)))
-  (magit-diff-setup-buffer (or rev "HEAD") nil args files 'unstaged))
+  (setq rev (or rev "HEAD"))
+  (magit-diff-setup-buffer rev nil args files
+                           (if (magit-rev-head-p rev) 'unstaged 'committed)))
 
 ;;;###autoload
 (defun magit-diff-staged (&optional rev args files)

[geza-herman]

Thanks, I can confirm that this is the problem. Using 'commited for the last parameter, magit works correctly, so I suppose this patch is correct.

A related question: if the diff is between HEAD and worktree ("d w" is used without "C-u"), shouldn't magit allow reversing? Currently it says "Cannot reverse unstaged changes". I understand that this is basically a discard operation (so allowing it would be redundant), but still. We have a diff buffer (shouldn't matter where the buffer came from), I'd like to apply/reverse a specific hunk. I understand that magit warns me when applying, because if it proceeded, apply would surely fail, there is no point trying it. But reversing would succeed. If I run "d r" (diff range), and enter "HEAD", I get the exact same diff buffer as "d w". And in this diff buffer reversing is allowed, and it does what I'd expect. Why doesn't magit just apply/reverse a hunk without being smart about it? I understand that discard needs to be clever, because discard only makes sense in certain kind of diff buffers. But apply/reverse makes sense in almost all kinds diff buffers. It just needs to apply/revert the hunk to the worktree. And if the operation will surely fail, then it makes sense to disallow it.

[kyleam]

[ sorry, just a quick reply ]

if the diff is between HEAD and worktree ("d w" is used without "C-u"), shouldn't magit allow reversing?

The (magit-rev-head-p rev) condition in the patch aims to preserve the behavior before e94b6eb, but yeah, I think that's a good question to think about here. That's one of the things I hope to have a more solid take on after with some digging.

If I run "d r" (diff range), and enter "HEAD", I get the exact same diff buffer as "d w".

Ah, before e94b6eb (or with magit--diff-use-recorded-type-p set to nil) those two behave the same (don't allow reversing).