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
=== modified file 'bzrlib/diff.py'
--- bzrlib/diff.py 2012-07-24 16:56:07 +0000
+++ bzrlib/diff.py 2014-09-16 17:36:26 +0000
@@ -1,4 +1,4 @@
1# Copyright (C) 2005-2011 Canonical Ltd.1# Copyright (C) 2005-2014 Canonical Ltd.
2#2#
3# This program is free software; you can redistribute it and/or modify3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by4# it under the terms of the GNU General Public License as published by
@@ -119,7 +119,7 @@
119119
120120
121def _spawn_external_diff(diffcmd, capture_errors=True):121def _spawn_external_diff(diffcmd, capture_errors=True):
122 """Spawn the externall diff process, and return the child handle.122 """Spawn the external diff process, and return the child handle.
123123
124 :param diffcmd: The command list to spawn124 :param diffcmd: The command list to spawn
125 :param capture_errors: Capture stderr as well as setting LANG=C125 :param capture_errors: Capture stderr as well as setting LANG=C
@@ -154,6 +154,40 @@
154154
155 return pipe155 return pipe
156156
157# diff style options as of GNU diff v3.2
158style_option_list = ['-c', '-C', '--context',
159 '-e', '--ed',
160 '-f', '--forward-ed',
161 '-q', '--brief',
162 '--normal',
163 '-n', '--rcs',
164 '-u', '-U', '--unified',
165 '-y', '--side-by-side',
166 '-D', '--ifdef']
167
168def default_style_unified(diff_opts):
169 """Default to unified diff style if alternative not specified in diff_opts.
170
171 diff only allows one style to be specified; they don't override.
172 Note that some of these take optargs, and the optargs can be
173 directly appended to the options.
174 This is only an approximate parser; it doesn't properly understand
175 the grammar.
176
177 :param diff_opts: List of options for external (GNU) diff.
178 :return: List of options with default style=='unified'.
179 """
180 for s in style_option_list:
181 for j in diff_opts:
182 if j.startswith(s):
183 break
184 else:
185 continue
186 break
187 else:
188 diff_opts.append('-u')
189 return diff_opts
190
157191
158def external_diff(old_filename, oldlines, new_filename, newlines, to_file,192def external_diff(old_filename, oldlines, new_filename, newlines, to_file,
159 diff_opts):193 diff_opts):
@@ -195,26 +229,7 @@
195 '--binary',229 '--binary',
196 ]230 ]
197231
198 # diff only allows one style to be specified; they don't override.232 diff_opts = default_style_unified(diff_opts)
199 # note that some of these take optargs, and the optargs can be
200 # directly appended to the options.
201 # this is only an approximate parser; it doesn't properly understand
202 # the grammar.
203 for s in ['-c', '-u', '-C', '-U',
204 '-e', '--ed',
205 '-q', '--brief',
206 '--normal',
207 '-n', '--rcs',
208 '-y', '--side-by-side',
209 '-D', '--ifdef']:
210 for j in diff_opts:
211 if j.startswith(s):
212 break
213 else:
214 continue
215 break
216 else:
217 diffcmd.append('-u')
218233
219 if diff_opts:234 if diff_opts:
220 diffcmd.extend(diff_opts)235 diffcmd.extend(diff_opts)
@@ -265,7 +280,7 @@
265 msg = 'exit code %d' % rc280 msg = 'exit code %d' % rc
266281
267 raise errors.BzrError('external diff failed with %s; command: %r'282 raise errors.BzrError('external diff failed with %s; command: %r'
268 % (rc, diffcmd))283 % (msg, diffcmd))
269284
270285
271 finally:286 finally:
@@ -282,7 +297,7 @@
282 old_abspath, e)297 old_abspath, e)
283 try:298 try:
284 os.remove(new_abspath)299 os.remove(new_abspath)
285 except OSError:300 except OSError, e:
286 if e.errno not in (errno.ENOENT,):301 if e.errno not in (errno.ENOENT,):
287 warning('Failed to delete temporary file: %s %s',302 warning('Failed to delete temporary file: %s %s',
288 new_abspath, e)303 new_abspath, e)
289304
=== modified file 'bzrlib/tests/test_diff.py'
--- bzrlib/tests/test_diff.py 2012-07-12 15:59:35 +0000
+++ bzrlib/tests/test_diff.py 2014-09-16 17:36:26 +0000
@@ -1,4 +1,4 @@
1# Copyright (C) 2005-2011 Canonical Ltd1# Copyright (C) 2005-2014 Canonical Ltd
2#2#
3# This program is free software; you can redistribute it and/or modify3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by4# it under the terms of the GNU General Public License as published by
@@ -143,6 +143,19 @@
143 'old', ['boo\n'], 'new', ['goo\n'],143 'old', ['boo\n'], 'new', ['goo\n'],
144 StringIO(), diff_opts=['-u'])144 StringIO(), diff_opts=['-u'])
145145
146 def test_default_style_unified(self):
147 """Check for default style '-u' only if no other style specified
148 in 'diff-options'.
149 """
150 # Verify that style defaults to unified, id est '-u' appended
151 # to option list, in the absence of an alternative style.
152 self.assertEqual(['-a', '-u'], diff.default_style_unified(["-a"]))
153 # Verify that for all valid style options, '-u' is not
154 # appended to option list.
155 for s in diff.style_option_list:
156 ret_opts = diff.default_style_unified(diff_opts=["%s" % (s,)])
157 self.assertEqual(["%s" % (s,)], ret_opts)
158
146 def test_internal_diff_default(self):159 def test_internal_diff_default(self):
147 # Default internal diff encoding is utf8160 # Default internal diff encoding is utf8
148 output = StringIO()161 output = StringIO()
@@ -1391,7 +1404,7 @@
1391 diff_obj._execute('old', 'new')1404 diff_obj._execute('old', 'new')
1392 self.assertEqual(output.getvalue().rstrip(), 'old new')1405 self.assertEqual(output.getvalue().rstrip(), 'old new')
13931406
1394 def test_excute_missing(self):1407 def test_execute_missing(self):
1395 diff_obj = diff.DiffFromTool(['a-tool-which-is-unlikely-to-exist'],1408 diff_obj = diff.DiffFromTool(['a-tool-which-is-unlikely-to-exist'],
1396 None, None, None)1409 None, None, None)
1397 self.addCleanup(diff_obj.finish)1410 self.addCleanup(diff_obj.finish)