Code review comment for lp:~ace17/bzr/fixDiff4

Here's what I'm noticing now that I'm digging into this problem a little more deeply:
0. First of all, I'm testing on trunk code
rwilbur@ordinate:~/src/bzr$ ./bzr --version
Bazaar (bzr) 2.7.0dev1
  from bzr checkout /home/rwilbur/src/bzr
    revision: 6597
    revid: <email address hidden>
    branch nick: bzr
  Python interpreter: /usr/bin/python 2.7.3
  Python standard library: /usr/lib/python2.7
  Platform: Linux-3.2.0-64-generic-pae-i686-with-Ubuntu-12.04-precise
[...]
1. You have one new file. 'File1' was just added and so has no previous contents in the working tree to calculate a difference from, so bzrlib/diff.py:DiffTree._show_diff() prints the banner
'=== added file 'File1' and then runs a special diff to handle no previous file. bzr creates an empty file with the date and time of the UNIX epoch as a signal to 'patch' that this is a new file and then runs the diff'ers from a list of DiffPath objects till one of them returns a result which is not DiffPath.CANNOT_DIFF or we run off the end of the list.

As your added 'File1' is lexically earlier than your modified 'File2' and the option '-f' is not a valid option for 'diff', the command ends with the error reported by diff when invoked with '-u -f'. I created a similar test case except my modified 'test.text' is lexically before my added 'test2.text' and so when I get an error on my added file it occurs after already processing the modified file, shown below.

~/src/bzr$ ./bzr status ../test
added:
  test2.text
modified:
  test.text
~/src/bzr$ ./bzr diff ../test/
=== modified file 'test.text'
--- test.text 2014-06-15 18:31:57 +0000
+++ test.text 2014-06-15 18:32:54 +0000
@@ -1,1 +1,2 @@
 test
+testing

=== added file 'test2.text'
--- test2.text 1970-01-01 00:00:00 +0000
+++ test2.text 2014-06-15 21:25:02 +0000
@@ -0,0 +1,1 @@
+test2

Now I invoke it with a '--using' clause to test.

~/src/bzr$ ./bzr diff --using=/bin/ls ../test/
=== modified file 'test.text'
/home/rwilbur/src/test/test.text
/tmp/bzr-diff-609Ozf/old/test.text
=== added file 'test2.text'
--- test2.text 1970-01-01 00:00:00 +0000
+++ test2.text 2014-06-15 21:25:02 +0000
@@ -0,0 +1,1 @@
+test2

It is only used on the modified file as the diff is simulated for the added file. Next I tried using 'cat' instead of 'ls' and this time added a 'diff-options' clause that should number the lines.

~/src/bzr$ ./bzr diff --using=/bin/cat --diff-options='-n' ../test/
=== modified file 'test.text'
     1 test
     2 test
     3 testing
=== added file 'test2.text'
a0 1
test2

So, on the modified file the line from the previous content and the two lines from the new version are concatenated and sequentially numbered. In this case, I'm not seeing the diff options override the using clause(/bin/cat is still being invoked). The added file has different output because the '-n' diff option was used to output a RCS format diff.

~/src/bzr$ ./bzr diff --using=/bin/cat --diff-options='-f' ../test/
=== modified file 'test.text'
/bin/cat: invalid option -- 'f'
Try `/bin/cat --help' for more information.
=== added file 'test2.text'
diff: conflicting output style options
diff: Try `diff --help' for more information.

bzr: ERROR: external diff failed with exit code 2; command: ['diff', '--label', u'test2.text\t1970-01-01 00:00:00 +0000', '/tmp/bzr-diff-old-V9LxhX', '--label', u'test2.text\t2014-06-15 21:25:02 +0000', '/tmp/bzr-diff-new-NW5SdC', '--binary', '-u', '-f']

This last example I used the same diff option as you tried, '-f', and for the modified file it is passed to 'cat' which doesn't know what to do with it, whereas for the added file it is run through the bzr diff option parser to see if it is a recognized diff format option. The bzr diff option parser doesn't recognize '-f' as a valid diff format option and so prepends '-u' as a default diff format. diff is then complaining because it is fed two conflicting output style options:
 '-u' Use the unified output format, showing three lines of context.
and
 '-f' Make output that looks vaguely like an `ed' script but has changes
     in the order they appear in the file.

So, I guess there are three issues which I can see:
1. The diff option parser could be changed to recognize '-f' as a valid format option.
2. The external diff tool specified by the 'using' clause could be used for the special cases: file added, removed, etc.
3. When using an external diff tool specified by the 'using' clause, if the 'diff-options' clause is present, pass it unmodified to the user-specified external diff tool.

I created a patch to add the other diff styles to the option parser so that it won't insert '-u' for any valid diff style that is specified in diff-options(#1, above). I'll push the branch up on Launchpad after I write a test for the option parser.

Looks like your branch covers numbers 2 and 3, above. Please write a test for
 bzr diff --using <> --diff-options '[...]'
in bzrlib/tests/test_diff.py to verify that passing --diff-options doesn't annul the --using clause. You may find TestDiffFromTool a relatively easy class to add your test to as you can set up a diff invocation and then grab the command that would be issued without worrying about having the target binary installed or checking the output (see for example TestDiffFromTool.test_from_string, TestDiffFromTool.test_from_string_u5).

Thanks for finding the error and pointing it out. If you desire assistance with writing the test, let me know.