Merge lp:~mbp/bzr/764108-diffoptions into lp:bzr

[Martin Pool]

This builds on sergei's work in https://bugs.launchpad.net/bzr/+bug/764108 to add an option to show function names within the diff, based on <https://code.launchpad.net/~maria-captains/bzr/diffoptions/+merge/54139>.

I was going to finish it off for him. I haven't looked at it for a while, but it's still assigned to me so I'm putting this up to get it back in circulation. There are a lot of things we possibly could do here, but perhaps we can look for a smaller incremental step.

[Jelmer Vernooij (jelmer)]

This looks good to me.

I wonder what happens if there is no external diff tool that we can use. Does external_diff() fail gracefully?

Similarly, not all external diff tools might display the function name.

We should probably also protect the test with a feature that checks if a functional external diff is available.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 8/2/2011 1:56 AM, Jelmer Vernooij wrote:
> This looks good to me.
>
> I wonder what happens if there is no external diff tool that we can
> use. Does external_diff() fail gracefully?
>
> Similarly, not all external diff tools might display the function
> name.
>
> We should probably also protect the test with a feature that checks
> if a functional external diff is available.

External diff will fail noisily. However:

 merge: approve

There is a text conflict in here that will need to be fixed before it
can land.

Martin, I thought you had created a helper for "iter_search_rules()"
that returned the 1 entry that you were most likely to want. That might
cleanup the check a bit.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk47wLQACgkQJdeBCYSNAAP3NgCffvb9PMZNes2HaT+qhYmd8YTk
Y8cAn0od51oMcxsfx1Wcn4VrGpZb13N2
=N00d
-----END PGP SIGNATURE-----

review: Approve

[Martin Packman (gz)]

As mentioned in the previous merge proposal, I'm concerned the fix is worse than the problem.

It's not very hard to do `bzr alias fdiff=diff --diff-options=-p` or similar. Throwing funny errors from a useful looking option if there isn't a correctly configured external tool that the installers don't bundle is asking for trouble.

[Martin Pool (mbp)]

OK; please leave this unlanded and I'll look into that.

Martin

Unmerged revisions

5731. By Martin Pool on 2011-04-18

merge trunk, including test_diff cleanups

5730. By Martin Pool on 2011-04-18

Add xfail test for showing function name in diff hunk (bug 764108)

5729. By Sergei Golubchik on 2011-03-20

support diff -F in all generated diffs
with per-file configuration rules

=== modified file 'bzrlib/diff.py'
--- bzrlib/diff.py	2011-07-15 15:31:01 +0000
+++ bzrlib/diff.py	2011-08-01 08:04:43 +0000
@@ -671,8 +671,18 @@
         try:
             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)
+            # if the user requested function names in the diff
+            # we use the external diff
+            diff_show_function_re = ()
+            if from_path and self.text_differ == internal_diff:
+                diff_show_function_re = self.old_tree.iter_search_rules(
+                    [from_path], ['diff_show_function_regex']).next()
+            if diff_show_function_re != ():
+                external_diff(from_label, from_text, to_label, to_text,
+                              self.to_file, ['-F', diff_show_function_re[0][1]])
+            else:
+                self.text_differ(from_label, from_text, to_label, to_text,
+                                 self.to_file, path_encoding=self.path_encoding)
         except errors.BinaryFile:
             self.to_file.write(
                   ("Binary files %s and %s differ\n" %

=== modified file 'bzrlib/tests/test_diff.py'
--- bzrlib/tests/test_diff.py	2011-07-21 06:46:34 +0000
+++ bzrlib/tests/test_diff.py	2011-08-01 08:04:43 +0000
@@ -1495,3 +1495,69 @@
         self.assertEqual(tree.branch.base, new_branch.base)
         self.assertIs(None, specific_files)
         self.assertEqual(tree.basedir, extra_trees[0].basedir)
+<<<<<<< TREE
+=======
+
+
+class TestGetTreesAndBranchesToDiff(TestGetTreesAndBranchesToDiffLocked):
+    """Apply the tests for get_trees_and_branches_to_diff_locked to the
+    deprecated get_trees_and_branches_to_diff function.
+    """
+
+    def call_gtabtd(self, path_list, revision_specs, old_url, new_url):
+        return self.applyDeprecated(
+            deprecated_in((2, 2, 0)), diff.get_trees_and_branches_to_diff,
+            path_list, revision_specs, old_url, new_url)
+
+
+class TestDiffShowFunction(tests.TestCaseWithTransport):
+    """Test showing the function enclosing each diff hunk.
+    
+    :see: https://bugs.launchpad.net/bzr/+bug/764108
+    """
+
+    def test_diff_with_function_rule(self):
+        tree = self.make_branch_and_tree('.')
+        self.build_tree_contents([
+            ('.bzrrules', '*   diff_show_function_regexp=function\n'),
+            ('hello', 'one\nfunction bee\ntwo\nthree\n')
+            ])
+        tree.add(['hello'])
+        rev_id = tree.commit('test_commit')
+        self.build_tree_contents([
+            ('hello', 'one a\nfunction bee\ntwo\nthree\nfour\n')])
+        diff_output = get_diff_as_string(tree.basis_tree(), tree, working_tree=tree)
+        diff_lines = diff_output.split('\n')
+        self.assertEqual(diff_lines[0], 
+            "=== modified file 'hello'")
+        # lines 1 and 2 have timestamps, which we don't care about testing
+        # here
+        self.assertEqualDiff('\n'.join(diff_lines[4:]),
+            '''\
+-one
++one a
+ function bee
+ two
+ three
++four
+
+''')
+        self.expectFailure(
+            "Can't get function names in diff from rules (bug 764108)",
+            self.assertEqual, diff_lines[3], 
+            "@@ -1,4 +1,5 @@ function bee")
+        
+
+    # TODO: Given two trees, with rules configured, they show 
+    # a diff that includes those function lines.
+    
+    # TODO: Using internal diff, and a function line, we show the right
+    # prefixes.
+    #
+    # TODO: Using external diff, and a function regexp, we generate the right
+    # external command lines.
+
+    # TODO: Test that in a tree with these rules on, we still don't include
+    # these settings in a diff generated for a bundle?  Or maybe we should
+    # anyhow?  It should be harmless and might be useful.
+>>>>>>> MERGE-SOURCE