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 e94b6ebf (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 e94b6ebf, 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 e94b6ebf (or with magit--diff-use-recorded-type-p set to nil) those two behave the same (don't allow reversing).

[kyleam]

Sorry for the delay.

Recap of the situation:

• before e94b6eb

  • in a buffer produced by magit-diff-range or magit-diff-working-tree and a revision that resolves to HEAD, calling magit-reverse aborts with "Cannot reverse unstaged changes" and calling magit-apply aborts with "Change is already in the working tree". magit-stage and magit-discard are permitted.

  • in a buffer produced by magit-diff-working-tree or magit-diff-working-tree with a revision that does not resolve to HEAD, magit-reverse and magit-apply are permitted, and magit-stage and magit-discard are not.

• after e94b6eb

  • in a buffer produced by magit-diff-range with any revision, calling magit-reverse and magit-apply are permitted, and calling magit-stage is not. For the single revision form (i.e. diff with the working tree), the magit-apply call will fail (as the changes are always already in the working tree). magit-discard is not permitted.

  • in a buffer produced by magit-diff-working-tree with any revision, calling magit-reverse aborts with "Cannot reverse unstaged changes" and calling magit-apply aborts with "Change is already in the working tree". magit-stage and magit-discard are permitted.

---

My opinion is that buffers produced by magit-diff-range or magit-diff-working-tree, for any revision, should always allow magit-reverse. Even when the revision resolves to HEAD, I think reverse should be allowed for magit-diff-working-tree because the underlying diff command (git diff HEAD) is not limited to unstaged changes. It also includes working tree changes that have been staged.

A minimal change (perhaps the way to go given nearing release) would be this:

diff --git a/lisp/magit-diff.el b/lisp/magit-diff.el
index 5ad87868..5aeba13c 100644
--- a/lisp/magit-diff.el
+++ b/lisp/magit-diff.el
@@ -1235,7 +1235,7 @@ (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))
+  (magit-diff-setup-buffer (or rev "HEAD") nil args files 'committed))

 ;;;###autoload
 (defun magit-diff-staged (&optional rev args files)

The committed type, like unstaged, isn't quite right, but at least it allows applying, and it matches what magit-diff-range currently does.

It has the advantage of allowing reversing and disallowing staging and discarding. Disallowing staging/discarding seems like a good idea given the stream of unstaged, staged, and (if revision isn't HEAD) committed things the git diff <revision> buffer may show.

The disadvantage is that it now allows magit-apply, which will always fail for magit-diff-working-tree.

Another option would be to introduce a new diff type for these working tree diffs, updating magit-diff-working-tree to always use it and magit-diff-range to use it if its rev-or-range arg is a single revision. Functionally the main advantage I see with this approach is that magit-apply could then know its a diff against the working tree and abort rather than letting the git apply fail.

patch

```diff diff --git a/lisp/magit-apply.el b/lisp/magit-apply.el index 7ad55e14..31cf5543 100644 --- a/lisp/magit-apply.el +++ b/lisp/magit-apply.el @@ -116,7 +116,7 @@ (defun magit-apply (&rest args) (interactive (and current-prefix-arg (list "--3way"))) (when-let ((s (magit-apply--get-selection))) (pcase (list (magit-diff-type) (magit-diff-scope)) - (`(,(or 'unstaged 'staged) ,_) + (`(,(or 'unstaged 'staged 'wtree) ,_) (user-error "Change is already in the working tree")) (`(untracked ,(or 'file 'files)) (call-interactively #'magit-am)) @@ -301,7 +301,8 @@ (defun magit-stage (&optional intent) ('(unstaged list nil) (magit-stage-modified)) (`(staged ,_ ,_) (user-error "Already staged")) (`(committed ,_ ,_) (user-error "Cannot stage committed changes")) - (`(undefined ,_ ,_) (user-error "Cannot stage this change"))) + (`(,(or 'wtree 'undefined) ,_ ,_) + (user-error "Cannot stage this change"))) (call-interactively #'magit-stage-file))) ;;;###autoload @@ -441,7 +442,8 @@ (defun magit-unstage () (`(committed ,_ ,_) (if magit-unstage-committed (magit-reverse-in-index) (user-error "Cannot unstage committed changes"))) - (`(undefined ,_ ,_) (user-error "Cannot unstage this change"))))) + (`(,(or 'wtree 'undefined) ,_ ,_) + (user-error "Cannot unstage this change"))))) ;;;###autoload (defun magit-unstage-buffer-file () @@ -515,7 +517,8 @@ (defun magit-discard () (when-let ((s (magit-apply--get-selection))) (pcase (list (magit-diff-type) (magit-diff-scope)) (`(committed ,_) (user-error "Cannot discard committed changes")) - (`(undefined ,_) (user-error "Cannot discard this change")) + (`(,(or 'wtree 'undefined) ,_) + (user-error "Cannot discard this change")) (`(,_ region) (magit-discard-region s)) (`(,_ hunk) (magit-discard-hunk s)) (`(,_ hunks) (magit-discard-hunks s)) diff --git a/lisp/magit-diff.el b/lisp/magit-diff.el index 5ad87868..379784e3 100644 --- a/lisp/magit-diff.el +++ b/lisp/magit-diff.el @@ -1224,7 +1224,9 @@ (defun magit-diff-range (rev-or-range &optional args files) (interactive (cons (magit-diff-read-range-or-commit "Diff for range" nil current-prefix-arg) (magit-diff-arguments))) - (magit-diff-setup-buffer rev-or-range nil args files 'committed)) + (magit-diff-setup-buffer + rev-or-range nil args files + (if (magit-commit-p rev-or-range) 'wtree 'committed))) ;;;###autoload (defun magit-diff-working-tree (&optional rev args files) @@ -1235,7 +1237,7 @@ (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)) + (magit-diff-setup-buffer (or rev "HEAD") nil args files 'wtree)) ;;;###autoload (defun magit-diff-staged (&optional rev args files) ```

[tarsius]

A minimal change (perhaps the way to go given nearing release) would be this:

Sounds good to me.

The disadvantage is that it now allows magit-apply, which will always fail for magit-diff-working-tree.

Maybe we can even live with that. Many changes cannot be applied; it's kinda expected that that frequently happens. I don't see much value in detecting that beforehand and preventing it in this one case, instead of just trying and failing, as we would in many other cases. Then again, we do disallow some other apply-variants when we know that they would fail, so I most likely could be convinced otherwise.

[kyleam]

Thanks @tarsius.

I don't see much value in detecting that beforehand and preventing it in this one case, instead of just trying and failing

I agree. I don't think it really matters whether we abort early or let the apply fail.

Will push out the change soon.