Merge lp:~richard-wilbur/bzr/update-diff-format-parser into lp:bzr

Proposed by Richard Wilbur on 2014-09-16
Status: Merged
Approved by: Richard Wilbur on 2014-09-22
Approved revision: 6598
Merged at revision: 6600
Proposed branch: lp:~richard-wilbur/bzr/update-diff-format-parser
Merge into: lp:bzr
Diff against target: 144 lines (+54/-26)
2 files modified
bzrlib/diff.py (+39/-24)
bzrlib/tests/test_diff.py (+15/-2)
To merge this branch: bzr merge lp:~richard-wilbur/bzr/update-diff-format-parser
Reviewer Review Type Date Requested Status
Richard Wilbur Approve on 2014-09-22
Review via email: mp+234855@code.launchpad.net

Commit message

Split diff format option parser into a separate function, update to include all format options for GNU diff v3.2, and test parser. Fixes lp:1370435

Description of the change

Split diff format option parser into a separate function for testability, update to include all format options for GNU diff v3.2, and test parser.

This is needed because we default to 'unified' diff format if no other format is specified, but the parser didn't recognize several valid format specifiers. If we add '-u' (unified diff format) to a set of options already containing a valid format specifier, diff rightfully complains.

Also fixed a diagnostic message, an exception handler, some spelling and latest copyright dates.

Fixes lp:1370435

To post a comment you must log in.
Richard Wilbur (richard-wilbur) wrote :

The changes are relatively trivial.

Can't reproduce the bug with these changes.

Passes the test suite.

+1

review: Approve
Richard Wilbur (richard-wilbur) wrote :

sent to pqm by email

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 2012-07-24 16:56:07 +0000
3+++ bzrlib/diff.py 2014-09-16 17:36:26 +0000
4@@ -1,4 +1,4 @@
5-# Copyright (C) 2005-2011 Canonical Ltd.
6+# Copyright (C) 2005-2014 Canonical Ltd.
7 #
8 # This program is free software; you can redistribute it and/or modify
9 # it under the terms of the GNU General Public License as published by
10@@ -119,7 +119,7 @@
11
12
13 def _spawn_external_diff(diffcmd, capture_errors=True):
14- """Spawn the externall diff process, and return the child handle.
15+ """Spawn the external diff process, and return the child handle.
16
17 :param diffcmd: The command list to spawn
18 :param capture_errors: Capture stderr as well as setting LANG=C
19@@ -154,6 +154,40 @@
20
21 return pipe
22
23+# diff style options as of GNU diff v3.2
24+style_option_list = ['-c', '-C', '--context',
25+ '-e', '--ed',
26+ '-f', '--forward-ed',
27+ '-q', '--brief',
28+ '--normal',
29+ '-n', '--rcs',
30+ '-u', '-U', '--unified',
31+ '-y', '--side-by-side',
32+ '-D', '--ifdef']
33+
34+def default_style_unified(diff_opts):
35+ """Default to unified diff style if alternative not specified in diff_opts.
36+
37+ diff only allows one style to be specified; they don't override.
38+ Note that some of these take optargs, and the optargs can be
39+ directly appended to the options.
40+ This is only an approximate parser; it doesn't properly understand
41+ the grammar.
42+
43+ :param diff_opts: List of options for external (GNU) diff.
44+ :return: List of options with default style=='unified'.
45+ """
46+ for s in style_option_list:
47+ for j in diff_opts:
48+ if j.startswith(s):
49+ break
50+ else:
51+ continue
52+ break
53+ else:
54+ diff_opts.append('-u')
55+ return diff_opts
56+
57
58 def external_diff(old_filename, oldlines, new_filename, newlines, to_file,
59 diff_opts):
60@@ -195,26 +229,7 @@
61 '--binary',
62 ]
63
64- # diff only allows one style to be specified; they don't override.
65- # note that some of these take optargs, and the optargs can be
66- # directly appended to the options.
67- # this is only an approximate parser; it doesn't properly understand
68- # the grammar.
69- for s in ['-c', '-u', '-C', '-U',
70- '-e', '--ed',
71- '-q', '--brief',
72- '--normal',
73- '-n', '--rcs',
74- '-y', '--side-by-side',
75- '-D', '--ifdef']:
76- for j in diff_opts:
77- if j.startswith(s):
78- break
79- else:
80- continue
81- break
82- else:
83- diffcmd.append('-u')
84+ diff_opts = default_style_unified(diff_opts)
85
86 if diff_opts:
87 diffcmd.extend(diff_opts)
88@@ -265,7 +280,7 @@
89 msg = 'exit code %d' % rc
90
91 raise errors.BzrError('external diff failed with %s; command: %r'
92- % (rc, diffcmd))
93+ % (msg, diffcmd))
94
95
96 finally:
97@@ -282,7 +297,7 @@
98 old_abspath, e)
99 try:
100 os.remove(new_abspath)
101- except OSError:
102+ except OSError, e:
103 if e.errno not in (errno.ENOENT,):
104 warning('Failed to delete temporary file: %s %s',
105 new_abspath, e)
106
107=== modified file 'bzrlib/tests/test_diff.py'
108--- bzrlib/tests/test_diff.py 2012-07-12 15:59:35 +0000
109+++ bzrlib/tests/test_diff.py 2014-09-16 17:36:26 +0000
110@@ -1,4 +1,4 @@
111-# Copyright (C) 2005-2011 Canonical Ltd
112+# Copyright (C) 2005-2014 Canonical Ltd
113 #
114 # This program is free software; you can redistribute it and/or modify
115 # it under the terms of the GNU General Public License as published by
116@@ -143,6 +143,19 @@
117 'old', ['boo\n'], 'new', ['goo\n'],
118 StringIO(), diff_opts=['-u'])
119
120+ def test_default_style_unified(self):
121+ """Check for default style '-u' only if no other style specified
122+ in 'diff-options'.
123+ """
124+ # Verify that style defaults to unified, id est '-u' appended
125+ # to option list, in the absence of an alternative style.
126+ self.assertEqual(['-a', '-u'], diff.default_style_unified(["-a"]))
127+ # Verify that for all valid style options, '-u' is not
128+ # appended to option list.
129+ for s in diff.style_option_list:
130+ ret_opts = diff.default_style_unified(diff_opts=["%s" % (s,)])
131+ self.assertEqual(["%s" % (s,)], ret_opts)
132+
133 def test_internal_diff_default(self):
134 # Default internal diff encoding is utf8
135 output = StringIO()
136@@ -1391,7 +1404,7 @@
137 diff_obj._execute('old', 'new')
138 self.assertEqual(output.getvalue().rstrip(), 'old new')
139
140- def test_excute_missing(self):
141+ def test_execute_missing(self):
142 diff_obj = diff.DiffFromTool(['a-tool-which-is-unlikely-to-exist'],
143 None, None, None)
144 self.addCleanup(diff_obj.finish)