Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Fix/first publish master needed #917

Closed
wants to merge 2 commits into from
Closed

Fix/first publish master needed #917

wants to merge 2 commits into from

Conversation

Nicolas-Reyland
Copy link

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

When publishing an atom package through the apm, if the repository is set on the default "main" branch (new git standard), you would get an error. This is because the publish script is looking for a "master" branch (old standard):

  • (line 252) checks if the repo is set to an "alternative" (not the main) branch, if one is found, accesses it
  • (line 254) it then tries to access the "master" (hard-coded 'branch.master.remote')

The new git convention is that the main branch should be named "main". Instead of checking for the master branch, I first check if there is a "main" branch, then I check if there is a "master" branch.

If I got something wrong, please tell me: this is my first ever pull request. My request might be bad, and I am aware that you normally don't tell people when their requests were rejected (it is written in this text file, in the comments), but please tell me why this might be the case, so I know how to do them in the future

Alternate Designs

You could also only check for the main branch, not checking for the master branch afterwards.

Benefits

You will not get the error saying the git repo is not valid (not an atom package, not sufficient permission or it doesn't exist)

Possible Drawbacks

I cannot think of a possible drawback.

Verification Process

Applicable Issues

@Nicolas-Reyland
Copy link
Author

This is my first pull request. If this pull request is rejected, please tell me why !
I forgot to add: I rebuilt everything and it succeeded.

@aminya
Copy link
Contributor

aminya commented Mar 6, 2021

Does this fix #908?

@Nicolas-Reyland
Copy link
Author

Does this fix #908?

It is supposed to fix #908 , yes. To be completely honest, I recompiled (using your instructions in the README), and everything recompiled smoothly, but I did not know how to use the version of apm I compiled (the bin/ directory content did not change after compiling?).
How can I test it ?

@Nicolas-Reyland
Copy link
Author

Doesn't fix. sry

@idleberg
Copy link

I think main should be the new default, but how about a command-line flag that allows specifying any other branch

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants