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

Set default branch from "master" to "HEAD" #510

Merged
merged 8 commits into from
Jul 10, 2020
Merged

Set default branch from "master" to "HEAD" #510

merged 8 commits into from
Jul 10, 2020

Conversation

MyKo101
Copy link
Contributor

@MyKo101 MyKo101 commented Jun 18, 2020

This PR replaces "master" within reference to repo branches with "HEAD", which grabs the latest commit from the default branch. This allows for repos that do not use the "master" convention to be targeted for download without the need to specify a reference (e.g. "main").

This will not effect repos that already use "master" as their default branch, as it will still be pulled. The only situation negatively effected by this PR is when a user is trying to target the "master" branch (and is thus using the previous default, ref="master") and it is not the default, which will be a rare edgecase.

It is also suggested that the remotes repository changes it's default branch to "main". Currently codecov requires a named branch and is incompatible with "HEAD", and so the badge (as it stands in this PR) will not display until this is done.

This resolves Issue #508

README.md Outdated
[![Windows Build status](https://ci.appveyor.com/api/projects/status/github/r-lib/remotes?svg=true)](https://ci.appveyor.com/project/gaborcsardi/remotes)
[![](https://www.r-pkg.org/badges/version/remotes)](https://www.r-pkg.org/pkg/remotes)
[![CRAN RStudio mirror downloads](https://cranlogs.r-pkg.org/badges/remotes)](https://www.r-pkg.org/pkg/remotes)
[![Coverage Status](https://img.shields.io/codecov/c/github/r-lib/remotes/master.svg)](https://codecov.io/github/r-lib/remotes?branch=master)
Copy link
Member

Choose a reason for hiding this comment

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

You need to keep ?branch=xyz here and for the travis case, otherwise the badge would show the status of the last running branch instead of the default one.

@jimhester
Copy link
Member

While I agree that a change from 'master' as the default branch of remotes would be preferable, I think we should wait to change the default branches on this and other repos until GitHub or git officially changes to a different default branch name. Right now the only 'official' word on this from GitHub is a twitter reply (https://twitter.com/natfriedman/status/1271253144442253312), and this change will likely break a number of workflows, so it needs to be done carefully.

For now could you update this PR to keep the default branch for remotes as 'master', and add a bullet to NEWS? It should briefly describe the change and end with (@yourname, #issuenumber).

@MyKo101
Copy link
Contributor Author

MyKo101 commented Jun 20, 2020

I understand the apprehension, as discussed on the Issue, some people's workflows will be largely affected. However, I believe it useful to have this ready to go and at some point this change will be needed.

I've added to the NEWS.md file as requested. This is actually my first PR (other than in my own repos), so any feedback on my style, etc... would be greatly appreciated.

@MyKo101 MyKo101 changed the title switch over to main convention Set default branch from "master" to "HEAD" Jun 21, 2020
NEWS.md Outdated Show resolved Hide resolved
R/install-github.R Outdated Show resolved Hide resolved
R/submodule.R Outdated Show resolved Hide resolved
R/submodule.R Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@MyKo101
Copy link
Contributor Author

MyKo101 commented Jun 23, 2020

I've addressed the comments by @gaborcsardi and @jimhester in my local clone of remotes, but wanted to fix a few other things before pushing it.

I currently have two errors being hit by the testthat.R section of devtools::check():

  • base curl download redirects (@test-download.R#298)
    I believe that the url used here: http://httpbin.org/absolute-redirect/1 is broken. Suggestions to replace would be useful.
  • install-github.R script is up to date (@test-script.R#10)
    I'm unfamiliar with make and brew, so I'm finding it difficult to repair this. I have tried running make in terminal and brew() in the same way as it is run in the test, but it is still failing. It's also worth mentioning that when I run devtools::test(), it is passing. And in my previous commit, this worked (both locally and with appveyor/travis). It would be useful if someone could point me in the right direction (I am eager to learn, however I would also like a quick-fix for this and I feel like it is probably a really simple solution that I don't know how to implement)

There is also a warning being flagged when I run devtools::test(), but I'm not sure how important this is:

test-install-github.R:208: warning: github_pull
'tar.exe -xf "C:\Users\mbrxsmbc\AppData\Local\Temp\RtmpMFii7R\file3a587a373233.tar.gz" -C 
"C:/Users/mbrxsmbc/AppData/Local/Temp/RtmpMFii7R/remotes3a5854ae6762"' returned error code 1

I have removed a reference to "master" in this test, however this warning remains with or without "master" included.

@MyKo101
Copy link
Contributor Author

MyKo101 commented Jul 1, 2020

Currently the only reason for the build failures is due to an issue with postmanlabs/httpbin#617.

@jimhester jimhester merged commit e7bf8e6 into r-lib:master Jul 10, 2020
@jimhester
Copy link
Member

Thanks a bunch!

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.

3 participants