Re: [PATCH v1 04/27] revision: provide implementation for diff merges tweaks
From: Junio C Hamano <hidden>
Date: 2020-12-03 00:52:06
Sergey Organov [off-list ref] 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.
quoted hunk
@@ -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. 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?