D3212: patch: implement a new worddiff algorithm

2018-04-17 Thread lothiraldan (Boris Feld)
lothiraldan added a comment. In https://phab.mercurial-scm.org/D3212#52767, @quark wrote: > F71015: 2018-04-12-193346_956x996_scrot.png  for git inspired colored output. I gave a try to the dim version and it was less readable than the

D3212: patch: implement a new worddiff algorithm

2018-04-16 Thread quark (Jun Wu)
This revision was automatically updated to reflect the committed changes. Closed by commit rHG35632d392279: patch: implement a new worddiff algorithm (authored by quark, committed by ). REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D3212?vs=7924=8335

D3212: patch: implement a new worddiff algorithm

2018-04-16 Thread durin42 (Augie Fackler)
durin42 added a comment. (I'll say that red-dimmed text is very hard for this moderate protan with a black terminal window.) REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3212 To: quark, #hg-reviewers, durin42, yuja Cc: indygreg, dhduvall, yuja, spectral,

D3212: patch: implement a new worddiff algorithm

2018-04-16 Thread durin42 (Augie Fackler)
durin42 added a comment. In the name of progress, I'm going to land these. If we find problems with color defaults, let's try and fix them in the RC period, but it's still an experimental feature so we've got plenty of wiggle room. REPOSITORY rHG Mercurial REVISION DETAIL

D3212: patch: implement a new worddiff algorithm

2018-04-12 Thread dhduvall (Danek Duvall)
dhduvall added a comment. I can't really tell what's going on there, frankly, unlike with the dim case, where it make sense, but I can always change the colors myself. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3212 To: quark, #hg-reviewers, durin42, yuja

D3212: patch: implement a new worddiff algorithm

2018-04-12 Thread quark (Jun Wu)
quark added a comment. I think one alternative is just to use `green_background` like `git/contrib/diff-highlight/diff-highlight.perl`. It satisfies all properties I'd like to have, and is supported by weird terminals including cmd.exe and less.exe. And is different from `diff.file_a`,

D3212: patch: implement a new worddiff algorithm

2018-04-12 Thread spectral (Kyle Lippincott)
spectral added inline comments. INLINE COMMENTS > dhduvall wrote in color.py:94 > I'm not sure what testing I did to claim "most", but I did make that comment > in 2011, and xterm didn't start supporting dim until 2014 (it had long > supported invisible), so that certainly helped inform that

D3212: patch: implement a new worddiff algorithm

2018-04-12 Thread dhduvall (Danek Duvall)
dhduvall added inline comments. INLINE COMMENTS > quark wrote in color.py:94 > There are not many choices - dim, 16/256 colors, or bold. We ended up with > 16/256 colors internally for wider support (ex. tmux). But I'd like to > express my (strong) options that: > > - diff.inserted.changed

D3212: patch: implement a new worddiff algorithm

2018-04-12 Thread yuja (Yuya Nishihara)
yuja added a comment. > Note the color configs are not entirely equivalent to the old code. I know. It would be roughly equivalent to the old code of `sim >= 0.0`. > Therefore I still think it's reasonable to avoid using "bold" in this patch to avoid noisy outputs. I agree on

D3212: patch: implement a new worddiff algorithm

2018-04-11 Thread quark (Jun Wu)
quark added a comment. In https://phab.mercurial-scm.org/D3212#51917, @yuja wrote: > Can you split a patch changing the color scheme so we can easily > back it out as needed? Note the color configs are not entirely equivalent to the old code. To give an example: -LINE-1

D3212: patch: implement a new worddiff algorithm

2018-04-11 Thread yuja (Yuya Nishihara)
yuja added a comment. > That said, I'm fine with changing the defaults to whatever. So feel > free to send follow-ups changing it. Can you split a patch changing the color scheme so we can easily back it out as needed? INLINE COMMENTS > quark wrote in patch.py:2536 > For a split

D3212: patch: implement a new worddiff algorithm

2018-04-10 Thread quark (Jun Wu)
quark added inline comments. INLINE COMMENTS > spectral wrote in color.py:94 > I understand the reasoning for wanting `diff.inserted` and > `diff.inserted.changed` to be the same color, but unfortunately I think it > might not be super feasible in upstream. > > 'dim' support in my terminfo

D3212: patch: implement a new worddiff algorithm

2018-04-10 Thread spectral (Kyle Lippincott)
spectral added subscribers: dhduvall, indygreg. spectral added inline comments. INLINE COMMENTS > quark wrote in color.py:94 > As I mentioned in the summary, I believe `diff.inserted` and > `diff.inserted.changed` should have a same color. And `diff.inserted` > probably shouldn't be bold. > >

D3212: patch: implement a new worddiff algorithm

2018-04-10 Thread quark (Jun Wu)
quark added a comment. Git first had a `contrib/diff-highlight/diff-highlight` script which inverts foreground/background for hunks with len(deleted_lines) = len(inserted_lines). Then the latest version shows diff inline. That is: common words [+inserted words with green

D3212: patch: implement a new worddiff algorithm

2018-04-10 Thread yuja (Yuya Nishihara)
yuja added a comment. BTW, what's the default of git? The comment in color.py is a bit scary, which says "most terminals don't support dim." If that's true, we shouldn't use "dim" by default. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3212 To: quark,

D3212: patch: implement a new worddiff algorithm

2018-04-10 Thread yuja (Yuya Nishihara)
yuja requested changes to this revision. yuja added a comment. This revision now requires changes to proceed. I have no opinion about the "dim" thingy, but the series generally looks good to me. Thanks for tackling on the painfully slow `SequenceMatcher.ratio()` issue. INLINE COMMENTS

D3212: patch: implement a new worddiff algorithm

2018-04-09 Thread quark (Jun Wu)
quark added inline comments. INLINE COMMENTS > spectral wrote in color.py:94 > These are the first uses of 'dim' in the default set of things, and I don't > think we can rely on it; for color.mode=auto, we really mean "ansi" (aka > ecma48) (unless on windows), and don't do any detection of

D3212: patch: implement a new worddiff algorithm

2018-04-09 Thread spectral (Kyle Lippincott)
spectral added inline comments. INLINE COMMENTS > color.py:94 > +'diff.deleted.changed': 'red', > +'diff.deleted.unchanged': 'red dim', > 'diff.diffline': 'bold', These are the first uses of 'dim' in the default set of things, and I don't think we can rely on it; for

D3212: patch: implement a new worddiff algorithm

2018-04-09 Thread quark (Jun Wu)
quark added a comment. This is the before and after comparison: F69700: worddiff-compare.png REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3212 To: quark, #hg-reviewers, durin42 Cc: mercurial-devel

D3212: patch: implement a new worddiff algorithm

2018-04-09 Thread quark (Jun Wu)
quark created this revision. Herald added a subscriber: mercurial-devel. Herald added a reviewer: hg-reviewers. REVISION SUMMARY The previous worddiff algorithm has many problems. The major problem is it does a "similarity check" that selects a subset of matched lines to do inline diffs. It