[PATCH] ls-tree: fix --no-full-name

[PATCH] ls-tree: fix --no-full-name

[René Scharfe]

Since 61fdbcf98b (ls-tree: migrate to parse-options, 2009-11-13) git
ls-tree has accepted the option --no-full-name, but it does the same
as --full-name, contrary to convention.  That's because it's defined
using OPT_SET_INT with a value of 0, where the negative variant sets
0 as well.

Turn --no-full-name into the opposite of --full-name by using OPT_BOOL
instead and storing the option's status directly in a variable named
"full_name" instead of in negated form in "chomp_prefix".

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/ls-tree.c          | 7 +++----
 t/t3101-ls-tree-dirname.sh | 8 ++++++++
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 53073d64cb..f558db5f3b 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -343,7 +343,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	struct object_id oid;
 	struct tree *tree;
 	int i, full_tree = 0;
-	int chomp_prefix = prefix && *prefix;
+	int full_name = !prefix || !*prefix;
 	read_tree_fn_t fn = NULL;
 	enum ls_tree_cmdmode cmdmode = MODE_DEFAULT;
 	int null_termination = 0;
@@ -365,8 +365,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 			    MODE_NAME_STATUS),
 		OPT_CMDMODE(0, "object-only", &cmdmode, N_("list only objects"),
 			    MODE_OBJECT_ONLY),
-		OPT_SET_INT(0, "full-name", &chomp_prefix,
-			    N_("use full path names"), 0),
+		OPT_BOOL(0, "full-name", &full_name, N_("use full path names")),
 		OPT_BOOL(0, "full-tree", &full_tree,
 			 N_("list entire tree; not just current directory "
 			    "(implies --full-name)")),
@@ -387,7 +386,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)

 	if (full_tree)
 		prefix = NULL;
-	options.prefix = chomp_prefix ? prefix : NULL;
+	options.prefix = full_name ? NULL : prefix;

 	/*
 	 * We wanted to detect conflicts between --name-only and
diff --git a/t/t3101-ls-tree-dirname.sh b/t/t3101-ls-tree-dirname.sh
index 217006d1bf..5af2dac0e4 100755
--- a/t/t3101-ls-tree-dirname.sh
+++ b/t/t3101-ls-tree-dirname.sh
@@ -154,6 +154,14 @@ EOF
 	test_output
 '

+test_expect_success 'ls-tree --no-full-name' '
+	git -C path0 ls-tree --no-full-name $tree a >current &&
+	cat >expected <<-EOF &&
+	040000 tree X	a
+	EOF
+	test_output
+'
+
 test_expect_success 'ls-tree --full-tree' '
 	(
 		cd path1/b/c &&
--
2.41.0

Re: [PATCH] ls-tree: fix --no-full-name

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Since 61fdbcf98b (ls-tree: migrate to parse-options, 2009-11-13) git
> ls-tree has accepted the option --no-full-name, but it does the same
> as --full-name, contrary to convention.  That's because it's defined
> using OPT_SET_INT with a value of 0, where the negative variant sets
> 0 as well.

Ouch.  Well spotted.

> Turn --no-full-name into the opposite of --full-name by using OPT_BOOL
> instead and storing the option's status directly in a variable named
> "full_name" instead of in negated form in "chomp_prefix".

Good solution, especially the flipping of the polarity of the
variable is very sensible.

I wonder if there are cases where it makes sense to allow the
"--no-" variant to an option parsed with OPT_SET_INT() that sets '0'
as the value?

Some random findings while reading hits from "git grep OPT_SET_INT":

 * "git am --[no-]keep-cr" is implemented as a pair of explicit
   PARSE_OPT_NONEG entries in the option[] array, but wouldn't it be
   sufficient to have a single OPT_SET_INT("keep-cr")?

 * "git branch --list --no-all" is accepted, sets filter.kind to 0,
   and triggers "fatal: filter_refs: invalid type".  Shouldn't we
   detect error much earlier?

 * "git bundle create --no-quiet" is accepted and sets the progress
   variable to 0, just like "--quiet" does, which is the same issue
   as the one fixed by your patch.

 * "git clone (--no-ipv4|--no-ipv6)" are accepted and uses
   TRANSPORT_FAMILY_ALL, presumably allowing both v4 and v6.
   Shouldn't we reject these?  "fetch" and "push" share the same
   issue.

 * "git status --no-(short|long|porcelain)" are accepted and use
   STATUS_FORMAT_NONE, which probably is OK.

 * "git commit --[no-](short|long|porcelain)" are accepted and
   behave as "git status" without doing any "git commit" thing,
   which should be corrected, I think.

 * "git describe --no-exact-match" is the same as "--exact-match",
   which is the same issue as the one fixed by your patch.

 * "git remote add" has an OPT_SET_INT() entry whose short and long
   forms are (0, NULL).  What is this supposed to do?  Shouldn't
   parse-options.c:parse_options_check() notice it as an error?

 * "git reset --(soft|hard|mixed|merge|keep)" all take the negated
   form and they all become "--mixed".  It may make sense to give
   all of them PARSE_OPT_NONEG.

 * "git show-branch --no-sparse" is the same as "--sparse",
   which is the same issue as the one fixed by your patch.

 * "git show-branch --no-topo-order" is the same as "--topo-order";
   as it is the default, we probably should give PARSE_OPT_NONEG.

 * "git show-branch --no-date-order" is the same as "--topo-order",
   which does sort-of make sense.  This (and the previous one)
   relies on REV_SORT_IN_GRAPH_ORDER enum being 0, which smells a
   bit brittle.

 * "git stash push --no-all" is the same as "--no-include-untracked",
   which smells iffy but probably is OK.

Anyway, the patch looks good.  Will queue.

Thanks.

> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  builtin/ls-tree.c          | 7 +++----
>  t/t3101-ls-tree-dirname.sh | 8 ++++++++
>  2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> index 53073d64cb..f558db5f3b 100644
> --- a/builtin/ls-tree.c
> +++ b/builtin/ls-tree.c
> @@ -343,7 +343,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>  	struct object_id oid;
>  	struct tree *tree;
>  	int i, full_tree = 0;
> -	int chomp_prefix = prefix && *prefix;
> +	int full_name = !prefix || !*prefix;
>  	read_tree_fn_t fn = NULL;
>  	enum ls_tree_cmdmode cmdmode = MODE_DEFAULT;
>  	int null_termination = 0;
> @@ -365,8 +365,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>  			    MODE_NAME_STATUS),
>  		OPT_CMDMODE(0, "object-only", &cmdmode, N_("list only objects"),
>  			    MODE_OBJECT_ONLY),
> -		OPT_SET_INT(0, "full-name", &chomp_prefix,
> -			    N_("use full path names"), 0),
> +		OPT_BOOL(0, "full-name", &full_name, N_("use full path names")),
>  		OPT_BOOL(0, "full-tree", &full_tree,
>  			 N_("list entire tree; not just current directory "
>  			    "(implies --full-name)")),
> @@ -387,7 +386,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>
>  	if (full_tree)
>  		prefix = NULL;
> -	options.prefix = chomp_prefix ? prefix : NULL;
> +	options.prefix = full_name ? NULL : prefix;
>
>  	/*
>  	 * We wanted to detect conflicts between --name-only and
> diff --git a/t/t3101-ls-tree-dirname.sh b/t/t3101-ls-tree-dirname.sh
> index 217006d1bf..5af2dac0e4 100755
> --- a/t/t3101-ls-tree-dirname.sh
> +++ b/t/t3101-ls-tree-dirname.sh
> @@ -154,6 +154,14 @@ EOF
>  	test_output
>  '
>
> +test_expect_success 'ls-tree --no-full-name' '
> +	git -C path0 ls-tree --no-full-name $tree a >current &&
> +	cat >expected <<-EOF &&
> +	040000 tree X	a
> +	EOF
> +	test_output
> +'
> +
>  test_expect_success 'ls-tree --full-tree' '
>  	(
>  		cd path1/b/c &&
> --
> 2.41.0

Re: [PATCH] ls-tree: fix --no-full-name

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>  * "git commit --[no-](short|long|porcelain)" are accepted and
>    behave as "git status" without doing any "git commit" thing,
>    which should be corrected, I think.

It turns out that this was very much deliberate that these options
imply "--dry-run", implemented in 7c9f7038 (commit: support
alternate status formats, 2009-09-05).

So there is nothing to fix here.

Re: [PATCH] ls-tree: fix --no-full-name

[René Scharfe]

Am 18.07.23 um 18:37 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
> I wonder if there are cases where it makes sense to allow the
> "--no-" variant to an option parsed with OPT_SET_INT() that sets '0'
> as the value?

I doubt it.

> Some random findings while reading hits from "git grep OPT_SET_INT":

Woah, so many!

>  * "git branch --list --no-all" is accepted, sets filter.kind to 0,
>    and triggers "fatal: filter_refs: invalid type".  Shouldn't we
>    detect error much earlier?

Yes.  And "git branch --no-copy" etc. are funny as well.

>  * "git bundle create --no-quiet" is accepted and sets the progress
>    variable to 0, just like "--quiet" does, which is the same issue
>    as the one fixed by your patch.

The same in pack-objects.  It's a bit trickier because of the presence
of a third state (--quiet, --progress and --all-progress).  The help
text changes of 8b95521edb (bundle: turn on --all-progress-implied by
default, 2023-03-04) state that only two states remain in git bundle
(--quiet and --all-progress), but that's not fully true because the
option --all-progress-implied is still wired up.  "git bundle
--no-all-progress-implied --progress" still gives git pack-objects a
lone --progress.

>  * "git clone (--no-ipv4|--no-ipv6)" are accepted and uses
>    TRANSPORT_FAMILY_ALL, presumably allowing both v4 and v6.
>    Shouldn't we reject these?  "fetch" and "push" share the same
>    issue.

Either that, or we could turn them into OPT_BITs and let --no-ipv6
mean "give me anything but IPv6", which currently happens to be
the same as --ipv4..

>  * "git remote add" has an OPT_SET_INT() entry whose short and long
>    forms are (0, NULL).  What is this supposed to do?  Shouldn't
>    parse-options.c:parse_options_check() notice it as an error?

It extends the help text of the previous option.  Horrible.

>  * "git stash push --no-all" is the same as "--no-include-untracked",
>    which smells iffy but probably is OK.

Hard to imagine a situation where a --no-all would be well-defined
and intuitive.

Overall I get the impression that having the negative form enabled by
default was not a good idea.  For boolean options it makes sense, for
options with arguments perhaps as well, but for OPT_SET_INT we would
have less confusion if the negated form was opt-in.

To make it easier discoverable we could let the short help include
the optional "no-" part, which would look like this:

usage: git ls-tree [<options>] <tree-ish> [<path>...]

    -d                    only show trees
    -r                    recurse into subtrees
    -t                    show trees when recursing
    -z                    terminate entries with NUL byte
    -l, --long            include object size
    --name-only           list only filenames
    --name-status         list only filenames
    --object-only         list only objects
    --[no-]full-name      use full path names
    --[no-]full-tree      list entire tree; not just current directory (implies --full-name)
    --format <format>     format to use for the output
    --[no-]abbrev[=<n>]   use <n> digits to display object names

Thoughts?

René

Re: [PATCH] ls-tree: fix --no-full-name

[René Scharfe]

Am 18.07.23 um 22:49 schrieb Junio C Hamano:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>  * "git commit --[no-](short|long|porcelain)" are accepted and
>>    behave as "git status" without doing any "git commit" thing,
>>    which should be corrected, I think.
>
> It turns out that this was very much deliberate that these options
> imply "--dry-run", implemented in 7c9f7038 (commit: support
> alternate status formats, 2009-09-05).
>
> So there is nothing to fix here.

The negated variants don't imply --dry-run and do actually commit
something.  Which kinda makes sense.

René

[PATCH] show-branch: fix --no-sparse

[René Scharfe]

Since 57343652a5 (show-branch: migrate to parse-options API, 2009-05-21)
git show-branch has accepted the option --no-sparse, but it does the
same as --sparse.  That's because it's defined using OPT_SET_INT with a
value of 0, which sets 0 when negated, too.

Turn --no-sparse into the opposite of --sparse by using OPT_BOOL and
storing the option's status directly in a variable named "sparse"
instead of in negative form in "dense".

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/show-branch.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index a86b3c7677..99b3f4a09a 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -649,7 +649,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 	int with_current_branch = 0;
 	int head_at = -1;
 	int topics = 0;
-	int dense = 1;
+	int sparse = 0;
 	const char *reflog_base = NULL;
 	struct option builtin_show_branch_options[] = {
 		OPT_BOOL('a', "all", &all_heads,
@@ -676,8 +676,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 			    REV_SORT_IN_GRAPH_ORDER),
 		OPT_BOOL(0, "topics", &topics,
 			 N_("show only commits not on the first branch")),
-		OPT_SET_INT(0, "sparse", &dense,
-			    N_("show merges reachable from only one tip"), 0),
+		OPT_BOOL(0, "sparse", &sparse,
+			 N_("show merges reachable from only one tip")),
 		OPT_SET_INT(0, "date-order", &sort_order,
 			    N_("topologically sort, maintaining date order "
 			       "where possible"),
@@ -940,7 +940,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 			    !is_merge_point &&
 			    (this_flag & (1u << REV_SHIFT)))
 				continue;
-			if (dense && is_merge &&
+			if (!sparse && is_merge &&
 			    omit_in_dense(commit, rev, num_rev))
 				continue;
 			for (i = 0; i < num_rev; i++) {
--
2.41.0

[PATCH] show-branch: disallow --no-{date,topo}-order

[René Scharfe]

Since 57343652a5 (show-branch: migrate to parse-options API, 2009-05-21)
git show-branch has accepted --no-date-order and --no-topo-order, but
both mean the same as --topo-order.  That's because they are defined
using OPT_SET_INT, which sets the value to 0 for negated options, and
the value of REV_SORT_IN_GRAPH_ORDER happens to be 0 as well.

Ordering chronologically or topologically is not a binary choice in
principle; we could add e.g. an option to sort alphabetically.  So
negating a sort order option makes no sense conceptually.  Forbid it.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/show-branch.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 99b3f4a09a..587d231dae 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -671,17 +671,17 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 			 N_("show possible merge bases")),
 		OPT_BOOL(0, "independent", &independent,
 			    N_("show refs unreachable from any other ref")),
-		OPT_SET_INT(0, "topo-order", &sort_order,
-			    N_("show commits in topological order"),
-			    REV_SORT_IN_GRAPH_ORDER),
+		OPT_SET_INT_F(0, "topo-order", &sort_order,
+			      N_("show commits in topological order"),
+			      REV_SORT_IN_GRAPH_ORDER, PARSE_OPT_NONEG),
 		OPT_BOOL(0, "topics", &topics,
 			 N_("show only commits not on the first branch")),
 		OPT_BOOL(0, "sparse", &sparse,
 			 N_("show merges reachable from only one tip")),
-		OPT_SET_INT(0, "date-order", &sort_order,
-			    N_("topologically sort, maintaining date order "
-			       "where possible"),
-			    REV_SORT_BY_COMMIT_DATE),
+		OPT_SET_INT_F(0, "date-order", &sort_order,
+			      N_("topologically sort, maintaining date order "
+				 "where possible"),
+			      REV_SORT_BY_COMMIT_DATE, PARSE_OPT_NONEG),
 		OPT_CALLBACK_F('g', "reflog", &reflog_base, N_("<n>[,<base>]"),
 			    N_("show <n> most recent ref-log entries starting at "
 			       "base"),
--
2.41.0

[PATCH] reset: disallow --no-{mixed,soft,hard,merge,keep}

[René Scharfe]

Since 5eee6b28b5 (Make builtin-reset.c use parse_options., 2008-03-04)
git reset has accepted the options --no-mixed, --no-soft and --no-hard,
9e8eceab73 (Add 'merge' mode to 'git reset', 2008-12-01) and 9bc454df08
(reset: add option "--keep" to "git reset", 2010-01-19) added --no-merge
and --no-keep, respectively.  They all do the same as --mixed, because
they are defined using OPT_SET_INT and the value of MIXED happens to be
0.  That's surprising and not very useful.  Disallow the negated forms.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/reset.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 7f18dc03b8..6292859d3c 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -338,15 +338,21 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
 		OPT_BOOL(0, "no-refresh", &no_refresh,
 				N_("skip refreshing the index after reset")),
-		OPT_SET_INT(0, "mixed", &reset_type,
-						N_("reset HEAD and index"), MIXED),
-		OPT_SET_INT(0, "soft", &reset_type, N_("reset only HEAD"), SOFT),
-		OPT_SET_INT(0, "hard", &reset_type,
-				N_("reset HEAD, index and working tree"), HARD),
-		OPT_SET_INT(0, "merge", &reset_type,
-				N_("reset HEAD, index and working tree"), MERGE),
-		OPT_SET_INT(0, "keep", &reset_type,
-				N_("reset HEAD but keep local changes"), KEEP),
+		OPT_SET_INT_F(0, "mixed", &reset_type,
+			      N_("reset HEAD and index"), MIXED,
+			      PARSE_OPT_NONEG),
+		OPT_SET_INT_F(0, "soft", &reset_type,
+			      N_("reset only HEAD"), SOFT,
+			      PARSE_OPT_NONEG),
+		OPT_SET_INT_F(0, "hard", &reset_type,
+			      N_("reset HEAD, index and working tree"), HARD,
+			      PARSE_OPT_NONEG),
+		OPT_SET_INT_F(0, "merge", &reset_type,
+			      N_("reset HEAD, index and working tree"), MERGE,
+			      PARSE_OPT_NONEG),
+		OPT_SET_INT_F(0, "keep", &reset_type,
+			      N_("reset HEAD but keep local changes"), KEEP,
+			      PARSE_OPT_NONEG),
 		OPT_CALLBACK_F(0, "recurse-submodules", NULL,
 			    "reset", "control recursive updating of submodules",
 			    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),
--
2.41.0

[PATCH] pack-objects: fix --no-quiet

[René Scharfe]

Since 99fb6e04cb (pack-objects: convert to use parse_options(),
2012-02-01) git pack-objects has accepted the option --no-quiet, but it
does the same as --quiet.  That's because it's defined using OPT_SET_INT
with a value of 0, which sets 0 when negated, too.

Make --no-quiet equivalent to --progress and ignore it if --all-progress
was given.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/pack-objects.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index fcf591c466..ccf451ce70 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4117,6 +4117,18 @@ static void add_extra_kept_packs(const struct string_list *names)
 	}
 }

+static int option_parse_quiet(const struct option *opt, const char *arg,
+			      int unset)
+{
+	BUG_ON_OPT_ARG(arg);
+
+	if (!unset)
+		progress = 0;
+	else if (!progress)
+		progress = 1;
+	return 0;
+}
+
 static int option_parse_index_version(const struct option *opt,
 				      const char *arg, int unset)
 {
@@ -4178,8 +4190,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		LIST_OBJECTS_FILTER_INIT;

 	struct option pack_objects_options[] = {
-		OPT_SET_INT('q', "quiet", &progress,
-			    N_("do not show progress meter"), 0),
+		OPT_CALLBACK_F('q', "quiet", NULL, NULL,
+			       N_("do not show progress meter"),
+			       PARSE_OPT_NOARG, option_parse_quiet),
 		OPT_SET_INT(0, "progress", &progress,
 			    N_("show progress meter"), 1),
 		OPT_SET_INT(0, "all-progress", &progress,
--
2.41.0

[PATCH] pack-objects: fix --no-keep-true-parents

[René Scharfe]

Since 99fb6e04cb (pack-objects: convert to use parse_options(),
2012-02-01) git pack-objects has accepted --no-keep-true-parents, but
this option does the same as --keep-true-parents.  That's because it's
defined using OPT_SET_INT with a value of 0, which sets 0 when negated
as well.

Turn --no-keep-true-parents into the opposite of --keep-true-parents by
using OPT_BOOL and storing the option's status directly in a variable
named "grants_keep_true_parents" instead of in negative form in
"grafts_replace_parents".

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/pack-objects.c | 4 ++--
 commit.c               | 2 +-
 environment.c          | 2 +-
 environment.h          | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 06b33d49e9..7bff1be7aa 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4255,8 +4255,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 				N_("ignore this pack")),
 		OPT_INTEGER(0, "compression", &pack_compression_level,
 			    N_("pack compression level")),
-		OPT_SET_INT(0, "keep-true-parents", &grafts_replace_parents,
-			    N_("do not hide commits by grafts"), 0),
+		OPT_BOOL(0, "keep-true-parents", &grafts_keep_true_parents,
+			 N_("do not hide commits by grafts")),
 		OPT_BOOL(0, "use-bitmap-index", &use_bitmap_index,
 			 N_("use a bitmap index if available to speed up counting objects")),
 		OPT_SET_INT(0, "write-bitmap-index", &write_bitmap_index,
diff --git a/commit.c b/commit.c
index 6507791061..b3223478bc 100644
--- a/commit.c
+++ b/commit.c
@@ -516,7 +516,7 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b
 		 * The clone is shallow if nr_parent < 0, and we must
 		 * not traverse its real parents even when we unhide them.
 		 */
-		if (graft && (graft->nr_parent < 0 || grafts_replace_parents))
+		if (graft && (graft->nr_parent < 0 || !grafts_keep_true_parents))
 			continue;
 		new_parent = lookup_commit(r, &parent);
 		if (!new_parent)
diff --git a/environment.c b/environment.c
index a0d1d070d1..f98d76f080 100644
--- a/environment.c
+++ b/environment.c
@@ -73,7 +73,7 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
 #endif
 enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
 char *notes_ref_name;
