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

Junio C Hamano [off-list ref] writes:

Sergey Organov [off-list ref] writes:

quoted

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.

quoted

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

quoted

+	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