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

[Junio C Hamano <gitster@pobox.com>]

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

>  void diff_merges_default_to_dense_combined(struct rev_info *revs) {
> -	if (revs->ignore_merges < 0) {
> -		/* There was no "-m" variant on the command line */
> +	if (revs->ignore_merges < 0) {		/* No -m */
>  		revs->ignore_merges = 0;
> -		if (!revs->first_parent_only && !revs->combine_merges) {
> -			/* No "--first-parent", "-c", or "--cc" */
> +		if (!revs->combine_merges) {	/* No -c/--cc" */
>  			revs->combine_merges = 1;
>  			revs->dense_combined_merges = 1;
>  		}
> diff --git a/diff-merges.h b/diff-merges.h
> index 648c410497cb..cf411414898d 100644
> --- a/diff-merges.h
> +++ b/diff-merges.h
> @@ -11,7 +11,7 @@ void diff_merges_setup_revs(struct rev_info *revs);
>  
>  void diff_merges_default_to_dense_combined(struct rev_info *revs);
>  
> -void diff_merges_first_parent_defaults_to_enable(struct rev_info *revs);
> +void diff_merges_default_to_enable(struct rev_info *revs);
>  
>  
>  #endif