Re: [PATCH v2 26/33] diff-merges: let new options enable diff without -p

Elijah Newren [off-list ref] writes:

On Fri, Dec 18, 2020 at 6:42 AM Sergey Organov [off-list ref] wrote:

quoted

Elijah Newren [off-list ref] writes:

quoted

On Wed, Dec 16, 2020 at 10:50 AM Sergey Organov [off-list ref] wrote:

quoted

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 <redacted>
---
 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)
 {

quoted

+       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.

An idiom (see my previous answer). I'm fine getting rid of it if you
guys find it weird (that I'm not).

I've never seen this idiom, and we apparently have no uses of it in
the code.  If it were near any code I was editing, I think I'd rip it
out as a preliminary cleanup patch...but maybe others have other
opinions.

Essentially, this is poor man replacement for non-existent:

    switch(s) {
      case "str11": case "str12":
         do_things1(); break;
      case "str21": case "str22":
         do_things2(); break;
      case "str31": case "str32":
         do_things3(); break;
      default:
         default(); break
    }

Notice that all the case statements, except the last one, look the same,
while with bare "if() else" constructs you get the first one looking
special:

    if (!strcmp(s, "str11") || !strcmp(s, "str12"))
       do_things1();
    else if (!strcmp(s, "str21") || !strcmp(s, "str22"))
       do_things2();
    else if (!strcmp(s, "str31") || !strcmp(s, "str32"))
       do_things3();
    else
       default();

Now, fixing it is as easy as:

    if (0);
    else if (!strcmp(s, "str11") || !strcmp(s, "str12"))
       do_things1();
    else if (!strcmp(s, "str21") || !strcmp(s, "str22"))
       do_things2();
    else if (!strcmp(s, "str31") || !strcmp(s, "str32"))
       do_things3();
    else
       default();

and it makes reordering and #ifdefing slightly less painful as well.

Just saying rather, than insisting on adopting or even accepting it for
the Git codebase.

Thanks,
-- Sergey