Create a function to return the git-link itself

[futuro]

git-link is incredibly useful, but it doesn't return the URL it creates. This introduces a new function which does so, allowing more tooling to make use of this great feature.

This builds from the conversation in #38 and issue #37.

[sshaw]

Hi @futuro, thanks for the PR.

This is definitely something that's needed. I believe my concern with the other PRs was compatibility in that the signatures and/or return value must be changed in order to have a solid function and that calling git-link or whatever non-interactively should not add to the kill ring.

This certainly avoids that (yay) but what about the rest of the global state? For example, if you're using if for an org capture template then won't all the checks git-link-url is doing on the current buffer produce wrong or non-deterministic results?

[futuro]

Hey @sshaw,

I'll admit that I'm totally ignorant about global state (and emacs hacking in general), so I don't really have a good answer for this. I'll say that the function which is calling git-link-url is wrapped inside a with-current-buffer, so it's running (from what I understand) inside of the buffer for the file I want the git link for.

I'd welcome any pointers on what to read to understand the global state that's at play, or idiomatic design patterns for issues like this.

[futuro]

Thinking about this more, @sshaw is your question about whether git-link-url can be called as a pure function, versus needing to be called inside the buffer you want the link from?

[sshaw]

Hi @futuro, sorry for the holdup

is your question about whether git-link-url can be called as a pure function, versus needing to be called inside the buffer you want the link from?

Yes, I think that a standalone function should be used. Something like:

(git-link-url file-or-buffer &optional start end remote use-branch)

[dotemacs]

Hi @sshaw,

Thanks for the super useful package!

Is there a blocker for this feature to be merged? Thanks

[sshaw]

Is there a blocker for this feature to be merged? Thanks

It does not implement a standalone version of git-link-url (or similar). In its current form its return value depends on global state (unless I'm missing something?).

[futuro]

I'll see if I can shake that out today.

[futuro]

While I sort this out, @sshaw, I've noticed that git-link works in the same way as my change -- my change is just to extract some functionality from git-link -- and that much of git-link depends on global state, such as with an interactive call or within a with-current-buffer body.

My current thought is to have git-link-url contain a with-current-buffer call and have everything else be within that macro, but I'd love your thoughts on this and whether it's violating some kind of elisp design principle.

[sshaw]

Hi,

Thanks for getting back to this!

I've noticed that git-link works in the same way as my change ... and that much of git-link depends on global state ...

The default case for git-link for interactive use, i.e., get a link of/based on the current buffer's contents and cursor position. This is okay.

My current thought is to have git-link-url contain a with-current-buffer call ... but I'd love your thoughts on this and whether it's violating some kind of elisp design principle.

This is where the problem lies. If it is going to be a legit stand-alone function it must not depend on things like the current buffer, cursor position, etc... Instead of being for interactive use it should have a signature similar to that outlined here and must return the URL and not add it to the kill ring.

I would say this is general principle for functions, etc...: don't depend on global state.

Once we have this baseline function it can be used for all use cases, including the interactive one.

Does that help?

[sshaw]

I would say this is general principle for functions, etc...: don't depend on global state.

Though we can depend on the output of git, which is sorta global state. If we didn't then we'd have more function arguments and I think the function would loose some of its utility then, as it would be forcing the caller to parse and pass in git output.

[futuro]

@sshaw after reviewing more of the code, I think I'm starting to see the way this could work. One question: what's the goal of the optional arg use-branch?

[sshaw]

... what's the goal of the optional arg use-branch?

That is supposed to be the equivalent of sorts of git-link-use-commit. Currently the default is to generate the link using the branch but time has shown that it's better to use the commit. So I think eventually git-link-use-commit will be git-link-use-branch to better reflect this. This argument is named to reflect that.

[futuro]

That is supposed to be the equivalent of sorts of git-link-use-commit.

Rockin, that makes sense.

Another question! You'd put the remote as an optional argument, but the function can't work without a specified remote (as far as I understand it, at least). I'm thinking about moving this to be a required argument; am I missing anything?

[futuro]

Ok @sshaw, this should be ready.

I went ahead with making remote a required argument, but let me know if I missed something and it should be optional.

[futuro]

J/k, sorry. I need to fix a void-variable reference. Will ping you once it's solid.

[futuro]

Ok, tested, and should be good to go for review @sshaw.

[sshaw]

Thanks a lot. I will have a looksy at or before Christmas Day 🎅🤶

[sshaw]

Hi, @futuro, lookin' good. Aside from my 2 comments I would squash this PR into a single commit.

Merry Christmas! 🎄

[futuro]

Hey @sshaw, thanks for the suggestions on user-error! I've never done error signaling/handling in elisp, so that was a fun excursion.

I've just pushed up that change, and it should be ready for review.

Just to clarify, are you asking me to squash all the commits in this PR, or are you saying that you're going to do a squash merge afterwards?

Thanks! And Happy New Year!

[sshaw]

Just to clarify, are you asking me to squash all the commits in this PR, or are you saying that you're going to do a squash merge afterwards?

Squash all commits, please.

[futuro]

@sshaw commits are squashed and everything should be ready to go.

[sshaw]

Closing to due lack of feedback/progress.