Re: [PATCH 10/26] diff-merges: new function diff_merges_suppress()

[Elijah Newren <newren@gmail.com>]

On Sun, Nov 1, 2020 at 11:36 AM Sergey Organov <sorganov@gmail.com> wrote:
>
> This function sets all the relevant flags to disabled state, so that
> no code that checks only one of them get it wrong.
>
> Then we call this new function everywhere where diff merges output
> suppression is needed.
>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  builtin/merge.c |  3 ++-
>  diff-merges.c   | 18 ++++++++++++++----
>  diff-merges.h   |  2 ++
>  fmt-merge-msg.c |  3 ++-
>  4 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 9d5359edc2f7..1f7b69982b40 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -14,6 +14,7 @@
>  #include "lockfile.h"
>  #include "run-command.h"
>  #include "diff.h"
> +#include "diff-merges.h"
>  #include "refs.h"
>  #include "refspec.h"
>  #include "commit.h"
> @@ -400,7 +401,7 @@ static void squash_message(struct commit *commit, struct commit_list *remotehead
>         printf(_("Squash commit -- not updating HEAD\n"));
>
>         repo_init_revisions(the_repository, &rev, NULL);
> -       rev.ignore_merges = 1;
> +       diff_merges_suppress(&rev);
>         rev.commit_format = CMIT_FMT_MEDIUM;
>
>         commit->object.flags |= UNINTERESTING;
> diff --git a/diff-merges.c b/diff-merges.c
> index 8536941e0b56..25bd9b12e667 100644
> --- a/diff-merges.c
> +++ b/diff-merges.c
> @@ -2,6 +2,13 @@
>
>  #include "revision.h"
>
> +static void suppress(struct rev_info *revs) {
> +       revs->ignore_merges = 1;
> +       revs->first_parent_merges = 0;
> +       revs->combine_merges = 0;
> +       revs->dense_combined_merges = 0;
> +}

The function name is not so helpful; why not put all this code
directly in diff_merges_suppress()?

> +
>  /*
>   * Public functions. They are in the order they are called.
>   */
> @@ -29,16 +36,15 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv) {
>                 revs->combine_merges = 1;
>         } else if (!strcmp(arg, "--cc")) {
>                 revs->diff = 1;
> -               revs->dense_combined_merges = 1;
> -               revs->combine_merges = 1;
> +               set_dense_combined(revs);
>         } else if (!strcmp(arg, "--no-diff-merges")) {
> -               revs->ignore_merges = 1;
> +               suppress(revs);
>         } 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;
> +                       suppress(revs);
>                 } else {
>                         die(_("unknown value for --diff-merges: %s"), optarg);
>                 }
> @@ -48,6 +54,10 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv) {
>         return 1;
>  }
>
> +void diff_merges_suppress(struct rev_info *revs) {
> +       suppress(revs);
> +}

...especially since all this function does is call suppress()?


> +
>  void diff_merges_default_to_first_parent(struct rev_info *revs) {
>         if (revs->ignore_merges < 0)            /* No -m */
>                 revs->ignore_merges = 0;
> diff --git a/diff-merges.h b/diff-merges.h
> index 4b023c385d00..32720bc9b40f 100644
> --- a/diff-merges.h
> +++ b/diff-merges.h
> @@ -7,6 +7,8 @@ void diff_merges_init_revs(struct rev_info *revs);
>
>  int diff_merges_parse_opts(struct rev_info *revs, const char **argv);
>
> +void diff_merges_suppress(struct rev_info *revs);
> +
>  void diff_merges_default_to_first_parent(struct rev_info *revs);
>
>  void diff_merges_default_to_dense_combined(struct rev_info *revs);
> diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
> index bd22e1ea8865..abc3403fb820 100644
> --- a/fmt-merge-msg.c
> +++ b/fmt-merge-msg.c
> @@ -2,6 +2,7 @@
>  #include "refs.h"
>  #include "object-store.h"
>  #include "diff.h"
> +#include "diff-merges.h"
>  #include "revision.h"
>  #include "tag.h"
>  #include "string-list.h"
> @@ -668,7 +669,7 @@ int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
>                 head = lookup_commit_or_die(&head_oid, "HEAD");
>                 repo_init_revisions(the_repository, &rev, NULL);
>                 rev.commit_format = CMIT_FMT_ONELINE;
> -               rev.ignore_merges = 1;
> +               diff_merges_suppress(&rev);
>                 rev.limited = 1;
>
>                 strbuf_complete_line(out);
> --
> 2.25.1
>