--dir-diff not working with partial path limiter

--dir-diff not working with partial path limiter

[Bernhard Kirchen]

Hello!

Today I started using --dir-diff and noticed a problem when specifying a
non-full path limiter. My diff tool is setup to be meld (*1).

OK while working directory is repo root; also OK while working directory is
repo subfolder "actual":
git difftool --dir-diff HEAD~1 HEAD -- actual/existing/path
=> meld opens with proper dir-diff.

NOT OK while working directory is repo subfolder "actual":
git difftool --dir-diff HEAD~1 HEAD -- existing/path
=> nothing happens, as if using "non/such/path" as the path limiter.

Because "git diff HEAD~1 HEAD -- existing/path" while the working directory
is the repo subfolder "actual" works, I epxected the difftool to work
similarly. Is this a bug?

Best,
Bernhard

(*1)
[diff]
tool = mydiffmeld
[difftool "mydiffmeld"]
cmd = meld --auto-compare --diff $LOCAL $REMOTE
[difftool]
prompt = false

[PATCH] difftool: fix argument handling in subdirs

[John Keeping]

On Mon, Jul 04, 2016 at 08:37:39PM +0200, Bernhard Kirchen wrote:
> Today I started using --dir-diff and noticed a problem when specifying a
> non-full path limiter. My diff tool is setup to be meld (*1).
> 
> OK while working directory is repo root; also OK while working directory is
> repo subfolder "actual":
> git difftool --dir-diff HEAD~1 HEAD -- actual/existing/path
> => meld opens with proper dir-diff.
> 
> NOT OK while working directory is repo subfolder "actual":
> git difftool --dir-diff HEAD~1 HEAD -- existing/path
> => nothing happens, as if using "non/such/path" as the path limiter.
> 
> Because "git diff HEAD~1 HEAD -- existing/path" while the working directory
> is the repo subfolder "actual" works, I epxected the difftool to work
> similarly. Is this a bug?

I think it is, yes.  The patch below fixes it for me and doesn't break
any existing tests, but I still don't understand why the separate
$diffrepo was needed originally, so I'm not certain this won't break
some other corner case.

-- >8 --
When in a subdirectory of a repository, path arguments should be
interpreted relative to the current directory not the root of the
working tree.

