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

fix: Don't assume master when checking git packages for upgrades #56

Merged
merged 1 commit into from Feb 15, 2023

Conversation

savetheclocktower
Copy link
Sponsor Contributor

Fixes #55.

It took me ages to figure out how to replicate the test-git-repo.git fixture, but i managed to create one from scratch that uses main instead of master. All existing tests pass.

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

The actual code changes here are simple and seem like a no brainer.

Literally three line changes from hardcoded values to dynamic ones that will continue to work in the future as far as we can reasonably guess.

But amazingly this PR also adds tests, new spec files, and everything needed to make sure this code works flawlessly.

I'd honestly say this is a 10/10 PR, and fully say we should get it merged! Thanks a ton for the contribution!

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

LGTM!

I think there may be places elsewhere in ppm that expect master in a hard-coded way, but I think we all want to move away from that, and this is as good a start on that as any.

[ EDIT: Big thanks for adding tests, by the way! I read every line, and glanced over every added fixture file (new basic git repo), LGTM. Change to the src/ file also looks good to me, I checked that perf is the same as before (thought for a second this added a new Git.open(), but I see now it only moves that from a later part to an earlier part of the function. Excellent! ]

Thanks for the contribution!

@savetheclocktower
Copy link
Sponsor Contributor Author

I think there may be places elsewhere in ppm that expect master in a hard-coded way, but I think we all want to move away from that, and this is as good a start on that as any.

The good news there is that I didn't find any others. The only implicit assumption of master that remains is in the code that converts a TextMate bundle on GitHub to an Atom package, and I can't imagine anyone is writing new TextMate bundles these days that would have a main default branch.

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.

ppm upgrade doesn't work for git-installed packages without a master branch
3 participants