[Bug] /etc/colordiffrc color_patches gets always overriden and can't redirect color to file

[jesusvazquez]

Hi!

I'm working on a set of scripts that are running on GitlabCI where I was trying to use colordiff to make the output more friendly.

For more context I'm using kubectl diff together with export KUBECTL_EXTERNAL_DIFF="colordiff" so I don't really have control over colordiff arguments like --color=yes. Instead I need to rely on /etc/colordiffrc.

The problem is:

• I override the configuration color_patches=yes inside /etc/colordiffrc Which given this logic https://github.com/daveewart/colordiff/blob/current/colordiff.pl#L213-L218 it makes $color_patch = 1
• Then a few lines below you find this logic https://github.com/daveewart/colordiff/blob/current/colordiff.pl#L284-L291 that based on color_mode which is defaulted to auto will always unset color_patch with $color_patch = undef;
• Finally the if that decides everything https://github.com/daveewart/colordiff/blob/current/colordiff.pl#L296-L297 always thinks color_patch is disabled.

How to reproduce

Set of steps to reproduce

❯ docker run -it alpine
/ # apk update
/ # apk add colordiff
/ # echo "foo" > 1
/ # echo "bar" > 2
/ # colordiff 1 2
# This will print out the diff patch with colors. EXPECTED
/ # colordiff 1 2  > testfile
/ # cat testfile
# This will print out the diff patch without colors. EXPECTED
/ # sed -i -e 's/color_patches=no/color_patches=yes/g' /etc/colordiffrc # This overrides the configuration to enable colors when doing redirects
/ # colordiff 1 2  > testfile
/ # cat testfile
# This will print out the diff patch without colors. HERE IS THE BUG

Here is an image:

Affected version

I found this in the latest packages of arch and alpine docker which points to colordiff v1.0.18

---

I hope the ticket is clear enough cc @daveewart

[jesusvazquez]

Workaround

We build our own docker containers for certain tools and in order to unblock myself in the meantime I've added this as a step in the build process:

# hadolint ignore=DL3018,DL3013,SC2016
RUN apk add --no-cache colordiff \
    # Necessary hack for colordiff due to an internal bug reported here https://github.com/daveewart/colordiff/issues/44
    # Also hadolint ignores SC2016 because $color_mode is a literal
    && sed -i -e 's/$color_mode\ =\ \"auto\"/$color_mode\ =\ \"yes\"/g' /usr/bin/colordiff \
    && echo "done"`

But if you're not using docker you can simply put sed -i -e 's/$color_mode\ =\ \"auto\"/$color_mode\ =\ \"yes\"/g' /usr/bin/colordiff in a convenient place for you.

:point_up: the workaround is essentially changing the default of color_mode to yes which actually makes sense for a tool that adds color :joy:

[jesusvazquez]

Gently ping to @daveewart just so that this does not fall through the cracks.

[daveewart]

Thanks for the ping. I saw this when you originally submitted and felt nervous about making any changes to the "does it add colour or not?" code because this is always contentious, everyone has different views about the defaults. But as you say, erring on the side of adding colour seems reasonable.

However the ordering of the logic - as you helpfully point out - shows that it is not consistent.

I'm considering just removing the final block from https://github.com/daveewart/colordiff/blob/current/colordiff.pl#L284-L291 so that block only changes color_patch if there's an explicit setting given for color_mode (i.e. '--color=yes') on the command line. (Proposed patch is remove lines 289 & 290)

Would that be sufficient?

I don't understand your final comment:

"Finally the if that decides everything https://github.com/daveewart/colordiff/blob/current/colordiff.pl#L296-L297 always thinks color_patch is disabled."

Can you explain, please?

[jesusvazquez]

Can you explain, please?

Yes, first remember that I'm not executing colordiff <params> but rather relying on /etc/colordiffrc and using something like export KUBECTL_EXTERNAL_DIFF=colordiff

What I meant is that, if colordiff is not executed with --color=yes, due to this logic here https://github.com/daveewart/colordiff/blob/current/colordiff.pl#L284-L291 color_patch is always set to undef, because color_mode is constant https://github.com/daveewart/colordiff/blob/current/colordiff.pl#L168.

color_mode is not configurable through /etc/colordiffrc only through --color param which is only possible on CLI.

So in the end this if block https://github.com/daveewart/colordiff/blob/current/colordiff.pl#L296-L297 always switches off color when writing to a file.

---

Would that be sufficient?

It is sufficient as long as color_mode can also be set using the configuration file. To enable color patches an user would then rely on color_mode=yes and color_patches=yes.

[inket]

I agree with this! In our case, some other script that we have no control over calls colordiff so we don't have any chance to append the --color parameter.

Having control of this behavior from the .colordiffrc file would be really helpful. Another suggestion would be to support environment variables. (e.g. COLORDIFF_COLOR=NO ./some-script)

[jesusvazquez]

Having control of this behavior from the .colordiffrc file would be really helpful. Another suggestion would be to support environment variables. (e.g. COLORDIFF_COLOR=NO ./some-script)

Yes that would also work :+1:

[daveewart]

Looking at this now, apologies again for the long delay. Don't seem to have much time to devote to this, these days...

[daveewart]

I've completely rewritten the logic handling color_mode and color_patches as per https://github.com/daveewart/colordiff/commit/2821e2cf917af6fc3ce246bfd43ce4165a0f883a

This should fix your issue, are you able to test with the latest Github version?

Setting

color_mode=yes color_patches=yes

in the config file should do what you want.

[inket]

Thank you so much for implementing this! In our case color_mode=no in .colordiffrc works as expected!

[jesusvazquez]

Thank you @daveewart !