{"thread":{"id":"35941","subject":"[PATCH] difftool: support repositories with .git-files","startedAt":"2014-02-24T03:12:35Z","lastAt":"2014-03-05T09:23:35Z","messageCount":8,"participants":["David Aguilar","Junio C Hamano","Jens Lehmann"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"235217","messageId":"1393211555-50270-1-git-send-email-davvid@gmail.com","threadId":"35941","inReplyTo":null,"subject":"[PATCH] difftool: support repositories with .git-files","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-02-24T03:12:35Z","receivedAt":"2014-02-24T03:12:35Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Modern versions of \"git submodule\" use .git-files to setup the\nsubmodule directory. When run in a \"git submodule\"-created\nrepository \"git difftool --dir-diff\" dies with the following\nerror:\n\n\t$ git difftool -d HEAD~\n\tfatal: This operation must be run in a work tree\n\tdiff --raw --no-abbrev -z HEAD~: command returned error: 128\n\ncore.worktree is relative to the .git directory but the logic\nin find_worktree() does not account for it.\n\nUse `git rev-parse --show-toplevel` to find the worktree so that\nthe dir-diff feature works inside a submodule.\n\nReported-by: Gábor Lipták \nHelped-by: Jens Lehmann \nHelped-by: John Keeping \nSigned-off-by: David Aguilar \n---\n git-difftool.perl | 18 ++----------------\n 1 file changed, 2 insertions(+), 16 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex e57d3d1..18ca61e 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -39,24 +39,10 @@ USAGE\n \n sub find_worktree\n {\n-\tmy ($repo) = @_;\n-\n \t# Git->repository->wc_path() does not honor changes to the working\n \t# tree location made by $ENV{GIT_WORK_TREE} or the 'core.worktree'\n \t# config variable.\n-\tmy $worktree;\n-\tmy $env_worktree = $ENV{GIT_WORK_TREE};\n-\tmy $core_worktree = Git::config('core.worktree');\n-\n-\tif (defined($env_worktree) and (length($env_worktree) > 0)) {\n-\t\t$worktree = $env_worktree;\n-\t} elsif (defined($core_worktree) and (length($core_worktree) > 0)) {\n-\t\t$worktree = $core_worktree;\n-\t} else {\n-\t\t$worktree = $repo->wc_path();\n-\t}\n-\n-\treturn $worktree;\n+\treturn Git::command_oneline('rev-parse', '--show-toplevel');\n }\n \n sub print_tool_help\n@@ -418,7 +404,7 @@ sub dir_diff\n \tmy $rc;\n \tmy $error = 0;\n \tmy $repo = Git->repository();\n-\tmy $workdir = find_worktree($repo);\n+\tmy $workdir = find_worktree();\n \tmy ($a, $b, $tmpdir, @worktree) =\n \t\tsetup_dir_diff($repo, $workdir, $symlinks);\n \n-- \n1.9.0.1.gd20a678\n"},{"id":"235274","messageId":"xmqqr46s9yzx.fsf@gitster.dls.corp.google.com","threadId":"35941","inReplyTo":"1393211555-50270-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH] difftool: support repositories with .git-files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-24T17:55:30Z","receivedAt":"2014-02-24T17:55:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> Modern versions of \"git submodule\" use .git-files to setup the\n> submodule directory. When run in a \"git submodule\"-created\n> repository \"git difftool --dir-diff\" dies with the following\n> error:\n>\n> \t$ git difftool -d HEAD~\n> \tfatal: This operation must be run in a work tree\n> \tdiff --raw --no-abbrev -z HEAD~: command returned error: 128\n>\n> core.worktree is relative to the .git directory but the logic\n> in find_worktree() does not account for it.\n>\n> Use `git rev-parse --show-toplevel` to find the worktree so that\n> the dir-diff feature works inside a submodule.\n>\n> Reported-by: Gábor Lipták \n> Helped-by: Jens Lehmann \n> Helped-by: John Keeping \n> Signed-off-by: David Aguilar \n> ---\n\nLooks good; thanks.\n\n> git-difftool.perl | 18 ++----------------\n> 1 file changed, 2 insertions(+), 16 deletions(-)\n>\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index e57d3d1..18ca61e 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -39,24 +39,10 @@ USAGE\n> \n> sub find_worktree\n> {\n> -\tmy ($repo) = @_;\n> -\n> \t# Git->repository->wc_path() does not honor changes to the working\n> \t# tree location made by $ENV{GIT_WORK_TREE} or the 'core.worktree'\n> \t# config variable.\n> -\tmy $worktree;\n> -\tmy $env_worktree = $ENV{GIT_WORK_TREE};\n> -\tmy $core_worktree = Git::config('core.worktree');\n> -\n> -\tif (defined($env_worktree) and (length($env_worktree) > 0)) {\n> -\t\t$worktree = $env_worktree;\n> -\t} elsif (defined($core_worktree) and (length($core_worktree) > 0)) {\n> -\t\t$worktree = $core_worktree;\n> -\t} else {\n> -\t\t$worktree = $repo->wc_path();\n> -\t}\n> -\n> -\treturn $worktree;\n> +\treturn Git::command_oneline('rev-parse', '--show-toplevel');\n> }\n> \n> sub print_tool_help\n> @@ -418,7 +404,7 @@ sub dir_diff\n> \tmy $rc;\n> \tmy $error = 0;\n> \tmy $repo = Git->repository();\n> -\tmy $workdir = find_worktree($repo);\n> +\tmy $workdir = find_worktree();\n> \tmy ($a, $b, $tmpdir, @worktree) =\n> \t\tsetup_dir_diff($repo, $workdir, $symlinks);\n"},{"id":"235300","messageId":"530BB548.3050603@web.de","threadId":"35941","inReplyTo":"xmqqr46s9yzx.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] difftool: support repositories with .git-files","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2014-02-24T21:10:32Z","receivedAt":"2014-02-24T21:10:32Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 24.02.2014 17:55, schrieb Junio C Hamano:\n> David Aguilar writes:\n> \n>> Modern versions of \"git submodule\" use .git-files to setup the\n>> submodule directory. When run in a \"git submodule\"-created\n>> repository \"git difftool --dir-diff\" dies with the following\n>> error:\n>>\n>> \t$ git difftool -d HEAD~\n>> \tfatal: This operation must be run in a work tree\n>> \tdiff --raw --no-abbrev -z HEAD~: command returned error: 128\n>>\n>> core.worktree is relative to the .git directory but the logic\n>> in find_worktree() does not account for it.\n>>\n>> Use `git rev-parse --show-toplevel` to find the worktree so that\n>> the dir-diff feature works inside a submodule.\n>>\n>> Reported-by: Gábor Lipták \n>> Helped-by: Jens Lehmann \n>> Helped-by: John Keeping \n>> Signed-off-by: David Aguilar \n>> ---\n> \n> Looks good; thanks.\n\n\nFWIW:\nTested-by: Jens Lehmann \n\nWhat about squashing this in to detect any future regressions?\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 2418528..d86ad68 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -434,4 +434,12 @@ test_expect_success PERL 'difftool --no-symlinks detects conflict ' '\n \t)\n '\n\n+test_expect_success PERL 'difftool properly honours gitlink and core.worktree' '\n+\tgit submodule add ./. submod/ule &&\n+\t(\n+\t\tcd submod/ule &&\n+\t\tgit difftool --tool=echo --dir-diff --cached\n+\t)\n+'\n+\n test_done\n"},{"id":"235330","messageId":"xmqq1tyr6pfw.fsf@gitster.dls.corp.google.com","threadId":"35941","inReplyTo":"530BB548.3050603@web.de","subject":"Re: [PATCH] difftool: support repositories with .git-files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-25T18:02:27Z","receivedAt":"2014-02-25T18:02:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann writes:\n\n> Am 24.02.2014 17:55, schrieb Junio C Hamano:\n>> David Aguilar writes:\n>> \n>>> Modern versions of \"git submodule\" use .git-files to setup the\n>>> submodule directory. When run in a \"git submodule\"-created\n>>> repository \"git difftool --dir-diff\" dies with the following\n>>> error:\n>>>\n>>> \t$ git difftool -d HEAD~\n>>> \tfatal: This operation must be run in a work tree\n>>> \tdiff --raw --no-abbrev -z HEAD~: command returned error: 128\n>>>\n>>> core.worktree is relative to the .git directory but the logic\n>>> in find_worktree() does not account for it.\n>>>\n>>> Use `git rev-parse --show-toplevel` to find the worktree so that\n>>> the dir-diff feature works inside a submodule.\n>>>\n>>> Reported-by: Gábor Lipták \n>>> Helped-by: Jens Lehmann \n>>> Helped-by: John Keeping \n>>> Signed-off-by: David Aguilar \n>>> ---\n>> \n>> Looks good; thanks.\n>\n>\n> FWIW:\n> Tested-by: Jens Lehmann \n>\n> What about squashing this in to detect any future regressions?\n>\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index 2418528..d86ad68 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -434,4 +434,12 @@ test_expect_success PERL 'difftool --no-symlinks detects conflict ' '\n> \t)\n> '\n>\n> +test_expect_success PERL 'difftool properly honours gitlink and core.worktree' '\n> +\tgit submodule add ./. submod/ule &&\n> +\t(\n> +\t\tcd submod/ule &&\n> +\t\tgit difftool --tool=echo --dir-diff --cached\n\nIn the context of this fix, finishing with 0 exit status may be all\nwe care about, but do we also care about things like in what\ndirectory the tool is invoked in, what arguments and extra\nenvironment settings (if any) it is given, and stuff like that?\n\nIn fact, the \"echo\" in the above is very misleading. The test\nrelies on the fact that immediately after the submod/ule is cloned,\n\"diff --cached\" does not have to call any tool backend---if you\nmodify some tracked file in its working tree and dropped --cached\non the command line, the command will fail with \"Huh? I do not know\nwhat 'echo' diff/merge backend is\", no?\n\n> +\t)\n> +'\n> +\n> test_done\n"},{"id":"235333","messageId":"530CFE56.6040807@web.de","threadId":"35941","inReplyTo":"xmqq1tyr6pfw.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] difftool: support repositories with .git-files","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2014-02-25T20:34:30Z","receivedAt":"2014-02-25T20:34:30Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 25.02.2014 18:02, schrieb Junio C Hamano:\n> Jens Lehmann writes:\n> \n>> Am 24.02.2014 17:55, schrieb Junio C Hamano:\n>>> David Aguilar writes:\n>>>\n>>>> Modern versions of \"git submodule\" use .git-files to setup the\n>>>> submodule directory. When run in a \"git submodule\"-created\n>>>> repository \"git difftool --dir-diff\" dies with the following\n>>>> error:\n>>>>\n>>>> \t$ git difftool -d HEAD~\n>>>> \tfatal: This operation must be run in a work tree\n>>>> \tdiff --raw --no-abbrev -z HEAD~: command returned error: 128\n>>>>\n>>>> core.worktree is relative to the .git directory but the logic\n>>>> in find_worktree() does not account for it.\n>>>>\n>>>> Use `git rev-parse --show-toplevel` to find the worktree so that\n>>>> the dir-diff feature works inside a submodule.\n>>>>\n>>>> Reported-by: Gábor Lipták \n>>>> Helped-by: Jens Lehmann \n>>>> Helped-by: John Keeping \n>>>> Signed-off-by: David Aguilar \n>>>> ---\n>>>\n>>> Looks good; thanks.\n>>\n>>\n>> FWIW:\n>> Tested-by: Jens Lehmann \n>>\n>> What about squashing this in to detect any future regressions?\n>>\n>> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n>> index 2418528..d86ad68 100755\n>> --- a/t/t7800-difftool.sh\n>> +++ b/t/t7800-difftool.sh\n>> @@ -434,4 +434,12 @@ test_expect_success PERL 'difftool --no-symlinks detects conflict ' '\n>> \t)\n>> '\n>>\n>> +test_expect_success PERL 'difftool properly honours gitlink and core.worktree' '\n>> +\tgit submodule add ./. submod/ule &&\n>> +\t(\n>> +\t\tcd submod/ule &&\n>> +\t\tgit difftool --tool=echo --dir-diff --cached\n> \n> In the context of this fix, finishing with 0 exit status may be all\n> we care about, but do we also care about things like in what\n> directory the tool is invoked in, what arguments and extra\n> environment settings (if any) it is given, and stuff like that?\n\nSure. But I just intended to test the fix (and the test can easily\nbe extended by people who know more about difftool than I do).\n\n> In fact, the \"echo\" in the above is very misleading. The test\n> relies on the fact that immediately after the submod/ule is cloned,\n> \"diff --cached\" does not have to call any tool backend---if you\n> modify some tracked file in its working tree and dropped --cached\n> on the command line, the command will fail with \"Huh? I do not know\n> what 'echo' diff/merge backend is\", no?\n\nRight, using echo was not the best choice here. I used it to avoid\nthe dependency to meld in the example of the OP (maybe using \"true\"\nas tool would have indicated that the tool is not important here,\nbut looking into this again a simple \"git difftool --dir-diff\"\nwithout any further arguments also shows that the fix is working).\n\nAas mentioned above, I'm not familiar with difftool and just wanted\nto share an easy way to test the fix. But I do not care too deeply\nabout this test, so feel free to ignore it.\n"},{"id":"235339","messageId":"xmqqd2ia6dvo.fsf@gitster.dls.corp.google.com","threadId":"35941","inReplyTo":"530CFE56.6040807@web.de","subject":"Re: [PATCH] difftool: support repositories with .git-files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-25T22:12:11Z","receivedAt":"2014-02-25T22:12:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann writes:\n\n>>> +test_expect_success PERL 'difftool properly honours gitlink and core.worktree' '\n>>> +\tgit submodule add ./. submod/ule &&\n>>> +\t(\n>>> +\t\tcd submod/ule &&\n>>> +\t\tgit difftool --tool=echo --dir-diff --cached\n>> \n>> In the context of this fix, finishing with 0 exit status may be all\n>> we care about, but do we also care about things like in what\n>> directory the tool is invoked in, what arguments and extra\n>> environment settings (if any) it is given, and stuff like that?\n>\n> Sure. But I just intended to test the fix (and the test can easily\n> be extended by people who know more about difftool than I do).\n\nYes, we need to start somewhere and I'd agree that it was a good\nstarting point.\n\n> Right, using echo was not the best choice here. I used it to avoid\n> the dependency to meld...\n\nPerhaps like this then? This is an \"a monkey sees what\ndifftool_test_setup does and then mimics\" patch ;-).\n\n t/t7800-difftool.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 2418528..595f808 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -434,4 +434,17 @@ test_expect_success PERL 'difftool --no-symlinks detects conflict ' '\n \t)\n '\n \n+test_expect_success PERL 'difftool properly honours gitlink and core.worktree' '\n+\tgit submodule add ./. submod/ule &&\n+\t(\n+\t\tcd submod/ule &&\n+\t\tgit config diff.tool checktrees &&\n+\t\tgit config difftool.checktrees.cmd '\\''\n+\t\t\ttest -d \"$LOCAL\" && test -d \"$REMOTE\"\n+\t\t'\\'' &&\n+\t\techo further >>file &&\n+\t\tgit difftool --tool=checktrees --dir-diff\n+\t)\n+'\n+\n test_done\n"},{"id":"235511","messageId":"530FAA2D.8060104@web.de","threadId":"35941","inReplyTo":"xmqqd2ia6dvo.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] difftool: support repositories with .git-files","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2014-02-27T21:12:13Z","receivedAt":"2014-02-27T21:12:13Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 25.02.2014 22:12, schrieb Junio C Hamano:\n> Jens Lehmann writes:\n> \n>>>> +test_expect_success PERL 'difftool properly honours gitlink and core.worktree' '\n>>>> +\tgit submodule add ./. submod/ule &&\n>>>> +\t(\n>>>> +\t\tcd submod/ule &&\n>>>> +\t\tgit difftool --tool=echo --dir-diff --cached\n>>>\n>>> In the context of this fix, finishing with 0 exit status may be all\n>>> we care about, but do we also care about things like in what\n>>> directory the tool is invoked in, what arguments and extra\n>>> environment settings (if any) it is given, and stuff like that?\n>>\n>> Sure. But I just intended to test the fix (and the test can easily\n>> be extended by people who know more about difftool than I do).\n> \n> Yes, we need to start somewhere and I'd agree that it was a good\n> starting point.\n> \n>> Right, using echo was not the best choice here. I used it to avoid\n>> the dependency to meld...\n> \n> Perhaps like this then? This is an \"a monkey sees what\n> difftool_test_setup does and then mimics\" patch ;-).\n\nNicely done :-)\n\n> t/t7800-difftool.sh | 13 +++++++++++++\n> 1 file changed, 13 insertions(+)\n> \n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index 2418528..595f808 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -434,4 +434,17 @@ test_expect_success PERL 'difftool --no-symlinks detects conflict ' '\n> \t)\n> '\n> \n> +test_expect_success PERL 'difftool properly honours gitlink and core.worktree' '\n> +\tgit submodule add ./. submod/ule &&\n> +\t(\n> +\t\tcd submod/ule &&\n> +\t\tgit config diff.tool checktrees &&\n> +\t\tgit config difftool.checktrees.cmd '\\''\n> +\t\t\ttest -d \"$LOCAL\" && test -d \"$REMOTE\"\n> +\t\t'\\'' &&\n> +\t\techo further >>file &&\n> +\t\tgit difftool --tool=checktrees --dir-diff\n> +\t)\n> +'\n> +\n> test_done\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"236078","messageId":"1394011415-99543-1-git-send-email-davvid@gmail.com","threadId":"35941","inReplyTo":"530FAA2D.8060104@web.de","subject":"[PATCH] t7800: add a difftool test for .git-files","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-03-05T09:23:35Z","receivedAt":"2014-03-05T09:23:35Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"From: Junio C Hamano \n\nSigned-off-by: David Aguilar \n---\nThis is a replacement patch for the current tip of da/difftool.\n\n t/t7800-difftool.sh | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 2418528..986b78e 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -434,4 +434,18 @@ test_expect_success PERL 'difftool --no-symlinks detects conflict ' '\n \t)\n '\n \n+test_expect_success PERL 'difftool properly honors gitlink and core.worktree' '\n+\tgit submodule add ./. submod/ule &&\n+\t(\n+\t\tcd submod/ule &&\n+\t\ttest_config diff.tool checktrees &&\n+\t\ttest_config difftool.checktrees.cmd '\\''\n+\t\t\ttest -d \"$LOCAL\" && test -d \"$REMOTE\" && echo good\n+\t\t'\\'' &&\n+\t\techo good>expect &&\n+\t\tgit difftool --tool=checktrees --dir-diff HEAD~ >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n-- \n1.8.5.5.2.g42fdfc9\n"}]}