-int grafts_replace_parents = 1;
+int grafts_keep_true_parents;
 int core_apply_sparse_checkout;
 int core_sparse_checkout_cone;
 int sparse_expect_files_outside_of_patterns;
diff --git a/environment.h b/environment.h
index 611aa0ffed..c5377473c6 100644
--- a/environment.h
+++ b/environment.h
@@ -193,7 +193,7 @@ extern enum object_creation_mode object_creation_mode;

 extern char *notes_ref_name;

-extern int grafts_replace_parents;
+extern int grafts_keep_true_parents;

 extern int repository_format_precious_objects;

--
2.41.0

[PATCH] branch: disallow --no-{all,remotes}

[René Scharfe]

Since 171edcbb49 (git-branch: introduce missing long forms for the
options, 2011-08-28) git branch accepts --no-all and --no-remotes, but
fails in both cases, reporting "fatal: filter_refs: invalid type".
Disallow the options --all and --remotes to be negated in the first
place.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/branch.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index a27bc0a3df..1bfd85f4d8 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -720,8 +720,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_STRING('u', "set-upstream-to", &new_upstream, N_("upstream"), N_("change the upstream info")),
 		OPT_BOOL(0, "unset-upstream", &unset_upstream, N_("unset the upstream info")),
 		OPT__COLOR(&branch_use_color, N_("use colored output")),
-		OPT_SET_INT('r', "remotes",     &filter.kind, N_("act on remote-tracking branches"),
-			FILTER_REFS_REMOTES),
+		OPT_SET_INT_F('r', "remotes", &filter.kind,
+			N_("act on remote-tracking branches"),
+			FILTER_REFS_REMOTES,
+			PARSE_OPT_NONEG),
 		OPT_CONTAINS(&filter.with_commit, N_("print only branches that contain the commit")),
 		OPT_NO_CONTAINS(&filter.no_commit, N_("print only branches that don't contain the commit")),
 		OPT_WITH(&filter.with_commit, N_("print only branches that contain the commit")),
@@ -729,8 +731,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT__ABBREV(&filter.abbrev),

 		OPT_GROUP(N_("Specific git-branch actions:")),
-		OPT_SET_INT('a', "all", &filter.kind, N_("list both remote-tracking and local branches"),
-			FILTER_REFS_REMOTES | FILTER_REFS_BRANCHES),
+		OPT_SET_INT_F('a', "all", &filter.kind,
+			N_("list both remote-tracking and local branches"),
+			FILTER_REFS_REMOTES | FILTER_REFS_BRANCHES,
+			PARSE_OPT_NONEG),
 		OPT_BIT('d', "delete", &delete, N_("delete fully merged branch"), 1),
 		OPT_BIT('D', NULL, &delete, N_("delete branch (even if not merged)"), 2),
 		OPT_BIT('m', "move", &rename, N_("move/rename a branch and its reflog"), 1),
--
2.41.0

[PATCH] am: unify definition of --keep-cr and --no-keep-cr

[René Scharfe]

The options --keep-cr and --no-keep-cr set the variable keep_cr to 1 and
0, respectively.  We don't usually define the negative variant
explicitly.  The extra help text only tells users that the option
overrules the config option am.keepcr, which conforms to convention.

So allow --keep-cr to be negated and drop the now redundant definition
of --no-keep-cr for consistency.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/am.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index dcb89439b1..a216024e1d 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2347,12 +2347,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)
 			N_("pass -b flag to git-mailinfo"), KEEP_NON_PATCH),
 		OPT_BOOL('m', "message-id", &state.message_id,
 			N_("pass -m flag to git-mailinfo")),
-		OPT_SET_INT_F(0, "keep-cr", &keep_cr,
+		OPT_SET_INT(0, "keep-cr", &keep_cr,
 			N_("pass --keep-cr flag to git-mailsplit for mbox format"),
-			1, PARSE_OPT_NONEG),
-		OPT_SET_INT_F(0, "no-keep-cr", &keep_cr,
-			N_("do not pass --keep-cr flag to git-mailsplit independent of am.keepcr"),
-			0, PARSE_OPT_NONEG),
+			1),
 		OPT_BOOL('c', "scissors", &state.scissors,
 			N_("strip everything before a scissors line")),
 		OPT_CALLBACK_F(0, "quoted-cr", &state.quoted_cr, N_("action"),
--
2.41.0

[PATCH] describe: fix --no-exact-match

[René Scharfe]

Since 2c33f75754 (Teach git-describe --exact-match to avoid expensive
tag searches, 2008-02-24) git describe accepts --no-exact-match, but it
does the same as --exact-match, an alias for --candidates=0.  That's
because it's defined using OPT_SET_INT with a value of 0, which sets 0
when negated as well.

Let --no-exact-match set the number of candidates to the default value
instead.  Users that need a more specific lack of exactitude can specify
their preferred value using --candidates, as before.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
Or should we just forbid --no-exact-match?

 builtin/describe.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 7ce23e1b8e..b28a4a1f82 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -25,6 +25,7 @@
 #include "wildmatch.h"

 #define MAX_TAGS	(FLAG_BITS - 1)
+#define DEFAULT_CANDIDATES 10

 define_commit_slab(commit_names, struct commit_name *);

@@ -41,7 +42,7 @@ static int tags;	/* Allow lightweight tags */
 static int longformat;
 static int first_parent;
 static int abbrev = -1; /* unspecified */
-static int max_candidates = 10;
+static int max_candidates = DEFAULT_CANDIDATES;
 static struct hashmap names;
 static int have_util;
 static struct string_list patterns = STRING_LIST_INIT_NODUP;
@@ -557,6 +558,15 @@ static void describe(const char *arg, int last_one)
 	strbuf_release(&sb);
 }

+static int option_parse_exact_match(const struct option *opt, const char *arg,
+				    int unset)
+{
+	BUG_ON_OPT_ARG(arg);
+
+	max_candidates = unset ? DEFAULT_CANDIDATES : 0;
+	return 0;
+}
+
 int cmd_describe(int argc, const char **argv, const char *prefix)
 {
 	int contains = 0;
@@ -568,8 +578,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "long",       &longformat, N_("always use long format")),
 		OPT_BOOL(0, "first-parent", &first_parent, N_("only follow first parent")),
 		OPT__ABBREV(&abbrev),
-		OPT_SET_INT(0, "exact-match", &max_candidates,
-			    N_("only output exact matches"), 0),
+		OPT_CALLBACK_F(0, "exact-match", NULL, NULL,
+			       N_("only output exact matches"),
+			       PARSE_OPT_NOARG, option_parse_exact_match),
 		OPT_INTEGER(0, "candidates", &max_candidates,
 			    N_("consider <n> most recent tags (default: 10)")),
 		OPT_STRING_LIST(0, "match", &patterns, N_("pattern"),
--
2.41.0

Re: [PATCH] describe: fix --no-exact-match

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Since 2c33f75754 (Teach git-describe --exact-match to avoid expensive
> tag searches, 2008-02-24) git describe accepts --no-exact-match, but it
> does the same as --exact-match, an alias for --candidates=0.  That's
> because it's defined using OPT_SET_INT with a value of 0, which sets 0
> when negated as well.
>
> Let --no-exact-match set the number of candidates to the default value
> instead.  Users that need a more specific lack of exactitude can specify
> their preferred value using --candidates, as before.
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> Or should we just forbid --no-exact-match?

We could do that, but setting it back to the default was also what I
did in my version I sent (<xmqqy1jcgbiv.fsf@gitster.g>) a few days
ago.

Re: [PATCH] ls-tree: fix --no-full-name

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Overall I get the impression that having the negative form enabled by
> default was not a good idea.  For boolean options it makes sense, for
> options with arguments perhaps as well, but for OPT_SET_INT we would
> have less confusion if the negated form was opt-in.
>
> To make it easier discoverable we could let the short help include
> the optional "no-" part, which would look like this:
>
> usage: git ls-tree [<options>] <tree-ish> [<path>...]
>
>     -d                    only show trees
>     -r                    recurse into subtrees
>     -t                    show trees when recursing
>     -z                    terminate entries with NUL byte
>     -l, --long            include object size
>     --name-only           list only filenames
>     --name-status         list only filenames
>     --object-only         list only objects
>     --[no-]full-name      use full path names
>     --[no-]full-tree      list entire tree; not just current directory (implies --full-name)
>     --format <format>     format to use for the output
>     --[no-]abbrev[=<n>]   use <n> digits to display object names
>
> Thoughts?

I like the "optional no- accepted" markings, but I suspect there may
be quite a lot of fallouts.

Re: [PATCH] show-branch: fix --no-sparse

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Since 57343652a5 (show-branch: migrate to parse-options API, 2009-05-21)
> git show-branch has accepted the option --no-sparse, but it does the
> same as --sparse.  That's because it's defined using OPT_SET_INT with a
> value of 0, which sets 0 when negated, too.

Hmph, am I expected to compare these patches with what I sent a few
days ago and pick whichever are the better ones?  Can I delegate
that task to somebody else ;-)?

        jc/am-parseopt-fix		<xmqqr0p5gjv3.fsf@gitster.g>
        jc/branch-parseopt-fix		<xmqqjzuxgjmi.fsf@gitster.g>
        jc/describe-parseopt-fix	<xmqqy1jcgbiv.fsf@gitster.g>
        jc/parse-options-reset		<xmqq1qh4c998.fsf@gitster.g>
        jc/parse-options-short-help	<xmqq5y6gg8fn.fsf@gitster.g>
        jc/parse-options-show-branch	<xmqqh6pzc15n.fsf@gitster.g>
        jc/transport-parseopt-fix	<xmqqedl4gag8.fsf@gitster.g>

Some of them are already in 'next' as they were so trivial.

Thanks.

> Turn --no-sparse into the opposite of --sparse by using OPT_BOOL and
> storing the option's status directly in a variable named "sparse"
> instead of in negative form in "dense".
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  builtin/show-branch.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/show-branch.c b/builtin/show-branch.c
> index a86b3c7677..99b3f4a09a 100644
> --- a/builtin/show-branch.c
> +++ b/builtin/show-branch.c
> @@ -649,7 +649,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
>  	int with_current_branch = 0;
>  	int head_at = -1;
>  	int topics = 0;
> -	int dense = 1;
> +	int sparse = 0;
>  	const char *reflog_base = NULL;
>  	struct option builtin_show_branch_options[] = {
>  		OPT_BOOL('a', "all", &all_heads,
> @@ -676,8 +676,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
>  			    REV_SORT_IN_GRAPH_ORDER),
>  		OPT_BOOL(0, "topics", &topics,
>  			 N_("show only commits not on the first branch")),
> -		OPT_SET_INT(0, "sparse", &dense,
> -			    N_("show merges reachable from only one tip"), 0),
> +		OPT_BOOL(0, "sparse", &sparse,
> +			 N_("show merges reachable from only one tip")),
>  		OPT_SET_INT(0, "date-order", &sort_order,
>  			    N_("topologically sort, maintaining date order "
>  			       "where possible"),
> @@ -940,7 +940,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
>  			    !is_merge_point &&
>  			    (this_flag & (1u << REV_SHIFT)))
>  				continue;
> -			if (dense && is_merge &&
> +			if (!sparse && is_merge &&
>  			    omit_in_dense(commit, rev, num_rev))
>  				continue;
>  			for (i = 0; i < num_rev; i++) {
> --
> 2.41.0

Re: [PATCH] show-branch: fix --no-sparse

[René Scharfe]

Am 21.07.23 um 16:42 schrieb Junio C Hamano:
>
> Hmph, am I expected to compare these patches with what I sent a few
> days ago and pick whichever are the better ones?  Can I delegate
> that task to somebody else ;-)?
>
>         jc/am-parseopt-fix		<xmqqr0p5gjv3.fsf@gitster.g>
>         jc/branch-parseopt-fix		<xmqqjzuxgjmi.fsf@gitster.g>
>         jc/describe-parseopt-fix	<xmqqy1jcgbiv.fsf@gitster.g>
>         jc/parse-options-reset		<xmqq1qh4c998.fsf@gitster.g>
>         jc/parse-options-short-help	<xmqq5y6gg8fn.fsf@gitster.g>
>         jc/parse-options-show-branch	<xmqqh6pzc15n.fsf@gitster.g>
>         jc/transport-parseopt-fix	<xmqqedl4gag8.fsf@gitster.g>

Aww, didn't see them.

Most differences are cosmetic, but in git describe you use two
OPT_SET_INT_F for the two OPT_SET_INT_Fs for --exact-match and
--no-exact-match.  The callback I chose is a little uglier on the
inside, but prettier on the outside -- occupies a single line in short
help.  Reminds me of the --[no-]keep-cr simplification in git am.

And you don't seem to have touched pack-objects; please check those two
patches of mine.

René

Re: [PATCH] describe: fix --no-exact-match

[Junio C Hamano]

From: René Scharfe <l.s.r@web.de>
Date: Fri, 21 Jul 2023 15:41:33 +0200

Since 2c33f75754 (Teach git-describe --exact-match to avoid expensive
tag searches, 2008-02-24) git describe accepts --no-exact-match, but it
does the same as --exact-match, an alias for --candidates=0.  That's
because it's defined using OPT_SET_INT with a value of 0, which sets 0
when negated as well.

Let --no-exact-match set the number of candidates to the default value
instead.  Users that need a more specific lack of exactitude can specify
their preferred value using --candidates, as before.

The "--no-exact-match" option was not covered in the tests, so let's
add a few.  Also add a case where --exact-match option is used on a
commit that cannot be described without distance from tags and make
sure the command fails.

Signed-off-by: René Scharfe <l.s.r@web.de>
[jc: added trivial tests]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * Stole tests from my version.

 builtin/describe.c  | 17 ++++++++++++++---
 t/t6120-describe.sh |  8 ++++++++
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 55b4baaa22..a5ba584cdd 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -24,6 +24,7 @@
 #include "commit-slab.h"
 
 #define MAX_TAGS	(FLAG_BITS - 1)
+#define DEFAULT_CANDIDATES 10
 
 define_commit_slab(commit_names, struct commit_name *);
 
@@ -40,7 +41,7 @@ static int tags;	/* Allow lightweight tags */
 static int longformat;
 static int first_parent;
 static int abbrev = -1; /* unspecified */
-static int max_candidates = 10;
+static int max_candidates = DEFAULT_CANDIDATES;
 static struct hashmap names;
 static int have_util;
 static struct string_list patterns = STRING_LIST_INIT_NODUP;
@@ -556,6 +557,15 @@ static void describe(const char *arg, int last_one)
 	strbuf_release(&sb);
 }
 
+static int option_parse_exact_match(const struct option *opt, const char *arg,
+				    int unset)
+{
+	BUG_ON_OPT_ARG(arg);
+
+	max_candidates = unset ? DEFAULT_CANDIDATES : 0;
+	return 0;
+}
+
 int cmd_describe(int argc, const char **argv, const char *prefix)
 {
 	int contains = 0;
@@ -567,8 +577,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "long",       &longformat, N_("always use long format")),
 		OPT_BOOL(0, "first-parent", &first_parent, N_("only follow first parent")),
 		OPT__ABBREV(&abbrev),
-		OPT_SET_INT(0, "exact-match", &max_candidates,
-			    N_("only output exact matches"), 0),
+		OPT_CALLBACK_F(0, "exact-match", NULL, NULL,
+			       N_("only output exact matches"),
+			       PARSE_OPT_NOARG, option_parse_exact_match),
 		OPT_INTEGER(0, "candidates", &max_candidates,
 			    N_("consider <n> most recent tags (default: 10)")),
 		OPT_STRING_LIST(0, "match", &patterns, N_("pattern"),
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index c9afcef201..0a5c487540 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -85,6 +85,7 @@ check_describe e-1-gHASH --tags HEAD^^
 check_describe c-2-gHASH --tags HEAD^^2
 check_describe B --tags HEAD^^2^
 check_describe e --tags HEAD^^^
+check_describe e --tags --exact-match HEAD^^^
 
 check_describe heads/main --all HEAD
 check_describe tags/c-6-gHASH --all HEAD^
@@ -96,6 +97,13 @@ check_describe A-3-gHASH --long HEAD^^2
 check_describe c-7-gHASH --tags
 check_describe e-3-gHASH --first-parent --tags
 
+check_describe c-7-gHASH --tags --no-exact-match HEAD
+check_describe e-3-gHASH --first-parent --tags --no-exact-match HEAD
+
+test_expect_success '--exact-match failure' '
+	test_must_fail git describe --exact-match HEAD 2>err
+'
+
 test_expect_success 'describe --contains defaults to HEAD without commit-ish' '
 	echo "A^0" >expect &&
 	git checkout A &&
-- 
2.41.0-376-gcba07a324d

Re: [PATCH] pack-objects: fix --no-keep-true-parents

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Since 99fb6e04cb (pack-objects: convert to use parse_options(),
> 2012-02-01) git pack-objects has accepted --no-keep-true-parents, but
> this option does the same as --keep-true-parents.  That's because it's
> defined using OPT_SET_INT with a value of 0, which sets 0 when negated
> as well.
>
> Turn --no-keep-true-parents into the opposite of --keep-true-parents by
> using OPT_BOOL and storing the option's status directly in a variable
> named "grants_keep_true_parents" instead of in negative form in

"grant" -> "graft" (locally fixed, no need to resend).

> "grafts_replace_parents".
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---

Thanks.

Re: [PATCH] ls-tree: fix --no-full-name

[René Scharfe]

Am 21.07.23 um 16:37 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> Overall I get the impression that having the negative form enabled by
>> default was not a good idea.  For boolean options it makes sense, for
>> options with arguments perhaps as well, but for OPT_SET_INT we would
>> have less confusion if the negated form was opt-in.
>>
>> To make it easier discoverable we could let the short help include
>> the optional "no-" part, which would look like this:
>>
>> usage: git ls-tree [<options>] <tree-ish> [<path>...]
>>
>>     -d                    only show trees
>>     -r                    recurse into subtrees
>>     -t                    show trees when recursing
>>     -z                    terminate entries with NUL byte
>>     -l, --long            include object size
>>     --name-only           list only filenames
>>     --name-status         list only filenames
>>     --object-only         list only objects
>>     --[no-]full-name      use full path names
>>     --[no-]full-tree      list entire tree; not just current directory (implies --full-name)
>>     --format <format>     format to use for the output
>>     --[no-]abbrev[=<n>]   use <n> digits to display object names
>>
>> Thoughts?
>
> I like the "optional no- accepted" markings, but I suspect there may
> be quite a lot of fallouts.

Some test expectations in t0040 and t1502 would have to be adjusted.

This reveals, by the way, that t1502 doesn't yet exercise the "!" flag
of "git rev-parse --parseopt" that turns on PARSE_OPT_NONEG.  I find
the "-h, --[no-]help" option strangely amusing..

--- >8 ----
Subject: [PATCH] parse-options: show negatability of options in short help

Add a "[no-]" prefix to options without the flag PARSE_OPT_NONEG to
document the fact that you can negate them.

This looks a bit strange for options that already start with "no-", e.g.
for the option --no-name of git show-branch:

    --[no-]no-name        suppress naming strings

You can actually use --no-no-name as an alias of --name, so the short
help is not wrong.  If we strip off any of the "no-"s, we lose either
the ability to see if the remaining one belongs to the documented
variant or to see if it can be negated.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 parse-options.c               | 10 ++++-
 t/t0040-parse-options.sh      | 44 ++++++++++---------
 t/t1502-rev-parse-parseopt.sh | 80 ++++++++++++++++++++---------------
 3 files changed, 77 insertions(+), 57 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index f8a155ee13..6323ca191d 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1136,8 +1136,14 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
 		}
 		if (opts->long_name && opts->short_name)
 			pos += fprintf(outfile, ", ");
-		if (opts->long_name)
-			pos += fprintf(outfile, "--%s", opts->long_name);
+		if (opts->long_name) {
+			const char *long_name = opts->long_name;
+			if (opts->flags & PARSE_OPT_NONEG)
+				pos += fprintf(outfile, "--%s", long_name);
+			else
+				pos += fprintf(outfile, "--[no-]%s", long_name);
+		}
+
 		if (opts->type == OPTION_NUMBER)
 			pos += utf8_fprintf(outfile, _("-NUM"));

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 7d7ecfd571..f39758d2ef 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -13,29 +13,32 @@ usage: test-tool parse-options <options>

     A helper function for the parse-options API.

-    --yes                 get a boolean
-    -D, --no-doubt        begins with 'no-'
+    --[no-]yes            get a boolean
+    -D, --[no-]no-doubt   begins with 'no-'
     -B, --no-fear         be brave
-    -b, --boolean         increment by one
-    -4, --or4             bitwise-or boolean with ...0100
-    --neg-or4             same as --no-or4
+    -b, --[no-]boolean    increment by one
+    -4, --[no-]or4        bitwise-or boolean with ...0100
+    --[no-]neg-or4        same as --no-or4

-    -i, --integer <n>     get a integer
+    -i, --[no-]integer <n>
+                          get a integer
     -j <n>                get a integer, too
     -m, --magnitude <n>   get a magnitude
-    --set23               set integer to 23
+    --[no-]set23          set integer to 23
     --mode1               set integer to 1 (cmdmode option)
     --mode2               set integer to 2 (cmdmode option)
-    -L, --length <str>    get length of <str>
-    -F, --file <file>     set file to <file>
+    -L, --[no-]length <str>
+                          get length of <str>
+    -F, --[no-]file <file>
+                          set file to <file>

 String options
-    -s, --string <string>
+    -s, --[no-]string <string>
                           get a string
-    --string2 <str>       get another string
-    --st <st>             get another string (pervert ordering)
+    --[no-]string2 <str>  get another string
+    --[no-]st <st>        get another string (pervert ordering)
     -o <str>              get another string
-    --list <str>          add str to list
+    --[no-]list <str>     add str to list

 Magic arguments
     -NUM                  set integer to NUM
@@ -44,16 +47,17 @@ Magic arguments
     --no-ambiguous        negative ambiguity

 Standard options
-    --abbrev[=<n>]        use <n> digits to display object names
-    -v, --verbose         be verbose
-    -n, --dry-run         dry run
-    -q, --quiet           be quiet
-    --expect <string>     expected output in the variable dump
+    --[no-]abbrev[=<n>]   use <n> digits to display object names
+    -v, --[no-]verbose    be verbose
+    -n, --[no-]dry-run    dry run
+    -q, --[no-]quiet      be quiet
+    --[no-]expect <string>
+                          expected output in the variable dump

 Alias
-    -A, --alias-source <string>
+    -A, --[no-]alias-source <string>
                           get a string
-    -Z, --alias-target <string>
+    -Z, --[no-]alias-target <string>
                           alias of --alias-source

 EOF
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index dd811b7fb4..0a67e2dd4f 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -64,33 +64,38 @@ test_expect_success 'test --parseopt help output' '
 |
 |    some-command does foo and bar!
 |
-|    -h, --help            show the help
-|    --foo                 some nifty option --foo
-|    --bar ...             some cool option --bar with an argument
-|    -b, --baz             a short and long option
+|    -h, --[no-]help       show the help
+|    --[no-]foo            some nifty option --foo
+|    --[no-]bar ...        some cool option --bar with an argument
+|    -b, --[no-]baz        a short and long option
 |
 |An option group Header
 |    -C[...]               option C with an optional argument
-|    -d, --data[=...]      short and long option with an optional argument
+|    -d, --[no-]data[=...]
+|                          short and long option with an optional argument
 |
 |Argument hints
 |    -B <arg>              short option required argument
-|    --bar2 <arg>          long option required argument
-|    -e, --fuz <with-space>
+|    --[no-]bar2 <arg>     long option required argument
+|    -e, --[no-]fuz <with-space>
 |                          short and long option required argument
 |    -s[<some>]            short option optional argument
-|    --long[=<data>]       long option optional argument
-|    -g, --fluf[=<path>]   short and long option optional argument
-|    --longest <very-long-argument-hint>
+|    --[no-]long[=<data>]  long option optional argument
+|    -g, --[no-]fluf[=<path>]
+|                          short and long option optional argument
+|    --[no-]longest <very-long-argument-hint>
 |                          a very long argument hint
-|    --pair <key=value>    with an equals sign in the hint
-|    --aswitch             help te=t contains? fl*g characters!`
-|    --bswitch <hint>      hint has trailing tab character
-|    --cswitch             switch has trailing tab character
-|    --short-hint <a>      with a one symbol hint
+|    --[no-]pair <key=value>
+|                          with an equals sign in the hint
+|    --[no-]aswitch        help te=t contains? fl*g characters!`
+|    --[no-]bswitch <hint>
+|                          hint has trailing tab character
+|    --[no-]cswitch        switch has trailing tab character
+|    --[no-]short-hint <a>
+|                          with a one symbol hint
 |
 |Extras
-|    --extra1              line above used to cause a segfault but no longer does
+|    --[no-]extra1         line above used to cause a segfault but no longer does
 |
 |EOF
 END_EXPECT
@@ -131,7 +136,7 @@ test_expect_success 'test --parseopt help-all output hidden switches' '
 |
 |    some-command does foo and bar!
 |
-|    --hidden1             A hidden switch
+|    --[no-]hidden1        A hidden switch
 |
 |EOF
 END_EXPECT
@@ -146,33 +151,38 @@ test_expect_success 'test --parseopt invalid switch help output' '
 |
 |    some-command does foo and bar!
 |
-|    -h, --help            show the help
-|    --foo                 some nifty option --foo
-|    --bar ...             some cool option --bar with an argument
-|    -b, --baz             a short and long option
+|    -h, --[no-]help       show the help
+|    --[no-]foo            some nifty option --foo
+|    --[no-]bar ...        some cool option --bar with an argument
+|    -b, --[no-]baz        a short and long option
 |
 |An option group Header
 |    -C[...]               option C with an optional argument
-|    -d, --data[=...]      short and long option with an optional argument
+|    -d, --[no-]data[=...]
+|                          short and long option with an optional argument
 |
 |Argument hints
 |    -B <arg>              short option required argument
-|    --bar2 <arg>          long option required argument
-|    -e, --fuz <with-space>
+|    --[no-]bar2 <arg>     long option required argument
+|    -e, --[no-]fuz <with-space>
 |                          short and long option required argument
 |    -s[<some>]            short option optional argument
-|    --long[=<data>]       long option optional argument
-|    -g, --fluf[=<path>]   short and long option optional argument
-|    --longest <very-long-argument-hint>
+|    --[no-]long[=<data>]  long option optional argument
+|    -g, --[no-]fluf[=<path>]
+|                          short and long option optional argument
+|    --[no-]longest <very-long-argument-hint>
 |                          a very long argument hint
-|    --pair <key=value>    with an equals sign in the hint
-|    --aswitch             help te=t contains? fl*g characters!`
-|    --bswitch <hint>      hint has trailing tab character
-|    --cswitch             switch has trailing tab character
-|    --short-hint <a>      with a one symbol hint
+|    --[no-]pair <key=value>
+|                          with an equals sign in the hint
+|    --[no-]aswitch        help te=t contains? fl*g characters!`
+|    --[no-]bswitch <hint>
+|                          hint has trailing tab character
+|    --[no-]cswitch        switch has trailing tab character
+|    --[no-]short-hint <a>
+|                          with a one symbol hint
 |
 |Extras
-|    --extra1              line above used to cause a segfault but no longer does
+|    --[no-]extra1         line above used to cause a segfault but no longer does
 |
 END_EXPECT
 	test_expect_code 129 git rev-parse --parseopt -- --does-not-exist 1>/dev/null 2>output < optionspec &&
@@ -297,7 +307,7 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:"
 	|   or:     [--another-option]
 	|   or: cmd [--yet-another-option]
 	|
-	|    -h, --help            show the help
+	|    -h, --[no-]help       show the help
 	|
 	|EOF
 	END_EXPECT
@@ -334,7 +344,7 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l
 	|    line
 	|    blurb
 	|
-	|    -h, --help            show the help
+	|    -h, --[no-]help       show the help
 	|
 	|EOF
 	END_EXPECT
--
2.41.0

Re: [PATCH] ls-tree: fix --no-full-name

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Some test expectations in t0040 and t1502 would have to be adjusted.
>
> This reveals, by the way, that t1502 doesn't yet exercise the "!" flag
> of "git rev-parse --parseopt" that turns on PARSE_OPT_NONEG.  I find
> the "-h, --[no-]help" option strangely amusing..
>
> --- >8 ----
> Subject: [PATCH] parse-options: show negatability of options in short help
>
> Add a "[no-]" prefix to options without the flag PARSE_OPT_NONEG to
> document the fact that you can negate them.
>
> This looks a bit strange for options that already start with "no-", e.g.
> for the option --no-name of git show-branch:
>
>     --[no-]no-name        suppress naming strings
>
> You can actually use --no-no-name as an alias of --name, so the short
> help is not wrong.  If we strip off any of the "no-"s, we lose either
> the ability to see if the remaining one belongs to the documented
> variant or to see if it can be negated.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  parse-options.c               | 10 ++++-
>  t/t0040-parse-options.sh      | 44 ++++++++++---------
>  t/t1502-rev-parse-parseopt.sh | 80 ++++++++++++++++++++---------------
>  3 files changed, 77 insertions(+), 57 deletions(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index f8a155ee13..6323ca191d 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -1136,8 +1136,14 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
>  		}
>  		if (opts->long_name && opts->short_name)
>  			pos += fprintf(outfile, ", ");
> -		if (opts->long_name)
> -			pos += fprintf(outfile, "--%s", opts->long_name);
> +		if (opts->long_name) {
> +			const char *long_name = opts->long_name;
> +			if (opts->flags & PARSE_OPT_NONEG)
> +				pos += fprintf(outfile, "--%s", long_name);
> +			else
> +				pos += fprintf(outfile, "--[no-]%s", long_name);
> +		}

This is a good starting point, but we should at least exempt
OPT_BOOL from this exercise, I would think, because ...

>      A helper function for the parse-options API.
>
> -    --yes                 get a boolean
> +    --[no-]yes            get a boolean

... they are designed to be prefixed with an optional "no-".


> -    -D, --no-doubt        begins with 'no-'
> +    -D, --[no-]no-doubt   begins with 'no-'

Hmph, I really really loved the neat trick to allow "no-doubt"
option to be "positivised" by _dropping_ the leading "no-" at around
0f1930c5 (parse-options: allow positivation of options starting,
with no-, 2012-02-25).

>  EOF

Many of the above are amusing and served as good demonstration to
show the blast radius, but it seems that most of them should be
marked with PARSE_OPT_NONEG.

> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
> index dd811b7fb4..0a67e2dd4f 100755
> --- a/t/t1502-rev-parse-parseopt.sh
> +++ b/t/t1502-rev-parse-parseopt.sh
> @@ -64,33 +64,38 @@ test_expect_success 'test --parseopt help output' '
>  |
>  |    some-command does foo and bar!
>  |
> -|    -h, --help            show the help
> -|    --foo                 some nifty option --foo
> -|    --bar ...             some cool option --bar with an argument
> -|    -b, --baz             a short and long option
> +|    -h, --[no-]help       show the help

Indeed it is amusing, but we probably should give PARSE_OPT_NONEG
appropriately, instead of changing the expectations, for many of the
changes we see here, I think.

Re: [PATCH] ls-tree: fix --no-full-name

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> This is a good starting point, but we should at least exempt
> OPT_BOOL from this exercise, I would think, because ...

No, no no, that was nonsense.  The literal "yes" fooled me.
There is no need to special-case OPT_BOOL at all.

Sorry for the noise.

Re: [PATCH] ls-tree: fix --no-full-name

[René Scharfe]

Am 21.07.23 um 22:09 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> -    -D, --no-doubt        begins with 'no-'
>> +    -D, --[no-]no-doubt   begins with 'no-'
>
> Hmph, I really really loved the neat trick to allow "no-doubt"
> option to be "positivised" by _dropping_ the leading "no-" at around
> 0f1930c5 (parse-options: allow positivation of options starting,
> with no-, 2012-02-25).

Yeah, if there is a better way to document A) that the "no-" is optional
and B) whether it's present by default, I'm all ears.

