Merge lp:~pauljnixon/bzr/context_in_diffs into lp:bzr

[Paul Nixon]

Patiencediff (used by default for "bzr diff" command) includes the ability to control how many lines of context to output for each changed section. I made this accessible as an option to bzr diff, as "--context" so an example would be "bzr diff -r-2 --context 6"

I have also made this accessible to Loggerhead which I will propose for merging if this goes in.

This is the first time I have proposed anything for merging, if I'm doing something wrong just let me know and I'll fix it.

[Jelmer Vernooij (jelmer)]

Hi Paul,

The changes don't look unreasonable. At a first glance, two notes: it would be nice to have a constant for the default number of context lines. Can you please add some basic tests for the new option?

Cheers,

Jelmer
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Paul <email address hidden> wrote:

Paul has proposed merging lp:~pauljnixon/bzr/context_in_diffs into lp:bzr.

Requested reviews:
bzr-core (bzr-core)

For more details, see:
https://code.launchpad.net/~pauljnixon/bzr/context_in_diffs/+merge/113793

Patiencediff (used by default for "bzr diff" command) includes the ability to control how many lines of context to output for each changed section. I made this accessible as an option to bzr diff, as "--context" so an example would be "bzr diff -r-2 --context 6"

I have also made this accessible to Loggerhead which I will propose for merging if this goes in.

This is the first time I have proposed anything for merging, if I'm doing something wrong just let me know and I'll fix it.
--
https://code.launchpad.net/~pauljnixon/bzr/context_in_diffs/+merge/113793
Your team Bazaar Codereview Subscribers is subscribed to branch lp:bzr.

[Paul Nixon (pauljnixon)]

Okay, I'll make those changes.

On Fri, Jul 6, 2012 at 6:28 PM, Jelmer Vernooij
<email address hidden> wrote:
> Hi Paul,
>
> The changes don't look unreasonable. At a first glance, two notes: it would be nice to have a constant for the default number of context lines. Can you please add some basic tests for the new option?
>
> Cheers,
>
> Jelmer
> --
> Sent from my Android phone with K-9 Mail. Please excuse my brevity.
>
> Paul <email address hidden> wrote:
>
> Paul has proposed merging lp:~pauljnixon/bzr/context_in_diffs into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> For more details, see:
> https://code.launchpad.net/~pauljnixon/bzr/context_in_diffs/+merge/113793
>
> Patiencediff (used by default for "bzr diff" command) includes the ability to control how many lines of context to output for each changed section. I made this accessible as an option to bzr diff, as "--context" so an example would be "bzr diff -r-2 --context 6"
>
> I have also made this accessible to Loggerhead which I will propose for merging if this goes in.
>
> This is the first time I have proposed anything for merging, if I'm doing something wrong just let me know and I'll fix it.
> --
> https://code.launchpad.net/~pauljnixon/bzr/context_in_diffs/+merge/113793
> Your team Bazaar Codereview Subscribers is subscribed to branch lp:bzr.
>
>
> https://code.launchpad.net/~pauljnixon/bzr/context_in_diffs/+merge/113793
> You are the owner of lp:~pauljnixon/bzr/context_in_diffs.

[Gordon Tyler (doxxx)]

I always forget this myself: add an entry to doc/en/release-notes/bzr-2.6.txt describing your change.

[Paul Nixon (pauljnixon)]

Jelmer, Gordon:

Thank you both for your advice.
I have added tests, changed the defaults to be consistent (3, the
default for patiencediff), and edited the release notes.
Is there anything else I should do?

Thank you,
---Paul

On Sun, Jul 8, 2012 at 7:04 PM, Gordon Tyler <email address hidden> wrote:
> I always forget this myself: add an entry to doc/en/release-notes/bzr-2.6.txt describing your change.
> --
> https://code.launchpad.net/~pauljnixon/bzr/context_in_diffs/+merge/113793

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2012-06-18 11:43:07 +0000
+++ bzrlib/builtins.py	2012-07-12 17:33:26 +0000
@@ -2335,13 +2335,18 @@
             help='Diff format to use.',
             lazy_registry=('bzrlib.diff', 'format_registry'),
             title='Diff format'),
