Sergey Organov <sorganov@xxxxxxxxx> writes:
> Move all the parsing code related to diffing merges into new
> parse_diff_merge_opts() function.
>
> Signed-off-by: Sergey Organov <sorganov@xxxxxxxxx>
> ---
> 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")) {
