On Wed, Dec 16, 2020 at 10:50 AM Sergey Organov <sorganov@xxxxxxxxx> wrote:
>
> New options don't have any visible effect unless -p is either given or
> implied, as unlike -c/-cc we don't imply -p with --diff-merges. To fix
> this, this patch adds new functionality by letting new options enable
> output of diffs for merge commits only.
>
> Add 'merges_need_diff' field and set it whenever diff output for merges is
> enabled by any of the new options.
>
> Extend diff output logic accordingly, to output diffs for merges when
> 'merges_need_diff' is set even when no -p has been provided.
>
> Signed-off-by: Sergey Organov <sorganov@xxxxxxxxx>
> ---
> diff-merges.c | 16 ++++++++++------
> log-tree.c | 13 +++++++++----
> revision.h | 1 +
> 3 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/diff-merges.c b/diff-merges.c
> index 725db2312074..ffe20d8daa4a 100644
> --- a/diff-merges.c
> +++ b/diff-merges.c
> @@ -10,6 +10,7 @@ static void suppress(struct rev_info *revs)
> revs->dense_combined_merges = 0;
> revs->combined_all_paths = 0;
> revs->combined_imply_patch = 0;
> + revs->merges_need_diff = 0;
> }
>
> static void set_separate(struct rev_info *revs)
> @@ -51,9 +52,11 @@ static void set_dense_combined(struct rev_info *revs)
>
> static void set_diff_merges(struct rev_info *revs, const char *optarg)
> {
> + if (!strcmp(optarg, "off") || !strcmp(optarg, "none")) {
> + suppress(revs);
> + return;
> + }
> if (0) ;
> - else if (!strcmp(optarg, "off") || !strcmp(optarg, "none"))
> - suppress(revs);
The "if (0) ;" is still really weird.
> else if (!strcmp(optarg, "first") || !strcmp(optarg, "first-parent"))
> set_first_parent(revs);
> else if (!strcmp(optarg, "sep") || !strcmp(optarg, "separate"))
> @@ -64,6 +67,7 @@ static void set_diff_merges(struct rev_info *revs, const char *optarg)
> set_dense_combined(revs);
> else
> die(_("unknown value for --diff-merges: %s"), optarg);
> + revs->merges_need_diff = 1;
I'd put this above the if-else-else block, to make it clearer why you
are returning early for the "off"/"none" case.
> }
>
> /*
> @@ -132,12 +136,12 @@ void diff_merges_setup_revs(struct rev_info *revs)
> revs->first_parent_merges = 0;
> if (revs->combined_all_paths && !revs->combine_merges)
> die("--combined-all-paths makes no sense without -c or --cc");
> - if (revs->combine_merges)
> +
> + if (revs->combined_imply_patch)
> revs->diff = 1;
> - if (revs->combined_imply_patch) {
> - /* Turn --cc/-c into -p --cc/-c when -p was not given */
> +
> + if (revs->combined_imply_patch || revs->merges_need_diff) {
> if (!revs->diffopt.output_format)
> revs->diffopt.output_format = DIFF_FORMAT_PATCH;
> }
> -
The random space changes squashed in here instead of being combined
with the earlier patches that introduced the relevant areas break up
the reading. Would be nice to clean this up.
> }
> diff --git a/log-tree.c b/log-tree.c
> index f9385b1dae6f..67060492ca0a 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -899,15 +899,21 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log
> int showed_log;
> struct commit_list *parents;
> struct object_id *oid;
> + int is_merge;
> + int regulars_need_diff = opt->diff || opt->diffopt.flags.exit_with_status;
So rev_info.diff has changed in meaning from
commits-need-to-show-a-diff, to non-merge-commits-need-to-show-a-diff.
That's a somewhat subtle semantic shift. Perhaps it's worth adding a
comment to the declaration of rev_info.diff to highlight this? (And
perhaps even rename the flag?)
> - if (!opt->diff && !opt->diffopt.flags.exit_with_status)
> + if (!regulars_need_diff && !opt->merges_need_diff)
> return 0;
>
> parse_commit_or_die(commit);
> oid = get_commit_tree_oid(commit);
>
> - /* Root commit? */
> parents = get_saved_parents(opt, commit);
> + is_merge = parents && parents->next;
> + if(!is_merge && !regulars_need_diff)
> + return 0;
> +
> + /* Root commit? */
> if (!parents) {
> if (opt->show_root_diff) {
> diff_root_tree_oid(oid, "", &opt->diffopt);
> @@ -916,8 +922,7 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log
> return !opt->loginfo;
> }
>
> - /* More than one parent? */
> - if (parents->next) {
> + if (is_merge) {
> if (opt->combine_merges)
> return do_diff_combined(opt, commit);
> if (opt->separate_merges) {
> diff --git a/revision.h b/revision.h
> index bfbae526ad6e..494d86142454 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -194,6 +194,7 @@ struct rev_info {
> always_show_header:1,
> /* Diff-merge flags */
> explicit_diff_merges: 1,
> + merges_need_diff: 1,
> separate_merges: 1,
> combine_merges:1,
> combined_all_paths:1,
> --
> 2.25.1
The rest makes sense.
