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.

Sign up for GitHub

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

Verify locally installed Git version is >=2.11 #349

Merged
merged 2 commits into from
Feb 21, 2019
Merged

Verify locally installed Git version is >=2.11 #349

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6a41a49
Verify locally installed Git version is >=2.11
itaisteinherz Feb 20, 2019
89a0c68
Add `async` keyword to async functions
itaisteinherz Feb 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions source/git-util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';
const execa = require('execa');
const semver = require('semver');

const latestTag = () => execa.stdout('git', ['describe', '--abbrev=0', '--tags']);

Expand Down Expand Up @@ -108,3 +109,14 @@ exports.verifyTagDoesNotExistOnRemote = async tagName => {
exports.commitLogFromRevision = revision => execa.stdout('git', ['log', '--format=%s %h', `${revision}..HEAD`]);

exports.push = () => execa('git', ['push', '--follow-tags']);

const gitVersion = async () => {
const {stdout} = await execa('git', ['version']);
return stdout.match(/git version (\d+\.\d+\.\d+).*/)[1];
};

exports.verifyRecentGitVersion = async () => {
if (semver.lt(await gitVersion(), '2.11.0')) {
Copy link
Collaborator Author

@itaisteinherz itaisteinherz Feb 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I just noticed that this could be moved to a separate method in version.js named isVersionLower, along with isVersionGreater. Doing so would keep this file (almost) entirely focused on git-related methods.
Also, we're currently not checking if the output returned by gitVersion is actually a version, which could lead to some issues in future versions of git.

@sindresorhus Thoughts?

Copy link
Owner

Choose a reason for hiding this comment

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

isVersionLower, along with isVersionGreater.

We don't really need both of these though. How about just one called isVersionGreaterThanOrEqualTo()? That's a mouthful though... Alternatively, isMinimumVersion() / isAtLeastVersion().

Also, we're currently not checking if the output returned by gitVersion is actually a version, which could lead to some issues in future versions of git.

We should definitely do that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't really need both of these though. How about just one called isVersionGreaterThanOrEqualTo()? That's a mouthful though... Alternatively, isMinimumVersion() / isAtLeastVersion().

In ui.js and prerequisite-tasks.js we're using !isVersionGreater(), which is the same as isVersionLowerThanOrEqualTo(). This requires us to have a way to check if v1 >= v2 as well as if v1 <= v2, which means we'll need to have at least 2 methods.

To solve this, we could keep isVersionGreater() and add another method for checking version equality isVersionEqualTo() (which is more readable in my opinion).

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds good. Then we should also rename isVersionGreater() => isVersionGreaterThan() for consistency.

throw new Error('Please upgrade git to version 2.11 or higher.');
}
};
8 changes: 6 additions & 2 deletions source/prerequisite-tasks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ module.exports = (input, pkg, options) => {
{
title: 'Ping npm registry',
skip: () => pkg.private || isExternalRegistry,
task: async () => npm.checkConnection()
itaisteinherz marked this conversation as resolved.
Show resolved Hide resolved
task: () => npm.checkConnection()
},
{
title: 'Verify user is authenticated',
Expand All @@ -33,9 +33,13 @@ module.exports = (input, pkg, options) => {
}
}
},
{
title: 'Verify git version is recent',
task: () => git.verifyRecentGitVersion()
},
{
title: 'Check git remote',
task: async () => git.verifyRemoteIsValid()
task: () => git.verifyRemoteIsValid()
},
{
title: 'Validate version',
Expand Down