{"thread":{"id":"38885","subject":"[PATCH 5/5] Update Bash completion script to include git log --merges option","startedAt":"2015-03-22T18:28:37Z","lastAt":"2015-03-23T05:14:24Z","messageCount":20,"participants":["Koosha Khajehmoogahi","Torsten Bögershausen","Eric Sunshine","Junio C Hamano","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"258299","messageId":"1427048921-28677-1-git-send-email-koosha@posteo.de","threadId":"38885","inReplyTo":null,"subject":"[PATCH 1/5] Add a new option 'merges' to revision.c","fromName":"Koosha Khajehmoogahi","fromEmail":"koosha@posteo.de","sentAt":"2015-03-22T18:28:37Z","receivedAt":"2015-03-22T18:28:37Z","isPatch":true,"sender":{"key":"koosha@posteo.de","avatar":null},"body":"revision.c: add a new option 'merges' with\npossible values of 'only', 'show' and 'hide'.\nThe option is used when showing the list of commits.\nThe value 'only' lists only merges. The value 'show'\nis the default behavior which shows the commits as well\nas merges and the value 'hide' makes it just list commit\nitems.\n\nSigned-off-by: Koosha Khajehmoogahi <koosha@posteo.de>\n---\n revision.c | 22 ++++++++++++++++++++++\n revision.h |  1 +\n 2 files changed, 23 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex 66520c6..edb7bed 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1678,6 +1678,23 @@ static void add_message_grep(struct rev_info *revs, const char *pattern)\n \tadd_grep(revs, pattern, GREP_PATTERN_BODY);\n }\n \n+int parse_merges_opt(struct rev_info *revs, const char *param)\n+{\n+\tif (!strcmp(param, \"show\")) {\n+\t\trevs->min_parents = 0;\n+\t\trevs->max_parents = -1;\n+\t} else if (!strcmp(param, \"only\")) {\n+\t\trevs->min_parents = 2;\n+\t\trevs->max_parents = -1;\n+\t} else if (!strcmp(param, \"hide\")) {\n+\t\trevs->min_parents = 0;\n+\t\trevs->max_parents = 1;\n+\t} else {\n+\t\treturn -1;\n+\t}\n+\treturn 0;\n+}\n+\n static int handle_revision_opt(struct rev_info *revs, int argc, const char **argv,\n \t\t\t       int *unkc, const char **unkv)\n {\n@@ -1800,9 +1817,14 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->show_all = 1;\n \t} else if (!strcmp(arg, \"--remove-empty\")) {\n \t\trevs->remove_empty_trees = 1;\n+\t} else if (starts_with(arg, \"--merges=\")) {\n+\t\tif (parse_merges_opt(revs, arg + 9))\n+\t\t\tdie(\"unknown option: %s\", arg);\n \t} else if (!strcmp(arg, \"--merges\")) {\n+\t\trevs->max_parents = -1;\n \t\trevs->min_parents = 2;\n \t} else if (!strcmp(arg, \"--no-merges\")) {\n+\t\trevs->min_parents = 0;\n \t\trevs->max_parents = 1;\n \t} else if (starts_with(arg, \"--min-parents=\")) {\n \t\trevs->min_parents = atoi(arg+14);\ndiff --git a/revision.h b/revision.h\nindex 0ea8b4e..f9df58c 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -240,6 +240,7 @@ extern int setup_revisions(int argc, const char **argv, struct rev_info *revs,\n extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,\n \t\t\t       const struct option *options,\n \t\t\t       const char * const usagestr[]);\n+extern int parse_merges_opt(struct rev_info *, const char *);\n #define REVARG_CANNOT_BE_FILENAME 01\n #define REVARG_COMMITTISH 02\n extern int handle_revision_arg(const char *arg, struct rev_info *revs,\n-- \n2.3.3.263.g095251d.dirty\n"},{"id":"258302","messageId":"1427048921-28677-2-git-send-email-koosha@posteo.de","threadId":"38885","inReplyTo":"1427048921-28677-1-git-send-email-koosha@posteo.de","subject":"[PATCH 2/5] Make git-log honor log.merges option","fromName":"Koosha Khajehmoogahi","fromEmail":"koosha@posteo.de","sentAt":"2015-03-22T18:28:38Z","receivedAt":"2015-03-22T18:28:38Z","isPatch":true,"sender":{"key":"koosha@posteo.de","avatar":null},"body":"Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>\n---\n builtin/log.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex dd8f3fc..c7a7aad 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -36,6 +36,7 @@ static int decoration_given;\n static int use_mailmap_config;\n static const char *fmt_patch_subject_prefix = \"PATCH\";\n static const char *fmt_pretty;\n+static const char *log_merges;\n \n static const char * const builtin_log_usage[] = {\n \tN_(\"git log [<options>] [<revision range>] [[--] <path>...]\"),\n@@ -386,6 +387,9 @@ static int git_log_config(const char *var, const char *value, void *cb)\n \t\t\tdecoration_style = 0; /* maybe warn? */\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"log.merges\")) {\n+\t\treturn git_config_string(&log_merges, var, value);\n+\t}\n \tif (!strcmp(var, \"log.showroot\")) {\n \t\tdefault_show_root = git_config_bool(var, value);\n \t\treturn 0;\n@@ -628,6 +632,8 @@ int cmd_log(int argc, const char **argv, const char *prefix)\n \n \tinit_revisions(&rev, prefix);\n \trev.always_show_header = 1;\n+\tif (log_merges && parse_merges_opt(&rev, log_merges))\n+\t\tdie(\"unknown config value for log.merges: %s\", log_merges);\n \tmemset(&opt, 0, sizeof(opt));\n \topt.def = \"HEAD\";\n \topt.revarg_opt = REVARG_COMMITTISH;\n-- \n2.3.3.263.g095251d.dirty\n"},{"id":"258300","messageId":"1427048921-28677-3-git-send-email-koosha@posteo.de","threadId":"38885","inReplyTo":"1427048921-28677-1-git-send-email-koosha@posteo.de","subject":"[PATCH 3/5] Update documentations for git-log to include the new --merges option and also its corresponding config option.","fromName":"Koosha Khajehmoogahi","fromEmail":"koosha@posteo.de","sentAt":"2015-03-22T18:28:39Z","receivedAt":"2015-03-22T18:28:39Z","isPatch":true,"sender":{"key":"koosha@posteo.de","avatar":null},"body":"Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>\n---\n Documentation/git-log.txt          | 3 +++\n Documentation/rev-list-options.txt | 6 ++++++\n 2 files changed, 9 insertions(+)\n\ndiff --git a/Documentation/git-log.txt b/Documentation/git-log.txt\nindex 1f7bc67..506125a 100644\n--- a/Documentation/git-log.txt\n+++ b/Documentation/git-log.txt\n@@ -190,6 +190,9 @@ log.showroot::\n \t`git log -p` output would be shown without a diff attached.\n \tThe default is `true`.\n \n+log.merges::\n+    Default for `--merges` option. Defaults to `show`.\n+\n mailmap.*::\n \tSee linkgit:git-shortlog[1].\n \ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 4ed8587..398e657 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -99,6 +99,12 @@ if it is part of the log message.\n --merges::\n \tPrint only merge commits. This is exactly the same as `--min-parents=2`.\n \n+--merges=show|hide|only::\n+\tIf show is specified, merge commits will be shown in conjunction with\n+\tother commits. If hide is specified, it will work like `--no-merges`.\n+\tIf only is specified, it will work like `--merges`. The default option\n+\tis show.\n+\n --no-merges::\n \tDo not print commits with more than one parent. This is\n \texactly the same as `--max-parents=1`.\n-- \n2.3.3.263.g095251d.dirty\n"},{"id":"258301","messageId":"1427048921-28677-4-git-send-email-koosha@posteo.de","threadId":"38885","inReplyTo":"1427048921-28677-1-git-send-email-koosha@posteo.de","subject":"[PATCH 4/5] Add tests for git-log --merges=show|hide|only","fromName":"Koosha Khajehmoogahi","fromEmail":"koosha@posteo.de","sentAt":"2015-03-22T18:28:40Z","receivedAt":"2015-03-22T18:28:40Z","isPatch":true,"sender":{"key":"koosha@posteo.de","avatar":null},"body":"Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>\n---\n t/t4202-log.sh | 141 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 141 insertions(+)\n\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 5f2b290..ab6f371 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -428,6 +428,147 @@ cat > expect <<\\EOF\n * initial\n EOF\n \n+cat > only_merges <<EOF\n+Merge tag 'reach'\n+Merge tags 'octopus-a' and 'octopus-b'\n+Merge branch 'tangle'\n+Merge branch 'side' (early part) into tangle\n+Merge branch 'master' (early part) into tangle\n+Merge branch 'side'\n+EOF\n+\n+cat > only_commits <<EOF\n+seventh\n+octopus-b\n+octopus-a\n+reach\n+tangle-a\n+side-2\n+side-1\n+Second\n+sixth\n+fifth\n+fourth\n+third\n+second\n+initial\n+EOF\n+\n+cat > both_commits_merges <<EOF\n+Merge tag 'reach'\n+Merge tags 'octopus-a' and 'octopus-b'\n+seventh\n+octopus-b\n+octopus-a\n+reach\n+Merge branch 'tangle'\n+Merge branch 'side' (early part) into tangle\n+Merge branch 'master' (early part) into tangle\n+tangle-a\n+Merge branch 'side'\n+side-2\n+side-1\n+Second\n+sixth\n+fifth\n+fourth\n+third\n+second\n+initial\n+EOF\n+\n+test_expect_success 'log with config log.merges=show' '\n+\tgit config log.merges show &&\n+    git log --pretty=tformat:%s >actual &&\n+\ttest_cmp both_commits_merges actual &&\n+    git config --unset log.merges\n+'\n+\n+test_expect_success 'log with config log.merges=only' '\n+\tgit config log.merges only &&\n+    git log --pretty=tformat:%s >actual &&\n+\ttest_cmp only_merges actual &&\n+    git config --unset log.merges\n+'\n+\n+test_expect_success 'log with config log.merges=hide' '\n+\tgit config log.merges hide &&\n+    git log --pretty=tformat:%s >actual &&\n+\ttest_cmp only_commits actual &&\n+    git config --unset log.merges\n+'\n+\n+test_expect_success 'log with config log.merges=show with git log --no-merges' '\n+\tgit config log.merges show &&\n+    git log --no-merges --pretty=tformat:%s >actual &&\n+\ttest_cmp only_commits actual &&\n+    git config --unset log.merges\n+'\n+\n+test_expect_success 'log with config log.merges=hide with git log ---merges' '\n+\tgit config log.merges hide &&\n+    git log --merges --pretty=tformat:%s >actual &&\n+\ttest_cmp only_merges actual &&\n+    git config --unset log.merges\n+'\n+\n+test_expect_success 'log --merges=show' '\n+    git log --merges=show --pretty=tformat:%s >actual &&\n+\ttest_cmp both_commits_merges actual\n+'\n+\n+test_expect_success 'log --merges=only' '\n+    git log --merges=only --pretty=tformat:%s >actual &&\n+\ttest_cmp only_merges actual\n+'\n+\n+test_expect_success 'log --merges=hide' '\n+    git log --merges=hide --pretty=tformat:%s >actual &&\n+\ttest_cmp only_commits actual\n+'\n+\n+test_expect_success 'log --merges=show with config log.merges=hide' '\n+\tgit config log.merges hide &&\n+    git log --merges=show --pretty=tformat:%s >actual &&\n+\ttest_cmp both_commits_merges actual &&\n+    git config --unset log.merges\n+'\n+\n+test_expect_success 'log --merges=show with config log.merges=only' '\n+\tgit config log.merges only &&\n+    git log --merges=show --pretty=tformat:%s >actual &&\n+\ttest_cmp both_commits_merges actual &&\n+    git config --unset log.merges\n+'\n+\n+test_expect_success 'log --merges=only with config log.merges=show' '\n+\tgit config log.merges show &&\n+    git log --merges=only --pretty=tformat:%s >actual &&\n+\ttest_cmp only_merges actual &&\n+    git config --unset log.merges\n+'\n+\n+test_expect_success 'log --merges=only with config log.merges=hide' '\n+\tgit config log.merges hide &&\n+    git log --merges=only --pretty=tformat:%s >actual &&\n+\ttest_cmp only_merges actual &&\n+    git config --unset log.merges\n+'\n+\n+test_expect_success 'log --merges=hide with config log.merges=show' '\n+\tgit config log.merges show &&\n+    git log --merges=hide --pretty=tformat:%s >actual &&\n+\ttest_cmp only_commits actual &&\n+    git config --unset log.merges\n+'\n+\n+test_expect_success 'log --merges=hide with config log.merges=only' '\n+\tgit config log.merges only &&\n+    git log --merges=hide --pretty=tformat:%s >actual &&\n+\ttest_cmp only_commits actual &&\n+    git config --unset log.merges\n+'\n+\n test_expect_success 'log --graph with merge' '\n \tgit log --graph --date-order --pretty=tformat:%s |\n \t\tsed \"s/ *\\$//\" >actual &&\n-- \n2.3.3.263.g095251d.dirty\n"},{"id":"258298","messageId":"1427048921-28677-5-git-send-email-koosha@posteo.de","threadId":"38885","inReplyTo":"1427048921-28677-1-git-send-email-koosha@posteo.de","subject":"[PATCH 5/5] Update Bash completion script to include git log --merges option","fromName":"Koosha Khajehmoogahi","fromEmail":"koosha@posteo.de","sentAt":"2015-03-22T18:28:41Z","receivedAt":"2015-03-22T18:28:41Z","isPatch":true,"sender":{"key":"koosha@posteo.de","avatar":null},"body":"Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>\n---\n contrib/completion/git-completion.bash | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 731c289..b63bb95 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1406,7 +1406,7 @@ _git_ls_tree ()\n __git_log_common_options=\"\n \t--not --all\n \t--branches --tags --remotes\n-\t--first-parent --merges --no-merges\n+\t--first-parent --merges --merges= --no-merges\n \t--max-count=\n \t--max-age= --since= --after=\n \t--min-age= --until= --before=\n@@ -1451,6 +1451,10 @@ _git_log ()\n \t\t__gitcomp \"long short\" \"\" \"${cur##--decorate=}\"\n \t\treturn\n \t\t;;\n+    --merges=*)\n+        __gitcomp \"show hide only\" \"\" \"${cur##--merges=}\"\n+        return\n+        ;;\n \t--*)\n \t\t__gitcomp \"\n \t\t\t$__git_log_common_options\n@@ -1861,6 +1865,10 @@ _git_config ()\n \t\t__gitcomp \"$__git_log_date_formats\"\n \t\treturn\n \t\t;;\n+\tlog.merges)\n+\t\t__gitcomp \"show hide only\"\n+\t\treturn\n+\t\t;;\n \tsendemail.aliasesfiletype)\n \t\t__gitcomp \"mutt mailrc pine elm gnus\"\n \t\treturn\n@@ -2150,6 +2158,7 @@ _git_config ()\n \t\tinteractive.singlekey\n \t\tlog.date\n \t\tlog.decorate\n+\t\tlog.merges\n \t\tlog.showroot\n \t\tmailmap.file\n \t\tman.\n-- \n2.3.3.263.g095251d.dirty\n"},{"id":"258310","messageId":"550F1E9F.40801@web.de","threadId":"38885","inReplyTo":"1427048921-28677-4-git-send-email-koosha@posteo.de","subject":"Re: [PATCH 4/5] Add tests for git-log --merges=show|hide|only","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-03-22T19:57:19Z","receivedAt":"2015-03-22T19:57:19Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 22.03.15 19:28, Koosha Khajehmoogahi wrote:\n> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>\n> ---\n>  t/t4202-log.sh | 141 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n>  1 file changed, 141 insertions(+)\n> \n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index 5f2b290..ab6f371 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -428,6 +428,147 @@ cat > expect <<\\EOF\n>  * initial\n>  EOF\n>  \n> +cat > only_merges <<EOF\n\n- please no space after the \">\"\n- please add the && at the end of the line:\ncat >only_merges <<EOF &&\n(And the same further down)\n\n> +test_expect_success 'log with config log.merges=show' '\n> +\tgit config log.merges show &&\nIndent with TAB is good\n> +    git log --pretty=tformat:%s >actual &&\nbut indent with 4 spaces not ideal, please use a TAB as well.\n> +\ttest_cmp both_commits_merges actual &&\n> +    git config --unset log.merges\nDo we need the unset here?\nThe log.merges is nicely set up before each test case, so can we drop the unset lines ?\n(Or do I miss something ?)\n"},{"id":"258311","messageId":"CAPig+cRvV2L3Wt3sMLHMngzeomLcD96Swfd3KSXJxnBEid2yVA@mail.gmail.com","threadId":"38885","inReplyTo":"550F1E9F.40801@web.de","subject":"Re: [PATCH 4/5] Add tests for git-log --merges=show|hide|only","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-22T20:19:01Z","receivedAt":"2015-03-22T20:19:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 22, 2015 at 3:57 PM, Torsten Bögershausen <tboegi@web.de> wrote:\n> On 22.03.15 19:28, Koosha Khajehmoogahi wrote:\n>> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>\n>> ---\n>> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n>> index 5f2b290..ab6f371 100755\n>> --- a/t/t4202-log.sh\n>> +++ b/t/t4202-log.sh\n>> @@ -428,6 +428,147 @@ cat > expect <<\\EOF\n>>  * initial\n>>  EOF\n>>\n>> +cat > only_merges <<EOF\n>\n> - please no space after the \">\"\n> - please add the && at the end of the line:\n> cat >only_merges <<EOF &&\n\nThis code is outside any test, so && has no impact (and would be\nslightly confusing). Better would be to move this setup code into a\n\"setup --merges=\" test, in which case the &&-chain would be\nappropriate.\n"},{"id":"258312","messageId":"CAPig+cRKc59804+yXtX8ZJagsJ4NVw-61Ru1kh1eBusGR1Ai_g@mail.gmail.com","threadId":"38885","inReplyTo":"1427048921-28677-1-git-send-email-koosha@posteo.de","subject":"Re: [PATCH 1/5] Add a new option 'merges' to revision.c","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-22T20:43:22Z","receivedAt":"2015-03-22T20:43:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 22, 2015 at 2:28 PM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:\n> Subject: Add a new option 'merges' to revision.c\n\nFor the subject, mention the area you're touching, followed by a\ncolon, followed by a short but meaningful summary of the change. Drop\ncapitalization.\n\n    revision: add --merges={show|only|hide} option\n\n> revision.c: add a new option 'merges' with\n\nNo need to mention \"revision.c\" here since the patch itself shows this clearly.\n\nConsidering that there is already a --merges option, it is somewhat\nmisleading and not terribly clear to say only that you're adding a new\n\"option 'merges'\". Better would be to spell out --merges= explicitly.\n\n> possible values of 'only', 'show' and 'hide'.\n> The option is used when showing the list of commits.\n> The value 'only' lists only merges. The value 'show'\n> is the default behavior which shows the commits as well\n> as merges and the value 'hide' makes it just list commit\n> items.\n>\n> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>\n\nConsidering that Junio actually wrote this patch[1], it would be more\ncorrect and considerate to attribute it to him. You can do so by\nensuring that there is a \"From:\" header at the very top of the commit\nmessage before mailing out the patch:\n\n    From: Junio C Hamano <gitster@pobox.com>\n\nThe customary way to indicate that you wrote the commit message and\nmade a few small adjustments to Junio's patch would be with a\nbracketed comment (starting with your initials) just before your\nsign-off. Something like this:\n\n    [kk: wrote commit message; added a couple missing\n    {min|max}_parents assignments]\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/265597\n\n> ---\n> diff --git a/revision.c b/revision.c\n> index 66520c6..edb7bed 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1678,6 +1678,23 @@ static void add_message_grep(struct rev_info *revs, const char *pattern)\n>         add_grep(revs, pattern, GREP_PATTERN_BODY);\n>  }\n>\n> +int parse_merges_opt(struct rev_info *revs, const char *param)\n> +{\n> +       if (!strcmp(param, \"show\")) {\n> +               revs->min_parents = 0;\n> +               revs->max_parents = -1;\n> +       } else if (!strcmp(param, \"only\")) {\n> +               revs->min_parents = 2;\n> +               revs->max_parents = -1;\n> +       } else if (!strcmp(param, \"hide\")) {\n> +               revs->min_parents = 0;\n> +               revs->max_parents = 1;\n> +       } else {\n> +               return -1;\n> +       }\n> +       return 0;\n> +}\n> +\n>  static int handle_revision_opt(struct rev_info *revs, int argc, const char **argv,\n>                                int *unkc, const char **unkv)\n>  {\n> @@ -1800,9 +1817,14 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n>                 revs->show_all = 1;\n>         } else if (!strcmp(arg, \"--remove-empty\")) {\n>                 revs->remove_empty_trees = 1;\n> +       } else if (starts_with(arg, \"--merges=\")) {\n> +               if (parse_merges_opt(revs, arg + 9))\n> +                       die(\"unknown option: %s\", arg);\n>         } else if (!strcmp(arg, \"--merges\")) {\n> +               revs->max_parents = -1;\n>                 revs->min_parents = 2;\n>         } else if (!strcmp(arg, \"--no-merges\")) {\n> +               revs->min_parents = 0;\n>                 revs->max_parents = 1;\n>         } else if (starts_with(arg, \"--min-parents=\")) {\n>                 revs->min_parents = atoi(arg+14);\n> diff --git a/revision.h b/revision.h\n> index 0ea8b4e..f9df58c 100644\n> --- a/revision.h\n> +++ b/revision.h\n> @@ -240,6 +240,7 @@ extern int setup_revisions(int argc, const char **argv, struct rev_info *revs,\n>  extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,\n>                                const struct option *options,\n>                                const char * const usagestr[]);\n> +extern int parse_merges_opt(struct rev_info *, const char *);\n>  #define REVARG_CANNOT_BE_FILENAME 01\n>  #define REVARG_COMMITTISH 02\n>  extern int handle_revision_arg(const char *arg, struct rev_info *revs,\n> --\n> 2.3.3.263.g095251d.dirty\n"},{"id":"258313","messageId":"CAPig+cSOc-V=DG=tq7q8sWjJjMpnx8vNkGk56eFFWCMU-u77Rg@mail.gmail.com","threadId":"38885","inReplyTo":"1427048921-28677-2-git-send-email-koosha@posteo.de","subject":"Re: [PATCH 2/5] Make git-log honor log.merges option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-22T20:50:47Z","receivedAt":"2015-03-22T20:50:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 22, 2015 at 2:28 PM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:\n> Subject: Make git-log honor log.merges option\n\nDrop capitalization, mention area you're touching, followed by colon,\nfollowed by short summary:\n\n    log: honor log.merges option\n\n> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>\n\nAs discussed in my review of patch 1/5, since Junio actually wrote\nthis patch[1], it would be more correct and considerate to attribute\nit to him by ensuring that a \"From:\" header is at the very top of the\ncommit message before mailing out the patch:\n\n    From: Junio C Hamano <gitster@pobox.com>\n\n(If Junio had also signed-off on his patch, you would also include his\nsign-off just above yours. In this case, he didn't sign off[1], so\nyour sign-off is all that is needed; and Junio will add his own if he\npicks up this series.)\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/265597\n\n> ---\n> diff --git a/builtin/log.c b/builtin/log.c\n> index dd8f3fc..c7a7aad 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -36,6 +36,7 @@ static int decoration_given;\n>  static int use_mailmap_config;\n>  static const char *fmt_patch_subject_prefix = \"PATCH\";\n>  static const char *fmt_pretty;\n> +static const char *log_merges;\n>\n>  static const char * const builtin_log_usage[] = {\n>         N_(\"git log [<options>] [<revision range>] [[--] <path>...]\"),\n> @@ -386,6 +387,9 @@ static int git_log_config(const char *var, const char *value, void *cb)\n>                         decoration_style = 0; /* maybe warn? */\n>                 return 0;\n>         }\n> +       if (!strcmp(var, \"log.merges\")) {\n> +               return git_config_string(&log_merges, var, value);\n> +       }\n>         if (!strcmp(var, \"log.showroot\")) {\n>                 default_show_root = git_config_bool(var, value);\n>                 return 0;\n> @@ -628,6 +632,8 @@ int cmd_log(int argc, const char **argv, const char *prefix)\n>\n>         init_revisions(&rev, prefix);\n>         rev.always_show_header = 1;\n> +       if (log_merges && parse_merges_opt(&rev, log_merges))\n> +               die(\"unknown config value for log.merges: %s\", log_merges);\n>         memset(&opt, 0, sizeof(opt));\n>         opt.def = \"HEAD\";\n>         opt.revarg_opt = REVARG_COMMITTISH;\n> --\n> 2.3.3.263.g095251d.dirty\n"},{"id":"258316","messageId":"CAPig+cTQK_NF9pULobpggg8eXEKN_exj2buBEmk26TKY=DkapA@mail.gmail.com","threadId":"38885","inReplyTo":"1427048921-28677-3-git-send-email-koosha@posteo.de","subject":"Re: [PATCH 3/5] Update documentations for git-log to include the new --merges option and also its corresponding config option.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-22T21:17:30Z","receivedAt":"2015-03-22T21:17:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 22, 2015 at 2:28 PM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:\n> Subject: Update documentations for git-log to include the new\n> --merges option and also its corresponding config option.\n\nThe Subject: should be a short summary of the change; ideally less\nthan 70 or 72 characters. The rest of the commit message can flesh out\nthe description if necessary. The subject on this patch is far too\nlong. Also, drop capitalization, mention the area you're touching, and\ndrop the final period. You might say instead:\n\n    Documentation: mention git-log --merges= and log.merges\n\nIn this case, it's probably not necessary to write anything else in\nthe commit message. The summary says enough, despite its conciseness.\n\nMore below.\n\n> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>\n> ---\n> diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt\n> index 1f7bc67..506125a 100644\n> --- a/Documentation/git-log.txt\n> +++ b/Documentation/git-log.txt\n> @@ -190,6 +190,9 @@ log.showroot::\n>         `git log -p` output would be shown without a diff attached.\n>         The default is `true`.\n>\n> +log.merges::\n> +    Default for `--merges` option. Defaults to `show`.\n\nTo disambiguate this from the --merges option, you should probably\nspell it out explicitly as `--merges=` rather than just `--merges`.\n\n>  mailmap.*::\n>         See linkgit:git-shortlog[1].\n>\n> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n> index 4ed8587..398e657 100644\n> --- a/Documentation/rev-list-options.txt\n> +++ b/Documentation/rev-list-options.txt\n> @@ -99,6 +99,12 @@ if it is part of the log message.\n>  --merges::\n>         Print only merge commits. This is exactly the same as `--min-parents=2`.\n>\n> +--merges=show|hide|only::\n> +       If show is specified, merge commits will be shown in conjunction with\n> +       other commits. If hide is specified, it will work like `--no-merges`.\n> +       If only is specified, it will work like `--merges`. The default option\n> +       is show.\n\nBy \"The default option is show\", do you mean when --merges= is not\nspecified? Perhaps it would be better to phrase it as:\n\n    Default is `show` if `--merges=` is not specified.\n\n> +\n>  --no-merges::\n>         Do not print commits with more than one parent. This is\n>         exactly the same as `--max-parents=1`.\n\nIt might make more sense to rewrite the --merges and --no-merges\noptions in terms of the new --merges= option. For instance, something\nlike this:\n\n     --merges::\n        Shorthand for `--merges=only`.\n\n    --merges={show|hide|only}::\n        `show`: show merge and non-merge commits\n\n        `hide`: omit merge commits; same as `--max-parents=1`\n\n        `only`: show only merge commits; same as `--min-parents=2`\n\n        Default is `show` if `--merges=` is not specified.\n\n     --no-merges::\n        Shorthand for `--merges=hide`.\n\nAlternately, --merges and --merges= could be combined like this\n(taking inspiration from --decorate[=]):\n\n    --merges[=show|hide|only]::\n\nBut then you need additional explanation about what --merges means\nwithout the '=' and following keyword.\n\n> --\n> 2.3.3.263.g095251d.dirty\n"},{"id":"258318","messageId":"CAPig+cTnyKZWAogK_7zyswg4JaPfA_=p_G+wVC4hAZe6t7ogGw@mail.gmail.com","threadId":"38885","inReplyTo":"1427048921-28677-5-git-send-email-koosha@posteo.de","subject":"Re: [PATCH 5/5] Update Bash completion script to include git log --merges option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-22T21:24:25Z","receivedAt":"2015-03-22T21:24:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 22, 2015 at 2:28 PM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:\n> Subject: Update Bash completion script to include git log --merges option\n\nNice to see a patch covering this oft-overlooked corner of the project.\n\nIt's misleading to say that you're updating it to include the --merges\noption, which it already knows about. Spell it out explicitly as\n--merges=. Also, drop capitalization, mention area you're touching,\nfollowed by colon, followed by short summary:\n\n    completion: teach about git-log --merges= and log.merges\n\n> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>\n> ---\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 731c289..b63bb95 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -1406,7 +1406,7 @@ _git_ls_tree ()\n>  __git_log_common_options=\"\n>         --not --all\n>         --branches --tags --remotes\n> -       --first-parent --merges --no-merges\n> +       --first-parent --merges --merges= --no-merges\n>         --max-count=\n>         --max-age= --since= --after=\n>         --min-age= --until= --before=\n> @@ -1451,6 +1451,10 @@ _git_log ()\n>                 __gitcomp \"long short\" \"\" \"${cur##--decorate=}\"\n>                 return\n>                 ;;\n> +    --merges=*)\n> +        __gitcomp \"show hide only\" \"\" \"${cur##--merges=}\"\n> +        return\n> +        ;;\n>         --*)\n>                 __gitcomp \"\n>                         $__git_log_common_options\n> @@ -1861,6 +1865,10 @@ _git_config ()\n>                 __gitcomp \"$__git_log_date_formats\"\n>                 return\n>                 ;;\n> +       log.merges)\n> +               __gitcomp \"show hide only\"\n> +               return\n> +               ;;\n>         sendemail.aliasesfiletype)\n>                 __gitcomp \"mutt mailrc pine elm gnus\"\n>                 return\n> @@ -2150,6 +2158,7 @@ _git_config ()\n>                 interactive.singlekey\n>                 log.date\n>                 log.decorate\n> +               log.merges\n>                 log.showroot\n>                 mailmap.file\n>                 man.\n> --\n> 2.3.3.263.g095251d.dirty\n"},{"id":"258320","messageId":"550F3D3B.7080104@posteo.de","threadId":"38885","inReplyTo":"550F1E9F.40801@web.de","subject":"Re: [PATCH 4/5] Add tests for git-log --merges=show|hide|only","fromName":"Koosha Khajehmoogahi","fromEmail":"koosha@posteo.de","sentAt":"2015-03-22T22:07:55Z","receivedAt":"2015-03-22T22:07:55Z","isPatch":true,"sender":{"key":"koosha@posteo.de","avatar":null},"body":"\n\nOn 03/22/2015 08:57 PM, Torsten Bögershausen wrote:\n> On 22.03.15 19:28, Koosha Khajehmoogahi wrote:\n>> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>\n>> ---\n>>  t/t4202-log.sh | 141 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n>>  1 file changed, 141 insertions(+)\n>>\n>> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n>> index 5f2b290..ab6f371 100755\n>> --- a/t/t4202-log.sh\n>> +++ b/t/t4202-log.sh\n>> @@ -428,6 +428,147 @@ cat > expect <<\\EOF\n>>  * initial\n>>  EOF\n>>  \n>> +cat > only_merges <<EOF\n> \n> - please no space after the \">\"\n> - please add the && at the end of the line:\n> cat >only_merges <<EOF &&\n> (And the same further down)\n> \n>> +test_expect_success 'log with config log.merges=show' '\n>> +\tgit config log.merges show &&\n> Indent with TAB is good\n>> +    git log --pretty=tformat:%s >actual &&\n> but indent with 4 spaces not ideal, please use a TAB as well.\n>> +\ttest_cmp both_commits_merges actual &&\n>> +    git config --unset log.merges\n> Do we need the unset here?\n> The log.merges is nicely set up before each test case, so can we drop the unset lines ?\n> (Or do I miss something ?)\n> \n\nGood point; we can drop only those unset lines whose next test sets the log.merges.\nHowever, if the next test does not set it, we must unset it as it affects the\ndefault behavior of git-log.\n\n-- \nKoosha\n"},{"id":"258321","messageId":"CAPig+cQUtUiXx2v6OBsTiE6EoitmxuCk0ua4RoaocNW0rzjLoQ@mail.gmail.com","threadId":"38885","inReplyTo":"1427048921-28677-4-git-send-email-koosha@posteo.de","subject":"Re: [PATCH 4/5] Add tests for git-log --merges=show|hide|only","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-22T22:26:02Z","receivedAt":"2015-03-22T22:26:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 22, 2015 at 2:28 PM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:\n> Subject: Add tests for git-log --merges=show|hide|only\n\nDrop capitalization, mention area you're touching, followed by colon,\nfollowed by short summary:\n\n    t4202-log: add --merges= tests\n\nMore below.\n\n> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>\n> ---\n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index 5f2b290..ab6f371 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -428,6 +428,147 @@ cat > expect <<\\EOF\n>  * initial\n>  EOF\n>\n> +cat > only_merges <<EOF\n> +Merge tag 'reach'\n> +Merge tags 'octopus-a' and 'octopus-b'\n> +Merge branch 'tangle'\n> +Merge branch 'side' (early part) into tangle\n> +Merge branch 'master' (early part) into tangle\n> +Merge branch 'side'\n> +EOF\n\nCurrent custom is to place this sort of setup code in its own test\nrather than having it at top-level. So, the above and below 'cat's\nshould all go in a test named \"setup --merges=\" (or something better).\nPrefixing EOF with '-' allows you to indent the here-doc content and\nEOF so that it reads nicely inside a test. Also prefix EOF with '\\' to\nindicate that you do not expect interpolation within the here-doc.\nTorsten already mentioned to drop the space following '>'. Finally,\nwithin the setup test, chain them all together with &&. So:\n\n    cat >only_merges <<-\\EOF &&\n    ...\n    EOF\n\nMore below.\n\n> +cat > only_commits <<EOF\n> +seventh\n> +octopus-b\n> +octopus-a\n> +reach\n> +tangle-a\n> +side-2\n> +side-1\n> +Second\n> +sixth\n> +fifth\n> +fourth\n> +third\n> +second\n> +initial\n> +EOF\n> +\n> +cat > both_commits_merges <<EOF\n> +Merge tag 'reach'\n> +Merge tags 'octopus-a' and 'octopus-b'\n> +seventh\n> +octopus-b\n> +octopus-a\n> +reach\n> +Merge branch 'tangle'\n> +Merge branch 'side' (early part) into tangle\n> +Merge branch 'master' (early part) into tangle\n> +tangle-a\n> +Merge branch 'side'\n> +side-2\n> +side-1\n> +Second\n> +sixth\n> +fifth\n> +fourth\n> +third\n> +second\n> +initial\n> +EOF\n\nt4202 already does this sort of fragile hard-coding of \"expected\"\noutput, so this isn't a particularly strong objection, but it would be\nmore robust if you could create these \"expected\" lists dynamically by\nissuing some git command other than the one you're testing. (That\ncould also be considered fodder for a follow-on patch if you don't\nconsider such a change worthwhile at this time.)\n\n> +\n> +test_expect_success 'log with config log.merges=show' '\n> +       git config log.merges show &&\n> +    git log --pretty=tformat:%s >actual &&\n\nTorsten already mentioned botched indentation. Use (8-character wide)\ntab for indentation everywhere.\n\n> +       test_cmp both_commits_merges actual &&\n> +    git config --unset log.merges\n\nIf the test_cmp fails (because the expected and actual output\ndiffered), then the git-config --unset will never be invoked. To\nensure that the config setting is undone when the test finishes\n(whether successful or not), use test_config().\n\nThe other option is to ensure that each test sets or clears log.merges\nas appropriate at the start of the test, and then not bother about\nresetting it. The shortcoming of this approach, however, is that any\ntests added following these new tests won't necessarily know that\nlog.merges is set or that they may need to clear it. Consequently,\ntest_config() at the start of each of these tests is probably the\nbetter approach.\n\nMore below.\n\n> +'\n> +\n> +test_expect_success 'log with config log.merges=only' '\n> +       git config log.merges only &&\n> +    git log --pretty=tformat:%s >actual &&\n> +       test_cmp only_merges actual &&\n> +    git config --unset log.merges\n> +'\n> +\n> +test_expect_success 'log with config log.merges=hide' '\n> +       git config log.merges hide &&\n> +    git log --pretty=tformat:%s >actual &&\n> +       test_cmp only_commits actual &&\n> +    git config --unset log.merges\n> +'\n> +\n> +test_expect_success 'log with config log.merges=show with git log --no-merges' '\n> +       git config log.merges show &&\n> +    git log --no-merges --pretty=tformat:%s >actual &&\n> +       test_cmp only_commits actual &&\n> +    git config --unset log.merges\n> +'\n> +\n> +test_expect_success 'log with config log.merges=hide with git log ---merges' '\n> +       git config log.merges hide &&\n> +    git log --merges --pretty=tformat:%s >actual &&\n> +       test_cmp only_merges actual &&\n> +    git config --unset log.merges\n> +'\n> +\n> +test_expect_success 'log --merges=show' '\n\nFor these tests which expect log.merges to be unset, it would be more\nrobust, and the intent clearer, if you actually ensured that it was\nunset. test_unconfig() at the start of the test would be the\nappropriate way to unset the config. It would also make the tests more\nself-contained, in case they are ever re-ordered.\n\nMore below.\n\n> +    git log --merges=show --pretty=tformat:%s >actual &&\n> +       test_cmp both_commits_merges actual\n> +'\n> +\n> +test_expect_success 'log --merges=only' '\n> +    git log --merges=only --pretty=tformat:%s >actual &&\n> +       test_cmp only_merges actual\n> +'\n> +\n> +test_expect_success 'log --merges=hide' '\n> +    git log --merges=hide --pretty=tformat:%s >actual &&\n> +       test_cmp only_commits actual\n> +'\n> +\n> +test_expect_success 'log --merges=show with config log.merges=hide' '\n> +       git config log.merges hide &&\n> +    git log --merges=show --pretty=tformat:%s >actual &&\n> +       test_cmp both_commits_merges actual &&\n> +    git config --unset log.merges\n> +'\n> +\n> +test_expect_success 'log --merges=show with config log.merges=only' '\n> +       git config log.merges only &&\n> +    git log --merges=show --pretty=tformat:%s >actual &&\n> +       test_cmp both_commits_merges actual &&\n> +    git config --unset log.merges\n> +'\n> +\n> +test_expect_success 'log --merges=only with config log.merges=show' '\n> +       git config log.merges show &&\n> +    git log --merges=only --pretty=tformat:%s >actual &&\n> +       test_cmp only_merges actual &&\n> +    git config --unset log.merges\n> +'\n> +\n> +test_expect_success 'log --merges=only with config log.merges=hide' '\n> +       git config log.merges hide &&\n> +    git log --merges=only --pretty=tformat:%s >actual &&\n> +       test_cmp only_merges actual &&\n> +    git config --unset log.merges\n> +'\n> +\n> +test_expect_success 'log --merges=hide with config log.merges=show' '\n> +       git config log.merges show &&\n> +    git log --merges=hide --pretty=tformat:%s >actual &&\n> +       test_cmp only_commits actual &&\n> +    git config --unset log.merges\n> +'\n> +\n> +test_expect_success 'log --merges=hide with config log.merges=only' '\n> +       git config log.merges only &&\n> +    git log --merges=hide --pretty=tformat:%s >actual &&\n> +       test_cmp only_commits actual &&\n> +    git config --unset log.merges\n> +'\n\nThere's an awful lot of repetition in the implementation these new\ntests. Such repetition invites errors when composing the tests and\nmakes them difficult to review (since it's easy to be sloppy in the\nreview as well). The situation could be improved by introducing a\nhelper function which performs the tests, controlled by a few\nparameters. For instance, the helper could accept three arguments: (1)\noptional value of --merges=, (2) optional value of log.merges, (3)\nname of \"expected\" file. It also suggests that you should normalize\nthe names of the \"expected\" files (\"expect_show\", \"expect_only\",\n\"expect_hide\") so that the 3rd argument can be short and sweet. Let's\nsay that the helper is test_log_merges(), then you would invoke it as:\n\n    test_log_merges \"\" \"\" show\n    test_log_merges \"\" show show\n    test_log_merges \"\" only only\n    ...\n    test_log_merges hide \"\" hide\n    ...\n    test_log_merges only hide only\n\nfor all combination of --merges= and log.merges.\n\n> +\n>  test_expect_success 'log --graph with merge' '\n>         git log --graph --date-order --pretty=tformat:%s |\n>                 sed \"s/ *\\$//\" >actual &&\n> --\n> 2.3.3.263.g095251d.dirty\n"},{"id":"258322","messageId":"CAPig+cT-EJMBZN72N=DREZjn=c1rxgMsnJ=HNS4R3sGTUPUu0A@mail.gmail.com","threadId":"38885","inReplyTo":"550F3D3B.7080104@posteo.de","subject":"Re: [PATCH 4/5] Add tests for git-log --merges=show|hide|only","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-22T22:40:13Z","receivedAt":"2015-03-22T22:40:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 22, 2015 at 6:07 PM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:\n> On 03/22/2015 08:57 PM, Torsten Bögershausen wrote:\n>> On 22.03.15 19:28, Koosha Khajehmoogahi wrote:\n>>> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>\n>>> ---\n>>> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n>>> index 5f2b290..ab6f371 100755\n>>> --- a/t/t4202-log.sh\n>>> +++ b/t/t4202-log.sh\n>>> @@ -428,6 +428,147 @@ cat > expect <<\\EOF\n>>>  * initial\n>>>  EOF\n>>>\n>>> +test_expect_success 'log with config log.merges=show' '\n>>> +    git config log.merges show &&\n>>> +    git log --pretty=tformat:%s >actual &&\n>>> +    test_cmp both_commits_merges actual &&\n>>> +    git config --unset log.merges\n>> Do we need the unset here?\n>> The log.merges is nicely set up before each test case, so can we drop the unset lines ?\n>> (Or do I miss something ?)\n>\n> Good point; we can drop only those unset lines whose next test sets the log.merges.\n> However, if the next test does not set it, we must unset it as it affects the\n> default behavior of git-log.\n\nSuch an approach would be too fragile. Tests may be re-ordered, added,\nor removed. Better is to make each test as self-contained as possible.\nSee my review[1] of this patch for alternate suggestions.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/266099\n"},{"id":"258323","messageId":"550F450F.6070207@posteo.de","threadId":"38885","inReplyTo":"CAPig+cT-EJMBZN72N=DREZjn=c1rxgMsnJ=HNS4R3sGTUPUu0A@mail.gmail.com","subject":"Re: [PATCH 4/5] Add tests for git-log --merges=show|hide|only","fromName":"Koosha Khajehmoogahi","fromEmail":"koosha@posteo.de","sentAt":"2015-03-22T22:41:19Z","receivedAt":"2015-03-22T22:41:19Z","isPatch":true,"sender":{"key":"koosha@posteo.de","avatar":null},"body":"\n\nOn 03/22/2015 11:40 PM, Eric Sunshine wrote:\n> On Sun, Mar 22, 2015 at 6:07 PM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:\n>> On 03/22/2015 08:57 PM, Torsten Bögershausen wrote:\n>>> On 22.03.15 19:28, Koosha Khajehmoogahi wrote:\n>>>> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>\n>>>> ---\n>>>> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n>>>> index 5f2b290..ab6f371 100755\n>>>> --- a/t/t4202-log.sh\n>>>> +++ b/t/t4202-log.sh\n>>>> @@ -428,6 +428,147 @@ cat > expect <<\\EOF\n>>>>  * initial\n>>>>  EOF\n>>>>\n>>>> +test_expect_success 'log with config log.merges=show' '\n>>>> +    git config log.merges show &&\n>>>> +    git log --pretty=tformat:%s >actual &&\n>>>> +    test_cmp both_commits_merges actual &&\n>>>> +    git config --unset log.merges\n>>> Do we need the unset here?\n>>> The log.merges is nicely set up before each test case, so can we drop the unset lines ?\n>>> (Or do I miss something ?)\n>>\n>> Good point; we can drop only those unset lines whose next test sets the log.merges.\n>> However, if the next test does not set it, we must unset it as it affects the\n>> default behavior of git-log.\n> \n> Such an approach would be too fragile. Tests may be re-ordered, added,\n> or removed. Better is to make each test as self-contained as possible.\n> See my review[1] of this patch for alternate suggestions.\n> \n> [1]: http://article.gmane.org/gmane.comp.version-control.git/266099\n> \n\nThat's why I wrote them this way actually. Also, thanks for your\nreview. I will refactor my code to consider your suggestions.\n\nThanks.\n-- \nKoosha\n"},{"id":"258328","messageId":"xmqqoankbodw.fsf@gitster.dls.corp.google.com","threadId":"38885","inReplyTo":"1427048921-28677-1-git-send-email-koosha@posteo.de","subject":"Re: [PATCH 1/5] Add a new option 'merges' to revision.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-22T23:31:23Z","receivedAt":"2015-03-22T23:31:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Koosha Khajehmoogahi <koosha@posteo.de> writes:\n\n> @@ -1800,9 +1817,14 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n>  \t\trevs->show_all = 1;\n>  \t} else if (!strcmp(arg, \"--remove-empty\")) {\n>  \t\trevs->remove_empty_trees = 1;\n> +\t} else if (starts_with(arg, \"--merges=\")) {\n> +\t\tif (parse_merges_opt(revs, arg + 9))\n> +\t\t\tdie(\"unknown option: %s\", arg);\n\nThis one makes sense to me (so does what the parse_merges_opt()\ndoes).\n\n>  \t} else if (!strcmp(arg, \"--merges\")) {\n> +\t\trevs->max_parents = -1;\n>  \t\trevs->min_parents = 2;\n\nBut is this change warranted?  An existing user who is not at all\ninterested in the new --merges= option may be relying on the fact\nthat \"--merges\" does not affect the value of max_parents and she can\nsay \"log --max-parents=2 --merges\" to see only the two-way merges,\nfor example.  This change just broke her, and I do not see why it is\na good thing.\n\n>  \t} else if (!strcmp(arg, \"--no-merges\")) {\n> +\t\trevs->min_parents = 0;\n>  \t\trevs->max_parents = 1;\n\nLikewise.\n"},{"id":"258332","messageId":"550F6175.9050606@posteo.de","threadId":"38885","inReplyTo":"xmqqoankbodw.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/5] Add a new option 'merges' to revision.c","fromName":"Koosha Khajehmoogahi","fromEmail":"koosha@posteo.de","sentAt":"2015-03-23T00:42:29Z","receivedAt":"2015-03-23T00:42:29Z","isPatch":true,"sender":{"key":"koosha@posteo.de","avatar":null},"body":"\n\nOn 03/23/2015 12:31 AM, Junio C Hamano wrote:\n> Koosha Khajehmoogahi <koosha@posteo.de> writes:\n> \n>> @@ -1800,9 +1817,14 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n>>  \t\trevs->show_all = 1;\n>>  \t} else if (!strcmp(arg, \"--remove-empty\")) {\n>>  \t\trevs->remove_empty_trees = 1;\n>> +\t} else if (starts_with(arg, \"--merges=\")) {\n>> +\t\tif (parse_merges_opt(revs, arg + 9))\n>> +\t\t\tdie(\"unknown option: %s\", arg);\n> \n> This one makes sense to me (so does what the parse_merges_opt()\n> does).\n\nIn fact this change was written by you in your previous kind review :-)\n\nI will add a 'From: Junio C Hamano <gitster@pobox.com>' header to my next patch.\n\n> \n>>  \t} else if (!strcmp(arg, \"--merges\")) {\n>> +\t\trevs->max_parents = -1;\n>>  \t\trevs->min_parents = 2;\n> \n> But is this change warranted?  An existing user who is not at all\n> interested in the new --merges= option may be relying on the fact\n> that \"--merges\" does not affect the value of max_parents and she can\n> say \"log --max-parents=2 --merges\" to see only the two-way merges,\n> for example.  This change just broke her, and I do not see why it is\n> a good thing.\n\nThe point is that if you have your log.merges conf option set to 'hide'\nand you use git log --merges (two mutually conflicting options),\ngit will silently exit without anything to show. We need to clear the\nmax_parents set by parse_merges_opt() as the user should be able to continue\nto use --merges without problem. But, your point is also valid.\n\n> \n>>  \t} else if (!strcmp(arg, \"--no-merges\")) {\n>> +\t\trevs->min_parents = 0;\n>>  \t\trevs->max_parents = 1;\n> \n> Likewise.\n> \n\nSimilarly, without this, if log.merges is set to 'only' and you use git log --no-merges,\nyou will still see the merges in the output.\n"},{"id":"258334","messageId":"20150323022333.Horde.P0Kh2JxuSiUs9IhG94BnsQ1@webmail.informatik.kit.edu","threadId":"38885","inReplyTo":"1427048921-28677-5-git-send-email-koosha@posteo.de","subject":"Re: [PATCH 5/5] Update Bash completion script to include git log --merges option","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-03-23T01:23:33Z","receivedAt":"2015-03-23T01:23:33Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\nI agree with Eric's comment about the subject line.\n\nQuoting Koosha Khajehmoogahi <koosha@posteo.de>:\n\n> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>\n> ---\n>   contrib/completion/git-completion.bash | 11 ++++++++++-\n>   1 file changed, 10 insertions(+), 1 deletion(-)\n>\n> diff --git a/contrib/completion/git-completion.bash\n> b/contrib/completion/git-completion.bash\n> index 731c289..b63bb95 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -1406,7 +1406,7 @@ _git_ls_tree ()\n>   __git_log_common_options=\"\n>   \t--not --all\n>   \t--branches --tags --remotes\n> -\t--first-parent --merges --no-merges\n> +\t--first-parent --merges --merges= --no-merges\n>   \t--max-count=\n>   \t--max-age= --since= --after=\n>   \t--min-age= --until= --before=\n> @@ -1451,6 +1451,10 @@ _git_log ()\n>   \t\t__gitcomp \"long short\" \"\" \"${cur##--decorate=}\"\n>   \t\treturn\n>   \t\t;;\n> +    --merges=*)\n> +        __gitcomp \"show hide only\" \"\" \"${cur##--merges=}\"\n> +        return\n> +        ;;\n\nFunny indentation, please use tabs instead of spaces.\n\nOtherwise this patch looks good.\n\n>   \t--*)\n>   \t\t__gitcomp \"\n>   \t\t\t$__git_log_common_options\n> @@ -1861,6 +1865,10 @@ _git_config ()\n>   \t\t__gitcomp \"$__git_log_date_formats\"\n>   \t\treturn\n>   \t\t;;\n> +\tlog.merges)\n> +\t\t__gitcomp \"show hide only\"\n> +\t\treturn\n> +\t\t;;\n>   \tsendemail.aliasesfiletype)\n>   \t\t__gitcomp \"mutt mailrc pine elm gnus\"\n>   \t\treturn\n> @@ -2150,6 +2158,7 @@ _git_config ()\n>   \t\tinteractive.singlekey\n>   \t\tlog.date\n>   \t\tlog.decorate\n> +\t\tlog.merges\n>   \t\tlog.showroot\n>   \t\tmailmap.file\n>   \t\tman.\n> --\n> 2.3.3.263.g095251d.dirty\n"},{"id":"258335","messageId":"xmqqegogbj4c.fsf@gitster.dls.corp.google.com","threadId":"38885","inReplyTo":"550F6175.9050606@posteo.de","subject":"Re: [PATCH 1/5] Add a new option 'merges' to revision.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-23T01:25:07Z","receivedAt":"2015-03-23T01:25:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Koosha Khajehmoogahi <koosha@posteo.de> writes:\n\n>>>  \t} else if (!strcmp(arg, \"--merges\")) {\n>>> +\t\trevs->max_parents = -1;\n>>>  \t\trevs->min_parents = 2;\n>> \n>> But is this change warranted?  An existing user who is not at all\n>> interested in the new --merges= option may be relying on the fact\n>> that \"--merges\" does not affect the value of max_parents and she can\n>> say \"log --max-parents=2 --merges\" to see only the two-way merges,\n>> for example.  This change just broke her, and I do not see why it is\n>> a good thing.\n>\n> The point is that if you have your log.merges conf option set to 'hide'\n> and you use git log --merges (two mutually conflicting options),\n> git will silently exit without anything to show.\n\nThat is not an excuse to break \"--merges\" and \"--no-merges\" for\nexisting users who do not care about setting log.merges option to\nanything.\n\nThe whole point of introducing a new \"--merges=show\" option was so\nthat people can sanely countermand log.merges configuration from the\ncommand line without breaking --merges and --no-merges, wasn't it?\n"},{"id":"258348","messageId":"550FA130.1030402@web.de","threadId":"38885","inReplyTo":"550F450F.6070207@posteo.de","subject":"Re: [PATCH 4/5] Add tests for git-log --merges=show|hide|only","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-03-23T05:14:24Z","receivedAt":"2015-03-23T05:14:24Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"Back to the original discussion:\n\n+test_expect_success 'log with config log.merges=show' '\n+    git config log.merges show &&\n+    git log --pretty=tformat:%s >actual &&\n+    test_cmp both_commits_merges actual &&\n+    git config --unset log.merges\n\nThese days I would probably shorten the code, the review time and\nthe execution time of the test and increase the clean-ness with 50%\nby simply writing\n\n+test_expect_success 'log with config log.merges=show' '\n+\tgit -c log.merges=show log --pretty=tformat:%s >actual &&\n+\ttest_cmp both_commits_merges actual\n+\t'\n"}]}