| 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
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 :-)
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?
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.
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