> Many of the above are amusing and served as good demonstration to
> show the blast radius, but it seems that most of them should be
> marked with PARSE_OPT_NONEG.

Hard to say for me -- these are synthetic test cases and I lack context
to make that decision.  In t0040 (t/helper/test-parse-options.c rather)
we do have a few PARSE_OPT_NONEG uses already.  In t1502 we need to add
some...

>
>> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
>> index dd811b7fb4..0a67e2dd4f 100755
>> --- a/t/t1502-rev-parse-parseopt.sh
>> +++ b/t/t1502-rev-parse-parseopt.sh
>> @@ -64,33 +64,38 @@ test_expect_success 'test --parseopt help output' '
>>  |
>>  |    some-command does foo and bar!
>>  |
>> -|    -h, --help            show the help
>> -|    --foo                 some nifty option --foo
>> -|    --bar ...             some cool option --bar with an argument
>> -|    -b, --baz             a short and long option
>> +|    -h, --[no-]help       show the help
>
> Indeed it is amusing, but we probably should give PARSE_OPT_NONEG
> appropriately, instead of changing the expectations, for many of the
> changes we see here, I think.

... and --help is the one obvious choice for me, because --no-help is
not supported, of course.  But we can use some more dedicated tests of
negation and double-negation.

René

[PATCH v2 0/5] show negatability of options in short help

[René Scharfe]

Changes since v1: More fallout, still reduce size of final patch:
- Disable negation of "git subtree" options that don't support it.
- Adjust t7900 in contrib/subtree/t to the changed output.
- Adjust git-rev-parse.txt to the changed output.
- Disable negation of --help in t1502.
- Add negation tests to t1502.
- Deduplicate expected output in t1502.

  subtree: disallow --no-{help,quiet,debug,branch,message}
  t1502, docs: disallow --no-help
  t1502: move optionspec help output to a file
  t1502: test option negation
  parse-options: show negatability of options in short help

 Documentation/git-rev-parse.txt    |  10 +--
 contrib/subtree/git-subtree.sh     |  10 +--
 contrib/subtree/t/t7900-subtree.sh |   2 +-
 parse-options.c                    |  10 ++-
 t/t0040-parse-options.sh           |  44 +++++-----
 t/t1502-rev-parse-parseopt.sh      | 131 ++++++++++++-----------------
 t/t1502/.gitattributes             |   1 +
 t/t1502/optionspec-neg             |   8 ++
 t/t1502/optionspec-neg.help        |  11 +++
 t/t1502/optionspec.help            |  39 +++++++++
 10 files changed, 157 insertions(+), 109 deletions(-)
 create mode 100644 t/t1502/.gitattributes
 create mode 100644 t/t1502/optionspec-neg
 create mode 100644 t/t1502/optionspec-neg.help
 create mode 100755 t/t1502/optionspec.help

--
2.41.0

[PATCH v2 1/5] subtree: disallow --no-{help,quiet,debug,branch,message}

[René Scharfe]

"git subtree" only handles the negated variant of the options annotate,
prefix, onto, rejoin, ignore-joins and squash explicitly.  help is
handled by "git rev-parse --parseopt" implicitly, but not its negated
form.  Disable negation for it and the for the rest of the options to
get a helpful error message when trying them.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 contrib/subtree/git-subtree.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 7db4c45676..e0c5d3b0de 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -33,19 +33,19 @@ git subtree split --prefix=<prefix> [<commit>]
 git subtree pull  --prefix=<prefix> <repository> <ref>
 git subtree push  --prefix=<prefix> <repository> <refspec>
 --
-h,help        show the help
-q,quiet       quiet
-d,debug       show debug messages
+h,help!       show the help
+q,quiet!      quiet
+d,debug!      show debug messages
 P,prefix=     the name of the subdir to split out
  options for 'split' (also: 'push')
 annotate=     add a prefix to commit message of new commits
-b,branch=     create a new branch from the split subtree
+b,branch!=    create a new branch from the split subtree
 ignore-joins  ignore prior --rejoin commits
 onto=         try connecting new tree to an existing one
 rejoin        merge the new branch back into HEAD
  options for 'add' and 'merge' (also: 'pull', 'split --rejoin', and 'push --rejoin')
 squash        merge subtree changes as a single commit
-m,message=    use the given message as the commit message for the merge commit
+m,message!=   use the given message as the commit message for the merge commit
 "

 indent=0
--
2.41.0

[PATCH v2 2/5] t1502, docs: disallow --no-help

[René Scharfe]

"git rev-parse --parseopt" handles the built-in options -h and --help,
but not --no-help.  Make test definitions and documentation examples
more realistic by disabling negation using the flag "!".

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 Documentation/git-rev-parse.txt | 2 +-
 t/t1502-rev-parse-parseopt.sh   | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index f26a7591e3..6e8ff9ace1 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -398,7 +398,7 @@ some-command [<options>] <args>...

 some-command does foo and bar!
 --
-h,help    show the help
+h,help!   show the help

 foo       some nifty option --foo
 bar=      some cool option --bar with an argument
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index dd811b7fb4..0cdc6eb8b3 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -9,7 +9,7 @@ test_expect_success 'setup optionspec' '
 |
 |some-command does foo and bar!
 |--
-|h,help    show the help
+|h,help!   show the help
 |
 |foo       some nifty option --foo
 |bar=      some cool option --bar with an argument
@@ -288,7 +288,7 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:"
 	|    [--another-option]
 	|cmd [--yet-another-option]
 	|--
-	|h,help    show the help
+	|h,help!   show the help
 	EOF

 	sed -e "s/^|//" >expect <<-\END_EXPECT &&
@@ -322,7 +322,7 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l
 	|line
 	|blurb
 	|--
-	|h,help    show the help
+	|h,help!   show the help
 	EOF

 	sed -e "s/^|//" >expect <<-\END_EXPECT &&
--
2.41.0

[PATCH v2 3/5] t1502: move optionspec help output to a file

[René Scharfe]

"git rev-parse --parseopt" shows the short help with its description of
all recognized options twice: When called with -h or --help, and after
reporting an unknown option.  Move the one for optionspec into a file
and use it in two tests to deduplicate that part.

"git rev-parse --parseopt -- --h" wraps the help text in "cat <<\EOF"
and "EOF".  Keep that part in the file to use it as is in the test that
needs it and simply remove it in the other one using sed.

Disable whitespace checking for the file using an attribute, as we need
to keep its spaces intact and wouldn't want a stray --whitespace=fix to
turn them into tabs.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 t/t1502-rev-parse-parseopt.sh | 79 ++++-------------------------------
 t/t1502/.gitattributes        |  1 +
 t/t1502/optionspec.help       | 34 +++++++++++++++
 3 files changed, 42 insertions(+), 72 deletions(-)
 create mode 100644 t/t1502/.gitattributes
 create mode 100755 t/t1502/optionspec.help

diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 0cdc6eb8b3..813ee5872f 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -58,44 +58,8 @@ EOF
 '

 test_expect_success 'test --parseopt help output' '
-	sed -e "s/^|//" >expect <<\END_EXPECT &&
-|cat <<\EOF
-|usage: some-command [options] <args>...
-|
-|    some-command does foo and bar!
-|
-|    -h, --help            show the help
-|    --foo                 some nifty option --foo
-|    --bar ...             some cool option --bar with an argument
-|    -b, --baz             a short and long option
-|
-|An option group Header
-|    -C[...]               option C with an optional argument
-|    -d, --data[=...]      short and long option with an optional argument
-|
-|Argument hints
-|    -B <arg>              short option required argument
-|    --bar2 <arg>          long option required argument
-|    -e, --fuz <with-space>
-|                          short and long option required argument
-|    -s[<some>]            short option optional argument
-|    --long[=<data>]       long option optional argument
-|    -g, --fluf[=<path>]   short and long option optional argument
-|    --longest <very-long-argument-hint>
-|                          a very long argument hint
-|    --pair <key=value>    with an equals sign in the hint
-|    --aswitch             help te=t contains? fl*g characters!`
-|    --bswitch <hint>      hint has trailing tab character
-|    --cswitch             switch has trailing tab character
-|    --short-hint <a>      with a one symbol hint
-|
-|Extras
-|    --extra1              line above used to cause a segfault but no longer does
-|
-|EOF
-END_EXPECT
 	test_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec &&
-	test_cmp expect output
+	test_cmp "$TEST_DIRECTORY/t1502/optionspec.help" output
 '

 test_expect_success 'test --parseopt help output no switches' '
@@ -140,41 +104,12 @@ END_EXPECT
 '

 test_expect_success 'test --parseopt invalid switch help output' '
-	sed -e "s/^|//" >expect <<\END_EXPECT &&
-|error: unknown option `does-not-exist'\''
-|usage: some-command [options] <args>...
-|
-|    some-command does foo and bar!
-|
-|    -h, --help            show the help
-|    --foo                 some nifty option --foo
-|    --bar ...             some cool option --bar with an argument
-|    -b, --baz             a short and long option
-|
-|An option group Header
-|    -C[...]               option C with an optional argument
-|    -d, --data[=...]      short and long option with an optional argument
-|
-|Argument hints
-|    -B <arg>              short option required argument
-|    --bar2 <arg>          long option required argument
-|    -e, --fuz <with-space>
-|                          short and long option required argument
-|    -s[<some>]            short option optional argument
-|    --long[=<data>]       long option optional argument
-|    -g, --fluf[=<path>]   short and long option optional argument
-|    --longest <very-long-argument-hint>
-|                          a very long argument hint
-|    --pair <key=value>    with an equals sign in the hint
-|    --aswitch             help te=t contains? fl*g characters!`
-|    --bswitch <hint>      hint has trailing tab character
-|    --cswitch             switch has trailing tab character
-|    --short-hint <a>      with a one symbol hint
-|
-|Extras
-|    --extra1              line above used to cause a segfault but no longer does
-|
-END_EXPECT
+	{
+		cat <<-\EOF &&
+		error: unknown option `does-not-exist'\''
+		EOF
+		sed -e 1d -e \$d <"$TEST_DIRECTORY/t1502/optionspec.help"
+	} >expect &&
 	test_expect_code 129 git rev-parse --parseopt -- --does-not-exist 1>/dev/null 2>output < optionspec &&
 	test_cmp expect output
 '
