{"thread":{"id":"58431","subject":"[PATCH 0/3] diff-merges: minor cleanups","startedAt":"2022-09-14T19:31:18Z","lastAt":"2022-09-16T16:28:37Z","messageCount":13,"participants":["Sergey Organov","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"463021","messageId":"20220914193102.5275-1-sorganov@gmail.com","threadId":"58431","inReplyTo":null,"subject":"[PATCH 0/3] diff-merges: minor cleanups","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-09-14T19:30:59Z","receivedAt":"2022-09-14T19:31:18Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"These are pure cleanups. Expect no changes of behavior.\n\nSigned-off-by: Sergey Organov <sorganov@gmail.com>\n\nSergey Organov (3):\n  diff-merges: cleanup func_by_opt()\n  diff-merges: cleanup set_diff_merges()\n  diff-merges: clarify log.diffMerges documentation\n\n Documentation/config/log.txt |  6 +++---\n diff-merges.c                | 40 +++++++++++++++++++++---------------\n 2 files changed, 27 insertions(+), 19 deletions(-)\n\n-- \n2.25.1\n\n"},{"id":"463022","messageId":"20220914193102.5275-2-sorganov@gmail.com","threadId":"58431","inReplyTo":"20220914193102.5275-1-sorganov@gmail.com","subject":"[PATCH 1/3] diff-merges: cleanup func_by_opt()","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-09-14T19:31:00Z","receivedAt":"2022-09-14T19:31:21Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Get rid of unneeded \"else\" statements in func_by_opt().\n\nSigned-off-by: Sergey Organov <sorganov@gmail.com>\n---\n diff-merges.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/diff-merges.c b/diff-merges.c\nindex 7f64156b8bfe..780ed08fc87f 100644\n--- a/diff-merges.c\n+++ b/diff-merges.c\n@@ -60,15 +60,15 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)\n \t\treturn suppress;\n \tif (!strcmp(optarg, \"1\") || !strcmp(optarg, \"first-parent\"))\n \t\treturn set_first_parent;\n-\telse if (!strcmp(optarg, \"separate\"))\n+\tif (!strcmp(optarg, \"separate\"))\n \t\treturn set_separate;\n-\telse if (!strcmp(optarg, \"c\") || !strcmp(optarg, \"combined\"))\n+\tif (!strcmp(optarg, \"c\") || !strcmp(optarg, \"combined\"))\n \t\treturn set_combined;\n-\telse if (!strcmp(optarg, \"cc\") || !strcmp(optarg, \"dense-combined\"))\n+\tif (!strcmp(optarg, \"cc\") || !strcmp(optarg, \"dense-combined\"))\n \t\treturn set_dense_combined;\n-\telse if (!strcmp(optarg, \"r\") || !strcmp(optarg, \"remerge\"))\n+\tif (!strcmp(optarg, \"r\") || !strcmp(optarg, \"remerge\"))\n \t\treturn set_remerge_diff;\n-\telse if (!strcmp(optarg, \"m\") || !strcmp(optarg, \"on\"))\n+\tif (!strcmp(optarg, \"m\") || !strcmp(optarg, \"on\"))\n \t\treturn set_to_default;\n \treturn NULL;\n }\n-- \n2.25.1\n\n"},{"id":"463023","messageId":"20220914193102.5275-3-sorganov@gmail.com","threadId":"58431","inReplyTo":"20220914193102.5275-1-sorganov@gmail.com","subject":"[PATCH 2/3] diff-merges: cleanup set_diff_merges()","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-09-14T19:31:01Z","receivedAt":"2022-09-14T19:31:24Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Get rid of special-casing of 'suppress' in set_diff_merges(). Instead\nset 'merges_need_diff' flag correctly in every option handling\nfunction.\n\nSigned-off-by: Sergey Organov <sorganov@gmail.com>\n---\n diff-merges.c | 30 +++++++++++++++++++-----------\n 1 file changed, 19 insertions(+), 11 deletions(-)\n\ndiff --git a/diff-merges.c b/diff-merges.c\nindex 780ed08fc87f..85cbefa5afd7 100644\n--- a/diff-merges.c\n+++ b/diff-merges.c\n@@ -20,9 +20,20 @@ static void suppress(struct rev_info *revs)\n \trevs->remerge_diff = 0;\n }\n \n+static void common_setup(struct rev_info *revs)\n+{\n+\tsuppress(revs);\n+\trevs->merges_need_diff = 1;\n+}\n+\n+static void set_none(struct rev_info *revs)\n+{\n+\tsuppress(revs);\n+}\n+\n static void set_separate(struct rev_info *revs)\n {\n-\tsuppress(revs);\n+\tcommon_setup(revs);\n \trevs->separate_merges = 1;\n \trevs->simplify_history = 0;\n }\n@@ -35,21 +46,21 @@ static void set_first_parent(struct rev_info *revs)\n \n static void set_combined(struct rev_info *revs)\n {\n-\tsuppress(revs);\n+\tcommon_setup(revs);\n \trevs->combine_merges = 1;\n \trevs->dense_combined_merges = 0;\n }\n \n static void set_dense_combined(struct rev_info *revs)\n {\n-\tsuppress(revs);\n+\tcommon_setup(revs);\n \trevs->combine_merges = 1;\n \trevs->dense_combined_merges = 1;\n }\n \n static void set_remerge_diff(struct rev_info *revs)\n {\n-\tsuppress(revs);\n+\tcommon_setup(revs);\n \trevs->remerge_diff = 1;\n \trevs->simplify_history = 0;\n }\n@@ -57,7 +68,7 @@ static void set_remerge_diff(struct rev_info *revs)\n static diff_merges_setup_func_t func_by_opt(const char *optarg)\n {\n \tif (!strcmp(optarg, \"off\") || !strcmp(optarg, \"none\"))\n-\t\treturn suppress;\n+\t\treturn set_none;\n \tif (!strcmp(optarg, \"1\") || !strcmp(optarg, \"first-parent\"))\n \t\treturn set_first_parent;\n \tif (!strcmp(optarg, \"separate\"))\n@@ -81,10 +92,6 @@ static void set_diff_merges(struct rev_info *revs, const char *optarg)\n \t\tdie(_(\"invalid value for '%s': '%s'\"), \"--diff-merges\", optarg);\n \n \tfunc(revs);\n-\n-\t/* NOTE: the merges_need_diff flag is cleared by func() call */\n-\tif (func != suppress)\n-\t\trevs->merges_need_diff = 1;\n }\n \n /*\n@@ -115,6 +122,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)\n \n \tif (!suppress_m_parsing && !strcmp(arg, \"-m\")) {\n \t\tset_to_default(revs);\n+\t\trevs->merges_need_diff = 0;\n \t} else if (!strcmp(arg, \"-c\")) {\n \t\tset_combined(revs);\n \t\trevs->merges_imply_patch = 1;\n@@ -125,7 +133,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)\n \t\tset_remerge_diff(revs);\n \t\trevs->merges_imply_patch = 1;\n \t} else if (!strcmp(arg, \"--no-diff-merges\")) {\n-\t\tsuppress(revs);\n+\t\tset_none(revs);\n \t} else if (!strcmp(arg, \"--combined-all-paths\")) {\n \t\trevs->combined_all_paths = 1;\n \t} else if ((argcount = parse_long_opt(\"diff-merges\", argv, &optarg))) {\n@@ -139,7 +147,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)\n \n void diff_merges_suppress(struct rev_info *revs)\n {\n-\tsuppress(revs);\n+\tset_none(revs);\n }\n \n void diff_merges_default_to_first_parent(struct rev_info *revs)\n-- \n2.25.1\n\n"},{"id":"463024","messageId":"20220914193102.5275-4-sorganov@gmail.com","threadId":"58431","inReplyTo":"20220914193102.5275-1-sorganov@gmail.com","subject":"[PATCH 3/3] diff-merges: clarify log.diffMerges documentation","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-09-14T19:31:02Z","receivedAt":"2022-09-14T19:31:27Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Signed-off-by: Sergey Organov <sorganov@gmail.com>\n---\n Documentation/config/log.txt | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config/log.txt b/Documentation/config/log.txt\nindex 5250ba45fb4e..cbe34d759221 100644\n--- a/Documentation/config/log.txt\n+++ b/Documentation/config/log.txt\n@@ -30,9 +30,9 @@ log.excludeDecoration::\n \toption.\n \n log.diffMerges::\n-\tSet default diff format to be used for merge commits. See\n-\t`--diff-merges` in linkgit:git-log[1] for details.\n-\tDefaults to `separate`.\n+\tSet diff format to be used when `--diff-merges=on` is\n+\tspecified, see `--diff-merges` in linkgit:git-log[1] for\n+\tdetails. Defaults to `separate`.\n \n log.follow::\n \tIf `true`, `git log` will act as if the `--follow` option was used when\n-- \n2.25.1\n\n"},{"id":"463058","messageId":"xmqqr10cmqbk.fsf@gitster.g","threadId":"58431","inReplyTo":"20220914193102.5275-4-sorganov@gmail.com","subject":"Re: [PATCH 3/3] diff-merges: clarify log.diffMerges documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-15T18:43:11Z","receivedAt":"2022-09-15T18:43:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> Signed-off-by: Sergey Organov <sorganov@gmail.com>\n> ---\n>  Documentation/config/log.txt | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt\n> index 5250ba45fb4e..cbe34d759221 100644\n> --- a/Documentation/config/log.txt\n> +++ b/Documentation/config/log.txt\n> @@ -30,9 +30,9 @@ log.excludeDecoration::\n>  \toption.\n>  \n>  log.diffMerges::\n> -\tSet default diff format to be used for merge commits. See\n> -\t`--diff-merges` in linkgit:git-log[1] for details.\n> -\tDefaults to `separate`.\n> +\tSet diff format to be used when `--diff-merges=on` is\n> +\tspecified, see `--diff-merges` in linkgit:git-log[1] for\n> +\tdetails. Defaults to `separate`.\n>  \n>  log.follow::\n>  \tIf `true`, `git log` will act as if the `--follow` option was used when\n\nIs the reason why the patch drops \"default\" because the value given\nis used only when --diff-merges=on is given, and does not kick in\nwhen \"--diff-merges=<format>\" is explicitly given?\n\nThe rest of the change does make sense.\n\n\n"},{"id":"463059","messageId":"xmqqfsgsmq4j.fsf@gitster.g","threadId":"58431","inReplyTo":"20220914193102.5275-2-sorganov@gmail.com","subject":"Re: [PATCH 1/3] diff-merges: cleanup func_by_opt()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-15T18:47:24Z","receivedAt":"2022-09-15T18:47:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> Get rid of unneeded \"else\" statements in func_by_opt().\n\nWhile it is true that loss of \"else\" will not change what the code\nmeans, this change feels subjective and I'd say it falls into \"once\ncommitted, it is not worth the patch noise to go in and change\"\ncategory, not a \"clean-up we should do\".\n\nI haven't looked at diff-merges.c for quite a while, but I did.  I\nnotice that the code is barely commented on what each helper\nfunction is supposed to do, etc., and very hard to follow.  The file\nneeds cleaning up in that area a lot more, I would say.\n\nThanks.\n\n> Signed-off-by: Sergey Organov <sorganov@gmail.com>\n> ---\n>  diff-merges.c | 10 +++++-----\n>  1 file changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/diff-merges.c b/diff-merges.c\n> index 7f64156b8bfe..780ed08fc87f 100644\n> --- a/diff-merges.c\n> +++ b/diff-merges.c\n> @@ -60,15 +60,15 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)\n>  \t\treturn suppress;\n>  \tif (!strcmp(optarg, \"1\") || !strcmp(optarg, \"first-parent\"))\n>  \t\treturn set_first_parent;\n> -\telse if (!strcmp(optarg, \"separate\"))\n> +\tif (!strcmp(optarg, \"separate\"))\n>  \t\treturn set_separate;\n> -\telse if (!strcmp(optarg, \"c\") || !strcmp(optarg, \"combined\"))\n> +\tif (!strcmp(optarg, \"c\") || !strcmp(optarg, \"combined\"))\n>  \t\treturn set_combined;\n> -\telse if (!strcmp(optarg, \"cc\") || !strcmp(optarg, \"dense-combined\"))\n> +\tif (!strcmp(optarg, \"cc\") || !strcmp(optarg, \"dense-combined\"))\n>  \t\treturn set_dense_combined;\n> -\telse if (!strcmp(optarg, \"r\") || !strcmp(optarg, \"remerge\"))\n> +\tif (!strcmp(optarg, \"r\") || !strcmp(optarg, \"remerge\"))\n>  \t\treturn set_remerge_diff;\n> -\telse if (!strcmp(optarg, \"m\") || !strcmp(optarg, \"on\"))\n> +\tif (!strcmp(optarg, \"m\") || !strcmp(optarg, \"on\"))\n>  \t\treturn set_to_default;\n>  \treturn NULL;\n>  }\n"},{"id":"463061","messageId":"xmqq35csmkuq.fsf@gitster.g","threadId":"58431","inReplyTo":"20220914193102.5275-3-sorganov@gmail.com","subject":"Re: [PATCH 2/3] diff-merges: cleanup set_diff_merges()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-15T20:41:17Z","receivedAt":"2022-09-15T20:41:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> Get rid of special-casing of 'suppress' in set_diff_merges(). Instead\n> set 'merges_need_diff' flag correctly in every option handling\n> function.\n>\n> Signed-off-by: Sergey Organov <sorganov@gmail.com>\n> ---\n>  diff-merges.c | 30 +++++++++++++++++++-----------\n>  1 file changed, 19 insertions(+), 11 deletions(-)\n\nLooks OK to me.\n\nEverybody else says set_X() but set_none() has nothing to do ther\nthan calling suppress(), so the change does not really make a\nfunctional difference (as you said in the cover letter).\n\nIs the idea that the original value in .merges_need_diff member does\nnot matter because in every case it is set to either 0 or 1?  Most\ncases call common_setup() to set it to 1 like this here...\n\n> +static void common_setup(struct rev_info *revs)\n> +{\n> +\tsuppress(revs);\n> +\trevs->merges_need_diff = 1;\n> +}\n\n... but this does not touch (in other words, it does not explicitly\nclear) it, ...\n\n> +static void set_none(struct rev_info *revs)\n> +{\n> +\tsuppress(revs);\n> +}\n\n ... so we still rely on somebody to set the .merges_need_diff\nto 0 initially, right?\n\n> -\n> -\t/* NOTE: the merges_need_diff flag is cleared by func() call */\n> -\tif (func != suppress)\n> -\t\trevs->merges_need_diff = 1;\n>  }\n\nIt is very good to see this one go.\n\n>  /*\n> @@ -115,6 +122,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)\n>  \n>  \tif (!suppress_m_parsing && !strcmp(arg, \"-m\")) {\n>  \t\tset_to_default(revs);\n> +\t\trevs->merges_need_diff = 0;\n\nI am wondering how this becomes necessary?  Is it because\nset_to_default() would flip the member to 1 unconditionally, or\nsomething?  If it weren't for this hunk, the lossage of the previous\nhunk is a very good clean-up, but if we need to do this, I cannot\nshake the feeling that we mostly shifted the dirt around, without\nreally cleaning it?  I dunno.\n\n> @@ -125,7 +133,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)\n>  \t\tset_remerge_diff(revs);\n>  \t\trevs->merges_imply_patch = 1;\n>  \t} else if (!strcmp(arg, \"--no-diff-merges\")) {\n> -\t\tsuppress(revs);\n> +\t\tset_none(revs);\n\nWe do not need to explicitly set .merges_need_diff to 0 here,\npresumably because it is initialized to 0 and nobody touched it,\nright?\n\n>  \t} else if (!strcmp(arg, \"--combined-all-paths\")) {\n>  \t\trevs->combined_all_paths = 1;\n>  \t} else if ((argcount = parse_long_opt(\"diff-merges\", argv, &optarg))) {\n> @@ -139,7 +147,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)\n>  \n>  void diff_merges_suppress(struct rev_info *revs)\n>  {\n> -\tsuppress(revs);\n> +\tset_none(revs);\n>  }\n>  \n>  void diff_merges_default_to_first_parent(struct rev_info *revs)\n"},{"id":"463075","messageId":"87wna3jwx8.fsf@osv.gnss.ru","threadId":"58431","inReplyTo":"xmqqfsgsmq4j.fsf@gitster.g","subject":"Re: [PATCH 1/3] diff-merges: cleanup func_by_opt()","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-09-16T13:01:07Z","receivedAt":"2022-09-16T13:01:15Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>> Get rid of unneeded \"else\" statements in func_by_opt().\n>\n> While it is true that loss of \"else\" will not change what the code\n> means, this change feels subjective and I'd say it falls into \"once\n> committed, it is not worth the patch noise to go in and change\"\n> category, not a \"clean-up we should do\".\n\nI agree the \"else\" vs \"no else\" is subjective, but the problem in fact\nis that the first \"if\", unlike the rest of them, already had no \"else\",\nmaking the code inconsistent. So the fix should either be adding one\n\"else\" to the first \"if\", or remove all of the \"else\". I chose the\nlatter, to end up with less noisy code.\n\n>\n> I haven't looked at diff-merges.c for quite a while, but I did.  I\n> notice that the code is barely commented on what each helper\n> function is supposed to do, etc., and very hard to follow.  The file\n> needs cleaning up in that area a lot more, I would say.\n\nI believe each helper function does exactly what its name suggests, so\nno comments are needed. I hate to add comments that actually just say\nthe same as function name, so I'd rathe consider to rename some\nfunctions instead should somebody point out to problematic ones.\n\nThat said, it seems Elijah Newren had not much trouble adding his\n--remerge-diff capability to the diff-merges code, so the code must be\nnot that hard to follow even in its current state.\n\nThanks,\n-- Sergey Organov.\n"},{"id":"463077","messageId":"87pmfvjv6i.fsf@osv.gnss.ru","threadId":"58431","inReplyTo":"xmqq35csmkuq.fsf@gitster.g","subject":"Re: [PATCH 2/3] diff-merges: cleanup set_diff_merges()","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-09-16T13:38:45Z","receivedAt":"2022-09-16T13:38:58Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>> Get rid of special-casing of 'suppress' in set_diff_merges(). Instead\n>> set 'merges_need_diff' flag correctly in every option handling\n>> function.\n>>\n>> Signed-off-by: Sergey Organov <sorganov@gmail.com>\n>> ---\n>>  diff-merges.c | 30 +++++++++++++++++++-----------\n>>  1 file changed, 19 insertions(+), 11 deletions(-)\n>\n> Looks OK to me.\n>\n> Everybody else says set_X() but set_none() has nothing to do ther\n> than calling suppress(), so the change does not really make a\n> functional difference (as you said in the cover letter).\n>\n> Is the idea that the original value in .merges_need_diff member does\n> not matter because in every case it is set to either 0 or 1?  Most\n> cases call common_setup() to set it to 1 like this here...\n\nYes, exactly.\n\n>\n>> +static void common_setup(struct rev_info *revs)\n>> +{\n>> +\tsuppress(revs);\n>> +\trevs->merges_need_diff = 1;\n>> +}\n>\n> ... but this does not touch (in other words, it does not explicitly\n> clear) it, ...\n\n>\n>> +static void set_none(struct rev_info *revs)\n>> +{\n>> +\tsuppress(revs);\n>> +}\n>\n>  ... so we still rely on somebody to set the .merges_need_diff\n> to 0 initially, right?\n\nNo, the suppress() does clear everything, .merges_need_diff included.\nThe suppress() ensures every next diff-merges option on the command-line\nactually overwrites and correctly sets everything.\n\n>\n>> -\n>> -\t/* NOTE: the merges_need_diff flag is cleared by func() call */\n>> -\tif (func != suppress)\n>> -\t\trevs->merges_need_diff = 1;\n>>  }\n>\n> It is very good to see this one go.\n\nYep, that was a kludge that bothered me for a while indeed.\n\n>\n>>  /*\n>> @@ -115,6 +122,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)\n>>  \n>>  \tif (!suppress_m_parsing && !strcmp(arg, \"-m\")) {\n>>  \t\tset_to_default(revs);\n>> +\t\trevs->merges_need_diff = 0;\n>\n> I am wondering how this becomes necessary?  Is it because\n> set_to_default() would flip the member to 1 unconditionally, or\n> something?\n\nYes, as all \"native\" -diff-merge= options set this bit to 1.\n\n> If it weren't for this hunk, the lossage of the previous\n> hunk is a very good clean-up, but if we need to do this, I cannot\n> shake the feeling that we mostly shifted the dirt around, without\n> really cleaning it?  I dunno.\n\nYes, I believe it does cleanup things in both these places.\n\nThe \"-m\" option indeed differs from the rest of the options in exactly\nthis thing: it wants .merges_need_diff=0 to suppress output unless '-p'\nis given as well.\n\nThe -c/--cc are in fact similar, but they don't need this line as they\nimply '-p' that in turn ignores .merges_need_diff, and generates output\nanyway, so -m code has:\n\n  revs->merges_need_diff = 0;\n\nwhereas -c and --cc code both have:\n\n  revs->merges_imply_patch = 1;\n\nThanks,\n-- Sergey Organov\n\n>\n>> @@ -125,7 +133,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)\n>>  \t\tset_remerge_diff(revs);\n>>  \t\trevs->merges_imply_patch = 1;\n>>  \t} else if (!strcmp(arg, \"--no-diff-merges\")) {\n>> -\t\tsuppress(revs);\n>> +\t\tset_none(revs);\n>\n> We do not need to explicitly set .merges_need_diff to 0 here,\n> presumably because it is initialized to 0 and nobody touched it,\n> right?\n\nNo, we rather do set it to 0 here, in suppress() called from set_none().\n\nThanks,\n-- Sergey Organov\n"},{"id":"463078","messageId":"87leqjjuu9.fsf@osv.gnss.ru","threadId":"58431","inReplyTo":"xmqqr10cmqbk.fsf@gitster.g","subject":"Re: [PATCH 3/3] diff-merges: clarify log.diffMerges documentation","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-09-16T13:46:06Z","receivedAt":"2022-09-16T13:46:17Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>> Signed-off-by: Sergey Organov <sorganov@gmail.com>\n>> ---\n>>  Documentation/config/log.txt | 6 +++---\n>>  1 file changed, 3 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt\n>> index 5250ba45fb4e..cbe34d759221 100644\n>> --- a/Documentation/config/log.txt\n>> +++ b/Documentation/config/log.txt\n>> @@ -30,9 +30,9 @@ log.excludeDecoration::\n>>  \toption.\n>>  \n>>  log.diffMerges::\n>> -\tSet default diff format to be used for merge commits. See\n>> -\t`--diff-merges` in linkgit:git-log[1] for details.\n>> -\tDefaults to `separate`.\n>> +\tSet diff format to be used when `--diff-merges=on` is\n>> +\tspecified, see `--diff-merges` in linkgit:git-log[1] for\n>> +\tdetails. Defaults to `separate`.\n>>  \n>>  log.follow::\n>>  \tIf `true`, `git log` will act as if the `--follow` option was used when\n>\n> Is the reason why the patch drops \"default\" because the value given\n> is used only when --diff-merges=on is given, and does not kick in\n> when \"--diff-merges=<format>\" is explicitly given?\n\nYes, exactly. It's the --diff-merges=on that uses the default diff\nformat. Well, in fact \"-m\" uses it as well, being a synonym for\n--diff-merges=on, so we might consider to mention \"-m\" here as well, but\nI thought it's not needed as we provide a link to corresponding piece of\ndocumentation already.\n\nI wanted to explicitly describe what log.diffMerges affects, instead of\nvague \"default diff format\" term causing a need to figure what in fact\nit means.\n\nThanks,\n-- Sergey Organov\n"},{"id":"463087","messageId":"xmqqy1uji9dz.fsf@gitster.g","threadId":"58431","inReplyTo":"87wna3jwx8.fsf@osv.gnss.ru","subject":"Re: [PATCH 1/3] diff-merges: cleanup func_by_opt()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-16T16:14:48Z","receivedAt":"2022-09-16T16:14:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Sergey Organov <sorganov@gmail.com> writes:\n>>\n>>> Get rid of unneeded \"else\" statements in func_by_opt().\n>>\n>> While it is true that loss of \"else\" will not change what the code\n>> means, this change feels subjective and I'd say it falls into \"once\n>> committed, it is not worth the patch noise to go in and change\"\n>> category, not a \"clean-up we should do\".\n>\n> I agree the \"else\" vs \"no else\" is subjective, but the problem in fact\n> is that the first \"if\", unlike the rest of them, already had no \"else\",\n> making the code inconsistent.\n\nThis is a static helper function about a single \"optarg\" string that\nwanted to say \"switch(optarg) { case \"off\": ... }\" but couldn't in\nC, and I happen to view if strcmp else if strcmp ... sequence on the\nsame string a poor-man's substitute for such a construct.  So my\ntake of it is to call the second \"if\" not being \"else if\" a problem,\nnot the rest of it.  If we add a new condition on a different input,\nmaking it \"if (x) ...; switch(optarg) { ... }\" that talks about\nsomething other than optarg, then writing it all with \"if\" without\n\"else if\" would make it harder to see the pattern, but I do not care\ntoo deeply either way, because this is unlikely to gain any logic\nmore involved than \"switch(optarg) { ... }\".\n\n> So the fix should either be adding one\n> \"else\" to the first \"if\", or remove all of the \"else\". I chose the\n> latter, to end up with less noisy code.\n\nYup, see above for the reason why I would choose else-if cascade if\nI had to but I do not care too deeply either way in this particular\ncase ;-)\n"},{"id":"463089","messageId":"xmqqo7vfi93h.fsf@gitster.g","threadId":"58431","inReplyTo":"87leqjjuu9.fsf@osv.gnss.ru","subject":"Re: [PATCH 3/3] diff-merges: clarify log.diffMerges documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-16T16:21:06Z","receivedAt":"2022-09-16T16:21:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> I wanted to explicitly describe what log.diffMerges affects, instead of\n> vague \"default diff format\" term causing a need to figure what in fact\n> it means.\n\nYup, excellent.\n"},{"id":"463090","messageId":"875yhnjnbo.fsf@osv.gnss.ru","threadId":"58431","inReplyTo":"xmqqy1uji9dz.fsf@gitster.g","subject":"Re: [PATCH 1/3] diff-merges: cleanup func_by_opt()","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-09-16T16:28:27Z","receivedAt":"2022-09-16T16:28:37Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>> Junio C Hamano <gitster@pobox.com> writes:\n>>\n>>> Sergey Organov <sorganov@gmail.com> writes:\n>>>\n>>>> Get rid of unneeded \"else\" statements in func_by_opt().\n>>>\n>>> While it is true that loss of \"else\" will not change what the code\n>>> means, this change feels subjective and I'd say it falls into \"once\n>>> committed, it is not worth the patch noise to go in and change\"\n>>> category, not a \"clean-up we should do\".\n>>\n>> I agree the \"else\" vs \"no else\" is subjective, but the problem in fact\n>> is that the first \"if\", unlike the rest of them, already had no \"else\",\n>> making the code inconsistent.\n>\n> This is a static helper function about a single \"optarg\" string that\n> wanted to say \"switch(optarg) { case \"off\": ... }\" but couldn't in\n> C, and I happen to view if strcmp else if strcmp ... sequence on the\n> same string a poor-man's substitute for such a construct.  So my\n> take of it is to call the second \"if\" not being \"else if\" a problem,\n> not the rest of it.  If we add a new condition on a different input,\n> making it \"if (x) ...; switch(optarg) { ... }\" that talks about\n> something other than optarg, then writing it all with \"if\" without\n> \"else if\" would make it harder to see the pattern, but I do not care\n> too deeply either way, because this is unlikely to gain any logic\n> more involved than \"switch(optarg) { ... }\".\n\nWell, I even tend to write this switch(optarg) as:\n\nif (0) ;\nelse if(...) {\n  ...\n}\nelse if(...) { \n  ...\n}\n\nto make the first actual \"case\" look the same way as the rest, and then\nthrough suitable defines it finally looks like:\n\nIF_SWITCH\nIF_CASE(...) {\n  ...\n}\nIF_CASE(...) {\n  ...\n}\n\nmaking the intent explicit.\n\nJust saying.\n\nThanks,\n-- Sergey Organov\n"}]}