D3212: patch: implement a new worddiff algorithm

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 old

D3212: patch: implement a new worddiff algorithm

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&id=8335 R

D3212: patch: implement a new worddiff algorithm

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, merc

D3212: patch: implement a new worddiff algorithm

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 https://phab.m

D3212: patch: implement a new worddiff algorithm

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 C

D3212: patch: implement a new worddiff algorithm

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`, `diff

D3212: patch: implement a new worddiff algorithm

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 co

D3212: patch: implement a new worddiff algorithm

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 and

D3212: patch: implement a new worddiff algorithm

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 th

D3212: patch: implement a new worddiff algorithm

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

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 lis

D3212: patch: implement a new worddiff algorithm

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 dat

D3212: patch: implement a new worddiff algorithm

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

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 color][-de

D3212: patch: implement a new worddiff algorithm

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

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

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 capa

D3212: patch: implement a new worddiff algorithm

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 color.mode=aut

D3212: patch: implement a new worddiff algorithm

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

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