{"thread":{"id":"50734","subject":"[PATCH 2/2] difftool: allow running outside Git worktrees with --no-index","startedAt":"2019-03-13T19:20:22Z","lastAt":"2019-03-19T00:25:51Z","messageCount":17,"participants":["Johannes Schindelin via GitGitGadget","Jeff King","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"371390","messageId":"9f6eb60eeeccf920af83652899c5bc2d40e2092f.1552504812.git.gitgitgadget@gmail.com","threadId":"50734","inReplyTo":"pull.163.git.gitgitgadget@gmail.com","subject":"[PATCH 2/2] difftool: allow running outside Git worktrees with --no-index","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-13T19:20:14Z","receivedAt":"2019-03-13T19:20:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nAs far as this developer can tell, the conversion from a Perl script to\na built-in caused the regression in the difftool that it no longer runs\noutside of a Git worktree (with `--no-index`, of course).\n\nIt is a bit embarrassing that it took over two years after retiring the\nPerl version to discover this regression, but at least we now know, and\ncan do something, about it.\n\nThis fixes https://github.com/git-for-windows/git/issues/2123\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/difftool.c  | 20 +++++++++++++++++---\n git.c               |  2 +-\n t/t7800-difftool.sh | 10 ++++++++++\n 3 files changed, 28 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 31eece0c8d..d08ef31f04 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -690,7 +690,7 @@ static int run_file_diff(int prompt, const char *prefix,\n int cmd_difftool(int argc, const char **argv, const char *prefix)\n {\n \tint use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,\n-\t    tool_help = 0;\n+\t    tool_help = 0, i, no_index = 0;\n \tstatic char *difftool_cmd = NULL, *extcmd = NULL;\n \tstruct option builtin_difftool_options[] = {\n \t\tOPT_BOOL('g', \"gui\", &use_gui_tool,\n@@ -727,8 +727,22 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \tif (tool_help)\n \t\treturn print_tool_help();\n \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+\tfor (i = 0; i < argc; i++)\n+\t\tif (!strcmp(argv[i], \"--\"))\n+\t\t\tbreak;\n+\t\telse if (!strcmp(argv[i], \"--no-index\")) {\n+\t\t\tno_index = 1;\n+\t\t\tbreak;\n+\t\t}\n+\n+\tif (!no_index && !startup_info->have_repository)\n+\t\tdie(_(\"difftool requires worktree or --no-index\"));\n+\n+\tif (!no_index){\n+\t\tsetup_work_tree();\n+\t\tsetenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);\n+\t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);\n+\t}\n \n \tif (use_gui_tool && diff_gui_tool && *diff_gui_tool)\n \t\tsetenv(\"GIT_DIFF_TOOL\", diff_gui_tool, 1);\ndiff --git a/git.c b/git.c\nindex 2014aab6b8..46365ed86a 100644\n--- a/git.c\n+++ b/git.c\n@@ -491,7 +491,7 @@ static struct cmd_struct commands[] = {\n \t{ \"diff-files\", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },\n \t{ \"diff-index\", cmd_diff_index, RUN_SETUP | NO_PARSEOPT },\n \t{ \"diff-tree\", cmd_diff_tree, RUN_SETUP | NO_PARSEOPT },\n-\t{ \"difftool\", cmd_difftool, RUN_SETUP | NEED_WORK_TREE },\n+\t{ \"difftool\", cmd_difftool, RUN_SETUP_GENTLY },\n \t{ \"fast-export\", cmd_fast_export, RUN_SETUP },\n \t{ \"fetch\", cmd_fetch, RUN_SETUP },\n \t{ \"fetch-pack\", cmd_fetch_pack, RUN_SETUP | NO_PARSEOPT },\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex bb9a7f4ff9..4907627656 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -705,4 +705,14 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'outside worktree' '\n+\tmkdir outside &&\n+\techo 1 >outside/1 &&\n+\techo 2 >outside/2 &&\n+\ttest_expect_code 1 env GIT_CEILING_DIRECTORIES=\"$(pwd)\" git \\\n+\t\t-c diff.tool=echo -c difftool.echo.cmd=\"echo \\$LOCAL \\$REMOTE\" \\\n+\t\t-C outside difftool --no-prompt --no-index 1 2 >out &&\n+\ttest \"1 2\" = \"$(cat out)\"\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"371391","messageId":"pull.163.git.gitgitgadget@gmail.com","threadId":"50734","inReplyTo":null,"subject":"[PATCH 0/2] Allow difftool to be run outside of Git worktrees","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-13T19:20:13Z","receivedAt":"2019-03-13T19:20:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It was reported in https://github.com/git-for-windows/git/issues/2123 that \ngit difftool --no-index fails to work outside worktrees, even if it should\nwork.\n\nI fear this is a regression I introduced over two years ago (!) when I\nconverted the Perl script to C.\n\nAt least now that I know about the bug, I can fix it.\n\nJohannes Schindelin (2):\n  difftool: remove obsolete (and misleading) comment\n  difftool: allow running outside Git worktrees with --no-index\n\n builtin/difftool.c  | 21 +++++++++++++++++----\n git.c               |  2 +-\n t/t7800-difftool.sh | 10 ++++++++++\n 3 files changed, 28 insertions(+), 5 deletions(-)\n\n\nbase-commit: e902e9bcae2010bc42648c80ab6adc6c5a16a4a5\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-163%2Fdscho%2Fdifftool-no-index-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-163/dscho/difftool-no-index-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/163\n-- \ngitgitgadget\n"},{"id":"371392","messageId":"2ad91f19c5b8c7e043e5944a1c075ad23612b59d.1552504812.git.gitgitgadget@gmail.com","threadId":"50734","inReplyTo":"pull.163.git.gitgitgadget@gmail.com","subject":"[PATCH 1/2] difftool: remove obsolete (and misleading) comment","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-13T19:20:14Z","receivedAt":"2019-03-13T19:20:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nWe will always spawn something from `git difftool`, so we will always\nhave to set `GIT_DIR` and `GIT_WORK_TREE`.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/difftool.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex a3ea60ea71..31eece0c8d 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -727,7 +727,6 @@ 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-- \ngitgitgadget\n\n"},{"id":"371398","messageId":"20190313204644.GA5397@sigill.intra.peff.net","threadId":"50734","inReplyTo":"9f6eb60eeeccf920af83652899c5bc2d40e2092f.1552504812.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] difftool: allow running outside Git worktrees with --no-index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-13T20:46:44Z","receivedAt":"2019-03-13T20:48:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 13, 2019 at 12:20:14PM -0700, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> \n> As far as this developer can tell, the conversion from a Perl script to\n> a built-in caused the regression in the difftool that it no longer runs\n> outside of a Git worktree (with `--no-index`, of course).\n> \n> It is a bit embarrassing that it took over two years after retiring the\n> Perl version to discover this regression, but at least we now know, and\n> can do something, about it.\n\nIf a bug falls in the forest, does it make a sound?\n\nI get the impression that `--no-index` is not used all that much, given\nhow long bugs seem to hang around in it (and doubly so when intersected\nwith the difftool).\n\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> +\tfor (i = 0; i < argc; i++)\n> +\t\tif (!strcmp(argv[i], \"--\"))\n> +\t\t\tbreak;\n> +\t\telse if (!strcmp(argv[i], \"--no-index\")) {\n> +\t\t\tno_index = 1;\n> +\t\t\tbreak;\n> +\t\t}\n\nInstead of this ad-hoc parsing, can we just add an OPT_BOOL(\"no-index\")\nto the parse-options array? We'll have already run parse_options() at\nthis point.\n\nWe'd just have to remember to add it back to the argv of diff\nsub-commands we run.\n\n> +\tif (!no_index && !startup_info->have_repository)\n> +\t\tdie(_(\"difftool requires worktree or --no-index\"));\n> +\n> +\tif (!no_index){\n> +\t\tsetup_work_tree();\n> +\t\tsetenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);\n> +\t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);\n> +\t}\n\nThis part makes sense.\n\nThere may be other subtle dependencies on having a repo from\nsub-functions we run, but I didn't see any from a quick scan. And\nanyway, if there is such a code path, it is no worse off than before\nyour patch (and in fact much better, because it would hopefully yield a\nBUG() that would tell us what we need to fix).\n\n> index 2014aab6b8..46365ed86a 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -491,7 +491,7 @@ static struct cmd_struct commands[] = {\n>  \t{ \"diff-files\", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },\n>  \t{ \"diff-index\", cmd_diff_index, RUN_SETUP | NO_PARSEOPT },\n>  \t{ \"diff-tree\", cmd_diff_tree, RUN_SETUP | NO_PARSEOPT },\n> -\t{ \"difftool\", cmd_difftool, RUN_SETUP | NEED_WORK_TREE },\n> +\t{ \"difftool\", cmd_difftool, RUN_SETUP_GENTLY },\n\nWe necessarily lost NEED_WORK_TREE, but that's OK because you added in a\nsetup_work_tree() in the function. Good.\n\n> +test_expect_success 'outside worktree' '\n> +\tmkdir outside &&\n> +\techo 1 >outside/1 &&\n> +\techo 2 >outside/2 &&\n> +\ttest_expect_code 1 env GIT_CEILING_DIRECTORIES=\"$(pwd)\" git \\\n> +\t\t-c diff.tool=echo -c difftool.echo.cmd=\"echo \\$LOCAL \\$REMOTE\" \\\n> +\t\t-C outside difftool --no-prompt --no-index 1 2 >out &&\n\nWe have a helper for running outside a repo. Because you have to set up\nthe \"outside\" space, it unfortunately doesn't shorten things as much as\nit does in some other spots:\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 4907627656..255a787614 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -706,12 +706,12 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n '\n \n test_expect_success 'outside worktree' '\n-\tmkdir outside &&\n-\techo 1 >outside/1 &&\n-\techo 2 >outside/2 &&\n-\ttest_expect_code 1 env GIT_CEILING_DIRECTORIES=\"$(pwd)\" git \\\n+\tmkdir non-repo &&\n+\techo 1 >non-repo/1 &&\n+\techo 2 >non-repo/2 &&\n+\ttest_expect_code 1 nongit git \\\n \t\t-c diff.tool=echo -c difftool.echo.cmd=\"echo \\$LOCAL \\$REMOTE\" \\\n-\t\t-C outside difftool --no-prompt --no-index 1 2 >out &&\n+\t\tdifftool --no-prompt --no-index 1 2 >out &&\n \ttest \"1 2\" = \"$(cat out)\"\n '\n \n\nbut it might be worth using anyway, just for consistency.\n\n> +\ttest \"1 2\" = \"$(cat out)\"\n\nA minor nit, but I think:\n\n  echo \"1 2\" >expect &&\n  test_cmp expect actual\n\nproduces nicer output on failure, and costs the same number of\nprocesses (it is an extra file write, but I think the main driver of\nperformance in the test suite is just processes).\n\n-Peff\n"},{"id":"371477","messageId":"pull.163.v2.git.gitgitgadget@gmail.com","threadId":"50734","inReplyTo":"pull.163.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] Allow difftool to be run outside of Git worktrees","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-14T11:25:02Z","receivedAt":"2019-03-14T11:25:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It was reported in https://github.com/git-for-windows/git/issues/2123 that \ngit difftool --no-index fails to work outside worktrees, even if it should\nwork.\n\nI fear this is a regression I introduced over two years ago (!) when I\nconverted the Perl script to C.\n\nAt least now that I know about the bug, I can fix it.\n\nChanges since v1:\n\n * Instead of ad-hoc parsing to look for --no-index, the OPT_ARGUMENT() was\n   enhanced and not has its first real user! After all those lonely, long\n   years (11 years, 1 month and 11 days), what a wonderful thing to\n   celebrate on π day.\n * The test now uses the nongit helper.\n * The test uses test_cmp to make diagnosing regressions easier.\n\nJohannes Schindelin (3):\n  difftool: remove obsolete (and misleading) comment\n  parse-options: make OPT_ARGUMENT()  more useful\n  difftool: allow running outside Git worktrees with --no-index\n\n Documentation/technical/api-parse-options.txt |  4 +++-\n builtin/difftool.c                            | 14 ++++++++++----\n git.c                                         |  2 +-\n parse-options.c                               |  2 ++\n parse-options.h                               |  4 ++--\n t/helper/test-parse-options.c                 |  2 +-\n t/t7800-difftool.sh                           | 10 ++++++++++\n 7 files changed, 29 insertions(+), 9 deletions(-)\n\n\nbase-commit: e902e9bcae2010bc42648c80ab6adc6c5a16a4a5\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-163%2Fdscho%2Fdifftool-no-index-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-163/dscho/difftool-no-index-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/163\n\nRange-diff vs v1:\n\n 1:  2ad91f19c5 = 1:  2ad91f19c5 difftool: remove obsolete (and misleading) comment\n -:  ---------- > 2:  10775638ad parse-options: make OPT_ARGUMENT()  more useful\n 2:  9f6eb60eee ! 3:  8cca9f800c difftool: allow running outside Git worktrees with --no-index\n     @@ -22,24 +22,24 @@\n       {\n       \tint use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,\n      -\t    tool_help = 0;\n     -+\t    tool_help = 0, i, no_index = 0;\n     ++\t    tool_help = 0, no_index = 0;\n       \tstatic char *difftool_cmd = NULL, *extcmd = NULL;\n       \tstruct option builtin_difftool_options[] = {\n       \t\tOPT_BOOL('g', \"gui\", &use_gui_tool,\n     +@@\n     + \t\t\t    \"tool returns a non - zero exit code\")),\n     + \t\tOPT_STRING('x', \"extcmd\", &extcmd, N_(\"command\"),\n     + \t\t\t   N_(\"specify a custom command for viewing diffs\")),\n     ++\t\tOPT_ARGUMENT(\"no-index\", &no_index, N_(\"passed to `diff`\")),\n     + \t\tOPT_END()\n     + \t};\n     + \n      @@\n       \tif (tool_help)\n       \t\treturn print_tool_help();\n       \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     -+\tfor (i = 0; i < argc; i++)\n     -+\t\tif (!strcmp(argv[i], \"--\"))\n     -+\t\t\tbreak;\n     -+\t\telse if (!strcmp(argv[i], \"--no-index\")) {\n     -+\t\t\tno_index = 1;\n     -+\t\t\tbreak;\n     -+\t\t}\n     -+\n      +\tif (!no_index && !startup_info->have_repository)\n      +\t\tdie(_(\"difftool requires worktree or --no-index\"));\n      +\n     @@ -73,13 +73,13 @@\n       '\n       \n      +test_expect_success 'outside worktree' '\n     -+\tmkdir outside &&\n     -+\techo 1 >outside/1 &&\n     -+\techo 2 >outside/2 &&\n     -+\ttest_expect_code 1 env GIT_CEILING_DIRECTORIES=\"$(pwd)\" git \\\n     ++\techo 1 >1 &&\n     ++\techo 2 >2 &&\n     ++\ttest_expect_code 1 nongit git \\\n      +\t\t-c diff.tool=echo -c difftool.echo.cmd=\"echo \\$LOCAL \\$REMOTE\" \\\n     -+\t\t-C outside difftool --no-prompt --no-index 1 2 >out &&\n     -+\ttest \"1 2\" = \"$(cat out)\"\n     ++\t\tdifftool --no-prompt --no-index ../1 ../2 >actual &&\n     ++\techo \"../1 ../2\" >expect &&\n     ++\ttest_cmp expect actual\n      +'\n      +\n       test_done\n\n-- \ngitgitgadget\n"},{"id":"371478","messageId":"2ad91f19c5b8c7e043e5944a1c075ad23612b59d.1552562701.git.gitgitgadget@gmail.com","threadId":"50734","inReplyTo":"pull.163.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] difftool: remove obsolete (and misleading) comment","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-14T11:25:03Z","receivedAt":"2019-03-14T11:25:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nWe will always spawn something from `git difftool`, so we will always\nhave to set `GIT_DIR` and `GIT_WORK_TREE`.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/difftool.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex a3ea60ea71..31eece0c8d 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -727,7 +727,6 @@ 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-- \ngitgitgadget\n\n"},{"id":"371479","messageId":"10775638ad8f2ef9b64b8dbaf71b80d8546e81d8.1552562701.git.gitgitgadget@gmail.com","threadId":"50734","inReplyTo":"pull.163.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] parse-options: make OPT_ARGUMENT() more useful","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-14T11:25:04Z","receivedAt":"2019-03-14T11:25:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\n`OPT_ARGUMENT()` is intended to keep the specified long option in `argv`\nand not to do anything else.\n\nHowever, it would make a lot of sense for the caller to know whether\nthis option was seen at all or not. For example, we want to teach `git\ndifftool` to work outside of any Git worktree, but only when\n`--no-index` was specified.\n\nNote: nothing in Git uses OPT_ARGUMENT(). Even worse, looking through\nthe commit history, one can easily see that nothing even\never used it, apart from the regression test.\n\nSo not only do we make `OPT_ARGUMENT()` more useful, we are also about\nto introduce its first real user!\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n Documentation/technical/api-parse-options.txt | 4 +++-\n parse-options.c                               | 2 ++\n parse-options.h                               | 4 ++--\n t/helper/test-parse-options.c                 | 2 +-\n 4 files changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\nindex 2b036d7838..2e2e7c10c6 100644\n--- a/Documentation/technical/api-parse-options.txt\n+++ b/Documentation/technical/api-parse-options.txt\n@@ -198,8 +198,10 @@ There are some macros to easily define options:\n \tThe filename will be prefixed by passing the filename along with\n \tthe prefix argument of `parse_options()` to `prefix_filename()`.\n \n-`OPT_ARGUMENT(long, description)`::\n+`OPT_ARGUMENT(long, &int_var, description)`::\n \tIntroduce a long-option argument that will be kept in `argv[]`.\n+\tIf this option was seen, `int_var` will be set to one (except\n+\tif a `NULL` pointer was passed).\n \n `OPT_NUMBER_CALLBACK(&var, description, func_ptr)`::\n \tRecognize numerical options like -123 and feed the integer as\ndiff --git a/parse-options.c b/parse-options.c\nindex cec74522e5..1d57802da0 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -286,6 +286,8 @@ static enum parse_opt_result parse_long_opt(\n \t\t\t\t\t     optname(options, flags));\n \t\t\tif (*rest)\n \t\t\t\tcontinue;\n+\t\t\tif (options->value)\n+\t\t\t\t*(int *)options->value = options->defval;\n \t\t\tp->out[p->cpidx++] = arg - 2;\n \t\t\treturn PARSE_OPT_DONE;\n \t\t}\ndiff --git a/parse-options.h b/parse-options.h\nindex 7d83e2971d..c3d45ba1ac 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -138,8 +138,8 @@ struct option {\n \t{ OPTION_CALLBACK, (s), (l), (v), (a), (h), (f), (cb) }\n \n #define OPT_END()                   { OPTION_END }\n-#define OPT_ARGUMENT(l, h)          { OPTION_ARGUMENT, 0, (l), NULL, NULL, \\\n-\t\t\t\t      (h), PARSE_OPT_NOARG}\n+#define OPT_ARGUMENT(l, v, h)       { OPTION_ARGUMENT, 0, (l), (v), NULL, \\\n+\t\t\t\t      (h), PARSE_OPT_NOARG, NULL, 1 }\n #define OPT_GROUP(h)                { OPTION_GROUP, 0, NULL, NULL, NULL, (h) }\n #define OPT_BIT(s, l, v, h, b)      OPT_BIT_F(s, l, v, h, b, 0)\n #define OPT_BITOP(s, l, v, h, set, clear) { OPTION_BITOP, (s), (l), (v), NULL, (h), \\\ndiff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c\nindex cc88fba057..2232b2f79e 100644\n--- a/t/helper/test-parse-options.c\n+++ b/t/helper/test-parse-options.c\n@@ -132,7 +132,7 @@ int cmd__parse_options(int argc, const char **argv)\n \t\tOPT_NOOP_NOARG(0, \"obsolete\"),\n \t\tOPT_STRING_LIST(0, \"list\", &list, \"str\", \"add str to list\"),\n \t\tOPT_GROUP(\"Magic arguments\"),\n-\t\tOPT_ARGUMENT(\"quux\", \"means --quux\"),\n+\t\tOPT_ARGUMENT(\"quux\", NULL, \"means --quux\"),\n \t\tOPT_NUMBER_CALLBACK(&integer, \"set integer to NUM\",\n \t\t\tnumber_callback),\n \t\t{ OPTION_COUNTUP, '+', NULL, &boolean, NULL, \"same as -b\",\n-- \ngitgitgadget\n\n"},{"id":"371480","messageId":"8cca9f800c2ce269a2ae644e4c15dc4115d3b0e2.1552562701.git.gitgitgadget@gmail.com","threadId":"50734","inReplyTo":"pull.163.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] difftool: allow running outside Git worktrees with --no-index","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-14T11:25:04Z","receivedAt":"2019-03-14T11:25:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nAs far as this developer can tell, the conversion from a Perl script to\na built-in caused the regression in the difftool that it no longer runs\noutside of a Git worktree (with `--no-index`, of course).\n\nIt is a bit embarrassing that it took over two years after retiring the\nPerl version to discover this regression, but at least we now know, and\ncan do something, about it.\n\nThis fixes https://github.com/git-for-windows/git/issues/2123\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/difftool.c  | 13 ++++++++++---\n git.c               |  2 +-\n t/t7800-difftool.sh | 10 ++++++++++\n 3 files changed, 21 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 31eece0c8d..4fff1e83f9 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -690,7 +690,7 @@ static int run_file_diff(int prompt, const char *prefix,\n int cmd_difftool(int argc, const char **argv, const char *prefix)\n {\n \tint use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,\n-\t    tool_help = 0;\n+\t    tool_help = 0, no_index = 0;\n \tstatic char *difftool_cmd = NULL, *extcmd = NULL;\n \tstruct option builtin_difftool_options[] = {\n \t\tOPT_BOOL('g', \"gui\", &use_gui_tool,\n@@ -714,6 +714,7 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \t\t\t    \"tool returns a non - zero exit code\")),\n \t\tOPT_STRING('x', \"extcmd\", &extcmd, N_(\"command\"),\n \t\t\t   N_(\"specify a custom command for viewing diffs\")),\n+\t\tOPT_ARGUMENT(\"no-index\", &no_index, N_(\"passed to `diff`\")),\n \t\tOPT_END()\n \t};\n \n@@ -727,8 +728,14 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \tif (tool_help)\n \t\treturn print_tool_help();\n \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+\tif (!no_index && !startup_info->have_repository)\n+\t\tdie(_(\"difftool requires worktree or --no-index\"));\n+\n+\tif (!no_index){\n+\t\tsetup_work_tree();\n+\t\tsetenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);\n+\t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);\n+\t}\n \n \tif (use_gui_tool && diff_gui_tool && *diff_gui_tool)\n \t\tsetenv(\"GIT_DIFF_TOOL\", diff_gui_tool, 1);\ndiff --git a/git.c b/git.c\nindex 2014aab6b8..46365ed86a 100644\n--- a/git.c\n+++ b/git.c\n@@ -491,7 +491,7 @@ static struct cmd_struct commands[] = {\n \t{ \"diff-files\", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },\n \t{ \"diff-index\", cmd_diff_index, RUN_SETUP | NO_PARSEOPT },\n \t{ \"diff-tree\", cmd_diff_tree, RUN_SETUP | NO_PARSEOPT },\n-\t{ \"difftool\", cmd_difftool, RUN_SETUP | NEED_WORK_TREE },\n+\t{ \"difftool\", cmd_difftool, RUN_SETUP_GENTLY },\n \t{ \"fast-export\", cmd_fast_export, RUN_SETUP },\n \t{ \"fetch\", cmd_fetch, RUN_SETUP },\n \t{ \"fetch-pack\", cmd_fetch_pack, RUN_SETUP | NO_PARSEOPT },\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex bb9a7f4ff9..480dd0633f 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -705,4 +705,14 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'outside worktree' '\n+\techo 1 >1 &&\n+\techo 2 >2 &&\n+\ttest_expect_code 1 nongit git \\\n+\t\t-c diff.tool=echo -c difftool.echo.cmd=\"echo \\$LOCAL \\$REMOTE\" \\\n+\t\tdifftool --no-prompt --no-index ../1 ../2 >actual &&\n+\techo \"../1 ../2\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"371483","messageId":"nycvar.QRO.7.76.6.1903141305550.41@tvgsbejvaqbjf.bet","threadId":"50734","inReplyTo":"20190313204644.GA5397@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] difftool: allow running outside Git worktrees with --no-index","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-14T12:16:45Z","receivedAt":"2019-03-14T12:17:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 13 Mar 2019, Jeff King wrote:\n\n> On Wed, Mar 13, 2019 at 12:20:14PM -0700, Johannes Schindelin via\n> GitGitGadget wrote:\n> \n> > From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> > \n> > As far as this developer can tell, the conversion from a Perl script to\n> > a built-in caused the regression in the difftool that it no longer runs\n> > outside of a Git worktree (with `--no-index`, of course).\n> > \n> > It is a bit embarrassing that it took over two years after retiring the\n> > Perl version to discover this regression, but at least we now know, and\n> > can do something, about it.\n> \n> If a bug falls in the forest, does it make a sound?\n\nI am glad you asked! Last time I checked, yes, it made a sound. It was a\nreally curious rattling sound. But I did not want to bother the bug again,\nso I left it alone.\n\n> I get the impression that `--no-index` is not used all that much, given\n> how long bugs seem to hang around in it (and doubly so when intersected\n> with the difftool).\n\nOr maybe `--no-index` is used in pretty canonical ways. I, for one, used\nto be a really heavy user of `--no-index` before `range-diff`, and it was\nalmost always with two files, sometimes with `--color-words`, sometimes\nwith `--patience`, sometimes both, but never anything crazy like using\nBash's `<(<command>)` syntax.\n\nIn other words, my take is that the ways in which `--no-index` is used are\nprobably not very different from one another, and the bugs lurk in\nreally rarely exercised code paths.\n\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> > +\tfor (i = 0; i < argc; i++)\n> > +\t\tif (!strcmp(argv[i], \"--\"))\n> > +\t\t\tbreak;\n> > +\t\telse if (!strcmp(argv[i], \"--no-index\")) {\n> > +\t\t\tno_index = 1;\n> > +\t\t\tbreak;\n> > +\t\t}\n> \n> Instead of this ad-hoc parsing, can we just add an OPT_BOOL(\"no-index\")\n> to the parse-options array? We'll have already run parse_options() at\n> this point.\n> \n> We'd just have to remember to add it back to the argv of diff\n> sub-commands we run.\n\nIt was that \"add it back\" that I was not keen to implement.\n\nBut you gave me an idea: why not teach parse_options() to optionally keep\nindividual parsed arguments in `argv`?\n\nAnd I was half-way finished with implementing that idea when I discovered\n`OPT_ARGUMENT()`. This seemed to be *almost* what I needed: it puts the\nargument immediately back into `argv`. However, it did not record that\nfact, so I would not know whether it was part of the command-line or not.\n\nSo I was already done with implementing `OPT_ARGUMENT_SEEN()`, based on\n`OPT_ARGUMENT()`, and testing it with my difftool patch, when it occurred\nto me to look what existing users of `OPT_ARGUMENT()` do. Guess what:\nthere are none, apart from that test helper used in t0040 to verify that\n`parse_options()` works as intended. And there were none other. In the\nentire commit history.\n\nIn the end, I changed `OPT_ARGUMENT()`, and I find the end result rather\npleasing.\n\n> > +\tif (!no_index && !startup_info->have_repository)\n> > +\t\tdie(_(\"difftool requires worktree or --no-index\"));\n> > +\n> > +\tif (!no_index){\n> > +\t\tsetup_work_tree();\n> > +\t\tsetenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);\n> > +\t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);\n> > +\t}\n> \n> This part makes sense.\n> \n> There may be other subtle dependencies on having a repo from\n> sub-functions we run, but I didn't see any from a quick scan. And\n> anyway, if there is such a code path, it is no worse off than before\n> your patch (and in fact much better, because it would hopefully yield a\n> BUG() that would tell us what we need to fix).\n\nIndeed.\n\n> > +test_expect_success 'outside worktree' '\n> > +\tmkdir outside &&\n> > +\techo 1 >outside/1 &&\n> > +\techo 2 >outside/2 &&\n> > +\ttest_expect_code 1 env GIT_CEILING_DIRECTORIES=\"$(pwd)\" git \\\n> > +\t\t-c diff.tool=echo -c difftool.echo.cmd=\"echo \\$LOCAL \\$REMOTE\" \\\n> > +\t\t-C outside difftool --no-prompt --no-index 1 2 >out &&\n> \n> We have a helper for running outside a repo. Because you have to set up\n> the \"outside\" space, it unfortunately doesn't shorten things as much as\n> it does in some other spots:\n> \n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index 4907627656..255a787614 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -706,12 +706,12 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n>  '\n>  \n>  test_expect_success 'outside worktree' '\n> -\tmkdir outside &&\n> -\techo 1 >outside/1 &&\n> -\techo 2 >outside/2 &&\n> -\ttest_expect_code 1 env GIT_CEILING_DIRECTORIES=\"$(pwd)\" git \\\n> +\tmkdir non-repo &&\n> +\techo 1 >non-repo/1 &&\n> +\techo 2 >non-repo/2 &&\n> +\ttest_expect_code 1 nongit git \\\n>  \t\t-c diff.tool=echo -c difftool.echo.cmd=\"echo \\$LOCAL \\$REMOTE\" \\\n> -\t\t-C outside difftool --no-prompt --no-index 1 2 >out &&\n> +\t\tdifftool --no-prompt --no-index 1 2 >out &&\n>  \ttest \"1 2\" = \"$(cat out)\"\n>  '\n>  \n> \n> but it might be worth using anyway, just for consistency.\n\nI totally agree. Thanks for pointing me to `nongit`; I was unaware of it.\n\nAnd I was able to shorten it a bit, because the files `1` and `2` do not\nneed to live in that `non-repo` directory.\n\nAgain, a rather pleasing change.\n\n> > +\ttest \"1 2\" = \"$(cat out)\"\n> \n> A minor nit, but I think:\n> \n>   echo \"1 2\" >expect &&\n>   test_cmp expect actual\n> \n> produces nicer output on failure, and costs the same number of\n> processes (it is an extra file write, but I think the main driver of\n> performance in the test suite is just processes).\n\nYou are totally right! After all, a regression test does not need to make\nanything easy when it passes. It needs to make it easy to act when it\nfails.\n\nThank you so much for your helpful suggestions!\nDscho\n"},{"id":"371556","messageId":"20190315030811.GA28943@sigill.intra.peff.net","threadId":"50734","inReplyTo":"nycvar.QRO.7.76.6.1903141305550.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] difftool: allow running outside Git worktrees with --no-index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-15T03:08:12Z","receivedAt":"2019-03-15T03:09:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 14, 2019 at 01:16:45PM +0100, Johannes Schindelin wrote:\n\n> In other words, my take is that the ways in which `--no-index` is used are\n> probably not very different from one another, and the bugs lurk in\n> really rarely exercised code paths.\n\nYeah, that's more likely (and consistent with the bugs I remember fixing\nin the last few years).\n\n> > We'd just have to remember to add it back to the argv of diff\n> > sub-commands we run.\n> \n> It was that \"add it back\" that I was not keen to implement.\n\nYes, I saw that we pass on the argv we get back from parse_options()\nliterally to the sub-functions. I was thinking you'd do some trickery\nwith an argv_array. But I like your approach of using OPT_ARGUMENT()\nmuch better.\n\n> So I was already done with implementing `OPT_ARGUMENT_SEEN()`, based on\n> `OPT_ARGUMENT()`, and testing it with my difftool patch, when it occurred\n> to me to look what existing users of `OPT_ARGUMENT()` do. Guess what:\n> there are none, apart from that test helper used in t0040 to verify that\n> `parse_options()` works as intended. And there were none other. In the\n> entire commit history.\n\nHeh. That is not the first time I have hit that with the parse-options\ncode.\n\nI see you posted the new patches, so I'll try to give a careful read in\nthat part of the thread.\n\n-Peff\n"},{"id":"371557","messageId":"20190315031553.GB28943@sigill.intra.peff.net","threadId":"50734","inReplyTo":"10775638ad8f2ef9b64b8dbaf71b80d8546e81d8.1552562701.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] parse-options: make OPT_ARGUMENT() more useful","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-15T03:15:53Z","receivedAt":"2019-03-15T03:17:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 14, 2019 at 04:25:04AM -0700, Johannes Schindelin via GitGitGadget wrote:\n\n> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\n> index 2b036d7838..2e2e7c10c6 100644\n> --- a/Documentation/technical/api-parse-options.txt\n> +++ b/Documentation/technical/api-parse-options.txt\n> @@ -198,8 +198,10 @@ There are some macros to easily define options:\n>  \tThe filename will be prefixed by passing the filename along with\n>  \tthe prefix argument of `parse_options()` to `prefix_filename()`.\n>  \n> -`OPT_ARGUMENT(long, description)`::\n> +`OPT_ARGUMENT(long, &int_var, description)`::\n>  \tIntroduce a long-option argument that will be kept in `argv[]`.\n> +\tIf this option was seen, `int_var` will be set to one (except\n> +\tif a `NULL` pointer was passed).\n\nSo this effectively makes it into a \"bool\" that we keep. I think that's\nfine. It always uses NOARG, so it is not like we would ever need to see\n\"we got --foo, and this is the argument it had\".\n\nI did wonder if it was possible for \"--no-foo\" to trigger this (leaving\nthe caller who looks at the int unsure if they saw \"--foo\" or\n\"--no-foo\"), but it seems that the parse-options code checks for\nOPTION_ARGUMENT before it ever looks at negation.\n\nCuriously, it also checks it before doing the usual prefix-matching\nmagic. So you could otherwise say \"--no-inde\", but OPT_ARGUMENT() will\nnot allow it. I think that's probably sane and not worth thinking\nfurther about, but it is an interesting quirk that a user could possibly\nrun into.\n\n> diff --git a/parse-options.c b/parse-options.c\n> index cec74522e5..1d57802da0 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -286,6 +286,8 @@ static enum parse_opt_result parse_long_opt(\n>  \t\t\t\t\t     optname(options, flags));\n>  \t\t\tif (*rest)\n>  \t\t\t\tcontinue;\n> +\t\t\tif (options->value)\n> +\t\t\t\t*(int *)options->value = options->defval;\n\nCute. You could actually assign any defval you like, though of course\nthe convenient OPT_ARGUMENT() macro just always uses 1.\n\nI wondered if you might need another cast for defval itself, but it's an\nintptr_t (so it's the types that use it as a string that need to cast to\n\"const char *\").\n\nThis looks very clean overall, and I agree it's much nicer than the\nalternatives for your use case.\n\n-Peff\n"},{"id":"371558","messageId":"20190315031718.GC28943@sigill.intra.peff.net","threadId":"50734","inReplyTo":"8cca9f800c2ce269a2ae644e4c15dc4115d3b0e2.1552562701.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/3] difftool: allow running outside Git worktrees with --no-index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-15T03:17:19Z","receivedAt":"2019-03-15T03:18:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 14, 2019 at 04:25:04AM -0700, Johannes Schindelin via GitGitGadget wrote:\n\n> @@ -714,6 +714,7 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n>  \t\t\t    \"tool returns a non - zero exit code\")),\n>  \t\tOPT_STRING('x', \"extcmd\", &extcmd, N_(\"command\"),\n>  \t\t\t   N_(\"specify a custom command for viewing diffs\")),\n> +\t\tOPT_ARGUMENT(\"no-index\", &no_index, N_(\"passed to `diff`\")),\n>  \t\tOPT_END()\n>  \t};\n\nMuch nicer.\n\n> +test_expect_success 'outside worktree' '\n> +\techo 1 >1 &&\n> +\techo 2 >2 &&\n> +\ttest_expect_code 1 nongit git \\\n> +\t\t-c diff.tool=echo -c difftool.echo.cmd=\"echo \\$LOCAL \\$REMOTE\" \\\n> +\t\tdifftool --no-prompt --no-index ../1 ../2 >actual &&\n> +\techo \"../1 ../2\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n\nAnd this fixed all of my nits from the previous version. The whole\nseries looks good to me.\n\n-Peff\n"},{"id":"371586","messageId":"nycvar.QRO.7.76.6.1903151413570.41@tvgsbejvaqbjf.bet","threadId":"50734","inReplyTo":"20190315031718.GC28943@sigill.intra.peff.net","subject":"Re: [PATCH v2 3/3] difftool: allow running outside Git worktrees with --no-index","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-15T13:20:10Z","receivedAt":"2019-03-15T13:20:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 14 Mar 2019, Jeff King wrote:\n\n> On Thu, Mar 14, 2019 at 04:25:04AM -0700, Johannes Schindelin via GitGitGadget wrote:\n> \n> > @@ -714,6 +714,7 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n> >  \t\t\t    \"tool returns a non - zero exit code\")),\n> >  \t\tOPT_STRING('x', \"extcmd\", &extcmd, N_(\"command\"),\n> >  \t\t\t   N_(\"specify a custom command for viewing diffs\")),\n> > +\t\tOPT_ARGUMENT(\"no-index\", &no_index, N_(\"passed to `diff`\")),\n> >  \t\tOPT_END()\n> >  \t};\n> \n> Much nicer.\n> \n> > +test_expect_success 'outside worktree' '\n> > +\techo 1 >1 &&\n> > +\techo 2 >2 &&\n> > +\ttest_expect_code 1 nongit git \\\n> > +\t\t-c diff.tool=echo -c difftool.echo.cmd=\"echo \\$LOCAL \\$REMOTE\" \\\n> > +\t\tdifftool --no-prompt --no-index ../1 ../2 >actual &&\n> > +\techo \"../1 ../2\" >expect &&\n> > +\ttest_cmp expect actual\n> > +'\n> \n> And this fixed all of my nits from the previous version. The whole\n> series looks good to me.\n\nThanks! (စ ͜ စ)\nDscho"},{"id":"371587","messageId":"nycvar.QRO.7.76.6.1903151421110.41@tvgsbejvaqbjf.bet","threadId":"50734","inReplyTo":"20190315031553.GB28943@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/3] parse-options: make OPT_ARGUMENT() more useful","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-15T13:24:50Z","receivedAt":"2019-03-15T13:25:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 14 Mar 2019, Jeff King wrote:\n\n> On Thu, Mar 14, 2019 at 04:25:04AM -0700, Johannes Schindelin via GitGitGadget wrote:\n> \n> > diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\n> > index 2b036d7838..2e2e7c10c6 100644\n> > --- a/Documentation/technical/api-parse-options.txt\n> > +++ b/Documentation/technical/api-parse-options.txt\n> > @@ -198,8 +198,10 @@ There are some macros to easily define options:\n> >  \tThe filename will be prefixed by passing the filename along with\n> >  \tthe prefix argument of `parse_options()` to `prefix_filename()`.\n> >  \n> > -`OPT_ARGUMENT(long, description)`::\n> > +`OPT_ARGUMENT(long, &int_var, description)`::\n> >  \tIntroduce a long-option argument that will be kept in `argv[]`.\n> > +\tIf this option was seen, `int_var` will be set to one (except\n> > +\tif a `NULL` pointer was passed).\n> \n> So this effectively makes it into a \"bool\" that we keep.\n\nAlmost. It is only a \"seen\" in the sense that you cannot re-set it to\n`false`.\n\nBut yeah, the difference is subtle.\n\n> I think that's fine. It always uses NOARG, so it is not like we would\n> ever need to see \"we got --foo, and this is the argument it had\".\n\nYep, I checked that.\n\n> I did wonder if it was possible for \"--no-foo\" to trigger this (leaving\n> the caller who looks at the int unsure if they saw \"--foo\" or\n> \"--no-foo\"), but it seems that the parse-options code checks for\n> OPTION_ARGUMENT before it ever looks at negation.\n\nI checked that, too.\n\n> Curiously, it also checks it before doing the usual prefix-matching\n> magic. So you could otherwise say \"--no-inde\", but OPT_ARGUMENT() will\n> not allow it. I think that's probably sane and not worth thinking\n> further about, but it is an interesting quirk that a user could possibly\n> run into.\n\nI missed that... ¯\\_(ツ)_/¯\n\n> > diff --git a/parse-options.c b/parse-options.c\n> > index cec74522e5..1d57802da0 100644\n> > --- a/parse-options.c\n> > +++ b/parse-options.c\n> > @@ -286,6 +286,8 @@ static enum parse_opt_result parse_long_opt(\n> >  \t\t\t\t\t     optname(options, flags));\n> >  \t\t\tif (*rest)\n> >  \t\t\t\tcontinue;\n> > +\t\t\tif (options->value)\n> > +\t\t\t\t*(int *)options->value = options->defval;\n> \n> Cute. You could actually assign any defval you like, though of course\n> the convenient OPT_ARGUMENT() macro just always uses 1.\n> \n> I wondered if you might need another cast for defval itself, but it's an\n> intptr_t (so it's the types that use it as a string that need to cast to\n> \"const char *\").\n\nExactly.\n\n> This looks very clean overall, and I agree it's much nicer than the\n> alternatives for your use case.\n\nThank you! 😊\nDscho"},{"id":"371765","messageId":"xmqq7ecw7vbb.fsf@gitster-ct.c.googlers.com","threadId":"50734","inReplyTo":"20190315031553.GB28943@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/3] parse-options: make OPT_ARGUMENT() more useful","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-18T02:47:20Z","receivedAt":"2019-03-18T02:47:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> +`OPT_ARGUMENT(long, &int_var, description)`::\n>>  \tIntroduce a long-option argument that will be kept in `argv[]`.\n>> +\tIf this option was seen, `int_var` will be set to one (except\n>> +\tif a `NULL` pointer was passed).\n>\n> So this effectively makes it into a \"bool\" that we keep. I think that's\n> fine. It always uses NOARG, so it is not like we would ever need to see\n> \"we got --foo, and this is the argument it had\".\n>\n> I did wonder if it was possible for \"--no-foo\" to trigger this (leaving\n> the caller who looks at the int unsure if they saw \"--foo\" or\n> \"--no-foo\"), but it seems that the parse-options code checks for\n> OPTION_ARGUMENT before it ever looks at negation.\n\nWhen a caller that needs to tell --no-foo and lack of any foo\nrelated option arises, we should be able to update the function\nfurther so that the caller can initialize the variable to -1\n(unspecified) and make sure that 0 is left upon seeing --no-foo\nso it's not a show stopper, I guess.\n\n\n"},{"id":"371842","messageId":"20190318210424.GA29661@sigill.intra.peff.net","threadId":"50734","inReplyTo":"xmqq7ecw7vbb.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 2/3] parse-options: make OPT_ARGUMENT() more useful","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-18T21:04:24Z","receivedAt":"2019-03-18T21:04:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 18, 2019 at 11:47:20AM +0900, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >> +`OPT_ARGUMENT(long, &int_var, description)`::\n> >>  \tIntroduce a long-option argument that will be kept in `argv[]`.\n> >> +\tIf this option was seen, `int_var` will be set to one (except\n> >> +\tif a `NULL` pointer was passed).\n> >\n> > So this effectively makes it into a \"bool\" that we keep. I think that's\n> > fine. It always uses NOARG, so it is not like we would ever need to see\n> > \"we got --foo, and this is the argument it had\".\n> >\n> > I did wonder if it was possible for \"--no-foo\" to trigger this (leaving\n> > the caller who looks at the int unsure if they saw \"--foo\" or\n> > \"--no-foo\"), but it seems that the parse-options code checks for\n> > OPTION_ARGUMENT before it ever looks at negation.\n> \n> When a caller that needs to tell --no-foo and lack of any foo\n> related option arises, we should be able to update the function\n> further so that the caller can initialize the variable to -1\n> (unspecified) and make sure that 0 is left upon seeing --no-foo\n> so it's not a show stopper, I guess.\n\nThe way it is written, I think the intent is that you would do:\n\n  OPT_ARGUMENT(\"foo\", &saw_foo, ...),\n  OPT_ARGUMENT(\"no-foo\", &saw_no_foo, ...),\n\nI'm happy to punt on it until it ever comes up (which I suspect may be\nnever).\n\n-Peff\n"},{"id":"371863","messageId":"xmqqd0mnzp4l.fsf@gitster-ct.c.googlers.com","threadId":"50734","inReplyTo":"20190318210424.GA29661@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/3] parse-options: make OPT_ARGUMENT() more useful","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-19T00:25:46Z","receivedAt":"2019-03-19T00:25:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The way it is written, I think the intent is that you would do:\n>\n>   OPT_ARGUMENT(\"foo\", &saw_foo, ...),\n>   OPT_ARGUMENT(\"no-foo\", &saw_no_foo, ...),\n>\n> I'm happy to punt on it until it ever comes up (which I suspect may be\n> never).\n\nYeah, having to have one extra element like the above may be ugly\nand less optimal API design, but something that is survivable, and\nwhen we actually need to use it, find it too ugly to live, and are\nmotivated to fix it, it will be fixed, so I won't be too worried\nabout it, either.\n"}]}