Re: [PATCH 1/9] difftool: parse options using Getopt::Long

[David Aguilar <davvid@gmail.com>]

On Fri, Mar 16, 2012 at 6:57 PM, Tim Henigan <tim.henigan@gmail.com> wrote:
> Replace custom option/argument parser with standard Getopt::Long
> module.  This shortens the code and makes it easier to understand.

I've also wanted to do the same in the past.  The one thing holding me
back was this note from the perldocs:

"If pass_through is also enabled, options processing will terminate at
the first unrecognized option, or non-option, whichever comes first."

http://search.cpan.org/~jv/Getopt-Long-2.38/lib/Getopt/Long.pm

That leads me to believe that using Getopt::Long would break this use case:

    $ git difftool --cached --tool xxdiff ...

According to the docs, it will stop at --cached and leave it (and the
rest) in @ARGV.  When git-diff runs it will see the --tool argument
and bail out.

Is this indeed the case?  I am a little ashamed that the difftool
tests do not cover this area.  That would be a valuable first step
towards exploring this approach, IMO.


BTW, I hate @ARGV parsing loops just as much as anyone!  I was not
ignorant of Getopt::Long, and no, I was not re-inventing the wheel for
no reason.  The reason it was done that way was so that we can forward
everything we don't know about to git-diff.

I haven't tried this patch, but my reading of the documentation leads
me to believe that this is a regression.



> Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
> ---
>  git-difftool.perl |  109 +++++++++++++++++++++--------------------------------
>  1 file changed, 44 insertions(+), 65 deletions(-)
>
> diff --git a/git-difftool.perl b/git-difftool.perl
> index 09b65f1..e365727 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -15,11 +15,8 @@ use strict;
>  use warnings;
>  use Cwd qw(abs_path);
>  use File::Basename qw(dirname);
> -
> -require Git;
> -
> -my $DIR = abs_path(dirname($0));
> -
> +use Getopt::Long qw(:config pass_through);
> +use Git;
>
>  sub usage
>  {
> @@ -33,6 +30,7 @@ USAGE
>
>  sub setup_environment
>  {
> +       my $DIR = abs_path(dirname($0));
>        $ENV{PATH} = "$DIR:$ENV{PATH}";
>        $ENV{GIT_PAGER} = '';
>        $ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
> @@ -47,75 +45,56 @@ sub exe
>        return $exe;
>  }
>
> -sub generate_command
> -{
> -       my @command = (exe('git'), 'diff');
> -       my $skip_next = 0;
> -       my $idx = -1;
> -       my $prompt = '';
> -       for my $arg (@ARGV) {
> -               $idx++;
> -               if ($skip_next) {
> -                       $skip_next = 0;
> -                       next;
> -               }
> -               if ($arg eq '-t' || $arg eq '--tool') {
> -                       usage() if $#ARGV <= $idx;
> -                       $ENV{GIT_DIFF_TOOL} = $ARGV[$idx + 1];
> -                       $skip_next = 1;
> -                       next;
> -               }
> -               if ($arg =~ /^--tool=/) {
> -                       $ENV{GIT_DIFF_TOOL} = substr($arg, 7);
> -                       next;
> -               }
> -               if ($arg eq '-x' || $arg eq '--extcmd') {
> -                       usage() if $#ARGV <= $idx;
> -                       $ENV{GIT_DIFFTOOL_EXTCMD} = $ARGV[$idx + 1];
> -                       $skip_next = 1;
> -                       next;
> -               }
> -               if ($arg =~ /^--extcmd=/) {
> -                       $ENV{GIT_DIFFTOOL_EXTCMD} = substr($arg, 9);
> -                       next;
> -               }
> -               if ($arg eq '-g' || $arg eq '--gui') {
> -                       eval {
> -                               my $tool = Git::command_oneline('config',
> -                                                               'diff.guitool');
> -                               if (length($tool)) {
> -                                       $ENV{GIT_DIFF_TOOL} = $tool;
> -                               }
> -                       };
> -                       next;
> -               }
> -               if ($arg eq '-y' || $arg eq '--no-prompt') {
> -                       $prompt = 'no';
> -                       next;
> -               }
> -               if ($arg eq '--prompt') {
> -                       $prompt = 'yes';
> -                       next;
> -               }
> -               if ($arg eq '-h') {
> -                       usage();
> -               }
> -               push @command, $arg;
> +# parse command-line options. all unrecognized options and arguments
> +# are passed through to the 'git diff' command.
> +my ($difftool_cmd, $extcmd, $gui, $help, $no_prompt, $prompt);
> +GetOptions('g|gui' => \$gui,
> +       'h' => \$help,
> +       'prompt' => \$prompt,
> +       't|tool:s' => \$difftool_cmd,
> +       'x|extcmd:s' => \$extcmd,
> +       'y|no-prompt' => \$no_prompt);
> +
> +if (defined($help)) {
> +       usage();
> +}
> +if (defined($difftool_cmd)) {
> +       if (length($difftool_cmd) > 0) {
> +               $ENV{GIT_DIFF_TOOL} = $difftool_cmd;
> +       } else {
> +               print "No <tool> given for --tool=<tool>\n";
> +               usage();
>        }
> -       if ($prompt eq 'yes') {
> -               $ENV{GIT_DIFFTOOL_PROMPT} = 'true';
> -       } elsif ($prompt eq 'no') {
> -               $ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
> +}
> +if (defined($extcmd)) {
> +       if (length($extcmd) > 0) {
> +               $ENV{GIT_DIFFTOOL_EXTCMD} = $extcmd;
> +       } else {
> +               print "No <cmd> given for --extcmd=<cmd>\n";
> +               usage();
> +       }
> +}
> +if (defined($gui)) {
> +       my $guitool = "";
> +       $guitool = Git::config('diff.guitool');
> +       if (length($guitool) > 0) {
> +               $ENV{GIT_DIFF_TOOL} = $guitool;
>        }
> -       return @command
> +}
> +if (defined($prompt)) {
> +       $ENV{GIT_DIFFTOOL_PROMPT} = 'true';
> +}
> +elsif (defined($no_prompt)) {
> +       $ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
>  }
>
>  setup_environment();
> +my @command = (exe('git'), 'diff', @ARGV);
>
>  # ActiveState Perl for Win32 does not implement POSIX semantics of
>  # exec* system call. It just spawns the given executable and finishes
>  # the starting program, exiting with code 0.
>  # system will at least catch the errors returned by git diff,
>  # allowing the caller of git difftool better handling of failures.
> -my $rc = system(generate_command());
> +my $rc = system(@command);
>  exit($rc | ($rc >> 8));
> --
> 1.7.9.1.290.gbd444
>



-- 
David