| Message ID | 20201108213838.4880-8-sorganov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-log: implement new --diff-merge options | expand |
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
Junio C Hamano <gitster@pobox.com> writes: > 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). I have no objections against this behavior, nor did I change it, so I don't understand what reasoning you disagree with. The code that handles --first-parent now explicitly says it needs corresponding format of diff output by default, exactly as you describe, so what's the problem? > > 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()". Doesn't sound good to me either. We usually do show merges. We only don't show any kind of diffs for them. > >> + 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"). Well, I think it's better to discuss final names instead of intermediate refactoring ones that will disappear anyway. At least as a fist step. If we get them right, intermediates could be fixed accordingly more easily, I think. [...] Thanks, -- Sergey Organov
diff --git a/builtin/log.c b/builtin/log.c index 77a7bba543ad..a7791c003c91 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -599,7 +599,10 @@ static int show_tree_object(const struct object_id *oid, static void show_setup_revisions_tweak(struct rev_info *rev, struct setup_revision_opt *opt) { - diff_merges_default_to_dense_combined(rev); + if (rev->first_parent_only) + diff_merges_default_to_enable(rev); + else + diff_merges_default_to_dense_combined(rev); if (!rev->diffopt.output_format) rev->diffopt.output_format = DIFF_FORMAT_PATCH; } @@ -724,7 +727,8 @@ static void log_setup_revisions_tweak(struct rev_info *rev, if (!rev->diffopt.output_format && rev->combine_merges) rev->diffopt.output_format = DIFF_FORMAT_PATCH; - diff_merges_first_parent_defaults_to_enable(rev); + if (rev->first_parent_only) + diff_merges_default_to_enable(rev); } int cmd_log(int argc, const char **argv, const char *prefix) diff --git a/diff-merges.c b/diff-merges.c index 85bf0b6d1d1d..9dd472803d15 100644 --- a/diff-merges.c +++ b/diff-merges.c @@ -54,17 +54,15 @@ void diff_merges_setup_revs(struct rev_info *revs) die("--combined-all-paths makes no sense without -c or --cc"); } -void diff_merges_first_parent_defaults_to_enable(struct rev_info *revs) { - if (revs->first_parent_only && revs->ignore_merges < 0) +void diff_merges_default_to_enable(struct rev_info *revs) { + if (revs->ignore_merges < 0) /* No -m */ revs->ignore_merges = 0; } 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
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() to match new semantics. Signed-off-by: Sergey Organov <sorganov@gmail.com> --- builtin/log.c | 8 ++++++-- diff-merges.c | 10 ++++------ diff-merges.h | 2 +- 3 files changed, 11 insertions(+), 9 deletions(-)