[PATCH 1 of 1 default] hgweb: column / side-by-side diff functionality

[Wujek Srujek]

Hi Patrick.

Thanks for your tips. How do I proceed now? I assume I fix the patch
according to your input and send it once more, right?
I am actually working on the other templates now, I also divided the patch
locally into more:
1. the core coldiff functionality, new hgrc text, but not called by anybody
yet (the templates don't know it)
2. the templates for each style, one style - one patch (the hgweb tests
will fail when this patch is applied, until you apply:
3. the 2 new tests and old tests fixed

Do you think the new queue is better, or should it all really by one patch?
(I did it because I thought the patch was too big and got lost / ignored.)

Some comments and questions below.

I would prefer to use "compare" or "comparison" rather than "diff". Other
> opinions about this?
>
> filecoldiff -> filecomp
> fullcoldiff -> fullcomparison


No problem.

I am not really convinced by the partial mode. Does it bring any
> information compared to a regular diff, besides a different formatting? For
> me, this file comparison view is really useful when diff does not give
> enough context to understand the change, in which case I prefer to have the
> full file.
>


> Compare:
>
>
> http://mezard.eu/hg/hg-does-it-look-good-for-you/diff/e1aa1ed30030/mercurial/revset.py
>
>
> http://mezard.eu/hg/hg-does-it-look-good-for-you/coldiff/e1aa1ed30030/mercurial/revset.py
>
>
> http://mezard.eu/hg/hg-does-it-look-good-for-you/coldiff/e1aa1ed30030/mercurial/revset.py?fullcoldiff=1
>
> I agree the last one would be better with a mean to navigate between
> groups.


It brings no additional information, I think, but it is just much easier to
read than regular diff, and much more concise than full side-by-side diff,
when the changes are scarce and at the end of a long file, for instance. In
your example link, one needs to scroll a long way to find the changes.
That's also because there is no UI to click to go to the next change, like
IDEs do, but I am not sure if I can do it without processing the whole diff
first, so that I can put such link at the top of a page (it is currently
streamed with generators). I think that's what you mean 'navigate between
groups'? Unless someone has another idea.
In my company, when I introduced mercurial, one of the pain-points was that
people said that they don't understand the regular diff (we use Jenkins CI
and we can see what scm changes triggered the build, and people often check
what others checked in), and showed me their old viewvc side-by-side diff.
(That's actually the reason I decided to try and implement it.) That tool
allows one to pick the full or partial mode, and since it isn't really a
big deal to have it as well in hgweb, here it is. But it can be dropped
easily ;d


> > +coldiff = filecoldiff
>
> Do we need this alias?


It's the same for diff, that's why I added that. Actually, in the
templates, the filediff is never used, it's always the alias (probably
because it's shorter?) and that's also the convention that I applied to
this patch.

Please do not import symbols from modules. It is unfortunate the two
> previous lines give a bad example but these are the only ones tolerated in
> the code base.
>
> > +from difflib import SequenceMatcher
>
> Ditto, just import difflib.


Ok.


> Note we may replace this one with our internal differ, but this could be
> done after this patch is accepted.


That's one of the thing I wanted to look into next, after I kind of finish
the current implementation. Good that someone else agreeds.

Maybe wrap:
>
>  "l%s" % leftlineno
>
> in parenthesis to avoid playing with operators priority.


Ok.


> > +            if type in ('equal'):
>
> This is the same than:
>
>  if type in 'equal':
>
> which asks if type is a substring of 'equal'. You wanted:
>
>  if type in ('equal',):
>
> or just:
>
>  if type == 'equal':


True, I already fixed it locally, with the == syntax.


> > +                for i in xrange(lhi - llo):
> > +                    yield diffline(type=type,
> > +                                   leftlineno=llo + i + 1,
> > +                                   leftline=leftlines[llo + i],
> > +                                   rightlineno=rlo + i + 1,
> > +                                   rightline=rightlines[rlo + i])
> > +            elif type == 'delete':
> > +                for i in xrange(llo, lhi):
> > +                    yield diffline(type=type,
> > +                                   leftlineno=i + 1,
> > +                                   leftline=leftlines[i],
> > +                                   rightlineno=None,
> > +                                   rightline=None)
> > +            elif type == 'insert':
> > +                for i in xrange(rlo, rhi):
> > +                    yield diffline(type=type,
> > +                                   leftlineno=None,
> > +                                   leftline=None,
> > +                                   rightlineno=i + 1,
> > +                                   rightline=rightlines[i])
> > +            else: # replace
>
> Note this case handles all the others, you can just keep this one.
>

What do you mean? Which one should I keep?

The last "autojunk" argument was added by python 2.7.1. There are ways to
> test function arguments count, but it would mean a different behaviour
> between python < 2.7.1 and >= 2.7.1. Also, this could be replaced later by
> bdiff.blocks(). Removing the last argument completely is probably the way
> to go.
>
>       s = SequenceMatcher(None, leftlines, rightlines)


Michał Sznajder, a reader of this list, already pointed it to me and I
fixed it already. Just like you say, I removed the last parameter so that
it behaves the same under all supported Pythons.


Thank you for the pointers.

wujek
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20120610/aa6711a4/attachment.html>