On Sat, Mar 17, 2012 at 10:32 AM, Jakub Narebski <jnareb@xxxxxxxxx> wrote:
> Tim Henigan <tim.henigan@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html