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
1=== modified file 'bzrlib/diff.py'
2--- bzrlib/diff.py 2011-07-15 15:31:01 +0000
3+++ bzrlib/diff.py 2011-08-01 08:04:43 +0000
4@@ -671,8 +671,18 @@
5 try:
6 from_text = _get_text(self.old_tree, from_file_id, from_path)
7 to_text = _get_text(self.new_tree, to_file_id, to_path)
8- self.text_differ(from_label, from_text, to_label, to_text,
9- self.to_file, path_encoding=self.path_encoding)
10+ # if the user requested function names in the diff
11+ # we use the external diff
12+ diff_show_function_re = ()
13+ if from_path and self.text_differ == internal_diff:
14+ diff_show_function_re = self.old_tree.iter_search_rules(
15+ [from_path], ['diff_show_function_regex']).next()
16+ if diff_show_function_re != ():
17+ external_diff(from_label, from_text, to_label, to_text,
18+ self.to_file, ['-F', diff_show_function_re[0][1]])
19+ else:
20+ self.text_differ(from_label, from_text, to_label, to_text,
21+ self.to_file, path_encoding=self.path_encoding)
22 except errors.BinaryFile:
23 self.to_file.write(
24 ("Binary files %s and %s differ\n" %
25
26=== modified file 'bzrlib/tests/test_diff.py'
27--- bzrlib/tests/test_diff.py 2011-07-21 06:46:34 +0000
28+++ bzrlib/tests/test_diff.py 2011-08-01 08:04:43 +0000
29@@ -1495,3 +1495,69 @@
30 self.assertEqual(tree.branch.base, new_branch.base)
31 self.assertIs(None, specific_files)
32 self.assertEqual(tree.basedir, extra_trees[0].basedir)
33+<<<<<<< TREE
34+=======
35+
36+
37+class TestGetTreesAndBranchesToDiff(TestGetTreesAndBranchesToDiffLocked):
38+ """Apply the tests for get_trees_and_branches_to_diff_locked to the
39+ deprecated get_trees_and_branches_to_diff function.
40+ """
41+
42+ def call_gtabtd(self, path_list, revision_specs, old_url, new_url):
43+ return self.applyDeprecated(
44+ deprecated_in((2, 2, 0)), diff.get_trees_and_branches_to_diff,
45+ path_list, revision_specs, old_url, new_url)
46+
47+
48+class TestDiffShowFunction(tests.TestCaseWithTransport):
49+ """Test showing the function enclosing each diff hunk.
50+
51+ :see: https://bugs.launchpad.net/bzr/+bug/764108
52+ """
53+
54+ def test_diff_with_function_rule(self):
55+ tree = self.make_branch_and_tree('.')
56+ self.build_tree_contents([
57+ ('.bzrrules', '* diff_show_function_regexp=function\n'),
58+ ('hello', 'one\nfunction bee\ntwo\nthree\n')
59+ ])
60+ tree.add(['hello'])
61+ rev_id = tree.commit('test_commit')
62+ self.build_tree_contents([
63+ ('hello', 'one a\nfunction bee\ntwo\nthree\nfour\n')])
64+ diff_output = get_diff_as_string(tree.basis_tree(), tree, working_tree=tree)
65+ diff_lines = diff_output.split('\n')
66+ self.assertEqual(diff_lines[0],
67+ "=== modified file 'hello'")
68+ # lines 1 and 2 have timestamps, which we don't care about testing
69+ # here
70+ self.assertEqualDiff('\n'.join(diff_lines[4:]),
71+ '''\
72+-one
73++one a
74+ function bee
75+ two
76+ three
77++four
78+
79+''')
80+ self.expectFailure(
81+ "Can't get function names in diff from rules (bug 764108)",
82+ self.assertEqual, diff_lines[3],
83+ "@@ -1,4 +1,5 @@ function bee")
84+
85+
86+ # TODO: Given two trees, with rules configured, they show
87+ # a diff that includes those function lines.
88+
89+ # TODO: Using internal diff, and a function line, we show the right
90+ # prefixes.
91+ #
92+ # TODO: Using external diff, and a function regexp, we generate the right
93+ # external command lines.
94+
95+ # TODO: Test that in a tree with these rules on, we still don't include
96+ # these settings in a diff generated for a bundle? Or maybe we should
97+ # anyhow? It should be harmless and might be useful.
98+>>>>>>> MERGE-SOURCE