* [PATCH 1/3] diff-parseopt: correct variable types that are used by parseopt
2019-05-24 9:24 [PATCH 0/3] fix diff-parseopt regressions Nguyễn Thái Ngọc Duy
@ 2019-05-24 9:24 ` Nguyễn Thái Ngọc Duy
2019-05-28 19:23 ` Junio C Hamano
2019-05-24 9:24 ` [PATCH 2/3] diff-parseopt: restore -U (no argument) behavior Nguyễn Thái Ngọc Duy
` (4 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2019-05-24 9:24 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason, bturner,
tmz, Nguyễn Thái Ngọc Duy
Most number-related OPT_ macros store the value in an 'int'
variable. Many of the variables in 'struct diff_options' have a
different type, but during the conversion to using parse_options() I
failed to notice and correct.
The problem was reported on s360x which is a big-endian
architechture. The variable to store '-w' option in this case is
xdl_opts, 'long' type, 8 bytes. But since parse_options() assumes
'int' (4 bytes), it will store bits in the wrong part of xdl_opts. The
problem was found on little-endian platforms because parse_options()
will accidentally store at the right part of xdl_opts.
There aren't much to say about the type change (except that 'int' for
xdl_opts should still be big enough, since Windows' long is the same
size as 'int' and nobody has complained so far). Some safety checks may
be implemented in the future to prevent class of bugs.
Reported-by: Todd Zullinger <tmz@pobox.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
diff.h | 70 +++++++++++++++++++++++++++++-----------------------------
1 file changed, 35 insertions(+), 35 deletions(-)
diff --git a/diff.h b/diff.h
index b20cbcc091..4527daf6b7 100644
--- a/diff.h
+++ b/diff.h
@@ -65,39 +65,39 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
#define DIFF_FLAGS_INIT { 0 }
struct diff_flags {
- unsigned recursive;
- unsigned tree_in_recursive;
- unsigned binary;
- unsigned text;
- unsigned full_index;
- unsigned silent_on_remove;
- unsigned find_copies_harder;
- unsigned follow_renames;
- unsigned rename_empty;
- unsigned has_changes;
- unsigned quick;
- unsigned no_index;
- unsigned allow_external;
- unsigned exit_with_status;
- unsigned reverse_diff;
- unsigned check_failed;
- unsigned relative_name;
- unsigned ignore_submodules;
- unsigned dirstat_cumulative;
- unsigned dirstat_by_file;
- unsigned allow_textconv;
- unsigned textconv_set_via_cmdline;
- unsigned diff_from_contents;
- unsigned dirty_submodules;
- unsigned ignore_untracked_in_submodules;
- unsigned ignore_dirty_submodules;
- unsigned override_submodule_config;
- unsigned dirstat_by_line;
- unsigned funccontext;
- unsigned default_follow_renames;
- unsigned stat_with_summary;
- unsigned suppress_diff_headers;
- unsigned dual_color_diffed_diffs;
+ unsigned int recursive;
+ unsigned int tree_in_recursive;
+ unsigned int binary;
+ unsigned int text;
+ unsigned int full_index;
+ unsigned int silent_on_remove;
+ unsigned int find_copies_harder;
+ unsigned int follow_renames;
+ unsigned int rename_empty;
+ unsigned int has_changes;
+ unsigned int quick;
+ unsigned int no_index;
+ unsigned int allow_external;
+ unsigned int exit_with_status;
+ unsigned int reverse_diff;
+ unsigned int check_failed;
+ unsigned int relative_name;
+ unsigned int ignore_submodules;
+ unsigned int dirstat_cumulative;
+ unsigned int dirstat_by_file;
+ unsigned int allow_textconv;
+ unsigned int textconv_set_via_cmdline;
+ unsigned int diff_from_contents;
+ unsigned int dirty_submodules;
+ unsigned int ignore_untracked_in_submodules;
+ unsigned int ignore_dirty_submodules;
+ unsigned int override_submodule_config;
+ unsigned int dirstat_by_line;
+ unsigned int funccontext;
+ unsigned int default_follow_renames;
+ unsigned int stat_with_summary;
+ unsigned int suppress_diff_headers;
+ unsigned int dual_color_diffed_diffs;
};
static inline void diff_flags_or(struct diff_flags *a,
@@ -151,7 +151,7 @@ struct diff_options {
int skip_stat_unmatch;
int line_termination;
int output_format;
- unsigned pickaxe_opts;
+ unsigned int pickaxe_opts;
int rename_score;
int rename_limit;
int needed_rename_limit;
@@ -169,7 +169,7 @@ struct diff_options {
const char *prefix;
int prefix_length;
const char *stat_sep;
- long xdl_opts;
+ int xdl_opts;
/* see Documentation/diff-options.txt */
char **anchors;
--
2.22.0.rc0.322.g2b0371e29a
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 1/3] diff-parseopt: correct variable types that are used by parseopt
2019-05-24 9:24 ` [PATCH 1/3] diff-parseopt: correct variable types that are used by parseopt Nguyễn Thái Ngọc Duy
@ 2019-05-28 19:23 ` Junio C Hamano
0 siblings, 0 replies; 18+ messages in thread
From: Junio C Hamano @ 2019-05-28 19:23 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Ævar Arnfjörð Bjarmason, bturner, tmz
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Most number-related OPT_ macros store the value in an 'int'
> variable. Many of the variables in 'struct diff_options' have a
> different type, but during the conversion to using parse_options() I
> failed to notice and correct.
Why does this patch need to be so noisy? "unsigned identifier" is
the same as "unsigned int identifier", isn't it?
That is, wouldn't this hunk ...
> @@ -169,7 +169,7 @@ struct diff_options {
> const char *prefix;
> int prefix_length;
> const char *stat_sep;
> - long xdl_opts;
> + int xdl_opts;
... the only one that matters?
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/3] diff-parseopt: restore -U (no argument) behavior
2019-05-24 9:24 [PATCH 0/3] fix diff-parseopt regressions Nguyễn Thái Ngọc Duy
2019-05-24 9:24 ` [PATCH 1/3] diff-parseopt: correct variable types that are used by parseopt Nguyễn Thái Ngọc Duy
@ 2019-05-24 9:24 ` Nguyễn Thái Ngọc Duy
2019-05-24 9:24 ` [PATCH 3/3] parse-options: check empty value in OPT_INTEGER and OPT_ABBREV Nguyễn Thái Ngọc Duy
` (3 subsequent siblings)
5 siblings, 0 replies; 18+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2019-05-24 9:24 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason, bturner,
tmz, Nguyễn Thái Ngọc Duy
Before d473e2e0e8 (diff.c: convert -U|--unified, 2019-01-27), -U and
--unified are implemented with a custom parser opt_arg() in diff.c. I
didn't check this code carefully and not realize that it's the
equivalent of PARSE_OPT_NONEG | PARSE_OPT_OPTARG.
In other words, if -U is specified without any argument, the option
should be accepted, and the default value should be used. Without
PARSE_OPT_OPTARG, parse_options() will reject this case and cause a
regression.
Reported-by: Bryan Turner <bturner@atlassian.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
diff.c | 10 ++++---
t/t4013-diff-various.sh | 2 ++
t/t4013/diff.diff_-U1_initial..side (new) | 29 ++++++++++++++++++++
t/t4013/diff.diff_-U2_initial..side (new) | 31 ++++++++++++++++++++++
t/t4013/diff.diff_-U_initial..side (new) | 32 +++++++++++++++++++++++
5 files changed, 100 insertions(+), 4 deletions(-)
diff --git a/diff.c b/diff.c
index 4d3cf83a27..80ddc11671 100644
--- a/diff.c
+++ b/diff.c
@@ -5211,9 +5211,11 @@ static int diff_opt_unified(const struct option *opt,
BUG_ON_OPT_NEG(unset);
- options->context = strtol(arg, &s, 10);
- if (*s)
- return error(_("%s expects a numerical value"), "--unified");
+ if (arg) {
+ options->context = strtol(arg, &s, 10);
+ if (*s)
+ return error(_("%s expects a numerical value"), "--unified");
+ }
enable_patch_output(&options->output_format);
return 0;
@@ -5272,7 +5274,7 @@ static void prep_parse_options(struct diff_options *options)
DIFF_FORMAT_PATCH, DIFF_FORMAT_NO_OUTPUT),
OPT_CALLBACK_F('U', "unified", options, N_("<n>"),
N_("generate diffs with <n> lines context"),
- PARSE_OPT_NONEG, diff_opt_unified),
+ PARSE_OPT_NONEG | PARSE_OPT_OPTARG, diff_opt_unified),
OPT_BOOL('W', "function-context", &options->flags.funccontext,
N_("generate diffs with <n> lines context")),
OPT_BIT_F(0, "raw", &options->output_format,
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 9f8f0e84ad..a9054d2db1 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -338,6 +338,8 @@ format-patch --inline --stdout initial..master^^
format-patch --stdout --cover-letter -n initial..master^
diff --abbrev initial..side
+diff -U initial..side
+diff -U1 initial..side
diff -r initial..side
diff --stat initial..side
diff -r --stat initial..side
diff --git a/t/t4013/diff.diff_-U1_initial..side b/t/t4013/diff.diff_-U1_initial..side
new file mode 100644
index 0000000000..b69f8f048a
--- /dev/null
+++ b/t/t4013/diff.diff_-U1_initial..side
@@ -0,0 +1,29 @@
+$ git diff -U1 initial..side
+diff --git a/dir/sub b/dir/sub
+index 35d242b..7289e35 100644
+--- a/dir/sub
++++ b/dir/sub
+@@ -2 +2,3 @@ A
+ B
++1
++2
+diff --git a/file0 b/file0
+index 01e79c3..f4615da 100644
+--- a/file0
++++ b/file0
+@@ -3 +3,4 @@
+ 3
++A
++B
++C
+diff --git a/file3 b/file3
+new file mode 100644
+index 0000000..7289e35
+--- /dev/null
++++ b/file3
+@@ -0,0 +1,4 @@
++A
++B
++1
++2
+$
diff --git a/t/t4013/diff.diff_-U2_initial..side b/t/t4013/diff.diff_-U2_initial..side
new file mode 100644
index 0000000000..8ffe04f203
--- /dev/null
+++ b/t/t4013/diff.diff_-U2_initial..side
@@ -0,0 +1,31 @@
+$ git diff -U2 initial..side
+diff --git a/dir/sub b/dir/sub
+index 35d242b..7289e35 100644
+--- a/dir/sub
++++ b/dir/sub
+@@ -1,2 +1,4 @@
+ A
+ B
++1
++2
+diff --git a/file0 b/file0
+index 01e79c3..f4615da 100644
+--- a/file0
++++ b/file0
+@@ -2,2 +2,5 @@
+ 2
+ 3
++A
++B
++C
+diff --git a/file3 b/file3
+new file mode 100644
+index 0000000..7289e35
+--- /dev/null
++++ b/file3
+@@ -0,0 +1,4 @@
++A
++B
++1
++2
+$
diff --git a/t/t4013/diff.diff_-U_initial..side b/t/t4013/diff.diff_-U_initial..side
new file mode 100644
index 0000000000..c66c0dd5c6
--- /dev/null
+++ b/t/t4013/diff.diff_-U_initial..side
@@ -0,0 +1,32 @@
+$ git diff -U initial..side
+diff --git a/dir/sub b/dir/sub
+index 35d242b..7289e35 100644
+--- a/dir/sub
++++ b/dir/sub
+@@ -1,2 +1,4 @@
+ A
+ B
++1
++2
+diff --git a/file0 b/file0
+index 01e79c3..f4615da 100644
+--- a/file0
++++ b/file0
+@@ -1,3 +1,6 @@
+ 1
+ 2
+ 3
++A
++B
++C
+diff --git a/file3 b/file3
+new file mode 100644
+index 0000000..7289e35
+--- /dev/null
++++ b/file3
+@@ -0,0 +1,4 @@
++A
++B
++1
++2
+$
--
2.22.0.rc0.322.g2b0371e29a
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 3/3] parse-options: check empty value in OPT_INTEGER and OPT_ABBREV
2019-05-24 9:24 [PATCH 0/3] fix diff-parseopt regressions Nguyễn Thái Ngọc Duy
2019-05-24 9:24 ` [PATCH 1/3] diff-parseopt: correct variable types that are used by parseopt Nguyễn Thái Ngọc Duy
2019-05-24 9:24 ` [PATCH 2/3] diff-parseopt: restore -U (no argument) behavior Nguyễn Thái Ngọc Duy
@ 2019-05-24 9:24 ` Nguyễn Thái Ngọc Duy
2019-05-24 9:36 ` [PATCH 4/3] parse-options: make compiler check value type mismatch Nguyễn Thái Ngọc Duy
` (2 subsequent siblings)
5 siblings, 0 replies; 18+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2019-05-24 9:24 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason, bturner,
tmz, Nguyễn Thái Ngọc Duy
When parsing the argument for OPT_INTEGER and OPT_ABBREV, we check if we
can parse the entire argument to a number with "if (*s)". There is one
missing check: if "arg" is empty to begin with, we fail to notice.
This could happen with long option by writing like
git diff --inter-hunk-context= blah blah
Before 16ed6c97cc (diff-parseopt: convert --inter-hunk-context,
2019-03-24), --inter-hunk-context is handled by a custom parser
opt_arg() and does detect this correctly.
This restores the bahvior for --inter-hunk-context and make sure all
other integer options are handled the same (sane) way. For OPT_ABBREV
this is new behavior. But it makes it consistent with the rest.
PS. OPT_MAGNITUDE has similar code but git_parse_ulong() does detect
empty "arg". So it's good to go.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
parse-options-cb.c | 3 +++
parse-options.c | 3 +++
2 files changed, 6 insertions(+)
diff --git a/parse-options-cb.c b/parse-options-cb.c
index 4b95d04a37..a3de795c58 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -16,6 +16,9 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)
if (!arg) {
v = unset ? 0 : DEFAULT_ABBREV;
} else {
+ if (!*arg)
+ return error(_("option `%s' expects a numerical value"),
+ opt->long_name);
v = strtol(arg, (char **)&arg, 10);
if (*arg)
return error(_("option `%s' expects a numerical value"),
diff --git a/parse-options.c b/parse-options.c
index 987e27cb91..87b26a1d92 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -195,6 +195,9 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
}
if (get_arg(p, opt, flags, &arg))
return -1;
+ if (!*arg)
+ return error(_("%s expects a numerical value"),
+ optname(opt, flags));
*(int *)opt->value = strtol(arg, (char **)&s, 10);
if (*s)
return error(_("%s expects a numerical value"),
--
2.22.0.rc0.322.g2b0371e29a
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 4/3] parse-options: make compiler check value type mismatch
2019-05-24 9:24 [PATCH 0/3] fix diff-parseopt regressions Nguyễn Thái Ngọc Duy
` (2 preceding siblings ...)
2019-05-24 9:24 ` [PATCH 3/3] parse-options: check empty value in OPT_INTEGER and OPT_ABBREV Nguyễn Thái Ngọc Duy
@ 2019-05-24 9:36 ` Nguyễn Thái Ngọc Duy
2019-05-24 17:36 ` [PATCH 0/3] fix diff-parseopt regressions Todd Zullinger
2019-05-29 9:11 ` [PATCH v2 " Nguyễn Thái Ngọc Duy
5 siblings, 0 replies; 18+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2019-05-24 9:36 UTC (permalink / raw)
To: pclouds; +Cc: avarab, bturner, git, gitster, tmz
There is a disconnect between the actual value type from parse-options
user and the parser itself. This is because we have to store 'value'
pointer as 'void *' and the compiler cannot help point out that the user
is passing a 'long' while the parser expects an 'int'. This could lead
to memory corruption.
In order to spot these type mismatch problems, a dummy inline function
is used to process the 'value' input with the right type, before the
input is stored in 'struct option'. This gives the compiler some context
to start complaining.
The catch though, is that we can only call a function in variable
declaration if it's in automatic scope. Global and static 'struct
option' variables will fail to build after this. But I think this is an
reasonable price to pay, compared to memory corruption.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
For the record, this is what I used to make patch 1/3 (I don't think I
could just rely on manual code inspection to catch these problems)
If we are doing something like this, then we have some clean up to do
first. I think it's worth doing though. But maybe there's a better way?
parse-options.h | 50 ++++++++++++++++++++++++++++++-------------------
1 file changed, 31 insertions(+), 19 deletions(-)
diff --git a/parse-options.h b/parse-options.h
index ac6ba8abf9..b6ea0ac66d 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -128,55 +128,67 @@ struct option {
intptr_t extra;
};
-#define OPT_BIT_F(s, l, v, h, b, f) { OPTION_BIT, (s), (l), (v), NULL, (h), \
+#define DEFINE_OPT_TYPE_CHECK(name, type) \
+ static inline void *_opt_ ## name(type *p) \
+ { \
+ return p; \
+ }
+
+DEFINE_OPT_TYPE_CHECK(int, int)
+DEFINE_OPT_TYPE_CHECK(ulong, unsigned long)
+DEFINE_OPT_TYPE_CHECK(string, const char *)
+DEFINE_OPT_TYPE_CHECK(string_list, struct string_list)
+DEFINE_OPT_TYPE_CHECK(timestamp, timestamp_t)
+
+#define OPT_BIT_F(s, l, v, h, b, f) { OPTION_BIT, (s), (l), _opt_int(v), NULL, (h), \
PARSE_OPT_NOARG|(f), NULL, (b) }
-#define OPT_COUNTUP_F(s, l, v, h, f) { OPTION_COUNTUP, (s), (l), (v), NULL, \
+#define OPT_COUNTUP_F(s, l, v, h, f) { OPTION_COUNTUP, (s), (l), _opt_int(v), NULL, \
(h), PARSE_OPT_NOARG|(f) }
-#define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), (v), NULL, \
+#define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \
(h), PARSE_OPT_NOARG | (f), NULL, (i) }
#define OPT_BOOL_F(s, l, v, h, f) OPT_SET_INT_F(s, l, v, h, 1, f)
#define OPT_CALLBACK_F(s, l, v, a, h, f, cb) \
{ OPTION_CALLBACK, (s), (l), (v), (a), (h), (f), (cb) }
-#define OPT_STRING_F(s, l, v, a, h, f) { OPTION_STRING, (s), (l), (v), (a), (h), (f) }
-#define OPT_INTEGER_F(s, l, v, h, f) { OPTION_INTEGER, (s), (l), (v), N_("n"), (h), (f) }
+#define OPT_STRING_F(s, l, v, a, h, f) { OPTION_STRING, (s), (l), _opt_string(v), (a), (h), (f) }
+#define OPT_INTEGER_F(s, l, v, h, f) { OPTION_INTEGER, (s), (l), _opt_int(v), N_("n"), (h), (f) }
#define OPT_END() { OPTION_END }
-#define OPT_ARGUMENT(l, v, h) { OPTION_ARGUMENT, 0, (l), (v), NULL, \
+#define OPT_ARGUMENT(l, v, h) { OPTION_ARGUMENT, 0, (l), _opt_int(v), NULL, \
(h), PARSE_OPT_NOARG, NULL, 1 }
#define OPT_GROUP(h) { OPTION_GROUP, 0, NULL, NULL, NULL, (h) }
#define OPT_BIT(s, l, v, h, b) OPT_BIT_F(s, l, v, h, b, 0)
-#define OPT_BITOP(s, l, v, h, set, clear) { OPTION_BITOP, (s), (l), (v), NULL, (h), \
+#define OPT_BITOP(s, l, v, h, set, clear) { OPTION_BITOP, (s), (l), _opt_int(v), NULL, (h), \
PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, \
(set), NULL, (clear) }
-#define OPT_NEGBIT(s, l, v, h, b) { OPTION_NEGBIT, (s), (l), (v), NULL, \
+#define OPT_NEGBIT(s, l, v, h, b) { OPTION_NEGBIT, (s), (l), _opt_int(v), NULL, \
(h), PARSE_OPT_NOARG, NULL, (b) }
#define OPT_COUNTUP(s, l, v, h) OPT_COUNTUP_F(s, l, v, h, 0)
#define OPT_SET_INT(s, l, v, h, i) OPT_SET_INT_F(s, l, v, h, i, 0)
#define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)
-#define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
+#define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \
(h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}
-#define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \
+#define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), _opt_int(v), NULL, \
(h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }
#define OPT_INTEGER(s, l, v, h) OPT_INTEGER_F(s, l, v, h, 0)
-#define OPT_MAGNITUDE(s, l, v, h) { OPTION_MAGNITUDE, (s), (l), (v), \
+#define OPT_MAGNITUDE(s, l, v, h) { OPTION_MAGNITUDE, (s), (l), _opt_ulong(v), \
N_("n"), (h), PARSE_OPT_NONEG }
#define OPT_STRING(s, l, v, a, h) OPT_STRING_F(s, l, v, a, h, 0)
#define OPT_STRING_LIST(s, l, v, a, h) \
- { OPTION_CALLBACK, (s), (l), (v), (a), \
+ { OPTION_CALLBACK, (s), (l), _opt_string_list(v), (a), \
(h), 0, &parse_opt_string_list }
-#define OPT_UYN(s, l, v, h) { OPTION_CALLBACK, (s), (l), (v), NULL, \
+#define OPT_UYN(s, l, v, h) { OPTION_CALLBACK, (s), (l), _opt_int(v), NULL, \
(h), PARSE_OPT_NOARG, &parse_opt_tertiary }
#define OPT_EXPIRY_DATE(s, l, v, h) \
- { OPTION_CALLBACK, (s), (l), (v), N_("expiry-date"),(h), 0, \
+ { OPTION_CALLBACK, (s), (l), _opt_timestamp(v), N_("expiry-date"),(h), 0, \
parse_opt_expiry_date_cb }
#define OPT_CALLBACK(s, l, v, a, h, f) OPT_CALLBACK_F(s, l, v, a, h, 0, f)
#define OPT_NUMBER_CALLBACK(v, h, f) \
{ OPTION_NUMBER, 0, NULL, (v), NULL, (h), \
PARSE_OPT_NOARG | PARSE_OPT_NONEG, (f) }
-#define OPT_FILENAME(s, l, v, h) { OPTION_FILENAME, (s), (l), (v), \
+#define OPT_FILENAME(s, l, v, h) { OPTION_FILENAME, (s), (l), _opt_string(v), \
N_("file"), (h) }
#define OPT_COLOR_FLAG(s, l, v, h) \
- { OPTION_CALLBACK, (s), (l), (v), N_("when"), (h), PARSE_OPT_OPTARG, \
+ { OPTION_CALLBACK, (s), (l), _opt_int(v), N_("when"), (h), PARSE_OPT_OPTARG, \
parse_opt_color_flag_cb, (intptr_t)"always" }
#define OPT_NOOP_NOARG(s, l) \
@@ -301,14 +313,14 @@ int parse_opt_passthru_argv(const struct option *, const char *, int);
#define OPT__VERBOSE(var, h) OPT_COUNTUP('v', "verbose", (var), (h))
#define OPT__QUIET(var, h) OPT_COUNTUP('q', "quiet", (var), (h))
#define OPT__VERBOSITY(var) \
- { OPTION_CALLBACK, 'v', "verbose", (var), NULL, N_("be more verbose"), \
+ { OPTION_CALLBACK, 'v', "verbose", _opt_int(var), NULL, N_("be more verbose"), \
PARSE_OPT_NOARG, &parse_opt_verbosity_cb, 0 }, \
- { OPTION_CALLBACK, 'q', "quiet", (var), NULL, N_("be more quiet"), \
+ { OPTION_CALLBACK, 'q', "quiet", _opt_int(var), NULL, N_("be more quiet"), \
PARSE_OPT_NOARG, &parse_opt_verbosity_cb, 0 }
#define OPT__DRY_RUN(var, h) OPT_BOOL('n', "dry-run", (var), (h))
#define OPT__FORCE(var, h, f) OPT_COUNTUP_F('f', "force", (var), (h), (f))
#define OPT__ABBREV(var) \
- { OPTION_CALLBACK, 0, "abbrev", (var), N_("n"), \
+ { OPTION_CALLBACK, 0, "abbrev", _opt_int(var), N_("n"), \
N_("use <n> digits to display SHA-1s"), \
PARSE_OPT_OPTARG, &parse_opt_abbrev_cb, 0 }
#define OPT__COLOR(var, h) \
--
2.22.0.rc0.322.g2b0371e29a
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 0/3] fix diff-parseopt regressions
2019-05-24 9:24 [PATCH 0/3] fix diff-parseopt regressions Nguyễn Thái Ngọc Duy
` (3 preceding siblings ...)
2019-05-24 9:36 ` [PATCH 4/3] parse-options: make compiler check value type mismatch Nguyễn Thái Ngọc Duy
@ 2019-05-24 17:36 ` Todd Zullinger
2019-05-24 20:58 ` Todd Zullinger
2019-05-25 10:22 ` Duy Nguyen
2019-05-29 9:11 ` [PATCH v2 " Nguyễn Thái Ngọc Duy
5 siblings, 2 replies; 18+ messages in thread
From: Todd Zullinger @ 2019-05-24 17:36 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
bturner
Hi,
Nguyễn Thái Ngọc Duy wrote:
> This should fix the diff tests failure on s360x. It's a serious problem
> and I plan to do something to prevent it from happening again.
Thanks for looking at this!
I applied this on top of master/2.22.0-rc1 and see a number
of compiler errors using gcc-9.1.1 with fedora's standard
compiler options for rpm builds.
Below are the compiler errors. This was from an s390x
build, but other arches had the same errors. The complete
build log is available here for a few weeks:
https://kojipkgs.fedoraproject.org//work/tasks/3166/35033166/build.log
cc -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
In file included from credential-store.c:5:
credential-store.c: In function 'cmd_main':
credential-store.c:156:25: warning: passing argument 1 of '_opt_string' from incompatible pointer type [-Wincompatible-pointer-types]
156 | OPT_STRING(0, "file", &file, "path",
| ^~~~~
| |
| char **
parse-options.h:152:82: note: in definition of macro 'OPT_STRING_F'
152 | #define OPT_STRING_F(s, l, v, a, h, f) { OPTION_STRING, (s), (l), _opt_string(v), (a), (h), (f) }
| ^
credential-store.c:156:3: note: in expansion of macro 'OPT_STRING'
156 | OPT_STRING(0, "file", &file, "path",
| ^~~~~~~~~~
parse-options.h:132:42: note: expected 'const char **' but argument is of type 'char **'
132 | static inline void *_opt_ ## name(type *p) \
| ^
parse-options.h:139:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'
139 | DEFINE_OPT_TYPE_CHECK(string, const char *)
| ^~~~~~~~~~~~~~~~~~~~~
* new link flags
cc -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
In file included from apply.c:20:
apply.c: In function 'apply_parse_options':
apply.c:5002:26: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]
5002 | OPT_INTEGER('C', NULL, &state->p_context,
| ^~~~~~~~~~~~~~~~~
| |
| unsigned int *
parse-options.h:153:79: note: in definition of macro 'OPT_INTEGER_F'
153 | #define OPT_INTEGER_F(s, l, v, h, f) { OPTION_INTEGER, (s), (l), _opt_int(v), N_("n"), (h), (f) }
| ^
apply.c:5002:3: note: in expansion of macro 'OPT_INTEGER'
5002 | OPT_INTEGER('C', NULL, &state->p_context,
| ^~~~~~~~~~~
parse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'
132 | static inline void *_opt_ ## name(type *p) \
| ^
parse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'
137 | DEFINE_OPT_TYPE_CHECK(int, int)
| ^~~~~~~~~~~~~~~~~~~~~
cc -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
In file included from diff.c:26:
diff.c: In function 'prep_parse_options':
diff.c:5278:37: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]
5278 | OPT_BOOL('W', "function-context", &options->flags.funccontext,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| unsigned int *
parse-options.h:147:78: note: in definition of macro 'OPT_SET_INT_F'
147 | #define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \
| ^
parse-options.h:167:37: note: in expansion of macro 'OPT_BOOL_F'
167 | #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)
| ^~~~~~~~~~
diff.c:5278:3: note: in expansion of macro 'OPT_BOOL'
5278 | OPT_BOOL('W', "function-context", &options->flags.funccontext,
| ^~~~~~~~
parse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'
132 | static inline void *_opt_ ## name(type *p) \
| ^
parse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'
137 | DEFINE_OPT_TYPE_CHECK(int, int)
| ^~~~~~~~~~~~~~~~~~~~~
diff.c:5342:29: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]
5342 | OPT_BOOL(0, "full-index", &options->flags.full_index,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| unsigned int *
parse-options.h:147:78: note: in definition of macro 'OPT_SET_INT_F'
147 | #define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \
| ^
parse-options.h:167:37: note: in expansion of macro 'OPT_BOOL_F'
167 | #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)
| ^~~~~~~~~~
diff.c:5342:3: note: in expansion of macro 'OPT_BOOL'
5342 | OPT_BOOL(0, "full-index", &options->flags.full_index,
| ^~~~~~~~
parse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'
132 | static inline void *_opt_ ## name(type *p) \
| ^
parse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'
137 | DEFINE_OPT_TYPE_CHECK(int, int)
| ^~~~~~~~~~~~~~~~~~~~~
diff.c:5400:37: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]
5400 | OPT_BOOL(0, "find-copies-harder", &options->flags.find_copies_harder,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| unsigned int *
parse-options.h:147:78: note: in definition of macro 'OPT_SET_INT_F'
147 | #define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \
| ^
parse-options.h:167:37: note: in expansion of macro 'OPT_BOOL_F'
167 | #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)
| ^~~~~~~~~~
diff.c:5400:3: note: in expansion of macro 'OPT_BOOL'
5400 | OPT_BOOL(0, "find-copies-harder", &options->flags.find_copies_harder,
| ^~~~~~~~
parse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'
132 | static inline void *_opt_ ## name(type *p) \
| ^
parse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'
137 | DEFINE_OPT_TYPE_CHECK(int, int)
| ^~~~~~~~~~~~~~~~~~~~~
diff.c:5405:31: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]
5405 | OPT_BOOL(0, "rename-empty", &options->flags.rename_empty,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| unsigned int *
parse-options.h:147:78: note: in definition of macro 'OPT_SET_INT_F'
147 | #define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \
| ^
parse-options.h:167:37: note: in expansion of macro 'OPT_BOOL_F'
167 | #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)
| ^~~~~~~~~~
diff.c:5405:3: note: in expansion of macro 'OPT_BOOL'
5405 | OPT_BOOL(0, "rename-empty", &options->flags.rename_empty,
| ^~~~~~~~
parse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'
132 | static inline void *_opt_ ## name(type *p) \
| ^
parse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'
137 | DEFINE_OPT_TYPE_CHECK(int, int)
| ^~~~~~~~~~~~~~~~~~~~~
diff.c:5469:25: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]
5469 | OPT_BOOL('a', "text", &options->flags.text,
| ^~~~~~~~~~~~~~~~~~~~
| |
| unsigned int *
parse-options.h:147:78: note: in definition of macro 'OPT_SET_INT_F'
147 | #define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \
| ^
parse-options.h:167:37: note: in expansion of macro 'OPT_BOOL_F'
167 | #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)
| ^~~~~~~~~~
diff.c:5469:3: note: in expansion of macro 'OPT_BOOL'
5469 | OPT_BOOL('a', "text", &options->flags.text,
| ^~~~~~~~
parse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'
132 | static inline void *_opt_ ## name(type *p) \
| ^
parse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'
137 | DEFINE_OPT_TYPE_CHECK(int, int)
| ^~~~~~~~~~~~~~~~~~~~~
diff.c:5471:23: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]
5471 | OPT_BOOL('R', NULL, &options->flags.reverse_diff,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| unsigned int *
parse-options.h:147:78: note: in definition of macro 'OPT_SET_INT_F'
147 | #define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \
| ^
parse-options.h:167:37: note: in expansion of macro 'OPT_BOOL_F'
167 | #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)
| ^~~~~~~~~~
diff.c:5471:3: note: in expansion of macro 'OPT_BOOL'
5471 | OPT_BOOL('R', NULL, &options->flags.reverse_diff,
| ^~~~~~~~
parse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'
132 | static inline void *_opt_ ## name(type *p) \
| ^
parse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'
137 | DEFINE_OPT_TYPE_CHECK(int, int)
| ^~~~~~~~~~~~~~~~~~~~~
diff.c:5473:28: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]
5473 | OPT_BOOL(0, "exit-code", &options->flags.exit_with_status,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| unsigned int *
parse-options.h:147:78: note: in definition of macro 'OPT_SET_INT_F'
147 | #define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \
| ^
parse-options.h:167:37: note: in expansion of macro 'OPT_BOOL_F'
167 | #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)
| ^~~~~~~~~~
diff.c:5473:3: note: in expansion of macro 'OPT_BOOL'
5473 | OPT_BOOL(0, "exit-code", &options->flags.exit_with_status,
| ^~~~~~~~
parse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'
132 | static inline void *_opt_ ## name(type *p) \
| ^
parse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'
137 | DEFINE_OPT_TYPE_CHECK(int, int)
| ^~~~~~~~~~~~~~~~~~~~~
diff.c:5475:24: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]
5475 | OPT_BOOL(0, "quiet", &options->flags.quick,
| ^~~~~~~~~~~~~~~~~~~~~
| |
| unsigned int *
parse-options.h:147:78: note: in definition of macro 'OPT_SET_INT_F'
147 | #define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \
| ^
parse-options.h:167:37: note: in expansion of macro 'OPT_BOOL_F'
167 | #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)
| ^~~~~~~~~~
diff.c:5475:3: note: in expansion of macro 'OPT_BOOL'
5475 | OPT_BOOL(0, "quiet", &options->flags.quick,
| ^~~~~~~~
parse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'
132 | static inline void *_opt_ ## name(type *p) \
| ^
parse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'
137 | DEFINE_OPT_TYPE_CHECK(int, int)
| ^~~~~~~~~~~~~~~~~~~~~
diff.c:5477:27: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]
5477 | OPT_BOOL(0, "ext-diff", &options->flags.allow_external,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| unsigned int *
parse-options.h:147:78: note: in definition of macro 'OPT_SET_INT_F'
147 | #define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \
| ^
parse-options.h:167:37: note: in expansion of macro 'OPT_BOOL_F'
167 | #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)
| ^~~~~~~~~~
diff.c:5477:3: note: in expansion of macro 'OPT_BOOL'
5477 | OPT_BOOL(0, "ext-diff", &options->flags.allow_external,
| ^~~~~~~~
parse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'
132 | static inline void *_opt_ ## name(type *p) \
| ^
parse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'
137 | DEFINE_OPT_TYPE_CHECK(int, int)
| ^~~~~~~~~~~~~~~~~~~~~
diff.c:5502:31: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]
5502 | OPT_BIT_F(0, "pickaxe-all", &options->pickaxe_opts,
| ^~~~~~~~~~~~~~~~~~~~~~
| |
| unsigned int *
parse-options.h:143:70: note: in definition of macro 'OPT_BIT_F'
143 | #define OPT_BIT_F(s, l, v, h, b, f) { OPTION_BIT, (s), (l), _opt_int(v), NULL, (h), \
| ^
parse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'
132 | static inline void *_opt_ ## name(type *p) \
| ^
parse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'
137 | DEFINE_OPT_TYPE_CHECK(int, int)
| ^~~~~~~~~~~~~~~~~~~~~
diff.c:5505:33: warning: pointer targets in passing argument 1 of '_opt_int' differ in signedness [-Wpointer-sign]
5505 | OPT_BIT_F(0, "pickaxe-regex", &options->pickaxe_opts,
| ^~~~~~~~~~~~~~~~~~~~~~
| |
| unsigned int *
parse-options.h:143:70: note: in definition of macro 'OPT_BIT_F'
143 | #define OPT_BIT_F(s, l, v, h, b, f) { OPTION_BIT, (s), (l), _opt_int(v), NULL, (h), \
| ^
parse-options.h:132:42: note: expected 'int *' but argument is of type 'unsigned int *'
132 | static inline void *_opt_ ## name(type *p) \
| ^
parse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'
137 | DEFINE_OPT_TYPE_CHECK(int, int)
| ^~~~~~~~~~~~~~~~~~~~~
cc -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
In file included from parse-options.c:2:
parse-options.h:140:43: warning: 'struct string_list' declared inside parameter list will not be visible outside of this definition or declaration
140 | DEFINE_OPT_TYPE_CHECK(string_list, struct string_list)
| ^~~~~~~~~~~
parse-options.h:132:36: note: in definition of macro 'DEFINE_OPT_TYPE_CHECK'
132 | static inline void *_opt_ ## name(type *p) \
| ^~~~
cc -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
In file included from parse-options-cb.c:2:
parse-options.h:140:43: warning: 'struct string_list' declared inside parameter list will not be visible outside of this definition or declaration
140 | DEFINE_OPT_TYPE_CHECK(string_list, struct string_list)
| ^~~~~~~~~~~
parse-options.h:132:36: note: in definition of macro 'DEFINE_OPT_TYPE_CHECK'
132 | static inline void *_opt_ ## name(type *p) \
| ^~~~
cc -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
In file included from imap-send.c:29:
parse-options.h:316:37: error: initializer element is not constant
316 | { OPTION_CALLBACK, 'v', "verbose", _opt_int(var), NULL, N_("be more verbose"), \
| ^~~~~~~~
imap-send.c:51:2: note: in expansion of macro 'OPT__VERBOSITY'
51 | OPT__VERBOSITY(&verbosity),
| ^~~~~~~~~~~~~~
parse-options.h:316:37: note: (near initialization for 'imap_send_options[0].value')
316 | { OPTION_CALLBACK, 'v', "verbose", _opt_int(var), NULL, N_("be more verbose"), \
| ^~~~~~~~
imap-send.c:51:2: note: in expansion of macro 'OPT__VERBOSITY'
51 | OPT__VERBOSITY(&verbosity),
| ^~~~~~~~~~~~~~
parse-options.h:318:35: error: initializer element is not constant
318 | { OPTION_CALLBACK, 'q', "quiet", _opt_int(var), NULL, N_("be more quiet"), \
| ^~~~~~~~
imap-send.c:51:2: note: in expansion of macro 'OPT__VERBOSITY'
51 | OPT__VERBOSITY(&verbosity),
| ^~~~~~~~~~~~~~
parse-options.h:318:35: note: (near initialization for 'imap_send_options[1].value')
318 | { OPTION_CALLBACK, 'q', "quiet", _opt_int(var), NULL, N_("be more quiet"), \
| ^~~~~~~~
imap-send.c:51:2: note: in expansion of macro 'OPT__VERBOSITY'
51 | OPT__VERBOSITY(&verbosity),
| ^~~~~~~~~~~~~~
parse-options.h:147:69: error: initializer element is not constant
147 | #define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \
| ^~~~~~~~
parse-options.h:149:37: note: in expansion of macro 'OPT_SET_INT_F'
149 | #define OPT_BOOL_F(s, l, v, h, f) OPT_SET_INT_F(s, l, v, h, 1, f)
| ^~~~~~~~~~~~~
parse-options.h:167:37: note: in expansion of macro 'OPT_BOOL_F'
167 | #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)
| ^~~~~~~~~~
imap-send.c:52:2: note: in expansion of macro 'OPT_BOOL'
52 | OPT_BOOL(0, "curl", &use_curl, "use libcurl to communicate with the IMAP server"),
| ^~~~~~~~
parse-options.h:147:69: note: (near initialization for 'imap_send_options[2].value')
147 | #define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \
| ^~~~~~~~
parse-options.h:149:37: note: in expansion of macro 'OPT_SET_INT_F'
149 | #define OPT_BOOL_F(s, l, v, h, f) OPT_SET_INT_F(s, l, v, h, 1, f)
| ^~~~~~~~~~~~~
parse-options.h:167:37: note: in expansion of macro 'OPT_BOOL_F'
167 | #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)
| ^~~~~~~~~~
imap-send.c:52:2: note: in expansion of macro 'OPT_BOOL'
52 | OPT_BOOL(0, "curl", &use_curl, "use libcurl to communicate with the IMAP server"),
| ^~~~~~~~
Thanks,
--
Todd
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 0/3] fix diff-parseopt regressions
2019-05-24 17:36 ` [PATCH 0/3] fix diff-parseopt regressions Todd Zullinger
@ 2019-05-24 20:58 ` Todd Zullinger
2019-05-25 10:22 ` Duy Nguyen
1 sibling, 0 replies; 18+ messages in thread
From: Todd Zullinger @ 2019-05-24 20:58 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
bturner
I wrote:
> Below are the compiler errors.
Well, to be precise, all but imap-send are warnings rather
than errors.
--
Todd
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] fix diff-parseopt regressions
2019-05-24 17:36 ` [PATCH 0/3] fix diff-parseopt regressions Todd Zullinger
2019-05-24 20:58 ` Todd Zullinger
@ 2019-05-25 10:22 ` Duy Nguyen
2019-05-25 20:48 ` Todd Zullinger
1 sibling, 1 reply; 18+ messages in thread
From: Duy Nguyen @ 2019-05-25 10:22 UTC (permalink / raw)
To: Todd Zullinger
Cc: Git Mailing List, Junio C Hamano,
Ævar Arnfjörð Bjarmason, Bryan Turner
On Sat, May 25, 2019 at 12:36 AM Todd Zullinger <tmz@pobox.com> wrote:
>
> Hi,
>
> Nguyễn Thái Ngọc Duy wrote:
> > This should fix the diff tests failure on s360x. It's a serious problem
> > and I plan to do something to prevent it from happening again.
>
> Thanks for looking at this!
>
> I applied this on top of master/2.22.0-rc1 and see a number
> of compiler errors using gcc-9.1.1 with fedora's standard
> compiler options for rpm builds.
That last patch 4/3 is not meant to be applied. Yes I've seen similar
compiler errors too. We have some cleaning up to do in order to build
with the last one. But I think there are no other serious errors
spotted by the last patch (there's one in builtin/column.c, but I
think nobody uses it much, so we can fix it alter)
--
Duy
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] fix diff-parseopt regressions
2019-05-25 10:22 ` Duy Nguyen
@ 2019-05-25 20:48 ` Todd Zullinger
0 siblings, 0 replies; 18+ messages in thread
From: Todd Zullinger @ 2019-05-25 20:48 UTC (permalink / raw)
To: Duy Nguyen
Cc: Git Mailing List, Junio C Hamano,
Ævar Arnfjörð Bjarmason, Bryan Turner
Duy Nguyen wrote:
> On Sat, May 25, 2019 at 12:36 AM Todd Zullinger <tmz@pobox.com> wrote:
>> I applied this on top of master/2.22.0-rc1 and see a number
>> of compiler errors using gcc-9.1.1 with fedora's standard
>> compiler options for rpm builds.
>
> That last patch 4/3 is not meant to be applied. Yes I've seen similar
> compiler errors too. We have some cleaning up to do in order to build
> with the last one.
D'oh! Sorry for missing that rather obvious part of your
previous message. The tests do indeed all pass on all the
architectures in the Fedora build system.
I'll go dig out my dunce cap from back in grade school. ;)
Thanks again,
--
Todd
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 0/3] fix diff-parseopt regressions
2019-05-24 9:24 [PATCH 0/3] fix diff-parseopt regressions Nguyễn Thái Ngọc Duy
` (4 preceding siblings ...)
2019-05-24 17:36 ` [PATCH 0/3] fix diff-parseopt regressions Todd Zullinger
@ 2019-05-29 9:11 ` Nguyễn Thái Ngọc Duy
2019-05-29 9:11 ` [PATCH v2 1/3] diff-parseopt: correct variable types that are used by parseopt Nguyễn Thái Ngọc Duy
` (3 more replies)
5 siblings, 4 replies; 18+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2019-05-29 9:11 UTC (permalink / raw)
To: pclouds; +Cc: avarab, bturner, git, gitster, tmz
v2 reduces diff noise. My C is rusty (and probably holey too). For some
reason I remember "unsigned" is equivalent to "unsigned short", not
"unsigned int".
Nguyễn Thái Ngọc Duy (3):
diff-parseopt: correct variable types that are used by parseopt
diff-parseopt: restore -U (no argument) behavior
parse-options: check empty value in OPT_INTEGER and OPT_ABBREV
diff.c | 10 ++++---
diff.h | 2 +-
parse-options-cb.c | 3 +++
parse-options.c | 3 +++
t/t4013-diff-various.sh | 2 ++
t/t4013/diff.diff_-U1_initial..side (new) | 29 ++++++++++++++++++++
t/t4013/diff.diff_-U2_initial..side (new) | 31 ++++++++++++++++++++++
t/t4013/diff.diff_-U_initial..side (new) | 32 +++++++++++++++++++++++
8 files changed, 107 insertions(+), 5 deletions(-)
create mode 100644 t/t4013/diff.diff_-U1_initial..side
create mode 100644 t/t4013/diff.diff_-U2_initial..side
create mode 100644 t/t4013/diff.diff_-U_initial..side
Interdiff dựa trên v1:
diff --git a/diff.h b/diff.h
index 4527daf6b7..d5e44baa96 100644
--- a/diff.h
+++ b/diff.h
@@ -65,39 +65,39 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
#define DIFF_FLAGS_INIT { 0 }
struct diff_flags {
- unsigned int recursive;
- unsigned int tree_in_recursive;
- unsigned int binary;
- unsigned int text;
- unsigned int full_index;
- unsigned int silent_on_remove;
- unsigned int find_copies_harder;
- unsigned int follow_renames;
- unsigned int rename_empty;
- unsigned int has_changes;
- unsigned int quick;
- unsigned int no_index;
- unsigned int allow_external;
- unsigned int exit_with_status;
- unsigned int reverse_diff;
- unsigned int check_failed;
- unsigned int relative_name;
- unsigned int ignore_submodules;
- unsigned int dirstat_cumulative;
- unsigned int dirstat_by_file;
- unsigned int allow_textconv;
- unsigned int textconv_set_via_cmdline;
- unsigned int diff_from_contents;
- unsigned int dirty_submodules;
- unsigned int ignore_untracked_in_submodules;
- unsigned int ignore_dirty_submodules;
- unsigned int override_submodule_config;
- unsigned int dirstat_by_line;
- unsigned int funccontext;
- unsigned int default_follow_renames;
- unsigned int stat_with_summary;
- unsigned int suppress_diff_headers;
- unsigned int dual_color_diffed_diffs;
+ unsigned recursive;
+ unsigned tree_in_recursive;
+ unsigned binary;
+ unsigned text;
+ unsigned full_index;
+ unsigned silent_on_remove;
+ unsigned find_copies_harder;
+ unsigned follow_renames;
+ unsigned rename_empty;
+ unsigned has_changes;
+ unsigned quick;
+ unsigned no_index;
+ unsigned allow_external;
+ unsigned exit_with_status;
+ unsigned reverse_diff;
+ unsigned check_failed;
+ unsigned relative_name;
+ unsigned ignore_submodules;
+ unsigned dirstat_cumulative;
+ unsigned dirstat_by_file;
+ unsigned allow_textconv;
+ unsigned textconv_set_via_cmdline;
+ unsigned diff_from_contents;
+ unsigned dirty_submodules;
+ unsigned ignore_untracked_in_submodules;
+ unsigned ignore_dirty_submodules;
+ unsigned override_submodule_config;
+ unsigned dirstat_by_line;
+ unsigned funccontext;
+ unsigned default_follow_renames;
+ unsigned stat_with_summary;
+ unsigned suppress_diff_headers;
+ unsigned dual_color_diffed_diffs;
};
static inline void diff_flags_or(struct diff_flags *a,
@@ -151,7 +151,7 @@ struct diff_options {
int skip_stat_unmatch;
int line_termination;
int output_format;
- unsigned int pickaxe_opts;
+ unsigned pickaxe_opts;
int rename_score;
int rename_limit;
int needed_rename_limit;
--
2.22.0.rc0.322.g2b0371e29a
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v2 1/3] diff-parseopt: correct variable types that are used by parseopt
2019-05-29 9:11 ` [PATCH v2 " Nguyễn Thái Ngọc Duy
@ 2019-05-29 9:11 ` Nguyễn Thái Ngọc Duy
2019-05-29 16:43 ` Eric Sunshine
2019-05-29 16:47 ` Todd Zullinger
2019-05-29 9:11 ` [PATCH v2 2/3] diff-parseopt: restore -U (no argument) behavior Nguyễn Thái Ngọc Duy
` (2 subsequent siblings)
3 siblings, 2 replies; 18+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2019-05-29 9:11 UTC (permalink / raw)
To: pclouds; +Cc: avarab, bturner, git, gitster, tmz
Most number-related OPT_ macros store the value in an 'int'
variable. Many of the variables in 'struct diff_options' have a
different type, but during the conversion to using parse_options() I
failed to notice and correct.
The problem was reported on s360x which is a big-endian
architechture. The variable to store '-w' option in this case is
xdl_opts, 'long' type, 8 bytes. But since parse_options() assumes
'int' (4 bytes), it will store bits in the wrong part of xdl_opts. The
problem was found on little-endian platforms because parse_options()
will accidentally store at the right part of xdl_opts.
There aren't much to say about the type change (except that 'int' for
xdl_opts should still be big enough, since Windows' long is the same
size as 'int' and nobody has complained so far). Some safety checks may
be implemented in the future to prevent class of bugs.
Reported-by: Todd Zullinger <tmz@pobox.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
diff.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/diff.h b/diff.h
index b20cbcc091..d5e44baa96 100644
--- a/diff.h
+++ b/diff.h
@@ -169,7 +169,7 @@ struct diff_options {
const char *prefix;
int prefix_length;
const char *stat_sep;
- long xdl_opts;
+ int xdl_opts;
/* see Documentation/diff-options.txt */
char **anchors;
--
2.22.0.rc0.322.g2b0371e29a
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2 1/3] diff-parseopt: correct variable types that are used by parseopt
2019-05-29 9:11 ` [PATCH v2 1/3] diff-parseopt: correct variable types that are used by parseopt Nguyễn Thái Ngọc Duy
@ 2019-05-29 16:43 ` Eric Sunshine
2019-05-29 16:47 ` Todd Zullinger
1 sibling, 0 replies; 18+ messages in thread
From: Eric Sunshine @ 2019-05-29 16:43 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: Ævar Arnfjörð Bjarmason, Bryan Turner, Git List,
Junio C Hamano, Todd Zullinger
On Wed, May 29, 2019 at 5:11 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> Most number-related OPT_ macros store the value in an 'int'
> variable. Many of the variables in 'struct diff_options' have a
> different type, but during the conversion to using parse_options() I
> failed to notice and correct.
>
> The problem was reported on s360x which is a big-endian
> architechture. The variable to store '-w' option in this case is
> xdl_opts, 'long' type, 8 bytes. But since parse_options() assumes
> 'int' (4 bytes), it will store bits in the wrong part of xdl_opts. The
> problem was found on little-endian platforms because parse_options()
Did you mean s/found/not &/ ?
> will accidentally store at the right part of xdl_opts.
>
> There aren't much to say about the type change (except that 'int' for
s/aren't/isn't/
> xdl_opts should still be big enough, since Windows' long is the same
> size as 'int' and nobody has complained so far). Some safety checks may
> be implemented in the future to prevent class of bugs.
>
> Reported-by: Todd Zullinger <tmz@pobox.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] diff-parseopt: correct variable types that are used by parseopt
2019-05-29 9:11 ` [PATCH v2 1/3] diff-parseopt: correct variable types that are used by parseopt Nguyễn Thái Ngọc Duy
2019-05-29 16:43 ` Eric Sunshine
@ 2019-05-29 16:47 ` Todd Zullinger
2019-05-29 19:54 ` Junio C Hamano
1 sibling, 1 reply; 18+ messages in thread
From: Todd Zullinger @ 2019-05-29 16:47 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: avarab, bturner, git, gitster
Nguyễn Thái Ngọc Duy wrote:
> Most number-related OPT_ macros store the value in an 'int'
> variable. Many of the variables in 'struct diff_options' have a
> different type, but during the conversion to using parse_options() I
> failed to notice and correct.
>
> The problem was reported on s360x which is a big-endian
> architechture. The variable to store '-w' option in this case is
> xdl_opts, 'long' type, 8 bytes. But since parse_options() assumes
> 'int' (4 bytes), it will store bits in the wrong part of xdl_opts. The
> problem was found on little-endian platforms because parse_options()
> will accidentally store at the right part of xdl_opts.
>
> There aren't much to say about the type change (except that 'int' for
> xdl_opts should still be big enough, since Windows' long is the same
> size as 'int' and nobody has complained so far). Some safety checks may
> be implemented in the future to prevent class of bugs.
>
> Reported-by: Todd Zullinger <tmz@pobox.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/diff.h b/diff.h
> index b20cbcc091..d5e44baa96 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -169,7 +169,7 @@ struct diff_options {
> const char *prefix;
> int prefix_length;
> const char *stat_sep;
> - long xdl_opts;
> + int xdl_opts;
>
> /* see Documentation/diff-options.txt */
> char **anchors;
FWIW, I ran this versions of the series through the fedora
buildsystem and noticed no issues on s390x or any other
architectures.
Thanks,
--
Todd
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 2/3] diff-parseopt: restore -U (no argument) behavior
2019-05-29 9:11 ` [PATCH v2 " Nguyễn Thái Ngọc Duy
2019-05-29 9:11 ` [PATCH v2 1/3] diff-parseopt: correct variable types that are used by parseopt Nguyễn Thái Ngọc Duy
@ 2019-05-29 9:11 ` Nguyễn Thái Ngọc Duy
2019-05-29 9:11 ` [PATCH v2 3/3] parse-options: check empty value in OPT_INTEGER and OPT_ABBREV Nguyễn Thái Ngọc Duy
2019-05-29 18:03 ` [PATCH v2 0/3] fix diff-parseopt regressions Junio C Hamano
3 siblings, 0 replies; 18+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2019-05-29 9:11 UTC (permalink / raw)
To: pclouds; +Cc: avarab, bturner, git, gitster, tmz
Before d473e2e0e8 (diff.c: convert -U|--unified, 2019-01-27), -U and
--unified are implemented with a custom parser opt_arg() in diff.c. I
didn't check this code carefully and not realize that it's the
equivalent of PARSE_OPT_NONEG | PARSE_OPT_OPTARG.
In other words, if -U is specified without any argument, the option
should be accepted, and the default value should be used. Without
PARSE_OPT_OPTARG, parse_options() will reject this case and cause a
regression.
Reported-by: Bryan Turner <bturner@atlassian.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
diff.c | 10 ++++---
t/t4013-diff-various.sh | 2 ++
t/t4013/diff.diff_-U1_initial..side (new) | 29 ++++++++++++++++++++
t/t4013/diff.diff_-U2_initial..side (new) | 31 ++++++++++++++++++++++
t/t4013/diff.diff_-U_initial..side (new) | 32 +++++++++++++++++++++++
5 files changed, 100 insertions(+), 4 deletions(-)
diff --git a/diff.c b/diff.c
index 4d3cf83a27..80ddc11671 100644
--- a/diff.c
+++ b/diff.c
@@ -5211,9 +5211,11 @@ static int diff_opt_unified(const struct option *opt,
BUG_ON_OPT_NEG(unset);
- options->context = strtol(arg, &s, 10);
- if (*s)
- return error(_("%s expects a numerical value"), "--unified");
+ if (arg) {
+ options->context = strtol(arg, &s, 10);
+ if (*s)
+ return error(_("%s expects a numerical value"), "--unified");
+ }
enable_patch_output(&options->output_format);
return 0;
@@ -5272,7 +5274,7 @@ static void prep_parse_options(struct diff_options *options)
DIFF_FORMAT_PATCH, DIFF_FORMAT_NO_OUTPUT),
OPT_CALLBACK_F('U', "unified", options, N_("<n>"),
N_("generate diffs with <n> lines context"),
- PARSE_OPT_NONEG, diff_opt_unified),
+ PARSE_OPT_NONEG | PARSE_OPT_OPTARG, diff_opt_unified),
OPT_BOOL('W', "function-context", &options->flags.funccontext,
N_("generate diffs with <n> lines context")),
OPT_BIT_F(0, "raw", &options->output_format,
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 9f8f0e84ad..a9054d2db1 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -338,6 +338,8 @@ format-patch --inline --stdout initial..master^^
format-patch --stdout --cover-letter -n initial..master^
diff --abbrev initial..side
+diff -U initial..side
+diff -U1 initial..side
diff -r initial..side
diff --stat initial..side
diff -r --stat initial..side
diff --git a/t/t4013/diff.diff_-U1_initial..side b/t/t4013/diff.diff_-U1_initial..side
new file mode 100644
index 0000000000..b69f8f048a
--- /dev/null
+++ b/t/t4013/diff.diff_-U1_initial..side
@@ -0,0 +1,29 @@
+$ git diff -U1 initial..side
+diff --git a/dir/sub b/dir/sub
+index 35d242b..7289e35 100644
+--- a/dir/sub
++++ b/dir/sub
+@@ -2 +2,3 @@ A
+ B
++1
++2
+diff --git a/file0 b/file0
+index 01e79c3..f4615da 100644
+--- a/file0
++++ b/file0
+@@ -3 +3,4 @@
+ 3
++A
++B
++C
+diff --git a/file3 b/file3
+new file mode 100644
+index 0000000..7289e35
+--- /dev/null
++++ b/file3
+@@ -0,0 +1,4 @@
++A
++B
++1
++2
+$
diff --git a/t/t4013/diff.diff_-U2_initial..side b/t/t4013/diff.diff_-U2_initial..side
new file mode 100644
index 0000000000..8ffe04f203
--- /dev/null
+++ b/t/t4013/diff.diff_-U2_initial..side
@@ -0,0 +1,31 @@
+$ git diff -U2 initial..side
+diff --git a/dir/sub b/dir/sub
+index 35d242b..7289e35 100644
+--- a/dir/sub
++++ b/dir/sub
+@@ -1,2 +1,4 @@
+ A
+ B
++1
++2
+diff --git a/file0 b/file0
+index 01e79c3..f4615da 100644
+--- a/file0
++++ b/file0
+@@ -2,2 +2,5 @@
+ 2
+ 3
++A
++B
++C
+diff --git a/file3 b/file3
+new file mode 100644
+index 0000000..7289e35
+--- /dev/null
++++ b/file3
+@@ -0,0 +1,4 @@
++A
++B
++1
++2
+$
diff --git a/t/t4013/diff.diff_-U_initial..side b/t/t4013/diff.diff_-U_initial..side
new file mode 100644
index 0000000000..c66c0dd5c6
--- /dev/null
+++ b/t/t4013/diff.diff_-U_initial..side
@@ -0,0 +1,32 @@
+$ git diff -U initial..side
+diff --git a/dir/sub b/dir/sub
+index 35d242b..7289e35 100644
+--- a/dir/sub
++++ b/dir/sub
+@@ -1,2 +1,4 @@
+ A
+ B
++1
++2
+diff --git a/file0 b/file0
+index 01e79c3..f4615da 100644
+--- a/file0
++++ b/file0
+@@ -1,3 +1,6 @@
+ 1
+ 2
+ 3
++A
++B
++C
+diff --git a/file3 b/file3
+new file mode 100644
+index 0000000..7289e35
+--- /dev/null
++++ b/file3
+@@ -0,0 +1,4 @@
++A
++B
++1
++2
+$
--
2.22.0.rc0.322.g2b0371e29a
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v2 3/3] parse-options: check empty value in OPT_INTEGER and OPT_ABBREV
2019-05-29 9:11 ` [PATCH v2 " Nguyễn Thái Ngọc Duy
2019-05-29 9:11 ` [PATCH v2 1/3] diff-parseopt: correct variable types that are used by parseopt Nguyễn Thái Ngọc Duy
2019-05-29 9:11 ` [PATCH v2 2/3] diff-parseopt: restore -U (no argument) behavior Nguyễn Thái Ngọc Duy
@ 2019-05-29 9:11 ` Nguyễn Thái Ngọc Duy
2019-05-29 18:03 ` [PATCH v2 0/3] fix diff-parseopt regressions Junio C Hamano
3 siblings, 0 replies; 18+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2019-05-29 9:11 UTC (permalink / raw)
To: pclouds; +Cc: avarab, bturner, git, gitster, tmz
When parsing the argument for OPT_INTEGER and OPT_ABBREV, we check if we
can parse the entire argument to a number with "if (*s)". There is one
missing check: if "arg" is empty to begin with, we fail to notice.
This could happen with long option by writing like
git diff --inter-hunk-context= blah blah
Before 16ed6c97cc (diff-parseopt: convert --inter-hunk-context,
2019-03-24), --inter-hunk-context is handled by a custom parser
opt_arg() and does detect this correctly.
This restores the bahvior for --inter-hunk-context and make sure all
other integer options are handled the same (sane) way. For OPT_ABBREV
this is new behavior. But it makes it consistent with the rest.
PS. OPT_MAGNITUDE has similar code but git_parse_ulong() does detect
empty "arg". So it's good to go.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
parse-options-cb.c | 3 +++
parse-options.c | 3 +++
2 files changed, 6 insertions(+)
diff --git a/parse-options-cb.c b/parse-options-cb.c
index 4b95d04a37..a3de795c58 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -16,6 +16,9 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)
if (!arg) {
v = unset ? 0 : DEFAULT_ABBREV;
} else {
+ if (!*arg)
+ return error(_("option `%s' expects a numerical value"),
+ opt->long_name);
v = strtol(arg, (char **)&arg, 10);
if (*arg)
return error(_("option `%s' expects a numerical value"),
diff --git a/parse-options.c b/parse-options.c
index 987e27cb91..87b26a1d92 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -195,6 +195,9 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
}
if (get_arg(p, opt, flags, &arg))
return -1;
+ if (!*arg)
+ return error(_("%s expects a numerical value"),
+ optname(opt, flags));
*(int *)opt->value = strtol(arg, (char **)&s, 10);
if (*s)
return error(_("%s expects a numerical value"),
--
2.22.0.rc0.322.g2b0371e29a
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2 0/3] fix diff-parseopt regressions
2019-05-29 9:11 ` [PATCH v2 " Nguyễn Thái Ngọc Duy
` (2 preceding siblings ...)
2019-05-29 9:11 ` [PATCH v2 3/3] parse-options: check empty value in OPT_INTEGER and OPT_ABBREV Nguyễn Thái Ngọc Duy
@ 2019-05-29 18:03 ` Junio C Hamano
3 siblings, 0 replies; 18+ messages in thread
From: Junio C Hamano @ 2019-05-29 18:03 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: avarab, bturner, git, tmz
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> v2 reduces diff noise. My C is rusty (and probably holey too). For some
> reason I remember "unsigned" is equivalent to "unsigned short", not
> "unsigned int".
FWIW, I do not mind s/unsigned ident/unsigned int ident/ to make the
type more explicit as a clean-up at some point. But I do think it
is a good idea (and I like this v2 because of that) to do that as a
separate step, and not mix with the real fix we see in the v2 1/3
patch.
Thanks.
^ permalink raw reply [flat|nested] 18+ messages in thread