tree.diffWorkDir #101

Activity

1. nkallen commented on Oct 5, 2013

   

   nkallen

   on Oct 5, 2013

   Contributor

   More actions

   I think this is broken ATM.

   Here is how to fix: edit v0.18.json and add "isOptional": true, to the diffOptions parameter of diffWorkDir. Recompile using the instructions in the readme. Use npm link to test this out; submit a pull request when you're done.
2. kmctown commented on Oct 6, 2013

   

   kmctown

   on Oct 6, 2013

   CollaboratorAuthor

   More actions

   I gave it a shot -- it changed the error message but still doesn't appear to work.

   Changed JSON for the opts parameter of diffWorkDir:

   {
     "name": "opts",
     "cType": "const git_diff_options *",
     "cppClassName": "GitDiffOptions",
     "jsClassName": "DiffOptions",
     "isOptional": true,
     "comment": "Structure with options to influence diff or NULL for defaults."
   }

   Here is how the function is getting called:

   tree.diffWorkDir(repo, function(diffList) {
     ...
   }

   Here is the error message:

   tree.diffWorkDir(repo, function(err, diffList) {
   Error: Callback is required and must be a Function.
   

   I wouldn't even mind passing in the DiffOpts as opposed to omitting the argument, but I don't see how to pass in a DiffOpts object as it doesn't appear to exist. Even if I try to just pass in a JSON object I don't know what to put in there. Tried specifying some fields/values based on libgit2 docs without success.
3. kmctown commented on Oct 7, 2013

   

   kmctown

   on Oct 7, 2013

   CollaboratorAuthor

   More actions

   I believe the root of the problem is in asyncFunction.cc.ejs. The code that generates the check functions doesn't take into account the fact some arguments may be optional and generates static indexing that may or may not be accurate at runtime.

   if (args.Length() == <%- jsArg %> || !args[<%- jsArg %>]->IsFunction()) {
       return ThrowException(Exception::Error(String::New("Callback is required and must be a Function.")));
   }

   In the case of diffWorkDir, this generates the following code in tree.cc:

   if (args.Length() == 0 || !args[0]->IsObject()) {
       return ThrowException(Exception::Error(String::New("Repository repo is required.")));
   }
   
   if (args.Length() == 2 || !args[2]->IsFunction()) {
       return ThrowException(Exception::Error(String::New("Callback is required and must be a Function.")));
   }

   When I omit the DiffOpts argument, argument 0 is the repo and argument 1 is the callback, which doesn't fly with the check functions.

   I believe this is the issue, but am not sure how to go about correcting it because I'm not familiar with the templates. Will spend some time looking into it.
4. nkallen commented on Oct 7, 2013

   

   nkallen

   on Oct 7, 2013

   Contributor

   More actions

   I think it'll work if you just pass null:

   tree.diffWorkDir(repo, null, function(err, diffList) {
5. nkallen commented on Oct 7, 2013

   

   nkallen

   on Oct 7, 2013

   Contributor

   More actions

   Currently, there's no way to instantiate a DiffOptions object directly... It is several hours of work to get this working, so I don't expect to get to it soon -- and you don't need this yet anyway.
6. kmctown commented on Oct 7, 2013

   

   kmctown

   on Oct 7, 2013

   CollaboratorAuthor

   More actions

   Thanks, passing null works. I tried that before but it only works after changing the DiffOptions argument to optional. Pull request has been submitted.
7. kmctown

   closed this as completedon Oct 7, 2013