genehack / Git-Wrapper

Perl wrapper for git(7) command line interface
http://genehack.github.com/Git-Wrapper
17 stars 23 forks source link

Need to strip ANSI colour codes from output #13

Closed karenetheridge closed 12 years ago

karenetheridge commented 12 years ago

My git config includes this:

[color] 
        diff = auto
        status = auto
        branch = auto
        interactive = auto
        ui = true

And this:

perl -MGit::Wrapper -wle'print Git::Wrapper->new(".")->branch'

...has "master" in green.

One side effect of this is that any code that compares branch strings literally (for example, [Git::CheckFor::CorrectBranch]) will fail.

All git commands need to be run with colors turned off, or alternatively the output needs to be cleaned up before returning it.

karenetheridge commented 12 years ago

This perl oneliner seems to produce inconsistent results - for example in two shells on the same box in the same repository, one produces colour and the other does not. (In both, 'git branch' on the command line does colour master in green.)

...after more comparisons, it turns out that the variable GIT_PAGER_IN_USE is the culprit. when set to 'true', the oneliner colours 'master' in green; when unset, it doesn't.

I have so far been unable to find any documentation on what this variable is for.. will keep looking.

karenetheridge commented 12 years ago

googling tells me: "git does not output color codes if the output is not a tty. To force the color codes, you can do GIT_PAGER_IN_USE=true ". So it should be sufficient to set $ENV{GIT_PAGER_IN_USE} = undef in Git::Wrapper, as apparently there is some program out there (that I must have run recently in this shell) which set that variable and naughtily left it set.

genehack commented 12 years ago

ENV statement added in 0.22, making it's way towards CPAN now -- thanks for reporting the problem; thanks even more for fixing it. 8^)

jf647 commented 12 years ago

Under 5.8 perls, doing

$ENV{GIT_PAGER_IN_USE} = undef

produces this warning:

Use of uninitialized value in scalar assignment at /home/jfitzgib/perlbrew/pkg/lib/5.8.8-linux-x86_64-linux-thread-multi/lib/perl5/Git/Wrapper.pm line 16.

You can either blindly delete from %ENV or wrap it in a localized warnings pragma:

{ no warnings 'uninitialized'; $ENV{GIT_PAGER_IN_USE} = undef; }

I've pushed a fix and created a pull request.

tommystanton commented 12 years ago

In my ~/.gitconfig, I had color.branch set to 'always' (I don't have a good reason for why). The fix implemented did not work in this case. With the latest Git-Wrapper repository checkout, the one-liner mentioned by @karenetheridge still shows ANSI color (I first noticed by the "new branch name is correct" test in t/basic.t failing).

I brainstormed for a while and experimented with $GIT_PAGER_IN_USE, but I didn't come up with an alternative fix. I changed my color.branch to 'auto' to fix for now.

karenetheridge commented 12 years ago

@tommystanton can you confirm if 'git status --porcelain --branch' works better for your settings? it seems to be color-free even when color.branch = always. If so, I'll attempt to update the distributions that call git branch.

karenetheridge commented 12 years ago

Alternatively (and I like this idea better), Git::Wrapper should create a wrapper method called 'branch_name', which DTRT to determine the branch name... something like:

sub branch_name
{
    my $self = shift;

    if ($self->supports_status_porcelain)
    {
         chomp (my $raw = $self->status({porcelain => 1,  branch => 1}));
         (my $branch = $raw) =~ s/^## //;
         return $branch;
    }

    undef $ENV{GIT_PAGER_IN_USE};
    my ($branch) = map { /^\*\s+(.+)/ ? $1 : () } $git->branch;
    return $branch;
}
tommystanton commented 12 years ago

@karenetheridge So what you're proposing is that Git::Wrapper could have an explicit 'branch_name' method, rather than the calling code having to call 'branch' (an implicit method that RUN() handles) and then parsing that string returned. Is that correct?

Git::Wrapper's 'status' method returns a Git::Wrapper::Statuses object when supports_status_porcelain() returns true, so that $self->status({porcelain ... line wouldn't work, given the context of that block containing it.

The last section of code (in the outer block) would still have ANSI color escapes in it when branch.color is set to always. This sort of line might help (idea taken from @sartak):

$branch =~ s/\e\[[\d;]*[a-zA-Z]//g;

The regex above is helpful, but I now realize that this ANSI drama could be avoided by passing the hashref { color=>'never' } to the 'branch' method.

genehack commented 12 years ago

I've been moving cross-country over the past week or so, but I'm at my destination and starting to dig out -- I'll look at these patches within the next day or so. Sorry about the lack of updates, but I didn't have as much Internet access on the road as I had hoped.

karenetheridge commented 12 years ago

@tommystanton yes exactly - getting the name of the current branch is a pretty common operation to do, and clearly it's non-trivial and TIMTOWTDI, so IMO it would make sense to wrap it up into a convenient accessor on Git::Wrapper.

genehack commented 12 years ago

First, apologies for letting this languish. My original "oh I'm here, therefore I'll have free cycles" was incredibly optimistic, it seems.

I think both of you had some good ideas, so I ended up slamming them together to produce pull request #25. Do either of you have any comments on that? Does it fix your issues?

genehack commented 12 years ago

I just merged #25 and will release here in a few minutes, so I'm marking this issue closed.

karenetheridge commented 12 years ago

genehack++