Re: [PATCH 5/5] difftool: Use symlinks when diffing against the worktree

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 23, 2012, 04:57 UTC
Message-ID: <7vzk6rnkgq.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1343015831-17498-6-git-send-email-davvid@gmail.com>

David Aguilar <davvid@gmail.com> writes:

Show 5 quoted lines

> +	# Do not copy back files when symlinks are used
> +	if ($symlinks) {
> +		exit(0);
> +	}
> +

Isn't this a bit risky, depending on the behaviour of the tool that eventually lead the user to invoke his favorite editor to muck with the files in the temporary directory? I think most sane people and their editors would follow symlinks and update the file the symlink points at when writing out the modified contents, but it should not be too much trouble to detect the case in which the editor unlinked the symlink and recreated a regular file in its place, and copy the file back when that happened, to make it even safer, no?

The most lazy solution would be to just remove the above block, and let the compare() compare the symlink $b/$file and the working tree file $workdir/$file that is pointed by it. We will find data losing case where the editor unlinks and creates that way automatically.

Optionally, you can update

	if (-e "$b/$file" && compare("$b/$file", "$workdir/$file")) {

with

	if (! -l "$b/$file" && -f _ && compare("$b/$file", "$workdir/$file")) {

to avoid the cost of comparison.

Show 14 quoted lines

>  	# 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
> +
>  	for my $file (@working_tree) {
>  		if (-e "$b/$file" && compare("$b/$file", "$workdir/$file")) {
>  			copy("$b/$file", "$workdir/$file") or die $!;
> -			chmod(stat("$b/$file")->mode, "$workdir/$file") or die $!;
> +			my $mode = stat("$b/$file")->mode;
> +			chmod($mode, "$workdir/$file") or die $!;
>  		}
>  	}
> +	exit(0);
>  }

Other than that, the series looked well thought-out.

Thanks.