magit-wip-mode doesn't work over Tramp

[rallyemax]

What happened?

Using magit-wip-mode over Tramp never results in after-save commits.

What did you expect to happen?

When magit-wip-mode is enabled, the globalized minor mode magit-wip-after-save-mode should enable magit-wip-after-save-local-mode in all buffers via the turn-on function magit-wip-after-save-local-mode-turn-on.

Steps to reproduce

1. Open a git-tracked file over Tramp.
2. Enable magit-wip-mode in the buffer.
3. Evaluate the buffer local variable after-save-hook. I posit that it will not contain magit-wip-commit-buffer-file
4. Evaluate (magit-file-tracked-p buffer-file-name). I posit that it will return nil.
5. (Unnecessary) Make a change to a git-tracked file and call magit-wip-log-current. There will be no WIP commits. This step is unnecessary because given steps 3 and 4, a WIP commit cannot happen.

System information

Magit 97a95f7 (3.3.0.50), but also tested head of main branch)
Git 2.41.0
Emacs 29.0.92 (I run Doom, but I also tested with vanilla Emacs)
Archlinux

Backtraces are not useful here because no errors result from the Tramp bugs in magit-wip.el.

Analysis

The turn-on function magit-wip-after-save-local-mode-turn-on only enables magit-wip-after-save-local-mode if the following is true: (magit-file-tracked-p buffer-file-name). But it will never be true with that argument in a Tramp buffer that is visiting a file over ssh/scp, because buffer-file-name will contain a Tramp connection prefix like "ssh:", and the underlying git binary will not recognize such a path as being tracked.

Similar Tramp issues have been addressed and resolved elsewhere in the magit codebase, for example in this commit, where the call has been changed to (magit-file-tracked-p (file-relative-name file)). This one does return t when visiting a git-tracked file via Tramp. There is also this commit, which created the function magit-convert-filename-for-git specifically to address Tramp filenames (the commit renamed a pre-existing function and added functionality to it). This function seems like what is needed in magit-wip.el.

It looks like the offending code in magit-wip was modified recently (on 1 April 2023). The commit message doesn’t mention the Tramp issue and the commit is part of a performance-related refactoring, but it looks like some references to the buffer file go through file-relative-name now. But the issue remains and there is still a call to (magit-file-tracked-p buffer-file-name)).

Attempted fix

I replaced the two occurrences in magit-wip.el of (magit-file-tracked-p buffer-file-name) with (magit-file-tracked-p (magit-convert-filename-for-git buffer-file-name). It seems to be working, but it makes me nervous to create a PR not knowing the codebase well. But if it helps, I'm happy to create a PR, even if only to initiate a discussion and review of my proposed changes.

(Edit) In particular, it feels to me that the fix should be to magit-file-tracked-p. Rather than having to modify paths passed to this function all over the codebase to account for Tramp, magit-file-tracked-p could detect that the passed path is a Tramp connection string/path and make the necessary adjustment itself. But this is a pretty low-level utility function used all over the codebase and I do not have a sufficiently comprehensive understanding of magit's internals to make the fix safely.

[tarsius]

Thanks for the report and detailed analysis.

It should be safe enough to teach that function (and magit-file-ignored-p for consistency) to always convert the filename.