From 7f125ff9094bfebbb381afa19380943a088c8fcb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?= Date: Wed, 29 May 2019 16:11:14 +0700 Subject: diff-parseopt: correct variable types that are used by parseopt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: Junio C Hamano diff --git a/diff.h b/diff.h index d9ad73f..7b66bf1 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; -- cgit v0.10.2-6-g49f6 From 8ef05193bcdda6a28cd41fa3ecd8f356061d49e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?= Date: Wed, 29 May 2019 16:11:15 +0700 Subject: diff-parseopt: restore -U (no argument) behavior MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: Junio C Hamano diff --git a/diff.c b/diff.c index 4bc9df7..0abab4a 100644 --- a/diff.c +++ b/diff.c @@ -4875,9 +4875,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; @@ -4895,7 +4897,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_("generate diffs with 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 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 7d985ff..3c4a3df 100755 --- a/t/t4013-diff-various.sh +++ b/t/t4013-diff-various.sh @@ -330,6 +330,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 0000000..b69f8f0 --- /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 0000000..8ffe04f --- /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 0000000..c66c0dd --- /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 +$ -- cgit v0.10.2-6-g49f6 From f7e68a08780e91d7c2f830f33457041407172b96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?= Date: Wed, 29 May 2019 16:11:16 +0700 Subject: parse-options: check empty value in OPT_INTEGER and OPT_ABBREV MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano diff --git a/parse-options-cb.c b/parse-options-cb.c index 2733393..0229349 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 cec7452..c8ee221 100644 --- a/parse-options.c +++ b/parse-options.c @@ -193,6 +193,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"), -- cgit v0.10.2-6-g49f6