Patchwork [RFC] patch: add index line for diff output

login
register
mail settings
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 - Dec. 31, 2016, 9:45 p.m.
# 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.
Sean Farley - Dec. 31, 2016, 9:48 p.m.
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.
Pierre-Yves David - Jan. 6, 2017, 6:50 a.m.
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
>
Sean Farley - Jan. 6, 2017, 7:45 p.m.
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).
Augie Fackler - Jan. 7, 2017, 5:54 p.m.
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'