The Git::repository object passed into setup_dir_diff() is configured to
handle this correctly but we create a new Git::repository here without
setting the WorkingSubdir argument.  By simply using the existing
repository, path arguments are handled relative to the current
directory.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 git-difftool.perl | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index ebd13ba..c9d3ef8 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -115,16 +115,9 @@ sub setup_dir_diff
 {
 	my ($repo, $workdir, $symlinks) = @_;
 
-	# Run the diff; exit immediately if no diff found
-	# 'Repository' and 'WorkingCopy' must be explicitly set to insure that
-	# if $GIT_DIR and $GIT_WORK_TREE are set in ENV, they are actually used
-	# by Git->repository->command*.
 	my $repo_path = $repo->repo_path();
-	my %repo_args = (Repository => $repo_path, WorkingCopy => $workdir);
-	my $diffrepo = Git->repository(%repo_args);
-
 	my @gitargs = ('diff', '--raw', '--no-abbrev', '-z', @ARGV);
-	my $diffrtn = $diffrepo->command_oneline(@gitargs);
+	my $diffrtn = $repo->command_oneline(@gitargs);
 	exit(0) unless defined($diffrtn);
 
 	# Build index info for left and right sides of the diff
@@ -176,12 +169,12 @@ EOF
 
 		if ($lmode eq $symlink_mode) {
 			$symlink{$src_path}{left} =
-				$diffrepo->command_oneline('show', "$lsha1");
+				$repo->command_oneline('show', "$lsha1");
 		}
 
 		if ($rmode eq $symlink_mode) {
 			$symlink{$dst_path}{right} =
-				$diffrepo->command_oneline('show', "$rsha1");
+				$repo->command_oneline('show', "$rsha1");
 		}
 
 		if ($lmode ne $null_mode and $status !~ /^C/) {
-- 
2.9.0.465.g8850cbc

Re: [PATCH] difftool: fix argument handling in subdirs

[David Aguilar]

[Cc'd Tim, who originally authored the dir-diff code]

On Tue, Jul 05, 2016 at 08:52:52PM +0100, John Keeping wrote:
> On Mon, Jul 04, 2016 at 08:37:39PM +0200, Bernhard Kirchen wrote:
> > Today I started using --dir-diff and noticed a problem when specifying a
> > non-full path limiter. My diff tool is setup to be meld (*1).
> > 
> > OK while working directory is repo root; also OK while working directory is
> > repo subfolder "actual":
> > git difftool --dir-diff HEAD~1 HEAD -- actual/existing/path
> > => meld opens with proper dir-diff.
> > 
> > NOT OK while working directory is repo subfolder "actual":
> > git difftool --dir-diff HEAD~1 HEAD -- existing/path
> > => nothing happens, as if using "non/such/path" as the path limiter.
> > 
> > Because "git diff HEAD~1 HEAD -- existing/path" while the working directory
> > is the repo subfolder "actual" works, I epxected the difftool to work
> > similarly. Is this a bug?
> 
> I think it is, yes.  The patch below fixes it for me and doesn't break
> any existing tests, but I still don't understand why the separate
> $diffrepo was needed originally, so I'm not certain this won't break
> some other corner case.


IIRC the original motivation for using a separate $diffrepo was
to handle GIT_DIR being set in the environment.

The lack of tests for that use case could be better, though.
Is that use case affected by this change?

Tim, do you remember why a new repo instance is used for that code path?


> -- >8 --
> When in a subdirectory of a repository, path arguments should be
> interpreted relative to the current directory not the root of the
> working tree.
> 
> The Git::repository object passed into setup_dir_diff() is configured to
> handle this correctly but we create a new Git::repository here without
> setting the WorkingSubdir argument.  By simply using the existing
> repository, path arguments are handled relative to the current
> directory.

I do like the sound of this rationale, though.

Tim, please let us know if you have a specific test case that is
not covered by this change.

BTW, `diff --raw` will still output paths that are relative to
the root but this is okay since the rest of the code expects
things to be root-relative, correct?


> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
>  git-difftool.perl | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/git-difftool.perl b/git-difftool.perl
> index ebd13ba..c9d3ef8 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -115,16 +115,9 @@ sub setup_dir_diff
>  {
>  	my ($repo, $workdir, $symlinks) = @_;
>  
> -	# Run the diff; exit immediately if no diff found
> -	# 'Repository' and 'WorkingCopy' must be explicitly set to insure that
> -	# if $GIT_DIR and $GIT_WORK_TREE are set in ENV, they are actually used
> -	# by Git->repository->command*.
>  	my $repo_path = $repo->repo_path();
> -	my %repo_args = (Repository => $repo_path, WorkingCopy => $workdir);
> -	my $diffrepo = Git->repository(%repo_args);
> -
>  	my @gitargs = ('diff', '--raw', '--no-abbrev', '-z', @ARGV);
> -	my $diffrtn = $diffrepo->command_oneline(@gitargs);
> +	my $diffrtn = $repo->command_oneline(@gitargs);
>  	exit(0) unless defined($diffrtn);
>  
>  	# Build index info for left and right sides of the diff
> @@ -176,12 +169,12 @@ EOF
>  
>  		if ($lmode eq $symlink_mode) {
>  			$symlink{$src_path}{left} =
> -				$diffrepo->command_oneline('show', "$lsha1");
> +				$repo->command_oneline('show', "$lsha1");
>  		}
>  
>  		if ($rmode eq $symlink_mode) {
>  			$symlink{$dst_path}{right} =
> -				$diffrepo->command_oneline('show', "$rsha1");
> +				$repo->command_oneline('show', "$rsha1");
>  		}
>  
>  		if ($lmode ne $null_mode and $status !~ /^C/) {
> -- 

Can you please also add a testcase to t/t7800-difftool.sh
demonstrating the problem fixed by this change?

Hopefully there's an existing test in there that can be adapted
to run dir-diffs in a subdirectory.

ciao,
-- 
David