Patchwork [4,of,4] hg: make --binary option for Git diffs default to off

login
register
mail settings
Submitter Alexander Fomin
Date April 4, 2017, 10:21 p.m.
Message ID <76f6cd23643c9816a688.1491344483@devvm2125.lla2.facebook.com>
Download mbox | patch
Permalink /patch/19963/
State Changes Requested
Headers show

Comments

Alexander Fomin - April 4, 2017, 10:21 p.m.
# HG changeset patch
# User Alexander Fomin <afomin@fb.com>
# Date 1491342655 25200
#      Tue Apr 04 14:50:55 2017 -0700
# Node ID 76f6cd23643c9816a688a1e12900a6856c5fabf6
# Parent  373acf7da1a621607a1cc062b96ad67a42a7e016
hg: make --binary option for Git diffs default to off

This patch makes --binary option default to off in order to avoid
generating binary diffs in Git mode unless explicitly requested by a
user (issue5510).
Ryan McElroy - April 5, 2017, 2:37 p.m.
On 4/4/17 11:21 PM, Alexander Fomin wrote:
> # HG changeset patch
> # User Alexander Fomin <afomin@fb.com>
> # Date 1491342655 25200
> #      Tue Apr 04 14:50:55 2017 -0700
> # Node ID 76f6cd23643c9816a688a1e12900a6856c5fabf6
> # Parent  373acf7da1a621607a1cc062b96ad67a42a7e016
> hg: make --binary option for Git diffs default to off

This patch needs a (BC) appended to its title since it changes backwards 
compatibility.

I'd like the community to weigh in on this patch once we have a 
land-ready version, so I'll wait for the next version before looping in 
more people to comment/review.

When you resend the series, remember to add a '--flag v2' to the `hg 
email` command so that people know it's a follow-up to a previously-send 
series.

>
> This patch makes --binary option default to off in order to avoid
> generating binary diffs in Git mode unless explicitly requested by a
> user (issue5510).

This isn't exactly issue5510. I'd suggest saying that this is "a 
follow-up as discussed in issue5510", or something like that.

>
> diff --git a/hgext/mq.py b/hgext/mq.py
> --- a/hgext/mq.py
> +++ b/hgext/mq.py
> @@ -513,6 +513,7 @@ class queue(object):
>           diffopts = patchmod.diffopts(self.ui, opts)
>           if self.gitmode == 'auto':
>               diffopts.upgrade = True
> +            diffopts.nobinary = False
>           elif self.gitmode == 'keep':
>               pass
>           elif self.gitmode in ('yes', 'no'):
> diff --git a/hgext/transplant.py b/hgext/transplant.py
> --- a/hgext/transplant.py
> +++ b/hgext/transplant.py
> @@ -143,6 +143,7 @@ class transplanter(object):
>           pulls = []
>           diffopts = patch.difffeatureopts(self.ui, opts)
>           diffopts.git = True
> +        diffopts.nobinary = False
>   
>           lock = tr = None
>           try:
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -162,6 +162,7 @@ def dorecord(ui, repo, commitfunc, cmdsu
>           diffopts = patch.difffeatureopts(ui, opts=opts, whitespace=True)
>           diffopts.nodates = True
>           diffopts.git = True
> +        diffopts.nobinary = False
>           diffopts.showfunc = True
>           originaldiff = patch.diff(repo, changes=status, opts=diffopts)
>           originalchunks = patch.parsepatch(originaldiff)
> diff --git a/mercurial/mdiff.py b/mercurial/mdiff.py
> --- a/mercurial/mdiff.py
> +++ b/mercurial/mdiff.py
> @@ -50,7 +50,7 @@ class diffopts(object):
>           'showfunc': False,
>           'git': False,
>           'nodates': False,
> -        'nobinary': False,
> +        'nobinary': True,
>           'noprefix': False,
>           'index': 0,
>           'ignorews': False,
> diff --git a/tests/autodiff.py b/tests/autodiff.py
> --- a/tests/autodiff.py
> +++ b/tests/autodiff.py
> @@ -26,6 +26,7 @@ def autodiff(ui, repo, *pats, **opts):
>       elif git == 'auto':
>           diffopts.git = False
>           diffopts.upgrade = True
> +        diffopts.nobinary = False
>       elif git == 'warn':
>           diffopts.git = False
>           diffopts.upgrade = True
> diff --git a/tests/test-diff-binary-file.t b/tests/test-diff-binary-file.t
> --- a/tests/test-diff-binary-file.t
> +++ b/tests/test-diff-binary-file.t
> @@ -19,7 +19,7 @@
>   
>     $ hg diff --nodates -r 0 -r 2
>   
> -  $ hg diff --git -r 0 -r 1
> +  $ hg diff --git --binary -r 0 -r 1
>     diff --git a/binfile.bin b/binfile.bin
>     index 37ba3d1c6f17137d9c5f5776fa040caf5fe73ff9..58dc31a9e2f40f74ff3b45903f7d620b8e5b7356
>     GIT binary patch
> @@ -64,7 +64,7 @@
>     
>   
>   
> -  $ hg diff --git -r 2 -r 3
> +  $ hg diff --git --binary -r 2 -r 3
>     diff --git a/binfile.bin b/nonbinfile
>     copy from binfile.bin
>     copy to nonbinfile
> diff --git a/tests/test-git-export.t b/tests/test-git-export.t
> --- a/tests/test-git-export.t
> +++ b/tests/test-git-export.t
> @@ -342,7 +342,7 @@ Binary diff:
>   
>     $ cp "$TESTDIR/binfile.bin" .
>     $ hg add binfile.bin
> -  $ hg diff --git > b.diff
> +  $ hg diff --git --binary > b.diff
>     $ cat b.diff
>     diff --git a/binfile.bin b/binfile.bin
>     new file mode 100644
> diff --git a/tests/test-import-eol.t b/tests/test-import-eol.t
> --- a/tests/test-import-eol.t
> +++ b/tests/test-import-eol.t
> @@ -131,7 +131,7 @@ Test --eol and binary patches
>     $ hg ci -Am addb
>     adding b
>     $ $PYTHON -c 'file("b", "wb").write("a\x00\nc\r\nd")'
> -  $ hg diff --git > bin.diff
> +  $ hg diff --git --binary > bin.diff
>     $ hg revert --no-backup b
>   
>   binary patch with --eol
>

Code/test changes look good to me. Thanks! Looking forward to a 
finalized second version that we can run with :-)
Yuya Nishihara - April 5, 2017, 2:59 p.m.
On Wed, 5 Apr 2017 15:37:51 +0100, Ryan McElroy wrote:
> On 4/4/17 11:21 PM, Alexander Fomin wrote:
> > # HG changeset patch
> > # User Alexander Fomin <afomin@fb.com>
> > # Date 1491342655 25200
> > #      Tue Apr 04 14:50:55 2017 -0700
> > # Node ID 76f6cd23643c9816a688a1e12900a6856c5fabf6
> > # Parent  373acf7da1a621607a1cc062b96ad67a42a7e016
> > hg: make --binary option for Git diffs default to off
> 
> This patch needs a (BC) appended to its title since it changes backwards 
> compatibility.
> 
> I'd like the community to weigh in on this patch once we have a 
> land-ready version, so I'll wait for the next version before looping in 
> more people to comment/review.

