This looks fantastic! A+ man. Can you switch the clones tests over to using another library to confirm that we work with other promise implementations? Bluebird or then would be good.

👯 !

I would also test vanilla node promises (since they exist in 0.12+, which is where our support starts).

Good call! I tested with native promises since that was the easiest, and got it to work. On the native side I just needed to replace a Nan::Get with a Nan::GetRealNamedProperty, and on the test side I had to modify some uses of Promise.resolve and Promise.reject.

It turns out (at least in node 4.2.3 that I was testing with) that native Promises don't let you call Promise.resolve or Promise.reject without the Promise context. That is, if you try:

var resolve = Promise.resolve;
resolve(1);

you get a crash. And we had test code like this:

    return Remote.delete(repository, "origin3")
      .then(function() {
        return Remote.lookup(repository, "origin3");
      })
      .then(Promise.reject, Promise.resolve);

The last line results in the same crash. Use cases like this just need to be modified to pass Promise.reject.bind(Promise) and Promise.resolve.bind(Promise) instead. That or we Promise.resolve = Promise.resolve.bind(Promise).

Would you like me to replace var Promise = require('nodegit-promise') with var Promise = global.Promise || require('nodegit-promise') as part of this PR (and fixup the uses of Promise.resolve and Promise.reject described above) - at least in tests?

Just FYI, the last thing I am working on is ensuring that PromiseCompletion objects get freed correctly. I don't think they are currently - even if I force garbage collection, I never seem to get a hit in the destructor.

Actually, I may have been wrong about needing GetRealNamedProperty instead of Get...

Yeah... best I can tell, GetRealNamedProperty is like Get except it doesn't run interceptors. Going back to Get.

Found and fixed the memory leak - the majority of the problem was with the fact that when using a FunctionTemplate, "The lifetime of the created function is equal to the lifetime of the context". I was creating templates and functions willy-nilly, and the way I was doing it was keeping handles on the PromiseCompletion instances.

Taking off the WIP, but someone better at Nan/v8 than me might want to take a close look at the code.

@srajko require('nodegit-promise') is scattered throughout the application in both the/liband the/test` folder. We should remove all of those and then make sure that the tests still run. I think that would be sufficient for this PR to be considered finished.

AppVeyor timed out. I'll re-run

https://github.com/nodegit/nodegit/pull/854/files#r49623812 might fix the binding issue?