feat: git-link-blame

[agzam]

Adds git-link-blame fn.

[sshaw]

Hi, thanks. Very nice idea.

Is it not possible to make it use a customization function/alist like the others (git-link-commit-remote-alist, git-link-remote-alist)?

This has a few benefits but one is it may not work with Bitbucket (or others) but the user will receive an error instead of an invalid link.

[agzam]

I don't know if we really need separate functions for each case in that list, maybe alist of regexes to match and replace. I just checked, github and gitlab have similar url structure, whereas for bitbucket it works if you replace "/src/" with "/annotate/". I am not sure about other forges like gitorious, I don't have any repos hosted there.

[agzam]

@sshaw Thank you for a prompt feedback. This one indeed turned out to be a very useful feature, it now lets you find associated PRs (that sadly cannot be easily discovered locally). I have made some modifications based on your comments. If you know what the values should be used for other than github, gitlab or bitbucket, feel free to make changes, or let me know what they should be and I'll make some adjustments. As well as the wording in docstrings and comments. Thanks!

[sshaw]

Hi, is there a reason to not write this out as a function similar to git-link, git-link-blame, etc..?

I can understand advising public functions that are not owned by the advising code, but here we're advise our own private function. From a maintenance point of view and config point of view this is a bit odd, i.e., currently everything is configured with a link function, but this is configured via regex path matching.

There are also some existing errors cases that can be avoid or that may be clearer without the current approach.

Thoughts?

[agzam]

I can understand advising public functions that are not owned by the advising code, but here we're advise our own private function.

Of course. The PR wasn't meant to be merged "as is", it's more like a prototype. I'm not very familiar with the package, and I wasn't sure if I should extract the bits in a separate function or do something else.

Also I think I saw in another PR someone extracting git-link--new already (?)

I think that problem is kinda orthogonal to this work, I can make another PR that extracts that function and adjust git-link-blame here so it doesn't need advising. Or if you say it's okay to do it in here, I can try that. I just wanted to keep it minimal, without impacting other, existing functions.

[sshaw]

Also I think I saw in another PR someone extracting git-link--new

Possibly. But a lot of these PRs are incomplete or break compatibility and require a 1.0 release

I think that problem is kinda orthogonal to this work, I can make another PR that extracts that function and adjust git-link-blame here so it doesn't need advising.

That makes sense –and would be great!

[sshaw]

Closing now. Feel free to reopen with the changes discussed.

[agzam]

Sorry, I got swamped with work and didn't get a chance to address the comments sooner. I made some changes here:

https://github.com/sshaw/git-link/compare/master...agzam:git-link-blame

• removed advising local function
• had to change when to if in git-link--new

Let me know if I should create a new PR.

[sshaw]

Hi, sorry for the late reply. Really I could have used this like 5 times over the past month.

While we're not advising I think the underlying issue is still there: we're having to hack around git-link. Ideally we have some function git-link--foo that we can call without from git-link-blame, git-link, etc... we don't have use various tricks to work around the existing shortcomings in the existing function(s).

If you have some ideas on this great but give me < 1 week to review this better and think about things.

Thanks so much!

[sshaw]

This part is the only part that differs between git-link and git-link-blame.

It and the interactive part, partially hampers doing things like #69.

What do you think of the following (not fully tested)

(defun git-link--current-buffer-data (remote handlers)
  (let (filename branch commit handler remote-info (remote-url (git-link--remote-url remote)))
    (if (null remote-url)
        (user-error "Remote `%s' not found" remote)

      (setq remote-info (git-link--parse-remote remote-url)
            filename    (git-link--relative-filename)
            branch      (git-link--branch)
            commit      (git-link--commit)
            handler     (git-link--handler handlers (car remote-info)))

      (cond ((null filename)
             (user-error "%s" "Can't figure out what to link to"))
            ((null (car remote-info))
             (user-error "Remote `%s' contains an unsupported URL" remote))
            ((not (functionp handler))
             (user-error "No handler found for `%s'" (car remote-info)))
            (t
             (let ((vc-revison (git-link--parse-vc-revision filename)))
               (when vc-revison
                 (setq filename (car vc-revison)
                       commit   (cdr vc-revison)))
               (list
                :handler  handler
                :hostname (car remote-info)
                :dirname  (cadr remote-info)
                :filename filename
                :branch   (if (or (git-link--using-git-timemachine)
                                  (git-link--using-magit-blob-mode)
                                  vc-revison
                                  git-link-use-commit)
                              nil
                            (url-hexify-string branch))
                :commit commit)))))))

(defun git-link--interactive-arguments (prefix)
  (if (equal '- prefix)
      (list (git-link--remote) nil nil)
    (let* ((remote (git-link--select-remote))
           (region (when (or buffer-file-name (git-link--using-magit-blob-mode))
                     (git-link--get-region))))

      (if (and (null git-link-use-single-line-number) (null (cadr region)))
          (list remote nil nil)
        (list remote (car region) (cadr region))))))

(defun git-link-blame (remote) 
  (interactive (git-link--interactive-arguments current-prefix-arg))

  (let ((data (git-link--current-buffer-data remote git-link-blame-remote-alist)))
    (git-link--new
     (funcall (plist-get data :handler)
              (plist-get data :hostname)
              (plist-get data :dirname)
              (plist-get data :filename)
              (plist-get data :branch)
              (plist-get data :commit)
              start
              end))))

This partially addresses #45 and I can eventually update git-link to use this code and ultimately take steps to separate the functions that depend on global state, i.e., (current-buffer) from those that are useful to call programmatically which, will also address some outstanding issues (e.g., #69).

Though maybe instead of remote git-link-blame should accept something more useful. I think branch may be a better option. There may be an issue for this too. Ultimately with a v1 release this will be changed for git-link and git-link-commit.

Thoughts?