Hi Elijah,
Elijah Newren <newren@xxxxxxxxx> writes:
> Hi Sergey,
>
> On Thu, Dec 3, 2020 at 12:06 PM Sergey Organov <sorganov@xxxxxxxxx> wrote:
>>
>> Elijah Newren <newren@xxxxxxxxx> writes:
>>
>> > On Sun, Nov 1, 2020 at 11:36 AM Sergey Organov <sorganov@xxxxxxxxx> 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@xxxxxxxxx>
>> >> ---
>> >> 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.
>
> Ah, I didn't catch the distinction and thus the reasoning in having
> the two different functions.
>
>> Besides, if diff_merges_suppress() is OK with you as interface function
>> name, why suppress() is not OK as local function name in diff-merges
>> module?
>
> Right, the fact that it's in the diff-merges module as opposed to more
> general diff-lib code is probably hint enough. I was skimming over
> the patches looking for things that jumped out, and although I had
> read that you were adding it to the new module, I forgot that when I
> saw the "suppress()" function name. Thanks for the reminder, and
> sorry for the noise.
Fine, thanks, and please don't mention it!
--
Sergey Organov
