{"thread":{"id":"51166","subject":"[PATCH 0/3] fix diff-parseopt regressions","startedAt":"2019-05-24T09:24:53Z","lastAt":"2019-05-29T19:55:04Z","messageCount":18,"participants":["Nguyễn Thái Ngọc Duy","Todd Zullinger","Duy Nguyen","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"376160","messageId":"20190524092442.701-1-pclouds@gmail.com","threadId":"51166","inReplyTo":null,"subject":"[PATCH 0/3] fix diff-parseopt regressions","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-24T09:24:39Z","receivedAt":"2019-05-24T09:24:53Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This should fix the diff tests failure on s360x. It's a serious problem\nand I plan to do something to prevent it from happening again.\n\nThe second patch should bring '-U' (no argument) back. Whether it makes\nsense to accept this behavior is not part of this conversion. We can\ndeal with that later.\n\nThe third patch also brings back a corner case behavior of\n--inter-hunk-context and as a result strengthens OPT_INTEGER() error\nhandling a bit.\n\nNguyễn Thái Ngọc Duy (3):\n diff-parseopt: correct variable types that are used by parseopt\n diff-parseopt: restore -U (no argument) behavior\n parse-options: check empty value in OPT_INTEGER and OPT_ABBREV\n\n diff.c | 10 ++--\n diff.h | 70 +++++++++++------------\n parse-options-cb.c | 3 +\n parse-options.c | 3 +\n t/t4013-diff-various.sh | 2 +\n t/t4013/diff.diff_-U1_initial..side (new) | 29 ++++++++++\n t/t4013/diff.diff_-U2_initial..side (new) | 31 ++++++++++\n t/t4013/diff.diff_-U_initial..side (new) | 32 +++++++++++\n 8 files changed, 141 insertions(+), 39 deletions(-)\n create mode 100644 t/t4013/diff.diff_-U1_initial..side\n create mode 100644 t/t4013/diff.diff_-U2_initial..side\n create mode 100644 t/t4013/diff.diff_-U_initial..side\n\n-- \n2.22.0.rc0.322.g2b0371e29a\n\n"},{"id":"376161","messageId":"20190524092442.701-2-pclouds@gmail.com","threadId":"51166","inReplyTo":"20190524092442.701-1-pclouds@gmail.com","subject":"[PATCH 1/3] diff-parseopt: correct variable types that are used by parseopt","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-24T09:24:40Z","receivedAt":"2019-05-24T09:24:57Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Most number-related OPT_ macros store the value in an 'int'\nvariable. Many of the variables in 'struct diff_options' have a\ndifferent type, but during the conversion to using parse_options() I\nfailed to notice and correct.\n\nThe problem was reported on s360x which is a big-endian\narchitechture. The variable to store '-w' option in this case is\nxdl_opts, 'long' type, 8 bytes. But since parse_options() assumes\n'int' (4 bytes), it will store bits in the wrong part of xdl_opts. The\nproblem was found on little-endian platforms because parse_options()\nwill accidentally store at the right part of xdl_opts.\n\nThere aren't much to say about the type change (except that 'int' for\nxdl_opts should still be big enough, since Windows' long is the same\nsize as 'int' and nobody has complained so far). Some safety checks may\nbe implemented in the future to prevent class of bugs.\n\nReported-by: Todd Zullinger \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n diff.h | 70 +++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 35 insertions(+), 35 deletions(-)\n\ndiff --git a/diff.h b/diff.h\nindex b20cbcc091..4527daf6b7 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -65,39 +65,39 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)\n \n #define DIFF_FLAGS_INIT { 0 }\n struct diff_flags {\n-\tunsigned recursive;\n-\tunsigned tree_in_recursive;\n-\tunsigned binary;\n-\tunsigned text;\n-\tunsigned full_index;\n-\tunsigned silent_on_remove;\n-\tunsigned find_copies_harder;\n-\tunsigned follow_renames;\n-\tunsigned rename_empty;\n-\tunsigned has_changes;\n-\tunsigned quick;\n-\tunsigned no_index;\n-\tunsigned allow_external;\n-\tunsigned exit_with_status;\n-\tunsigned reverse_diff;\n-\tunsigned check_failed;\n-\tunsigned relative_name;\n-\tunsigned ignore_submodules;\n-\tunsigned dirstat_cumulative;\n-\tunsigned dirstat_by_file;\n-\tunsigned allow_textconv;\n-\tunsigned textconv_set_via_cmdline;\n-\tunsigned diff_from_contents;\n-\tunsigned dirty_submodules;\n-\tunsigned ignore_untracked_in_submodules;\n-\tunsigned ignore_dirty_submodules;\n-\tunsigned override_submodule_config;\n-\tunsigned dirstat_by_line;\n-\tunsigned funccontext;\n-\tunsigned default_follow_renames;\n-\tunsigned stat_with_summary;\n-\tunsigned suppress_diff_headers;\n-\tunsigned dual_color_diffed_diffs;\n+\tunsigned int recursive;\n+\tunsigned int tree_in_recursive;\n+\tunsigned int binary;\n+\tunsigned int text;\n+\tunsigned int full_index;\n+\tunsigned int silent_on_remove;\n+\tunsigned int find_copies_harder;\n+\tunsigned int follow_renames;\n+\tunsigned int rename_empty;\n+\tunsigned int has_changes;\n+\tunsigned int quick;\n+\tunsigned int no_index;\n+\tunsigned int allow_external;\n+\tunsigned int exit_with_status;\n+\tunsigned int reverse_diff;\n+\tunsigned int check_failed;\n+\tunsigned int relative_name;\n+\tunsigned int ignore_submodules;\n+\tunsigned int dirstat_cumulative;\n+\tunsigned int dirstat_by_file;\n+\tunsigned int allow_textconv;\n+\tunsigned int textconv_set_via_cmdline;\n+\tunsigned int diff_from_contents;\n+\tunsigned int dirty_submodules;\n+\tunsigned int ignore_untracked_in_submodules;\n+\tunsigned int ignore_dirty_submodules;\n+\tunsigned int override_submodule_config;\n+\tunsigned int dirstat_by_line;\n+\tunsigned int funccontext;\n+\tunsigned int default_follow_renames;\n+\tunsigned int stat_with_summary;\n+\tunsigned int suppress_diff_headers;\n+\tunsigned int dual_color_diffed_diffs;\n };\n \n static inline void diff_flags_or(struct diff_flags *a,\n@@ -151,7 +151,7 @@ struct diff_options {\n \tint skip_stat_unmatch;\n \tint line_termination;\n \tint output_format;\n-\tunsigned pickaxe_opts;\n+\tunsigned int pickaxe_opts;\n \tint rename_score;\n \tint rename_limit;\n \tint needed_rename_limit;\n@@ -169,7 +169,7 @@ struct diff_options {\n \tconst char *prefix;\n \tint prefix_length;\n \tconst char *stat_sep;\n-\tlong xdl_opts;\n+\tint xdl_opts;\n \n \t/* see Documentation/diff-options.txt */\n \tchar **anchors;\n-- \n2.22.0.rc0.322.g2b0371e29a\n\n"},{"id":"376162","messageId":"20190524092442.701-3-pclouds@gmail.com","threadId":"51166","inReplyTo":"20190524092442.701-1-pclouds@gmail.com","subject":"[PATCH 2/3] diff-parseopt: restore -U (no argument) behavior","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-24T09:24:41Z","receivedAt":"2019-05-24T09:25:02Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Before d473e2e0e8 (diff.c: convert -U|--unified, 2019-01-27), -U and\n--unified are implemented with a custom parser opt_arg() in diff.c. I\ndidn't check this code carefully and not realize that it's the\nequivalent of PARSE_OPT_NONEG | PARSE_OPT_OPTARG.\n\nIn other words, if -U is specified without any argument, the option\nshould be accepted, and the default value should be used. Without\nPARSE_OPT_OPTARG, parse_options() will reject this case and cause a\nregression.\n\nReported-by: Bryan Turner \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n diff.c | 10 ++++---\n t/t4013-diff-various.sh | 2 ++\n t/t4013/diff.diff_-U1_initial..side (new) | 29 ++++++++++++++++++++\n t/t4013/diff.diff_-U2_initial..side (new) | 31 ++++++++++++++++++++++\n t/t4013/diff.diff_-U_initial..side (new) | 32 +++++++++++++++++++++++\n 5 files changed, 100 insertions(+), 4 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 4d3cf83a27..80ddc11671 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5211,9 +5211,11 @@ static int diff_opt_unified(const struct option *opt,\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\toptions->context = strtol(arg, &s, 10);\n-\tif (*s)\n-\t\treturn error(_(\"%s expects a numerical value\"), \"--unified\");\n+\tif (arg) {\n+\t\toptions->context = strtol(arg, &s, 10);\n+\t\tif (*s)\n+\t\t\treturn error(_(\"%s expects a numerical value\"), \"--unified\");\n+\t}\n \tenable_patch_output(&options->output_format);\n \n \treturn 0;\n@@ -5272,7 +5274,7 @@ static void prep_parse_options(struct diff_options *options)\n \t\t\t DIFF_FORMAT_PATCH, DIFF_FORMAT_NO_OUTPUT),\n \t\tOPT_CALLBACK_F('U', \"unified\", options, N_(\"\"),\n \t\t\t N_(\"generate diffs with lines context\"),\n-\t\t\t PARSE_OPT_NONEG, diff_opt_unified),\n+\t\t\t PARSE_OPT_NONEG | PARSE_OPT_OPTARG, diff_opt_unified),\n \t\tOPT_BOOL('W', \"function-context\", &options->flags.funccontext,\n \t\t\t N_(\"generate diffs with lines context\")),\n \t\tOPT_BIT_F(0, \"raw\", &options->output_format,\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 9f8f0e84ad..a9054d2db1 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -338,6 +338,8 @@ format-patch --inline --stdout initial..master^^\n format-patch --stdout --cover-letter -n initial..master^\n \n diff --abbrev initial..side\n+diff -U initial..side\n+diff -U1 initial..side\n diff -r initial..side\n diff --stat initial..side\n diff -r --stat initial..side\ndiff --git a/t/t4013/diff.diff_-U1_initial..side b/t/t4013/diff.diff_-U1_initial..side\nnew file mode 100644\nindex 0000000000..b69f8f048a\n--- /dev/null\n+++ b/t/t4013/diff.diff_-U1_initial..side\n@@ -0,0 +1,29 @@\n+$ git diff -U1 initial..side\n+diff --git a/dir/sub b/dir/sub\n+index 35d242b..7289e35 100644\n+--- a/dir/sub\n++++ b/dir/sub\n+@@ -2 +2,3 @@ A\n+ B\n++1\n++2\n+diff --git a/file0 b/file0\n+index 01e79c3..f4615da 100644\n+--- a/file0\n++++ b/file0\n+@@ -3 +3,4 @@\n+ 3\n++A\n++B\n++C\n+diff --git a/file3 b/file3\n+new file mode 100644\n+index 0000000..7289e35\n+--- /dev/null\n++++ b/file3\n+@@ -0,0 +1,4 @@\n++A\n++B\n++1\n++2\n+$\ndiff --git a/t/t4013/diff.diff_-U2_initial..side b/t/t4013/diff.diff_-U2_initial..side\nnew file mode 100644\nindex 0000000000..8ffe04f203\n--- /dev/null\n+++ b/t/t4013/diff.diff_-U2_initial..side\n@@ -0,0 +1,31 @@\n+$ git diff -U2 initial..side\n+diff --git a/dir/sub b/dir/sub\n+index 35d242b..7289e35 100644\n+--- a/dir/sub\n++++ b/dir/sub\n+@@ -1,2 +1,4 @@\n+ A\n+ B\n++1\n++2\n+diff --git a/file0 b/file0\n+index 01e79c3..f4615da 100644\n+--- a/file0\n++++ b/file0\n+@@ -2,2 +2,5 @@\n+ 2\n+ 3\n++A\n++B\n++C\n+diff --git a/file3 b/file3\n+new file mode 100644\n+index 0000000..7289e35\n+--- /dev/null\n++++ b/file3\n+@@ -0,0 +1,4 @@\n++A\n++B\n++1\n++2\n+$\ndiff --git a/t/t4013/diff.diff_-U_initial..side b/t/t4013/diff.diff_-U_initial..side\nnew file mode 100644\nindex 0000000000..c66c0dd5c6\n--- /dev/null\n+++ b/t/t4013/diff.diff_-U_initial..side\n@@ -0,0 +1,32 @@\n+$ git diff -U initial..side\n+diff --git a/dir/sub b/dir/sub\n+index 35d242b..7289e35 100644\n+--- a/dir/sub\n++++ b/dir/sub\n+@@ -1,2 +1,4 @@\n+ A\n+ B\n++1\n++2\n+diff --git a/file0 b/file0\n+index 01e79c3..f4615da 100644\n+--- a/file0\n++++ b/file0\n+@@ -1,3 +1,6 @@\n+ 1\n+ 2\n+ 3\n++A\n++B\n++C\n+diff --git a/file3 b/file3\n+new file mode 100644\n+index 0000000..7289e35\n+--- /dev/null\n++++ b/file3\n+@@ -0,0 +1,4 @@\n++A\n++B\n++1\n++2\n+$\n-- \n2.22.0.rc0.322.g2b0371e29a\n\n"},{"id":"376163","messageId":"20190524092442.701-4-pclouds@gmail.com","threadId":"51166","inReplyTo":"20190524092442.701-1-pclouds@gmail.com","subject":"[PATCH 3/3] parse-options: check empty value in OPT_INTEGER and OPT_ABBREV","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-24T09:24:42Z","receivedAt":"2019-05-24T09:25:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When parsing the argument for OPT_INTEGER and OPT_ABBREV, we check if we\ncan parse the entire argument to a number with \"if (*s)\". There is one\nmissing check: if \"arg\" is empty to begin with, we fail to notice.\n\nThis could happen with long option by writing like\n\n git diff --inter-hunk-context= blah blah\n\nBefore 16ed6c97cc (diff-parseopt: convert --inter-hunk-context,\n2019-03-24), --inter-hunk-context is handled by a custom parser\nopt_arg() and does detect this correctly.\n\nThis restores the bahvior for --inter-hunk-context and make sure all\nother integer options are handled the same (sane) way. For OPT_ABBREV\nthis is new behavior. But it makes it consistent with the rest.\n\nPS. OPT_MAGNITUDE has similar code but git_parse_ulong() does detect\nempty \"arg\". So it's good to go.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n parse-options-cb.c | 3 +++\n parse-options.c | 3 +++\n 2 files changed, 6 insertions(+)\n\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex 4b95d04a37..a3de795c58 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -16,6 +16,9 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)\n \tif (!arg) {\n \t\tv = unset ? 0 : DEFAULT_ABBREV;\n \t} else {\n+\t\tif (!*arg)\n+\t\t\treturn error(_(\"option `%s' expects a numerical value\"),\n+\t\t\t\t opt->long_name);\n \t\tv = strtol(arg, (char **)&arg, 10);\n \t\tif (*arg)\n \t\t\treturn error(_(\"option `%s' expects a numerical value\"),\ndiff --git a/parse-options.c b/parse-options.c\nindex 987e27cb91..87b26a1d92 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -195,6 +195,9 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \t\t}\n \t\tif (get_arg(p, opt, flags, &arg))\n \t\t\treturn -1;\n+\t\tif (!*arg)\n+\t\t\treturn error(_(\"%s expects a numerical value\"),\n+\t\t\t\t optname(opt, flags));\n \t\t*(int *)opt->value = strtol(arg, (char **)&s, 10);\n \t\tif (*s)\n \t\t\treturn error(_(\"%s expects a numerical value\"),\n-- \n2.22.0.rc0.322.g2b0371e29a\n\n"},{"id":"376167","messageId":"20190524093611.1165-1-pclouds@gmail.com","threadId":"51166","inReplyTo":"20190524092442.701-1-pclouds@gmail.com","subject":"[PATCH 4/3] parse-options: make compiler check value type mismatch","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-24T09:36:11Z","receivedAt":"2019-05-24T09:36:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"There is a disconnect between the actual value type from parse-options\nuser and the parser itself. This is because we have to store 'value'\npointer as 'void *' and the compiler cannot help point out that the user\nis passing a 'long' while the parser expects an 'int'. This could lead\nto memory corruption.\n\nIn order to spot these type mismatch problems, a dummy inline function\nis used to process the 'value' input with the right type, before the\ninput is stored in 'struct option'. This gives the compiler some context\nto start complaining.\n\nThe catch though, is that we can only call a function in variable\ndeclaration if it's in automatic scope. Global and static 'struct\noption' variables will fail to build after this. But I think this is an\nreasonable price to pay, compared to memory corruption.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n For the record, this is what I used to make patch 1/3 (I don't think I\n could just rely on manual code inspection to catch these problems)\n\n If we are doing something like this, then we have some clean up to do\n first. I think it's worth doing though. But maybe there's a better way?\n\n parse-options.h | 50 ++++++++++++++++++++++++++++++-------------------\n 1 file changed, 31 insertions(+), 19 deletions(-)\n\ndiff --git a/parse-options.h b/parse-options.h\nindex ac6ba8abf9..b6ea0ac66d 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -128,55 +128,67 @@ struct option {\n \tintptr_t extra;\n };\n \n-#define OPT_BIT_F(s, l, v, h, b, f) { OPTION_BIT, (s), (l), (v), NULL, (h), \\\n+#define DEFINE_OPT_TYPE_CHECK(name, type) \\\n+\tstatic inline void *_opt_ ## name(type *p)\t\\\n+\t{\t\t\t\t\t\t\\\n+\t\treturn p;\t\t\t\t\\\n+\t}\n+\n+DEFINE_OPT_TYPE_CHECK(int, int)\n+DEFINE_OPT_TYPE_CHECK(ulong, unsigned long)\n+DEFINE_OPT_TYPE_CHECK(string, const char *)\n+DEFINE_OPT_TYPE_CHECK(string_list, struct string_list)\n+DEFINE_OPT_TYPE_CHECK(timestamp, timestamp_t)\n+\n+#define OPT_BIT_F(s, l, v, h, b, f) { OPTION_BIT, (s), (l), _opt_int(v), NULL, (h), \\\n \t\t\t\t PARSE_OPT_NOARG|(f), NULL, (b) }\n-#define OPT_COUNTUP_F(s, l, v, h, f) { OPTION_COUNTUP, (s), (l), (v), NULL, \\\n+#define OPT_COUNTUP_F(s, l, v, h, f) { OPTION_COUNTUP, (s), (l), _opt_int(v), NULL, \\\n \t\t\t\t (h), PARSE_OPT_NOARG|(f) }\n-#define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), (v), NULL, \\\n+#define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \\\n \t\t\t\t\t (h), PARSE_OPT_NOARG | (f), NULL, (i) }\n #define OPT_BOOL_F(s, l, v, h, f) OPT_SET_INT_F(s, l, v, h, 1, f)\n #define OPT_CALLBACK_F(s, l, v, a, h, f, cb)\t\t\t\\\n \t{ OPTION_CALLBACK, (s), (l), (v), (a), (h), (f), (cb) }\n-#define OPT_STRING_F(s, l, v, a, h, f) { OPTION_STRING, (s), (l), (v), (a), (h), (f) }\n-#define OPT_INTEGER_F(s, l, v, h, f) { OPTION_INTEGER, (s), (l), (v), N_(\"n\"), (h), (f) }\n+#define OPT_STRING_F(s, l, v, a, h, f) { OPTION_STRING, (s), (l), _opt_string(v), (a), (h), (f) }\n+#define OPT_INTEGER_F(s, l, v, h, f) { OPTION_INTEGER, (s), (l), _opt_int(v), N_(\"n\"), (h), (f) }\n \n #define OPT_END() { OPTION_END }\n-#define OPT_ARGUMENT(l, v, h) { OPTION_ARGUMENT, 0, (l), (v), NULL, \\\n+#define OPT_ARGUMENT(l, v, h) { OPTION_ARGUMENT, 0, (l), _opt_int(v), NULL, \\\n \t\t\t\t (h), PARSE_OPT_NOARG, NULL, 1 }\n #define OPT_GROUP(h) { OPTION_GROUP, 0, NULL, NULL, NULL, (h) }\n #define OPT_BIT(s, l, v, h, b) OPT_BIT_F(s, l, v, h, b, 0)\n-#define OPT_BITOP(s, l, v, h, set, clear) { OPTION_BITOP, (s), (l), (v), NULL, (h), \\\n+#define OPT_BITOP(s, l, v, h, set, clear) { OPTION_BITOP, (s), (l), _opt_int(v), NULL, (h), \\\n \t\t\t\t\t PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, \\\n \t\t\t\t\t (set), NULL, (clear) }\n-#define OPT_NEGBIT(s, l, v, h, b) { OPTION_NEGBIT, (s), (l), (v), NULL, \\\n+#define OPT_NEGBIT(s, l, v, h, b) { OPTION_NEGBIT, (s), (l), _opt_int(v), NULL, \\\n \t\t\t\t (h), PARSE_OPT_NOARG, NULL, (b) }\n #define OPT_COUNTUP(s, l, v, h) OPT_COUNTUP_F(s, l, v, h, 0)\n #define OPT_SET_INT(s, l, v, h, i) OPT_SET_INT_F(s, l, v, h, i, 0)\n #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)\n-#define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \\\n+#define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \\\n \t\t\t\t (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}\n-#define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \\\n+#define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), _opt_int(v), NULL, \\\n \t\t\t\t (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }\n #define OPT_INTEGER(s, l, v, h) OPT_INTEGER_F(s, l, v, h, 0)\n-#define OPT_MAGNITUDE(s, l, v, h) { OPTION_MAGNITUDE, (s), (l), (v), \\\n+#define OPT_MAGNITUDE(s, l, v, h) { OPTION_MAGNITUDE, (s), (l), _opt_ulong(v), \\\n \t\t\t\t N_(\"n\"), (h), PARSE_OPT_NONEG }\n #define OPT_STRING(s, l, v, a, h) OPT_STRING_F(s, l, v, a, h, 0)\n #define OPT_STRING_LIST(s, l, v, a, h) \\\n-\t\t\t\t { OPTION_CALLBACK, (s), (l), (v), (a), \\\n+\t\t\t\t { OPTION_CALLBACK, (s), (l), _opt_string_list(v), (a), \\\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+#define OPT_UYN(s, l, v, h) { OPTION_CALLBACK, (s), (l), _opt_int(v), NULL, \\\n \t\t\t\t (h), PARSE_OPT_NOARG, &parse_opt_tertiary }\n #define OPT_EXPIRY_DATE(s, l, v, h) \\\n-\t{ OPTION_CALLBACK, (s), (l), (v), N_(\"expiry-date\"),(h), 0,\t\\\n+\t{ OPTION_CALLBACK, (s), (l), _opt_timestamp(v), N_(\"expiry-date\"),(h), 0,\t\\\n \t parse_opt_expiry_date_cb }\n #define OPT_CALLBACK(s, l, v, a, h, f) OPT_CALLBACK_F(s, l, v, a, h, 0, f)\n #define OPT_NUMBER_CALLBACK(v, h, f) \\\n \t{ OPTION_NUMBER, 0, NULL, (v), NULL, (h), \\\n \t PARSE_OPT_NOARG | PARSE_OPT_NONEG, (f) }\n-#define OPT_FILENAME(s, l, v, h) { OPTION_FILENAME, (s), (l), (v), \\\n+#define OPT_FILENAME(s, l, v, h) { OPTION_FILENAME, (s), (l), _opt_string(v), \\\n \t\t\t\t N_(\"file\"), (h) }\n #define OPT_COLOR_FLAG(s, l, v, h) \\\n-\t{ OPTION_CALLBACK, (s), (l), (v), N_(\"when\"), (h), PARSE_OPT_OPTARG, \\\n+\t{ OPTION_CALLBACK, (s), (l), _opt_int(v), N_(\"when\"), (h), PARSE_OPT_OPTARG, \\\n \t\tparse_opt_color_flag_cb, (intptr_t)\"always\" }\n \n #define OPT_NOOP_NOARG(s, l) \\\n@@ -301,14 +313,14 @@ int parse_opt_passthru_argv(const struct option *, const char *, int);\n #define OPT__VERBOSE(var, h) OPT_COUNTUP('v', \"verbose\", (var), (h))\n #define OPT__QUIET(var, h) OPT_COUNTUP('q', \"quiet\", (var), (h))\n #define OPT__VERBOSITY(var) \\\n-\t{ OPTION_CALLBACK, 'v', \"verbose\", (var), NULL, N_(\"be more verbose\"), \\\n+\t{ OPTION_CALLBACK, 'v', \"verbose\", _opt_int(var), NULL, N_(\"be more verbose\"), \\\n \t PARSE_OPT_NOARG, &parse_opt_verbosity_cb, 0 }, \\\n-\t{ OPTION_CALLBACK, 'q', \"quiet\", (var), NULL, N_(\"be more quiet\"), \\\n+\t{ OPTION_CALLBACK, 'q', \"quiet\", _opt_int(var), NULL, N_(\"be more quiet\"), \\\n \t PARSE_OPT_NOARG, &parse_opt_verbosity_cb, 0 }\n #define OPT__DRY_RUN(var, h) OPT_BOOL('n', \"dry-run\", (var), (h))\n #define OPT__FORCE(var, h, f) OPT_COUNTUP_F('f', \"force\", (var), (h), (f))\n #define OPT__ABBREV(var) \\\n-\t{ OPTION_CALLBACK, 0, \"abbrev\", (var), N_(\"n\"),\t\\\n+\t{ OPTION_CALLBACK, 0, \"abbrev\", _opt_int(var), N_(\"n\"),\t\\\n \t N_(\"use digits to display SHA-1s\"),\t\\\n \t PARSE_OPT_OPTARG, &parse_opt_abbrev_cb, 0 }\n #define OPT__COLOR(var, h) \\\n-- \n2.22.0.rc0.322.g2b0371e29a\n\n"},{"id":"376196","messageId":"20190524173642.GQ3654@pobox.com","threadId":"51166","inReplyTo":"20190524092442.701-1-pclouds@gmail.com","subject":"Re: [PATCH 0/3] fix diff-parseopt regressions","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-05-24T17:36:42Z","receivedAt":"2019-05-24T17:37:03Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Hi,\n\nNguyễn Thái Ngọc Duy wrote:\n> This should fix the diff tests failure on s360x. It's a serious problem\n> and I plan to do something to prevent it from happening again.\n\nThanks for looking at this!\n\nI applied this on top of master/2.22.0-rc1 and see a number\nof compiler errors using gcc-9.1.1 with fedora's standard\ncompiler options for rpm builds.\n\nBelow are the compiler errors. This was from an s390x\nbuild, but other arches had the same errors. The complete\nbuild log is available here for a few weeks:\nhttps://kojipkgs.fedoraproject.org//work/tasks/3166/35033166/build.log\n\ncc -o credential-store.o -c -MF ./.depend/credential-store.o.d -MQ credential-store.o -MMD -MP -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -march=zEC12 -mtune=z13 -fasynchronous-unwind-tables -fstack-clash-protection -I. -DHAVE_SYSINFO -DGIT_HOST_CPU=\"\\\"s390x\\\"\" -DUSE_LIBPCRE2 -DHAVE_ALLOCA_H -DUSE_CURL_FOR_IMAP_SEND -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C=\"\\\"cache.h\\\"\" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C=\"\\\"git-compat-util.h\\\"\" -DSHA256_BLK -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_GETDELIM '-DPROCFS_EXECUTABLE_PATH=\"/proc/self/exe\"' -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -DSHELL_PATH='\"/bin/sh\"' -DPAGER_ENV='\"LESS=FRX LV=-c\"' credential-store.c\nIn file included from credential-store.c:5:\ncredential-store.c: In function 'cmd_main':\ncredential-store.c:156:25: warning: passing argument 1 of '_opt_string' from incompatible pointer type [-Wincompatible-pointer-types]\n 156 | OPT_STRING(0, \"file\", &file, \"path\",\n | ^~~~~\n | |\n | char **\nparse-options.h:152:82: note: in definition of macro 'OPT_STRING_F'\n 152 | #define OPT_STRING_F(s, l, v, a, h, f) { OPTION_STRING, (s), (l), _opt_string(v), (a), (h), (f) }\n | ^\ncredential-store.c:156:3: note: in expansion of macro 'OPT_STRING'\n 156 | OPT_STRING(0, \"file\", &file, \"path\",\n | ^~~~~~~~~~\nparse-options.h:132:42: note: expected 'const char **' but argument is of type 'char **'\n 132 | static inline void *_opt_ ## name(type *p) \\\n | ^\nparse-options.h:139:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'\n 139 | DEFINE_OPT_TYPE_CHECK(string, const char *)\n | ^~~~~~~~~~~~~~~~~~~~~\n * new link flags\n\n\ncc -o apply.o -c -MF ./.depend/apply.o.d -MQ apply.o -MMD -MP -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -march=zEC12 -mtune=z13 -fasynchronous-unwind-tables -fstack-clash-protection -I. -DHAVE_SYSINFO -DGIT_HOST_CPU=\"\\\"s390x\\\"\" -DUSE_LIBPCRE2 -DHAVE_ALLOCA_H -DUSE_CURL_FOR_IMAP_SEND -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C=\"\\\"cache.h\\\"\" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C=\"\\\"git-compat-util.h\\\"\" -DSHA256_BLK -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_GETDELIM '-DPROCFS_EXECUTABLE_PATH=\"/proc/self/exe\"' -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -DSHELL_PATH='\"/bin/sh\"' -DPAGER_ENV='\"LESS=FRX LV=-c\"' apply.c\nIn file included from apply.c:20:\napply.c: In function 'apply_parse_options':\napply.c:5002:26: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]\n 5002 | OPT_INTEGER('C', NULL, &state->p_context,\n | ^~~~~~~~~~~~~~~~~\n | |\n | unsigned int *\nparse-options.h:153:79: note: in definition of macro 'OPT_INTEGER_F'\n 153 | #define OPT_INTEGER_F(s, l, v, h, f) { OPTION_INTEGER, (s), (l), _opt_int(v), N_(\"n\"), (h), (f) }\n | ^\napply.c:5002:3: note: in expansion of macro 'OPT_INTEGER'\n 5002 | OPT_INTEGER('C', NULL, &state->p_context,\n | ^~~~~~~~~~~\nparse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'\n 132 | static inline void *_opt_ ## name(type *p) \\\n | ^\nparse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'\n 137 | DEFINE_OPT_TYPE_CHECK(int, int)\n | ^~~~~~~~~~~~~~~~~~~~~\n\n\ncc -o diff.o -c -MF ./.depend/diff.o.d -MQ diff.o -MMD -MP -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -march=zEC12 -mtune=z13 -fasynchronous-unwind-tables -fstack-clash-protection -I. -DHAVE_SYSINFO -DGIT_HOST_CPU=\"\\\"s390x\\\"\" -DUSE_LIBPCRE2 -DHAVE_ALLOCA_H -DUSE_CURL_FOR_IMAP_SEND -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C=\"\\\"cache.h\\\"\" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C=\"\\\"git-compat-util.h\\\"\" -DSHA256_BLK -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_GETDELIM '-DPROCFS_EXECUTABLE_PATH=\"/proc/self/exe\"' -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -DSHELL_PATH='\"/bin/sh\"' -DPAGER_ENV='\"LESS=FRX LV=-c\"' diff.c\nIn file included from diff.c:26:\ndiff.c: In function 'prep_parse_options':\ndiff.c:5278:37: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]\n 5278 | OPT_BOOL('W', \"function-context\", &options->flags.funccontext,\n | ^~~~~~~~~~~~~~~~~~~~~~~~~~~\n | |\n | unsigned int *\nparse-options.h:147:78: note: in definition of macro 'OPT_SET_INT_F'\n 147 | #define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \\\n | ^\nparse-options.h:167:37: note: in expansion of macro 'OPT_BOOL_F'\n 167 | #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)\n | ^~~~~~~~~~\ndiff.c:5278:3: note: in expansion of macro 'OPT_BOOL'\n 5278 | OPT_BOOL('W', \"function-context\", &options->flags.funccontext,\n | ^~~~~~~~\nparse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'\n 132 | static inline void *_opt_ ## name(type *p) \\\n | ^\nparse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'\n 137 | DEFINE_OPT_TYPE_CHECK(int, int)\n | ^~~~~~~~~~~~~~~~~~~~~\ndiff.c:5342:29: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]\n 5342 | OPT_BOOL(0, \"full-index\", &options->flags.full_index,\n | ^~~~~~~~~~~~~~~~~~~~~~~~~~\n | |\n | unsigned int *\nparse-options.h:147:78: note: in definition of macro 'OPT_SET_INT_F'\n 147 | #define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \\\n | ^\nparse-options.h:167:37: note: in expansion of macro 'OPT_BOOL_F'\n 167 | #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)\n | ^~~~~~~~~~\ndiff.c:5342:3: note: in expansion of macro 'OPT_BOOL'\n 5342 | OPT_BOOL(0, \"full-index\", &options->flags.full_index,\n | ^~~~~~~~\nparse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'\n 132 | static inline void *_opt_ ## name(type *p) \\\n | ^\nparse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'\n 137 | DEFINE_OPT_TYPE_CHECK(int, int)\n | ^~~~~~~~~~~~~~~~~~~~~\ndiff.c:5400:37: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]\n 5400 | OPT_BOOL(0, \"find-copies-harder\", &options->flags.find_copies_harder,\n | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n | |\n | unsigned int *\nparse-options.h:147:78: note: in definition of macro 'OPT_SET_INT_F'\n 147 | #define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \\\n | ^\nparse-options.h:167:37: note: in expansion of macro 'OPT_BOOL_F'\n 167 | #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)\n | ^~~~~~~~~~\ndiff.c:5400:3: note: in expansion of macro 'OPT_BOOL'\n 5400 | OPT_BOOL(0, \"find-copies-harder\", &options->flags.find_copies_harder,\n | ^~~~~~~~\nparse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'\n 132 | static inline void *_opt_ ## name(type *p) \\\n | ^\nparse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'\n 137 | DEFINE_OPT_TYPE_CHECK(int, int)\n | ^~~~~~~~~~~~~~~~~~~~~\ndiff.c:5405:31: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]\n 5405 | OPT_BOOL(0, \"rename-empty\", &options->flags.rename_empty,\n | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~\n | |\n | unsigned int *\nparse-options.h:147:78: note: in definition of macro 'OPT_SET_INT_F'\n 147 | #define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \\\n | ^\nparse-options.h:167:37: note: in expansion of macro 'OPT_BOOL_F'\n 167 | #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)\n | ^~~~~~~~~~\ndiff.c:5405:3: note: in expansion of macro 'OPT_BOOL'\n 5405 | OPT_BOOL(0, \"rename-empty\", &options->flags.rename_empty,\n | ^~~~~~~~\nparse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'\n 132 | static inline void *_opt_ ## name(type *p) \\\n | ^\nparse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'\n 137 | DEFINE_OPT_TYPE_CHECK(int, int)\n | ^~~~~~~~~~~~~~~~~~~~~\ndiff.c:5469:25: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]\n 5469 | OPT_BOOL('a', \"text\", &options->flags.text,\n | ^~~~~~~~~~~~~~~~~~~~\n | |\n | unsigned int *\nparse-options.h:147:78: note: in definition of macro 'OPT_SET_INT_F'\n 147 | #define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \\\n | ^\nparse-options.h:167:37: note: in expansion of macro 'OPT_BOOL_F'\n 167 | #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)\n | ^~~~~~~~~~\ndiff.c:5469:3: note: in expansion of macro 'OPT_BOOL'\n 5469 | OPT_BOOL('a', \"text\", &options->flags.text,\n | ^~~~~~~~\nparse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'\n 132 | static inline void *_opt_ ## name(type *p) \\\n | ^\nparse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'\n 137 | DEFINE_OPT_TYPE_CHECK(int, int)\n | ^~~~~~~~~~~~~~~~~~~~~\ndiff.c:5471:23: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]\n 5471 | OPT_BOOL('R', NULL, &options->flags.reverse_diff,\n | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~\n | |\n | unsigned int *\nparse-options.h:147:78: note: in definition of macro 'OPT_SET_INT_F'\n 147 | #define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \\\n | ^\nparse-options.h:167:37: note: in expansion of macro 'OPT_BOOL_F'\n 167 | #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)\n | ^~~~~~~~~~\ndiff.c:5471:3: note: in expansion of macro 'OPT_BOOL'\n 5471 | OPT_BOOL('R', NULL, &options->flags.reverse_diff,\n | ^~~~~~~~\nparse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'\n 132 | static inline void *_opt_ ## name(type *p) \\\n | ^\nparse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'\n 137 | DEFINE_OPT_TYPE_CHECK(int, int)\n | ^~~~~~~~~~~~~~~~~~~~~\ndiff.c:5473:28: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]\n 5473 | OPT_BOOL(0, \"exit-code\", &options->flags.exit_with_status,\n | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n | |\n | unsigned int *\nparse-options.h:147:78: note: in definition of macro 'OPT_SET_INT_F'\n 147 | #define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \\\n | ^\nparse-options.h:167:37: note: in expansion of macro 'OPT_BOOL_F'\n 167 | #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)\n | ^~~~~~~~~~\ndiff.c:5473:3: note: in expansion of macro 'OPT_BOOL'\n 5473 | OPT_BOOL(0, \"exit-code\", &options->flags.exit_with_status,\n | ^~~~~~~~\nparse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'\n 132 | static inline void *_opt_ ## name(type *p) \\\n | ^\nparse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'\n 137 | DEFINE_OPT_TYPE_CHECK(int, int)\n | ^~~~~~~~~~~~~~~~~~~~~\ndiff.c:5475:24: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]\n 5475 | OPT_BOOL(0, \"quiet\", &options->flags.quick,\n | ^~~~~~~~~~~~~~~~~~~~~\n | |\n | unsigned int *\nparse-options.h:147:78: note: in definition of macro 'OPT_SET_INT_F'\n 147 | #define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \\\n | ^\nparse-options.h:167:37: note: in expansion of macro 'OPT_BOOL_F'\n 167 | #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)\n | ^~~~~~~~~~\ndiff.c:5475:3: note: in expansion of macro 'OPT_BOOL'\n 5475 | OPT_BOOL(0, \"quiet\", &options->flags.quick,\n | ^~~~~~~~\nparse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'\n 132 | static inline void *_opt_ ## name(type *p) \\\n | ^\nparse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'\n 137 | DEFINE_OPT_TYPE_CHECK(int, int)\n | ^~~~~~~~~~~~~~~~~~~~~\ndiff.c:5477:27: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]\n 5477 | OPT_BOOL(0, \"ext-diff\", &options->flags.allow_external,\n | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n | |\n | unsigned int *\nparse-options.h:147:78: note: in definition of macro 'OPT_SET_INT_F'\n 147 | #define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \\\n | ^\nparse-options.h:167:37: note: in expansion of macro 'OPT_BOOL_F'\n 167 | #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)\n | ^~~~~~~~~~\ndiff.c:5477:3: note: in expansion of macro 'OPT_BOOL'\n 5477 | OPT_BOOL(0, \"ext-diff\", &options->flags.allow_external,\n | ^~~~~~~~\nparse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'\n 132 | static inline void *_opt_ ## name(type *p) \\\n | ^\nparse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'\n 137 | DEFINE_OPT_TYPE_CHECK(int, int)\n | ^~~~~~~~~~~~~~~~~~~~~\ndiff.c:5502:31: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]\n 5502 | OPT_BIT_F(0, \"pickaxe-all\", &options->pickaxe_opts,\n | ^~~~~~~~~~~~~~~~~~~~~~\n | |\n | unsigned int *\nparse-options.h:143:70: note: in definition of macro 'OPT_BIT_F'\n 143 | #define OPT_BIT_F(s, l, v, h, b, f) { OPTION_BIT, (s), (l), _opt_int(v), NULL, (h), \\\n | ^\nparse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'\n 132 | static inline void *_opt_ ## name(type *p) \\\n | ^\nparse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'\n 137 | DEFINE_OPT_TYPE_CHECK(int, int)\n | ^~~~~~~~~~~~~~~~~~~~~\ndiff.c:5505:33: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]\n 5505 | OPT_BIT_F(0, \"pickaxe-regex\", &options->pickaxe_opts,\n | ^~~~~~~~~~~~~~~~~~~~~~\n | |\n | unsigned int *\nparse-options.h:143:70: note: in definition of macro 'OPT_BIT_F'\n 143 | #define OPT_BIT_F(s, l, v, h, b, f) { OPTION_BIT, (s), (l), _opt_int(v), NULL, (h), \\\n | ^\nparse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'\n 132 | static inline void *_opt_ ## name(type *p) \\\n | ^\nparse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'\n 137 | DEFINE_OPT_TYPE_CHECK(int, int)\n | ^~~~~~~~~~~~~~~~~~~~~\n\n\ncc -o parse-options.o -c -MF ./.depend/parse-options.o.d -MQ parse-options.o -MMD -MP -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -march=zEC12 -mtune=z13 -fasynchronous-unwind-tables -fstack-clash-protection -I. -DHAVE_SYSINFO -DGIT_HOST_CPU=\"\\\"s390x\\\"\" -DUSE_LIBPCRE2 -DHAVE_ALLOCA_H -DUSE_CURL_FOR_IMAP_SEND -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C=\"\\\"cache.h\\\"\" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C=\"\\\"git-compat-util.h\\\"\" -DSHA256_BLK -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_GETDELIM '-DPROCFS_EXECUTABLE_PATH=\"/proc/self/exe\"' -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -DSHELL_PATH='\"/bin/sh\"' -DPAGER_ENV='\"LESS=FRX LV=-c\"' parse-options.c\nIn file included from parse-options.c:2:\nparse-options.h:140:43: warning: 'struct string_list' declared inside parameter list will not be visible outside of this definition or declaration\n 140 | DEFINE_OPT_TYPE_CHECK(string_list, struct string_list)\n | ^~~~~~~~~~~\nparse-options.h:132:36: note: in definition of macro 'DEFINE_OPT_TYPE_CHECK'\n 132 | static inline void *_opt_ ## name(type *p) \\\n | ^~~~\n\ncc -o parse-options-cb.o -c -MF ./.depend/parse-options-cb.o.d -MQ parse-options-cb.o -MMD -MP -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -march=zEC12 -mtune=z13 -fasynchronous-unwind-tables -fstack-clash-protection -I. -DHAVE_SYSINFO -DGIT_HOST_CPU=\"\\\"s390x\\\"\" -DUSE_LIBPCRE2 -DHAVE_ALLOCA_H -DUSE_CURL_FOR_IMAP_SEND -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C=\"\\\"cache.h\\\"\" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C=\"\\\"git-compat-util.h\\\"\" -DSHA256_BLK -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_GETDELIM '-DPROCFS_EXECUTABLE_PATH=\"/proc/self/exe\"' -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -DSHELL_PATH='\"/bin/sh\"' -DPAGER_ENV='\"LESS=FRX LV=-c\"' parse-options-cb.c\nIn file included from parse-options-cb.c:2:\nparse-options.h:140:43: warning: 'struct string_list' declared inside parameter list will not be visible outside of this definition or declaration\n 140 | DEFINE_OPT_TYPE_CHECK(string_list, struct string_list)\n | ^~~~~~~~~~~\nparse-options.h:132:36: note: in definition of macro 'DEFINE_OPT_TYPE_CHECK'\n 132 | static inline void *_opt_ ## name(type *p) \\\n | ^~~~\n\n\ncc -o imap-send.o -c -MF ./.depend/imap-send.o.d -MQ imap-send.o -MMD -MP -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -march=zEC12 -mtune=z13 -fasynchronous-unwind-tables -fstack-clash-protection -I. -DHAVE_SYSINFO -DGIT_HOST_CPU=\"\\\"s390x\\\"\" -DUSE_LIBPCRE2 -DHAVE_ALLOCA_H -DUSE_CURL_FOR_IMAP_SEND -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C=\"\\\"cache.h\\\"\" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C=\"\\\"git-compat-util.h\\\"\" -DSHA256_BLK -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_GETDELIM '-DPROCFS_EXECUTABLE_PATH=\"/proc/self/exe\"' -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -DSHELL_PATH='\"/bin/sh\"' -DPAGER_ENV='\"LESS=FRX LV=-c\"' imap-send.c\nIn file included from imap-send.c:29:\nparse-options.h:316:37: error: initializer element is not constant\n 316 | { OPTION_CALLBACK, 'v', \"verbose\", _opt_int(var), NULL, N_(\"be more verbose\"), \\\n | ^~~~~~~~\nimap-send.c:51:2: note: in expansion of macro 'OPT__VERBOSITY'\n 51 | OPT__VERBOSITY(&verbosity),\n | ^~~~~~~~~~~~~~\nparse-options.h:316:37: note: (near initialization for 'imap_send_options[0].value')\n 316 | { OPTION_CALLBACK, 'v', \"verbose\", _opt_int(var), NULL, N_(\"be more verbose\"), \\\n | ^~~~~~~~\nimap-send.c:51:2: note: in expansion of macro 'OPT__VERBOSITY'\n 51 | OPT__VERBOSITY(&verbosity),\n | ^~~~~~~~~~~~~~\nparse-options.h:318:35: error: initializer element is not constant\n 318 | { OPTION_CALLBACK, 'q', \"quiet\", _opt_int(var), NULL, N_(\"be more quiet\"), \\\n | ^~~~~~~~\nimap-send.c:51:2: note: in expansion of macro 'OPT__VERBOSITY'\n 51 | OPT__VERBOSITY(&verbosity),\n | ^~~~~~~~~~~~~~\nparse-options.h:318:35: note: (near initialization for 'imap_send_options[1].value')\n 318 | { OPTION_CALLBACK, 'q', \"quiet\", _opt_int(var), NULL, N_(\"be more quiet\"), \\\n | ^~~~~~~~\nimap-send.c:51:2: note: in expansion of macro 'OPT__VERBOSITY'\n 51 | OPT__VERBOSITY(&verbosity),\n | ^~~~~~~~~~~~~~\nparse-options.h:147:69: error: initializer element is not constant\n 147 | #define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \\\n | ^~~~~~~~\nparse-options.h:149:37: note: in expansion of macro 'OPT_SET_INT_F'\n 149 | #define OPT_BOOL_F(s, l, v, h, f) OPT_SET_INT_F(s, l, v, h, 1, f)\n | ^~~~~~~~~~~~~\nparse-options.h:167:37: note: in expansion of macro 'OPT_BOOL_F'\n 167 | #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)\n | ^~~~~~~~~~\nimap-send.c:52:2: note: in expansion of macro 'OPT_BOOL'\n 52 | OPT_BOOL(0, \"curl\", &use_curl, \"use libcurl to communicate with the IMAP server\"),\n | ^~~~~~~~\nparse-options.h:147:69: note: (near initialization for 'imap_send_options[2].value')\n 147 | #define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \\\n | ^~~~~~~~\nparse-options.h:149:37: note: in expansion of macro 'OPT_SET_INT_F'\n 149 | #define OPT_BOOL_F(s, l, v, h, f) OPT_SET_INT_F(s, l, v, h, 1, f)\n | ^~~~~~~~~~~~~\nparse-options.h:167:37: note: in expansion of macro 'OPT_BOOL_F'\n 167 | #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)\n | ^~~~~~~~~~\nimap-send.c:52:2: note: in expansion of macro 'OPT_BOOL'\n 52 | OPT_BOOL(0, \"curl\", &use_curl, \"use libcurl to communicate with the IMAP server\"),\n | ^~~~~~~~\n\nThanks,\n\n-- \nTodd\n"},{"id":"376205","messageId":"20190524205841.GA28239@pobox.com","threadId":"51166","inReplyTo":"20190524173642.GQ3654@pobox.com","subject":"Re: [PATCH 0/3] fix diff-parseopt regressions","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-05-24T20:58:41Z","receivedAt":"2019-05-24T20:58:53Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"I wrote:\n> Below are the compiler errors.\n\nWell, to be precise, all but imap-send are warnings rather\nthan errors.\n\n-- \nTodd\n"},{"id":"376212","messageId":"CACsJy8AgstSQKnAVf7Krg8yn03ewBw0mhOWq=CgYER-6tA8ptw@mail.gmail.com","threadId":"51166","inReplyTo":"20190524173642.GQ3654@pobox.com","subject":"Re: [PATCH 0/3] fix diff-parseopt regressions","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-25T10:22:54Z","receivedAt":"2019-05-25T10:23:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, May 25, 2019 at 12:36 AM Todd Zullinger wrote:\n>\n> Hi,\n>\n> Nguyễn Thái Ngọc Duy wrote:\n> > This should fix the diff tests failure on s360x. It's a serious problem\n> > and I plan to do something to prevent it from happening again.\n>\n> Thanks for looking at this!\n>\n> I applied this on top of master/2.22.0-rc1 and see a number\n> of compiler errors using gcc-9.1.1 with fedora's standard\n> compiler options for rpm builds.\n\nThat last patch 4/3 is not meant to be applied. Yes I've seen similar\ncompiler errors too. We have some cleaning up to do in order to build\nwith the last one. But I think there are no other serious errors\nspotted by the last patch (there's one in builtin/column.c, but I\nthink nobody uses it much, so we can fix it alter)\n-- \nDuy\n"},{"id":"376222","messageId":"20190525204840.GT3654@pobox.com","threadId":"51166","inReplyTo":"CACsJy8AgstSQKnAVf7Krg8yn03ewBw0mhOWq=CgYER-6tA8ptw@mail.gmail.com","subject":"Re: [PATCH 0/3] fix diff-parseopt regressions","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-05-25T20:48:40Z","receivedAt":"2019-05-25T20:48:46Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Duy Nguyen wrote:\n> On Sat, May 25, 2019 at 12:36 AM Todd Zullinger wrote:\n>> I applied this on top of master/2.22.0-rc1 and see a number\n>> of compiler errors using gcc-9.1.1 with fedora's standard\n>> compiler options for rpm builds.\n> \n> That last patch 4/3 is not meant to be applied. Yes I've seen similar\n> compiler errors too. We have some cleaning up to do in order to build\n> with the last one.\n\nD'oh! Sorry for missing that rather obvious part of your\nprevious message. The tests do indeed all pass on all the\narchitectures in the Fedora build system.\n\nI'll go dig out my dunce cap from back in grade school. ;)\n\nThanks again,\n\n-- \nTodd\n"},{"id":"376343","messageId":"xmqqtvde1iyu.fsf@gitster-ct.c.googlers.com","threadId":"51166","inReplyTo":"20190524092442.701-2-pclouds@gmail.com","subject":"Re: [PATCH 1/3] diff-parseopt: correct variable types that are used by parseopt","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-28T19:23:37Z","receivedAt":"2019-05-28T19:23:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Most number-related OPT_ macros store the value in an 'int'\n> variable. Many of the variables in 'struct diff_options' have a\n> different type, but during the conversion to using parse_options() I\n> failed to notice and correct.\n\nWhy does this patch need to be so noisy? \"unsigned identifier\" is\nthe same as \"unsigned int identifier\", isn't it?\n\nThat is, wouldn't this hunk ...\n\n> @@ -169,7 +169,7 @@ struct diff_options {\n> \tconst char *prefix;\n> \tint prefix_length;\n> \tconst char *stat_sep;\n> -\tlong xdl_opts;\n> +\tint xdl_opts;\n\n... the only one that matters?\n"},{"id":"376385","messageId":"20190529091116.21898-1-pclouds@gmail.com","threadId":"51166","inReplyTo":"20190524092442.701-1-pclouds@gmail.com","subject":"[PATCH v2 0/3] fix diff-parseopt regressions","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-29T09:11:13Z","receivedAt":"2019-05-29T09:11:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"v2 reduces diff noise. My C is rusty (and probably holey too). For some\nreason I remember \"unsigned\" is equivalent to \"unsigned short\", not\n\"unsigned int\".\n\nNguyễn Thái Ngọc Duy (3):\n diff-parseopt: correct variable types that are used by parseopt\n diff-parseopt: restore -U (no argument) behavior\n parse-options: check empty value in OPT_INTEGER and OPT_ABBREV\n\n diff.c | 10 ++++---\n diff.h | 2 +-\n parse-options-cb.c | 3 +++\n parse-options.c | 3 +++\n t/t4013-diff-various.sh | 2 ++\n t/t4013/diff.diff_-U1_initial..side (new) | 29 ++++++++++++++++++++\n t/t4013/diff.diff_-U2_initial..side (new) | 31 ++++++++++++++++++++++\n t/t4013/diff.diff_-U_initial..side (new) | 32 +++++++++++++++++++++++\n 8 files changed, 107 insertions(+), 5 deletions(-)\n create mode 100644 t/t4013/diff.diff_-U1_initial..side\n create mode 100644 t/t4013/diff.diff_-U2_initial..side\n create mode 100644 t/t4013/diff.diff_-U_initial..side\n\nInterdiff dựa trên v1:\ndiff --git a/diff.h b/diff.h\nindex 4527daf6b7..d5e44baa96 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -65,39 +65,39 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)\n \n #define DIFF_FLAGS_INIT { 0 }\n struct diff_flags {\n-\tunsigned int recursive;\n-\tunsigned int tree_in_recursive;\n-\tunsigned int binary;\n-\tunsigned int text;\n-\tunsigned int full_index;\n-\tunsigned int silent_on_remove;\n-\tunsigned int find_copies_harder;\n-\tunsigned int follow_renames;\n-\tunsigned int rename_empty;\n-\tunsigned int has_changes;\n-\tunsigned int quick;\n-\tunsigned int no_index;\n-\tunsigned int allow_external;\n-\tunsigned int exit_with_status;\n-\tunsigned int reverse_diff;\n-\tunsigned int check_failed;\n-\tunsigned int relative_name;\n-\tunsigned int ignore_submodules;\n-\tunsigned int dirstat_cumulative;\n-\tunsigned int dirstat_by_file;\n-\tunsigned int allow_textconv;\n-\tunsigned int textconv_set_via_cmdline;\n-\tunsigned int diff_from_contents;\n-\tunsigned int dirty_submodules;\n-\tunsigned int ignore_untracked_in_submodules;\n-\tunsigned int ignore_dirty_submodules;\n-\tunsigned int override_submodule_config;\n-\tunsigned int dirstat_by_line;\n-\tunsigned int funccontext;\n-\tunsigned int default_follow_renames;\n-\tunsigned int stat_with_summary;\n-\tunsigned int suppress_diff_headers;\n-\tunsigned int dual_color_diffed_diffs;\n+\tunsigned recursive;\n+\tunsigned tree_in_recursive;\n+\tunsigned binary;\n+\tunsigned text;\n+\tunsigned full_index;\n+\tunsigned silent_on_remove;\n+\tunsigned find_copies_harder;\n+\tunsigned follow_renames;\n+\tunsigned rename_empty;\n+\tunsigned has_changes;\n+\tunsigned quick;\n+\tunsigned no_index;\n+\tunsigned allow_external;\n+\tunsigned exit_with_status;\n+\tunsigned reverse_diff;\n+\tunsigned check_failed;\n+\tunsigned relative_name;\n+\tunsigned ignore_submodules;\n+\tunsigned dirstat_cumulative;\n+\tunsigned dirstat_by_file;\n+\tunsigned allow_textconv;\n+\tunsigned textconv_set_via_cmdline;\n+\tunsigned diff_from_contents;\n+\tunsigned dirty_submodules;\n+\tunsigned ignore_untracked_in_submodules;\n+\tunsigned ignore_dirty_submodules;\n+\tunsigned override_submodule_config;\n+\tunsigned dirstat_by_line;\n+\tunsigned funccontext;\n+\tunsigned default_follow_renames;\n+\tunsigned stat_with_summary;\n+\tunsigned suppress_diff_headers;\n+\tunsigned dual_color_diffed_diffs;\n };\n \n static inline void diff_flags_or(struct diff_flags *a,\n@@ -151,7 +151,7 @@ struct diff_options {\n \tint skip_stat_unmatch;\n \tint line_termination;\n \tint output_format;\n-\tunsigned int pickaxe_opts;\n+\tunsigned pickaxe_opts;\n \tint rename_score;\n \tint rename_limit;\n \tint needed_rename_limit;\n-- \n2.22.0.rc0.322.g2b0371e29a\n\n"},{"id":"376386","messageId":"20190529091116.21898-2-pclouds@gmail.com","threadId":"51166","inReplyTo":"20190529091116.21898-1-pclouds@gmail.com","subject":"[PATCH v2 1/3] diff-parseopt: correct variable types that are used by parseopt","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-29T09:11:14Z","receivedAt":"2019-05-29T09:11:35Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Most number-related OPT_ macros store the value in an 'int'\nvariable. Many of the variables in 'struct diff_options' have a\ndifferent type, but during the conversion to using parse_options() I\nfailed to notice and correct.\n\nThe problem was reported on s360x which is a big-endian\narchitechture. The variable to store '-w' option in this case is\nxdl_opts, 'long' type, 8 bytes. But since parse_options() assumes\n'int' (4 bytes), it will store bits in the wrong part of xdl_opts. The\nproblem was found on little-endian platforms because parse_options()\nwill accidentally store at the right part of xdl_opts.\n\nThere aren't much to say about the type change (except that 'int' for\nxdl_opts should still be big enough, since Windows' long is the same\nsize as 'int' and nobody has complained so far). Some safety checks may\nbe implemented in the future to prevent class of bugs.\n\nReported-by: Todd Zullinger \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n diff.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff.h b/diff.h\nindex b20cbcc091..d5e44baa96 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -169,7 +169,7 @@ struct diff_options {\n \tconst char *prefix;\n \tint prefix_length;\n \tconst char *stat_sep;\n-\tlong xdl_opts;\n+\tint xdl_opts;\n \n \t/* see Documentation/diff-options.txt */\n \tchar **anchors;\n-- \n2.22.0.rc0.322.g2b0371e29a\n\n"},{"id":"376388","messageId":"20190529091116.21898-3-pclouds@gmail.com","threadId":"51166","inReplyTo":"20190529091116.21898-1-pclouds@gmail.com","subject":"[PATCH v2 2/3] diff-parseopt: restore -U (no argument) behavior","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-29T09:11:15Z","receivedAt":"2019-05-29T09:11:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Before d473e2e0e8 (diff.c: convert -U|--unified, 2019-01-27), -U and\n--unified are implemented with a custom parser opt_arg() in diff.c. I\ndidn't check this code carefully and not realize that it's the\nequivalent of PARSE_OPT_NONEG | PARSE_OPT_OPTARG.\n\nIn other words, if -U is specified without any argument, the option\nshould be accepted, and the default value should be used. Without\nPARSE_OPT_OPTARG, parse_options() will reject this case and cause a\nregression.\n\nReported-by: Bryan Turner \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n diff.c | 10 ++++---\n t/t4013-diff-various.sh | 2 ++\n t/t4013/diff.diff_-U1_initial..side (new) | 29 ++++++++++++++++++++\n t/t4013/diff.diff_-U2_initial..side (new) | 31 ++++++++++++++++++++++\n t/t4013/diff.diff_-U_initial..side (new) | 32 +++++++++++++++++++++++\n 5 files changed, 100 insertions(+), 4 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 4d3cf83a27..80ddc11671 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5211,9 +5211,11 @@ static int diff_opt_unified(const struct option *opt,\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\toptions->context = strtol(arg, &s, 10);\n-\tif (*s)\n-\t\treturn error(_(\"%s expects a numerical value\"), \"--unified\");\n+\tif (arg) {\n+\t\toptions->context = strtol(arg, &s, 10);\n+\t\tif (*s)\n+\t\t\treturn error(_(\"%s expects a numerical value\"), \"--unified\");\n+\t}\n \tenable_patch_output(&options->output_format);\n \n \treturn 0;\n@@ -5272,7 +5274,7 @@ static void prep_parse_options(struct diff_options *options)\n \t\t\t DIFF_FORMAT_PATCH, DIFF_FORMAT_NO_OUTPUT),\n \t\tOPT_CALLBACK_F('U', \"unified\", options, N_(\"\"),\n \t\t\t N_(\"generate diffs with lines context\"),\n-\t\t\t PARSE_OPT_NONEG, diff_opt_unified),\n+\t\t\t PARSE_OPT_NONEG | PARSE_OPT_OPTARG, diff_opt_unified),\n \t\tOPT_BOOL('W', \"function-context\", &options->flags.funccontext,\n \t\t\t N_(\"generate diffs with lines context\")),\n \t\tOPT_BIT_F(0, \"raw\", &options->output_format,\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 9f8f0e84ad..a9054d2db1 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -338,6 +338,8 @@ format-patch --inline --stdout initial..master^^\n format-patch --stdout --cover-letter -n initial..master^\n \n diff --abbrev initial..side\n+diff -U initial..side\n+diff -U1 initial..side\n diff -r initial..side\n diff --stat initial..side\n diff -r --stat initial..side\ndiff --git a/t/t4013/diff.diff_-U1_initial..side b/t/t4013/diff.diff_-U1_initial..side\nnew file mode 100644\nindex 0000000000..b69f8f048a\n--- /dev/null\n+++ b/t/t4013/diff.diff_-U1_initial..side\n@@ -0,0 +1,29 @@\n+$ git diff -U1 initial..side\n+diff --git a/dir/sub b/dir/sub\n+index 35d242b..7289e35 100644\n+--- a/dir/sub\n++++ b/dir/sub\n+@@ -2 +2,3 @@ A\n+ B\n++1\n++2\n+diff --git a/file0 b/file0\n+index 01e79c3..f4615da 100644\n+--- a/file0\n++++ b/file0\n+@@ -3 +3,4 @@\n+ 3\n++A\n++B\n++C\n+diff --git a/file3 b/file3\n+new file mode 100644\n+index 0000000..7289e35\n+--- /dev/null\n++++ b/file3\n+@@ -0,0 +1,4 @@\n++A\n++B\n++1\n++2\n+$\ndiff --git a/t/t4013/diff.diff_-U2_initial..side b/t/t4013/diff.diff_-U2_initial..side\nnew file mode 100644\nindex 0000000000..8ffe04f203\n--- /dev/null\n+++ b/t/t4013/diff.diff_-U2_initial..side\n@@ -0,0 +1,31 @@\n+$ git diff -U2 initial..side\n+diff --git a/dir/sub b/dir/sub\n+index 35d242b..7289e35 100644\n+--- a/dir/sub\n++++ b/dir/sub\n+@@ -1,2 +1,4 @@\n+ A\n+ B\n++1\n++2\n+diff --git a/file0 b/file0\n+index 01e79c3..f4615da 100644\n+--- a/file0\n++++ b/file0\n+@@ -2,2 +2,5 @@\n+ 2\n+ 3\n++A\n++B\n++C\n+diff --git a/file3 b/file3\n+new file mode 100644\n+index 0000000..7289e35\n+--- /dev/null\n++++ b/file3\n+@@ -0,0 +1,4 @@\n++A\n++B\n++1\n++2\n+$\ndiff --git a/t/t4013/diff.diff_-U_initial..side b/t/t4013/diff.diff_-U_initial..side\nnew file mode 100644\nindex 0000000000..c66c0dd5c6\n--- /dev/null\n+++ b/t/t4013/diff.diff_-U_initial..side\n@@ -0,0 +1,32 @@\n+$ git diff -U initial..side\n+diff --git a/dir/sub b/dir/sub\n+index 35d242b..7289e35 100644\n+--- a/dir/sub\n++++ b/dir/sub\n+@@ -1,2 +1,4 @@\n+ A\n+ B\n++1\n++2\n+diff --git a/file0 b/file0\n+index 01e79c3..f4615da 100644\n+--- a/file0\n++++ b/file0\n+@@ -1,3 +1,6 @@\n+ 1\n+ 2\n+ 3\n++A\n++B\n++C\n+diff --git a/file3 b/file3\n+new file mode 100644\n+index 0000000..7289e35\n+--- /dev/null\n++++ b/file3\n+@@ -0,0 +1,4 @@\n++A\n++B\n++1\n++2\n+$\n-- \n2.22.0.rc0.322.g2b0371e29a\n\n"},{"id":"376389","messageId":"20190529091116.21898-4-pclouds@gmail.com","threadId":"51166","inReplyTo":"20190529091116.21898-1-pclouds@gmail.com","subject":"[PATCH v2 3/3] parse-options: check empty value in OPT_INTEGER and OPT_ABBREV","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-29T09:11:16Z","receivedAt":"2019-05-29T09:11:45Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When parsing the argument for OPT_INTEGER and OPT_ABBREV, we check if we\ncan parse the entire argument to a number with \"if (*s)\". There is one\nmissing check: if \"arg\" is empty to begin with, we fail to notice.\n\nThis could happen with long option by writing like\n\n git diff --inter-hunk-context= blah blah\n\nBefore 16ed6c97cc (diff-parseopt: convert --inter-hunk-context,\n2019-03-24), --inter-hunk-context is handled by a custom parser\nopt_arg() and does detect this correctly.\n\nThis restores the bahvior for --inter-hunk-context and make sure all\nother integer options are handled the same (sane) way. For OPT_ABBREV\nthis is new behavior. But it makes it consistent with the rest.\n\nPS. OPT_MAGNITUDE has similar code but git_parse_ulong() does detect\nempty \"arg\". So it's good to go.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n parse-options-cb.c | 3 +++\n parse-options.c | 3 +++\n 2 files changed, 6 insertions(+)\n\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex 4b95d04a37..a3de795c58 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -16,6 +16,9 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)\n \tif (!arg) {\n \t\tv = unset ? 0 : DEFAULT_ABBREV;\n \t} else {\n+\t\tif (!*arg)\n+\t\t\treturn error(_(\"option `%s' expects a numerical value\"),\n+\t\t\t\t opt->long_name);\n \t\tv = strtol(arg, (char **)&arg, 10);\n \t\tif (*arg)\n \t\t\treturn error(_(\"option `%s' expects a numerical value\"),\ndiff --git a/parse-options.c b/parse-options.c\nindex 987e27cb91..87b26a1d92 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -195,6 +195,9 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \t\t}\n \t\tif (get_arg(p, opt, flags, &arg))\n \t\t\treturn -1;\n+\t\tif (!*arg)\n+\t\t\treturn error(_(\"%s expects a numerical value\"),\n+\t\t\t\t optname(opt, flags));\n \t\t*(int *)opt->value = strtol(arg, (char **)&s, 10);\n \t\tif (*s)\n \t\t\treturn error(_(\"%s expects a numerical value\"),\n-- \n2.22.0.rc0.322.g2b0371e29a\n\n"},{"id":"376403","messageId":"CAPig+cQgQcdkY2S3MO-hiLynDOJ9hkkFCtdAf9JQb82aqK1BRQ@mail.gmail.com","threadId":"51166","inReplyTo":"20190529091116.21898-2-pclouds@gmail.com","subject":"Re: [PATCH v2 1/3] diff-parseopt: correct variable types that are used by parseopt","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-05-29T16:43:15Z","receivedAt":"2019-05-29T16:43:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, May 29, 2019 at 5:11 AM Nguyễn Thái Ngọc Duy wrote:\n> Most number-related OPT_ macros store the value in an 'int'\n> variable. Many of the variables in 'struct diff_options' have a\n> different type, but during the conversion to using parse_options() I\n> failed to notice and correct.\n>\n> The problem was reported on s360x which is a big-endian\n> architechture. The variable to store '-w' option in this case is\n> xdl_opts, 'long' type, 8 bytes. But since parse_options() assumes\n> 'int' (4 bytes), it will store bits in the wrong part of xdl_opts. The\n> problem was found on little-endian platforms because parse_options()\n\nDid you mean s/found/not &/ ?\n\n> will accidentally store at the right part of xdl_opts.\n>\n> There aren't much to say about the type change (except that 'int' for\n\ns/aren't/isn't/\n\n> xdl_opts should still be big enough, since Windows' long is the same\n> size as 'int' and nobody has complained so far). Some safety checks may\n> be implemented in the future to prevent class of bugs.\n>\n> Reported-by: Todd Zullinger \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n"},{"id":"376404","messageId":"20190529164755.GE3654@pobox.com","threadId":"51166","inReplyTo":"20190529091116.21898-2-pclouds@gmail.com","subject":"Re: [PATCH v2 1/3] diff-parseopt: correct variable types that are used by parseopt","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-05-29T16:47:55Z","receivedAt":"2019-05-29T16:48:01Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Nguyễn Thái Ngọc Duy wrote:\n> Most number-related OPT_ macros store the value in an 'int'\n> variable. Many of the variables in 'struct diff_options' have a\n> different type, but during the conversion to using parse_options() I\n> failed to notice and correct.\n> \n> The problem was reported on s360x which is a big-endian\n> architechture. The variable to store '-w' option in this case is\n> xdl_opts, 'long' type, 8 bytes. But since parse_options() assumes\n> 'int' (4 bytes), it will store bits in the wrong part of xdl_opts. The\n> problem was found on little-endian platforms because parse_options()\n> will accidentally store at the right part of xdl_opts.\n> \n> There aren't much to say about the type change (except that 'int' for\n> xdl_opts should still be big enough, since Windows' long is the same\n> size as 'int' and nobody has complained so far). Some safety checks may\n> be implemented in the future to prevent class of bugs.\n> \n> Reported-by: Todd Zullinger \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> diff.h | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/diff.h b/diff.h\n> index b20cbcc091..d5e44baa96 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -169,7 +169,7 @@ struct diff_options {\n> \tconst char *prefix;\n> \tint prefix_length;\n> \tconst char *stat_sep;\n> -\tlong xdl_opts;\n> +\tint xdl_opts;\n> \n> \t/* see Documentation/diff-options.txt */\n> \tchar **anchors;\n\nFWIW, I ran this versions of the series through the fedora\nbuildsystem and noticed no issues on s390x or any other\narchitectures.\n\nThanks,\n\n-- \nTodd\n"},{"id":"376410","messageId":"xmqq8supyw6r.fsf@gitster-ct.c.googlers.com","threadId":"51166","inReplyTo":"20190529091116.21898-1-pclouds@gmail.com","subject":"Re: [PATCH v2 0/3] fix diff-parseopt regressions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-29T18:03:56Z","receivedAt":"2019-05-29T18:04:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> v2 reduces diff noise. My C is rusty (and probably holey too). For some\n> reason I remember \"unsigned\" is equivalent to \"unsigned short\", not\n> \"unsigned int\".\n\nFWIW, I do not mind s/unsigned ident/unsigned int ident/ to make the\ntype more explicit as a clean-up at some point. But I do think it\nis a good idea (and I like this v2 because of that) to do that as a\nseparate step, and not mix with the real fix we see in the v2 1/3\npatch.\n\nThanks.\n"},{"id":"376416","messageId":"xmqqv9xtxch9.fsf@gitster-ct.c.googlers.com","threadId":"51166","inReplyTo":"20190529164755.GE3654@pobox.com","subject":"Re: [PATCH v2 1/3] diff-parseopt: correct variable types that are used by parseopt","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-29T19:54:58Z","receivedAt":"2019-05-29T19:55:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Todd Zullinger writes:\n\n> FWIW, I ran this versions of the series through the fedora\n> buildsystem and noticed no issues on s390x or any other\n> architectures.\n>\n> Thanks,\n\nThanks, both.\n"}]}