Add git init to `ember new` #1369

rondale-sc · 2014-07-13T06:25:52Z

No description provided.

rondale-sc · 2014-07-13T06:28:34Z

This is still slightly a work in progress, I'm having an issue with this throwing an error.

But I wanted to get some peoples thoughts on what I have so far.

I'm pretty sure the commit message could contain more information, but I'm not sure what.
I'm not sure about whether this should be opt-out (as it is currently implemented) or opt in
I'm not sure why tmp isn't being cleared on SIGINT

Any thoughts would be great! :))

rwjblue · 2014-07-13T06:28:48Z

lib/tasks/git-init.js

+    }).then(noop, function(){
+      ui.write(chalk.yellow('Something went wrong with git commit'));
+    }).finally(function(){
+      ui.write(chalk.green('Sucessfully initialized git'));


Finally happens for both resolve and reject cases, so this message is misleading (as it may not have been successful).

Ah, since that is the case what is used for success. I know catch is used for errors.

rondale-sc · 2014-07-13T13:22:31Z

@jakecraige Thanks for taking such a thorough look through this! I really appreciate it :))))

stefanpenner · 2014-07-13T19:02:49Z

lib/tasks/git-init.js

+    if(commandOptions.skipGit) { return Promise.resolve(); }
+
+    return exec('git init')
+    .then(function() {


indentation is incorrect

stefanpenner · 2014-07-13T19:05:00Z

should this detect if git is available and automatically "skip" if for some reason it is not?

ccoenen · 2014-07-14T07:20:00Z

Yes, that would be preferable. It might also not be on everybody's path. Especially MSysGit doesn't do this by default (which i think is a bad default, but that's besides the point).

edit: Damn, you people are fast :D

rondale-sc · 2014-07-14T13:29:25Z

@ccoenen Does rondale-sc@9abcdb1 look like it will do the trick in regard to if git isn't present? The build isn't passing atm, but that'll be fixed sometime early this week (ideally. :)

stefanpenner · 2014-07-14T14:16:59Z

lib/tasks/git-init.js

+
+    return exec('git --version')
+      .catch(function(){
+        return Promise.resolve();


You can leave out return Promise.resolve(); just leave an empty function.

That being said, can we check the error to be the one we expect, if it is "handle it" otherwise rethrow it

Absolutely. I think exec of git --version will just return non-zero if it isn't on the system. I'm going to investigate to see if this the best way to detect git. Pretty sure it isn't which git but I'll check again just to be sure.

which is not installed by default on Window's boxes. So I think git --version is your best bet.

I think git --version is the way to go.

Alternatively, I could use the bash/zsh builtin command -v git which will return the string "git" or nothing depending on availability. That should work for most people on windows and bash/zsh, but I'm not sure it's availability would be wide enough for people with other setups.

If we do go with git --version from exec the fact that it doesn't error should be enough (/cc @stefanpenner). I'm not sure we could get more specific with the error handling. But maybe I'm missing something obvious.

I think this is close to needing a rebase to clean it up some.

command -v won't work on a "plain" windows commandline (cmd.exe shell, which is, sadly, probably the most widely used shell).

Banging against git itself and seeing what happens is really probably the best option cross platform.

This was my thought as well @ccoenen. I was kinda hoping that people programming on windows might use mysysgit which comes with a bash emulator. But you're right there is probably a non trivial segment using cmd.exe or powershell. Eitherway, git --version should do the trick in this case.

rwjblue · 2014-07-16T11:37:26Z

Looks good, only made a small suggestion. Also, have we tested this when git is not available (likely kinda hard), but tweaking the git --version call to use xgit --version locally for testing would do the trick)? I just want to ensure that it does not go boom...

rondale-sc · 2014-07-16T14:12:10Z

@rjackson I tested with xgit and talked to stefan. It now appropriately does the check, and if the check fails doesn't continue. I'm doing some regex matching now to determine if it fails in the way we expect, and if it fails in a different way it will throw the original error. I think this is ready

Oh and I added tomster! :)))

bcardarella · 2014-07-16T14:16:58Z

@stefanpenner what about an opt-in setting in the rc file?

rwjblue · 2014-07-16T14:29:33Z

I don't think that the .ember-cli file works at the moment.

rwjblue · 2014-07-16T14:31:01Z

@rondale-sc - I don't think you will be able to do a meaningful regex, as it is completely dependent on system type, shell, whatever else.

rondale-sc · 2014-07-16T14:37:45Z

@rjackson I think I was able to get decent coverage with this. I tested on maverics, windows 7, and linux (ubuntu). I think this could become more robust if we encounter other systems/shells that aren't currently covered.

thoughts?

rwjblue · 2014-07-16T15:29:26Z

I generally disagree that you can ever be comprehensive. You would have to handle every shell combination on every supported platform to be certain.

I am completely opposed to an error caused here having the ability (if the error is not caught) to cause ember new to fail.

ccoenen · 2014-07-16T15:32:34Z

lib/tasks/git-init.js

+  /command not found/, // *nix
+  /not recognized as an internal or external command/, // cmd.exe
+  /not recognized as the name of a cmdlet, function, script file, or operable program/ // Powershell
+];


