diff mbox series

[07/14] parse-options: allow ll_callback with OPTION_CALLBACK

Message ID 20190127003535.28341-8-pclouds@gmail.com (mailing list archive)
State New, archived
Headers show
Series nd/diff-parseopt part 1 | expand

Commit Message

Duy Nguyen Jan. 27, 2019, 12:35 a.m. UTC 
OPTION_CALLBACK is much simpler/safer to use, but parse_opt_cb does
not allow access to parse_opt_ctx_t, which sometimes is useful
(e.g. to obtain the prefix).

Extending parse_opt_cb to take parse_opt_cb could result in a lot of
changes. Instead let's just allow ll_callback to be used with
OPTION_CALLBACK. The user will have to be careful, not to change
anything in ctx, or return wrong result code. But that's the price for
ll_callback.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/merge.c        |  2 ++
 builtin/update-index.c | 20 +++++++++++++++-----
 parse-options-cb.c     |  4 +++-
 parse-options.c        | 42 ++++++++++++++++++++++++++++--------------
 parse-options.h        |  5 +++--
 5 files changed, 51 insertions(+), 22 deletions(-)

Comments

Derrick Stolee April 15, 2019, 2:06 p.m. UTC | #1 
On 1/26/2019 7:35 PM, Nguyễn Thái Ngọc Duy wrote:
> @@ -238,7 +249,10 @@ static enum parse_opt_result parse_short_opt(struct parse_opt_ctx_t *p,
>  			len++;
>  		arg = xmemdupz(p->opt, len);
>  		p->opt = p->opt[len] ? p->opt + len : NULL;
> -		rc = (*numopt->callback)(numopt, arg, 0) ? (-1) : 0;
> +		if (numopt->callback)
> +			rc = (*numopt->callback)(numopt, arg, 0) ? (-1) : 0;
> +		else
> +			rc = (*numopt->ll_callback)(p, numopt, arg, 0);
>  		free(arg);
>  		return rc;
>  	}

Hi Duy,

This "else" condition is unreachable. This block is only hit when we have a "-<n>"
option, using OPT_NUMBER_CALLBACK, which is implemented by filling "callback", never
"ll_callback".

I recommend reverting this diff segment, but please let me know if I'm missing something.

Thanks,
-Stolee
Duy Nguyen April 16, 2019, 8:52 a.m. UTC | #2 
On Mon, Apr 15, 2019 at 9:06 PM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 1/26/2019 7:35 PM, Nguyễn Thái Ngọc Duy wrote:
> > @@ -238,7 +249,10 @@ static enum parse_opt_result parse_short_opt(struct parse_opt_ctx_t *p,
> >                       len++;
> >               arg = xmemdupz(p->opt, len);
> >               p->opt = p->opt[len] ? p->opt + len : NULL;
> > -             rc = (*numopt->callback)(numopt, arg, 0) ? (-1) : 0;
> > +             if (numopt->callback)
> > +                     rc = (*numopt->callback)(numopt, arg, 0) ? (-1) : 0;
> > +             else
> > +                     rc = (*numopt->ll_callback)(p, numopt, arg, 0);
> >               free(arg);
> >               return rc;
> >       }
>
> Hi Duy,
>
> This "else" condition is unreachable. This block is only hit when we have a "-<n>"
> option, using OPT_NUMBER_CALLBACK, which is implemented by filling "callback", never
> "ll_callback".

It does not mean ll_callback cannot be used in the future though. We
have three options

1. drop the else clause
2. replace with "else BUG();"
3. implement proper else clause

Option #1 to me sounds wrong. If you don't support something, yell up.
Silently ignoring it only makes it harder to track down to this
unsupported location when it becomes reachable, however unlikely that
is.

Which leaves options #2 and #3. If you think this one line is risky
enough, I'll send a patch to replace it with BUG().

> I recommend reverting this diff segment, but please let me know if I'm missing something.
>
> Thanks,
> -Stolee
Derrick Stolee April 16, 2019, 2:24 p.m. UTC | #3 
On 4/16/2019 4:52 AM, Duy Nguyen wrote:
> On Mon, Apr 15, 2019 at 9:06 PM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 1/26/2019 7:35 PM, Nguyễn Thái Ngọc Duy wrote:
>>> @@ -238,7 +249,10 @@ static enum parse_opt_result parse_short_opt(struct parse_opt_ctx_t *p,
>>>                       len++;
>>>               arg = xmemdupz(p->opt, len);
>>>               p->opt = p->opt[len] ? p->opt + len : NULL;
>>> -             rc = (*numopt->callback)(numopt, arg, 0) ? (-1) : 0;
>>> +             if (numopt->callback)
>>> +                     rc = (*numopt->callback)(numopt, arg, 0) ? (-1) : 0;
>>> +             else
>>> +                     rc = (*numopt->ll_callback)(p, numopt, arg, 0);
>>>               free(arg);
>>>               return rc;
>>>       }
>>
>> Hi Duy,
>>
>> This "else" condition is unreachable. This block is only hit when we have a "-<n>"
>> option, using OPT_NUMBER_CALLBACK, which is implemented by filling "callback", never
>> "ll_callback".
> 
> It does not mean ll_callback cannot be used in the future though.

