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

From

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

Date

Aug 10, 2023, 19:10 UTC

Message-ID

<09f499ad-28a5-0d8b-8af6-97475bdc614b@web.de>

In-Reply-To

<20230810004127.GD795985@coredump.intra.peff.net>

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.

> 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)

> 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.

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), \