Patchwork D6356: mdiff: prepare mdiff to be used for run-tests to replace unidiff

login
register
mail settings
Submitter phabricator
Date May 10, 2019, 5:28 p.m.
Message ID <differential-rev-PHID-DREV-bllgrg5mbv6xxsrub5oe-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/39996/
State New
Headers show

Comments

phabricator - May 10, 2019, 5:28 p.m.
sangeet259 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Patch 1/2
  
  This patch adds some functions to mdiff.py that will be called from run-tests.py
  which will be added in the second part of this patch series.
  
  The future changes to run-tests.py will enable one to use mdiif
  to diff rather than unidiff whenever the mercurial is installed in the system.
  
  Why do I need to split the patches into two ?
  The reason to split what would have been a single patch into two is because
  for the next patch to be able to use the mdiff during tests,
  this revision has to be there in the system's mercurial installation.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6356

AFFECTED FILES
  mercurial/mdiff.py

CHANGE DETAILS




To: sangeet259, #hg-reviewers
Cc: mercurial-devel
phabricator - May 10, 2019, 5:34 p.m.
martinvonz added a comment.


  > The future changes to run-tests.py will enable one to use mdiif
  >  to diff rather than unidiff whenever the mercurial is installed in the system.
  
  Perhaps a naive question, but why is that desirable? How will I (as someone running tests) notice?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6356

To: sangeet259, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - May 10, 2019, 5:40 p.m.
sangeet259 added a comment.


  In https://phab.mercurial-scm.org/D6356#92451, @martinvonz wrote:
  
  > > The future changes to run-tests.py will enable one to use mdiif
  > >  to diff rather than unidiff whenever the mercurial is installed in the system.
  >
  > Perhaps a naive question, but why is that desirable? How will I (as someone running tests) notice?
  
  
  @martinvonz 
  This patch is a breakdown of the earlier patch https://phab.mercurial-scm.org/D5514
  
  There <https://phab.mercurial-scm.org/D5514> I have detailed why is this required.
  In short, the diff output of tests is a bit ugly compared to what hg diff does, which is much nicer.
  
  I am thinking of putting that descriptive commit message in the patch 2/2.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6356

To: sangeet259, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - May 10, 2019, 5:44 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D6356#92453, @sangeet259 wrote:
  
  > In https://phab.mercurial-scm.org/D6356#92451, @martinvonz wrote:
  >
  > > > The future changes to run-tests.py will enable one to use mdiif
  > > >  to diff rather than unidiff whenever the mercurial is installed in the system.
  > >
  > > Perhaps a naive question, but why is that desirable? How will I (as someone running tests) notice?
  >
  >
  > @martinvonz 
  >  This patch is a breakdown of the earlier patch https://phab.mercurial-scm.org/D5514
  
  
  Oh, nice! I sometimes use `run-tests.py -i` and just accept an unreadable diff because I know that `hg diff` might make it easier to read. I had not understood why :)
  
  > There <https://phab.mercurial-scm.org/D5514> I have detailed why is this required.
  >  In short, the diff output of tests is a bit ugly compared to what hg diff does, which is much nicer.
  > 
  > I am thinking of putting that descriptive commit message in the patch 2/2.
  
  Perhaps also adding a "which gives simpler diffs" or something like that to this patch?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6356

To: sangeet259, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - May 10, 2019, 6:16 p.m.
sangeet259 added a comment.


  > Oh, nice! I sometimes use run-tests.py -i and just accept an unreadable diff because I know that hg diff might make it easier to read. I had not understood why :)
  
  Glad to know you will be helped :)
  
  Added some description to the commit message!

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6356

To: sangeet259, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - May 14, 2019, 5:07 p.m.
pulkit added a comment.


  > Why do I need to split the patches into two ?
  >  The reason to split what would have been a single patch into two is because
  >  for the next patch to be able to use the mdiff during tests,
  >  this revision has to be there in the system's mercurial installation.
  
  I didn't get the last line. Can you explain more?
  
  I think both can be folded in one and should be good.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6356

To: sangeet259, #hg-reviewers
Cc: pulkit, martinvonz, mercurial-devel
phabricator - May 15, 2019, 8 a.m.
sangeet259 added a subscriber: durin42.
sangeet259 added a comment.


  In https://phab.mercurial-scm.org/D6356#92644, @pulkit wrote:
  
  > > Why do I need to split the patches into two ?
  > >  The reason to split what would have been a single patch into two is because
  > >  for the next patch to be able to use the mdiff during tests,
  > >  this revision has to be there in the system's mercurial installation.
  >
  > I didn't get the last line. Can you explain more?
  >
  > I think both can be folded in one and should be good.
  
  
  @pulkit 
  Well the next patch imports mercurial and then calls `mdff.new_diff` . If I combine those two patches then you can not see the change as while testing https://phab.mercurial-scm.org/D6359 , if there `mdiff` doesn't already have new_diff 
  it will always fall back to unified_diff as happened with @durin42  , while he was trying to see the alternate path as he trying to test https://phab.mercurial-scm.org/D5514 .
  
  What I am trying to do here is, once https://phab.mercurial-scm.org/D6356 is has added `new_diff` to `mdiff`, you can then have this revision installed in your system or wherever one tests this, and then check https://phab.mercurial-scm.org/D6359 to see the alternate path it follows.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6356

