smarter handling of redirections and add --color option to override auto settings

[Manuel López-Ibáñez]

If diff is aliased to colordiff as suggested, then:

$ diff -aur | grep -v "^Only" > file.diff

produces a broken patch.

colordiff should be as smart as "git diff", that is, if the output is a tty, then use a pager and color, if not, then don't use color.

The attached patch does this.

Several improvements are possible:

1) Use "less -r" by default and if that fails just warn and keep STDOUT as-is.
2) Add an option like in grep:

    --color[=WHEN],
      --colour[=WHEN] use markers to highlight the matching strings;
                            WHEN is `always', `never', or `auto'
(the default is 'auto')

3) Integrate colordiff in GNU diff re-using the code used in grep.

ProblemType: Bug
DistroRelease: Ubuntu 12.04
Package: colordiff 1.0.9-1 [modified: usr/bin/colordiff]
ProcVersionSignature: Ubuntu 3.2.0-39.62-generic-pae 3.2.39
Uname: Linux 3.2.0-39-generic-pae i686
NonfreeKernelModules: fglrx
ApportVersion: 2.0.1-0ubuntu17.1
Architecture: i386
Date: Fri Mar 29 11:54:01 2013
EcryptfsInUse: Yes
InstallationMedia: Kubuntu 12.04 LTS "Precise Pangolin" - Release i386 (20120423)
MarkForUpload: True
PackageArchitecture: all
SourcePackage: colordiff
UpgradeStatus: No upgrade log present (probably fresh install)

[Manuel López-Ibáñez (manuellopezibanez)]

Updated pach implementing (2)

[davee (davee-sungate)]

You've submitted a patch against colordiff 1.0.9 which is now very old: further versions aren't making it into Ubuntu yet, because they are stuck awaiting the Debian Wheezy freeze to end. See http://www.colordiff.org/ for the latest actual release 1.0.13 and see https://github.com/daveewart/colordiff for the most recent code: the issues you report have already been addressed and fixed (at least in part, as far as I can see).

If you have a further patch, please base it on the most recent code. There are also some pending contributions (see the Pull Requests) which I've not yet had time to review.

Thanks Manuel for your contribution :-)

[Ubuntu Foundations Team Bug Bot (crichton)]

The attachment "patch for smarter enable/disable of color and use of pager" seems to be a patch. If it isn't, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are a member of the ~ubuntu-reviewers, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issues please contact him.]

[Manuel López-Ibáñez (manuellopezibanez)]

Hi davee,

I see you added an option --color, but the defaults for --color=auto are different from my patch. In the latest version:

$ diff -aur | grep -v "^Only" > file.diff

produces a broken patch, while my version does the right thing.

Also, my version will page automatically with less with colors, like git diff does.

[Manuel López-Ibáñez (manuellopezibanez)]

In my version, 'svn diff | colordiff" will work exactly as git diff "pager + colors" and "svn diff | colordiff | grep -v "^Only" > patch.diff" will do the right thing also.

In fact, one can do:

alias svndiff='svn diff | colordiff'

and get the git behaviour with correct handling of pipes.

[Manuel López-Ibáñez (manuellopezibanez)]

Anyway, if you don't like it, no offense taken. I will use my version, and rebase it if I ever need a newer version. I just found it so useful that I cannot imagine not having the automatic pager. Perhaps someone else will find this page useful too, so I upload the whole script here.

[davee (davee-sungate)]

https://github.com/daveewart/colordiff/pull/6 describes the use cases considered for the previous change which is not quite the same as yours.

I'm often finding myself fielding conflicting patches from users with different expectations about how colordiff should behave and that makes progress tricky. (There are situations where retaining colours through pipes can be helpful: some 'while'/'for' loops I've been shown, for example).

My general approach is, therefore, that unless a patch is clearly changing a *fault*, I'd prefer to leave the default behaviour untouched. With that in mind, if some colordiff users see alternative behaviours as more correct, I'm open to including additional *options* in the code, so long as the default behaviour of such options is 'no change to existing behaviour'. This should allow all users to configure the stock colordiff to their own requirements without upsetting existing users who like the existing behaviour.

This is quite a tricky balancing act and over the years I've had patches from user A asking to changing the behaviour of pipes/TTY, followed by a patch from user B *reverting those very same changes*! :-) And, not long afterwards a patch from user C with a patch similar to user A etc. :-D

I see where you're getting at with your code: remove colour from everything going to a pipe by default and enforce a pager: the current release version allows colour to a pipe precisely so that it can be piped to a pager. I see your idea behind including a call to 'less' in the code, but that in itself is a change of behaviour which might blow up the face of some other long-support expected behaviour ...

Sorry to reject the patch and I totally understand your reasons: hopefully the above explains why I can't simply consider your contribution as a direct improvement. Thanks again, Manuel; good luck with colordiff :-)

[Manuel López-Ibáñez (manuellopezibanez)]

I like my approach (and git diff approach) more because it is the least surprising:

$ diff | program > file

should produce a valid diff and

$ diff --color=yes | program > file

should produce a colored diff.

I expect that if GNU diff ever gets colored output, it will have this behaviour (like grep and git diff do).

But I fully understand that changing the defaults once set is difficult. Feel free to close this as wontfix.

[davee (davee-sungate)]

To be honest, I agree with you. There is a history of people using 'less' as the 'program' in your examples, though, which complicates things. I'm closing as WONTFIX, but I like your idea, Manuel, and am going to keep a reference to the specifics: if there's a reason to make future definitive change here, then I think this is the way to go.

Cheers :-)

[davee (davee-sungate)]

I don't have permissions to set this to WONTFIX: can someone who has such permissions please do so? Thanks...