Merge lp:~ace17/bzr/fixDiff into lp:bzr

[Sebastien Alaiwan]

Fixes a bug causing --diff-options being passed to the external GNU diff program when a custom diff tool has been specified.

ace@ANTEC bzr-test % bzr status -V
added:
  File1
modified:
  File2
ace@ANTEC bzr-test % bzr diff --using gvimdiff --diff-options "-f"
=== added file 'File1'
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'File1\t1970-01-01 00:00:00 +0000', '/tmp/bzr-diff-old-x6Y5up', '--label', u'File1\t2014-06-11 17:22:29 +0000', '/tmp/bzr-diff-new-MNV8Ut', '--binary', '-u', '-f']

[Richard Wilbur (richard-wilbur)]

I'd be happy to review this merge proposal but when I look at the web page it says:
    Updating diff...

    An updated diff will be available in a few minutes. Reload to see the changes.

I tried reloading the page ≥5 minutes later with identical results.

Next I checked the web page for the branch as I can generally look at the diff's for each revision and understand what changes are entailed. Surprisingly it said the branch is empty with no revisions!

I'm not sure what happened to cause this situation. My normal recipe for creating a branch is as follows:
bzr branch lp:bzr
cd bzr
[edit the file(s) with your favourite editor]
bzr commit -m 'Fix external diff....'
bzr push lp:~ace17/bzr/fixDiff

Then visit here:
https://code.launchpad.net/~ace17/bzr/fixDiff/+register-merge

Fill out the form and submit.

review: Needs Information

[Sebastien Alaiwan (ace17)]

Hi, thanks for your reply.
Here's how I did:

bzr branch --stacked lp:bzr
cd bzr
[edit the file(s) with your favourite editor]
bzr commit -m 'Fix external diff....'
bzr push lp:~ace17/bzr/fixDiff

I received a message from launchpad:
"Launchpad encountered an internal error during the following operation: generating the diff for a merge proposal. It was logged with id OOPS-8239c35b3d57eabeca064b77c3513d04. Sorry for the inconvenience."

I use "--stacked" because the whole bzr repository is very long to download. This might have cause the issue. I'm retrying to recreate the whole thing ('bzr reconfigure --unstacked' seems to have no effect)

[Sebastien Alaiwan (ace17)]

OK, i've created a new branch (which seems to contain revisions this time!).
Richard, once you've read these two messages I'll delete this broken merge proposal. Let's continue the discussion here: https://code.launchpad.net/~ace17/bzr/fixDiff4/+merge/222900

[Richard Wilbur (richard-wilbur)]

Sebastien, thanks for creating one that works. I'm sorry that the
"stacked" branch didn't work. I have never tried a "stacked" branch
so I hadn't previously encountered that problem. To be continued on
the new branch.

[lp:~ace17/bzr/fixDiff]

6599. By Sebastien Alaiwan on 2014-09-24

Merge

Unmerged revisions

6599. By Sebastien Alaiwan on 2014-09-24

Merge

6598. By Sebastien Alaiwan on 2014-06-11

Avoid calling 'external_diff' with options passed to --diff-options if --using is specified. Fixes the following from failing 'bzr add somefile.txt ; bzr diff --using gvimdiff --diff-options '-f -n -d'

=== modified file 'bzrlib/diff.py'
--- bzrlib/diff.py	2012-07-24 16:56:07 +0000
+++ bzrlib/diff.py	2014-06-12 05:05:27 +0000
@@ -930,17 +930,18 @@
         """
         if using is not None:
             extra_factories = [DiffFromTool.make_from_diff_tree(using, external_diff_options)]
+            diff_file = internal_diff
         else:
             extra_factories = []
-        if external_diff_options:
-            opts = external_diff_options.split()
-            def diff_file(olab, olines, nlab, nlines, to_file, path_encoding=None, context_lines=None):
-                """:param path_encoding: not used but required
-                        to match the signature of internal_diff.
-                """
-                external_diff(olab, olines, nlab, nlines, to_file, opts)
-        else:
-            diff_file = internal_diff
+            if external_diff_options:
+                opts = external_diff_options.split()
+                def diff_file(olab, olines, nlab, nlines, to_file, path_encoding=None, context_lines=None):
+                    """:param path_encoding: not used but required
+                            to match the signature of internal_diff.
+                    """
+                    external_diff(olab, olines, nlab, nlines, to_file, opts)
+            else:
+                diff_file = internal_diff
         diff_text = DiffText(old_tree, new_tree, to_file, path_encoding,
                              old_label, new_label, diff_file, context_lines=context_lines)
         return klass(old_tree, new_tree, to_file, path_encoding, diff_text,