Patchwork [V2,1/2] diff: add the --output option

login
register
mail settings
Submitter Ahmed S. Darwish
Date July 1, 2013, 4:25 p.m.
Message ID <20130701162559.GA10595@darwish>
Download mbox | patch
Permalink /patch/1776/
State Changes Requested, archived
Headers show

Comments

Ahmed S. Darwish - July 1, 2013, 4:25 p.m.
# HG changeset patch
# User Ahmed S. Darwish <a.darwish@vireton.com>
# Date 1372695210 -7200
# Node ID c1aa2fdfc7b7a8fff58b192259dcdf4f74f7ada7
# Parent  648d1974b3f328947ee6cf2d00c66815a33cd208
diff: add the --output option

For all shells which cannot save a command standard output correctly,
this option is now introduced.

The most common example is Microsoft PowerShell, where the piped
output gets corrupted if saved using the standard, unix-like, stdout
rediction, '>' operator. By transforming the piped output to a
different encoding, PowerShell saves 'hg diff' patch output to a
format __not understandable__ by GNU patch and 'hg patch' commands.

Windows PowerShell is installed by default on all Windows 7+
machines (Windows 7, 8, Server 2008, and Server 2012). An easily
invokable 'hg diff > temp.patch' command should thus be available
on these systems.

For a similar real-world scenario, please check:

http://www.webcitation.org/6Hiiqf425 - Archived from the original
post at http://nbevans.wordpress.com/2011/02/22/lightweight-shelving-of-your-work-in-progress-with-mercurial/
Augie Fackler - July 9, 2013, 9:52 p.m.
On Jul 1, 2013, at 12:25 PM, Ahmed S. Darwish <darwish.07@gmail.com> wrote:

> # HG changeset patch
> # User Ahmed S. Darwish <a.darwish@vireton.com>
> # Date 1372695210 -7200
> # Node ID c1aa2fdfc7b7a8fff58b192259dcdf4f74f7ada7
> # Parent  648d1974b3f328947ee6cf2d00c66815a33cd208
> diff: add the --output option
> 
> For all shells which cannot save a command standard output correctly,
> this option is now introduced.
> 
> The most common example is Microsoft PowerShell, where the piped
> output gets corrupted if saved using the standard, unix-like, stdout
> rediction, '>' operator. By transforming the piped output to a
> different encoding, PowerShell saves 'hg diff' patch output to a
> format __not understandable__ by GNU patch and 'hg patch' commands.
> 
> Windows PowerShell is installed by default on all Windows 7+
> machines (Windows 7, 8, Server 2008, and Server 2012). An easily
> invokable 'hg diff > temp.patch' command should thus be available
> on these systems.
> 
> For a similar real-world scenario, please check:
> 
> http://www.webcitation.org/6Hiiqf425 - Archived from the original
> post at http://nbevans.wordpress.com/2011/02/22/lightweight-shelving-of-your-work-in-progress-with-mercurial/
> 
> diff -r 648d1974b3f3 -r c1aa2fdfc7b7 mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py	Sun Jun 30 15:19:39 2013 -0500
> +++ b/mercurial/cmdutil.py	Mon Jul 01 18:13:30 2013 +0200
> @@ -7,6 +7,7 @@
> 
> from node import hex, nullid, nullrev, short
> from i18n import _
> +from datetime import datetime
> import os, sys, errno, re, tempfile
> import util, scmutil, templater, patch, error, templatekw, revlog, copies
> import match as matchmod
> @@ -123,6 +124,16 @@
>         limit = None
>     return limit
> 
> +def fsfriendly(user):
> +    """return committer's username in a short and filesystem-friendly
> +    manner.  This basically implies removing email info, white spaces,
> +    and other problematic characters for common filesystems.
> +    reference: MSDN - Naming Files, Paths, and Namespaces.
> +    """
> +    user = re.sub('\<[^<]*@[^>]*\>', '', user)
> +    user = re.sub("[\W]", '', user)
> +    return user.lower()
> +

This function could really stand to be its own patch.

