Junio C Hamano <gitster@xxxxxxxxx> writes:
> Sergey Organov <sorganov@xxxxxxxxx> writes:
>
>> - if (rev->ignore_merges < 0) {
>> - /* There was no "-m" variant on the command line */
>> - rev->ignore_merges = 0;
>> - if (!rev->first_parent_only && !rev->combine_merges) {
>> - /* No "--first-parent", "-c", or "--cc" */
>> - rev->combine_merges = 1;
>> - rev->dense_combined_merges = 1;
>> - }
>> - }
>> + rev_diff_merges_default_to_dense_combined(rev);
>
> The name makes sense. "Unless otherwise specified, we do not ignore
> merges. Further, when we are not doing first-parent traversal,
> default to dense combined merges, unless told otherwise" is what the
> code says, and the name of the helper captures it well.
>
>> @@ -731,8 +723,7 @@ static void log_setup_revisions_tweak(struct rev_info *rev,
>> if (!rev->diffopt.output_format && rev->combine_merges)
>> rev->diffopt.output_format = DIFF_FORMAT_PATCH;
>>
>> - if (rev->first_parent_only && rev->ignore_merges < 0)
>> - rev->ignore_merges = 0;
>> + rev_diff_merges_first_parent_defaults_to_enable(rev);
>
> This on the other hand is not so great a name. "When we are doing
> first-parent traversal, do not exclude merge commits from being
> shown" is what log_setup_revisions_tweak() does here. "default to
> enable" is not clear at all what we are enabling.
As this name goes away later, let's relax this one, and focus on the
final name?
>
> Is your naming convention that "rev_diff_" is the common prefix?
> What's the significance of "_diff" part? After all, these are about
> tweaking the setting in the rev_info structure, so how about
>
> revinfo_show_merges_in_dense_combined_by_default(rev);
> revinfo_show_merges_by_default_with_first_parent(rev);
>
> perhaps, using just "revinfo_" as the common prefix?
The prefixes here were selected just to have some to aid in refactoring,
without much thought. As all the prefixes change pretty soon in the
series anyway, can we let these be?
Thanks,
-- Sergey Organov
