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

>
> Elijah Newren <newren@gmail.com> writes:
>
> > 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;
>
> Do you have better suggestion? suppress_output()? supress_diff()?
>
> > why not put all this code directly in diff_merges_suppress()?
>
> I prefer the style where module implementation functions don't call its
> interface functions, only vice versa.
>
> >
> >> +
> >>  /*
> >>   * 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()?
>
> Yes, it does, but it doesn't mean it will be that way forever. Interface
> function might need to perform additional checks or actions, in general.