{"thread":{"id":"59999","subject":"[PATCH] ls-tree: fix --no-full-name","startedAt":"2023-07-18T15:44:19Z","lastAt":"2023-08-12T05:11:21Z","messageCount":66,"participants":["René Scharfe","Junio C Hamano","Phillip Wood","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"479599","messageId":"d392a005-4eba-7cc7-9554-cdb8dc53975e@web.de","threadId":"59999","inReplyTo":null,"subject":"[PATCH] ls-tree: fix --no-full-name","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-18T15:44:13Z","receivedAt":"2023-07-18T15:44:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Since 61fdbcf98b (ls-tree: migrate to parse-options, 2009-11-13) git\nls-tree has accepted the option --no-full-name, but it does the same\nas --full-name, contrary to convention. That's because it's defined\nusing OPT_SET_INT with a value of 0, where the negative variant sets\n0 as well.\n\nTurn --no-full-name into the opposite of --full-name by using OPT_BOOL\ninstead and storing the option's status directly in a variable named\n\"full_name\" instead of in negated form in \"chomp_prefix\".\n\nSigned-off-by: René Scharfe \n---\n builtin/ls-tree.c | 7 +++----\n t/t3101-ls-tree-dirname.sh | 8 ++++++++\n 2 files changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\nindex 53073d64cb..f558db5f3b 100644\n--- a/builtin/ls-tree.c\n+++ b/builtin/ls-tree.c\n@@ -343,7 +343,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n \tstruct object_id oid;\n \tstruct tree *tree;\n \tint i, full_tree = 0;\n-\tint chomp_prefix = prefix && *prefix;\n+\tint full_name = !prefix || !*prefix;\n \tread_tree_fn_t fn = NULL;\n \tenum ls_tree_cmdmode cmdmode = MODE_DEFAULT;\n \tint null_termination = 0;\n@@ -365,8 +365,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n \t\t\t MODE_NAME_STATUS),\n \t\tOPT_CMDMODE(0, \"object-only\", &cmdmode, N_(\"list only objects\"),\n \t\t\t MODE_OBJECT_ONLY),\n-\t\tOPT_SET_INT(0, \"full-name\", &chomp_prefix,\n-\t\t\t N_(\"use full path names\"), 0),\n+\t\tOPT_BOOL(0, \"full-name\", &full_name, N_(\"use full path names\")),\n \t\tOPT_BOOL(0, \"full-tree\", &full_tree,\n \t\t\t N_(\"list entire tree; not just current directory \"\n \t\t\t \"(implies --full-name)\")),\n@@ -387,7 +386,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n\n \tif (full_tree)\n \t\tprefix = NULL;\n-\toptions.prefix = chomp_prefix ? prefix : NULL;\n+\toptions.prefix = full_name ? NULL : prefix;\n\n \t/*\n \t * We wanted to detect conflicts between --name-only and\ndiff --git a/t/t3101-ls-tree-dirname.sh b/t/t3101-ls-tree-dirname.sh\nindex 217006d1bf..5af2dac0e4 100755\n--- a/t/t3101-ls-tree-dirname.sh\n+++ b/t/t3101-ls-tree-dirname.sh\n@@ -154,6 +154,14 @@ EOF\n \ttest_output\n '\n\n+test_expect_success 'ls-tree --no-full-name' '\n+\tgit -C path0 ls-tree --no-full-name $tree a >current &&\n+\tcat >expected <<-EOF &&\n+\t040000 tree X\ta\n+\tEOF\n+\ttest_output\n+'\n+\n test_expect_success 'ls-tree --full-tree' '\n \t(\n \t\tcd path1/b/c &&\n--\n2.41.0\n"},{"id":"479600","messageId":"xmqqo7k9fa5x.fsf@gitster.g","threadId":"59999","inReplyTo":"d392a005-4eba-7cc7-9554-cdb8dc53975e@web.de","subject":"Re: [PATCH] ls-tree: fix --no-full-name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-18T16:37:30Z","receivedAt":"2023-07-18T16:37:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Since 61fdbcf98b (ls-tree: migrate to parse-options, 2009-11-13) git\n> ls-tree has accepted the option --no-full-name, but it does the same\n> as --full-name, contrary to convention. That's because it's defined\n> using OPT_SET_INT with a value of 0, where the negative variant sets\n> 0 as well.\n\nOuch. Well spotted.\n\n> Turn --no-full-name into the opposite of --full-name by using OPT_BOOL\n> instead and storing the option's status directly in a variable named\n> \"full_name\" instead of in negated form in \"chomp_prefix\".\n\nGood solution, especially the flipping of the polarity of the\nvariable is very sensible.\n\nI wonder if there are cases where it makes sense to allow the\n\"--no-\" variant to an option parsed with OPT_SET_INT() that sets '0'\nas the value?\n\nSome random findings while reading hits from \"git grep OPT_SET_INT\":\n\n * \"git am --[no-]keep-cr\" is implemented as a pair of explicit\n PARSE_OPT_NONEG entries in the option[] array, but wouldn't it be\n sufficient to have a single OPT_SET_INT(\"keep-cr\")?\n\n * \"git branch --list --no-all\" is accepted, sets filter.kind to 0,\n and triggers \"fatal: filter_refs: invalid type\". Shouldn't we\n detect error much earlier?\n\n * \"git bundle create --no-quiet\" is accepted and sets the progress\n variable to 0, just like \"--quiet\" does, which is the same issue\n as the one fixed by your patch.\n\n * \"git clone (--no-ipv4|--no-ipv6)\" are accepted and uses\n TRANSPORT_FAMILY_ALL, presumably allowing both v4 and v6.\n Shouldn't we reject these? \"fetch\" and \"push\" share the same\n issue.\n\n * \"git status --no-(short|long|porcelain)\" are accepted and use\n STATUS_FORMAT_NONE, which probably is OK.\n\n * \"git commit --[no-](short|long|porcelain)\" are accepted and\n behave as \"git status\" without doing any \"git commit\" thing,\n which should be corrected, I think.\n\n * \"git describe --no-exact-match\" is the same as \"--exact-match\",\n which is the same issue as the one fixed by your patch.\n\n * \"git remote add\" has an OPT_SET_INT() entry whose short and long\n forms are (0, NULL). What is this supposed to do? Shouldn't\n parse-options.c:parse_options_check() notice it as an error?\n\n * \"git reset --(soft|hard|mixed|merge|keep)\" all take the negated\n form and they all become \"--mixed\". It may make sense to give\n all of them PARSE_OPT_NONEG.\n\n * \"git show-branch --no-sparse\" is the same as \"--sparse\",\n which is the same issue as the one fixed by your patch.\n\n * \"git show-branch --no-topo-order\" is the same as \"--topo-order\";\n as it is the default, we probably should give PARSE_OPT_NONEG.\n\n * \"git show-branch --no-date-order\" is the same as \"--topo-order\",\n which does sort-of make sense. This (and the previous one)\n relies on REV_SORT_IN_GRAPH_ORDER enum being 0, which smells a\n bit brittle.\n\n * \"git stash push --no-all\" is the same as \"--no-include-untracked\",\n which smells iffy but probably is OK.\n\nAnyway, the patch looks good. Will queue.\n\nThanks.\n\n> Signed-off-by: René Scharfe \n> ---\n> builtin/ls-tree.c | 7 +++----\n> t/t3101-ls-tree-dirname.sh | 8 ++++++++\n> 2 files changed, 11 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\n> index 53073d64cb..f558db5f3b 100644\n> --- a/builtin/ls-tree.c\n> +++ b/builtin/ls-tree.c\n> @@ -343,7 +343,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n> \tstruct object_id oid;\n> \tstruct tree *tree;\n> \tint i, full_tree = 0;\n> -\tint chomp_prefix = prefix && *prefix;\n> +\tint full_name = !prefix || !*prefix;\n> \tread_tree_fn_t fn = NULL;\n> \tenum ls_tree_cmdmode cmdmode = MODE_DEFAULT;\n> \tint null_termination = 0;\n> @@ -365,8 +365,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n> \t\t\t MODE_NAME_STATUS),\n> \t\tOPT_CMDMODE(0, \"object-only\", &cmdmode, N_(\"list only objects\"),\n> \t\t\t MODE_OBJECT_ONLY),\n> -\t\tOPT_SET_INT(0, \"full-name\", &chomp_prefix,\n> -\t\t\t N_(\"use full path names\"), 0),\n> +\t\tOPT_BOOL(0, \"full-name\", &full_name, N_(\"use full path names\")),\n> \t\tOPT_BOOL(0, \"full-tree\", &full_tree,\n> \t\t\t N_(\"list entire tree; not just current directory \"\n> \t\t\t \"(implies --full-name)\")),\n> @@ -387,7 +386,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n>\n> \tif (full_tree)\n> \t\tprefix = NULL;\n> -\toptions.prefix = chomp_prefix ? prefix : NULL;\n> +\toptions.prefix = full_name ? NULL : prefix;\n>\n> \t/*\n> \t * We wanted to detect conflicts between --name-only and\n> diff --git a/t/t3101-ls-tree-dirname.sh b/t/t3101-ls-tree-dirname.sh\n> index 217006d1bf..5af2dac0e4 100755\n> --- a/t/t3101-ls-tree-dirname.sh\n> +++ b/t/t3101-ls-tree-dirname.sh\n> @@ -154,6 +154,14 @@ EOF\n> \ttest_output\n> '\n>\n> +test_expect_success 'ls-tree --no-full-name' '\n> +\tgit -C path0 ls-tree --no-full-name $tree a >current &&\n> +\tcat >expected <<-EOF &&\n> +\t040000 tree X\ta\n> +\tEOF\n> +\ttest_output\n> +'\n> +\n> test_expect_success 'ls-tree --full-tree' '\n> \t(\n> \t\tcd path1/b/c &&\n> --\n> 2.41.0\n"},{"id":"479607","messageId":"xmqq4jm1gd2h.fsf@gitster.g","threadId":"59999","inReplyTo":"xmqqo7k9fa5x.fsf@gitster.g","subject":"Re: [PATCH] ls-tree: fix --no-full-name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-18T20:49:26Z","receivedAt":"2023-07-18T20:49:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> * \"git commit --[no-](short|long|porcelain)\" are accepted and\n> behave as \"git status\" without doing any \"git commit\" thing,\n> which should be corrected, I think.\n\nIt turns out that this was very much deliberate that these options\nimply \"--dry-run\", implemented in 7c9f7038 (commit: support\nalternate status formats, 2009-09-05).\n\nSo there is nothing to fix here.\n"},{"id":"479721","messageId":"ab1b29da-20ab-7139-dcb5-5b8858d31dee@web.de","threadId":"59999","inReplyTo":"xmqq4jm1gd2h.fsf@gitster.g","subject":"Re: [PATCH] ls-tree: fix --no-full-name","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-21T12:41:39Z","receivedAt":"2023-07-21T12:41:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 18.07.23 um 22:49 schrieb Junio C Hamano:\n> Junio C Hamano writes:\n>\n>> * \"git commit --[no-](short|long|porcelain)\" are accepted and\n>> behave as \"git status\" without doing any \"git commit\" thing,\n>> which should be corrected, I think.\n>\n> It turns out that this was very much deliberate that these options\n> imply \"--dry-run\", implemented in 7c9f7038 (commit: support\n> alternate status formats, 2009-09-05).\n>\n> So there is nothing to fix here.\n\nThe negated variants don't imply --dry-run and do actually commit\nsomething. Which kinda makes sense.\n\nRené\n"},{"id":"479722","messageId":"a6326aaa-4f05-4d00-8906-2f50ea8e1e7a@web.de","threadId":"59999","inReplyTo":"xmqqo7k9fa5x.fsf@gitster.g","subject":"Re: [PATCH] ls-tree: fix --no-full-name","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-21T12:41:35Z","receivedAt":"2023-07-21T12:41:51Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 18.07.23 um 18:37 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n> I wonder if there are cases where it makes sense to allow the\n> \"--no-\" variant to an option parsed with OPT_SET_INT() that sets '0'\n> as the value?\n\nI doubt it.\n\n> Some random findings while reading hits from \"git grep OPT_SET_INT\":\n\nWoah, so many!\n\n> * \"git branch --list --no-all\" is accepted, sets filter.kind to 0,\n> and triggers \"fatal: filter_refs: invalid type\". Shouldn't we\n> detect error much earlier?\n\nYes. And \"git branch --no-copy\" etc. are funny as well.\n\n> * \"git bundle create --no-quiet\" is accepted and sets the progress\n> variable to 0, just like \"--quiet\" does, which is the same issue\n> as the one fixed by your patch.\n\nThe same in pack-objects. It's a bit trickier because of the presence\nof a third state (--quiet, --progress and --all-progress). The help\ntext changes of 8b95521edb (bundle: turn on --all-progress-implied by\ndefault, 2023-03-04) state that only two states remain in git bundle\n(--quiet and --all-progress), but that's not fully true because the\noption --all-progress-implied is still wired up. \"git bundle\n--no-all-progress-implied --progress\" still gives git pack-objects a\nlone --progress.\n\n> * \"git clone (--no-ipv4|--no-ipv6)\" are accepted and uses\n> TRANSPORT_FAMILY_ALL, presumably allowing both v4 and v6.\n> Shouldn't we reject these? \"fetch\" and \"push\" share the same\n> issue.\n\nEither that, or we could turn them into OPT_BITs and let --no-ipv6\nmean \"give me anything but IPv6\", which currently happens to be\nthe same as --ipv4..\n\n> * \"git remote add\" has an OPT_SET_INT() entry whose short and long\n> forms are (0, NULL). What is this supposed to do? Shouldn't\n> parse-options.c:parse_options_check() notice it as an error?\n\nIt extends the help text of the previous option. Horrible.\n\n> * \"git stash push --no-all\" is the same as \"--no-include-untracked\",\n> which smells iffy but probably is OK.\n\nHard to imagine a situation where a --no-all would be well-defined\nand intuitive.\n\nOverall I get the impression that having the negative form enabled by\ndefault was not a good idea. For boolean options it makes sense, for\noptions with arguments perhaps as well, but for OPT_SET_INT we would\nhave less confusion if the negated form was opt-in.\n\nTo make it easier discoverable we could let the short help include\nthe optional \"no-\" part, which would look like this:\n\nusage: git ls-tree [] [...]\n\n -d only show trees\n -r recurse into subtrees\n -t show trees when recursing\n -z terminate entries with NUL byte\n -l, --long include object size\n --name-only list only filenames\n --name-status list only filenames\n --object-only list only objects\n --[no-]full-name use full path names\n --[no-]full-tree list entire tree; not just current directory (implies --full-name)\n --format format to use for the output\n --[no-]abbrev[=] use digits to display object names\n\nThoughts?\n\nRené\n"},{"id":"479723","messageId":"3f590f19-11a2-36d7-2520-1d1625ca1a25@web.de","threadId":"59999","inReplyTo":"xmqqo7k9fa5x.fsf@gitster.g","subject":"[PATCH] show-branch: fix --no-sparse","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-21T12:41:43Z","receivedAt":"2023-07-21T12:41:53Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Since 57343652a5 (show-branch: migrate to parse-options API, 2009-05-21)\ngit show-branch has accepted the option --no-sparse, but it does the\nsame as --sparse. That's because it's defined using OPT_SET_INT with a\nvalue of 0, which sets 0 when negated, too.\n\nTurn --no-sparse into the opposite of --sparse by using OPT_BOOL and\nstoring the option's status directly in a variable named \"sparse\"\ninstead of in negative form in \"dense\".\n\nSuggested-by: Junio C Hamano \nSigned-off-by: René Scharfe \n---\n builtin/show-branch.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex a86b3c7677..99b3f4a09a 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -649,7 +649,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \tint with_current_branch = 0;\n \tint head_at = -1;\n \tint topics = 0;\n-\tint dense = 1;\n+\tint sparse = 0;\n \tconst char *reflog_base = NULL;\n \tstruct option builtin_show_branch_options[] = {\n \t\tOPT_BOOL('a', \"all\", &all_heads,\n@@ -676,8 +676,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\t REV_SORT_IN_GRAPH_ORDER),\n \t\tOPT_BOOL(0, \"topics\", &topics,\n \t\t\t N_(\"show only commits not on the first branch\")),\n-\t\tOPT_SET_INT(0, \"sparse\", &dense,\n-\t\t\t N_(\"show merges reachable from only one tip\"), 0),\n+\t\tOPT_BOOL(0, \"sparse\", &sparse,\n+\t\t\t N_(\"show merges reachable from only one tip\")),\n \t\tOPT_SET_INT(0, \"date-order\", &sort_order,\n \t\t\t N_(\"topologically sort, maintaining date order \"\n \t\t\t \"where possible\"),\n@@ -940,7 +940,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\t !is_merge_point &&\n \t\t\t (this_flag & (1u << REV_SHIFT)))\n \t\t\t\tcontinue;\n-\t\t\tif (dense && is_merge &&\n+\t\t\tif (!sparse && is_merge &&\n \t\t\t omit_in_dense(commit, rev, num_rev))\n \t\t\t\tcontinue;\n \t\t\tfor (i = 0; i < num_rev; i++) {\n--\n2.41.0\n"},{"id":"479724","messageId":"417cab57-2952-9687-d8cb-72f5661a673f@web.de","threadId":"59999","inReplyTo":"xmqqo7k9fa5x.fsf@gitster.g","subject":"[PATCH] show-branch: disallow --no-{date,topo}-order","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-21T12:41:46Z","receivedAt":"2023-07-21T12:42:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Since 57343652a5 (show-branch: migrate to parse-options API, 2009-05-21)\ngit show-branch has accepted --no-date-order and --no-topo-order, but\nboth mean the same as --topo-order. That's because they are defined\nusing OPT_SET_INT, which sets the value to 0 for negated options, and\nthe value of REV_SORT_IN_GRAPH_ORDER happens to be 0 as well.\n\nOrdering chronologically or topologically is not a binary choice in\nprinciple; we could add e.g. an option to sort alphabetically. So\nnegating a sort order option makes no sense conceptually. Forbid it.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: René Scharfe \n---\n builtin/show-branch.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 99b3f4a09a..587d231dae 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -671,17 +671,17 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\t N_(\"show possible merge bases\")),\n \t\tOPT_BOOL(0, \"independent\", &independent,\n \t\t\t N_(\"show refs unreachable from any other ref\")),\n-\t\tOPT_SET_INT(0, \"topo-order\", &sort_order,\n-\t\t\t N_(\"show commits in topological order\"),\n-\t\t\t REV_SORT_IN_GRAPH_ORDER),\n+\t\tOPT_SET_INT_F(0, \"topo-order\", &sort_order,\n+\t\t\t N_(\"show commits in topological order\"),\n+\t\t\t REV_SORT_IN_GRAPH_ORDER, PARSE_OPT_NONEG),\n \t\tOPT_BOOL(0, \"topics\", &topics,\n \t\t\t N_(\"show only commits not on the first branch\")),\n \t\tOPT_BOOL(0, \"sparse\", &sparse,\n \t\t\t N_(\"show merges reachable from only one tip\")),\n-\t\tOPT_SET_INT(0, \"date-order\", &sort_order,\n-\t\t\t N_(\"topologically sort, maintaining date order \"\n-\t\t\t \"where possible\"),\n-\t\t\t REV_SORT_BY_COMMIT_DATE),\n+\t\tOPT_SET_INT_F(0, \"date-order\", &sort_order,\n+\t\t\t N_(\"topologically sort, maintaining date order \"\n+\t\t\t\t \"where possible\"),\n+\t\t\t REV_SORT_BY_COMMIT_DATE, PARSE_OPT_NONEG),\n \t\tOPT_CALLBACK_F('g', \"reflog\", &reflog_base, N_(\"[,]\"),\n \t\t\t N_(\"show most recent ref-log entries starting at \"\n \t\t\t \"base\"),\n--\n2.41.0\n"},{"id":"479725","messageId":"b35ec7dc-3c52-6688-4292-19ab83257aaf@web.de","threadId":"59999","inReplyTo":"xmqqo7k9fa5x.fsf@gitster.g","subject":"[PATCH] reset: disallow --no-{mixed,soft,hard,merge,keep}","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-21T12:41:49Z","receivedAt":"2023-07-21T12:42:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Since 5eee6b28b5 (Make builtin-reset.c use parse_options., 2008-03-04)\ngit reset has accepted the options --no-mixed, --no-soft and --no-hard,\n9e8eceab73 (Add 'merge' mode to 'git reset', 2008-12-01) and 9bc454df08\n(reset: add option \"--keep\" to \"git reset\", 2010-01-19) added --no-merge\nand --no-keep, respectively. They all do the same as --mixed, because\nthey are defined using OPT_SET_INT and the value of MIXED happens to be\n0. That's surprising and not very useful. Disallow the negated forms.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: René Scharfe \n---\n builtin/reset.c | 24 +++++++++++++++---------\n 1 file changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 7f18dc03b8..6292859d3c 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -338,15 +338,21 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\tOPT__QUIET(&quiet, N_(\"be quiet, only report errors\")),\n \t\tOPT_BOOL(0, \"no-refresh\", &no_refresh,\n \t\t\t\tN_(\"skip refreshing the index after reset\")),\n-\t\tOPT_SET_INT(0, \"mixed\", &reset_type,\n-\t\t\t\t\t\tN_(\"reset HEAD and index\"), MIXED),\n-\t\tOPT_SET_INT(0, \"soft\", &reset_type, N_(\"reset only HEAD\"), SOFT),\n-\t\tOPT_SET_INT(0, \"hard\", &reset_type,\n-\t\t\t\tN_(\"reset HEAD, index and working tree\"), HARD),\n-\t\tOPT_SET_INT(0, \"merge\", &reset_type,\n-\t\t\t\tN_(\"reset HEAD, index and working tree\"), MERGE),\n-\t\tOPT_SET_INT(0, \"keep\", &reset_type,\n-\t\t\t\tN_(\"reset HEAD but keep local changes\"), KEEP),\n+\t\tOPT_SET_INT_F(0, \"mixed\", &reset_type,\n+\t\t\t N_(\"reset HEAD and index\"), MIXED,\n+\t\t\t PARSE_OPT_NONEG),\n+\t\tOPT_SET_INT_F(0, \"soft\", &reset_type,\n+\t\t\t N_(\"reset only HEAD\"), SOFT,\n+\t\t\t PARSE_OPT_NONEG),\n+\t\tOPT_SET_INT_F(0, \"hard\", &reset_type,\n+\t\t\t N_(\"reset HEAD, index and working tree\"), HARD,\n+\t\t\t PARSE_OPT_NONEG),\n+\t\tOPT_SET_INT_F(0, \"merge\", &reset_type,\n+\t\t\t N_(\"reset HEAD, index and working tree\"), MERGE,\n+\t\t\t PARSE_OPT_NONEG),\n+\t\tOPT_SET_INT_F(0, \"keep\", &reset_type,\n+\t\t\t N_(\"reset HEAD but keep local changes\"), KEEP,\n+\t\t\t PARSE_OPT_NONEG),\n \t\tOPT_CALLBACK_F(0, \"recurse-submodules\", NULL,\n \t\t\t \"reset\", \"control recursive updating of submodules\",\n \t\t\t PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),\n--\n2.41.0\n"},{"id":"479726","messageId":"9f2e78f8-9d97-880e-dd54-eb86a8172b1f@web.de","threadId":"59999","inReplyTo":"xmqqo7k9fa5x.fsf@gitster.g","subject":"[PATCH] pack-objects: fix --no-quiet","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-21T12:41:53Z","receivedAt":"2023-07-21T12:42:20Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Since 99fb6e04cb (pack-objects: convert to use parse_options(),\n2012-02-01) git pack-objects has accepted the option --no-quiet, but it\ndoes the same as --quiet. That's because it's defined using OPT_SET_INT\nwith a value of 0, which sets 0 when negated, too.\n\nMake --no-quiet equivalent to --progress and ignore it if --all-progress\nwas given.\n\nSigned-off-by: René Scharfe \n---\n builtin/pack-objects.c | 17 +++++++++++++++--\n 1 file changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex fcf591c466..ccf451ce70 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4117,6 +4117,18 @@ static void add_extra_kept_packs(const struct string_list *names)\n \t}\n }\n\n+static int option_parse_quiet(const struct option *opt, const char *arg,\n+\t\t\t int unset)\n+{\n+\tBUG_ON_OPT_ARG(arg);\n+\n+\tif (!unset)\n+\t\tprogress = 0;\n+\telse if (!progress)\n+\t\tprogress = 1;\n+\treturn 0;\n+}\n+\n static int option_parse_index_version(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n {\n@@ -4178,8 +4190,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tLIST_OBJECTS_FILTER_INIT;\n\n \tstruct option pack_objects_options[] = {\n-\t\tOPT_SET_INT('q', \"quiet\", &progress,\n-\t\t\t N_(\"do not show progress meter\"), 0),\n+\t\tOPT_CALLBACK_F('q', \"quiet\", NULL, NULL,\n+\t\t\t N_(\"do not show progress meter\"),\n+\t\t\t PARSE_OPT_NOARG, option_parse_quiet),\n \t\tOPT_SET_INT(0, \"progress\", &progress,\n \t\t\t N_(\"show progress meter\"), 1),\n \t\tOPT_SET_INT(0, \"all-progress\", &progress,\n--\n2.41.0\n"},{"id":"479727","messageId":"fe7e43a0-5f08-73a7-fd04-b9127592b45c@web.de","threadId":"59999","inReplyTo":"xmqqo7k9fa5x.fsf@gitster.g","subject":"[PATCH] pack-objects: fix --no-keep-true-parents","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-21T12:41:56Z","receivedAt":"2023-07-21T12:42:28Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Since 99fb6e04cb (pack-objects: convert to use parse_options(),\n2012-02-01) git pack-objects has accepted --no-keep-true-parents, but\nthis option does the same as --keep-true-parents. That's because it's\ndefined using OPT_SET_INT with a value of 0, which sets 0 when negated\nas well.\n\nTurn --no-keep-true-parents into the opposite of --keep-true-parents by\nusing OPT_BOOL and storing the option's status directly in a variable\nnamed \"grants_keep_true_parents\" instead of in negative form in\n\"grafts_replace_parents\".\n\nSigned-off-by: René Scharfe \n---\n builtin/pack-objects.c | 4 ++--\n commit.c | 2 +-\n environment.c | 2 +-\n environment.h | 2 +-\n 4 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 06b33d49e9..7bff1be7aa 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4255,8 +4255,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"ignore this pack\")),\n \t\tOPT_INTEGER(0, \"compression\", &pack_compression_level,\n \t\t\t N_(\"pack compression level\")),\n-\t\tOPT_SET_INT(0, \"keep-true-parents\", &grafts_replace_parents,\n-\t\t\t N_(\"do not hide commits by grafts\"), 0),\n+\t\tOPT_BOOL(0, \"keep-true-parents\", &grafts_keep_true_parents,\n+\t\t\t N_(\"do not hide commits by grafts\")),\n \t\tOPT_BOOL(0, \"use-bitmap-index\", &use_bitmap_index,\n \t\t\t N_(\"use a bitmap index if available to speed up counting objects\")),\n \t\tOPT_SET_INT(0, \"write-bitmap-index\", &write_bitmap_index,\ndiff --git a/commit.c b/commit.c\nindex 6507791061..b3223478bc 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -516,7 +516,7 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n \t\t * The clone is shallow if nr_parent < 0, and we must\n \t\t * not traverse its real parents even when we unhide them.\n \t\t */\n-\t\tif (graft && (graft->nr_parent < 0 || grafts_replace_parents))\n+\t\tif (graft && (graft->nr_parent < 0 || !grafts_keep_true_parents))\n \t\t\tcontinue;\n \t\tnew_parent = lookup_commit(r, &parent);\n \t\tif (!new_parent)\ndiff --git a/environment.c b/environment.c\nindex a0d1d070d1..f98d76f080 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -73,7 +73,7 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;\n #endif\n enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;\n char *notes_ref_name;\n-int grafts_replace_parents = 1;\n+int grafts_keep_true_parents;\n int core_apply_sparse_checkout;\n int core_sparse_checkout_cone;\n int sparse_expect_files_outside_of_patterns;\ndiff --git a/environment.h b/environment.h\nindex 611aa0ffed..c5377473c6 100644\n--- a/environment.h\n+++ b/environment.h\n@@ -193,7 +193,7 @@ extern enum object_creation_mode object_creation_mode;\n\n extern char *notes_ref_name;\n\n-extern int grafts_replace_parents;\n+extern int grafts_keep_true_parents;\n\n extern int repository_format_precious_objects;\n\n--\n2.41.0\n"},{"id":"479728","messageId":"bf273082-4a33-8855-a928-5fff2c7e5ef4@web.de","threadId":"59999","inReplyTo":"xmqqo7k9fa5x.fsf@gitster.g","subject":"[PATCH] branch: disallow --no-{all,remotes}","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-21T12:42:01Z","receivedAt":"2023-07-21T12:42:30Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Since 171edcbb49 (git-branch: introduce missing long forms for the\noptions, 2011-08-28) git branch accepts --no-all and --no-remotes, but\nfails in both cases, reporting \"fatal: filter_refs: invalid type\".\nDisallow the options --all and --remotes to be negated in the first\nplace.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: René Scharfe \n---\n builtin/branch.c | 12 ++++++++----\n 1 file changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex a27bc0a3df..1bfd85f4d8 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -720,8 +720,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING('u', \"set-upstream-to\", &new_upstream, N_(\"upstream\"), N_(\"change the upstream info\")),\n \t\tOPT_BOOL(0, \"unset-upstream\", &unset_upstream, N_(\"unset the upstream info\")),\n \t\tOPT__COLOR(&branch_use_color, N_(\"use colored output\")),\n-\t\tOPT_SET_INT('r', \"remotes\", &filter.kind, N_(\"act on remote-tracking branches\"),\n-\t\t\tFILTER_REFS_REMOTES),\n+\t\tOPT_SET_INT_F('r', \"remotes\", &filter.kind,\n+\t\t\tN_(\"act on remote-tracking branches\"),\n+\t\t\tFILTER_REFS_REMOTES,\n+\t\t\tPARSE_OPT_NONEG),\n \t\tOPT_CONTAINS(&filter.with_commit, N_(\"print only branches that contain the commit\")),\n \t\tOPT_NO_CONTAINS(&filter.no_commit, N_(\"print only branches that don't contain the commit\")),\n \t\tOPT_WITH(&filter.with_commit, N_(\"print only branches that contain the commit\")),\n@@ -729,8 +731,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tOPT__ABBREV(&filter.abbrev),\n\n \t\tOPT_GROUP(N_(\"Specific git-branch actions:\")),\n-\t\tOPT_SET_INT('a', \"all\", &filter.kind, N_(\"list both remote-tracking and local branches\"),\n-\t\t\tFILTER_REFS_REMOTES | FILTER_REFS_BRANCHES),\n+\t\tOPT_SET_INT_F('a', \"all\", &filter.kind,\n+\t\t\tN_(\"list both remote-tracking and local branches\"),\n+\t\t\tFILTER_REFS_REMOTES | FILTER_REFS_BRANCHES,\n+\t\t\tPARSE_OPT_NONEG),\n \t\tOPT_BIT('d', \"delete\", &delete, N_(\"delete fully merged branch\"), 1),\n \t\tOPT_BIT('D', NULL, &delete, N_(\"delete branch (even if not merged)\"), 2),\n \t\tOPT_BIT('m', \"move\", &rename, N_(\"move/rename a branch and its reflog\"), 1),\n--\n2.41.0\n"},{"id":"479729","messageId":"3fc80266-3fc3-32f7-a918-3f7a854dfdb9@web.de","threadId":"59999","inReplyTo":"xmqqo7k9fa5x.fsf@gitster.g","subject":"[PATCH] am: unify definition of --keep-cr and --no-keep-cr","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-21T12:42:06Z","receivedAt":"2023-07-21T12:42:42Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The options --keep-cr and --no-keep-cr set the variable keep_cr to 1 and\n0, respectively. We don't usually define the negative variant\nexplicitly. The extra help text only tells users that the option\noverrules the config option am.keepcr, which conforms to convention.\n\nSo allow --keep-cr to be negated and drop the now redundant definition\nof --no-keep-cr for consistency.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: René Scharfe \n---\n builtin/am.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex dcb89439b1..a216024e1d 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2347,12 +2347,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"pass -b flag to git-mailinfo\"), KEEP_NON_PATCH),\n \t\tOPT_BOOL('m', \"message-id\", &state.message_id,\n \t\t\tN_(\"pass -m flag to git-mailinfo\")),\n-\t\tOPT_SET_INT_F(0, \"keep-cr\", &keep_cr,\n+\t\tOPT_SET_INT(0, \"keep-cr\", &keep_cr,\n \t\t\tN_(\"pass --keep-cr flag to git-mailsplit for mbox format\"),\n-\t\t\t1, PARSE_OPT_NONEG),\n-\t\tOPT_SET_INT_F(0, \"no-keep-cr\", &keep_cr,\n-\t\t\tN_(\"do not pass --keep-cr flag to git-mailsplit independent of am.keepcr\"),\n-\t\t\t0, PARSE_OPT_NONEG),\n+\t\t\t1),\n \t\tOPT_BOOL('c', \"scissors\", &state.scissors,\n \t\t\tN_(\"strip everything before a scissors line\")),\n \t\tOPT_CALLBACK_F(0, \"quoted-cr\", &state.quoted_cr, N_(\"action\"),\n--\n2.41.0\n"},{"id":"479732","messageId":"4eea7e15-6594-93e2-27b5-3d6e3c0baac6@web.de","threadId":"59999","inReplyTo":"xmqqo7k9fa5x.fsf@gitster.g","subject":"[PATCH] describe: fix --no-exact-match","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-21T13:41:33Z","receivedAt":"2023-07-21T13:41:43Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Since 2c33f75754 (Teach git-describe --exact-match to avoid expensive\ntag searches, 2008-02-24) git describe accepts --no-exact-match, but it\ndoes the same as --exact-match, an alias for --candidates=0. That's\nbecause it's defined using OPT_SET_INT with a value of 0, which sets 0\nwhen negated as well.\n\nLet --no-exact-match set the number of candidates to the default value\ninstead. Users that need a more specific lack of exactitude can specify\ntheir preferred value using --candidates, as before.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: René Scharfe \n---\nOr should we just forbid --no-exact-match?\n\n builtin/describe.c | 17 ++++++++++++++---\n 1 file changed, 14 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 7ce23e1b8e..b28a4a1f82 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -25,6 +25,7 @@\n #include \"wildmatch.h\"\n\n #define MAX_TAGS\t(FLAG_BITS - 1)\n+#define DEFAULT_CANDIDATES 10\n\n define_commit_slab(commit_names, struct commit_name *);\n\n@@ -41,7 +42,7 @@ static int tags;\t/* Allow lightweight tags */\n static int longformat;\n static int first_parent;\n static int abbrev = -1; /* unspecified */\n-static int max_candidates = 10;\n+static int max_candidates = DEFAULT_CANDIDATES;\n static struct hashmap names;\n static int have_util;\n static struct string_list patterns = STRING_LIST_INIT_NODUP;\n@@ -557,6 +558,15 @@ static void describe(const char *arg, int last_one)\n \tstrbuf_release(&sb);\n }\n\n+static int option_parse_exact_match(const struct option *opt, const char *arg,\n+\t\t\t\t int unset)\n+{\n+\tBUG_ON_OPT_ARG(arg);\n+\n+\tmax_candidates = unset ? DEFAULT_CANDIDATES : 0;\n+\treturn 0;\n+}\n+\n int cmd_describe(int argc, const char **argv, const char *prefix)\n {\n \tint contains = 0;\n@@ -568,8 +578,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"long\", &longformat, N_(\"always use long format\")),\n \t\tOPT_BOOL(0, \"first-parent\", &first_parent, N_(\"only follow first parent\")),\n \t\tOPT__ABBREV(&abbrev),\n-\t\tOPT_SET_INT(0, \"exact-match\", &max_candidates,\n-\t\t\t N_(\"only output exact matches\"), 0),\n+\t\tOPT_CALLBACK_F(0, \"exact-match\", NULL, NULL,\n+\t\t\t N_(\"only output exact matches\"),\n+\t\t\t PARSE_OPT_NOARG, option_parse_exact_match),\n \t\tOPT_INTEGER(0, \"candidates\", &max_candidates,\n \t\t\t N_(\"consider most recent tags (default: 10)\")),\n \t\tOPT_STRING_LIST(0, \"match\", &patterns, N_(\"pattern\"),\n--\n2.41.0\n"},{"id":"479733","messageId":"xmqqzg3pz76d.fsf@gitster.g","threadId":"59999","inReplyTo":"4eea7e15-6594-93e2-27b5-3d6e3c0baac6@web.de","subject":"Re: [PATCH] describe: fix --no-exact-match","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-21T14:10:50Z","receivedAt":"2023-07-21T14:11:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Since 2c33f75754 (Teach git-describe --exact-match to avoid expensive\n> tag searches, 2008-02-24) git describe accepts --no-exact-match, but it\n> does the same as --exact-match, an alias for --candidates=0. That's\n> because it's defined using OPT_SET_INT with a value of 0, which sets 0\n> when negated as well.\n>\n> Let --no-exact-match set the number of candidates to the default value\n> instead. Users that need a more specific lack of exactitude can specify\n> their preferred value using --candidates, as before.\n>\n> Suggested-by: Junio C Hamano \n> Signed-off-by: René Scharfe \n> ---\n> Or should we just forbid --no-exact-match?\n\nWe could do that, but setting it back to the default was also what I\ndid in my version I sent () a few days\nago.\n"},{"id":"479735","messageId":"xmqq351hz5xp.fsf@gitster.g","threadId":"59999","inReplyTo":"a6326aaa-4f05-4d00-8906-2f50ea8e1e7a@web.de","subject":"Re: [PATCH] ls-tree: fix --no-full-name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-21T14:37:38Z","receivedAt":"2023-07-21T14:38:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Overall I get the impression that having the negative form enabled by\n> default was not a good idea. For boolean options it makes sense, for\n> options with arguments perhaps as well, but for OPT_SET_INT we would\n> have less confusion if the negated form was opt-in.\n>\n> To make it easier discoverable we could let the short help include\n> the optional \"no-\" part, which would look like this:\n>\n> usage: git ls-tree [] [...]\n>\n> -d only show trees\n> -r recurse into subtrees\n> -t show trees when recursing\n> -z terminate entries with NUL byte\n> -l, --long include object size\n> --name-only list only filenames\n> --name-status list only filenames\n> --object-only list only objects\n> --[no-]full-name use full path names\n> --[no-]full-tree list entire tree; not just current directory (implies --full-name)\n> --format format to use for the output\n> --[no-]abbrev[=] use digits to display object names\n>\n> Thoughts?\n\nI like the \"optional no- accepted\" markings, but I suspect there may\nbe quite a lot of fallouts.\n\n"},{"id":"479736","messageId":"xmqqwmytxr59.fsf@gitster.g","threadId":"59999","inReplyTo":"3f590f19-11a2-36d7-2520-1d1625ca1a25@web.de","subject":"Re: [PATCH] show-branch: fix --no-sparse","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-21T14:42:26Z","receivedAt":"2023-07-21T14:43:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Since 57343652a5 (show-branch: migrate to parse-options API, 2009-05-21)\n> git show-branch has accepted the option --no-sparse, but it does the\n> same as --sparse. That's because it's defined using OPT_SET_INT with a\n> value of 0, which sets 0 when negated, too.\n\nHmph, am I expected to compare these patches with what I sent a few\ndays ago and pick whichever are the better ones? Can I delegate\nthat task to somebody else ;-)?\n\n jc/am-parseopt-fix\t\t\n jc/branch-parseopt-fix\t\t\n jc/describe-parseopt-fix\t\n jc/parse-options-reset\t\t\n jc/parse-options-short-help\t\n jc/parse-options-show-branch\t\n jc/transport-parseopt-fix\t\n\nSome of them are already in 'next' as they were so trivial.\n\nThanks.\n\n> Turn --no-sparse into the opposite of --sparse by using OPT_BOOL and\n> storing the option's status directly in a variable named \"sparse\"\n> instead of in negative form in \"dense\".\n>\n> Suggested-by: Junio C Hamano \n> Signed-off-by: René Scharfe \n> ---\n> builtin/show-branch.c | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/show-branch.c b/builtin/show-branch.c\n> index a86b3c7677..99b3f4a09a 100644\n> --- a/builtin/show-branch.c\n> +++ b/builtin/show-branch.c\n> @@ -649,7 +649,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n> \tint with_current_branch = 0;\n> \tint head_at = -1;\n> \tint topics = 0;\n> -\tint dense = 1;\n> +\tint sparse = 0;\n> \tconst char *reflog_base = NULL;\n> \tstruct option builtin_show_branch_options[] = {\n> \t\tOPT_BOOL('a', \"all\", &all_heads,\n> @@ -676,8 +676,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n> \t\t\t REV_SORT_IN_GRAPH_ORDER),\n> \t\tOPT_BOOL(0, \"topics\", &topics,\n> \t\t\t N_(\"show only commits not on the first branch\")),\n> -\t\tOPT_SET_INT(0, \"sparse\", &dense,\n> -\t\t\t N_(\"show merges reachable from only one tip\"), 0),\n> +\t\tOPT_BOOL(0, \"sparse\", &sparse,\n> +\t\t\t N_(\"show merges reachable from only one tip\")),\n> \t\tOPT_SET_INT(0, \"date-order\", &sort_order,\n> \t\t\t N_(\"topologically sort, maintaining date order \"\n> \t\t\t \"where possible\"),\n> @@ -940,7 +940,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n> \t\t\t !is_merge_point &&\n> \t\t\t (this_flag & (1u << REV_SHIFT)))\n> \t\t\t\tcontinue;\n> -\t\t\tif (dense && is_merge &&\n> +\t\t\tif (!sparse && is_merge &&\n> \t\t\t omit_in_dense(commit, rev, num_rev))\n> \t\t\t\tcontinue;\n> \t\t\tfor (i = 0; i < num_rev; i++) {\n> --\n> 2.41.0\n"},{"id":"479749","messageId":"b4912737-4ee1-11a5-847e-39c8eb2967d0@web.de","threadId":"59999","inReplyTo":"xmqqwmytxr59.fsf@gitster.g","subject":"Re: [PATCH] show-branch: fix --no-sparse","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-21T16:30:35Z","receivedAt":"2023-07-21T16:43:32Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.07.23 um 16:42 schrieb Junio C Hamano:\n>\n> Hmph, am I expected to compare these patches with what I sent a few\n> days ago and pick whichever are the better ones? Can I delegate\n> that task to somebody else ;-)?\n>\n> jc/am-parseopt-fix\t\t\n> jc/branch-parseopt-fix\t\t\n> jc/describe-parseopt-fix\t\n> jc/parse-options-reset\t\t\n> jc/parse-options-short-help\t\n> jc/parse-options-show-branch\t\n> jc/transport-parseopt-fix\t\n\nAww, didn't see them.\n\nMost differences are cosmetic, but in git describe you use two\nOPT_SET_INT_F for the two OPT_SET_INT_Fs for --exact-match and\n--no-exact-match. The callback I chose is a little uglier on the\ninside, but prettier on the outside -- occupies a single line in short\nhelp. Reminds me of the --[no-]keep-cr simplification in git am.\n\nAnd you don't seem to have touched pack-objects; please check those two\npatches of mine.\n\nRené\n"},{"id":"479750","messageId":"xmqqy1j9urmz.fsf@gitster.g","threadId":"59999","inReplyTo":"4eea7e15-6594-93e2-27b5-3d6e3c0baac6@web.de","subject":"Re: [PATCH] describe: fix --no-exact-match","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-21T17:00:04Z","receivedAt":"2023-07-21T17:00:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: René Scharfe \nDate: Fri, 21 Jul 2023 15:41:33 +0200\n\nSince 2c33f75754 (Teach git-describe --exact-match to avoid expensive\ntag searches, 2008-02-24) git describe accepts --no-exact-match, but it\ndoes the same as --exact-match, an alias for --candidates=0. That's\nbecause it's defined using OPT_SET_INT with a value of 0, which sets 0\nwhen negated as well.\n\nLet --no-exact-match set the number of candidates to the default value\ninstead. Users that need a more specific lack of exactitude can specify\ntheir preferred value using --candidates, as before.\n\nThe \"--no-exact-match\" option was not covered in the tests, so let's\nadd a few. Also add a case where --exact-match option is used on a\ncommit that cannot be described without distance from tags and make\nsure the command fails.\n\nSigned-off-by: René Scharfe \n[jc: added trivial tests]\nSigned-off-by: Junio C Hamano \n---\n * Stole tests from my version.\n\n builtin/describe.c | 17 ++++++++++++++---\n t/t6120-describe.sh | 8 ++++++++\n 2 files changed, 22 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 55b4baaa22..a5ba584cdd 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -24,6 +24,7 @@\n #include \"commit-slab.h\"\n \n #define MAX_TAGS\t(FLAG_BITS - 1)\n+#define DEFAULT_CANDIDATES 10\n \n define_commit_slab(commit_names, struct commit_name *);\n \n@@ -40,7 +41,7 @@ static int tags;\t/* Allow lightweight tags */\n static int longformat;\n static int first_parent;\n static int abbrev = -1; /* unspecified */\n-static int max_candidates = 10;\n+static int max_candidates = DEFAULT_CANDIDATES;\n static struct hashmap names;\n static int have_util;\n static struct string_list patterns = STRING_LIST_INIT_NODUP;\n@@ -556,6 +557,15 @@ static void describe(const char *arg, int last_one)\n \tstrbuf_release(&sb);\n }\n \n+static int option_parse_exact_match(const struct option *opt, const char *arg,\n+\t\t\t\t int unset)\n+{\n+\tBUG_ON_OPT_ARG(arg);\n+\n+\tmax_candidates = unset ? DEFAULT_CANDIDATES : 0;\n+\treturn 0;\n+}\n+\n int cmd_describe(int argc, const char **argv, const char *prefix)\n {\n \tint contains = 0;\n@@ -567,8 +577,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"long\", &longformat, N_(\"always use long format\")),\n \t\tOPT_BOOL(0, \"first-parent\", &first_parent, N_(\"only follow first parent\")),\n \t\tOPT__ABBREV(&abbrev),\n-\t\tOPT_SET_INT(0, \"exact-match\", &max_candidates,\n-\t\t\t N_(\"only output exact matches\"), 0),\n+\t\tOPT_CALLBACK_F(0, \"exact-match\", NULL, NULL,\n+\t\t\t N_(\"only output exact matches\"),\n+\t\t\t PARSE_OPT_NOARG, option_parse_exact_match),\n \t\tOPT_INTEGER(0, \"candidates\", &max_candidates,\n \t\t\t N_(\"consider most recent tags (default: 10)\")),\n \t\tOPT_STRING_LIST(0, \"match\", &patterns, N_(\"pattern\"),\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex c9afcef201..0a5c487540 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -85,6 +85,7 @@ check_describe e-1-gHASH --tags HEAD^^\n check_describe c-2-gHASH --tags HEAD^^2\n check_describe B --tags HEAD^^2^\n check_describe e --tags HEAD^^^\n+check_describe e --tags --exact-match HEAD^^^\n \n check_describe heads/main --all HEAD\n check_describe tags/c-6-gHASH --all HEAD^\n@@ -96,6 +97,13 @@ check_describe A-3-gHASH --long HEAD^^2\n check_describe c-7-gHASH --tags\n check_describe e-3-gHASH --first-parent --tags\n \n+check_describe c-7-gHASH --tags --no-exact-match HEAD\n+check_describe e-3-gHASH --first-parent --tags --no-exact-match HEAD\n+\n+test_expect_success '--exact-match failure' '\n+\ttest_must_fail git describe --exact-match HEAD 2>err\n+'\n+\n test_expect_success 'describe --contains defaults to HEAD without commit-ish' '\n \techo \"A^0\" >expect &&\n \tgit checkout A &&\n-- \n2.41.0-376-gcba07a324d\n\n"},{"id":"479751","messageId":"xmqqtttxurgi.fsf@gitster.g","threadId":"59999","inReplyTo":"fe7e43a0-5f08-73a7-fd04-b9127592b45c@web.de","subject":"Re: [PATCH] pack-objects: fix --no-keep-true-parents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-21T17:03:57Z","receivedAt":"2023-07-21T17:04:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Since 99fb6e04cb (pack-objects: convert to use parse_options(),\n> 2012-02-01) git pack-objects has accepted --no-keep-true-parents, but\n> this option does the same as --keep-true-parents. That's because it's\n> defined using OPT_SET_INT with a value of 0, which sets 0 when negated\n> as well.\n>\n> Turn --no-keep-true-parents into the opposite of --keep-true-parents by\n> using OPT_BOOL and storing the option's status directly in a variable\n> named \"grants_keep_true_parents\" instead of in negative form in\n\n\"grant\" -> \"graft\" (locally fixed, no need to resend).\n\n> \"grafts_replace_parents\".\n>\n> Signed-off-by: René Scharfe \n> ---\n\nThanks.\n"},{"id":"479768","messageId":"43ca3f01-ba11-6c29-a8e8-4e6c262a68cc@web.de","threadId":"59999","inReplyTo":"xmqq351hz5xp.fsf@gitster.g","subject":"Re: [PATCH] ls-tree: fix --no-full-name","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-21T19:29:35Z","receivedAt":"2023-07-21T19:29:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.07.23 um 16:37 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> Overall I get the impression that having the negative form enabled by\n>> default was not a good idea. For boolean options it makes sense, for\n>> options with arguments perhaps as well, but for OPT_SET_INT we would\n>> have less confusion if the negated form was opt-in.\n>>\n>> To make it easier discoverable we could let the short help include\n>> the optional \"no-\" part, which would look like this:\n>>\n>> usage: git ls-tree [] [...]\n>>\n>> -d only show trees\n>> -r recurse into subtrees\n>> -t show trees when recursing\n>> -z terminate entries with NUL byte\n>> -l, --long include object size\n>> --name-only list only filenames\n>> --name-status list only filenames\n>> --object-only list only objects\n>> --[no-]full-name use full path names\n>> --[no-]full-tree list entire tree; not just current directory (implies --full-name)\n>> --format format to use for the output\n>> --[no-]abbrev[=] use digits to display object names\n>>\n>> Thoughts?\n>\n> I like the \"optional no- accepted\" markings, but I suspect there may\n> be quite a lot of fallouts.\n\nSome test expectations in t0040 and t1502 would have to be adjusted.\n\nThis reveals, by the way, that t1502 doesn't yet exercise the \"!\" flag\nof \"git rev-parse --parseopt\" that turns on PARSE_OPT_NONEG. I find\nthe \"-h, --[no-]help\" option strangely amusing..\n\n--- >8 ----\nSubject: [PATCH] parse-options: show negatability of options in short help\n\nAdd a \"[no-]\" prefix to options without the flag PARSE_OPT_NONEG to\ndocument the fact that you can negate them.\n\nThis looks a bit strange for options that already start with \"no-\", e.g.\nfor the option --no-name of git show-branch:\n\n --[no-]no-name suppress naming strings\n\nYou can actually use --no-no-name as an alias of --name, so the short\nhelp is not wrong. If we strip off any of the \"no-\"s, we lose either\nthe ability to see if the remaining one belongs to the documented\nvariant or to see if it can be negated.\n\nSigned-off-by: René Scharfe \n---\n parse-options.c | 10 ++++-\n t/t0040-parse-options.sh | 44 ++++++++++---------\n t/t1502-rev-parse-parseopt.sh | 80 ++++++++++++++++++++---------------\n 3 files changed, 77 insertions(+), 57 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex f8a155ee13..6323ca191d 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -1136,8 +1136,14 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n \t\t}\n \t\tif (opts->long_name && opts->short_name)\n \t\t\tpos += fprintf(outfile, \", \");\n-\t\tif (opts->long_name)\n-\t\t\tpos += fprintf(outfile, \"--%s\", opts->long_name);\n+\t\tif (opts->long_name) {\n+\t\t\tconst char *long_name = opts->long_name;\n+\t\t\tif (opts->flags & PARSE_OPT_NONEG)\n+\t\t\t\tpos += fprintf(outfile, \"--%s\", long_name);\n+\t\t\telse\n+\t\t\t\tpos += fprintf(outfile, \"--[no-]%s\", long_name);\n+\t\t}\n+\n \t\tif (opts->type == OPTION_NUMBER)\n \t\t\tpos += utf8_fprintf(outfile, _(\"-NUM\"));\n\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 7d7ecfd571..f39758d2ef 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -13,29 +13,32 @@ usage: test-tool parse-options \n\n A helper function for the parse-options API.\n\n- --yes get a boolean\n- -D, --no-doubt begins with 'no-'\n+ --[no-]yes get a boolean\n+ -D, --[no-]no-doubt begins with 'no-'\n -B, --no-fear be brave\n- -b, --boolean increment by one\n- -4, --or4 bitwise-or boolean with ...0100\n- --neg-or4 same as --no-or4\n+ -b, --[no-]boolean increment by one\n+ -4, --[no-]or4 bitwise-or boolean with ...0100\n+ --[no-]neg-or4 same as --no-or4\n\n- -i, --integer get a integer\n+ -i, --[no-]integer \n+ get a integer\n -j get a integer, too\n -m, --magnitude get a magnitude\n- --set23 set integer to 23\n+ --[no-]set23 set integer to 23\n --mode1 set integer to 1 (cmdmode option)\n --mode2 set integer to 2 (cmdmode option)\n- -L, --length get length of \n- -F, --file set file to \n+ -L, --[no-]length \n+ get length of \n+ -F, --[no-]file \n+ set file to \n\n String options\n- -s, --string \n+ -s, --[no-]string \n get a string\n- --string2 get another string\n- --st get another string (pervert ordering)\n+ --[no-]string2 get another string\n+ --[no-]st get another string (pervert ordering)\n -o get another string\n- --list add str to list\n+ --[no-]list add str to list\n\n Magic arguments\n -NUM set integer to NUM\n@@ -44,16 +47,17 @@ Magic arguments\n --no-ambiguous negative ambiguity\n\n Standard options\n- --abbrev[=] use digits to display object names\n- -v, --verbose be verbose\n- -n, --dry-run dry run\n- -q, --quiet be quiet\n- --expect expected output in the variable dump\n+ --[no-]abbrev[=] use digits to display object names\n+ -v, --[no-]verbose be verbose\n+ -n, --[no-]dry-run dry run\n+ -q, --[no-]quiet be quiet\n+ --[no-]expect \n+ expected output in the variable dump\n\n Alias\n- -A, --alias-source \n+ -A, --[no-]alias-source \n get a string\n- -Z, --alias-target \n+ -Z, --[no-]alias-target \n alias of --alias-source\n\n EOF\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex dd811b7fb4..0a67e2dd4f 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -64,33 +64,38 @@ test_expect_success 'test --parseopt help output' '\n |\n | some-command does foo and bar!\n |\n-| -h, --help show the help\n-| --foo some nifty option --foo\n-| --bar ... some cool option --bar with an argument\n-| -b, --baz a short and long option\n+| -h, --[no-]help show the help\n+| --[no-]foo some nifty option --foo\n+| --[no-]bar ... some cool option --bar with an argument\n+| -b, --[no-]baz a short and long option\n |\n |An option group Header\n | -C[...] option C with an optional argument\n-| -d, --data[=...] short and long option with an optional argument\n+| -d, --[no-]data[=...]\n+| short and long option with an optional argument\n |\n |Argument hints\n | -B short option required argument\n-| --bar2 long option required argument\n-| -e, --fuz \n+| --[no-]bar2 long option required argument\n+| -e, --[no-]fuz \n | short and long option required argument\n | -s[] short option optional argument\n-| --long[=] long option optional argument\n-| -g, --fluf[=] short and long option optional argument\n-| --longest \n+| --[no-]long[=] long option optional argument\n+| -g, --[no-]fluf[=]\n+| short and long option optional argument\n+| --[no-]longest \n | a very long argument hint\n-| --pair with an equals sign in the hint\n-| --aswitch help te=t contains? fl*g characters!`\n-| --bswitch hint has trailing tab character\n-| --cswitch switch has trailing tab character\n-| --short-hint with a one symbol hint\n+| --[no-]pair \n+| with an equals sign in the hint\n+| --[no-]aswitch help te=t contains? fl*g characters!`\n+| --[no-]bswitch \n+| hint has trailing tab character\n+| --[no-]cswitch switch has trailing tab character\n+| --[no-]short-hint \n+| with a one symbol hint\n |\n |Extras\n-| --extra1 line above used to cause a segfault but no longer does\n+| --[no-]extra1 line above used to cause a segfault but no longer does\n |\n |EOF\n END_EXPECT\n@@ -131,7 +136,7 @@ test_expect_success 'test --parseopt help-all output hidden switches' '\n |\n | some-command does foo and bar!\n |\n-| --hidden1 A hidden switch\n+| --[no-]hidden1 A hidden switch\n |\n |EOF\n END_EXPECT\n@@ -146,33 +151,38 @@ test_expect_success 'test --parseopt invalid switch help output' '\n |\n | some-command does foo and bar!\n |\n-| -h, --help show the help\n-| --foo some nifty option --foo\n-| --bar ... some cool option --bar with an argument\n-| -b, --baz a short and long option\n+| -h, --[no-]help show the help\n+| --[no-]foo some nifty option --foo\n+| --[no-]bar ... some cool option --bar with an argument\n+| -b, --[no-]baz a short and long option\n |\n |An option group Header\n | -C[...] option C with an optional argument\n-| -d, --data[=...] short and long option with an optional argument\n+| -d, --[no-]data[=...]\n+| short and long option with an optional argument\n |\n |Argument hints\n | -B short option required argument\n-| --bar2 long option required argument\n-| -e, --fuz \n+| --[no-]bar2 long option required argument\n+| -e, --[no-]fuz \n | short and long option required argument\n | -s[] short option optional argument\n-| --long[=] long option optional argument\n-| -g, --fluf[=] short and long option optional argument\n-| --longest \n+| --[no-]long[=] long option optional argument\n+| -g, --[no-]fluf[=]\n+| short and long option optional argument\n+| --[no-]longest \n | a very long argument hint\n-| --pair with an equals sign in the hint\n-| --aswitch help te=t contains? fl*g characters!`\n-| --bswitch hint has trailing tab character\n-| --cswitch switch has trailing tab character\n-| --short-hint with a one symbol hint\n+| --[no-]pair \n+| with an equals sign in the hint\n+| --[no-]aswitch help te=t contains? fl*g characters!`\n+| --[no-]bswitch \n+| hint has trailing tab character\n+| --[no-]cswitch switch has trailing tab character\n+| --[no-]short-hint \n+| with a one symbol hint\n |\n |Extras\n-| --extra1 line above used to cause a segfault but no longer does\n+| --[no-]extra1 line above used to cause a segfault but no longer does\n |\n END_EXPECT\n \ttest_expect_code 129 git rev-parse --parseopt -- --does-not-exist 1>/dev/null 2>output < optionspec &&\n@@ -297,7 +307,7 @@ test_expect_success 'test --parseopt help output: \"wrapped\" options normal \"or:\"\n \t| or: [--another-option]\n \t| or: cmd [--yet-another-option]\n \t|\n-\t| -h, --help show the help\n+\t| -h, --[no-]help show the help\n \t|\n \t|EOF\n \tEND_EXPECT\n@@ -334,7 +344,7 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l\n \t| line\n \t| blurb\n \t|\n-\t| -h, --help show the help\n+\t| -h, --[no-]help show the help\n \t|\n \t|EOF\n \tEND_EXPECT\n--\n2.41.0\n"},{"id":"479769","messageId":"xmqq4jlxuiuu.fsf@gitster.g","threadId":"59999","inReplyTo":"43ca3f01-ba11-6c29-a8e8-4e6c262a68cc@web.de","subject":"Re: [PATCH] ls-tree: fix --no-full-name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-21T20:09:45Z","receivedAt":"2023-07-21T20:09:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Some test expectations in t0040 and t1502 would have to be adjusted.\n>\n> This reveals, by the way, that t1502 doesn't yet exercise the \"!\" flag\n> of \"git rev-parse --parseopt\" that turns on PARSE_OPT_NONEG. I find\n> the \"-h, --[no-]help\" option strangely amusing..\n>\n> --- >8 ----\n> Subject: [PATCH] parse-options: show negatability of options in short help\n>\n> Add a \"[no-]\" prefix to options without the flag PARSE_OPT_NONEG to\n> document the fact that you can negate them.\n>\n> This looks a bit strange for options that already start with \"no-\", e.g.\n> for the option --no-name of git show-branch:\n>\n> --[no-]no-name suppress naming strings\n>\n> You can actually use --no-no-name as an alias of --name, so the short\n> help is not wrong. If we strip off any of the \"no-\"s, we lose either\n> the ability to see if the remaining one belongs to the documented\n> variant or to see if it can be negated.\n>\n> Signed-off-by: René Scharfe \n> ---\n> parse-options.c | 10 ++++-\n> t/t0040-parse-options.sh | 44 ++++++++++---------\n> t/t1502-rev-parse-parseopt.sh | 80 ++++++++++++++++++++---------------\n> 3 files changed, 77 insertions(+), 57 deletions(-)\n>\n> diff --git a/parse-options.c b/parse-options.c\n> index f8a155ee13..6323ca191d 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -1136,8 +1136,14 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n> \t\t}\n> \t\tif (opts->long_name && opts->short_name)\n> \t\t\tpos += fprintf(outfile, \", \");\n> -\t\tif (opts->long_name)\n> -\t\t\tpos += fprintf(outfile, \"--%s\", opts->long_name);\n> +\t\tif (opts->long_name) {\n> +\t\t\tconst char *long_name = opts->long_name;\n> +\t\t\tif (opts->flags & PARSE_OPT_NONEG)\n> +\t\t\t\tpos += fprintf(outfile, \"--%s\", long_name);\n> +\t\t\telse\n> +\t\t\t\tpos += fprintf(outfile, \"--[no-]%s\", long_name);\n> +\t\t}\n\nThis is a good starting point, but we should at least exempt\nOPT_BOOL from this exercise, I would think, because ...\n\n> A helper function for the parse-options API.\n>\n> - --yes get a boolean\n> + --[no-]yes get a boolean\n\n... they are designed to be prefixed with an optional \"no-\".\n\n\n> - -D, --no-doubt begins with 'no-'\n> + -D, --[no-]no-doubt begins with 'no-'\n\nHmph, I really really loved the neat trick to allow \"no-doubt\"\noption to be \"positivised\" by _dropping_ the leading \"no-\" at around\n0f1930c5 (parse-options: allow positivation of options starting,\nwith no-, 2012-02-25).\n\n> EOF\n\nMany of the above are amusing and served as good demonstration to\nshow the blast radius, but it seems that most of them should be\nmarked with PARSE_OPT_NONEG.\n\n> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\n> index dd811b7fb4..0a67e2dd4f 100755\n> --- a/t/t1502-rev-parse-parseopt.sh\n> +++ b/t/t1502-rev-parse-parseopt.sh\n> @@ -64,33 +64,38 @@ test_expect_success 'test --parseopt help output' '\n> |\n> | some-command does foo and bar!\n> |\n> -| -h, --help show the help\n> -| --foo some nifty option --foo\n> -| --bar ... some cool option --bar with an argument\n> -| -b, --baz a short and long option\n> +| -h, --[no-]help show the help\n\nIndeed it is amusing, but we probably should give PARSE_OPT_NONEG\nappropriately, instead of changing the expectations, for many of the\nchanges we see here, I think.\n\n"},{"id":"479770","messageId":"xmqqzg3pt424.fsf@gitster.g","threadId":"59999","inReplyTo":"xmqq4jlxuiuu.fsf@gitster.g","subject":"Re: [PATCH] ls-tree: fix --no-full-name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-21T20:14:43Z","receivedAt":"2023-07-21T20:14:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> This is a good starting point, but we should at least exempt\n> OPT_BOOL from this exercise, I would think, because ...\n\nNo, no no, that was nonsense. The literal \"yes\" fooled me.\nThere is no need to special-case OPT_BOOL at all.\n\nSorry for the noise.\n"},{"id":"479803","messageId":"1535f30e-3cf9-1a0a-04af-4ba4a7c46d15@web.de","threadId":"59999","inReplyTo":"xmqq4jlxuiuu.fsf@gitster.g","subject":"Re: [PATCH] ls-tree: fix --no-full-name","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-24T12:29:33Z","receivedAt":"2023-07-24T12:30:22Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.07.23 um 22:09 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> - -D, --no-doubt begins with 'no-'\n>> + -D, --[no-]no-doubt begins with 'no-'\n>\n> Hmph, I really really loved the neat trick to allow \"no-doubt\"\n> option to be \"positivised\" by _dropping_ the leading \"no-\" at around\n> 0f1930c5 (parse-options: allow positivation of options starting,\n> with no-, 2012-02-25).\n\nYeah, if there is a better way to document A) that the \"no-\" is optional\nand B) whether it's present by default, I'm all ears.\n\n> Many of the above are amusing and served as good demonstration to\n> show the blast radius, but it seems that most of them should be\n> marked with PARSE_OPT_NONEG.\n\nHard to say for me -- these are synthetic test cases and I lack context\nto make that decision. In t0040 (t/helper/test-parse-options.c rather)\nwe do have a few PARSE_OPT_NONEG uses already. In t1502 we need to add\nsome...\n\n>\n>> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\n>> index dd811b7fb4..0a67e2dd4f 100755\n>> --- a/t/t1502-rev-parse-parseopt.sh\n>> +++ b/t/t1502-rev-parse-parseopt.sh\n>> @@ -64,33 +64,38 @@ test_expect_success 'test --parseopt help output' '\n>> |\n>> | some-command does foo and bar!\n>> |\n>> -| -h, --help show the help\n>> -| --foo some nifty option --foo\n>> -| --bar ... some cool option --bar with an argument\n>> -| -b, --baz a short and long option\n>> +| -h, --[no-]help show the help\n>\n> Indeed it is amusing, but we probably should give PARSE_OPT_NONEG\n> appropriately, instead of changing the expectations, for many of the\n> changes we see here, I think.\n\n... and --help is the one obvious choice for me, because --no-help is\nnot supported, of course. But we can use some more dedicated tests of\nnegation and double-negation.\n\nRené\n"},{"id":"479804","messageId":"4d01e971-07cb-4f11-3cc6-9d9f21e590c1@web.de","threadId":"59999","inReplyTo":"xmqq4jlxuiuu.fsf@gitster.g","subject":"[PATCH v2 0/5] show negatability of options in short help","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-24T12:29:47Z","receivedAt":"2023-07-24T12:30:26Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Changes since v1: More fallout, still reduce size of final patch:\n- Disable negation of \"git subtree\" options that don't support it.\n- Adjust t7900 in contrib/subtree/t to the changed output.\n- Adjust git-rev-parse.txt to the changed output.\n- Disable negation of --help in t1502.\n- Add negation tests to t1502.\n- Deduplicate expected output in t1502.\n\n subtree: disallow --no-{help,quiet,debug,branch,message}\n t1502, docs: disallow --no-help\n t1502: move optionspec help output to a file\n t1502: test option negation\n parse-options: show negatability of options in short help\n\n Documentation/git-rev-parse.txt | 10 +--\n contrib/subtree/git-subtree.sh | 10 +--\n contrib/subtree/t/t7900-subtree.sh | 2 +-\n parse-options.c | 10 ++-\n t/t0040-parse-options.sh | 44 +++++-----\n t/t1502-rev-parse-parseopt.sh | 131 ++++++++++++-----------------\n t/t1502/.gitattributes | 1 +\n t/t1502/optionspec-neg | 8 ++\n t/t1502/optionspec-neg.help | 11 +++\n t/t1502/optionspec.help | 39 +++++++++\n 10 files changed, 157 insertions(+), 109 deletions(-)\n create mode 100644 t/t1502/.gitattributes\n create mode 100644 t/t1502/optionspec-neg\n create mode 100644 t/t1502/optionspec-neg.help\n create mode 100755 t/t1502/optionspec.help\n\n--\n2.41.0\n"},{"id":"479805","messageId":"2cfa6142-8acc-eb35-bf69-ede986202435@web.de","threadId":"59999","inReplyTo":"4d01e971-07cb-4f11-3cc6-9d9f21e590c1@web.de","subject":"[PATCH v2 1/5] subtree: disallow --no-{help,quiet,debug,branch,message}","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-24T12:34:38Z","receivedAt":"2023-07-24T12:34:47Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"\"git subtree\" only handles the negated variant of the options annotate,\nprefix, onto, rejoin, ignore-joins and squash explicitly. help is\nhandled by \"git rev-parse --parseopt\" implicitly, but not its negated\nform. Disable negation for it and the for the rest of the options to\nget a helpful error message when trying them.\n\nSigned-off-by: René Scharfe \n---\n contrib/subtree/git-subtree.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 7db4c45676..e0c5d3b0de 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -33,19 +33,19 @@ git subtree split --prefix= []\n git subtree pull --prefix= \n git subtree push --prefix= \n --\n-h,help show the help\n-q,quiet quiet\n-d,debug show debug messages\n+h,help! show the help\n+q,quiet! quiet\n+d,debug! show debug messages\n P,prefix= the name of the subdir to split out\n options for 'split' (also: 'push')\n annotate= add a prefix to commit message of new commits\n-b,branch= create a new branch from the split subtree\n+b,branch!= create a new branch from the split subtree\n ignore-joins ignore prior --rejoin commits\n onto= try connecting new tree to an existing one\n rejoin merge the new branch back into HEAD\n options for 'add' and 'merge' (also: 'pull', 'split --rejoin', and 'push --rejoin')\n squash merge subtree changes as a single commit\n-m,message= use the given message as the commit message for the merge commit\n+m,message!= use the given message as the commit message for the merge commit\n \"\n\n indent=0\n--\n2.41.0\n"},{"id":"479806","messageId":"472f65b4-76a1-619f-269e-e8edf8f60463@web.de","threadId":"59999","inReplyTo":"4d01e971-07cb-4f11-3cc6-9d9f21e590c1@web.de","subject":"[PATCH v2 2/5] t1502, docs: disallow --no-help","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-24T12:36:45Z","receivedAt":"2023-07-24T12:36:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"\"git rev-parse --parseopt\" handles the built-in options -h and --help,\nbut not --no-help. Make test definitions and documentation examples\nmore realistic by disabling negation using the flag \"!\".\n\nSigned-off-by: René Scharfe \n---\n Documentation/git-rev-parse.txt | 2 +-\n t/t1502-rev-parse-parseopt.sh | 6 +++---\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt\nindex f26a7591e3..6e8ff9ace1 100644\n--- a/Documentation/git-rev-parse.txt\n+++ b/Documentation/git-rev-parse.txt\n@@ -398,7 +398,7 @@ some-command [] ...\n\n some-command does foo and bar!\n --\n-h,help show the help\n+h,help! show the help\n\n foo some nifty option --foo\n bar= some cool option --bar with an argument\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex dd811b7fb4..0cdc6eb8b3 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -9,7 +9,7 @@ test_expect_success 'setup optionspec' '\n |\n |some-command does foo and bar!\n |--\n-|h,help show the help\n+|h,help! show the help\n |\n |foo some nifty option --foo\n |bar= some cool option --bar with an argument\n@@ -288,7 +288,7 @@ test_expect_success 'test --parseopt help output: \"wrapped\" options normal \"or:\"\n \t| [--another-option]\n \t|cmd [--yet-another-option]\n \t|--\n-\t|h,help show the help\n+\t|h,help! show the help\n \tEOF\n\n \tsed -e \"s/^|//\" >expect <<-\\END_EXPECT &&\n@@ -322,7 +322,7 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l\n \t|line\n \t|blurb\n \t|--\n-\t|h,help show the help\n+\t|h,help! show the help\n \tEOF\n\n \tsed -e \"s/^|//\" >expect <<-\\END_EXPECT &&\n--\n2.41.0\n"},{"id":"479807","messageId":"c4840046-118a-cadb-a0b2-3fd7081778a6@web.de","threadId":"59999","inReplyTo":"4d01e971-07cb-4f11-3cc6-9d9f21e590c1@web.de","subject":"[PATCH v2 3/5] t1502: move optionspec help output to a file","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-24T12:38:38Z","receivedAt":"2023-07-24T12:38:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"\"git rev-parse --parseopt\" shows the short help with its description of\nall recognized options twice: When called with -h or --help, and after\nreporting an unknown option. Move the one for optionspec into a file\nand use it in two tests to deduplicate that part.\n\n\"git rev-parse --parseopt -- --h\" wraps the help text in \"cat <<\\EOF\"\nand \"EOF\". Keep that part in the file to use it as is in the test that\nneeds it and simply remove it in the other one using sed.\n\nDisable whitespace checking for the file using an attribute, as we need\nto keep its spaces intact and wouldn't want a stray --whitespace=fix to\nturn them into tabs.\n\nSigned-off-by: René Scharfe \n---\n t/t1502-rev-parse-parseopt.sh | 79 ++++-------------------------------\n t/t1502/.gitattributes | 1 +\n t/t1502/optionspec.help | 34 +++++++++++++++\n 3 files changed, 42 insertions(+), 72 deletions(-)\n create mode 100644 t/t1502/.gitattributes\n create mode 100755 t/t1502/optionspec.help\n\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex 0cdc6eb8b3..813ee5872f 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -58,44 +58,8 @@ EOF\n '\n\n test_expect_success 'test --parseopt help output' '\n-\tsed -e \"s/^|//\" >expect <<\\END_EXPECT &&\n-|cat <<\\EOF\n-|usage: some-command [options] ...\n-|\n-| some-command does foo and bar!\n-|\n-| -h, --help show the help\n-| --foo some nifty option --foo\n-| --bar ... some cool option --bar with an argument\n-| -b, --baz a short and long option\n-|\n-|An option group Header\n-| -C[...] option C with an optional argument\n-| -d, --data[=...] short and long option with an optional argument\n-|\n-|Argument hints\n-| -B short option required argument\n-| --bar2 long option required argument\n-| -e, --fuz \n-| short and long option required argument\n-| -s[] short option optional argument\n-| --long[=] long option optional argument\n-| -g, --fluf[=] short and long option optional argument\n-| --longest \n-| a very long argument hint\n-| --pair with an equals sign in the hint\n-| --aswitch help te=t contains? fl*g characters!`\n-| --bswitch hint has trailing tab character\n-| --cswitch switch has trailing tab character\n-| --short-hint with a one symbol hint\n-|\n-|Extras\n-| --extra1 line above used to cause a segfault but no longer does\n-|\n-|EOF\n-END_EXPECT\n \ttest_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec &&\n-\ttest_cmp expect output\n+\ttest_cmp \"$TEST_DIRECTORY/t1502/optionspec.help\" output\n '\n\n test_expect_success 'test --parseopt help output no switches' '\n@@ -140,41 +104,12 @@ END_EXPECT\n '\n\n test_expect_success 'test --parseopt invalid switch help output' '\n-\tsed -e \"s/^|//\" >expect <<\\END_EXPECT &&\n-|error: unknown option `does-not-exist'\\''\n-|usage: some-command [options] ...\n-|\n-| some-command does foo and bar!\n-|\n-| -h, --help show the help\n-| --foo some nifty option --foo\n-| --bar ... some cool option --bar with an argument\n-| -b, --baz a short and long option\n-|\n-|An option group Header\n-| -C[...] option C with an optional argument\n-| -d, --data[=...] short and long option with an optional argument\n-|\n-|Argument hints\n-| -B short option required argument\n-| --bar2 long option required argument\n-| -e, --fuz \n-| short and long option required argument\n-| -s[] short option optional argument\n-| --long[=] long option optional argument\n-| -g, --fluf[=] short and long option optional argument\n-| --longest \n-| a very long argument hint\n-| --pair with an equals sign in the hint\n-| --aswitch help te=t contains? fl*g characters!`\n-| --bswitch hint has trailing tab character\n-| --cswitch switch has trailing tab character\n-| --short-hint with a one symbol hint\n-|\n-|Extras\n-| --extra1 line above used to cause a segfault but no longer does\n-|\n-END_EXPECT\n+\t{\n+\t\tcat <<-\\EOF &&\n+\t\terror: unknown option `does-not-exist'\\''\n+\t\tEOF\n+\t\tsed -e 1d -e \\$d <\"$TEST_DIRECTORY/t1502/optionspec.help\"\n+\t} >expect &&\n \ttest_expect_code 129 git rev-parse --parseopt -- --does-not-exist 1>/dev/null 2>output < optionspec &&\n \ttest_cmp expect output\n '\ndiff --git a/t/t1502/.gitattributes b/t/t1502/.gitattributes\nnew file mode 100644\nindex 0000000000..562b12e16e\n--- /dev/null\n+++ b/t/t1502/.gitattributes\n@@ -0,0 +1 @@\n+* -whitespace\ndiff --git a/t/t1502/optionspec.help b/t/t1502/optionspec.help\nnew file mode 100755\nindex 0000000000..844eac6704\n--- /dev/null\n+++ b/t/t1502/optionspec.help\n@@ -0,0 +1,34 @@\n+cat <<\\EOF\n+usage: some-command [options] ...\n+\n+ some-command does foo and bar!\n+\n+ -h, --help show the help\n+ --foo some nifty option --foo\n+ --bar ... some cool option --bar with an argument\n+ -b, --baz a short and long option\n+\n+An option group Header\n+ -C[...] option C with an optional argument\n+ -d, --data[=...] short and long option with an optional argument\n+\n+Argument hints\n+ -B short option required argument\n+ --bar2 long option required argument\n+ -e, --fuz \n+ short and long option required argument\n+ -s[] short option optional argument\n+ --long[=] long option optional argument\n+ -g, --fluf[=] short and long option optional argument\n+ --longest \n+ a very long argument hint\n+ --pair with an equals sign in the hint\n+ --aswitch help te=t contains? fl*g characters!`\n+ --bswitch hint has trailing tab character\n+ --cswitch switch has trailing tab character\n+ --short-hint with a one symbol hint\n+\n+Extras\n+ --extra1 line above used to cause a segfault but no longer does\n+\n+EOF\n--\n2.41.0\n"},{"id":"479808","messageId":"831cfa35-8c83-cd24-4348-c9c3769643c7@web.de","threadId":"59999","inReplyTo":"4d01e971-07cb-4f11-3cc6-9d9f21e590c1@web.de","subject":"[PATCH v2 4/5] t1502: test option negation","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-24T12:39:41Z","receivedAt":"2023-07-24T12:39:48Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add tests for checking the \"git rev-parse --parseopt\" flag \"!\" and\nwhether options can be negated with a \"no-\" prefix.\n\nSigned-off-by: René Scharfe \n---\n t/t1502-rev-parse-parseopt.sh | 44 +++++++++++++++++++++++++++++++++++\n t/t1502/optionspec-neg | 8 +++++++\n t/t1502/optionspec-neg.help | 11 +++++++++\n 3 files changed, 63 insertions(+)\n create mode 100644 t/t1502/optionspec-neg\n create mode 100644 t/t1502/optionspec-neg.help\n\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex 813ee5872f..0f7c2db4c0 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -3,6 +3,22 @@\n test_description='test git rev-parse --parseopt'\n . ./test-lib.sh\n\n+check_invalid_long_option () {\n+\tspec=\"$1\"\n+\topt=\"$2\"\n+\ttest_expect_success \"test --parseopt invalid switch $opt help output for $spec\" '\n+\t\t{\n+\t\t\tcat <<-\\EOF &&\n+\t\t\terror: unknown option `'${opt#--}\\''\n+\t\t\tEOF\n+\t\t\tsed -e 1d -e \\$d <\"$TEST_DIRECTORY/t1502/$spec.help\"\n+\t\t} >expect &&\n+\t\ttest_expect_code 129 git rev-parse --parseopt -- $opt \\\n+\t\t\t2>output <\"$TEST_DIRECTORY/t1502/$spec\" &&\n+\t\ttest_cmp expect output\n+\t'\n+}\n+\n test_expect_success 'setup optionspec' '\n \tsed -e \"s/^|//\" >optionspec <<\\EOF\n |some-command [options] ...\n@@ -278,4 +294,32 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l\n \ttest_cmp expect actual\n '\n\n+test_expect_success 'test --parseopt help output for optionspec-neg' '\n+\ttest_expect_code 129 git rev-parse --parseopt -- \\\n+\t\t-h >output <\"$TEST_DIRECTORY/t1502/optionspec-neg\" &&\n+\ttest_cmp \"$TEST_DIRECTORY/t1502/optionspec-neg.help\" output\n+'\n+\n+test_expect_success 'test --parseopt valid options for optionspec-neg' '\n+\tcat >expect <<-\\EOF &&\n+\tset -- --foo --no-foo --no-bar --positive-only --no-negative --\n+\tEOF\n+\tgit rev-parse --parseopt -- <\"$TEST_DIRECTORY/t1502/optionspec-neg\" >output \\\n+\t --foo --no-foo --no-bar --positive-only --no-negative &&\n+\ttest_cmp expect output\n+'\n+\n+test_expect_success 'test --parseopt positivated option for optionspec-neg' '\n+\tcat >expect <<-\\EOF &&\n+\tset -- --no-no-bar --no-no-bar --\n+\tEOF\n+\tgit rev-parse --parseopt -- <\"$TEST_DIRECTORY/t1502/optionspec-neg\" >output \\\n+\t --no-no-bar --bar &&\n+\ttest_cmp expect output\n+'\n+\n+check_invalid_long_option optionspec-neg --no-positive-only\n+check_invalid_long_option optionspec-neg --negative\n+check_invalid_long_option optionspec-neg --no-no-negative\n+\n test_done\ndiff --git a/t/t1502/optionspec-neg b/t/t1502/optionspec-neg\nnew file mode 100644\nindex 0000000000..28992ee303\n--- /dev/null\n+++ b/t/t1502/optionspec-neg\n@@ -0,0 +1,8 @@\n+some-command [options] ...\n+\n+some-command does foo and bar!\n+--\n+foo\t\tcan be negated\n+no-bar\t\tcan be positivated\n+positive-only!\tcannot be negated\n+no-negative!\tcannot be positivated\ndiff --git a/t/t1502/optionspec-neg.help b/t/t1502/optionspec-neg.help\nnew file mode 100644\nindex 0000000000..591f4dcd9f\n--- /dev/null\n+++ b/t/t1502/optionspec-neg.help\n@@ -0,0 +1,11 @@\n+cat <<\\EOF\n+usage: some-command [options] ...\n+\n+ some-command does foo and bar!\n+\n+ --foo can be negated\n+ --no-bar can be positivated\n+ --positive-only cannot be negated\n+ --no-negative cannot be positivated\n+\n+EOF\n--\n2.41.0\n"},{"id":"479809","messageId":"eac754dd-0821-cfcb-7f6e-a00a76fde5b2@web.de","threadId":"59999","inReplyTo":"4d01e971-07cb-4f11-3cc6-9d9f21e590c1@web.de","subject":"[PATCH v2 5/5] parse-options: show negatability of options in short help","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-24T12:40:41Z","receivedAt":"2023-07-24T12:40:53Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add a \"[no-]\" prefix to options without the flag PARSE_OPT_NONEG to\ndocument the fact that you can negate them.\n\nThis looks a bit strange for options that already start with \"no-\", e.g.\nfor the option --no-name of git show-branch:\n\n --[no-]no-name suppress naming strings\n\nYou can actually use --no-no-name as an alias of --name, so the short\nhelp is not wrong. If we strip off any of the \"no-\"s, we lose either\nthe ability to see if the remaining one belongs to the documented\nvariant or to see if it can be negated.\n\nSigned-off-by: René Scharfe \n---\n Documentation/git-rev-parse.txt | 8 +++---\n contrib/subtree/t/t7900-subtree.sh | 2 +-\n parse-options.c | 10 +++++--\n t/t0040-parse-options.sh | 44 ++++++++++++++++--------------\n t/t1502-rev-parse-parseopt.sh | 2 +-\n t/t1502/optionspec-neg.help | 4 +--\n t/t1502/optionspec.help | 35 ++++++++++++++----------\n 7 files changed, 60 insertions(+), 45 deletions(-)\n\ndiff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt\nindex 6e8ff9ace1..6a4968f68a 100644\n--- a/Documentation/git-rev-parse.txt\n+++ b/Documentation/git-rev-parse.txt\n@@ -424,10 +424,10 @@ usage: some-command [] ...\n some-command does foo and bar!\n\n -h, --help show the help\n- --foo some nifty option --foo\n- --bar ... some cool option --bar with an argument\n- --baz another cool option --baz with a named argument\n- --qux[=] qux may take a path argument but has meaning by itself\n+ --[no-]foo some nifty option --foo\n+ --[no-]bar ... some cool option --bar with an argument\n+ --[no-]baz another cool option --baz with a named argument\n+ --[no-]qux[=] qux may take a path argument but has meaning by itself\n\n An option group Header\n -C[...] option C with an optional argument\ndiff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\nindex 341c169eca..49a21dd7c9 100755\n--- a/contrib/subtree/t/t7900-subtree.sh\n+++ b/contrib/subtree/t/t7900-subtree.sh\n@@ -71,7 +71,7 @@ test_expect_success 'shows short help text for -h' '\n \ttest_expect_code 129 git subtree -h >out 2>err &&\n \ttest_must_be_empty err &&\n \tgrep -e \"^ *or: git subtree pull\" out &&\n-\tgrep -e --annotate out\n+\tgrep -F -e \"--[no-]annotate\" out\n '\n\n #\ndiff --git a/parse-options.c b/parse-options.c\nindex f8a155ee13..6323ca191d 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -1136,8 +1136,14 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n \t\t}\n \t\tif (opts->long_name && opts->short_name)\n \t\t\tpos += fprintf(outfile, \", \");\n-\t\tif (opts->long_name)\n-\t\t\tpos += fprintf(outfile, \"--%s\", opts->long_name);\n+\t\tif (opts->long_name) {\n+\t\t\tconst char *long_name = opts->long_name;\n+\t\t\tif (opts->flags & PARSE_OPT_NONEG)\n+\t\t\t\tpos += fprintf(outfile, \"--%s\", long_name);\n+\t\t\telse\n+\t\t\t\tpos += fprintf(outfile, \"--[no-]%s\", long_name);\n+\t\t}\n+\n \t\tif (opts->type == OPTION_NUMBER)\n \t\t\tpos += utf8_fprintf(outfile, _(\"-NUM\"));\n\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 7d7ecfd571..f39758d2ef 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -13,29 +13,32 @@ usage: test-tool parse-options \n\n A helper function for the parse-options API.\n\n- --yes get a boolean\n- -D, --no-doubt begins with 'no-'\n+ --[no-]yes get a boolean\n+ -D, --[no-]no-doubt begins with 'no-'\n -B, --no-fear be brave\n- -b, --boolean increment by one\n- -4, --or4 bitwise-or boolean with ...0100\n- --neg-or4 same as --no-or4\n+ -b, --[no-]boolean increment by one\n+ -4, --[no-]or4 bitwise-or boolean with ...0100\n+ --[no-]neg-or4 same as --no-or4\n\n- -i, --integer get a integer\n+ -i, --[no-]integer \n+ get a integer\n -j get a integer, too\n -m, --magnitude get a magnitude\n- --set23 set integer to 23\n+ --[no-]set23 set integer to 23\n --mode1 set integer to 1 (cmdmode option)\n --mode2 set integer to 2 (cmdmode option)\n- -L, --length get length of \n- -F, --file set file to \n+ -L, --[no-]length \n+ get length of \n+ -F, --[no-]file \n+ set file to \n\n String options\n- -s, --string \n+ -s, --[no-]string \n get a string\n- --string2 get another string\n- --st get another string (pervert ordering)\n+ --[no-]string2 get another string\n+ --[no-]st get another string (pervert ordering)\n -o get another string\n- --list add str to list\n+ --[no-]list add str to list\n\n Magic arguments\n -NUM set integer to NUM\n@@ -44,16 +47,17 @@ Magic arguments\n --no-ambiguous negative ambiguity\n\n Standard options\n- --abbrev[=] use digits to display object names\n- -v, --verbose be verbose\n- -n, --dry-run dry run\n- -q, --quiet be quiet\n- --expect expected output in the variable dump\n+ --[no-]abbrev[=] use digits to display object names\n+ -v, --[no-]verbose be verbose\n+ -n, --[no-]dry-run dry run\n+ -q, --[no-]quiet be quiet\n+ --[no-]expect \n+ expected output in the variable dump\n\n Alias\n- -A, --alias-source \n+ -A, --[no-]alias-source \n get a string\n- -Z, --alias-target \n+ -Z, --[no-]alias-target \n alias of --alias-source\n\n EOF\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex 0f7c2db4c0..f0737593c3 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -111,7 +111,7 @@ test_expect_success 'test --parseopt help-all output hidden switches' '\n |\n | some-command does foo and bar!\n |\n-| --hidden1 A hidden switch\n+| --[no-]hidden1 A hidden switch\n |\n |EOF\n END_EXPECT\ndiff --git a/t/t1502/optionspec-neg.help b/t/t1502/optionspec-neg.help\nindex 591f4dcd9f..156e5f0ed9 100644\n--- a/t/t1502/optionspec-neg.help\n+++ b/t/t1502/optionspec-neg.help\n@@ -3,8 +3,8 @@ usage: some-command [options] ...\n\n some-command does foo and bar!\n\n- --foo can be negated\n- --no-bar can be positivated\n+ --[no-]foo can be negated\n+ --[no-]no-bar can be positivated\n --positive-only cannot be negated\n --no-negative cannot be positivated\n\ndiff --git a/t/t1502/optionspec.help b/t/t1502/optionspec.help\nindex 844eac6704..64e8ce9f98 100755\n--- a/t/t1502/optionspec.help\n+++ b/t/t1502/optionspec.help\n@@ -4,31 +4,36 @@ usage: some-command [options] ...\n some-command does foo and bar!\n\n -h, --help show the help\n- --foo some nifty option --foo\n- --bar ... some cool option --bar with an argument\n- -b, --baz a short and long option\n+ --[no-]foo some nifty option --foo\n+ --[no-]bar ... some cool option --bar with an argument\n+ -b, --[no-]baz a short and long option\n\n An option group Header\n -C[...] option C with an optional argument\n- -d, --data[=...] short and long option with an optional argument\n+ -d, --[no-]data[=...]\n+ short and long option with an optional argument\n\n Argument hints\n -B short option required argument\n- --bar2 long option required argument\n- -e, --fuz \n+ --[no-]bar2 long option required argument\n+ -e, --[no-]fuz \n short and long option required argument\n -s[] short option optional argument\n- --long[=] long option optional argument\n- -g, --fluf[=] short and long option optional argument\n- --longest \n+ --[no-]long[=] long option optional argument\n+ -g, --[no-]fluf[=]\n+ short and long option optional argument\n+ --[no-]longest \n a very long argument hint\n- --pair with an equals sign in the hint\n- --aswitch help te=t contains? fl*g characters!`\n- --bswitch hint has trailing tab character\n- --cswitch switch has trailing tab character\n- --short-hint with a one symbol hint\n+ --[no-]pair \n+ with an equals sign in the hint\n+ --[no-]aswitch help te=t contains? fl*g characters!`\n+ --[no-]bswitch \n+ hint has trailing tab character\n+ --[no-]cswitch switch has trailing tab character\n+ --[no-]short-hint \n+ with a one symbol hint\n\n Extras\n- --extra1 line above used to cause a segfault but no longer does\n+ --[no-]extra1 line above used to cause a segfault but no longer does\n\n EOF\n--\n2.41.0\n"},{"id":"479826","messageId":"xmqqr0oxnnx4.fsf@gitster.g","threadId":"59999","inReplyTo":"1535f30e-3cf9-1a0a-04af-4ba4a7c46d15@web.de","subject":"Re: [PATCH] ls-tree: fix --no-full-name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-24T18:51:19Z","receivedAt":"2023-07-24T18:51:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Am 21.07.23 um 22:09 schrieb Junio C Hamano:\n>> René Scharfe writes:\n>>\n>>> - -D, --no-doubt begins with 'no-'\n>>> + -D, --[no-]no-doubt begins with 'no-'\n>>\n>> Hmph, I really really loved the neat trick to allow \"no-doubt\"\n>> option to be \"positivised\" by _dropping_ the leading \"no-\" at around\n>> 0f1930c5 (parse-options: allow positivation of options starting,\n>> with no-, 2012-02-25).\n>\n> Yeah, if there is a better way to document A) that the \"no-\" is optional\n> and B) whether it's present by default, I'm all ears.\n\nSome options take \"no-\" prefix while some others do not, so\nindicating that \"this can take negative forms\" vs \"this do not take\nnegative forms\" by \"--[no-]xyzzy\" and \"--frotz\" makes sense.\n\nYikes. There are tons of options whose names begin with \"no-\" and\nmarked PARSE_OPT_NONEG, so \"an option '--no-nitfol' that does not\nhave the 'no-' part in [brackets] can drop 'no-' to make it\npositive\" would not fly as a rule/convention.\n\nIf we do not mind getting longer, we could say\n\n\t-D, --no-doubt, --doubt\n\nand explain in the description that --no-doubt is the same as -D and\n--doubt is the default. It is making the developers responsible for\nclarify, which is not very satisfying.\n\nWe may not reject \"--no-no-doubt\" but with the positivization\nsupport, double negation is not something we'd encourage without\nfeeling embarrassed.\n\n> Hard to say for me -- these are synthetic test cases and I lack context\n> to make that decision. In t0040 (t/helper/test-parse-options.c rather)\n> we do have a few PARSE_OPT_NONEG uses already. In t1502 we need to add\n> some...\n\nTrue. The test coverage will be hurt if we start futzing with\nOPT_NONEG bit \"randomly\".\n\n>>> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\n>>> index dd811b7fb4..0a67e2dd4f 100755\n>>> --- a/t/t1502-rev-parse-parseopt.sh\n>>> +++ b/t/t1502-rev-parse-parseopt.sh\n>>> @@ -64,33 +64,38 @@ test_expect_success 'test --parseopt help output' '\n>>> |\n>>> | some-command does foo and bar!\n>>> |\n>>> -| -h, --help show the help\n>>> -| --foo some nifty option --foo\n>>> -| --bar ... some cool option --bar with an argument\n>>> -| -b, --baz a short and long option\n>>> +| -h, --[no-]help show the help\n>>\n>> Indeed it is amusing, but we probably should give PARSE_OPT_NONEG\n>> appropriately, instead of changing the expectations, for many of the\n>> changes we see here, I think.\n>\n> ... and --help is the one obvious choice for me, because --no-help is\n> not supported, of course. But we can use some more dedicated tests of\n> negation and double-negation.\n\nYeah.\n"},{"id":"479829","messageId":"9e8225dd-1e8b-8af2-c3e1-0c5834694244@web.de","threadId":"59999","inReplyTo":"xmqqr0oxnnx4.fsf@gitster.g","subject":"Re: [PATCH] ls-tree: fix --no-full-name","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-24T20:09:36Z","receivedAt":"2023-07-24T20:09:47Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 24.07.23 um 20:51 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> Am 21.07.23 um 22:09 schrieb Junio C Hamano:\n>>> René Scharfe writes:\n>>>\n>>>> - -D, --no-doubt begins with 'no-'\n>>>> + -D, --[no-]no-doubt begins with 'no-'\n>>>\n>>> Hmph, I really really loved the neat trick to allow \"no-doubt\"\n>>> option to be \"positivised\" by _dropping_ the leading \"no-\" at around\n>>> 0f1930c5 (parse-options: allow positivation of options starting,\n>>> with no-, 2012-02-25).\n>>\n>> Yeah, if there is a better way to document A) that the \"no-\" is optional\n>> and B) whether it's present by default, I'm all ears.\n>\n> Some options take \"no-\" prefix while some others do not, so\n> indicating that \"this can take negative forms\" vs \"this do not take\n> negative forms\" by \"--[no-]xyzzy\" and \"--frotz\" makes sense.\n>\n> Yikes. There are tons of options whose names begin with \"no-\" and\n> marked PARSE_OPT_NONEG, so \"an option '--no-nitfol' that does not\n> have the 'no-' part in [brackets] can drop 'no-' to make it\n> positive\" would not fly as a rule/convention.\n>\n> If we do not mind getting longer, we could say\n>\n> \t-D, --no-doubt, --doubt\n>\n> and explain in the description that --no-doubt is the same as -D and\n> --doubt is the default. It is making the developers responsible for\n> clarify, which is not very satisfying.\n\nAdjusting all explanations manually seems quite tedious.\n\n> We may not reject \"--no-no-doubt\" but with the positivization\n> support, double negation is not something we'd encourage without\n> feeling embarrassed.\n\nRight. Perhaps --[[no-]no-]doubt? Looks a bit silly with its nested\nbrackets, but it's more correct, because it documents all three accepted\nforms, including the no-less one.\n\nRené\n"},{"id":"479831","messageId":"xmqqh6ptnies.fsf@gitster.g","threadId":"59999","inReplyTo":"9e8225dd-1e8b-8af2-c3e1-0c5834694244@web.de","subject":"Re: [PATCH] ls-tree: fix --no-full-name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-24T20:50:19Z","receivedAt":"2023-07-24T20:50:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Am 24.07.23 um 20:51 schrieb Junio C Hamano:\n>> René Scharfe writes:\n>>\n>>> Am 21.07.23 um 22:09 schrieb Junio C Hamano:\n>>>> René Scharfe writes:\n>>>>\n>>>>> - -D, --no-doubt begins with 'no-'\n>>>>> + -D, --[no-]no-doubt begins with 'no-'\n>>>>\n>>>> Hmph, I really really loved the neat trick to allow \"no-doubt\"\n>>>> option to be \"positivised\" by _dropping_ the leading \"no-\" at around\n>>>> 0f1930c5 (parse-options: allow positivation of options starting,\n>>>> with no-, 2012-02-25).\n>>>\n>>> Yeah, if there is a better way to document A) that the \"no-\" is optional\n>>> and B) whether it's present by default, I'm all ears.\n>>\n>> Some options take \"no-\" prefix while some others do not, so\n>> indicating that \"this can take negative forms\" vs \"this do not take\n>> negative forms\" by \"--[no-]xyzzy\" and \"--frotz\" makes sense.\n>>\n>> Yikes. There are tons of options whose names begin with \"no-\" and\n>> marked PARSE_OPT_NONEG, so \"an option '--no-nitfol' that does not\n>> have the 'no-' part in [brackets] can drop 'no-' to make it\n>> positive\" would not fly as a rule/convention.\n>>\n>> If we do not mind getting longer, we could say\n>>\n>> \t-D, --no-doubt, --doubt\n>>\n>> and explain in the description that --no-doubt is the same as -D and\n>> --doubt is the default. It is making the developers responsible for\n>> clarify, which is not very satisfying.\n>\n> Adjusting all explanations manually seems quite tedious.\n>\n>> We may not reject \"--no-no-doubt\" but with the positivization\n>> support, double negation is not something we'd encourage without\n>> feeling embarrassed.\n>\n> Right. Perhaps --[[no-]no-]doubt? Looks a bit silly with its nested\n> brackets, but it's more correct, because it documents all three accepted\n> forms, including the no-less one.\n\nIt may look a bit silly but looks very tempting. Also it is not\nmuch longer than \"--[no-]no-doubt\".\n"},{"id":"479960","messageId":"a19879db-d45a-ee42-1ad5-497e4e9eb8df@web.de","threadId":"59999","inReplyTo":"xmqqh6ptnies.fsf@gitster.g","subject":"Re: [PATCH] ls-tree: fix --no-full-name","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-28T06:12:39Z","receivedAt":"2023-07-28T06:12:53Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 24.07.23 um 22:50 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> Am 24.07.23 um 20:51 schrieb Junio C Hamano:\n>>> René Scharfe writes:\n>>>\n>>>> Am 21.07.23 um 22:09 schrieb Junio C Hamano:\n>>>>> René Scharfe writes:\n>>>>>\n>>>>>> - -D, --no-doubt begins with 'no-'\n>>>>>> + -D, --[no-]no-doubt begins with 'no-'\n>>>>>\n>>>>> Hmph, I really really loved the neat trick to allow \"no-doubt\"\n>>>>> option to be \"positivised\" by _dropping_ the leading \"no-\" at around\n>>>>> 0f1930c5 (parse-options: allow positivation of options starting,\n>>>>> with no-, 2012-02-25).\n>>>>\n>>>> Yeah, if there is a better way to document A) that the \"no-\" is optional\n>>>> and B) whether it's present by default, I'm all ears.\n>>>\n>>> Some options take \"no-\" prefix while some others do not, so\n>>> indicating that \"this can take negative forms\" vs \"this do not take\n>>> negative forms\" by \"--[no-]xyzzy\" and \"--frotz\" makes sense.\n>>>\n>>> Yikes. There are tons of options whose names begin with \"no-\" and\n>>> marked PARSE_OPT_NONEG, so \"an option '--no-nitfol' that does not\n>>> have the 'no-' part in [brackets] can drop 'no-' to make it\n>>> positive\" would not fly as a rule/convention.\n>>>\n>>> If we do not mind getting longer, we could say\n>>>\n>>> \t-D, --no-doubt, --doubt\n>>>\n>>> and explain in the description that --no-doubt is the same as -D and\n>>> --doubt is the default. It is making the developers responsible for\n>>> clarify, which is not very satisfying.\n>>\n>> Adjusting all explanations manually seems quite tedious.\n>>\n>>> We may not reject \"--no-no-doubt\" but with the positivization\n>>> support, double negation is not something we'd encourage without\n>>> feeling embarrassed.\n>>\n>> Right. Perhaps --[[no-]no-]doubt? Looks a bit silly with its nested\n>> brackets, but it's more correct, because it documents all three accepted\n>> forms, including the no-less one.\n>\n> It may look a bit silly but looks very tempting. Also it is not\n> much longer than \"--[no-]no-doubt\".\n\nYes, it's quite compact. But is it they still legible?\n\n --no-index find in contents not managed by git\n --[no-]no-index find in contents not managed by git\n --[[no-]no-]index find in contents not managed by git\n --[no-[no-]]index find in contents not managed by git\n\nThe last two document all three variants, but is it still obvious that\nthe help text is supposed to be about the one with a single \"no-\"?\nThat's something that has to be learned, I suspect. No good making the\nshort help too cryptic. Hmm, how about:\n\n --no-index, --[no-[no-]]index\n find in contents not managed by git\n\nSomewhat redundant, but highlights the documented variant.\n\nRené\n"},{"id":"479962","messageId":"b17b4f6b-199f-973b-3aa9-ef995a61bb3d@gmail.com","threadId":"59999","inReplyTo":"a19879db-d45a-ee42-1ad5-497e4e9eb8df@web.de","subject":"Re: [PATCH] ls-tree: fix --no-full-name","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-07-28T09:45:12Z","receivedAt":"2023-07-28T09:45:18Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 28/07/2023 07:12, René Scharfe wrote:\n>>> Right. Perhaps --[[no-]no-]doubt? Looks a bit silly with its nested\n>>> brackets, but it's more correct, because it documents all three accepted\n>>> forms, including the no-less one.\n>>\n>> It may look a bit silly but looks very tempting. Also it is not\n>> much longer than \"--[no-]no-doubt\".\n> \n> Yes, it's quite compact. But is it they still legible?\n> \n> --no-index find in contents not managed by git\n> --[no-]no-index find in contents not managed by git\n> --[[no-]no-]index find in contents not managed by git\n> --[no-[no-]]index find in contents not managed by git\n> \n> The last two document all three variants, but is it still obvious that\n> the help text is supposed to be about the one with a single \"no-\"?\n> That's something that has to be learned, I suspect. No good making the\n> short help too cryptic. Hmm, how about:\n> \n> --no-index, --[no-[no-]]index\n> find in contents not managed by git\n\nI think spelling out the positive and negative options separately makes \nit much clearer, but in that case I think we'd be better just to show\n\n\t--no-index, --index\n\nadding \"[no-[no]]\" is just going to confuse users. If we had a \nconvention of \"[, ]; \" then I think \nit should be clear to users how to negate a given option\n\n -v, --invert-match; --no-invert-match\n\t\t\t\tshow non-matching lines\n -I, --no-index; --index\tfind in contents not managed by git\n\nSpelling out both versions is a bit verbose but I think it is worth it \nto avoid \"--[no-]no-index\"\n\nOne other thought is to mark options that can be negated with a symbol \nlike '*' and add a footnote saying those options can be negated.\n\nBest Wishes\n\nPhillip\n"},{"id":"479982","messageId":"31b71333-de8b-d9a8-3ec4-1bad9cae2bf3@web.de","threadId":"59999","inReplyTo":"b17b4f6b-199f-973b-3aa9-ef995a61bb3d@gmail.com","subject":"Re: [PATCH] ls-tree: fix --no-full-name","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-29T20:40:19Z","receivedAt":"2023-07-29T20:40:33Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 28.07.23 um 11:45 schrieb Phillip Wood:\n> On 28/07/2023 07:12, René Scharfe wrote:\n>>>> Right.  Perhaps --[[no-]no-]doubt?  Looks a bit silly with its nested\n>>>> brackets, but it's more correct, because it documents all three accepted\n>>>> forms, including the no-less one.\n>>>\n>>> It may look a bit silly but looks very tempting.  Also it is not\n>>> much longer than \"--[no-]no-doubt\".\n>>\n>> Yes, it's quite compact.  But is it they still legible?\n>>\n>>      --no-index            find in contents not managed by git\n>>      --[no-]no-index       find in contents not managed by git\n>>      --[[no-]no-]index     find in contents not managed by git\n>>      --[no-[no-]]index     find in contents not managed by git\n>>\n>> The last two document all three variants, but is it still obvious that\n>> the help text is supposed to be about the one with a single \"no-\"?\n>> That's something that has to be learned, I suspect.  No good making the\n>> short help too cryptic.  Hmm, how about:\n>>\n>>      --no-index, --[no-[no-]]index\n>>                            find in contents not managed by git\n>\n> I think spelling out the positive and negative options separately\n> makes it much clearer, but in that case I think we'd be better just\n> to show\n>\n>     --no-index, --index\n>\n> adding \"[no-[no]]\" is just going to confuse users. If we had a\n> convention of \"[, ]; \" then I\n> think it should be clear to users how to negate a given option\n>\n>     -v, --invert-match; --no-invert-match\n>                 show non-matching lines\n>     -I, --no-index; --index    find in contents not managed by git\n>\n> Spelling out both versions is a bit verbose but I think it is worth\n> it to avoid \"--[no-]no-index\"\n\nI kinda like it, even though it is quite verbose and adds a new syntax\nelement.\n\nA bit more verbose still: Document the negative form on its own line\nwith a generated description -- requires no new syntax:\n\n -v, --invert-match show non-matching lines\n --no-invert-match opposite of --invert-match, default\n -I, --no-index find in contents not managed by git\n --index opposite of --no-index, default\n\n> One other thought is to mark options that can be negated with a\n> symbol like '*' and add a footnote saying those options can be\n> negated.\n\nSure, adding a layer of indirection would work.\n\nAll these imperfect solutions make me wish we could get rid of the\nproblem, e.g. by converting all \"no-\" options to their positive variants\nand mentioning that they are the default. Won't fly, though, because\nsome of them have short forms, and we don't follow the convention of\nuppercase short options negating lowercase ones, so we have to document\nthem anyway.\n\nRené\n\n"},{"id":"479997","messageId":"xmqqjzug14jo.fsf@gitster.g","threadId":"59999","inReplyTo":"31b71333-de8b-d9a8-3ec4-1bad9cae2bf3@web.de","subject":"Re: [PATCH] ls-tree: fix --no-full-name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-31T15:31:07Z","receivedAt":"2023-07-31T15:31:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> A bit more verbose still: Document the negative form on its own line\n> with a generated description -- requires no new syntax:\n>\n> -v, --invert-match show non-matching lines\n> --no-invert-match opposite of --invert-match, default\n> -I, --no-index find in contents not managed by git\n> --index opposite of --no-index, default\n\nI would expect _(\"opposite of %s, default\") is acceptable by l10n\nfolks, and assuming it is, the above would be a good approach.\n\n> All these imperfect solutions make me wish we could get rid of the\n> problem, e.g. by converting all \"no-\" options to their positive variants\n> and mentioning that they are the default. Won't fly, though,\n> because ...\n\nTrue.\n\nThanks.\n"},{"id":"480152","messageId":"xmqqbkfm3gne.fsf@gitster.g","threadId":"59999","inReplyTo":"xmqqjzug14jo.fsf@gitster.g","subject":"Re: [PATCH] ls-tree: fix --no-full-name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-04T16:40:21Z","receivedAt":"2023-08-04T16:40:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> René Scharfe writes:\n>\n>> A bit more verbose still: Document the negative form on its own line\n>> with a generated description -- requires no new syntax:\n>>\n>> -v, --invert-match show non-matching lines\n>> --no-invert-match opposite of --invert-match, default\n>> -I, --no-index find in contents not managed by git\n>> --index opposite of --no-index, default\n>\n> I would expect _(\"opposite of %s, default\") is acceptable by l10n\n> folks, and assuming it is, the above would be a good approach.\n\nI was seeing what is likely to be in the -rc1 that will happen in\nnext week, and noticed that this discussion is left unconcluded. I\nam tempted to declare that the latest iteration that shows the\nnegation of \"--no-foo\" as \"--[no-]no-foo\" is \"good enough\" for now,\nand leave the above improvement as one potential future direction.\n\nObjections? Otherwise the 5-patch series will be in 'next'.\n\nThanks.\n"},{"id":"480168","messageId":"a9d51bb4-2897-c4ca-f8b3-85f50902b041@gmail.com","threadId":"59999","inReplyTo":"xmqqbkfm3gne.fsf@gitster.g","subject":"Re: [PATCH] ls-tree: fix --no-full-name","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-08-04T19:48:50Z","receivedAt":"2023-08-04T19:48:57Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 04/08/2023 17:40, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> René Scharfe writes:\n>>\n>>> A bit more verbose still: Document the negative form on its own line\n>>> with a generated description -- requires no new syntax:\n>>>\n>>> -v, --invert-match show non-matching lines\n>>> --no-invert-match opposite of --invert-match, default\n>>> -I, --no-index find in contents not managed by git\n>>> --index opposite of --no-index, default\n>>\n>> I would expect _(\"opposite of %s, default\") is acceptable by l10n\n>> folks, and assuming it is, the above would be a good approach.\n> \n> I was seeing what is likely to be in the -rc1 that will happen in\n> next week, and noticed that this discussion is left unconcluded. I\n> am tempted to declare that the latest iteration that shows the\n> negation of \"--no-foo\" as \"--[no-]no-foo\" is \"good enough\" for now,\n> and leave the above improvement as one potential future direction.\n\nWhile it could certainly be improved in the future I think \n\"--[no-]no-foo\" is probably good enough. I definitely prefer it over \n\"--[[no-]no]-foo\"\n\nBest Wishes\n\nPhillip\n\n> Objections? Otherwise the 5-patch series will be in 'next'.\n> \n> Thanks.\n"},{"id":"480185","messageId":"67e28f23-abd7-01d2-a1a7-18befc86a21e@web.de","threadId":"59999","inReplyTo":"a9d51bb4-2897-c4ca-f8b3-85f50902b041@gmail.com","subject":"Re: [PATCH] ls-tree: fix --no-full-name","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-08-05T10:40:28Z","receivedAt":"2023-08-05T10:40:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 04.08.23 um 21:48 schrieb Phillip Wood:\n> On 04/08/2023 17:40, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>>\n>>> René Scharfe writes:\n>>>\n>>>> A bit more verbose still: Document the negative form on its own line\n>>>> with a generated description -- requires no new syntax:\n>>>>\n>>>>      -v, --invert-match         show non-matching lines\n>>>>      --no-invert-match          opposite of --invert-match, default\n>>>>      -I, --no-index             find in contents not managed by git\n>>>>      --index                    opposite of --no-index, default\n>>>\n>>> I would expect _(\"opposite of %s, default\") is acceptable by l10n\n>>> folks, and assuming it is, the above would be a good approach.\n\nThe \", default\" part is not correct in all cases, though, in particular\nwhen the default depends on the output being a terminal, or arguably\nwhen it's dependent on a config setting. We better drop it from the\ngenerated message.\n\n>> I was seeing what is likely to be in the -rc1 that will happen in\n>> next week, and noticed that this discussion is left unconcluded.  I\n>> am tempted to declare that the latest iteration that shows the\n>> negation of \"--no-foo\" as \"--[no-]no-foo\" is \"good enough\" for now,\n>> and leave the above improvement as one potential future direction.\n>\n> While it could certainly be improved in the future I think\n> \"--[no-]no-foo\" is probably good enough. I definitely prefer it over\n> \"--[[no-]no]-foo\"\n\nGenerating help lines for the opposite variant of all negatable options\nfeels quite spammy. It almost doubles the length of the short help\nbecause we have so many of them. Doing that only for --no-... options\nis much more compatc because most options are positive:\n\n -v, --[no-]invert-match show non-matching lines\n -I, --no-index find in contents not managed by git\n --index opposite of --no-index\n\n>> Objections?  Otherwise the 5-patch series will be in 'next'.\n\nPatch 5 has a trivial merge conflict in t/t0040-parse-options.sh due to\n448abbba63 (short help: allow multi-line opthelp, 2023-07-18), which is\neasily resolved by adding --longhelp, and a silent automatic mismerge of\nt/t1502/optionspec.help due to c512643e67 (short help: allow a gap\nsmaller than USAGE_GAP, 2023-07-18), which requires removing three line\nnewlines.\n\nHow about dropping patch 5 for now? We don't need to rush this pretty\nintrusive change of output. And the first four patches are worthwhile\nin their own right, I think.\n\nRené\n"},{"id":"480187","messageId":"e4d46d97-1cd4-7fea-afd1-7de5023b1b09@web.de","threadId":"59999","inReplyTo":"xmqq4jlxuiuu.fsf@gitster.g","subject":"[PATCH v3 0/8] show negatability of options in short help","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-08-05T14:33:07Z","receivedAt":"2023-08-05T14:33:17Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Changes since v2:\n- Rebase, affects only patch 5.\n- Add patch 7 for documenting negative options on their own line.\n- Add preparatory patch 6.\n- Add bonus patch 8.\n\n subtree: disallow --no-{help,quiet,debug,branch,message}\n t1502, docs: disallow --no-help\n t1502: move optionspec help output to a file\n t1502: test option negation\n parse-options: show negatability of options in short help\n parse-options: factor out usage_indent() and usage_padding()\n parse-options: no --[no-]no-...\n parse-options: simplify usage_padding()\n\n Documentation/git-rev-parse.txt | 10 +--\n contrib/subtree/git-subtree.sh | 10 +--\n contrib/subtree/t/t7900-subtree.sh | 2 +-\n parse-options.c | 70 ++++++++++-----\n t/t0040-parse-options.sh | 44 +++++-----\n t/t1502-rev-parse-parseopt.sh | 131 ++++++++++++-----------------\n t/t1502/.gitattributes | 1 +\n t/t1502/optionspec-neg | 8 ++\n t/t1502/optionspec-neg.help | 12 +++\n t/t1502/optionspec.help | 36 ++++++++\n 10 files changed, 199 insertions(+), 125 deletions(-)\n create mode 100644 t/t1502/.gitattributes\n create mode 100644 t/t1502/optionspec-neg\n create mode 100644 t/t1502/optionspec-neg.help\n create mode 100755 t/t1502/optionspec.help\n\nRange-Diff gegen v2:\n1: 26c03bd70c = 1: ee280b3484 subtree: disallow --no-{help,quiet,debug,branch,message}\n2: ad9e7d1393 = 2: 556e79ce55 t1502, docs: disallow --no-help\n3: 1bb68a4a40 = 3: 274e65ec1c t1502: move optionspec help output to a file\n4: 47ab167d1c = 4: 89b0455305 t1502: test option negation\n5: 961c5dfcf7 ! 5: 7cd3abcff7 parse-options: show negatability of options in short help\n @@ t/t0040-parse-options.sh: usage: test-tool parse-options \n + set file to \n\n String options\n -- -s, --string \n -+ -s, --[no-]string \n - get a string\n +- -s, --string get a string\n - --string2 get another string\n - --st get another string (pervert ordering)\n ++ -s, --[no-]string \n ++ get a string\n + --[no-]string2 get another string\n + --[no-]st get another string (pervert ordering)\n -o get another string\n + --longhelp help text of this entry\n + spans multiple lines\n - --list add str to list\n + --[no-]list add str to list\n\n @@ t/t1502/optionspec.help: usage: some-command [options] ...\n An option group Header\n -C[...] option C with an optional argument\n - -d, --data[=...] short and long option with an optional argument\n -+ -d, --[no-]data[=...]\n -+ short and long option with an optional argument\n ++ -d, --[no-]data[=...] short and long option with an optional argument\n\n Argument hints\n -B short option required argument\n @@ t/t1502/optionspec.help: usage: some-command [options] ...\n + --[no-]pair \n + with an equals sign in the hint\n + --[no-]aswitch help te=t contains? fl*g characters!`\n -+ --[no-]bswitch \n -+ hint has trailing tab character\n ++ --[no-]bswitch hint has trailing tab character\n + --[no-]cswitch switch has trailing tab character\n -+ --[no-]short-hint \n -+ with a one symbol hint\n ++ --[no-]short-hint with a one symbol hint\n\n Extras\n - --extra1 line above used to cause a segfault but no longer does\n-: ---------- > 6: 46dcdb902d parse-options: factor out usage_indent() and usage_padding()\n-: ---------- > 7: fdeca0d6d2 parse-options: no --[no-]no-...\n-: ---------- > 8: 08b2d1e861 parse-options: simplify usage_padding()\n--\n2.41.0\n"},{"id":"480188","messageId":"d9bb18cc-0290-c244-c2ef-19050d69d584@web.de","threadId":"59999","inReplyTo":"e4d46d97-1cd4-7fea-afd1-7de5023b1b09@web.de","subject":"[PATCH v3 1/8] subtree: disallow --no-{help,quiet,debug,branch,message}","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-08-05T14:37:40Z","receivedAt":"2023-08-05T14:37:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"\"git subtree\" only handles the negated variant of the options annotate,\nprefix, onto, rejoin, ignore-joins and squash explicitly. help is\nhandled by \"git rev-parse --parseopt\" implicitly, but not its negated\nform. Disable negation for it and the for the rest of the options to\nget a helpful error message when trying them.\n\nSigned-off-by: René Scharfe \n---\n contrib/subtree/git-subtree.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 7db4c45676..e0c5d3b0de 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -33,19 +33,19 @@ git subtree split --prefix= []\n git subtree pull --prefix= \n git subtree push --prefix= \n --\n-h,help show the help\n-q,quiet quiet\n-d,debug show debug messages\n+h,help! show the help\n+q,quiet! quiet\n+d,debug! show debug messages\n P,prefix= the name of the subdir to split out\n options for 'split' (also: 'push')\n annotate= add a prefix to commit message of new commits\n-b,branch= create a new branch from the split subtree\n+b,branch!= create a new branch from the split subtree\n ignore-joins ignore prior --rejoin commits\n onto= try connecting new tree to an existing one\n rejoin merge the new branch back into HEAD\n options for 'add' and 'merge' (also: 'pull', 'split --rejoin', and 'push --rejoin')\n squash merge subtree changes as a single commit\n-m,message= use the given message as the commit message for the merge commit\n+m,message!= use the given message as the commit message for the merge commit\n \"\n\n indent=0\n--\n2.41.0\n"},{"id":"480189","messageId":"b8339723-ce30-b87e-041f-eb4c130c2fb1@web.de","threadId":"59999","inReplyTo":"e4d46d97-1cd4-7fea-afd1-7de5023b1b09@web.de","subject":"[PATCH v3 2/8] t1502, docs: disallow --no-help","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-08-05T14:37:55Z","receivedAt":"2023-08-05T14:38:04Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"\"git rev-parse --parseopt\" handles the built-in options -h and --help,\nbut not --no-help. Make test definitions and documentation examples\nmore realistic by disabling negation.\n\nSigned-off-by: René Scharfe \n---\n Documentation/git-rev-parse.txt | 2 +-\n t/t1502-rev-parse-parseopt.sh | 6 +++---\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt\nindex f26a7591e3..6e8ff9ace1 100644\n--- a/Documentation/git-rev-parse.txt\n+++ b/Documentation/git-rev-parse.txt\n@@ -398,7 +398,7 @@ some-command [] ...\n\n some-command does foo and bar!\n --\n-h,help show the help\n+h,help! show the help\n\n foo some nifty option --foo\n bar= some cool option --bar with an argument\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex dd811b7fb4..0cdc6eb8b3 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -9,7 +9,7 @@ test_expect_success 'setup optionspec' '\n |\n |some-command does foo and bar!\n |--\n-|h,help show the help\n+|h,help! show the help\n |\n |foo some nifty option --foo\n |bar= some cool option --bar with an argument\n@@ -288,7 +288,7 @@ test_expect_success 'test --parseopt help output: \"wrapped\" options normal \"or:\"\n \t| [--another-option]\n \t|cmd [--yet-another-option]\n \t|--\n-\t|h,help show the help\n+\t|h,help! show the help\n \tEOF\n\n \tsed -e \"s/^|//\" >expect <<-\\END_EXPECT &&\n@@ -322,7 +322,7 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l\n \t|line\n \t|blurb\n \t|--\n-\t|h,help show the help\n+\t|h,help! show the help\n \tEOF\n\n \tsed -e \"s/^|//\" >expect <<-\\END_EXPECT &&\n--\n2.41.0\n"},{"id":"480190","messageId":"d5df2863-f925-7a1b-9e82-c6f55d8cb931@web.de","threadId":"59999","inReplyTo":"e4d46d97-1cd4-7fea-afd1-7de5023b1b09@web.de","subject":"[PATCH v3 3/8] t1502: move optionspec help output to a file","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-08-05T14:38:56Z","receivedAt":"2023-08-05T14:39:05Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"\"git rev-parse --parseopt\" shows the short help with its description of\nall recognized options twice: When called with -h or --help, and after\nreporting an unknown option. Move the one for optionspec into a file\nand use it in two tests to deduplicate that part.\n\n\"git rev-parse --parseopt -- --h\" wraps the help text in \"cat <<\\EOF\"\nand \"EOF\". Keep that part in the file to use it as is in the test that\nneeds it and simply remove it in the other one using sed.\n\nDisable whitespace checking for the file using an attribute, as we need\nto keep its spaces intact and wouldn't want a stray --whitespace=fix\nturn them into tabs.\n\nSigned-off-by: René Scharfe \n---\n t/t1502-rev-parse-parseopt.sh | 79 ++++-------------------------------\n t/t1502/.gitattributes | 1 +\n t/t1502/optionspec.help | 34 +++++++++++++++\n 3 files changed, 42 insertions(+), 72 deletions(-)\n create mode 100644 t/t1502/.gitattributes\n create mode 100755 t/t1502/optionspec.help\n\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex 0cdc6eb8b3..813ee5872f 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -58,44 +58,8 @@ EOF\n '\n\n test_expect_success 'test --parseopt help output' '\n-\tsed -e \"s/^|//\" >expect <<\\END_EXPECT &&\n-|cat <<\\EOF\n-|usage: some-command [options] ...\n-|\n-| some-command does foo and bar!\n-|\n-| -h, --help show the help\n-| --foo some nifty option --foo\n-| --bar ... some cool option --bar with an argument\n-| -b, --baz a short and long option\n-|\n-|An option group Header\n-| -C[...] option C with an optional argument\n-| -d, --data[=...] short and long option with an optional argument\n-|\n-|Argument hints\n-| -B short option required argument\n-| --bar2 long option required argument\n-| -e, --fuz \n-| short and long option required argument\n-| -s[] short option optional argument\n-| --long[=] long option optional argument\n-| -g, --fluf[=] short and long option optional argument\n-| --longest \n-| a very long argument hint\n-| --pair with an equals sign in the hint\n-| --aswitch help te=t contains? fl*g characters!`\n-| --bswitch hint has trailing tab character\n-| --cswitch switch has trailing tab character\n-| --short-hint with a one symbol hint\n-|\n-|Extras\n-| --extra1 line above used to cause a segfault but no longer does\n-|\n-|EOF\n-END_EXPECT\n \ttest_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec &&\n-\ttest_cmp expect output\n+\ttest_cmp \"$TEST_DIRECTORY/t1502/optionspec.help\" output\n '\n\n test_expect_success 'test --parseopt help output no switches' '\n@@ -140,41 +104,12 @@ END_EXPECT\n '\n\n test_expect_success 'test --parseopt invalid switch help output' '\n-\tsed -e \"s/^|//\" >expect <<\\END_EXPECT &&\n-|error: unknown option `does-not-exist'\\''\n-|usage: some-command [options] ...\n-|\n-| some-command does foo and bar!\n-|\n-| -h, --help show the help\n-| --foo some nifty option --foo\n-| --bar ... some cool option --bar with an argument\n-| -b, --baz a short and long option\n-|\n-|An option group Header\n-| -C[...] option C with an optional argument\n-| -d, --data[=...] short and long option with an optional argument\n-|\n-|Argument hints\n-| -B short option required argument\n-| --bar2 long option required argument\n-| -e, --fuz \n-| short and long option required argument\n-| -s[] short option optional argument\n-| --long[=] long option optional argument\n-| -g, --fluf[=] short and long option optional argument\n-| --longest \n-| a very long argument hint\n-| --pair with an equals sign in the hint\n-| --aswitch help te=t contains? fl*g characters!`\n-| --bswitch hint has trailing tab character\n-| --cswitch switch has trailing tab character\n-| --short-hint with a one symbol hint\n-|\n-|Extras\n-| --extra1 line above used to cause a segfault but no longer does\n-|\n-END_EXPECT\n+\t{\n+\t\tcat <<-\\EOF &&\n+\t\terror: unknown option `does-not-exist'\\''\n+\t\tEOF\n+\t\tsed -e 1d -e \\$d <\"$TEST_DIRECTORY/t1502/optionspec.help\"\n+\t} >expect &&\n \ttest_expect_code 129 git rev-parse --parseopt -- --does-not-exist 1>/dev/null 2>output < optionspec &&\n \ttest_cmp expect output\n '\ndiff --git a/t/t1502/.gitattributes b/t/t1502/.gitattributes\nnew file mode 100644\nindex 0000000000..562b12e16e\n--- /dev/null\n+++ b/t/t1502/.gitattributes\n@@ -0,0 +1 @@\n+* -whitespace\ndiff --git a/t/t1502/optionspec.help b/t/t1502/optionspec.help\nnew file mode 100755\nindex 0000000000..844eac6704\n--- /dev/null\n+++ b/t/t1502/optionspec.help\n@@ -0,0 +1,34 @@\n+cat <<\\EOF\n+usage: some-command [options] ...\n+\n+ some-command does foo and bar!\n+\n+ -h, --help show the help\n+ --foo some nifty option --foo\n+ --bar ... some cool option --bar with an argument\n+ -b, --baz a short and long option\n+\n+An option group Header\n+ -C[...] option C with an optional argument\n+ -d, --data[=...] short and long option with an optional argument\n+\n+Argument hints\n+ -B short option required argument\n+ --bar2 long option required argument\n+ -e, --fuz \n+ short and long option required argument\n+ -s[] short option optional argument\n+ --long[=] long option optional argument\n+ -g, --fluf[=] short and long option optional argument\n+ --longest \n+ a very long argument hint\n+ --pair with an equals sign in the hint\n+ --aswitch help te=t contains? fl*g characters!`\n+ --bswitch hint has trailing tab character\n+ --cswitch switch has trailing tab character\n+ --short-hint with a one symbol hint\n+\n+Extras\n+ --extra1 line above used to cause a segfault but no longer does\n+\n+EOF\n--\n2.41.0\n"},{"id":"480191","messageId":"0c774b88-effa-75a4-27e9-3c21a11c0dba@web.de","threadId":"59999","inReplyTo":"e4d46d97-1cd4-7fea-afd1-7de5023b1b09@web.de","subject":"[PATCH v3 4/8] t1502: test option negation","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-08-05T14:39:52Z","receivedAt":"2023-08-05T14:40:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add tests for checking the \"git rev-parse --parseopt\" flag \"!\" and\nwhether options can be negated with a \"no-\" prefix.\n\nSigned-off-by: René Scharfe \n---\n t/t1502-rev-parse-parseopt.sh | 44 +++++++++++++++++++++++++++++++++++\n t/t1502/optionspec-neg | 8 +++++++\n t/t1502/optionspec-neg.help | 11 +++++++++\n 3 files changed, 63 insertions(+)\n create mode 100644 t/t1502/optionspec-neg\n create mode 100644 t/t1502/optionspec-neg.help\n\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex 813ee5872f..0f7c2db4c0 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -3,6 +3,22 @@\n test_description='test git rev-parse --parseopt'\n . ./test-lib.sh\n\n+check_invalid_long_option () {\n+\tspec=\"$1\"\n+\topt=\"$2\"\n+\ttest_expect_success \"test --parseopt invalid switch $opt help output for $spec\" '\n+\t\t{\n+\t\t\tcat <<-\\EOF &&\n+\t\t\terror: unknown option `'${opt#--}\\''\n+\t\t\tEOF\n+\t\t\tsed -e 1d -e \\$d <\"$TEST_DIRECTORY/t1502/$spec.help\"\n+\t\t} >expect &&\n+\t\ttest_expect_code 129 git rev-parse --parseopt -- $opt \\\n+\t\t\t2>output <\"$TEST_DIRECTORY/t1502/$spec\" &&\n+\t\ttest_cmp expect output\n+\t'\n+}\n+\n test_expect_success 'setup optionspec' '\n \tsed -e \"s/^|//\" >optionspec <<\\EOF\n |some-command [options] ...\n@@ -278,4 +294,32 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l\n \ttest_cmp expect actual\n '\n\n+test_expect_success 'test --parseopt help output for optionspec-neg' '\n+\ttest_expect_code 129 git rev-parse --parseopt -- \\\n+\t\t-h >output <\"$TEST_DIRECTORY/t1502/optionspec-neg\" &&\n+\ttest_cmp \"$TEST_DIRECTORY/t1502/optionspec-neg.help\" output\n+'\n+\n+test_expect_success 'test --parseopt valid options for optionspec-neg' '\n+\tcat >expect <<-\\EOF &&\n+\tset -- --foo --no-foo --no-bar --positive-only --no-negative --\n+\tEOF\n+\tgit rev-parse --parseopt -- <\"$TEST_DIRECTORY/t1502/optionspec-neg\" >output \\\n+\t --foo --no-foo --no-bar --positive-only --no-negative &&\n+\ttest_cmp expect output\n+'\n+\n+test_expect_success 'test --parseopt positivated option for optionspec-neg' '\n+\tcat >expect <<-\\EOF &&\n+\tset -- --no-no-bar --no-no-bar --\n+\tEOF\n+\tgit rev-parse --parseopt -- <\"$TEST_DIRECTORY/t1502/optionspec-neg\" >output \\\n+\t --no-no-bar --bar &&\n+\ttest_cmp expect output\n+'\n+\n+check_invalid_long_option optionspec-neg --no-positive-only\n+check_invalid_long_option optionspec-neg --negative\n+check_invalid_long_option optionspec-neg --no-no-negative\n+\n test_done\ndiff --git a/t/t1502/optionspec-neg b/t/t1502/optionspec-neg\nnew file mode 100644\nindex 0000000000..28992ee303\n--- /dev/null\n+++ b/t/t1502/optionspec-neg\n@@ -0,0 +1,8 @@\n+some-command [options] ...\n+\n+some-command does foo and bar!\n+--\n+foo\t\tcan be negated\n+no-bar\t\tcan be positivated\n+positive-only!\tcannot be negated\n+no-negative!\tcannot be positivated\ndiff --git a/t/t1502/optionspec-neg.help b/t/t1502/optionspec-neg.help\nnew file mode 100644\nindex 0000000000..591f4dcd9f\n--- /dev/null\n+++ b/t/t1502/optionspec-neg.help\n@@ -0,0 +1,11 @@\n+cat <<\\EOF\n+usage: some-command [options] ...\n+\n+ some-command does foo and bar!\n+\n+ --foo can be negated\n+ --no-bar can be positivated\n+ --positive-only cannot be negated\n+ --no-negative cannot be positivated\n+\n+EOF\n--\n2.41.0\n"},{"id":"480192","messageId":"025bb465-635d-33ed-c847-924ead2635c5@web.de","threadId":"59999","inReplyTo":"e4d46d97-1cd4-7fea-afd1-7de5023b1b09@web.de","subject":"[PATCH v3 5/8] parse-options: show negatability of options in short help","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-08-05T14:40:59Z","receivedAt":"2023-08-05T14:41:10Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add a \"[no-]\" prefix to options without the flag PARSE_OPT_NONEG to\ndocument the fact that you can negate them.\n\nThis looks a bit strange for options that already start with \"no-\", e.g.\nfor the option --no-name of git show-branch:\n\n --[no-]no-name suppress naming strings\n\nYou can actually use --no-no-name as an alias of --name, so the short\nhelp is not wrong. If we strip off any of the \"no-\"s, we lose either\nthe ability to see if the remaining one belongs to the documented\nvariant or to see if it can be negated.\n\nSigned-off-by: René Scharfe \n---\n Documentation/git-rev-parse.txt | 8 +++---\n contrib/subtree/t/t7900-subtree.sh | 2 +-\n parse-options.c | 10 +++++--\n t/t0040-parse-options.sh | 45 +++++++++++++++++-------------\n t/t1502-rev-parse-parseopt.sh | 2 +-\n t/t1502/optionspec-neg.help | 4 +--\n t/t1502/optionspec.help | 32 +++++++++++----------\n 7 files changed, 58 insertions(+), 45 deletions(-)\n\ndiff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt\nindex 6e8ff9ace1..6a4968f68a 100644\n--- a/Documentation/git-rev-parse.txt\n+++ b/Documentation/git-rev-parse.txt\n@@ -424,10 +424,10 @@ usage: some-command [] ...\n some-command does foo and bar!\n\n -h, --help show the help\n- --foo some nifty option --foo\n- --bar ... some cool option --bar with an argument\n- --baz another cool option --baz with a named argument\n- --qux[=] qux may take a path argument but has meaning by itself\n+ --[no-]foo some nifty option --foo\n+ --[no-]bar ... some cool option --bar with an argument\n+ --[no-]baz another cool option --baz with a named argument\n+ --[no-]qux[=] qux may take a path argument but has meaning by itself\n\n An option group Header\n -C[...] option C with an optional argument\ndiff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\nindex 341c169eca..49a21dd7c9 100755\n--- a/contrib/subtree/t/t7900-subtree.sh\n+++ b/contrib/subtree/t/t7900-subtree.sh\n@@ -71,7 +71,7 @@ test_expect_success 'shows short help text for -h' '\n \ttest_expect_code 129 git subtree -h >out 2>err &&\n \ttest_must_be_empty err &&\n \tgrep -e \"^ *or: git subtree pull\" out &&\n-\tgrep -e --annotate out\n+\tgrep -F -e \"--[no-]annotate\" out\n '\n\n #\ndiff --git a/parse-options.c b/parse-options.c\nindex 87c9fae634..b750bf91cd 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -1137,8 +1137,14 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n \t\t}\n \t\tif (opts->long_name && opts->short_name)\n \t\t\tpos += fprintf(outfile, \", \");\n-\t\tif (opts->long_name)\n-\t\t\tpos += fprintf(outfile, \"--%s\", opts->long_name);\n+\t\tif (opts->long_name) {\n+\t\t\tconst char *long_name = opts->long_name;\n+\t\t\tif (opts->flags & PARSE_OPT_NONEG)\n+\t\t\t\tpos += fprintf(outfile, \"--%s\", long_name);\n+\t\t\telse\n+\t\t\t\tpos += fprintf(outfile, \"--[no-]%s\", long_name);\n+\t\t}\n+\n \t\tif (opts->type == OPTION_NUMBER)\n \t\t\tpos += utf8_fprintf(outfile, _(\"-NUM\"));\n\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex e19a199636..1dfc431d52 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -13,30 +13,34 @@ usage: test-tool parse-options \n\n A helper function for the parse-options API.\n\n- --yes get a boolean\n- -D, --no-doubt begins with 'no-'\n+ --[no-]yes get a boolean\n+ -D, --[no-]no-doubt begins with 'no-'\n -B, --no-fear be brave\n- -b, --boolean increment by one\n- -4, --or4 bitwise-or boolean with ...0100\n- --neg-or4 same as --no-or4\n+ -b, --[no-]boolean increment by one\n+ -4, --[no-]or4 bitwise-or boolean with ...0100\n+ --[no-]neg-or4 same as --no-or4\n\n- -i, --integer get a integer\n+ -i, --[no-]integer \n+ get a integer\n -j get a integer, too\n -m, --magnitude get a magnitude\n- --set23 set integer to 23\n+ --[no-]set23 set integer to 23\n --mode1 set integer to 1 (cmdmode option)\n --mode2 set integer to 2 (cmdmode option)\n- -L, --length get length of \n- -F, --file set file to \n+ -L, --[no-]length \n+ get length of \n+ -F, --[no-]file \n+ set file to \n\n String options\n- -s, --string get a string\n- --string2 get another string\n- --st get another string (pervert ordering)\n+ -s, --[no-]string \n+ get a string\n+ --[no-]string2 get another string\n+ --[no-]st get another string (pervert ordering)\n -o get another string\n --longhelp help text of this entry\n spans multiple lines\n- --list add str to list\n+ --[no-]list add str to list\n\n Magic arguments\n -NUM set integer to NUM\n@@ -45,16 +49,17 @@ Magic arguments\n --no-ambiguous negative ambiguity\n\n Standard options\n- --abbrev[=] use digits to display object names\n- -v, --verbose be verbose\n- -n, --dry-run dry run\n- -q, --quiet be quiet\n- --expect expected output in the variable dump\n+ --[no-]abbrev[=] use digits to display object names\n+ -v, --[no-]verbose be verbose\n+ -n, --[no-]dry-run dry run\n+ -q, --[no-]quiet be quiet\n+ --[no-]expect \n+ expected output in the variable dump\n\n Alias\n- -A, --alias-source \n+ -A, --[no-]alias-source \n get a string\n- -Z, --alias-target \n+ -Z, --[no-]alias-target \n alias of --alias-source\n\n EOF\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex 0f7c2db4c0..f0737593c3 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -111,7 +111,7 @@ test_expect_success 'test --parseopt help-all output hidden switches' '\n |\n | some-command does foo and bar!\n |\n-| --hidden1 A hidden switch\n+| --[no-]hidden1 A hidden switch\n |\n |EOF\n END_EXPECT\ndiff --git a/t/t1502/optionspec-neg.help b/t/t1502/optionspec-neg.help\nindex 591f4dcd9f..156e5f0ed9 100644\n--- a/t/t1502/optionspec-neg.help\n+++ b/t/t1502/optionspec-neg.help\n@@ -3,8 +3,8 @@ usage: some-command [options] ...\n\n some-command does foo and bar!\n\n- --foo can be negated\n- --no-bar can be positivated\n+ --[no-]foo can be negated\n+ --[no-]no-bar can be positivated\n --positive-only cannot be negated\n --no-negative cannot be positivated\n\ndiff --git a/t/t1502/optionspec.help b/t/t1502/optionspec.help\nindex 844eac6704..cbdd54d41b 100755\n--- a/t/t1502/optionspec.help\n+++ b/t/t1502/optionspec.help\n@@ -4,31 +4,33 @@ usage: some-command [options] ...\n some-command does foo and bar!\n\n -h, --help show the help\n- --foo some nifty option --foo\n- --bar ... some cool option --bar with an argument\n- -b, --baz a short and long option\n+ --[no-]foo some nifty option --foo\n+ --[no-]bar ... some cool option --bar with an argument\n+ -b, --[no-]baz a short and long option\n\n An option group Header\n -C[...] option C with an optional argument\n- -d, --data[=...] short and long option with an optional argument\n+ -d, --[no-]data[=...] short and long option with an optional argument\n\n Argument hints\n -B short option required argument\n- --bar2 long option required argument\n- -e, --fuz \n+ --[no-]bar2 long option required argument\n+ -e, --[no-]fuz \n short and long option required argument\n -s[] short option optional argument\n- --long[=] long option optional argument\n- -g, --fluf[=] short and long option optional argument\n- --longest \n+ --[no-]long[=] long option optional argument\n+ -g, --[no-]fluf[=]\n+ short and long option optional argument\n+ --[no-]longest \n a very long argument hint\n- --pair with an equals sign in the hint\n- --aswitch help te=t contains? fl*g characters!`\n- --bswitch hint has trailing tab character\n- --cswitch switch has trailing tab character\n- --short-hint with a one symbol hint\n+ --[no-]pair \n+ with an equals sign in the hint\n+ --[no-]aswitch help te=t contains? fl*g characters!`\n+ --[no-]bswitch hint has trailing tab character\n+ --[no-]cswitch switch has trailing tab character\n+ --[no-]short-hint with a one symbol hint\n\n Extras\n- --extra1 line above used to cause a segfault but no longer does\n+ --[no-]extra1 line above used to cause a segfault but no longer does\n\n EOF\n--\n2.41.0\n"},{"id":"480193","messageId":"65090b9f-182e-86a5-955d-184fa7164d38@web.de","threadId":"59999","inReplyTo":"e4d46d97-1cd4-7fea-afd1-7de5023b1b09@web.de","subject":"[PATCH v3 6/8] parse-options: factor out usage_indent() and usage_padding()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-08-05T14:43:04Z","receivedAt":"2023-08-05T14:43:14Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Extract functions for printing spaces before and after options. We'll\nneed them in the next commit.\n\nSigned-off-by: René Scharfe \n---\n parse-options.c | 39 ++++++++++++++++++++++++---------------\n 1 file changed, 24 insertions(+), 15 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex b750bf91cd..4b76fc81e9 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -1020,9 +1020,28 @@ static int usage_argh(const struct option *opts, FILE *outfile)\n \treturn utf8_fprintf(outfile, s, opts->argh ? _(opts->argh) : _(\"...\"));\n }\n\n+static int usage_indent(FILE *outfile)\n+{\n+\treturn fprintf(outfile, \" \");\n+}\n+\n #define USAGE_OPTS_WIDTH 24\n #define USAGE_GAP 2\n\n+static void usage_padding(FILE *outfile, size_t pos)\n+{\n+\tint pad;\n+\tif (pos == USAGE_OPTS_WIDTH + 1)\n+\t\tpad = -1;\n+\telse if (pos <= USAGE_OPTS_WIDTH)\n+\t\tpad = USAGE_OPTS_WIDTH - pos;\n+\telse {\n+\t\tfputc('\\n', outfile);\n+\t\tpad = USAGE_OPTS_WIDTH;\n+\t}\n+\tfprintf(outfile, \"%*s\", pad + USAGE_GAP, \"\");\n+}\n+\n static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n \t\t\t\t\t\t\t const char * const *usagestr,\n \t\t\t\t\t\t\t const struct option *opts,\n@@ -1108,7 +1127,6 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n\n \tfor (; opts->type != OPTION_END; opts++) {\n \t\tsize_t pos;\n-\t\tint pad;\n \t\tconst char *cp, *np;\n\n \t\tif (opts->type == OPTION_SUBCOMMAND)\n@@ -1128,7 +1146,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n \t\t\tneed_newline = 0;\n \t\t}\n\n-\t\tpos = fprintf(outfile, \" \");\n+\t\tpos = usage_indent(outfile);\n \t\tif (opts->short_name) {\n \t\t\tif (opts->flags & PARSE_OPT_NODASH)\n \t\t\t\tpos += fprintf(outfile, \"%c\", opts->short_name);\n@@ -1152,16 +1170,8 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n \t\t !(opts->flags & PARSE_OPT_NOARG))\n \t\t\tpos += usage_argh(opts, outfile);\n\n-\t\tif (pos == USAGE_OPTS_WIDTH + 1)\n-\t\t\tpad = -1;\n-\t\telse if (pos <= USAGE_OPTS_WIDTH)\n-\t\t\tpad = USAGE_OPTS_WIDTH - pos;\n-\t\telse {\n-\t\t\tfputc('\\n', outfile);\n-\t\t\tpad = USAGE_OPTS_WIDTH;\n-\t\t}\n \t\tif (opts->type == OPTION_ALIAS) {\n-\t\t\tfprintf(outfile, \"%*s\", pad + USAGE_GAP, \"\");\n+\t\t\tusage_padding(outfile, pos);\n \t\t\tfprintf_ln(outfile, _(\"alias of --%s\"),\n \t\t\t\t (const char *)opts->value);\n \t\t\tcontinue;\n@@ -1169,12 +1179,11 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n\n \t\tfor (cp = _(opts->help); *cp; cp = np) {\n \t\t\tnp = strchrnul(cp, '\\n');\n-\t\t\tfprintf(outfile,\n-\t\t\t\t\"%*s%.*s\\n\", pad + USAGE_GAP, \"\",\n-\t\t\t\t(int)(np - cp), cp);\n+\t\t\tusage_padding(outfile, pos);\n+\t\t\tfprintf(outfile, \"%.*s\\n\", (int)(np - cp), cp);\n \t\t\tif (*np)\n \t\t\t\tnp++;\n-\t\t\tpad = USAGE_OPTS_WIDTH;\n+\t\t\tpos = 0;\n \t\t}\n \t}\n \tfputc('\\n', outfile);\n--\n2.41.0\n"},{"id":"480194","messageId":"21b40894-1f1b-9e3e-8fe9-754ca9fe685c@web.de","threadId":"59999","inReplyTo":"e4d46d97-1cd4-7fea-afd1-7de5023b1b09@web.de","subject":"[PATCH v3 7/8] parse-options: no --[no-]no-...","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-08-05T14:44:45Z","receivedAt":"2023-08-05T14:44:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Avoid showing an optional \"no-\" for options that already start with a\n\"no-\" in the short help, as that double negation is confusing. Document\nthe opposite variant on its own line with a generated help text instead,\nunless it's defined and documented explicitly already.\n\nSigned-off-by: René Scharfe \n---\n parse-options.c | 25 ++++++++++++++++++++++++-\n t/t0040-parse-options.sh | 3 ++-\n t/t1502/optionspec-neg.help | 3 ++-\n 3 files changed, 28 insertions(+), 3 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 4b76fc81e9..4a8d380ceb 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -1042,11 +1042,22 @@ static void usage_padding(FILE *outfile, size_t pos)\n \tfprintf(outfile, \"%*s\", pad + USAGE_GAP, \"\");\n }\n\n+static const struct option *find_option_by_long_name(const struct option *opts,\n+\t\t\t\t\t\t const char *long_name)\n+{\n+\tfor (; opts->type != OPTION_END; opts++) {\n+\t\tif (opts->long_name && !strcmp(opts->long_name, long_name))\n+\t\t\treturn opts;\n+\t}\n+\treturn NULL;\n+}\n+\n static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n \t\t\t\t\t\t\t const char * const *usagestr,\n \t\t\t\t\t\t\t const struct option *opts,\n \t\t\t\t\t\t\t int full, int err)\n {\n+\tconst struct option *all_opts = opts;\n \tFILE *outfile = err ? stderr : stdout;\n \tint need_newline;\n\n@@ -1128,6 +1139,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n \tfor (; opts->type != OPTION_END; opts++) {\n \t\tsize_t pos;\n \t\tconst char *cp, *np;\n+\t\tconst char *positive_name = NULL;\n\n \t\tif (opts->type == OPTION_SUBCOMMAND)\n \t\t\tcontinue;\n@@ -1157,7 +1169,8 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n \t\t\tpos += fprintf(outfile, \", \");\n \t\tif (opts->long_name) {\n \t\t\tconst char *long_name = opts->long_name;\n-\t\t\tif (opts->flags & PARSE_OPT_NONEG)\n+\t\t\tif ((opts->flags & PARSE_OPT_NONEG) ||\n+\t\t\t skip_prefix(long_name, \"no-\", &positive_name))\n \t\t\t\tpos += fprintf(outfile, \"--%s\", long_name);\n \t\t\telse\n \t\t\t\tpos += fprintf(outfile, \"--[no-]%s\", long_name);\n@@ -1185,6 +1198,16 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n \t\t\t\tnp++;\n \t\t\tpos = 0;\n \t\t}\n+\n+\t\tif (positive_name) {\n+\t\t\tif (find_option_by_long_name(all_opts, positive_name))\n+\t\t\t\tcontinue;\n+\t\t\tpos = usage_indent(outfile);\n+\t\t\tpos += fprintf(outfile, \"--%s\", positive_name);\n+\t\t\tusage_padding(outfile, pos);\n+\t\t\tfprintf_ln(outfile, _(\"opposite of --no-%s\"),\n+\t\t\t\t positive_name);\n+\t\t}\n \t}\n \tfputc('\\n', outfile);\n\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 1dfc431d52..a0ad6192d6 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -14,7 +14,8 @@ usage: test-tool parse-options \n A helper function for the parse-options API.\n\n --[no-]yes get a boolean\n- -D, --[no-]no-doubt begins with 'no-'\n+ -D, --no-doubt begins with 'no-'\n+ --doubt opposite of --no-doubt\n -B, --no-fear be brave\n -b, --[no-]boolean increment by one\n -4, --[no-]or4 bitwise-or boolean with ...0100\ndiff --git a/t/t1502/optionspec-neg.help b/t/t1502/optionspec-neg.help\nindex 156e5f0ed9..4cd3c83e55 100644\n--- a/t/t1502/optionspec-neg.help\n+++ b/t/t1502/optionspec-neg.help\n@@ -4,7 +4,8 @@ usage: some-command [options] ...\n some-command does foo and bar!\n\n --[no-]foo can be negated\n- --[no-]no-bar can be positivated\n+ --no-bar can be positivated\n+ --bar opposite of --no-bar\n --positive-only cannot be negated\n --no-negative cannot be positivated\n\n--\n2.41.0\n"},{"id":"480196","messageId":"a3a93950-9a0c-d68c-a32a-06c83819d72b@web.de","threadId":"59999","inReplyTo":"e4d46d97-1cd4-7fea-afd1-7de5023b1b09@web.de","subject":"[PATCH v3 8/8] parse-options: simplify usage_padding()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-08-05T14:52:42Z","receivedAt":"2023-08-05T14:52:51Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"c512643e67 (short help: allow a gap smaller than USAGE_GAP, 2023-07-18)\neffectively did away with the two-space gap between options and their\ndescription; one space is enough now. Incorporate USAGE_GAP into\nUSAGE_OPTS_WIDTH, merge the two cases with enough space on the line and\nincorporate the newline into the format for the remaining case. The\noutput remains the same.\n\nSigned-off-by: René Scharfe \n---\nRemoved an extra empty line added mid-submission, so the cover letter\nstats are off by one.\n\n parse-options.c | 17 +++++------------\n 1 file changed, 5 insertions(+), 12 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 4a8d380ceb..b00d868816 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -1025,21 +1025,14 @@ static int usage_indent(FILE *outfile)\n \treturn fprintf(outfile, \" \");\n }\n\n-#define USAGE_OPTS_WIDTH 24\n-#define USAGE_GAP 2\n+#define USAGE_OPTS_WIDTH 26\n\n static void usage_padding(FILE *outfile, size_t pos)\n {\n-\tint pad;\n-\tif (pos == USAGE_OPTS_WIDTH + 1)\n-\t\tpad = -1;\n-\telse if (pos <= USAGE_OPTS_WIDTH)\n-\t\tpad = USAGE_OPTS_WIDTH - pos;\n-\telse {\n-\t\tfputc('\\n', outfile);\n-\t\tpad = USAGE_OPTS_WIDTH;\n-\t}\n-\tfprintf(outfile, \"%*s\", pad + USAGE_GAP, \"\");\n+\tif (pos < USAGE_OPTS_WIDTH)\n+\t\tfprintf(outfile, \"%*s\", USAGE_OPTS_WIDTH - (int)pos, \"\");\n+\telse\n+\t\tfprintf(outfile, \"\\n%*s\", USAGE_OPTS_WIDTH, \"\");\n }\n\n static const struct option *find_option_by_long_name(const struct option *opts,\n--\n2.41.0\n"},{"id":"480198","messageId":"xmqqo7jljdk7.fsf@gitster.g","threadId":"59999","inReplyTo":"a3a93950-9a0c-d68c-a32a-06c83819d72b@web.de","subject":"Re: [PATCH v3 8/8] parse-options: simplify usage_padding()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-05T23:04:56Z","receivedAt":"2023-08-05T23:05:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> c512643e67 (short help: allow a gap smaller than USAGE_GAP, 2023-07-18)\n> effectively did away with the two-space gap between options and their\n> description; one space is enough now.\n\nMy intention was \"we strongly prefer two or more, but on a rare\noccasion where the leading strong is long and can afford to give us\nonly one, we take it instead of breaking the line\", but stepping\nback and thinking about it again, you are absolutely right. I just\nshrunk the minimum gap from 2 to 1.\n"},{"id":"480340","messageId":"20230808212720.GA760752@coredump.intra.peff.net","threadId":"59999","inReplyTo":"4eea7e15-6594-93e2-27b5-3d6e3c0baac6@web.de","subject":"Re: [PATCH] describe: fix --no-exact-match","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-08T21:27:20Z","receivedAt":"2023-08-08T21:27:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 21, 2023 at 03:41:33PM +0200, René Scharfe wrote:\n\n> +static int option_parse_exact_match(const struct option *opt, const char *arg,\n> +\t\t\t\t int unset)\n> +{\n> +\tBUG_ON_OPT_ARG(arg);\n> +\n> +\tmax_candidates = unset ? DEFAULT_CANDIDATES : 0;\n> +\treturn 0;\n> +}\n\nI wanted to call out a style question here. The \"opt\" parameter is\nunused, since it manipulates the \"max_candidates\" global directly. I can\nadd an UNUSED annotation to satisfy -Wunused-parameter, but in such\ncases I've usually been modifying them like:\n\n int *value = opt->value;\n ...\n *value = unset ? DEFAULT_CANDIDATES : 0;\n\nso that the callback operates on the value passed in the options list.\n\nBut I see you converted that value to NULL here:\n\n> @@ -568,8 +578,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n> \t\tOPT_BOOL(0, \"long\", &longformat, N_(\"always use long format\")),\n> \t\tOPT_BOOL(0, \"first-parent\", &first_parent, N_(\"only follow first parent\")),\n> \t\tOPT__ABBREV(&abbrev),\n> -\t\tOPT_SET_INT(0, \"exact-match\", &max_candidates,\n> -\t\t\t N_(\"only output exact matches\"), 0),\n> +\t\tOPT_CALLBACK_F(0, \"exact-match\", NULL, NULL,\n> +\t\t\t N_(\"only output exact matches\"),\n> +\t\t\t PARSE_OPT_NOARG, option_parse_exact_match),\n\nso at least the result does not have the subtle gotcha that existed in\nother cases I changed. :)\n\nSo before I sent a patch (either to switch to using opt->value, or to\nadd an UNUSED annotation), I wanted to see what you (or others) thought\nbetween the two. I.e., should we have a rule of \"try not to operate on\nglobal data via option callbacks\" or is that just being too pedantic for\none-off callbacks like this?\n\n-Peff\n"},{"id":"480341","messageId":"20230808212852.GA760815@coredump.intra.peff.net","threadId":"59999","inReplyTo":"20230808212720.GA760752@coredump.intra.peff.net","subject":"Re: [PATCH] describe: fix --no-exact-match","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-08T21:28:52Z","receivedAt":"2023-08-08T21:28:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 08, 2023 at 05:27:21PM -0400, Jeff King wrote:\n\n> So before I sent a patch (either to switch to using opt->value, or to\n> add an UNUSED annotation), I wanted to see what you (or others) thought\n> between the two. I.e., should we have a rule of \"try not to operate on\n> global data via option callbacks\" or is that just being too pedantic for\n> one-off callbacks like this?\n\nOh, and btw, the same situation exists for the \"pack-objects --no-quiet\"\nearlier in this thread (which is why I was trying to establish the\ngeneral approach).\n\n-Peff\n"},{"id":"480346","messageId":"xmqqzg3156sy.fsf@gitster.g","threadId":"59999","inReplyTo":"20230808212720.GA760752@coredump.intra.peff.net","subject":"Re: [PATCH] describe: fix --no-exact-match","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-09T01:43:41Z","receivedAt":"2023-08-09T01:43:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Jul 21, 2023 at 03:41:33PM +0200, René Scharfe wrote:\n>\n>> +static int option_parse_exact_match(const struct option *opt, const char *arg,\n>> +\t\t\t\t int unset)\n>> +{\n>> +\tBUG_ON_OPT_ARG(arg);\n>> +\n>> +\tmax_candidates = unset ? DEFAULT_CANDIDATES : 0;\n>> +\treturn 0;\n>> +}\n>\n> I wanted to call out a style question here. The \"opt\" parameter is\n> unused, since it manipulates the \"max_candidates\" global directly. I can\n> add an UNUSED annotation to satisfy -Wunused-parameter, but in such\n> cases I've usually been modifying them like:\n>\n> int *value = opt->value;\n> ...\n> *value = unset ? DEFAULT_CANDIDATES : 0;\n>\n> so that the callback operates on the value passed in the options list.\n\nYeah, I wasn't paying attention to that particular detail, but I\ntend to think that using opt->value to point at the variable would\nmake sense here. That approach matches what is internally done by\nOPT_STRING_LIST() and OPT_STRVEC(), the built-in types that are\nimplemented via the same OPTION_CALLBACK mechanism.\n\nOne good thing about it is that it makes it clear, without looking\nat the implementation of the callback function, which variables are\naffected only by looking at what OPT_CALLBACK() says.\n\n> So before I sent a patch (either to switch to using opt->value, or to\n> add an UNUSED annotation), I wanted to see what you (or others) thought\n> between the two. I.e., should we have a rule of \"try not to operate on\n> global data via option callbacks\" or is that just being too pedantic for\n> one-off callbacks like this?\n\nSo, that was my preference, but I may be missing some obvious\ndownsides. I am interested in hearing from René, who often shows\nbetter taste than I do in these cases ;-)\n\nThanks.\n"},{"id":"480361","messageId":"20230809140902.GA775795@coredump.intra.peff.net","threadId":"59999","inReplyTo":"xmqqzg3156sy.fsf@gitster.g","subject":"Re: [PATCH] describe: fix --no-exact-match","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-09T14:09:02Z","receivedAt":"2023-08-09T14:09:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 08, 2023 at 06:43:41PM -0700, Junio C Hamano wrote:\n\n> > So before I sent a patch (either to switch to using opt->value, or to\n> > add an UNUSED annotation), I wanted to see what you (or others) thought\n> > between the two. I.e., should we have a rule of \"try not to operate on\n> > global data via option callbacks\" or is that just being too pedantic for\n> > one-off callbacks like this?\n> \n> So, that was my preference, but I may be missing some obvious\n> downsides. I am interested in hearing from René, who often shows\n> better taste than I do in these cases ;-)\n\nMe too. :) The main downsides, I think, are:\n\n 1. It's a little more ugly.\n\n 2. We lose type safety, as the variable address passes through a void\n pointer (but that is true of all option callbacks).\n\nHere's what it looks like, for reference.\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex b28a4a1f82..718b5c3073 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -561,9 +561,11 @@ static void describe(const char *arg, int last_one)\n static int option_parse_exact_match(const struct option *opt, const char *arg,\n \t\t\t\t int unset)\n {\n+\tint *val = opt->value;\n+\n \tBUG_ON_OPT_ARG(arg);\n \n-\tmax_candidates = unset ? DEFAULT_CANDIDATES : 0;\n+\t*val = unset ? DEFAULT_CANDIDATES : 0;\n \treturn 0;\n }\n \n@@ -578,7 +580,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"long\", &longformat, N_(\"always use long format\")),\n \t\tOPT_BOOL(0, \"first-parent\", &first_parent, N_(\"only follow first parent\")),\n \t\tOPT__ABBREV(&abbrev),\n-\t\tOPT_CALLBACK_F(0, \"exact-match\", NULL, NULL,\n+\t\tOPT_CALLBACK_F(0, \"exact-match\", &max_candidates, NULL,\n \t\t\t N_(\"only output exact matches\"),\n \t\t\t PARSE_OPT_NOARG, option_parse_exact_match),\n \t\tOPT_INTEGER(0, \"candidates\", &max_candidates,\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex d2a162d528..74c2225620 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4120,12 +4120,14 @@ static void add_extra_kept_packs(const struct string_list *names)\n static int option_parse_quiet(const struct option *opt, const char *arg,\n \t\t\t int unset)\n {\n+\tint *val = opt->value;\n+\n \tBUG_ON_OPT_ARG(arg);\n \n \tif (!unset)\n-\t\tprogress = 0;\n-\telse if (!progress)\n-\t\tprogress = 1;\n+\t\t*val = 0;\n+\telse if (!*val)\n+\t\t*val = 1;\n \treturn 0;\n }\n \n@@ -4190,7 +4192,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tLIST_OBJECTS_FILTER_INIT;\n \n \tstruct option pack_objects_options[] = {\n-\t\tOPT_CALLBACK_F('q', \"quiet\", NULL, NULL,\n+\t\tOPT_CALLBACK_F('q', \"quiet\", &progress, NULL,\n \t\t\t N_(\"do not show progress meter\"),\n \t\t\t PARSE_OPT_NOARG, option_parse_quiet),\n \t\tOPT_SET_INT(0, \"progress\", &progress,\n"},{"id":"480363","messageId":"22e5a87a-cd35-9793-5b6f-6eb368fdf40e@web.de","threadId":"59999","inReplyTo":"20230809140902.GA775795@coredump.intra.peff.net","subject":"Re: [PATCH] describe: fix --no-exact-match","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-08-09T16:41:13Z","receivedAt":"2023-08-09T16:41:28Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.08.23 um 16:09 schrieb Jeff King:\n> On Tue, Aug 08, 2023 at 06:43:41PM -0700, Junio C Hamano wrote:\n>\n>>> So before I sent a patch (either to switch to using opt->value, or to\n>>> add an UNUSED annotation), I wanted to see what you (or others) thought\n>>> between the two. I.e., should we have a rule of \"try not to operate on\n>>> global data via option callbacks\" or is that just being too pedantic for\n>>> one-off callbacks like this?\n>>\n>> So, that was my preference, but I may be missing some obvious\n>> downsides. I am interested in hearing from René, who often shows\n>> better taste than I do in these cases ;-)\n>\n> Me too. :)\n\nI'm pretty sure I didn't think much about it, copied the style from the\nother callback functions in builtin/pack-objects.c and was glad to not\nhave to deal with void pointers.\n\nAnd sorry for the unused parameter warning. Just checked; there are\n170+ of those remaining before we can enable it in developer mode. :-/\nSeems worthwhile, though, especially not slapping UNUSED blindly on\nthem.\n\n> The main downsides, I think, are:\n>\n> 1. It's a little more ugly.\n>\n> 2. We lose type safety, as the variable address passes through a void\n> pointer (but that is true of all option callbacks).\n\nAn upside is that we can reuse the callback if we are careful to wire\nit up to a variable of the correct type. Another is that we can use\nit on local variables.\n\nHmm, we could make these callbacks type-safe fairly easily by adding\npointers of all relevant types to struct option, like \"int *value_int\".\nHow many types would we need?\n\n $ git grep -h ' = opt->value;' | sed 's/\\*.*$//; s/^ *//' | sort -u | wc -l\n 37\n\nOh. Do we really need all those? Anyway, if we added at least the\nmost common ones, we'd be better off already, I'd expect:\n\n $ % git grep -h ' = opt->value;' | sed 's/\\*.*$//; s/^ *//' | sort | uniq -c | sort -nr | head -10\n 29 struct diff_options\n 12 int\n 7 struct grep_opt\n 6 struct rebase_options\n 6 struct apply_state\n 5 struct strbuf\n 5 char\n 4 struct note_data\n 3 struct string_list\n 2 struct strvec\n\nIncreasing the size of the struct like that would increase the total\nmemory footprint by a few KB at most -- tolerable.\n\n> Here's what it looks like, for reference.\n>\n> diff --git a/builtin/describe.c b/builtin/describe.c\n> index b28a4a1f82..718b5c3073 100644\n> --- a/builtin/describe.c\n> +++ b/builtin/describe.c\n> @@ -561,9 +561,11 @@ static void describe(const char *arg, int last_one)\n> static int option_parse_exact_match(const struct option *opt, const char *arg,\n> \t\t\t\t int unset)\n> {\n> +\tint *val = opt->value;\n\nThis line would assign opt->value_int instead...\n\n> +\n> \tBUG_ON_OPT_ARG(arg);\n>\n> -\tmax_candidates = unset ? DEFAULT_CANDIDATES : 0;\n> +\t*val = unset ? DEFAULT_CANDIDATES : 0;\n> \treturn 0;\n> }\n>\n> @@ -578,7 +580,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n> \t\tOPT_BOOL(0, \"long\", &longformat, N_(\"always use long format\")),\n> \t\tOPT_BOOL(0, \"first-parent\", &first_parent, N_(\"only follow first parent\")),\n> \t\tOPT__ABBREV(&abbrev),\n> -\t\tOPT_CALLBACK_F(0, \"exact-match\", NULL, NULL,\n> +\t\tOPT_CALLBACK_F(0, \"exact-match\", &max_candidates, NULL,\n\n... but the macro OP_CALLBACK_F could no longer be used, because we'd\nneed to select one of the many typed pointers.\n\nMacros like OPT_BOOL could be changed to use the appropriate typed\npointer.\n\nThoughts? Too much churn?\n\n> \t\t\t N_(\"only output exact matches\"),\n> \t\t\t PARSE_OPT_NOARG, option_parse_exact_match),\n> \t\tOPT_INTEGER(0, \"candidates\", &max_candidates,\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index d2a162d528..74c2225620 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -4120,12 +4120,14 @@ static void add_extra_kept_packs(const struct string_list *names)\n> static int option_parse_quiet(const struct option *opt, const char *arg,\n> \t\t\t int unset)\n> {\n> +\tint *val = opt->value;\n> +\n> \tBUG_ON_OPT_ARG(arg);\n>\n> \tif (!unset)\n> -\t\tprogress = 0;\n> -\telse if (!progress)\n> -\t\tprogress = 1;\n> +\t\t*val = 0;\n> +\telse if (!*val)\n> +\t\t*val = 1;\n> \treturn 0;\n> }\n>\n> @@ -4190,7 +4192,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> \t\tLIST_OBJECTS_FILTER_INIT;\n>\n> \tstruct option pack_objects_options[] = {\n> -\t\tOPT_CALLBACK_F('q', \"quiet\", NULL, NULL,\n> +\t\tOPT_CALLBACK_F('q', \"quiet\", &progress, NULL,\n> \t\t\t N_(\"do not show progress meter\"),\n> \t\t\t PARSE_OPT_NOARG, option_parse_quiet),\n> \t\tOPT_SET_INT(0, \"progress\", &progress,\n"},{"id":"480387","messageId":"xmqqzg30m3vo.fsf@gitster.g","threadId":"59999","inReplyTo":"22e5a87a-cd35-9793-5b6f-6eb368fdf40e@web.de","subject":"Re: [PATCH] describe: fix --no-exact-match","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-09T19:07:07Z","receivedAt":"2023-08-09T19:07:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> Here's what it looks like, for reference.\n>>\n>> diff --git a/builtin/describe.c b/builtin/describe.c\n>> index b28a4a1f82..718b5c3073 100644\n>> --- a/builtin/describe.c\n>> +++ b/builtin/describe.c\n>> @@ -561,9 +561,11 @@ static void describe(const char *arg, int last_one)\n>> static int option_parse_exact_match(const struct option *opt, const char *arg,\n>> \t\t\t\t int unset)\n>> {\n>> +\tint *val = opt->value;\n>\n> This line would assign opt->value_int instead...\n>\n>> +\n>> \tBUG_ON_OPT_ARG(arg);\n>>\n>> -\tmax_candidates = unset ? DEFAULT_CANDIDATES : 0;\n>> +\t*val = unset ? DEFAULT_CANDIDATES : 0;\n>> \treturn 0;\n>> }\n>> ...\n> Thoughts? Too much churn?\n\nSorry, but I am not sure what is being proposed. opt->value_int\nwould presumably be defined as a pointer to an int. The calling\nside presumably defines the options[] array entry so that the\n&max_candidates address is given to opt->value_int and not to\nopt->value_someothertype, and if you try to pass a pointer of a\nwrong type, it would be caught as a type mismatch by the compiler,\nso the above covers both ends (i.e. the address &max_candidates is\npassed in the correct member of the struct, and the callback takes\nval out of the correct member of the struct), as long as the\ncallback uses the right type. If &max_candidates is int now, and\nthe above code is written to use opt->value_int, anybody who needs\nto update max_candidates to ulong must make sure three things match,\ni.e. the type of max_candidates variable itself, the member the\ncaller uses to pass &max_candidates in the struct (i.e. change to\nuse value_ulong and not value_int), and the type of the local\nvariable the callback function uses.\n\nBut I am failing to imagine how the calling side actually would look\nlike. Can we do something along the lines of\n\n #define OPT_CALLBACK(s, l, v, a, h, cb)\n\t.short_name = (s),\n\t.long_name = (l),\n\t.value_ ## typeof(v) = &v,\n\t.help = (h),\n\t.callback = (cb),\n\nwith a clever CPP trick? It sounds like either too much churn or\ntoo much magic or both, at least to me.\n\nThanks.\n"},{"id":"480415","messageId":"20230810002638.GC795985@coredump.intra.peff.net","threadId":"59999","inReplyTo":"xmqqzg30m3vo.fsf@gitster.g","subject":"Re: [PATCH] describe: fix --no-exact-match","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-10T00:26:38Z","receivedAt":"2023-08-10T00:26:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 09, 2023 at 12:07:07PM -0700, Junio C Hamano wrote:\n\n> But I am failing to imagine how the calling side actually would look\n> like. Can we do something along the lines of\n> \n> #define OPT_CALLBACK(s, l, v, a, h, cb)\n> \t.short_name = (s),\n> \t.long_name = (l),\n> \t.value_ ## typeof(v) = &v,\n> \t.help = (h),\n> \t.callback = (cb),\n> \n> with a clever CPP trick? It sounds like either too much churn or\n> too much magic or both, at least to me.\n\nSadly, I think \"typeof\" is not sufficiently portable, and there is no\nreplacement. You'd have to do something like:\n\n int my_foo;\n ...\n OPT_CALLBACK('f', \"foo\", int, &my_foo, ...etc);\n\nNot great, but it might not be _too_ bad given that most helpers like\nOPT_BOOL() can just say \"int\" behind the scenes.\n\nThat said, I don't recall these void pointers being a large source of\nerrors in the past. So while it's a fun type-system puzzle, the\neffort/reward ratio might not be favorable.\n\n-Peff\n"},{"id":"480417","messageId":"20230810004127.GD795985@coredump.intra.peff.net","threadId":"59999","inReplyTo":"22e5a87a-cd35-9793-5b6f-6eb368fdf40e@web.de","subject":"Re: [PATCH] describe: fix --no-exact-match","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-10T00:41:27Z","receivedAt":"2023-08-10T00:41:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 09, 2023 at 06:41:13PM +0200, René Scharfe wrote:\n\n> And sorry for the unused parameter warning. Just checked; there are\n> 170+ of those remaining before we can enable it in developer mode. :-/\n> Seems worthwhile, though, especially not slapping UNUSED blindly on\n> them.\n\nI know, I'm working on it. :) There were more than 800 when I started.\nI'm hoping to send out the final patches during this 2.43 cycle.\n\n> Oh. Do we really need all those? Anyway, if we added at least the\n> most common ones, we'd be better off already, I'd expect:\n> \n> $ % git grep -h ' = opt->value;' | sed 's/\\*.*$//; s/^ *//' | sort | uniq -c | sort -nr | head -10\n> 29 struct diff_options\n> 12 int\n> 7 struct grep_opt\n> 6 struct rebase_options\n> 6 struct apply_state\n> 5 struct strbuf\n> 5 char\n> 4 struct note_data\n> 3 struct string_list\n> 2 struct strvec\n> \n> Increasing the size of the struct like that would increase the total\n> memory footprint by a few KB at most -- tolerable.\n\nHmm, I was thinking that \"int_value\" might not be so bad. But it seems\nlike a pretty bad layering violation for parse-options.c to know about\n\"struct apply_state\" and so on. That really is what void pointers are\nfor.\n\nAs for size, you should be able to use a union. All of the types inside\nthe struct are pointers, so they'd all be the same size. Of course then\nyou lose some of the safety. If the caller assigned to \"int_value\" that\nis distinct from \"foo_value\", then you can be sure you will get a NULL\nand segfault upon accessing foo_value. With a union, you get whatever\ntype-punning undefined behavior the compiler sees fit. And the point is\nmaking sure the two match.\n\nWe really don't care about separate storage, though. We want type\nsafety. Maybe an option there would be to let the callback function take\nthe appropriate type, and cast it. I.e., something like:\n\n /* define a callback taking the real type */\n int my_int_opt(int *value, struct option *opt, ...etc...) { ...body... }\n\n /* when mentioning a callback, include the type, and make sure that\n * the value and the callback both match it */\n #define OPT_CALLBACK(s, l, type, v, cb, ...etc...) \\\n { ..., \\\n value.type = (v), \\\n callback = (int (*)(type *, struct option *opt, ...etc...), \\\n }\n ...\n OPT_CALLBACK('f', \"foo\", int, &my_local_int, my_int_opt)\n\nI'm pretty sure that falls afoul of the standard, though, because we\neventually need to call that function, and we'll do so through a\nfunction pointer that doesn't match its declaration.\n\nI'm not sure there's a portable and non-insane way of doing what we want\nhere. At least at compile-time. At run-time you could record type\ninformation in an enum and check it in the callback. That seems like\na lot of work for little reward, though.\n\n> > Here's what it looks like, for reference.\n> >\n> > diff --git a/builtin/describe.c b/builtin/describe.c\n> > index b28a4a1f82..718b5c3073 100644\n> > --- a/builtin/describe.c\n> > +++ b/builtin/describe.c\n> > @@ -561,9 +561,11 @@ static void describe(const char *arg, int last_one)\n> > static int option_parse_exact_match(const struct option *opt, const char *arg,\n> > \t\t\t\t int unset)\n> > {\n> > +\tint *val = opt->value;\n> \n> This line would assign opt->value_int instead...\n\nRight, though you would not even need it. I snuck it in there because it\ngives us an implicit cast from the void pointer. Without it, the current\ncode would have to do:\n\n *(int *)opt->value = unset ? DEFAULT_CANDIDATES : 0;\n\nwhich was too ugly (especially the \"progress\" one which mentions the\nvalue three times). But if you had pre-cast variables, you could do:\n\n *opt->value.int = unset ? DEFAULT_CANDIDATES : 0;\n\ndirectly.\n\n-Peff\n"},{"id":"480420","messageId":"xmqqo7jfk8xq.fsf@gitster.g","threadId":"59999","inReplyTo":"20230810002638.GC795985@coredump.intra.peff.net","subject":"Re: [PATCH] describe: fix --no-exact-match","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-10T01:00:49Z","receivedAt":"2023-08-10T01:00:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> int my_foo;\n> ...\n> OPT_CALLBACK('f', \"foo\", int, &my_foo, ...etc);\n>\n> Not great, but it might not be _too_ bad given that most helpers like\n> OPT_BOOL() can just say \"int\" behind the scenes.\n>\n> That said, I don't recall these void pointers being a large source of\n> errors in the past. So while it's a fun type-system puzzle, the\n> effort/reward ratio might not be favorable.\n\nI tend to agree on both counts, it is a fun puzzle, and it probably\nis not going to give us sufficient reward. The fact that \"int\" and\ntypeof(my_foo) need to be manually kept straight defeats the \"type\nsafety\" justification for this mental exercise.\n\nThanks.\n"},{"id":"480463","messageId":"09f499ad-28a5-0d8b-8af6-97475bdc614b@web.de","threadId":"59999","inReplyTo":"20230810004127.GD795985@coredump.intra.peff.net","subject":"Re: [PATCH] describe: fix --no-exact-match","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-08-10T19:10:33Z","receivedAt":"2023-08-10T19:10:48Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"\n\nAm 10.08.23 um 02:41 schrieb Jeff King:\n> On Wed, Aug 09, 2023 at 06:41:13PM +0200, René Scharfe wrote:\n>\n>> And sorry for the unused parameter warning. Just checked; there are\n>> 170+ of those remaining before we can enable it in developer mode. :-/\n>> Seems worthwhile, though, especially not slapping UNUSED blindly on\n>> them.\n>\n> I know, I'm working on it. :) There were more than 800 when I started.\n> I'm hoping to send out the final patches during this 2.43 cycle.\n>\n>> Oh. Do we really need all those? Anyway, if we added at least the\n>> most common ones, we'd be better off already, I'd expect:\n>>\n>> $ % git grep -h ' = opt->value;' | sed 's/\\*.*$//; s/^ *//' | sort | uniq -c | sort -nr | head -10\n>> 29 struct diff_options\n>> 12 int\n>> 7 struct grep_opt\n>> 6 struct rebase_options\n>> 6 struct apply_state\n>> 5 struct strbuf\n>> 5 char\n>> 4 struct note_data\n>> 3 struct string_list\n>> 2 struct strvec\n>>\n>> Increasing the size of the struct like that would increase the total\n>> memory footprint by a few KB at most -- tolerable.\n>\n> Hmm, I was thinking that \"int_value\" might not be so bad. But it seems\n> like a pretty bad layering violation for parse-options.c to know about\n> \"struct apply_state\" and so on. That really is what void pointers are\n> for.\n\nTrue, keeping a list of external types in parse-options.h is clumsy and\nugly.\n\n> As for size, you should be able to use a union. All of the types inside\n> the struct are pointers, so they'd all be the same size. Of course then\n> you lose some of the safety. If the caller assigned to \"int_value\" that\n> is distinct from \"foo_value\", then you can be sure you will get a NULL\n> and segfault upon accessing foo_value. With a union, you get whatever\n> type-punning undefined behavior the compiler sees fit. And the point is\n> making sure the two match.\n\nWhich makes a union a non-starter -- we need a reliable error.\n\n> We really don't care about separate storage, though. We want type\n> safety. Maybe an option there would be to let the callback function take\n> the appropriate type, and cast it. I.e., something like:\n>\n> /* define a callback taking the real type */\n> int my_int_opt(int *value, struct option *opt, ...etc...) { ...body... }\n>\n> /* when mentioning a callback, include the type, and make sure that\n> * the value and the callback both match it */\n> #define OPT_CALLBACK(s, l, type, v, cb, ...etc...) \\\n> { ..., \\\n> value.type = (v), \\\n> callback = (int (*)(type *, struct option *opt, ...etc...), \\\n> }\n> ...\n> OPT_CALLBACK('f', \"foo\", int, &my_local_int, my_int_opt)\n\nThe idea is good, even though I don't understand how your specific\nexample would work. Dabbled with something similar for typed qsort\n(need to clean it up and submit it one of these days).\n\n> I'm pretty sure that falls afoul of the standard, though, because we\n> eventually need to call that function, and we'll do so through a\n> function pointer that doesn't match its declaration.\n\nFighting possible type mismatches by adding a certain type mismatch\nwon't fly..\n\n> I'm not sure there's a portable and non-insane way of doing what we want\n> here. At least at compile-time.\n\nWe need a wrapper with the correct signature. The wrapper is plugged\ninto struct option. The typed callback is called by the wrapper and\ncan be used for a type check in the struct macro. Demo patch below.\n\n\n> At run-time you could record type\n> information in an enum and check it in the callback. That seems like\n> a lot of work for little reward, though.\n\nAgreed -- runtime type checks are for scripting languages..\n\nRené\n\n\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex b28a4a1f82..ce16c36de2 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -558,15 +558,17 @@ static void describe(const char *arg, int last_one)\n \tstrbuf_release(&sb);\n }\n\n-static int option_parse_exact_match(const struct option *opt, const char *arg,\n-\t\t\t\t int unset)\n+static int option_parse_exact_match(const struct option *opt UNUSED,\n+\t\t\t\t const char *arg, int unset, int *value)\n {\n \tBUG_ON_OPT_ARG(arg);\n\n-\tmax_candidates = unset ? DEFAULT_CANDIDATES : 0;\n+\t*value = unset ? DEFAULT_CANDIDATES : 0;\n \treturn 0;\n }\n\n+DEFINE_PARSE_OPT_CB(option_parse_exact_match);\n+\n int cmd_describe(int argc, const char **argv, const char *prefix)\n {\n \tint contains = 0;\n@@ -578,9 +580,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"long\", &longformat, N_(\"always use long format\")),\n \t\tOPT_BOOL(0, \"first-parent\", &first_parent, N_(\"only follow first parent\")),\n \t\tOPT__ABBREV(&abbrev),\n-\t\tOPT_CALLBACK_F(0, \"exact-match\", NULL, NULL,\n-\t\t\t N_(\"only output exact matches\"),\n-\t\t\t PARSE_OPT_NOARG, option_parse_exact_match),\n+\t\tOPT_CALLBACK_F_T(0, \"exact-match\", &max_candidates, NULL,\n+\t\t\t\t N_(\"only output exact matches\"),\n+\t\t\t\t PARSE_OPT_NOARG, option_parse_exact_match),\n \t\tOPT_INTEGER(0, \"candidates\", &max_candidates,\n \t\t\t N_(\"consider most recent tags (default: 10)\")),\n \t\tOPT_STRING_LIST(0, \"match\", &patterns, N_(\"pattern\"),\ndiff --git a/parse-options.h b/parse-options.h\nindex 57a7fe9d91..f957931cfa 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -67,6 +67,14 @@ enum parse_opt_result {\n struct option;\n typedef int parse_opt_cb(const struct option *, const char *arg, int unset);\n\n+#define DEFINE_PARSE_OPT_CB(name)\t\t\t\t\\\n+static inline int name ## __void(const struct option *opt,\t\\\n+\t\t\t\t const char *arg, int unset)\t\\\n+{\t\t\t\t\t\t\t\t\\\n+\treturn name(opt, arg, unset, opt->value);\t\t\\\n+}\t\t\t\t\t\t\t\t\\\n+struct option\n+\n struct parse_opt_ctx_t;\n typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,\n \t\t\t\t\t const struct option *opt,\n@@ -198,6 +206,16 @@ struct option {\n \t.flags = (f), \\\n \t.callback = (cb), \\\n }\n+#define OPT_CALLBACK_F_T(s, l, v, a, h, f, cb) { \\\n+\t.type = OPTION_CALLBACK, \\\n+\t.short_name = (s) + (0 ? cb(NULL, NULL, 0, (v)) : 0), \\\n+\t.long_name = (l), \\\n+\t.value = (v), \\\n+\t.argh = (a), \\\n+\t.help = (h), \\\n+\t.flags = (f), \\\n+\t.callback = cb ## __void, \\\n+}\n #define OPT_STRING_F(s, l, v, a, h, f) { \\\n \t.type = OPTION_STRING, \\\n \t.short_name = (s), \\\n"},{"id":"480464","messageId":"b5fcc572-c710-5869-7a40-ba784d8edf0e@web.de","threadId":"59999","inReplyTo":"xmqqo7jfk8xq.fsf@gitster.g","subject":"Re: [PATCH] describe: fix --no-exact-match","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-08-10T19:45:50Z","receivedAt":"2023-08-10T19:46:31Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.08.23 um 03:00 schrieb Junio C Hamano:\n> Jeff King writes:\n>\n>> int my_foo;\n>> ...\n>> OPT_CALLBACK('f', \"foo\", int, &my_foo, ...etc);\n>>\n>> Not great, but it might not be _too_ bad given that most helpers like\n>> OPT_BOOL() can just say \"int\" behind the scenes.\n>>\n>> That said, I don't recall these void pointers being a large source of\n>> errors in the past. So while it's a fun type-system puzzle, the\n>> effort/reward ratio might not be favorable.\n\nI have vague memories of changing a callback from using a string_list\nto a strset (or whatever) and getting crashes out of some other callers\nthat used it non-obviously via some macro. A compile-time check would\nhave helped me.\n\n> I tend to agree on both counts, it is a fun puzzle, and it probably\n> is not going to give us sufficient reward. The fact that \"int\" and\n> typeof(my_foo) need to be manually kept straight defeats the \"type\n> safety\" justification for this mental exercise.\n\nThe many-pointers idea is a bit silly, admittedly.\n\nHaving to declare a type manually is how most of C's type safety works,\nthough, so a certain amount of that is unavoidable.\n\ntypeof (along with typeof_unqual) is nice, but I guess it will take a\nfew years (or decades?) before we can expect it to be supported by all\nrelevant platforms.\n\nThe typed-callback-with-void-wrapper idea seems promising. Let's see\nif we can get it into usable shape.\n\nRené\n"},{"id":"480544","messageId":"20230811151102.GE2303200@coredump.intra.peff.net","threadId":"59999","inReplyTo":"09f499ad-28a5-0d8b-8af6-97475bdc614b@web.de","subject":"Re: [PATCH] describe: fix --no-exact-match","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-11T15:11:02Z","receivedAt":"2023-08-11T15:11:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 10, 2023 at 09:10:33PM +0200, René Scharfe wrote:\n\n> > I'm not sure there's a portable and non-insane way of doing what we want\n> > here. At least at compile-time.\n> \n> We need a wrapper with the correct signature. The wrapper is plugged\n> into struct option. The typed callback is called by the wrapper and\n> can be used for a type check in the struct macro. Demo patch below.\n\nOK, clever. So we have two functions, one with a real body, and the\nother which is used with the void pointer. How do we make sure that the\nreal-body one matches the type passed to OPT_CALLBACK(), if it is only\nseeing the void wrapper? I guess that is this bit in short_name:\n\n> +#define OPT_CALLBACK_F_T(s, l, v, a, h, f, cb) { \\\n> +\t.type = OPTION_CALLBACK, \\\n> +\t.short_name = (s) + (0 ? cb(NULL, NULL, 0, (v)) : 0), \\\n\nwhich would cause the compiler to barf, and presumably eliminate the\ndead code (or at the very least never call it at runtime).\n\nSo I think that works. Though...\n\n> +#define DEFINE_PARSE_OPT_CB(name)\t\t\t\t\\\n> +static inline int name ## __void(const struct option *opt,\t\\\n> +\t\t\t\t const char *arg, int unset)\t\\\n> +{\t\t\t\t\t\t\t\t\\\n> +\treturn name(opt, arg, unset, opt->value);\t\t\\\n> +}\t\t\t\t\t\t\t\t\\\n\nwe are defining an inline function with the explicit goal of passing it\nas a function pointer. I don't remember all of the standard's rules\nhere. Are we guaranteed that it will create a linkable version if\nnecessary?\n\nWe could probably drop the \"inline\" (and perhaps would need to add\nMAYBE_UNUSED in such a case).\n\n> diff --git a/builtin/describe.c b/builtin/describe.c\n> index b28a4a1f82..ce16c36de2 100644\n> --- a/builtin/describe.c\n> +++ b/builtin/describe.c\n> @@ -558,15 +558,17 @@ static void describe(const char *arg, int last_one)\n> \tstrbuf_release(&sb);\n> }\n> \n> -static int option_parse_exact_match(const struct option *opt, const char *arg,\n> -\t\t\t\t int unset)\n> +static int option_parse_exact_match(const struct option *opt UNUSED,\n> +\t\t\t\t const char *arg, int unset, int *value)\n> {\n> \tBUG_ON_OPT_ARG(arg);\n> \n> -\tmax_candidates = unset ? DEFAULT_CANDIDATES : 0;\n> +\t*value = unset ? DEFAULT_CANDIDATES : 0;\n> \treturn 0;\n> }\n> \n> +DEFINE_PARSE_OPT_CB(option_parse_exact_match);\n\nI wondered about combining these, like:\n\n DEFINE_PARSE_OPT_CB(option_parse_exact_match, int) {\n\t...the real body here...\n }\n\nBut I guess that may confuse non-compiler parsers, and it doesn't leave\nroom for annotations like the UNUSED above (which ironically is still\nneeded, since now we pass opt->value as its own parameter).\n\nSo I dunno. Clever, for sure, and I think it would work. I'm not sure if\nthe extra code merits the return or not.\n\n-Peff\n"},{"id":"480545","messageId":"20230811151323.GF2303200@coredump.intra.peff.net","threadId":"59999","inReplyTo":"09f499ad-28a5-0d8b-8af6-97475bdc614b@web.de","subject":"Re: [PATCH] describe: fix --no-exact-match","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-11T15:13:23Z","receivedAt":"2023-08-11T15:13:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 10, 2023 at 09:10:33PM +0200, René Scharfe wrote:\n\n> +#define DEFINE_PARSE_OPT_CB(name)\t\t\t\t\\\n> +static inline int name ## __void(const struct option *opt,\t\\\n> +\t\t\t\t const char *arg, int unset)\t\\\n> +{\t\t\t\t\t\t\t\t\\\n> +\treturn name(opt, arg, unset, opt->value);\t\t\\\n> +}\t\t\t\t\t\t\t\t\\\n> +struct option\n\nBTW, I wondered what this extra \"struct option\" was doing. I guess it is\nthere to soak up the semi-colon of:\n\n DEFINE_PARSE_OPT_CB(foo);\n\nwith a noop declaration. Both clever and gross. :)\n\n-Peff\n"},{"id":"480566","messageId":"fbeadf7d-c16f-0612-6256-fc4355e89638@web.de","threadId":"59999","inReplyTo":"20230811151102.GE2303200@coredump.intra.peff.net","subject":"Re: [PATCH] describe: fix --no-exact-match","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-08-11T17:59:12Z","receivedAt":"2023-08-11T17:59:27Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.08.23 um 17:11 schrieb Jeff King:\n> On Thu, Aug 10, 2023 at 09:10:33PM +0200, René Scharfe wrote:\n>\n>>> I'm not sure there's a portable and non-insane way of doing what we want\n>>> here. At least at compile-time.\n>>\n>> We need a wrapper with the correct signature. The wrapper is plugged\n>> into struct option. The typed callback is called by the wrapper and\n>> can be used for a type check in the struct macro. Demo patch below.\n>\n> OK, clever. So we have two functions, one with a real body, and the\n> other which is used with the void pointer. How do we make sure that the\n> real-body one matches the type passed to OPT_CALLBACK(), if it is only\n> seeing the void wrapper? I guess that is this bit in short_name:\n>\n>> +#define OPT_CALLBACK_F_T(s, l, v, a, h, f, cb) { \\\n>> +\t.type = OPTION_CALLBACK, \\\n>> +\t.short_name = (s) + (0 ? cb(NULL, NULL, 0, (v)) : 0), \\\n>\n> which would cause the compiler to barf, and presumably eliminate the\n> dead code (or at the very least never call it at runtime).\n\nYes, a fake call that doesn't make it into the generated object code,\nbut is real enough for type checking.\n\n> So I think that works. Though...\n>\n>> +#define DEFINE_PARSE_OPT_CB(name)\t\t\t\t\\\n>> +static inline int name ## __void(const struct option *opt,\t\\\n>> +\t\t\t\t const char *arg, int unset)\t\\\n>> +{\t\t\t\t\t\t\t\t\\\n>> +\treturn name(opt, arg, unset, opt->value);\t\t\\\n>> +}\t\t\t\t\t\t\t\t\\\n>\n> we are defining an inline function with the explicit goal of passing it\n> as a function pointer. I don't remember all of the standard's rules\n> here. Are we guaranteed that it will create a linkable version if\n> necessary?\n\nI don't see on which basis the compiler could refuse. We can't expect\nthe function address to be the same across compilation units, but we\ndon't need that. If there's a compiler that won't do it or a standards\nverse that makes this dubious then I'd like to know very much.\n\n> We could probably drop the \"inline\" (and perhaps would need to add\n> MAYBE_UNUSED in such a case).\n\nYes, we can use a non-inline function as well, but then we'd need to\ndefine it once somewhere and declare it separately, though, which is\ninconvenient. It needs to have the same visibility as the typed\nfunction.\n\nWe better not use MAYBE_UNUSED because the wrapper is the actually\ninvoked callback.\n\n>> diff --git a/builtin/describe.c b/builtin/describe.c\n>> index b28a4a1f82..ce16c36de2 100644\n>> --- a/builtin/describe.c\n>> +++ b/builtin/describe.c\n>> @@ -558,15 +558,17 @@ static void describe(const char *arg, int last_one)\n>> \tstrbuf_release(&sb);\n>> }\n>>\n>> -static int option_parse_exact_match(const struct option *opt, const char *arg,\n>> -\t\t\t\t int unset)\n>> +static int option_parse_exact_match(const struct option *opt UNUSED,\n>> +\t\t\t\t const char *arg, int unset, int *value)\n>> {\n>> \tBUG_ON_OPT_ARG(arg);\n>>\n>> -\tmax_candidates = unset ? DEFAULT_CANDIDATES : 0;\n>> +\t*value = unset ? DEFAULT_CANDIDATES : 0;\n>> \treturn 0;\n>> }\n>>\n>> +DEFINE_PARSE_OPT_CB(option_parse_exact_match);\n>\n> I wondered about combining these, like:\n>\n> DEFINE_PARSE_OPT_CB(option_parse_exact_match, int) {\n> \t...the real body here...\n> }\n>\n> But I guess that may confuse non-compiler parsers, and it doesn't leave\n> room for annotations like the UNUSED above (which ironically is still\n> needed, since now we pass opt->value as its own parameter).\n\nI get the idea. Something that looks like a function attribute or\ndecorator. I don't see no nice way to achieve that, though.\n\n> So I dunno. Clever, for sure, and I think it would work. I'm not sure if\n> the extra code merits the return or not.\n\nSure, why check types -- script languages get work done as well. (I'm\nfresh off a Python basics training, nice quirky language..) But we're\nin C land and static typing is supposed to help us get our operations\ncorrect and portable.\n\nA good example in parseopt: The patch below adds type checking to the\nint options and yields 79 warning about incompatible pointers, because\nenum pointers were used in integer option definitions. The storage size\nof enums depends on the member values and the compiler; an enum could be\nchar-sized. When we access such a thing with an int pointer we write up\nto seven bytes of garbage ... somewhere. We better fix that.\n\nIt also gives me 73 sign warnings because we like to use unsigned int to\nstore int options for some reason. No problem storage-wise. I have to\nadmit that I don't really understand why -Wpointer-sign is in -Wall and\nnot only in -Wpedantic. But it could mean a user gives a negative value\nfor something and we interpret it as a huge value. Probably still worth\nfixing to avoid the general weirdness.\n\nMy point is: If we don't check types we deviate. It just happens.\n\nRené\n\n\nPS: We could simplify the type check like this:\n\n\t.short_name = (s) + (0 && (v) == (int *)0), \\\n\nThis yields -Wcompare-distinct-pointer-types for all 152 cases, which\nmay suffice. But comparisons of pointers are only defined for those\nthat point to the same object or just past it if I'm not mistaken, so\nthis way works only by accident and the comparison part of the warning\nis misleading anyway. Sill looking for a simpler type check..\n\n\n\ndiff --git a/parse-options.h b/parse-options.h\nindex f957931cfa..dafea28b55 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -168,9 +168,14 @@ struct option {\n \tparse_opt_subcommand_fn *subcommand_fn;\n };\n\n+static inline int parse_options_noop_int_ptr(int *p UNUSED)\n+{\n+\treturn 0;\n+}\n+\n #define OPT_BIT_F(s, l, v, h, b, f) { \\\n \t.type = OPTION_BIT, \\\n-\t.short_name = (s), \\\n+\t.short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \\\n \t.long_name = (l), \\\n \t.value = (v), \\\n \t.help = (h), \\\n@@ -180,7 +185,7 @@ struct option {\n }\n #define OPT_COUNTUP_F(s, l, v, h, f) { \\\n \t.type = OPTION_COUNTUP, \\\n-\t.short_name = (s), \\\n+\t.short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \\\n \t.long_name = (l), \\\n \t.value = (v), \\\n \t.help = (h), \\\n@@ -188,7 +193,7 @@ struct option {\n }\n #define OPT_SET_INT_F(s, l, v, h, i, f) { \\\n \t.type = OPTION_SET_INT, \\\n-\t.short_name = (s), \\\n+\t.short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \\\n \t.long_name = (l), \\\n \t.value = (v), \\\n \t.help = (h), \\\n@@ -227,7 +232,7 @@ struct option {\n }\n #define OPT_INTEGER_F(s, l, v, h, f) { \\\n \t.type = OPTION_INTEGER, \\\n-\t.short_name = (s), \\\n+\t.short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \\\n \t.long_name = (l), \\\n \t.value = (v), \\\n \t.argh = N_(\"n\"), \\\n@@ -245,7 +250,7 @@ struct option {\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) { \\\n \t.type = OPTION_BITOP, \\\n-\t.short_name = (s), \\\n+\t.short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \\\n \t.long_name = (l), \\\n \t.value = (v), \\\n \t.help = (h), \\\n@@ -255,7 +260,7 @@ struct option {\n }\n #define OPT_NEGBIT(s, l, v, h, b) { \\\n \t.type = OPTION_NEGBIT, \\\n-\t.short_name = (s), \\\n+\t.short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \\\n \t.long_name = (l), \\\n \t.value = (v), \\\n \t.help = (h), \\\n@@ -267,7 +272,7 @@ struct option {\n #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)\n #define OPT_HIDDEN_BOOL(s, l, v, h) { \\\n \t.type = OPTION_SET_INT, \\\n-\t.short_name = (s), \\\n+\t.short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \\\n \t.long_name = (l), \\\n \t.value = (v), \\\n \t.help = (h), \\\n@@ -276,7 +281,7 @@ struct option {\n }\n #define OPT_CMDMODE_F(s, l, v, h, i, f) { \\\n \t.type = OPTION_SET_INT, \\\n-\t.short_name = (s), \\\n+\t.short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \\\n \t.long_name = (l), \\\n \t.value = (v), \\\n \t.help = (h), \\\n"},{"id":"480567","messageId":"6a6c86cd-9545-f0b3-8a0f-71d29c0b8833@web.de","threadId":"59999","inReplyTo":"20230811151323.GF2303200@coredump.intra.peff.net","subject":"Re: [PATCH] describe: fix --no-exact-match","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-08-11T17:59:15Z","receivedAt":"2023-08-11T17:59:59Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.08.23 um 17:13 schrieb Jeff King:\n> On Thu, Aug 10, 2023 at 09:10:33PM +0200, René Scharfe wrote:\n>\n>> +#define DEFINE_PARSE_OPT_CB(name)\t\t\t\t\\\n>> +static inline int name ## __void(const struct option *opt,\t\\\n>> +\t\t\t\t const char *arg, int unset)\t\\\n>> +{\t\t\t\t\t\t\t\t\\\n>> +\treturn name(opt, arg, unset, opt->value);\t\t\\\n>> +}\t\t\t\t\t\t\t\t\\\n>> +struct option\n>\n> BTW, I wondered what this extra \"struct option\" was doing. I guess it is\n> there to soak up the semi-colon of:\n>\n> DEFINE_PARSE_OPT_CB(foo);\n>\n> with a noop declaration. Both clever and gross. :)\n\nYou guessed correctly. It was short and handy..\n\nA production-grade patch would use the function header.\n\nRené\n"},{"id":"480573","messageId":"20230811182415.GC2816191@coredump.intra.peff.net","threadId":"59999","inReplyTo":"fbeadf7d-c16f-0612-6256-fc4355e89638@web.de","subject":"Re: [PATCH] describe: fix --no-exact-match","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-11T18:24:15Z","receivedAt":"2023-08-11T18:24:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 11, 2023 at 07:59:12PM +0200, René Scharfe wrote:\n\n> > we are defining an inline function with the explicit goal of passing it\n> > as a function pointer. I don't remember all of the standard's rules\n> > here. Are we guaranteed that it will create a linkable version if\n> > necessary?\n> \n> I don't see on which basis the compiler could refuse. We can't expect\n> the function address to be the same across compilation units, but we\n> don't need that. If there's a compiler that won't do it or a standards\n> verse that makes this dubious then I'd like to know very much.\n\nI seem to recall some quirks where an inline function that is not called\ndirectly is not required to be compiled at all, and the compiler can\nassume that there is a definition available in another translation unit.\n\nBut I think that only applies when no storage-class specifier is\nprovided. In this case, you said \"static\", so I think it's OK?\n\nIt's possible I'm mis-remembering the issues, too. One problem is that\npre-C99, you might end up with the opposite problem (a compiled function\nwith visible linkage that conflicts with other translation units at link\ntime). E.g. here:\n\n https://stackoverflow.com/questions/51533082/clarification-over-internal-linkage-of-inline-functions-in-c/51533367#51533367\n\nBut I think with \"static\" we should always be OK.\n\n> > So I dunno. Clever, for sure, and I think it would work. I'm not sure if\n> > the extra code merits the return or not.\n> \n> Sure, why check types -- script languages get work done as well. (I'm\n> fresh off a Python basics training, nice quirky language..) But we're\n> in C land and static typing is supposed to help us get our operations\n> correct and portable.\n\nDon't get me wrong, I like type checking. It's just that doing weird\nthings with the language and pre-processor also carries a cost,\nespecially in an open source project where new folks may show up and say\n\"what the hell is this macro doing?\". That's a friction for new\ndevelopers, even if they're comfortable with idiomatic C.\n\nThat said...\n\n> A good example in parseopt: The patch below adds type checking to the\n> int options and yields 79 warning about incompatible pointers, because\n> enum pointers were used in integer option definitions. The storage size\n> of enums depends on the member values and the compiler; an enum could be\n> char-sized. When we access such a thing with an int pointer we write up\n> to seven bytes of garbage ... somewhere. We better fix that.\n\n...I do find this evidence compelling.\n\n-Peff\n"},{"id":"480622","messageId":"6e05309d-c2be-c949-ae83-c4dd3247e790@web.de","threadId":"59999","inReplyTo":"20230811182415.GC2816191@coredump.intra.peff.net","subject":"Re: [PATCH] describe: fix --no-exact-match","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-08-12T05:11:03Z","receivedAt":"2023-08-12T05:11:21Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.08.23 um 20:24 schrieb Jeff King:\n> On Fri, Aug 11, 2023 at 07:59:12PM +0200, René Scharfe wrote:\n>\n>>> we are defining an inline function with the explicit goal of passing it\n>>> as a function pointer. I don't remember all of the standard's rules\n>>> here. Are we guaranteed that it will create a linkable version if\n>>> necessary?\n>>\n>> I don't see on which basis the compiler could refuse. We can't expect\n>> the function address to be the same across compilation units, but we\n>> don't need that. If there's a compiler that won't do it or a standards\n>> verse that makes this dubious then I'd like to know very much.\n>\n> I seem to recall some quirks where an inline function that is not called\n> directly is not required to be compiled at all, and the compiler can\n> assume that there is a definition available in another translation unit.\n\nC99 says in 6.7.4 Function specifiers: \"It is unspecified whether a call\nto the function uses the inline definition or the external definition.\",\nreferring to functions with both types of definition. So a compiler\ncould ignore the inline version for those.\n\n> But I think that only applies when no storage-class specifier is\n> provided. In this case, you said \"static\", so I think it's OK?\n\nThat's how I understand it as well -- there is no external version to\nchoose and the compiler is not free to ignore the inline one.\n\n> It's possible I'm mis-remembering the issues, too. One problem is that\n> pre-C99, you might end up with the opposite problem (a compiled function\n> with visible linkage that conflicts with other translation units at link\n> time). E.g. here:\n>\n> https://stackoverflow.com/questions/51533082/clarification-over-internal-linkage-of-inline-functions-in-c/51533367#51533367\n>\n> But I think with \"static\" we should always be OK.\n\n*nod*\n\n> Don't get me wrong, I like type checking. It's just that doing weird\n> things with the language and pre-processor also carries a cost,\n> especially in an open source project where new folks may show up and say\n> \"what the hell is this macro doing?\". That's a friction for new\n> developers, even if they're comfortable with idiomatic C.\n\nSure, but that ship has sailed in this specific case. Standard option\nparsing would use getopt or getopt_long, neither of which has void\npointers. We already carry the cost of our OPT_ macros. Let's ease it.\n\n> That said...\n>\n>> A good example in parseopt: The patch below adds type checking to the\n>> int options and yields 79 warning about incompatible pointers, because\n>> enum pointers were used in integer option definitions. The storage size\n>> of enums depends on the member values and the compiler; an enum could be\n>> char-sized. When we access such a thing with an int pointer we write up\n>> to seven bytes of garbage ... somewhere. We better fix that.\n>\n> ...I do find this evidence compelling.\n\nIt's 3 instead of 7 bytes of garbage, but the point still stands..\n\nRené\n"}]}