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

From: Jakub Narebski <jnareb@gmail.com>
Date: Mar 17, 2012, 14:32 UTC
Message-ID: <m3fwd7l2mu.fsf@localhost.localdomain>
In-Reply-To: <1331949574-15192-1-git-send-email-tim.henigan@gmail.com>

Tim Henigan <tim.henigan@gmail.com> writes:

Show 10 quoted lines

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

Show 23 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");
> +	}
> +	# TODO: if the diff including working copy files and those
> +	# files were modified during the diff, then the changes
> +	# should be copied back to the working tree
> +} else {
> +	if (defined($prompt)) {
> +		$ENV{GIT_DIFFTOOL_PROMPT} = 'true';
> +	}
> +	elsif (defined($no_prompt)) {
> +		$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
> +	}
> +
> +	$ENV{GIT_PAGER} = '';
> +	$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
> +	git_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';
> +}

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?

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)?

-- 
Jakub Narebski