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

Default to accepting either master or main as the devbranch #1727

Closed
wants to merge 3 commits into from

Conversation

oxinabox
Copy link
Contributor

@oxinabox oxinabox commented Nov 10, 2021

Closes #1443

Basically this introduces a magic default value for devbranch which will match to either master or main.
This allows people to rename their default branch in git, without having to also having to add in the devbranch argument in the docs/make.jl.
Which is good if you have many repos to switch over.

This implementation doesn't depend on e.g. reading the git settings to find out the default branch, or probing git to see what branches exist.
It just naively accepts either name.
Which makes for easy implementation.

Technically this is breaking, if someone was using main as the name of some other branch unrelated to it being the devbranch, and didn't have devbranch="master" (or similar) set then suddenly docs would start deploying every time they pushed to that unrelated main branch.
I don't know if this is a realistic concern.
There are 3 responses to that:

  • A) It is not realistic don't worry about it, add this feature as nonbreaking.
  • B) It is realistic, add this feature as a breaking release. People who are not doing anything weird will have to update their docs/Project.toml
  • C) It is realistic, we should not do this, it is too spooky. People should stick to master or hard-code devbranch="main"

struct CommonDevBranches end

matches_devbranch(branchname, devbranch) = branchname == devbranch
matches_devbranch(branchname, ::CommonDevBranches) = branchname ∈ ("main", "master", "dev")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
matches_devbranch(branchname, ::CommonDevBranches) = branchname ("main", "master", "dev")
matches_devbranch(branchname, ::CommonDevBranches) = branchname ("main", "master")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before I was considering also allowing dev.
But the only repo i know that uses dev is MLJ, and they do not see the devbranch=dev argument.
And so accepting that would potentially break their workflow.
https://github.com/alan-turing-institute/MLJ.jl/blob/96562a60a70aebc3f0d768147ff9dab4549ddfa5/docs/make.jl#L98-L101

src/deployconfig.jl Show resolved Hide resolved
Comment on lines 253 to 263
@warn """
Possible deploydocs() misconfiguration: main vs master
Documenter's configured primary development branch (`devbranch`) is "master", but the
current branch (\$TRAVIS_BRANCH) is "main". This can happen because Documenter uses
GitHub's old default primary branch name as the default value for `devbranch`.
current branch (\$TRAVIS_BRANCH) is "main".

If your primary development branch is 'main', you must explicitly pass `devbranch = "main"`
to deploydocs.
Or remove the `devbranch` setting to fallback to the default which accepts both.

See #1443 for more discussion: https://github.com/JuliaDocs/Documenter.jl/issues/1443
"""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can just delete this whole condition and warning message?
(and the other several copies of it for the other CI providers)

It seems like people are not going to do it by mistake anymore, with the new default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mortenpi you added this warning in #1489
what do you think of just removing it?

@oxinabox
Copy link
Contributor Author

Bump

@oxinabox
Copy link
Contributor Author

Poking people from original issue
@simeonschaub you openned the original issue #1443 would this resolve it?
@KristofferC you had strong opinions on #1443 does this avoid your concerns?

@simeonschaub
Copy link
Contributor

Alternatively, could we query git for the name of the default branch? I.e. something like this: https://davidwalsh.name/get-default-branch-name.

I think this is probably fine as a solution to #1443, but I am wondering if we can avoid hard coding a set of common default branch names.

@oxinabox
Copy link
Contributor Author

Alternatively, could we query git for the name of the default branch? I.e. something like this: https://davidwalsh.name/get-default-branch-name.

We could, but a problem is a surprising number of CI environments don't have git installed by default.
We could deal with that, but it adds complexity.

@KristofferC
Copy link
Member

@KristofferC you had strong opinions on #1443 does this avoid your concerns?

I wouldn't really say I had strong opinions... I just wanted old code to keep working (and preferred a design where there is no need to "chase" the default branch which apparently cannot be relied on to stay fixed). I haven't looked at the code here but if it works as described it should reduce the level of urgency and can be improved at a later point by querying the default branch.

We could, but a problem is a surprising number of CI environments don't have git installed by default.

Doesn't Documenter push the docs via git?

@oxinabox
Copy link
Contributor Author

oxinabox commented Nov 23, 2021

Doesn't Documenter push the docs via git?

Good point. Looks like it does.
If you deploy using Documenter at least.
but on gitlab at least there are other ways to deploy -- i guess though that doesn't matter though as this code is only hit if you are deploying using Documenter.
I might make a secondary PR at some point as an alternative to this, if people think is way is too invasive.
Or can be a follow up later if we think this is a good solution but that that might be better.
like you say

The stuff in https://davidwalsh.name/get-default-branch-name looks kinda fragile.
in that it is regexing stuff out of human readable output.

@fredrikekre
Copy link
Member

julia> function default_branch()
           str = read(`git remote show origin`, String)
           m = match(r"^\s*HEAD branch:\s*(.*)$"m, str)
           return m === nothing ? "master" : String(m[1])
       end
default_branch (generic function with 1 method)

julia> default_branch()
"master"

maybe?

@oxinabox
Copy link
Contributor Author

It's a bit harder than that according to https://davidwalsh.name/get-default-branch-name#comment-515994
which says if LANG is not english that won't work

Apparently if we translate git ls-remote --symref https://github.com/cli/cli HEAD | awk -F'[/\t]' 'NR == 1 {print $3}'
according to the next comment, but that doesn't work for me.

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.

deprecate devbranch defaulting to master?
4 participants