these will break for other languages for sure. here's what i get from a non-existent command:

C:\Users\.....\workspace>blarg Der Befehl "blarg" ist entweder falsch geschrieben oder konnte nicht gefunden werden.

I don't think we should include regexes.

@ccoenen What error code does it throw? I think on mac I get 127. Maybe that will be more reliable?

%errorlevel% for various commands.

C:\Users\...\workspace>blarg Der Befehl "blarg" ist entweder falsch geschrieben oder konnte nicht gefunden werden. C:\Users\---\workspace>echo %errorlevel% 9009 C:\Users\---\workspace>git status fatal: Not a git repository (or any of the parent directories): .git C:\Users\---\workspace>echo %errorlevel% 128 C:\Users\---\workspace>git version git version 1.9.0.msysgit.0 C:\Users\---\workspace>echo %errorlevel% 0

Other than the 9009, it looks pretty standard. I wasn't able to find any authorative source or documentation on this, though. But to be honest, i haven't been doing much work shelling out on windows.

ccoenen · 2014-07-16T15:36:33Z

I commented in code, but wanted to make sure that it's not overlooked. Regex-Matching the response really is a recipe for desaster. We should absolutely not do that, unless it's well-defined output from, say, git status --porcelain)

The simple reason is, that there's just too many systems out there. It will break for any number of reasons. In my case, the reason will be: I am on a german system. My error messages read like this:

Der Befehl "git" ist entweder falsch geschrieben oder
konnte nicht gefunden werden.

rwjblue · 2014-07-16T15:37:28Z

IMHO, if git --version fails we should just abort the git init task. This is a wonderful convenience, but should not prevent usage in the case that we cannot handle all possible scenarios.

rondale-sc · 2014-07-16T15:38:04Z

@rjackson How do we do that without swallowing errors elsewhere?

rwjblue · 2014-07-17T01:10:45Z

@rondale-sc - If you just change the catch that you have currently implemented (the one that matches patterns) to:

.catch(function() {
  // if git is not found or an error was thrown during the `git`
  // init process just swallow any errors here
});

That catch would only be catching errors related to the git functionality (which IMHO is completely convenience and not 100% required). Those errors should just be swallowed.

rondale-sc · 2014-07-17T04:49:13Z

@rjackson done! :)

rwjblue · 2014-07-17T11:21:20Z

Looks like we need a rebase here.

rondale-sc · 2014-07-17T13:47:51Z

@rjackson good to go

stefanpenner · 2014-07-19T02:35:51Z

lib/tasks/git-init.js

+var path          = require('path');
+var pkg           = require('../../package.json');
+var fs            = require('fs');
+var _             = require('lodash-node');


can you explicitly require the subset of lodash you are using like we do: https://github.com/stefanpenner/ember-cli/blob/45ab16638ae21225febdebc1e50cef62f4747eca/lib/utilities/parse-options.js#L3

^^ is still important

rwjblue · 2014-07-19T03:24:30Z

Stef left some comments, will also need a rebase.

benlesh · 2014-07-21T16:24:16Z

Maybe I'm misunderstanding the intent here. But it sounds like you're adding a feature to Ember-Cli that is unrelated to anything Ember-Cli is concerned with.

There are large organizations I've worked for that don't use Git and/or don't want to use Git. Food for thought.

I also foresee evil:

