{"thread":{"id":"25917","subject":"[PATCH v3 00/10] update-index: migrate to parse-options API","startedAt":"2010-12-01T23:27:29Z","lastAt":"2010-12-06T07:57:42Z","messageCount":21,"participants":["Jonathan Nieder","Stephen Boyd","René Scharfe"],"isPatch":true,"patchVersion":3,"patchTotal":10},"messages":[{"id":"157048","messageId":"20101201232728.GA31815@burratino","threadId":"25917","inReplyTo":null,"subject":"[PATCH v3 00/10] update-index: migrate to parse-options API","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-01T23:27:29Z","receivedAt":"2010-12-01T23:27:29Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"This adapts \"git update-index\" to use the parse-options API\n(with resulting perks like nice \"-h\" output).  Doing so reveals\nsome potential improvements to parse-options infrastructure, too.\n\nSee [1] for the previous version.  This version incorporates the\nlast few suggestions by Stephen.  The iffiest bit is still\nhandling of the --cacheinfo option.\n\nThanks to Stephen and Junio for advice.  Patches applies to maint,\nfor no particular reason.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/159386/focus=162463\n\nJonathan Nieder (7):\n  parse-options: clearer reporting of API misuse\n  parse-options: move NODASH sanity checks to parse_options_check\n  parse-options: sanity check PARSE_OPT_NOARG flag\n  parse-options: never suppress arghelp if LITERAL_ARGHELP is set\n  parse-options: allow git commands to invent new option types\n  parse-options: make resuming easier after\n    PARSE_OPT_STOP_AT_NON_OPTION\n  update-index: migrate to parse-options API\n\nNguyễn Thái Ngọc Duy (1):\n  setup: save prefix (original cwd relative to toplevel) in\n    startup_info\n\nStephen Boyd (2):\n  parse-options: Don't call parse_options_check() so much\n  parse-options: do not infer PARSE_OPT_NOARG from option type\n\n builtin/blame.c        |    2 +-\n builtin/shortlog.c     |    2 +-\n builtin/update-index.c |  392 ++++++++++++++++++++++++++++++------------------\n cache.h                |    1 +\n parse-options.c        |   85 +++++------\n parse-options.h        |   11 +-\n setup.c                |    4 +-\n 7 files changed, 299 insertions(+), 198 deletions(-)\n"},{"id":"157049","messageId":"20101201232852.GB31815@burratino","threadId":"25917","inReplyTo":"20101201232728.GA31815@burratino","subject":"[PATCH 01/10] parse-options: Don't call parse_options_check() so much","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-01T23:28:52Z","receivedAt":"2010-12-01T23:28:52Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Stephen Boyd <bebarino@gmail.com>\n\nparse_options_check() is being called for each invocation of\nparse_options_step() which can be quite a bit for some commands. The\ncommit introducing this function cb9d398 (parse-options: add\nparse_options_check to validate option specs., 2009-06-09) had the\ncorrect motivation and explicitly states that parse_options_check()\nshould be called from parse_options_start(). However, the implementation\ndiffers from the motivation. Fix it.\n\nSigned-off-by: Stephen Boyd <bebarino@gmail.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nI would prefer to put \"options\" before flags, but seemed better to\njust get this out there.  Unchanged.\n\n builtin/blame.c    |    2 +-\n builtin/shortlog.c |    2 +-\n parse-options.c    |    7 +++----\n parse-options.h    |    2 +-\n 4 files changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 1015354..adf344c 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2299,7 +2299,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tdashdash_pos = 0;\n \n \tparse_options_start(&ctx, argc, argv, prefix, PARSE_OPT_KEEP_DASHDASH |\n-\t\t\t    PARSE_OPT_KEEP_ARGV0);\n+\t\t\t    PARSE_OPT_KEEP_ARGV0, options);\n \tfor (;;) {\n \t\tswitch (parse_options_step(&ctx, options, blame_opt_usage)) {\n \t\tcase PARSE_OPT_HELP:\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 2135b0d..8473391 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -269,7 +269,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \tshortlog_init(&log);\n \tinit_revisions(&rev, prefix);\n \tparse_options_start(&ctx, argc, argv, prefix, PARSE_OPT_KEEP_DASHDASH |\n-\t\t\t    PARSE_OPT_KEEP_ARGV0);\n+\t\t\t    PARSE_OPT_KEEP_ARGV0, options);\n \n \tfor (;;) {\n \t\tswitch (parse_options_step(&ctx, options, shortlog_usage)) {\ndiff --git a/parse-options.c b/parse-options.c\nindex 0fa79bc..196ba71 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -338,7 +338,7 @@ static void parse_options_check(const struct option *opts)\n \n void parse_options_start(struct parse_opt_ctx_t *ctx,\n \t\t\t int argc, const char **argv, const char *prefix,\n-\t\t\t int flags)\n+\t\t\t int flags, const struct option *options)\n {\n \tmemset(ctx, 0, sizeof(*ctx));\n \tctx->argc = argc - 1;\n@@ -350,6 +350,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,\n \tif ((flags & PARSE_OPT_KEEP_UNKNOWN) &&\n \t    (flags & PARSE_OPT_STOP_AT_NON_OPTION))\n \t\tdie(\"STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together\");\n+\tparse_options_check(options);\n }\n \n static int usage_with_options_internal(struct parse_opt_ctx_t *,\n@@ -362,8 +363,6 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n {\n \tint internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);\n \n-\tparse_options_check(options);\n-\n \t/* we must reset ->opt, unknown short option leave it dangling */\n \tctx->opt = NULL;\n \n@@ -452,7 +451,7 @@ int parse_options(int argc, const char **argv, const char *prefix,\n {\n \tstruct parse_opt_ctx_t ctx;\n \n-\tparse_options_start(&ctx, argc, argv, prefix, flags);\n+\tparse_options_start(&ctx, argc, argv, prefix, flags, options);\n \tswitch (parse_options_step(&ctx, options, usagestr)) {\n \tcase PARSE_OPT_HELP:\n \t\texit(129);\ndiff --git a/parse-options.h b/parse-options.h\nindex d982f0f..cfa03d5 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -180,7 +180,7 @@ struct parse_opt_ctx_t {\n \n extern void parse_options_start(struct parse_opt_ctx_t *ctx,\n \t\t\t\tint argc, const char **argv, const char *prefix,\n-\t\t\t\tint flags);\n+\t\t\t\tint flags, const struct option *options);\n \n extern int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\t      const struct option *options,\n-- \n1.7.2.3\n"},{"id":"157050","messageId":"20101201232923.GC31815@burratino","threadId":"25917","inReplyTo":"20101201232728.GA31815@burratino","subject":"[PATCH 02/10] parse-options: clearer reporting of API misuse","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-01T23:29:23Z","receivedAt":"2010-12-01T23:29:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The PARSE_OPT_LASTARG_DEFAULT flag is meant for options like\n--contains that (1) traditionally had a mandatory argument and\n(2) have some better behavior to use when appearing in the final\nposition.  It makes no sense to combine this with OPTARG, so ever\nsince v1.6.4-rc0~71 (parse-options: add parse_options_check to\nvalidate option specs, 2009-07-09) this mistake is flagged with\n\n\terror: `--option` uses incompatible flags LASTARG_DEFAULT and OPTARG\n\nand an exit status representing an error in commandline usage.\n\nUnfortunately that which might confuse scripters calling such an\nerroneous program into thinking the _script_ contains an error.\nClarify that it is an internal error by dying with a message beginning\n\"fatal: BUG: ...\" and status 128.\n\nWhile at it, clean up parse_options_check to prepare for more checks.\n\nLong term, it would be nicer to make such checks happen at compile\ntime.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n parse-options.c |   25 ++++++++++---------------\n 1 files changed, 10 insertions(+), 15 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 196ba71..12f100b 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -11,6 +11,13 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,\n #define OPT_SHORT 1\n #define OPT_UNSET 2\n \n+static NORETURN void optbug(const struct option *opt, const char *reason)\n+{\n+\tif (opt->long_name)\n+\t\tdie(\"BUG: option '%s' %s\", opt->long_name, reason);\n+\tdie(\"BUG: switch '%c' %s\", opt->short_name, reason);\n+}\n+\n static int opterror(const struct option *opt, const char *reason, int flags)\n {\n \tif (flags & OPT_SHORT)\n@@ -316,24 +323,12 @@ static void check_typos(const char *arg, const struct option *options)\n \n static void parse_options_check(const struct option *opts)\n {\n-\tint err = 0;\n-\n \tfor (; opts->type != OPTION_END; opts++) {\n \t\tif ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) &&\n-\t\t    (opts->flags & PARSE_OPT_OPTARG)) {\n-\t\t\tif (opts->long_name) {\n-\t\t\t\terror(\"`--%s` uses incompatible flags \"\n-\t\t\t\t      \"LASTARG_DEFAULT and OPTARG\", opts->long_name);\n-\t\t\t} else {\n-\t\t\t\terror(\"`-%c` uses incompatible flags \"\n-\t\t\t\t      \"LASTARG_DEFAULT and OPTARG\", opts->short_name);\n-\t\t\t}\n-\t\t\terr |= 1;\n-\t\t}\n+\t\t    (opts->flags & PARSE_OPT_OPTARG))\n+\t\t\toptbug(opts, \"uses incompatible flags \"\n+\t\t\t\t\"LASTARG_DEFAULT and OPTARG\");\n \t}\n-\n-\tif (err)\n-\t\texit(129);\n }\n \n void parse_options_start(struct parse_opt_ctx_t *ctx,\n-- \n1.7.2.3\n"},{"id":"157051","messageId":"20101201232950.GD31815@burratino","threadId":"25917","inReplyTo":"20101201232728.GA31815@burratino","subject":"[PATCH 03/10] parse-options: move NODASH sanity checks to parse_options_check","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-01T23:29:51Z","receivedAt":"2010-12-01T23:29:51Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"A dashless switch (like '(' passed to 'git grep') cannot be negated,\ncannot be attached to an argument, and cannot have a long form.\nCurrently parse-options runs the related sanity checks when the\ndashless option is used; better to always check them at the start of\noption parsing, so mistakes can be caught more quickly.\n\nThe error message at the new call site is less specific about the\nnature of the error, for simplicity.  On the other hand, it is more\nspecific in that it prints which switch was problematic.  Before:\n\n\tfatal: BUG: dashless options can't be long\n\nAfter:\n\n\tfatal: BUG: switch '(' uses feature not supported for dashless options\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n parse-options.c |   14 +++++++-------\n 1 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 12f100b..c825620 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -288,13 +288,6 @@ static int parse_nodash_opt(struct parse_opt_ctx_t *p, const char *arg,\n \tfor (; options->type != OPTION_END; options++) {\n \t\tif (!(options->flags & PARSE_OPT_NODASH))\n \t\t\tcontinue;\n-\t\tif ((options->flags & PARSE_OPT_OPTARG) ||\n-\t\t    !(options->flags & PARSE_OPT_NOARG))\n-\t\t\tdie(\"BUG: dashless options don't support arguments\");\n-\t\tif (!(options->flags & PARSE_OPT_NONEG))\n-\t\t\tdie(\"BUG: dashless options don't support negation\");\n-\t\tif (options->long_name)\n-\t\t\tdie(\"BUG: dashless options can't be long\");\n \t\tif (options->short_name == arg[0] && arg[1] == '\\0')\n \t\t\treturn get_value(p, options, OPT_SHORT);\n \t}\n@@ -328,6 +321,13 @@ static void parse_options_check(const struct option *opts)\n \t\t    (opts->flags & PARSE_OPT_OPTARG))\n \t\t\toptbug(opts, \"uses incompatible flags \"\n \t\t\t\t\"LASTARG_DEFAULT and OPTARG\");\n+\t\tif (opts->flags & PARSE_OPT_NODASH &&\n+\t\t    ((opts->flags & PARSE_OPT_OPTARG) ||\n+\t\t     !(opts->flags & PARSE_OPT_NOARG) ||\n+\t\t     !(opts->flags & PARSE_OPT_NONEG) ||\n+\t\t     opts->long_name))\n+\t\t\toptbug(opts, \"uses feature \"\n+\t\t\t\t\"not supported for dashless options\");\n \t}\n }\n \n-- \n1.7.2.3\n"},{"id":"157052","messageId":"20101201233020.GE31815@burratino","threadId":"25917","inReplyTo":"20101201232728.GA31815@burratino","subject":"[PATCH 04/10] parse-options: sanity check PARSE_OPT_NOARG flag","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-01T23:30:20Z","receivedAt":"2010-12-01T23:30:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Some option types cannot use an argument --- boolean options that\nwould set a bit or flag or increment a counter, for example.  If\nconfigured in the flag word to accept an argument anyway, the result\nis an argument that is advertised in \"program -h\" output only to be\nrejected by parse-options::get_value.\n\nLuckily all current users of these option types use PARSE_OPT_NOARG\nand do not use PARSE_OPT_OPTARG.  Add a check to ensure that that\nremains true.  The check is run once for each invocation of\nparse_option_start().\n\nImproved-by: Stephen Boyd <bebarino@gmail.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n parse-options.c |   13 +++++++++++++\n 1 files changed, 13 insertions(+), 0 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex c825620..e4d0b82 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -328,6 +328,19 @@ static void parse_options_check(const struct option *opts)\n \t\t     opts->long_name))\n \t\t\toptbug(opts, \"uses feature \"\n \t\t\t\t\"not supported for dashless options\");\n+\t\tswitch (opts->type) {\n+\t\tcase OPTION_BOOLEAN:\n+\t\tcase OPTION_BIT:\n+\t\tcase OPTION_NEGBIT:\n+\t\tcase OPTION_SET_INT:\n+\t\tcase OPTION_SET_PTR:\n+\t\tcase OPTION_NUMBER:\n+\t\t\tif ((opts->flags & PARSE_OPT_OPTARG) ||\n+\t\t\t    !(opts->flags & PARSE_OPT_NOARG))\n+\t\t\t\toptbug(opts, \"should not accept an argument\");\n+\t\tdefault:\n+\t\t\t; /* ok. (usually accepts an argument) */\n+\t\t}\n \t}\n }\n \n-- \n1.7.2.3\n"},{"id":"157053","messageId":"20101201233040.GF31815@burratino","threadId":"25917","inReplyTo":"20101201232728.GA31815@burratino","subject":"[PATCH 05/10] parse-options: do not infer PARSE_OPT_NOARG from option type","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-01T23:30:40Z","receivedAt":"2010-12-01T23:30:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Stephen Boyd <bebarino@gmail.com>\n\nSimplify the \"takes no value\" error path by relying on PARSE_OPT_NOARG\nbeing set correctly.  That is:\n\n - if the PARSE_OPT_NOARG flag is set, reject --opt=value\n   regardless of the option type;\n - if the PARSE_OPT_NOARG flag is unset, accept --opt=value\n   regardless of the option type.\n\nThis way, the accepted usage more closely matches the usage advertised\nwith --help-all.\n\nNo functional change intended, since the NOARG flag is only used\nwith \"boolean-only\" option types in existing parse_options callers.\n\nSigned-off-by: Stephen Boyd <bebarino@gmail.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n parse-options.c |   19 ++-----------------\n 1 files changed, 2 insertions(+), 17 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex e4d0b82..684d330 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -62,23 +62,8 @@ static int get_value(struct parse_opt_ctx_t *p,\n \t\treturn opterror(opt, \"takes no value\", flags);\n \tif (unset && (opt->flags & PARSE_OPT_NONEG))\n \t\treturn opterror(opt, \"isn't available\", flags);\n-\n-\tif (!(flags & OPT_SHORT) && p->opt) {\n-\t\tswitch (opt->type) {\n-\t\tcase OPTION_CALLBACK:\n-\t\t\tif (!(opt->flags & PARSE_OPT_NOARG))\n-\t\t\t\tbreak;\n-\t\t\t/* FALLTHROUGH */\n-\t\tcase OPTION_BOOLEAN:\n-\t\tcase OPTION_BIT:\n-\t\tcase OPTION_NEGBIT:\n-\t\tcase OPTION_SET_INT:\n-\t\tcase OPTION_SET_PTR:\n-\t\t\treturn opterror(opt, \"takes no value\", flags);\n-\t\tdefault:\n-\t\t\tbreak;\n-\t\t}\n-\t}\n+\tif (!(flags & OPT_SHORT) && p->opt && (opt->flags & PARSE_OPT_NOARG))\n+\t\treturn opterror(opt, \"takes no value\", flags);\n \n \tswitch (opt->type) {\n \tcase OPTION_BIT:\n-- \n1.7.2.3\n"},{"id":"157054","messageId":"20101201233136.GG31815@burratino","threadId":"25917","inReplyTo":"20101201232728.GA31815@burratino","subject":"[PATCH 06/10] parse-options: never suppress arghelp if LITERAL_ARGHELP is set","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-01T23:31:36Z","receivedAt":"2010-12-01T23:31:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The PARSE_OPT_LITERAL_ARGHELP flag allows a program to override the\nstandard \"<argument> for mandatory, [argument] for optional\" markup in\nits help message.  Extend it to override the usual \"no text for\ndisallowed\", too (for the PARSE_OPT_NOARG | PARSE_OPT_LITERAL_ARGHELP\ncase, which was previously meaningless), to be more intuitive.\n\nThe motivation is to allow update-index to correctly advertise\n\n\t--cacheinfo <mode> <object> <path>\n\t                      add the specified entry to the index\n\nwhile abusing PARSE_OPT_NOARG to disallow the \"sticked form\"\n\n\t--cacheinfo=<mode> <object> <path>\n\nNoticed-by: Stephen Boyd <bebarino@gmail.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nThis seems like the intuitive thing to do, but the motivating use case\nis iffy.  Might be better to introduce a PARSE_OPT_NOSTICKED flag.\n\n parse-options.c |    3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 684d330..b640ac5 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -533,7 +533,8 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n \t\tif (opts->type == OPTION_NUMBER)\n \t\t\tpos += fprintf(outfile, \"-NUM\");\n \n-\t\tif (!(opts->flags & PARSE_OPT_NOARG))\n+\t\tif ((opts->flags & PARSE_OPT_LITERAL_ARGHELP) ||\n+\t\t    !(opts->flags & PARSE_OPT_NOARG))\n \t\t\tpos += usage_argh(opts, outfile);\n \n \t\tif (pos <= USAGE_OPTS_WIDTH)\n-- \n1.7.2.3\n"},{"id":"157055","messageId":"20101201233216.GH31815@burratino","threadId":"25917","inReplyTo":"20101201232728.GA31815@burratino","subject":"[PATCH 07/10] parse-options: allow git commands to invent new option types","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-01T23:32:16Z","receivedAt":"2010-12-01T23:32:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"parse-options provides a variety of option behaviors, including\nOPTION_CALLBACK, which should take care of just about any sane\nbehavior.  All supported behaviors obey the following constraint:\n\n A --foo option can only accept (and base its behavior on)\n one argument, which would be the following command-line\n argument in the \"unsticked\" form.\n\nAlas, some existing git commands have options that do not obey that\nconstraint.  For example, update-index --cacheinfo takes three\narguments, and update-index --resolve takes all later parameters as\narguments.\n\nIntroduces an OPTION_LOWLEVEL_CALLBACK backdoor to parse-options so\nsuch option types can be supported without tempting inventors of other\ncommands through mention in the public API.  Commands can set the\ncallback field to a function accepting three arguments: the option\nparsing context, the option itself, and a flag indicating whether the\nthe option was negated.  When the option is encountered, that function\nis called to take over from get_value().  The return value should be\nzero for success, -1 for usage errors.\n\nThanks to Stephen Boyd for API guidance.\n\nImproved-by: Stephen Boyd <bebarino@gmail.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n parse-options.c |    3 +++\n parse-options.h |    8 +++++++-\n 2 files changed, 10 insertions(+), 1 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex b640ac5..4c58e7f 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -66,6 +66,9 @@ static int get_value(struct parse_opt_ctx_t *p,\n \t\treturn opterror(opt, \"takes no value\", flags);\n \n \tswitch (opt->type) {\n+\tcase OPTION_LOWLEVEL_CALLBACK:\n+\t\treturn (*(parse_opt_ll_cb *)opt->callback)(p, opt, unset);\n+\n \tcase OPTION_BIT:\n \t\tif (unset)\n \t\t\t*(int *)opt->value &= ~opt->defval;\ndiff --git a/parse-options.h b/parse-options.h\nindex cfa03d5..ab1bdf0 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -17,6 +17,7 @@ enum parse_opt_type {\n \tOPTION_STRING,\n \tOPTION_INTEGER,\n \tOPTION_CALLBACK,\n+\tOPTION_LOWLEVEL_CALLBACK,\n \tOPTION_FILENAME\n };\n \n@@ -43,6 +44,10 @@ enum parse_opt_option_flags {\n struct option;\n typedef int parse_opt_cb(const struct option *, const char *arg, int unset);\n \n+struct parse_opt_ctx_t;\n+typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,\n+\t\t\t\tconst struct option *opt, int unset);\n+\n /*\n  * `type`::\n  *   holds the type of the option, you must have an OPTION_END last in your\n@@ -87,7 +92,8 @@ typedef int parse_opt_cb(const struct option *, const char *arg, int unset);\n  *\t\t\t\tuseful for users of OPTION_NEGBIT.\n  *\n  * `callback`::\n- *   pointer to the callback to use for OPTION_CALLBACK.\n+ *   pointer to the callback to use for OPTION_CALLBACK or\n+ *   OPTION_LOWLEVEL_CALLBACK.\n  *\n  * `defval`::\n  *   default value to fill (*->value) with for PARSE_OPT_OPTARG.\n-- \n1.7.2.3\n"},{"id":"157056","messageId":"20101201233255.GI31815@burratino","threadId":"25917","inReplyTo":"20101201232728.GA31815@burratino","subject":"[PATCH 08/10] parse-options: make resuming easier after PARSE_OPT_STOP_AT_NON_OPTION","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-01T23:32:55Z","receivedAt":"2010-12-01T23:32:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Introduce a PARSE_OPT_NON_OPTION state, so parse_option_step()\ncallers can easily distinguish between non-options and other\nreasons for option parsing termination (like \"--\").\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n parse-options.c |    3 ++-\n parse-options.h |    1 +\n 2 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 4c58e7f..4b000e6 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -369,7 +369,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\tif (parse_nodash_opt(ctx, arg, options) == 0)\n \t\t\t\tcontinue;\n \t\t\tif (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION)\n-\t\t\t\tbreak;\n+\t\t\t\treturn PARSE_OPT_NON_OPTION;\n \t\t\tctx->out[ctx->cpidx++] = ctx->argv[0];\n \t\t\tcontinue;\n \t\t}\n@@ -451,6 +451,7 @@ int parse_options(int argc, const char **argv, const char *prefix,\n \tswitch (parse_options_step(&ctx, options, usagestr)) {\n \tcase PARSE_OPT_HELP:\n \t\texit(129);\n+\tcase PARSE_OPT_NON_OPTION:\n \tcase PARSE_OPT_DONE:\n \t\tbreak;\n \tdefault: /* PARSE_OPT_UNKNOWN */\ndiff --git a/parse-options.h b/parse-options.h\nindex ab1bdf0..b897d6b 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -167,6 +167,7 @@ extern NORETURN void usage_msg_opt(const char *msg,\n enum {\n \tPARSE_OPT_HELP = -1,\n \tPARSE_OPT_DONE,\n+\tPARSE_OPT_NON_OPTION,\n \tPARSE_OPT_UNKNOWN\n };\n \n-- \n1.7.2.3\n"},{"id":"157057","messageId":"20101201233322.GJ31815@burratino","threadId":"25917","inReplyTo":"20101201232728.GA31815@burratino","subject":"[PATCH 09/10] setup: save prefix (original cwd relative to toplevel) in startup_info","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-01T23:33:22Z","receivedAt":"2010-12-01T23:33:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n\nSave the path from the original cwd to the cwd at the end of the\nsetup procedure in the startup_info struct introduced in e37c1329\n(2010-08-05).  The value cannot vary from thread to thread anyway,\nsince the cwd is global.\n\nSo now in your builtin command, instead of passing prefix around,\nwhen you want to convert a user-supplied path to a cwd-relative\npath, you can use startup_info->prefix directly.\n\nCaveat: As with the return value from setup_git_directory_gently(),\nstartup_info->prefix would be NULL when the original cwd is not a\nsubdir of the toplevel.\n\nLonger term, this would allow the prefix to be reused when several\nnoncooperating functions require access to the same repository (for\nexample, when accessing configuration before running a builtin).\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n cache.h |    1 +\n setup.c |    4 +++-\n 2 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 33decd9..222d9cf 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1117,6 +1117,7 @@ const char *split_cmdline_strerror(int cmdline_errno);\n /* git.c */\n struct startup_info {\n \tint have_repository;\n+\tconst char *prefix;\n };\n extern struct startup_info *startup_info;\n \ndiff --git a/setup.c b/setup.c\nindex a3b76de..833db12 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -512,8 +512,10 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \tconst char *prefix;\n \n \tprefix = setup_git_directory_gently_1(nongit_ok);\n-\tif (startup_info)\n+\tif (startup_info) {\n \t\tstartup_info->have_repository = !nongit_ok || !*nongit_ok;\n+\t\tstartup_info->prefix = prefix;\n+\t}\n \treturn prefix;\n }\n \n-- \n1.7.2.3\n"},{"id":"157058","messageId":"20101201233406.GK31815@burratino","threadId":"25917","inReplyTo":"20101201232728.GA31815@burratino","subject":"[PATCH 10/10] update-index: migrate to parse-options API","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-01T23:34:06Z","receivedAt":"2010-12-01T23:34:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"--refresh and --really-refresh accept flags (like -q) and modify\nan error indicator.  It might make sense to make the error\nindicator global, but just pass the flags and a pointer to the error\nindicator in a struct instead.\n\n--cacheinfo wants 3 arguments.  Use the OPTION_LOWLEVEL_CALLBACK\nextension to grab them and PARSE_OPT_NOARG to disallow the \"sticked\"\n--cacheinfo=foo form.  (The resulting message\n\n\t$ git update-index --cacheinfo=foo\n\terror: option `cacheinfo' takes no value\n\nis unfortunately incorrect.)\n\n--assume-unchanged and --no-assume-unchanged probably should use the\nOPT_UYN feature; but use a callback for now so the existing MARK_FLAG\nand UNMARK_FLAG values can be used.\n\n--stdin and --index-info are still constrained to be the last argument\n(implemented using the OPTION_LOWLEVEL_CALLBACK extension).\n\n--unresolve and --again consume all arguments that come after them\n(also using OPTION_LOWLEVEL_CALLBACK).\n\nThe order of options matters.  Each path on the command line is\naffected only by the options that come before it.  A custom\nargument-parsing loop with parse_options_step() brings that about.\n\nIn exchange for all the fuss, we get the usual perks: support for\nun-sticked options, better usage error messages, more useful -h\noutput, and argument parsing code that should be easier to tweak\nin the future.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n builtin/update-index.c |  392 ++++++++++++++++++++++++++++++------------------\n 1 files changed, 243 insertions(+), 149 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 62d9f3f..869f5b1 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -10,6 +10,7 @@\n #include \"builtin.h\"\n #include \"refs.h\"\n #include \"resolve-undo.h\"\n+#include \"parse-options.h\"\n \n /*\n  * Default to not allowing changes to the list of files. The\n@@ -397,8 +398,10 @@ static void read_index_info(int line_termination)\n \tstrbuf_release(&uq);\n }\n \n-static const char update_index_usage[] =\n-\"git update-index [-q] [--add] [--replace] [--remove] [--unmerged] [--refresh] [--really-refresh] [--cacheinfo] [--chmod=(+|-)x] [--assume-unchanged] [--skip-worktree|--no-skip-worktree] [--info-only] [--force-remove] [--stdin] [--index-info] [--unresolve] [--again | -g] [--ignore-missing] [-z] [--verbose] [--] [<file>...]\";\n+static const char * const update_index_usage[] = {\n+\t\"git update-index [options] [--] [<file>...]\",\n+\tNULL\n+};\n \n static unsigned char head_sha1[20];\n static unsigned char merge_head_sha1[20];\n@@ -578,16 +581,211 @@ static int do_reupdate(int ac, const char **av,\n \treturn 0;\n }\n \n+struct refresh_params {\n+\tunsigned int flags;\n+\tint *has_errors;\n+};\n+\n+static int refresh(struct refresh_params *o, unsigned int flag)\n+{\n+\tsetup_work_tree();\n+\t*o->has_errors |= refresh_cache(o->flags | flag);\n+\treturn 0;\n+}\n+\n+static int refresh_callback(const struct option *opt,\n+\t\t\t\tconst char *arg, int unset)\n+{\n+\treturn refresh(opt->value, 0);\n+}\n+\n+static int really_refresh_callback(const struct option *opt,\n+\t\t\t\tconst char *arg, int unset)\n+{\n+\treturn refresh(opt->value, REFRESH_REALLY);\n+}\n+\n+static int chmod_callback(const struct option *opt,\n+\t\t\t\tconst char *arg, int unset)\n+{\n+\tchar *flip = opt->value;\n+\tif ((arg[0] != '-' && arg[0] != '+') || arg[1] != 'x' || arg[2])\n+\t\treturn error(\"option 'chmod' expects \\\"+x\\\" or \\\"-x\\\"\");\n+\t*flip = arg[0];\n+\treturn 0;\n+}\n+\n+static int resolve_undo_clear_callback(const struct option *opt,\n+\t\t\t\tconst char *arg, int unset)\n+{\n+\tresolve_undo_clear();\n+\treturn 0;\n+}\n+\n+static int cacheinfo_callback(struct parse_opt_ctx_t *ctx,\n+\t\t\t\tconst struct option *opt, int unset)\n+{\n+\tunsigned char sha1[20];\n+\tunsigned int mode;\n+\n+\tif (ctx->argc <= 3)\n+\t\treturn error(\"option 'cacheinfo' expects three arguments\");\n+\tif (strtoul_ui(*++ctx->argv, 8, &mode) ||\n+\t    get_sha1_hex(*++ctx->argv, sha1) ||\n+\t    add_cacheinfo(mode, sha1, *++ctx->argv, 0))\n+\t\tdie(\"git update-index: --cacheinfo cannot add %s\", *ctx->argv);\n+\tctx->argc -= 3;\n+\treturn 0;\n+}\n+\n+static int stdin_cacheinfo_callback(struct parse_opt_ctx_t *ctx,\n+\t\t\t      const struct option *opt, int unset)\n+{\n+\tint *line_termination = opt->value;\n+\n+\tif (ctx->argc != 1)\n+\t\treturn error(\"option '%s' must be the last argument\", opt->long_name);\n+\tallow_add = allow_replace = allow_remove = 1;\n+\tread_index_info(*line_termination);\n+\treturn 0;\n+}\n+\n+static int stdin_callback(struct parse_opt_ctx_t *ctx,\n+\t\t\t\tconst struct option *opt, int unset)\n+{\n+\tint *read_from_stdin = opt->value;\n+\n+\tif (ctx->argc != 1)\n+\t\treturn error(\"option '%s' must be the last argument\", opt->long_name);\n+\t*read_from_stdin = 1;\n+\treturn 0;\n+}\n+\n+static int unresolve_callback(struct parse_opt_ctx_t *ctx,\n+\t\t\t\tconst struct option *opt, int flags)\n+{\n+\tint *has_errors = opt->value;\n+\tconst char *prefix = startup_info->prefix;\n+\n+\t/* consume remaining arguments. */\n+\t*has_errors = do_unresolve(ctx->argc, ctx->argv,\n+\t\t\t\tprefix, prefix ? strlen(prefix) : 0);\n+\tif (*has_errors)\n+\t\tactive_cache_changed = 0;\n+\n+\tctx->argv += ctx->argc - 1;\n+\tctx->argc = 1;\n+\treturn 0;\n+}\n+\n+static int reupdate_callback(struct parse_opt_ctx_t *ctx,\n+\t\t\t\tconst struct option *opt, int flags)\n+{\n+\tint *has_errors = opt->value;\n+\tconst char *prefix = startup_info->prefix;\n+\n+\t/* consume remaining arguments. */\n+\tsetup_work_tree();\n+\t*has_errors = do_reupdate(ctx->argc, ctx->argv,\n+\t\t\t\tprefix, prefix ? strlen(prefix) : 0);\n+\tif (*has_errors)\n+\t\tactive_cache_changed = 0;\n+\n+\tctx->argv += ctx->argc - 1;\n+\tctx->argc = 1;\n+\treturn 0;\n+}\n+\n int cmd_update_index(int argc, const char **argv, const char *prefix)\n {\n-\tint i, newfd, entries, has_errors = 0, line_termination = '\\n';\n-\tint allow_options = 1;\n+\tint newfd, entries, has_errors = 0, line_termination = '\\n';\n \tint read_from_stdin = 0;\n \tint prefix_length = prefix ? strlen(prefix) : 0;\n \tchar set_executable_bit = 0;\n-\tunsigned int refresh_flags = 0;\n+\tstruct refresh_params refresh_args = {0, &has_errors};\n \tint lock_error = 0;\n \tstruct lock_file *lock_file;\n+\tstruct parse_opt_ctx_t ctx;\n+\tint parseopt_state = PARSE_OPT_UNKNOWN;\n+\tstruct option options[] = {\n+\t\tOPT_BIT('q', NULL, &refresh_args.flags,\n+\t\t\t\"continue refresh even when index needs update\",\n+\t\t\tREFRESH_QUIET),\n+\t\tOPT_BIT(0, \"ignore-submodules\", &refresh_args.flags,\n+\t\t\t\"refresh: ignore submodules\",\n+\t\t\tREFRESH_IGNORE_SUBMODULES),\n+\t\tOPT_SET_INT(0, \"add\", &allow_add,\n+\t\t\t\"do not ignore new files\", 1),\n+\t\tOPT_SET_INT(0, \"replace\", &allow_replace,\n+\t\t\t\"let files replace directories and vice-versa\", 1),\n+\t\tOPT_SET_INT(0, \"remove\", &allow_remove,\n+\t\t\t\"notice files missing from worktree\", 1),\n+\t\tOPT_BIT(0, \"unmerged\", &refresh_args.flags,\n+\t\t\t\"refresh even if index contains unmerged entries\",\n+\t\t\tREFRESH_UNMERGED),\n+\t\t{OPTION_CALLBACK, 0, \"refresh\", &refresh_args, NULL,\n+\t\t\t\"refresh stat information\",\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG,\n+\t\t\trefresh_callback},\n+\t\t{OPTION_CALLBACK, 0, \"really-refresh\", &refresh_args, NULL,\n+\t\t\t\"like --refresh, but ignore assume-unchanged setting\",\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG,\n+\t\t\treally_refresh_callback},\n+\t\t{OPTION_LOWLEVEL_CALLBACK, 0, \"cacheinfo\", NULL,\n+\t\t\t\"<mode> <object> <path>\",\n+\t\t\t\"add the specified entry to the index\",\n+\t\t\tPARSE_OPT_NOARG |\t/* disallow --cacheinfo=<mode> form */\n+\t\t\tPARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n+\t\t\t(parse_opt_cb *) cacheinfo_callback},\n+\t\t{OPTION_CALLBACK, 0, \"chmod\", &set_executable_bit, \"(+/-)x\",\n+\t\t\t\"override the executable bit of the listed files\",\n+\t\t\tPARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n+\t\t\tchmod_callback},\n+\t\t{OPTION_SET_INT, 0, \"assume-unchanged\", &mark_valid_only, NULL,\n+\t\t\t\"mark files as \\\"not changing\\\"\",\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},\n+\t\t{OPTION_SET_INT, 0, \"no-assume-unchanged\", &mark_valid_only, NULL,\n+\t\t\t\"clear assumed-unchanged bit\",\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, UNMARK_FLAG},\n+\t\t{OPTION_SET_INT, 0, \"skip-worktree\", &mark_skip_worktree_only, NULL,\n+\t\t\t\"mark files as \\\"index-only\\\"\",\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},\n+\t\t{OPTION_SET_INT, 0, \"no-skip-worktree\", &mark_skip_worktree_only, NULL,\n+\t\t\t\"clear skip-worktree bit\",\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, UNMARK_FLAG},\n+\t\tOPT_SET_INT(0, \"info-only\", &info_only,\n+\t\t\t\"add to index only; do not add content to object database\", 1),\n+\t\tOPT_SET_INT(0, \"force-remove\", &force_remove,\n+\t\t\t\"remove named paths even if present in worktree\", 1),\n+\t\tOPT_SET_INT('z', NULL, &line_termination,\n+\t\t\t\"with --stdin: input lines are terminated by null bytes\", '\\0'),\n+\t\t{OPTION_LOWLEVEL_CALLBACK, 0, \"stdin\", &read_from_stdin, NULL,\n+\t\t\t\"read list of paths to be updated from standard input\",\n+\t\t\tPARSE_OPT_NONEG | PARSE_OPT_NOARG,\n+\t\t\t(parse_opt_cb *) stdin_callback},\n+\t\t{OPTION_LOWLEVEL_CALLBACK, 0, \"index-info\", &line_termination, NULL,\n+\t\t\t\"add entries from standard input to the index\",\n+\t\t\tPARSE_OPT_NONEG | PARSE_OPT_NOARG,\n+\t\t\t(parse_opt_cb *) stdin_cacheinfo_callback},\n+\t\t{OPTION_LOWLEVEL_CALLBACK, 0, \"unresolve\", &has_errors, NULL,\n+\t\t\t\"repopulate stages #2 and #3 for the listed paths\",\n+\t\t\tPARSE_OPT_NONEG | PARSE_OPT_NOARG,\n+\t\t\t(parse_opt_cb *) unresolve_callback},\n+\t\t{OPTION_LOWLEVEL_CALLBACK, 'g', \"again\", &has_errors, NULL,\n+\t\t\t\"only update entries that differ from HEAD\",\n+\t\t\tPARSE_OPT_NONEG | PARSE_OPT_NOARG,\n+\t\t\t(parse_opt_cb *) reupdate_callback},\n+\t\tOPT_BIT(0, \"ignore-missing\", &refresh_args.flags,\n+\t\t\t\"ignore files missing from worktree\",\n+\t\t\tREFRESH_IGNORE_MISSING),\n+\t\tOPT_SET_INT(0, \"verbose\", &verbose,\n+\t\t\t\"report actions to standard output\", 1),\n+\t\t{OPTION_CALLBACK, 0, \"clear-resolve-undo\", NULL, NULL,\n+\t\t\t\"(for porcelains) forget saved unresolved conflicts\",\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG,\n+\t\t\tresolve_undo_clear_callback},\n+\t\tOPT_END()\n+\t};\n \n \tgit_config(git_default_config, NULL);\n \n@@ -602,151 +800,48 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \tif (entries < 0)\n \t\tdie(\"cache corrupted\");\n \n-\tfor (i = 1 ; i < argc; i++) {\n-\t\tconst char *path = argv[i];\n-\t\tconst char *p;\n+\t/*\n+\t * Custom copy of parse_options() because we want to handle\n+\t * filename arguments as they come.\n+\t */\n+\tparse_options_start(&ctx, argc, argv, prefix,\n+\t\t\t    PARSE_OPT_STOP_AT_NON_OPTION, options);\n+\twhile (ctx.argc) {\n+\t\tif (parseopt_state != PARSE_OPT_DONE)\n+\t\t\tparseopt_state = parse_options_step(&ctx, options,\n+\t\t\t\t\t\t\t    update_index_usage);\n+\t\tif (!ctx.argc)\n+\t\t\tbreak;\n+\t\tswitch (parseopt_state) {\n+\t\tcase PARSE_OPT_HELP:\n+\t\t\texit(129);\n+\t\tcase PARSE_OPT_NON_OPTION:\n+\t\tcase PARSE_OPT_DONE:\n+\t\t{\n+\t\t\tconst char *path = ctx.argv[0];\n+\t\t\tconst char *p;\n \n-\t\tif (allow_options && *path == '-') {\n-\t\t\tif (!strcmp(path, \"--\")) {\n-\t\t\t\tallow_options = 0;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"-q\")) {\n-\t\t\t\trefresh_flags |= REFRESH_QUIET;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--ignore-submodules\")) {\n-\t\t\t\trefresh_flags |= REFRESH_IGNORE_SUBMODULES;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--add\")) {\n-\t\t\t\tallow_add = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--replace\")) {\n-\t\t\t\tallow_replace = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--remove\")) {\n-\t\t\t\tallow_remove = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--unmerged\")) {\n-\t\t\t\trefresh_flags |= REFRESH_UNMERGED;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--refresh\")) {\n-\t\t\t\tsetup_work_tree();\n-\t\t\t\thas_errors |= refresh_cache(refresh_flags);\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--really-refresh\")) {\n-\t\t\t\tsetup_work_tree();\n-\t\t\t\thas_errors |= refresh_cache(REFRESH_REALLY | refresh_flags);\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--cacheinfo\")) {\n-\t\t\t\tunsigned char sha1[20];\n-\t\t\t\tunsigned int mode;\n-\n-\t\t\t\tif (i+3 >= argc)\n-\t\t\t\t\tdie(\"git update-index: --cacheinfo <mode> <sha1> <path>\");\n-\n-\t\t\t\tif (strtoul_ui(argv[i+1], 8, &mode) ||\n-\t\t\t\t    get_sha1_hex(argv[i+2], sha1) ||\n-\t\t\t\t    add_cacheinfo(mode, sha1, argv[i+3], 0))\n-\t\t\t\t\tdie(\"git update-index: --cacheinfo\"\n-\t\t\t\t\t    \" cannot add %s\", argv[i+3]);\n-\t\t\t\ti += 3;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--chmod=-x\") ||\n-\t\t\t    !strcmp(path, \"--chmod=+x\")) {\n-\t\t\t\tif (argc <= i+1)\n-\t\t\t\t\tdie(\"git update-index: %s <path>\", path);\n-\t\t\t\tset_executable_bit = path[8];\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--assume-unchanged\")) {\n-\t\t\t\tmark_valid_only = MARK_FLAG;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--no-assume-unchanged\")) {\n-\t\t\t\tmark_valid_only = UNMARK_FLAG;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--no-skip-worktree\")) {\n-\t\t\t\tmark_skip_worktree_only = UNMARK_FLAG;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--skip-worktree\")) {\n-\t\t\t\tmark_skip_worktree_only = MARK_FLAG;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--info-only\")) {\n-\t\t\t\tinfo_only = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--force-remove\")) {\n-\t\t\t\tforce_remove = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"-z\")) {\n-\t\t\t\tline_termination = 0;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--stdin\")) {\n-\t\t\t\tif (i != argc - 1)\n-\t\t\t\t\tdie(\"--stdin must be at the end\");\n-\t\t\t\tread_from_stdin = 1;\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--index-info\")) {\n-\t\t\t\tif (i != argc - 1)\n-\t\t\t\t\tdie(\"--index-info must be at the end\");\n-\t\t\t\tallow_add = allow_replace = allow_remove = 1;\n-\t\t\t\tread_index_info(line_termination);\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--unresolve\")) {\n-\t\t\t\thas_errors = do_unresolve(argc - i, argv + i,\n-\t\t\t\t\t\t\t  prefix, prefix_length);\n-\t\t\t\tif (has_errors)\n-\t\t\t\t\tactive_cache_changed = 0;\n-\t\t\t\tgoto finish;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--again\") || !strcmp(path, \"-g\")) {\n-\t\t\t\tsetup_work_tree();\n-\t\t\t\thas_errors = do_reupdate(argc - i, argv + i,\n-\t\t\t\t\t\t\t prefix, prefix_length);\n-\t\t\t\tif (has_errors)\n-\t\t\t\t\tactive_cache_changed = 0;\n-\t\t\t\tgoto finish;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--ignore-missing\")) {\n-\t\t\t\trefresh_flags |= REFRESH_IGNORE_MISSING;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--verbose\")) {\n-\t\t\t\tverbose = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--clear-resolve-undo\")) {\n-\t\t\t\tresolve_undo_clear();\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"-h\") || !strcmp(path, \"--help\"))\n-\t\t\t\tusage(update_index_usage);\n-\t\t\tdie(\"unknown option %s\", path);\n+\t\t\tsetup_work_tree();\n+\t\t\tp = prefix_path(prefix, prefix_length, path);\n+\t\t\tupdate_one(p, NULL, 0);\n+\t\t\tif (set_executable_bit)\n+\t\t\t\tchmod_path(set_executable_bit, p);\n+\t\t\tif (p < path || p > path + strlen(path))\n+\t\t\t\tfree((char *)p);\n+\t\t\tctx.argc--;\n+\t\t\tctx.argv++;\n+\t\t\tbreak;\n+\t\t}\n+\t\tcase PARSE_OPT_UNKNOWN:\n+\t\t\tif (ctx.argv[0][1] == '-')\n+\t\t\t\terror(\"unknown option '%s'\", ctx.argv[0] + 2);\n+\t\t\telse\n+\t\t\t\terror(\"unknown switch '%c'\", *ctx.opt);\n+\t\t\tusage_with_options(update_index_usage, options);\n \t\t}\n-\t\tsetup_work_tree();\n-\t\tp = prefix_path(prefix, prefix_length, path);\n-\t\tupdate_one(p, NULL, 0);\n-\t\tif (set_executable_bit)\n-\t\t\tchmod_path(set_executable_bit, p);\n-\t\tif (p < path || p > path + strlen(path))\n-\t\t\tfree((char *)p);\n \t}\n+\targc = parse_options_end(&ctx);\n+\n \tif (read_from_stdin) {\n \t\tstruct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT;\n \n@@ -770,10 +865,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_release(&buf);\n \t}\n \n- finish:\n \tif (active_cache_changed) {\n \t\tif (newfd < 0) {\n-\t\t\tif (refresh_flags & REFRESH_QUIET)\n+\t\t\tif (refresh_args.flags & REFRESH_QUIET)\n \t\t\t\texit(128);\n \t\t\tunable_to_lock_index_die(get_index_file(), lock_error);\n \t\t}\n-- \n1.7.2.3\n"},{"id":"157075","messageId":"20101202045707.GA32125@burratino","threadId":"25917","inReplyTo":"20101201232923.GC31815@burratino","subject":"Re: [PATCH 02/10] parse-options: clearer reporting of API misuse","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-02T04:57:07Z","receivedAt":"2010-12-02T04:57:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -316,24 +323,12 @@ static void check_typos(const char *arg, const struct option *options)\n>  \n>  static void parse_options_check(const struct option *opts)\n>  {\n> -\tint err = 0;\n> -\n>  \tfor (; opts->type != OPTION_END; opts++) {\n>  \t\tif ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) &&\n> -\t\t    (opts->flags & PARSE_OPT_OPTARG)) {\n> -\t\t\tif (opts->long_name) {\n> -\t\t\t\terror(\"`--%s` uses incompatible flags \"\n> -\t\t\t\t      \"LASTARG_DEFAULT and OPTARG\", opts->long_name);\n> -\t\t\t} else {\n> -\t\t\t\terror(\"`-%c` uses incompatible flags \"\n> -\t\t\t\t      \"LASTARG_DEFAULT and OPTARG\", opts->short_name);\n> -\t\t\t}\n> -\t\t\terr |= 1;\n> -\t\t}\n> +\t\t    (opts->flags & PARSE_OPT_OPTARG))\n> +\t\t\toptbug(opts, \"uses incompatible flags \"\n> +\t\t\t\t\"LASTARG_DEFAULT and OPTARG\");\n>  \t}\n> -\n> -\tif (err)\n> -\t\texit(129);\n\nHmph, this is simpler but it does not report all errors any more.\nSo it would be better to do:\n\n\tint err = 0;\n\n\tfor (; opts->type != OPTION_END; opts++) {\n\t\tif ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) &&\n\t\t    (opts->flags & PARSE_OPT_OPTARG))\n\t\t\terr |= optbug(opts, \"uses incompatible flags \"\n\t\t\t\t\t\t\"LASTARG_DEFAULT and OPTARG\");\n\t}\n\tif (err)\n\t\texit(128);\n\nSorry about that.\n"},{"id":"157084","messageId":"20101202060118.GB32125@burratino","threadId":"25917","inReplyTo":"20101202045707.GA32125@burratino","subject":"[PATCH 02/10 v2] parse-options: clearer reporting of API misuse","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-02T06:01:18Z","receivedAt":"2010-12-02T06:01:18Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The PARSE_OPT_LASTARG_DEFAULT flag is meant for options like\n--contains that (1) traditionally had a mandatory argument and\n(2) have some better behavior to use when appearing in the final\nposition.  It makes no sense to combine this with OPTARG, so ever\nsince v1.6.4-rc0~71 (parse-options: add parse_options_check to\nvalidate option specs, 2009-07-09) this mistake is flagged with\n\n\terror: `--option` uses incompatible flags LASTARG_DEFAULT and OPTARG\n\nand an exit status representing an error in commandline usage.\n\nUnfortunately that which might confuse scripters calling such an\nerroneous program into thinking the _script_ contains an error.\nClarify that it is an internal error by dying with a message beginning\n\"error: BUG: ...\" and status 128.\n\nWhile at it, clean up parse_options_check to prepare for more checks.\n\nLong term, it would be nicer to make such checks happen at compile\ntime.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nJonathan Nieder wrote:\n> Jonathan Nieder wrote:\n\n>> --- a/parse-options.c\n>> +++ b/parse-options.c\n>> @@ -316,24 +323,12 @@ static void check_typos(const char *arg, const struct option *options)\n[...]\n>> +\t\t    (opts->flags & PARSE_OPT_OPTARG))\n>> +\t\t\toptbug(opts, \"uses incompatible flags \"\n>> +\t\t\t\t\"LASTARG_DEFAULT and OPTARG\");\n>>  \t}\n>> -\n>> -\tif (err)\n>> -\t\texit(129);\n>\n> Hmph, this is simpler but it does not report all errors any more.\n> So it would be better to do:\n\nLike this.  (Looks okay now.  Famous last words...)\n\n parse-options.c |   23 +++++++++++------------\n 1 files changed, 11 insertions(+), 12 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 196ba71..97d7ff7 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -11,6 +11,13 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,\n #define OPT_SHORT 1\n #define OPT_UNSET 2\n \n+static int optbug(const struct option *opt, const char *reason)\n+{\n+\tif (opt->long_name)\n+\t\treturn error(\"BUG: option '%s' %s\", opt->long_name, reason);\n+\treturn error(\"BUG: switch '%c' %s\", opt->short_name, reason);\n+}\n+\n static int opterror(const struct option *opt, const char *reason, int flags)\n {\n \tif (flags & OPT_SHORT)\n@@ -320,20 +327,12 @@ static void parse_options_check(const struct option *opts)\n \n \tfor (; opts->type != OPTION_END; opts++) {\n \t\tif ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) &&\n-\t\t    (opts->flags & PARSE_OPT_OPTARG)) {\n-\t\t\tif (opts->long_name) {\n-\t\t\t\terror(\"`--%s` uses incompatible flags \"\n-\t\t\t\t      \"LASTARG_DEFAULT and OPTARG\", opts->long_name);\n-\t\t\t} else {\n-\t\t\t\terror(\"`-%c` uses incompatible flags \"\n-\t\t\t\t      \"LASTARG_DEFAULT and OPTARG\", opts->short_name);\n-\t\t\t}\n-\t\t\terr |= 1;\n-\t\t}\n+\t\t    (opts->flags & PARSE_OPT_OPTARG))\n+\t\t\terr |= optbug(opts, \"uses incompatible flags \"\n+\t\t\t\t\t\"LASTARG_DEFAULT and OPTARG\");\n \t}\n-\n \tif (err)\n-\t\texit(129);\n+\t\texit(128);\n }\n \n void parse_options_start(struct parse_opt_ctx_t *ctx,\n-- \n1.7.2.3\n"},{"id":"157082","messageId":"20101202060505.GC32125@burratino","threadId":"25917","inReplyTo":"20101201232950.GD31815@burratino","subject":"[PATCH 03/10 v2] parse-options: move NODASH sanity checks to parse_options_check","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-02T06:05:05Z","receivedAt":"2010-12-02T06:05:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"A dashless switch (like '(' passed to 'git grep') cannot be negated,\ncannot be attached to an argument, and cannot have a long form.\nCurrently parse-options runs the related sanity checks when the\ndashless option is used; better to always check them at the start of\noption parsing, so mistakes can be caught more quickly.\n\nThe error message at the new call site is less specific about the\nnature of the error, for simplicity.  On the other hand, it prints\nwhich switch was problematic.  Before:\n\n\tfatal: BUG: dashless options can't be long\n\nAfter:\n\n\terror: BUG: switch '(' uses feature not supported for dashless options\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nChange since v1:\n - rebase on top of patch 2 v2.\n\n parse-options.c |   14 +++++++-------\n 1 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 97d7ff7..9f6d305 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -288,13 +288,6 @@ static int parse_nodash_opt(struct parse_opt_ctx_t *p, const char *arg,\n \tfor (; options->type != OPTION_END; options++) {\n \t\tif (!(options->flags & PARSE_OPT_NODASH))\n \t\t\tcontinue;\n-\t\tif ((options->flags & PARSE_OPT_OPTARG) ||\n-\t\t    !(options->flags & PARSE_OPT_NOARG))\n-\t\t\tdie(\"BUG: dashless options don't support arguments\");\n-\t\tif (!(options->flags & PARSE_OPT_NONEG))\n-\t\t\tdie(\"BUG: dashless options don't support negation\");\n-\t\tif (options->long_name)\n-\t\t\tdie(\"BUG: dashless options can't be long\");\n \t\tif (options->short_name == arg[0] && arg[1] == '\\0')\n \t\t\treturn get_value(p, options, OPT_SHORT);\n \t}\n@@ -330,6 +323,13 @@ static void parse_options_check(const struct option *opts)\n \t\t    (opts->flags & PARSE_OPT_OPTARG))\n \t\t\terr |= optbug(opts, \"uses incompatible flags \"\n \t\t\t\t\t\"LASTARG_DEFAULT and OPTARG\");\n+\t\tif (opts->flags & PARSE_OPT_NODASH &&\n+\t\t    ((opts->flags & PARSE_OPT_OPTARG) ||\n+\t\t     !(opts->flags & PARSE_OPT_NOARG) ||\n+\t\t     !(opts->flags & PARSE_OPT_NONEG) ||\n+\t\t     opts->long_name))\n+\t\t\terr |= optbug(opts, \"uses feature \"\n+\t\t\t\t\t\"not supported for dashless options\");\n \t}\n \tif (err)\n \t\texit(128);\n-- \n1.7.2.3\n"},{"id":"157083","messageId":"20101202060857.GD32125@burratino","threadId":"25917","inReplyTo":"20101201233020.GE31815@burratino","subject":"[PATCH 04/10 v2] parse-options: sanity check PARSE_OPT_NOARG flag","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-02T06:08:57Z","receivedAt":"2010-12-02T06:08:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Some option types cannot use an argument --- boolean options that\nwould set a bit or flag or increment a counter, for example.  If\nconfigured in the flag word to accept an argument anyway, the result\nis an argument that is advertised in \"program -h\" output only to be\nrejected by parse-options::get_value.\n\nLuckily all current users of these option types use PARSE_OPT_NOARG\nand do not use PARSE_OPT_OPTARG.  Add a check to ensure that that\nremains true.  The check is run once for each invocation of\nparse_option_start().\n\nImproved-by: Stephen Boyd <bebarino@gmail.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nChanges since v1:\n - adapt for updated patch 2/10\n\n parse-options.c |   13 +++++++++++++\n 1 files changed, 13 insertions(+), 0 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 9f6d305..74ab0c8 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -330,6 +330,19 @@ static void parse_options_check(const struct option *opts)\n \t\t     opts->long_name))\n \t\t\terr |= optbug(opts, \"uses feature \"\n \t\t\t\t\t\"not supported for dashless options\");\n+\t\tswitch (opts->type) {\n+\t\tcase OPTION_BOOLEAN:\n+\t\tcase OPTION_BIT:\n+\t\tcase OPTION_NEGBIT:\n+\t\tcase OPTION_SET_INT:\n+\t\tcase OPTION_SET_PTR:\n+\t\tcase OPTION_NUMBER:\n+\t\t\tif ((opts->flags & PARSE_OPT_OPTARG) ||\n+\t\t\t    !(opts->flags & PARSE_OPT_NOARG))\n+\t\t\t\terr |= optbug(opts, \"should not accept an argument\");\n+\t\tdefault:\n+\t\t\t; /* ok. (usually accepts an argument) */\n+\t\t}\n \t}\n \tif (err)\n \t\texit(128);\n-- \n1.7.2.3\n"},{"id":"157085","messageId":"20101202061345.GE32125@burratino","threadId":"25917","inReplyTo":"20101201232923.GC31815@burratino","subject":"[PATCH 02/10 v2 resend] parse-options: clearer reporting of API misuse","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-02T06:13:45Z","receivedAt":"2010-12-02T06:13:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The PARSE_OPT_LASTARG_DEFAULT flag is meant for options like\n--contains that (1) traditionally had a mandatory argument and\n(2) have some better behavior to use when appearing in the final\nposition.  It makes no sense to combine this with OPTARG, so ever\nsince v1.6.4-rc0~71 (parse-options: add parse_options_check to\nvalidate option specs, 2009-07-09) this mistake is flagged with\n\n\terror: `--option` uses incompatible flags LASTARG_DEFAULT and OPTARG\n\nand an exit status representing an error in commandline usage.\n\nUnfortunately that which might confuse scripters calling such an\nerroneous program into thinking the _script_ contains an error.\nClarify that it is an internal error by dying with a message beginning\n\"error: BUG: ...\" and status 128.\n\nWhile at it, clean up parse_options_check to prepare for more checks.\n\nLong term, it would be nicer to make such checks happen at compile\ntime.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n[Resending because vger doesn't seem to have accepted the last copy.\nSorry for the patch flood.]\n\nJonathan Nieder wrote:\n> Jonathan Nieder wrote:\n\n>> --- a/parse-options.c\n>> +++ b/parse-options.c\n>> @@ -316,24 +323,12 @@ static void check_typos(const char *arg, const struct option *options)\n[...]\n>> +\t\t    (opts->flags & PARSE_OPT_OPTARG))\n>> +\t\t\toptbug(opts, \"uses incompatible flags \"\n>> +\t\t\t\t\"LASTARG_DEFAULT and OPTARG\");\n>>  \t}\n>> -\n>> -\tif (err)\n>> -\t\texit(129);\n>\n> Hmph, this is simpler but it does not report all errors any more.\n> So it would be better to do:\n\nLike this.  (Looks okay now.  Famous last words...)\n\n parse-options.c |   23 +++++++++++------------\n 1 files changed, 11 insertions(+), 12 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 196ba71..97d7ff7 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -11,6 +11,13 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,\n #define OPT_SHORT 1\n #define OPT_UNSET 2\n \n+static int optbug(const struct option *opt, const char *reason)\n+{\n+\tif (opt->long_name)\n+\t\treturn error(\"BUG: option '%s' %s\", opt->long_name, reason);\n+\treturn error(\"BUG: switch '%c' %s\", opt->short_name, reason);\n+}\n+\n static int opterror(const struct option *opt, const char *reason, int flags)\n {\n \tif (flags & OPT_SHORT)\n@@ -320,20 +327,12 @@ static void parse_options_check(const struct option *opts)\n \n \tfor (; opts->type != OPTION_END; opts++) {\n \t\tif ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) &&\n-\t\t    (opts->flags & PARSE_OPT_OPTARG)) {\n-\t\t\tif (opts->long_name) {\n-\t\t\t\terror(\"`--%s` uses incompatible flags \"\n-\t\t\t\t      \"LASTARG_DEFAULT and OPTARG\", opts->long_name);\n-\t\t\t} else {\n-\t\t\t\terror(\"`-%c` uses incompatible flags \"\n-\t\t\t\t      \"LASTARG_DEFAULT and OPTARG\", opts->short_name);\n-\t\t\t}\n-\t\t\terr |= 1;\n-\t\t}\n+\t\t    (opts->flags & PARSE_OPT_OPTARG))\n+\t\t\terr |= optbug(opts, \"uses incompatible flags \"\n+\t\t\t\t\t\"LASTARG_DEFAULT and OPTARG\");\n \t}\n-\n \tif (err)\n-\t\texit(129);\n+\t\texit(128);\n }\n \n void parse_options_start(struct parse_opt_ctx_t *ctx,\n-- \n1.7.2.3\n"},{"id":"157190","messageId":"4CF8B55F.8020203@gmail.com","threadId":"25917","inReplyTo":"20101201233136.GG31815@burratino","subject":"Re: [PATCH 06/10] parse-options: never suppress arghelp if LITERAL_ARGHELP is set","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2010-12-03T09:16:15Z","receivedAt":"2010-12-03T09:16:15Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"On 12/01/10 15:31, Jonathan Nieder wrote:\n> The PARSE_OPT_LITERAL_ARGHELP flag allows a program to override the\n> standard \"<argument> for mandatory, [argument] for optional\" markup in\n> its help message.  Extend it to override the usual \"no text for\n> disallowed\", too (for the PARSE_OPT_NOARG | PARSE_OPT_LITERAL_ARGHELP\n> case, which was previously meaningless), to be more intuitive.\n> \n> The motivation is to allow update-index to correctly advertise\n> \n> \t--cacheinfo <mode> <object> <path>\n> \t                      add the specified entry to the index\n> \n> while abusing PARSE_OPT_NOARG to disallow the \"sticked form\"\n> \n> \t--cacheinfo=<mode> <object> <path>\n> \n> Noticed-by: Stephen Boyd <bebarino@gmail.com>\n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n> ---\n> This seems like the intuitive thing to do, but the motivating use case\n> is iffy.  Might be better to introduce a PARSE_OPT_NOSTICKED flag.\n\nparse-options should accept both forms of --cacheinfo above if the\noption isn't marked PARSE_OPT_NOARG. Marking it NOARG to get rid of the\nequals sign in the usage seems wrong when both the equals sign and no\nequals sign can be accepted. The fault lies in the implementation of the\nlow-level callback. It needs to handle more of the parsing.\n\nIf --cacheinfo is marked PARSE_OPT_NOARG it should show\n\n\t--cacheinfo\n\nIf --cacheinfo is marked PARSE_OPT_OPTARG it should show\n\n\t--cacheinfo[=<argh>]\n\nIf --cacheinfo isn't marked either of the above it should show\n\n\t--cacheinfo <argh>\n\nwhich looks like what you want and it also says it takes an argument\nunconditionally (perfect?). On top of that, the equals sign is optional\n(or at least should be). At the point of get_value() p->opt will contain\nwhatever is after the equals sign for the option that is parsed. For\noptions which take no value (NOARG) they'll fail the test if they have\nanything with an equals sign after it.\n\nBy not marking the cacheinfo option NOARG, we've successfully gotten\nwhat we want so far. Now we just need to make the cacheinfo callback a\nbit more like get_arg() and have it look at the context argument. If\np->opt is set, we should pull <mode> out of that and make sure we have\ntwo more arguments. If p->opt isn't set, we should make sure we have 3\narguments and basically do what is already there.\n\nI know this is a bit more code, but it also means that we don't have\nthis approach bite someone else down the line when they make assumptions\nabout options marked as NOARG not taking arguments.\n\nWe should probably add another check like \"if argh is set and\nPARSE_OPT_NOARG is true error out\" so this can't be done.\n"},{"id":"157191","messageId":"20101203094020.GA11910@burratino","threadId":"25917","inReplyTo":"4CF8B55F.8020203@gmail.com","subject":"Re: [PATCH 06/10] parse-options: never suppress arghelp if LITERAL_ARGHELP is set","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-03T09:40:20Z","receivedAt":"2010-12-03T09:40:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stephen Boyd wrote:\n> On 12/01/10 15:31, Jonathan Nieder wrote:\n\n>> The motivation is to allow update-index to correctly advertise\n>> \n>> \t--cacheinfo <mode> <object> <path>\n>> \t                      add the specified entry to the index\n>> \n>> while abusing PARSE_OPT_NOARG to disallow the \"sticked form\"\n>> \n>> \t--cacheinfo=<mode> <object> <path>\n[...]\n> parse-options should accept both forms of --cacheinfo above if the\n> option isn't marked PARSE_OPT_NOARG. Marking it NOARG to get rid of the\n> equals sign in the usage seems wrong when both the equals sign and no\n> equals sign can be accepted.\n\nJust to clarify: the NOARG was not meant to affect the usage message\nbut the actual accepted usage.  The idea was that\n\n\tgit update-index --cacheinfo=100644 87a8767c87b file.c\n\nshould be rejected, because if it is accepted that would tempt people\nto try\n\n\tgit update-index --cacheinfo=100644 -q 87a8767c87b file.c\n\nwhich fails.  That is, the argument to --cacheinfo is not <mode>,\nsince --cacheinfo takes _three_ arguments and therefore the sticked\nform sends a wrong message.\n\n> I know this is a bit more code, but it also means that we don't have\n> this approach bite someone else down the line when they make assumptions\n> about options marked as NOARG not taking arguments.\n>\n> We should probably add another check like \"if argh is set and\n> PARSE_OPT_NOARG is true error out\" so this can't be done.\n\nI agree with your conclusion.  Let's drop this patch, and I'll look\ninto adding the check and a PARSE_OPT_NOSTICKED flag.\n"},{"id":"157230","messageId":"4CF92EA6.8060206@gmail.com","threadId":"25917","inReplyTo":"20101203094020.GA11910@burratino","subject":"Re: [PATCH 06/10] parse-options: never suppress arghelp if LITERAL_ARGHELP is set","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2010-12-03T17:53:42Z","receivedAt":"2010-12-03T17:53:42Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"On 12/03/10 01:40, Jonathan Nieder wrote:\n> \n> Just to clarify: the NOARG was not meant to affect the usage message\n> but the actual accepted usage.  The idea was that\n> \n> \tgit update-index --cacheinfo=100644 87a8767c87b file.c\n> \n> should be rejected, because if it is accepted that would tempt people\n> to try\n> \n> \tgit update-index --cacheinfo=100644 -q 87a8767c87b file.c\n> \n> which fails.  That is, the argument to --cacheinfo is not <mode>,\n> since --cacheinfo takes _three_ arguments and therefore the sticked\n> form sends a wrong message.\n> \n\nSorry, I don't quite understand why we should reject the sticked form\nwhen we don't advertise its usage anywhere (man pages or usageh). Maybe\nI'm just not thinking right since I'm' optimistic people won't do that\n-q thing.\n\nBut if you really want to do it we don't really need to add a new flag\nright? We can just die in the cacheinfo callback if the context's opt\npointer is set. If this ever becomes a common thing we can add the flag\nlater. Putting a comment like \"if only we had PARSE_OPT_NOSTICKED...\"\nwould be fine too.\n\nI'll be fine with or without the flag though.\n"},{"id":"157363","messageId":"4CFBD693.20305@lsrfire.ath.cx","threadId":"25917","inReplyTo":"20101201232852.GB31815@burratino","subject":"Re: [PATCH 01/10] parse-options: Don't call parse_options_check() so much","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-12-05T18:14:43Z","receivedAt":"2010-12-05T18:14:43Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 02.12.2010 00:28, schrieb Jonathan Nieder:\n> From: Stephen Boyd <bebarino@gmail.com>\n> \n> parse_options_check() is being called for each invocation of\n> parse_options_step() which can be quite a bit for some commands. The\n> commit introducing this function cb9d398 (parse-options: add\n> parse_options_check to validate option specs., 2009-06-09) had the\n> correct motivation and explicitly states that parse_options_check()\n> should be called from parse_options_start(). However, the implementation\n> differs from the motivation. Fix it.\n\nGood idea.\n\n>  void parse_options_start(struct parse_opt_ctx_t *ctx,\n>  \t\t\t int argc, const char **argv, const char *prefix,\n> -\t\t\t int flags)\n> +\t\t\t int flags, const struct option *options)\n\nIt might be better to put options before flags, i.e. to use the same\norder as in parse_options().\n\nRené\n"},{"id":"157389","messageId":"4CFC9776.7040000@gmail.com","threadId":"25917","inReplyTo":"4CFBD693.20305@lsrfire.ath.cx","subject":"Re: [PATCH 01/10] parse-options: Don't call parse_options_check() so much","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2010-12-06T07:57:42Z","receivedAt":"2010-12-06T07:57:42Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"On 12/05/10 10:14, René Scharfe wrote:\n> Am 02.12.2010 00:28, schrieb Jonathan Nieder:\n>> From: Stephen Boyd <bebarino@gmail.com>\n>>  void parse_options_start(struct parse_opt_ctx_t *ctx,\n>>  \t\t\t int argc, const char **argv, const char *prefix,\n>> -\t\t\t int flags)\n>> +\t\t\t int flags, const struct option *options)\n> \n> It might be better to put options before flags, i.e. to use the same\n> order as in parse_options().\n> \n\nJonathan mentioned that too. Sounds good to me. Here's an updated patch.\n\n--->8----8<----\nSubject: [PATCH] parse-options: Don't call parse_options_check() so much\n\nparse_options_check() is being called for each invocation of\nparse_options_step which can be quite a bit for some commands. The\ncommit introducing this function cb9d398 (parse-options: add\nparse_options_check to validate option specs., 2009-06-09) had the\ncorrect motivation and explicitly states that parse_options_check()\nshould be called from parse_options_start(). However, the implementation\ndiffers from the motivation. Fix it.\n\nSigned-off-by: Stephen Boyd <bebarino@gmail.com>\n---\n builtin/blame.c    |    4 ++--\n builtin/shortlog.c |    4 ++--\n parse-options.c    |    7 +++----\n parse-options.h    |    2 +-\n 4 files changed, 8 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex cb25ec9..aa30ec5 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2325,8 +2325,8 @@ int cmd_blame(int argc, const char **argv, const\nchar *prefix)\n \tsave_commit_buffer = 0;\n \tdashdash_pos = 0;\n\n-\tparse_options_start(&ctx, argc, argv, prefix, PARSE_OPT_KEEP_DASHDASH |\n-\t\t\t    PARSE_OPT_KEEP_ARGV0);\n+\tparse_options_start(&ctx, argc, argv, prefix, options,\n+\t\t\t    PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0);\n \tfor (;;) {\n \t\tswitch (parse_options_step(&ctx, options, blame_opt_usage)) {\n \t\tcase PARSE_OPT_HELP:\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 2135b0d..1a21e4b 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -268,8 +268,8 @@ int cmd_shortlog(int argc, const char **argv, const\nchar *prefix)\n \tgit_config(git_default_config, NULL);\n \tshortlog_init(&log);\n \tinit_revisions(&rev, prefix);\n-\tparse_options_start(&ctx, argc, argv, prefix, PARSE_OPT_KEEP_DASHDASH |\n-\t\t\t    PARSE_OPT_KEEP_ARGV0);\n+\tparse_options_start(&ctx, argc, argv, prefix, options,\n+\t\t\t    PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0);\n\n \tfor (;;) {\n \t\tswitch (parse_options_step(&ctx, options, shortlog_usage)) {\ndiff --git a/parse-options.c b/parse-options.c\nindex 0fa79bc..9bbbc6a 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -338,7 +338,7 @@ static void parse_options_check(const struct option\n*opts)\n\n void parse_options_start(struct parse_opt_ctx_t *ctx,\n \t\t\t int argc, const char **argv, const char *prefix,\n-\t\t\t int flags)\n+\t\t\t const struct option *options, int flags)\n {\n \tmemset(ctx, 0, sizeof(*ctx));\n \tctx->argc = argc - 1;\n@@ -350,6 +350,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,\n \tif ((flags & PARSE_OPT_KEEP_UNKNOWN) &&\n \t    (flags & PARSE_OPT_STOP_AT_NON_OPTION))\n \t\tdie(\"STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together\");\n+\tparse_options_check(options);\n }\n\n static int usage_with_options_internal(struct parse_opt_ctx_t *,\n@@ -362,8 +363,6 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n {\n \tint internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);\n\n-\tparse_options_check(options);\n-\n \t/* we must reset ->opt, unknown short option leave it dangling */\n \tctx->opt = NULL;\n\n@@ -452,7 +451,7 @@ int parse_options(int argc, const char **argv, const\nchar *prefix,\n {\n \tstruct parse_opt_ctx_t ctx;\n\n-\tparse_options_start(&ctx, argc, argv, prefix, flags);\n+\tparse_options_start(&ctx, argc, argv, prefix, options, flags);\n \tswitch (parse_options_step(&ctx, options, usagestr)) {\n \tcase PARSE_OPT_HELP:\n \t\texit(129);\ndiff --git a/parse-options.h b/parse-options.h\nindex ae8647d..7b53414 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -180,7 +180,7 @@ struct parse_opt_ctx_t {\n\n extern void parse_options_start(struct parse_opt_ctx_t *ctx,\n \t\t\t\tint argc, const char **argv, const char *prefix,\n-\t\t\t\tint flags);\n+\t\t\t\tconst struct option *options, int flags);\n\n extern int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\t      const struct option *options,\n-- \n"}]}