I like the new behavior, but I think this is a big BC. Maybe we'll have
to wait for a compat config knob?
Ryan McElroy - April 5, 2017, 3:37 p.m.
On 4/5/17 3:59 PM, Yuya Nishihara wrote:
> On Wed, 5 Apr 2017 15:37:51 +0100, Ryan McElroy wrote:
>> On 4/4/17 11:21 PM, Alexander Fomin wrote:
>>> # HG changeset patch
>>> # User Alexander Fomin <afomin@fb.com>
>>> # Date 1491342655 25200
>>> #      Tue Apr 04 14:50:55 2017 -0700
>>> # Node ID 76f6cd23643c9816a688a1e12900a6856c5fabf6
>>> # Parent  373acf7da1a621607a1cc062b96ad67a42a7e016
>>> hg: make --binary option for Git diffs default to off
>> This patch needs a (BC) appended to its title since it changes backwards
>> compatibility.
>>
>> I'd like the community to weigh in on this patch once we have a
>> land-ready version, so I'll wait for the next version before looping in
>> more people to comment/review.
> I like the new behavior, but I think this is a big BC. Maybe we'll have
> to wait for a compat config knob?

Maybe, but it will be good to have a patch in "ready to go mode" 
regardless of whether we can accept it right now or not. Fixing up the 
series is straightforward regardless.
Alexander Fomin - April 5, 2017, 10:04 p.m.
On 05/04/2017, 07:37, "Ryan McElroy" <rm@fb.com> wrote:
> 

> On 4/4/17 11:21 PM, Alexander Fomin wrote:

> > # HG changeset patch

> > # User Alexander Fomin <afomin@fb.com>

> > # Date 1491342655 25200

> > #      Tue Apr 04 14:50:55 2017 -0700

> > # Node ID 76f6cd23643c9816a688a1e12900a6856c5fabf6

> > # Parent  373acf7da1a621607a1cc062b96ad67a42a7e016

> > hg: make --binary option for Git diffs default to off

> 

> This patch needs a (BC) appended to its title since it changes backwards

> compatibility.

Ah, sorry for that. Will be fixed.
> 

