Merge pull request #281 from vidartf/add-diff-driver-args

Use diff args for git diff driver

nbdime/args.py

@@ -197,9 +197,10 @@ def add_diff_args(parser):
         help="process/ignore metadata.")
 
 
+
+diff_exclusives = ('sources', 'outputs', 'attachments', 'metadata')
 def process_diff_flags(args):
-    exclusives = ('sources', 'outputs', 'attachments', 'metadata')
-    process_exclusive_ignorables(args, exclusives)
+    process_exclusive_ignorables(args, diff_exclusives)
     set_notebook_diff_targets(args.sources, args.outputs,
                               args.attachments, args.metadata)
 

nbdime/gitdiffdriver.py

@@ -24,7 +24,7 @@
 from subprocess import check_call, CalledProcessError
 
 from . import nbdiffapp
-from .args import add_git_config_subcommand
+from .args import add_git_config_subcommand, add_diff_args, diff_exclusives
 from .utils import locate_gitattributes, ensure_dir_exists
 
 
@@ -77,6 +77,7 @@ def main(args=None):
     diff_parser = subparsers.add_parser('diff',
         description="The actual entrypoint for the diff tool. Git will call this."
     )
+    add_diff_args(diff_parser)
     # Argument list
     # path old-file old-hex old-mode new-file new-hex new-mode [ rename-to ]
     diff_parser.add_argument('path')
@@ -97,7 +98,9 @@ def main(args=None):
 
     opts = parser.parse_args(args)
     if opts.subcommand == 'diff':
-        return nbdiffapp.main([opts.a, opts.b])
+        return nbdiffapp.main(
+            [opts.a, opts.b] +
+            ['--%s' % name for name in diff_exclusives if getattr(opts, name)])
     elif opts.subcommand == 'config':
         opts.config_func(opts.scope)
         return 0

nbdime/tests/conftest.py

@@ -17,12 +17,13 @@
 
 from jsonschema import Draft4Validator as Validator
 from jsonschema import RefResolver
-from pytest import yield_fixture, fixture, skip
+from pytest import fixture, skip
 import nbformat
 
 from .utils import call, have_git
 
 from nbdime.webapp.nbdimeserver import init_app
+from nbdime.diffing.notebooks import set_notebook_diff_targets
 
 pjoin = os.path.join
 
@@ -149,7 +150,7 @@ def git_repo2(tmpdir, request, filespath, needs_git):
     return repo
 
 
-@yield_fixture
+@fixture
 def reset_log():
     # clear root logger handlers before test and reset afterwards
     handlers = list(logging.getLogger().handlers)
@@ -323,3 +324,12 @@ def unique_port():
     global _port
     _port += 1
     return _port
+
+
+@fixture()
+def reset_diff_targets():
+    try:
+        yield
+    finally:
+        # Reset diff targets (global variable)
+        set_notebook_diff_targets()

nbdime/tests/test_cli_apps.py

@@ -106,7 +106,7 @@ def test_nbdiff_app_unicode_safe(filespath):
     check_call([sys.executable, '-m', 'nbdime.nbdiffapp', afn, bfn], env=env)
 
 
-def test_nbdiff_app_only_source(filespath, tmpdir):
+def test_nbdiff_app_only_source(filespath, tmpdir, reset_diff_targets):
     afn = os.path.join(filespath, "multilevel-test-base.ipynb")
     bfn = os.path.join(filespath, "multilevel-test-local.ipynb")
     dfn = os.path.join(tmpdir.dirname, "diff_output.json")
@@ -122,7 +122,7 @@ def test_nbdiff_app_only_source(filespath, tmpdir):
         diff = diff[0]['diff']
 
 
-def test_nbdiff_app_ignore_source(filespath, tmpdir):
+def test_nbdiff_app_ignore_source(filespath, tmpdir, reset_diff_targets):
     afn = os.path.join(filespath, "multilevel-test-base.ipynb")
     bfn = os.path.join(filespath, "multilevel-test-local.ipynb")
     dfn = os.path.join(tmpdir.dirname, "diff_output.json")
