Subject: Re: [PATCH v1 07/27] diff-merges: move checks for first_parent_only out of the module

From: Sergey Organov <[email-masked]>
Date: Thu, 03 Dec 2020 23:22:14 +0300

Junio C Hamano <[email-masked]> writes:

> Sergey Organov <[email-masked]> writes:
>
>> The checks for first_parent_only don't in fact belong to this module,
>> as the primary purpose of this flag is history traversal limiting, so
>> get it out of this module and rename the
>>
>> diff_merges_first_parent_defaults_to_enable()
>>
>> to
>>
>> diff_merges_default_to_enable()
>
> Again, neither is a great name.
>
> More importantly, I do not think that I agree with the reasoning
> given in the first paragraph.  The first-parent-only traversal means
> that we pretend there is no second and later parents, so it makes
> quite a lot of sense that the choice of that option affects how a
> merge commit discovered during the traversal is show by default
> (namely, as if it is just a single-parent commit).

I have no objections against this behavior, nor did I change it, so I
don't understand what reasoning you disagree with.

The code that handles --first-parent now explicitly says it needs
corresponding format of diff output by default, exactly as you describe,
so what's the problem?

>
> If there are other reasons why we want to force the callers to check
> for first-parent option (for example, it may make it easier to tweak
> how each caller makes its decisions separately in later steps of
> this series), the changes proposed by this step may be a right
> solution, so I am not outright opposed to this patch, but the
> rationale given above is not a strong enough justification for the
> change, at least to me.
>
>> +void diff_merges_default_to_enable(struct rev_info *revs) {
>
> Perhaps "show_merges_by_default()".

Doesn't sound good to me either. We usually do show merges. We only
don't show any kind of diffs for them.

>
>> +	if (revs->ignore_merges < 0)		/* No -m */
>>  		revs->ignore_merges = 0;
>
> Perhaps "show_merges_in_cc_by_default()" (or "cc" spelled out as
> "dense_combined").

Well, I think it's better to discuss final names instead of intermediate
refactoring ones that will disappear anyway. At least as a fist step. If
we get them right, intermediates could be fixed accordingly more
easily, I think.

[...]

Thanks,
-- Sergey Organov