| Submitter | Sean Farley |
|---|---|
| Date | Dec. 31, 2016, 9:45 p.m. |
| Message ID | <e461e74f54822fef345f.1483220754@1.0.0.127.in-addr.arpa> |
| Download | mbox | patch |
| Permalink | /patch/18061/ |
| State | Superseded |
| Headers | show |
Comments
Sean Farley <sean@farley.io> writes: > # HG changeset patch > # User Sean Farley <sean@farley.io> > # Date 1483220517 21600 > # Sat Dec 31 15:41:57 2016 -0600 > # Node ID e461e74f54822fef345ffb79bc806e32f03e93fe > # Parent bd762c3d68423f7fa9e5e2b97425ed7108e0b8fc > patch: add index line for diff output > > This helps highlighting in third-party diff coloring (which assumes git > output). Is there any reason not to always output this as git does? > > The upside is that third-party tools don't need to change. The downside > is all the churn in the tests. > > > diff --git a/mercurial/patch.py b/mercurial/patch.py > --- a/mercurial/patch.py > +++ b/mercurial/patch.py > @@ -2499,10 +2499,19 @@ def trydiff(repo, revs, ctx1, ctx2, modi > text = mdiff.b85diff(content1, content2) > if text: > header.append('index %s..%s' % > (gitindex(content1), gitindex(content2))) > else: > + if opts.git: > + flag = flag1 > + if flag is None: > + flag = flag2 > + header.append('index %s..%s %s' % > + (short(gitindex(content1)), > + short(gitindex(content2)), > + gitmode[flag])) > + > text = mdiff.unidiff(content1, date1, > content2, date2, > path1, path2, opts=opts) > if header and (text or len(header) > 1): > yield '\n'.join(header) + '\n' > > diff --git a/mercurial/patch.py b/mercurial/patch.py > index 376266343330..333637393631 100644 > --- a/mercurial/patch.py > +++ b/mercurial/patch.py > @@ -2499,10 +2499,19 @@ def trydiff(repo, revs, ctx1, ctx2, modi > text = mdiff.b85diff(content1, content2) > if text: > header.append('index %s..%s' % > (gitindex(content1), gitindex(content2))) > else: > + if opts.git: > + flag = flag1 > + if flag is None: > + flag = flag2 > + header.append('index %s..%s %s' % > + (short(gitindex(content1)), > + short(gitindex(content2)), > + gitmode[flag])) > + > text = mdiff.unidiff(content1, date1, > content2, date2, > path1, path2, opts=opts) > if header and (text or len(header) > 1): > yield '\n'.join(header) + '\n' The diff is here twice because I didn't use my other patch to commit this. Whoops.
On 12/31/2016 10:45 PM, Sean Farley wrote: > # HG changeset patch > # User Sean Farley <sean@farley.io> > # Date 1483220517 21600 > # Sat Dec 31 15:41:57 2016 -0600 > # Node ID e461e74f54822fef345ffb79bc806e32f03e93fe > # Parent bd762c3d68423f7fa9e5e2b97425ed7108e0b8fc > patch: add index line for diff output TL;DR: let's make it a config option. It is usually recommended to include example of output change for such patches. The change seems to from: > diff --git a/mercurial/patch.py b/mercurial/patch.py > --- a/mercurial/patch.py > +++ b/mercurial/patch.py > @@ -2499,10 +2499,19 @@ def trydiff(repo, revs, ctx1, ctx2, modi to > diff --git a/mercurial/patch.py b/mercurial/patch.py > index 376266343330..333637393631 100644 > --- a/mercurial/patch.py > +++ b/mercurial/patch.py > This helps highlighting in third-party diff coloring (which assumes git > output). Is there any reason not to always output this as git does? Fun fact: we apparently have been doing git style diff wrong for over 10- years (and yes, the doc seems to confirm the 'index' line is required). I'm not very enthusiastic at the idea of updating our output to include this. Part of that come from the fact we have been -not- including it for the past 10 years so updating that output seems a bit bold. In addition, Mercurial does not put the idea of "index" or filenode hash forward much and from a user point of view I don't see too much value in changing that. So I would rather us to not touch the default output of 'hg diff --git' > The upside is that third-party tools don't need to change. The downside > is all the churn in the tests. Helping third party tool to work with Mercurial is definitely a large win. In addition, being able to actually produce valid git diff seems "useful". As we have a diff config section already have a [diff] section with tens of config knob there I suggest we add one for this. From reading "git diff" help there is a "full" variant of that index line. What about: [diff] extendedheader.index = [none,short,full] > diff --git a/mercurial/patch.py b/mercurial/patch.py > --- a/mercurial/patch.py > +++ b/mercurial/patch.py > @@ -2499,10 +2499,19 @@ def trydiff(repo, revs, ctx1, ctx2, modi > text = mdiff.b85diff(content1, content2) > if text: > header.append('index %s..%s' % > (gitindex(content1), gitindex(content2))) > else: > + if opts.git: > + flag = flag1 > + if flag is None: > + flag = flag2 > + header.append('index %s..%s %s' % > + (short(gitindex(content1)), > + short(gitindex(content2)), > + gitmode[flag])) > + > text = mdiff.unidiff(content1, date1, > content2, date2, > path1, path2, opts=opts) > if header and (text or len(header) > 1): > yield '\n'.join(header) + '\n' > > diff --git a/mercurial/patch.py b/mercurial/patch.py > index 376266343330..333637393631 100644 > --- a/mercurial/patch.py > +++ b/mercurial/patch.py > @@ -2499,10 +2499,19 @@ def trydiff(repo, revs, ctx1, ctx2, modi > text = mdiff.b85diff(content1, content2) > if text: > header.append('index %s..%s' % > (gitindex(content1), gitindex(content2))) > else: > + if opts.git: > + flag = flag1 > + if flag is None: > + flag = flag2 > + header.append('index %s..%s %s' % > + (short(gitindex(content1)), > + short(gitindex(content2)), > + gitmode[flag])) > + > text = mdiff.unidiff(content1, date1, > content2, date2, > path1, path2, opts=opts) > if header and (text or len(header) > 1): > yield '\n'.join(header) + '\n' > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel >
Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes: > On 12/31/2016 10:45 PM, Sean Farley wrote: >> # HG changeset patch >> # User Sean Farley <sean@farley.io> >> # Date 1483220517 21600 >> # Sat Dec 31 15:41:57 2016 -0600 >> # Node ID e461e74f54822fef345ffb79bc806e32f03e93fe >> # Parent bd762c3d68423f7fa9e5e2b97425ed7108e0b8fc >> patch: add index line for diff output > > TL;DR: let's make it a config option. Sure, that's fine with me. > It is usually recommended to include example of output change for such > patches. > > The change seems to from: > > > diff --git a/mercurial/patch.py b/mercurial/patch.py > > --- a/mercurial/patch.py > > +++ b/mercurial/patch.py > > @@ -2499,10 +2499,19 @@ def trydiff(repo, revs, ctx1, ctx2, modi > > to > > > diff --git a/mercurial/patch.py b/mercurial/patch.py > > index 376266343330..333637393631 100644 > > --- a/mercurial/patch.py > > +++ b/mercurial/patch.py Oh, good point, sorry I forgot. >> This helps highlighting in third-party diff coloring (which assumes git >> output). Is there any reason not to always output this as git does? > > Fun fact: we apparently have been doing git style diff wrong for over > 10- years (and yes, the doc seems to confirm the 'index' line is required). Ha! > I'm not very enthusiastic at the idea of updating our output to include > this. Part of that come from the fact we have been -not- including it > for the past 10 years so updating that output seems a bit bold. In > addition, Mercurial does not put the idea of "index" or filenode hash > forward much and from a user point of view I don't see too much value in > changing that. Yeah, I would tend to agree here. Bit of a shame but not a big deal. > So I would rather us to not touch the default output of 'hg diff --git' > >> The upside is that third-party tools don't need to change. The downside >> is all the churn in the tests. > > Helping third party tool to work with Mercurial is definitely a large > win. In addition, being able to actually produce valid git diff seems > "useful". > > As we have a diff config section already have a [diff] section with tens > of config knob there I suggest we add one for this. From reading "git > diff" help there is a "full" variant of that index line. What about: > > [diff] > extendedheader.index = [none,short,full] Ok, that makes sense to me. There are also other headers (rename similarity comes to mind).
On Fri, Jan 06, 2017 at 11:45:28AM -0800, Sean Farley wrote: > Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes: > > > On 12/31/2016 10:45 PM, Sean Farley wrote: > >> # HG changeset patch > >> # User Sean Farley <sean@farley.io> > >> # Date 1483220517 21600 > >> # Sat Dec 31 15:41:57 2016 -0600 > >> # Node ID e461e74f54822fef345ffb79bc806e32f03e93fe > >> # Parent bd762c3d68423f7fa9e5e2b97425ed7108e0b8fc > >> patch: add index line for diff output > > > > TL;DR: let's make it a config option. > > Sure, that's fine with me. > > > It is usually recommended to include example of output change for such > > patches. > > > > The change seems to from: > > > > > diff --git a/mercurial/patch.py b/mercurial/patch.py > > > --- a/mercurial/patch.py > > > +++ b/mercurial/patch.py > > > @@ -2499,10 +2499,19 @@ def trydiff(repo, revs, ctx1, ctx2, modi > > > > to > > > > > diff --git a/mercurial/patch.py b/mercurial/patch.py > > > index 376266343330..333637393631 100644 > > > --- a/mercurial/patch.py > > > +++ b/mercurial/patch.py > > Oh, good point, sorry I forgot. > > >> This helps highlighting in third-party diff coloring (which assumes git > >> output). Is there any reason not to always output this as git does? > > > > Fun fact: we apparently have been doing git style diff wrong for over > > 10- years (and yes, the doc seems to confirm the 'index' line is required). > > Ha! > > > I'm not very enthusiastic at the idea of updating our output to include > > this. Part of that come from the fact we have been -not- including it > > for the past 10 years so updating that output seems a bit bold. In > > addition, Mercurial does not put the idea of "index" or filenode hash > > forward much and from a user point of view I don't see too much value in > > changing that. > > Yeah, I would tend to agree here. Bit of a shame but not a big deal. I'm kind of +0 on adding the index lines, since the docs on git-format extended diffs document it, we'll get a lot of nice parser integration for free by doing this "right". Sigh. > > > So I would rather us to not touch the default output of 'hg diff --git' > > > >> The upside is that third-party tools don't need to change. The downside > >> is all the churn in the tests. > > > > Helping third party tool to work with Mercurial is definitely a large > > win. In addition, being able to actually produce valid git diff seems > > "useful". > > > > As we have a diff config section already have a [diff] section with tens > > of config knob there I suggest we add one for this. From reading "git > > diff" help there is a "full" variant of that index line. What about: > > > > [diff] > > extendedheader.index = [none,short,full] > > Ok, that makes sense to me. There are also other headers (rename > similarity comes to mind). If we go this route, please include prominent documentation that to produce pedantically correct git diffs this config knob needs to be specified. (Also consider dropping this on my FriendlyHgPlan page as something we should make part of FriendlyHg...) > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Patch
diff --git a/mercurial/patch.py b/mercurial/patch.py --- a/mercurial/patch.py +++ b/mercurial/patch.py @@ -2499,10 +2499,19 @@ def trydiff(repo, revs, ctx1, ctx2, modi text = mdiff.b85diff(content1, content2) if text: header.append('index %s..%s' % (gitindex(content1), gitindex(content2))) else: + if opts.git: + flag = flag1 + if flag is None: + flag = flag2 + header.append('index %s..%s %s' % + (short(gitindex(content1)), + short(gitindex(content2)), + gitmode[flag])) + text = mdiff.unidiff(content1, date1, content2, date2, path1, path2, opts=opts) if header and (text or len(header) > 1): yield '\n'.join(header) + '\n' diff --git a/mercurial/patch.py b/mercurial/patch.py index 376266343330..333637393631 100644 --- a/mercurial/patch.py +++ b/mercurial/patch.py @@ -2499,10 +2499,19 @@ def trydiff(repo, revs, ctx1, ctx2, modi text = mdiff.b85diff(content1, content2) if text: header.append('index %s..%s' % (gitindex(content1), gitindex(content2))) else: + if opts.git: + flag = flag1 + if flag is None: + flag = flag2 + header.append('index %s..%s %s' % + (short(gitindex(content1)), + short(gitindex(content2)), + gitmode[flag])) + text = mdiff.unidiff(content1, date1, content2, date2, path1, path2, opts=opts) if header and (text or len(header) > 1): yield '\n'.join(header) + '\n'