Re: [PATCH 7/9] difftool: teach difftool to handle directory diffs

From: Tim Henigan <tim.henigan@gmail.com>
Date: Mar 18, 2012, 00:58 UTC
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:

Show 19 quoted lines

> 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.

Show 12 quoted lines

>> +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.