"Can you have it automagically add an initial commit?"
"Can you integrate a version bump?"
"... with a commit?"
"... and a push?"
"Can you add support for SVN? TFS? Hg?"

The node prerequisite is one thing, because it's completely necessary. But I think I can type "git init" all on my own.

If you must, you must... but at least this should probably be an "opt-in" rather than an "opt-out".

jdjkelly · 2014-07-21T16:39:23Z

@Blesh by the same token - it's really easy to remove a git repository. I think this comes down to being a smart default - one that's easily removed without having to know really anything about ember-cli.

Add git init to `ember new`

benlesh · 2014-07-21T20:00:07Z

@jdjkelly "smart defaults" are used to justify junkware on customer electronics. Having an "opinionated" framework like Ember, and an "opinionated" build tool like Ember-CLI is an amazing boon... However, "opinionated" crosses the line when it's dealing with things that really aren't its concern. Ember-CLI shouldn't tell me what VCS I should use.

I personally always use Git. But there is still a very large portion of the population that does not.

This being GitHub, which means near 100% of the people reading this are Git users, I'm sure I'm the only voice of dissent on this issue... but I think this is a really bad idea. I'm willing to be convinced otherwise.

stefanpenner · 2014-07-21T20:22:57Z

For people not using git. Nothing happens

rwjblue · 2014-07-21T20:33:51Z

Ember CLI is essentially a curated bag of best practices. This is simple to opt-out of, and if you don't have git it silently continues.

@rondale-sc - Would you please submit a PR to the documentation to mention how to opt-out of this via --skip-git? It can't be merged until 0.0.40 is shipped, but it will make this easier once we release.

mehulkar · 2014-07-22T16:42:42Z

dat initial commit msg...

joostdevries · 2015-01-28T10:39:07Z

@stefanpenner Any reason we're always showing Could not find watchman, falling back to NodeWatcher for file system events but we're not showing Could not find git, no repository initialized here?

rwjblue reviewed Jul 13, 2014
View reviewed changes

stefanpenner reviewed Jul 13, 2014
View reviewed changes

stefanpenner reviewed Jul 14, 2014
View reviewed changes

ccoenen reviewed Jul 16, 2014
View reviewed changes

stefanpenner reviewed Jul 19, 2014
View reviewed changes

Add git init to ember new

b0ebb41

stefanpenner added a commit that referenced this pull request Jul 21, 2014

Merge pull request #1369 from rondale-sc/add_git_init_to_ember_new

2e53f90

Add git init to `ember new`

stefanpenner merged commit 2e53f90 into ember-cli:master Jul 21, 2014

rondale-sc mentioned this pull request Jul 22, 2014

Update getting started to reflect recent git init addition #1433

Merged

jakecraige mentioned this pull request Jul 28, 2014

Uncaught Error: Could not find module /app #1512

Closed

joostdevries mentioned this pull request Jan 28, 2015

Cannot call method 'slice' of null #3129

Closed

Add git init to ember new #1369

Add git init to ember new #1369

Conversation

rondale-sc commented Jul 13, 2014

rondale-sc commented Jul 13, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rondale-sc commented Jul 13, 2014

Choose a reason for hiding this comment

stefanpenner commented Jul 13, 2014

ccoenen commented Jul 14, 2014

rondale-sc commented Jul 14, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwjblue commented Jul 16, 2014

rondale-sc commented Jul 16, 2014

bcardarella commented Jul 16, 2014

rwjblue commented Jul 16, 2014

rwjblue commented Jul 16, 2014

rondale-sc commented Jul 16, 2014

rwjblue commented Jul 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccoenen commented Jul 16, 2014

rwjblue commented Jul 16, 2014

rondale-sc commented Jul 16, 2014

rwjblue commented Jul 17, 2014

rondale-sc commented Jul 17, 2014

rwjblue commented Jul 17, 2014

rondale-sc commented Jul 17, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwjblue commented Jul 19, 2014

benlesh commented Jul 21, 2014

I also foresee evil:

jdjkelly commented Jul 21, 2014

benlesh commented Jul 21, 2014

stefanpenner commented Jul 21, 2014

rwjblue commented Jul 21, 2014

mehulkar commented Jul 22, 2014

joostdevries commented Jan 28, 2015

Add git init to `ember new` #1369

Add git init to `ember new` #1369