{"thread":{"id":"49759","subject":"[PATCH 0/13] parseopt fixes from -Wunused-parameters","startedAt":"2018-11-05T06:37:35Z","lastAt":"2018-11-05T18:52:02Z","messageCount":21,"participants":["Jeff King","Junio C Hamano","Duy Nguyen","Carlo Marcelo Arenas Belón"],"isPatch":true,"patchVersion":1,"patchTotal":13},"messages":[{"id":"362457","messageId":"20181105063718.GA24877@sigill.intra.peff.net","threadId":"49759","inReplyTo":null,"subject":"[PATCH 0/13] parseopt fixes from -Wunused-parameters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-05T06:37:18Z","receivedAt":"2018-11-05T06:37:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Continuing my exploration of what -Wunused-parameters can show us, here\nare some bug-fixes related to parse-options callbacks.\n\nThis is the last of the actual bug-fixes I've found. After this, I have\nabout 60 patches worth of cleanups (i.e., dropping the unused\nparameters), and then I have a series to annotate parameters that must\nbe unused (e.g., for functions that must conform to callback\ninterfaces). After we can start compiling with -Wunused-parameters,\nassuming we don't find the annotations too cumbersome.\n\nBut this series fixes real bugs. These first four fix segfaults:\n\n [01/13]: apply: mark include/exclude options as NONEG\n [02/13]: am: handle --no-patch-format option\n [03/13]: ls-files: mark exclude options as NONEG\n [04/13]: pack-objects: mark index-version option as NONEG\n\nAnd these four fix cases where we just quietly do the wrong thing:\n\n [05/13]: cat-file: mark batch options with NONEG\n [06/13]: status: mark --find-renames option with NONEG\n [07/13]: format-patch: mark \"--no-numbered\" option with NONEG\n [08/13]: show-branch: mark --reflog option as NONEG\n\nThese ones are just message improvements:\n\n [09/13]: tag: mark \"--message\" option with NONEG\n [10/13]: cat-file: report an error on multiple --batch options\n [11/13]: apply: return -1 from option callback instead of calling exit(1)\n\nThis one is a segfault, but it has no callers. ;)\n\n [12/13]: parse-options: drop OPT_DATE()\n\nAnd then this last one is mostly about annotating the callbacks. It\ndoesn't strictly need to happen here, but the alternative is that I'd\neventually have to deal with it in the later series I mentioned.\n\n [13/13]: assert NOARG/NONEG behavior of parse-options callbacks\n\n apply.c | 24 +++++++++++++++++++++---\n builtin/am.c | 4 +++-\n builtin/blame.c | 4 ++++\n builtin/cat-file.c | 10 +++++++---\n builtin/checkout-index.c | 2 ++\n builtin/clean.c | 1 +\n builtin/commit.c | 5 ++++-\n builtin/fetch.c | 2 ++\n builtin/grep.c | 14 +++++++++++++-\n builtin/init-db.c | 1 +\n builtin/interpret-trailers.c | 2 ++\n builtin/log.c | 12 +++++++++++-\n builtin/ls-files.c | 14 +++++++++++---\n builtin/merge-file.c | 2 ++\n builtin/merge.c | 1 +\n builtin/notes.c | 7 +++++++\n builtin/pack-objects.c | 5 ++++-\n builtin/read-tree.c | 3 +++\n builtin/rebase.c | 6 ++++++\n builtin/show-branch.c | 3 ++-\n builtin/show-ref.c | 1 +\n builtin/tag.c | 6 ++++--\n builtin/update-index.c | 21 +++++++++++++++++++--\n parse-options-cb.c | 14 +++++++-------\n parse-options.h | 18 ++++++++++++++----\n ref-filter.c | 2 ++\n t/helper/test-parse-options.c | 2 +-\n t/t0040-parse-options.sh | 22 ----------------------\n 28 files changed, 155 insertions(+), 53 deletions(-)\n\n-Peff\n"},{"id":"362458","messageId":"20181105063819.GA25864@sigill.intra.peff.net","threadId":"49759","inReplyTo":"20181105063718.GA24877@sigill.intra.peff.net","subject":"[PATCH 01/13] apply: mark include/exclude options as NONEG","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-05T06:38:19Z","receivedAt":"2018-11-05T06:38:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The options callback for \"git apply --no-include\" is not ready to handle\nthe \"unset\" parameter, and as a result will segfault when it adds a NULL\nargument to the include list (likewise for \"--no-exclude\").\n\nIn theory this might be used to clear the list, but since both\n\"--include\" and \"--exclude\" add to the same list, it's not immediately\nobvious what the semantics should be. Let's punt on that for now and\njust disallow the broken options.\n\nSigned-off-by: Jeff King \n---\n apply.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 073d5f0451..d1ca6addeb 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4939,10 +4939,10 @@ int apply_parse_options(int argc, const char **argv,\n \tstruct option builtin_apply_options[] = {\n \t\t{ OPTION_CALLBACK, 0, \"exclude\", state, N_(\"path\"),\n \t\t\tN_(\"don't apply changes matching the given path\"),\n-\t\t\t0, apply_option_parse_exclude },\n+\t\t\tPARSE_OPT_NONEG, apply_option_parse_exclude },\n \t\t{ OPTION_CALLBACK, 0, \"include\", state, N_(\"path\"),\n \t\t\tN_(\"apply changes matching the given path\"),\n-\t\t\t0, apply_option_parse_include },\n+\t\t\tPARSE_OPT_NONEG, apply_option_parse_include },\n \t\t{ OPTION_CALLBACK, 'p', NULL, state, N_(\"num\"),\n \t\t\tN_(\"remove leading slashes from traditional diff paths\"),\n \t\t\t0, apply_option_parse_p },\n-- \n2.19.1.1505.g9cd28186cf\n\n"},{"id":"362459","messageId":"20181105063839.GB25864@sigill.intra.peff.net","threadId":"49759","inReplyTo":"20181105063718.GA24877@sigill.intra.peff.net","subject":"[PATCH 02/13] am: handle --no-patch-format option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-05T06:38:39Z","receivedAt":"2018-11-05T06:38:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Running \"git am --no-patch-format\" will currently segfault, since it\ntries to parse a NULL argument. Instead, let's have it cancel any\nprevious --patch-format option.\n\nSigned-off-by: Jeff King \n---\n builtin/am.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 3ee9a9d2a9..dcb880b699 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2165,7 +2165,9 @@ static int parse_opt_patchformat(const struct option *opt, const char *arg, int\n {\n \tint *opt_value = opt->value;\n \n-\tif (!strcmp(arg, \"mbox\"))\n+\tif (unset)\n+\t\t*opt_value = PATCH_FORMAT_UNKNOWN;\n+\telse if (!strcmp(arg, \"mbox\"))\n \t\t*opt_value = PATCH_FORMAT_MBOX;\n \telse if (!strcmp(arg, \"stgit\"))\n \t\t*opt_value = PATCH_FORMAT_STGIT;\n-- \n2.19.1.1505.g9cd28186cf\n\n"},{"id":"362460","messageId":"20181105063919.GC25864@sigill.intra.peff.net","threadId":"49759","inReplyTo":"20181105063718.GA24877@sigill.intra.peff.net","subject":"[PATCH 03/13] ls-files: mark exclude options as NONEG","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-05T06:39:20Z","receivedAt":"2018-11-05T06:39:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Running \"git ls-files --no-exclude\" will currently segfault, as its\noption callback does not handle the \"unset\" parameter.\n\nIn theory this could be used to clear the exclude list, but it is not\nclear how that would interact with the other exclude options, nor is the\ncurrent code capable of clearing the list. Let's just disable the broken\noption.\n\nNote that --no-exclude-from will similarly segfault, but\n--no-exclude-standard will not. It just silently does the wrong thing\n(pretending as if --exclude-standard was specified).\n\nSigned-off-by: Jeff King \n---\n builtin/ls-files.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 7f9919a362..2787453eb1 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -548,15 +548,16 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t\t\t N_(\"show resolve-undo information\")),\n \t\t{ OPTION_CALLBACK, 'x', \"exclude\", &exclude_list, N_(\"pattern\"),\n \t\t\tN_(\"skip files matching pattern\"),\n-\t\t\t0, option_parse_exclude },\n+\t\t\tPARSE_OPT_NONEG, option_parse_exclude },\n \t\t{ OPTION_CALLBACK, 'X', \"exclude-from\", &dir, N_(\"file\"),\n \t\t\tN_(\"exclude patterns are read from \"),\n-\t\t\t0, option_parse_exclude_from },\n+\t\t\tPARSE_OPT_NONEG, option_parse_exclude_from },\n \t\tOPT_STRING(0, \"exclude-per-directory\", &dir.exclude_per_dir, N_(\"file\"),\n \t\t\tN_(\"read additional per-directory exclude patterns in \")),\n \t\t{ OPTION_CALLBACK, 0, \"exclude-standard\", &dir, NULL,\n \t\t\tN_(\"add the standard git exclusions\"),\n-\t\t\tPARSE_OPT_NOARG, option_parse_exclude_standard },\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG,\n+\t\t\toption_parse_exclude_standard },\n \t\tOPT_SET_INT_F(0, \"full-name\", &prefix_len,\n \t\t\t N_(\"make the output relative to the project top directory\"),\n \t\t\t 0, PARSE_OPT_NONEG),\n-- \n2.19.1.1505.g9cd28186cf\n\n"},{"id":"362461","messageId":"20181105063938.GD25864@sigill.intra.peff.net","threadId":"49759","inReplyTo":"20181105063718.GA24877@sigill.intra.peff.net","subject":"[PATCH 04/13] pack-objects: mark index-version option as NONEG","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-05T06:39:38Z","receivedAt":"2018-11-05T06:39:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Running \"git pack-objects --no-index-version\" will segfault, since the\ncallback is not prepared to handle the \"unset\" flag.\n\nIn theory this might be used to counteract an earlier \"--index-version\",\nor override a pack.indexversion config setting. But the semantics aren't\nimmediately obvious, and it's unlikely anybody wants this. Let's just\ndisable the broken option for now.\n\nSigned-off-by: Jeff King \n---\n builtin/pack-objects.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex e50c6cd1ff..4eac2f997b 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3252,7 +3252,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"similar to --all-progress when progress meter is shown\")),\n \t\t{ OPTION_CALLBACK, 0, \"index-version\", NULL, N_(\"[,]\"),\n \t\t N_(\"write the pack index file in the specified idx format version\"),\n-\t\t 0, option_parse_index_version },\n+\t\t PARSE_OPT_NONEG, option_parse_index_version },\n \t\tOPT_MAGNITUDE(0, \"max-pack-size\", &pack_size_limit,\n \t\t\t N_(\"maximum size of each output pack file\")),\n \t\tOPT_BOOL(0, \"local\", &local,\n-- \n2.19.1.1505.g9cd28186cf\n\n"},{"id":"362462","messageId":"20181105064010.GE25864@sigill.intra.peff.net","threadId":"49759","inReplyTo":"20181105063718.GA24877@sigill.intra.peff.net","subject":"[PATCH 05/13] cat-file: mark batch options with NONEG","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-05T06:40:10Z","receivedAt":"2018-11-05T06:40:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Running \"cat-file --no-batch\" will behave as if \"--batch\" was given,\nsince the option callback does not handle the \"unset\" flag (likewise for\n\"--no-batch-check\").\n\nIn theory this might be used to cancel an earlier --batch, but it's not\nimmediately obvious how that would interact with --batch-check. Let's\njust disallow the negated form of both options.\n\nSigned-off-by: Jeff King \n---\n builtin/cat-file.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 8d97c84725..4a5289079c 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -631,10 +631,12 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"buffer\", &batch.buffer_output, N_(\"buffer --batch output\")),\n \t\t{ OPTION_CALLBACK, 0, \"batch\", &batch, \"format\",\n \t\t\tN_(\"show info and content of objects fed from the standard input\"),\n-\t\t\tPARSE_OPT_OPTARG, batch_option_callback },\n+\t\t\tPARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n+\t\t\tbatch_option_callback },\n \t\t{ OPTION_CALLBACK, 0, \"batch-check\", &batch, \"format\",\n \t\t\tN_(\"show info about objects fed from the standard input\"),\n-\t\t\tPARSE_OPT_OPTARG, batch_option_callback },\n+\t\t\tPARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n+\t\t\tbatch_option_callback },\n \t\tOPT_BOOL(0, \"follow-symlinks\", &batch.follow_symlinks,\n \t\t\t N_(\"follow in-tree symlinks (used with --batch or --batch-check)\")),\n \t\tOPT_BOOL(0, \"batch-all-objects\", &batch.all_objects,\n-- \n2.19.1.1505.g9cd28186cf\n\n"},{"id":"362463","messageId":"20181105064053.GF25864@sigill.intra.peff.net","threadId":"49759","inReplyTo":"20181105063718.GA24877@sigill.intra.peff.net","subject":"[PATCH 06/13] status: mark --find-renames option with NONEG","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-05T06:40:53Z","receivedAt":"2018-11-05T06:40:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If you run \"git status --no-find-renames\", it will behave the same as\n\"--find-renames\", because we ignore the \"unset\" parameter (we see a NULL\n\"arg\", but since the score argument is optional, we just think that the\nuser did not provide a score).\n\nWe already have a separate \"--no-renames\" to disable renames, so there's\nnot much point in supporting \"--no-find-renames\". Let's just flag it as\nan error.\n\nSigned-off-by: Jeff King \n---\n builtin/commit.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 074bd9a551..4d7754ca43 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1335,7 +1335,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"no-renames\", &no_renames, N_(\"do not detect renames\")),\n \t\t{ OPTION_CALLBACK, 'M', \"find-renames\", &rename_score_arg,\n \t\t N_(\"n\"), N_(\"detect renames, optionally set similarity index\"),\n-\t\t PARSE_OPT_OPTARG, opt_parse_rename_score },\n+\t\t PARSE_OPT_OPTARG | PARSE_OPT_NONEG, opt_parse_rename_score },\n \t\tOPT_END(),\n \t};\n \n-- \n2.19.1.1505.g9cd28186cf\n\n"},{"id":"362464","messageId":"20181105064112.GG25864@sigill.intra.peff.net","threadId":"49759","inReplyTo":"20181105063718.GA24877@sigill.intra.peff.net","subject":"[PATCH 07/13] format-patch: mark \"--no-numbered\" option with NONEG","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-05T06:41:12Z","receivedAt":"2018-11-05T06:41:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have separate parse-options entries for \"numbered\" and \"no-numbered\",\nwhich means that we accept \"--no-no-numbered\". It does not behave\nsensibly, though (it ignores the \"unset\" flag and acts like\n\"--no-numbered\").\n\nWe could fix that, but obviously this is silly and unintentional. Let's\njust disallow it.\n\nSigned-off-by: Jeff King \n---\n builtin/log.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 061d4fd864..41188e723c 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1508,7 +1508,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\t PARSE_OPT_NOARG, numbered_callback },\n \t\t{ OPTION_CALLBACK, 'N', \"no-numbered\", &numbered, NULL,\n \t\t\t N_(\"use [PATCH] even with multiple patches\"),\n-\t\t\t PARSE_OPT_NOARG, no_numbered_callback },\n+\t\t\t PARSE_OPT_NOARG | PARSE_OPT_NONEG, no_numbered_callback },\n \t\tOPT_BOOL('s', \"signoff\", &do_signoff, N_(\"add Signed-off-by:\")),\n \t\tOPT_BOOL(0, \"stdout\", &use_stdout,\n \t\t\t N_(\"print patches to standard out\")),\n-- \n2.19.1.1505.g9cd28186cf\n\n"},{"id":"362465","messageId":"20181105064240.GH25864@sigill.intra.peff.net","threadId":"49759","inReplyTo":"20181105063718.GA24877@sigill.intra.peff.net","subject":"[PATCH 08/13] show-branch: mark --reflog option as NONEG","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-05T06:42:40Z","receivedAt":"2018-11-05T06:42:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Running \"git show-branch --no-reflog\" will behave as if \"--reflog\" was\ngiven with no options, which makes no sense.\n\nIn theory this option might be used to cancel an earlier \"--reflog\"\noption, but the semantics are not clear. Let's punt on it and just\ndisallow the broken option.\n\nSigned-off-by: Jeff King \n---\nOn most of these I tried to decide whether there was a sensible\nbehavior, and actually do the fix. But I have to admit I have no idea\nhow \"show-branch -g\" is supposed to work, which is the main reason I\npunted here. If somebody cares enough to write a patch, they can\nsupersede this (but we should do something, because the current behavior\nis nonsense).\n\n builtin/show-branch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 65f4a4c83c..b1b540f7f6 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -674,7 +674,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t{ OPTION_CALLBACK, 'g', \"reflog\", &reflog_base, N_(\"[,]\"),\n \t\t\t N_(\"show most recent ref-log entries starting at \"\n \t\t\t \"base\"),\n-\t\t\t PARSE_OPT_OPTARG,\n+\t\t\t PARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n \t\t\t parse_reflog_param },\n \t\tOPT_END()\n \t};\n-- \n2.19.1.1505.g9cd28186cf\n\n"},{"id":"362466","messageId":"20181105064312.GI25864@sigill.intra.peff.net","threadId":"49759","inReplyTo":"20181105063718.GA24877@sigill.intra.peff.net","subject":"[PATCH 09/13] tag: mark \"--message\" option with NONEG","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-05T06:43:12Z","receivedAt":"2018-11-05T06:43:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We do not allow \"--no-message\" to work now, as the option callback\nreturns \"-1\" when it sees a NULL arg. However, that will cause\nparse-options to exit(129) without printing anything further, leaving\nthe user confused about what happened.\n\nInstead, let's explicitly mark it as PARSE_OPT_NONEG, which will give a\nuseful error message (and print the usual -h output).\n\nIn theory this could be used to override an earlier \"-m\", but it's not\nclear how it would interact with other message options (e.g., would it\nalso clear data read for \"-F\"?). Since it's already disabled and nobody\nis asking for it, let's punt on that and just improve the error message.\n\nSigned-off-by: Jeff King \n---\n builtin/tag.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex f623632186..6a396a5090 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -390,8 +390,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tOPT_GROUP(N_(\"Tag creation options\")),\n \t\tOPT_BOOL('a', \"annotate\", &annotate,\n \t\t\t\t\tN_(\"annotated tag, needs a message\")),\n-\t\tOPT_CALLBACK('m', \"message\", &msg, N_(\"message\"),\n-\t\t\t N_(\"tag message\"), parse_msg_arg),\n+\t\t{ OPTION_CALLBACK, 'm', \"message\", &msg, N_(\"message\"),\n+\t\t N_(\"tag message\"), PARSE_OPT_NONEG, parse_msg_arg },\n \t\tOPT_FILENAME('F', \"file\", &msgfile, N_(\"read message from file\")),\n \t\tOPT_BOOL('e', \"edit\", &edit_flag, N_(\"force edit of tag message\")),\n \t\tOPT_BOOL('s', \"sign\", &opt.sign, N_(\"annotated and GPG-signed tag\")),\n-- \n2.19.1.1505.g9cd28186cf\n\n"},{"id":"362467","messageId":"20181105064344.GJ25864@sigill.intra.peff.net","threadId":"49759","inReplyTo":"20181105063718.GA24877@sigill.intra.peff.net","subject":"[PATCH 10/13] cat-file: report an error on multiple --batch options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-05T06:43:44Z","receivedAt":"2018-11-05T06:43:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The options callback for --batch and --batch-check detects when the two\nmutually incompatible options are used. But it simply returns an error\ncode to parse-options, meaning the program will quit without any kind of\nmessage to the user.\n\nInstead, let's use error() to print something and return -1. Note that\nthis flips the error return from 1 to -1, but negative values are more\nidiomatic here (and parse-options treats them the same).\n\nSigned-off-by: Jeff King \n---\n builtin/cat-file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 4a5289079c..0f6b692df6 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -596,7 +596,7 @@ static int batch_option_callback(const struct option *opt,\n \tstruct batch_options *bo = opt->value;\n \n \tif (bo->enabled) {\n-\t\treturn 1;\n+\t\treturn error(_(\"only one batch option may be specified\"));\n \t}\n \n \tbo->enabled = 1;\n-- \n2.19.1.1505.g9cd28186cf\n\n"},{"id":"362468","messageId":"20181105064358.GK25864@sigill.intra.peff.net","threadId":"49759","inReplyTo":"20181105063718.GA24877@sigill.intra.peff.net","subject":"[PATCH 11/13] apply: return -1 from option callback instead of calling exit(1)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-05T06:43:59Z","receivedAt":"2018-11-05T06:44:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The option callback for \"apply --whitespace\" exits with status \"1\" on\nerror. It makes more sense for it to just return an error to\nparse-options. That code will exit, too, but it will use status \"129\"\nthat is customary for option errors.\n\nThe exit() dates back to aaf6c447aa (builtin/apply: make\nparse_whitespace_option() return -1 instead of die()ing, 2016-08-08).\nThat commit gives no reason why we'd prefer the current exit status (it\nlooks like it was just bumping the \"die\" up a level in the callstack,\nbut did not go as far as it could have).\n\nSigned-off-by: Jeff King \n---\n apply.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex d1ca6addeb..c4c9f849b1 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4812,7 +4812,7 @@ static int apply_option_parse_whitespace(const struct option *opt,\n \tstruct apply_state *state = opt->value;\n \tstate->whitespace_option = arg;\n \tif (parse_whitespace_option(state, arg))\n-\t\texit(1);\n+\t\treturn -1;\n \treturn 0;\n }\n \n-- \n2.19.1.1505.g9cd28186cf\n\n"},{"id":"362469","messageId":"20181105064427.GL25864@sigill.intra.peff.net","threadId":"49759","inReplyTo":"20181105063718.GA24877@sigill.intra.peff.net","subject":"[PATCH 12/13] parse-options: drop OPT_DATE()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-05T06:44:27Z","receivedAt":"2018-11-05T06:44:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are no users of OPT_DATE except for test-parse-options; its\nonly caller went away in 27ec394a97 (prune: introduce OPT_EXPIRY_DATE()\nand use it, 2013-04-25).\n\nIt also has a bug: it does not specify PARSE_OPT_NONEG, but its callback\ndoes not respect the \"unset\" flag, and will feed NULL to approxidate()\nand segfault. Probably this should be marked with NONEG, or the callback\nshould set the timestamp to some sentinel value (e.g,. \"0\", or\n\"(time_t)-1\").\n\nBut since there are no callers, deleting it means we don't even have to\nthink about what the right behavior should be.\n\nSigned-off-by: Jeff King \n---\n parse-options-cb.c | 7 -------\n parse-options.h | 4 ----\n t/helper/test-parse-options.c | 1 -\n t/t0040-parse-options.sh | 22 ----------------------\n 4 files changed, 34 deletions(-)\n\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex e8236534ac..6a61166b26 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -28,13 +28,6 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n-int parse_opt_approxidate_cb(const struct option *opt, const char *arg,\n-\t\t\t int unset)\n-{\n-\t*(timestamp_t *)(opt->value) = approxidate(arg);\n-\treturn 0;\n-}\n-\n int parse_opt_expiry_date_cb(const struct option *opt, const char *arg,\n \t\t\t int unset)\n {\ndiff --git a/parse-options.h b/parse-options.h\nindex dd14911a29..c3f2d2eceb 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -150,9 +150,6 @@ struct option {\n \t\t\t\t (h), 0, &parse_opt_string_list }\n #define OPT_UYN(s, l, v, h) { OPTION_CALLBACK, (s), (l), (v), NULL, \\\n \t\t\t\t (h), PARSE_OPT_NOARG, &parse_opt_tertiary }\n-#define OPT_DATE(s, l, v, h) \\\n-\t{ OPTION_CALLBACK, (s), (l), (v), N_(\"time\"),(h), 0,\t\\\n-\t parse_opt_approxidate_cb }\n #define OPT_EXPIRY_DATE(s, l, v, h) \\\n \t{ OPTION_CALLBACK, (s), (l), (v), N_(\"expiry-date\"),(h), 0,\t\\\n \t parse_opt_expiry_date_cb }\n@@ -232,7 +229,6 @@ extern struct option *parse_options_concat(struct option *a, struct option *b);\n \n /*----- some often used options -----*/\n extern int parse_opt_abbrev_cb(const struct option *, const char *, int);\n-extern int parse_opt_approxidate_cb(const struct option *, const char *, int);\n extern int parse_opt_expiry_date_cb(const struct option *, const char *, int);\n extern int parse_opt_color_flag_cb(const struct option *, const char *, int);\n extern int parse_opt_verbosity_cb(const struct option *, const char *, int);\ndiff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c\nindex 9cb8a0ea0f..f0623bb42b 100644\n--- a/t/helper/test-parse-options.c\n+++ b/t/helper/test-parse-options.c\n@@ -119,7 +119,6 @@ int cmd__parse_options(int argc, const char **argv)\n \t\tOPT_INTEGER('j', NULL, &integer, \"get a integer, too\"),\n \t\tOPT_MAGNITUDE('m', \"magnitude\", &magnitude, \"get a magnitude\"),\n \t\tOPT_SET_INT(0, \"set23\", &integer, \"set integer to 23\", 23),\n-\t\tOPT_DATE('t', NULL, ×tamp, \"get timestamp of