Skip to content

Allow for remote names other than origin. #2509

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

RomeoV
Copy link

@RomeoV RomeoV commented May 25, 2024

We introduce getremotename that tries to check the remote.pushDefault option to determine the remote name, and falls back to origin if it's not set.

Fixes #2464 and perhaps #1916, as well as https://discourse.julialang.org/t/creating-documentation-when-remote-name-is-not-origin/110466.

We introduce `getremotename` that tries to check the
`remote.pushDefault` option to determine the remote name, and falls back
to `origin` if it's not set.
@mortenpi
Copy link
Member

Generally, I think trying to use pushDefault makes sense. But I have a couple of concerns:

  1. It will warn when remote.pushDefault is not set. However, it doesn't seem that remote.pushDefault is set in the normal case (e.g. you have just git clone-d a repo). So it seems that 99% of the time this code will warn (you can see that in the CI logs too), which is not great.

  2. I also wonder what is the right behavior when you do have origin, but pushDefault points to a different remote? I generally don't set pushDefault, but the use case I can imagine is when I copy a repo that I don't have access to, then fork it on GitHub and set pushDefault to the fork under my user. But if I were to build docs locally, I think I would prefer if it would pick origin over my fork. So maybe it should use origin if available, and if there is not remote called origin, it will pick the second best on based on pushDefault.

    Or maybe first step could be to just pick the only remote that is available, even if it is not called origin? And only use pushDefault if you have multiple remotes, none are called origin, and then use pushDefault to disambiguate?

  3. It would be good to add a note somewhere on the docs/src/lib/remote-links.md manual page about this behavior.

  4. Ideally there would also be a test. We have some related tests in test/repolinks.jl

@RomeoV
Copy link
Author

RomeoV commented May 29, 2024

Thanks for the thoughts. I have changed the workflow to default to origin, then remotes.pushDefault with a warning, and then just "" (which was the behavior before) with another warning.
I also added a related comment to the docs, and wrote some tests.

"""
$(TYPEDSIGNATURES)

Determines the GitHub remote of a directory, returninf 'origin' if that remote exists,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Determines the GitHub remote of a directory, returninf 'origin' if that remote exists,
Determines the GitHub remote of a directory, returning `origin` if that remote exists,

@assert !isempty(pushdefault)
@warn "Remote 'origin' not found. Using `git config --get remote.pushDefault` instead, which is set to '$(pushdefault)'."
return pushdefault
catch ; end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this PR needs to be reformatted with runic

Comment on lines +440 to +444
try
originurl = readchomp(setenv(`$(git()) config --get remote.origin.url`; dir=dir))
@assert !isempty(originurl)
return "origin"
catch; end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You seem to be using @assert plus try/catch for control flow. That seems like a bad idea for multiple reasons (e.g. future Julia version might allow disabling @assert checks, which is, I think, mainly intended to enforce to check runtime invariants, which is not what this code does).

How about just e.g. this

Suggested change
try
originurl = readchomp(setenv(`$(git()) config --get remote.origin.url`; dir=dir))
@assert !isempty(originurl)
return "origin"
catch; end
originurl = readchomp(setenv(`$(git()) config --get remote.origin.url`; dir=dir))
if !isempty(originurl)
return "origin"
end

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same applies to the rest of the code, of course

@@ -616,7 +655,7 @@ it out automatically.
`root` is the the directory where `git` gets run. `varname` is just informational and used
to construct the warning messages.
"""
function git_remote_head_branch(varname, root; remotename = "origin", fallback = "master")
function git_remote_head_branch(varname, root; fallback = "master")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring still references remotename. So either that needs to be adjusted. Alternatively, keep the kwarg here, but with a new default value nothing; and then later only call getremotename if remotename === nothing ?

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.

make.jl fails with git error
3 participants