Skip to content

Magit and Dired: fall back to current directory if not on file #110

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Magit and Dired: fall back to current directory if not on file #110

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions git-link.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,12 +391,14 @@ return (FILENAME . REVISION) otherwise nil."
(when (null filename)
(cond
((eq major-mode 'dired-mode)
(setq filename (dired-file-name-at-point)))
(setq filename (or (dired-file-name-at-point)
default-directory)))
((git-link--using-magit-blob-mode)
(setq filename magit-buffer-file-name))
((and (string-match-p "^magit-" (symbol-name major-mode))
(fboundp 'magit-file-at-point))
(setq filename (magit-file-at-point)))))
((string-match-p "^magit-" (symbol-name major-mode))
(setq filename (or (and (fboundp 'magit-file-at-point)
(magit-file-at-point))
default-directory)))))

(if (and dir filename
;; Make sure filename is not above dir, e.g. "/foo/repo-root/.."
Expand Down Expand Up @@ -797,6 +799,8 @@ With a double prefix argument invert the value of
(message "Remote `%s' contains an unsupported URL" remote))
((not (functionp handler))
(message "No handler found for %s" (car remote-info)))
((equal filename "")
(git-link-homepage remote))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR —with videos! 📼

Will have an in-depth look next week (please message if you do not hear from me!) but my initial thoughts on this line is that, all things being equal, this is a reasonable default but, one upgrading from existing versions could deem this a bug: "Link missing path". But, it is also possible I'm missing some broader context here that negates this.

There is also a plan for v1.0, where breaking changes will be make to existing defaults and this kinda thing may be more acceptable.

Thoughts?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the special case for repository root, but this also ended up discarding branch / commit information.

The main motivator for me to put the special case in is that, at least for GitHub, an empty path after /blob (.../blob/main/) causes a file not found error instead of redirecting to /tree (as is done for eg. .../blob/main/filename).

It's probably better do it properly, though, and adapt the handlers to return the right url for this.

;; TODO: null ret val
(t
(let ((vc-revison (git-link--parse-vc-revision filename)))
Expand Down