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
