Re: [PATCH/GSoC] parse-options: Add a new nousage opt

Note Before: I have decided not to apply for GSoC with Git this year,
as I was already late, and all the remaining time got taken by the
proposal I wrote for Debian, and college studies / exams.

I definitely want to work with Git in the future too, it has always
piqued my interest being something that I use daily.
I want to get this change done as well, if that is okay.

On Thu, Mar 24, 2016 at 4:01 AM, Jeff King [off-list ref] wrote:

On Sun, Mar 20, 2016 at 12:16:45PM +0530, Chirayu Desai wrote:

quoted

diff --git a/parse-options-cb.c b/parse-options-cb.c
index 239898d946..ac2ea4d674 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c

@@ -85,11 +85,15 @@ int parse_opt_commits(const struct option *opt, const char *arg, int unset)

      if (!arg)
              return -1;
-     if (get_sha1(arg, sha1))
-             return error("malformed object name %s", arg);
+     if (get_sha1(arg, sha1)) {
+             error("malformed object name %s", arg);
+             return -3;
+     }

Now that we have a few meaningful return values, should we have some
enum that gives them human-readable names?

E.g., why don't we allow "-2" here? I think it is because
parse_options_step internally uses it for "I don't know about that
option". But maybe we should have something like:

  enum PARSE_OPT_ERROR {
          PARSE_OPT_ERR_USAGE = -1,
          PARSE_OPT_ERR_UNKNOWN_OPTION = -2,
          PARSE_OPT_ERR_FAIL_QUIETLY = -3,
  }

(I don't quite like the final name, but I couldn't think of anything
better).

I agree, this would be much better and clearer than using hard coded values.

quoted

diff --git a/parse-options.c b/parse-options.c
index 47a9192060..d136c1afd0 100644
--- a/parse-options.c
+++ b/parse-options.c

@@ -158,6 +158,9 @@ static int get_value(struct parse_opt_ctx_t *p,
                      return (*opt->callback)(opt, NULL, 0) ? (-1) : 0;
              if (get_arg(p, opt, flags, &arg))
                      return -1;
+             if (opt->flags & PARSE_OPT_NOUSAGE) {
+                     return (*opt->callback)(opt, arg, 0);
+             }
              return (*opt->callback)(opt, arg, 0) ? (-1) : 0;

Here you use PARSE_OPT_NOUSAGE to pass the callback's value directly
back to the rest of the option-parsing code. But can't we just intercept
"-3" always? It's possible that another callback is using it to
generically return an error, but it seems like a rather low risk, and
the resulting code is much simpler.

I don't get what you mean by intercepting '-3'.
The idea was that other options could use it in the future to return
arbitary values.

Or we could go the opposite direction. If a callback is annotated with
PARSE_OPT_NOUSAGE, why do we even need to care about its return value?
The callback could continue to return -1, and we would simply suppress
the usage message.

That would also work, but I feel that this is cleaner.

quoted

      case OPTION_INTEGER:

@@ -504,6 +507,8 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
                      goto show_usage_error;
              case -2:
                      goto unknown;
+             case -3:
+                     return PARSE_OPT_DONE;
              }
              continue;
 unknown:

If I understand correctly, this is now getting the value from the
callback directly. What happens if a callback returns "-4" or "4"?

That could be handled in the future if somebody decides to use that.
Now this makes using the above "return -1 always but don't print usage if flags
contain PARSE_OPT_NOUSAGE" option look much better.

Also, this covers the parse_long_opt() call, but there are two
parse_short_opt() calls earlier. Wouldn't they need to learn the same
logic?

I didn't add it right now as both "--contains" and "--with" are long opts.

-Peff

Thanks,
Chirayu