| Message ID | 20201108213838.4880-5-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: > - if (rev->ignore_merges < 0) { > - /* There was no "-m" variant on the command line */ > - rev->ignore_merges = 0; > - if (!rev->first_parent_only && !rev->combine_merges) { > - /* No "--first-parent", "-c", or "--cc" */ > - rev->combine_merges = 1; > - rev->dense_combined_merges = 1; > - } > - } > + rev_diff_merges_default_to_dense_combined(rev); The name makes sense. "Unless otherwise specified, we do not ignore merges. Further, when we are not doing first-parent traversal, default to dense combined merges, unless told otherwise" is what the code says, and the name of the helper captures it well. > @@ -731,8 +723,7 @@ 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; > > - if (rev->first_parent_only && rev->ignore_merges < 0) > - rev->ignore_merges = 0; > + rev_diff_merges_first_parent_defaults_to_enable(rev); This on the other hand is not so great a name. "When we are doing first-parent traversal, do not exclude merge commits from being shown" is what log_setup_revisions_tweak() does here. "default to enable" is not clear at all what we are enabling. Is your naming convention that "rev_diff_" is the common prefix? What's the significance of "_diff" part? After all, these are about tweaking the setting in the rev_info structure, so how about revinfo_show_merges_in_dense_combined_by_default(rev); revinfo_show_merges_by_default_with_first_parent(rev); perhaps, using just "revinfo_" as the common prefix?
Junio C Hamano <gitster@pobox.com> writes: > Is your naming convention that "rev_diff_" is the common prefix? > What's the significance of "_diff" part? After all, these are about > tweaking the setting in the rev_info structure, so how about > > revinfo_show_merges_in_dense_combined_by_default(rev); > revinfo_show_merges_by_default_with_first_parent(rev); > > perhaps, using just "revinfo_" as the common prefix? Actually, I'd like to take these suggestions back. I do not think these names that describe "what the function does" are good names at all. If we can name them after what they are for, we'd be much better off. Given that one is part of the tweak for "show" and the other part of "log", I presume that one is to set-up default for showing a merge in the context of the "show" command, and the other is the same for the "log" (actually "log -p") command. So perhaps diff_merges_set_default_for_show(revs); diff_merges_set_default_for_log(revs); are good names that tells us where they should be used and what for. The former is to set up the default parameters to handle a merge commit in the context of the "show" command, and the latter is the same for the "log" command. Naming them that way, we can then tweak what should happen for "show" by default without touching the caller if we wanted to (perhaps, "when doing first-parent traversal, really pretend that all the commits we see are single-parent, so show the change relative to its first parent by default without even requiring -m" might be something the latter can learn later). Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Is your naming convention that "rev_diff_" is the common prefix? >> What's the significance of "_diff" part? After all, these are about >> tweaking the setting in the rev_info structure, so how about >> >> revinfo_show_merges_in_dense_combined_by_default(rev); >> revinfo_show_merges_by_default_with_first_parent(rev); >> >> perhaps, using just "revinfo_" as the common prefix? > > Actually, I'd like to take these suggestions back. I do not think > these names that describe "what the function does" are good names at > all. If we can name them after what they are for, we'd be much > better off. > > Given that one is part of the tweak for "show" and the other part of > "log", I presume that one is to set-up default for showing a merge > in the context of the "show" command, and the other is the same for > the "log" (actually "log -p") command. So perhaps > > diff_merges_set_default_for_show(revs); > diff_merges_set_default_for_log(revs); > > are good names that tells us where they should be used and what for. Sorry, here I disagree, see below. > The former is to set up the default parameters to handle a merge > commit in the context of the "show" command, and the latter is the > same for the "log" command. Naming them that way, we can then tweak > what should happen for "show" by default without touching the caller > if we wanted to (perhaps, "when doing first-parent traversal, really > pretend that all the commits we see are single-parent, so show the > change relative to its first parent by default without even > requiring -m" might be something the latter can learn later). I have multiple objections to this: 1. diff-merges, as a module or unit, is designed to provide needed services to callers, not to dictate where exactly these services are to be used. 2. diff-merges, as a module, should not care nor know if "git log" or "git show" (or whatever top-level command), exists at all, so the suggested names are not a good idea to have in the module. 3. The decision how exactly merge commits are to be shown for given user command should better be expressed in that command, not hidden in the implementation of diff-merges module. 4. Giving functions names by their place of call won't allow to sensibly reuse them in other places, if needed, without renaming. Overall, I believe the functions in the diff-merges unit must be called after what they are doing to the format of the diff output for merges, not after the places they are (currently) called at. The functions you suggest, on the other hand, are fine and may have their place, but they are at higher level of abstraction and should not be part of the low-level diff-merges module. Thanks, -- Sergey Organov
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> - if (rev->ignore_merges < 0) { >> - /* There was no "-m" variant on the command line */ >> - rev->ignore_merges = 0; >> - if (!rev->first_parent_only && !rev->combine_merges) { >> - /* No "--first-parent", "-c", or "--cc" */ >> - rev->combine_merges = 1; >> - rev->dense_combined_merges = 1; >> - } >> - } >> + rev_diff_merges_default_to_dense_combined(rev); > > The name makes sense. "Unless otherwise specified, we do not ignore > merges. Further, when we are not doing first-parent traversal, > default to dense combined merges, unless told otherwise" is what the > code says, and the name of the helper captures it well. > >> @@ -731,8 +723,7 @@ 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; >> >> - if (rev->first_parent_only && rev->ignore_merges < 0) >> - rev->ignore_merges = 0; >> + rev_diff_merges_first_parent_defaults_to_enable(rev); > > This on the other hand is not so great a name. "When we are doing > first-parent traversal, do not exclude merge commits from being > shown" is what log_setup_revisions_tweak() does here. "default to > enable" is not clear at all what we are enabling. As this name goes away later, let's relax this one, and focus on the final name? > > Is your naming convention that "rev_diff_" is the common prefix? > What's the significance of "_diff" part? After all, these are about > tweaking the setting in the rev_info structure, so how about > > revinfo_show_merges_in_dense_combined_by_default(rev); > revinfo_show_merges_by_default_with_first_parent(rev); > > perhaps, using just "revinfo_" as the common prefix? The prefixes here were selected just to have some to aid in refactoring, without much thought. As all the prefixes change pretty soon in the series anyway, can we let these be? Thanks, -- Sergey Organov
diff --git a/builtin/log.c b/builtin/log.c index 0a7ed4bef92b..717855a49e90 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -598,15 +598,7 @@ 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) { - if (rev->ignore_merges < 0) { - /* There was no "-m" variant on the command line */ - rev->ignore_merges = 0; - if (!rev->first_parent_only && !rev->combine_merges) { - /* No "--first-parent", "-c", or "--cc" */ - rev->combine_merges = 1; - rev->dense_combined_merges = 1; - } - } + rev_diff_merges_default_to_dense_combined(rev); if (!rev->diffopt.output_format) rev->diffopt.output_format = DIFF_FORMAT_PATCH; } @@ -731,8 +723,7 @@ 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; - if (rev->first_parent_only && rev->ignore_merges < 0) - rev->ignore_merges = 0; + rev_diff_merges_first_parent_defaults_to_enable(rev); } int cmd_log(int argc, const char **argv, const char *prefix) diff --git a/revision.c b/revision.c index bc568fb79778..ce90c2991657 100644 --- a/revision.c +++ b/revision.c @@ -2207,6 +2207,23 @@ static void setup_diff_merges_revs(struct rev_info *revs) die("--combined-all-paths makes no sense without -c or --cc"); } +void rev_diff_merges_first_parent_defaults_to_enable(struct rev_info *revs) { + if (revs->first_parent_only && revs->ignore_merges < 0) + revs->ignore_merges = 0; +} + +void rev_diff_merges_default_to_dense_combined(struct rev_info *revs) { + if (revs->ignore_merges < 0) { + /* There was no "-m" variant on the command line */ + revs->ignore_merges = 0; + if (!revs->first_parent_only && !revs->combine_merges) { + /* No "--first-parent", "-c", or "--cc" */ + revs->combine_merges = 1; + revs->dense_combined_merges = 1; + } + } +} + static int handle_revision_opt(struct rev_info *revs, int argc, const char **argv, int *unkc, const char **unkv, const struct setup_revision_opt* opt) diff --git a/revision.h b/revision.h index f6bf860d19e5..3dd0229f4edc 100644 --- a/revision.h +++ b/revision.h @@ -456,4 +456,7 @@ int rewrite_parents(struct rev_info *revs, */ struct commit_list *get_saved_parents(struct rev_info *revs, const struct commit *commit); +void rev_diff_merges_default_to_dense_combined(struct rev_info *revs); +void rev_diff_merges_first_parent_defaults_to_enable(struct rev_info *revs); + #endif
Use these implementations from show_setup_revisions_tweak() and log_setup_revisions_tweak() in builtin/log.c. This completes moving of management of diff merges parameters to a single place, where we can finally observe them simultaneously. Signed-off-by: Sergey Organov <sorganov@gmail.com> --- builtin/log.c | 13 ++----------- revision.c | 17 +++++++++++++++++ revision.h | 3 +++ 3 files changed, 22 insertions(+), 11 deletions(-)