@@ -439,7 +439,7 @@ def _wait_up(url, interval=0.1, check=None):
             break
 
 
-@pytest.mark.timeout(timeout=WEB_TEST_TIMEOUT)
+@pytest.mark.timeout(timeout=3*WEB_TEST_TIMEOUT)
 def test_difftool(git_repo, request, unique_port):
     nbdime.gitdifftool.main(['config', '--enable'])
     cmd = get_output('git config --get --local difftool.nbdime.cmd').strip()
@@ -478,7 +478,7 @@ def _term():
     assert process.poll() == 0
 
 
-@pytest.mark.timeout(timeout=WEB_TEST_TIMEOUT)
+@pytest.mark.timeout(timeout=3*WEB_TEST_TIMEOUT)
 def test_mergetool(git_repo, request, unique_port):
     nbdime.gitmergetool.main(['config', '--enable'])
     cmd = get_output('git config --get --local mergetool.nbdime.cmd').strip()

nbdime/tests/test_git_diffdriver.py

@@ -39,6 +39,27 @@
 
 """
 
+expected_source_only = """nbdiff {0} {1}
+--- {0}  {2}
++++ {1}  {3}
+## modified /cells/0/source:
+@@ -1,3 +1,3 @@
+-def foe(x, y):
++def foo(x, y):
+     return x + y
+-foe(3, 2)
++foo(1, 2)
+
+## modified /cells/1/source:
+@@ -1,3 +1,3 @@
+-def foo(x, y):
++def foe(x, y):
+     return x * y
+-foo(1, 2)
++foe(1, 2)
+
+"""
+
 
 def test_git_diff_driver(capsys, nocolor, needs_git):
     # Simulate a call from `git diff` to check basic driver functionality
@@ -60,3 +81,25 @@ def test_git_diff_driver(capsys, nocolor, needs_git):
         assert r == 0
         cap_out = capsys.readouterr()[0]
         assert cap_out == expected_output.format(fn1, fn2, t1, t2)
+
+
+def test_git_diff_driver_flags(capsys, nocolor, needs_git, reset_diff_targets):
+    # Simulate a call from `git diff` to check basic driver functionality
+    test_dir = os.path.abspath(os.path.dirname(__file__))
+
+    fn1 = pjoin(test_dir, 'files/foo--1.ipynb')
+    fn2 = pjoin(test_dir, 'files/foo--2.ipynb')
+    t1 = file_timestamp(fn1)
+    t2 = file_timestamp(fn2)
+
+    mock_argv = [
+        '/mock/path/git-nbdiffdriver', 'diff', '-s',
+        fn1,
+        fn1, 'invalid_mock_checksum', '100644',
+        fn2, 'invalid_mock_checksum', '100644']
+
+    with mock.patch('sys.argv', mock_argv):
+        r = gdd_main()
+        assert r == 0
+        cap_out = capsys.readouterr()[0]
+        assert cap_out == expected_source_only.format(fn1, fn2, t1, t2)

nbdime/tests/test_merge_notebooks.py

@@ -187,7 +187,6 @@ def _check(partial, expected_partial, decisions, expected_conflicts):
     for e, d in zip(expected_conflicts, conflicts):
         # Only check keys specified in expectation value
         for k in sorted(e.keys()):
-            #if d[k] != e[k]: import ipdb; ipdb.set_trace()
             assert d[k] == e[k]
 
 
@@ -892,64 +891,61 @@ def test_autoresolve_empty_strategies():
     _check(partial, expected_partial, decisions, expected_conflicts)
 
 
-class TestMergeDiffIgnores:
+def test_only_sources(db, reset_diff_targets):
+    base = db["mixed-conflicts--1"]
+    local = db["mixed-conflicts--2"]
+    remote = db["mixed-conflicts--3"]
+    set_notebook_diff_targets(True, False, False, False)
 
-    def teardown(self):
-        set_notebook_diff_targets(True, True, True, True)
+    merge_args = copy.deepcopy(args)
+    merge_args.merge_strategy = "mergetool"
 
-    def test_only_sources(self, db):
-        base = db["mixed-conflicts--1"]
-        local = db["mixed-conflicts--2"]
-        remote = db["mixed-conflicts--3"]
-        set_notebook_diff_targets(True, False, False, False)
+    partial, decisions = merge_notebooks(base, local, remote, merge_args)
 
-        merge_args = copy.deepcopy(args)
-        merge_args.merge_strategy = "mergetool"
+    assert len(decisions) > 0
+    for d in decisions:
+        path = d['common_path']
+        # Still have some decisions on cell root, so avoid with len == 2 check
+        assert len(path) == 2 or path[2] == 'source'
 
-        partial, decisions = merge_notebooks(base, local, remote, merge_args)
 
-        assert len(decisions) > 0
-        for d in decisions:
-            path = d['common_path']
-            # Still have some decisions on cell root, so avoid with len == 2 check
-            assert len(path) == 2 or path[2] == 'source'
+def test_only_outputs(db, reset_diff_targets):
+    base = db["mixed-conflicts--1"]
+    local = db["mixed-conflicts--2"]
+    remote = db["mixed-conflicts--3"]
+    set_notebook_diff_targets(False, True, False, False)
 
-    def test_only_outputs(self, db):
-        base = db["mixed-conflicts--1"]
-        local = db["mixed-conflicts--2"]
-        remote = db["mixed-conflicts--3"]
-        set_notebook_diff_targets(False, True, False, False)
+    merge_args = copy.deepcopy(args)
+    merge_args.merge_strategy = "mergetool"
 
-        merge_args = copy.deepcopy(args)
-        merge_args.merge_strategy = "mergetool"
+    partial, decisions = merge_notebooks(base, local, remote, merge_args)
 
-        partial, decisions = merge_notebooks(base, local, remote, merge_args)
+    assert len(decisions) > 0
+    for d in decisions:
+        path = d['common_path']
+        # Still have some decisions on cell root, so avoid with len == 2 check
+        assert len(path) == 2 or path[2] == 'outputs'
 
-        assert len(decisions) > 0
-        for d in decisions:
-            path = d['common_path']
-            # Still have some decisions on cell root, so avoid with len == 2 check
-            assert len(path) == 2 or path[2] == 'outputs'
 
-    def test_only_metadata(self, db):
-        base = db["mixed-conflicts--1"]
-        local = db["mixed-conflicts--2"]
-        remote = db["mixed-conflicts--3"]
-        set_notebook_diff_targets(False, False, False, True)
+def test_only_metadata(db, reset_diff_targets):
+    base = db["mixed-conflicts--1"]
+    local = db["mixed-conflicts--2"]
+    remote = db["mixed-conflicts--3"]
+    set_notebook_diff_targets(False, False, False, True)
 
-        merge_args = copy.deepcopy(args)
-        merge_args.merge_strategy = "mergetool"
+    merge_args = copy.deepcopy(args)
+    merge_args.merge_strategy = "mergetool"
 
-        partial, decisions = merge_notebooks(base, local, remote, merge_args)
+    partial, decisions = merge_notebooks(base, local, remote, merge_args)
 
-        assert len(decisions) > 0
-        for d in decisions:
-            path = d['common_path']
-            # Still have some decisions on cell root, so avoid with len == 2 check
-            assert (
-                len(path) == 2 or
-                path[0] == 'metadata' or
-                path[2] == 'metadata' or
-                path[4] == 'metadata'
-            )
+    assert len(decisions) > 0
+    for d in decisions:
+        path = d['common_path']
+        # Still have some decisions on cell root, so avoid with len == 2 check
+        assert (
+            len(path) == 2 or
+            path[0] == 'metadata' or
+            path[2] == 'metadata' or
+            path[4] == 'metadata'
+        )
 

nbdime/webapp/nbdimeserver.py

@@ -313,6 +313,7 @@ def init_app(on_port=None, closable=False, **params):
     app = make_app(**params)
     if port != 0 or on_port is None:
         app.listen(port, address=ip)
+        _logger.info('Listening on %s, port %d' % (ip, port))
     else:
         sockets = netutil.bind_sockets(0, ip)
         server = httpserver.HTTPServer(app)