search

daveewart / colordiff

Primary development for colordiff
http://www.colordiff.org/
GNU General Public License v2.0
177 stars 37 forks source link

do not colorize diff if output piped to other program and --color=auto #27

Closed andrey-starodubtsev closed 7 years ago

andrey-starodubtsev commented 9 years ago

Without this change svn diff | colordiff | view - contains escape symbols for colorizing

daveewart commented 9 years ago

Hi Andrey: can you explain the issue you're trying to fix here? What is 'view' in your command line: my understanding is it's the read-only Vim editor - is that what you're doing, sending diff output into Vim? If so, why use colordiff in that pipe at all, what does it achieve if you're wanting to throw away the colours? Alternatively, you can use 'less -r' to page the coloured output.

Bit of background information regarding the development history: I'm reluctant to change the pipe/file output behaviour of colordiff because it has been toggled a few times in the past, only to cause problems for those people who think it should work The Other Way! :-)

andrey-starodubtsev commented 9 years ago

Hi! I use colordiff for small diffs which fit in one-two screens and use view aka vim -R as pager. Do not use less for paging usually cause vim makes it easier to copy part of diff, make advanced search, filter redundant/uninteresting parts of patch etc. But this patch helps in other situations as well - let's suppose I want somewhat filter patch before redirecting it to file, i.e. svn diff | colordiff | dos2unix > 1 - in that case for --color=auto it would be more intuitive (I suppose =) ) to check if output device is terminal or not instead of is it file or not. And last but not least - grep --color=auto, ls -G (MacOSX version) and ack work the same way - ack use terminal colors if output is terminal (-t) or it works on Windows (MSWindows uses different techinque to colorize ouput in console)

daveewart commented 9 years ago

Unfortunately I'm not prepared to break functionality of piping colordiff output into less, because that is an incredibly common use case. Therefore I won't be accepting this patch.

If you wish to avoid using colours in your example svn diff | colordiff | dos2unix ... then surely you should omit colordiff entirely, i.e. svn diff | dos2unix ... ? The addition of colordiff to the pipe adds absolutely nothing if you are expecting it (via your patch) to not output colours in this context!

I take your point that other utilities may behave in the manner you expect, but the fact remains that colordiff is a tool expressly designed to add colour to output (unlike those other utilities where colour is an ancillary role) and it is commonly used as I've indicated to do exactly that. That being the case, I always err on the side of "add colour" rather than "don't add colour".

andrey-starodubtsev commented 9 years ago

I see your point. May I suggest additional option to change this behavior, something which can be put in colordiffrc or create alias with? Something like '--do-not-colorize-if-output-is-not-term'... to be honest, I'm not too good with option names =)

daveewart commented 9 years ago

No harm with an extra option, I suppose: I'll put something in place. As per usual practices, of course, the default behaviour will be the existing behaviour.

daveewart commented 7 years ago

Apologies for letting this one stay idle for so long.

Development moved on and so I can't merge this directly, however please see new option

colordiff --color-term-output-only=yes

which should work as you require. Latest commit on master branch includes this in colordiff.pl, docs not yet updated but will be before formal 1.0.17 release.