Junio C Hamano <gitster@xxxxxxxxx> writes:
> Sergey Organov <sorganov@xxxxxxxxx> 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
