threads / patch / 51166

patch, 3 partsfix diff-parseopt regressions

Subject: [PATCH 0/3] fix diff-parseopt regressions

## tl;dr

18 messages between May 24, 2019 and May 29, 2019. Diffs are folded; open one to read it.

replies: 17people: 4as markdown or json

Nguyễn Thái Ngọc Duy· May 24, 2019, 09:24 UTC · lore

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.

The second patch should bring '-U' (no argument) back. Whether it makes sense to accept this behavior is not part of this conversion. We can deal with that later.

The third patch also brings back a corner case behavior of --inter-hunk-context and as a result strengthens OPT_INTEGER() error handling a bit.

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                                    | 70 +++++++++++------------
 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, 141 insertions(+), 39 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
-- 
2.22.0.rc0.322.g2b0371e29a
Nguyễn Thái Ngọc Duy· May 24, 2019, 09:24 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 1/3] diff-parseopt: correct variable types that are used by parseopt

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(-)
Show changes to diff.h +35 −35
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
Junio C Hamano· May 28, 2019, 19:23 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 1/3] diff-parseopt: correct variable types that are used by parseopt

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 ...
Show 6 quoted lines
> @@ -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?
Nguyễn Thái Ngọc Duy· May 24, 2019, 09:24 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 2/3] diff-parseopt: restore -U (no argument) behavior

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(-)
Show changes to 5 files +91 −4

diff.c, t/t4013-diff-various.sh, t/t4013/diff.diff_-U1_initial..side, t/t4013/diff.diff_-U2_initial..side, t/t4013/diff.diff_-U_initial..side

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
Nguyễn Thái Ngọc Duy· May 24, 2019, 09:24 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 3/3] parse-options: check empty value in OPT_INTEGER and OPT_ABBREV

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(+)
Show changes to 2 files +6 −0

parse-options-cb.c, parse-options.c

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
Nguyễn Thái Ngọc Duy· May 24, 2019, 09:36 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 4/3] parse-options: make compiler check value type mismatch

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(-)
Show changes to parse-options.h +31 −19
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
Todd Zullinger· May 24, 2019, 17:36 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 0/3] fix diff-parseopt regressions

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
Duy Nguyen· May 25, 2019, 10:22 UTC · re: Todd Zullinger · lore

Re: [PATCH 0/3] fix diff-parseopt regressions

On Sat, May 25, 2019 at 12:36 AM Todd Zullinger <tmz@pobox.com> wrote:
Show 12 quoted lines
>
> 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
Todd Zullinger· May 25, 2019, 20:48 UTC · re: Duy Nguyen · lore

Re: [PATCH 0/3] fix diff-parseopt regressions

Duy Nguyen wrote:
Show 8 quoted lines
> 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
Nguyễn Thái Ngọc Duy· May 29, 2019, 09:11 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 0/3] fix diff-parseopt regressions

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:
Show changes to diff.h +34 −34
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
Nguyễn Thái Ngọc Duy· May 29, 2019, 09:11 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 1/3] diff-parseopt: correct variable types that are used by parseopt

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(-)
Show changes to diff.h +1 −1
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
Eric Sunshine· May 29, 2019, 16:43 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v2 1/3] diff-parseopt: correct variable types that are used by parseopt

On Wed, May 29, 2019 at 5:11 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
Show 10 quoted lines
> 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/
Show 6 quoted lines
> 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>
Todd Zullinger· May 29, 2019, 16:47 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v2 1/3] diff-parseopt: correct variable types that are used by parseopt

Nguyễn Thái Ngọc Duy wrote:
Show 36 quoted lines
> 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
Junio C Hamano· May 29, 2019, 19:54 UTC · re: Todd Zullinger · lore

Re: [PATCH v2 1/3] diff-parseopt: correct variable types that are used by parseopt

Todd Zullinger <tmz@pobox.com> writes:
Show 5 quoted lines
> FWIW, I ran this versions of the series through the fedora
> buildsystem and noticed no issues on s390x or any other
> architectures.
>
> Thanks,
Thanks, both.
Nguyễn Thái Ngọc Duy· May 29, 2019, 09:11 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 2/3] diff-parseopt: restore -U (no argument) behavior

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(-)
Show changes to 5 files +91 −4

diff.c, t/t4013-diff-various.sh, t/t4013/diff.diff_-U1_initial..side, t/t4013/diff.diff_-U2_initial..side, t/t4013/diff.diff_-U_initial..side

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
Nguyễn Thái Ngọc Duy· May 29, 2019, 09:11 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 3/3] parse-options: check empty value in OPT_INTEGER and OPT_ABBREV

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(+)
Show changes to 2 files +6 −0

parse-options-cb.c, parse-options.c

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
Junio C Hamano· May 29, 2019, 18:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v2 0/3] fix diff-parseopt regressions

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.

← back to recent threads