Re: [PATCH 1/9] difftool: parse options using Getopt::Long
- From
David Aguilar <davvid@gmail.com>
- Date
- Mar 17, 2012, 03:21 UTC
- Message-ID
- <CAJDDKr7BTz-2THw1JaJEDcK1G4Uxwc88=gFsHCSONHAtqtypRw@mail.gmail.com>
- In-Reply-To
- <1331949442-15039-2-git-send-email-tim.henigan@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.
Show 151 quoted lines
> 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