> I'd like the community to weigh in on this patch once we have a

> land-ready version, so I'll wait for the next version before looping in

> more people to comment/review.

> 

> When you resend the series, remember to add a '--flag v2' to the `hg

> email` command so that people know it's a follow-up to a previously-send

> series.

> 

> >

> > This patch makes --binary option default to off in order to avoid

> > generating binary diffs in Git mode unless explicitly requested by a

> > user (issue5510).

> 

> This isn't exactly issue5510. I'd suggest saying that this is "a

> follow-up as discussed in issue5510", or something like that.

> 

Yep, you’re right.
> >

> > diff --git a/hgext/mq.py b/hgext/mq.py

> > --- a/hgext/mq.py

> > +++ b/hgext/mq.py

> > @@ -513,6 +513,7 @@ class queue(object):

> >           diffopts = patchmod.diffopts(self.ui, opts)

> >           if self.gitmode == 'auto':

> >               diffopts.upgrade = True

> > +            diffopts.nobinary = False

> >           elif self.gitmode == 'keep':

> >               pass

> >           elif self.gitmode in ('yes', 'no'):

> > diff --git a/hgext/transplant.py b/hgext/transplant.py

> > --- a/hgext/transplant.py

> > +++ b/hgext/transplant.py

> > @@ -143,6 +143,7 @@ class transplanter(object):

> >           pulls = []

> >           diffopts = patch.difffeatureopts(self.ui, opts)

> >           diffopts.git = True

> > +        diffopts.nobinary = False

> >

> >           lock = tr = None

> >           try:

> > diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py

> > --- a/mercurial/cmdutil.py

> > +++ b/mercurial/cmdutil.py

> > @@ -162,6 +162,7 @@ def dorecord(ui, repo, commitfunc, cmdsu

> >           diffopts = patch.difffeatureopts(ui, opts=opts, whitespace=True)

> >           diffopts.nodates = True

> >           diffopts.git = True

> > +        diffopts.nobinary = False

> >           diffopts.showfunc = True

> >           originaldiff = patch.diff(repo, changes=status, opts=diffopts)

> >           originalchunks = patch.parsepatch(originaldiff)

> > diff --git a/mercurial/mdiff.py b/mercurial/mdiff.py

> > --- a/mercurial/mdiff.py

> > +++ b/mercurial/mdiff.py

> > @@ -50,7 +50,7 @@ class diffopts(object):

> >           'showfunc': False,

> >           'git': False,

> >           'nodates': False,

> > -        'nobinary': False,

> > +        'nobinary': True,

> >           'noprefix': False,

> >           'index': 0,

> >           'ignorews': False,

> > diff --git a/tests/autodiff.py b/tests/autodiff.py

> > --- a/tests/autodiff.py

> > +++ b/tests/autodiff.py

> > @@ -26,6 +26,7 @@ def autodiff(ui, repo, *pats, **opts):

> >       elif git == 'auto':

> >           diffopts.git = False

> >           diffopts.upgrade = True

> > +        diffopts.nobinary = False

> >       elif git == 'warn':

> >           diffopts.git = False

> >           diffopts.upgrade = True

> > diff --git a/tests/test-diff-binary-file.t b/tests/test-diff-binary-file.t

> > --- a/tests/test-diff-binary-file.t

> > +++ b/tests/test-diff-binary-file.t

> > @@ -19,7 +19,7 @@

> >

> >     $ hg diff --nodates -r 0 -r 2

> >

> > -  $ hg diff --git -r 0 -r 1

> > +  $ hg diff --git --binary -r 0 -r 1

> >     diff --git a/binfile.bin b/binfile.bin

> >     index 37ba3d1c6f17137d9c5f5776fa040caf5fe73ff9..58dc31a9e2f40f74ff3b45903f7d620b8e5b7356

> >     GIT binary patch

> > @@ -64,7 +64,7 @@

> >

> >

> >

> > -  $ hg diff --git -r 2 -r 3

> > +  $ hg diff --git --binary -r 2 -r 3

> >     diff --git a/binfile.bin b/nonbinfile

> >     copy from binfile.bin

> >     copy to nonbinfile

> > diff --git a/tests/test-git-export.t b/tests/test-git-export.t

> > --- a/tests/test-git-export.t

> > +++ b/tests/test-git-export.t

> > @@ -342,7 +342,7 @@ Binary diff:

> >

> >     $ cp "$TESTDIR/binfile.bin" .

> >     $ hg add binfile.bin

> > -  $ hg diff --git > b.diff

> > +  $ hg diff --git --binary > b.diff

> >     $ cat b.diff

> >     diff --git a/binfile.bin b/binfile.bin

> >     new file mode 100644

> > diff --git a/tests/test-import-eol.t b/tests/test-import-eol.t

> > --- a/tests/test-import-eol.t

> > +++ b/tests/test-import-eol.t

> > @@ -131,7 +131,7 @@ Test --eol and binary patches

> >     $ hg ci -Am addb

> >     adding b

> >     $ $PYTHON -c 'file("b", "wb").write("a\x00\nc\r\nd")'

> > -  $ hg diff --git > bin.diff

> > +  $ hg diff --git --binary > bin.diff

> >     $ hg revert --no-backup b

> >

> >   binary patch with --eol

> >

> 

> Code/test changes look good to me. Thanks! Looking forward to a

> finalized second version that we can run with :-)

