Add option to not add to kill ring.

[mijoharas]

Additionally return the string from the git-link--new function. This makes it simpler to automatically call git-link functions from other code.

[sshaw]

Hi thanks. Yes a longstanding and sadly ongoing issue with this module.

I think issue with global solution is that the function call should be "local" to the code in which it's being called and not needlessly depend on global behavior. You can overcome this with let call so maybe that is sufficient? It is certainly is some Elisp type antics.

I think this is the problem with other solutions that have been submitted: #118 (comment)
What do you think?

Though after all this time not having this functionality, maybe it is a fine trade-off.

[sshaw]

Oh and 1 important point: other code should not be calling git-link--new directly this is a "private" function per conventions. A new function should be make available for code-only calling.

[mijoharas]

Hi, so, I'm not directly calling git-link--new in what I'm trying to do, but since the return value of git-link--new is the return value of all of the public functions, I would need to modify that to modify the public functions. Before this change I had hacked in a cl-letf to locally override the git-link--new function because I didn't want what it did. With this change I can get rid of that hack.

Note: I am calling this interactively for my usecase. (and using a local let binding for it with the new code). Could you expand on the problems with calling it interactively like this that you mention in #118 (comment)?

Let me show you one of the functions that I was writing:

Before this change (hacky):

  (defun mike/magit-go-to-github-commit-hash-copied ()
    "Go to the github commit page at the hash under the chunk of the magit blame"
    (interactive)
    (let* ((hash (oref (magit-current-blame-chunk) orig-rev)) ;; elpa/28.2/develop/magit-20230523.1431/magit-blame.el:920:28
           (base_url
            (cl-letf (((symbol-function 'git-link--new) (lambda (x) x)))
              (call-interactively 'git-link-homepage)))
           (full_url (concat base_url "/commits/" hash)))

      (message full_url)
      (browse-url full_url)
      )
    )

After this change (imo, cleaner):

  (defun mike/magit-go-to-github-commit-hash-copied ()
    "Go to the github commit page at the hash under the chunk of the magit blame"
    (interactive)
    (require 'git-link)
    (let* ((hash (oref (magit-current-blame-chunk) orig-rev))
           (git-link-open-in-browser nil)
           (git-link-add-to-kill-ring nil)
           (base_url
            (call-interactively 'git-link-homepage))
           (full_url (concat base_url "/commits/" hash)))
      (message full_url)
      (browse-url full_url)
      )
    )

[mijoharas]

Note: I don't see myself globally setting that flag, I only really want it for local modification, but it seemed like the cleanest way to do what I wanted, and I figured someone might want to configure it.

Note 2: I think I may have tried calling the function explicitly with remote/start/end instead of calling it interactively, but then found I was just duplicating some of the logic of the function, so figured call-interactively was better for my usecase but 🤷‍♂️ .

[sshaw]

Thanks for the explanation and sorry for the delay. Makes sense.

I think there is also a feature request for linking to the blame for the given line. I should probably look into that as certainly others are doing what you're doing

[sshaw]

The business about call-interactively, I can't remember the issues maybe with argument handling? There was a write-up on Emacs Wiki(?) about it but it's no longer there so maybe no longer an issue. 🤷

[mijoharas]

Thanks! (For what it's worth, in the meantime I reread the other issues and do think a "pure" function would probably be good in general, but for what I want the call-interactively is better to not duplicate the extra code in the interactive block, I'll report back if I ever run into troubles using it though).

Thanks again!