Merge pull request #98 from danvk/lazy-diff

Make diff generation lazy

Author: danvk

tests/pair_test.py

@@ -2,6 +2,8 @@
 
 from nose.tools import *
 from webdiff import util
+from webdiff import diff
+from webdiff import dirdiff
 
 def test_pairing():
     a_files = [
@@ -21,7 +23,7 @@ def test_pairing():
         'static/jsdifflib/diffview.js',
         'templates/heartbeat.html']
 
-    pairs = util.pair_files(a_files, b_files)
+    pairs = dirdiff.pair_files(a_files, b_files)
     pairs.sort()
 
     eq_(
@@ -34,31 +36,33 @@ def test_pairing():
          ('templates/heartbeat.html', 'templates/heartbeat.html'),
          ], pairs)
 
+
 def test_pairing_with_move():
     testdir = 'testdata/renamedfile'
-    diff = util.find_diff('%s/left/dir' % testdir, '%s/right/dir' % testdir)
-    eq_([{'a': 'file.json',
-          'a_path': 'testdata/renamedfile/left/dir/file.json',
-          'path': 'file.json',
+    diffs = dirdiff.diff('%s/left/dir' % testdir, '%s/right/dir' % testdir)
+    eq_([{
+          'a': 'file.json',
           'b': 'renamed.json',
-          'b_path': 'testdata/renamedfile/right/dir/renamed.json',
           'type': 'move',
-          'no_changes': True,
-          'idx': 0},
-         {'a': 'file.json',
-          'a_path': 'testdata/renamedfile/left/dir/file.json',
-          'path': 'file.json',
+         },
+         {
+          'a': 'file.json',
           'b': None,
-          'b_path': None,
           'type': 'delete',
-          'no_changes': False,
-          'idx': 1}], diff)
+         }], [diff.get_thin_dict(d) for d in diffs])
+
+
+class TinyDiff(object):
+    def __init__(self, a, b):
+        self.a_path = a
+        self.b_path = b
+
 
 def test_is_image_diff():
-    assert     util.is_image_diff({'a': 'foo.png', 'b': 'bar.png'})
-    assert not util.is_image_diff({'a': 'foo.png.gz', 'b': 'bar.png.gz'})
-    assert not util.is_image_diff({'a': 'foo.txt', 'b': 'bar.txt'})
-    assert     util.is_image_diff({'a': 'foo.png', 'b': None})
-    assert not util.is_image_diff({'a': 'foo.txt', 'b': None})
-    assert     util.is_image_diff({'a': None, 'b': 'foo.png'})
-    assert not util.is_image_diff({'a': None, 'b': 'foo.txt'})
+    assert     diff.is_image_diff(TinyDiff('foo.png', 'bar.png'))
+    assert not diff.is_image_diff(TinyDiff('foo.png.gz', 'bar.png.gz'))
+    assert not diff.is_image_diff(TinyDiff('foo.txt', 'bar.txt'))
+    assert     diff.is_image_diff(TinyDiff('foo.png', None))
+    assert not diff.is_image_diff(TinyDiff('foo.txt', None))
+    assert     diff.is_image_diff(TinyDiff(None, 'foo.png'))
+    assert not diff.is_image_diff(TinyDiff(None, 'foo.txt'))

tests/util_test.js