diff --git a/t/t1502/.gitattributes b/t/t1502/.gitattributes
new file mode 100644
index 0000000000..562b12e16e
--- /dev/null
+++ b/t/t1502/.gitattributes
@@ -0,0 +1 @@
+* -whitespace
diff --git a/t/t1502/optionspec.help b/t/t1502/optionspec.help
new file mode 100755
index 0000000000..844eac6704
--- /dev/null
+++ b/t/t1502/optionspec.help
@@ -0,0 +1,34 @@
+cat <<\EOF
+usage: some-command [options] <args>...
+
+    some-command does foo and bar!
+
+    -h, --help            show the help
+    --foo                 some nifty option --foo
+    --bar ...             some cool option --bar with an argument
+    -b, --baz             a short and long option
+
+An option group Header
+    -C[...]               option C with an optional argument
+    -d, --data[=...]      short and long option with an optional argument
+
+Argument hints
+    -B <arg>              short option required argument
+    --bar2 <arg>          long option required argument
+    -e, --fuz <with-space>
+                          short and long option required argument
+    -s[<some>]            short option optional argument
+    --long[=<data>]       long option optional argument
+    -g, --fluf[=<path>]   short and long option optional argument
+    --longest <very-long-argument-hint>
+                          a very long argument hint
+    --pair <key=value>    with an equals sign in the hint
+    --aswitch             help te=t contains? fl*g characters!`
+    --bswitch <hint>      hint has trailing tab character
+    --cswitch             switch has trailing tab character
+    --short-hint <a>      with a one symbol hint
+
+Extras
+    --extra1              line above used to cause a segfault but no longer does
+
+EOF
--
2.41.0

[PATCH v2 4/5] t1502: test option negation

[René Scharfe]

Add tests for checking the "git rev-parse --parseopt" flag "!" and
whether options can be negated with a "no-" prefix.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 t/t1502-rev-parse-parseopt.sh | 44 +++++++++++++++++++++++++++++++++++
 t/t1502/optionspec-neg        |  8 +++++++
 t/t1502/optionspec-neg.help   | 11 +++++++++
 3 files changed, 63 insertions(+)
 create mode 100644 t/t1502/optionspec-neg
 create mode 100644 t/t1502/optionspec-neg.help

diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 813ee5872f..0f7c2db4c0 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -3,6 +3,22 @@
 test_description='test git rev-parse --parseopt'
 . ./test-lib.sh

+check_invalid_long_option () {
+	spec="$1"
+	opt="$2"
+	test_expect_success "test --parseopt invalid switch $opt help output for $spec" '
+		{
+			cat <<-\EOF &&
+			error: unknown option `'${opt#--}\''
+			EOF
+			sed -e 1d -e \$d <"$TEST_DIRECTORY/t1502/$spec.help"
+		} >expect &&
+		test_expect_code 129 git rev-parse --parseopt -- $opt \
+			2>output <"$TEST_DIRECTORY/t1502/$spec" &&
+		test_cmp expect output
+	'
+}
+
 test_expect_success 'setup optionspec' '
 	sed -e "s/^|//" >optionspec <<\EOF
 |some-command [options] <args>...
@@ -278,4 +294,32 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l
 	test_cmp expect actual
 '

+test_expect_success 'test --parseopt help output for optionspec-neg' '
+	test_expect_code 129 git rev-parse --parseopt -- \
+		-h >output <"$TEST_DIRECTORY/t1502/optionspec-neg" &&
+	test_cmp "$TEST_DIRECTORY/t1502/optionspec-neg.help" output
+'
+
+test_expect_success 'test --parseopt valid options for optionspec-neg' '
+	cat >expect <<-\EOF &&
+	set -- --foo --no-foo --no-bar --positive-only --no-negative --
+	EOF
+	git rev-parse --parseopt -- <"$TEST_DIRECTORY/t1502/optionspec-neg" >output \
+	       --foo --no-foo --no-bar --positive-only --no-negative &&
+	test_cmp expect output
+'
+
+test_expect_success 'test --parseopt positivated option for optionspec-neg' '
+	cat >expect <<-\EOF &&
+	set -- --no-no-bar --no-no-bar --
+	EOF
+	git rev-parse --parseopt -- <"$TEST_DIRECTORY/t1502/optionspec-neg" >output \
+	       --no-no-bar --bar &&
+	test_cmp expect output
+'
+
+check_invalid_long_option optionspec-neg --no-positive-only
+check_invalid_long_option optionspec-neg --negative
+check_invalid_long_option optionspec-neg --no-no-negative
+
 test_done
diff --git a/t/t1502/optionspec-neg b/t/t1502/optionspec-neg
new file mode 100644
index 0000000000..28992ee303
--- /dev/null
+++ b/t/t1502/optionspec-neg
@@ -0,0 +1,8 @@
+some-command [options] <args>...
+
+some-command does foo and bar!
+--
+foo		can be negated
+no-bar		can be positivated
+positive-only!	cannot be negated
+no-negative!	cannot be positivated
diff --git a/t/t1502/optionspec-neg.help b/t/t1502/optionspec-neg.help
new file mode 100644
index 0000000000..591f4dcd9f
--- /dev/null
+++ b/t/t1502/optionspec-neg.help
@@ -0,0 +1,11 @@
+cat <<\EOF
+usage: some-command [options] <args>...
+
+    some-command does foo and bar!
+
+    --foo                 can be negated
+    --no-bar              can be positivated
+    --positive-only       cannot be negated
+    --no-negative         cannot be positivated
+
+EOF
--
2.41.0

[PATCH v2 5/5] parse-options: show negatability of options in short help

[René Scharfe]

Add a "[no-]" prefix to options without the flag PARSE_OPT_NONEG to
document the fact that you can negate them.

This looks a bit strange for options that already start with "no-", e.g.
for the option --no-name of git show-branch:

    --[no-]no-name        suppress naming strings

You can actually use --no-no-name as an alias of --name, so the short
help is not wrong.  If we strip off any of the "no-"s, we lose either
the ability to see if the remaining one belongs to the documented
variant or to see if it can be negated.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 Documentation/git-rev-parse.txt    |  8 +++---
 contrib/subtree/t/t7900-subtree.sh |  2 +-
 parse-options.c                    | 10 +++++--
 t/t0040-parse-options.sh           | 44 ++++++++++++++++--------------
 t/t1502-rev-parse-parseopt.sh      |  2 +-
 t/t1502/optionspec-neg.help        |  4 +--
 t/t1502/optionspec.help            | 35 ++++++++++++++----------
 7 files changed, 60 insertions(+), 45 deletions(-)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 6e8ff9ace1..6a4968f68a 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -424,10 +424,10 @@ usage: some-command [<options>] <args>...
     some-command does foo and bar!

     -h, --help            show the help
-    --foo                 some nifty option --foo
-    --bar ...             some cool option --bar with an argument
-    --baz <arg>           another cool option --baz with a named argument
-    --qux[=<path>]        qux may take a path argument but has meaning by itself
+    --[no-]foo            some nifty option --foo
+    --[no-]bar ...        some cool option --bar with an argument
+    --[no-]baz <arg>      another cool option --baz with a named argument
+    --[no-]qux[=<path>]   qux may take a path argument but has meaning by itself

 An option group Header
     -C[...]               option C with an optional argument
diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
index 341c169eca..49a21dd7c9 100755
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -71,7 +71,7 @@ test_expect_success 'shows short help text for -h' '
 	test_expect_code 129 git subtree -h >out 2>err &&
 	test_must_be_empty err &&
 	grep -e "^ *or: git subtree pull" out &&
-	grep -e --annotate out
+	grep -F -e "--[no-]annotate" out
 '

 #
diff --git a/parse-options.c b/parse-options.c
index f8a155ee13..6323ca191d 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1136,8 +1136,14 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
 		}
 		if (opts->long_name && opts->short_name)
 			pos += fprintf(outfile, ", ");
-		if (opts->long_name)
-			pos += fprintf(outfile, "--%s", opts->long_name);
+		if (opts->long_name) {
+			const char *long_name = opts->long_name;
+			if (opts->flags & PARSE_OPT_NONEG)
+				pos += fprintf(outfile, "--%s", long_name);
+			else
+				pos += fprintf(outfile, "--[no-]%s", long_name);
+		}
+
 		if (opts->type == OPTION_NUMBER)
 			pos += utf8_fprintf(outfile, _("-NUM"));

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 7d7ecfd571..f39758d2ef 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -13,29 +13,32 @@ usage: test-tool parse-options <options>

     A helper function for the parse-options API.

-    --yes                 get a boolean
-    -D, --no-doubt        begins with 'no-'
+    --[no-]yes            get a boolean
+    -D, --[no-]no-doubt   begins with 'no-'
     -B, --no-fear         be brave
-    -b, --boolean         increment by one
-    -4, --or4             bitwise-or boolean with ...0100
-    --neg-or4             same as --no-or4
+    -b, --[no-]boolean    increment by one
+    -4, --[no-]or4        bitwise-or boolean with ...0100
+    --[no-]neg-or4        same as --no-or4

-    -i, --integer <n>     get a integer
+    -i, --[no-]integer <n>
+                          get a integer
     -j <n>                get a integer, too
     -m, --magnitude <n>   get a magnitude
-    --set23               set integer to 23
+    --[no-]set23          set integer to 23
     --mode1               set integer to 1 (cmdmode option)
     --mode2               set integer to 2 (cmdmode option)
-    -L, --length <str>    get length of <str>
-    -F, --file <file>     set file to <file>
+    -L, --[no-]length <str>
+                          get length of <str>
+    -F, --[no-]file <file>
+                          set file to <file>

 String options
-    -s, --string <string>
+    -s, --[no-]string <string>
                           get a string
-    --string2 <str>       get another string
-    --st <st>             get another string (pervert ordering)
+    --[no-]string2 <str>  get another string
+    --[no-]st <st>        get another string (pervert ordering)
     -o <str>              get another string
-    --list <str>          add str to list
+    --[no-]list <str>     add str to list

 Magic arguments
     -NUM                  set integer to NUM
@@ -44,16 +47,17 @@ Magic arguments
     --no-ambiguous        negative ambiguity

 Standard options
-    --abbrev[=<n>]        use <n> digits to display object names
-    -v, --verbose         be verbose
-    -n, --dry-run         dry run
-    -q, --quiet           be quiet
-    --expect <string>     expected output in the variable dump
+    --[no-]abbrev[=<n>]   use <n> digits to display object names
+    -v, --[no-]verbose    be verbose
+    -n, --[no-]dry-run    dry run
+    -q, --[no-]quiet      be quiet
+    --[no-]expect <string>
+                          expected output in the variable dump

 Alias
-    -A, --alias-source <string>
+    -A, --[no-]alias-source <string>
                           get a string
-    -Z, --alias-target <string>
+    -Z, --[no-]alias-target <string>
                           alias of --alias-source

 EOF
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 0f7c2db4c0..f0737593c3 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -111,7 +111,7 @@ test_expect_success 'test --parseopt help-all output hidden switches' '
 |
 |    some-command does foo and bar!
 |
-|    --hidden1             A hidden switch
+|    --[no-]hidden1        A hidden switch
 |
 |EOF
 END_EXPECT
diff --git a/t/t1502/optionspec-neg.help b/t/t1502/optionspec-neg.help
index 591f4dcd9f..156e5f0ed9 100644
--- a/t/t1502/optionspec-neg.help
+++ b/t/t1502/optionspec-neg.help
@@ -3,8 +3,8 @@ usage: some-command [options] <args>...

     some-command does foo and bar!

-    --foo                 can be negated
-    --no-bar              can be positivated
+    --[no-]foo            can be negated
+    --[no-]no-bar         can be positivated
     --positive-only       cannot be negated
     --no-negative         cannot be positivated

diff --git a/t/t1502/optionspec.help b/t/t1502/optionspec.help
index 844eac6704..64e8ce9f98 100755
--- a/t/t1502/optionspec.help
+++ b/t/t1502/optionspec.help
@@ -4,31 +4,36 @@ usage: some-command [options] <args>...
     some-command does foo and bar!

     -h, --help            show the help
-    --foo                 some nifty option --foo
-    --bar ...             some cool option --bar with an argument
-    -b, --baz             a short and long option
+    --[no-]foo            some nifty option --foo
+    --[no-]bar ...        some cool option --bar with an argument
+    -b, --[no-]baz        a short and long option

 An option group Header
     -C[...]               option C with an optional argument
-    -d, --data[=...]      short and long option with an optional argument
+    -d, --[no-]data[=...]
+                          short and long option with an optional argument

 Argument hints
     -B <arg>              short option required argument
-    --bar2 <arg>          long option required argument
-    -e, --fuz <with-space>
+    --[no-]bar2 <arg>     long option required argument
+    -e, --[no-]fuz <with-space>
                           short and long option required argument
     -s[<some>]            short option optional argument
-    --long[=<data>]       long option optional argument
-    -g, --fluf[=<path>]   short and long option optional argument
-    --longest <very-long-argument-hint>
+    --[no-]long[=<data>]  long option optional argument
+    -g, --[no-]fluf[=<path>]
+                          short and long option optional argument
+    --[no-]longest <very-long-argument-hint>
                           a very long argument hint
-    --pair <key=value>    with an equals sign in the hint
-    --aswitch             help te=t contains? fl*g characters!`
-    --bswitch <hint>      hint has trailing tab character
-    --cswitch             switch has trailing tab character
-    --short-hint <a>      with a one symbol hint
+    --[no-]pair <key=value>
+                          with an equals sign in the hint
+    --[no-]aswitch        help te=t contains? fl*g characters!`
+    --[no-]bswitch <hint>
+                          hint has trailing tab character
+    --[no-]cswitch        switch has trailing tab character
+    --[no-]short-hint <a>
+                          with a one symbol hint

 Extras
-    --extra1              line above used to cause a segfault but no longer does
+    --[no-]extra1         line above used to cause a segfault but no longer does

 EOF
--
2.41.0

Re: [PATCH] ls-tree: fix --no-full-name

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Am 21.07.23 um 22:09 schrieb Junio C Hamano:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> -    -D, --no-doubt        begins with 'no-'
>>> +    -D, --[no-]no-doubt   begins with 'no-'
>>
>> Hmph, I really really loved the neat trick to allow "no-doubt"
>> option to be "positivised" by _dropping_ the leading "no-" at around
>> 0f1930c5 (parse-options: allow positivation of options starting,
>> with no-, 2012-02-25).
>
> Yeah, if there is a better way to document A) that the "no-" is optional
> and B) whether it's present by default, I'm all ears.

Some options take "no-" prefix while some others do not, so
indicating that "this can take negative forms" vs "this do not take
negative forms" by "--[no-]xyzzy" and "--frotz" makes sense.

Yikes.  There are tons of options whose names begin with "no-" and
marked PARSE_OPT_NONEG, so "an option '--no-nitfol' that does not
have the 'no-' part in [brackets] can drop 'no-' to make it
positive" would not fly as a rule/convention.

If we do not mind getting longer, we could say

	-D, --no-doubt, --doubt

and explain in the description that --no-doubt is the same as -D and
--doubt is the default.  It is making the developers responsible for
clarify, which is not very satisfying.

We may not reject "--no-no-doubt" but with the positivization
support, double negation is not something we'd encourage without
feeling embarrassed.

> Hard to say for me -- these are synthetic test cases and I lack context
> to make that decision.  In t0040 (t/helper/test-parse-options.c rather)
> we do have a few PARSE_OPT_NONEG uses already.  In t1502 we need to add
> some...

True.  The test coverage will be hurt if we start futzing with
OPT_NONEG bit "randomly".

>>> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
>>> index dd811b7fb4..0a67e2dd4f 100755
>>> --- a/t/t1502-rev-parse-parseopt.sh
>>> +++ b/t/t1502-rev-parse-parseopt.sh
>>> @@ -64,33 +64,38 @@ test_expect_success 'test --parseopt help output' '
>>>  |
>>>  |    some-command does foo and bar!
>>>  |
>>> -|    -h, --help            show the help
>>> -|    --foo                 some nifty option --foo
>>> -|    --bar ...             some cool option --bar with an argument
>>> -|    -b, --baz             a short and long option
>>> +|    -h, --[no-]help       show the help
>>
>> Indeed it is amusing, but we probably should give PARSE_OPT_NONEG
>> appropriately, instead of changing the expectations, for many of the
>> changes we see here, I think.
>
> ... and --help is the one obvious choice for me, because --no-help is
> not supported, of course.  But we can use some more dedicated tests of
> negation and double-negation.

Yeah.

Re: [PATCH] ls-tree: fix --no-full-name

[René Scharfe]

Am 24.07.23 um 20:51 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> Am 21.07.23 um 22:09 schrieb Junio C Hamano:
>>> René Scharfe <l.s.r@web.de> writes:
>>>
>>>> -    -D, --no-doubt        begins with 'no-'
>>>> +    -D, --[no-]no-doubt   begins with 'no-'
>>>
>>> Hmph, I really really loved the neat trick to allow "no-doubt"
>>> option to be "positivised" by _dropping_ the leading "no-" at around
>>> 0f1930c5 (parse-options: allow positivation of options starting,
>>> with no-, 2012-02-25).
>>
>> Yeah, if there is a better way to document A) that the "no-" is optional
>> and B) whether it's present by default, I'm all ears.
>
> Some options take "no-" prefix while some others do not, so
> indicating that "this can take negative forms" vs "this do not take
> negative forms" by "--[no-]xyzzy" and "--frotz" makes sense.
>
> Yikes.  There are tons of options whose names begin with "no-" and
> marked PARSE_OPT_NONEG, so "an option '--no-nitfol' that does not
> have the 'no-' part in [brackets] can drop 'no-' to make it
> positive" would not fly as a rule/convention.
>
> If we do not mind getting longer, we could say
>
> 	-D, --no-doubt, --doubt
>
> and explain in the description that --no-doubt is the same as -D and
> --doubt is the default.  It is making the developers responsible for
> clarify, which is not very satisfying.

Adjusting all explanations manually seems quite tedious.

> We may not reject "--no-no-doubt" but with the positivization
> support, double negation is not something we'd encourage without
> feeling embarrassed.

Right.  Perhaps --[[no-]no-]doubt?  Looks a bit silly with its nested
brackets, but it's more correct, because it documents all three accepted
forms, including the no-less one.

René

Re: [PATCH] ls-tree: fix --no-full-name

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Am 24.07.23 um 20:51 schrieb Junio C Hamano:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> Am 21.07.23 um 22:09 schrieb Junio C Hamano:
>>>> René Scharfe <l.s.r@web.de> writes:
>>>>
>>>>> -    -D, --no-doubt        begins with 'no-'
>>>>> +    -D, --[no-]no-doubt   begins with 'no-'
>>>>
>>>> Hmph, I really really loved the neat trick to allow "no-doubt"
>>>> option to be "positivised" by _dropping_ the leading "no-" at around
>>>> 0f1930c5 (parse-options: allow positivation of options starting,
>>>> with no-, 2012-02-25).
>>>
>>> Yeah, if there is a better way to document A) that the "no-" is optional
>>> and B) whether it's present by default, I'm all ears.
>>
>> Some options take "no-" prefix while some others do not, so
>> indicating that "this can take negative forms" vs "this do not take
>> negative forms" by "--[no-]xyzzy" and "--frotz" makes sense.
>>
>> Yikes.  There are tons of options whose names begin with "no-" and
>> marked PARSE_OPT_NONEG, so "an option '--no-nitfol' that does not
>> have the 'no-' part in [brackets] can drop 'no-' to make it
>> positive" would not fly as a rule/convention.
>>
>> If we do not mind getting longer, we could say
>>
>> 	-D, --no-doubt, --doubt
>>
>> and explain in the description that --no-doubt is the same as -D and
>> --doubt is the default.  It is making the developers responsible for
>> clarify, which is not very satisfying.
>
> Adjusting all explanations manually seems quite tedious.
>
>> We may not reject "--no-no-doubt" but with the positivization
>> support, double negation is not something we'd encourage without
>> feeling embarrassed.
>
> Right.  Perhaps --[[no-]no-]doubt?  Looks a bit silly with its nested
> brackets, but it's more correct, because it documents all three accepted
> forms, including the no-less one.

It may look a bit silly but looks very tempting.  Also it is not
much longer than "--[no-]no-doubt".

Re: [PATCH] ls-tree: fix --no-full-name

[René Scharfe]

Am 24.07.23 um 22:50 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> Am 24.07.23 um 20:51 schrieb Junio C Hamano:
>>> René Scharfe <l.s.r@web.de> writes:
>>>
>>>> Am 21.07.23 um 22:09 schrieb Junio C Hamano:
>>>>> René Scharfe <l.s.r@web.de> writes:
>>>>>
>>>>>> -    -D, --no-doubt        begins with 'no-'
>>>>>> +    -D, --[no-]no-doubt   begins with 'no-'
>>>>>
>>>>> Hmph, I really really loved the neat trick to allow "no-doubt"
>>>>> option to be "positivised" by _dropping_ the leading "no-" at around
>>>>> 0f1930c5 (parse-options: allow positivation of options starting,
>>>>> with no-, 2012-02-25).
>>>>
>>>> Yeah, if there is a better way to document A) that the "no-" is optional
>>>> and B) whether it's present by default, I'm all ears.
>>>
>>> Some options take "no-" prefix while some others do not, so
>>> indicating that "this can take negative forms" vs "this do not take
>>> negative forms" by "--[no-]xyzzy" and "--frotz" makes sense.
>>>
>>> Yikes.  There are tons of options whose names begin with "no-" and
>>> marked PARSE_OPT_NONEG, so "an option '--no-nitfol' that does not
>>> have the 'no-' part in [brackets] can drop 'no-' to make it
>>> positive" would not fly as a rule/convention.
>>>
>>> If we do not mind getting longer, we could say
>>>
>>> 	-D, --no-doubt, --doubt
>>>
>>> and explain in the description that --no-doubt is the same as -D and
>>> --doubt is the default.  It is making the developers responsible for
>>> clarify, which is not very satisfying.
>>
>> Adjusting all explanations manually seems quite tedious.
>>
>>> We may not reject "--no-no-doubt" but with the positivization
>>> support, double negation is not something we'd encourage without
>>> feeling embarrassed.
>>
>> Right.  Perhaps --[[no-]no-]doubt?  Looks a bit silly with its nested
>> brackets, but it's more correct, because it documents all three accepted
>> forms, including the no-less one.
>
> It may look a bit silly but looks very tempting.  Also it is not
> much longer than "--[no-]no-doubt".

