Junio C Hamano <gitster@xxxxxxxxx> writes: > Sergey Organov <sorganov@xxxxxxxxx> 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
