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.
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?
Thanks,
-- Sergey Organov
