Bazaar

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

  1. 764108-diffoptions
  2. Merge into bzr.dev
Proposed by Martin Pool
Status: Work in progress
Proposed branch: lp:~mbp/bzr/764108-diffoptions
Merge into: lp:bzr
Diff against target: 98 lines (+78/-2) (has conflicts)
2 files modified
bzrlib/diff.py (+12/-2)
bzrlib/tests/test_diff.py (+66/-0)
Text conflict in bzrlib/tests/test_diff.py
To merge this branch: bzr merge lp:~mbp/bzr/764108-diffoptions
Reviewer Review Type Date Requested Status
John A Meinel Approve
Review via email: mp+69966@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Jelmer Vernooij (jelmer) 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.

Reply
Revision history for this message
John A Meinel (jameinel) wrote :

-----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
Reply
Revision history for this message
Martin Packman (gz) wrote :

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.

Reply
Revision history for this message
Martin Pool (mbp) wrote :

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

Martin

Reply

Unmerged revisions

5731. By Martin Pool

merge trunk, including test_diff cleanups

5730. By Martin Pool

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

5729. By Sergei Golubchik

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
671 try:671 try:
672 from_text = _get_text(self.old_tree, from_file_id, from_path)672 from_text = _get_text(self.old_tree, from_file_id, from_path)
673 to_text = _get_text(self.new_tree, to_file_id, to_path)673 to_text = _get_text(self.new_tree, to_file_id, to_path)
674 self.text_differ(from_label, from_text, to_label, to_text,674 # if the user requested function names in the diff
675 self.to_file, path_encoding=self.path_encoding)675 # we use the external diff
676 diff_show_function_re = ()
677 if from_path and self.text_differ == internal_diff:
678 diff_show_function_re = self.old_tree.iter_search_rules(
679 [from_path], ['diff_show_function_regex']).next()
680 if diff_show_function_re != ():
681 external_diff(from_label, from_text, to_label, to_text,
682 self.to_file, ['-F', diff_show_function_re[0][1]])
683 else:
684 self.text_differ(from_label, from_text, to_label, to_text,
685 self.to_file, path_encoding=self.path_encoding)
676 except errors.BinaryFile:686 except errors.BinaryFile:
677 self.to_file.write(687 self.to_file.write(
678 ("Binary files %s and %s differ\n" %688 ("Binary files %s and %s differ\n" %
679689
=== 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 @@
1495 self.assertEqual(tree.branch.base, new_branch.base)1495 self.assertEqual(tree.branch.base, new_branch.base)
1496 self.assertIs(None, specific_files)1496 self.assertIs(None, specific_files)
1497 self.assertEqual(tree.basedir, extra_trees[0].basedir)1497 self.assertEqual(tree.basedir, extra_trees[0].basedir)
1498<<<<<<< TREE
1499=======
1500
1501
1502class TestGetTreesAndBranchesToDiff(TestGetTreesAndBranchesToDiffLocked):
1503 """Apply the tests for get_trees_and_branches_to_diff_locked to the
1504 deprecated get_trees_and_branches_to_diff function.
1505 """
1506
1507 def call_gtabtd(self, path_list, revision_specs, old_url, new_url):
1508 return self.applyDeprecated(
1509 deprecated_in((2, 2, 0)), diff.get_trees_and_branches_to_diff,
1510 path_list, revision_specs, old_url, new_url)
1511
1512
1513class TestDiffShowFunction(tests.TestCaseWithTransport):
1514 """Test showing the function enclosing each diff hunk.
1515
1516 :see: https://bugs.launchpad.net/bzr/+bug/764108
1517 """
1518
1519 def test_diff_with_function_rule(self):
1520 tree = self.make_branch_and_tree('.')
1521 self.build_tree_contents([
1522 ('.bzrrules', '* diff_show_function_regexp=function\n'),
1523 ('hello', 'one\nfunction bee\ntwo\nthree\n')
1524 ])
1525 tree.add(['hello'])
1526 rev_id = tree.commit('test_commit')
1527 self.build_tree_contents([
1528 ('hello', 'one a\nfunction bee\ntwo\nthree\nfour\n')])
1529 diff_output = get_diff_as_string(tree.basis_tree(), tree, working_tree=tree)
1530 diff_lines = diff_output.split('\n')
1531 self.assertEqual(diff_lines[0],
1532 "=== modified file 'hello'")
1533 # lines 1 and 2 have timestamps, which we don't care about testing
1534 # here
1535 self.assertEqualDiff('\n'.join(diff_lines[4:]),
1536 '''\
1537-one
1538+one a
1539 function bee
1540 two
1541 three
1542+four
1543
1544''')
1545 self.expectFailure(
1546 "Can't get function names in diff from rules (bug 764108)",
1547 self.assertEqual, diff_lines[3],
1548 "@@ -1,4 +1,5 @@ function bee")
1549
1550
1551 # TODO: Given two trees, with rules configured, they show
1552 # a diff that includes those function lines.
1553
1554 # TODO: Using internal diff, and a function line, we show the right
1555 # prefixes.
1556 #
1557 # TODO: Using external diff, and a function regexp, we generate the right
1558 # external command lines.
1559
1560 # TODO: Test that in a tree with these rules on, we still don't include
1561 # these settings in a diff generated for a bundle? Or maybe we should
1562 # anyhow? It should be harmless and might be useful.
1563>>>>>>> MERGE-SOURCE