Skip to content

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

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 ?

@fingolfin
Copy link
Collaborator

@RomeoV are you still interested in this? If so, some conflicts need to be resolved now and a bunch of comments should be addressed.

If not, let's close it?

@fingolfin fingolfin added the Status: Waiting for Author The issue or pull request needs some action by its author label Oct 31, 2025
@RomeoV
Copy link
Author

RomeoV commented Oct 31, 2025

I have no bandwidth right now for this unfortunately, and also no use at this time. So we can close the PR. I appreciate your review and thoughtful comments, though. If you would like this merged i can address the comments some time later in November.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: Waiting for Author The issue or pull request needs some action by its author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

make.jl fails with git error

3 participants