Bug#378563: colordiff: causes lossage if the diff is redirected
On Thursday, 20.07.2006 at 01:05 +0200, Adam Borowski wrote:
+exec /usr/bin/diff,@ARGV unless -t STDOUT;
It's not as simple as that, though: the above will fail if colordiff
(or 'diff' if aliased to colordiff) is used in a pipe. In other
words, this is an incomplete fix.
Uhm, isn't the whole point of the fix to correct just this issue?
If the output isn't redirected, -t will be true and colordiff will
continue.
No. The above refers to colordiff *with your patch applied*. It fails
in the way I described. However, see below...
In detail...
i.e. (assuming 'alias diff=colordiff')
diff file1 file2 - this will work and colour-highlight the output
diff file1 file2 my.patch - this will *not* colour-highlight, as intended
but
someprog | diff my.patch - this will give an error
/usr/bin/diff: missing operand after `/usr/bin/diff'
/usr/bin/diff: Try `/usr/bin/diff --help' for more information.
So it's working correctly:
/usr/bin/diff
/usr/bin/diff: missing operand after `/usr/bin/diff'
/usr/bin/diff: Try `/usr/bin/diff --help' for more information.
You need to always specify at least two arguments to diff, one of
which can be -. With or without colordiff.
The difficulty here is that colordiff has the functionality (which is in
*addition* to what diff provides) in that it can colourise diff-style
output fed to it via a pipe, i.e.
cvs diff | colordiff
Clearly the above syntax will fail when using /usr/bin/diff in place of
colordiff, and in fact makes no logical sense anyway.
My question is therefore: can colordiff be made to (at least) not break
when used in the following format?
cvs diff | colordiff my.patch
The above statement will (a) fail with an error with your patch or (b)
put colour codes into the patch file without your patch. I can't see
any easy way to make it do (c) drop the colours and be essentially a
'dummy' filter, just dropping everything into the my.patch. And, to be
honest, I'm sure there's any *point* in supporting (c).
Given this, and the fact that colordiff's entire raison d'être is to
colour highlight, my feeling is that a better option is this: rather
than try to make colordiff *avoid* highlighting in certain
circumstances, this limitation is explained in the man page.
In this case, you can't recommend aliasing 'diff' to 'colordiff' then.
I think it's perfectly reasonable to suggest aliasing in this way, so
long as it is explicitly explained that doing so is a trade-off: you
may need to refer to /usr/bin/diff explicitly when trying to *avoid*
colourising or similar.
A documentation change might actually be a more thorough solution:
it will make the user know what they're letting themselves in for
by aliasing 'diff' to 'colordiff'.
I would say there are two choices:
1) allow aliasing and then handle the case of !isatty(1)
2) tell people they _can't_ use the alias.
Leaving this as documentation only would work, too -- but if
someone makes the alias anyway, they will lose the moment they try
to make a patch.
You can't *tell* people they can or can't use the alias, one can
only choose to *recommend* they make such an alias, or to *not*
recommend they make such an alias :-)
In my opinion, leaving the functionality as is is probably the best
solution and here's my reasoning:
1. Applying your patch means that it will become *impossible* to
redirect *colourised* output to a file. I realise this is what your
patch is trying to detect and avoid, but some people might want to do
this, and colordiff is a reasonable tool to enable them to do so!
2. Explaining the double-edged nature of aliasing 'diff' to 'colordiff'
in the documentation is probably the best way to make users aware of
this issue: the documentation can state that users may find making an
alias convenient, but that they need to be aware that aliasing it to
*diff* has repercussions; perhaps one could recommend aliasing it to
something else instead ('cdiff', 'ciff', 'diffc' etc.) In other words,
leave it up to the user.
Are we approaching a consensus now, Adam? ;-)
Dave.
--
Dave Ewart - [EMAIL PROTECTED] - jabber: [EMAIL PROTECTED] - freenode: davee
All email from me is now digitally signed, key from http://www.sungate.co.uk/
Fingerprint: AEC5 9360 0A35 7F66 66E9 82E4 9E10 6769 CD28 DA92
signature.asc
Description: Digital signature
Bug#378563: colordiff: causes lossage if the diff is redirected
On Monday, 17.07.2006 at 22:19 +0200, Adam Borowski wrote: --- colordiff~ 2006-07-17 15:01:56.564810563 +0200 +++ colordiff 2006-07-17 15:01:56.564810563 +0200 @@ -31,6 +31,8 @@ use Getopt::Long qw(:config pass_through); use IPC::Open2; +exec /usr/bin/diff,@ARGV unless -t STDOUT; + my $app_name = 'colordiff'; my $version = '1.0.4'; my $author = 'Dave Ewart'; (Of course, the exact place doesn't really matter.) It's not as simple as that, though: the above will fail if colordiff (or 'diff' if aliased to colordiff) is used in a pipe. In other words, this is an incomplete fix. In detail... i.e. (assuming 'alias diff=colordiff') diff file1 file2 - this will work and colour-highlight the output diff file1 file2 my.patch - this will *not* colour-highlight, as intended but someprog | diff my.patch - this will give an error /usr/bin/diff: missing operand after `/usr/bin/diff' /usr/bin/diff: Try `/usr/bin/diff --help' for more information. Given this, and the fact that colordiff's entire raison d'être is to colour highlight, my feeling is that a better option is this: rather than try to make colordiff *avoid* highlighting in certain circumstances, this limitation is explained in the man page. A documentation change might actually be a more thorough solution: it will make the user know what they're letting themselves in for by aliasing 'diff' to 'colordiff'. Thoughts on that, Adam? I already reported this bug upstream over three years ago, but it appears to be left unfixed. At least, the bug bit me today again :p [Odd - I've got no record of you ever contacting me, which is strange since I'm usually fairly obsessive about keeping that sort of email. Did I ever reply?! Apologies if not, but I don't think I got your message.] I'm afraid I didn't use mail, just the SourceForge bug report system: https://sourceforge.net/tracker/?func=detailaid=745456group_id=65829atid=512406 I'm not sure if they mail you any new bug reports by default; I would probably fail to notice such reports on my projects for years, too. Well, that's news to me - never got any notifications of those bug reports. I'll go take a look at them ... Dave. -- Dave Ewart - [EMAIL PROTECTED] - jabber: [EMAIL PROTECTED] - freenode: davee All email from me is now digitally signed, key from http://www.sungate.co.uk/ Fingerprint: AEC5 9360 0A35 7F66 66E9 82E4 9E10 6769 CD28 DA92 signature.asc Description: Digital signature
Bug#378563: colordiff: causes lossage if the diff is redirected
On Wed, Jul 19, 2006 at 07:24:40PM +0100, Dave Ewart wrote: +exec /usr/bin/diff,@ARGV unless -t STDOUT; It's not as simple as that, though: the above will fail if colordiff (or 'diff' if aliased to colordiff) is used in a pipe. In other words, this is an incomplete fix. Uhm, isn't the whole point of the fix to correct just this issue? If the output isn't redirected, -t will be true and colordiff will continue. In detail... i.e. (assuming 'alias diff=colordiff') diff file1 file2 - this will work and colour-highlight the output diff file1 file2 my.patch - this will *not* colour-highlight, as intended but someprog | diff my.patch - this will give an error /usr/bin/diff: missing operand after `/usr/bin/diff' /usr/bin/diff: Try `/usr/bin/diff --help' for more information. So it's working correctly: /usr/bin/diff /usr/bin/diff: missing operand after `/usr/bin/diff' /usr/bin/diff: Try `/usr/bin/diff --help' for more information. You need to always specify at least two arguments to diff, one of which can be -. With or without colordiff. Given this, and the fact that colordiff's entire raison d'être is to colour highlight, my feeling is that a better option is this: rather than try to make colordiff *avoid* highlighting in certain circumstances, this limitation is explained in the man page. In this case, you can't recommend aliasing 'diff' to 'colordiff' then. A documentation change might actually be a more thorough solution: it will make the user know what they're letting themselves in for by aliasing 'diff' to 'colordiff'. I would say there are two choices: 1) allow aliasing and then handle the case of !isatty(1) 2) tell people they _can't_ use the alias. Leaving this as documentation only would work, too -- but if someone makes the alias anyway, they will lose the moment they try to make a patch. Regards, -- 1KB // Microsoft corollary to Hanlon's razor: // Never attribute to stupidity what can be // adequately explained by malice. -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#378563: colordiff: causes lossage if the diff is redirected
Package: colordiff Version: 1.0.4-4 Severity: important Tags: patch Hello. If the output is not a terminal (like, when using diff to make a patch), colordiff will still happily colorize the output. This is bad if diff is an alias to colordiff, as suggested in the manpage. The fix is simple: exec /usr/bin/diff,@ARGV unless -t STDOUT; I already reported this bug upstream over three years ago, but it appears to be left unfixed. At least, the bug bit me today again :p -- System Information: Debian Release: 3.1 Architecture: i386 (i686) Kernel: Linux 2.6.8 Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8) Versions of packages colordiff depends on: ii perl 5.8.4-8sarge4 Larry Wall's Practical Extraction -- no debconf information -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#378563: colordiff: causes lossage if the diff is redirected
On Monday, 17.07.2006 at 15:30 +0200, Adam Borowski wrote: Hello. If the output is not a terminal (like, when using diff to make a patch), colordiff will still happily colorize the output. This is bad if diff is an alias to colordiff, as suggested in the manpage. That's a very good point :-) The fix is simple: exec /usr/bin/diff,@ARGV unless -t STDOUT; It's unclear to me exactly what you're suggesting: are you suggesting an alternative means of aliasing 'diff', or is the above a modification to the code? I'd find it helpful if you could turn the above into a patch, thanks. I already reported this bug upstream over three years ago, but it appears to be left unfixed. At least, the bug bit me today again :p [Odd - I've got no record of you ever contacting me, which is strange since I'm usually fairly obsessive about keeping that sort of email. Did I ever reply?! Apologies if not, but I don't think I got your message.] I'll get this fixed now though, if you can clarify the nature of your fix above. Thanks, Dave. -- Dave Ewart - [EMAIL PROTECTED] - jabber: [EMAIL PROTECTED] - freenode: davee All email from me is now digitally signed, key from http://www.sungate.co.uk/ Fingerprint: AEC5 9360 0A35 7F66 66E9 82E4 9E10 6769 CD28 DA92 signature.asc Description: Digital signature
Bug#378563: colordiff: causes lossage if the diff is redirected
On Mon, Jul 17, 2006 at 07:22:11PM +0100, Dave Ewart wrote: On Monday, 17.07.2006 at 15:30 +0200, Adam Borowski wrote: Hello. If the output is not a terminal (like, when using diff to make a patch), colordiff will still happily colorize the output. This is bad if diff is an alias to colordiff, as suggested in the manpage. The fix is simple: exec /usr/bin/diff,@ARGV unless -t STDOUT; It's unclear to me exactly what you're suggesting: are you suggesting an alternative means of aliasing 'diff', or is the above a modification to the code? Sorry, my bad. I meant the latter. I'd find it helpful if you could turn the above into a patch, thanks. --- colordiff~ 2006-07-17 15:01:56.564810563 +0200 +++ colordiff 2006-07-17 15:01:56.564810563 +0200 @@ -31,6 +31,8 @@ use Getopt::Long qw(:config pass_through); use IPC::Open2; +exec /usr/bin/diff,@ARGV unless -t STDOUT; + my $app_name = 'colordiff'; my $version = '1.0.4'; my $author = 'Dave Ewart'; (Of course, the exact place doesn't really matter.) I already reported this bug upstream over three years ago, but it appears to be left unfixed. At least, the bug bit me today again :p [Odd - I've got no record of you ever contacting me, which is strange since I'm usually fairly obsessive about keeping that sort of email. Did I ever reply?! Apologies if not, but I don't think I got your message.] I'm afraid I didn't use mail, just the SourceForge bug report system: https://sourceforge.net/tracker/?func=detailaid=745456group_id=65829atid=512406 I'm not sure if they mail you any new bug reports by default; I would probably fail to notice such reports on my projects for years, too. Cheers and schtuff, -- 1KB // Microsoft corollary to Hanlon's razor: // Never attribute to stupidity what can be // adequately explained by malice. -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
