{"thread":{"id":"45043","subject":"[PATCH] Remove --no-gui option from difftool usage string","startedAt":"2017-02-04T02:56:32Z","lastAt":"2017-02-08T15:38:27Z","messageCount":12,"participants":["Denton Liu","Jacob Keller","David Aguilar","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"310829","messageId":"20170204025617.GA9221@arch-attack.localdomain","threadId":"45043","inReplyTo":null,"subject":"[PATCH] Remove --no-gui option from difftool usage string","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2017-02-04T02:56:17Z","receivedAt":"2017-02-04T02:56:32Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The --no-gui option not documented in the manpage, nor is it actually\nused in the source code. This change removes it from the usage help\nthat's printed.\n\nSigned-off-by: Denton Liu \n---\n git-difftool.perl | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex a5790d03a..657d5622d 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -29,8 +29,8 @@ sub usage\n \tprint << 'USAGE';\n usage: git difftool [-t|--tool=] [--tool-help]\n [-x|--extcmd=]\n- [-g|--gui] [--no-gui]\n- [--prompt] [-y|--no-prompt]\n+ [-g|--gui] [--prompt]\n+ [-y|--no-prompt]\n [-d|--dir-diff]\n ['git diff' options]\n USAGE\n-- \n2.11.0\n\n"},{"id":"310834","messageId":"CA+P7+xoXWP=Cuqw3Pa1sFCChiw6wbNTEpvNm3WDuBHQPc_OjnA@mail.gmail.com","threadId":"45043","inReplyTo":"20170204025617.GA9221@arch-attack.localdomain","subject":"Re: [PATCH] Remove --no-gui option from difftool usage string","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-02-04T05:58:09Z","receivedAt":"2017-02-04T05:58:37Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Feb 3, 2017 at 6:56 PM, Denton Liu wrote:\n> The --no-gui option not documented in the manpage, nor is it actually\n> used in the source code. This change removes it from the usage help\n> that's printed.\n>\n> Signed-off-by: Denton Liu \n> ---\n> git-difftool.perl | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index a5790d03a..657d5622d 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -29,8 +29,8 @@ sub usage\n> print << 'USAGE';\n> usage: git difftool [-t|--tool=] [--tool-help]\n> [-x|--extcmd=]\n> - [-g|--gui] [--no-gui]\n> - [--prompt] [-y|--no-prompt]\n> + [-g|--gui] [--prompt]\n> + [-y|--no-prompt]\n> [-d|--dir-diff]\n> ['git diff' options]\n> USAGE\n> --\n> 2.11.0\n>\n\nAren't \"--no-foo\" options automatically created for booleans when\nusing our option parsing code?\n\nThanks,\nJake\n"},{"id":"310836","messageId":"20170204062351.GA11277@arch-attack.localdomain","threadId":"45043","inReplyTo":"CA+P7+xoXWP=Cuqw3Pa1sFCChiw6wbNTEpvNm3WDuBHQPc_OjnA@mail.gmail.com","subject":"Re: [PATCH] Remove --no-gui option from difftool usage string","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2017-02-04T06:23:51Z","receivedAt":"2017-02-04T06:23:59Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Fri, Feb 03, 2017 at 09:58:09PM -0800, Jacob Keller wrote:\n> On Fri, Feb 3, 2017 at 6:56 PM, Denton Liu wrote:\n> > The --no-gui option not documented in the manpage, nor is it actually\n> > used in the source code. This change removes it from the usage help\n> > that's printed.\n> >\n> > Signed-off-by: Denton Liu \n> > ---\n> > git-difftool.perl | 4 ++--\n> > 1 file changed, 2 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/git-difftool.perl b/git-difftool.perl\n> > index a5790d03a..657d5622d 100755\n> > --- a/git-difftool.perl\n> > +++ b/git-difftool.perl\n> > @@ -29,8 +29,8 @@ sub usage\n> > print << 'USAGE';\n> > usage: git difftool [-t|--tool=] [--tool-help]\n> > [-x|--extcmd=]\n> > - [-g|--gui] [--no-gui]\n> > - [--prompt] [-y|--no-prompt]\n> > + [-g|--gui] [--prompt]\n> > + [-y|--no-prompt]\n> > [-d|--dir-diff]\n> > ['git diff' options]\n> > USAGE\n> > --\n> > 2.11.0\n> >\n> \n> Aren't \"--no-foo\" options automatically created for booleans when\n> using our option parsing code?\n> \n> Thanks,\n> Jake\n\nSorry, I guess I didn't notice that. Would it make sense to document it\nin the manpage then?\n\n--\nDenton Liu\n"},{"id":"310869","messageId":"20170205102246.a3wwipbnmqrxxbio@gmail.com","threadId":"45043","inReplyTo":"20170204062351.GA11277@arch-attack.localdomain","subject":"Re: [PATCH] Remove --no-gui option from difftool usage string","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2017-02-05T10:22:46Z","receivedAt":"2017-02-05T10:22:54Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Fri, Feb 03, 2017 at 10:23:51PM -0800, Denton Liu wrote:\n> On Fri, Feb 03, 2017 at 09:58:09PM -0800, Jacob Keller wrote:\n> > On Fri, Feb 3, 2017 at 6:56 PM, Denton Liu wrote:\n> > > The --no-gui option not documented in the manpage, nor is it actually\n> > > used in the source code. This change removes it from the usage help\n> > > that's printed.\n> > >\n> > > Signed-off-by: Denton Liu \n> > > ---\n> > > git-difftool.perl | 4 ++--\n> > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/git-difftool.perl b/git-difftool.perl\n> > > index a5790d03a..657d5622d 100755\n> > > --- a/git-difftool.perl\n> > > +++ b/git-difftool.perl\n> > > @@ -29,8 +29,8 @@ sub usage\n> > > print << 'USAGE';\n> > > usage: git difftool [-t|--tool=] [--tool-help]\n> > > [-x|--extcmd=]\n> > > - [-g|--gui] [--no-gui]\n> > > - [--prompt] [-y|--no-prompt]\n> > > + [-g|--gui] [--prompt]\n> > > + [-y|--no-prompt]\n> > > [-d|--dir-diff]\n> > > ['git diff' options]\n> > > USAGE\n> > > --\n> > > 2.11.0\n> > >\n> > \n> > Aren't \"--no-foo\" options automatically created for booleans when\n> > using our option parsing code?\n> > \n> > Thanks,\n> > Jake\n> \n> Sorry, I guess I didn't notice that. Would it make sense to document it\n> in the manpage then?\n\nThe general \"--no-*\" convention is mentioned in \"git help cli\",\nbut the manpages and usage strings across all commands are\ninconsistent about mentioning the \"--no-*\" variants; some do,\nsome don't.\n\nIMO it probably wouldn't hurt to document --no-gui in the manpage.\n\ncheers,\n-- \nDavid\n"},{"id":"310879","messageId":"20170205201751.z4rfmy5xxaqg472l@gmail.com","threadId":"45043","inReplyTo":"20170204025617.GA9221@arch-attack.localdomain","subject":"[BUG] was: Re: [PATCH] Remove --no-gui option from difftool usage string","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2017-02-05T20:17:51Z","receivedAt":"2017-02-05T20:18:01Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"\nOn Fri, Feb 03, 2017 at 06:56:17PM -0800, Denton Liu wrote:\n> The --no-gui option not documented in the manpage, nor is it actually\n> used in the source code. This change removes it from the usage help\n> that's printed.\n> \n> Signed-off-by: Denton Liu \n> ---\n> git-difftool.perl | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n\n[Dscho, I found a bug, see below]\n\nI forgot to mention that the scripted difftool has been\nrewritten in C and will be going away soon.\nbuiltin/difftool.c is already in \"next\".\n\nNew patches against difftool should target the builtin.\n\nRegarding removing it from the usage string, IMO that can be\nconsidered a good change if the rationale were instead that\nwe never expect users to ever type \"--no-gui\" in practice.\n\nWasting the short usage string screen space with a useless to\n99.99% users option is bad for usability. From that perspective\nwe shouldn't mention it there, so reframing the commit message\ntowards that argument would make for a better motivation.\n\nRemoving the mention from the usage string and adding it to the\nmanpage would be the a good change from that perspective as well.\n\n\nBTW, in \"next\", it seems that the builtin difftool crashes when\ndoing \"git difftool -h\", so we should probably add a test\nfor that too...\n\nFrom the tip of next:\n\n$ git difftool -h\nfatal: BUG: setup_git_env called without repository\n-- \nDavid\n"},{"id":"310886","messageId":"20170205212338.17667-1-davvid@gmail.com","threadId":"45043","inReplyTo":"20170205201751.z4rfmy5xxaqg472l@gmail.com","subject":"[PATCH] difftool: fix bug when printing usage","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2017-02-05T21:23:38Z","receivedAt":"2017-02-05T21:23:47Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"\"git difftool -h\" reports an error:\n\n\tfatal: BUG: setup_git_env called without repository\n\nDefer repository setup so that the help option processing happens before\nthe repository is initialized.\n\nAdd tests to ensure that the basic usage works inside and outside of a\nrepository.\n\nSigned-off-by: David Aguilar \n---\nThis bug exists in both \"master\" and \"next\".\nThis patch has been tested on both branches.\n\n builtin/difftool.c | 8 ++++----\n t/t7800-difftool.sh | 13 +++++++++++++\n 2 files changed, 17 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex b5e85ab079..d13350ce83 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -647,10 +647,6 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\t/* NEEDSWORK: once we no longer spawn anything, remove this */\n-\tsetenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);\n-\tsetenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);\n-\n \tgit_config(difftool_config, NULL);\n \tsymlinks = has_symlinks;\n \n@@ -661,6 +657,10 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \tif (tool_help)\n \t\treturn print_tool_help();\n \n+\t/* NEEDSWORK: once we no longer spawn anything, remove this */\n+\tsetenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);\n+\tsetenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);\n+\n \tif (use_gui_tool && diff_gui_tool && *diff_gui_tool)\n \t\tsetenv(\"GIT_DIFF_TOOL\", diff_gui_tool, 1);\n \telse if (difftool_cmd) {\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex aa0ef02597..21e2ac4ad6 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -23,6 +23,19 @@ prompt_given ()\n \ttest \"$prompt\" = \"Launch 'test-tool' [Y/n]? branch\"\n }\n \n+test_expect_success 'basic usage requires no repo' '\n+\tlines=$(git difftool -h | grep ^usage: | wc -l) &&\n+\ttest \"$lines\" -eq 1 &&\n+\t# create a ceiling directory to prevent Git from finding a repo\n+\tmkdir -p not/repo &&\n+\tceiling=\"$PWD/not\" &&\n+\tlines=$(cd not/repo &&\n+\t\tGIT_CEILING_DIRECTORIES=\"$ceiling\" git difftool -h |\n+\t\tgrep ^usage: | wc -l) &&\n+\ttest \"$lines\" -eq 1 &&\n+\trmdir -p not/repo\n+'\n+\n # Create a file on master and change it on branch\n test_expect_success 'setup' '\n \techo master >file &&\n-- \n2.12.0.rc0.209.g3d1e53e462\n\n"},{"id":"310923","messageId":"alpine.DEB.2.20.1702061716120.3496@virtualbox","threadId":"45043","inReplyTo":"20170205212338.17667-1-davvid@gmail.com","subject":"Re: [PATCH] difftool: fix bug when printing usage","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-06T16:58:49Z","receivedAt":"2017-02-06T16:59:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi David,\n\nOn Sun, 5 Feb 2017, David Aguilar wrote:\n\n> \"git difftool -h\" reports an error:\n> \n> \tfatal: BUG: setup_git_env called without repository\n> \n> Defer repository setup so that the help option processing happens before\n> the repository is initialized.\n> \n> Add tests to ensure that the basic usage works inside and outside of a\n> repository.\n> \n> Signed-off-by: David Aguilar \n> ---\n> This bug exists in both \"master\" and \"next\".\n> This patch has been tested on both branches.\n\nThanks for noticing and for patching!\n\n> diff --git a/builtin/difftool.c b/builtin/difftool.c\n> index b5e85ab079..d13350ce83 100644\n> --- a/builtin/difftool.c\n> +++ b/builtin/difftool.c\n> @@ -647,10 +647,6 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n> \t\tOPT_END()\n> \t};\n> \n> -\t/* NEEDSWORK: once we no longer spawn anything, remove this */\n> -\tsetenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);\n> -\tsetenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);\n> -\n> \tgit_config(difftool_config, NULL);\n> \tsymlinks = has_symlinks;\n> \n> @@ -661,6 +657,10 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n> \tif (tool_help)\n> \t\treturn print_tool_help();\n> \n> +\t/* NEEDSWORK: once we no longer spawn anything, remove this */\n> +\tsetenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);\n> +\tsetenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);\n> +\n> \tif (use_gui_tool && diff_gui_tool && *diff_gui_tool)\n> \t\tsetenv(\"GIT_DIFF_TOOL\", diff_gui_tool, 1);\n> \telse if (difftool_cmd) {\n\nAaargh. You know, when you are used to code review systems where you can\neasily increase the context of the diff hunks, static patches are...\ntedious to review.\n\nFor the record: the context between those two hunks is:\n\n argc = parse_options(argc, argv, prefix, builtin_difftool_options,\n builtin_difftool_usage, PARSE_OPT_KEEP_UNKNOWN |\n PARSE_OPT_KEEP_DASHDASH);\n\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index aa0ef02597..21e2ac4ad6 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -23,6 +23,19 @@ prompt_given ()\n> \ttest \"$prompt\" = \"Launch 'test-tool' [Y/n]? branch\"\n> }\n> \n> +test_expect_success 'basic usage requires no repo' '\n> +\tlines=$(git difftool -h | grep ^usage: | wc -l) &&\n> +\ttest \"$lines\" -eq 1 &&\n\nIt may be easier to debug future breakages if you wrote\n\n\tgit difftool -h | grep ^usage: >output &&\n\ttest_line_count = 1 output &&\n\nor even better (changing the semantics now):\n\n\ttest_expect_code 129 git difftool -h >output &&\n\tgrep ^usage: output &&\n\n> +\t# create a ceiling directory to prevent Git from finding a repo\n> +\tmkdir -p not/repo &&\n> +\tceiling=\"$PWD/not\" &&\n> +\tlines=$(cd not/repo &&\n> +\t\tGIT_CEILING_DIRECTORIES=\"$ceiling\" git difftool -h |\n> +\t\tgrep ^usage: | wc -l) &&\n> +\ttest \"$lines\" -eq 1 &&\n\nLikewise, this would become\n\n\tGIT_CEILING_DIRECTORIES=\"$PWD/not\" \\\n\ttest_expect_code 129 git -C not/repo difftool -h >output &&\n\tgrep ^usage: output\n\n> +\trmdir -p not/repo\n\nI am not sure how portable `rmdir -p` is. Why not use\n\n\ttest_when_finished rm -r not\n\ninstead?\n\nBut those are all really minor bike-sheddings, and I promised myself that\nI would avoid those (as I find them rather pointless) unless there is at\nleast one real benefit. In this case, I think the result becomes more\nreadable:\n\n-- snip --\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 21e2ac4ad6d..7d7cb63d61e 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -24,16 +24,14 @@ prompt_given ()\n }\n \n test_expect_success 'basic usage requires no repo' '\n-\tlines=$(git difftool -h | grep ^usage: | wc -l) &&\n-\ttest \"$lines\" -eq 1 &&\n-\t# create a ceiling directory to prevent Git from finding a repo\n-\tmkdir -p not/repo &&\n-\tceiling=\"$PWD/not\" &&\n-\tlines=$(cd not/repo &&\n-\t\tGIT_CEILING_DIRECTORIES=\"$ceiling\" git difftool -h |\n-\t\tgrep ^usage: | wc -l) &&\n-\ttest \"$lines\" -eq 1 &&\n-\trmdir -p not/repo\n+\ttest_expect_code 129 git difftool -h >output &&\n+\tgrep ^usage: output &&\n+\t# create a ceiling directory to prevent Git from finding a repo\n+\tmkdir -p not/repo &&\n+\ttest_when_finished rm -r not &&\n+\tGIT_CEILING_DIRECTORIES=\"$PWD/not\" \\\n+\ttest_expect_code 129 git -C not/repo difftool -h >output &&\n+\tgrep ^usage: output\n '\n \n # Create a file on master and change it on branch\n-- snap --\n\nMore importantly: When I read $PWD all kinds of alarm bells go off in my\nhead, as I immediately think of all the issues we have on Windows due to\nGit's regression test using POSIX paths all over the place.\n\nIn this particular instance, it *does* work, magically, because the POSIX\npath $PWD is automatically converted (by the MSYS2 runtime) into a Windows\npath when git.exe is called.\n\nRead: I actually tested this, in line with my resolution to avoid to\nreview merely patches (which would likely miss serious bugs) and instead\ntry to do full code reviews, including testing. That means that I will\nreview less, but better.\n\nInsofar as I am the author of the builtin difftool:\n\nAcked-by: Johannes Schindelin \n\nThanks,\nDscho\n"},{"id":"310927","messageId":"xmqqinon2nnt.fsf@gitster.mtv.corp.google.com","threadId":"45043","inReplyTo":"alpine.DEB.2.20.1702061716120.3496@virtualbox","subject":"Re: [PATCH] difftool: fix bug when printing usage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-06T18:13:10Z","receivedAt":"2017-02-06T18:13:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> +test_expect_success 'basic usage requires no repo' '\n>> +\tlines=$(git difftool -h | grep ^usage: | wc -l) &&\n>> +\ttest \"$lines\" -eq 1 &&\n>\n> It may be easier to debug future breakages if you wrote\n>\n> \tgit difftool -h | grep ^usage: >output &&\n> \ttest_line_count = 1 output &&\n> or even better (changing the semantics now):\n>\n> \ttest_expect_code 129 git difftool -h >output &&\n> \tgrep ^usage: output &&\n\nTrue.\n\n>> +\t# create a ceiling directory to prevent Git from finding a repo\n>> +\tmkdir -p not/repo &&\n>> +\tceiling=\"$PWD/not\" &&\n>> +\tlines=$(cd not/repo &&\n>> +\t\tGIT_CEILING_DIRECTORIES=\"$ceiling\" git difftool -h |\n>> +\t\tgrep ^usage: | wc -l) &&\n>> +\ttest \"$lines\" -eq 1 &&\n>\n> Likewise, this would become\n>\n> \tGIT_CEILING_DIRECTORIES=\"$PWD/not\" \\\n> \ttest_expect_code 129 git -C not/repo difftool -h >output &&\n> \tgrep ^usage: output\n\nI agree with the intent, but the execution here is \"Not quite\".\ntest_expect_code being a shell function, it does not take the\n\"one-shot environment assignment for this single invocation,\" like\nexternal commands do.\n\n> More importantly: When I read $PWD all kinds of alarm bells go off in my\n> head, as I immediately think of all the issues we have on Windows due to\n> Git's regression test using POSIX paths all over the place.\n\nAnd we appreciate that somebody who is more familiar with the issue\nis watching ;-).\n\n> Insofar as I am the author of the builtin difftool:\n>\n> Acked-by: Johannes Schindelin \n\nOK, thanks.\n"},{"id":"310977","messageId":"alpine.DEB.2.20.1702071220290.3496@virtualbox","threadId":"45043","inReplyTo":"xmqqinon2nnt.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] difftool: fix bug when printing usage","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-07T11:21:47Z","receivedAt":"2017-02-07T11:21:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 6 Feb 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > [... and he quoted someone ...]\n> >\n> >> +\t# create a ceiling directory to prevent Git from finding a repo\n> >> +\tmkdir -p not/repo &&\n> >> +\tceiling=\"$PWD/not\" &&\n> >> +\tlines=$(cd not/repo &&\n> >> +\t\tGIT_CEILING_DIRECTORIES=\"$ceiling\" git difftool -h |\n> >> +\t\tgrep ^usage: | wc -l) &&\n> >> +\ttest \"$lines\" -eq 1 &&\n> >\n> > Likewise, this would become\n> >\n> > \tGIT_CEILING_DIRECTORIES=\"$PWD/not\" \\\n> > \ttest_expect_code 129 git -C not/repo difftool -h >output &&\n> > \tgrep ^usage: output\n> \n> I agree with the intent, but the execution here is \"Not quite\".\n> test_expect_code being a shell function, it does not take the\n> \"one-shot environment assignment for this single invocation,\" like\n> external commands do.\n\nSo now that we know what is wrong, can you please enlighten me about what\nis right?\n\nThanks,\nJohannes\n"},{"id":"311002","messageId":"xmqqh945zs3c.fsf@gitster.mtv.corp.google.com","threadId":"45043","inReplyTo":"alpine.DEB.2.20.1702071220290.3496@virtualbox","subject":"Re: [PATCH] difftool: fix bug when printing usage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-07T20:03:19Z","receivedAt":"2017-02-07T20:03:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> > Likewise, this would become\n>> >\n>> > \tGIT_CEILING_DIRECTORIES=\"$PWD/not\" \\\n>> > \ttest_expect_code 129 git -C not/repo difftool -h >output &&\n>> > \tgrep ^usage: output\n>> \n>> I agree with the intent, but the execution here is \"Not quite\".\n>> test_expect_code being a shell function, it does not take the\n>> \"one-shot environment assignment for this single invocation,\" like\n>> external commands do.\n>\n> So now that we know what is wrong, can you please enlighten me about what\n> is right?\n\nDavid's original is just fine, isn't it?\n"},{"id":"311003","messageId":"xmqqd1etzrxj.fsf@gitster.mtv.corp.google.com","threadId":"45043","inReplyTo":"xmqqh945zs3c.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] difftool: fix bug when printing usage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-07T20:06:48Z","receivedAt":"2017-02-07T20:06:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Schindelin writes:\n>\n>>> > Likewise, this would become\n>>> >\n>>> > \tGIT_CEILING_DIRECTORIES=\"$PWD/not\" \\\n>>> > \ttest_expect_code 129 git -C not/repo difftool -h >output &&\n>>> > \tgrep ^usage: output\n>>> \n>>> I agree with the intent, but the execution here is \"Not quite\".\n>>> test_expect_code being a shell function, it does not take the\n>>> \"one-shot environment assignment for this single invocation,\" like\n>>> external commands do.\n>>\n>> So now that we know what is wrong, can you please enlighten me about what\n>> is right?\n>\n> David's original is just fine, isn't it?\n\nI've also seen people use \"env VAR=VAL git command\" as the command\nto be tested in t/ scripts. You can run that under test_expect_code,\nmethinks.\n"},{"id":"311063","messageId":"alpine.DEB.2.20.1702081604420.3496@virtualbox","threadId":"45043","inReplyTo":"xmqqd1etzrxj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] difftool: fix bug when printing usage","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-08T15:06:19Z","receivedAt":"2017-02-08T15:38:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 7 Feb 2017, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Johannes Schindelin writes:\n> >\n> >>> > Likewise, this would become\n> >>> >\n> >>> > \tGIT_CEILING_DIRECTORIES=\"$PWD/not\" \\\n> >>> > \ttest_expect_code 129 git -C not/repo difftool -h >output &&\n> >>> > \tgrep ^usage: output\n> >>> \n> >>> I agree with the intent, but the execution here is \"Not quite\".\n> >>> test_expect_code being a shell function, it does not take the\n> >>> \"one-shot environment assignment for this single invocation,\" like\n> >>> external commands do.\n> >>\n> >> So now that we know what is wrong, can you please enlighten me about\n> >> what is right?\n> >\n> > David's original is just fine, isn't it?\n> \n> I've also seen people use \"env VAR=VAL git command\" as the command to be\n> tested in t/ scripts. You can run that under test_expect_code,\n> methinks.\n\nThat is exactly what David ended up sending out as follow-up patches.\n\nI did not mean to be critical, I just found it to be more helpful to\naccompany \"that does not work\" comments with \"but this does\" in the past.\n\nCiao,\nJohannes\n"}]}