+        Option('context',
+            help='How many lines of context to show.',
+            type=int,
+            ),
         ]
     aliases = ['di', 'dif']
     encoding_type = 'exact'
 
     @display_command
     def run(self, revision=None, file_list=None, diff_options=None,
-            prefix=None, old=None, new=None, using=None, format=None):
+            prefix=None, old=None, new=None, using=None, format=None, 
+            context=None):
         from bzrlib.diff import (get_trees_and_branches_to_diff_locked,
             show_diff_trees)
 
@@ -2380,7 +2385,7 @@
                                old_label=old_label, new_label=new_label,
                                extra_trees=extra_trees,
                                path_encoding=path_encoding,
-                               using=using,
+                               using=using, context=context,
                                format_cls=format)
 
 
 
=== modified file 'bzrlib/diff.py'
--- bzrlib/diff.py	2011-12-18 12:46:49 +0000
+++ bzrlib/diff.py	2012-07-12 17:33:26 +0000
@@ -72,7 +72,7 @@
 
 def internal_diff(old_filename, oldlines, new_filename, newlines, to_file,
                   allow_binary=False, sequence_matcher=None,
-                  path_encoding='utf8'):
+                  path_encoding='utf8', context_lines=3):
     # FIXME: difflib is wrong if there is no trailing newline.
     # The syntax used by patch seems to be "\ No newline at
     # end of file" following the last diff line from that
@@ -98,7 +98,7 @@
     ud = patiencediff.unified_diff(oldlines, newlines,
                       fromfile=old_filename.encode(path_encoding, 'replace'),
                       tofile=new_filename.encode(path_encoding, 'replace'),
-                      sequencematcher=sequence_matcher)
+                      n=context_lines, sequencematcher=sequence_matcher)
 
     ud = list(ud)
     if len(ud) == 0: # Identical contents, nothing to do
@@ -426,7 +426,8 @@
                     extra_trees=None,
                     path_encoding='utf8',
                     using=None,
-                    format_cls=None):
+                    format_cls=None,
+                    context=3):
     """Show in text form the changes from one tree to another.
 
     :param to_file: The output stream.
@@ -439,6 +440,8 @@
         otherwise is supposed to be utf8
     :param format_cls: Formatter class (DiffTree subclass)
     """
+    if context is None:
+        context = 3
     if format_cls is None:
         format_cls = DiffTree
     old_tree.lock_read()
@@ -451,7 +454,8 @@
             differ = format_cls.from_trees_options(old_tree, new_tree, to_file,
                                                    path_encoding,
                                                    external_diff_options,
-                                                   old_label, new_label, using)
+                                                   old_label, new_label, using,
+                                                   context_lines=context)
             return differ.show_diff(specific_files, extra_trees)
         finally:
             new_tree.unlock()
@@ -615,13 +619,15 @@
     # or removed in a diff.
     EPOCH_DATE = '1970-01-01 00:00:00 +0000'
 
-    def __init__(self, old_tree, new_tree, to_file, path_encoding='utf-8',
-                 old_label='', new_label='', text_differ=internal_diff):
+    def __init__(self, old_tree, new_tree, to_file, path_encoding='utf-8', 
+                 old_label='', new_label='', text_differ=internal_diff, 
+                 context_lines=3):
         DiffPath.__init__(self, old_tree, new_tree, to_file, path_encoding)
         self.text_differ = text_differ
         self.old_label = old_label
         self.new_label = new_label
         self.path_encoding = path_encoding
+        self.context_lines = context_lines
 
     def diff(self, file_id, old_path, new_path, old_kind, new_kind):
         """Compare two files in unified diff format
@@ -675,7 +681,8 @@
             from_text = _get_text(self.old_tree, from_file_id, from_path)
             to_text = _get_text(self.new_tree, to_file_id, to_path)
             self.text_differ(from_label, from_text, to_label, to_text,
-                             self.to_file, path_encoding=self.path_encoding)
+                             self.to_file, path_encoding=self.path_encoding,
+                             context_lines=self.context_lines)
         except errors.BinaryFile:
             self.to_file.write(
                   ("Binary files %s and %s differ\n" %
@@ -905,7 +912,7 @@
     @classmethod
     def from_trees_options(klass, old_tree, new_tree, to_file,
                            path_encoding, external_diff_options, old_label,
-                           new_label, using):
+                           new_label, using, context_lines):
         """Factory for producing a DiffTree.
 
         Designed to accept options used by show_diff_trees.
