Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix issue #4 without changing existing behaviour, just introduce new opti... #6

Merged
merged 1 commit into from Jan 26, 2013

Conversation

ymattw
Copy link

@ymattw ymattw commented Jan 22, 2013

Thanks for being looking into my request deeply.

How about just introduce a new option '--color=yes|no|auto' to override $color_patch variable (color_patches setting), and don't change the original "-f STDOUT" condition but force colors on/off according to the value of $colormode. The key change would be

-if ((-f STDOUT) && ($color_patch == 0)) {
+if (((-f STDOUT) && ($color_patch == 0)) || ($colormode eq "no")) {

I am sending a new pull request based on the start point where I forked: a79dc09.

~/ws/colordiff $ git log --pretty='%h %s (%an)' -2
a0e7e9c fix issue 4 without changing existing behaviour, just introduce new option --color=yes|no|auto (Matthew Wang)
a79dc09 Add '--fakeexitcode' option. (Dave Ewart)

I tested with following use cases:

With color_patches=yes

  • Expects color
    • git diff a79dc0 | ./colordiff.pl
    • git diff a79dc0 | ./colordiff.pl --color=yes
    • git diff a79dc0 | ./colordiff.pl --color=auto
    • git diff a79dc0 | ./colordiff.pl | less -r
    • git diff a79dc0 | ./colordiff.pl --color=yes | less -r
    • git diff a79dc0 | ./colordiff.pl --color=auto | less -r
    • git diff a79dc0 | ./colordiff.pl > out
    • git diff a79dc0 | ./colordiff.pl --color=yes > out
    • git diff a79dc0 | ./colordiff.pl --color=auto > out
  • Expects no color
    • git diff a79dc0 | ./colordiff.pl --color=no
    • git diff a79dc0 | ./colordiff.pl --color=no | less -r
    • git diff a79dc0 | ./colordiff.pl --color=no > out

With color_patches=no

  • Expects color
    • git diff a79dc0 | ./colordiff.pl
    • git diff a79dc0 | ./colordiff.pl --color=yes
    • git diff a79dc0 | ./colordiff.pl --color=auto
    • git diff a79dc0 | ./colordiff.pl | less -r
    • git diff a79dc0 | ./colordiff.pl --color=yes | less -r
    • git diff a79dc0 | ./colordiff.pl --color=auto | less -r
    • git diff a79dc0 | ./colordiff.pl --color=yes > out
  • Expects no color
    • git diff a79dc0 | ./colordiff.pl > out
    • git diff a79dc0 | ./colordiff.pl --color=auto > out
    • git diff a79dc0 | ./colordiff.pl --color=no
    • git diff a79dc0 | ./colordiff.pl --color=no | less -r
    • git diff a79dc0 | ./colordiff.pl --color=no > out

@daveewart
Copy link
Owner

Matt: this looks much closer to what I thought your first patch was doing. I'll look it over properly and get it merged in. Many thanks...

@daveewart
Copy link
Owner

Code looks fine, Matt, so I'll merge this: the comments in the README need changing (and in fact README is built from colordiff.xml - it only exists in its own right because it's nice having a README file on GitHub. I'll make the appropriate changes here, though.

daveewart added a commit that referenced this pull request Jan 26, 2013
Merge ymattw's changes to introduce '--color' option.
@daveewart daveewart merged commit 5fb657c into daveewart:current Jan 26, 2013
@daveewart
Copy link
Owner

Matt: code is merged, I've made some minor changes and have pushed it back here. Cheers :-)

@ymattw
Copy link
Author

ymattw commented Jan 26, 2013

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants