Ejecting should ensure you have clean git status by milocosmopolitan · Pull Request #2090 · react/create-react-app · GitHub

#2082

Wow, great! I'll take a peek at this soon.
Thank you!

You might want to use https://github.com/sindresorhus/find-up

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.

@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 😄.

See 1288's files for inspiration.

@Timer, I've made a change according to your suggestion.

You might want to use git status --porcelain for easier parsing.

See comment above.

Cool feature! Thanks for the PR! I saw some things, so I hope you don't mind the extra review. 😄

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.

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.'

Thanks for those suggestions @sunjay, @taylorlapeyre. I personally liked @taylorlapeyre version. I will make an update soon.

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?

You are right, I think untracked files should be exclude. I will make pull request on this tonight.

…eact-app

…react-app

Hi! Could you please rebase this? I'd love to get this in.

@Timer thought it was merged
#2221

Oh, yeah I thought we had. Must've been a duplicate PR.

milocosmopolitan added 4 commits May 1, 2017 10:18

when build is failing due to ESLint errors, filter warnings out

61cfcec

Ejecting should ensure you have clean git status

bdabc3e

Ejecting should ensure you have clean git status

6ff17ae

comment indication

eca2714

facebook-github-bot added the CLA Signed label May 5, 2017

Timer mentioned this pull request May 5, 2017

Ejecting should ensure you have clean git status (if you use git) and ask you to commit #2082

Closed

Timer added this to the 0.10.0 milestone May 5, 2017

milocosmopolitan added 2 commits May 5, 2017 18:32

fixed es6 const {} issue for version less than node 7

8be4f32

fix no-inner-declarations of function

0d68efe

jamiebuilds reviewed May 5, 2017

View reviewed changes

remove checking .git directory

fe3bb7f

gaearon reviewed May 8, 2017

View reviewed changes

gaearon suggested changes May 8, 2017

View reviewed changes

gaearon modified the milestones: 0.11.0, 0.10.0, 0.10.1 May 8, 2017

Give the output in an easy-to-parse format for scripts

cf57f57

sunjay reviewed May 10, 2017

View reviewed changes

milocosmopolitan and others added 7 commits May 16, 2017 22:04

Merge branch 'master' of https://github.com/milocosmopolitan/create-r…

44ae1e2

…eact-app

Merge branch 'master' into eject

c3dc075

improved error message

e7e0392

count dirty files only

a72073c

Merge branch 'master' of https://github.com/facebookincubator/create-…

70b9b5b

…react-app

solve merge conflict

e317052

Update build.js

ad7cd2c

gaearon removed this from the 1.0.1 milestone May 19, 2017

gaearon modified the milestones: 1.0.x, 1.0.1 May 19, 2017

Timer added the tag: enhancement label Jun 28, 2017

Timer closed this Jul 2, 2017

Dubes mentioned this pull request Jul 20, 2017

Improve untracked files message during eject #2797

Closed

milocosmopolitan mentioned this pull request Jun 17, 2017

Ejecting should ensure you have clean git status #2221

Merged

lock Bot locked and limited conversation to collaborators Jan 21, 2019

+                  try {
+                    let stdout = execSync(`git status -s`).toString();
+                    let status = { dirty: 0, untracked: 0 };
+                    stdout.trim().split(/\r?\n/).forEach(file => {

+                  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.'

+                function statusSync() {
+                  try {
+                    let stdout = execSync(`git status --porcelain`).toString();
+                    let status = { dirty: 0, untracked: 0 };

                 }
+                // Make sure there are no dirty git status
+                const git = fs.existsSync(path.join(process.cwd(), '.git'));

Ejecting should ensure you have clean git status by milocosmopolitan · Pull Request #2090 · react/create-react-app · GitHub

Uh oh!

Uh oh!

Ejecting should ensure you have clean git status - #2090

Ejecting should ensure you have clean git status#2090milocosmopolitan wants to merge 15 commits intoreact:masterreact/create-react-app:masterfrom milocosmopolitan:ejectCopy head branch name to clipboard

Conversation

milocosmopolitan commented May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Timer commented May 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Timer May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon left a comment

Choose a reason for hiding this comment

Uh oh!

sunjay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taylorlapeyre May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Timer commented Jun 28, 2017

Uh oh!

milocosmopolitan commented Jul 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Timer commented Jul 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Ejecting should ensure you have clean git status#2090
milocosmopolitan wants to merge 15 commits into
react:masterfrom
milocosmopolitan:eject

milocosmopolitan commented May 5, 2017 •
edited
Loading

Timer May 5, 2017 •
edited
Loading

taylorlapeyre May 10, 2017 •
edited
Loading

milocosmopolitan commented Jul 2, 2017 •
edited
Loading