Re: [PATCH 2/2] difftool --dir-diff: symlink all files matching the working tree

[David Aguilar <davvid@gmail.com>]

On Wed, Mar 13, 2013 at 1:33 PM, John Keeping <john@keeping.me.uk> wrote:
> Some users like to edit files in their diff tool when using "git
> difftool --dir-diff --symlink" to compare against the working tree but
> difftool currently only created symlinks when a file contains unstaged
> changes.
>
> Change this behaviour so that symlinks are created whenever the
> right-hand side of the comparison has the same SHA1 as the file in the
> working tree.
>
> Note that textconv filters are handled in the same way as by git-diff
> and if a clean filter is not the inverse of its smudge filter we already
> get a null SHA1 from "diff --raw" and will symlink the file without
> going through the new hash-object based check.
>
> Reported-by: Matt McClure <matthewlmcclure@gmail.com>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
>  Documentation/git-difftool.txt |  4 +++-
>  git-difftool.perl              | 21 ++++++++++++++++++---
>  t/t7800-difftool.sh            | 14 ++++++++++++++
>  3 files changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
> index e575fea..8361e6e 100644
> --- a/Documentation/git-difftool.txt
> +++ b/Documentation/git-difftool.txt
> @@ -72,7 +72,9 @@ with custom merge tool commands and has the same value as `$MERGED`.
>  --symlinks::
>  --no-symlinks::
>         'git difftool''s default behavior is create symlinks to the
> -       working tree when run in `--dir-diff` mode.
> +       working tree when run in `--dir-diff` mode and the right-hand
> +       side of the comparison yields the same content as the file in
> +       the working tree.
>  +
>  Specifying `--no-symlinks` instructs 'git difftool' to create copies
>  instead.  `--no-symlinks` is the default on Windows.
> 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";
>                         }
>                 }
>         }
> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index eb1d3f8..8102ce1 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -370,6 +370,20 @@ test_expect_success PERL 'difftool --dir-diff' '
>         echo "$diff" | stdin_contains file
>  '
>
> +write_script .git/CHECK_SYMLINKS <<\EOF &&

Tiny nit.  Is there any downside to leaving this file
at the root instead of inside the .git dir?

> +#!/bin/sh
> +test -L "$2/file" &&
> +test -L "$2/file2" &&
> +test -L "$2/sub/sub"
> +echo $?
> +EOF
> +
> +test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '
> +       result=$(git difftool --dir-diff --symlink \
> +               --extcmd "./.git/CHECK_SYMLINKS" branch HEAD) &&
> +       test "$result" = 0
> +'
> +

How about something like this?

+       echo 0 >expect &&
+       git difftool --dir-diff --symlink \
+               --extcmd ./CHECK_SYMLINKS branch HEAD >actual &&
+       test_cmp expect actual

(sans gmail whitespace damage) so that we can keep it chained with &&.
Ah.. it seems your branch is based on master, perhaps?

There's stuff cooking in next for difftool's tests.
I'm not sure if this patch is based on top of them.
Can you rebase the tests so that the chaining is done like it is in 'next'?
-- 
David