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

[John Keeping <john@keeping.me.uk>]

On Wed, Mar 13, 2013 at 08:41:29PM -0700, David Aguilar wrote:
> On Wed, Mar 13, 2013 at 1:33 PM, John Keeping <john@keeping.me.uk> wrote:
> > 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?

I followed what some of the other uses of write_script (in other tests)
did.  I think putting it under .git is slightly better because it won't
show up as untracked in the repository but that shouldn't matter here,
so I'm happy to change it in a re-roll.

> > +#!/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 &&.

I hadn't considered using test_cmp, if we go that way I wonder if we can
do slightly better for future debugging.  Something like this perhaps?

+write_script .git/CHECK_SYMLINKS <<\EOF &&
+for f in file file2 sub/sub
+do
+	echo "$f"
+	readlink "$2/$f"
+done >actual
+EOF
+
+test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '
+	cat <<EOF >expect &&
+file
+$(pwd)/file
+file2
+$(pwd)/file2
+sub/sub
+$(pwd)/sub/sub
+EOF
+       git difftool --dir-diff --symlink \
+               --extcmd "./.git/CHECK_SYMLINKS" branch HEAD &&
+	test_cmp actual expect
+'

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

Yes it is based on master.  The cleanup on next looks good, I'll base
the re-roll on that.


John