Re: [PATCH v1 05/27] revision: move diff merges functions to its own diff-merges.c

[Sergey Organov <sorganov@gmail.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> diff --git a/diff-merges.h b/diff-merges.h
>> new file mode 100644
>> index 000000000000..e0cca3d935d3
>> --- /dev/null
>> +++ b/diff-merges.h
>> @@ -0,0 +1,12 @@
>> +#ifndef DIFF_MERGES_H
>> +#define DIFF_MERGES_H
>
> Describe what "diff-merges" module is about in a comment here.

Sure, will do.

> This matters because in 07/27 the log message complains that the
> first-parent traversal option is checked in the module but it is out
> of place.  Without defining what the "module" is about, the readers
> would have a hard time agreeing with the justification.

Yes, agreed.

>
> My guess is that this is about deciding how a merge commit is shown
> in 'log -p' and 'show' output, comparing the commit with its
> parent(s) in patch output.  With that explained, it may make sense
> for 07/27 to complain that --first-parent that is primarily a
> traversal option does also affect how a merge is shown (I do not
> necessarily agree with the complaint, though)

The is no complaint that --first-parent affects how merge is shown, at
least I didn't ever mean to complain about it.

From me there is a complaint that there is no way (before this series)
to make merge to be shown in the same manner without affecting
traversal, but that's not the problem of --first-parent user-visible
behavior.

I believe that in general, it'd be best that if an option makes more
than 1 thing, it does them by simply implying other options, so that
user is able to fine-tune each aspect of behavior independently.

For example, the design of --first-parent could have been that it simply
implies --follow-first-parent and --diff-merges=first-parent, each of
which do exactly one thing: the first defines how graph traversal is
performed, and the second defines how merge commits are shown.

OTOH, the resulting design of --first-parent after this series is an
example of another approach, also acceptable, where an option performs
its 1 primary action directly, and the rest -- by implying other
options. This design also covers all the possibilities, provided each
implicit option could be overridden.

Thanks,
Sergey Organov