{"thread":{"id":"56487","subject":"[PATCH 0/2] parse-options.c: remove OPT_ARGUMENT","startedAt":"2021-09-11T18:21:18Z","lastAt":"2021-09-13T19:27:25Z","messageCount":17,"participants":["Ævar Arnfjörð Bjarmason","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"435579","messageId":"cover-0.2-00000000000-20210911T182009Z-avarab@gmail.com","threadId":"56487","inReplyTo":null,"subject":"[PATCH 0/2] parse-options.c: remove OPT_ARGUMENT","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-11T18:21:10Z","receivedAt":"2021-09-11T18:21:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"We've only ever used parse-option.c's OPT_ARGUMENT() in one place, as\nit turns out we can use an OPT_BOOL there instead. and get a net\nreduction in code & complexity.\n\nÆvar Arnfjörð Bjarmason (2):\n  difftool: use \"struct strvec\" API in run_{dir,file}_diff()\n  parse-options API: remove OPTION_ARGUMENT feature\n\n Documentation/technical/api-parse-options.txt |  5 --\n builtin/difftool.c                            | 50 +++++++++++--------\n parse-options.c                               | 13 -----\n parse-options.h                               |  3 --\n t/helper/test-parse-options.c                 |  1 -\n t/t0040-parse-options.sh                      |  5 --\n 6 files changed, 28 insertions(+), 49 deletions(-)\n\n-- \n2.33.0.995.ga5ea46173a2\n\n"},{"id":"435580","messageId":"patch-1.2-e7481eb0c0c-20210911T182009Z-avarab@gmail.com","threadId":"56487","inReplyTo":"cover-0.2-00000000000-20210911T182009Z-avarab@gmail.com","subject":"[PATCH 1/2] difftool: use \"struct strvec\" API in run_{dir,file}_diff()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-11T18:21:11Z","receivedAt":"2021-09-11T18:21:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The underlying run_command() API can take either the \"struct strvec\nargs\", or a \"const char **argv\". Let's move to the former to use the\nmore \"native\" version of run_command() in both of these functions.\n\nThis change probably isn't worth in on its own, but sets us up to\nsimplify API use even more in a subsequent commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/difftool.c | 46 +++++++++++++++++++++++++---------------------\n 1 file changed, 25 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 6a9242a8032..e656514bcac 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -331,7 +331,7 @@ static int checkout_path(unsigned mode, struct object_id *oid,\n }\n \n static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n-\t\t\tint argc, const char **argv)\n+\t\t\tstruct strvec *args)\n {\n \tchar tmpdir[PATH_MAX];\n \tstruct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;\n@@ -393,10 +393,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tchild.clean_on_exit = 1;\n \tchild.dir = prefix;\n \tchild.out = -1;\n-\tstrvec_pushl(&child.args, \"diff\", \"--raw\", \"--no-abbrev\", \"-z\",\n-\t\t     NULL);\n-\tfor (i = 0; i < argc; i++)\n-\t\tstrvec_push(&child.args, argv[i]);\n+\tchild.argv = args->v;\n+\n \tif (start_command(&child))\n \t\tdie(\"could not obtain raw diff\");\n \tfp = xfdopen(child.out, \"r\");\n@@ -663,30 +661,30 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tstrbuf_release(&rdir);\n \tstrbuf_release(&wtdir);\n \tstrbuf_release(&buf);\n+\tstrvec_clear(args);\n \n \treturn ret;\n }\n \n-static int run_file_diff(int prompt, const char *prefix,\n-\t\t\t int argc, const char **argv)\n+static int run_file_diff(int prompt, const char *prefix, struct strvec *args)\n {\n-\tstruct strvec args = STRVEC_INIT;\n-\tconst char *env[] = {\n-\t\t\"GIT_PAGER=\", \"GIT_EXTERNAL_DIFF=git-difftool--helper\", NULL,\n-\t\tNULL\n-\t};\n-\tint i;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tint ret;\n \n+\tstrvec_pushl(&cmd.env_array, \"GIT_PAGER=\",\n+\t\t     \"GIT_EXTERNAL_DIFF=git-difftool--helper\", NULL);\n \tif (prompt > 0)\n-\t\tenv[2] = \"GIT_DIFFTOOL_PROMPT=true\";\n+\t\tstrvec_push(&cmd.env_array, \"GIT_DIFFTOOL_PROMPT=true\");\n \telse if (!prompt)\n-\t\tenv[2] = \"GIT_DIFFTOOL_NO_PROMPT=true\";\n+\t\tstrvec_push(&cmd.env_array, \"GIT_DIFFTOOL_NO_PROMPT=true\");\n \n+\tcmd.git_cmd = 1;\n+\tcmd.dir = prefix;\n+\tcmd.argv = args->v;\n \n-\tstrvec_push(&args, \"diff\");\n-\tfor (i = 0; i < argc; i++)\n-\t\tstrvec_push(&args, argv[i]);\n-\treturn run_command_v_opt_cd_env(args.v, RUN_GIT_CMD, prefix, env);\n+\tret = run_command(&cmd);\n+\tstrvec_clear(args);\n+\treturn ret;\n }\n \n int cmd_difftool(int argc, const char **argv, const char *prefix)\n@@ -719,6 +717,7 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \t\tOPT_ARGUMENT(\"no-index\", &no_index, N_(\"passed to `diff`\")),\n \t\tOPT_END()\n \t};\n+\tstruct strvec args = STRVEC_INIT;\n \n \tgit_config(difftool_config, NULL);\n \tsymlinks = has_symlinks;\n@@ -768,7 +767,12 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \t * will invoke a separate instance of 'git-difftool--helper' for\n \t * each file that changed.\n \t */\n+\tstrvec_push(&args, \"diff\");\n+\tif (dir_diff)\n+\t\tstrvec_pushl(&args, \"--raw\", \"--no-abbrev\", \"-z\", NULL);\n+\tstrvec_pushv(&args, argv);\n+\n \tif (dir_diff)\n-\t\treturn run_dir_diff(extcmd, symlinks, prefix, argc, argv);\n-\treturn run_file_diff(prompt, prefix, argc, argv);\n+\t\treturn run_dir_diff(extcmd, symlinks, prefix, &args);\n+\treturn run_file_diff(prompt, prefix, &args);\n }\n-- \n2.33.0.995.ga5ea46173a2\n\n"},{"id":"435581","messageId":"patch-2.2-28b43789b11-20210911T182009Z-avarab@gmail.com","threadId":"56487","inReplyTo":"cover-0.2-00000000000-20210911T182009Z-avarab@gmail.com","subject":"[PATCH 2/2] parse-options API: remove OPTION_ARGUMENT feature","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-11T18:21:12Z","receivedAt":"2021-09-11T18:21:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As was noted in 1a85b49b87a (parse-options: make OPT_ARGUMENT() more\nuseful, 2019-03-14) there's only ever been one user of the\nOPT_ARGUMENT(), that user was added in 20de316e334 (difftool: allow\nrunning outside Git worktrees with --no-index, 2019-03-14).\n\nThe OPT_ARGUMENT() feature itself was added way back in\n580d5bffdea (parse-options: new option type to treat an option-like\nparameter as an argument., 2008-03-02), but as discussed in\n1a85b49b87a wasn't used until 20de316e334 in 2019.\n\nNow that the preceding commit has migrated this code over to using\n\"struct strvec\" to manage the \"args\" member of a \"struct\nchild_process\", we can just use that directly instead of relying on\nOPT_ARGUMENT.\n\nThis has a minor change in behavior in that if we'll pass --no-index\nwe'll now always pass it as the first argument, before we'd pass it in\nwhatever position the caller did. Preserving this was the real value\nof OPT_ARGUMENT(), but as it turns out we didn't need that either. We\ncan always inject it as the first argument, the other end will parse\nit just the same.\n\nNote that we cannot remove the \"out\" and \"cpidx\" members of \"struct\nparse_opt_ctx_t\" added in 580d5bffdea, while they were introduced with\nOPT_ARGUMENT() we since used them for other things.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Documentation/technical/api-parse-options.txt |  5 -----\n builtin/difftool.c                            |  4 +++-\n parse-options.c                               | 13 -------------\n parse-options.h                               |  3 ---\n t/helper/test-parse-options.c                 |  1 -\n t/t0040-parse-options.sh                      |  5 -----\n 6 files changed, 3 insertions(+), 28 deletions(-)\n\ndiff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\nindex 5a60bbfa7f4..acfd5dc1d8b 100644\n--- a/Documentation/technical/api-parse-options.txt\n+++ b/Documentation/technical/api-parse-options.txt\n@@ -198,11 +198,6 @@ 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, &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\n \tif it was an argument to the function given by `func_ptr`.\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex e656514bcac..9aa77a91043 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -714,7 +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_BOOL(0, \"no-index\", &no_index, N_(\"passed to `diff`\")),\n \t\tOPT_END()\n \t};\n \tstruct strvec args = STRVEC_INIT;\n@@ -768,6 +768,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \t * each file that changed.\n \t */\n \tstrvec_push(&args, \"diff\");\n+\tif (no_index)\n+\t\tstrvec_push(&args, \"--no-index\");\n \tif (dir_diff)\n \t\tstrvec_pushl(&args, \"--raw\", \"--no-abbrev\", \"-z\", NULL);\n \tstrvec_pushv(&args, argv);\ndiff --git a/parse-options.c b/parse-options.c\nindex 2abff136a17..55c5821b08d 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -310,19 +310,6 @@ static enum parse_opt_result parse_long_opt(\n again:\n \t\tif (!skip_prefix(arg, long_name, &rest))\n \t\t\trest = NULL;\n-\t\tif (options->type == OPTION_ARGUMENT) {\n-\t\t\tif (!rest)\n-\t\t\t\tcontinue;\n-\t\t\tif (*rest == '=')\n-\t\t\t\treturn error(_(\"%s takes no value\"),\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}\n \t\tif (!rest) {\n \t\t\t/* abbreviated? */\n \t\t\tif (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&\ndiff --git a/parse-options.h b/parse-options.h\nindex a845a9d9527..39d90882548 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -8,7 +8,6 @@\n enum parse_opt_type {\n \t/* special types */\n \tOPTION_END,\n-\tOPTION_ARGUMENT,\n \tOPTION_GROUP,\n \tOPTION_NUMBER,\n \tOPTION_ALIAS,\n@@ -155,8 +154,6 @@ struct option {\n #define OPT_INTEGER_F(s, l, v, h, f)     { OPTION_INTEGER, (s), (l), (v), N_(\"n\"), (h), (f) }\n \n #define OPT_END()                   { OPTION_END }\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 2051ce57db7..a282b6ff13e 100644\n--- a/t/helper/test-parse-options.c\n+++ b/t/helper/test-parse-options.c\n@@ -134,7 +134,6 @@ 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\", 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\",\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex ad4746d899a..da310ed29b1 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -37,7 +37,6 @@ String options\n     --list <str>          add str to list\n \n Magic arguments\n-    --quux                means --quux\n     -NUM                  set integer to NUM\n     +                     same as -b\n     --ambiguous           positive ambiguity\n@@ -263,10 +262,6 @@ test_expect_success 'detect possible typos' '\n \ttest_cmp typo.err output.err\n '\n \n-test_expect_success 'keep some options as arguments' '\n-\ttest-tool parse-options --expect=\"arg 00: --quux\" --quux\n-'\n-\n cat >expect <<\\EOF\n Callback: \"four\", 0\n boolean: 5\n-- \n2.33.0.995.ga5ea46173a2\n\n"},{"id":"435668","messageId":"YT6BnnXeAWn8BycF@coredump.intra.peff.net","threadId":"56487","inReplyTo":"patch-1.2-e7481eb0c0c-20210911T182009Z-avarab@gmail.com","subject":"Re: [PATCH 1/2] difftool: use \"struct strvec\" API in run_{dir,file}_diff()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-12T22:39:26Z","receivedAt":"2021-09-12T22:39:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 11, 2021 at 08:21:11PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> The underlying run_command() API can take either the \"struct strvec\n> args\", or a \"const char **argv\". Let's move to the former to use the\n> more \"native\" version of run_command() in both of these functions.\n\nIt sounds like we're moving to use child.args (the strvec interface)\ninstead of child.argv (the const char one). Which I support; I'd like to\neventually get rid of the argv interface entirely because it has\nmemory-ownership semantics that are easy to get wrong.\n\nBut this...\n\n> @@ -393,10 +393,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n>  \tchild.clean_on_exit = 1;\n>  \tchild.dir = prefix;\n>  \tchild.out = -1;\n> -\tstrvec_pushl(&child.args, \"diff\", \"--raw\", \"--no-abbrev\", \"-z\",\n> -\t\t     NULL);\n> -\tfor (i = 0; i < argc; i++)\n> -\t\tstrvec_push(&child.args, argv[i]);\n> +\tchild.argv = args->v;\n> +\n\n...is going in the opposite direction.\n\nI'd much rather see us continue to use child.args here, like:\n\n  strvec_pushv(&child.args, args->v);\n\nThough really I do think passing the strvec into run_dir_diff() is\nquestionable in the first place. The caller depends on us to free the\nmemory in the strvec for them, which is...subtle.\n\nIt does let you immediately return here:\n\n>  \tif (dir_diff)\n> -\t\treturn run_dir_diff(extcmd, symlinks, prefix, argc, argv);\n> -\treturn run_file_diff(prompt, prefix, argc, argv);\n> +\t\treturn run_dir_diff(extcmd, symlinks, prefix, &args);\n> +\treturn run_file_diff(prompt, prefix, &args);\n\nwithout doing any cleanup. But I think just:\n\n  if (dir_diff)\n\tret = run_dir_diff(...);\n  else\n\tret = run_file_diff(...);\n\n  strvec_clear(&args);\n  return ret;\n\nwould be a lot more obvious.\n\nI almost suggested that this could be done even simpler by having the\ncaller pass in one of two pre-made argv lists (in addition to what's in\nthe original argv). But the _actual_ benefit you want here is making it\neasy to construct those lists in the shared code of the caller. Your\ncommit message didn't at all make that clear (so to me it looked like\nyour \"cleanup\" was actively making things worse).\n\nIt only becomes apparent with the second patch. I would have found it\nmuch easier to understand with something like the patch below. And then\na further patch to use strvec_pushv instead of manually looping (even\ngetting rid of the argc parameters entirely!), and one to convert\nrun_file_diff() to use a struct child_process (which fixes its memory\nleak).\n\n\n-- >8 --\ndifftool: prepare \"diff\" cmdline in cmd_difftool()\n\nWe call into either run_dir_diff() or run_file_diff(), each of which\nsets up a child argv starting with \"diff\" and some hard-coded options\n(depending on which mode we're using). Let's extract that logic into the\ncaller, which will make it easier to modify the options for cases which\naffect both functions.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/difftool.c | 13 ++++++++-----\n 1 file changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 6a9242a803..91a8e51b0c 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -393,8 +393,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tchild.clean_on_exit = 1;\n \tchild.dir = prefix;\n \tchild.out = -1;\n-\tstrvec_pushl(&child.args, \"diff\", \"--raw\", \"--no-abbrev\", \"-z\",\n-\t\t     NULL);\n \tfor (i = 0; i < argc; i++)\n \t\tstrvec_push(&child.args, argv[i]);\n \tif (start_command(&child))\n@@ -683,7 +681,6 @@ static int run_file_diff(int prompt, const char *prefix,\n \t\tenv[2] = \"GIT_DIFFTOOL_NO_PROMPT=true\";\n \n \n-\tstrvec_push(&args, \"diff\");\n \tfor (i = 0; i < argc; i++)\n \t\tstrvec_push(&args, argv[i]);\n \treturn run_command_v_opt_cd_env(args.v, RUN_GIT_CMD, prefix, env);\n@@ -719,6 +716,7 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \t\tOPT_ARGUMENT(\"no-index\", &no_index, N_(\"passed to `diff`\")),\n \t\tOPT_END()\n \t};\n+\tstruct strvec args = STRVEC_INIT;\n \n \tgit_config(difftool_config, NULL);\n \tsymlinks = has_symlinks;\n@@ -768,7 +766,12 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \t * will invoke a separate instance of 'git-difftool--helper' for\n \t * each file that changed.\n \t */\n+\tstrvec_push(&args, \"diff\");\n+\tif (dir_diff)\n+\t\tstrvec_pushl(&args, \"--raw\", \"--no-abbrev\", \"-z\", NULL);\n+\tstrvec_pushv(&args, argv);\n+\n \tif (dir_diff)\n-\t\treturn run_dir_diff(extcmd, symlinks, prefix, argc, argv);\n-\treturn run_file_diff(prompt, prefix, argc, argv);\n+\t\treturn run_dir_diff(extcmd, symlinks, prefix, args.nr, args.v);\n+\treturn run_file_diff(prompt, prefix, args.nr, args.v);\n }\n-- \n2.33.0.811.g40f7f3a89c\n"},{"id":"435669","messageId":"YT6CMdITxaZZqufr@coredump.intra.peff.net","threadId":"56487","inReplyTo":"YT6BnnXeAWn8BycF@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] difftool: use \"struct strvec\" API in run_{dir,file}_diff()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-12T22:41:53Z","receivedAt":"2021-09-12T22:41:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 12, 2021 at 06:39:26PM -0400, Jeff King wrote:\n\n> It only becomes apparent with the second patch. I would have found it\n> much easier to understand with something like the patch below. And then\n> a further patch to use strvec_pushv instead of manually looping (even\n> getting rid of the argc parameters entirely!), and one to convert\n> run_file_diff() to use a struct child_process (which fixes its memory\n> leak).\n> \n> -- >8 --\n> difftool: prepare \"diff\" cmdline in cmd_difftool()\n\nNote that this actually introduces a new leak of the strvec in the\ncaller. So it would probably want the \"ret =\" think I suggested to be\nsquashed in.\n\n(I wasn't really planning to make a finished patch, but just trying to\nillustrate what had confused me in your original. I ended up closer than\nI had planned, though).\n\n-Peff\n"},{"id":"435670","messageId":"YT6ChAivw2Vk7Jkd@coredump.intra.peff.net","threadId":"56487","inReplyTo":"patch-2.2-28b43789b11-20210911T182009Z-avarab@gmail.com","subject":"Re: [PATCH 2/2] parse-options API: remove OPTION_ARGUMENT feature","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-12T22:43:16Z","receivedAt":"2021-09-12T22:43:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 11, 2021 at 08:21:12PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> As was noted in 1a85b49b87a (parse-options: make OPT_ARGUMENT() more\n> useful, 2019-03-14) there's only ever been one user of the\n> OPT_ARGUMENT(), that user was added in 20de316e334 (difftool: allow\n> running outside Git worktrees with --no-index, 2019-03-14).\n> \n> The OPT_ARGUMENT() feature itself was added way back in\n> 580d5bffdea (parse-options: new option type to treat an option-like\n> parameter as an argument., 2008-03-02), but as discussed in\n> 1a85b49b87a wasn't used until 20de316e334 in 2019.\n> \n> Now that the preceding commit has migrated this code over to using\n> \"struct strvec\" to manage the \"args\" member of a \"struct\n> child_process\", we can just use that directly instead of relying on\n> OPT_ARGUMENT.\n\nYeah, the change in difftool here looks fine (regardless of how the\ncleanup in the first patch gets there). And I'm happy to see this\nsomewhat weird OPT_ macro go away if nobody is using it.\n\n-Peff\n"},{"id":"435671","messageId":"xmqqlf41ibd1.fsf@gitster.g","threadId":"56487","inReplyTo":"YT6BnnXeAWn8BycF@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] difftool: use \"struct strvec\" API in run_{dir,file}_diff()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-13T00:10:34Z","receivedAt":"2021-09-13T00:13:42Z","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> On Sat, Sep 11, 2021 at 08:21:11PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> The underlying run_command() API can take either the \"struct strvec\n>> args\", or a \"const char **argv\". Let's move to the former to use the\n>> more \"native\" version of run_command() in both of these functions.\n>\n> It sounds like we're moving to use child.args (the strvec interface)\n> instead of child.argv (the const char one). Which I support; I'd like to\n> eventually get rid of the argv interface entirely because it has\n> memory-ownership semantics that are easy to get wrong.\n>\n> But this...\n>\n>> @@ -393,10 +393,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n>>  \tchild.clean_on_exit = 1;\n>>  \tchild.dir = prefix;\n>>  \tchild.out = -1;\n>> -\tstrvec_pushl(&child.args, \"diff\", \"--raw\", \"--no-abbrev\", \"-z\",\n>> -\t\t     NULL);\n>> -\tfor (i = 0; i < argc; i++)\n>> -\t\tstrvec_push(&child.args, argv[i]);\n>> +\tchild.argv = args->v;\n>> +\n>\n> ...is going in the opposite direction.\n>\n> I'd much rather see us continue to use child.args here, like:\n>\n>   strvec_pushv(&child.args, args->v);\n>\n> Though really I do think passing the strvec into run_dir_diff() is\n> questionable in the first place. The caller depends on us to free the\n> memory in the strvec for them, which is...subtle.\n> ...\n> +\tstrvec_push(&args, \"diff\");\n> +\tif (dir_diff)\n> +\t\tstrvec_pushl(&args, \"--raw\", \"--no-abbrev\", \"-z\", NULL);\n> +\tstrvec_pushv(&args, argv);\n> +\n>  \tif (dir_diff)\n> -\t\treturn run_dir_diff(extcmd, symlinks, prefix, argc, argv);\n> -\treturn run_file_diff(prompt, prefix, argc, argv);\n> +\t\treturn run_dir_diff(extcmd, symlinks, prefix, args.nr, args.v);\n> +\treturn run_file_diff(prompt, prefix, args.nr, args.v);\n>  }\n\nYes, I have to say that the end result of not having to rely on the\nstrvec type, in order to just call a main()- like function, makes it\nmuch more pleasant read.\n\n"},{"id":"435683","messageId":"cover-v2-0.4-00000000000-20210913T033204Z-avarab@gmail.com","threadId":"56487","inReplyTo":"cover-0.2-00000000000-20210911T182009Z-avarab@gmail.com","subject":"[PATCH v2 0/4] difftool refactoring + remove OPT_ARGUMENT() macro","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-13T03:35:36Z","receivedAt":"2021-09-13T03:45:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"We've only ever used parse-option.c's OPT_ARGUMENT() in one place, as\nit turns out we can use an OPT_BOOL there instead. and get a net\nreduction in code & complexity.\n\nI think this v2 should address the comments Jeff King had in\nhttps://lore.kernel.org/git/YT6BnnXeAWn8BycF@coredump.intra.peff.net/;\nthere's now an amended version of his proposed patch as part of this,\nbut I prepended it with another one to prepare the \"struct\nchild_process\" in cmd_difftool().\n\nDoing so nicely gets around the question of the strvec memory\nmanagement, since we can trust the run-command.c API to do that for\nus, but couldn't in my v1 when we'd copy our own \"struct strvec *\" to\nits \"args\".\n\nJeff King (1):\n  difftool: prepare \"diff\" cmdline in cmd_difftool()\n\nÆvar Arnfjörð Bjarmason (3):\n  difftool: prepare \"struct child_process\" in cmd_difftool()\n  difftool: use run_command() API in run_file_diff()\n  parse-options API: remove OPTION_ARGUMENT feature\n\n Documentation/technical/api-parse-options.txt |  5 --\n builtin/difftool.c                            | 51 ++++++++++---------\n parse-options.c                               | 13 -----\n parse-options.h                               |  3 --\n t/helper/test-parse-options.c                 |  1 -\n t/t0040-parse-options.sh                      |  5 --\n 6 files changed, 26 insertions(+), 52 deletions(-)\n\nRange-diff against v1:\n1:  e7481eb0c0c ! 1:  f57c6c9b069 difftool: use \"struct strvec\" API in run_{dir,file}_diff()\n    @@ Metadata\n     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## Commit message ##\n    -    difftool: use \"struct strvec\" API in run_{dir,file}_diff()\n    +    difftool: prepare \"struct child_process\" in cmd_difftool()\n     \n    -    The underlying run_command() API can take either the \"struct strvec\n    -    args\", or a \"const char **argv\". Let's move to the former to use the\n    -    more \"native\" version of run_command() in both of these functions.\n    -\n    -    This change probably isn't worth in on its own, but sets us up to\n    -    simplify API use even more in a subsequent commit.\n    +    Move the preparation of the \"struct child_process\" from run_dir_diff()\n    +    to its only caller, cmd_difftool(). This is in preparation for\n    +    migrating run_file_diff() to using the run_command() API directly, and\n    +    to move more of the shared setup of the two to cmd_difftool().\n     \n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n    @@ builtin/difftool.c: static int checkout_path(unsigned mode, struct object_id *oi\n      \n      static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n     -\t\t\tint argc, const char **argv)\n    -+\t\t\tstruct strvec *args)\n    ++\t\t\tint argc, const char **argv,\n    ++\t\t\tstruct child_process *child)\n      {\n      \tchar tmpdir[PATH_MAX];\n      \tstruct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;\n     @@ builtin/difftool.c: static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n    - \tchild.clean_on_exit = 1;\n    - \tchild.dir = prefix;\n    - \tchild.out = -1;\n    + \tstruct index_state wtindex;\n    + \tstruct checkout lstate, rstate;\n    + \tint rc, flags = RUN_GIT_CMD, err = 0;\n    +-\tstruct child_process child = CHILD_PROCESS_INIT;\n    + \tconst char *helper_argv[] = { \"difftool--helper\", NULL, NULL, NULL };\n    + \tstruct hashmap wt_modified, tmp_modified;\n    + \tint indices_loaded = 0;\n    +@@ builtin/difftool.c: static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n    + \trdir_len = rdir.len;\n    + \twtdir_len = wtdir.len;\n    + \n    +-\tchild.no_stdin = 1;\n    +-\tchild.git_cmd = 1;\n    +-\tchild.use_shell = 0;\n    +-\tchild.clean_on_exit = 1;\n    +-\tchild.dir = prefix;\n    +-\tchild.out = -1;\n     -\tstrvec_pushl(&child.args, \"diff\", \"--raw\", \"--no-abbrev\", \"-z\",\n    --\t\t     NULL);\n    --\tfor (i = 0; i < argc; i++)\n    ++\tchild->no_stdin = 1;\n    ++\tchild->git_cmd = 1;\n    ++\tchild->use_shell = 0;\n    ++\tchild->clean_on_exit = 1;\n    ++\tchild->dir = prefix;\n    ++\tchild->out = -1;\n    ++\tstrvec_pushl(&child->args, \"diff\", \"--raw\", \"--no-abbrev\", \"-z\",\n    + \t\t     NULL);\n    + \tfor (i = 0; i < argc; i++)\n     -\t\tstrvec_push(&child.args, argv[i]);\n    -+\tchild.argv = args->v;\n    -+\n    - \tif (start_command(&child))\n    +-\tif (start_command(&child))\n    ++\t\tstrvec_push(&child->args, argv[i]);\n    ++\tif (start_command(child))\n      \t\tdie(\"could not obtain raw diff\");\n    - \tfp = xfdopen(child.out, \"r\");\n    -@@ builtin/difftool.c: static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n    - \tstrbuf_release(&rdir);\n    - \tstrbuf_release(&wtdir);\n    - \tstrbuf_release(&buf);\n    -+\tstrvec_clear(args);\n    - \n    - \treturn ret;\n    - }\n    +-\tfp = xfdopen(child.out, \"r\");\n    ++\tfp = xfdopen(child->out, \"r\");\n      \n    --static int run_file_diff(int prompt, const char *prefix,\n    --\t\t\t int argc, const char **argv)\n    -+static int run_file_diff(int prompt, const char *prefix, struct strvec *args)\n    - {\n    --\tstruct strvec args = STRVEC_INIT;\n    --\tconst char *env[] = {\n    --\t\t\"GIT_PAGER=\", \"GIT_EXTERNAL_DIFF=git-difftool--helper\", NULL,\n    --\t\tNULL\n    --\t};\n    --\tint i;\n    -+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n    -+\tint ret;\n    - \n    -+\tstrvec_pushl(&cmd.env_array, \"GIT_PAGER=\",\n    -+\t\t     \"GIT_EXTERNAL_DIFF=git-difftool--helper\", NULL);\n    - \tif (prompt > 0)\n    --\t\tenv[2] = \"GIT_DIFFTOOL_PROMPT=true\";\n    -+\t\tstrvec_push(&cmd.env_array, \"GIT_DIFFTOOL_PROMPT=true\");\n    - \telse if (!prompt)\n    --\t\tenv[2] = \"GIT_DIFFTOOL_NO_PROMPT=true\";\n    -+\t\tstrvec_push(&cmd.env_array, \"GIT_DIFFTOOL_NO_PROMPT=true\");\n    - \n    -+\tcmd.git_cmd = 1;\n    -+\tcmd.dir = prefix;\n    -+\tcmd.argv = args->v;\n    - \n    --\tstrvec_push(&args, \"diff\");\n    --\tfor (i = 0; i < argc; i++)\n    --\t\tstrvec_push(&args, argv[i]);\n    --\treturn run_command_v_opt_cd_env(args.v, RUN_GIT_CMD, prefix, env);\n    -+\tret = run_command(&cmd);\n    -+\tstrvec_clear(args);\n    -+\treturn ret;\n    - }\n    + \t/* Build index info for left and right sides of the diff */\n    + \ti = 0;\n    +@@ builtin/difftool.c: static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n      \n    - int cmd_difftool(int argc, const char **argv, const char *prefix)\n    + \tfclose(fp);\n    + \tfp = NULL;\n    +-\tif (finish_command(&child)) {\n    ++\tif (finish_command(child)) {\n    + \t\tret = error(\"error occurred running diff --raw\");\n    + \t\tgoto finish;\n    + \t}\n     @@ builtin/difftool.c: int cmd_difftool(int argc, const char **argv, const char *prefix)\n      \t\tOPT_ARGUMENT(\"no-index\", &no_index, N_(\"passed to `diff`\")),\n      \t\tOPT_END()\n      \t};\n    -+\tstruct strvec args = STRVEC_INIT;\n    ++\tstruct child_process child = CHILD_PROCESS_INIT;\n      \n      \tgit_config(difftool_config, NULL);\n      \tsymlinks = has_symlinks;\n     @@ builtin/difftool.c: int cmd_difftool(int argc, const char **argv, const char *prefix)\n    - \t * will invoke a separate instance of 'git-difftool--helper' for\n      \t * each file that changed.\n      \t */\n    -+\tstrvec_push(&args, \"diff\");\n    -+\tif (dir_diff)\n    -+\t\tstrvec_pushl(&args, \"--raw\", \"--no-abbrev\", \"-z\", NULL);\n    -+\tstrvec_pushv(&args, argv);\n    -+\n      \tif (dir_diff)\n     -\t\treturn run_dir_diff(extcmd, symlinks, prefix, argc, argv);\n    --\treturn run_file_diff(prompt, prefix, argc, argv);\n    -+\t\treturn run_dir_diff(extcmd, symlinks, prefix, &args);\n    -+\treturn run_file_diff(prompt, prefix, &args);\n    ++\t\treturn run_dir_diff(extcmd, symlinks, prefix, argc, argv, &child);\n    + \treturn run_file_diff(prompt, prefix, argc, argv);\n      }\n-:  ----------- > 2:  1c2794115c7 difftool: prepare \"diff\" cmdline in cmd_difftool()\n-:  ----------- > 3:  2b093bd71fc difftool: use run_command() API in run_file_diff()\n2:  28b43789b11 ! 4:  4fddce0a38d parse-options API: remove OPTION_ARGUMENT feature\n    @@ builtin/difftool.c: int cmd_difftool(int argc, const char **argv, const char *pr\n     +\t\tOPT_BOOL(0, \"no-index\", &no_index, N_(\"passed to `diff`\")),\n      \t\tOPT_END()\n      \t};\n    - \tstruct strvec args = STRVEC_INIT;\n    + \tstruct child_process child = CHILD_PROCESS_INIT;\n     @@ builtin/difftool.c: int cmd_difftool(int argc, const char **argv, const char *prefix)\n      \t * each file that changed.\n      \t */\n    - \tstrvec_push(&args, \"diff\");\n    + \tstrvec_push(&child.args, \"diff\");\n     +\tif (no_index)\n    -+\t\tstrvec_push(&args, \"--no-index\");\n    ++\t\tstrvec_push(&child.args, \"--no-index\");\n      \tif (dir_diff)\n    - \t\tstrvec_pushl(&args, \"--raw\", \"--no-abbrev\", \"-z\", NULL);\n    - \tstrvec_pushv(&args, argv);\n    + \t\tstrvec_pushl(&child.args, \"--raw\", \"--no-abbrev\", \"-z\", NULL);\n    + \tstrvec_pushv(&child.args, argv);\n     \n      ## parse-options.c ##\n     @@ parse-options.c: static enum parse_opt_result parse_long_opt(\n-- \n2.33.0.999.ga5f89b684e9\n\n"},{"id":"435684","messageId":"patch-v2-2.4-1c2794115c7-20210913T033204Z-avarab@gmail.com","threadId":"56487","inReplyTo":"cover-v2-0.4-00000000000-20210913T033204Z-avarab@gmail.com","subject":"[PATCH v2 2/4] difftool: prepare \"diff\" cmdline in cmd_difftool()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-13T03:35:38Z","receivedAt":"2021-09-13T03:45:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nWe call into either run_dir_diff() or run_file_diff(), each of which\nsets up a child argv starting with \"diff\" and some hard-coded options\n(depending on which mode we're using). Let's extract that logic into the\ncaller, which will make it easier to modify the options for cases which\naffect both functions.\n\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/difftool.c | 15 +++++++--------\n 1 file changed, 7 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 9f08a8f3fd2..f8fcc67640f 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -331,7 +331,6 @@ static int checkout_path(unsigned mode, struct object_id *oid,\n }\n \n static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n-\t\t\tint argc, const char **argv,\n \t\t\tstruct child_process *child)\n {\n \tchar tmpdir[PATH_MAX];\n@@ -393,10 +392,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tchild->clean_on_exit = 1;\n \tchild->dir = prefix;\n \tchild->out = -1;\n-\tstrvec_pushl(&child->args, \"diff\", \"--raw\", \"--no-abbrev\", \"-z\",\n-\t\t     NULL);\n-\tfor (i = 0; i < argc; i++)\n-\t\tstrvec_push(&child->args, argv[i]);\n \tif (start_command(child))\n \t\tdie(\"could not obtain raw diff\");\n \tfp = xfdopen(child->out, \"r\");\n@@ -683,7 +678,6 @@ static int run_file_diff(int prompt, const char *prefix,\n \t\tenv[2] = \"GIT_DIFFTOOL_NO_PROMPT=true\";\n \n \n-\tstrvec_push(&args, \"diff\");\n \tfor (i = 0; i < argc; i++)\n \t\tstrvec_push(&args, argv[i]);\n \treturn run_command_v_opt_cd_env(args.v, RUN_GIT_CMD, prefix, env);\n@@ -769,7 +763,12 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \t * will invoke a separate instance of 'git-difftool--helper' for\n \t * each file that changed.\n \t */\n+\tstrvec_push(&child.args, \"diff\");\n+\tif (dir_diff)\n+\t\tstrvec_pushl(&child.args, \"--raw\", \"--no-abbrev\", \"-z\", NULL);\n+\tstrvec_pushv(&child.args, argv);\n+\n \tif (dir_diff)\n-\t\treturn run_dir_diff(extcmd, symlinks, prefix, argc, argv, &child);\n-\treturn run_file_diff(prompt, prefix, argc, argv);\n+\t\treturn run_dir_diff(extcmd, symlinks, prefix, &child);\n+\treturn run_file_diff(prompt, prefix, child.args.nr, child.args.v);\n }\n-- \n2.33.0.999.ga5f89b684e9\n\n"},{"id":"435685","messageId":"patch-v2-1.4-f57c6c9b069-20210913T033204Z-avarab@gmail.com","threadId":"56487","inReplyTo":"cover-v2-0.4-00000000000-20210913T033204Z-avarab@gmail.com","subject":"[PATCH v2 1/4] difftool: prepare \"struct child_process\" in cmd_difftool()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-13T03:35:37Z","receivedAt":"2021-09-13T03:45:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move the preparation of the \"struct child_process\" from run_dir_diff()\nto its only caller, cmd_difftool(). This is in preparation for\nmigrating run_file_diff() to using the run_command() API directly, and\nto move more of the shared setup of the two to cmd_difftool().\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/difftool.c | 29 +++++++++++++++--------------\n 1 file changed, 15 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 6a9242a8032..9f08a8f3fd2 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -331,7 +331,8 @@ static int checkout_path(unsigned mode, struct object_id *oid,\n }\n \n static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n-\t\t\tint argc, const char **argv)\n+\t\t\tint argc, const char **argv,\n+\t\t\tstruct child_process *child)\n {\n \tchar tmpdir[PATH_MAX];\n \tstruct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;\n@@ -352,7 +353,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tstruct index_state wtindex;\n \tstruct checkout lstate, rstate;\n \tint rc, flags = RUN_GIT_CMD, err = 0;\n-\tstruct child_process child = CHILD_PROCESS_INIT;\n \tconst char *helper_argv[] = { \"difftool--helper\", NULL, NULL, NULL };\n \tstruct hashmap wt_modified, tmp_modified;\n \tint indices_loaded = 0;\n@@ -387,19 +387,19 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \trdir_len = rdir.len;\n \twtdir_len = wtdir.len;\n \n-\tchild.no_stdin = 1;\n-\tchild.git_cmd = 1;\n-\tchild.use_shell = 0;\n-\tchild.clean_on_exit = 1;\n-\tchild.dir = prefix;\n-\tchild.out = -1;\n-\tstrvec_pushl(&child.args, \"diff\", \"--raw\", \"--no-abbrev\", \"-z\",\n+\tchild->no_stdin = 1;\n+\tchild->git_cmd = 1;\n+\tchild->use_shell = 0;\n+\tchild->clean_on_exit = 1;\n+\tchild->dir = prefix;\n+\tchild->out = -1;\n+\tstrvec_pushl(&child->args, \"diff\", \"--raw\", \"--no-abbrev\", \"-z\",\n \t\t     NULL);\n \tfor (i = 0; i < argc; i++)\n-\t\tstrvec_push(&child.args, argv[i]);\n-\tif (start_command(&child))\n+\t\tstrvec_push(&child->args, argv[i]);\n+\tif (start_command(child))\n \t\tdie(\"could not obtain raw diff\");\n-\tfp = xfdopen(child.out, \"r\");\n+\tfp = xfdopen(child->out, \"r\");\n \n \t/* Build index info for left and right sides of the diff */\n \ti = 0;\n@@ -525,7 +525,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \n \tfclose(fp);\n \tfp = NULL;\n-\tif (finish_command(&child)) {\n+\tif (finish_command(child)) {\n \t\tret = error(\"error occurred running diff --raw\");\n \t\tgoto finish;\n \t}\n@@ -719,6 +719,7 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \t\tOPT_ARGUMENT(\"no-index\", &no_index, N_(\"passed to `diff`\")),\n \t\tOPT_END()\n \t};\n+\tstruct child_process child = CHILD_PROCESS_INIT;\n \n \tgit_config(difftool_config, NULL);\n \tsymlinks = has_symlinks;\n@@ -769,6 +770,6 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \t * each file that changed.\n \t */\n \tif (dir_diff)\n-\t\treturn run_dir_diff(extcmd, symlinks, prefix, argc, argv);\n+\t\treturn run_dir_diff(extcmd, symlinks, prefix, argc, argv, &child);\n \treturn run_file_diff(prompt, prefix, argc, argv);\n }\n-- \n2.33.0.999.ga5f89b684e9\n\n"},{"id":"435686","messageId":"patch-v2-3.4-2b093bd71fc-20210913T033204Z-avarab@gmail.com","threadId":"56487","inReplyTo":"cover-v2-0.4-00000000000-20210913T033204Z-avarab@gmail.com","subject":"[PATCH v2 3/4] difftool: use run_command() API in run_file_diff()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-13T03:35:39Z","receivedAt":"2021-09-13T03:45:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the run_file_diff() function to use the run_command() API\ndirectly, instead of invoking the run_command_v_opt_cd_env() wrapper.\n\nThis allows it, like run_dir_diff(), to use the \"args\" from \"struct\nstrvec\", instead of the \"const char **argv\" passed into\ncmd_difftool(). This will be used in the subsequent commit to get rid\nof OPT_ARGUMENT() from cmd_difftool().\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/difftool.c | 13 ++++++-------\n 1 file changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex f8fcc67640f..de2e5545c81 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -663,24 +663,23 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n }\n \n static int run_file_diff(int prompt, const char *prefix,\n-\t\t\t int argc, const char **argv)\n+\t\t\t struct child_process *child)\n {\n-\tstruct strvec args = STRVEC_INIT;\n \tconst char *env[] = {\n \t\t\"GIT_PAGER=\", \"GIT_EXTERNAL_DIFF=git-difftool--helper\", NULL,\n \t\tNULL\n \t};\n-\tint i;\n \n \tif (prompt > 0)\n \t\tenv[2] = \"GIT_DIFFTOOL_PROMPT=true\";\n \telse if (!prompt)\n \t\tenv[2] = \"GIT_DIFFTOOL_NO_PROMPT=true\";\n \n+\tchild->git_cmd = 1;\n+\tchild->dir = prefix;\n+\tstrvec_pushv(&child->env_array, env);\n \n-\tfor (i = 0; i < argc; i++)\n-\t\tstrvec_push(&args, argv[i]);\n-\treturn run_command_v_opt_cd_env(args.v, RUN_GIT_CMD, prefix, env);\n+\treturn run_command(child);\n }\n \n int cmd_difftool(int argc, const char **argv, const char *prefix)\n@@ -770,5 +769,5 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \n \tif (dir_diff)\n \t\treturn run_dir_diff(extcmd, symlinks, prefix, &child);\n-\treturn run_file_diff(prompt, prefix, child.args.nr, child.args.v);\n+\treturn run_file_diff(prompt, prefix, &child);\n }\n-- \n2.33.0.999.ga5f89b684e9\n\n"},{"id":"435687","messageId":"patch-v2-4.4-4fddce0a38d-20210913T033204Z-avarab@gmail.com","threadId":"56487","inReplyTo":"cover-v2-0.4-00000000000-20210913T033204Z-avarab@gmail.com","subject":"[PATCH v2 4/4] parse-options API: remove OPTION_ARGUMENT feature","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-13T03:35:40Z","receivedAt":"2021-09-13T03:45:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As was noted in 1a85b49b87a (parse-options: make OPT_ARGUMENT() more\nuseful, 2019-03-14) there's only ever been one user of the\nOPT_ARGUMENT(), that user was added in 20de316e334 (difftool: allow\nrunning outside Git worktrees with --no-index, 2019-03-14).\n\nThe OPT_ARGUMENT() feature itself was added way back in\n580d5bffdea (parse-options: new option type to treat an option-like\nparameter as an argument., 2008-03-02), but as discussed in\n1a85b49b87a wasn't used until 20de316e334 in 2019.\n\nNow that the preceding commit has migrated this code over to using\n\"struct strvec\" to manage the \"args\" member of a \"struct\nchild_process\", we can just use that directly instead of relying on\nOPT_ARGUMENT.\n\nThis has a minor change in behavior in that if we'll pass --no-index\nwe'll now always pass it as the first argument, before we'd pass it in\nwhatever position the caller did. Preserving this was the real value\nof OPT_ARGUMENT(), but as it turns out we didn't need that either. We\ncan always inject it as the first argument, the other end will parse\nit just the same.\n\nNote that we cannot remove the \"out\" and \"cpidx\" members of \"struct\nparse_opt_ctx_t\" added in 580d5bffdea, while they were introduced with\nOPT_ARGUMENT() we since used them for other things.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Documentation/technical/api-parse-options.txt |  5 -----\n builtin/difftool.c                            |  4 +++-\n parse-options.c                               | 13 -------------\n parse-options.h                               |  3 ---\n t/helper/test-parse-options.c                 |  1 -\n t/t0040-parse-options.sh                      |  5 -----\n 6 files changed, 3 insertions(+), 28 deletions(-)\n\ndiff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\nindex 5a60bbfa7f4..acfd5dc1d8b 100644\n--- a/Documentation/technical/api-parse-options.txt\n+++ b/Documentation/technical/api-parse-options.txt\n@@ -198,11 +198,6 @@ 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, &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\n \tif it was an argument to the function given by `func_ptr`.\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex de2e5545c81..bb9fe7245a4 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -709,7 +709,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_BOOL(0, \"no-index\", &no_index, N_(\"passed to `diff`\")),\n \t\tOPT_END()\n \t};\n \tstruct child_process child = CHILD_PROCESS_INIT;\n@@ -763,6 +763,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \t * each file that changed.\n \t */\n \tstrvec_push(&child.args, \"diff\");\n+\tif (no_index)\n+\t\tstrvec_push(&child.args, \"--no-index\");\n \tif (dir_diff)\n \t\tstrvec_pushl(&child.args, \"--raw\", \"--no-abbrev\", \"-z\", NULL);\n \tstrvec_pushv(&child.args, argv);\ndiff --git a/parse-options.c b/parse-options.c\nindex 2abff136a17..55c5821b08d 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -310,19 +310,6 @@ static enum parse_opt_result parse_long_opt(\n again:\n \t\tif (!skip_prefix(arg, long_name, &rest))\n \t\t\trest = NULL;\n-\t\tif (options->type == OPTION_ARGUMENT) {\n-\t\t\tif (!rest)\n-\t\t\t\tcontinue;\n-\t\t\tif (*rest == '=')\n-\t\t\t\treturn error(_(\"%s takes no value\"),\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}\n \t\tif (!rest) {\n \t\t\t/* abbreviated? */\n \t\t\tif (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&\ndiff --git a/parse-options.h b/parse-options.h\nindex a845a9d9527..39d90882548 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -8,7 +8,6 @@\n enum parse_opt_type {\n \t/* special types */\n \tOPTION_END,\n-\tOPTION_ARGUMENT,\n \tOPTION_GROUP,\n \tOPTION_NUMBER,\n \tOPTION_ALIAS,\n@@ -155,8 +154,6 @@ struct option {\n #define OPT_INTEGER_F(s, l, v, h, f)     { OPTION_INTEGER, (s), (l), (v), N_(\"n\"), (h), (f) }\n \n #define OPT_END()                   { OPTION_END }\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 2051ce57db7..a282b6ff13e 100644\n--- a/t/helper/test-parse-options.c\n+++ b/t/helper/test-parse-options.c\n@@ -134,7 +134,6 @@ 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\", 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\",\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex ad4746d899a..da310ed29b1 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -37,7 +37,6 @@ String options\n     --list <str>          add str to list\n \n Magic arguments\n-    --quux                means --quux\n     -NUM                  set integer to NUM\n     +                     same as -b\n     --ambiguous           positive ambiguity\n@@ -263,10 +262,6 @@ test_expect_success 'detect possible typos' '\n \ttest_cmp typo.err output.err\n '\n \n-test_expect_success 'keep some options as arguments' '\n-\ttest-tool parse-options --expect=\"arg 00: --quux\" --quux\n-'\n-\n cat >expect <<\\EOF\n Callback: \"four\", 0\n boolean: 5\n-- \n2.33.0.999.ga5f89b684e9\n\n"},{"id":"435693","messageId":"xmqqh7epgfcp.fsf@gitster.g","threadId":"56487","inReplyTo":"patch-v2-4.4-4fddce0a38d-20210913T033204Z-avarab@gmail.com","subject":"Re: [PATCH v2 4/4] parse-options API: remove OPTION_ARGUMENT feature","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-13T06:27:18Z","receivedAt":"2021-09-13T06:27:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Now that the preceding commit has migrated this code over to using\n> \"struct strvec\" to manage the \"args\" member of a \"struct\n> child_process\", we can just use that directly instead of relying on\n> OPT_ARGUMENT.\n\nNice.\n"},{"id":"435758","messageId":"YT+SkhLsfE/yIqhN@coredump.intra.peff.net","threadId":"56487","inReplyTo":"patch-v2-3.4-2b093bd71fc-20210913T033204Z-avarab@gmail.com","subject":"Re: [PATCH v2 3/4] difftool: use run_command() API in run_file_diff()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-13T18:04:02Z","receivedAt":"2021-09-13T18:04:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 13, 2021 at 05:35:39AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Change the run_file_diff() function to use the run_command() API\n> directly, instead of invoking the run_command_v_opt_cd_env() wrapper.\n> \n> This allows it, like run_dir_diff(), to use the \"args\" from \"struct\n> strvec\", instead of the \"const char **argv\" passed into\n> cmd_difftool(). This will be used in the subsequent commit to get rid\n> of OPT_ARGUMENT() from cmd_difftool().\n\nIt also fixes the existing leak of its \"args\" strvec.\n\nI like the general direction here of putting the child_process in the\nparent. There's a few opportunities for further cleanup it opens, but\nI'm happy whether you want to pursue them or not (I'm also happy to do\nthem as real patches on top myself, but don't want to de-rail your\nseries).\n\n>  static int run_file_diff(int prompt, const char *prefix,\n> -\t\t\t int argc, const char **argv)\n> +\t\t\t struct child_process *child)\n>  {\n> -\tstruct strvec args = STRVEC_INIT;\n>  \tconst char *env[] = {\n>  \t\t\"GIT_PAGER=\", \"GIT_EXTERNAL_DIFF=git-difftool--helper\", NULL,\n>  \t\tNULL\n>  \t};\n> -\tint i;\n>  \n>  \tif (prompt > 0)\n>  \t\tenv[2] = \"GIT_DIFFTOOL_PROMPT=true\";\n>  \telse if (!prompt)\n>  \t\tenv[2] = \"GIT_DIFFTOOL_NO_PROMPT=true\";\n\nThis \"save a NULL in env where we might stick something in later\" is\nugly. Now that we have a child_process, it might be nicer as:\n\n  strvec_push(&env.args, \"GIT_PAGER=\");\n  strvec_push(&env.args, \"GIT_EXTERNAL_DIFF=git-difftool--helper\");\n  if (prompt > 0)\n\tstrvec_push(&env.args, \"GIT_DIFFTOOL_PROMPT=true\");\n  else if (!prompt)\n\tstrvec_push(&env.args, \"GIT_DIFFTOOL_NO_PROMPT=true\");\n\n> +\tchild->git_cmd = 1;\n> +\tchild->dir = prefix;\n\nThese are the same in run_dir_diff(). We could do more of the shared\nprep of the child_process in the caller. Likewise, we might want to pick\nup run_dir_diff()'s no_stdin and clean_on_exit settings (and unsetting\nuse_shell, but I think that is already pointless; it defaults to false\nin the first place, and git_cmd takes precedence over it anyway).\n\n-Peff\n"},{"id":"435759","messageId":"YT+Sv6gdNTMuP5NC@coredump.intra.peff.net","threadId":"56487","inReplyTo":"cover-v2-0.4-00000000000-20210913T033204Z-avarab@gmail.com","subject":"Re: [PATCH v2 0/4] difftool refactoring + remove OPT_ARGUMENT() macro","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-13T18:04:47Z","receivedAt":"2021-09-13T18:04:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 13, 2021 at 05:35:36AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> I think this v2 should address the comments Jeff King had in\n> https://lore.kernel.org/git/YT6BnnXeAWn8BycF@coredump.intra.peff.net/;\n> there's now an amended version of his proposed patch as part of this,\n> but I prepended it with another one to prepare the \"struct\n> child_process\" in cmd_difftool().\n> \n> Doing so nicely gets around the question of the strvec memory\n> management, since we can trust the run-command.c API to do that for\n> us, but couldn't in my v1 when we'd copy our own \"struct strvec *\" to\n> its \"args\".\n\nThanks for taking my suggestions into account. I like this approach, and\nI don't see any bugs. I replied separately to patch 3 with some further\nopportunities for cleanup, but with or without them, this is all a\nstrict improvement.\n\n-Peff\n"},{"id":"435772","messageId":"87a6kg46xh.fsf@evledraar.gmail.com","threadId":"56487","inReplyTo":"YT+SkhLsfE/yIqhN@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/4] difftool: use run_command() API in run_file_diff()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-13T19:13:43Z","receivedAt":"2021-09-13T19:22:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Sep 13 2021, Jeff King wrote:\n\n> On Mon, Sep 13, 2021 at 05:35:39AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Change the run_file_diff() function to use the run_command() API\n>> directly, instead of invoking the run_command_v_opt_cd_env() wrapper.\n>> \n>> This allows it, like run_dir_diff(), to use the \"args\" from \"struct\n>> strvec\", instead of the \"const char **argv\" passed into\n>> cmd_difftool(). This will be used in the subsequent commit to get rid\n>> of OPT_ARGUMENT() from cmd_difftool().\n>\n> It also fixes the existing leak of its \"args\" strvec.\n>\n> I like the general direction here of putting the child_process in the\n> parent. There's a few opportunities for further cleanup it opens, but\n> I'm happy whether you want to pursue them or not (I'm also happy to do\n> them as real patches on top myself, but don't want to de-rail your\n> series).\n\n*nod*\n\n>>  static int run_file_diff(int prompt, const char *prefix,\n>> -\t\t\t int argc, const char **argv)\n>> +\t\t\t struct child_process *child)\n>>  {\n>> -\tstruct strvec args = STRVEC_INIT;\n>>  \tconst char *env[] = {\n>>  \t\t\"GIT_PAGER=\", \"GIT_EXTERNAL_DIFF=git-difftool--helper\", NULL,\n>>  \t\tNULL\n>>  \t};\n>> -\tint i;\n>>  \n>>  \tif (prompt > 0)\n>>  \t\tenv[2] = \"GIT_DIFFTOOL_PROMPT=true\";\n>>  \telse if (!prompt)\n>>  \t\tenv[2] = \"GIT_DIFFTOOL_NO_PROMPT=true\";\n>\n> This \"save a NULL in env where we might stick something in later\" is\n> ugly. Now that we have a child_process, it might be nicer as:\n>\n>   strvec_push(&env.args, \"GIT_PAGER=\");\n>   strvec_push(&env.args, \"GIT_EXTERNAL_DIFF=git-difftool--helper\");\n>   if (prompt > 0)\n> \tstrvec_push(&env.args, \"GIT_DIFFTOOL_PROMPT=true\");\n>   else if (!prompt)\n> \tstrvec_push(&env.args, \"GIT_DIFFTOOL_NO_PROMPT=true\");\n\nYes, I had something like that in the earlier round, but decided to try\nto keep the diff minimal, churn and all that. So it would be nice, but\nlet's do it as some later cleanup if someone's interested.\n\n>> +\tchild->git_cmd = 1;\n>> +\tchild->dir = prefix;\n>\n> These are the same in run_dir_diff(). We could do more of the shared\n> prep of the child_process in the caller. Likewise, we might want to pick\n> up run_dir_diff()'s no_stdin and clean_on_exit settings (and unsetting\n> use_shell, but I think that is already pointless; it defaults to false\n> in the first place, and git_cmd takes precedence over it anyway).\n\n*nod*, but can be left for some other time.\n\nAside: I did most of that removal of \"argv\" from the child_process\nstruct you suggested, it's in\navar/run-command-use-less-argv-use-args-instead in my repo if you're\ncurious / wanted to pick any of that up. I won't be submitting it any\ntime soon for the reasons noted upthread.\n\nIn any case it has rather large semantic and textual conflicts with the\noutstanding hook.[ch] topic, but with that branch + that applied + a few\nchanges on top I was able to get the whole tree compiling & testing\nsuccessfully earlier.\n"},{"id":"435775","messageId":"YT+mGUkQAG7eVZp0@coredump.intra.peff.net","threadId":"56487","inReplyTo":"87a6kg46xh.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 3/4] difftool: use run_command() API in run_file_diff()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-13T19:27:21Z","receivedAt":"2021-09-13T19:27:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 13, 2021 at 09:13:43PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Yes, I had something like that in the earlier round, but decided to try\n> to keep the diff minimal, churn and all that. So it would be nice, but\n> let's do it as some later cleanup if someone's interested.\n\nOK, let's leave it for now, then.\n\n> Aside: I did most of that removal of \"argv\" from the child_process\n> struct you suggested, it's in\n> avar/run-command-use-less-argv-use-args-instead in my repo if you're\n> curious / wanted to pick any of that up. I won't be submitting it any\n> time soon for the reasons noted upthread.\n\nYeah, it's rather far-reaching, which is why I didn't do a\nmass-conversion when I introduced \"args\" in c460c0ecdc (run-command:\nstore an optional argv_array, 2014-05-15). My plan was to clean up spots\nover time as we touched them, but of course that's quite a slow process.\n\nMostly I just didn't want to see any sites going the _other_ direction,\nas in your original version of the series. :)\n\nI'm quite happy with what you ended up with here.\n\n-Peff\n"}]}