{"thread":{"id":"42774","subject":"--dir-diff not working with partial path limiter","startedAt":"2016-07-04T18:47:41Z","lastAt":"2016-07-12T02:10:57Z","messageCount":3,"participants":["Bernhard Kirchen","John Keeping","David Aguilar"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"290801","messageId":"155b7339538.2774.8c011de0e6d4f677db1e190e9d3169b9@rwth-aachen.de","threadId":"42774","inReplyTo":"OFEE90CED0.0832E3D4-ONC1257FE9.0053D856-C1257FE6.00660366@lancom.de","subject":"--dir-diff not working with partial path limiter","fromName":"Bernhard Kirchen","fromEmail":"bernhard.kirchen@rwth-aachen.de","sentAt":"2016-07-04T18:37:39Z","receivedAt":"2016-07-04T18:47:41Z","isPatch":false,"sender":{"key":"bernhard.kirchen@rwth-aachen.de","avatar":null},"body":"Hello!\n\nToday I started using --dir-diff and noticed a problem when specifying a\nnon-full path limiter. My diff tool is setup to be meld (*1).\n\nOK while working directory is repo root; also OK while working directory is\nrepo subfolder \"actual\":\ngit difftool --dir-diff HEAD~1 HEAD -- actual/existing/path\n=> meld opens with proper dir-diff.\n\nNOT OK while working directory is repo subfolder \"actual\":\ngit difftool --dir-diff HEAD~1 HEAD -- existing/path\n=> nothing happens, as if using \"non/such/path\" as the path limiter.\n\nBecause \"git diff HEAD~1 HEAD -- existing/path\" while the working directory\nis the repo subfolder \"actual\" works, I epxected the difftool to work\nsimilarly. Is this a bug?\n\nBest,\nBernhard\n\n(*1)\n[diff]\ntool = mydiffmeld\n[difftool \"mydiffmeld\"]\ncmd = meld --auto-compare --diff $LOCAL $REMOTE\n[difftool]\nprompt = false\n\n\n"},{"id":"290857","messageId":"20160705195252.hzf5hvrcub3g32gg@john.keeping.me.uk","threadId":"42774","inReplyTo":"155b7339538.2774.8c011de0e6d4f677db1e190e9d3169b9@rwth-aachen.de","subject":"[PATCH] difftool: fix argument handling in subdirs","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2016-07-05T19:52:52Z","receivedAt":"2016-07-05T19:53:12Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Mon, Jul 04, 2016 at 08:37:39PM +0200, Bernhard Kirchen wrote:\n> Today I started using --dir-diff and noticed a problem when specifying a\n> non-full path limiter. My diff tool is setup to be meld (*1).\n> \n> OK while working directory is repo root; also OK while working directory is\n> repo subfolder \"actual\":\n> git difftool --dir-diff HEAD~1 HEAD -- actual/existing/path\n> => meld opens with proper dir-diff.\n> \n> NOT OK while working directory is repo subfolder \"actual\":\n> git difftool --dir-diff HEAD~1 HEAD -- existing/path\n> => nothing happens, as if using \"non/such/path\" as the path limiter.\n> \n> Because \"git diff HEAD~1 HEAD -- existing/path\" while the working directory\n> is the repo subfolder \"actual\" works, I epxected the difftool to work\n> similarly. Is this a bug?\n\nI think it is, yes.  The patch below fixes it for me and doesn't break\nany existing tests, but I still don't understand why the separate\n$diffrepo was needed originally, so I'm not certain this won't break\nsome other corner case.\n\n-- >8 --\nWhen in a subdirectory of a repository, path arguments should be\ninterpreted relative to the current directory not the root of the\nworking tree.\n\nThe Git::repository object passed into setup_dir_diff() is configured to\nhandle this correctly but we create a new Git::repository here without\nsetting the WorkingSubdir argument.  By simply using the existing\nrepository, path arguments are handled relative to the current\ndirectory.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n git-difftool.perl | 13 +++----------\n 1 file changed, 3 insertions(+), 10 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex ebd13ba..c9d3ef8 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -115,16 +115,9 @@ sub setup_dir_diff\n {\n \tmy ($repo, $workdir, $symlinks) = @_;\n \n-\t# Run the diff; exit immediately if no diff found\n-\t# 'Repository' and 'WorkingCopy' must be explicitly set to insure that\n-\t# if $GIT_DIR and $GIT_WORK_TREE are set in ENV, they are actually used\n-\t# by Git->repository->command*.\n \tmy $repo_path = $repo->repo_path();\n-\tmy %repo_args = (Repository => $repo_path, WorkingCopy => $workdir);\n-\tmy $diffrepo = Git->repository(%repo_args);\n-\n \tmy @gitargs = ('diff', '--raw', '--no-abbrev', '-z', @ARGV);\n-\tmy $diffrtn = $diffrepo->command_oneline(@gitargs);\n+\tmy $diffrtn = $repo->command_oneline(@gitargs);\n \texit(0) unless defined($diffrtn);\n \n \t# Build index info for left and right sides of the diff\n@@ -176,12 +169,12 @@ EOF\n \n \t\tif ($lmode eq $symlink_mode) {\n \t\t\t$symlink{$src_path}{left} =\n-\t\t\t\t$diffrepo->command_oneline('show', \"$lsha1\");\n+\t\t\t\t$repo->command_oneline('show', \"$lsha1\");\n \t\t}\n \n \t\tif ($rmode eq $symlink_mode) {\n \t\t\t$symlink{$dst_path}{right} =\n-\t\t\t\t$diffrepo->command_oneline('show', \"$rsha1\");\n+\t\t\t\t$repo->command_oneline('show', \"$rsha1\");\n \t\t}\n \n \t\tif ($lmode ne $null_mode and $status !~ /^C/) {\n-- \n2.9.0.465.g8850cbc\n\n"},{"id":"291234","messageId":"20160712021048.GA20679@gmail.com","threadId":"42774","inReplyTo":"20160705195252.hzf5hvrcub3g32gg@john.keeping.me.uk","subject":"Re: [PATCH] difftool: fix argument handling in subdirs","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2016-07-12T02:10:48Z","receivedAt":"2016-07-12T02:10:57Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"[Cc'd Tim, who originally authored the dir-diff code]\n\nOn Tue, Jul 05, 2016 at 08:52:52PM +0100, John Keeping wrote:\n> On Mon, Jul 04, 2016 at 08:37:39PM +0200, Bernhard Kirchen wrote:\n> > Today I started using --dir-diff and noticed a problem when specifying a\n> > non-full path limiter. My diff tool is setup to be meld (*1).\n> > \n> > OK while working directory is repo root; also OK while working directory is\n> > repo subfolder \"actual\":\n> > git difftool --dir-diff HEAD~1 HEAD -- actual/existing/path\n> > => meld opens with proper dir-diff.\n> > \n> > NOT OK while working directory is repo subfolder \"actual\":\n> > git difftool --dir-diff HEAD~1 HEAD -- existing/path\n> > => nothing happens, as if using \"non/such/path\" as the path limiter.\n> > \n> > Because \"git diff HEAD~1 HEAD -- existing/path\" while the working directory\n> > is the repo subfolder \"actual\" works, I epxected the difftool to work\n> > similarly. Is this a bug?\n> \n> I think it is, yes.  The patch below fixes it for me and doesn't break\n> any existing tests, but I still don't understand why the separate\n> $diffrepo was needed originally, so I'm not certain this won't break\n> some other corner case.\n\n\nIIRC the original motivation for using a separate $diffrepo was\nto handle GIT_DIR being set in the environment.\n\nThe lack of tests for that use case could be better, though.\nIs that use case affected by this change?\n\nTim, do you remember why a new repo instance is used for that code path?\n\n\n> -- >8 --\n> When in a subdirectory of a repository, path arguments should be\n> interpreted relative to the current directory not the root of the\n> working tree.\n> \n> The Git::repository object passed into setup_dir_diff() is configured to\n> handle this correctly but we create a new Git::repository here without\n> setting the WorkingSubdir argument.  By simply using the existing\n> repository, path arguments are handled relative to the current\n> directory.\n\nI do like the sound of this rationale, though.\n\nTim, please let us know if you have a specific test case that is\nnot covered by this change.\n\nBTW, `diff --raw` will still output paths that are relative to\nthe root but this is okay since the rest of the code expects\nthings to be root-relative, correct?\n\n\n> Signed-off-by: John Keeping <john@keeping.me.uk>\n> ---\n>  git-difftool.perl | 13 +++----------\n>  1 file changed, 3 insertions(+), 10 deletions(-)\n> \n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index ebd13ba..c9d3ef8 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -115,16 +115,9 @@ sub setup_dir_diff\n>  {\n>  \tmy ($repo, $workdir, $symlinks) = @_;\n>  \n> -\t# Run the diff; exit immediately if no diff found\n> -\t# 'Repository' and 'WorkingCopy' must be explicitly set to insure that\n> -\t# if $GIT_DIR and $GIT_WORK_TREE are set in ENV, they are actually used\n> -\t# by Git->repository->command*.\n>  \tmy $repo_path = $repo->repo_path();\n> -\tmy %repo_args = (Repository => $repo_path, WorkingCopy => $workdir);\n> -\tmy $diffrepo = Git->repository(%repo_args);\n> -\n>  \tmy @gitargs = ('diff', '--raw', '--no-abbrev', '-z', @ARGV);\n> -\tmy $diffrtn = $diffrepo->command_oneline(@gitargs);\n> +\tmy $diffrtn = $repo->command_oneline(@gitargs);\n>  \texit(0) unless defined($diffrtn);\n>  \n>  \t# Build index info for left and right sides of the diff\n> @@ -176,12 +169,12 @@ EOF\n>  \n>  \t\tif ($lmode eq $symlink_mode) {\n>  \t\t\t$symlink{$src_path}{left} =\n> -\t\t\t\t$diffrepo->command_oneline('show', \"$lsha1\");\n> +\t\t\t\t$repo->command_oneline('show', \"$lsha1\");\n>  \t\t}\n>  \n>  \t\tif ($rmode eq $symlink_mode) {\n>  \t\t\t$symlink{$dst_path}{right} =\n> -\t\t\t\t$diffrepo->command_oneline('show', \"$rsha1\");\n> +\t\t\t\t$repo->command_oneline('show', \"$rsha1\");\n>  \t\t}\n>  \n>  \t\tif ($lmode ne $null_mode and $status !~ /^C/) {\n> -- \n\nCan you please also add a testcase to t/t7800-difftool.sh\ndemonstrating the problem fixed by this change?\n\nHopefully there's an existing test in there that can be adapted\nto run dir-diffs in a subdirectory.\n\nciao,\n-- \nDavid\n"}]}