Got git targets working (when they're pointing to github).

Got git targets working (when they're pointing to github).

As a stop-gap to getting git working completely in tavern, I've used
Github's APIs to download a tar.gz of the master/head.

For example, the following pubspec works:

name: git-test
dependencies:
  test_tavern_library:
    git:
      url: git@github.com/Pat-Laub/test_tavern_library


Note: only github repos will work, and only those with a valid pubspec
file.

R=tapted@chromium.org

Committed: https://github.com/tapted/bleeding_edge/commit/a758118

[pajamallama, 2014-01-29 02:40:36]

Git is working :)

[tapted, 2014-01-29 03:09:41]

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
File dart/sdk/lib/_internal/pub/lib/src/source/git.dart (right):

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/source/git.dart:45: var oldUrl = _getUrl(id);
a comment, like: ~"If git is not installed, and the url is on github, try
downloading a snapshot of HEAD".

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/source/git.dart:48: String repo =
gitUri.pathSegments[1].split('.')[0];
Maybe require 'github.com' to be in pathSegments[1], and if that's not the case,
throw the original exception

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/source/git.dart:51: log.io('Downloading
${id.name} ${id.version} from $url...');
nit: keep quotes consistent - maybe stick with double quotes here

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/source/git.dart:54: "cache", "git", id.name);
nit: indent 2 more spaces

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/source/git.dart:60: } else {
nit: don't need the else, since there's an early return statement above

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/source/git.dart:125: // Future<PackageId>
resolveId(PackageId id) {
does this need to commented out? (i.e. what's calling it?)

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
File dart/sdk/lib/_internal/pub/lib/src/wrap/http_wrap.dart (right):

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/wrap/http_wrap.dart:30: if
(request.readyState != 4)
wow - how did they not make this an enum... Maybe in the log line mention that 4
means "complete". But also is this expected to happen? Should it return in this
casE?

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
File dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart (right):

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:218: var splitted =
zipFilename.split('/');
maybe 'parts' instead of 'splitted'

[pajamallama, 2014-01-29 05:33:09]

Nit fixes & responses

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
File dart/sdk/lib/_internal/pub/lib/src/source/git.dart (right):

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/source/git.dart:45: var oldUrl = _getUrl(id);
On 2014/01/29 03:09:42, tapted wrote:
> a comment, like: ~"If git is not installed, and the url is on github, try
> downloading a snapshot of HEAD". 

Done.

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/source/git.dart:48: String repo =
gitUri.pathSegments[1].split('.')[0];
On 2014/01/29 03:09:42, tapted wrote:
> Maybe require 'github.com' to be in pathSegments[1], and if that's not the
case,
> throw the original exception

Done.

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/source/git.dart:51: log.io('Downloading
${id.name} ${id.version} from $url...');
On 2014/01/29 03:09:42, tapted wrote:
> nit: keep quotes consistent - maybe stick with double quotes here

Done.

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/source/git.dart:54: "cache", "git", id.name);
On 2014/01/29 03:09:42, tapted wrote:
> nit: indent 2 more spaces

Done.

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/source/git.dart:60: } else {
On 2014/01/29 03:09:42, tapted wrote:
> nit: don't need the else, since there's an early return statement above

This is all for if git is installed on the command line (I wanted to delete it
but that always seems to be the wrong answer..).

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/source/git.dart:125: // Future<PackageId>
resolveId(PackageId id) {
On 2014/01/29 03:09:42, tapted wrote:
> does this need to commented out? (i.e. what's calling it?)

Entrypoint (line 103) calls 'source.resolveId(id)' which returns id itself if
the Source abstract method is called but stuffs things up if the
GitSource.resolveId is called (_revisitionAt starts up git CLI).

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
File dart/sdk/lib/_internal/pub/lib/src/wrap/http_wrap.dart (right):

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/wrap/http_wrap.dart:30: if
(request.readyState != 4)
On 2014/01/29 03:09:42, tapted wrote:
> wow - how did they not make this an enum... Maybe in the log line mention that
4
> means "complete". But also is this expected to happen? Should it return in
this
> casE?

I added this to try catch the error where api.github.com redirects the app to a
page outside the manifest's permissions. Just tested it then & it didn't catch
that, so removing it.

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
File dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart (right):

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:218: var splitted =
zipFilename.split('/');
On 2014/01/29 03:09:42, tapted wrote:
> maybe 'parts' instead of 'splitted'

Done.

[tapted, 2014-01-29 06:02:46]

lgtm - just some nits

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
File dart/sdk/lib/_internal/pub/lib/src/source/git.dart (right):

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/source/git.dart:48: String repo =
gitUri.pathSegments[1].split('.')[0];
On 2014/01/29 05:33:09, pajamallama wrote:
> On 2014/01/29 03:09:42, tapted wrote:
> > Maybe require 'github.com' to be in pathSegments[1], and if that's not the
> case,
> > throw the original exception
> 
> Done.

Can you update the URI example in the CL description too :)

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/source/git.dart:60: } else {
On 2014/01/29 05:33:09, pajamallama wrote:
> On 2014/01/29 03:09:42, tapted wrote:
> > nit: don't need the else, since there's an early return statement above
> 
> This is all for if git is installed on the command line (I wanted to delete it
> but that always seems to be the wrong answer..).

Yeah, if we can get both dart2js and dart CLI happy with the same codebase then
there's a better chance to maintain/upstream this. 

But here I just meant you don't need to indent the 'else' -- the early return
will exit the function, so the else is redundant. (and avoiding unnecessary
indentation tends to improve readability).

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/source/git.dart:125: // Future<PackageId>
resolveId(PackageId id) {
On 2014/01/29 05:33:09, pajamallama wrote:
> On 2014/01/29 03:09:42, tapted wrote:
> > does this need to commented out? (i.e. what's calling it?)
> 
> Entrypoint (line 103) calls 'source.resolveId(id)' which returns id itself if
> the Source abstract method is called but stuffs things up if the
> GitSource.resolveId is called (_revisitionAt starts up git CLI).

Oooh inheritance - fancy. I would just rename the function `resolveGitId` rather
than comment it out for now. And add a

TODO(..): Rename this back to resolveId once the commit hash can be fetched.

https://codereview.appspot.com/58100043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/source/git.dart (right):

https://codereview.appspot.com/58100043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/source/git.dart:52: "Cannot get '${id.name}'
from Git (${_getUrl(id)}).\n"
nit: indent this and the next ilne two more spaces

[pajamallama, 2014-01-29 23:23:32]

Will update CL description after I can ask you what you want added.

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
File dart/sdk/lib/_internal/pub/lib/src/source/git.dart (right):

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/source/git.dart:60: } else {
On 2014/01/29 06:02:46, tapted wrote:
> On 2014/01/29 05:33:09, pajamallama wrote:
> > On 2014/01/29 03:09:42, tapted wrote:
> > > nit: don't need the else, since there's an early return statement above
> > 
> > This is all for if git is installed on the command line (I wanted to delete
it
> > but that always seems to be the wrong answer..).
> 
> Yeah, if we can get both dart2js and dart CLI happy with the same codebase
then
> there's a better chance to maintain/upstream this. 
> 
> But here I just meant you don't need to indent the 'else' -- the early return
> will exit the function, so the else is redundant. (and avoiding unnecessary
> indentation tends to improve readability).

Done.

https://codereview.appspot.com/58100043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/source/git.dart:125: // Future<PackageId>
resolveId(PackageId id) {
On 2014/01/29 06:02:46, tapted wrote:
> On 2014/01/29 05:33:09, pajamallama wrote:
> > On 2014/01/29 03:09:42, tapted wrote:
> > > does this need to commented out? (i.e. what's calling it?)
> > 
> > Entrypoint (line 103) calls 'source.resolveId(id)' which returns id itself
if
> > the Source abstract method is called but stuffs things up if the
> > GitSource.resolveId is called (_revisitionAt starts up git CLI).
> 
> Oooh inheritance - fancy. I would just rename the function `resolveGitId`
rather
> than comment it out for now. And add a
> 
> TODO(..): Rename this back to resolveId once the commit hash can be fetched.

Done.

https://codereview.appspot.com/58100043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/source/git.dart (right):

https://codereview.appspot.com/58100043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/source/git.dart:52: "Cannot get '${id.name}'
from Git (${_getUrl(id)}).\n"
On 2014/01/29 06:02:46, tapted wrote:
> nit: indent this and the next ilne two more spaces

Done.

[pajamallama, 2014-01-29 23:24:05]

Committed patchset #3 manually as ra758118 (presubmit successful).

[tapted, 2014-01-30 00:28:28]

On 2014/01/29 23:23:32, pajamallama wrote:
> Will update CL description after I can ask you what you want added.

There's no point updating it after the commit, since it's now in the git log.
(you would need to reset --hard && git push --force to revert it and re-commit).

Maybe I grokked the URL incorrectly, but the `@github.com` in the URI looks
weird. I would expect

  url: git://github.com/dart-lang/foo.git

or

  url: git://tapted@gmail.com:@github.com/dart-lang/foo.git

if the URL required a login.

Pub also seems to have supported

  git: https://tapted@gmail.com:@github.com/dart-lang/foo.git

at some point. But http://pub.dartlang.org/doc/pubspec.html#git-packages is now
missing :/