Re: [PATCH v1 01/27] revision: factor out parsing of diff-merge related options
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 3, 2020, 00:33 UTC
- Message-ID
- <xmqqv9djk9tv.fsf@gitster.c.googlers.com>
- In-Reply-To
- <20201108213838.4880-2-sorganov@gmail.com>
Sergey Organov <sorganov@gmail.com> writes:
Show 43 quoted lines
> 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.
Show 5 quoted lines
> + 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 ...
Show 5 quoted lines
> + } else > + return 0; > + > + return 1; > +}
Show 10 quoted lines
> @@ -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?
Show 16 quoted lines
> - } 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")) {