On Tue, Mar 12, 2013 at 5:17 PM, John Keeping <j...@keeping.me.uk> wrote:
> On Tue, Mar 12, 2013 at 04:48:16PM -0600, Matt McClure wrote:
>> On Mar 12, 2013, at 4:16 PM, Junio C Hamano <gits...@pobox.com> wrote:
>>
>> > Matt McClure <matthewlmccl...@gmail.com> writes:
>> >
>> > * If you are comparing two trees, and especially if your RHS is not
>> >   HEAD, you will send everything to a temporary without
>> >   symlinks. Any edit made by the user will be lost.
>>
>> I think you're suggesting to use a symlink any time the content of any
>> given RHS revision is the same as the working tree.
>>
>> I imagine that might confuse me as a user. It would create
>> circumstances where some files are symlinked and others aren't for
>> reasons that won't be straightforward.
>>
>> I imagine solving that case, I might instead implement a copy back to
>> the working tree with conflict detection/resolution. Some earlier
>> iterations of the directory diff feature used copy back without
>> conflict detection and created situations where I clobbered my own
>> changes by finishing a directory diff after making edits concurrently.
>
> The code to copy back working tree files is already there, it just
> triggers using the same logic as the creation of symlinks in the first
> place and doesn't attempt any conflict detection.  I suspect that any
> more comprehensive solution will need to restrict the use of "git
> difftool -d" whenever the index contains unmerged entries or when there
> are both staged and unstaged changes, since the merge resolution will
> cause these states to be lost.
>
> The implementation of Junio's suggestion is relatively straightforward
> (this is untested, although t7800 passes, and can probably be improved
> by someone better versed in Perl).  Does this work for your original
> scenario?

This is a nice straightforward approach.

As Junio mentioned, a good next step would be this patch
in combination with making the truly temporary files
created by dir-diff readonly.

Will that need a win32 platform check?
Does anyone want to take this and whip it into a proper patch?

> -- >8 --
> diff --git a/git-difftool.perl b/git-difftool.perl
> index 0a90de4..5f093ae 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -83,6 +83,21 @@ sub exit_cleanup
>         exit($status | ($status >> 8));
>  }
>
> +sub use_wt_file
> +{
> +       my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
> +       my $null_sha1 = '0' x 40;
> +
> +       if ($sha1 eq $null_sha1) {
> +               return 1;
> +       } elsif (not $symlinks) {
> +               return 0;
> +       }
> +
> +       my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file");
> +       return $sha1 eq $wt_sha1;
> +}
> +
>  sub setup_dir_diff
>  {
>         my ($repo, $workdir, $symlinks) = @_;
> @@ -159,10 +174,10 @@ EOF
>                 }
>
>                 if ($rmode ne $null_mode) {
> -                       if ($rsha1 ne $null_sha1) {
> -                               $rindex .= "$rmode $rsha1\t$dst_path\0";
> -                       } else {
> +                       if (use_wt_file($repo, $workdir, $dst_path, $rsha1, 
> $symlinks)) {
>                                 push(@working_tree, $dst_path);
> +                       } else {
> +                               $rindex .= "$rmode $rsha1\t$dst_path\0";
>                         }
>                 }
>         }
> --
> 1.8.2.rc2.4.g7799588
>
-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to