This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6356

mdiff: prepare mdiff to be used for run-tests to replace unidiff
Needs RevisionPublic

Authored by sangeet259 on May 10 2019, 1:28 PM.

Details

Reviewers
baymax
Group Reviewers
hg-reviewers
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.

hg diff uses mdiff which which gives simpler and more readable diffs than unidiff

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.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

sangeet259 created this revision.May 10 2019, 1:28 PM
martinvonz added a subscriber: martinvonz.May 10 2019, 1:34 PM

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?

sangeet259 added a comment.May 10 2019, 1:40 PM
In 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 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.

martinvonz added a comment.May 10 2019, 1:44 PM
In D6356#92453, @sangeet259 wrote:
In 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 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?

sangeet259 edited the summary of this revision. (Show Details)May 10 2019, 2:15 PM
sangeet259 added a comment.May 10 2019, 2:16 PM

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!

pulkit added a subscriber: pulkit.May 14 2019, 1:07 PM

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.

sangeet259 added a subscriber: durin42.May 15 2019, 4:00 AM
In 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 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 D5514 .

What I am trying to do here is, once 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 D6359 to see the alternate path it follows.

pulkit added a comment.May 15 2019, 12:00 PM

Patch 1/2

we don't add that to commit messages.

mercurial/mdiff.py
531

may be worth to rename it to prepare_mdiff_input.

540

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)
544

I was unable to find any getdiff in this file.

552

nit: return itertools.chain.from_iterable(hunklines)

557

backwards compatibility with what?

562

nit: return process_mdiff(uheaders, hunks)

pulkit added a comment.May 15 2019, 12:21 PM
In D6356#92695, @sangeet259 wrote:
In 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 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 D5514 .
What I am trying to do here is, once 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 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.

sangeet259 added a comment.Jun 10 2019, 4:44 PM

@pulkit

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.

Yes, that is what I was trying to say.
But apart from that, I see the two changes as doing two logically separable things, to deserve to be broken into two commits.

Is that justified?

pulkit added a comment.Jun 10 2019, 6:41 PM
In D6356#94599, @sangeet259 wrote:

@pulkit

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.

Yes, that is what I was trying to say.
But apart from that, I see the two changes as doing two logically separable things, to deserve to be broken into two commits.
Is that justified?

To be honest, the motivation behind this patch is the next patch. It will be nice to have them as one patch, so that when in future someone does annotate, they can see the whole functionality in one patch. But I fine with accepting as separate patches too.

Also, this patch still has some un-addressed comments incase you missed them.

baymax requested changes to this revision.Jan 24 2020, 12:32 PM

There seems to have been no activities on this Diff for the past 3 Months.

By policy, we are automatically moving it out of the need-review state.

Please, move it back to need-review without hesitation if this diff should still be discussed.

:baymax:need-review-idle:

This revision now requires changes to proceed.Jan 24 2020, 12:32 PM

Revision Contents
Changeset List

PathPackages
Mmercurial/mdiff.py (36 lines)

Diff 15049

mercurial/mdiff.py

# mdiff.py - diff and patch routines for mercurial # mdiff.py - diff and patch routines for mercurial
# #
# Copyright 2005, 2006 Matt Mackall <mpm@selenic.com> # Copyright 2005, 2006 Matt Mackall <mpm@selenic.com>
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
from __future__ import absolute_import from __future__ import absolute_import
import datetime
import itertools
import re import re
import struct import struct
import zlib import zlib
from .i18n import _ from .i18n import _
from . import ( from . import (
encoding, encoding,
error, error,
def get_matching_blocks(a, b): def get_matching_blocks(a, b):
return [(d[0], d[2], d[1] - d[0]) for d in bdiff.blocks(a, b)] return [(d[0], d[2], d[1] - d[0]) for d in bdiff.blocks(a, b)]
def trivialdiffheader(length): def trivialdiffheader(length):
return struct.pack(">lll", 0, 0, length) if length else '' return struct.pack(">lll", 0, 0, length) if length else ''
def replacediffheader(oldlen, newlen): def replacediffheader(oldlen, newlen):
return struct.pack(">lll", 0, oldlen, newlen) return struct.pack(">lll", 0, oldlen, newlen)
def prepare_mdiff(expected, output):
pulkitUnsubmitted
Not Done

may be worth to rename it to prepare_mdiff_input.

pulkit: may be worth to rename it to `prepare_mdiff_input`.
"""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
pulkitUnsubmitted
Not Done

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)
pulkit: We can do ``` date = datetime.datetime.now().strftime("%a %b %d %y %H:%M:%S %Y %z") return "".
def process_mdiff(uheaders, hunks):
"""Process the output of mdiff into a list of lines,
to be used by getdiff"""
pulkitUnsubmitted
Not Done

I was unable to find any getdiff in this file.

pulkit: I was unable to find any `getdiff` in this file.
# 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
pulkitUnsubmitted
Not Done

nit: return itertools.chain.from_iterable(hunklines)

pulkit: nit: `return itertools.chain.from_iterable(hunklines)`
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.
pulkitUnsubmitted
Not Done

backwards compatibility with what?

pulkit: backwards compatibility with what?
"""
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)
pulkitUnsubmitted
Not Done

nit: return process_mdiff(uheaders, hunks)

pulkit: nit: `return process_mdiff(uheaders, hunks)`
return difflines