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

From: Sergey Organov <sorganov@gmail.com>
Date: Dec 3, 2020, 16:44 UTC
Message-ID: <87k0tyhma7.fsf@osv.gnss.ru>
In-Reply-To: <xmqqa6uvk8mv.fsf@gitster.c.googlers.com>

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

Show 12 quoted lines

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

Show 7 quoted lines

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