To: sangeet259, #hg-reviewers
Cc: durin42, pulkit, martinvonz, mercurial-devel
phabricator - May 15, 2019, 4 p.m.
pulkit added a comment.


  > Patch 1/2
  
  we don't add that to commit messages.

INLINE COMMENTS

> mdiff.py:531
> +
> +def prepare_mdiff(expected, output):
> +    """Prepare the inputs for the mdiff.unidiff function"""

may be worth to rename it to `prepare_mdiff_input`.

> mdiff.py:540
> +    opts = diffopts(noprefix=True)
> +    return exp, date1, out, date2, opts
> +

We can do

  date = datetime.datetime.now().strftime("%a %b %d %y %H:%M:%S %Y %z")
  return "".join(expected), date, "".join(output), date, diffopts(noprefix=True)

> mdiff.py:544
> +    """Process the output of mdiff into a list of lines,
> +    to be used by getdiff"""
> +    # the hunklines are in the hunks generator

I was unable to find any `getdiff` in this file.

> mdiff.py:552
> +    difflines = itertools.chain.from_iterable(hunklines)
> +    return difflines
> +

nit: `return itertools.chain.from_iterable(hunklines)`

> mdiff.py:557
> +    The API of new_diff is designed to be same as difflib.unified_diff.
> +    This is done for backwards compatibility and resuing existing code.
> +    """

backwards compatibility with what?

> mdiff.py:562
> +                            ref, err, binary=False, opts=opts)
> +    difflines = process_mdiff(uheaders, hunks)
> +    return difflines

nit: `return process_mdiff(uheaders, hunks)`

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6356

To: sangeet259, #hg-reviewers
Cc: durin42, pulkit, martinvonz, mercurial-devel
phabricator - May 15, 2019, 4:21 p.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D6356#92695, @sangeet259 wrote:
  
  > In https://phab.mercurial-scm.org/D6356#92644, @pulkit wrote:
  >
  > > > Why do I need to split the patches into two ?
  > > >  The reason to split what would have been a single patch into two is because
  > > >  for the next patch to be able to use the mdiff during tests,
  > > >  this revision has to be there in the system's mercurial installation.
  > >
  > > I didn't get the last line. Can you explain more?
  > >
  > > I think both can be folded in one and should be good.
  >
  >
  > @pulkit 
  >  Well the next patch imports mercurial and then calls `mdff.new_diff` . If I combine those two patches then you can not see the change as while testing https://phab.mercurial-scm.org/D6359 , if there `mdiff` doesn't already have new_diff 
  >  it will always fall back to unified_diff as happened with @durin42  , while he was trying to see the alternate path as he trying to test https://phab.mercurial-scm.org/D5514 .
  >
  > What I am trying to do here is, once https://phab.mercurial-scm.org/D6356 is has added `new_diff` to `mdiff`, you can then have this revision installed in your system or wherever one tests this, and then check https://phab.mercurial-scm.org/D6359 to see the alternate path it follows.
  
  
  I was trying to test it and unable to get this to work both with a single patch and two splitted patches.
  
  My understanding is that the 1st patch should be present in the mercurial installed in the system, which is what you said right? If that's correct, we can go ahead and have this whole as one patch as I don't think people usually do install system hg on patch to patch basis.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6356

To: sangeet259, #hg-reviewers
Cc: durin42, pulkit, martinvonz, mercurial-devel

Patch

diff --git a/mercurial/mdiff.py b/mercurial/mdiff.py
--- a/mercurial/mdiff.py
+++ b/mercurial/mdiff.py
@@ -7,6 +7,8 @@ 
 
 from __future__ import absolute_import
 
+import datetime
+import itertools
 import re
 import struct
 import zlib
@@ -525,3 +527,37 @@ 
 
 def replacediffheader(oldlen, newlen):
     return struct.pack(">lll", 0, oldlen, newlen)
+
+def prepare_mdiff(expected, output):
+    """Prepare the inputs for the mdiff.unidiff function"""
+    date1 = datetime.datetime.now().strftime("%a %b %d %y %H:%M:%S %Y %z")
+    date2 = date1
+    # join all the different elements into a single string
+    # to regenerate that file
+    exp = "".join(expected)
+    out = "".join(output)
+    opts = diffopts(noprefix=True)
+    return exp, date1, out, date2, opts
+
+def process_mdiff(uheaders, hunks):
+    """Process the output of mdiff into a list of lines,
+    to be used by getdiff"""
+    # the hunklines are in the hunks generator
+    # get the hunklines from the (hunkrange,hunklines) tuple
+    hunklines = [x[1] for x in hunks]
+    # extract and insert the headers at the beginnng of the hunklines list
+    headers = [uheaders[0].split("\t")[0]+"\n", uheaders[1].split("\t")[0]+"\n"]
+    hunklines.insert(0, headers)
+    difflines = itertools.chain.from_iterable(hunklines)
+    return difflines
+
+def new_diff(expected, output, ref, err):
+    """
+    The API of new_diff is designed to be same as difflib.unified_diff.
+    This is done for backwards compatibility and resuing existing code.
+    """
+    exp, date1, out, date2, opts = prepare_mdiff(expected, output)
+    uheaders, hunks = unidiff(exp, date1, out, date2,
+                            ref, err, binary=False, opts=opts)
+    difflines = process_mdiff(uheaders, hunks)
+    return difflines