tarmstrong / nbdiff

A diffing and merging tool for the IPython Notebook.
http://nbdiff.org
MIT License
212 stars 28 forks source link

Make nbdiff without arguments work with a hg repo #231

Closed tmbb closed 10 years ago

tmbb commented 10 years ago

This pull request uses python-hglib instead of raw hg commands. This adds another depencency to the project but makes the code much simpler.

I haven't written any tests yet. I might write them in the future, when I have more time.

tmbb commented 10 years ago

(only works when called in the root folder of the repository)

bpipev commented 10 years ago

Good work! It's working. There's one small problem though. It tries to diff unmerged notebooks as well, which it shouldn't.

tmbb commented 10 years ago

I have to study more about hg to fix that (in fact, I usually program solo, and as such I don't understand much about merging changes). Thanks for noticing the issue.

tmbb commented 10 years ago

I've read hg's documentation and aparently, a file is unmerged if and only if the command 'hg resolve --list' returns something like: U filename

Is this right? If it is right, then I know how to fix the problem.

bpipev commented 10 years ago

Yes, I think that is correct.

tmbb commented 10 years ago

How are you testing my pull request? Do you have a repo with unmerged files I could use? I can create it myself, of course. I was just wondering if you have a particulary hairy repository I could test my patch with.

tarmstrong commented 10 years ago

@tmbb : our manual testing scripts aren't terribly documented -- sorry about that -- but you can find them in scripts/.

./make_merge_conflict.py (not the .sh) will create a folder with a git repository in it that you can test with. The examples are very gentle but they do save you the hassle of creating your own merge conflicts.

tarmstrong commented 10 years ago

Strike my last, they are actually reasonably well-documented :) . See the readme at https://github.com/tarmstrong/nbdiff/tree/master/scripts

tmbb commented 10 years ago

nbdiff no longer tries to diff unmerged notebooks

bpipev commented 10 years ago

I've tested your latest changes and here are some bugs I found:

bobi@bobi-VirtualBox:~/git$ nbdiff
Traceback (most recent call last):
  File "/usr/local/bin/nbdiff", line 9, in <module>
    load_entry_point('nbdiff==1.0.4', 'console_scripts', 'nbdiff')()
  File "/home/bobi/git/nbdiff_other/nbdiff/nbdiff/commands.py", line 120, in diff
    modified_notebooks = vcs.get_modified_notebooks()
  File "/home/bobi/git/nbdiff_other/nbdiff/nbdiff/adapter/hg_adapter.py", line 79, in get_modified_notebooks
    unmerged = [path for (status, path) in client.resolve(listfiles=True)
  File "/usr/local/lib/python2.7/dist-packages/hglib/client.py", line 1257, in resolve
    out = self.rawcommand(args)
  File "/usr/local/lib/python2.7/dist-packages/hglib/client.py", line 179, in rawcommand
    raise error.CommandError(args, ret, out, err)
hglib.error.CommandError: (255, '', "abort: no repository found in '/home/bobi/git' (.hg not found)!")
bobi@bobi-VirtualBox:~/git/tests/hg_diff_root/First_inner$ nbdiff
Traceback (most recent call last):
  File "/usr/local/bin/nbdiff", line 9, in <module>
    load_entry_point('nbdiff==1.0.4', 'console_scripts', 'nbdiff')()
  File "/home/bobi/git/nbdiff_other/nbdiff/nbdiff/commands.py", line 120, in diff
    modified_notebooks = vcs.get_modified_notebooks()
  File "/home/bobi/git/nbdiff_other/nbdiff/nbdiff/adapter/hg_adapter.py", line 87, in get_modified_notebooks
    current_local_notebook = open(abspath)
IOError: [Errno 2] No such file or directory: '/home/bobi/git/tests/hg_diff_root/First_inner/First_inner/Second_inner/User_Interface.ipynb'
tmbb commented 10 years ago

@bpipev I can't reproduce your errors, and in my setup the tests pass (the Travis CI build tool says one test fails). Any ideas? (sorry for my inexperience with git, github and version control in general)

bpipev commented 10 years ago

The build error was not your fault. Sometimes it randomly fails for some reason. I just rebuilt it and it's fine. I'm not sure why you can't reproduce the errors though...

tmbb commented 10 years ago

About the error, are you sure you are using either revisions af290f9 or 83b75d6? In one of the older revisions I got those errors, but not anymore

bpipev commented 10 years ago

I'm sure I have the latest revision. When you do hglib.open(dirpath) where dirpath is a path that doesn't have a repo, do you get an exception? Because I don't.

tmbb commented 10 years ago

I get a ServerError exception.

tmbb commented 10 years ago

Well, if you supply an empty string or None it opens the same repo as calling hg in the same repo would open. But if you call it on an invalid nonempty string it raises a ServerError

bpipev commented 10 years ago

No matter what string I give it, I don't get any exception.

bpipev commented 10 years ago

Anyway, for now can you add something like

client = hglib.open(dirpath)
client.root() 
return client, dirpath

to check if it's inside hg repo?

tmbb commented 10 years ago

So, when you run hglib.open(), you don't get an error, but when you run client.root() you get an error. Is that right?

Also, you seem to be testing this in a virtual box. What OS is it emulating? (I can't test it myself in a virtualbox because my computer seems to be far too slow to run it)

bpipev commented 10 years ago

So, when you run hglib.open(), you don't get an error, but when you run client.root() you get an error. Is that right?

That is correct. I think that any function except open would give me that error.

Also, you seem to be testing this in a virtual box. What OS is it emulating? (I can't test it myself in a virtualbox because my computer seems to be far too slow to run it)

I'm running Ubuntu 14.04 64bit.

tmbb commented 10 years ago

The code should work now. The error we depend on is now raised by client.root() instead of hglib.open(path). The code is also much simpler now, because hglib does all the work of trying to find out if we are inside a hg repo and getting its root.

bpipev commented 10 years ago

Looks good to me. Good work!