Patch

diff --git a/hgext/mq.py b/hgext/mq.py
--- a/hgext/mq.py
+++ b/hgext/mq.py
@@ -513,6 +513,7 @@  class queue(object):
         diffopts = patchmod.diffopts(self.ui, opts)
         if self.gitmode == 'auto':
             diffopts.upgrade = True
+            diffopts.nobinary = False
         elif self.gitmode == 'keep':
             pass
         elif self.gitmode in ('yes', 'no'):
diff --git a/hgext/transplant.py b/hgext/transplant.py
--- a/hgext/transplant.py
+++ b/hgext/transplant.py
@@ -143,6 +143,7 @@  class transplanter(object):
         pulls = []
         diffopts = patch.difffeatureopts(self.ui, opts)
         diffopts.git = True
+        diffopts.nobinary = False
 
         lock = tr = None
         try:
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -162,6 +162,7 @@  def dorecord(ui, repo, commitfunc, cmdsu
         diffopts = patch.difffeatureopts(ui, opts=opts, whitespace=True)
         diffopts.nodates = True
         diffopts.git = True
+        diffopts.nobinary = False
         diffopts.showfunc = True
         originaldiff = patch.diff(repo, changes=status, opts=diffopts)
         originalchunks = patch.parsepatch(originaldiff)
diff --git a/mercurial/mdiff.py b/mercurial/mdiff.py
--- a/mercurial/mdiff.py
+++ b/mercurial/mdiff.py
@@ -50,7 +50,7 @@  class diffopts(object):
         'showfunc': False,
         'git': False,
         'nodates': False,
-        'nobinary': False,
+        'nobinary': True,
         'noprefix': False,
         'index': 0,
         'ignorews': False,
diff --git a/tests/autodiff.py b/tests/autodiff.py
--- a/tests/autodiff.py
+++ b/tests/autodiff.py
@@ -26,6 +26,7 @@  def autodiff(ui, repo, *pats, **opts):
     elif git == 'auto':
         diffopts.git = False
         diffopts.upgrade = True
+        diffopts.nobinary = False
     elif git == 'warn':
         diffopts.git = False
         diffopts.upgrade = True
diff --git a/tests/test-diff-binary-file.t b/tests/test-diff-binary-file.t
--- a/tests/test-diff-binary-file.t
+++ b/tests/test-diff-binary-file.t
@@ -19,7 +19,7 @@ 
 
   $ hg diff --nodates -r 0 -r 2
 
-  $ hg diff --git -r 0 -r 1
+  $ hg diff --git --binary -r 0 -r 1
   diff --git a/binfile.bin b/binfile.bin
   index 37ba3d1c6f17137d9c5f5776fa040caf5fe73ff9..58dc31a9e2f40f74ff3b45903f7d620b8e5b7356
   GIT binary patch
@@ -64,7 +64,7 @@ 
   
 
 
-  $ hg diff --git -r 2 -r 3
+  $ hg diff --git --binary -r 2 -r 3
   diff --git a/binfile.bin b/nonbinfile
   copy from binfile.bin
   copy to nonbinfile
diff --git a/tests/test-git-export.t b/tests/test-git-export.t
--- a/tests/test-git-export.t
+++ b/tests/test-git-export.t
@@ -342,7 +342,7 @@  Binary diff:
 
   $ cp "$TESTDIR/binfile.bin" .
   $ hg add binfile.bin
-  $ hg diff --git > b.diff
+  $ hg diff --git --binary > b.diff
   $ cat b.diff
   diff --git a/binfile.bin b/binfile.bin
   new file mode 100644
diff --git a/tests/test-import-eol.t b/tests/test-import-eol.t
--- a/tests/test-import-eol.t
+++ b/tests/test-import-eol.t
@@ -131,7 +131,7 @@  Test --eol and binary patches
   $ hg ci -Am addb
   adding b
   $ $PYTHON -c 'file("b", "wb").write("a\x00\nc\r\nd")'
-  $ hg diff --git > bin.diff
+  $ hg diff --git --binary > bin.diff
   $ hg revert --no-backup b
 
 binary patch with --eol