@@ -926,7 +933,7 @@
             extra_factories = []
         if external_diff_options:
             opts = external_diff_options.split()
-            def diff_file(olab, olines, nlab, nlines, to_file, path_encoding=None):
+            def diff_file(olab, olines, nlab, nlines, to_file, path_encoding=None, context_lines=None):
                 """:param path_encoding: not used but required
                         to match the signature of internal_diff.
                 """
@@ -934,7 +941,7 @@
         else:
             diff_file = internal_diff
         diff_text = DiffText(old_tree, new_tree, to_file, path_encoding,
-                             old_label, new_label, diff_file)
+                             old_label, new_label, diff_file, context_lines=context_lines)
         return klass(old_tree, new_tree, to_file, path_encoding, diff_text,
                      extra_factories)
 
 
=== modified file 'bzrlib/tests/test_diff.py'
--- bzrlib/tests/test_diff.py	2012-01-25 21:13:15 +0000
+++ bzrlib/tests/test_diff.py	2012-07-12 17:33:26 +0000
@@ -211,6 +211,69 @@
         self.assertIsInstance(output.getvalue(), str,
             'internal_diff should return bytestrings')
 
+    def test_internal_diff_default_context(self):
+        output = StringIO()
+        diff.internal_diff('old', ['same_text\n','same_text\n','same_text\n',
+                           'same_text\n','same_text\n','old_text\n'],
+                           'new', ['same_text\n','same_text\n','same_text\n',
+                           'same_text\n','same_text\n','new_text\n'], output)
+        lines = output.getvalue().splitlines(True)
+        self.check_patch(lines)
+        self.assertEquals(['--- old\n',
+                           '+++ new\n',
+                           '@@ -3,4 +3,4 @@\n',
+                           ' same_text\n',
+                           ' same_text\n',
+                           ' same_text\n',
+                           '-old_text\n',
+                           '+new_text\n',
+                           '\n',
+                          ]
+                          , lines)
+
+    def test_internal_diff_no_context(self):
+        output = StringIO()
+        diff.internal_diff('old', ['same_text\n','same_text\n','same_text\n',
+                           'same_text\n','same_text\n','old_text\n'],
+                           'new', ['same_text\n','same_text\n','same_text\n',
+                           'same_text\n','same_text\n','new_text\n'], output,
+                           context_lines=0)
+        lines = output.getvalue().splitlines(True)
+        self.check_patch(lines)
+        self.assertEquals(['--- old\n',
+                           '+++ new\n',
+                           '@@ -6,1 +6,1 @@\n',
+                           '-old_text\n',
+                           '+new_text\n',
+                           '\n',
+                          ]
+                          , lines)
+
+    def test_internal_diff_more_context(self):
+        output = StringIO()
+        diff.internal_diff('old', ['same_text\n','same_text\n','same_text\n',
+                           'same_text\n','same_text\n','old_text\n'],
+                           'new', ['same_text\n','same_text\n','same_text\n',
+                           'same_text\n','same_text\n','new_text\n'], output,
+                           context_lines=4)
+        lines = output.getvalue().splitlines(True)
+        self.check_patch(lines)
+        self.assertEquals(['--- old\n',
+                           '+++ new\n',
+                           '@@ -2,5 +2,5 @@\n',
+                           ' same_text\n',
+                           ' same_text\n',
+                           ' same_text\n',
+                           ' same_text\n',
+                           '-old_text\n',
+                           '+new_text\n',
+                           '\n',
+                          ]
+                          , lines)
+
+
+
+
 
 class TestDiffFiles(tests.TestCaseInTempDir):
 
 
=== modified file 'doc/en/release-notes/bzr-2.6.txt'
--- doc/en/release-notes/bzr-2.6.txt	2012-07-10 12:19:23 +0000
+++ doc/en/release-notes/bzr-2.6.txt	2012-07-12 17:33:26 +0000
@@ -20,6 +20,10 @@
 
 .. New commands, options, etc that users may wish to try out.
 
+* New option '--context' for 'bzr diff' command, to configure the amount of 
+  context (i.e. showing lines that have not changed).  Also available as the 
+  named parameter 'context_lines' to bzrlib.diff.internal_diff(). (Paul Nixon)
+
 Improvements
 ************