[PATCH] send external diff to a file (bug 4047)

[James Henstridge]

On 5/4/06, John Arbash Meinel <john at arbash-meinel.com> wrote:
> James Henstridge wrote:
> > This patch is based on a patch I did a while ago to allow redirecting
> > diff output to a file when using an external diff process.  Currently
> > the external_diff() function contains the following check which
> > prevents this:
> >
> > 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 to_file != sys.stdout:
> >        raise NotImplementedError("sorry, can't send external diff
> > other than to stdout yet",
> >                                  to_file)
> >    ...
> >
> >
> > The patch alters external_diff() to use the subprocess module to
> > execute the external command rather than os.spawnv() which makes it
> > possible to direct the output to a file descriptor other than stdout. I
> > also added a simple test for the external_diff() function as Martin
> > requested in the bug report.
> >
> > I haven't added support for directing output to file like objects
> > without an associated file descriptor (e.g. StringIO), so left a check
> > for that condition in the code.
> >
> > The changes are in my bzr branch here (currently at revno 1695):
> >    http://people.ubuntu.com/~jamesh/bzr/bzr.smallfixes
> >
> > James.
> >
>
> And sys.stdout isn't guaranteed to be a real file either, since it may
> have been internally changed.
> So this fix is a good thing in general.
>

[...]
> These imports should really happen at the module level, not inside the
> diff function.

Is this a new coding guideline for bzr?  I noticed a lot of other
functions that contain imports (including this one), so I just
followed the existing style.  I guessed that this was to remove the
cost of the imports if the function in question did not get called.

If the recommended style has changed, I can fix things up.

>
> Otherwise it looks good.


James.