threads / patch / 60186

patchname-rev: use OPT_HIDDEN_BOOL for --peel-tag

Subject: [PATCH] name-rev: use OPT_HIDDEN_BOOL for --peel-tag

## tl;dr

3 messages between Sep 2, 2023 and Sep 7, 2023. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

René Scharfe· Sep 2, 2023, 18:38 UTC · lore

adfc1857bd (describe: fix --contains when a tag is given as input, 2013-07-18) added the option --peel-tag, defining it using a positional struct option initializer and a comment indicating that it's intended to be a hidden OPT_BOOL. 4741edd549 (Remove deprecated OPTION_BOOLEAN for parsing arguments, 2013-08-03) added the macro OPT_HIDDEN_BOOL, which allows to express this more succinctly. Use it.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/name-rev.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Show changes to builtin/name-rev.c +2 −7

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index c706fa3720..2dd1807c4e 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -582,12 +582,8 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "undefined", &allow_undefined, N_("allow to print `undefined` names (default)")),
 		OPT_BOOL(0, "always",     &always,
 			   N_("show abbreviated commit object as fallback")),
-		{
-			/* A Hidden OPT_BOOL */
-			OPTION_SET_INT, 0, "peel-tag", &peel_tag, NULL,
-			N_("dereference tags in the input (internal use)"),
-			PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1,
-		},
+		OPT_HIDDEN_BOOL(0, "peel-tag", &peel_tag,
+			   N_("dereference tags in the input (internal use)")),
 		OPT_END(),
 	};

--
2.42.0

Jeff King· Sep 5, 2023, 07:17 UTC · re: René Scharfe · lore

Re: [PATCH] name-rev: use OPT_HIDDEN_BOOL for --peel-tag

On Sat, Sep 02, 2023 at 08:38:34PM +0200, René Scharfe wrote:

Show 6 quoted lines

> adfc1857bd (describe: fix --contains when a tag is given as input,
> 2013-07-18) added the option --peel-tag, defining it using a positional
> struct option initializer and a comment indicating that it's intended to
> be a hidden OPT_BOOL.  4741edd549 (Remove deprecated OPTION_BOOLEAN for
> parsing arguments, 2013-08-03) added the macro OPT_HIDDEN_BOOL, which
> allows to express this more succinctly.  Use it.

Yeah, this is a definite improvement.

OPT_HIDDEN_BOOL() itself is a little funny to me. I guess back then we did not have the "_F" variants, but really it is just:

  OPT_BOOL_F(0, "peel-tag", &peel_tag,
	     N_("dereference tags in the input (internal use)")),
	     PARSE_OPT_HIDDEN);

which would remove one more special case (after all, being hidden is orthogonal to the type). But there are enough of them that maybe having a special name for this is worth it. I dunno. But we could probably simplify the definition, at least. :)

-Peff

René Scharfe· Sep 7, 2023, 20:20 UTC · re: Jeff King · lore

Re: [PATCH] name-rev: use OPT_HIDDEN_BOOL for --peel-tag

Am 05.09.23 um 09:17 schrieb Jeff King:

Show 11 quoted lines

> On Sat, Sep 02, 2023 at 08:38:34PM +0200, René Scharfe wrote:
>
> OPT_HIDDEN_BOOL() itself is a little funny to me. I guess back then we
> did not have the "_F" variants, but really it is just:
>
>   OPT_BOOL_F(0, "peel-tag", &peel_tag,
> 	     N_("dereference tags in the input (internal use)")),
> 	     PARSE_OPT_HIDDEN);
>
> which would remove one more special case (after all, being hidden is
> orthogonal to the type).

Good point!

> But there are enough of them that maybe having
> a special name for this is worth it. I dunno.

A special case just for a flag feels wasteful. _F is more general at a reasonable price of some verbosity:

	OPT_HIDDEN_BOOL(s, l, v, h),
	OPT_BOOL_F(s, l, v, h, PARSE_OPT_HIDDEN),

Hindsight..

> But we could probably simplify the definition, at least. :)

Sure, that would be some kind of consolation price.

René