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

[Paul Nixon]

Add option to specify how much context bzr should use in diffs

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"

As per requests made of the previous merge proposal, I have changed the defaults to be constant, added tests, and updated the release notes.

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

[Jelmer Vernooij (jelmer)]

Hi Paul,

Thanks for the changes; it would still be nice to have a constant for the default number of context lines, rather than having that constant duplicated in various places in the code.

[Paul Nixon (pauljnixon)]

Hello Jelmer:

The first time you asked, I didn't understand what you meant, and thought you wanted it to always be 3 instead of being 3 in some places and 5 in one (something I had done for testing and forgot to take out). Now it's defined as default_context_amount at the top of bzrlib/diff.py. Is that the right place for it?

Thanks,
---Paul

> Hi Paul,
>
> Thanks for the changes; it would still be nice to have a constant for the
> default number of context lines, rather than having that constant duplicated
> in various places in the code.

[Jelmer Vernooij (jelmer)]

> Hello Jelmer:
>
> The first time you asked, I didn't understand what you meant, and thought you
> wanted it to always be 3 instead of being 3 in some places and 5 in one
> (something I had done for testing and forgot to take out). Now it's defined
> as default_context_amount at the top of bzrlib/diff.py. Is that the right
> place for it?
Yep, that seems reasonable. It would be nice though to have its name in all-caps, consistent with the other constant names.

Cheers,

Jelmer

[Paul Nixon (pauljnixon)]

Hello Jelmer:

The change you requested has been made. Should I be re-proposing for merging when I update the branch?

Thank you,
---Paul

[Jelmer Vernooij (jelmer)]

Thanks! Looks good to me now.

review: Approve

[Martin Packman (gz)]

I'd like -C as a short option, which might be a bit confusing as this is really more like -U, but that shouldn't hold this branch up from landing.

review: Approve

[Martin Packman (gz)]

Can you execute the contributor agreement? It can be done online without too much fuss:

<http://www.canonical.com/contributors>

[Paul Nixon (pauljnixon)]

Hello Martin:

I submitted an agreement.

Thank you,
---Paul

On Sat, Jul 28, 2012 at 8:56 AM, Martin Packman
<email address hidden> wrote:
> Can you execute the contributor agreement? It can be done online without too much fuss:
>
> <http://www.canonical.com/contributors>
> --
> https://code.launchpad.net/~pauljnixon/bzr/context_in_diffs/+merge/115819
> You are the owner of lp:~pauljnixon/bzr/context_in_diffs.

[Martin Packman (gz)]

Thanks Paul!

[Martin Packman (gz)]

sent to pqm by email

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2012-07-23 17:56:45 +0000
+++ bzrlib/builtins.py	2012-07-24 17:00:28 +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-24 17:00:28 +0000
@@ -49,6 +49,7 @@
     )
 from bzrlib.trace import mutter, note, warning
 
+DEFAULT_CONTEXT_AMOUNT = 3
 
 class AtTemplate(string.Template):
     """Templating class that uses @ instead of $."""
@@ -72,7 +73,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=DEFAULT_CONTEXT_AMOUNT):
     # 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 +99,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 +427,8 @@
                     extra_trees=None,
                     path_encoding='utf8',
                     using=None,
-                    format_cls=None):
+                    format_cls=None,
+                    context=DEFAULT_CONTEXT_AMOUNT):
     """Show in text form the changes from one tree to another.
 
     :param to_file: The output stream.
@@ -439,6 +441,8 @@
         otherwise is supposed to be utf8
     :param format_cls: Formatter class (DiffTree subclass)
     """
+    if context is None:
+        context = DEFAULT_CONTEXT_AMOUNT
     if format_cls is None:
         format_cls = DiffTree
     old_tree.lock_read()
@@ -451,7 +455,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 +620,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=DEFAULT_CONTEXT_AMOUNT):
         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 +682,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 +913,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 +934,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 +942,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-24 17:00:28 +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-24 12:53:36 +0000
+++ doc/en/release-notes/bzr-2.6.txt	2012-07-24 17:00:28 +0000
@@ -21,6 +21,10 @@
 * ``bzr switch --store`` now stores uncommitted changes in the branch, and
   restores them when switching back to the branch. (Aaron Bentley)
 
+* 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
 ************