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

[James Henstridge]

On 09/06/06, John Arbash Meinel <john at arbash-meinel.com> wrote:
> 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.

Okay, I've fixed the import ordering.

> > @@ -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.

I hadn't thought of that.  The documentation seems to indicate that it
will still fail for Windows NT derivatives (i.e. most Windows users):
    http://docs.python.org/lib/module-tempfile.html#l2h-2202

I suppose direct mkstemp use would be appropriate here (I haven't
implemented this yet).

> > === 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.

I've made this change now.  That should make the tests not fail unless
(a) the user is on Windows and (b) they have "diff" in the path.  The
code will need to switch away from NamedTemporaryFile to fix that
case.

The new changes are in my branch.  I'll post another patch with the
NamedTemporaryFile stuff changed.  I'd need someone else to test if it
fixes the problem on Windows though.

James.