@@ -1,5 +1,9 @@
 QUnit.test('util.filePairDisplayName', function(assert) {
-  assert.deepEqual(filePairDisplayName({type: 'delete', path: 'dir/file.json'}),
+  assert.deepEqual(filePairDisplayName({
+                      type: 'delete',
+                      a: 'dir/file.json',
+                      b: null
+                   }),
                    'dir/file.json');
 
   var rename = function(a, b) {

webdiff/app.py

@@ -17,6 +17,7 @@
 from flask import (Flask, render_template, send_from_directory, send_file,
                    request, jsonify, Response)
 
+import diff
 import util
 import argparser
 
@@ -46,8 +47,6 @@ class Config:
 app.config.from_object(Config)
 app.config.from_envvar('WEBDIFF_CONFIG', silent=True)
 
-A_DIR = None
-B_DIR = None
 DIFF = None
 PORT = None
 
@@ -75,20 +74,31 @@ def update_last_request_ms():
     LAST_REQUEST_MS = time.time() * 1000
 
 
+def error(code, message):
+    e = {"code": code, "message": message}
+    response = jsonify(e)
+    response.status_code = 400
+    return response
+
+
 @app.route("/<side>/get_contents", methods=['POST'])
 def get_contents(side):
-    assert side in ('a', 'b')
+    if side not in ('a', 'b'):
+        return error('invalid side', 'Side must be "a" or "b", got %s' % side)
+
+    # TODO: switch to index? might be simpler
     path = request.form.get('path', '')
     if not path:
-        e = {"code": "incomplete",
-             "message": "Incomplete request (need path)"}
-        response = jsonify(e)
-        response.status_code = 400
-        return response
+        return error('incomplete', 'Incomplete request (need path)')
+
+    idx = diff.find_diff_index(DIFF, side, path)
+    if idx is None:
+        return error('not found', 'Invalid path on side %s: %s' % (side, path))
+
+    d = DIFF[idx]
+    abs_path = d.a_path if side == 'a' else d.b_path
 
     try:
-        abs_path = os.path.join(A_DIR if side == 'a' else B_DIR,
-                                os.path.normpath(path))  # / --> \ on windows
         is_binary = util.is_binary_file(abs_path)
         if is_binary:
             size = os.path.getsize(abs_path)
@@ -97,51 +107,43 @@ def get_contents(side):
             contents = open(abs_path).read()
         return Response(contents, mimetype='text/plain')
     except Exception:
-        e = {"code": "read-error",
-             "message": "Unable to read %s" % abs_path}
-        response = jsonify(e)
-        response.status_code = 400
-        return response
+        return error('read-error', 'Unable to read %s' % abs_path)
 
 
 @app.route("/<side>/image/<path:path>")
 def get_image(side, path):
-    assert side in ('a', 'b')
+    if side not in ('a', 'b'):
+        return error('invalid side', 'Side must be "a" or "b", got %s' % side)
+
+    # TODO: switch to index? might be simpler
     if not path:
-        e = {"code": "incomplete",
-             "message": "Incomplete request (need path)"}
-        response = jsonify(e)
-        response.status_code = 400
-        return response
+        return error('incomplete', 'Incomplete request (need path)')
 
     mime_type, enc = mimetypes.guess_type(path)
     if not mime_type or not mime_type.startswith('image/') or enc is not None:
-        e = {"code": "wrongtype",
-             "message": "Requested file of type (%s, %s) as image" % (
-                 mime_type, enc)}
-        response = jsonify(e)
-        response.status_code = 400
-        return response
+        return error('wrongtype', 'Requested file of type (%s, %s) as image' % (
+                 mime_type, enc))
+
+    idx = diff.find_diff_index(DIFF, side, path)
+    if idx is None:
+        return error('not found', 'Invalid path on side %s: %s' % (side, path))
+
+    d = DIFF[idx]
+    abs_path = d.a_path if side == 'a' else d.b_path
 
     try:
-        abs_path = os.path.join(A_DIR if side == 'a' else B_DIR,
-                                os.path.normpath(path))  # / --> \ on windows
         contents = open(abs_path).read()
         return Response(contents, mimetype=mime_type)
     except Exception:
-        e = {"code": "read-error",
-             "message": "Unable to read %s" % abs_path}
-        response = jsonify(e)
-        response.status_code = 400
-        return response
+        return error('read-error', 'Unable to read %s' % abs_path)
 
 
 @app.route("/pdiff/<int:idx>")
 def get_pdiff(idx):
     idx = int(idx)
-    pair = DIFF[idx]
+    d = DIFF[idx]
     try:
-        _, pdiff_image = util.generate_pdiff_image(pair['a_path'], pair['b_path'])
+        _, pdiff_image = util.generate_pdiff_image(d.a_path, d.b_path)
         dilated_image = util.generate_dilated_pdiff_image(pdiff_image)
     except util.ImageMagickNotAvailableError:
         return 'ImageMagick is not available', 501
@@ -153,9 +155,9 @@ def get_pdiff(idx):
 @app.route("/pdiffbbox/<int:idx>")
 def get_pdiff_bbox(idx):
     idx = int(idx)
-    pair = DIFF[idx]
+    d = DIFF[idx]
     try:
-        _, pdiff_image = util.generate_pdiff_image(pair['a_path'], pair['b_path'])
+        _, pdiff_image = util.generate_pdiff_image(d.a_path, d.b_path)
         bbox = util.get_pdiff_bbox(pdiff_image)
     except util.ImageMagickNotAvailableError:
         return 'ImageMagick is not available', 501
@@ -173,10 +175,17 @@ def index():
 @app.route("/<int:idx>")
 def file_diff(idx):
     idx = int(idx)
+    pairs = diff.get_thin_list(DIFF)
     return render_template('file_diff.html',
                            idx=idx,
                            has_magick=util.is_imagemagick_available(),
-                           pairs=DIFF)
+                           pairs=pairs)
+
+
+@app.route('/thick/<int:idx>')
+def thick_diff(idx):
+    idx = int(idx)
+    return jsonify(diff.get_thick_dict(DIFF[idx]))
 
 
 @app.route('/favicon.ico')
@@ -254,24 +263,23 @@ def is_webdiff_from_head():
 
 
 def run():
-    global A_DIR, B_DIR, DIFF, PORT
+    global DIFF, PORT
     try:
         parsed_args = argparser.parse(sys.argv[1:])
     except argparser.UsageError as e:
         sys.stderr.write('Error: %s\n\n' % e.message)
         usage_and_die()
 
-    A_DIR, B_DIR, DIFF = util.diff_for_args(parsed_args)
+    DIFF = argparser.diff_for_args(parsed_args)
 
     if app.config['TESTING'] or app.config['DEBUG']:
-        sys.stderr.write('Diffing:\nA: %s\nB: %s\n\n' % (A_DIR, B_DIR))
+        sys.stderr.write('Diff:\n%s' % DIFF)
 
     PORT = pick_a_port(parsed_args)
 
     sys.stderr.write('''Serving diffs on http://localhost:%s
 Close the browser tab or hit Ctrl-C when you're done.
 ''' % PORT)
-    logging.info('Diff: %r', DIFF)
     Timer(0.1, open_browser).start()
     app.run(host='0.0.0.0', port=PORT)
 

webdiff/argparser.py

@@ -3,7 +3,10 @@
 import os
 import re
 
+import dirdiff
+import githubdiff
 import github_fetcher
+from localfilediff import LocalFileDiff
 
 class UsageError(Exception):
     pass
@@ -72,3 +75,26 @@ def parse(args):
             out['files'] = (a, b)
 
     return out
+
+
+# TODO: move into dirdiff?
+def _shim_for_file_diff(a_file, b_file):
+    '''Sets A_DIR, B_DIR and DIFF to do a one-file diff.'''
+    dirname = os.path.dirname
+    basename = os.path.basename
+    return LocalFileDiff(dirname(a_file), basename(a_file),
+                         dirname(b_file), basename(b_file),
+                         False)  # probably not a move
+
+
+def diff_for_args(args):
+    '''Returns a list of Diff objects for parsed command line args.'''
+    if 'dirs' in args:
+        return dirdiff.diff(*args['dirs'])
+
+    if 'files' in args:
+        return [_shim_for_file_diff(*args['files'])]
+
+    if 'github' in args:
+        gh = args['github']
+        return githubdiff.fetch_pull_request(gh['owner'], gh['repo'], gh['num'])