Yes, it's quite compact.  But is it they still legible?

    --no-index            find in contents not managed by git
    --[no-]no-index       find in contents not managed by git
    --[[no-]no-]index     find in contents not managed by git
    --[no-[no-]]index     find in contents not managed by git

The last two document all three variants, but is it still obvious that
the help text is supposed to be about the one with a single "no-"?
That's something that has to be learned, I suspect.  No good making the
short help too cryptic.  Hmm, how about:

    --no-index, --[no-[no-]]index
                          find in contents not managed by git

Somewhat redundant, but highlights the documented variant.

René

Re: [PATCH] ls-tree: fix --no-full-name

[Phillip Wood]

On 28/07/2023 07:12, René Scharfe wrote:
>>> Right.  Perhaps --[[no-]no-]doubt?  Looks a bit silly with its nested
>>> brackets, but it's more correct, because it documents all three accepted
>>> forms, including the no-less one.
>>
>> It may look a bit silly but looks very tempting.  Also it is not
>> much longer than "--[no-]no-doubt".
> 
> Yes, it's quite compact.  But is it they still legible?
> 
>      --no-index            find in contents not managed by git
>      --[no-]no-index       find in contents not managed by git
>      --[[no-]no-]index     find in contents not managed by git
>      --[no-[no-]]index     find in contents not managed by git
> 
> The last two document all three variants, but is it still obvious that
> the help text is supposed to be about the one with a single "no-"?
> That's something that has to be learned, I suspect.  No good making the
> short help too cryptic.  Hmm, how about:
> 
>      --no-index, --[no-[no-]]index
>                            find in contents not managed by git

I think spelling out the positive and negative options separately makes 
it much clearer, but in that case I think we'd be better just to show

	--no-index, --index

adding "[no-[no]]" is just going to confuse users. If we had a 
convention of "[<short>, ]<positive long>; <negative long>" then I think 
it should be clear to users how to negate a given option

     -v, --invert-match; --no-invert-match
				show non-matching lines
     -I, --no-index; --index	find in contents not managed by git

Spelling out both versions is a bit verbose but I think it is worth it 
to avoid "--[no-]no-index"

One other thought is to mark options that can be negated with a symbol 
like '*' and add a footnote saying those options can be negated.

Best Wishes

Phillip

Re: [PATCH] ls-tree: fix --no-full-name

[René Scharfe]

Am 28.07.23 um 11:45 schrieb Phillip Wood:
> On 28/07/2023 07:12, René Scharfe wrote:
>>>> Right.  Perhaps --[[no-]no-]doubt?  Looks a bit silly with its nested
>>>> brackets, but it's more correct, because it documents all three accepted
>>>> forms, including the no-less one.
>>>
>>> It may look a bit silly but looks very tempting.  Also it is not
>>> much longer than "--[no-]no-doubt".
>>
>> Yes, it's quite compact.  But is it they still legible?
>>
>>      --no-index            find in contents not managed by git
>>      --[no-]no-index       find in contents not managed by git
>>      --[[no-]no-]index     find in contents not managed by git
>>      --[no-[no-]]index     find in contents not managed by git
>>
>> The last two document all three variants, but is it still obvious that
>> the help text is supposed to be about the one with a single "no-"?
>> That's something that has to be learned, I suspect.  No good making the
>> short help too cryptic.  Hmm, how about:
>>
>>      --no-index, --[no-[no-]]index
>>                            find in contents not managed by git
>
> I think spelling out the positive and negative options separately
> makes it much clearer, but in that case I think we'd be better just
> to show
>
>     --no-index, --index
>
> adding "[no-[no]]" is just going to confuse users. If we had a
> convention of "[<short>, ]<positive long>; <negative long>" then I
> think it should be clear to users how to negate a given option
>
>     -v, --invert-match; --no-invert-match
>                 show non-matching lines
>     -I, --no-index; --index    find in contents not managed by git
>
> Spelling out both versions is a bit verbose but I think it is worth
> it to avoid "--[no-]no-index"

I kinda like it, even though it is quite verbose and adds a new syntax
element.

A bit more verbose still: Document the negative form on its own line
with a generated description -- requires no new syntax:

    -v, --invert-match         show non-matching lines
    --no-invert-match          opposite of --invert-match, default
    -I, --no-index             find in contents not managed by git
    --index                    opposite of --no-index, default

> One other thought is to mark options that can be negated with a
> symbol like '*' and add a footnote saying those options can be
> negated.

Sure, adding a layer of indirection would work.

All these imperfect solutions make me wish we could get rid of the
problem, e.g. by converting all "no-" options to their positive variants
and mentioning that they are the default.  Won't fly, though, because
some of them have short forms, and we don't follow the convention of
uppercase short options negating lowercase ones, so we have to document
them anyway.

René

Re: [PATCH] ls-tree: fix --no-full-name

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> A bit more verbose still: Document the negative form on its own line
> with a generated description -- requires no new syntax:
>
>     -v, --invert-match         show non-matching lines
>     --no-invert-match          opposite of --invert-match, default
>     -I, --no-index             find in contents not managed by git
>     --index                    opposite of --no-index, default

I would expect _("opposite of %s, default") is acceptable by l10n
folks, and assuming it is, the above would be a good approach.

> All these imperfect solutions make me wish we could get rid of the
> problem, e.g. by converting all "no-" options to their positive variants
> and mentioning that they are the default.  Won't fly, though,
> because ...

True.

Thanks.

Re: [PATCH] ls-tree: fix --no-full-name

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> René Scharfe <l.s.r@web.de> writes:
>
>> A bit more verbose still: Document the negative form on its own line
>> with a generated description -- requires no new syntax:
>>
>>     -v, --invert-match         show non-matching lines
>>     --no-invert-match          opposite of --invert-match, default
>>     -I, --no-index             find in contents not managed by git
>>     --index                    opposite of --no-index, default
>
> I would expect _("opposite of %s, default") is acceptable by l10n
> folks, and assuming it is, the above would be a good approach.

I was seeing what is likely to be in the -rc1 that will happen in
next week, and noticed that this discussion is left unconcluded.  I
am tempted to declare that the latest iteration that shows the
negation of "--no-foo" as "--[no-]no-foo" is "good enough" for now,
and leave the above improvement as one potential future direction.

Objections?  Otherwise the 5-patch series will be in 'next'.

Thanks.

Re: [PATCH] ls-tree: fix --no-full-name

[Phillip Wood]

On 04/08/2023 17:40, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> A bit more verbose still: Document the negative form on its own line
>>> with a generated description -- requires no new syntax:
>>>
>>>      -v, --invert-match         show non-matching lines
>>>      --no-invert-match          opposite of --invert-match, default
>>>      -I, --no-index             find in contents not managed by git
>>>      --index                    opposite of --no-index, default
>>
>> I would expect _("opposite of %s, default") is acceptable by l10n
>> folks, and assuming it is, the above would be a good approach.
> 
> I was seeing what is likely to be in the -rc1 that will happen in
> next week, and noticed that this discussion is left unconcluded.  I
> am tempted to declare that the latest iteration that shows the
> negation of "--no-foo" as "--[no-]no-foo" is "good enough" for now,
> and leave the above improvement as one potential future direction.

While it could certainly be improved in the future I think 
"--[no-]no-foo" is probably good enough. I definitely prefer it over 
"--[[no-]no]-foo"

Best Wishes

Phillip

> Objections?  Otherwise the 5-patch series will be in 'next'.
> 
> Thanks.

Re: [PATCH] ls-tree: fix --no-full-name

[René Scharfe]

Am 04.08.23 um 21:48 schrieb Phillip Wood:
> On 04/08/2023 17:40, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> René Scharfe <l.s.r@web.de> writes:
>>>
>>>> A bit more verbose still: Document the negative form on its own line
>>>> with a generated description -- requires no new syntax:
>>>>
>>>>      -v, --invert-match         show non-matching lines
>>>>      --no-invert-match          opposite of --invert-match, default
>>>>      -I, --no-index             find in contents not managed by git
>>>>      --index                    opposite of --no-index, default
>>>
>>> I would expect _("opposite of %s, default") is acceptable by l10n
>>> folks, and assuming it is, the above would be a good approach.

The ", default" part is not correct in all cases, though, in particular
when the default depends on the output being a terminal, or arguably
when it's dependent on a config setting.  We better drop it from the
generated message.

>> I was seeing what is likely to be in the -rc1 that will happen in
>> next week, and noticed that this discussion is left unconcluded.  I
>> am tempted to declare that the latest iteration that shows the
>> negation of "--no-foo" as "--[no-]no-foo" is "good enough" for now,
>> and leave the above improvement as one potential future direction.
>
> While it could certainly be improved in the future I think
> "--[no-]no-foo" is probably good enough. I definitely prefer it over
> "--[[no-]no]-foo"

Generating help lines for the opposite variant of all negatable options
feels quite spammy.  It almost doubles the length of the short help
because we have so many of them.  Doing that only for --no-... options
is much more compatc because most options are positive:

    -v, --[no-]invert-match    show non-matching lines
    -I, --no-index             find in contents not managed by git
    --index                    opposite of --no-index

>> Objections?  Otherwise the 5-patch series will be in 'next'.

Patch 5 has a trivial merge conflict in t/t0040-parse-options.sh due to
448abbba63 (short help: allow multi-line opthelp, 2023-07-18), which is
easily resolved by adding --longhelp, and a silent automatic mismerge of
t/t1502/optionspec.help due to c512643e67 (short help: allow a gap
smaller than USAGE_GAP, 2023-07-18), which requires removing three line
newlines.

How about dropping patch 5 for now?  We don't need to rush this pretty
intrusive change of output.  And the first four patches are worthwhile
in their own right, I think.

René

[PATCH v3 0/8] show negatability of options in short help

[René Scharfe]

Changes since v2:
- Rebase, affects only patch 5.
- Add patch 7 for documenting negative options on their own line.
- Add preparatory patch 6.
- Add bonus patch 8.

  subtree: disallow --no-{help,quiet,debug,branch,message}
  t1502, docs: disallow --no-help
  t1502: move optionspec help output to a file
  t1502: test option negation
  parse-options: show negatability of options in short help
  parse-options: factor out usage_indent() and usage_padding()
  parse-options: no --[no-]no-...
  parse-options: simplify usage_padding()

 Documentation/git-rev-parse.txt    |  10 +--
 contrib/subtree/git-subtree.sh     |  10 +--
 contrib/subtree/t/t7900-subtree.sh |   2 +-
 parse-options.c                    |  70 ++++++++++-----
 t/t0040-parse-options.sh           |  44 +++++-----
 t/t1502-rev-parse-parseopt.sh      | 131 ++++++++++++-----------------
 t/t1502/.gitattributes             |   1 +
 t/t1502/optionspec-neg             |   8 ++
 t/t1502/optionspec-neg.help        |  12 +++
 t/t1502/optionspec.help            |  36 ++++++++
 10 files changed, 199 insertions(+), 125 deletions(-)
 create mode 100644 t/t1502/.gitattributes
 create mode 100644 t/t1502/optionspec-neg
 create mode 100644 t/t1502/optionspec-neg.help
 create mode 100755 t/t1502/optionspec.help

Range-Diff gegen v2:
1:  26c03bd70c = 1:  ee280b3484 subtree: disallow --no-{help,quiet,debug,branch,message}
2:  ad9e7d1393 = 2:  556e79ce55 t1502, docs: disallow --no-help
3:  1bb68a4a40 = 3:  274e65ec1c t1502: move optionspec help output to a file
4:  47ab167d1c = 4:  89b0455305 t1502: test option negation
5:  961c5dfcf7 ! 5:  7cd3abcff7 parse-options: show negatability of options in short help
    @@ t/t0040-parse-options.sh: usage: test-tool parse-options <options>
     +                          set file to <file>

      String options
    --    -s, --string <string>
    -+    -s, --[no-]string <string>
    -                           get a string
    +-    -s, --string <string> get a string
     -    --string2 <str>       get another string
     -    --st <st>             get another string (pervert ordering)
    ++    -s, --[no-]string <string>
    ++                          get a string
     +    --[no-]string2 <str>  get another string
     +    --[no-]st <st>        get another string (pervert ordering)
          -o <str>              get another string
    +     --longhelp            help text of this entry
    +                           spans multiple lines
     -    --list <str>          add str to list
     +    --[no-]list <str>     add str to list

    @@ t/t1502/optionspec.help: usage: some-command [options] <args>...
      An option group Header
          -C[...]               option C with an optional argument
     -    -d, --data[=...]      short and long option with an optional argument
    -+    -d, --[no-]data[=...]
    -+                          short and long option with an optional argument
    ++    -d, --[no-]data[=...] short and long option with an optional argument

      Argument hints
          -B <arg>              short option required argument
    @@ t/t1502/optionspec.help: usage: some-command [options] <args>...
     +    --[no-]pair <key=value>
     +                          with an equals sign in the hint
     +    --[no-]aswitch        help te=t contains? fl*g characters!`
    -+    --[no-]bswitch <hint>
    -+                          hint has trailing tab character
    ++    --[no-]bswitch <hint> hint has trailing tab character
     +    --[no-]cswitch        switch has trailing tab character
    -+    --[no-]short-hint <a>
    -+                          with a one symbol hint
    ++    --[no-]short-hint <a> with a one symbol hint

      Extras
     -    --extra1              line above used to cause a segfault but no longer does
-:  ---------- > 6:  46dcdb902d parse-options: factor out usage_indent() and usage_padding()
-:  ---------- > 7:  fdeca0d6d2 parse-options: no --[no-]no-...
-:  ---------- > 8:  08b2d1e861 parse-options: simplify usage_padding()
--
2.41.0

[PATCH v3 1/8] subtree: disallow --no-{help,quiet,debug,branch,message}

[René Scharfe]

"git subtree" only handles the negated variant of the options annotate,
prefix, onto, rejoin, ignore-joins and squash explicitly.  help is
handled by "git rev-parse --parseopt" implicitly, but not its negated
form.  Disable negation for it and the for the rest of the options to
get a helpful error message when trying them.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 contrib/subtree/git-subtree.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 7db4c45676..e0c5d3b0de 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -33,19 +33,19 @@ git subtree split --prefix=<prefix> [<commit>]
 git subtree pull  --prefix=<prefix> <repository> <ref>
 git subtree push  --prefix=<prefix> <repository> <refspec>
 --
-h,help        show the help
-q,quiet       quiet
-d,debug       show debug messages
+h,help!       show the help
+q,quiet!      quiet
+d,debug!      show debug messages
 P,prefix=     the name of the subdir to split out
  options for 'split' (also: 'push')
 annotate=     add a prefix to commit message of new commits
-b,branch=     create a new branch from the split subtree
+b,branch!=    create a new branch from the split subtree
 ignore-joins  ignore prior --rejoin commits
 onto=         try connecting new tree to an existing one
 rejoin        merge the new branch back into HEAD
  options for 'add' and 'merge' (also: 'pull', 'split --rejoin', and 'push --rejoin')
 squash        merge subtree changes as a single commit
-m,message=    use the given message as the commit message for the merge commit
+m,message!=   use the given message as the commit message for the merge commit
 "

 indent=0
--
2.41.0

[PATCH v3 2/8] t1502, docs: disallow --no-help

[René Scharfe]

"git rev-parse --parseopt" handles the built-in options -h and --help,
but not --no-help.  Make test definitions and documentation examples
more realistic by disabling negation.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 Documentation/git-rev-parse.txt | 2 +-
 t/t1502-rev-parse-parseopt.sh   | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index f26a7591e3..6e8ff9ace1 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -398,7 +398,7 @@ some-command [<options>] <args>...

 some-command does foo and bar!
 --
-h,help    show the help
+h,help!   show the help

 foo       some nifty option --foo
 bar=      some cool option --bar with an argument
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index dd811b7fb4..0cdc6eb8b3 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -9,7 +9,7 @@ test_expect_success 'setup optionspec' '
 |
 |some-command does foo and bar!
 |--
-|h,help    show the help
+|h,help!   show the help
 |
 |foo       some nifty option --foo
 |bar=      some cool option --bar with an argument
@@ -288,7 +288,7 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:"
 	|    [--another-option]
 	|cmd [--yet-another-option]
 	|--
-	|h,help    show the help
+	|h,help!   show the help
 	EOF

 	sed -e "s/^|//" >expect <<-\END_EXPECT &&
@@ -322,7 +322,7 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l
 	|line
 	|blurb
 	|--
-	|h,help    show the help
+	|h,help!   show the help
 	EOF

 	sed -e "s/^|//" >expect <<-\END_EXPECT &&
--
2.41.0

[PATCH v3 3/8] t1502: move optionspec help output to a file

[René Scharfe]

"git rev-parse --parseopt" shows the short help with its description of
all recognized options twice: When called with -h or --help, and after
reporting an unknown option.  Move the one for optionspec into a file
and use it in two tests to deduplicate that part.

"git rev-parse --parseopt -- --h" wraps the help text in "cat <<\EOF"
and "EOF".  Keep that part in the file to use it as is in the test that
needs it and simply remove it in the other one using sed.

Disable whitespace checking for the file using an attribute, as we need
to keep its spaces intact and wouldn't want a stray --whitespace=fix
turn them into tabs.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 t/t1502-rev-parse-parseopt.sh | 79 ++++-------------------------------
 t/t1502/.gitattributes        |  1 +
 t/t1502/optionspec.help       | 34 +++++++++++++++
 3 files changed, 42 insertions(+), 72 deletions(-)
 create mode 100644 t/t1502/.gitattributes
 create mode 100755 t/t1502/optionspec.help

diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 0cdc6eb8b3..813ee5872f 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -58,44 +58,8 @@ EOF
 '

 test_expect_success 'test --parseopt help output' '
-	sed -e "s/^|//" >expect <<\END_EXPECT &&
-|cat <<\EOF
-|usage: some-command [options] <args>...
-|
-|    some-command does foo and bar!
-|
-|    -h, --help            show the help
-|    --foo                 some nifty option --foo
-|    --bar ...             some cool option --bar with an argument
-|    -b, --baz             a short and long option
-|
-|An option group Header
-|    -C[...]               option C with an optional argument
-|    -d, --data[=...]      short and long option with an optional argument
-|
-|Argument hints
-|    -B <arg>              short option required argument
-|    --bar2 <arg>          long option required argument
-|    -e, --fuz <with-space>
-|                          short and long option required argument
-|    -s[<some>]            short option optional argument
-|    --long[=<data>]       long option optional argument
-|    -g, --fluf[=<path>]   short and long option optional argument
-|    --longest <very-long-argument-hint>
-|                          a very long argument hint
-|    --pair <key=value>    with an equals sign in the hint
-|    --aswitch             help te=t contains? fl*g characters!`
-|    --bswitch <hint>      hint has trailing tab character
-|    --cswitch             switch has trailing tab character
-|    --short-hint <a>      with a one symbol hint
-|
-|Extras
-|    --extra1              line above used to cause a segfault but no longer does
-|
-|EOF
-END_EXPECT
 	test_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec &&
-	test_cmp expect output
+	test_cmp "$TEST_DIRECTORY/t1502/optionspec.help" output
 '

 test_expect_success 'test --parseopt help output no switches' '
@@ -140,41 +104,12 @@ END_EXPECT
 '

 test_expect_success 'test --parseopt invalid switch help output' '
