{"thread":{"id":"27241","subject":"[PATCH 2] Add default merge options for all branches","startedAt":"2011-05-02T19:23:49Z","lastAt":"2011-05-09T15:39:39Z","messageCount":46,"participants":["Michael Grubb","Miklos Vajna","Junio C Hamano","Jonathan Nieder","Jens Lehmann","John Szakmeister","Jeff King","Thiago Farina"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"166897","messageId":"4DBF04C5.1080608@dailyvoid.com","threadId":"27241","inReplyTo":null,"subject":"[PATCH 2] Add default merge options for all branches","fromName":"Michael Grubb","fromEmail":"devel@dailyvoid.com","sentAt":"2011-05-02T19:23:49Z","receivedAt":"2011-05-02T19:23:49Z","isPatch":true,"sender":{"key":"devel@dailyvoid.com","avatar":"https://gravatar.com/avatar/5ff035e599312653f041129e68041a9078c7e1f3268eaac3899c04e353d2133e?d=mp&s=160"},"body":"Add support for branch.*.mergeoptions for setting default options for\nall branches. This new value shares semantics with the existing\nbranch..mergeoptions variable. If a branch specific value is\nfound, that value will be used.\n\nThe need for this arises from the fact that there is currently not an\neasy way to set merge options for all branches. Instead of having to\nspecify merge options for each individual branch there should be a way\nto set defaults for all branches and then override a specific branch's\noptions.\n\nThe approach taken is to make note of whether a branch specific\nmergeoptions key has been seen and only apply the global value if it\nhasn't.\n\nSigned-off-by: Michael Grubb \n---\n Documentation/git-merge.txt | 3 +++\n builtin/merge.c | 27 ++++++++++++++++++++++-----\n t/t7600-merge.sh | 27 +++++++++++++++++++++++++++\n 3 files changed, 52 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\nindex e2e6aba..eaab3e4 100644\n--- a/Documentation/git-merge.txt\n+++ b/Documentation/git-merge.txt\n@@ -307,6 +307,9 @@ branch..mergeoptions::\n \tSets default options for merging into branch . The syntax and\n \tsupported options are the same as those of 'git merge', but option\n \tvalues containing whitespace characters are currently not supported.\n+\tThe special value '*' for may be used to configure default\n+\toptions for all branches. Values for specific branch names will\n+\toverride the this default.\n \n SEE ALSO\n --------\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 0bdd19a..9e5b6bd 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -505,9 +505,18 @@ cleanup:\n \n static int git_merge_config(const char *k, const char *v, void *cb)\n {\n-\tif (branch && !prefixcmp(k, \"branch.\") &&\n-\t\t!prefixcmp(k + 7, branch) &&\n-\t\t!strcmp(k + 7 + strlen(branch), \".mergeoptions\")) {\n+\tstatic int branch_merge_options_set = 0;\n+\tint merge_option_mode = 0;\n+\n+\tif (!strcmp(k, \"branch.*.mergeoptions\"))\n+\t\tmerge_option_mode = 1;\n+\telse if (branch && !prefixcmp(k, \"branch.\") &&\n+\t\t\t !prefixcmp(k + 7, branch) &&\n+\t\t\t !strcmp(k + 7 + strlen(branch), \".mergeoptions\"))\n+\t\tmerge_option_mode = 2;\n+\n+\tif ((merge_option_mode == 1 && !branch_merge_options_set) ||\n+\t\t merge_option_mode == 2) {\n \t\tconst char **argv;\n \t\tint argc;\n \t\tchar *buf;\n@@ -515,14 +524,22 @@ static int git_merge_config(const char *k, const char *v, void *cb)\n \t\tbuf = xstrdup(v);\n \t\targc = split_cmdline(buf, &argv);\n \t\tif (argc < 0)\n-\t\t\tdie(_(\"Bad branch.%s.mergeoptions string: %s\"), branch,\n-\t\t\t split_cmdline_strerror(argc));\n+\t\t{\n+\t\t\tif (merge_option_mode == 1)\n+\t\t\t\tdie(_(\"Bad merge.mergeoptions string: %s\"), \n+\t\t\t\t\tsplit_cmdline_strerror(argc));\n+\t\t\telse\n+\t\t\t\tdie(_(\"Bad branch.%s.mergeoptions string: %s\"), branch,\n+\t\t\t\t\tsplit_cmdline_strerror(argc));\n+\t\t}\n \t\targv = xrealloc(argv, sizeof(*argv) * (argc + 2));\n \t\tmemmove(argv + 1, argv, sizeof(*argv) * (argc + 1));\n \t\targc++;\n \t\tparse_options(argc, argv, NULL, builtin_merge_options,\n \t\t\t builtin_merge_usage, 0);\n \t\tfree(buf);\n+\t\tif (merge_option_mode == 2)\n+\t\t\tbranch_merge_options_set = 1;\n \t}\n \n \tif (!strcmp(k, \"merge.diffstat\") || !strcmp(k, \"merge.stat\"))\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 87d5d78..bfb7348 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -415,6 +415,33 @@ test_expect_success 'merge c0 with c1 (no-ff)' '\n \n test_debug 'git log --graph --decorate --oneline --all'\n \n+test_expect_success 'merge c0 with c1 (global no-ff)' '\n+\tgit reset --hard c0 &&\n+\tgit config --unset branch.master.mergeoptions &&\n+\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n+\ttest_tick &&\n+\tgit merge c1 &&\n+\tgit config --remove-section \"branch.*\" &&\n+\tverify_merge file result.1 &&\n+\tverify_parents $c0 $c1\n+'\n+\n+test_debug 'git log --graph --decorate --oneline --all'\n+\n+test_expect_success 'combine merge.mergeoptions with branch.x.mergeoptions' '\n+\tgit reset --hard c0 &&\n+\tgit config --remove-section branch.master &&\n+\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n+\tgit config branch.master.mergeoptions \"--ff\" &&\n+\ttest_tick &&\n+\tgit merge c1 &&\n+\tgit config --remove-section \"branch.*\" &&\n+\tverify_merge file result.1 &&\n+\tverify_parents \"$c0\"\n+'\n+\n+test_debug 'git log --graph --decorate --oneline --all'\n+\n test_expect_success 'combining --squash and --no-ff is refused' '\n \ttest_must_fail git merge --squash --no-ff c1 &&\n \ttest_must_fail git merge --no-ff --squash c1\n-- \n1.7.5\n"},{"id":"166914","messageId":"20110502224735.GU21056@genesis.frugalware.org","threadId":"27241","inReplyTo":"4DBF04C5.1080608@dailyvoid.com","subject":"Re: [PATCH 2] Add default merge options for all branches","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2011-05-02T22:47:35Z","receivedAt":"2011-05-02T22:47:35Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Mon, May 02, 2011 at 02:23:49PM -0500, Michael Grubb wrote:\n> \t\tif (argc < 0)\n> -\t\t\tdie(_(\"Bad branch.%s.mergeoptions string: %s\"), branch,\n> -\t\t\t split_cmdline_strerror(argc));\n> +\t\t{\n> +\t\t\tif (merge_option_mode == 1)\n\ncheckpatch.pl from the kernel tree may help you - { belongs to the end\nof the previous line.\n\nThanks.\n"},{"id":"166915","messageId":"7voc3kk748.fsf@alter.siamese.dyndns.org","threadId":"27241","inReplyTo":"4DBF04C5.1080608@dailyvoid.com","subject":"Re: [PATCH 2] Add default merge options for all branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-02T23:36:23Z","receivedAt":"2011-05-02T23:36:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Grubb writes:\n\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 0bdd19a..9e5b6bd 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -505,9 +505,18 @@ cleanup:\n> \n> static int git_merge_config(const char *k, const char *v, void *cb)\n> {\n> -\tif (branch && !prefixcmp(k, \"branch.\") &&\n> -\t\t!prefixcmp(k + 7, branch) &&\n> -\t\t!strcmp(k + 7 + strlen(branch), \".mergeoptions\")) {\n> +\tstatic int branch_merge_options_set = 0;\n\nI prefer to avoid \"static int\" that you cannot easily clear here. It\nwould make it impossible to call the function twice.\n\nI think it is easily doable by using the callback parameter (cb).\n\nI am also wondering how this will scale, both in the direction of \"later\nit is likely that we would want to support a glob not just '*' here\", and\nalso \"later it is likely that we would want to support other per-branch\nvariables, not just \"mergeoptions\" here\".\n"},{"id":"166919","messageId":"4DBF9426.4090402@dailyvoid.com","threadId":"27241","inReplyTo":"7voc3kk748.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2] Add default merge options for all branches","fromName":"Michael Grubb","fromEmail":"devel@dailyvoid.com","sentAt":"2011-05-03T05:35:34Z","receivedAt":"2011-05-03T05:35:34Z","isPatch":true,"sender":{"key":"devel@dailyvoid.com","avatar":"https://gravatar.com/avatar/5ff035e599312653f041129e68041a9078c7e1f3268eaac3899c04e353d2133e?d=mp&s=160"},"body":"\nOn 5/2/11 6:36 PM, Junio C Hamano wrote:\n> Michael Grubb writes:\n> \n>> diff --git a/builtin/merge.c b/builtin/merge.c\n>> index 0bdd19a..9e5b6bd 100644\n>> --- a/builtin/merge.c\n>> +++ b/builtin/merge.c\n>> @@ -505,9 +505,18 @@ cleanup:\n>> \n>> static int git_merge_config(const char *k, const char *v, void *cb)\n>> {\n>> -\tif (branch && !prefixcmp(k, \"branch.\") &&\n>> -\t\t!prefixcmp(k + 7, branch) &&\n>> -\t\t!strcmp(k + 7 + strlen(branch), \".mergeoptions\")) {\n>> +\tstatic int branch_merge_options_set = 0;\n> \n> I prefer to avoid \"static int\" that you cannot easily clear here. It\n> would make it impossible to call the function twice.\n>\n> I think it is easily doable by using the callback parameter (cb).\n>\nToo right. I've reworked that bit to use the cb parameter. With an eye\nto the future I've created a new struct for this for future's sake, so\nthat this one feature doesn't monopolize the cb parameter in the future.\n \n> I am also wondering how this will scale, both in the direction of \"later\n> it is likely that we would want to support a glob not just '*' here\", and\n> also \"later it is likely that we would want to support other per-branch\n> variables, not just \"mergeoptions\" here\".\nIt seems that the easiest way would be to move this particular feature out into\nit's own function at some point to pave the way for doing more complex things with\nthe branch configurations.\n\nNew patch forthcoming.\n\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"166920","messageId":"4DBF94E9.2010502@dailyvoid.com","threadId":"27241","inReplyTo":"4DBF04C5.1080608@dailyvoid.com","subject":"[PATCH v3] Add default merge options for all branches","fromName":"Michael Grubb","fromEmail":"devel@dailyvoid.com","sentAt":"2011-05-03T05:38:49Z","receivedAt":"2011-05-03T05:38:49Z","isPatch":true,"sender":{"key":"devel@dailyvoid.com","avatar":"https://gravatar.com/avatar/5ff035e599312653f041129e68041a9078c7e1f3268eaac3899c04e353d2133e?d=mp&s=160"},"body":"Add support for branch.*.mergeoptions for setting default options for\nall branches. This new value shares semantics with the existing\nbranch..mergeoptions variable. If a branch specific value is\nfound, that value will be used.\n\nThe need for this arises from the fact that there is currently not an\neasy way to set merge options for all branches. Instead of having to\nspecify merge options for each individual branch there should be a way\nto set defaults for all branches and then override a specific branch's\noptions.\n\nThe approach taken is to make note of whether a branch specific\nmergeoptions key has been seen and only apply the global value if it\nhasn't.\n\nSigned-off-by: Michael Grubb \n---\n Documentation/git-merge.txt | 3 +++\n builtin/merge.c | 40 +++++++++++++++++++++++++++++++++-------\n t/t7600-merge.sh | 27 +++++++++++++++++++++++++++\n 3 files changed, 63 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\nindex e2e6aba..eaab3e4 100644\n--- a/Documentation/git-merge.txt\n+++ b/Documentation/git-merge.txt\n@@ -307,6 +307,9 @@ branch..mergeoptions::\n \tSets default options for merging into branch . The syntax and\n \tsupported options are the same as those of 'git merge', but option\n \tvalues containing whitespace characters are currently not supported.\n+\tThe special value '*' for may be used to configure default\n+\toptions for all branches. Values for specific branch names will\n+\toverride the this default.\n \n SEE ALSO\n --------\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex d171c63..9fe129f 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -32,6 +32,13 @@\n #define NO_FAST_FORWARD (1<<2)\n #define NO_TRIVIAL (1<<3)\n \n+#define MERGEOPTIONS_DEFAULT (1<<0)\n+#define MERGEOPTIONS_BRANCH (1<<1)\n+\n+struct merge_options_cb {\n+\tint override_default;\n+};\n+\n struct strategy {\n \tconst char *name;\n \tunsigned attr;\n@@ -505,24 +512,42 @@ cleanup:\n \n static int git_merge_config(const char *k, const char *v, void *cb)\n {\n-\tif (branch && !prefixcmp(k, \"branch.\") &&\n-\t\t!prefixcmp(k + 7, branch) &&\n-\t\t!strcmp(k + 7 + strlen(branch), \".mergeoptions\")) {\n+\tint merge_option_mode = 0;\n+\tstruct merge_options_cb *merge_options =\n+\t\t(struct merge_options_cb *)cb;\n+\n+\tif (!strcmp(k, \"branch.*.mergeoptions\"))\n+\t\tmerge_option_mode = MERGEOPTIONS_DEFAULT;\n+\telse if (branch && !prefixcmp(k, \"branch.\") &&\n+\t\t\t !prefixcmp(k + 7, branch) &&\n+\t\t\t !strcmp(k + 7 + strlen(branch), \".mergeoptions\"))\n+\t\tmerge_option_mode = MERGEOPTIONS_BRANCH;\n+\n+\tif ((merge_option_mode == MERGEOPTIONS_DEFAULT &&\n+\t\t!merge_options->override_default) ||\n+\t\tmerge_option_mode == MERGEOPTIONS_BRANCH) {\n \t\tconst char **argv;\n \t\tint argc;\n \t\tchar *buf;\n \n \t\tbuf = xstrdup(v);\n \t\targc = split_cmdline(buf, &argv);\n-\t\tif (argc < 0)\n-\t\t\tdie(_(\"Bad branch.%s.mergeoptions string: %s\"), branch,\n-\t\t\t split_cmdline_strerror(argc));\n+\t\tif (argc < 0) {\n+\t\t\tif (merge_option_mode == 1)\n+\t\t\t\tdie(_(\"Bad merge.mergeoptions string: %s\"), \n+\t\t\t\t\tsplit_cmdline_strerror(argc));\n+\t\t\telse\n+\t\t\t\tdie(_(\"Bad branch.%s.mergeoptions string: %s\"), branch,\n+\t\t\t\t\tsplit_cmdline_strerror(argc));\n+\t\t}\n \t\targv = xrealloc(argv, sizeof(*argv) * (argc + 2));\n \t\tmemmove(argv + 1, argv, sizeof(*argv) * (argc + 1));\n \t\targc++;\n \t\tparse_options(argc, argv, NULL, builtin_merge_options,\n \t\t\t builtin_merge_usage, 0);\n \t\tfree(buf);\n+\t\tif (merge_option_mode == MERGEOPTIONS_BRANCH)\n+\t\t\tmerge_options->override_default = 1;\n \t}\n \n \tif (!strcmp(k, \"merge.diffstat\") || !strcmp(k, \"merge.stat\"))\n@@ -987,6 +1012,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tconst char *head_arg;\n \tint flag, head_invalid = 0, i;\n \tint best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;\n+\tstruct merge_options_cb merge_options = {0};\n \tstruct commit_list *common = NULL;\n \tconst char *best_strategy = NULL, *wt_strategy = NULL;\n \tstruct commit_list **remotes = &remoteheads;\n@@ -1004,7 +1030,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tif (is_null_sha1(head))\n \t\thead_invalid = 1;\n \n-\tgit_config(git_merge_config, NULL);\n+\tgit_config(git_merge_config, &merge_options);\n \n \t/* for color.ui */\n \tif (diff_use_color_default == -1)\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex e84e822..cea2b31 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -415,6 +415,33 @@ test_expect_success 'merge c0 with c1 (no-ff)' '\n \n test_debug 'git log --graph --decorate --oneline --all'\n \n+test_expect_success 'merge c0 with c1 (global no-ff)' '\n+\tgit reset --hard c0 &&\n+\tgit config --unset branch.master.mergeoptions &&\n+\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n+\ttest_tick &&\n+\tgit merge c1 &&\n+\tgit config --remove-section \"branch.*\" &&\n+\tverify_merge file result.1 &&\n+\tverify_parents $c0 $c1\n+'\n+\n+test_debug 'git log --graph --decorate --oneline --all'\n+\n+test_expect_success 'combine merge.mergeoptions with branch.x.mergeoptions' '\n+\tgit reset --hard c0 &&\n+\tgit config --remove-section branch.master &&\n+\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n+\tgit config branch.master.mergeoptions \"--ff\" &&\n+\ttest_tick &&\n+\tgit merge c1 &&\n+\tgit config --remove-section \"branch.*\" &&\n+\tverify_merge file result.1 &&\n+\tverify_parents \"$c0\"\n+'\n+\n+test_debug 'git log --graph --decorate --oneline --all'\n+\n test_expect_success 'combining --squash and --no-ff is refused' '\n \ttest_must_fail git merge --squash --no-ff c1 &&\n \ttest_must_fail git merge --no-ff --squash c1\n-- \n1.7.5\n"},{"id":"166930","messageId":"20110503090351.GA27862@elie","threadId":"27241","inReplyTo":"4DBF94E9.2010502@dailyvoid.com","subject":"Re: [PATCH v3] Add default merge options for all branches","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-03T09:03:52Z","receivedAt":"2011-05-03T09:03:52Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMichael Grubb wrote:\n\n> Add support for branch.*.mergeoptions for setting default options for\n> all branches. This new value shares semantics with the existing\n> branch..mergeoptions variable. If a branch specific value is\n> found, that value will be used.\n\nSo in the future one might be able to do things like\n\n\t[branch \"git-gui/*\"]\n\t\tmergeoptions = -s subtree\n\nInteresting.\n\n> The need for this arises from the fact that there is currently not an\n> easy way to set merge options for all branches.\n\nI'm curious: what merge options/workflows does this tend to be useful\nfor? The above explanation seems a bit abstract (though already\nconvincing).\n\n> The approach taken is to make note of whether a branch specific\n> mergeoptions key has been seen and only apply the global value if it\n> hasn't.\n\nWhat happens if the global value is seen first?\n\nOn to the code. Warning: nitpicks ahead.\n\n[...]\n> +++ b/builtin/merge.c\n> @@ -32,6 +32,13 @@\n> #define NO_FAST_FORWARD (1<<2)\n> #define NO_TRIVIAL (1<<3)\n> \n> +#define MERGEOPTIONS_DEFAULT (1<<0)\n> +#define MERGEOPTIONS_BRANCH (1<<1)\n\nAre these bitflags?\n\n> @@ -505,24 +512,42 @@ cleanup:\n> \n> static int git_merge_config(const char *k, const char *v, void *cb)\n> {\n> +\tint merge_option_mode = 0;\n> +\tstruct merge_options_cb *merge_options =\n> +\t\t(struct merge_options_cb *)cb;\n\nThis cast should not needed, I'd think.\n\n[...]\n> -\tif (branch && !prefixcmp(k, \"branch.\") &&\n> -\t\t!prefixcmp(k + 7, branch) &&\n> -\t\t!strcmp(k + 7 + strlen(branch), \".mergeoptions\")) {\n> +\tif (!strcmp(k, \"branch.*.mergeoptions\"))\n> +\t\tmerge_option_mode = MERGEOPTIONS_DEFAULT;\n> +\telse if (branch && !prefixcmp(k, \"branch.\") &&\n> +\t\t\t !prefixcmp(k + 7, branch) &&\n> +\t\t\t !strcmp(k + 7 + strlen(branch), \".mergeoptions\"))\n> +\t\tmerge_option_mode = MERGEOPTIONS_BRANCH;\n> +\n> +\tif ((merge_option_mode == MERGEOPTIONS_DEFAULT &&\n> +\t\t!merge_options->override_default) ||\n> +\t\tmerge_option_mode == MERGEOPTIONS_BRANCH) {\n> \t\tconst char **argv;\n\nIt is hard to see at a glance where the \"if\" condition ends and\nthe body begins. Why not\n\n\tif ((merge_option_mode == MERGEOPTIONS_DEFAULT &&\n\t !merge_options->override_default) ||\n\t merge_option_mode == MERGEOPTIONS_BRANCH) {\n\t\tconst char **argv;\n\t\t...\n\nor\n\n\tif (merge_option_mode == MERGEOPTIONS_BRANCH ? 1 :\n\t merge_option_mode == MERGEOPTIONS_DEFAULT ?\n\t\t\t!merge_options->override_default : 0) {\n\t\tconst char **argv;\n\t\t...\n\nor even\n\n\tif (merge_option_mode == MERGEOPTIONS_DEFAULT &&\n\t merge_options->override_default)\n\t\tmerge_option_mode = 0;\n\n\tif (merge_option_mode) {\n\t\tconst char **argv;\n\t\t...\n\n?\n\t \n> \t\tint argc;\n> \t\tchar *buf;\n> \n> \t\tbuf = xstrdup(v);\n> \t\targc = split_cmdline(buf, &argv);\n> -\t\tif (argc < 0)\n> -\t\t\tdie(_(\"Bad branch.%s.mergeoptions string: %s\"), branch,\n> -\t\t\t split_cmdline_strerror(argc));\n> +\t\tif (argc < 0) {\n> +\t\t\tif (merge_option_mode == 1)\n> +\t\t\t\tdie(_(\"Bad merge.mergeoptions string: %s\"), \n> +\t\t\t\t\tsplit_cmdline_strerror(argc));\n\nmerge.*.mergeoptions, no?\n\n> +\t\t\telse\n> +\t\t\t\tdie(_(\"Bad branch.%s.mergeoptions string: %s\"), branch,\n> +\t\t\t\t\tsplit_cmdline_strerror(argc));\n> +\t\t}\n> \t\targv = xrealloc(argv, sizeof(*argv) * (argc + 2));\n> \t\tmemmove(argv + 1, argv, sizeof(*argv) * (argc + 1));\n> \t\targc++;\n> \t\tparse_options(argc, argv, NULL, builtin_merge_options,\n> \t\t\t builtin_merge_usage, 0);\n> \t\tfree(buf);\n> +\t\tif (merge_option_mode == MERGEOPTIONS_BRANCH)\n> +\t\t\tmerge_options->override_default = 1;\n\nCould be clearer to put this next to the code that checks\noverride_default.\n\n[...]\n> --- a/t/t7600-merge.sh\n> +++ b/t/t7600-merge.sh\n> @@ -415,6 +415,33 @@ test_expect_success 'merge c0 with c1 (no-ff)' '\n> \n> test_debug 'git log --graph --decorate --oneline --all'\n> \n> +test_expect_success 'merge c0 with c1 (global no-ff)' '\n> +\tgit reset --hard c0 &&\n> +\tgit config --unset branch.master.mergeoptions &&\n\nBetter to make that\n\n\ttest_might_fail git config --unset ...\n\nso it will still work if earlier tests stop setting that\nvariable.\n\n> +\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n> +\ttest_tick &&\n> +\tgit merge c1 &&\n> +\tgit config --remove-section \"branch.*\" &&\n> +\tverify_merge file result.1 &&\n> +\tverify_parents $c0 $c1\n> +'\n> +\n> +test_debug 'git log --graph --decorate --oneline --all'\n\nYuck. Did anything come of the idea of a --between-tests option to\nuse an arbitrary command here automatically? (Not your fault.) \n\n> +\n> +test_expect_success 'combine merge.mergeoptions with branch.x.mergeoptions' '\n> +\tgit reset --hard c0 &&\n> +\tgit config --remove-section branch.master &&\n\nCould make sense to use test_might_fail for this one, too.\n\n> +\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n> +\tgit config branch.master.mergeoptions \"--ff\" &&\n> +\ttest_tick &&\n> +\tgit merge c1 &&\n> +\tgit config --remove-section \"branch.*\" &&\n> +\tverify_merge file result.1 &&\n> +\tverify_parents \"$c0\"\n> +'\n> +\n> +test_debug 'git log --graph --decorate --oneline --all'\n\nNice, a clean patch with a few reasonable tests.\n\nWith whichever of the changes below make sense,\nReviewed-by: Jonathan Nieder \n\nThanks.\n---\n builtin/merge.c | 37 ++++++++++++++++++++-----------------\n t/t7600-merge.sh | 4 ++--\n 2 files changed, 22 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 9fe129f..7156e92 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -32,8 +32,8 @@\n #define NO_FAST_FORWARD (1<<2)\n #define NO_TRIVIAL (1<<3)\n \n-#define MERGEOPTIONS_DEFAULT (1<<0)\n-#define MERGEOPTIONS_BRANCH (1<<1)\n+#define MERGEOPTIONS_DEFAULT 1\n+#define MERGEOPTIONS_BRANCH 2\n \n struct merge_options_cb {\n \tint override_default;\n@@ -513,8 +513,7 @@ cleanup:\n static int git_merge_config(const char *k, const char *v, void *cb)\n {\n \tint merge_option_mode = 0;\n-\tstruct merge_options_cb *merge_options =\n-\t\t(struct merge_options_cb *)cb;\n+\tstruct merge_options_cb *merge_options = cb;\n \n \tif (!strcmp(k, \"branch.*.mergeoptions\"))\n \t\tmerge_option_mode = MERGEOPTIONS_DEFAULT;\n@@ -523,31 +522,35 @@ static int git_merge_config(const char *k, const char *v, void *cb)\n \t\t\t !strcmp(k + 7 + strlen(branch), \".mergeoptions\"))\n \t\tmerge_option_mode = MERGEOPTIONS_BRANCH;\n \n-\tif ((merge_option_mode == MERGEOPTIONS_DEFAULT &&\n-\t\t!merge_options->override_default) ||\n-\t\tmerge_option_mode == MERGEOPTIONS_BRANCH) {\n+\t/*\n+\t * If an applicable [branch \"foo\"] mergeoptions setting was\n+\t * seen already, let it mask the [branch \"*\"] defaults.\n+\t */\n+\tif (merge_options->override_default &&\n+\t merge_option_mode == MERGEOPTIONS_DEFAULT)\n+\t\tmerge_option_mode = 0;\n+\n+\tif (merge_option_mode == MERGEOPTIONS_BRANCH)\n+\t\tmerge_options->override_default = 1;\n+\n+\tif (merge_option_mode) {\n \t\tconst char **argv;\n \t\tint argc;\n \t\tchar *buf;\n \n \t\tbuf = xstrdup(v);\n \t\targc = split_cmdline(buf, &argv);\n-\t\tif (argc < 0) {\n-\t\t\tif (merge_option_mode == 1)\n-\t\t\t\tdie(_(\"Bad merge.mergeoptions string: %s\"), \n-\t\t\t\t\tsplit_cmdline_strerror(argc));\n-\t\t\telse\n-\t\t\t\tdie(_(\"Bad branch.%s.mergeoptions string: %s\"), branch,\n-\t\t\t\t\tsplit_cmdline_strerror(argc));\n-\t\t}\n+\t\tif (argc < 0)\n+\t\t\tdie(_(\"Bad merge.%s.mergeoptions string: %s\"), \n+\t\t\t merge_option_mode == MERGEOPTIONS_DEFAULT ?\n+\t\t\t\t\t\t\t\"*\" : branch,\n+\t\t\t split_cmdline_strerror(argc));\n \t\targv = xrealloc(argv, sizeof(*argv) * (argc + 2));\n \t\tmemmove(argv + 1, argv, sizeof(*argv) * (argc + 1));\n \t\targc++;\n \t\tparse_options(argc, argv, NULL, builtin_merge_options,\n \t\t\t builtin_merge_usage, 0);\n \t\tfree(buf);\n-\t\tif (merge_option_mode == MERGEOPTIONS_BRANCH)\n-\t\t\tmerge_options->override_default = 1;\n \t}\n \n \tif (!strcmp(k, \"merge.diffstat\") || !strcmp(k, \"merge.stat\"))\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex cea2b31..ff807f4 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -417,7 +417,7 @@ test_debug 'git log --graph --decorate --oneline --all'\n \n test_expect_success 'merge c0 with c1 (global no-ff)' '\n \tgit reset --hard c0 &&\n-\tgit config --unset branch.master.mergeoptions &&\n+\ttest_might_fail git config --unset branch.master.mergeoptions &&\n \tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n \ttest_tick &&\n \tgit merge c1 &&\n@@ -430,7 +430,7 @@ test_debug 'git log --graph --decorate --oneline --all'\n \n test_expect_success 'combine merge.mergeoptions with branch.x.mergeoptions' '\n \tgit reset --hard c0 &&\n-\tgit config --remove-section branch.master &&\n+\ttest_might_fail git config --remove-section branch.master &&\n \tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n \tgit config branch.master.mergeoptions \"--ff\" &&\n \ttest_tick &&\n-- \n1.7.5\n"},{"id":"166931","messageId":"20110503094943.GA32714@elie","threadId":"27241","inReplyTo":"20110503090351.GA27862@elie","subject":"Re: [PATCH v3] Add default merge options for all branches","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-03T09:49:44Z","receivedAt":"2011-05-03T09:49:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Michael Grubb wrote:\n\n>> The approach taken is to make note of whether a branch specific\n>> mergeoptions key has been seen and only apply the global value if it\n>> hasn't.\n>\n> What happens if the global value is seen first?\n[...]\n> With whichever of the changes below make sense,\n> Reviewed-by: Jonathan Nieder \n\nAgh, what I meant is \"except as noted above\". I am still worried\nabout (and have not carefully reviewed) the following case:\n\n\t[branch \"*\"]\n\t\tmergeoptions = --foo\n\n\t[branch \"topic\"]\n\t\tmergeoptions = --bar\n\nDoes it produce different behavior from\n\n\t[branch \"topic\"]\n\t\tmergeoptions = --bar\n\n\t[branch \"*\"]\n\t\tmergeoptions = --foo\n\n? If so, is that a good thing, and is it documented?\n\nSorry for the lack of clarity.\n"},{"id":"166947","messageId":"4DC03182.505@dailyvoid.com","threadId":"27241","inReplyTo":"20110503090351.GA27862@elie","subject":"Re: [PATCH v3] Add default merge options for all branches","fromName":"Michael Grubb","fromEmail":"devel@dailyvoid.com","sentAt":"2011-05-03T16:46:58Z","receivedAt":"2011-05-03T16:46:58Z","isPatch":true,"sender":{"key":"devel@dailyvoid.com","avatar":"https://gravatar.com/avatar/5ff035e599312653f041129e68041a9078c7e1f3268eaac3899c04e353d2133e?d=mp&s=160"},"body":"\n\nOn 5/3/11 4:03 AM, Jonathan Nieder wrote:\n> Hi,\n> \n> Michael Grubb wrote:\n> \n>> Add support for branch.*.mergeoptions for setting default options for\n>> all branches. This new value shares semantics with the existing\n>> branch..mergeoptions variable. If a branch specific value is\n>> found, that value will be used.\n> \n> So in the future one might be able to do things like\n> \n> \t[branch \"git-gui/*\"]\n> \t\tmergeoptions = -s subtree\n> \n> Interesting.\n> \n>> The need for this arises from the fact that there is currently not an\n>> easy way to set merge options for all branches.\n> \n> I'm curious: what merge options/workflows does this tend to be useful\n> for? The above explanation seems a bit abstract (though already\n> convincing).\nFor myself I've adopted Vincent Driessen's branching model\n(http://nvie.com/posts/a-successful-git-branching-model/)\nWhich specifically recommends using --no-ff when merging to preserve\nthe history of the existence of a topic branch once those branches are\nremoved. But it would equally be beneficial for folks that want the\n--log, etc option turned on for every merge.\n\n> \n>> The approach taken is to make note of whether a branch specific\n>> mergeoptions key has been seen and only apply the global value if it\n>> hasn't.\n> \n> What happens if the global value is seen first?\nI will code a test for this, but if the global option is seen first then\nthose options are used, unless/until a branch specific option is seen which will\noverride the globals. If the branch specific option is seen first then the global\noptions are ignored. (I did test this btw, but I didn't add it in the unit tests, bad me).\n\n> \n> On to the code. Warning: nitpicks ahead.\n> \n> [...]\n>> +++ b/builtin/merge.c\n>> @@ -32,6 +32,13 @@\n>> #define NO_FAST_FORWARD (1<<2)\n>> #define NO_TRIVIAL (1<<3)\n>> \n>> +#define MERGEOPTIONS_DEFAULT (1<<0)\n>> +#define MERGEOPTIONS_BRANCH (1<<1)\n> \n> Are these bitflags?\nI had been using literals in the git_merge_config function but thought better of it and added\nthese defines. I'm not really using them as bitflags right now. Perhaps an enum, or just plain\nliterals are better. I'm not clear what the best practice is in this case, so I'm certainly open\nto a better/more clear way of doing this.\n\n> \n>> @@ -505,24 +512,42 @@ cleanup:\n>> \n>> static int git_merge_config(const char *k, const char *v, void *cb)\n>> {\n>> +\tint merge_option_mode = 0;\n>> +\tstruct merge_options_cb *merge_options =\n>> +\t\t(struct merge_options_cb *)cb;\n> \n> This cast should not needed, I'd think.\nIndeed. It has been removed.\n\n> \n> [...]\n>> -\tif (branch && !prefixcmp(k, \"branch.\") &&\n>> -\t\t!prefixcmp(k + 7, branch) &&\n>> -\t\t!strcmp(k + 7 + strlen(branch), \".mergeoptions\")) {\n>> +\tif (!strcmp(k, \"branch.*.mergeoptions\"))\n>> +\t\tmerge_option_mode = MERGEOPTIONS_DEFAULT;\n>> +\telse if (branch && !prefixcmp(k, \"branch.\") &&\n>> +\t\t\t !prefixcmp(k + 7, branch) &&\n>> +\t\t\t !strcmp(k + 7 + strlen(branch), \".mergeoptions\"))\n>> +\t\tmerge_option_mode = MERGEOPTIONS_BRANCH;\n>> +\n>> +\tif ((merge_option_mode == MERGEOPTIONS_DEFAULT &&\n>> +\t\t!merge_options->override_default) ||\n>> +\t\tmerge_option_mode == MERGEOPTIONS_BRANCH) {\n>> \t\tconst char **argv;\n> \n> It is hard to see at a glance where the \"if\" condition ends and\n> the body begins. Why not\nI completely agree, though believe it or not, this was what the original code looked like\nso I was following it's lead. Here is what the original looks like:\n\n\tif (branch && !prefixcmp(k, \"branch.\") &&\n\t\t!prefixcmp(k + 7, branch) &&\n\t\t!strcmp(k + 7 + strlen(branch), \".mergeoptions\")) {\n\t\tconst char **argv;\n\t\tint argc;\n\t\tchar *buf;\n\nI would, of course, be happy to clear this up.\n> \n> \tif ((merge_option_mode == MERGEOPTIONS_DEFAULT &&\n> \t !merge_options->override_default) ||\n> \t merge_option_mode == MERGEOPTIONS_BRANCH) {\n> \t\tconst char **argv;\n> \t\t...\n> \n> or\n> \n> \tif (merge_option_mode == MERGEOPTIONS_BRANCH ? 1 :\n> \t merge_option_mode == MERGEOPTIONS_DEFAULT ?\n> \t\t\t!merge_options->override_default : 0) {\n> \t\tconst char **argv;\n> \t\t...\n> \n> or even\n> \n> \tif (merge_option_mode == MERGEOPTIONS_DEFAULT &&\n> \t merge_options->override_default)\n> \t\tmerge_option_mode = 0;\n> \n> \tif (merge_option_mode) {\n> \t\tconst char **argv;\n> \t\t...\n> \n> ?\n> \t \n>> \t\tint argc;\n>> \t\tchar *buf;\n>> \n>> \t\tbuf = xstrdup(v);\n>> \t\targc = split_cmdline(buf, &argv);\n>> -\t\tif (argc < 0)\n>> -\t\t\tdie(_(\"Bad branch.%s.mergeoptions string: %s\"), branch,\n>> -\t\t\t split_cmdline_strerror(argc));\n>> +\t\tif (argc < 0) {\n>> +\t\t\tif (merge_option_mode == 1)\n>> +\t\t\t\tdie(_(\"Bad merge.mergeoptions string: %s\"), \n>> +\t\t\t\t\tsplit_cmdline_strerror(argc));\n> \n> merge.*.mergeoptions, no?\n> \n>> +\t\t\telse\n>> +\t\t\t\tdie(_(\"Bad branch.%s.mergeoptions string: %s\"), branch,\n>> +\t\t\t\t\tsplit_cmdline_strerror(argc));\n>> +\t\t}\n>> \t\targv = xrealloc(argv, sizeof(*argv) * (argc + 2));\n>> \t\tmemmove(argv + 1, argv, sizeof(*argv) * (argc + 1));\n>> \t\targc++;\n>> \t\tparse_options(argc, argv, NULL, builtin_merge_options,\n>> \t\t\t builtin_merge_usage, 0);\n>> \t\tfree(buf);\n>> +\t\tif (merge_option_mode == MERGEOPTIONS_BRANCH)\n>> +\t\t\tmerge_options->override_default = 1;\n> \n> Could be clearer to put this next to the code that checks\n> override_default.\nYes. I had put this there originally so that it would only be set if there were no errors in parsing the options\nbut after chasing the rabbit down the whole, if there are errors parse_options won't return, so I'll move this\nup closer to the override_default check.\n> \n> [...]\n>> --- a/t/t7600-merge.sh\n>> +++ b/t/t7600-merge.sh\n>> @@ -415,6 +415,33 @@ test_expect_success 'merge c0 with c1 (no-ff)' '\n>> \n>> test_debug 'git log --graph --decorate --oneline --all'\n>> \n>> +test_expect_success 'merge c0 with c1 (global no-ff)' '\n>> +\tgit reset --hard c0 &&\n>> +\tgit config --unset branch.master.mergeoptions &&\n> \n> Better to make that\n> \n> \ttest_might_fail git config --unset ...\n> \n> so it will still work if earlier tests stop setting that\n> variable.\n> \n>> +\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n>> +\ttest_tick &&\n>> +\tgit merge c1 &&\n>> +\tgit config --remove-section \"branch.*\" &&\n>> +\tverify_merge file result.1 &&\n>> +\tverify_parents $c0 $c1\n>> +'\n>> +\n>> +test_debug 'git log --graph --decorate --oneline --all'\n> \n> Yuck. Did anything come of the idea of a --between-tests option to\n> use an arbitrary command here automatically? (Not your fault.) \n> \n>> +\n>> +test_expect_success 'combine merge.mergeoptions with branch.x.mergeoptions' '\n>> +\tgit reset --hard c0 &&\n>> +\tgit config --remove-section branch.master &&\n> \n> Could make sense to use test_might_fail for this one, too.\n> \n>> +\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n>> +\tgit config branch.master.mergeoptions \"--ff\" &&\n>> +\ttest_tick &&\n>> +\tgit merge c1 &&\n>> +\tgit config --remove-section \"branch.*\" &&\n>> +\tverify_merge file result.1 &&\n>> +\tverify_parents \"$c0\"\n>> +'\n>> +\n>> +test_debug 'git log --graph --decorate --oneline --all'\n> \n> Nice, a clean patch with a few reasonable tests.\nThanks for all the constructive criticism, Jonathan.\nI will make the recommended changes and post a new patch.\n\n> \n> With whichever of the changes below make sense,\n> Reviewed-by: Jonathan Nieder \n> \n> Thanks.\n> ---\n> builtin/merge.c | 37 ++++++++++++++++++++-----------------\n> t/t7600-merge.sh | 4 ++--\n> 2 files changed, 22 insertions(+), 19 deletions(-)\n> \n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 9fe129f..7156e92 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -32,8 +32,8 @@\n> #define NO_FAST_FORWARD (1<<2)\n> #define NO_TRIVIAL (1<<3)\n> \n> -#define MERGEOPTIONS_DEFAULT (1<<0)\n> -#define MERGEOPTIONS_BRANCH (1<<1)\n> +#define MERGEOPTIONS_DEFAULT 1\n> +#define MERGEOPTIONS_BRANCH 2\n> \n> struct merge_options_cb {\n> \tint override_default;\n> @@ -513,8 +513,7 @@ cleanup:\n> static int git_merge_config(const char *k, const char *v, void *cb)\n> {\n> \tint merge_option_mode = 0;\n> -\tstruct merge_options_cb *merge_options =\n> -\t\t(struct merge_options_cb *)cb;\n> +\tstruct merge_options_cb *merge_options = cb;\n> \n> \tif (!strcmp(k, \"branch.*.mergeoptions\"))\n> \t\tmerge_option_mode = MERGEOPTIONS_DEFAULT;\n> @@ -523,31 +522,35 @@ static int git_merge_config(const char *k, const char *v, void *cb)\n> \t\t\t !strcmp(k + 7 + strlen(branch), \".mergeoptions\"))\n> \t\tmerge_option_mode = MERGEOPTIONS_BRANCH;\n> \n> -\tif ((merge_option_mode == MERGEOPTIONS_DEFAULT &&\n> -\t\t!merge_options->override_default) ||\n> -\t\tmerge_option_mode == MERGEOPTIONS_BRANCH) {\n> +\t/*\n> +\t * If an applicable [branch \"foo\"] mergeoptions setting was\n> +\t * seen already, let it mask the [branch \"*\"] defaults.\n> +\t */\n> +\tif (merge_options->override_default &&\n> +\t merge_option_mode == MERGEOPTIONS_DEFAULT)\n> +\t\tmerge_option_mode = 0;\n> +\n> +\tif (merge_option_mode == MERGEOPTIONS_BRANCH)\n> +\t\tmerge_options->override_default = 1;\n> +\n> +\tif (merge_option_mode) {\n> \t\tconst char **argv;\n> \t\tint argc;\n> \t\tchar *buf;\n> \n> \t\tbuf = xstrdup(v);\n> \t\targc = split_cmdline(buf, &argv);\n> -\t\tif (argc < 0) {\n> -\t\t\tif (merge_option_mode == 1)\n> -\t\t\t\tdie(_(\"Bad merge.mergeoptions string: %s\"), \n> -\t\t\t\t\tsplit_cmdline_strerror(argc));\n> -\t\t\telse\n> -\t\t\t\tdie(_(\"Bad branch.%s.mergeoptions string: %s\"), branch,\n> -\t\t\t\t\tsplit_cmdline_strerror(argc));\n> -\t\t}\n> +\t\tif (argc < 0)\n> +\t\t\tdie(_(\"Bad merge.%s.mergeoptions string: %s\"), \n> +\t\t\t merge_option_mode == MERGEOPTIONS_DEFAULT ?\n> +\t\t\t\t\t\t\t\"*\" : branch,\n> +\t\t\t split_cmdline_strerror(argc));\n> \t\targv = xrealloc(argv, sizeof(*argv) * (argc + 2));\n> \t\tmemmove(argv + 1, argv, sizeof(*argv) * (argc + 1));\n> \t\targc++;\n> \t\tparse_options(argc, argv, NULL, builtin_merge_options,\n> \t\t\t builtin_merge_usage, 0);\n> \t\tfree(buf);\n> -\t\tif (merge_option_mode == MERGEOPTIONS_BRANCH)\n> -\t\t\tmerge_options->override_default = 1;\n> \t}\n> \n> \tif (!strcmp(k, \"merge.diffstat\") || !strcmp(k, \"merge.stat\"))\n> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\n> index cea2b31..ff807f4 100755\n> --- a/t/t7600-merge.sh\n> +++ b/t/t7600-merge.sh\n> @@ -417,7 +417,7 @@ test_debug 'git log --graph --decorate --oneline --all'\n> \n> test_expect_success 'merge c0 with c1 (global no-ff)' '\n> \tgit reset --hard c0 &&\n> -\tgit config --unset branch.master.mergeoptions &&\n> +\ttest_might_fail git config --unset branch.master.mergeoptions &&\n> \tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n> \ttest_tick &&\n> \tgit merge c1 &&\n> @@ -430,7 +430,7 @@ test_debug 'git log --graph --decorate --oneline --all'\n> \n> test_expect_success 'combine merge.mergeoptions with branch.x.mergeoptions' '\n> \tgit reset --hard c0 &&\n> -\tgit config --remove-section branch.master &&\n> +\ttest_might_fail git config --remove-section branch.master &&\n> \tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n> \tgit config branch.master.mergeoptions \"--ff\" &&\n> \ttest_tick &&\n"},{"id":"166957","messageId":"7vk4e7ir9v.fsf@alter.siamese.dyndns.org","threadId":"27241","inReplyTo":"20110503090351.GA27862@elie","subject":"Re: [PATCH v3] Add default merge options for all branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-03T18:16:12Z","receivedAt":"2011-05-03T18:16:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> So in the future one might be able to do things like\n>\n> \t[branch \"git-gui/*\"]\n> \t\tmergeoptions = -s subtree\n>\n> Interesting.\n>\n>> The need for this arises from the fact that there is currently not an\n>> easy way to set merge options for all branches.\n>\n> I'm curious: what merge options/workflows does this tend to be useful\n> for?\n\nI actually am curious myself, too. I want to see a real-life example.\n\nThe \"git-gui/*\" example you gave is unfortunately not it. It specifies the\nbranch at the wrong end. Whether I am merging into \"master\" or \"next\", I\nwould want \"-s subtree\" when I am merging \"git-gui/*\" project, but all my\nmerges to \"master\" or \"next\" do not necessarily want to use \"-s subtree\".\n\nIt might turn out to be that \"branch..mergeoptions\" is a\nill-conceived idea to begin with.\n\n>> The approach taken is to make note of whether a branch specific\n>> mergeoptions key has been seen and only apply the global value if it\n>> hasn't.\n>\n> What happens if the global value is seen first?\n\nIf implemented correctly, it should use the specific one and fall back to\nthe wildcard one. Another issue is if the values should be cumulative or\noverriding, but in the remainder I'd assume we want overriding.\n\n>> @@ -505,24 +512,42 @@ cleanup:\n>> \n>> static int git_merge_config(const char *k, const char *v, void *cb)\n>> {\n>> +\tint merge_option_mode = 0;\n>> +\tstruct merge_options_cb *merge_options =\n>> +\t\t(struct merge_options_cb *)cb;\n>\n> This cast should not needed, I'd think.\n\nCorrect. That is the whole point of using (void *) as a parameter, so\nthat it can be assigned to the real expected type easily without cast.\n\n>> +\tif (!strcmp(k, \"branch.*.mergeoptions\"))\n>> +\t\tmerge_option_mode = MERGEOPTIONS_DEFAULT;\n>> +\telse if (branch && !prefixcmp(k, \"branch.\") &&\n>> +\t\t\t !prefixcmp(k + 7, branch) &&\n>> +\t\t\t !strcmp(k + 7 + strlen(branch), \".mergeoptions\"))\n>> +\t\tmerge_option_mode = MERGEOPTIONS_BRANCH;\n>> +\n>> +\tif ((merge_option_mode == MERGEOPTIONS_DEFAULT &&\n>> +\t\t!merge_options->override_default) ||\n>> +\t\tmerge_option_mode == MERGEOPTIONS_BRANCH) {\n>> \t\tconst char **argv;\n>\n> It is hard to see at a glance where the \"if\" condition ends and\n> the body begins. Why not\n> ...\n> or even\n> ...\n> ?\n\nWhy not have two string pointers in merge_options_cb structure that are\ninitialized to NULL and holds the matched config key and the value?\n\nWhen we are looking at a (k, v) pair in this function, if there is no\nprevious key in cb, we store (k, v) in cb and return. If there already is\na previous key, we see if k is more specific than that key, and replace\n(k, v) in cb with what we currently have. Otherwise we do not do\nanything.\n\nWhen git_config() returns, the caller will find the final value in cb.\n\n>> +\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n>> +\ttest_tick &&\n>> +\tgit merge c1 &&\n>> +\tgit config --remove-section \"branch.*\" &&\n>> +\tverify_merge file result.1 &&\n>> +\tverify_parents $c0 $c1\n>> +'\n>> +\n>> +test_debug 'git log --graph --decorate --oneline --all'\n>\n> Yuck. Did anything come of the idea of a --between-tests option to\n> use an arbitrary command here automatically? (Not your fault.) \n\nI actually think test_debug should go inside the previous test. Why not\nhave it immediately after \"git merge c1\" above?\n"},{"id":"166963","messageId":"4DC0608F.9040208@dailyvoid.com","threadId":"27241","inReplyTo":"20110503090351.GA27862@elie","subject":"[PATCH v4] Add default merge options for all branches","fromName":"Michael Grubb","fromEmail":"devel@dailyvoid.com","sentAt":"2011-05-03T20:07:43Z","receivedAt":"2011-05-03T20:07:43Z","isPatch":true,"sender":{"key":"devel@dailyvoid.com","avatar":"https://gravatar.com/avatar/5ff035e599312653f041129e68041a9078c7e1f3268eaac3899c04e353d2133e?d=mp&s=160"},"body":"Add support for branch.*.mergeoptions for setting default options for\nall branches. This new value shares semantics with the existing\nbranch..mergeoptions variable. If a branch specific value is\nfound, that value will be used.\n\nThe need for this arises from the fact that there is currently not an\neasy way to set merge options for all branches. Instead of having to\nspecify merge options for each individual branch there should be a way\nto set defaults for all branches and then override a specific branch's\noptions.\n\nIn order to facilitate future features centered around this new\n\"globlike\" syntax a new struct has been created to keep track of the\nbranch.*.* options. Currently it only supports branch.*.mergeoptions,\nbut can be easily modified in the future to support other branch\nspecific options as well. The mechanism will \"vote\" on specific-\"ness\"\nof the configuration key and ultimately only use the most specific\noptions. This is not cumulative but overriding.\n\nSigned-off-by: Michael Grubb \n---\n Documentation/git-merge.txt | 3 +\n builtin/merge.c | 90 +++++++++++++++++++++++++++++++++---------\n t/t7600-merge.sh | 39 ++++++++++++++++++\n 3 files changed, 112 insertions(+), 20 deletions(-)\n\ndiff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\nindex e2e6aba..eaab3e4 100644\n--- a/Documentation/git-merge.txt\n+++ b/Documentation/git-merge.txt\n@@ -307,6 +307,9 @@ branch..mergeoptions::\n \tSets default options for merging into branch . The syntax and\n \tsupported options are the same as those of 'git merge', but option\n \tvalues containing whitespace characters are currently not supported.\n+\tThe special value '*' for may be used to configure default\n+\toptions for all branches. Values for specific branch names will\n+\toverride the this default.\n \n SEE ALSO\n --------\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex d171c63..d6ce85e 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -32,6 +32,21 @@\n #define NO_FAST_FORWARD (1<<2)\n #define NO_TRIVIAL (1<<3)\n \n+/* This is for branch.. blocks\n+ * the vote member holds a value between\n+ * 0.0 and 1.0 which measures how closely\n+ * a branch name matches the key member.\n+ * where branch.*.mergeoptions would be 0.1 and\n+ * branch..mergeoptions would be 1.0\n+ * Also it is called vote because I couldn't come\n+ * up with a better name.\n+ */\n+struct merge_options_cb {\n+\tchar *key;\n+\tchar *value;\n+\tfloat vote;\n+};\n+\n struct strategy {\n \tconst char *name;\n \tunsigned attr;\n@@ -503,28 +518,60 @@ cleanup:\n \tstrbuf_release(&bname);\n }\n \n-static int git_merge_config(const char *k, const char *v, void *cb)\n+static void parse_git_merge_options(const char *k, const char *v,\n+\t\t\tvoid *cb)\n {\n-\tif (branch && !prefixcmp(k, \"branch.\") &&\n-\t\t!prefixcmp(k + 7, branch) &&\n-\t\t!strcmp(k + 7 + strlen(branch), \".mergeoptions\")) {\n-\t\tconst char **argv;\n-\t\tint argc;\n-\t\tchar *buf;\n-\n-\t\tbuf = xstrdup(v);\n-\t\targc = split_cmdline(buf, &argv);\n-\t\tif (argc < 0)\n-\t\t\tdie(_(\"Bad branch.%s.mergeoptions string: %s\"), branch,\n-\t\t\t split_cmdline_strerror(argc));\n-\t\targv = xrealloc(argv, sizeof(*argv) * (argc + 2));\n-\t\tmemmove(argv + 1, argv, sizeof(*argv) * (argc + 1));\n-\t\targc++;\n-\t\tparse_options(argc, argv, NULL, builtin_merge_options,\n-\t\t\t builtin_merge_usage, 0);\n-\t\tfree(buf);\n+\tstruct merge_options_cb *merge_options = cb;\n+\tint changed = 0;\n+\n+\t/* We only handle mergeoptions for now */\n+\tif (suffixcmp(k, \".mergeoptions\"))\n+\t\treturn;\n+\n+\tif (!prefixcmp(k, \"branch.*\") && merge_options->vote <= 0.1 ) {\n+\t\tmerge_options->vote = 0.1;\n+\t\tchanged = 1;\n+\t} else if (branch && !prefixcmp(k, \"branch.\") &&\n+\t\t\t\t!prefixcmp(k + 7, branch) &&\n+\t\t\t\tmerge_options->vote < 1.0) {\n+\t\tmerge_options->vote = 1.0;\n+\t\tchanged = 1;\n \t}\n \n+\tif (changed) {\n+\t\tmerge_options->key = (char *)k;\n+\t\tmerge_options->value = (char *)v;\n+\t}\n+}\n+\n+static void apply_merge_options(struct merge_options_cb *opts)\n+{\n+\tconst char **argv;\n+\tint argc;\n+\tchar *buf;\n+\n+\tif ( opts == NULL )\n+\t\treturn;\n+\n+\tbuf = xstrdup(opts->value);\n+\targc = split_cmdline(buf, &argv);\n+\tif (argc < 0)\n+\t\tdie(_(\"Bad %s string: %s\"), \n+\t\t\topts->key, split_cmdline_strerror(argc));\n+\n+\targv = xrealloc(argv, sizeof(*argv) * (argc + 2));\n+\tmemmove(argv + 1, argv, sizeof(*argv) * (argc + 1));\n+\targc++;\n+\tparse_options(argc, argv, NULL, builtin_merge_options,\n+\t\t\t builtin_merge_usage, 0);\n+\tfree(buf);\n+}\n+\n+static int git_merge_config(const char *k, const char *v, void *cb)\n+{\n+\tif (cb != NULL && branch && !prefixcmp(k, \"branch.\"))\n+\t\tparse_git_merge_options(k, v, cb);\n+\n \tif (!strcmp(k, \"merge.diffstat\") || !strcmp(k, \"merge.stat\"))\n \t\tshow_diffstat = git_config_bool(k, v);\n \telse if (!strcmp(k, \"pull.twohead\"))\n@@ -987,6 +1034,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tconst char *head_arg;\n \tint flag, head_invalid = 0, i;\n \tint best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;\n+\tstruct merge_options_cb merge_options = {NULL, NULL, 0.0};\n \tstruct commit_list *common = NULL;\n \tconst char *best_strategy = NULL, *wt_strategy = NULL;\n \tstruct commit_list **remotes = &remoteheads;\n@@ -1004,7 +1052,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tif (is_null_sha1(head))\n \t\thead_invalid = 1;\n \n-\tgit_config(git_merge_config, NULL);\n+\tgit_config(git_merge_config, &merge_options);\n+\tif (merge_options.key != NULL && merge_options.value != NULL)\n+\t\tapply_merge_options(&merge_options);\n \n \t/* for color.ui */\n \tif (diff_use_color_default == -1)\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex e84e822..5b1f8e1 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -415,6 +415,45 @@ test_expect_success 'merge c0 with c1 (no-ff)' '\n \n test_debug 'git log --graph --decorate --oneline --all'\n \n+test_expect_success 'merge c0 with c1 (default no-ff)' '\n+\tgit reset --hard c0 &&\n+\ttest_might_fail git config --unset branch.master.mergeoptions &&\n+\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n+\ttest_tick &&\n+\tgit merge c1 &&\n+\tgit config --remove-section \"branch.*\" &&\n+\tverify_merge file result.1 &&\n+\tverify_parents $c0 $c1\n+'\n+\n+test_debug 'git log --graph --decorate --oneline --all'\n+\n+test_expect_success 'combine branch.*.mergeoptions with branch.x.mergeoptions' '\n+\tgit reset --hard c0 &&\n+\ttest_might_fail git config --remove-section branch.master &&\n+\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n+\tgit config branch.master.mergeoptions \"--ff\" &&\n+\ttest_tick &&\n+\tgit merge c1 &&\n+\tgit config --remove-section \"branch.*\" &&\n+\tverify_merge file result.1 &&\n+\tverify_parents \"$c0\"\n+'\n+\n+test_expect_success 'reverse branch.x.mergeoptions with branch.*.mergeoptions' '\n+\tgit reset --hard c0 &&\n+\ttest_might_fail git config --remove-section branch.master &&\n+\tgit config branch.master.mergeoptions \"--ff\" &&\n+\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n+\ttest_tick &&\n+\tgit merge c1 &&\n+\tgit config --remove-section \"branch.*\" &&\n+\tverify_merge file result.1 &&\n+\tverify_parents \"$c0\"\n+'\n+\n+test_debug 'git log --graph --decorate --oneline --all'\n+\n test_expect_success 'combining --squash and --no-ff is refused' '\n \ttest_must_fail git merge --squash --no-ff c1 &&\n \ttest_must_fail git merge --no-ff --squash c1\n-- \n1.7.5\n"},{"id":"166964","messageId":"4DC0641D.5070403@dailyvoid.com","threadId":"27241","inReplyTo":"7vk4e7ir9v.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] Add default merge options for all branches","fromName":"Michael Grubb","fromEmail":"devel@dailyvoid.com","sentAt":"2011-05-03T20:22:53Z","receivedAt":"2011-05-03T20:22:53Z","isPatch":true,"sender":{"key":"devel@dailyvoid.com","avatar":"https://gravatar.com/avatar/5ff035e599312653f041129e68041a9078c7e1f3268eaac3899c04e353d2133e?d=mp&s=160"},"body":"\n\nOn 5/3/11 1:16 PM, Junio C Hamano wrote:\n> Jonathan Nieder writes:\n> \n>> So in the future one might be able to do things like\n>>\n>> \t[branch \"git-gui/*\"]\n>> \t\tmergeoptions = -s subtree\n>>\n>> Interesting.\n>>\n>>> The need for this arises from the fact that there is currently not an\n>>> easy way to set merge options for all branches.\n>>\n>> I'm curious: what merge options/workflows does this tend to be useful\n>> for?\n> \n> I actually am curious myself, too. I want to see a real-life example.\n> \nIn my reply to Jonathan I gave the example of turning off fast forward merges\nglobally for all branches (and then perhaps turning them back on for specific branches).\nThe same could be done with --log or any other option that might make since to turn on\nfor all branches without having to specifically name each branch in the config file.\n\nAlso as previously cited in my earlier reply to Jonathan, a good real life example can be\nfound in Vincent Driessen's branching model where he creates ephemeral feature/topic branches\nfor development then deletes them when they get merged back into the mainline development tree.\nHowever, he wants to keep the history that there was a merge from a branch instead of it just looking\nlike a straight line. The link to his work is here: http://nvie.com/posts/a-successful-git-branching-model/\n\n\n> The \"git-gui/*\" example you gave is unfortunately not it. It specifies the\n> branch at the wrong end. Whether I am merging into \"master\" or \"next\", I\n> would want \"-s subtree\" when I am merging \"git-gui/*\" project, but all my\n> merges to \"master\" or \"next\" do not necessarily want to use \"-s subtree\".\n> \n> It might turn out to be that \"branch..mergeoptions\" is a\n> ill-conceived idea to begin with.\n> \n>>> The approach taken is to make note of whether a branch specific\n>>> mergeoptions key has been seen and only apply the global value if it\n>>> hasn't.\n>>\n>> What happens if the global value is seen first?\n> \n> If implemented correctly, it should use the specific one and fall back to\n> the wildcard one. Another issue is if the values should be cumulative or\n> overriding, but in the remainder I'd assume we want overriding.\n> \nThere is now a unit test for this scenario.\n\n>>> @@ -505,24 +512,42 @@ cleanup:\n>>> \n>>> static int git_merge_config(const char *k, const char *v, void *cb)\n>>> {\n>>> +\tint merge_option_mode = 0;\n>>> +\tstruct merge_options_cb *merge_options =\n>>> +\t\t(struct merge_options_cb *)cb;\n>>\n>> This cast should not needed, I'd think.\n> \n> Correct. That is the whole point of using (void *) as a parameter, so\n> that it can be assigned to the real expected type easily without cast.\n> \nThis has been corrected in the latest version of the patch.\n\n>>> +\tif (!strcmp(k, \"branch.*.mergeoptions\"))\n>>> +\t\tmerge_option_mode = MERGEOPTIONS_DEFAULT;\n>>> +\telse if (branch && !prefixcmp(k, \"branch.\") &&\n>>> +\t\t\t !prefixcmp(k + 7, branch) &&\n>>> +\t\t\t !strcmp(k + 7 + strlen(branch), \".mergeoptions\"))\n>>> +\t\tmerge_option_mode = MERGEOPTIONS_BRANCH;\n>>> +\n>>> +\tif ((merge_option_mode == MERGEOPTIONS_DEFAULT &&\n>>> +\t\t!merge_options->override_default) ||\n>>> +\t\tmerge_option_mode == MERGEOPTIONS_BRANCH) {\n>>> \t\tconst char **argv;\n>>\n>> It is hard to see at a glance where the \"if\" condition ends and\n>> the body begins. Why not\n>> ...\n>> or even\n>> ...\n>> ?\n> \n> Why not have two string pointers in merge_options_cb structure that are\n> initialized to NULL and holds the matched config key and the value?\n> \nThat was a great idea. I've reworked things around this and also moved\nto a voting type method for determining priority of the keys. So no more\ndefines needed. I think keeping all that state in the struct is much better\nand scales better as well.\n\n> When we are looking at a (k, v) pair in this function, if there is no\n> previous key in cb, we store (k, v) in cb and return. If there already is\n> a previous key, we see if k is more specific than that key, and replace\n> (k, v) in cb with what we currently have. Otherwise we do not do\n> anything.\n> \n> When git_config() returns, the caller will find the final value in cb.\n> \n>>> +\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n>>> +\ttest_tick &&\n>>> +\tgit merge c1 &&\n>>> +\tgit config --remove-section \"branch.*\" &&\n>>> +\tverify_merge file result.1 &&\n>>> +\tverify_parents $c0 $c1\n>>> +'\n>>> +\n>>> +test_debug 'git log --graph --decorate --oneline --all'\n>>\n>> Yuck. Did anything come of the idea of a --between-tests option to\n>> use an arbitrary command here automatically? (Not your fault.) \n> \n> I actually think test_debug should go inside the previous test. Why not\n> have it immediately after \"git merge c1\" above?\n\nAgain I followed the existing pattern here. I didn't want to be the odd man out.\nDo you want me to make that change?\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"166965","messageId":"4DC06733.6010904@dailyvoid.com","threadId":"27241","inReplyTo":"4DC0608F.9040208@dailyvoid.com","subject":"Re: [PATCH v4] Add default merge options for all branches","fromName":"Michael Grubb","fromEmail":"devel@dailyvoid.com","sentAt":"2011-05-03T20:36:03Z","receivedAt":"2011-05-03T20:36:03Z","isPatch":true,"sender":{"key":"devel@dailyvoid.com","avatar":"https://gravatar.com/avatar/5ff035e599312653f041129e68041a9078c7e1f3268eaac3899c04e353d2133e?d=mp&s=160"},"body":"This patch has a trailing whitespace on one line.\nI'm resubmitting to fix it.\n\nOn 5/3/11 3:07 PM, Michael Grubb wrote:\n> Add support for branch.*.mergeoptions for setting default options for\n> all branches. This new value shares semantics with the existing\n> branch..mergeoptions variable. If a branch specific value is\n> found, that value will be used.\n> \n> The need for this arises from the fact that there is currently not an\n> easy way to set merge options for all branches. Instead of having to\n> specify merge options for each individual branch there should be a way\n> to set defaults for all branches and then override a specific branch's\n> options.\n> \n> In order to facilitate future features centered around this new\n> \"globlike\" syntax a new struct has been created to keep track of the\n> branch.*.* options. Currently it only supports branch.*.mergeoptions,\n> but can be easily modified in the future to support other branch\n> specific options as well. The mechanism will \"vote\" on specific-\"ness\"\n> of the configuration key and ultimately only use the most specific\n> options. This is not cumulative but overriding.\n> \n> Signed-off-by: Michael Grubb \n> ---\n> Documentation/git-merge.txt | 3 +\n> builtin/merge.c | 90 +++++++++++++++++++++++++++++++++---------\n> t/t7600-merge.sh | 39 ++++++++++++++++++\n> 3 files changed, 112 insertions(+), 20 deletions(-)\n> \n> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\n> index e2e6aba..eaab3e4 100644\n> --- a/Documentation/git-merge.txt\n> +++ b/Documentation/git-merge.txt\n> @@ -307,6 +307,9 @@ branch..mergeoptions::\n> \tSets default options for merging into branch . The syntax and\n> \tsupported options are the same as those of 'git merge', but option\n> \tvalues containing whitespace characters are currently not supported.\n> +\tThe special value '*' for may be used to configure default\n> +\toptions for all branches. Values for specific branch names will\n> +\toverride the this default.\n> \n> SEE ALSO\n> --------\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index d171c63..d6ce85e 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -32,6 +32,21 @@\n> #define NO_FAST_FORWARD (1<<2)\n> #define NO_TRIVIAL (1<<3)\n> \n> +/* This is for branch.. blocks\n> + * the vote member holds a value between\n> + * 0.0 and 1.0 which measures how closely\n> + * a branch name matches the key member.\n> + * where branch.*.mergeoptions would be 0.1 and\n> + * branch..mergeoptions would be 1.0\n> + * Also it is called vote because I couldn't come\n> + * up with a better name.\n> + */\n> +struct merge_options_cb {\n> +\tchar *key;\n> +\tchar *value;\n> +\tfloat vote;\n> +};\n> +\n> struct strategy {\n> \tconst char *name;\n> \tunsigned attr;\n> @@ -503,28 +518,60 @@ cleanup:\n> \tstrbuf_release(&bname);\n> }\n> \n> -static int git_merge_config(const char *k, const char *v, void *cb)\n> +static void parse_git_merge_options(const char *k, const char *v,\n> +\t\t\tvoid *cb)\n> {\n> -\tif (branch && !prefixcmp(k, \"branch.\") &&\n> -\t\t!prefixcmp(k + 7, branch) &&\n> -\t\t!strcmp(k + 7 + strlen(branch), \".mergeoptions\")) {\n> -\t\tconst char **argv;\n> -\t\tint argc;\n> -\t\tchar *buf;\n> -\n> -\t\tbuf = xstrdup(v);\n> -\t\targc = split_cmdline(buf, &argv);\n> -\t\tif (argc < 0)\n> -\t\t\tdie(_(\"Bad branch.%s.mergeoptions string: %s\"), branch,\n> -\t\t\t split_cmdline_strerror(argc));\n> -\t\targv = xrealloc(argv, sizeof(*argv) * (argc + 2));\n> -\t\tmemmove(argv + 1, argv, sizeof(*argv) * (argc + 1));\n> -\t\targc++;\n> -\t\tparse_options(argc, argv, NULL, builtin_merge_options,\n> -\t\t\t builtin_merge_usage, 0);\n> -\t\tfree(buf);\n> +\tstruct merge_options_cb *merge_options = cb;\n> +\tint changed = 0;\n> +\n> +\t/* We only handle mergeoptions for now */\n> +\tif (suffixcmp(k, \".mergeoptions\"))\n> +\t\treturn;\n> +\n> +\tif (!prefixcmp(k, \"branch.*\") && merge_options->vote <= 0.1 ) {\n> +\t\tmerge_options->vote = 0.1;\n> +\t\tchanged = 1;\n> +\t} else if (branch && !prefixcmp(k, \"branch.\") &&\n> +\t\t\t\t!prefixcmp(k + 7, branch) &&\n> +\t\t\t\tmerge_options->vote < 1.0) {\n> +\t\tmerge_options->vote = 1.0;\n> +\t\tchanged = 1;\n> \t}\n> \n> +\tif (changed) {\n> +\t\tmerge_options->key = (char *)k;\n> +\t\tmerge_options->value = (char *)v;\n> +\t}\n> +}\n> +\n> +static void apply_merge_options(struct merge_options_cb *opts)\n> +{\n> +\tconst char **argv;\n> +\tint argc;\n> +\tchar *buf;\n> +\n> +\tif ( opts == NULL )\n> +\t\treturn;\n> +\n> +\tbuf = xstrdup(opts->value);\n> +\targc = split_cmdline(buf, &argv);\n> +\tif (argc < 0)\n> +\t\tdie(_(\"Bad %s string: %s\"), \n> +\t\t\topts->key, split_cmdline_strerror(argc));\n> +\n> +\targv = xrealloc(argv, sizeof(*argv) * (argc + 2));\n> +\tmemmove(argv + 1, argv, sizeof(*argv) * (argc + 1));\n> +\targc++;\n> +\tparse_options(argc, argv, NULL, builtin_merge_options,\n> +\t\t\t builtin_merge_usage, 0);\n> +\tfree(buf);\n> +}\n> +\n> +static int git_merge_config(const char *k, const char *v, void *cb)\n> +{\n> +\tif (cb != NULL && branch && !prefixcmp(k, \"branch.\"))\n> +\t\tparse_git_merge_options(k, v, cb);\n> +\n> \tif (!strcmp(k, \"merge.diffstat\") || !strcmp(k, \"merge.stat\"))\n> \t\tshow_diffstat = git_config_bool(k, v);\n> \telse if (!strcmp(k, \"pull.twohead\"))\n> @@ -987,6 +1034,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n> \tconst char *head_arg;\n> \tint flag, head_invalid = 0, i;\n> \tint best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;\n> +\tstruct merge_options_cb merge_options = {NULL, NULL, 0.0};\n> \tstruct commit_list *common = NULL;\n> \tconst char *best_strategy = NULL, *wt_strategy = NULL;\n> \tstruct commit_list **remotes = &remoteheads;\n> @@ -1004,7 +1052,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n> \tif (is_null_sha1(head))\n> \t\thead_invalid = 1;\n> \n> -\tgit_config(git_merge_config, NULL);\n> +\tgit_config(git_merge_config, &merge_options);\n> +\tif (merge_options.key != NULL && merge_options.value != NULL)\n> +\t\tapply_merge_options(&merge_options);\n> \n> \t/* for color.ui */\n> \tif (diff_use_color_default == -1)\n> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\n> index e84e822..5b1f8e1 100755\n> --- a/t/t7600-merge.sh\n> +++ b/t/t7600-merge.sh\n> @@ -415,6 +415,45 @@ test_expect_success 'merge c0 with c1 (no-ff)' '\n> \n> test_debug 'git log --graph --decorate --oneline --all'\n> \n> +test_expect_success 'merge c0 with c1 (default no-ff)' '\n> +\tgit reset --hard c0 &&\n> +\ttest_might_fail git config --unset branch.master.mergeoptions &&\n> +\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n> +\ttest_tick &&\n> +\tgit merge c1 &&\n> +\tgit config --remove-section \"branch.*\" &&\n> +\tverify_merge file result.1 &&\n> +\tverify_parents $c0 $c1\n> +'\n> +\n> +test_debug 'git log --graph --decorate --oneline --all'\n> +\n> +test_expect_success 'combine branch.*.mergeoptions with branch.x.mergeoptions' '\n> +\tgit reset --hard c0 &&\n> +\ttest_might_fail git config --remove-section branch.master &&\n> +\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n> +\tgit config branch.master.mergeoptions \"--ff\" &&\n> +\ttest_tick &&\n> +\tgit merge c1 &&\n> +\tgit config --remove-section \"branch.*\" &&\n> +\tverify_merge file result.1 &&\n> +\tverify_parents \"$c0\"\n> +'\n> +\n> +test_expect_success 'reverse branch.x.mergeoptions with branch.*.mergeoptions' '\n> +\tgit reset --hard c0 &&\n> +\ttest_might_fail git config --remove-section branch.master &&\n> +\tgit config branch.master.mergeoptions \"--ff\" &&\n> +\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n> +\ttest_tick &&\n> +\tgit merge c1 &&\n> +\tgit config --remove-section \"branch.*\" &&\n> +\tverify_merge file result.1 &&\n> +\tverify_parents \"$c0\"\n> +'\n> +\n> +test_debug 'git log --graph --decorate --oneline --all'\n> +\n> test_expect_success 'combining --squash and --no-ff is refused' '\n> \ttest_must_fail git merge --squash --no-ff c1 &&\n> \ttest_must_fail git merge --no-ff --squash c1\n"},{"id":"166966","messageId":"4DC06788.1020607@dailyvoid.com","threadId":"27241","inReplyTo":"20110503090351.GA27862@elie","subject":"[PATCH v4.1] Add default merge options for all branches","fromName":"Michael Grubb","fromEmail":"devel@dailyvoid.com","sentAt":"2011-05-03T20:37:28Z","receivedAt":"2011-05-03T20:37:28Z","isPatch":true,"sender":{"key":"devel@dailyvoid.com","avatar":"https://gravatar.com/avatar/5ff035e599312653f041129e68041a9078c7e1f3268eaac3899c04e353d2133e?d=mp&s=160"},"body":"Add support for branch.*.mergeoptions for setting default options for\nall branches. This new value shares semantics with the existing\nbranch..mergeoptions variable. If a branch specific value is\nfound, that value will be used.\n\nThe need for this arises from the fact that there is currently not an\neasy way to set merge options for all branches. Instead of having to\nspecify merge options for each individual branch there should be a way\nto set defaults for all branches and then override a specific branch's\noptions.\n\nIn order to facilitate future features centered around this new\n\"globlike\" syntax a new struct has been created to keep track of the\nbranch.*.* options. Currently it only supports branch.*.mergeoptions,\nbut can be easily modified in the future to support other branch\nspecific options as well. The mechanism will \"vote\" on specific-\"ness\"\nof the configuration key and ultimately only use the most specific\noptions. This is not cumulative but overriding.\n\nSigned-off-by: Michael Grubb \n---\n Documentation/git-merge.txt | 3 +\n builtin/merge.c | 90 +++++++++++++++++++++++++++++++++---------\n t/t7600-merge.sh | 39 ++++++++++++++++++\n 3 files changed, 112 insertions(+), 20 deletions(-)\n\ndiff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\nindex e2e6aba..eaab3e4 100644\n--- a/Documentation/git-merge.txt\n+++ b/Documentation/git-merge.txt\n@@ -307,6 +307,9 @@ branch..mergeoptions::\n \tSets default options for merging into branch . The syntax and\n \tsupported options are the same as those of 'git merge', but option\n \tvalues containing whitespace characters are currently not supported.\n+\tThe special value '*' for may be used to configure default\n+\toptions for all branches. Values for specific branch names will\n+\toverride the this default.\n \n SEE ALSO\n --------\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex d171c63..fa56205 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -32,6 +32,21 @@\n #define NO_FAST_FORWARD (1<<2)\n #define NO_TRIVIAL (1<<3)\n \n+/* This is for branch.. blocks\n+ * the vote member holds a value between\n+ * 0.0 and 1.0 which measures how closely\n+ * a branch name matches the key member.\n+ * where branch.*.mergeoptions would be 0.1 and\n+ * branch..mergeoptions would be 1.0\n+ * Also it is called vote because I couldn't come\n+ * up with a better name.\n+ */\n+struct merge_options_cb {\n+\tchar *key;\n+\tchar *value;\n+\tfloat vote;\n+};\n+\n struct strategy {\n \tconst char *name;\n \tunsigned attr;\n@@ -503,28 +518,60 @@ cleanup:\n \tstrbuf_release(&bname);\n }\n \n-static int git_merge_config(const char *k, const char *v, void *cb)\n+static void parse_git_merge_options(const char *k, const char *v,\n+\t\t\tvoid *cb)\n {\n-\tif (branch && !prefixcmp(k, \"branch.\") &&\n-\t\t!prefixcmp(k + 7, branch) &&\n-\t\t!strcmp(k + 7 + strlen(branch), \".mergeoptions\")) {\n-\t\tconst char **argv;\n-\t\tint argc;\n-\t\tchar *buf;\n-\n-\t\tbuf = xstrdup(v);\n-\t\targc = split_cmdline(buf, &argv);\n-\t\tif (argc < 0)\n-\t\t\tdie(_(\"Bad branch.%s.mergeoptions string: %s\"), branch,\n-\t\t\t split_cmdline_strerror(argc));\n-\t\targv = xrealloc(argv, sizeof(*argv) * (argc + 2));\n-\t\tmemmove(argv + 1, argv, sizeof(*argv) * (argc + 1));\n-\t\targc++;\n-\t\tparse_options(argc, argv, NULL, builtin_merge_options,\n-\t\t\t builtin_merge_usage, 0);\n-\t\tfree(buf);\n+\tstruct merge_options_cb *merge_options = cb;\n+\tint changed = 0;\n+\n+\t/* We only handle mergeoptions for now */\n+\tif (suffixcmp(k, \".mergeoptions\"))\n+\t\treturn;\n+\n+\tif (!prefixcmp(k, \"branch.*\") && merge_options->vote <= 0.1 ) {\n+\t\tmerge_options->vote = 0.1;\n+\t\tchanged = 1;\n+\t} else if (branch && !prefixcmp(k, \"branch.\") &&\n+\t\t\t\t!prefixcmp(k + 7, branch) &&\n+\t\t\t\tmerge_options->vote < 1.0) {\n+\t\tmerge_options->vote = 1.0;\n+\t\tchanged = 1;\n \t}\n \n+\tif (changed) {\n+\t\tmerge_options->key = (char *)k;\n+\t\tmerge_options->value = (char *)v;\n+\t}\n+}\n+\n+static void apply_merge_options(struct merge_options_cb *opts)\n+{\n+\tconst char **argv;\n+\tint argc;\n+\tchar *buf;\n+\n+\tif ( opts == NULL )\n+\t\treturn;\n+\n+\tbuf = xstrdup(opts->value);\n+\targc = split_cmdline(buf, &argv);\n+\tif (argc < 0)\n+\t\tdie(_(\"Bad %s string: %s\"),\n+\t\t\topts->key, split_cmdline_strerror(argc));\n+\n+\targv = xrealloc(argv, sizeof(*argv) * (argc + 2));\n+\tmemmove(argv + 1, argv, sizeof(*argv) * (argc + 1));\n+\targc++;\n+\tparse_options(argc, argv, NULL, builtin_merge_options,\n+\t\t\t builtin_merge_usage, 0);\n+\tfree(buf);\n+}\n+\n+static int git_merge_config(const char *k, const char *v, void *cb)\n+{\n+\tif (cb != NULL && branch && !prefixcmp(k, \"branch.\"))\n+\t\tparse_git_merge_options(k, v, cb);\n+\n \tif (!strcmp(k, \"merge.diffstat\") || !strcmp(k, \"merge.stat\"))\n \t\tshow_diffstat = git_config_bool(k, v);\n \telse if (!strcmp(k, \"pull.twohead\"))\n@@ -987,6 +1034,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tconst char *head_arg;\n \tint flag, head_invalid = 0, i;\n \tint best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;\n+\tstruct merge_options_cb merge_options = {NULL, NULL, 0.0};\n \tstruct commit_list *common = NULL;\n \tconst char *best_strategy = NULL, *wt_strategy = NULL;\n \tstruct commit_list **remotes = &remoteheads;\n@@ -1004,7 +1052,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tif (is_null_sha1(head))\n \t\thead_invalid = 1;\n \n-\tgit_config(git_merge_config, NULL);\n+\tgit_config(git_merge_config, &merge_options);\n+\tif (merge_options.key != NULL && merge_options.value != NULL)\n+\t\tapply_merge_options(&merge_options);\n \n \t/* for color.ui */\n \tif (diff_use_color_default == -1)\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex e84e822..5b1f8e1 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -415,6 +415,45 @@ test_expect_success 'merge c0 with c1 (no-ff)' '\n \n test_debug 'git log --graph --decorate --oneline --all'\n \n+test_expect_success 'merge c0 with c1 (default no-ff)' '\n+\tgit reset --hard c0 &&\n+\ttest_might_fail git config --unset branch.master.mergeoptions &&\n+\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n+\ttest_tick &&\n+\tgit merge c1 &&\n+\tgit config --remove-section \"branch.*\" &&\n+\tverify_merge file result.1 &&\n+\tverify_parents $c0 $c1\n+'\n+\n+test_debug 'git log --graph --decorate --oneline --all'\n+\n+test_expect_success 'combine branch.*.mergeoptions with branch.x.mergeoptions' '\n+\tgit reset --hard c0 &&\n+\ttest_might_fail git config --remove-section branch.master &&\n+\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n+\tgit config branch.master.mergeoptions \"--ff\" &&\n+\ttest_tick &&\n+\tgit merge c1 &&\n+\tgit config --remove-section \"branch.*\" &&\n+\tverify_merge file result.1 &&\n+\tverify_parents \"$c0\"\n+'\n+\n+test_expect_success 'reverse branch.x.mergeoptions with branch.*.mergeoptions' '\n+\tgit reset --hard c0 &&\n+\ttest_might_fail git config --remove-section branch.master &&\n+\tgit config branch.master.mergeoptions \"--ff\" &&\n+\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n+\ttest_tick &&\n+\tgit merge c1 &&\n+\tgit config --remove-section \"branch.*\" &&\n+\tverify_merge file result.1 &&\n+\tverify_parents \"$c0\"\n+'\n+\n+test_debug 'git log --graph --decorate --oneline --all'\n+\n test_expect_success 'combining --squash and --no-ff is refused' '\n \ttest_must_fail git merge --squash --no-ff c1 &&\n \ttest_must_fail git merge --no-ff --squash c1\n-- \n1.7.5\n"},{"id":"166967","messageId":"4DC0679C.7090705@web.de","threadId":"27241","inReplyTo":"7vk4e7ir9v.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] Add default merge options for all branches","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2011-05-03T20:37:48Z","receivedAt":"2011-05-03T20:37:48Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 03.05.2011 20:16, schrieb Junio C Hamano:\n> Jonathan Nieder writes:\n>>> The need for this arises from the fact that there is currently not an\n>>> easy way to set merge options for all branches.\n>>\n>> I'm curious: what merge options/workflows does this tend to be useful\n>> for?\n> \n> I actually am curious myself, too. I want to see a real-life example.\n\nAt my dayjob the reviewer of a branch does the merge of a topic into\nmaster. And to always record who did the review even if the merge is\na fast forward we use the \"branch.master.mergeoptions=--no-ff\" config\nin all our repos, set by our installer.\n\nBut that doesn't work out of the box for our release branches, we have\nto manually set stuff like \"branch.v1.23-4.mergeoptions=--no-ff\" for\nevery release branch in every clone where merges are done. This is easy\nto forget and as we use git gui for our daily work, we can't simply use\nthe command line option --no-ff.\n\nSo we would greatly benefit from this patch (and actually I looked into\ndoing it myself some time ago but postponed it to do some submodule\nstuff first ;-)\n"},{"id":"166968","messageId":"20110503204442.GI1019@elie","threadId":"27241","inReplyTo":"4DC0608F.9040208@dailyvoid.com","subject":"Re: [PATCH v4] Add default merge options for all branches","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-03T20:44:42Z","receivedAt":"2011-05-03T20:44:42Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Grubb wrote:\n\n> In order to facilitate future features centered around this new\n> \"globlike\" syntax a new struct has been created to keep track of the\n> branch.*.* options. Currently it only supports branch.*.mergeoptions,\n> but can be easily modified in the future to support other branch\n> specific options as well. The mechanism will \"vote\" on specific-\"ness\"\n> of the configuration key and ultimately only use the most specific\n> options.\n\nMy immediate reactions:\n\n - Is this really needed? If we need some priority field when other\n branch globs are being supported, can't we add it then?\n\n - Floating point? *turns to flee*\n\n> --- a/Documentation/git-merge.txt\n> +++ b/Documentation/git-merge.txt\n> @@ -307,6 +307,9 @@ branch..mergeoptions::\n> \tSets default options for merging into branch . The syntax and\n> \tsupported options are the same as those of 'git merge', but option\n> \tvalues containing whitespace characters are currently not supported.\n> +\tThe special value '*' for may be used to configure default\n> +\toptions for all branches. Values for specific branch names will\n> +\toverride the this default.\n\nIn what sense are they overridden? For example, if I write\n\n\t[branch \"*\"]\n\t\tmergeoptions = --no-ff\n\n\t[branch \"master\"]\n\t\tmergeoptions = --log=5\n\nand merge another branch into master, will the effect be as though I\nwrote --no-ff --log=5 or just --log=5?\n\nI'm starting to suspect it might be simpler to add a new \"[merge] no-ff\"\nconfiguration item, like the existing \"[merge] log\".\n\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -32,6 +32,21 @@\n> #define NO_FAST_FORWARD (1<<2)\n> #define NO_TRIVIAL (1<<3)\n> \n> +/* This is for branch.. blocks\n> + * the vote member holds a value between\n> + * 0.0 and 1.0 which measures how closely\n> + * a branch name matches the key member.\n> + * where branch.*.mergeoptions would be 0.1 and\n> + * branch..mergeoptions would be 1.0\n> + * Also it is called vote because I couldn't come\n> + * up with a better name.\n> + */\n\nStyle: comments should look like this:\n\n\t/*\n\t * Explanation comes here, with each line being\n\t * approximately the same length as the next one.\n\t */\n\n\"It is called vote because I couldn't come up with a better name\" does\nnot belong in a comment unless you are asking the reader to fix it.\nIn that case, I would write something like\n\n\tfloat vote;\t/* NEEDSWORK: give this a better name */\n\nor\n\n\tfloat priority;\n\n> +struct merge_options_cb {\n> +\tchar *key;\n> +\tchar *value;\n> +\tfloat vote;\n> +};\n\nSince I didn't read the comment, I'm not sure what these represent.\nWho is responsible for freeing them? Is the key a glob?\n\n> @@ -503,28 +518,60 @@ cleanup:\n> \tstrbuf_release(&bname);\n> }\n> \n> -static int git_merge_config(const char *k, const char *v, void *cb)\n> +static void parse_git_merge_options(const char *k, const char *v,\n> +\t\t\tvoid *cb)\n> {\n> -\tif (branch && !prefixcmp(k, \"branch.\") &&\n> -\t\t!prefixcmp(k + 7, branch) &&\n> -\t\t!strcmp(k + 7 + strlen(branch), \".mergeoptions\")) {\n> -\t\tconst char **argv;\n> -\t\tint argc;\n> -\t\tchar *buf;\n> -\n> -\t\tbuf = xstrdup(v);\n> -\t\targc = split_cmdline(buf, &argv);\n> -\t\tif (argc < 0)\n> -\t\t\tdie(_(\"Bad branch.%s.mergeoptions string: %s\"), branch,\n> -\t\t\t split_cmdline_strerror(argc));\n> -\t\targv = xrealloc(argv, sizeof(*argv) * (argc + 2));\n> -\t\tmemmove(argv + 1, argv, sizeof(*argv) * (argc + 1));\n> -\t\targc++;\n> -\t\tparse_options(argc, argv, NULL, builtin_merge_options,\n> -\t\t\t builtin_merge_usage, 0);\n> -\t\tfree(buf);\n> +\tstruct merge_options_cb *merge_options = cb;\n> +\tint changed = 0;\n> +\n> +\t/* We only handle mergeoptions for now */\n> +\tif (suffixcmp(k, \".mergeoptions\"))\n> +\t\treturn;\n\nThe function is called parse_git_merge_options; I wonder if this \"for\nnow\" is an old comment from when it had a different name.\n\n> +\n> +\tif (!prefixcmp(k, \"branch.*\") && merge_options->vote <= 0.1 ) {\n> +\t\tmerge_options->vote = 0.1;\n> +\t\tchanged = 1;\n\nStyle: parens. What happens if I use\n\n\t[branch \"*aster\"]\n\t\tmergeoptions = ...\n\nor\n\n\t[branch \"*.c\"]\n\t\tmergeoptions = ...\n\n? Where does 0.1 come from?\n\n> +\t} else if (branch && !prefixcmp(k, \"branch.\") &&\n> +\t\t\t\t!prefixcmp(k + 7, branch) &&\n> +\t\t\t\tmerge_options->vote < 1.0) {\n> +\t\tmerge_options->vote = 1.0;\n> +\t\tchanged = 1;\n\nWhat does changed mean?\n\n> \t}\n> \n> +\tif (changed) {\n> +\t\tmerge_options->key = (char *)k;\n> +\t\tmerge_options->value = (char *)v;\n\nWhy not make the struct fields const?\n\n> +\t}\n> +}\n> +\n> +static void apply_merge_options(struct merge_options_cb *opts)\n> +{\n> +\tconst char **argv;\n> +\tint argc;\n> +\tchar *buf;\n> +\n> +\tif ( opts == NULL )\n\nStyle: parens. It would be more idiomatic to say\n\n\tif (!opts)\n\n> +\t\treturn;\n> +\n> +\tbuf = xstrdup(opts->value);\n> +\targc = split_cmdline(buf, &argv);\n> +\tif (argc < 0)\n> +\t\tdie(_(\"Bad %s string: %s\"), \n> +\t\t\topts->key, split_cmdline_strerror(argc));\n> +\n> +\targv = xrealloc(argv, sizeof(*argv) * (argc + 2));\n> +\tmemmove(argv + 1, argv, sizeof(*argv) * (argc + 1));\n> +\targc++;\n> +\tparse_options(argc, argv, NULL, builtin_merge_options,\n> +\t\t\t builtin_merge_usage, 0);\n> +\tfree(buf);\n> +}\n\nIdea: the series could be made easier to read if splitting this off\ninto its own function were a separate patch. Anyway, splitting it out\nwas a good idea; thanks for doing that.\n\nIt seems you changed the whitespace while unindenting? The whitespace\non the continuation line for parse_options(...) arguments is\nespecially confusing (I suspect you are using a tabwidth of 6, but\nthat doesn't make sense, either, since opts->key is not an argument to\n_. The official rendering uses a tabwidth of 8).\n\n> +\n> +static int git_merge_config(const char *k, const char *v, void *cb)\n> +{\n> +\tif (cb != NULL && branch && !prefixcmp(k, \"branch.\"))\n> +\t\tparse_git_merge_options(k, v, cb);\n\nMore idiomatic to say\n\n\tif (!cb && branch && !prefixcmp(k, \"branch.\"))\n\t\t...\n\nChanging behavior when cb is NULL seems odd to me. Would it ever\nactually be NULL?\n\n> @@ -1004,7 +1052,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n> \tif (is_null_sha1(head))\n> \t\thead_invalid = 1;\n> \n> -\tgit_config(git_merge_config, NULL);\n> +\tgit_config(git_merge_config, &merge_options);\n> +\tif (merge_options.key != NULL && merge_options.value != NULL)\n> +\t\tapply_merge_options(&merge_options);\n\nMore idiomatic to say\n\n\tif (merge_options.key && merge_options.value)\n\nBut this seems odd, too --- when would \"key\" be non-null without\n\"value\" being so?\n\n> --- a/t/t7600-merge.sh\n> +++ b/t/t7600-merge.sh\n> @@ -415,6 +415,45 @@ test_expect_success 'merge c0 with c1 (no-ff)' '\n[...]\n> +test_expect_success 'combine branch.*.mergeoptions with branch.x.mergeoptions' '\n> +\tgit reset --hard c0 &&\n> +\ttest_might_fail git config --remove-section branch.master &&\n> +\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n> +\tgit config branch.master.mergeoptions \"--ff\" &&\n> +\ttest_tick &&\n> +\tgit merge c1 &&\n> +\tgit config --remove-section \"branch.*\" &&\n> +\tverify_merge file result.1 &&\n> +\tverify_parents \"$c0\"\n> +'\n> +\n> +test_expect_success 'reverse branch.x.mergeoptions with branch.*.mergeoptions' '\n> +\tgit reset --hard c0 &&\n> +\ttest_might_fail git config --remove-section branch.master &&\n> +\tgit config branch.master.mergeoptions \"--ff\" &&\n> +\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n> +\ttest_tick &&\n> +\tgit merge c1 &&\n\nThanks. This might pass by mistake with a \"git config\" implementation\nthat sorts its entries when writing them; since I think git is\nintended to allow users writing to the config file directly, too,\nmaybe something along the lines of\n\n\tcat >>.git/config <<-\\EOF &&\n\t[branch \"master\"]\n\t\tmergeoptions = --ff\n\n\t[branch \"*\"]\n\t\tmergeoptions = --no-ff\n\tEOF\n\ncould be interesting.\n\nAlso --ff and --no-ff do not have orthogonal effect. What happens\nwhen the merge options do (e.g., --ff and --log)?\n\n> +\tgit config --remove-section \"branch.*\" &&\n> +\tverify_merge file result.1 &&\n> +\tverify_parents \"$c0\"\n> +'\n\nThanks again.\n\nHope that helps,\nJonathan\n"},{"id":"166969","messageId":"20110503205045.GK1019@elie","threadId":"27241","inReplyTo":"4DC0641D.5070403@dailyvoid.com","subject":"Re: [PATCH v3] Add default merge options for all branches","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-03T20:50:45Z","receivedAt":"2011-05-03T20:50:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Grubb wrote:\n> On 5/3/11 1:16 PM, Junio C Hamano wrote:\n>> Jonathan Nieder writes:\n\n>>> Yuck. Did anything come of the idea of a --between-tests option to\n>>> use an arbitrary command here automatically? (Not your fault.) \n>>\n>> I actually think test_debug should go inside the previous test. Why not\n>> have it immediately after \"git merge c1\" above?\n>\n> Again I followed the existing pattern here. I didn't want to be the odd man out.\n> Do you want me to make that change?\n\nNo, I was just reminding the list at large about how annoying it is.\n\nYou did right to mimic the style of surrounding tests. Thank you.\n"},{"id":"166974","messageId":"7vwri7h26z.fsf@alter.siamese.dyndns.org","threadId":"27241","inReplyTo":"4DC0608F.9040208@dailyvoid.com","subject":"Re: [PATCH v4] Add default merge options for all branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-03T22:03:16Z","receivedAt":"2011-05-03T22:03:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Grubb writes:\n\n/*\n * Our Multi-line comments begin with a line with\n * slash asterisk then newline.\n */\n\n> +/* This is for branch.. blocks\n> + * the vote member holds a value between\n> + * 0.0 and 1.0 which measures how closely\n> + * a branch name matches the key member.\n> + * where branch.*.mergeoptions would be 0.1 and\n> + * branch..mergeoptions would be 1.0\n> + * Also it is called vote because I couldn't come\n> + * up with a better name.\n> + */\n\nHow about simply dropping that \"vote\" thing? I do not want to see\nunnecessary float creeping into our codebase.\n\nThe k and v parameters are volatile from the point of view of this\nfunction. You need to xstrdup() them to keep a copy.\n\nThere is no need to store \"branch.\" part in cb->key, as it is common\nacross the variables.\n\nThe logic would probably look like this:\n\n\tif (prefixcmp(k, \"branch.\"))\n\t\treturn;\n\tk += 7; /* past \"branch.\" part */\n\teon = strrchr(k, '.'); /* end-of-name 8/\n if (!eon || strcmp(eon, \".mergeoptions\"))\n \treturn;\n\n\t/* k thru eon is the name or wildcard */\n\tspec = xmemdupz(k, eon - k);\n /*\n * NEEDSWORK: for now we say \"*\" matches; we would need\n * to turn the following into something like:\n *\tif (has_wildcard(spec) \n\t *\t\t? !glob_matches(spec, branch)\n\t *\t\t: strcmp(spec, branch)) {\n *\t\tfree(spec);\n *\t\treturn;\n *\t}\n */\n\tif (strcmp(spec, \"*\") && strcmp(spec, branch)) {\n \tfree(spec);\n return;\n\t}\n\n if (!merge_options->option ||\n cmp_specificity(merge_options->spec, spec) < 0) {\n\t\t/* use this one */\n free(merge_options->spec);\n free(merge_options->option);\n merge_options->option = xstrdup(v);\n merge_options->spec = spec;\n\t\treturn;\n\t}\n free(spec);\n\nAnd then cmp_specificity() would say something like:\n\n\tstatic int cmp_specificity(const char *a, const char *b)\n {\n \tswitch ((!strcmp(a, \"*\") ? 2 : 0) |\n \t(!strcmp(b, \"*\") ? 1 : 0)) {\n\t\tcase 3:\n /*\n * NEEDSWORK: when we start truly globbing,\n * we need to decide \"foo/*\" is more specific than\n * \"*\" and the like. But for now we do not have to\n * worry about that case.\n */\n\t\tcase 0:\n return -1; /* later one wins if they are the same */\n\t\tcase 1:\n\t\t\treturn 1;\n\t\tcase 2:\n\t\t\treturn -1;\n\t\t}\n\t}\n\nmeaning, the ones with wildcard are weaker than the ones without.\n"},{"id":"166975","messageId":"7vsjsvgzzf.fsf@alter.siamese.dyndns.org","threadId":"27241","inReplyTo":"20110503204442.GI1019@elie","subject":"Re: [PATCH v4] Add default merge options for all branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-03T22:51:00Z","receivedAt":"2011-05-03T22:51:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> In what sense are they overridden? For example, if I write\n>\n> \t[branch \"*\"]\n> \t\tmergeoptions = --no-ff\n>\n> \t[branch \"master\"]\n> \t\tmergeoptions = --log=5\n>\n> and merge another branch into master, will the effect be as though I\n> wrote --no-ff --log=5 or just --log=5?\n\nI think the latter is overriding, and the former is cumulative.\n\n> I'm starting to suspect it might be simpler to add a new \"[merge] no-ff\"\n> configuration item, like the existing \"[merge] log\".\n\nSurely\n\n\t[merge]\n \tlog = false\n ff = false\n\nwould be a lot simpler and probably far easier to explain.\n\nDoes\n\n\t[merge]\n\t\tlog = false\n\t[branch \"master\"]\n \tmergeoptions = --log=5\n\ndo the right thing with the current codebase?\n"},{"id":"166988","messageId":"7vzkn3f5wo.fsf@alter.siamese.dyndns.org","threadId":"27241","inReplyTo":"7vsjsvgzzf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4] Add default merge options for all branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-04T04:25:59Z","receivedAt":"2011-05-04T04:25:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jonathan Nieder writes:\n>\n>> I'm starting to suspect it might be simpler to add a new \"[merge] no-ff\"\n>> configuration item, like the existing \"[merge] log\".\n>\n> Surely\n>\n> \t[merge]\n> \tlog = false\n> ff = false\n>\n> would be a lot simpler and probably far easier to explain.\n\nYes, it is far simpler and easier to explain. I'll leave the tests and\nthe commit log message to people who are more interested in this topic\nthan I am ;-)\n\n Documentation/merge-config.txt | 6 ++++++\n builtin/merge.c | 3 +++\n 2 files changed, 9 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\nindex 8920258..2aa4408 100644\n--- a/Documentation/merge-config.txt\n+++ b/Documentation/merge-config.txt\n@@ -16,6 +16,12 @@ merge.defaultToUpstream::\n \tto their corresponding remote tracking branches, and the tips of\n \tthese tracking branches are merged.\n \n+merge.ff::\n+\tDo not generate a merge commit if the merge resolved as a\n+\tfast-forward; only update the branch pointer instead. Setting\n+\tthis to `false` would be equivalent to giving `--no-ff` from\n+\tthe command line.\n+\n merge.log::\n \tIn addition to branch names, populate the log message with at\n \tmost the specified number of one-line descriptions from the\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex d171c63..5194f04 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -541,6 +541,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)\n \t\tif (is_bool && shortlog_len)\n \t\t\tshortlog_len = DEFAULT_MERGE_LOG_LEN;\n \t\treturn 0;\n+\t} else if (!strcmp(k, \"merge.ff\")) {\n+\t\tallow_fast_forward = git_config_bool(k, v);\n+\t\treturn 0;\n \t} else if (!strcmp(k, \"merge.defaulttoupstream\")) {\n \t\tdefault_to_upstream = git_config_bool(k, v);\n \t\treturn 0;\n"},{"id":"166989","messageId":"4DC0D605.20204@dailyvoid.com","threadId":"27241","inReplyTo":"7vzkn3f5wo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4] Add default merge options for all branches","fromName":"Michael Grubb","fromEmail":"devel@dailyvoid.com","sentAt":"2011-05-04T04:28:53Z","receivedAt":"2011-05-04T04:28:53Z","isPatch":true,"sender":{"key":"devel@dailyvoid.com","avatar":"https://gravatar.com/avatar/5ff035e599312653f041129e68041a9078c7e1f3268eaac3899c04e353d2133e?d=mp&s=160"},"body":"I take this to mean that my patch is no longer needed/wanted?\n\nOn 5/3/11 11:25 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> Jonathan Nieder writes:\n>>\n>>> I'm starting to suspect it might be simpler to add a new \"[merge] no-ff\"\n>>> configuration item, like the existing \"[merge] log\".\n>>\n>> Surely\n>>\n>> \t[merge]\n>> \tlog = false\n>> ff = false\n>>\n>> would be a lot simpler and probably far easier to explain.\n> \n> Yes, it is far simpler and easier to explain. I'll leave the tests and\n> the commit log message to people who are more interested in this topic\n> than I am ;-)\n> \n> Documentation/merge-config.txt | 6 ++++++\n> builtin/merge.c | 3 +++\n> 2 files changed, 9 insertions(+), 0 deletions(-)\n> \n> diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\n> index 8920258..2aa4408 100644\n> --- a/Documentation/merge-config.txt\n> +++ b/Documentation/merge-config.txt\n> @@ -16,6 +16,12 @@ merge.defaultToUpstream::\n> \tto their corresponding remote tracking branches, and the tips of\n> \tthese tracking branches are merged.\n> \n> +merge.ff::\n> +\tDo not generate a merge commit if the merge resolved as a\n> +\tfast-forward; only update the branch pointer instead. Setting\n> +\tthis to `false` would be equivalent to giving `--no-ff` from\n> +\tthe command line.\n> +\n> merge.log::\n> \tIn addition to branch names, populate the log message with at\n> \tmost the specified number of one-line descriptions from the\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index d171c63..5194f04 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -541,6 +541,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)\n> \t\tif (is_bool && shortlog_len)\n> \t\t\tshortlog_len = DEFAULT_MERGE_LOG_LEN;\n> \t\treturn 0;\n> +\t} else if (!strcmp(k, \"merge.ff\")) {\n> +\t\tallow_fast_forward = git_config_bool(k, v);\n> +\t\treturn 0;\n> \t} else if (!strcmp(k, \"merge.defaulttoupstream\")) {\n> \t\tdefault_to_upstream = git_config_bool(k, v);\n> \t\treturn 0;\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"166991","messageId":"20110504045802.GF8187@elie","threadId":"27241","inReplyTo":"4DC0D605.20204@dailyvoid.com","subject":"Re: [PATCH v4] Add default merge options for all branches","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-04T04:58:03Z","receivedAt":"2011-05-04T04:58:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Grubb wrote:\n\n> I take this to mean that my patch is no longer needed/wanted?\n\nNo, as usual it means that Junio was thinking and sent us his\nthoughts.\n\nDo you like it? If so, the way to move it forward would be to add\ntests and write a commit log message. If not, the way to move things\nforward would be to explain what use case it misses and work on a\nbetter patch that takes care of them.\n\nHope that helps,\nJonathan\n"},{"id":"167014","messageId":"BANLkTin0Mi0sOWR5_mYnONW816cVoaXtbA@mail.gmail.com","threadId":"27241","inReplyTo":"7vzkn3f5wo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4] Add default merge options for all branches","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2011-05-04T10:58:08Z","receivedAt":"2011-05-04T10:58:08Z","isPatch":true,"sender":{"key":"john@szakmeister.net","avatar":"https://avatars.githubusercontent.com/u/448087?v=4"},"body":"On Wed, May 4, 2011 at 12:25 AM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Jonathan Nieder writes:\n>>\n>>> I'm starting to suspect it might be simpler to add a new \"[merge] no-ff\"\n>>> configuration item, like the existing \"[merge] log\".\n>>\n>> Surely\n>>\n>>       [merge]\n>>               log = false\n>>               ff = false\n>>\n>> would be a lot simpler and probably far easier to explain.\n>\n> Yes, it is far simpler and easier to explain.  I'll leave the tests and\n> the commit log message to people who are more interested in this topic\n> than I am ;-)\n\nI like the idea. I assume it can be overridden on the command line too?\n\n-John\n"},{"id":"167036","messageId":"4DC1A1BC.5010601@dailyvoid.com","threadId":"27241","inReplyTo":"20110504045802.GF8187@elie","subject":"Re: [PATCH v4] Add default merge options for all branches","fromName":"Michael Grubb","fromEmail":"devel@dailyvoid.com","sentAt":"2011-05-04T18:58:04Z","receivedAt":"2011-05-04T18:58:04Z","isPatch":true,"sender":{"key":"devel@dailyvoid.com","avatar":"https://gravatar.com/avatar/5ff035e599312653f041129e68041a9078c7e1f3268eaac3899c04e353d2133e?d=mp&s=160"},"body":"Well, it certainly serves *my* immediate needs and addresses the specific use case that I was originally working on. Though I think that what we've come up with would benefit the codebase in general if for no other reason than it lays some ground work for future features and is a bit more generic. I also wouldn't mind going a step further and working on the globbing feature in a separate series.\n\nHere are some corrections to the previous patch:\n\nThis patch makes the branch specific options more flexible by laying\nthe ground work for supporting globs for the branch names.\nAt this time only the value '*' is supported which will set default\nvalues for all branches. In the future this may be expanded to support\ntrue globbing.\n\nSigned-off-by: Michael Grubb \n---\n builtin/merge.c | 120 ++++++++++++++++++++++++++++++++++-------------------\n t/t7600-merge.sh | 27 ++++++++----\n 2 files changed, 95 insertions(+), 52 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex fa56205..9e6ec5f 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -31,20 +31,12 @@\n #define DEFAULT_OCTOPUS (1<<1)\n #define NO_FAST_FORWARD (1<<2)\n #define NO_TRIVIAL (1<<3)\n+#define MERGE_OPTIONS_INIT {NULL, NULL, NULL}\n \n-/* This is for branch.. blocks\n- * the vote member holds a value between\n- * 0.0 and 1.0 which measures how closely\n- * a branch name matches the key member.\n- * where branch.*.mergeoptions would be 0.1 and\n- * branch..mergeoptions would be 1.0\n- * Also it is called vote because I couldn't come\n- * up with a better name.\n- */\n struct merge_options_cb {\n-\tchar *key;\n-\tchar *value;\n-\tfloat vote;\n+\tchar *option;\n+\tchar *spec;\n+\tchar *name;\n };\n \n struct strategy {\n@@ -518,30 +510,11 @@ cleanup:\n \tstrbuf_release(&bname);\n }\n \n-static void parse_git_merge_options(const char *k, const char *v,\n-\t\t\tvoid *cb)\n+static void free_merge_options_cb(struct merge_options_cb *data)\n {\n-\tstruct merge_options_cb *merge_options = cb;\n-\tint changed = 0;\n-\n-\t/* We only handle mergeoptions for now */\n-\tif (suffixcmp(k, \".mergeoptions\"))\n-\t\treturn;\n-\n-\tif (!prefixcmp(k, \"branch.*\") && merge_options->vote <= 0.1 ) {\n-\t\tmerge_options->vote = 0.1;\n-\t\tchanged = 1;\n-\t} else if (branch && !prefixcmp(k, \"branch.\") &&\n-\t\t\t\t!prefixcmp(k + 7, branch) &&\n-\t\t\t\tmerge_options->vote < 1.0) {\n-\t\tmerge_options->vote = 1.0;\n-\t\tchanged = 1;\n-\t}\n-\n-\tif (changed) {\n-\t\tmerge_options->key = (char *)k;\n-\t\tmerge_options->value = (char *)v;\n-\t}\n+\tfree(data->option);\n+\tfree(data->spec);\n+\tfree(data->name);\n }\n \n static void apply_merge_options(struct merge_options_cb *opts)\n@@ -550,26 +523,85 @@ static void apply_merge_options(struct merge_options_cb *opts)\n \tint argc;\n \tchar *buf;\n \n-\tif ( opts == NULL )\n+\tif (!opts)\n \t\treturn;\n \n-\tbuf = xstrdup(opts->value);\n+\tbuf = xstrdup(opts->option);\n \targc = split_cmdline(buf, &argv);\n \tif (argc < 0)\n-\t\tdie(_(\"Bad %s string: %s\"),\n-\t\t\topts->key, split_cmdline_strerror(argc));\n+\t\tdie(_(\"Bad branch.%s.%s string: %s\"),\n+\t\t\topts->spec, opts->name, split_cmdline_strerror(argc));\n \n \targv = xrealloc(argv, sizeof(*argv) * (argc + 2));\n \tmemmove(argv + 1, argv, sizeof(*argv) * (argc + 1));\n \targc++;\n \tparse_options(argc, argv, NULL, builtin_merge_options,\n-\t\t\t builtin_merge_usage, 0);\n+\t\tbuiltin_merge_usage, 0);\n \tfree(buf);\n }\n \n+/*\n+ * This function returns -1 if the first argument is less specific\n+ * than the second, and 1 vice versa. Returns -1 if both arguments\n+ * are the same.\n+ */\n+static int cmp_specificity(const char *a, const char *b)\n+{\n+\tswitch((!strcmp(a, \"*\") ? 2 : 0) |\n+\t\t(!strcmp(b, \"*\") ? 1 : 0)) {\n+\tcase 3:\n+\tcase 0:\n+\t\treturn -1; /* later one wins if they are the same */\n+\tcase 1:\n+\t\treturn 1;\n+\tcase 2:\n+\t\treturn -1;\n+\t}\n+\treturn 0; /* should never happen */\n+}\n+\n+static void parse_git_merge_options(const char *k, const char *v,\n+\t\t\tvoid *cb)\n+{\n+\tstruct merge_options_cb *merge_options = cb;\n+\tchar *spec, *eon; /* end-of-name */\n+\n+\tk += 7; /* not interested in the leading \"branch.\" */\n+\teon = strrchr(k, '.');\n+\tif (!eon || strcmp(eon, \".mergeoptions\"))\n+\t\treturn;\n+\n+\t/* k through eon is name or glob */\n+\tspec = xmemdupz(k, eon - k);\n+\t/*\n+\t * NEEDSWORK: for now we say \"*\" matches; we would need\n+\t * to turn the following into something like:\n+\t *\tif (has_wildcard(spec)\n+\t *\t\t? !glob_matches(spec, branch)\n+\t *\t\t: strcmp(spec, branch)) {\n+\t *\t\tfree(spec);\n+\t *\t\treturn;\n+\t *\t}\n+\t */\n+\tif (strcmp(spec, \"*\") && strcmp(spec, branch)) {\n+\t\tfree(spec);\n+\t\treturn;\n+\t}\n+\n+\tif (!merge_options->option ||\n+\t\t\tcmp_specificity(merge_options->spec, spec) < 0) {\n+\t\tfree_merge_options_cb(merge_options);\n+\t\tmerge_options->option = xstrdup(v);\n+\t\tmerge_options->name = xstrdup(eon + 1);\n+\t\tmerge_options->spec = spec;\n+\t\treturn;\n+\t}\n+\tfree(spec);\n+}\n+\n static int git_merge_config(const char *k, const char *v, void *cb)\n {\n-\tif (cb != NULL && branch && !prefixcmp(k, \"branch.\"))\n+\tif (branch && !prefixcmp(k, \"branch.\"))\n \t\tparse_git_merge_options(k, v, cb);\n \n \tif (!strcmp(k, \"merge.diffstat\") || !strcmp(k, \"merge.stat\"))\n@@ -1034,7 +1066,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tconst char *head_arg;\n \tint flag, head_invalid = 0, i;\n \tint best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;\n-\tstruct merge_options_cb merge_options = {NULL, NULL, 0.0};\n+\tstruct merge_options_cb merge_options = MERGE_OPTIONS_INIT;\n \tstruct commit_list *common = NULL;\n \tconst char *best_strategy = NULL, *wt_strategy = NULL;\n \tstruct commit_list **remotes = &remoteheads;\n@@ -1053,8 +1085,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\thead_invalid = 1;\n \n \tgit_config(git_merge_config, &merge_options);\n-\tif (merge_options.key != NULL && merge_options.value != NULL)\n+\tif (merge_options.option) {\n \t\tapply_merge_options(&merge_options);\n+\t\tfree_merge_options_cb(&merge_options);\n+\t}\n \n \t/* for color.ui */\n \tif (diff_use_color_default == -1)\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 5b1f8e1..d7a60b2 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -417,37 +417,46 @@ test_debug 'git log --graph --decorate --oneline --all'\n \n test_expect_success 'merge c0 with c1 (default no-ff)' '\n \tgit reset --hard c0 &&\n-\ttest_might_fail git config --unset branch.master.mergeoptions &&\n-\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n+\tcat >.git/config <<-\\EOF &&\n+\t[branch \"*\"]\n+\t\tmergeoptions = --no-ff\n+\tEOF\n \ttest_tick &&\n \tgit merge c1 &&\n \tgit config --remove-section \"branch.*\" &&\n \tverify_merge file result.1 &&\n \tverify_parents $c0 $c1\n '\n-\n test_debug 'git log --graph --decorate --oneline --all'\n \n test_expect_success 'combine branch.*.mergeoptions with branch.x.mergeoptions' '\n \tgit reset --hard c0 &&\n-\ttest_might_fail git config --remove-section branch.master &&\n-\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n-\tgit config branch.master.mergeoptions \"--ff\" &&\n+\tcat >.git/config <<-\\EOF &&\n+\t[branch \"*\"]\n+\t\tmergeoptions = --no-ff\n+\t[branch \"master\"]\n+\t\tmergeoptions = --ff\n+\tEOF\n \ttest_tick &&\n \tgit merge c1 &&\n \tgit config --remove-section \"branch.*\" &&\n+\tgit config --remove-section \"branch.master\" &&\n \tverify_merge file result.1 &&\n \tverify_parents \"$c0\"\n '\n \n test_expect_success 'reverse branch.x.mergeoptions with branch.*.mergeoptions' '\n \tgit reset --hard c0 &&\n-\ttest_might_fail git config --remove-section branch.master &&\n-\tgit config branch.master.mergeoptions \"--ff\" &&\n-\tgit config \"branch.*.mergeoptions\" \"--no-ff\" &&\n+\tcat >.git/config <<-\\EOF &&\n+\t[branch \"master\"]\n+\t\tmergeoptions = --ff\n+\t[branch \"*\"]\n+\t\tmergeoptions = --no-ff\n+\tEOF\n \ttest_tick &&\n \tgit merge c1 &&\n \tgit config --remove-section \"branch.*\" &&\n+\tgit config --remove-section \"branch.master\" &&\n \tverify_merge file result.1 &&\n \tverify_parents \"$c0\"\n '\n-- \n1.7.5\n\n\n\nOn 5/3/11 11:58 PM, Jonathan Nieder wrote:\n> Michael Grubb wrote:\n> \n>> I take this to mean that my patch is no longer needed/wanted?\n> \n> No, as usual it means that Junio was thinking and sent us his\n> thoughts.\n> \n> Do you like it? If so, the way to move it forward would be to add\n> tests and write a commit log message. If not, the way to move things\n> forward would be to explain what use case it misses and work on a\n> better patch that takes care of them.\n> \n> Hope that helps,\n> Jonathan\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"167048","messageId":"7vei4edu8o.fsf@alter.siamese.dyndns.org","threadId":"27241","inReplyTo":"4DC1A1BC.5010601@dailyvoid.com","subject":"Re: [PATCH v4] Add default merge options for all branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-04T21:35:35Z","receivedAt":"2011-05-04T21:35:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Grubb writes:\n\n> Well, it certainly serves *my* immediate needs and addresses the\n> specific use case that I was originally working on. Though I think that\n> what we've come up with would benefit the codebase in general if for no\n> other reason than it lays some ground work for future features...\n\nThe discussion was fruitful, I think, and it may have laid the groundwork\nat the _conceptual level_. I however do not think that the approach the\npatch takes is generic enough, even if we add globbing of the branch name,\nto build on other things that can come out of the future directions the\ndiscussion suggested.\n\nFor example, why does \"merge.c\" hold the parser for \"branch.*.*\", when\nthere are a lot more configuration variables that apply per branch that\nare totally unrelated to merge? Don't these branch.*.* variables benefit\nfrom having a wildcard support as well? If we wanted to add such support,\nwhich configuration parser should be called by many pgorams that deal with\nbranches (e.g. \"checkout -b\", \"branch\", \"fetch\", ...), and where that\nparser should be defined in? I suspect the answer might be \"branch.c\",\nbut I do not htink we explored the uses widely enough to make that\ndecision yet.\n\nEven if I limit the discussion to \"mergeoptions\" [*1*], why can I specify\nthe merge options based on what branch I am currently on, but not based on\nwhat branch I am merging to my current branch? When on master branch,\nshould branch.*.mo augument what I have in branch.master.mo, or should it\noverwrite it? I may want to say \"all my merges should use --log\" and\n\"when on master I want --no-ff\". Should branch.master.mo repeat \"--log\",\nor should the values on the two variables automatically combine? If so in\nwhat order? Am I allowed to say \"Use 5 lines of --log\" for generic one,\nand then \"Use 10 more lines of --log than generic\" for branch.master.mo,\nso that later I can easily change my mind and update branch.*.mo to use 10\nlines, and I get automatically 20 lines for the master branch? If not why\nnot?\n\netc.etc.etc....\n\nI am not saying some of these issues are unsolvable, nor we should solve\nall these problems right now, but I do not think these issues are\nsomething you are trying to solve with this patch, nor the approach this\npatch happens to use was designed with these problems in mind (I certainly\ndidn't, when I made many suggestions I see in this round of your patch) to\nbecome a foundation to solve them in the future.\n\nThe suggestion by Jonathan does not have such a design issue that requires\nus to open a huge can of worms right now and potentially result in a\nsolution that is overengineered to address a wrong problem [*2*].\n\nIf it solves the original issue without such downsides, that would be more\npreferrable, I think; no?\n\n[Footnote]\n\n*1* I personally think \"branch..mergeoptions\" was a mistake. If it\nwere separate \"branch..merge-ff\", \"branch..merge-log\", ... it\nmight have been more clear what the combining semantics should be. But\nthat is not going to change, so I'd rather not to extend the support for\nit, until we come up with something more sensible.\n\n*2* For example, globbing to match the current branch name could become an\noverengineered solution to a wrong problem, if it turns out that it is not\nuseful to decide things based solely on the current branch name.\n"},{"id":"167054","messageId":"4DC1CE16.5030808@dailyvoid.com","threadId":"27241","inReplyTo":"20110503090351.GA27862@elie","subject":"[PATCH v5] Add default merge options for all branches","fromName":"Michael Grubb","fromEmail":"devel@dailyvoid.com","sentAt":"2011-05-04T22:07:18Z","receivedAt":"2011-05-04T22:07:18Z","isPatch":true,"sender":{"key":"devel@dailyvoid.com","avatar":"https://gravatar.com/avatar/5ff035e599312653f041129e68041a9078c7e1f3268eaac3899c04e353d2133e?d=mp&s=160"},"body":"Add merge.ff flag to support turning off fast-forward merges by default.\nThis flag is overridden if any branch.x.mergeoptions turn it back on.\n\n Documentation/merge-config.txt | 6 ++++++\n builtin/merge.c | 3 +++\n 2 files changed, 9 insertions(+), 0 deletions(-)\n\nSigned-off-by: Junio C Hamano \nReported-by: Michael Grubb \n---\n Documentation/merge-config.txt | 6 ++++++\n builtin/merge.c | 3 +++\n t/t7600-merge.sh | 23 +++++++++++++++++++++++\n 3 files changed, 32 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\nindex 8920258..2aa4408 100644\n--- a/Documentation/merge-config.txt\n+++ b/Documentation/merge-config.txt\n@@ -16,6 +16,12 @@ merge.defaultToUpstream::\n \tto their corresponding remote tracking branches, and the tips of\n \tthese tracking branches are merged.\n \n+merge.ff::\n+\tDo not generate a merge commit if the merge resolved as a\n+\tfast-forward; only update the branch pointer instead. Setting\n+\tthis to `false` would be equivalent to giving `--no-ff` from\n+\tthe command line.\n+\n merge.log::\n \tIn addition to branch names, populate the log message with at\n \tmost the specified number of one-line descriptions from the\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex d171c63..5194f04 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -541,6 +541,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)\n \t\tif (is_bool && shortlog_len)\n \t\t\tshortlog_len = DEFAULT_MERGE_LOG_LEN;\n \t\treturn 0;\n+\t} else if (!strcmp(k, \"merge.ff\")) {\n+\t\tallow_fast_forward = git_config_bool(k, v);\n+\t\treturn 0;\n \t} else if (!strcmp(k, \"merge.defaulttoupstream\")) {\n \t\tdefault_to_upstream = git_config_bool(k, v);\n \t\treturn 0;\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex e84e822..21c25d4 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -415,6 +415,29 @@ test_expect_success 'merge c0 with c1 (no-ff)' '\n \n test_debug 'git log --graph --decorate --oneline --all'\n \n+test_expect_success 'merge c0 with c1 (merge.ff=false)' '\n+\tgit reset --hard c0 &&\n+\tgit config merge.ff false &&\n+\ttest_tick &&\n+\tgit merge c1 &&\n+\tgit config --remove-section merge &&\n+\tverify_merge file result.1 &&\n+\tverify_parents $c0 $c1\n+'\n+test_debug 'git log --graph --decorate --oneline --all'\n+\n+test_expect_success 'combine branch.master.mergeoptions with merge.ff' '\n+\tgit reset --hard c0 &&\n+\tgit config branch.master.mergeoptions --ff\n+\tgit config merge.ff false\n+\ttest_tick &&\n+\tgit merge c1 &&\n+\tgit config --remove-section \"branch.master\" &&\n+\tgit config --remove-section \"merge\" &&\n+\tverify_merge file result.1 &&\n+\tverify_parents \"$c0\"\n+'\n+\n test_expect_success 'combining --squash and --no-ff is refused' '\n \ttest_must_fail git merge --squash --no-ff c1 &&\n \ttest_must_fail git merge --no-ff --squash c1\n"},{"id":"167058","messageId":"7vsjsuc704.fsf@alter.siamese.dyndns.org","threadId":"27241","inReplyTo":"4DC1CE16.5030808@dailyvoid.com","subject":"Re: [PATCH v5] Add default merge options for all branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-05T00:42:51Z","receivedAt":"2011-05-05T00:42:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n\nI think we still need to work on this a bit more, but I found an unrelated\nand nastier issue before this patch can sanely be applied.\n\n> +test_expect_success 'combine branch.master.mergeoptions with merge.ff' '\n> +\tgit reset --hard c0 &&\n> +\tgit config branch.master.mergeoptions --ff\n> +\tgit config merge.ff false\n> +\ttest_tick &&\n> +\tgit merge c1 &&\n> +\tgit config --remove-section \"branch.master\" &&\n> +\tgit config --remove-section \"merge\" &&\n> +\tverify_merge file result.1 &&\n> +\tverify_parents \"$c0\"\n> +'\n\nIf you insert an \"exit\" after this test and inspect the resulting commit,\nyou will see that it created a merge.\n\n\tSide note: I think verify_parents is buggy. It only makes sure\n\tthat the earlier parents of HEAD match the commits given, and does\n\tnot care if there actually are more parents.\n\nIn any case, your test exposed an ancient breakage ever since the\nper-branch mergeoptions was introduced back when git-merge was a shell\nscript (aec7b36 (git-merge: add support for branch..mergeoptions,\n2007-09-24).\n\nI am not going to fix verify_parents tonight, as I have other git things\nto do.\n\n-- >8 --\nSubject: [PATCH] merge: fix branch..mergeoptions\n\nThe parsing of the additional command line parameters supplied to\nthe branch..mergeoptions configuration variable was implemented\nat the wrong stage. If any merge-related variable came after we read\nbranch..mergeoptions, the earlier value was overwritten.\n\nWe should first read all the merge.* configuration, override them by\nreading from branch..mergeoptions and then finally read from\nthe command line.\n\nThis patch should fix it, even though I now strongly suspect that\nbranch..mergeoptions that gives a single command line that\nneeds to be parsed was likely to be an ill-conceived idea to begin\nwith. Sigh...\n\nSigned-off-by: Junio C Hamano \n---\n builtin-merge.c | 39 +++++++++++++++++++++++++--------------\n t/t7600-merge.sh | 32 ++++++++++++++++++++++++++++++++\n 2 files changed, 57 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin-merge.c b/builtin-merge.c\nindex 3aaec7b..01389a3 100644\n--- a/builtin-merge.c\n+++ b/builtin-merge.c\n@@ -54,6 +54,7 @@ static size_t use_strategies_nr, use_strategies_alloc;\n static const char **xopts;\n static size_t xopts_nr, xopts_alloc;\n static const char *branch;\n+static char *branch_mergeoptions;\n static int verbosity;\n static int allow_rerere_auto;\n \n@@ -474,25 +475,33 @@ cleanup:\n \tstrbuf_release(&bname);\n }\n \n+static void parse_branch_merge_options(char *bmo)\n+{\n+\tconst char **argv;\n+\tint argc;\n+\tchar *buf;\n+\n+\tif (!bmo)\n+\t\treturn;\n+\targc = split_cmdline(bmo, &argv);\n+\tif (argc < 0)\n+\t\tdie(\"Bad branch.%s.mergeoptions string\", branch);\n+\targv = xrealloc(argv, sizeof(*argv) * (argc + 2));\n+\tmemmove(argv + 1, argv, sizeof(*argv) * (argc + 1));\n+\targc++;\n+\tparse_options(argc, argv, NULL, builtin_merge_options,\n+\t\t builtin_merge_usage, 0);\n+\tfree(buf);\n+}\n+\n static int git_merge_config(const char *k, const char *v, void *cb)\n {\n \tif (branch && !prefixcmp(k, \"branch.\") &&\n \t\t!prefixcmp(k + 7, branch) &&\n \t\t!strcmp(k + 7 + strlen(branch), \".mergeoptions\")) {\n-\t\tconst char **argv;\n-\t\tint argc;\n-\t\tchar *buf;\n-\n-\t\tbuf = xstrdup(v);\n-\t\targc = split_cmdline(buf, &argv);\n-\t\tif (argc < 0)\n-\t\t\tdie(\"Bad branch.%s.mergeoptions string\", branch);\n-\t\targv = xrealloc(argv, sizeof(*argv) * (argc + 2));\n-\t\tmemmove(argv + 1, argv, sizeof(*argv) * (argc + 1));\n-\t\targc++;\n-\t\tparse_options(argc, argv, NULL, builtin_merge_options,\n-\t\t\t builtin_merge_usage, 0);\n-\t\tfree(buf);\n+\t\tfree(branch_mergeoptions);\n+\t\tbranch_mergeoptions = xstrdup(v);\n+\t\treturn 0;\n \t}\n \n \tif (!strcmp(k, \"merge.diffstat\") || !strcmp(k, \"merge.stat\"))\n@@ -918,6 +927,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tif (diff_use_color_default == -1)\n \t\tdiff_use_color_default = git_use_color_default;\n \n+\tif (branch_mergeoptions)\n+\t\tparse_branch_merge_options(branch_mergeoptions);\n \targc = parse_options(argc, argv, prefix, builtin_merge_options,\n \t\t\tbuiltin_merge_usage, 0);\n \tif (verbosity < 0)\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 57f6d2b..56c653d 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -372,6 +372,38 @@ test_expect_success 'merge c1 with c2 (no-commit in config)' '\n \n test_debug 'gitk --all'\n \n+test_expect_success 'merge c1 with c2 (log in config)' '\n+\tgit config branch.master.mergeoptions \"\" &&\n+\tgit reset --hard c1 &&\n+\tgit merge --log c2 &&\n+\tgit show -s --pretty=tformat:%s%n%b >expect &&\n+\n+\tgit config branch.master.mergeoptions --log &&\n+\tgit reset --hard c1 &&\n+\tgit merge c2 &&\n+\tgit show -s --pretty=tformat:%s%n%b >actual &&\n+\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'merge c1 with c2 (log in config gets overridden)' '\n+\t(\n+\t\tgit config --remove-section branch.master\n+\t\tgit config --remove-section merge\n+\t)\n+\tgit reset --hard c1 &&\n+\tgit merge c2 &&\n+\tgit show -s --pretty=tformat:%s%n%b >expect &&\n+\n+\tgit config branch.master.mergeoptions \"--no-log\" &&\n+\tgit config merge.log true &&\n+\tgit reset --hard c1 &&\n+\tgit merge c2 &&\n+\tgit show -s --pretty=tformat:%s%n%b >actual &&\n+\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'merge c1 with c2 (squash in config)' '\n \tgit reset --hard c1 &&\n \tgit config branch.master.mergeoptions \"--squash\" &&\n-- \n1.7.5.284.g84c3a8\n\n \n"},{"id":"167281","messageId":"7v7ha3zhv0.fsf@alter.siamese.dyndns.org","threadId":"27241","inReplyTo":"7vsjsuc704.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v5] Add default merge options for all branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-06T20:36:35Z","receivedAt":"2011-05-06T20:36:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> In any case, your test exposed an ancient breakage ever since the\n> per-branch mergeoptions was introduced back when git-merge was a shell\n> script (aec7b36 (git-merge: add support for branch..mergeoptions,\n> 2007-09-24).\n>\n> -- >8 --\n> Subject: [PATCH] merge: fix branch..mergeoptions\n> ...\n\nAnd then on top of that fix, we can do this.\n\nI have a seemingly unrelated change to the existing test but that was\nbecause it only made sure that the --ff-only option made the command fail\nwhen it should fail, without making sure that it does not interfere when\nit should succeed. A typical symptom of \"showing off shiny new toy\nbecause I am too excited\" developer disease, I would guess. I didn't want\nto forget to fix it.\n\n-- >8 --\nSubject: [PATCH] merge: introduce merge.ff configuration variable\n\nThis variable gives the default setting for --ff, --no-ff or --ff-only\noptions of \"git merge\" command.\n\nHelped-by: Michael Grubb \nHelped-by: Jonathan Nieder \nSigned-off-by: Junio C Hamano \n---\n\n * If we were doing the command line option of \"merge\" from scratch today,\n we probably would have done --ff, --ff=no, and --ff=only, instead of a\n separate --ff-only. We could still add the latter two as a consistency\n synonyms without deprecating anything, though.\n\n Documentation/merge-config.txt | 10 +++++++++\n builtin/merge.c | 9 ++++++++\n t/t7600-merge.sh | 43 ++++++++++++++++++++++++++++++++++++++-\n 3 files changed, 60 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\nindex 8920258..861bd6f 100644\n--- a/Documentation/merge-config.txt\n+++ b/Documentation/merge-config.txt\n@@ -16,6 +16,16 @@ merge.defaultToUpstream::\n \tto their corresponding remote tracking branches, and the tips of\n \tthese tracking branches are merged.\n \n+merge.ff::\n+\tBy default, git does not create an extra merge commit when merging\n+\ta commit that is a descendant of the current commit. Instead, the\n+\ttip of the current branch is fast-forwarded. When set to `false`,\n+\tthis variable tells git to create an extra merge commit in such\n+\ta case (equivalent to giving the `--no-ff` option from the command\n+\tline). When set to `only`, only such fast-forward merges are\n+\tallowed (equivalent to giving the `--ff-only` option from the\n+\tcommand line).\n+\n merge.log::\n \tIn addition to branch names, populate the log message with at\n \tmost the specified number of one-line descriptions from the\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 4fa789a..1c3ff13 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -550,6 +550,15 @@ static int git_merge_config(const char *k, const char *v, void *cb)\n \t\tif (is_bool && shortlog_len)\n \t\t\tshortlog_len = DEFAULT_MERGE_LOG_LEN;\n \t\treturn 0;\n+\t} else if (!strcmp(k, \"merge.ff\")) {\n+\t\tint boolval = git_config_maybe_bool(k, v);\n+\t\tif (0 <= boolval) {\n+\t\t\tallow_fast_forward = boolval;\n+\t\t} else if (v && !strcmp(v, \"only\")) {\n+\t\t\tallow_fast_forward = 1;\n+\t\t\tfast_forward_only = 1;\n+\t\t} /* do not barf on values from future versions of git */\n+\t\treturn 0;\n \t} else if (!strcmp(k, \"merge.defaulttoupstream\")) {\n \t\tdefault_to_upstream = git_config_bool(k, v);\n \t\treturn 0;\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 5463f87..4f1d4eb 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -225,12 +225,28 @@ test_expect_success 'merge c1 with c2 and c3' '\n \n test_debug 'git log --graph --decorate --oneline --all'\n \n-test_expect_success 'failing merges with --ff-only' '\n+test_expect_success 'merges with --ff-only' '\n \tgit reset --hard c1 &&\n \ttest_tick &&\n \ttest_must_fail git merge --ff-only c2 &&\n \ttest_must_fail git merge --ff-only c3 &&\n-\ttest_must_fail git merge --ff-only c2 c3\n+\ttest_must_fail git merge --ff-only c2 c3 &&\n+\tgit reset --hard c0 &&\n+\tgit merge c3 &&\n+\tverify_head $c3\n+'\n+\n+test_expect_success 'merges with merge.ff=only' '\n+\tgit reset --hard c1 &&\n+\ttest_tick &&\n+\ttest_when_finished \"git config --unset merge.ff\" &&\n+\tgit config merge.ff only &&\n+\ttest_must_fail git merge c2 &&\n+\ttest_must_fail git merge c3 &&\n+\ttest_must_fail git merge c2 c3 &&\n+\tgit reset --hard c0 &&\n+\tgit merge c3 &&\n+\tverify_head $c3\n '\n \n test_expect_success 'merge c0 with c1 (no-commit)' '\n@@ -447,6 +463,29 @@ test_expect_success 'merge c0 with c1 (no-ff)' '\n \n test_debug 'git log --graph --decorate --oneline --all'\n \n+test_expect_success 'merge c0 with c1 (merge.ff=false)' '\n+\tgit reset --hard c0 &&\n+\tgit config merge.ff false &&\n+\ttest_tick &&\n+\tgit merge c1 &&\n+\tgit config --remove-section merge &&\n+\tverify_merge file result.1 &&\n+\tverify_parents $c0 $c1\n+'\n+test_debug 'git log --graph --decorate --oneline --all'\n+\n+test_expect_success 'combine branch.master.mergeoptions with merge.ff' '\n+\tgit reset --hard c0 &&\n+\tgit config branch.master.mergeoptions --ff\n+\tgit config merge.ff false\n+\ttest_tick &&\n+\tgit merge c1 &&\n+\tgit config --remove-section \"branch.master\" &&\n+\tgit config --remove-section \"merge\" &&\n+\tverify_merge file result.1 &&\n+\tverify_parents \"$c0\"\n+'\n+\n test_expect_success 'combining --squash and --no-ff is refused' '\n \ttest_must_fail git merge --squash --no-ff c1 &&\n \ttest_must_fail git merge --no-ff --squash c1\n-- \n1.7.5.1.268.gce5bd\n"},{"id":"167282","messageId":"20110506205441.GA20182@elie","threadId":"27241","inReplyTo":"7vsjsuc704.fsf@alter.siamese.dyndns.org","subject":"[PATCH 0/2] tests: make verify_merge check that the number of parents is right","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-06T20:54:41Z","receivedAt":"2011-05-06T20:54:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> \tSide note: I think verify_parents is buggy. It only makes sure\n> \tthat the earlier parents of HEAD match the commits given, and does\n> \tnot care if there actually are more parents.\n\nHow about this?\n\nJonathan Nieder (2):\n tests: eliminate unnecessary setup test assertions\n tests: teach verify_parents to check for extra parents\n\n t/t6010-merge-base.sh | 62 +++++++++++-----------\n t/t7600-merge.sh | 135 ++++++++++++++++++++++++-------------------------\n 2 files changed, 98 insertions(+), 99 deletions(-)\n"},{"id":"167283","messageId":"20110506205851.GB20182@elie","threadId":"27241","inReplyTo":"20110506205441.GA20182@elie","subject":"[PATCH 1/2] tests: eliminate unnecessary setup test assertions","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-06T20:58:52Z","receivedAt":"2011-05-06T20:58:52Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Most of git's tests write files and define shell functions and\nvariables that will last throughout a test script at the top of\nthe script, before all test assertions:\n\n\t. ./test-lib.sh\n\n\tVAR='some value'\n\texport VAR\n\n\t>empty\n\n\tfn () {\n\t\tdo something\n\t}\n\n\ttest_expect_success 'setup' '\n\t\t... nontrivial commands go here ...\n\t'\n\nTwo scripts use a different style with this kind of trivial code\nenclosed by a test assertion; fix them. The usual style is easier to\nread since there is less indentation to keep track of and no need to\nworry about nested quotes; and on the other hand, because the commands\nin question are trivial, it should not make the test suite any worse\nat catching future bugs in git.\n\nWhile at it, make some other small tweaks:\n\n - spell function definitions with a space before () for consistency\n with other scripts;\n\n - use the self-contained command \"git mktree \n---\nI should have done this long ago. Sorry for the eyesore.\n\n t/t6010-merge-base.sh | 62 +++++++++++-----------\n t/t7600-merge.sh | 134 ++++++++++++++++++++++++-------------------------\n 2 files changed, 97 insertions(+), 99 deletions(-)\n\ndiff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh\nindex 082032e..f80bba8 100755\n--- a/t/t6010-merge-base.sh\n+++ b/t/t6010-merge-base.sh\n@@ -8,38 +8,38 @@ test_description='Merge base and parent list computation.\n \n . ./test-lib.sh\n \n+M=1130000000\n+Z=+0000\n+\n+GIT_COMMITTER_EMAIL=git@comm.iter.xz\n+GIT_COMMITTER_NAME='C O Mmiter'\n+GIT_AUTHOR_NAME='A U Thor'\n+GIT_AUTHOR_EMAIL=git@au.thor.xz\n+export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL\n+\n+doit () {\n+\tOFFSET=$1 &&\n+\tNAME=$2 &&\n+\tshift 2 &&\n+\n+\tPARENTS= &&\n+\tfor P\n+\tdo\n+\t\tPARENTS=\"${PARENTS}-p $P \"\n+\tdone &&\n+\n+\tGIT_COMMITTER_DATE=\"$(($M + $OFFSET)) $Z\" &&\n+\tGIT_AUTHOR_DATE=$GIT_COMMITTER_DATE &&\n+\texport GIT_COMMITTER_DATE GIT_AUTHOR_DATE &&\n+\n+\tcommit=$(echo $NAME | git commit-tree $T $PARENTS) &&\n+\n+\techo $commit >.git/refs/tags/$NAME &&\n+\techo $commit\n+}\n+\n test_expect_success 'setup' '\n-\tT=$(git write-tree) &&\n-\n-\tM=1130000000 &&\n-\tZ=+0000 &&\n-\n-\tGIT_COMMITTER_EMAIL=git@comm.iter.xz &&\n-\tGIT_COMMITTER_NAME=\"C O Mmiter\" &&\n-\tGIT_AUTHOR_NAME=\"A U Thor\" &&\n-\tGIT_AUTHOR_EMAIL=git@au.thor.xz &&\n-\texport GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&\n-\n-\tdoit() {\n-\t\tOFFSET=$1 &&\n-\t\tNAME=$2 &&\n-\t\tshift 2 &&\n-\n-\t\tPARENTS= &&\n-\t\tfor P\n-\t\tdo\n-\t\t\tPARENTS=\"${PARENTS}-p $P \"\n-\t\tdone &&\n-\n-\t\tGIT_COMMITTER_DATE=\"$(($M + $OFFSET)) $Z\" &&\n-\t\tGIT_AUTHOR_DATE=$GIT_COMMITTER_DATE &&\n-\t\texport GIT_COMMITTER_DATE GIT_AUTHOR_DATE &&\n-\n-\t\tcommit=$(echo $NAME | git commit-tree $T $PARENTS) &&\n-\n-\t\techo $commit >.git/refs/tags/$NAME &&\n-\t\techo $commit\n-\t}\n+\tT=$(git mktree file &&\n-\tprintf \"%s\\n\" \"1 X\" 2 3 4 5 6 7 8 9 >file.1 &&\n-\tprintf \"%s\\n\" 1 2 3 4 \"5 X\" 6 7 8 9 >file.5 &&\n-\tprintf \"%s\\n\" 1 2 3 4 5 6 7 8 \"9 X\" >file.9 &&\n-\tprintf \"%s\\n\" \"1 X\" 2 3 4 5 6 7 8 9 >result.1 &&\n-\tprintf \"%s\\n\" \"1 X\" 2 3 4 \"5 X\" 6 7 8 9 >result.1-5 &&\n-\tprintf \"%s\\n\" \"1 X\" 2 3 4 \"5 X\" 6 7 8 \"9 X\" >result.1-5-9 &&\n+printf '%s\\n' 1 2 3 4 5 6 7 8 9 >file\n+printf '%s\\n' '1 X' 2 3 4 5 6 7 8 9 >file.1\n+printf '%s\\n' 1 2 3 4 '5 X' 6 7 8 9 >file.5\n+printf '%s\\n' 1 2 3 4 5 6 7 8 '9 X' >file.9\n+printf '%s\\n' '1 X' 2 3 4 5 6 7 8 9 >result.1\n+printf '%s\\n' '1 X' 2 3 4 '5 X' 6 7 8 9 >result.1-5\n+printf '%s\\n' '1 X' 2 3 4 '5 X' 6 7 8 '9 X' >result.1-5-9\n \n-\tcreate_merge_msgs() {\n-\t\techo \"Merge commit '\\''c2'\\''\" >msg.1-5 &&\n-\t\techo \"Merge commit '\\''c2'\\''; commit '\\''c3'\\''\" >msg.1-5-9 &&\n-\t\t{\n-\t\t\techo \"Squashed commit of the following:\" &&\n-\t\t\techo &&\n-\t\t\tgit log --no-merges ^HEAD c1\n-\t\t} >squash.1 &&\n-\t\t{\n-\t\t\techo \"Squashed commit of the following:\" &&\n-\t\t\techo &&\n-\t\t\tgit log --no-merges ^HEAD c2\n-\t\t} >squash.1-5 &&\n-\t\t{\n-\t\t\techo \"Squashed commit of the following:\" &&\n-\t\t\techo &&\n-\t\t\tgit log --no-merges ^HEAD c2 c3\n-\t\t} >squash.1-5-9 &&\n-\t\techo >msg.nolog &&\n-\t\t{\n-\t\t\techo \"* commit '\\''c3'\\'':\" &&\n-\t\t\techo \" commit 3\" &&\n-\t\t\techo\n-\t\t} >msg.log\n-\t} &&\n+create_merge_msgs () {\n+\techo \"Merge commit 'c2'\" >msg.1-5 &&\n+\techo \"Merge commit 'c2'; commit 'c3'\" >msg.1-5-9 &&\n+\t{\n+\t\techo \"Squashed commit of the following:\" &&\n+\t\techo &&\n+\t\tgit log --no-merges ^HEAD c1\n+\t} >squash.1 &&\n+\t{\n+\t\techo \"Squashed commit of the following:\" &&\n+\t\techo &&\n+\t\tgit log --no-merges ^HEAD c2\n+\t} >squash.1-5 &&\n+\t{\n+\t\techo \"Squashed commit of the following:\" &&\n+\t\techo &&\n+\t\tgit log --no-merges ^HEAD c2 c3\n+\t} >squash.1-5-9 &&\n+\techo >msg.nolog &&\n+\t{\n+\t\techo \"* commit 'c3':\" &&\n+\t\techo \" commit 3\" &&\n+\t\techo\n+\t} >msg.log\n+}\n \n-\tverify_merge() {\n-\t\ttest_cmp \"$2\" \"$1\" &&\n-\t\tgit update-index --refresh &&\n-\t\tgit diff --exit-code &&\n-\t\tif test -n \"$3\"\n-\t\tthen\n-\t\t\tgit show -s --pretty=format:%s HEAD >msg.act &&\n-\t\t\ttest_cmp \"$3\" msg.act\n-\t\tfi\n-\t} &&\n+verify_merge () {\n+\ttest_cmp \"$2\" \"$1\" &&\n+\tgit update-index --refresh &&\n+\tgit diff --exit-code &&\n+\tif test -n \"$3\"\n+\tthen\n+\t\tgit show -s --pretty=format:%s HEAD >msg.act &&\n+\t\ttest_cmp \"$3\" msg.act\n+\tfi\n+}\n \n-\tverify_head() {\n-\t\techo \"$1\" >head.expected &&\n-\t\tgit rev-parse HEAD >head.actual &&\n-\t\ttest_cmp head.expected head.actual\n-\t} &&\n+verify_head () {\n+\techo \"$1\" >head.expected &&\n+\tgit rev-parse HEAD >head.actual &&\n+\ttest_cmp head.expected head.actual\n+}\n \n-\tverify_parents() {\n-\t\tprintf \"%s\\n\" \"$@\" >parents.expected &&\n-\t\t>parents.actual &&\n-\t\ti=1 &&\n-\t\twhile test $i -le $#\n-\t\tdo\n-\t\t\tgit rev-parse HEAD^$i >>parents.actual &&\n-\t\t\ti=$(expr $i + 1) ||\n-\t\t\treturn 1\n-\t\tdone &&\n-\t\ttest_cmp parents.expected parents.actual\n-\t} &&\n+verify_parents () {\n+\tprintf '%s\\n' \"$@\" >parents.expected &&\n+\t>parents.actual &&\n+\ti=1 &&\n+\twhile test $i -le $#\n+\tdo\n+\t\tgit rev-parse HEAD^$i >>parents.actual &&\n+\t\ti=$(expr $i + 1) ||\n+\t\treturn 1\n+\tdone &&\n+\ttest_cmp parents.expected parents.actual\n+}\n \n-\tverify_mergeheads() {\n-\t\tprintf \"%s\\n\" \"$@\" >mergehead.expected &&\n-\t\ttest_cmp mergehead.expected .git/MERGE_HEAD\n-\t} &&\n+verify_mergeheads () {\n+\tprintf '%s\\n' \"$@\" >mergehead.expected &&\n+\ttest_cmp mergehead.expected .git/MERGE_HEAD\n+}\n \n-\tverify_no_mergehead() {\n-\t\t! test -e .git/MERGE_HEAD\n-\t}\n-'\n+verify_no_mergehead () {\n+\t! test -e .git/MERGE_HEAD\n+}\n \n test_expect_success 'setup' '\n \tgit add file &&\n-- \n1.7.5.1\n"},{"id":"167284","messageId":"20110506210021.GC20182@elie","threadId":"27241","inReplyTo":"20110506205441.GA20182@elie","subject":"[PATCH 2/2] tests: teach verify_parents to check for extra parents","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-06T21:00:21Z","receivedAt":"2011-05-06T21:00:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Currently verify_parents only makes sure that the earlier parents of\nHEAD match the commits given, and does not care if there are more\nparents. This makes it harder than one would like to check that, for\nexample, parent reduction works correctly when making an octopus.\n\nFix it by checking that HEAD^(n+1) is not a valid commit name.\nNoticed while working on a new test that was supposed to create a\nfast-forward one commit ahead but actually created a merge.\n\nReported-by: Junio C Hamano \nSigned-off-by: Jonathan Nieder \n---\n t/t7600-merge.sh | 1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex c665acd..9af748a 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -89,6 +89,7 @@ verify_parents () {\n \t\ti=$(expr $i + 1) ||\n \t\treturn 1\n \tdone &&\n+\ttest_must_fail git rev-parse --verify HEAD^$(($# + 1)) &&\n \ttest_cmp parents.expected parents.actual\n }\n \n-- \n1.7.5.1\n"},{"id":"167285","messageId":"20110506213257.GD20182@elie","threadId":"27241","inReplyTo":"7vsjsuc704.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v5] Add default merge options for all branches","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-06T21:32:57Z","receivedAt":"2011-05-06T21:32:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> Subject: [PATCH] merge: fix branch..mergeoptions\n\nTo be more precise, if I understand correctly:\n\n\tmerge: allow branch..mergeoptions to override merge.*\n\n> This patch should fix it, even though I now strongly suspect that\n> branch..mergeoptions that gives a single command line that\n> needs to be parsed was likely to be an ill-conceived idea to begin\n> with. Sigh...\n\nYes, and introducing branch..ff and branch..log might\nstill be a good idea.\n\nThe patch looks mostly good. I see only one actual problem, marked\nwith [*] below.\n\n> --- a/builtin-merge.c\n> +++ b/builtin-merge.c\n> @@ -54,6 +54,7 @@ static size_t use_strategies_nr, use_strategies_alloc;\n> static const char **xopts;\n> static size_t xopts_nr, xopts_alloc;\n> static const char *branch;\n> +static char *branch_mergeoptions;\n> static int verbosity;\n> static int allow_rerere_auto;\n> \n> @@ -474,25 +475,33 @@ cleanup:\n> \tstrbuf_release(&bname);\n> }\n> \n> +static void parse_branch_merge_options(char *bmo)\n> +{\n> +\tconst char **argv;\n> +\tint argc;\n> +\tchar *buf;\n> +\n> +\tif (!bmo)\n> +\t\treturn;\n> +\targc = split_cmdline(bmo, &argv);\n> +\tif (argc < 0)\n> +\t\tdie(\"Bad branch.%s.mergeoptions string\", branch);\n> +\targv = xrealloc(argv, sizeof(*argv) * (argc + 2));\n> +\tmemmove(argv + 1, argv, sizeof(*argv) * (argc + 1));\n\nThis is not new code, but it might make sense to do\n\n\targv[0] = \"merge.*.options\";\n\nfor a saner error message when someone tries\n\n\t[branch \"master\"]\n\t\tmergeoptions = --nonsense\n\n> +\targc++;\n> +\tparse_options(argc, argv, NULL, builtin_merge_options,\n> +\t\t builtin_merge_usage, 0);\n> +\tfree(buf);\n\n[*]\nThis buf seems to be left over. (I don't think the intent is to\ncall free on an uninitialized pointer. ;-))\n\n[...]\n> -\t\tfree(buf);\n> +\t\tfree(branch_mergeoptions);\n> +\t\tbranch_mergeoptions = xstrdup(v);\n\nIt is tempting to do\n\n\tsize_t len;\n\n\tlen = strlen(v);\n\tbranch_mergeoptions = xrealloc(branch_mergeoptions, len + 1);\n\tmemcpy(branch_mergeoptions, v, len + 1);\n\nbut free + xstrdup is simpler and clearer. Makes sense.\n\n> +test_expect_success 'merge c1 with c2 (log in config gets overridden)' '\n> +\t(\n> +\t\tgit config --remove-section branch.master\n> +\t\tgit config --remove-section merge\n> +\t)\n\nSince this patch is meant to apply to a very old git, we cannot use\ntest_might_fail. Makes sense: it can be fixed up later to use\n&&-friendly syntax as part of a series introducing checks to make\nsure we don't regress in that.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"167286","messageId":"7vzkmzy0mk.fsf@alter.siamese.dyndns.org","threadId":"27241","inReplyTo":"20110506210021.GC20182@elie","subject":"Re: [PATCH 2/2] tests: teach verify_parents to check for extra parents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-06T21:34:11Z","receivedAt":"2011-05-06T21:34:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Currently verify_parents only makes sure that the earlier parents of\n> HEAD match the commits given, and does not care if there are more\n> parents. This makes it harder than one would like to check that, for\n> example, parent reduction works correctly when making an octopus.\n>\n> Fix it by checking that HEAD^(n+1) is not a valid commit name.\n> Noticed while working on a new test that was supposed to create a\n> fast-forward one commit ahead but actually created a merge.\n>\n> Reported-by: Junio C Hamano \n> Signed-off-by: Jonathan Nieder \n> ---\n> t/t7600-merge.sh | 1 +\n> 1 files changed, 1 insertions(+), 0 deletions(-)\n>\n> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\n> index c665acd..9af748a 100755\n> --- a/t/t7600-merge.sh\n> +++ b/t/t7600-merge.sh\n> @@ -89,6 +89,7 @@ verify_parents () {\n> \t\ti=$(expr $i + 1) ||\n> \t\treturn 1\n> \tdone &&\n> +\ttest_must_fail git rev-parse --verify HEAD^$(($# + 1)) &&\n\nIsn't $i at this point the same as that complex $(()) line noise?\n\n> \ttest_cmp parents.expected parents.actual\n> }\n"},{"id":"167289","messageId":"20110506214238.GE20182@elie","threadId":"27241","inReplyTo":"7vzkmzy0mk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] tests: teach verify_parents to check for extra parents","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-06T21:42:38Z","receivedAt":"2011-05-06T21:42:38Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> --- a/t/t7600-merge.sh\n>> +++ b/t/t7600-merge.sh\n>> @@ -89,6 +89,7 @@ verify_parents () {\n>> \t\ti=$(expr $i + 1) ||\n>> \t\treturn 1\n>> \tdone &&\n>> +\ttest_must_fail git rev-parse --verify HEAD^$(($# + 1)) &&\n>\n> Isn't $i at this point the same as that complex $(()) line noise?\n\nYes, that sounds better.\n"},{"id":"167290","messageId":"20110506214801.GA17848@sigill.intra.peff.net","threadId":"27241","inReplyTo":"20110506205851.GB20182@elie","subject":"Re: [PATCH 1/2] tests: eliminate unnecessary setup test assertions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-06T21:48:01Z","receivedAt":"2011-05-06T21:48:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 06, 2011 at 03:58:52PM -0500, Jonathan Nieder wrote:\n\n> Two scripts use a different style with this kind of trivial code\n> enclosed by a test assertion; fix them. The usual style is easier to\n> read since there is less indentation to keep track of and no need to\n> worry about nested quotes; and on the other hand, because the commands\n> in question are trivial, it should not make the test suite any worse\n> at catching future bugs in git.\n\nThanks. Glancing at the first few hunks, this change didn't seem like a\nbig cleanup, but then I got to the hunk with all the ugly '\\'' bits. :)\n\nThe patch looks correct to me (reviewing with \"git diff -b\" was a big\nhelp).\n\n> While at it, make some other small tweaks:\n> \n> - spell function definitions with a space before () for consistency\n> with other scripts;\n\nHmm.\n\n $ cd t\n $ git grep ' ()' *.sh | wc -l\n 271\n $ git grep '[^ ]()' *.sh | wc -l\n 247\n\nI'm not sure you are making things any more consistent. But I don't\nreally care either way.\n\nJust for giggles, I was curious who introduced the styles:\n\n pattern_authors() {\n git grep -n \"$@\" |\n while IFS=: read file line match; do\n git blame -L \"$line,$line\" \"$file\"\n done |\n perl -lpe '/\\((.*?) \\d+-\\d+-\\d+/; $_=$1'\n }\n\n $ pattern_authors ' ()' t/*.sh | sort | uniq -c | sort -rn\n 84 Junio C Hamano\n 32 Jonathan Nieder\n 16 Thomas Rast\n 16 Johan Herland\n 12 Johannes Sixt\n 12 Johannes Schindelin\n ...\n\n $ pattern_authors '[^ ]()' t/*.sh | sort | uniq -c | sort -rn\n 52 Jeff King\n 26 Jonathan Nieder\n 18 Nguyễn Thái Ngọc Duy\n 16 Wincent Colaiuta\n 14 Jon Seymour\n 10 Tay Ray Chuan\n ...\n\nSo there are definitely particular people who prefer different styles\n(and I recalled that Junio and I differed on this style point, which is\nconfirmed here). Interestingly, you are the only person to fall right in\nthe middle. I guess that means you are good at emulating surrounding\ncode. :)\n\n-Peff\n"},{"id":"167291","messageId":"20110506215946.GF20182@elie","threadId":"27241","inReplyTo":"7v7ha3zhv0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v5] Add default merge options for all branches","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-06T21:59:47Z","receivedAt":"2011-05-06T21:59:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> --- a/Documentation/merge-config.txt\n> +++ b/Documentation/merge-config.txt\n> @@ -16,6 +16,16 @@ merge.defaultToUpstream::\n> \tto their corresponding remote tracking branches, and the tips of\n> \tthese tracking branches are merged.\n> \n> +merge.ff::\n[...]\n> +\tline). When set to `only`, only such fast-forward merges are\n> +\tallowed (equivalent to giving the `--ff-only` option from the\n> +\tcommand line).\n\nA habitual \"git pull\" user that uses git in a read-only fashion to get\nthe latest development snapshot of a project's code would probably\nfind this handy.\n\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -550,6 +550,15 @@ static int git_merge_config(const char *k, const char *v, void *cb)\n> \t\tif (is_bool && shortlog_len)\n> \t\t\tshortlog_len = DEFAULT_MERGE_LOG_LEN;\n> \t\treturn 0;\n> +\t} else if (!strcmp(k, \"merge.ff\")) {\n> +\t\tint boolval = git_config_maybe_bool(k, v);\n> +\t\tif (0 <= boolval) {\n> +\t\t\tallow_fast_forward = boolval;\n> +\t\t} else if (v && !strcmp(v, \"only\")) {\n> +\t\t\tallow_fast_forward = 1;\n> +\t\t\tfast_forward_only = 1;\n> +\t\t} /* do not barf on values from future versions of git */\n\nPerhaps deserves a test, like so (meant for squashing)?\n\n-- >8 --\nSubject: tests: check git does not barf on merge.ff values for future versions of git\n\nMaybe some day in the future we will want to support a syntax\nlike\n\n\t[merge]\n\t\tff = branch1\n\t\tff = branch2\n\t\tff = branch3\n\nin addition to the currently permitted \"true\", \"false\", and \"only\"\nvalues. So make sure we continue to treat such configurations as\nthough an unknown variable had been defined rather than erroring out,\nuntil it is time to implement such a thing, so configuration files\nusing such a facility can be shared between present and future git.\n\nWhile at it, add a few missing && and start the \"combining --squash\nand --no-ff\" test with a known state so we can be sure it does not\nsucceed or fail for the wrong reason.\n\nSigned-off-by: Jonathan Nieder \n---\n t/t7600-merge.sh | 16 ++++++++++++++--\n 1 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 89e0b77..1e20f2e 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -35,6 +35,7 @@ printf '%s\\n' 1 2 3 4 5 6 7 8 '9 X' >file.9\n printf '%s\\n' '1 X' 2 3 4 5 6 7 8 9 >result.1\n printf '%s\\n' '1 X' 2 3 4 '5 X' 6 7 8 9 >result.1-5\n printf '%s\\n' '1 X' 2 3 4 '5 X' 6 7 8 '9 X' >result.1-5-9\n+>empty\n \n create_merge_msgs () {\n \techo \"Merge commit 'c2'\" >msg.1-5 &&\n@@ -475,8 +476,8 @@ test_debug 'git log --graph --decorate --oneline --all'\n \n test_expect_success 'combine branch.master.mergeoptions with merge.ff' '\n \tgit reset --hard c0 &&\n-\tgit config branch.master.mergeoptions --ff\n-\tgit config merge.ff false\n+\tgit config branch.master.mergeoptions --ff &&\n+\tgit config merge.ff false &&\n \ttest_tick &&\n \tgit merge c1 &&\n \tgit config --remove-section \"branch.master\" &&\n@@ -485,7 +486,18 @@ test_expect_success 'combine branch.master.mergeoptions with merge.ff' '\n \tverify_parents \"$c0\"\n '\n \n+test_expect_success 'tolerate unknown values for merge.ff' '\n+\tgit reset --hard c0 &&\n+\tgit config merge.ff something-new &&\n+\ttest_tick &&\n+\tgit merge c1 2>message &&\n+\tgit config --remove-section \"merge\" &&\n+\tverify_head \"$c1\" &&\n+\ttest_cmp empty message\n+'\n+\n test_expect_success 'combining --squash and --no-ff is refused' '\n+\tgit reset --hard c0 &&\n \ttest_must_fail git merge --squash --no-ff c1 &&\n \ttest_must_fail git merge --no-ff --squash c1\n '\n-- \n1.7.5.1\n"},{"id":"167292","messageId":"7vsjsrxzdp.fsf@alter.siamese.dyndns.org","threadId":"27241","inReplyTo":"20110506213257.GD20182@elie","subject":"Re: [PATCH v5] Add default merge options for all branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-06T22:01:06Z","receivedAt":"2011-05-06T22:01:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n>> +static void parse_branch_merge_options(char *bmo)\n>> +{\n>> +\tconst char **argv;\n>> +\tint argc;\n>> +\tchar *buf;\n>> +\n>> +\tif (!bmo)\n>> +\t\treturn;\n>> +\targc = split_cmdline(bmo, &argv);\n>> +\tif (argc < 0)\n>> +\t\tdie(\"Bad branch.%s.mergeoptions string\", branch);\n>> +\targv = xrealloc(argv, sizeof(*argv) * (argc + 2));\n>> +\tmemmove(argv + 1, argv, sizeof(*argv) * (argc + 1));\n>\n> This is not new code, but it might make sense to do\n>\n> \targv[0] = \"merge.*.options\";\n>\n> for a saner error message when someone tries\n>\n> \t[branch \"master\"]\n> \t\tmergeoptions = --nonsense\n\nYes, either we stuff a fixed string \"branch.*.mergeoptions\" to argv[0], or\nuse another static to recall which variable gave us that value and use\nit. The former is of course easier and less nicer.\n\n>> +\targc++;\n>> +\tparse_options(argc, argv, NULL, builtin_merge_options,\n>> +\t\t builtin_merge_usage, 0);\n>> +\tfree(buf);\n>\n> [*]\n> This buf seems to be left over. (I don't think the intent is to\n> call free on an uninitialized pointer. ;-))\n\nI also forgot to free argv[].\n"},{"id":"167294","messageId":"20110506221300.GB17848@sigill.intra.peff.net","threadId":"27241","inReplyTo":"20110506214801.GA17848@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] tests: eliminate unnecessary setup test assertions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-06T22:13:00Z","receivedAt":"2011-05-06T22:13:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 06, 2011 at 05:48:01PM -0400, Jeff King wrote:\n\n> pattern_authors() {\n> git grep -n \"$@\" |\n> while IFS=: read file line match; do\n> git blame -L \"$line,$line\" \"$file\"\n> done |\n> perl -lpe '/\\((.*?) \\d+-\\d+-\\d+/; $_=$1'\n> }\n\nTwo minor complaints on git-blame; maybe somebody can point out\nsomething clever I've missed.\n\n 1. blame's \"-L\" understands patterns already. So in theory I could\n tell it \"blame all lines that match pattern X\". But I don't think\n there is a way to do that (it tries looking to for _one_ range to\n blame for each -L, not a set of ranges).\n\n 2. Parsing the human-readable output blame output sucks. But parsing\n --porcelain is annoyingly complex for quick-and-dirty things like\n this. It doesn't repeat the commit information per-line.\n\n I guess we could have an inefficient --line-porcelain format that\n breaks down ranges into single lines and repeats the commit info\n for each one.\n\n The clever among you may notice that in this particular case,\n though, I could have gotten away with regular --porcelain as I\n blame a single line at a time.\n\n-Peff\n"},{"id":"167295","messageId":"20110506222629.GA27945@elie","threadId":"27241","inReplyTo":"20110506214801.GA17848@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] tests: eliminate unnecessary setup test assertions","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-06T22:26:29Z","receivedAt":"2011-05-06T22:26:29Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> So there are definitely particular people who prefer different styles\n> (and I recalled that Junio and I differed on this style point, which is\n> confirmed here). Interestingly, you are the only person to fall right in\n> the middle. I guess that means you are good at emulating surrounding\n> code. :)\n\nProbably my older code leaves out the space more often, and newer code\nincludes it. There is an odd kind of logic that can be used to\njustify including or not including the space, namely:\n\nIn C, a function definition starts with an expression that looks\nsomething like a function call, as in \"double sin(double x);\". So\nwhen you want to know everything there is to know about the sine\nfunction, you can do a \"git grep -F -e 'sin('\", and it will return to\nyou its definition and a list of callers.\n\nThe shell function definition syntax looks oddly like an old-style C\nprototype \"f()\". But do not be misled: to duplicate the above\nproperty familiar from C, one needs to include a space before the\nparentheses, so \"git grep -F -e 'f '\" will return its definition and a\nlist of callers.\n\nOf course the same argument works backwards: if you want the\ndefinition without the callers, then only the spaceless syntax will\nallow you to grep for 'f()'.\n\nUnlike brace placement, this seems to be a question of style with no\nright answer. :)\n"},{"id":"167296","messageId":"7voc3fxy6b.fsf@alter.siamese.dyndns.org","threadId":"27241","inReplyTo":"20110506221300.GB17848@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] tests: eliminate unnecessary setup test assertions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-06T22:27:08Z","receivedAt":"2011-05-06T22:27:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Two minor complaints on git-blame; maybe somebody can point out\n> something clever I've missed.\n\n> 1. blame's \"-L\" understands patterns already.\n\nTeaching blame to take multiple -L options has been one of many\nlongstanding todo item for me. Someday.\n\n> 2. Parsing the human-readable output blame output sucks. But parsing\n> --porcelain is annoyingly complex for quick-and-dirty things like\n> this. It doesn't repeat the commit information per-line.\n\nNon-repetition was quite deliberate, as the reader was expected to have\nmemory proportional to the number of lines in the range, but I agree it is\nnot friendly for quick and dirty hack.\n\nYou should be able to add a command line option that disables the early\nreturn at the beginning of emit_one_suspect_detail() with a 5-6 lines of\npatch.\n"},{"id":"167297","messageId":"20110506222951.GA24474@sigill.intra.peff.net","threadId":"27241","inReplyTo":"7voc3fxy6b.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] tests: eliminate unnecessary setup test assertions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-06T22:29:51Z","receivedAt":"2011-05-06T22:29:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 06, 2011 at 03:27:08PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Two minor complaints on git-blame; maybe somebody can point out\n> > something clever I've missed.\n> \n> > 1. blame's \"-L\" understands patterns already.\n> \n> Teaching blame to take multiple -L options has been one of many\n> longstanding todo item for me. Someday.\n\nI think multiple -L is not quite enough. I want a single \"-L\" that\nmatches every instance of a pattern, like:\n\n -L \"/ ()/,+0\"\n\n> > 2. Parsing the human-readable output blame output sucks. But parsing\n> > --porcelain is annoyingly complex for quick-and-dirty things like\n> > this. It doesn't repeat the commit information per-line.\n> \n> Non-repetition was quite deliberate, as the reader was expected to have\n> memory proportional to the number of lines in the range, but I agree it is\n> not friendly for quick and dirty hack.\n> \n> You should be able to add a command line option that disables the early\n> return at the beginning of emit_one_suspect_detail() with a 5-6 lines of\n> patch.\n\nI tried that, and it is slightly more involved. You also need to break a\nmulti-line run of lines that blame to a single suspect into its\nconstituent lines. I am 75% of the way to such a patch if you are\ninterested. It's not a lot of code, but it takes some refactoring of\nemit_porcelain.\n\n-Peff\n"},{"id":"167338","messageId":"7voc3eupy3.fsf@alter.siamese.dyndns.org","threadId":"27241","inReplyTo":"20110506222951.GA24474@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] tests: eliminate unnecessary setup test assertions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-07T22:05:24Z","receivedAt":"2011-05-07T22:05:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think multiple -L is not quite enough. I want a single \"-L\" that\n> matches every instance of a pattern, like:\n>\n> -L \"/ ()/,+0\"\n\nOf course I am aware that needs to be a part of the multiple -L topic;\nafter all I was the one who invented -L \"$regexp\" support ;-)\n"},{"id":"167495","messageId":"20110509133153.GA10998@sigill.intra.peff.net","threadId":"27241","inReplyTo":"20110506222951.GA24474@sigill.intra.peff.net","subject":"[PATCH 0/3] blame --line-porcelain","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-09T13:31:54Z","receivedAt":"2011-05-09T13:31:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 06, 2011 at 06:29:51PM -0400, Jeff King wrote:\n\n> > Non-repetition was quite deliberate, as the reader was expected to have\n> > memory proportional to the number of lines in the range, but I agree it is\n> > not friendly for quick and dirty hack.\n> > \n> > You should be able to add a command line option that disables the early\n> > return at the beginning of emit_one_suspect_detail() with a 5-6 lines of\n> > patch.\n> \n> I tried that, and it is slightly more involved. You also need to break a\n> multi-line run of lines that blame to a single suspect into its\n> constituent lines. I am 75% of the way to such a patch if you are\n> interested. It's not a lot of code, but it takes some refactoring of\n> emit_porcelain.\n\nIt turned out to not be too bad. Here's the series.\n\n [1/3]: add tests for various blame formats\n [2/3]: blame: refactor porcelain output\n [3/3]: blame: add --line-porcelain output format\n\n-Peff\n"},{"id":"167496","messageId":"20110509133330.GA11022@sigill.intra.peff.net","threadId":"27241","inReplyTo":"20110509133153.GA10998@sigill.intra.peff.net","subject":"[PATCH 1/3] add tests for various blame formats","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-09T13:33:30Z","receivedAt":"2011-05-09T13:33:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We don't seem to have any tests for \"blame --porcelain\".\nLet's at least do a trivial test on a simple example.\n\nSigned-off-by: Jeff King \n---\nI always feel funny putting human-readable output in a test. Maybe it is\nnot worth including the \"normal\" output test below, but I assume it's\ngoing to remain pretty stable.\n\n t/t8008-blame-formats.sh | 71 ++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 71 insertions(+), 0 deletions(-)\n create mode 100755 t/t8008-blame-formats.sh\n\ndiff --git a/t/t8008-blame-formats.sh b/t/t8008-blame-formats.sh\nnew file mode 100755\nindex 0000000..387d1a6\n--- /dev/null\n+++ b/t/t8008-blame-formats.sh\n@@ -0,0 +1,71 @@\n+#!/bin/sh\n+\n+test_description='blame output in various formats on a simple case'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo a >file &&\n+\tgit add file\n+\ttest_tick &&\n+\tgit commit -m one &&\n+\techo b >>file &&\n+\techo c >>file &&\n+\techo d >>file &&\n+\ttest_tick &&\n+\tgit commit -a -m two\n+'\n+\n+cat >expect <<'EOF'\n+^baf5e0b (A U Thor 2005-04-07 15:13:13 -0700 1) a\n+8825379d (A U Thor 2005-04-07 15:14:13 -0700 2) b\n+8825379d (A U Thor 2005-04-07 15:14:13 -0700 3) c\n+8825379d (A U Thor 2005-04-07 15:14:13 -0700 4) d\n+EOF\n+test_expect_success 'normal blame output' '\n+\tgit blame file >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+ID1=baf5e0b3869e0b2b2beb395a3720c7b51eac94fc\n+COMMIT1='author A U Thor\n+author-mail \n+author-time 1112911993\n+author-tz -0700\n+committer C O Mitter\n+committer-mail \n+committer-time 1112911993\n+committer-tz -0700\n+summary one\n+boundary\n+filename file'\n+ID2=8825379dfb8a1267b58e8e5bcf69eec838f685ec\n+COMMIT2='author A U Thor\n+author-mail \n+author-time 1112912053\n+author-tz -0700\n+committer C O Mitter\n+committer-mail \n+committer-time 1112912053\n+committer-tz -0700\n+summary two\n+previous baf5e0b3869e0b2b2beb395a3720c7b51eac94fc file\n+filename file'\n+\n+cat >expect <actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n1.7.5.rc2.8.gc085\n"},{"id":"167497","messageId":"20110509133402.GB11022@sigill.intra.peff.net","threadId":"27241","inReplyTo":"20110509133153.GA10998@sigill.intra.peff.net","subject":"[PATCH 2/3] blame: refactor porcelain output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-09T13:34:02Z","receivedAt":"2011-05-09T13:34:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is in preparation for adding more porcelain output\noptions. The three changes are:\n\n 1. emit_porcelain now receives the format option flags\n\n 2. emit_one_suspect_detail takes an optional \"repeat\"\n parameter to suppress the \"show only once\" behavior\n\n 3. The code for emitting porcelain suspect is factored\n into its own function for repeatability.\n\nThere should be no functional changes.\n\nSigned-off-by: Jeff King \n---\nI broke this out for readability. I can break each of the 3 out into a\nseparate patch if that helps, but it seemed excessive.\n\n builtin/blame.c | 25 ++++++++++++++++---------\n 1 files changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 4242e4b..d74e18f 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1484,13 +1484,14 @@ static void write_filename_info(const char *path)\n /*\n * Porcelain/Incremental format wants to show a lot of details per\n * commit. Instead of repeating this every line, emit it only once,\n- * the first time each commit appears in the output.\n+ * the first time each commit appears in the output (unless the\n+ * user has specifically asked for us to repeat).\n */\n-static int emit_one_suspect_detail(struct origin *suspect)\n+static int emit_one_suspect_detail(struct origin *suspect, int repeat)\n {\n \tstruct commit_info ci;\n \n-\tif (suspect->commit->object.flags & METAINFO_SHOWN)\n+\tif (!repeat && suspect->commit->object.flags & METAINFO_SHOWN)\n \t\treturn 0;\n \n \tsuspect->commit->object.flags |= METAINFO_SHOWN;\n@@ -1529,7 +1530,7 @@ static void found_guilty_entry(struct blame_entry *ent)\n \t\tprintf(\"%s %d %d %d\\n\",\n \t\t sha1_to_hex(suspect->commit->object.sha1),\n \t\t ent->s_lno + 1, ent->lno + 1, ent->num_lines);\n-\t\temit_one_suspect_detail(suspect);\n+\t\temit_one_suspect_detail(suspect, 0);\n \t\twrite_filename_info(suspect->path);\n \t\tmaybe_flush_or_die(stdout, \"stdout\");\n \t}\n@@ -1619,7 +1620,15 @@ static const char *format_time(unsigned long time, const char *tz_str,\n #define OUTPUT_NO_AUTHOR 0200\n #define OUTPUT_SHOW_EMAIL\t0400\n \n-static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent)\n+static void emit_porcelain_details(struct origin *suspect, int repeat)\n+{\n+\tif (emit_one_suspect_detail(suspect, repeat) ||\n+\t (suspect->commit->object.flags & MORE_THAN_ONE_PATH))\n+\t\twrite_filename_info(suspect->path);\n+}\n+\n+static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent,\n+\t\t\t int opt)\n {\n \tint cnt;\n \tconst char *cp;\n@@ -1633,9 +1642,7 @@ static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent)\n \t ent->s_lno + 1,\n \t ent->lno + 1,\n \t ent->num_lines);\n-\tif (emit_one_suspect_detail(suspect) ||\n-\t (suspect->commit->object.flags & MORE_THAN_ONE_PATH))\n-\t\twrite_filename_info(suspect->path);\n+\temit_porcelain_details(suspect, 0);\n \n \tcp = nth_line(sb, ent->lno);\n \tfor (cnt = 0; cnt < ent->num_lines; cnt++) {\n@@ -1756,7 +1763,7 @@ static void output(struct scoreboard *sb, int option)\n \n \tfor (ent = sb->ent; ent; ent = ent->next) {\n \t\tif (option & OUTPUT_PORCELAIN)\n-\t\t\temit_porcelain(sb, ent);\n+\t\t\temit_porcelain(sb, ent, option);\n \t\telse {\n \t\t\temit_other(sb, ent, option);\n \t\t}\n-- \n1.7.5.rc2.8.gc085\n"},{"id":"167498","messageId":"20110509133442.GC11022@sigill.intra.peff.net","threadId":"27241","inReplyTo":"20110509133153.GA10998@sigill.intra.peff.net","subject":"[PATCH 3/3] blame: add --line-porcelain output format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-09T13:34:42Z","receivedAt":"2011-05-09T13:34:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is just like --porcelain, except that we always output\nthe commit information for each line, not just the first\ntime it is referenced. This can make quick and dirty scripts\nmuch easier to write; see the example added to the blame\ndocumentation.\n\nSigned-off-by: Jeff King \n---\nI'm not 100% happy with the name, but I couldn't think of anything\nbetter. Something like --verbose-porcelain works, but is a little too\nvague. Suggestions welcome.\n\n Documentation/blame-options.txt | 5 +++++\n Documentation/git-blame.txt | 13 +++++++++++++\n builtin/blame.c | 10 ++++++++--\n t/t8008-blame-formats.sh | 19 +++++++++++++++++++\n 4 files changed, 45 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\nindex 16e3c68..e76195a 100644\n--- a/Documentation/blame-options.txt\n+++ b/Documentation/blame-options.txt\n@@ -52,6 +52,11 @@ of lines before or after the line given by .\n --porcelain::\n \tShow in a format designed for machine consumption.\n \n+--line-porcelain::\n+\tShow the porcelain format, but output commit information for\n+\teach line, not just the first time a commit is referenced.\n+\tImplies --porcelain.\n+\n --incremental::\n \tShow the result incrementally in a format designed for\n \tmachine consumption.\ndiff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt\nindex bb8edb4..9516914 100644\n--- a/Documentation/git-blame.txt\n+++ b/Documentation/git-blame.txt\n@@ -105,6 +105,19 @@ The contents of the actual line is output after the above\n header, prefixed by a TAB. This is to allow adding more\n header elements later.\n \n+The porcelain format generally suppresses commit information that has\n+already been seen. For example, two lines that are blamed to the same\n+commit will both be shown, but the details for that commit will be shown\n+only once. This is more efficient, but may require more state be kept by\n+the reader. The `--line-porcelain` option can be used to output full\n+commit information for each line, allowing simpler (but less efficient)\n+usage like:\n+\n+\t# count the number of lines attributed to each author\n+\tgit blame --line-porcelain file |\n+\tsed -n 's/^author //p' |\n+\tsort | uniq -c | sort -rn\n+\n \n SPECIFYING RANGES\n -----------------\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex d74e18f..6c26672 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1619,6 +1619,7 @@ static const char *format_time(unsigned long time, const char *tz_str,\n #define OUTPUT_SHOW_SCORE 0100\n #define OUTPUT_NO_AUTHOR 0200\n #define OUTPUT_SHOW_EMAIL\t0400\n+#define OUTPUT_LINE_PORCELAIN 01000\n \n static void emit_porcelain_details(struct origin *suspect, int repeat)\n {\n@@ -1630,6 +1631,7 @@ static void emit_porcelain_details(struct origin *suspect, int repeat)\n static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent,\n \t\t\t int opt)\n {\n+\tint repeat = opt & OUTPUT_LINE_PORCELAIN;\n \tint cnt;\n \tconst char *cp;\n \tstruct origin *suspect = ent->suspect;\n@@ -1642,15 +1644,18 @@ static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent,\n \t ent->s_lno + 1,\n \t ent->lno + 1,\n \t ent->num_lines);\n-\temit_porcelain_details(suspect, 0);\n+\temit_porcelain_details(suspect, repeat);\n \n \tcp = nth_line(sb, ent->lno);\n \tfor (cnt = 0; cnt < ent->num_lines; cnt++) {\n \t\tchar ch;\n-\t\tif (cnt)\n+\t\tif (cnt) {\n \t\t\tprintf(\"%s %d %d\\n\", hex,\n \t\t\t ent->s_lno + 1 + cnt,\n \t\t\t ent->lno + 1 + cnt);\n+\t\t\tif (repeat)\n+\t\t\t\temit_porcelain_details(suspect, 1);\n+\t\t}\n \t\tputchar('\\t');\n \t\tdo {\n \t\t\tch = *cp++;\n@@ -2307,6 +2312,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT('f', \"show-name\", &output_option, \"Show original filename (Default: auto)\", OUTPUT_SHOW_NAME),\n \t\tOPT_BIT('n', \"show-number\", &output_option, \"Show original linenumber (Default: off)\", OUTPUT_SHOW_NUMBER),\n \t\tOPT_BIT('p', \"porcelain\", &output_option, \"Show in a format designed for machine consumption\", OUTPUT_PORCELAIN),\n+\t\tOPT_BIT(0, \"line-porcelain\", &output_option, \"Show porcelain format with per-line commit information\", OUTPUT_PORCELAIN|OUTPUT_LINE_PORCELAIN),\n \t\tOPT_BIT('c', NULL, &output_option, \"Use the same output mode as git-annotate (Default: off)\", OUTPUT_ANNOTATE_COMPAT),\n \t\tOPT_BIT('t', NULL, &output_option, \"Show raw timestamp (Default: off)\", OUTPUT_RAW_TIMESTAMP),\n \t\tOPT_BIT('l', NULL, &output_option, \"Show long commit SHA1 (Default: off)\", OUTPUT_LONG_OBJECT_NAME),\ndiff --git a/t/t8008-blame-formats.sh b/t/t8008-blame-formats.sh\nindex 387d1a6..d15f8b3 100755\n--- a/t/t8008-blame-formats.sh\n+++ b/t/t8008-blame-formats.sh\n@@ -68,4 +68,23 @@ test_expect_success 'blame --porcelain output' '\n \ttest_cmp expect actual\n '\n \n+cat >expect <actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.5.rc2.8.gc085\n"},{"id":"167504","messageId":"BANLkTi=EvgRsrULEFiZsOOM4cdfVFATcSg@mail.gmail.com","threadId":"27241","inReplyTo":"20110509133402.GB11022@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] blame: refactor porcelain output","fromName":"Thiago Farina","fromEmail":"tfransosi@gmail.com","sentAt":"2011-05-09T15:39:39Z","receivedAt":"2011-05-09T15:39:39Z","isPatch":true,"sender":{"key":"tfransosi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/970071?v=4"},"body":"On Mon, May 9, 2011 at 10:34 AM, Jeff King wrote:\n> This is in preparation for adding more porcelain output\n> options. The three changes are:\n>\n>  1. emit_porcelain now receives the format option flags\n>\n>  2. emit_one_suspect_detail takes an optional \"repeat\"\n>     parameter to suppress the \"show only once\" behavior\n>\n>  3. The code for emitting porcelain suspect is factored\n>     into its own function for repeatability.\n>\n> There should be no functional changes.\n>\n> Signed-off-by: Jeff King \n> ---\n> I broke this out for readability. I can break each of the 3 out into a\n> separate patch if that helps, but it seemed excessive.\n>\n>  builtin/blame.c |   25 ++++++++++++++++---------\n>  1 files changed, 16 insertions(+), 9 deletions(-)\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 4242e4b..d74e18f 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -1484,13 +1484,14 @@ static void write_filename_info(const char *path)\n>  /*\n>  * Porcelain/Incremental format wants to show a lot of details per\n>  * commit.  Instead of repeating this every line, emit it only once,\n> - * the first time each commit appears in the output.\n> + * the first time each commit appears in the output (unless the\n> + * user has specifically asked for us to repeat).\n>  */\n> -static int emit_one_suspect_detail(struct origin *suspect)\n> +static int emit_one_suspect_detail(struct origin *suspect, int repeat)\n>  {\n>        struct commit_info ci;\n>\n> -       if (suspect->commit->object.flags & METAINFO_SHOWN)\n> +       if (!repeat && suspect->commit->object.flags & METAINFO_SHOWN)\n\nMaybe would be worth adding parentheses here:\n\nif (!repeat && (...))\n return 0;\n\n?\n\nProbably is fine as is though.\n"}]}