Hi Duy,
On Thu, 17 Jan 2019, Nguyễn Thái Ngọc Duy wrote:
> +static int diff_opt_break_rewrites(const struct option *opt,
> + const char *arg, int unset)
> +{
> + int *break_opt = opt->value;
> + int opt1, opt2;
> +
> + BUG_ON_OPT_NEG(unset);
> + if (!arg)
> + arg = "";
> + opt1 = parse_rename_score(&arg);
> + switch (*arg) {
> + case '\0':
> + opt2 = 0;
> + break;
> + case '/':
> + arg++;
> + opt2 = parse_rename_score(&arg);
> + break;
> + }
This code snippet is anywhere in the spectrum between smart, cute, clever,
hard to reason about, and difficult to validate. Granted, Git for Windows
SDK's GCC v7.3.0 seems to be able to figure out (somehow...) that this
does not leave `opt2` uninitialized. But Ubuntu 16.04's default GCC
version (which I believe is v5.3.1) is not.
And likewise, human readers have to spend way too much time thinking about
this. So I would strongly suggest to save everybody and their compiler
some time by squashing this in:
-- snip --
t a/diff.c b/diff.c
index 381259e987a5..855e6ddcb2b9 100644
--- a/diff.c
+++ b/diff.c
@@ -4949,16 +4949,13 @@ static int diff_opt_break_rewrites(const struct option *opt,
const char *arg, int unset)
{
int *break_opt = opt->value;
- int opt1, opt2;
+ int opt1, opt2 = 0;
BUG_ON_OPT_NEG(unset);
if (!arg)
arg = "";
opt1 = parse_rename_score(&arg);
switch (*arg) {
- case '\0':
- opt2 = 0;
- break;
case '/':
arg++;
opt2 = parse_rename_score(&arg);
-- snap --
Not only is the result a lot easier to understand and to reason about, it also
saves 3 lines.
Ciao,
Johannes
P.S.: Please do not send the entire 78 "re-rolled" patches my way, should
you choose to send another iteration of this unsplit patch series, but
just this one. TIA
> + if (*arg != 0)
> + return error(_("%s expects <n>/<m> form"), opt->long_name);
> + *break_opt = opt1 | (opt2 << 16);
> + return 0;
> +}
> +
> static int diff_opt_char(const struct option *opt,
> const char *arg, int unset)
> {
> @@ -5014,6 +5039,12 @@ static void prep_parse_options(struct diff_options *options)
> N_("specify the character to indicate a context instead of ' '"),
> PARSE_OPT_NONEG, diff_opt_char),
>
> + OPT_GROUP(N_("Diff rename options")),
> + OPT_CALLBACK_F('B', "break-rewrites", &options->break_opt, N_("<n>[/<m>]"),
> + N_("break complete rewrite changes into pairs of delete and create"),
> + PARSE_OPT_NONEG | PARSE_OPT_OPTARG,
> + diff_opt_break_rewrites),
> +
> OPT_GROUP(N_("Diff other options")),
> { OPTION_CALLBACK, 0, "output", options, N_("<file>"),
> N_("Output to a specific file"),
> @@ -5047,12 +5078,7 @@ int diff_opt_parse(struct diff_options *options,
> return ac;
>
> /* renames options */
> - if (starts_with(arg, "-B") ||
> - skip_to_optional_arg(arg, "--break-rewrites", NULL)) {
> - if ((options->break_opt = diff_scoreopt_parse(arg)) == -1)
> - return error("invalid argument to -B: %s", arg+2);
> - }
> - else if (starts_with(arg, "-M") ||
> + if (starts_with(arg, "-M") ||
> skip_to_optional_arg(arg, "--find-renames", NULL)) {
> if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)
> return error("invalid argument to -M: %s", arg+2);
> @@ -5331,17 +5357,14 @@ int parse_rename_score(const char **cp_p)
>
> static int diff_scoreopt_parse(const char *opt)
> {
> - int opt1, opt2, cmd;
> + int opt1, cmd;
>
> if (*opt++ != '-')
> return -1;
> cmd = *opt++;
> if (cmd == '-') {
> /* convert the long-form arguments into short-form versions */
> - if (skip_prefix(opt, "break-rewrites", &opt)) {
> - if (*opt == 0 || *opt++ == '=')
> - cmd = 'B';
> - } else if (skip_prefix(opt, "find-copies", &opt)) {
> + if (skip_prefix(opt, "find-copies", &opt)) {
> if (*opt == 0 || *opt++ == '=')
> cmd = 'C';
> } else if (skip_prefix(opt, "find-renames", &opt)) {
> @@ -5349,25 +5372,13 @@ static int diff_scoreopt_parse(const char *opt)
> cmd = 'M';
> }
> }
> - if (cmd != 'M' && cmd != 'C' && cmd != 'B')
> - return -1; /* that is not a -M, -C, or -B option */
> + if (cmd != 'M' && cmd != 'C')
> + return -1; /* that is not a -M, or -C option */
>
> opt1 = parse_rename_score(&opt);
> - if (cmd != 'B')
> - opt2 = 0;
> - else {
> - if (*opt == 0)
> - opt2 = 0;
> - else if (*opt != '/')
> - return -1; /* we expect -B80/99 or -B80 */
> - else {
> - opt++;
> - opt2 = parse_rename_score(&opt);
> - }
> - }
> if (*opt != 0)
> return -1;
> - return opt1 | (opt2 << 16);
> + return opt1;
> }
>
> struct diff_queue_struct diff_queued_diff;
> --
> 2.20.0.482.g66447595a7
>
>
>