> def makefilename(repo, pat, node, desc=None,
>                   total=None, seqno=None, revwidth=None, pathname=None):
>     node_expander = {
> @@ -134,6 +145,8 @@
>     expander = {
>         '%': lambda: '%',
>         'b': lambda: os.path.basename(repo.root),
> +        'u': lambda: fsfriendly(repo.ui.username()),
> +        'd': lambda: datetime.now().strftime("%Y_%m_%d-%H_%M_%S")
>         }
> 
>     try:
> diff -r 648d1974b3f3 -r c1aa2fdfc7b7 mercurial/commands.py
> --- a/mercurial/commands.py	Sun Jun 30 15:19:39 2013 -0500
> +++ b/mercurial/commands.py	Mon Jul 01 18:13:30 2013 +0200
> @@ -2676,7 +2676,9 @@
>         ui.warn("%s\n" % res2)
> 
> @command('^diff',
> -    [('r', 'rev', [], _('revision'), _('REV')),
> +    [('o', 'output', '',
> +     _('print output to file with formatted name'), _('FORMAT')),
> +    ('r', 'rev', [], _('revision'), _('REV')),
>     ('c', 'change', '', _('change made by revision'), _('REV'))
>     ] + diffopts + diffopts2 + walkopts + subrepoopts,
>     _('[OPTION]... ([-c REV] | [-r REV1 [-r REV2]]) [FILE]...'))
> @@ -2692,6 +2694,21 @@
>        default to comparing against the working directory's first
>        parent changeset if no revisions are specified.
> 
> +    Output may be to a file, in which case the name of the file is
> +    given using a format string. The formatting rules are as follows:
> +
> +    :``%%``: literal "%" character
> +    :``%b``: basename of the exporting repository
> +    :``%u``: committer username, in a filesystem-friendly manner
> +    :``%d``: current local date and time
> +
> +    .. note::
> +       Do not save diff output using Windows PowerShell (R) pipeline
> +       `|` or standard output redirection `>` facilities. They change
> +       the resulting diffs encoding, making them unappliable by
> +       :hg:`patch` or GNU patch afterwards. Use the
> +       :hg:`diff --output` option instead.
> +
>     When two revision arguments are given, then changes are shown
>     between those revisions. If only one revision is specified then
>     that revision is compared to the working directory, and, when no
> @@ -2737,6 +2754,7 @@
>     Returns 0 on success.
>     """
> 
> +    fname = opts.get('output')
>     revs = opts.get('rev')
>     change = opts.get('change')
>     stat = opts.get('stat')
> @@ -2754,10 +2772,14 @@
>     if reverse:
>         node1, node2 = node2, node1
> 
> +    fp = None
> +    if fname:
> +        fp = cmdutil.makefileobj(repo, fname)
> +
>     diffopts = patch.diffopts(ui, opts)
>     m = scmutil.match(repo[node2], pats, opts)
>     cmdutil.diffordiffstat(ui, repo, diffopts, node1, node2, m, stat=stat,
> -                           listsubrepos=opts.get('subrepos'))
> +                           fp=fp, listsubrepos=opts.get('subrepos'))
> 
> @command('^export',
>     [('o', 'output', '',
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - July 9, 2013, 10:10 p.m.
On 1 juil. 2013, at 18:25, Ahmed S. Darwish wrote:

> # HG changeset patch
> # User Ahmed S. Darwish <a.darwish@vireton.com>
> # Date 1372695210 -7200
> # Node ID c1aa2fdfc7b7a8fff58b192259dcdf4f74f7ada7
> # Parent  648d1974b3f328947ee6cf2d00c66815a33cd208
> diff: add the --output option
> 
> For all shells which cannot save a command standard output correctly,
> this option is now introduced.
> 
> The most common example is Microsoft PowerShell, where the piped
> output gets corrupted if saved using the standard, unix-like, stdout
> rediction, '>' operator. By transforming the piped output to a
> different encoding, PowerShell saves 'hg diff' patch output to a
> format __not understandable__ by GNU patch and 'hg patch' commands.

Really, that is an impressive level of failure. From the example you pasted it seems that line ending get converted.

But why are you adding it to diff only ? Output of any Mercurial commands can get corrupted. And multiple other commands export diff (log -p, export, …) A global setting would make more sense.

However, I would be sad to see such flag added when the shell is to blame. Maybe an environment variable could do the job: `HGSTDOUT=<filename> hg diff`. That would (1) allow any commands to redirect it's output. (2) avoid adding a very visible flag for all users of a sensible shell.

(question: have you considered patch hg import so it can import patch using such line ending ?)
Matt Mackall - July 9, 2013, 10:16 p.m.
On Wed, 2013-07-10 at 00:10 +0200, Pierre-Yves David wrote:
> (question: have you considered patch hg import so it can import patch using such line ending ?)

It's not just line endings. Powershell can be relied on to mangle
anything non-ASCII too.
Ahmed S. Darwish - July 9, 2013, 11:20 p.m.
Hi,

On Wed, Jul 10, 2013 at 12:10:20AM +0200, Pierre-Yves David wrote:
> 
> On 1 juil. 2013, at 18:25, Ahmed S. Darwish wrote:
> 
> > # HG changeset patch
> > # User Ahmed S. Darwish <a.darwish@vireton.com>
> > # Date 1372695210 -7200
> > # Node ID c1aa2fdfc7b7a8fff58b192259dcdf4f74f7ada7
> > # Parent  648d1974b3f328947ee6cf2d00c66815a33cd208
> > diff: add the --output option
> > 
> > For all shells which cannot save a command standard output correctly,
> > this option is now introduced.
> > 
> > The most common example is Microsoft PowerShell, where the piped
> > output gets corrupted if saved using the standard, unix-like, stdout
> > rediction, '>' operator. By transforming the piped output to a
> > different encoding, PowerShell saves 'hg diff' patch output to a
> > format __not understandable__ by GNU patch and 'hg patch' commands.
> 
> Really, that is an impressive level of failure. From the example you
> pasted it seems that line ending get converted.
> 

Yes, but the main problem is not about line-endings; it's about the
entire patch encoding getting transformed from ASCII to UTF-16, making
it unappliable by all of our common command-line tools.

> But why are you adding it to diff only ? Output of any Mercurial
> commands can get corrupted. And multiple other commands export
> diff (log -p, export, …) A global setting would make more sense.
> 

I don't think shelving code using `hg log -p' is that common, but
I might be wrong. `export' and `cat' already have their own --output
option.

> However, I would be sad to see such flag added when the shell is to
> blame. 

I can understand your concern, but for good or bad PowerShell is
now the defacto shell for Windows 7+ machines :-(

The `--output FORMAT' option is already available for several other
commands; I don't think adding it for `hg diff' would break __any__
expectations.

> (question: have you considered patch hg import so it can import
> patch using such line ending ?)
> 

`hg import' recognizing UTF16-encoded patches?

>
> Pierre-Yves David
> 

Thanks,

--
Darwish
http://darwish.chasingpointers.com
Ahmed S. Darwish - July 9, 2013, 11:43 p.m.
On Tue, Jul 09, 2013 at 05:52:23PM -0400, Augie Fackler wrote:
> 
> On Jul 1, 2013, at 12:25 PM, Ahmed S. Darwish <darwish.07@gmail.com> wrote:
> ...
> > 
> > diff -r 648d1974b3f3 -r c1aa2fdfc7b7 mercurial/cmdutil.py
> > --- a/mercurial/cmdutil.py	Sun Jun 30 15:19:39 2013 -0500
> > +++ b/mercurial/cmdutil.py	Mon Jul 01 18:13:30 2013 +0200
> > @@ -7,6 +7,7 @@
> > 
> > from node import hex, nullid, nullrev, short
> > from i18n import _
> > +from datetime import datetime
> > import os, sys, errno, re, tempfile
> > import util, scmutil, templater, patch, error, templatekw, revlog, copies
> > import match as matchmod
> > @@ -123,6 +124,16 @@
> >         limit = None
> >     return limit
> > 
> > +def fsfriendly(user):
> > +    """return committer's username in a short and filesystem-friendly
> > +    manner.  This basically implies removing email info, white spaces,
> > +    and other problematic characters for common filesystems.
> > +    reference: MSDN - Naming Files, Paths, and Namespaces.
> > +    """
> > +    user = re.sub('\<[^<]*@[^>]*\>', '', user)
> > +    user = re.sub("[\W]", '', user)
> > +    return user.lower()
> > +
> 
> This function could really stand to be its own patch.
> 

So should I now send a V3 version for this patch series?

Thanks,

--
Darwish
http://darwish.chasingpointers.com
Augie Fackler - July 9, 2013, 11:43 p.m.
On Jul 9, 2013, at 7:43 PM, "Ahmed S. Darwish" <darwish.07@gmail.com> wrote:

>>> +def fsfriendly(user):
>>> +    """return committer's username in a short and filesystem-friendly
>>> +    manner.  This basically implies removing email info, white spaces,
>>> +    and other problematic characters for common filesystems.
>>> +    reference: MSDN - Naming Files, Paths, and Namespaces.
>>> +    """
>>> +    user = re.sub('\<[^<]*@[^>]*\>', '', user)
>>> +    user = re.sub("[\W]", '', user)
>>> +    return user.lower()
>>> +
>> 
>> This function could really stand to be its own patch.
>> 
> 
> So should I now send a V3 version for this patch series?
> 

Yes please.
Pierre-Yves David - July 10, 2013, 12:17 a.m.
On 10 juil. 2013, at 01:20, Ahmed S. Darwish wrote:

> On Wed, Jul 10, 2013 at 12:10:20AM +0200, Pierre-Yves David wrote:
>> 
>> But why are you adding it to diff only ? Output of any Mercurial
>> commands can get corrupted. And multiple other commands export
>> diff (log -p, export, …) A global setting would make more sense.
>> 
> 
> I don't think shelving code using `hg log -p' is that common, but
> I might be wrong. `export' and `cat' already have their own --output
> option.
> 
>> However, I would be sad to see such flag added when the shell is to
>> blame. 
> 
> I can understand your concern, but for good or bad PowerShell is
> now the defacto shell for Windows 7+ machines :-(
> 
> The `--output FORMAT' option is already available for several other
> commands; I don't think adding it for `hg diff' would break __any__
> expectations.

The fact that cat and export already have this --output option changes things and should be reminded in the commit message.

This move the question from "adding an new --output flag" to "extending the --output flag to diff" and this seems much more reasonable.

Notes that in cat and export the option may be formatted. How that transpose to diff ? (not a blocking question, imho)
Ahmed S. Darwish - July 11, 2013, 8:23 p.m.
Hi,

On Wed, Jul 10, 2013 at 02:17:06AM +0200, Pierre-Yves David wrote:
> 
> On 10 juil. 2013, at 01:20, Ahmed S. Darwish wrote:
> > 
> > I don't think shelving code using `hg log -p' is that common, but
> > I might be wrong. `export' and `cat' already have their own --output
> > option.
> > 
...
> > 
> > The `--output FORMAT' option is already available for several other
> > commands; I don't think adding it for `hg diff' would break __any__
> > expectations.
> 
> The fact that cat and export already have this --output option
> changes things and should be reminded in the commit message.
> 
> This move the question from "adding an new --output flag" to
> "extending the --output flag to diff" and this seems much more
> reasonable.
> 

I see; will make that point clear in the commit message of V3.

>
> Notes that in cat and export the option may be formatted. How
> that transpose to diff ? (not a blocking question, imho)
> 

I've added the '%u' and '%o' format options for diff --output.
All of the format options for export and cat (except '%b') do
not make sense in our case.

>
> Pierre-Yves David
>

Thanks,

--
Darwish
http://darwish.chasingpointers.com
Ahmed S. Darwish - July 11, 2013, 9:02 p.m.
Hi!

On Tue, Jul 09, 2013 at 10:25:14PM -0500, Kevin Bullock wrote:
> On 1 Jul 2013, at 11:25 AM, Ahmed S. Darwish wrote:
> 
> > # HG changeset patch
> > # User Ahmed S. Darwish <a.darwish@vireton.com>
> > # Date 1372695210 -7200
> > # Node ID c1aa2fdfc7b7a8fff58b192259dcdf4f74f7ada7
> > # Parent  648d1974b3f328947ee6cf2d00c66815a33cd208
> > diff: add the --output option
> > 
> > For all shells which cannot save a command standard output correctly,
> > this option is now introduced.
> 
> Missing tests. More comments below.
> 

Will do.

> > 
> > diff -r 648d1974b3f3 -r c1aa2fdfc7b7 mercurial/cmdutil.py
> > --- a/mercurial/cmdutil.py	Sun Jun 30 15:19:39 2013 -0500
> > +++ b/mercurial/cmdutil.py	Mon Jul 01 18:13:30 2013 +0200
> > @@ -7,6 +7,7 @@
> > 
> > from node import hex, nullid, nullrev, short
> > from i18n import _
> > +from datetime import datetime
> > import os, sys, errno, re, tempfile
> > import util, scmutil, templater, patch, error, templatekw, revlog, copies
> > import match as matchmod
> > @@ -123,6 +124,16 @@
> >         limit = None
> >     return limit
> > 
> > +def fsfriendly(user):
> > +    """return committer's username in a short and filesystem-friendly
> > +    manner.  This basically implies removing email info, white spaces,
> > +    and other problematic characters for common filesystems.
> > +    reference: MSDN - Naming Files, Paths, and Namespaces.
> > +    """
> > +    user = re.sub('\<[^<]*@[^>]*\>', '', user)
> > +    user = re.sub("[\W]", '', user)
> > +    return user.lower()
> > +
> > def makefilename(repo, pat, node, desc=None,
> >                   total=None, seqno=None, revwidth=None, pathname=None):
> >     node_expander = {
> > @@ -134,6 +145,8 @@
> >     expander = {
> >         '%': lambda: '%',
> >         'b': lambda: os.path.basename(repo.root),
> > +        'u': lambda: fsfriendly(repo.ui.username()),
> > +        'd': lambda: datetime.now().strftime("%Y_%m_%d-%H_%M_%S")
> 
...
> Oh, and repo.ui.username() is _not_ (necessarily) the
> committer; I'm not sure that "committer" has any meaning
> for diff unless it's of a single rev.

Indeed.

I'll thus modify the behavior to the following: If the '-c REV'
option was added to diff, include _that_ commit's author. In all
other cases, use repo.ui.username().

>
> It _would_, however, have meaning for 'export'. And "date"
> would have a different meaning for diff vs. export as well.
> 

I've ommitted "date"(%d) and "committer"(%u) from the documentation
of 'hg export --output' on purpose. I did not feel it would be of
much importance there. They're thus, at least for now, exclusive to
diff --output.

Thanks,

--
Darwish
http://darwish.chasingpointers.com
Ahmed S. Darwish - July 12, 2013, 6:46 p.m.
On Fri, Jul 12, 2013 at 12:36:40PM -0500, Kevin Bullock wrote:
> On 11 Jul 2013, at 4:02 PM, Ahmed S. Darwish wrote:
> 
> > On Tue, Jul 09, 2013 at 10:25:14PM -0500, Kevin Bullock wrote:
> >> Oh, and repo.ui.username() is _not_ (necessarily) the
> >> committer; I'm not sure that "committer" has any meaning
> >> for diff unless it's of a single rev.
> > 
> > Indeed.
> > 
> > I'll thus modify the behavior to the following: If the '-c REV'
> > option was added to diff, include _that_ commit's author. In all
> > other cases, use repo.ui.username().
> > 
> >> 
> >> It _would_, however, have meaning for 'export'. And "date"
> >> would have a different meaning for diff vs. export as well.
> >> 
> > 
> > I've ommitted "date"(%d) and "committer"(%u) from the documentation
> > of 'hg export --output' on purpose. I did not feel it would be of
> > much importance there. They're thus, at least for now, exclusive to
> > diff --output.
> 
> Except that they're not, if I understand your patch correctly --
> they're just undocumented for other things that use makefilename.
> 

Yes. I'll check if the new flags can be added exclusively to
'diff --output'.

thanks,

--
Darwish
http://darwish.chasingpointers.com
Matt Mackall - July 16, 2013, 6:16 p.m.
On Fri, 2013-07-12 at 20:46 +0200, Ahmed S. Darwish wrote:
> On Fri, Jul 12, 2013 at 12:36:40PM -0500, Kevin Bullock wrote:
> > On 11 Jul 2013, at 4:02 PM, Ahmed S. Darwish wrote:
> > 
> > > On Tue, Jul 09, 2013 at 10:25:14PM -0500, Kevin Bullock wrote:
> > >> Oh, and repo.ui.username() is _not_ (necessarily) the
> > >> committer; I'm not sure that "committer" has any meaning
> > >> for diff unless it's of a single rev.
> > > 
> > > Indeed.
> > > 
> > > I'll thus modify the behavior to the following: If the '-c REV'
> > > option was added to diff, include _that_ commit's author. In all
> > > other cases, use repo.ui.username().
> > > 
> > >> 
> > >> It _would_, however, have meaning for 'export'. And "date"
> > >> would have a different meaning for diff vs. export as well.
> > >> 
> > > 
> > > I've ommitted "date"(%d) and "committer"(%u) from the documentation
> > > of 'hg export --output' on purpose. I did not feel it would be of
> > > much importance there. They're thus, at least for now, exclusive to
> > > diff --output.
> > 
> > Except that they're not, if I understand your patch correctly --
> > they're just undocumented for other things that use makefilename.
> > 
> 
> Yes. I'll check if the new flags can be added exclusively to
> 'diff --output'.

No, please don't. Just because you haven't thought of a use for a
feature that you've trivially created doesn't mean it doesn't exist.

Patch

diff -r 648d1974b3f3 -r c1aa2fdfc7b7 mercurial/cmdutil.py
--- a/mercurial/cmdutil.py	Sun Jun 30 15:19:39 2013 -0500
+++ b/mercurial/cmdutil.py	Mon Jul 01 18:13:30 2013 +0200
@@ -7,6 +7,7 @@ 
 
 from node import hex, nullid, nullrev, short
 from i18n import _
+from datetime import datetime
 import os, sys, errno, re, tempfile
 import util, scmutil, templater, patch, error, templatekw, revlog, copies
 import match as matchmod
@@ -123,6 +124,16 @@ 
         limit = None
     return limit
 
+def fsfriendly(user):
+    """return committer's username in a short and filesystem-friendly
+    manner.  This basically implies removing email info, white spaces,
+    and other problematic characters for common filesystems.
+    reference: MSDN - Naming Files, Paths, and Namespaces.
+    """
+    user = re.sub('\<[^<]*@[^>]*\>', '', user)
+    user = re.sub("[\W]", '', user)
+    return user.lower()
+
 def makefilename(repo, pat, node, desc=None,
                   total=None, seqno=None, revwidth=None, pathname=None):
     node_expander = {
@@ -134,6 +145,8 @@ 
     expander = {
         '%': lambda: '%',
         'b': lambda: os.path.basename(repo.root),
+        'u': lambda: fsfriendly(repo.ui.username()),
+        'd': lambda: datetime.now().strftime("%Y_%m_%d-%H_%M_%S")
         }
 
     try:
diff -r 648d1974b3f3 -r c1aa2fdfc7b7 mercurial/commands.py
--- a/mercurial/commands.py	Sun Jun 30 15:19:39 2013 -0500
+++ b/mercurial/commands.py	Mon Jul 01 18:13:30 2013 +0200
@@ -2676,7 +2676,9 @@ 
         ui.warn("%s\n" % res2)
 
 @command('^diff',
-    [('r', 'rev', [], _('revision'), _('REV')),
+    [('o', 'output', '',
+     _('print output to file with formatted name'), _('FORMAT')),
+    ('r', 'rev', [], _('revision'), _('REV')),
     ('c', 'change', '', _('change made by revision'), _('REV'))
     ] + diffopts + diffopts2 + walkopts + subrepoopts,
     _('[OPTION]... ([-c REV] | [-r REV1 [-r REV2]]) [FILE]...'))
@@ -2692,6 +2694,21 @@ 
        default to comparing against the working directory's first
        parent changeset if no revisions are specified.
 
+    Output may be to a file, in which case the name of the file is
+    given using a format string. The formatting rules are as follows:
+
+    :``%%``: literal "%" character
+    :``%b``: basename of the exporting repository
+    :``%u``: committer username, in a filesystem-friendly manner
+    :``%d``: current local date and time
+
+    .. note::
+       Do not save diff output using Windows PowerShell (R) pipeline
+       `|` or standard output redirection `>` facilities. They change
+       the resulting diffs encoding, making them unappliable by
+       :hg:`patch` or GNU patch afterwards. Use the
+       :hg:`diff --output` option instead.
+
     When two revision arguments are given, then changes are shown
     between those revisions. If only one revision is specified then
     that revision is compared to the working directory, and, when no
@@ -2737,6 +2754,7 @@ 
     Returns 0 on success.
     """
 
+    fname = opts.get('output')
     revs = opts.get('rev')
     change = opts.get('change')
     stat = opts.get('stat')
@@ -2754,10 +2772,14 @@ 
     if reverse:
         node1, node2 = node2, node1
 
+    fp = None
+    if fname:
+        fp = cmdutil.makefileobj(repo, fname)
+
     diffopts = patch.diffopts(ui, opts)
     m = scmutil.match(repo[node2], pats, opts)
     cmdutil.diffordiffstat(ui, repo, diffopts, node1, node2, m, stat=stat,
-                           listsubrepos=opts.get('subrepos'))
+                           fp=fp, listsubrepos=opts.get('subrepos'))
 
 @command('^export',
     [('o', 'output', '',