That's not a very good reason to add it now. YAGNI.

> We
> have three options
> 
> 1. drop the else clause
> 2. replace with "else BUG();"
> 3. implement proper else clause
> 
> Option #1 to me sounds wrong. If you don't support something, yell up.
> Silently ignoring it only makes it harder to track down to this
> unsupported location when it becomes reachable, however unlikely that
> is.
> 
> Which leaves options #2 and #3. If you think this one line is risky
> enough, I'll send a patch to replace it with BUG().

It's not about risk, but the fact that it is pointless. The only way to get to
this block is to create a 'struct option' with type OPTION_NUMBER manually
(ignoring the OPT_NUMBER_CALLBACK macro), which _should_ be unsupported. If
someone goes to the pain of adding a way to instantiate with a low-level callback,
then they should add this 'if' statement.

If you are going to add protection from an incorrect instantiation of 'callback'
in a use of OPT_NUMBER_CALLBACK, then the proper place to do that is probably in
parse_options_check(), where you were already doing callback-vs-ll_callback checks
in the case of OPTION_CALLBACK and OPTION_LOWLEVEL_CALLBACK.

However, the only places where the OPT_NUMBER_CALLBACK option appears are
builtin/grep.c and t/helper/test-parse-options.c. I don't imagine a need to
add low-level callbacks any time soon.

>> I recommend reverting this diff segment, but please let me know if I'm missing something.

I still think this is the easiest way to remove the dead code.

I'll try to submit a patch later. I'm still not in full work mode, so I may be slow.

Thanks,
-Stolee
diff mbox series

Patch

