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

> Sergey Organov <sorganov@gmail.com> 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).

>
> 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()".

>
>> +	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").