From: Tim Henigan <tim.henigan@gmail.com>
Date: Sun, 18 Mar 2012 00:58:12 GMT
Subject: Re: [PATCH 7/9] difftool: teach difftool to handle directory diffs
Message-ID: <CAFouetg1JEBsO4SyHxpaphQZQtmbo8ugbHoUWnhSXku4DWuvVA@mail.gmail.com>
In-Reply-To: <m3fwd7l2mu.fsf@localhost.localdomain>

On Sat, Mar 17, 2012 at 10:32 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> Tim Henigan <tim.henigan@gmail.com> writes:
>
>> +     foreach my $path (keys %submodule) {
>> +             if (defined $submodule{$path}{left}) {
>> +                     open(SUBMOD, ">$ldir/$path") or die $!;
>> +                     print(SUBMOD "Subproject commit $submodule{$path}{left}");
>> +                     close(SUBMOD);
>> +             }
>> +             if (defined $submodule{$path}{right}) {
>> +                     open(SUBMOD, ">$rdir/$path") or die $!;
>> +                     print(SUBMOD "Subproject commit $submodule{$path}{right}");
>> +                     close(SUBMOD);
>
> Could you please use modern Perl, and use lexical filehandles instead
> of globs, and 3-arg version of 'open', i.e.
>
>  +                     open my $submod_fh, ">", "$ldir/$path" or die $!;
>  +                     print $submod_fh "Subproject commit $submodule{$path}{left}";
>  +                     close $submod_fh;

I will make this change in v2.  Thank you for the review and the reminder.


>> +if (defined($dirdiff)) {
>> +     my ($a, $b) = setup_dir_diff();
>> +     if (defined($extcmd)) {
>> +             system("$extcmd $a $b");
>> +     } else {
>> +             $ENV{GIT_DIFFTOOL_DIRDIFF} = 'true';
>> +             system("git difftool--helper $a $b");
>> +     }
>
> Why you use 'system' (and not in list form which does not require
> escaping shell characters) instead of git_cmd_try for first
> "git difftool--helper" invocation?

There is no good reason to use 'system' here.  I will change this to
'Git::command_noisy' in v2.


> Is $extcmd and resultof setup_dir_diff() to be treated as shell
> snippet, and used in string form of 'system' without escaping shell
> metacharacters (like ' ' in filename)?

In v2, I will change to the list form of 'system' for the call to
$extcmd. I think I made some bad assumptions about the contents of
$extcmd, $a and $b because of my local test setup.  However, changing
to list form should correct the oversight.