diff --git a/builtin/merge.c b/builtin/merge.c
index de64d7850e..563a16f38a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -114,11 +114,13 @@  static int option_parse_message(const struct option *opt,
 
 static enum parse_opt_result option_read_message(struct parse_opt_ctx_t *ctx,
 						 const struct option *opt,
+						 const char *arg_not_used,
 						 int unset)
 {
 	struct strbuf *buf = opt->value;
 	const char *arg;
 
+	BUG_ON_OPT_ARG(arg_not_used);
 	if (unset)
 		BUG("-F cannot be negated");
 
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 21c84e5590..7abde20169 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -848,13 +848,15 @@  static int parse_new_style_cacheinfo(const char *arg,
 }
 
 static enum parse_opt_result cacheinfo_callback(
-	struct parse_opt_ctx_t *ctx, const struct option *opt, int unset)
+	struct parse_opt_ctx_t *ctx, const struct option *opt,
+	const char *arg, int unset)
 {
 	struct object_id oid;
 	unsigned int mode;
 	const char *path;
 
 	BUG_ON_OPT_NEG(unset);
+	BUG_ON_OPT_ARG(arg);
 
 	if (!parse_new_style_cacheinfo(ctx->argv[1], &mode, &oid, &path)) {
 		if (add_cacheinfo(mode, &oid, path, 0))
@@ -874,11 +876,13 @@  static enum parse_opt_result cacheinfo_callback(
 }
 
 static enum parse_opt_result stdin_cacheinfo_callback(
-	struct parse_opt_ctx_t *ctx, const struct option *opt, int unset)
+	struct parse_opt_ctx_t *ctx, const struct option *opt,
+	const char *arg, int unset)
 {
 	int *nul_term_line = opt->value;
 
 	BUG_ON_OPT_NEG(unset);
+	BUG_ON_OPT_ARG(arg);
 
 	if (ctx->argc != 1)
 		return error("option '%s' must be the last argument", opt->long_name);
@@ -888,11 +892,13 @@  static enum parse_opt_result stdin_cacheinfo_callback(
 }
 
 static enum parse_opt_result stdin_callback(
-	struct parse_opt_ctx_t *ctx, const struct option *opt, int unset)
+	struct parse_opt_ctx_t *ctx, const struct option *opt,
+	const char *arg, int unset)
 {
 	int *read_from_stdin = opt->value;
 
 	BUG_ON_OPT_NEG(unset);
+	BUG_ON_OPT_ARG(arg);
 
 	if (ctx->argc != 1)
 		return error("option '%s' must be the last argument", opt->long_name);
@@ -901,12 +907,14 @@  static enum parse_opt_result stdin_callback(
 }
 
 static enum parse_opt_result unresolve_callback(
-	struct parse_opt_ctx_t *ctx, const struct option *opt, int unset)
+	struct parse_opt_ctx_t *ctx, const struct option *opt,
+	const char *arg, int unset)
 {
 	int *has_errors = opt->value;
 	const char *prefix = startup_info->prefix;
 
 	BUG_ON_OPT_NEG(unset);
+	BUG_ON_OPT_ARG(arg);
 
 	/* consume remaining arguments. */
 	*has_errors = do_unresolve(ctx->argc, ctx->argv,
@@ -920,12 +928,14 @@  static enum parse_opt_result unresolve_callback(
 }
 
 static enum parse_opt_result reupdate_callback(
-	struct parse_opt_ctx_t *ctx, const struct option *opt, int unset)
+	struct parse_opt_ctx_t *ctx, const struct option *opt,
+	const char *arg, int unset)
 {
 	int *has_errors = opt->value;
 	const char *prefix = startup_info->prefix;
 
 	BUG_ON_OPT_NEG(unset);
+	BUG_ON_OPT_ARG(arg);
 
 	/* consume remaining arguments. */
 	setup_work_tree();
diff --git a/parse-options-cb.c b/parse-options-cb.c
index ec01ef722b..2733393546 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -171,8 +171,10 @@  int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
  * parse_options().
  */
 enum parse_opt_result parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx,
-					   const struct option *opt, int unset)
+					   const struct option *opt,
+					   const char *arg, int unset)
 {
+	BUG_ON_OPT_ARG(arg);
 	return PARSE_OPT_UNKNOWN;
 }
 
diff --git a/parse-options.c b/parse-options.c
index 50c340474c..cec74522e5 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -95,7 +95,7 @@  static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
 
 	switch (opt->type) {
 	case OPTION_LOWLEVEL_CALLBACK:
-		return opt->ll_callback(p, opt, unset);
+		return opt->ll_callback(p, opt, NULL, unset);
 
 	case OPTION_BIT:
 		if (unset)
@@ -161,16 +161,27 @@  static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
 		return err;
 
 	case OPTION_CALLBACK:
+	{
+		const char *p_arg = NULL;
+		int p_unset;
+
 		if (unset)
-			return (*opt->callback)(opt, NULL, 1) ? (-1) : 0;
-		if (opt->flags & PARSE_OPT_NOARG)
-			return (*opt->callback)(opt, NULL, 0) ? (-1) : 0;
-		if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
-			return (*opt->callback)(opt, NULL, 0) ? (-1) : 0;
-		if (get_arg(p, opt, flags, &arg))
+			p_unset = 1;
+		else if (opt->flags & PARSE_OPT_NOARG)
+			p_unset = 0;
+		else if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
+			p_unset = 0;
+		else if (get_arg(p, opt, flags, &arg))
 			return -1;
-		return (*opt->callback)(opt, arg, 0) ? (-1) : 0;
-
+		else {
+			p_unset = 0;
+			p_arg = arg;
+		}
+		if (opt->callback)
+			return (*opt->callback)(opt, p_arg, p_unset) ? (-1) : 0;
+		else
+			return (*opt->ll_callback)(p, opt, p_arg, p_unset);
+	}
 	case OPTION_INTEGER:
 		if (unset) {
 			*(int *)opt->value = 0;
@@ -238,7 +249,10 @@  static enum parse_opt_result parse_short_opt(struct parse_opt_ctx_t *p,
 			len++;
 		arg = xmemdupz(p->opt, len);
 		p->opt = p->opt[len] ? p->opt + len : NULL;
-		rc = (*numopt->callback)(numopt, arg, 0) ? (-1) : 0;
+		if (numopt->callback)
+			rc = (*numopt->callback)(numopt, arg, 0) ? (-1) : 0;
+		else
+			rc = (*numopt->ll_callback)(p, numopt, arg, 0);
 		free(arg);
 		return rc;
 	}
@@ -414,10 +428,10 @@  static void parse_options_check(const struct option *opts)
 				err |= optbug(opts, "should not accept an argument");
 			break;
 		case OPTION_CALLBACK:
-			if (!opts->callback)
-				BUG("OPTION_CALLBACK needs a callback");
-			if (opts->ll_callback)
-				BUG("OPTION_CALLBACK needs no ll_callback");
+			if (!opts->callback && !opts->ll_callback)
+				BUG("OPTION_CALLBACK needs one callback");
+			if (opts->callback && opts->ll_callback)
+				BUG("OPTION_CALLBACK can't have two callbacks");
 			break;
 		case OPTION_LOWLEVEL_CALLBACK:
 			if (!opts->ll_callback)
diff --git a/parse-options.h b/parse-options.h
index 4e49185027..ce75278804 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -50,7 +50,8 @@  typedef int parse_opt_cb(const struct option *, const char *arg, int unset);
 
 struct parse_opt_ctx_t;
 typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
-					      const struct option *opt, int unset);
+					      const struct option *opt,
+					      const char *arg, int unset);
 
 /*
  * `type`::
@@ -267,7 +268,7 @@  int parse_opt_commits(const struct option *, const char *, int);
 int parse_opt_tertiary(const struct option *, const char *, int);
 int parse_opt_string_list(const struct option *, const char *, int);
 int parse_opt_noop_cb(const struct option *, const char *, int);
-int parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx, const struct option *, int);
+int parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx, const struct option *, const char *, int);
 int parse_opt_passthru(const struct option *, const char *, int);
 int parse_opt_passthru_argv(const struct option *, const char *, int);