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

From: David Aguilar <davvid@gmail.com>
Date: Jul 12, 2016, 02:10 UTC
Message-ID: <20160712021048.GA20679@gmail.com>
In-Reply-To: <20160705195252.hzf5hvrcub3g32gg@john.keeping.me.uk>

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

On Tue, Jul 05, 2016 at 08:52:52PM +0100, John Keeping wrote:

Show 21 quoted lines

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

Show 10 quoted lines

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

Show 43 quoted lines

> 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