-	sed -e "s/^|//" >expect <<\END_EXPECT &&
-|error: unknown option `does-not-exist'\''
-|usage: some-command [options] <args>...
-|
-|    some-command does foo and bar!
-|
-|    -h, --help            show the help
-|    --foo                 some nifty option --foo
-|    --bar ...             some cool option --bar with an argument
-|    -b, --baz             a short and long option
-|
-|An option group Header
-|    -C[...]               option C with an optional argument
-|    -d, --data[=...]      short and long option with an optional argument
-|
-|Argument hints
-|    -B <arg>              short option required argument
-|    --bar2 <arg>          long option required argument
-|    -e, --fuz <with-space>
-|                          short and long option required argument
-|    -s[<some>]            short option optional argument
-|    --long[=<data>]       long option optional argument
-|    -g, --fluf[=<path>]   short and long option optional argument
-|    --longest <very-long-argument-hint>
-|                          a very long argument hint
-|    --pair <key=value>    with an equals sign in the hint
-|    --aswitch             help te=t contains? fl*g characters!`
-|    --bswitch <hint>      hint has trailing tab character
-|    --cswitch             switch has trailing tab character
-|    --short-hint <a>      with a one symbol hint
-|
-|Extras
-|    --extra1              line above used to cause a segfault but no longer does
-|
-END_EXPECT
+	{
+		cat <<-\EOF &&
+		error: unknown option `does-not-exist'\''
+		EOF
+		sed -e 1d -e \$d <"$TEST_DIRECTORY/t1502/optionspec.help"
+	} >expect &&
 	test_expect_code 129 git rev-parse --parseopt -- --does-not-exist 1>/dev/null 2>output < optionspec &&
 	test_cmp expect output
 '
diff --git a/t/t1502/.gitattributes b/t/t1502/.gitattributes
new file mode 100644
index 0000000000..562b12e16e
--- /dev/null
+++ b/t/t1502/.gitattributes
@@ -0,0 +1 @@
+* -whitespace
diff --git a/t/t1502/optionspec.help b/t/t1502/optionspec.help
new file mode 100755
index 0000000000..844eac6704
--- /dev/null
+++ b/t/t1502/optionspec.help
@@ -0,0 +1,34 @@
+cat <<\EOF
+usage: some-command [options] <args>...
+
+    some-command does foo and bar!
+
+    -h, --help            show the help
+    --foo                 some nifty option --foo
+    --bar ...             some cool option --bar with an argument
+    -b, --baz             a short and long option
+
+An option group Header
+    -C[...]               option C with an optional argument
+    -d, --data[=...]      short and long option with an optional argument
+
+Argument hints
+    -B <arg>              short option required argument
+    --bar2 <arg>          long option required argument
+    -e, --fuz <with-space>
+                          short and long option required argument
+    -s[<some>]            short option optional argument
+    --long[=<data>]       long option optional argument
+    -g, --fluf[=<path>]   short and long option optional argument
+    --longest <very-long-argument-hint>
+                          a very long argument hint
+    --pair <key=value>    with an equals sign in the hint
+    --aswitch             help te=t contains? fl*g characters!`
+    --bswitch <hint>      hint has trailing tab character
+    --cswitch             switch has trailing tab character
+    --short-hint <a>      with a one symbol hint
+
+Extras
+    --extra1              line above used to cause a segfault but no longer does
+
+EOF
--
2.41.0

[PATCH v3 4/8] t1502: test option negation

[René Scharfe]

Add tests for checking the "git rev-parse --parseopt" flag "!" and
whether options can be negated with a "no-" prefix.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 t/t1502-rev-parse-parseopt.sh | 44 +++++++++++++++++++++++++++++++++++
 t/t1502/optionspec-neg        |  8 +++++++
 t/t1502/optionspec-neg.help   | 11 +++++++++
 3 files changed, 63 insertions(+)
 create mode 100644 t/t1502/optionspec-neg
 create mode 100644 t/t1502/optionspec-neg.help

diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 813ee5872f..0f7c2db4c0 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -3,6 +3,22 @@
 test_description='test git rev-parse --parseopt'
 . ./test-lib.sh

+check_invalid_long_option () {
+	spec="$1"
+	opt="$2"
+	test_expect_success "test --parseopt invalid switch $opt help output for $spec" '
+		{
+			cat <<-\EOF &&
+			error: unknown option `'${opt#--}\''
+			EOF
+			sed -e 1d -e \$d <"$TEST_DIRECTORY/t1502/$spec.help"
+		} >expect &&
+		test_expect_code 129 git rev-parse --parseopt -- $opt \
+			2>output <"$TEST_DIRECTORY/t1502/$spec" &&
+		test_cmp expect output
+	'
+}
+
 test_expect_success 'setup optionspec' '
 	sed -e "s/^|//" >optionspec <<\EOF
 |some-command [options] <args>...
@@ -278,4 +294,32 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l
 	test_cmp expect actual
 '

+test_expect_success 'test --parseopt help output for optionspec-neg' '
+	test_expect_code 129 git rev-parse --parseopt -- \
+		-h >output <"$TEST_DIRECTORY/t1502/optionspec-neg" &&
+	test_cmp "$TEST_DIRECTORY/t1502/optionspec-neg.help" output
+'
+
+test_expect_success 'test --parseopt valid options for optionspec-neg' '
+	cat >expect <<-\EOF &&
+	set -- --foo --no-foo --no-bar --positive-only --no-negative --
+	EOF
+	git rev-parse --parseopt -- <"$TEST_DIRECTORY/t1502/optionspec-neg" >output \
+	       --foo --no-foo --no-bar --positive-only --no-negative &&
+	test_cmp expect output
+'
+
+test_expect_success 'test --parseopt positivated option for optionspec-neg' '
+	cat >expect <<-\EOF &&
+	set -- --no-no-bar --no-no-bar --
+	EOF
+	git rev-parse --parseopt -- <"$TEST_DIRECTORY/t1502/optionspec-neg" >output \
+	       --no-no-bar --bar &&
+	test_cmp expect output
+'
+
+check_invalid_long_option optionspec-neg --no-positive-only
+check_invalid_long_option optionspec-neg --negative
+check_invalid_long_option optionspec-neg --no-no-negative
+
 test_done
diff --git a/t/t1502/optionspec-neg b/t/t1502/optionspec-neg
new file mode 100644
index 0000000000..28992ee303
--- /dev/null
+++ b/t/t1502/optionspec-neg
@@ -0,0 +1,8 @@
+some-command [options] <args>...
+
+some-command does foo and bar!
+--
+foo		can be negated
+no-bar		can be positivated
+positive-only!	cannot be negated
+no-negative!	cannot be positivated
diff --git a/t/t1502/optionspec-neg.help b/t/t1502/optionspec-neg.help
new file mode 100644
index 0000000000..591f4dcd9f
--- /dev/null
+++ b/t/t1502/optionspec-neg.help
@@ -0,0 +1,11 @@
+cat <<\EOF
+usage: some-command [options] <args>...
+
+    some-command does foo and bar!
+
+    --foo                 can be negated
+    --no-bar              can be positivated
+    --positive-only       cannot be negated
+    --no-negative         cannot be positivated
+
+EOF
--
2.41.0

[PATCH v3 5/8] parse-options: show negatability of options in short help

[René Scharfe]

Add a "[no-]" prefix to options without the flag PARSE_OPT_NONEG to
document the fact that you can negate them.

This looks a bit strange for options that already start with "no-", e.g.
for the option --no-name of git show-branch:

    --[no-]no-name        suppress naming strings

You can actually use --no-no-name as an alias of --name, so the short
help is not wrong.  If we strip off any of the "no-"s, we lose either
the ability to see if the remaining one belongs to the documented
variant or to see if it can be negated.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 Documentation/git-rev-parse.txt    |  8 +++---
 contrib/subtree/t/t7900-subtree.sh |  2 +-
 parse-options.c                    | 10 +++++--
 t/t0040-parse-options.sh           | 45 +++++++++++++++++-------------
 t/t1502-rev-parse-parseopt.sh      |  2 +-
 t/t1502/optionspec-neg.help        |  4 +--
 t/t1502/optionspec.help            | 32 +++++++++++----------
 7 files changed, 58 insertions(+), 45 deletions(-)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 6e8ff9ace1..6a4968f68a 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -424,10 +424,10 @@ usage: some-command [<options>] <args>...
     some-command does foo and bar!

     -h, --help            show the help
-    --foo                 some nifty option --foo
-    --bar ...             some cool option --bar with an argument
-    --baz <arg>           another cool option --baz with a named argument
-    --qux[=<path>]        qux may take a path argument but has meaning by itself
+    --[no-]foo            some nifty option --foo
+    --[no-]bar ...        some cool option --bar with an argument
+    --[no-]baz <arg>      another cool option --baz with a named argument
+    --[no-]qux[=<path>]   qux may take a path argument but has meaning by itself

 An option group Header
     -C[...]               option C with an optional argument
diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
index 341c169eca..49a21dd7c9 100755
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -71,7 +71,7 @@ test_expect_success 'shows short help text for -h' '
 	test_expect_code 129 git subtree -h >out 2>err &&
 	test_must_be_empty err &&
 	grep -e "^ *or: git subtree pull" out &&
-	grep -e --annotate out
+	grep -F -e "--[no-]annotate" out
 '

 #
diff --git a/parse-options.c b/parse-options.c
index 87c9fae634..b750bf91cd 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1137,8 +1137,14 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
 		}
 		if (opts->long_name && opts->short_name)
 			pos += fprintf(outfile, ", ");
-		if (opts->long_name)
-			pos += fprintf(outfile, "--%s", opts->long_name);
+		if (opts->long_name) {
+			const char *long_name = opts->long_name;
+			if (opts->flags & PARSE_OPT_NONEG)
+				pos += fprintf(outfile, "--%s", long_name);
+			else
+				pos += fprintf(outfile, "--[no-]%s", long_name);
+		}
+
 		if (opts->type == OPTION_NUMBER)
 			pos += utf8_fprintf(outfile, _("-NUM"));

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index e19a199636..1dfc431d52 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -13,30 +13,34 @@ usage: test-tool parse-options <options>

     A helper function for the parse-options API.

-    --yes                 get a boolean
-    -D, --no-doubt        begins with 'no-'
+    --[no-]yes            get a boolean
+    -D, --[no-]no-doubt   begins with 'no-'
     -B, --no-fear         be brave
-    -b, --boolean         increment by one
-    -4, --or4             bitwise-or boolean with ...0100
-    --neg-or4             same as --no-or4
+    -b, --[no-]boolean    increment by one
+    -4, --[no-]or4        bitwise-or boolean with ...0100
+    --[no-]neg-or4        same as --no-or4

-    -i, --integer <n>     get a integer
+    -i, --[no-]integer <n>
+                          get a integer
     -j <n>                get a integer, too
     -m, --magnitude <n>   get a magnitude
-    --set23               set integer to 23
+    --[no-]set23          set integer to 23
     --mode1               set integer to 1 (cmdmode option)
     --mode2               set integer to 2 (cmdmode option)
-    -L, --length <str>    get length of <str>
-    -F, --file <file>     set file to <file>
+    -L, --[no-]length <str>
+                          get length of <str>
+    -F, --[no-]file <file>
+                          set file to <file>

 String options
-    -s, --string <string> get a string
-    --string2 <str>       get another string
-    --st <st>             get another string (pervert ordering)
+    -s, --[no-]string <string>
+                          get a string
+    --[no-]string2 <str>  get another string
+    --[no-]st <st>        get another string (pervert ordering)
     -o <str>              get another string
     --longhelp            help text of this entry
                           spans multiple lines
-    --list <str>          add str to list
+    --[no-]list <str>     add str to list

 Magic arguments
     -NUM                  set integer to NUM
@@ -45,16 +49,17 @@ Magic arguments
     --no-ambiguous        negative ambiguity

 Standard options
-    --abbrev[=<n>]        use <n> digits to display object names
-    -v, --verbose         be verbose
-    -n, --dry-run         dry run
-    -q, --quiet           be quiet
-    --expect <string>     expected output in the variable dump
+    --[no-]abbrev[=<n>]   use <n> digits to display object names
+    -v, --[no-]verbose    be verbose
+    -n, --[no-]dry-run    dry run
+    -q, --[no-]quiet      be quiet
+    --[no-]expect <string>
+                          expected output in the variable dump

 Alias
-    -A, --alias-source <string>
+    -A, --[no-]alias-source <string>
                           get a string
-    -Z, --alias-target <string>
+    -Z, --[no-]alias-target <string>
                           alias of --alias-source

 EOF
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 0f7c2db4c0..f0737593c3 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -111,7 +111,7 @@ test_expect_success 'test --parseopt help-all output hidden switches' '
 |
 |    some-command does foo and bar!
 |
-|    --hidden1             A hidden switch
+|    --[no-]hidden1        A hidden switch
 |
 |EOF
 END_EXPECT
diff --git a/t/t1502/optionspec-neg.help b/t/t1502/optionspec-neg.help
index 591f4dcd9f..156e5f0ed9 100644
--- a/t/t1502/optionspec-neg.help
+++ b/t/t1502/optionspec-neg.help
@@ -3,8 +3,8 @@ usage: some-command [options] <args>...

     some-command does foo and bar!

-    --foo                 can be negated
-    --no-bar              can be positivated
+    --[no-]foo            can be negated
+    --[no-]no-bar         can be positivated
     --positive-only       cannot be negated
     --no-negative         cannot be positivated

diff --git a/t/t1502/optionspec.help b/t/t1502/optionspec.help
index 844eac6704..cbdd54d41b 100755
--- a/t/t1502/optionspec.help
+++ b/t/t1502/optionspec.help
@@ -4,31 +4,33 @@ usage: some-command [options] <args>...
     some-command does foo and bar!

     -h, --help            show the help
-    --foo                 some nifty option --foo
-    --bar ...             some cool option --bar with an argument
-    -b, --baz             a short and long option
+    --[no-]foo            some nifty option --foo
+    --[no-]bar ...        some cool option --bar with an argument
+    -b, --[no-]baz        a short and long option

 An option group Header
     -C[...]               option C with an optional argument
-    -d, --data[=...]      short and long option with an optional argument
+    -d, --[no-]data[=...] short and long option with an optional argument

 Argument hints
     -B <arg>              short option required argument
-    --bar2 <arg>          long option required argument
-    -e, --fuz <with-space>
+    --[no-]bar2 <arg>     long option required argument
+    -e, --[no-]fuz <with-space>
                           short and long option required argument
     -s[<some>]            short option optional argument
-    --long[=<data>]       long option optional argument
-    -g, --fluf[=<path>]   short and long option optional argument
-    --longest <very-long-argument-hint>
+    --[no-]long[=<data>]  long option optional argument
+    -g, --[no-]fluf[=<path>]
+                          short and long option optional argument
+    --[no-]longest <very-long-argument-hint>
                           a very long argument hint
-    --pair <key=value>    with an equals sign in the hint
-    --aswitch             help te=t contains? fl*g characters!`
-    --bswitch <hint>      hint has trailing tab character
-    --cswitch             switch has trailing tab character
-    --short-hint <a>      with a one symbol hint
+    --[no-]pair <key=value>
+                          with an equals sign in the hint
+    --[no-]aswitch        help te=t contains? fl*g characters!`
+    --[no-]bswitch <hint> hint has trailing tab character
+    --[no-]cswitch        switch has trailing tab character
+    --[no-]short-hint <a> with a one symbol hint

 Extras
-    --extra1              line above used to cause a segfault but no longer does
+    --[no-]extra1         line above used to cause a segfault but no longer does

 EOF
--
2.41.0

[PATCH v3 6/8] parse-options: factor out usage_indent() and usage_padding()

[René Scharfe]

Extract functions for printing spaces before and after options.  We'll
need them in the next commit.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 parse-options.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index b750bf91cd..4b76fc81e9 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1020,9 +1020,28 @@ static int usage_argh(const struct option *opts, FILE *outfile)
 	return utf8_fprintf(outfile, s, opts->argh ? _(opts->argh) : _("..."));
 }

+static int usage_indent(FILE *outfile)
+{
+	return fprintf(outfile, "    ");
+}
+
 #define USAGE_OPTS_WIDTH 24
 #define USAGE_GAP         2

+static void usage_padding(FILE *outfile, size_t pos)
+{
+	int pad;
+	if (pos == USAGE_OPTS_WIDTH + 1)
+		pad = -1;
+	else if (pos <= USAGE_OPTS_WIDTH)
+		pad = USAGE_OPTS_WIDTH - pos;
+	else {
+		fputc('\n', outfile);
+		pad = USAGE_OPTS_WIDTH;
+	}
+	fprintf(outfile, "%*s", pad + USAGE_GAP, "");
+}
+
 static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 							 const char * const *usagestr,
 							 const struct option *opts,
@@ -1108,7 +1127,6 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t

 	for (; opts->type != OPTION_END; opts++) {
 		size_t pos;
-		int pad;
 		const char *cp, *np;

 		if (opts->type == OPTION_SUBCOMMAND)
@@ -1128,7 +1146,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
 			need_newline = 0;
 		}

-		pos = fprintf(outfile, "    ");
+		pos = usage_indent(outfile);
 		if (opts->short_name) {
 			if (opts->flags & PARSE_OPT_NODASH)
 				pos += fprintf(outfile, "%c", opts->short_name);
@@ -1152,16 +1170,8 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
 		    !(opts->flags & PARSE_OPT_NOARG))
 			pos += usage_argh(opts, outfile);

-		if (pos == USAGE_OPTS_WIDTH + 1)
-			pad = -1;
-		else if (pos <= USAGE_OPTS_WIDTH)
-			pad = USAGE_OPTS_WIDTH - pos;
-		else {
-			fputc('\n', outfile);
-			pad = USAGE_OPTS_WIDTH;
-		}
 		if (opts->type == OPTION_ALIAS) {
-			fprintf(outfile, "%*s", pad + USAGE_GAP, "");
+			usage_padding(outfile, pos);
 			fprintf_ln(outfile, _("alias of --%s"),
 				   (const char *)opts->value);
 			continue;
@@ -1169,12 +1179,11 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t

 		for (cp = _(opts->help); *cp; cp = np) {
 			np = strchrnul(cp, '\n');
-			fprintf(outfile,
-				"%*s%.*s\n", pad + USAGE_GAP, "",
-				(int)(np - cp), cp);
+			usage_padding(outfile, pos);
+			fprintf(outfile, "%.*s\n", (int)(np - cp), cp);
 			if (*np)
 				np++;
-			pad = USAGE_OPTS_WIDTH;
+			pos = 0;
 		}
 	}
 	fputc('\n', outfile);
--
2.41.0

[PATCH v3 7/8] parse-options: no --[no-]no-...

[René Scharfe]

Avoid showing an optional "no-" for options that already start with a
"no-" in the short help, as that double negation is confusing.  Document
the opposite variant on its own line with a generated help text instead,
unless it's defined and documented explicitly already.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 parse-options.c             | 25 ++++++++++++++++++++++++-
 t/t0040-parse-options.sh    |  3 ++-
 t/t1502/optionspec-neg.help |  3 ++-
 3 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 4b76fc81e9..4a8d380ceb 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1042,11 +1042,22 @@ static void usage_padding(FILE *outfile, size_t pos)
 	fprintf(outfile, "%*s", pad + USAGE_GAP, "");
 }

+static const struct option *find_option_by_long_name(const struct option *opts,
+						     const char *long_name)
+{
+	for (; opts->type != OPTION_END; opts++) {
+		if (opts->long_name && !strcmp(opts->long_name, long_name))
+			return opts;
+	}
+	return NULL;
+}
+
 static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 							 const char * const *usagestr,
 							 const struct option *opts,
 							 int full, int err)
 {
+	const struct option *all_opts = opts;
 	FILE *outfile = err ? stderr : stdout;
 	int need_newline;

@@ -1128,6 +1139,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
 	for (; opts->type != OPTION_END; opts++) {
 		size_t pos;
 		const char *cp, *np;
+		const char *positive_name = NULL;

 		if (opts->type == OPTION_SUBCOMMAND)
 			continue;
@@ -1157,7 +1169,8 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
 			pos += fprintf(outfile, ", ");
 		if (opts->long_name) {
 			const char *long_name = opts->long_name;
-			if (opts->flags & PARSE_OPT_NONEG)
+			if ((opts->flags & PARSE_OPT_NONEG) ||
+			    skip_prefix(long_name, "no-", &positive_name))
 				pos += fprintf(outfile, "--%s", long_name);
 			else
 				pos += fprintf(outfile, "--[no-]%s", long_name);
@@ -1185,6 +1198,16 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
 				np++;
 			pos = 0;
 		}
+
+		if (positive_name) {
+			if (find_option_by_long_name(all_opts, positive_name))
+				continue;
+			pos = usage_indent(outfile);
+			pos += fprintf(outfile, "--%s", positive_name);
+			usage_padding(outfile, pos);
+			fprintf_ln(outfile, _("opposite of --no-%s"),
+				   positive_name);
+		}
 	}
 	fputc('\n', outfile);

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 1dfc431d52..a0ad6192d6 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -14,7 +14,8 @@ usage: test-tool parse-options <options>
     A helper function for the parse-options API.

     --[no-]yes            get a boolean
-    -D, --[no-]no-doubt   begins with 'no-'
+    -D, --no-doubt        begins with 'no-'
+    --doubt               opposite of --no-doubt
     -B, --no-fear         be brave
     -b, --[no-]boolean    increment by one
     -4, --[no-]or4        bitwise-or boolean with ...0100
diff --git a/t/t1502/optionspec-neg.help b/t/t1502/optionspec-neg.help
index 156e5f0ed9..4cd3c83e55 100644
--- a/t/t1502/optionspec-neg.help
+++ b/t/t1502/optionspec-neg.help
@@ -4,7 +4,8 @@ usage: some-command [options] <args>...
     some-command does foo and bar!

     --[no-]foo            can be negated
-    --[no-]no-bar         can be positivated
+    --no-bar              can be positivated
+    --bar                 opposite of --no-bar
     --positive-only       cannot be negated
     --no-negative         cannot be positivated

--
2.41.0

[PATCH v3 8/8] parse-options: simplify usage_padding()

[René Scharfe]

c512643e67 (short help: allow a gap smaller than USAGE_GAP, 2023-07-18)
effectively did away with the two-space gap between options and their
description; one space is enough now.  Incorporate USAGE_GAP into
USAGE_OPTS_WIDTH, merge the two cases with enough space on the line and
incorporate the newline into the format for the remaining case.  The
output remains the same.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
Removed an extra empty line added mid-submission, so the cover letter
stats are off by one.

 parse-options.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 4a8d380ceb..b00d868816 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1025,21 +1025,14 @@ static int usage_indent(FILE *outfile)
 	return fprintf(outfile, "    ");
 }

-#define USAGE_OPTS_WIDTH 24
-#define USAGE_GAP         2
+#define USAGE_OPTS_WIDTH 26

 static void usage_padding(FILE *outfile, size_t pos)
 {
-	int pad;
-	if (pos == USAGE_OPTS_WIDTH + 1)
-		pad = -1;
-	else if (pos <= USAGE_OPTS_WIDTH)
-		pad = USAGE_OPTS_WIDTH - pos;
-	else {
-		fputc('\n', outfile);
-		pad = USAGE_OPTS_WIDTH;
-	}
-	fprintf(outfile, "%*s", pad + USAGE_GAP, "");
+	if (pos < USAGE_OPTS_WIDTH)
+		fprintf(outfile, "%*s", USAGE_OPTS_WIDTH - (int)pos, "");
+	else
+		fprintf(outfile, "\n%*s", USAGE_OPTS_WIDTH, "");
 }

 static const struct option *find_option_by_long_name(const struct option *opts,
--
2.41.0

Re: [PATCH v3 8/8] parse-options: simplify usage_padding()

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> c512643e67 (short help: allow a gap smaller than USAGE_GAP, 2023-07-18)
> effectively did away with the two-space gap between options and their
> description; one space is enough now.

My intention was "we strongly prefer two or more, but on a rare
occasion where the leading strong is long and can afford to give us
only one, we take it instead of breaking the line", but stepping
back and thinking about it again, you are absolutely right.  I just
shrunk the minimum gap from 2 to 1.

Re: [PATCH] describe: fix --no-exact-match

[Jeff King]

On Fri, Jul 21, 2023 at 03:41:33PM +0200, René Scharfe wrote:

> +static int option_parse_exact_match(const struct option *opt, const char *arg,
> +				    int unset)
> +{
> +	BUG_ON_OPT_ARG(arg);
> +
> +	max_candidates = unset ? DEFAULT_CANDIDATES : 0;
> +	return 0;
> +}

I wanted to call out a style question here. The "opt" parameter is
unused, since it manipulates the "max_candidates" global directly. I can
add an UNUSED annotation to satisfy -Wunused-parameter, but in such
cases I've usually been modifying them like:

  int *value = opt->value;
  ...
  *value = unset ? DEFAULT_CANDIDATES : 0;

so that the callback operates on the value passed in the options list.

But I see you converted that value to NULL here:

> @@ -568,8 +578,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
>  		OPT_BOOL(0, "long",       &longformat, N_("always use long format")),
>  		OPT_BOOL(0, "first-parent", &first_parent, N_("only follow first parent")),
>  		OPT__ABBREV(&abbrev),
> -		OPT_SET_INT(0, "exact-match", &max_candidates,
> -			    N_("only output exact matches"), 0),
> +		OPT_CALLBACK_F(0, "exact-match", NULL, NULL,
> +			       N_("only output exact matches"),
> +			       PARSE_OPT_NOARG, option_parse_exact_match),

so at least the result does not have the subtle gotcha that existed in
other cases I changed. :)

So before I sent a patch (either to switch to using opt->value, or to
add an UNUSED annotation), I wanted to see what you (or others) thought
between the two. I.e., should we have a rule of "try not to operate on
global data via option callbacks" or is that just being too pedantic for
one-off callbacks like this?

-Peff

Re: [PATCH] describe: fix --no-exact-match

[Jeff King]

On Tue, Aug 08, 2023 at 05:27:21PM -0400, Jeff King wrote:

> So before I sent a patch (either to switch to using opt->value, or to
> add an UNUSED annotation), I wanted to see what you (or others) thought
> between the two. I.e., should we have a rule of "try not to operate on
> global data via option callbacks" or is that just being too pedantic for
> one-off callbacks like this?

Oh, and btw, the same situation exists for the "pack-objects --no-quiet"
earlier in this thread (which is why I was trying to establish the
general approach).

-Peff

Re: [PATCH] describe: fix --no-exact-match

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jul 21, 2023 at 03:41:33PM +0200, René Scharfe wrote:
>
>> +static int option_parse_exact_match(const struct option *opt, const char *arg,
>> +				    int unset)
>> +{
>> +	BUG_ON_OPT_ARG(arg);
>> +
>> +	max_candidates = unset ? DEFAULT_CANDIDATES : 0;
>> +	return 0;
>> +}
>
> I wanted to call out a style question here. The "opt" parameter is
> unused, since it manipulates the "max_candidates" global directly. I can
> add an UNUSED annotation to satisfy -Wunused-parameter, but in such
> cases I've usually been modifying them like:
>
>   int *value = opt->value;
>   ...
>   *value = unset ? DEFAULT_CANDIDATES : 0;
>
> so that the callback operates on the value passed in the options list.

Yeah, I wasn't paying attention to that particular detail, but I
tend to think that using opt->value to point at the variable would
make sense here.  That approach matches what is internally done by
OPT_STRING_LIST() and OPT_STRVEC(), the built-in types that are
implemented via the same OPTION_CALLBACK mechanism.

One good thing about it is that it makes it clear, without looking
at the implementation of the callback function, which variables are
affected only by looking at what OPT_CALLBACK() says.

> So before I sent a patch (either to switch to using opt->value, or to
> add an UNUSED annotation), I wanted to see what you (or others) thought
> between the two. I.e., should we have a rule of "try not to operate on
> global data via option callbacks" or is that just being too pedantic for
> one-off callbacks like this?

So, that was my preference, but I may be missing some obvious
downsides.  I am interested in hearing from René, who often shows
better taste than I do in these cases ;-)

Thanks.

Re: [PATCH] describe: fix --no-exact-match

[Jeff King]

On Tue, Aug 08, 2023 at 06:43:41PM -0700, Junio C Hamano wrote:

> > So before I sent a patch (either to switch to using opt->value, or to
> > add an UNUSED annotation), I wanted to see what you (or others) thought
> > between the two. I.e., should we have a rule of "try not to operate on
> > global data via option callbacks" or is that just being too pedantic for
> > one-off callbacks like this?
> 
> So, that was my preference, but I may be missing some obvious
> downsides.  I am interested in hearing from René, who often shows
> better taste than I do in these cases ;-)

Me too. :) The main downsides, I think, are:

  1. It's a little more ugly.

  2. We lose type safety, as the variable address passes through a void
     pointer (but that is true of all option callbacks).

Here's what it looks like, for reference.

diff --git a/builtin/describe.c b/builtin/describe.c
index b28a4a1f82..718b5c3073 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -561,9 +561,11 @@ static void describe(const char *arg, int last_one)
 static int option_parse_exact_match(const struct option *opt, const char *arg,
 				    int unset)
 {
+	int *val = opt->value;
+
 	BUG_ON_OPT_ARG(arg);
 
-	max_candidates = unset ? DEFAULT_CANDIDATES : 0;
+	*val = unset ? DEFAULT_CANDIDATES : 0;
 	return 0;
 }
 
@@ -578,7 +580,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "long",       &longformat, N_("always use long format")),
 		OPT_BOOL(0, "first-parent", &first_parent, N_("only follow first parent")),
 		OPT__ABBREV(&abbrev),
-		OPT_CALLBACK_F(0, "exact-match", NULL, NULL,
+		OPT_CALLBACK_F(0, "exact-match", &max_candidates, NULL,
 			       N_("only output exact matches"),
 			       PARSE_OPT_NOARG, option_parse_exact_match),
 		OPT_INTEGER(0, "candidates", &max_candidates,
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index d2a162d528..74c2225620 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4120,12 +4120,14 @@ static void add_extra_kept_packs(const struct string_list *names)
 static int option_parse_quiet(const struct option *opt, const char *arg,
 			      int unset)
 {
+	int *val = opt->value;
+
 	BUG_ON_OPT_ARG(arg);
 
 	if (!unset)
-		progress = 0;
-	else if (!progress)
-		progress = 1;
+		*val = 0;
+	else if (!*val)
+		*val = 1;
 	return 0;
 }
 
@@ -4190,7 +4192,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		LIST_OBJECTS_FILTER_INIT;
 
 	struct option pack_objects_options[] = {
-		OPT_CALLBACK_F('q', "quiet", NULL, NULL,
+		OPT_CALLBACK_F('q', "quiet", &progress, NULL,
 			       N_("do not show progress meter"),
 			       PARSE_OPT_NOARG, option_parse_quiet),
 		OPT_SET_INT(0, "progress", &progress,

Re: [PATCH] describe: fix --no-exact-match

[René Scharfe]

Am 09.08.23 um 16:09 schrieb Jeff King:
> On Tue, Aug 08, 2023 at 06:43:41PM -0700, Junio C Hamano wrote:
>
>>> So before I sent a patch (either to switch to using opt->value, or to
>>> add an UNUSED annotation), I wanted to see what you (or others) thought
>>> between the two. I.e., should we have a rule of "try not to operate on
>>> global data via option callbacks" or is that just being too pedantic for
>>> one-off callbacks like this?
>>
>> So, that was my preference, but I may be missing some obvious
>> downsides.  I am interested in hearing from René, who often shows
>> better taste than I do in these cases ;-)
>
> Me too. :)

I'm pretty sure I didn't think much about it, copied the style from the
other callback functions in builtin/pack-objects.c and was glad to not
have to deal with void pointers.

And sorry for the unused parameter warning.  Just checked; there are
170+ of those remaining before we can enable it in developer mode.  :-/
Seems worthwhile, though, especially not slapping UNUSED blindly on
them.

> The main downsides, I think, are:
>
>   1. It's a little more ugly.
>
>   2. We lose type safety, as the variable address passes through a void
>      pointer (but that is true of all option callbacks).

An upside is that we can reuse the callback if we are careful to wire
it up to a variable of the correct type.  Another is that we can use
it on local variables.

Hmm, we could make these callbacks type-safe fairly easily by adding
pointers of all relevant types to struct option, like "int *value_int".
How many types would we need?

   $ git grep -h ' = opt->value;' | sed 's/\*.*$//; s/^ *//' | sort -u | wc -l
   37

Oh.  Do we really need all those?  Anyway, if we added at least the
most common ones, we'd be better off already, I'd expect:

   $ % git grep -h ' = opt->value;' | sed 's/\*.*$//; s/^ *//' | sort | uniq -c | sort -nr | head -10
     29 struct diff_options
     12 int
      7 struct grep_opt
      6 struct rebase_options
      6 struct apply_state
      5 struct strbuf
      5 char
      4 struct note_data
      3 struct string_list
      2 struct strvec

Increasing the size of the struct like that would increase the total
memory footprint by a few KB at most -- tolerable.

> Here's what it looks like, for reference.
>
> diff --git a/builtin/describe.c b/builtin/describe.c
> index b28a4a1f82..718b5c3073 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -561,9 +561,11 @@ static void describe(const char *arg, int last_one)
>  static int option_parse_exact_match(const struct option *opt, const char *arg,
>  				    int unset)
>  {
> +	int *val = opt->value;

This line would assign opt->value_int instead...

> +
>  	BUG_ON_OPT_ARG(arg);
>
> -	max_candidates = unset ? DEFAULT_CANDIDATES : 0;
> +	*val = unset ? DEFAULT_CANDIDATES : 0;
>  	return 0;
>  }
>
> @@ -578,7 +580,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
>  		OPT_BOOL(0, "long",       &longformat, N_("always use long format")),
>  		OPT_BOOL(0, "first-parent", &first_parent, N_("only follow first parent")),
>  		OPT__ABBREV(&abbrev),
> -		OPT_CALLBACK_F(0, "exact-match", NULL, NULL,
> +		OPT_CALLBACK_F(0, "exact-match", &max_candidates, NULL,

... but the macro OP_CALLBACK_F could no longer be used, because we'd
need to select one of the many typed pointers.

Macros like OPT_BOOL could be changed to use the appropriate typed
pointer.

Thoughts?  Too much churn?

>  			       N_("only output exact matches"),
>  			       PARSE_OPT_NOARG, option_parse_exact_match),
>  		OPT_INTEGER(0, "candidates", &max_candidates,
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index d2a162d528..74c2225620 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -4120,12 +4120,14 @@ static void add_extra_kept_packs(const struct string_list *names)
>  static int option_parse_quiet(const struct option *opt, const char *arg,
>  			      int unset)
>  {
> +	int *val = opt->value;
> +
>  	BUG_ON_OPT_ARG(arg);
>
>  	if (!unset)
> -		progress = 0;
> -	else if (!progress)
> -		progress = 1;
> +		*val = 0;
> +	else if (!*val)
> +		*val = 1;
>  	return 0;
>  }
>
> @@ -4190,7 +4192,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>  		LIST_OBJECTS_FILTER_INIT;
>
>  	struct option pack_objects_options[] = {
> -		OPT_CALLBACK_F('q', "quiet", NULL, NULL,
> +		OPT_CALLBACK_F('q', "quiet", &progress, NULL,
>  			       N_("do not show progress meter"),
>  			       PARSE_OPT_NOARG, option_parse_quiet),
>  		OPT_SET_INT(0, "progress", &progress,

Re: [PATCH] describe: fix --no-exact-match

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

>> Here's what it looks like, for reference.
>>
>> diff --git a/builtin/describe.c b/builtin/describe.c
>> index b28a4a1f82..718b5c3073 100644
>> --- a/builtin/describe.c
>> +++ b/builtin/describe.c
>> @@ -561,9 +561,11 @@ static void describe(const char *arg, int last_one)
>>  static int option_parse_exact_match(const struct option *opt, const char *arg,
>>  				    int unset)
>>  {
>> +	int *val = opt->value;
>
> This line would assign opt->value_int instead...
>
>> +
>>  	BUG_ON_OPT_ARG(arg);
>>
>> -	max_candidates = unset ? DEFAULT_CANDIDATES : 0;
>> +	*val = unset ? DEFAULT_CANDIDATES : 0;
>>  	return 0;
>>  }
>> ...
> Thoughts?  Too much churn?

Sorry, but I am not sure what is being proposed.  opt->value_int
would presumably be defined as a pointer to an int.  The calling
side presumably defines the options[] array entry so that the
&max_candidates address is given to opt->value_int and not to
opt->value_someothertype, and if you try to pass a pointer of a
wrong type, it would be caught as a type mismatch by the compiler,
so the above covers both ends (i.e. the address &max_candidates is
passed in the correct member of the struct, and the callback takes
val out of the correct member of the struct), as long as the
callback uses the right type.  If &max_candidates is int now, and
the above code is written to use opt->value_int, anybody who needs
to update max_candidates to ulong must make sure three things match,
i.e. the type of max_candidates variable itself, the member the
caller uses to pass &max_candidates in the struct (i.e. change to
use value_ulong and not value_int), and the type of the local
variable the callback function uses.

But I am failing to imagine how the calling side actually would look
like.  Can we do something along the lines of

    #define OPT_CALLBACK(s, l, v, a, h, cb)
	.short_name = (s),
	.long_name = (l),
	.value_ ## typeof(v) = &v,
	.help = (h),
	.callback = (cb),

with a clever CPP trick?  It sounds like either too much churn or
too much magic or both, at least to me.

Thanks.

Re: [PATCH] describe: fix --no-exact-match

[Jeff King]

On Wed, Aug 09, 2023 at 12:07:07PM -0700, Junio C Hamano wrote:

> But I am failing to imagine how the calling side actually would look
> like.  Can we do something along the lines of
> 
>     #define OPT_CALLBACK(s, l, v, a, h, cb)
> 	.short_name = (s),
> 	.long_name = (l),
> 	.value_ ## typeof(v) = &v,
> 	.help = (h),
> 	.callback = (cb),
> 
> with a clever CPP trick?  It sounds like either too much churn or
> too much magic or both, at least to me.

Sadly, I think "typeof" is not sufficiently portable, and there is no
replacement. You'd have to do something like:

  int my_foo;
  ...
  OPT_CALLBACK('f', "foo", int, &my_foo, ...etc);

Not great, but it might not be _too_ bad given that most helpers like
OPT_BOOL() can just say "int" behind the scenes.

That said, I don't recall these void pointers being a large source of
errors in the past. So while it's a fun type-system puzzle, the
effort/reward ratio might not be favorable.

-Peff

Re: [PATCH] describe: fix --no-exact-match

[Jeff King]

On Wed, Aug 09, 2023 at 06:41:13PM +0200, René Scharfe wrote:

> And sorry for the unused parameter warning.  Just checked; there are
> 170+ of those remaining before we can enable it in developer mode.  :-/
> Seems worthwhile, though, especially not slapping UNUSED blindly on
> them.

I know, I'm working on it. :) There were more than 800 when I started.
I'm hoping to send out the final patches during this 2.43 cycle.

> Oh.  Do we really need all those?  Anyway, if we added at least the
> most common ones, we'd be better off already, I'd expect:
> 
>    $ % git grep -h ' = opt->value;' | sed 's/\*.*$//; s/^ *//' | sort | uniq -c | sort -nr | head -10
>      29 struct diff_options
>      12 int
>       7 struct grep_opt
>       6 struct rebase_options
>       6 struct apply_state
>       5 struct strbuf
>       5 char
>       4 struct note_data
>       3 struct string_list
>       2 struct strvec
> 
> Increasing the size of the struct like that would increase the total
> memory footprint by a few KB at most -- tolerable.

Hmm, I was thinking that "int_value" might not be so bad. But it seems
like a pretty bad layering violation for parse-options.c to know about
"struct apply_state" and so on. That really is what void pointers are
for.

As for size, you should be able to use a union. All of the types inside
the struct are pointers, so they'd all be the same size. Of course then
you lose some of the safety. If the caller assigned to "int_value" that
is distinct from "foo_value", then you can be sure you will get a NULL
and segfault upon accessing foo_value. With a union, you get whatever
type-punning undefined behavior the compiler sees fit. And the point is
making sure the two match.

We really don't care about separate storage, though. We want type
safety. Maybe an option there would be to let the callback function take
the appropriate type, and cast it. I.e., something like:

  /* define a callback taking the real type */
  int my_int_opt(int *value, struct option *opt, ...etc...) { ...body... }

  /* when mentioning a callback, include the type, and make sure that
   * the value and the callback both match it */
  #define OPT_CALLBACK(s, l, type, v, cb, ...etc...) \
    { ..., \
      value.type = (v), \
      callback = (int (*)(type *, struct option *opt, ...etc...), \
    }
  ...
  OPT_CALLBACK('f', "foo", int, &my_local_int, my_int_opt)

I'm pretty sure that falls afoul of the standard, though, because we
eventually need to call that function, and we'll do so through a
function pointer that doesn't match its declaration.

I'm not sure there's a portable and non-insane way of doing what we want
here. At least at compile-time. At run-time you could record type
information in an enum and check it in the callback. That seems like
a lot of work for little reward, though.

> > Here's what it looks like, for reference.
> >
> > diff --git a/builtin/describe.c b/builtin/describe.c
> > index b28a4a1f82..718b5c3073 100644
> > --- a/builtin/describe.c
> > +++ b/builtin/describe.c
> > @@ -561,9 +561,11 @@ static void describe(const char *arg, int last_one)
> >  static int option_parse_exact_match(const struct option *opt, const char *arg,
> >  				    int unset)
> >  {
> > +	int *val = opt->value;
> 
> This line would assign opt->value_int instead...

Right, though you would not even need it. I snuck it in there because it
gives us an implicit cast from the void pointer. Without it, the current
code would have to do:

  *(int *)opt->value = unset ? DEFAULT_CANDIDATES : 0;

which was too ugly (especially the "progress" one which mentions the
value three times). But if you had pre-cast variables, you could do:

  *opt->value.int = unset ? DEFAULT_CANDIDATES : 0;

directly.

-Peff

Re: [PATCH] describe: fix --no-exact-match

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   int my_foo;
>   ...
>   OPT_CALLBACK('f', "foo", int, &my_foo, ...etc);
>
> Not great, but it might not be _too_ bad given that most helpers like
> OPT_BOOL() can just say "int" behind the scenes.
>
> That said, I don't recall these void pointers being a large source of
> errors in the past. So while it's a fun type-system puzzle, the
> effort/reward ratio might not be favorable.

I tend to agree on both counts, it is a fun puzzle, and it probably
is not going to give us sufficient reward.  The fact that "int" and
typeof(my_foo) need to be manually kept straight defeats the "type
safety" justification for this mental exercise.

Thanks.

Re: [PATCH] describe: fix --no-exact-match

[René Scharfe]

Am 10.08.23 um 02:41 schrieb Jeff King:
> On Wed, Aug 09, 2023 at 06:41:13PM +0200, René Scharfe wrote:
>
>> And sorry for the unused parameter warning.  Just checked; there are
>> 170+ of those remaining before we can enable it in developer mode.  :-/
>> Seems worthwhile, though, especially not slapping UNUSED blindly on
>> them.
>
> I know, I'm working on it. :) There were more than 800 when I started.
> I'm hoping to send out the final patches during this 2.43 cycle.
>
>> Oh.  Do we really need all those?  Anyway, if we added at least the
>> most common ones, we'd be better off already, I'd expect:
>>
>>    $ % git grep -h ' = opt->value;' | sed 's/\*.*$//; s/^ *//' | sort | uniq -c | sort -nr | head -10
>>      29 struct diff_options
>>      12 int
>>       7 struct grep_opt
>>       6 struct rebase_options
>>       6 struct apply_state
>>       5 struct strbuf
>>       5 char
>>       4 struct note_data
>>       3 struct string_list
>>       2 struct strvec
>>
>> Increasing the size of the struct like that would increase the total
>> memory footprint by a few KB at most -- tolerable.
>
> Hmm, I was thinking that "int_value" might not be so bad. But it seems
> like a pretty bad layering violation for parse-options.c to know about
> "struct apply_state" and so on. That really is what void pointers are
> for.

True, keeping a list of external types in parse-options.h is clumsy and
ugly.

> As for size, you should be able to use a union. All of the types inside
> the struct are pointers, so they'd all be the same size. Of course then
> you lose some of the safety. If the caller assigned to "int_value" that
> is distinct from "foo_value", then you can be sure you will get a NULL
> and segfault upon accessing foo_value. With a union, you get whatever
> type-punning undefined behavior the compiler sees fit. And the point is
> making sure the two match.

Which makes a union a non-starter -- we need a reliable error.

> We really don't care about separate storage, though. We want type
> safety. Maybe an option there would be to let the callback function take
> the appropriate type, and cast it. I.e., something like:
>
>   /* define a callback taking the real type */
>   int my_int_opt(int *value, struct option *opt, ...etc...) { ...body... }
>
>   /* when mentioning a callback, include the type, and make sure that
>    * the value and the callback both match it */
>   #define OPT_CALLBACK(s, l, type, v, cb, ...etc...) \
>     { ..., \
>       value.type = (v), \
>       callback = (int (*)(type *, struct option *opt, ...etc...), \
>     }
>   ...
>   OPT_CALLBACK('f', "foo", int, &my_local_int, my_int_opt)

The idea is good, even though I don't understand how your specific
example would work.  Dabbled with something similar for typed qsort
(need to clean it up and submit it one of these days).

> I'm pretty sure that falls afoul of the standard, though, because we
> eventually need to call that function, and we'll do so through a
> function pointer that doesn't match its declaration.

Fighting possible type mismatches by adding a certain type mismatch
won't fly..

> I'm not sure there's a portable and non-insane way of doing what we want
> here. At least at compile-time.

We need a wrapper with the correct signature.  The wrapper is plugged
into struct option.  The typed callback is called by the wrapper and
can be used for a type check in the struct macro.  Demo patch below.


> At run-time you could record type
> information in an enum and check it in the callback. That seems like
> a lot of work for little reward, though.

Agreed -- runtime type checks are for scripting languages..

René



diff --git a/builtin/describe.c b/builtin/describe.c
index b28a4a1f82..ce16c36de2 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -558,15 +558,17 @@ static void describe(const char *arg, int last_one)
 	strbuf_release(&sb);
 }

-static int option_parse_exact_match(const struct option *opt, const char *arg,
-				    int unset)
+static int option_parse_exact_match(const struct option *opt UNUSED,
+				    const char *arg, int unset, int *value)
 {
 	BUG_ON_OPT_ARG(arg);

-	max_candidates = unset ? DEFAULT_CANDIDATES : 0;
+	*value = unset ? DEFAULT_CANDIDATES : 0;
 	return 0;
 }

+DEFINE_PARSE_OPT_CB(option_parse_exact_match);
+
 int cmd_describe(int argc, const char **argv, const char *prefix)
 {
 	int contains = 0;
@@ -578,9 +580,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "long",       &longformat, N_("always use long format")),
 		OPT_BOOL(0, "first-parent", &first_parent, N_("only follow first parent")),
 		OPT__ABBREV(&abbrev),
-		OPT_CALLBACK_F(0, "exact-match", NULL, NULL,
-			       N_("only output exact matches"),
-			       PARSE_OPT_NOARG, option_parse_exact_match),
+		OPT_CALLBACK_F_T(0, "exact-match", &max_candidates, NULL,
+				 N_("only output exact matches"),
+				 PARSE_OPT_NOARG, option_parse_exact_match),
 		OPT_INTEGER(0, "candidates", &max_candidates,
 			    N_("consider <n> most recent tags (default: 10)")),
 		OPT_STRING_LIST(0, "match", &patterns, N_("pattern"),
diff --git a/parse-options.h b/parse-options.h
index 57a7fe9d91..f957931cfa 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -67,6 +67,14 @@ enum parse_opt_result {
 struct option;
 typedef int parse_opt_cb(const struct option *, const char *arg, int unset);

+#define DEFINE_PARSE_OPT_CB(name)				\
+static inline int name ## __void(const struct option *opt,	\
+				 const char *arg, int unset)	\
+{								\
+	return name(opt, arg, unset, opt->value);		\
+}								\
+struct option
+
 struct parse_opt_ctx_t;
 typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
 					      const struct option *opt,
@@ -198,6 +206,16 @@ struct option {
 	.flags = (f), \
 	.callback = (cb), \
 }
+#define OPT_CALLBACK_F_T(s, l, v, a, h, f, cb) { \
+	.type = OPTION_CALLBACK, \
+	.short_name = (s) + (0 ? cb(NULL, NULL, 0, (v)) : 0), \
+	.long_name = (l), \
+	.value = (v), \
+	.argh = (a), \
+	.help = (h), \
+	.flags = (f), \
+	.callback = cb ## __void, \
+}
 #define OPT_STRING_F(s, l, v, a, h, f) { \
 	.type = OPTION_STRING, \
 	.short_name = (s), \

Re: [PATCH] describe: fix --no-exact-match

[René Scharfe]

Am 10.08.23 um 03:00 schrieb Junio C Hamano:
> Jeff King <peff@peff.net> writes:
>
>>   int my_foo;
>>   ...
>>   OPT_CALLBACK('f', "foo", int, &my_foo, ...etc);
>>
>> Not great, but it might not be _too_ bad given that most helpers like
>> OPT_BOOL() can just say "int" behind the scenes.
>>
>> That said, I don't recall these void pointers being a large source of
>> errors in the past. So while it's a fun type-system puzzle, the
>> effort/reward ratio might not be favorable.

I have vague memories of changing a callback from using a string_list
to a strset (or whatever) and getting crashes out of some other callers
that used it non-obviously via some macro.  A compile-time check would
have helped me.

> I tend to agree on both counts, it is a fun puzzle, and it probably
> is not going to give us sufficient reward.  The fact that "int" and
> typeof(my_foo) need to be manually kept straight defeats the "type
> safety" justification for this mental exercise.

The many-pointers idea is a bit silly, admittedly.

Having to declare a type manually is how most of C's type safety works,
though, so a certain amount of that is unavoidable.

typeof (along with typeof_unqual) is nice, but I guess it will take a
few years (or decades?) before we can expect it to be supported by all
relevant platforms.

The typed-callback-with-void-wrapper idea seems promising.  Let's see
if we can get it into usable shape.

René

Re: [PATCH] describe: fix --no-exact-match

[Jeff King]

On Thu, Aug 10, 2023 at 09:10:33PM +0200, René Scharfe wrote:

> > I'm not sure there's a portable and non-insane way of doing what we want
> > here. At least at compile-time.
> 
> We need a wrapper with the correct signature.  The wrapper is plugged
> into struct option.  The typed callback is called by the wrapper and
> can be used for a type check in the struct macro.  Demo patch below.

OK, clever. So we have two functions, one with a real body, and the
other which is used with the void pointer. How do we make sure that the
real-body one matches the type passed to OPT_CALLBACK(), if it is only
seeing the void wrapper? I guess that is this bit in short_name:

> +#define OPT_CALLBACK_F_T(s, l, v, a, h, f, cb) { \
> +	.type = OPTION_CALLBACK, \
> +	.short_name = (s) + (0 ? cb(NULL, NULL, 0, (v)) : 0), \

which would cause the compiler to barf, and presumably eliminate the
dead code (or at the very least never call it at runtime).

So I think that works. Though...

> +#define DEFINE_PARSE_OPT_CB(name)				\
> +static inline int name ## __void(const struct option *opt,	\
> +				 const char *arg, int unset)	\
> +{								\
> +	return name(opt, arg, unset, opt->value);		\
> +}								\

we are defining an inline function with the explicit goal of passing it
as a function pointer. I don't remember all of the standard's rules
here. Are we guaranteed that it will create a linkable version if
necessary?

We could probably drop the "inline" (and perhaps would need to add
MAYBE_UNUSED in such a case).

> diff --git a/builtin/describe.c b/builtin/describe.c
> index b28a4a1f82..ce16c36de2 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -558,15 +558,17 @@ static void describe(const char *arg, int last_one)
>  	strbuf_release(&sb);
>  }
> 
> -static int option_parse_exact_match(const struct option *opt, const char *arg,
> -				    int unset)
> +static int option_parse_exact_match(const struct option *opt UNUSED,
> +				    const char *arg, int unset, int *value)
>  {
>  	BUG_ON_OPT_ARG(arg);
> 
> -	max_candidates = unset ? DEFAULT_CANDIDATES : 0;
> +	*value = unset ? DEFAULT_CANDIDATES : 0;
>  	return 0;
>  }
> 
> +DEFINE_PARSE_OPT_CB(option_parse_exact_match);

I wondered about combining these, like:

  DEFINE_PARSE_OPT_CB(option_parse_exact_match, int) {
	...the real body here...
  }

But I guess that may confuse non-compiler parsers, and it doesn't leave
room for annotations like the UNUSED above (which ironically is still
needed, since now we pass opt->value as its own parameter).

So I dunno. Clever, for sure, and I think it would work. I'm not sure if
the extra code merits the return or not.

-Peff

Re: [PATCH] describe: fix --no-exact-match

[Jeff King]

On Thu, Aug 10, 2023 at 09:10:33PM +0200, René Scharfe wrote:

> +#define DEFINE_PARSE_OPT_CB(name)				\
> +static inline int name ## __void(const struct option *opt,	\
> +				 const char *arg, int unset)	\
> +{								\
> +	return name(opt, arg, unset, opt->value);		\
> +}								\
> +struct option

BTW, I wondered what this extra "struct option" was doing. I guess it is
there to soak up the semi-colon of:

  DEFINE_PARSE_OPT_CB(foo);

with a noop declaration. Both clever and gross. :)

-Peff

Re: [PATCH] describe: fix --no-exact-match

[René Scharfe]

Am 11.08.23 um 17:11 schrieb Jeff King:
> On Thu, Aug 10, 2023 at 09:10:33PM +0200, René Scharfe wrote:
>
>>> I'm not sure there's a portable and non-insane way of doing what we want
>>> here. At least at compile-time.
>>
>> We need a wrapper with the correct signature.  The wrapper is plugged
>> into struct option.  The typed callback is called by the wrapper and
>> can be used for a type check in the struct macro.  Demo patch below.
>
> OK, clever. So we have two functions, one with a real body, and the
> other which is used with the void pointer. How do we make sure that the
> real-body one matches the type passed to OPT_CALLBACK(), if it is only
> seeing the void wrapper? I guess that is this bit in short_name:
>
>> +#define OPT_CALLBACK_F_T(s, l, v, a, h, f, cb) { \
>> +	.type = OPTION_CALLBACK, \
>> +	.short_name = (s) + (0 ? cb(NULL, NULL, 0, (v)) : 0), \
>
> which would cause the compiler to barf, and presumably eliminate the
> dead code (or at the very least never call it at runtime).

Yes, a fake call that doesn't make it into the generated object code,
but is real enough for type checking.

> So I think that works. Though...
>
>> +#define DEFINE_PARSE_OPT_CB(name)				\
>> +static inline int name ## __void(const struct option *opt,	\
>> +				 const char *arg, int unset)	\
>> +{								\
>> +	return name(opt, arg, unset, opt->value);		\
>> +}								\
>
> we are defining an inline function with the explicit goal of passing it
> as a function pointer. I don't remember all of the standard's rules
> here. Are we guaranteed that it will create a linkable version if
> necessary?

I don't see on which basis the compiler could refuse.  We can't expect
the function address to be the same across compilation units, but we
don't need that.  If there's a compiler that won't do it or a standards
verse that makes this dubious then I'd like to know very much.

> We could probably drop the "inline" (and perhaps would need to add
> MAYBE_UNUSED in such a case).

Yes, we can use a non-inline function as well, but then we'd need to
define it once somewhere and declare it separately, though, which is
inconvenient.  It needs to have the same visibility as the typed
function.

We better not use MAYBE_UNUSED because the wrapper is the actually
invoked callback.

>> diff --git a/builtin/describe.c b/builtin/describe.c
>> index b28a4a1f82..ce16c36de2 100644
>> --- a/builtin/describe.c
>> +++ b/builtin/describe.c
>> @@ -558,15 +558,17 @@ static void describe(const char *arg, int last_one)
>>  	strbuf_release(&sb);
>>  }
>>
>> -static int option_parse_exact_match(const struct option *opt, const char *arg,
>> -				    int unset)
>> +static int option_parse_exact_match(const struct option *opt UNUSED,
>> +				    const char *arg, int unset, int *value)
>>  {
>>  	BUG_ON_OPT_ARG(arg);
>>
>> -	max_candidates = unset ? DEFAULT_CANDIDATES : 0;
>> +	*value = unset ? DEFAULT_CANDIDATES : 0;
>>  	return 0;
>>  }
>>
>> +DEFINE_PARSE_OPT_CB(option_parse_exact_match);
>
> I wondered about combining these, like:
>
>   DEFINE_PARSE_OPT_CB(option_parse_exact_match, int) {
> 	...the real body here...
>   }
>
> But I guess that may confuse non-compiler parsers, and it doesn't leave
> room for annotations like the UNUSED above (which ironically is still
> needed, since now we pass opt->value as its own parameter).

I get the idea.  Something that looks like a function attribute or
decorator.  I don't see no nice way to achieve that, though.

> So I dunno. Clever, for sure, and I think it would work. I'm not sure if
> the extra code merits the return or not.

Sure, why check types -- script languages get work done as well.  (I'm
fresh off a Python basics training, nice quirky language..)  But we're
in C land and static typing is supposed to help us get our operations
correct and portable.

A good example in parseopt: The patch below adds type checking to the
int options and yields 79 warning about incompatible pointers, because
enum pointers were used in integer option definitions.  The storage size
of enums depends on the member values and the compiler; an enum could be
char-sized.  When we access such a thing with an int pointer we write up
to seven bytes of garbage ... somewhere.  We better fix that.

It also gives me 73 sign warnings because we like to use unsigned int to
store int options for some reason.  No problem storage-wise.  I have to
admit that I don't really understand why -Wpointer-sign is in -Wall and
not only in -Wpedantic.  But it could mean a user gives a negative value
for something and we interpret it as a huge value.  Probably still worth
fixing to avoid the general weirdness.

My point is: If we don't check types we deviate.  It just happens.

René


PS: We could simplify the type check like this:

	.short_name = (s) + (0 && (v) == (int *)0), \

This yields -Wcompare-distinct-pointer-types for all 152 cases, which
may suffice.  But comparisons of pointers are only defined for those
that point to the same object or just past it if I'm not mistaken, so
this way works only by accident and the comparison part of the warning
is misleading anyway.  Sill looking for a simpler type check..



diff --git a/parse-options.h b/parse-options.h
index f957931cfa..dafea28b55 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -168,9 +168,14 @@ struct option {
 	parse_opt_subcommand_fn *subcommand_fn;
 };

+static inline int parse_options_noop_int_ptr(int *p UNUSED)
+{
+	return 0;
+}
+
 #define OPT_BIT_F(s, l, v, h, b, f) { \
 	.type = OPTION_BIT, \
-	.short_name = (s), \
+	.short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \
 	.long_name = (l), \
 	.value = (v), \
 	.help = (h), \
@@ -180,7 +185,7 @@ struct option {
 }
 #define OPT_COUNTUP_F(s, l, v, h, f) { \
 	.type = OPTION_COUNTUP, \
-	.short_name = (s), \
+	.short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \
 	.long_name = (l), \
 	.value = (v), \
 	.help = (h), \
@@ -188,7 +193,7 @@ struct option {
 }
 #define OPT_SET_INT_F(s, l, v, h, i, f) { \
 	.type = OPTION_SET_INT, \
-	.short_name = (s), \
+	.short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \
 	.long_name = (l), \
 	.value = (v), \
 	.help = (h), \
@@ -227,7 +232,7 @@ struct option {
 }
 #define OPT_INTEGER_F(s, l, v, h, f) { \
 	.type = OPTION_INTEGER, \
-	.short_name = (s), \
+	.short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \
 	.long_name = (l), \
 	.value = (v), \
 	.argh = N_("n"), \
@@ -245,7 +250,7 @@ struct option {
 #define OPT_BIT(s, l, v, h, b)      OPT_BIT_F(s, l, v, h, b, 0)
 #define OPT_BITOP(s, l, v, h, set, clear) { \
 	.type = OPTION_BITOP, \
-	.short_name = (s), \
+	.short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \
 	.long_name = (l), \
 	.value = (v), \
 	.help = (h), \
@@ -255,7 +260,7 @@ struct option {
 }
 #define OPT_NEGBIT(s, l, v, h, b) { \
 	.type = OPTION_NEGBIT, \
-	.short_name = (s), \
+	.short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \
 	.long_name = (l), \
 	.value = (v), \
 	.help = (h), \
@@ -267,7 +272,7 @@ struct option {
 #define OPT_BOOL(s, l, v, h)        OPT_BOOL_F(s, l, v, h, 0)
 #define OPT_HIDDEN_BOOL(s, l, v, h) { \
 	.type = OPTION_SET_INT, \
-	.short_name = (s), \
+	.short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \
 	.long_name = (l), \
 	.value = (v), \
 	.help = (h), \
@@ -276,7 +281,7 @@ struct option {
 }
 #define OPT_CMDMODE_F(s, l, v, h, i, f) { \
 	.type = OPTION_SET_INT, \
-	.short_name = (s), \
+	.short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \
 	.long_name = (l), \
 	.value = (v), \
 	.help = (h), \

Re: [PATCH] describe: fix --no-exact-match

[René Scharfe]

Am 11.08.23 um 17:13 schrieb Jeff King:
> On Thu, Aug 10, 2023 at 09:10:33PM +0200, René Scharfe wrote:
>
>> +#define DEFINE_PARSE_OPT_CB(name)				\
>> +static inline int name ## __void(const struct option *opt,	\
>> +				 const char *arg, int unset)	\
>> +{								\
>> +	return name(opt, arg, unset, opt->value);		\
>> +}								\
>> +struct option
>
> BTW, I wondered what this extra "struct option" was doing. I guess it is
> there to soak up the semi-colon of:
>
>   DEFINE_PARSE_OPT_CB(foo);
>
> with a noop declaration. Both clever and gross. :)

You guessed correctly.  It was short and handy..

A production-grade patch would use the function header.

René

Re: [PATCH] describe: fix --no-exact-match

[Jeff King]

On Fri, Aug 11, 2023 at 07:59:12PM +0200, René Scharfe wrote:

> > we are defining an inline function with the explicit goal of passing it
> > as a function pointer. I don't remember all of the standard's rules
> > here. Are we guaranteed that it will create a linkable version if
> > necessary?
> 
> I don't see on which basis the compiler could refuse.  We can't expect
> the function address to be the same across compilation units, but we
> don't need that.  If there's a compiler that won't do it or a standards
> verse that makes this dubious then I'd like to know very much.

I seem to recall some quirks where an inline function that is not called
directly is not required to be compiled at all, and the compiler can
assume that there is a definition available in another translation unit.

But I think that only applies when no storage-class specifier is
provided. In this case, you said "static", so I think it's OK?

It's possible I'm mis-remembering the issues, too. One problem is that
pre-C99, you might end up with the opposite problem (a compiled function
with visible linkage that conflicts with other translation units at link
time). E.g. here:

  https://stackoverflow.com/questions/51533082/clarification-over-internal-linkage-of-inline-functions-in-c/51533367#51533367

But I think with "static" we should always be OK.

> > So I dunno. Clever, for sure, and I think it would work. I'm not sure if
> > the extra code merits the return or not.
> 
> Sure, why check types -- script languages get work done as well.  (I'm
> fresh off a Python basics training, nice quirky language..)  But we're
> in C land and static typing is supposed to help us get our operations
> correct and portable.

Don't get me wrong, I like type checking. It's just that doing weird
things with the language and pre-processor also carries a cost,
especially in an open source project where new folks may show up and say
"what the hell is this macro doing?". That's a friction for new
developers, even if they're comfortable with idiomatic C.

That said...

> A good example in parseopt: The patch below adds type checking to the
> int options and yields 79 warning about incompatible pointers, because
> enum pointers were used in integer option definitions.  The storage size
> of enums depends on the member values and the compiler; an enum could be
> char-sized.  When we access such a thing with an int pointer we write up
> to seven bytes of garbage ... somewhere.  We better fix that.

...I do find this evidence compelling.

-Peff

Re: [PATCH] describe: fix --no-exact-match

[René Scharfe]

Am 11.08.23 um 20:24 schrieb Jeff King:
> On Fri, Aug 11, 2023 at 07:59:12PM +0200, René Scharfe wrote:
>
>>> we are defining an inline function with the explicit goal of passing it
>>> as a function pointer. I don't remember all of the standard's rules
>>> here. Are we guaranteed that it will create a linkable version if
>>> necessary?
>>
>> I don't see on which basis the compiler could refuse.  We can't expect
>> the function address to be the same across compilation units, but we
>> don't need that.  If there's a compiler that won't do it or a standards
>> verse that makes this dubious then I'd like to know very much.
>
> I seem to recall some quirks where an inline function that is not called
> directly is not required to be compiled at all, and the compiler can
> assume that there is a definition available in another translation unit.

C99 says in 6.7.4 Function specifiers: "It is unspecified whether a call
to the function uses the inline definition or the external definition.",
referring to functions with both types of definition.  So a compiler
could ignore the inline version for those.

> But I think that only applies when no storage-class specifier is
> provided. In this case, you said "static", so I think it's OK?

That's how I understand it as well -- there is no external version to
choose and the compiler is not free to ignore the inline one.

> It's possible I'm mis-remembering the issues, too. One problem is that
> pre-C99, you might end up with the opposite problem (a compiled function
> with visible linkage that conflicts with other translation units at link
> time). E.g. here:
>
>   https://stackoverflow.com/questions/51533082/clarification-over-internal-linkage-of-inline-functions-in-c/51533367#51533367
>
> But I think with "static" we should always be OK.

*nod*

> Don't get me wrong, I like type checking. It's just that doing weird
> things with the language and pre-processor also carries a cost,
> especially in an open source project where new folks may show up and say
> "what the hell is this macro doing?". That's a friction for new
> developers, even if they're comfortable with idiomatic C.

Sure, but that ship has sailed in this specific case.  Standard option
parsing would use getopt or getopt_long, neither of which has void
pointers.  We already carry the cost of our OPT_ macros.  Let's ease it.

> That said...
>
>> A good example in parseopt: The patch below adds type checking to the
>> int options and yields 79 warning about incompatible pointers, because
>> enum pointers were used in integer option definitions.  The storage size
>> of enums depends on the member values and the compiler; an enum could be
>> char-sized.  When we access such a thing with an int pointer we write up
>> to seven bytes of garbage ... somewhere.  We better fix that.
>
> ...I do find this evidence compelling.

It's 3 instead of 7 bytes of garbage, but the point still stands..

René