diff mbox series

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

Message ID 20190524092442.701-2-pclouds@gmail.com (mailing list archive)
State New, archived
Headers show
Series fix diff-parseopt regressions | expand

Commit Message

Duy Nguyen May 24, 2019, 9:24 a.m. UTC
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(-)

Comments

Junio C Hamano May 28, 2019, 7:23 p.m. UTC | #1
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?
diff mbox series

Patch

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;