[MERGE] get rid of os.spawnvp() usage for external diff

[John Arbash Meinel]

James Henstridge wrote:
> This is a patch I did a while back to allow sending diff output to a
> file when using an external diff rather than just stdout.
> 
> To do this, it changed external_diff() to use the subprocess module to
> invoke diff rather than os.spawnvp().  Since os.spawnvp() is not
> available on Windows (according to the Python documentation), this
> should also improve cross platform compatibility.
> 
> The changes are available in following branch at revision 1698:
>    http://people.ubuntu.com/~jamesh/bzr/bzr.smallfixes
> 
> The patch should fix bugs 4047 and 48914.
> 
> James.

+1 on general concept, but it needs a little bit of cleanup.

> 
> ------------------------------------------------------------------------
> 
> === modified file 'bzrlib/diff.py'
> --- bzrlib/diff.py	2006-06-06 12:05:43 +0000
> +++ bzrlib/diff.py	2006-06-07 04:49:39 +0000
> @@ -14,6 +14,10 @@
>  # along with this program; if not, write to the Free Software
>  # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>  
> +import sys
> +import subprocess
> +from tempfile import NamedTemporaryFile
> +
>  import time

^- We put imports in alphabetical order, with standard library modules
together, and a blank line, and then bzrlib imports, and then 2 blank
lines. So the above should be:

import subprocess
import sys
from tempfile import NamedTemporaryFile
import time

from bzrlib ...

I'm happy to see import sys moved up here where it belongs anyway.

>  
>  from bzrlib.delta import compare_trees
> @@ -81,18 +85,13 @@
>  def external_diff(old_filename, oldlines, new_filename, newlines, to_file,
>                    diff_opts):
>      """Display a diff by calling out to the external diff program."""
> -    import sys
> +    if not hasattr(to_file, 'fileno'):
> +        raise NotImplementedError("sorry, can't send external diff other "
> +                                  "than to a file descriptor", to_file)
>      
> -    if to_file != sys.stdout:
> -        raise NotImplementedError("sorry, can't send external diff other than to stdout yet",
> -                                  to_file)
> -
>      # make sure our own output is properly ordered before the diff
>      to_file.flush()
>  
> -    from tempfile import NamedTemporaryFile
> -    import os
> -
>      oldtmpf = NamedTemporaryFile()
>      newtmpf = NamedTemporaryFile()
>  
> @@ -142,7 +141,11 @@
>          if diff_opts:
>              diffcmd.extend(diff_opts)
>  
> -        rc = os.spawnvp(os.P_WAIT, 'diff', diffcmd)
> +        pipe = subprocess.Popen(diffcmd,
> +                                stdin=subprocess.PIPE,
> +                                stdout=to_file)
> +        pipe.stdin.close()
> +        rc = pipe.wait()
>          

The only problem that I know of, is that NamedTemporaryFile cannot be
opened by a third-party program under windows. I'm not sure if children
processes can open it. But at one point (IIRC), I tried using it for
'gpg --clearsign', and that would fail because gpg couldn't open the
file. I suggest we test it, and if it works, we probably won't have any
problems.


>          if rc != 0 and rc != 1:
>              # returns 1 if files differ; that's OK
> @@ -174,7 +177,6 @@
>      supplies any two trees.
>      """
>      if output is None:
> -        import sys
>          output = sys.stdout
>  
>      if from_spec is None:
> @@ -221,7 +223,6 @@
>      The more general form is show_diff_trees(), where the caller
>      supplies any two trees.
>      """
> -    import sys
>      output = sys.stdout
>      def spec_tree(spec):
>          revision_id = spec.in_store(tree.branch).rev_id
> 
> === modified file 'bzrlib/tests/test_diff.py'
> --- bzrlib/tests/test_diff.py	2006-06-06 12:05:44 +0000
> +++ bzrlib/tests/test_diff.py	2006-06-07 04:51:36 +0000
> @@ -16,12 +16,12 @@
>  
>  import os
>  from cStringIO import StringIO
> +from tempfile import TemporaryFile
>  
> -from bzrlib.diff import internal_diff, show_diff_trees
> +from bzrlib.diff import internal_diff, external_diff, show_diff_trees
>  from bzrlib.errors import BinaryFile
>  import bzrlib.patiencediff
>  from bzrlib.tests import TestCase, TestCaseWithTransport, TestCaseInTempDir
> -from bzrlib.tests import TestCase, TestCaseInTempDir
>  
>  
>  def udiff_lines(old, new, allow_binary=False):
> @@ -30,6 +30,15 @@
>      output.seek(0, 0)
>      return output.readlines()
>  
> +def external_udiff_lines(old, new):
> +    output = TemporaryFile()
> +    external_diff('old', old, 'new', new, output, diff_opts=['-u'])
> +    output.seek(0, 0)
> +    lines = output.readlines()
> +    output.close()
> +    return lines
> +
> +
>  

^- one too many blank lines here (there should be 2 not 3).
However, there is also the problem that this expects an external diff to
exist on all platforms, which is not the case (most of the time there
will be no 'diff' in your path on windows, and I'm not really sure about
Mac).

So I think there needs to be a try/except around the 'external_diff'
call, which translates the OSError e.errno==errno.ENOENT into a
TestSkipped, which states that there is no external diff program to run.

>  class TestDiff(TestCase):
>  
> @@ -78,6 +87,10 @@
>          udiff_lines([1023 * 'a' + '\x00'], [], allow_binary=True)
>          udiff_lines([], [1023 * 'a' + '\x00'], allow_binary=True)
>  
> +    def test_external_diff(self):
> +        lines = external_udiff_lines(['boo\n'], ['goo\n'])
> +        self.check_patch(lines)
> +        
>      def test_internal_diff_default(self):
>          # Default internal diff encoding is utf8
>          output = StringIO()
> 

Otherwise I am very happy to see some tests for this, and the patch
looks pretty good.

John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 249 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060608/9e9416ed/attachment.pgp