Add new function magithub-browse-file #373

manateelazycat · 2018-09-16T03:41:17Z

Open git file in user browser

magithub-browse just open git repo
magithub-browse-file is location file link in browser.

Open git file in user browser magithub-browse just open git repo magithub-browse-file is location file link in browse.

manateelazycat · 2018-09-16T03:42:07Z

@vermiculus
Hi, your magithub is really cool.

I write a new function magithub-browse-file , please review code.

Thanks for magithub! ;)

Jump to file with line number if you in buffer of git file. Jump to file if you in dired mode. Jump to parent directory if you not in buffer with non-dired mode.

vermiculus · 2018-09-16T13:46:33Z

Thanks! I fixed some issues with the test infrastructure that were preventing your PR from being tested. Can you rebase onto master?

vermiculus · 2018-09-16T13:47:45Z

magithub.el

+  (interactive)
+  (unless (magithub-github-repository-p)
+    (user-error "Not a GitHub repository"))
+  (let* ((repo (magithub-repo))


Use (let-alist (magithub-repo) …) instead of this let* form. Then, you can refer to (alist-get 'html_url repo) as simply .html_url. This is the pattern used throughout the rest of magithub.

Done, please review newest version.

vermiculus · 2018-09-16T13:49:44Z

magithub.el

+    (user-error "Not a GitHub repository"))
+  (let* ((repo (magithub-repo))
+         (html-url (alist-get 'html_url repo))
+         (branch (alist-get 'default_branch repo))


What if we're on a local branch that is not the default branch? If I'm on some branch B, presumably we should browse that file as it exists on branch B (i.e., github.com/user/repo/blob/B/file.txt).

Done, please review newest version.

vermiculus · 2018-09-16T13:50:54Z

magithub.el

+  (let* ((repo (magithub-repo))
+         (html-url (alist-get 'html_url repo))
+         (branch (alist-get 'default_branch repo))
+         (github-branch-path (apply 'concat (mapcar 'file-name-as-directory (list html-url "blob" branch))))


I think we can safely assume the directory separator that browsers will respect will be / – you can simplify this into a call to (format …) (or mapconcat if you prefer).

Done, please review newest version.

vermiculus · 2018-09-16T13:51:16Z

magithub.el

+         (file-relative-path (if (buffer-file-name)
+                                 ;; Get file relative path and line number if `buffer-file-name' is non-nil.
+                                 (concat (buffer-file-name) "#L" (prin1-to-string (line-number-at-pos)))
+                               (if (equal major-mode 'dired-mode)


Use derived-mode-p.

Done, please review newest version.

vermiculus · 2018-09-16T13:54:01Z

This PR will implement requested feature #280.

Hi, vermiculus This version improve code with: ``` 1. Use magit-get-current-branch fetch current branch instead "master" branch. 2. Use format instead file-name-as-directory since browser always use / as path separator. 3. Use derived-mode-p instead (equal major-mode ...) 4. Use html_url need use alist-get, so not use let-alist. Thanks for tips. ;) 5. Use expand-file-name wrap default-directory, otherwise path that start with ~ will break code. ```

manateelazycat · 2018-09-16T14:29:42Z

@vermiculus
I have rebase new version, please review code, thanks! ;)

vermiculus · 2018-09-16T14:44:11Z

magithub.el

+  (interactive)
+  (unless (magithub-github-repository-p)
+    (user-error "Not a GitHub repository"))
+  (let* ((github-branch-path (format "%s/%s/%s/"


Use let-alist.

Something like this?

(github-branch-path (let-alist (magithub-repo) (format "%s/%s/%s/" .html_url "blob" (magit-get-current-branch))))

manateelazycat · 2018-09-16T14:59:44Z

@vermiculus
I have use let-alist build new version, check again, thanks.

vermiculus

it's looking better!

vermiculus · 2018-09-16T15:15:36Z

magithub.el

+                               (format "%s/%s/%s/"
+                                       .html_url
+                                       "blob"
+                                       (magit-get-current-branch))))


This does not necessarily return the name of the remote branch. This is a hard problem, though; I can work on it today unless you feel up to the challenge. See also #358 (comment)

I found magit-get-current-branch use

(magit-git-string "symbolic-ref" "--short" "HEAD")

I research some resource with Google, do you mean we shoud use below code to fetch currenet local branch?

(magit-git-string "rev-parse" "--abbrev-ref" "HEAD")

Or we need command "git branch" then grep branch name that start with * ?

vermiculus · 2018-09-16T15:16:48Z

magithub.el

+                              (magit-toplevel)
+                              ""
+                              (expand-file-name
+                               (if (buffer-file-name)


use if-let here to avoid calling (buffer-file-name) twice.

I have changed in my local version

…) get current local branch

manateelazycat · 2018-09-16T16:04:13Z

@vermiculus
Newest version with two changed:

1. Use if-let avoid call buffer-file-name twice
2. Use (magit-git-string "rev-parse" "--abbrev-ref" "HEAD") get current local branch

Leave message to me if you have any suggestion, I have to sleep, it's 24:00 in China. ;)

manateelazycat · 2018-09-17T14:00:46Z

@vermiculus Newest version is fine? Or any other improve suggestion?

vermiculus · 2018-09-17T16:10:18Z

I have not had the time to review. This could take up to a week or more. Keep in mind I have a full time job and a home life :-)

manateelazycat · 2018-09-17T16:23:48Z

I have not had the time to review. This could take up to a week or more. Keep in mind I have a full time job and a home life :-)

I also have full time job.

Unfortunately review one line code need a week or more.

No intention to offend, just feel that there is no contribution to enthusiasm, a little sad.

Anyway, hope you can review and merge my patch when you have time.

Have a nice day. ;)

vermiculus · 2018-09-18T17:29:20Z

magithub.el

+                                       .html_url
+                                       "blob"
+                                       (magit-git-string "rev-parse" "--abbrev-ref" "HEAD"))))
+         (file-relative-path (replace-regexp-in-string


string-remove-prefix may make more sense if that's what you're trying to do. If (magit-top-level) contains any regexp-sensitive characters (like [), this form could crash.

I will use string-remove-prefix instead

vermiculus · 2018-09-18T17:54:32Z

magithub.el

+                               (format "%s/%s/%s/"
+                                       .html_url
+                                       "blob"
+                                       (magit-git-string "rev-parse" "--abbrev-ref" "HEAD"))))


This does not resolve the remote-tracking branch problem discussed earlier.

If you get rid of --abbrev-ref, though, that will return the full hash. That is definitely constant between local and remote, so it could be used instead. It's not perfect, but at least it's safe.

Wow, it's cool, good to know that.

1. Use string-remove-prefix instead replace-regexp-in-string 2. Use (magit-git-string "rev-parse" "HEAD") get branch hash path.

manateelazycat · 2018-09-18T23:28:04Z

@vermiculus
I have update new version, please review it when you have time.

Thanks

vermiculus

A few things to make the function more usable.

vermiculus · 2018-09-19T14:45:02Z

magithub.el

+  (let* ((github-branch-path (let-alist (magithub-repo)
+                               (format "%s/%s/%s/"
+                                       .html_url
+                                       "blob"


you can incorporate this literal value directly into the format string

I don't understand how to improve this line with your suggestion. ;)

vermiculus · 2018-09-19T15:18:15Z

magithub.el

+Jump to file with line number if you in buffer of git file.
+Jump to file if you in dired mode.
+Jump to parent directory if you not in buffer with non-dired mode."
+  (interactive)


The (interactive) form can give us a value for a formal parameter when used interactively. That way, magithub-browse-file could take a file directly (e.g., (magithub-browse-file "some/path/relative/to/topdir.txt")).

Your interactive form could look like this (untested):

(interactive (list ;; provide a default if we're in a git repository (when-let ((toplevel (magit-toplevel))) ;; remove the directory-part -- just get the path ;; relative to topdir (string-remove-prefix toplevel (let (filepath) (cond ;; the current buffer is attached to a file; use that ((setq filepath (buffer-file-name)) (format "%s#L%s" filepath (line-number-at-pos))) ;; we're in dired;use the file at point ((derived-mode-p 'dired-mode) (dired-file-name-at-point)) ;; dunno; use the current directory (t default-directory)))))))

Note this moves a lot of the logic out of the main body of the function -- logic that's for interactive use only, anyway.

Or better yet, split out the arguments into FILE and LINE.

No, it's ugly that put everything in interactive, I will write new version to handle file and line argument.

1. Support argument file and line now, if current git repo is A, we can browse file even it in git repo B 2. Browse git repo's homepage if relative path is empty. 3. Move format code out from let binding.

manateelazycat · 2018-09-20T02:50:06Z

@vermiculus

I upload new version that support argument file and line.
Put everything in interactive is very ugly, I use &optional instead.

Please let me know if you have any suggestions.

Thanks!

-- Andy

vermiculus · 2018-09-21T18:09:47Z

Looks good. I'm curious though: why do you think it's ugly? It's idiomatic.

I've opened bbatsov/emacs-lisp-style-guide#51 for discussion and guidance.

manateelazycat · 2018-09-21T23:47:39Z

Looks good. I'm curious though: why do you think it's ugly? It's idiomatic.

I've opened bbatsov/emacs-lisp-style-guide#51 for discussion and guidance.

Haha, It's personal style.

I prefer use &optional style making the first impression of program feels very simple,
then other people more willing try to contribute.

It will make other people feels complicated if we put everything in interactive expression.

BTW, it's time to merge ? haha.

vermiculus · 2018-09-22T02:04:41Z

I don't agree with your reasoning, but I'll agree that it's good enough to merge. I may change my mind about the interactive form later after it's merged, though :-)

Please squash your commits into one.

manateelazycat · 2018-09-22T03:02:05Z

I don't agree with your reasoning, but I'll agree that it's good enough to merge. I may change my mind about the interactive form later after it's merged, though :-)

Please squash your commits into one.

Hmm, can't you just click "Merged PR" button in github ? ;)

vermiculus · 2018-09-22T03:31:59Z

I still prefer to merge only one squashed commit to keep the history clean and understandable.

manateelazycat · 2018-09-22T07:50:00Z

@vermiculus Please review #377

I close this PR now, thanks!

vermiculus · 2018-09-22T13:04:12Z

In the future, force-push to the pull request branch instead. You do not need to create a new pull request.

Add new function magithub-browse-file

dba4ebe

Open git file in user browser magithub-browse just open git repo magithub-browse-file is location file link in browse.

Make magithub-browse-file support file line number and dired mode.

aac9e82

Jump to file with line number if you in buffer of git file. Jump to file if you in dired mode. Jump to parent directory if you not in buffer with non-dired mode.

vermiculus requested changes Sep 16, 2018

View reviewed changes

Use let-alist instead alist-get

dac7894

vermiculus requested changes Sep 16, 2018

View reviewed changes

If if-let and use (magit-git-string "rev-parse" "--abbrev-ref" "HEAD"…

8737949

…) get current local branch

vermiculus requested changes Sep 18, 2018

View reviewed changes

Fix with vermiculus suggestions

946c331

1. Use string-remove-prefix instead replace-regexp-in-string 2. Use (magit-git-string "rev-parse" "HEAD") get branch hash path.

vermiculus requested changes Sep 19, 2018

View reviewed changes

Support file and line argument now.

5a95627

1. Support argument file and line now, if current git repo is A, we can browse file even it in git repo B 2. Browse git repo's homepage if relative path is empty. 3. Move format code out from let binding.

manateelazycat closed this Sep 22, 2018

Add new function magithub-browse-file #373

Add new function magithub-browse-file #373

Conversation

manateelazycat commented Sep 16, 2018 • edited Loading

manateelazycat commented Sep 16, 2018

vermiculus commented Sep 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vermiculus commented Sep 16, 2018 • edited Loading

manateelazycat commented Sep 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manateelazycat commented Sep 16, 2018

vermiculus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manateelazycat commented Sep 16, 2018

manateelazycat commented Sep 17, 2018

vermiculus commented Sep 17, 2018 • edited Loading

manateelazycat commented Sep 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manateelazycat commented Sep 18, 2018

vermiculus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manateelazycat commented Sep 20, 2018

vermiculus commented Sep 21, 2018 • edited Loading

manateelazycat commented Sep 21, 2018

vermiculus commented Sep 22, 2018

manateelazycat commented Sep 22, 2018

vermiculus commented Sep 22, 2018

manateelazycat commented Sep 22, 2018

vermiculus commented Sep 22, 2018

manateelazycat commented Sep 16, 2018 •

edited

Loading

vermiculus commented Sep 16, 2018 •

edited

Loading

vermiculus commented Sep 17, 2018 •

edited

Loading

manateelazycat commented Sep 17, 2018 •

edited

Loading

vermiculus commented Sep 21, 2018 •

edited

Loading