Tux / App-ccdiff

A colored diff that also colors inside changed lines
Artistic License 2.0
18 stars 5 forks source link

Example ~/bin/git-ccdiff improvements #16

Closed drmuey closed 5 years ago

drmuey commented 6 years ago

Greeting, I enjoyed your Glasgow talk on ccdiff and look forward to using it.

I noticed a couple things in the example ~/bin/git-ccdiff that could be improved, open to a pull request?

  1. $commit and $file aren’t used.
  2. -f will never get into @git because it will match the if before it
  3. It has funny indenting

Below is a version that removes those variables, changes the if()s around, and was run through perltidy. Seems to work ok

#!/usr/bin/env perl

# See https://github.com/Tux/App-ccdiff for information about this script

use 5.14.2;
use warnings;

my @opt;

my @git = qw( git difftool );
for (@ARGV) {
    if (-f) {
        push @git, $_;
    }
    elsif (m/^-/) {
        push @opt, $_;
    }
    else {
        push @git, "$_~1..$_";
    }
}

@opt and $ENV{CCDIFF_OPTIONS} = join " " => @opt;
system @git;

Might even be cool to include the file itself in the repo and have the README git integration example download it from git? That way it's more concise and it avoid the problems of shells that interpolate $ vars.

Tux commented 6 years ago

Greeting, I enjoyed your Glasgow talk on ccdiff and look forward to using it.

Thank you!

I noticed a couple things in the example ~/bin/git-ccdiff that could be improved, open to a pull request?

Sure, but let me explain some parts first ...

  1. $commit and $file aren’t used.

Not in the version on github, and that is because I am still in conversation with the authors of git themselves, as there are a few things that are either impossible or quirky, so there need to be workarounds or official support. Until that is clear, the script is in a state of flux

  1. -f will never get into @git because it will match the if before it

No :) read again. -f is the file-test operator, not m/-f/

  1. It has funny indenting

Funny to you perhaps, but it is the only logical indenting and the only acceptable indenting. Read CONTRIBUTING.md. This is not going to change

Se also style guide and .perltidyrc

Somehow I hope that after digesting my explanation, you'll be the next to use this indentation style and promote it to the world :)

Below is a version that removes those variables, changes the if()s around, and was run through perltidy.

Unacceptable due to wrong indentation :) Install .perltidyrc in your $HOME to make perltidy work more or less as expected (by me).


#!/usr/bin/env perl

I think the community will appreciate this ^^. I personally never use it for a lot of reasons. The most important being that I quite often change my $PATH to have the perl version I am working on to be the current perl, and it is more than likely that that version does not have the modules installed that the tool(s) need. Hence I use a fixed path of a perl that I know is stable and has all stuff installed that my tools require

...

Might even be cool to include the file itself in the repo and have the README git integration example download it from git?

It already is: git-ccdiff

I have pushed a change that reflects this and shows the installation