| Message ID | 20201108213838.4880-2-sorganov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-log: implement new --diff-merge options | expand |
Sergey Organov <sorganov@gmail.com> writes: > Move all the parsing code related to diffing merges into new > parse_diff_merge_opts() function. > > Signed-off-by: Sergey Organov <sorganov@gmail.com> > --- > revision.c | 66 ++++++++++++++++++++++++++++++++---------------------- > 1 file changed, 39 insertions(+), 27 deletions(-) > > diff --git a/revision.c b/revision.c > index aa6221204081..a09f4872acd7 100644 > --- a/revision.c > +++ b/revision.c > @@ -2153,6 +2153,44 @@ static void add_message_grep(struct rev_info *revs, const char *pattern) > add_grep(revs, pattern, GREP_PATTERN_BODY); > } > > +static int parse_diff_merge_opts(struct rev_info *revs, const char **argv) { > + int argcount; > + const char *optarg; > + const char *arg = argv[0]; > + > + if (!strcmp(arg, "-m")) { > + /* > + * To "diff-index", "-m" means "match missing", and to the "log" > + * family of commands, it means "show full diff for merges". Set > + * both fields appropriately. > + */ > + revs->ignore_merges = 0; > + revs->match_missing = 1; > + } else if (!strcmp(arg, "-c")) { > + revs->diff = 1; > + revs->dense_combined_merges = 0; > + revs->combine_merges = 1; > + } else if (!strcmp(arg, "--cc")) { > + revs->diff = 1; > + revs->dense_combined_merges = 1; > + revs->combine_merges = 1; > + } else if (!strcmp(arg, "--no-diff-merges")) { > + revs->ignore_merges = 1; > + } else if (!strcmp(arg, "--combined-all-paths")) { > + revs->diff = 1; > + revs->combined_all_paths = 1; > + } else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) { The reordering of if/else if/ cascade from the original, which serves no apparent purpose, makes reviewing a tad harder than necessary, but the conversion at this point seems wrong. The original allowed parse_long_opt() to potentially return 2 (i.e. "diff-merges" and "off" appear as separate tokens on the command line), but the return value stored in argcount here is discarded, without affecting the return value from this function, which is fed back to the original in handle_revision_opt() below. > + if (!strcmp(optarg, "off")) { > + revs->ignore_merges = 1; > + } else { > + die(_("unknown value for --diff-merges: %s"), optarg); > + } To correct the above bug, it probably is sufficient to add return argcount; here. That will be fed back to ... > + } else > + return 0; > + > + return 1; > +} > @@ -2349,34 +2387,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg > ... > - } else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) { > - if (!strcmp(optarg, "off")) { > - revs->ignore_merges = 1; > - } else { > - die(_("unknown value for --diff-merges: %s"), optarg); > - } > + } else if ((argcount = parse_diff_merge_opts(revs, argv))) { > return argcount; ... this part and cause the number of options and arguments consumed to the caller of handle_revision_opt(). I wonder if we can cover it with a test to prevent a similar mistake in the future? > - } else if (!strcmp(arg, "--no-diff-merges")) { > - revs->ignore_merges = 1; > - } else if (!strcmp(arg, "-c")) { > - revs->diff = 1; > - revs->dense_combined_merges = 0; > - revs->combine_merges = 1; > - } else if (!strcmp(arg, "--combined-all-paths")) { > - revs->diff = 1; > - revs->combined_all_paths = 1; > - } else if (!strcmp(arg, "--cc")) { > - revs->diff = 1; > - revs->dense_combined_merges = 1; > - revs->combine_merges = 1; > } else if (!strcmp(arg, "-v")) { > revs->verbose_header = 1; > } else if (!strcmp(arg, "--pretty")) {
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> Move all the parsing code related to diffing merges into new >> parse_diff_merge_opts() function. >> >> Signed-off-by: Sergey Organov <sorganov@gmail.com> >> --- >> revision.c | 66 ++++++++++++++++++++++++++++++++---------------------- >> 1 file changed, 39 insertions(+), 27 deletions(-) >> >> diff --git a/revision.c b/revision.c >> index aa6221204081..a09f4872acd7 100644 >> --- a/revision.c >> +++ b/revision.c >> @@ -2153,6 +2153,44 @@ static void add_message_grep(struct rev_info *revs, const char *pattern) >> add_grep(revs, pattern, GREP_PATTERN_BODY); >> } >> >> +static int parse_diff_merge_opts(struct rev_info *revs, const char **argv) { >> + int argcount; >> + const char *optarg; >> + const char *arg = argv[0]; >> + >> + if (!strcmp(arg, "-m")) { >> + /* >> + * To "diff-index", "-m" means "match missing", and to the "log" >> + * family of commands, it means "show full diff for merges". Set >> + * both fields appropriately. >> + */ >> + revs->ignore_merges = 0; >> + revs->match_missing = 1; >> + } else if (!strcmp(arg, "-c")) { >> + revs->diff = 1; >> + revs->dense_combined_merges = 0; >> + revs->combine_merges = 1; >> + } else if (!strcmp(arg, "--cc")) { >> + revs->diff = 1; >> + revs->dense_combined_merges = 1; >> + revs->combine_merges = 1; >> + } else if (!strcmp(arg, "--no-diff-merges")) { >> + revs->ignore_merges = 1; >> + } else if (!strcmp(arg, "--combined-all-paths")) { >> + revs->diff = 1; >> + revs->combined_all_paths = 1; >> + } else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) { > > The reordering of if/else if/ cascade from the original, which > serves no apparent purpose, makes reviewing a tad harder than > necessary, but the conversion at this point seems wrong. Sorry for the reordering, I didn't notice it when I cleared up the series. > > The original allowed parse_long_opt() to potentially return 2 > (i.e. "diff-merges" and "off" appear as separate tokens on the > command line), but the return value stored in argcount here is > discarded, without affecting the return value from this function, > which is fed back to the original in handle_revision_opt() below. Nice catch! > >> + if (!strcmp(optarg, "off")) { >> + revs->ignore_merges = 1; >> + } else { >> + die(_("unknown value for --diff-merges: %s"), optarg); >> + } > > To correct the above bug, it probably is sufficient to add > > return argcount; > > here. Right, but not enough. "argcount" should also be set to 1 at the beginning of the function, to avoid returning uninitialized value here. Fixed for the next series. Thanks, -- Sergey Organov
Sergey Organov <sorganov@gmail.com> writes: >>> + if (!strcmp(optarg, "off")) { >>> + revs->ignore_merges = 1; >>> + } else { >>> + die(_("unknown value for --diff-merges: %s"), optarg); >>> + } >> >> To correct the above bug, it probably is sufficient to add >> >> return argcount; >> >> here. > > Right, but not enough. "argcount" should also be set to 1 at the > beginning of the function, to avoid returning uninitialized value here. You seem to be a bit confused. The suggested fix is to ... +static int parse_diff_merge_opts(struct rev_info *revs, const char **argv) { + int argcount; + const char *optarg; + const char *arg = argv[0]; + + if (!strcmp(arg, "-m")) { + /* + * To "diff-index", "-m" means "match missing", and to the "log" + * family of commands, it means "show full diff for merges". Set + * both fields appropriately. + */ + revs->ignore_merges = 0; + revs->match_missing = 1; + } else if (!strcmp(arg, "-c")) { + revs->diff = 1; + revs->dense_combined_merges = 0; + revs->combine_merges = 1; + } else if (!strcmp(arg, "--cc")) { + revs->diff = 1; + revs->dense_combined_merges = 1; + revs->combine_merges = 1; + } else if (!strcmp(arg, "--no-diff-merges")) { + revs->ignore_merges = 1; + } else if (!strcmp(arg, "--combined-all-paths")) { + revs->diff = 1; + revs->combined_all_paths = 1; + } else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) { + if (!strcmp(optarg, "off")) { + revs->ignore_merges = 1; + } else { + die(_("unknown value for --diff-merges: %s"), optarg); + } ... add return argcount; here. We know argcount has a valid value that was returned from parse_long_opt() at this point. Nobody else needs to look at the argcount variable. + } else + return 0; + + return 1; +} Incidentally, that is consistent with the way the original function handled the "diff-merges" option at ... static int handle_revision_opt(struct rev_info *revs, int argc, const char **argv, int *unkc, const char **unkv, const struct setup_revision_opt* opt) { const char *arg = argv[0]; const char *optarg; int argcount; const unsigned hexsz = the_hash_algo->hexsz; ... - revs->ignore_merges = 0; - revs->match_missing = 1; - } else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) { - if (!strcmp(optarg, "off")) { - revs->ignore_merges = 1; - } else { - die(_("unknown value for --diff-merges: %s"), optarg); - } + } else if ((argcount = parse_diff_merge_opts(revs, argv))) { return argcount; ... this point. We asked parse_long_opt() to supply the return value for us, and then returned that to our caller. - } else if (!strcmp(arg, "--no-diff-merges")) { - revs->ignore_merges = 1;
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >>>> + if (!strcmp(optarg, "off")) { >>>> + revs->ignore_merges = 1; >>>> + } else { >>>> + die(_("unknown value for --diff-merges: %s"), optarg); >>>> + } >>> >>> To correct the above bug, it probably is sufficient to add >>> >>> return argcount; >>> >>> here. >> >> Right, but not enough. "argcount" should also be set to 1 at the >> beginning of the function, to avoid returning uninitialized value here. > > You seem to be a bit confused. > > The suggested fix is to ... > > +static int parse_diff_merge_opts(struct rev_info *revs, const char **argv) { > + int argcount; > + const char *optarg; > + const char *arg = argv[0]; > + > + if (!strcmp(arg, "-m")) { > + /* > + * To "diff-index", "-m" means "match missing", and to the "log" > + * family of commands, it means "show full diff for merges". Set > + * both fields appropriately. > + */ > + revs->ignore_merges = 0; > + revs->match_missing = 1; > + } else if (!strcmp(arg, "-c")) { > + revs->diff = 1; > + revs->dense_combined_merges = 0; > + revs->combine_merges = 1; > + } else if (!strcmp(arg, "--cc")) { > + revs->diff = 1; > + revs->dense_combined_merges = 1; > + revs->combine_merges = 1; > + } else if (!strcmp(arg, "--no-diff-merges")) { > + revs->ignore_merges = 1; > + } else if (!strcmp(arg, "--combined-all-paths")) { > + revs->diff = 1; > + revs->combined_all_paths = 1; > + } else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) { > + if (!strcmp(optarg, "off")) { > + revs->ignore_merges = 1; > + } else { > + die(_("unknown value for --diff-merges: %s"), optarg); > + } > > ... add > > return argcount; > > here. We know argcount has a valid value that was returned from > parse_long_opt() at this point. [...] OK, thanks, I now see what you meant, yet I now implemented it slightly differently, as I finally need one point of return from the function with non-zero value. The end result is hopefully the same though. Thanks, -- Sergey Organov
diff --git a/revision.c b/revision.c index aa6221204081..a09f4872acd7 100644 --- a/revision.c +++ b/revision.c @@ -2153,6 +2153,44 @@ static void add_message_grep(struct rev_info *revs, const char *pattern) add_grep(revs, pattern, GREP_PATTERN_BODY); } +static int parse_diff_merge_opts(struct rev_info *revs, const char **argv) { + int argcount; + const char *optarg; + const char *arg = argv[0]; + + if (!strcmp(arg, "-m")) { + /* + * To "diff-index", "-m" means "match missing", and to the "log" + * family of commands, it means "show full diff for merges". Set + * both fields appropriately. + */ + revs->ignore_merges = 0; + revs->match_missing = 1; + } else if (!strcmp(arg, "-c")) { + revs->diff = 1; + revs->dense_combined_merges = 0; + revs->combine_merges = 1; + } else if (!strcmp(arg, "--cc")) { + revs->diff = 1; + revs->dense_combined_merges = 1; + revs->combine_merges = 1; + } else if (!strcmp(arg, "--no-diff-merges")) { + revs->ignore_merges = 1; + } else if (!strcmp(arg, "--combined-all-paths")) { + revs->diff = 1; + revs->combined_all_paths = 1; + } else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) { + if (!strcmp(optarg, "off")) { + revs->ignore_merges = 1; + } else { + die(_("unknown value for --diff-merges: %s"), optarg); + } + } else + return 0; + + return 1; +} + static int handle_revision_opt(struct rev_info *revs, int argc, const char **argv, int *unkc, const char **unkv, const struct setup_revision_opt* opt) @@ -2349,34 +2387,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg revs->diff = 1; revs->diffopt.flags.recursive = 1; revs->diffopt.flags.tree_in_recursive = 1; - } else if (!strcmp(arg, "-m")) { - /* - * To "diff-index", "-m" means "match missing", and to the "log" - * family of commands, it means "show full diff for merges". Set - * both fields appropriately. - */ - revs->ignore_merges = 0; - revs->match_missing = 1; - } else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) { - if (!strcmp(optarg, "off")) { - revs->ignore_merges = 1; - } else { - die(_("unknown value for --diff-merges: %s"), optarg); - } + } else if ((argcount = parse_diff_merge_opts(revs, argv))) { return argcount; - } else if (!strcmp(arg, "--no-diff-merges")) { - revs->ignore_merges = 1; - } else if (!strcmp(arg, "-c")) { - revs->diff = 1; - revs->dense_combined_merges = 0; - revs->combine_merges = 1; - } else if (!strcmp(arg, "--combined-all-paths")) { - revs->diff = 1; - revs->combined_all_paths = 1; - } else if (!strcmp(arg, "--cc")) { - revs->diff = 1; - revs->dense_combined_merges = 1; - revs->combine_merges = 1; } else if (!strcmp(arg, "-v")) { revs->verbose_header = 1; } else if (!strcmp(arg, "--pretty")) {
Move all the parsing code related to diffing merges into new parse_diff_merge_opts() function. Signed-off-by: Sergey Organov <sorganov@gmail.com> --- revision.c | 66 ++++++++++++++++++++++++++++++++---------------------- 1 file changed, 39 insertions(+), 27 deletions(-)