From: John Keeping <john@keeping.me.uk>
Date: Thu, 14 Mar 2013 22:24:15 GMT
Subject: Re: [PATCH v2 3/3] difftool --dir-diff: symlink all files matching the working tree
Message-ID: <20130314222415.GC4256@serenity.lan>
In-Reply-To: <7v620ty8lc.fsf@alter.siamese.dyndns.org>

On Thu, Mar 14, 2013 at 02:28:31PM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> > index 3aab6e1..70e09b6 100755
> > --- a/t/t7800-difftool.sh
> > +++ b/t/t7800-difftool.sh
> > @@ -340,6 +340,28 @@ test_expect_success PERL 'difftool --dir-diff' '
> >  	stdin_contains file <output
> >  '
> >  
> > +write_script .git/CHECK_SYMLINKS <<\EOF
> > +for f in file file2 sub/sub
> > +do
> > +	echo "$f"
> > +	readlink "$2/$f"
> > +done >actual
> > +EOF
> 
> When you later want to enhance the test to check a combination of
> difftool arguments where some paths are expected to become links and
> others are expected to become real files, wouldn't this helper
> become a bit awkward to use?  The element that expects a real file
> could be an empty line to what corresponds to the output from
> readlink, but still...
> 
> If t/ directory (or when the test is run with --root=<there>) is
> aliased with symlinks in such a way that "cd <there> && $(pwd)" does
> not match <there>, would this check with $(pwd) still work, I have
> to wonder?

It looks like t3903 uses "ls -l" for this sort of test, perhaps
something like this covers these cases better:

    write_script .git/CHECK_SYMLINKS <<\EOF
    for f in file file2 sub/sub
    do
        ls -l "$2/$f" >"$f".actual
    done
    EOF

    ...

    workdir=$(git rev-parse --show-toplevel)
    grep "-> $workdir/file" file.actual
    grep "-> $workdir/file2" file2.actual
    grep "-> $workdir/sub/sub" sub/sub.actual

It looks like we already rely on that output format in t3903 so I think
that is safe, but it would be nice to have a better way to say "does
this link point to that file?".  I can't think of a way to do that that
doesn't seem far too complicated for what's required here.

> > +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
> 
> You can do this to align them nicer (note the "-" before EOF):
> 
> 	cat >expect <<-EOF &&
> 	file
>         $(pwd)/file
>         ...
>         EOF
> 
> > +	git difftool --dir-diff --symlink \
> > +		--extcmd "./.git/CHECK_SYMLINKS" branch HEAD &&
> > +	test_cmp actual expect
> > +'
> > +
>
> Thanks.