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

switch defaultBranchName to main on new repo #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wesinator
Copy link

thanks for this project to maintain atom

This PR makes the default branch name main when adding a new repo .
Ideally this would be configurable in the settings, but this is a stopgap.
atom#2794

@Spiker985
Copy link
Member

Arguably, if possible it would be nice to just default to whatever the git config would use

But stopgap indeed

@Spiker985
Copy link
Member

@pulsar-edit/core-admin core-maintain has no management permissions to this repo

@mauricioszabo
Copy link

Can't we just put this behind a config?

@Spiker985
Copy link
Member

Can't we just put this behind a config?

Or even just prompt for it when creating the repo (if it doesn't already)

@wesinator
Copy link
Author

Arguably, if possible it would be nice to just default to whatever the git config would use

Yes actually that would be best. That may require completely new tooling in atom for getting data from the system command-line interface?

@confused-Techie
Copy link
Member

@Spiker985 @mauricioszabo So while there are better options, for the purpose of this PR how do we feel about using a config value for now? A setting on the package then at a later time we can have something a bit more complex?

@confused-Techie
Copy link
Member

Also @Spiker985 perms added to this repo for core

@confused-Techie
Copy link
Member

@wesinator I'd love to be able to get your PR in, but like mentioned we aren't sure of what the best methodology would be here.
Personally I do feel allowing this to be configurable until a later date is the smartest move we could make and have the default be main. So unless anyone has objections to this, would you be alright making that change or with me pushing the changes directly?

@wesinator
Copy link
Author

@wesinator I'd love to be able to get your PR in, but like mentioned we aren't sure of what the best methodology would be here. Personally I do feel allowing this to be configurable until a later date is the smartest move we could make and have the default be main. So unless anyone has objections to this, would you be alright making that change or with me pushing the changes directly?

sounds good. Can you push the change?

Ultimately I think using the value from git config init.defaultBranch as default would be ideal

@confused-Techie
Copy link
Member

Ultimately I think using the value from git config init.defaultBranch as default would be ideal

Absolutely right there, that there does exist the best option. But putting it as a config for now would be the simplest and most effective. Lets the users choose what's best for them

@DeeDeeG
Copy link
Member

DeeDeeG commented Mar 7, 2023

I was testing this out locally with ppm link, and I'm not sure this does what it's intending to.

To see if I could get it to work, I kept trying stuff, and now I have no mention of master anywhere in my local github/lib/ files, and it's still initializing new git repos with the default branch master.

And on closer look, the code edited by this PR seems to be oriented toward "publishing" a repo, which I suppose is pushing a local repo to GitHub.com.

I suppose the default branch of the local repo being created is either not being explicitly told to use master, or it may be configured as master in a dependency (dugite or libgit2...?).

Wish I knew more for certain. For the time being I'm not sure exactly what this does do, so I might be disinclined to merge it until we can demonstrate it as working?

@confused-Techie
Copy link
Member

I was testing this out locally with ppm link, and I'm not sure this does what it's intending to.

To see if I could get it to work, I kept trying stuff, and now I have no mention of master anywhere in my local github/lib/ files, and it's still initializing new git repos with the default branch master.

And on closer look, the code edited by this PR seems to be oriented toward "publishing" a repo, which I suppose is pushing a local repo to GitHub.com.

I suppose the default branch of the local repo being created is either not being explicitly told to use master, or it may be configured as master in a dependency (dugite or libgit2...?).

Wish I knew more for certain. For the time being I'm not sure exactly what this does do, so I might be disinclined to merge it until we can demonstrate it as working?

I appreciate you testing as well as you have. Considering the .isEmpty() call in the if statement, I'd assume this only can have any effect on pushing or modifying an empty repo? Although I'd assume an empty repo still has to contain it's configuration such as branch names right?

@DeeDeeG
Copy link
Member

DeeDeeG commented Mar 15, 2023

Yeah, whether by reading or testing a few things, I couldn't really understand what this change did/when this code path would come into play, honestly.

[ . . . ] an empty repo still has to contain it's configuration such as branch names right?

Yes, you could cd into a repo created by the github package, before any commits were even made, and do git status and check out the metadata inside .git/, and I saw that it was on master branch even with no commits, for instance.

So as far as I can tell, something else would need to be done to make new repos made by github package have a different default branch name. I'm unclear if it can be done in this repo, or if it has to be done in one of this package's dependencies, or if it is dictated by the system git config possibly being shelled out to, or...

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.

5 participants