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

Junio C Hamano [off-list ref] writes:

Sergey Organov [off-list ref] writes:

quoted

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