Skip to content
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.

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

Only hexify branch name if it's not nil. #127

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

mijoharas
Copy link
Contributor

When we pass the branch name through to the handler, we do this https://github.com/sshaw/git-link/blob/master/git-link.el#L864
that calls url-hexify-string on the branch, whether it's nil or not. Passing a nil branch through to it will give "" which is truthy, which means that the (or branch commit) line in each of the handlers, e.g.: https://github.com/sshaw/git-link/blob/master/git-link.el#L594 will always evaluate to the truth-ey empty string "" and will give an incorrect url.

Fix that by only calling url-hexify-string if the branch is non-nil already.

@sshaw
Copy link
Owner

sshaw commented Aug 4, 2024

Thanks.

How can I reproduce this issue?

Even though we don't call url-hexify-string the format string "%s" still exists so won't that get interpolated with nil anyways?

@mijoharas
Copy link
Contributor Author

mijoharas commented Aug 6, 2024

hey, so, I just called git-link on a commit that wasn't part of a branch. i.e. from this commit, we do git checkout HEAD~1:

and then when calling git-link I get something like https://github.com/sshaw/git-link/blob//README.md?plain=1#L1 (which is a link that doesn't exist) instead of the expected

# git-link

(i.e. yes, %s gets interpolated with nil but that leads to an invalid url, rather than a correct one with the fix).

@sshaw
Copy link
Owner

sshaw commented Aug 9, 2024

Oh yes, I see. Thank you!

@sshaw sshaw merged commit a0a2fff into sshaw:master Aug 9, 2024
11 checks passed
@mijoharas
Copy link
Contributor Author

ahhh, sorry should have checked the issues, could have linked to them myself 😅 (went straight to the code to just figure out what was wrong). Thanks again 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants