-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
Ejecting should ensure you have clean git status #2090
Conversation
Wow, great! I'll take a peek at this soon. |
@@ -36,6 +37,31 @@ prompt( | |||
process.exit(1); | |||
} | |||
|
|||
// Make sure there are no dirty git status | |||
const git = fs.existsSync(path.join(process.cwd(), '.git')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to use https://github.com/sindresorhus/find-up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @thejameskyle, can you explain what are the benefits of using that library? Is it because it returns promised result? Cause I was trying to avoid using third-party library as possible. If promise is necessary I could just use instantiate Promise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@milocosmopolitan someone might have their git directory several levels up the tree, especially if it's a monorepo.
However, you shouldn't be checking for the existence of .git
at all -- simply execute a git command in the directory and see if it errors out 😄.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See 1288's files for inspiration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Timer, I've made a change according to your suggestion.
try { | ||
let stdout = execSync(`git status -s`).toString(); | ||
let status = { dirty: 0, untracked: 0 }; | ||
stdout.trim().split(/\r?\n/).forEach(file => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to use git status --porcelain
for easier parsing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool feature! Thanks for the PR! I saw some things, so I hope you don't mind the extra review. 😄
console.error( | ||
`Git repository has ${status.dirty} dirty ${status.dirty > 1 ? 'files' : 'file'}. ` + | ||
'We cannot continue as you would lose all the changes in that file or directory. ' + | ||
'Please push commit before and run this command again.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this error message covers all the right bases. The grammar however is a bit confusing.
The first sentence mentions that you have 1 or more dirty files, but then the second sentence only refers to a single file or directory. The third sentence isn't totally clear grammatically and it states that you need to push as well as commit in order to make the directory clean. While pushing may be ideal, it isn't the same as committing and only committing is really necessary here.
What do you think of this as the error message instead?
`This git repository has ${status.dirty} dirty ${status.dirty > 1 ? 'files' : 'file'}. ` +
'We cannot continue as you would lose all of your changes. ' +
'Please commit your changes with `git commit` and then run this command again.'
This way, the user knows what to do to fix the problem and the message is completely clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My attempt at the grammar on this:
`This git repository has ${status.dirty} ${status.dirty > 1 ? 'files' : 'file'} with uncommitted changes.` +
'Ejecting would cause these files to be overwritten. ' +
'Please commit your changes with `git commit` and then run this command again.'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for those suggestions @sunjay, @taylorlapeyre. I personally liked @taylorlapeyre version. I will make an update soon.
function statusSync() { | ||
try { | ||
let stdout = execSync(`git status --porcelain`).toString(); | ||
let status = { dirty: 0, untracked: 0 }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're also counting untracked files, why isn't that used in the dirty check below? If it shouldn't be used, why is it being counted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I think untracked files should be exclude. I will make pull request on this tonight.
Hi! Could you please rebase this? I'd love to get this in. |
Oh, yeah I thought we had. Must've been a duplicate PR. |
#2082