{"thread":{"id":"14747","subject":"[PATCH 3/4] Add a new test for using a custom merge strategy","startedAt":"2008-07-29T23:16:57Z","lastAt":"2008-07-30T17:27:02Z","messageCount":10,"participants":["Miklos Vajna","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"85540","messageId":"cover.1217372486.git.vmiklos@frugalware.org","threadId":"14747","inReplyTo":"7v4p68eame.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 0/4] Allow custom merge strategies, take 4","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-07-29T23:16:57Z","receivedAt":"2008-07-29T23:16:57Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Tue, Jul 29, 2008 at 12:47:05PM -0700, Junio C Hamano <gitster@pobox.com> wrote:\n> This feels overly wasteful.  Granted, this is not a performance critical\n> codepath, but you list all commands that begin with \"git-merge-\" in\n> is_git_command(), only to discard it and then iterate over 140+ main_cmds\n> list only to cull the ones whose name do not appear in the strategies\n> list.\n>\n> Perhaps this shows that changing the function is_git_command() is a wrong\n> approach (for one thing, with the custom prefix, it is not about \"Is it a\n> git command\" anymore).  Wouldn't it be easier to read if you did this part\n> like this instead?\n>\n>  * make load_command_list capable of loading into a \"struct cmdnames\" (or\n>    pair if you want) supplied by the caller;\n>\n>  * use it to grab all commands whose name begin with \"git-merge-\" here;\n>\n>  * Check if name appears in that list; if it doesn't, you already have the\n>    list of commands that could be merge strategy for error reporting.\n>\n> If you also update list_commands() not to run load_command_list() itself\n> but take caller-supplied list of commands, then the API would become much\n> cleaner.  The caller would not be limited to \"filter with prefix\" anymore.\n>\n> Hmm?\n\nI like the idea. ;-)\n\nHere is what I did:\n\n- I changed only load_command_list() and list_commands().\n  load_command_list() still supports filtering, but the result is loaded\n  to a pair of \"struct cmdnames\", supplied by the caller.\n\n- list_commands() works from this pair, additionally supporting the\n  custom title.\n\n- export add_cmdname(), exclude_cmds() and is_in_cmdlist() without any\n  modification in help.h\n\nThe rest is done in builtin-merge.c, as you suggested.\n\nThe nice thing is that before this change builtin-merge accepted for\nexample '-s index' (because git-merge-index was a valid command), but\nnow this is no longer true.\n\nMiklos Vajna (4):\n  builtin-help: make some internal functions available to other\n    builtins\n  builtin-merge: allow using a custom strategy\n  Add a new test for using a custom merge strategy\n  Add a second testcase for handling invalid strategies in git-merge\n\n Makefile                |    1 +\n builtin-merge.c         |   42 +++++++++++++++++++++----\n help.c                  |   77 ++++++++++++++++++++++++-----------------------\n help.h                  |   23 ++++++++++++++\n t/t7600-merge.sh        |    5 +++\n t/t7606-merge-custom.sh |   46 ++++++++++++++++++++++++++++\n 6 files changed, 149 insertions(+), 45 deletions(-)\n create mode 100644 help.h\n create mode 100755 t/t7606-merge-custom.sh\n"},{"id":"85539","messageId":"a2d2bc675801bb03e3035ec0102eb27f08f27f1b.1217372486.git.vmiklos@frugalware.org","threadId":"14747","inReplyTo":"cover.1217372486.git.vmiklos@frugalware.org","subject":"[PATCH 1/4] builtin-help: make some internal functions available to other builtins","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-07-29T23:16:58Z","receivedAt":"2008-07-29T23:16:58Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"Make load_command_list() capable of filtering for a given prefix and\nloading into a pair of \"struct cmdnames\" supplied by the caller.\n\nMake the static add_cmdname(), exclude_cmds() and is_in_cmdlist()\nfunctions non-static.\n\nMake list_commands() accept a custom title, and work from a pair of\n\"struct cmdnames\" supplied by the caller.\n\nSigned-off-by: Miklos Vajna <vmiklos@frugalware.org>\n---\n Makefile |    1 +\n help.c   |   77 +++++++++++++++++++++++++++++++------------------------------\n help.h   |   23 ++++++++++++++++++\n 3 files changed, 63 insertions(+), 38 deletions(-)\n create mode 100644 help.h\n\ndiff --git a/Makefile b/Makefile\nindex 52c67c1..83d79af 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -355,6 +355,7 @@ LIB_H += git-compat-util.h\n LIB_H += graph.h\n LIB_H += grep.h\n LIB_H += hash.h\n+LIB_H += help.h\n LIB_H += list-objects.h\n LIB_H += ll-merge.h\n LIB_H += log-tree.h\ndiff --git a/help.c b/help.c\nindex 3cb1962..88c0d5b 100644\n--- a/help.c\n+++ b/help.c\n@@ -9,6 +9,7 @@\n #include \"common-cmds.h\"\n #include \"parse-options.h\"\n #include \"run-command.h\"\n+#include \"help.h\"\n \n static struct man_viewer_list {\n \tstruct man_viewer_list *next;\n@@ -300,18 +301,11 @@ static inline void mput_char(char c, unsigned int num)\n \t\tputchar(c);\n }\n \n-static struct cmdnames {\n-\tint alloc;\n-\tint cnt;\n-\tstruct cmdname {\n-\t\tsize_t len;\n-\t\tchar name[1];\n-\t} **names;\n-} main_cmds, other_cmds;\n+struct cmdnames main_cmds, other_cmds;\n \n-static void add_cmdname(struct cmdnames *cmds, const char *name, int len)\n+void add_cmdname(struct cmdnames *cmds, const char *name, int len)\n {\n-\tstruct cmdname *ent = xmalloc(sizeof(*ent) + len);\n+\tstruct cmdname *ent = xmalloc(sizeof(*ent) + len + 1);\n \n \tent->len = len;\n \tmemcpy(ent->name, name, len);\n@@ -342,7 +336,7 @@ static void uniq(struct cmdnames *cmds)\n \tcmds->cnt = j;\n }\n \n-static void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes)\n+void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes)\n {\n \tint ci, cj, ei;\n \tint cmp;\n@@ -418,11 +412,11 @@ static int is_executable(const char *name)\n }\n \n static unsigned int list_commands_in_dir(struct cmdnames *cmds,\n-\t\t\t\t\t const char *path)\n+\t\t\t\t\t const char *path,\n+\t\t\t\t\t const char *prefix)\n {\n \tunsigned int longest = 0;\n-\tconst char *prefix = \"git-\";\n-\tint prefix_len = strlen(prefix);\n+\tint prefix_len;\n \tDIR *dir = opendir(path);\n \tstruct dirent *de;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -430,6 +424,9 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,\n \n \tif (!dir)\n \t\treturn 0;\n+\tif (!prefix)\n+\t\tprefix = \"git-\";\n+\tprefix_len = strlen(prefix);\n \n \tstrbuf_addf(&buf, \"%s/\", path);\n \tlen = buf.len;\n@@ -460,7 +457,9 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,\n \treturn longest;\n }\n \n-static unsigned int load_command_list(void)\n+unsigned int load_command_list(const char *prefix,\n+\t\tstruct cmdnames *main_cmds,\n+\t\tstruct cmdnames *other_cmds)\n {\n \tunsigned int longest = 0;\n \tunsigned int len;\n@@ -469,7 +468,7 @@ static unsigned int load_command_list(void)\n \tconst char *exec_path = git_exec_path();\n \n \tif (exec_path)\n-\t\tlongest = list_commands_in_dir(&main_cmds, exec_path);\n+\t\tlongest = list_commands_in_dir(main_cmds, exec_path, prefix);\n \n \tif (!env_path) {\n \t\tfprintf(stderr, \"PATH not set\\n\");\n@@ -481,7 +480,7 @@ static unsigned int load_command_list(void)\n \t\tif ((colon = strchr(path, PATH_SEP)))\n \t\t\t*colon = 0;\n \n-\t\tlen = list_commands_in_dir(&other_cmds, path);\n+\t\tlen = list_commands_in_dir(other_cmds, path, prefix);\n \t\tif (len > longest)\n \t\t\tlongest = len;\n \n@@ -491,36 +490,38 @@ static unsigned int load_command_list(void)\n \t}\n \tfree(paths);\n \n-\tqsort(main_cmds.names, main_cmds.cnt,\n-\t      sizeof(*main_cmds.names), cmdname_compare);\n-\tuniq(&main_cmds);\n+\tqsort(main_cmds->names, main_cmds->cnt,\n+\t      sizeof(*main_cmds->names), cmdname_compare);\n+\tuniq(main_cmds);\n \n-\tqsort(other_cmds.names, other_cmds.cnt,\n-\t      sizeof(*other_cmds.names), cmdname_compare);\n-\tuniq(&other_cmds);\n-\texclude_cmds(&other_cmds, &main_cmds);\n+\tqsort(other_cmds->names, other_cmds->cnt,\n+\t      sizeof(*other_cmds->names), cmdname_compare);\n+\tuniq(other_cmds);\n+\texclude_cmds(other_cmds, main_cmds);\n \n \treturn longest;\n }\n \n-static void list_commands(void)\n+void list_commands(const char *title, unsigned int longest,\n+\t\tstruct cmdnames *main_cmds, struct cmdnames *other_cmds)\n {\n-\tunsigned int longest = load_command_list();\n \tconst char *exec_path = git_exec_path();\n \n-\tif (main_cmds.cnt) {\n-\t\tprintf(\"available git commands in '%s'\\n\", exec_path);\n-\t\tprintf(\"----------------------------\");\n-\t\tmput_char('-', strlen(exec_path));\n+\tif (main_cmds->cnt) {\n+\t\tprintf(\"available %s in '%s'\\n\", title, exec_path);\n+\t\tprintf(\"----------------\");\n+\t\tmput_char('-', strlen(title) + strlen(exec_path));\n \t\tputchar('\\n');\n-\t\tpretty_print_string_list(&main_cmds, longest);\n+\t\tpretty_print_string_list(main_cmds, longest);\n \t\tputchar('\\n');\n \t}\n \n-\tif (other_cmds.cnt) {\n-\t\tprintf(\"git commands available from elsewhere on your $PATH\\n\");\n-\t\tprintf(\"---------------------------------------------------\\n\");\n-\t\tpretty_print_string_list(&other_cmds, longest);\n+\tif (other_cmds->cnt) {\n+\t\tprintf(\"%s available from elsewhere on your $PATH\\n\", title);\n+\t\tprintf(\"---------------------------------------\");\n+\t\tmput_char('-', strlen(title));\n+\t\tputchar('\\n');\n+\t\tpretty_print_string_list(other_cmds, longest);\n \t\tputchar('\\n');\n \t}\n }\n@@ -542,7 +543,7 @@ void list_common_cmds_help(void)\n \t}\n }\n \n-static int is_in_cmdlist(struct cmdnames *c, const char *s)\n+int is_in_cmdlist(struct cmdnames *c, const char *s)\n {\n \tint i;\n \tfor (i = 0; i < c->cnt; i++)\n@@ -553,7 +554,6 @@ static int is_in_cmdlist(struct cmdnames *c, const char *s)\n \n static int is_git_command(const char *s)\n {\n-\tload_command_list();\n \treturn is_in_cmdlist(&main_cmds, s) ||\n \t\tis_in_cmdlist(&other_cmds, s);\n }\n@@ -698,8 +698,9 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n \t\t\tbuiltin_help_usage, 0);\n \n \tif (show_all) {\n+\t\tunsigned int longest = load_command_list(\"git-\", &main_cmds, &other_cmds);\n \t\tprintf(\"usage: %s\\n\\n\", git_usage_string);\n-\t\tlist_commands();\n+\t\tlist_commands(\"git commands\", longest, &main_cmds, &other_cmds);\n \t\tprintf(\"%s\\n\", git_more_info_string);\n \t\treturn 0;\n \t}\ndiff --git a/help.h b/help.h\nnew file mode 100644\nindex 0000000..d614e54\n--- /dev/null\n+++ b/help.h\n@@ -0,0 +1,23 @@\n+#ifndef HELP_H\n+#define HELP_H\n+\n+struct cmdnames {\n+\tint alloc;\n+\tint cnt;\n+\tstruct cmdname {\n+\t\tsize_t len;\n+\t\tchar name[FLEX_ARRAY];\n+\t} **names;\n+};\n+\n+unsigned int load_command_list(const char *prefix,\n+\t\tstruct cmdnames *main_cmds,\n+\t\tstruct cmdnames *other_cmds);\n+void add_cmdname(struct cmdnames *cmds, const char *name, int len);\n+/* Here we require that excludes is a sorted list. */\n+void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes);\n+int is_in_cmdlist(struct cmdnames *c, const char *s);\n+void list_commands(const char *title, unsigned int longest,\n+\t\tstruct cmdnames *main_cmds, struct cmdnames *other_cmds);\n+\n+#endif /* HELP_H */\n-- \n1.6.0.rc0.14.g95f8.dirty\n"},{"id":"85538","messageId":"18a8b4aaf4cc4bc720bd673166d4a7722ed79556.1217372486.git.vmiklos@frugalware.org","threadId":"14747","inReplyTo":"a2d2bc675801bb03e3035ec0102eb27f08f27f1b.1217372486.git.vmiklos@frugalware.org","subject":"[PATCH 2/4] builtin-merge: allow using a custom strategy","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-07-29T23:16:59Z","receivedAt":"2008-07-29T23:16:59Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"Allow using a custom strategy, as long as it's named git-merge-foo. The\nerror handling is now done using is_git_command(). The list of available\nstrategies is now shown by list_commands().\n\nIf an invalid strategy is supplied, like -s foobar, then git-merge would\nlist all git-merge-* commands. This is not perfect, since for example\ngit-merge-index is not a valid strategy.\n\nThese are removed from the output by scanning the list of main commands;\nif the git-merge-foo command is listed in the all_strategy list, then\nit's shown, otherwise excluded. This does not exclude commands somewhere\nelse in the PATH, where custom strategies are expected.\n\nSigned-off-by: Miklos Vajna <vmiklos@frugalware.org>\n---\n builtin-merge.c |   42 +++++++++++++++++++++++++++++++++++-------\n 1 files changed, 35 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin-merge.c b/builtin-merge.c\nindex e78fa18..99b307a 100644\n--- a/builtin-merge.c\n+++ b/builtin-merge.c\n@@ -22,6 +22,7 @@\n #include \"log-tree.h\"\n #include \"color.h\"\n #include \"rerere.h\"\n+#include \"help.h\"\n \n #define DEFAULT_TWOHEAD (1<<0)\n #define DEFAULT_OCTOPUS (1<<1)\n@@ -77,7 +78,9 @@ static int option_parse_message(const struct option *opt,\n static struct strategy *get_strategy(const char *name)\n {\n \tint i;\n-\tstruct strbuf err;\n+\tstruct strategy *ret;\n+\tstatic struct cmdnames main_cmds, other_cmds;\n+\tstatic int longest = 0;\n \n \tif (!name)\n \t\treturn NULL;\n@@ -86,12 +89,37 @@ static struct strategy *get_strategy(const char *name)\n \t\tif (!strcmp(name, all_strategy[i].name))\n \t\t\treturn &all_strategy[i];\n \n-\tstrbuf_init(&err, 0);\n-\tfor (i = 0; i < ARRAY_SIZE(all_strategy); i++)\n-\t\tstrbuf_addf(&err, \" %s\", all_strategy[i].name);\n-\tfprintf(stderr, \"Could not find merge strategy '%s'.\\n\", name);\n-\tfprintf(stderr, \"Available strategies are:%s.\\n\", err.buf);\n-\texit(1);\n+\tif (!longest) {\n+\t\tstruct cmdnames not_strategies;\n+\n+\t\tmemset(&main_cmds, 0, sizeof(struct cmdnames));\n+\t\tmemset(&other_cmds, 0, sizeof(struct cmdnames));\n+\t\tmemset(&not_strategies, 0, sizeof(struct cmdnames));\n+\t\tlongest = load_command_list(\"git-merge-\", &main_cmds,\n+\t\t\t\t&other_cmds);\n+\t\tfor (i = 0; i < main_cmds.cnt; i++) {\n+\t\t\tint j, found = 0;\n+\t\t\tstruct cmdname *ent = main_cmds.names[i];\n+\t\t\tfor (j = 0; j < ARRAY_SIZE(all_strategy); j++)\n+\t\t\t\tif (!strncmp(ent->name, all_strategy[j].name, ent->len)\n+\t\t\t\t\t\t&& !all_strategy[j].name[ent->len])\n+\t\t\t\t\tfound = 1;\n+\t\t\tif (!found)\n+\t\t\t\tadd_cmdname(&not_strategies, ent->name, ent->len);\n+\t\t\texclude_cmds(&main_cmds, &not_strategies);\n+\t\t}\n+\t}\n+\tif (!is_in_cmdlist(&main_cmds, name) && !is_in_cmdlist(&other_cmds, name)) {\n+\n+\t\tfprintf(stderr, \"Could not find merge strategy '%s'.\\n\\n\", name);\n+\t\tlist_commands(\"strategies\", longest, &main_cmds, &other_cmds);\n+\t\texit(1);\n+\t}\n+\n+\tret = xmalloc(sizeof(struct strategy));\n+\tmemset(ret, 0, sizeof(struct strategy));\n+\tret->name = xstrdup(name);\n+\treturn ret;\n }\n \n static void append_strategy(struct strategy *s)\n-- \n1.6.0.rc0.14.g95f8.dirty\n"},{"id":"85536","messageId":"ee13073cd83ff4a3cffb926b21cc54583e67f869.1217372486.git.vmiklos@frugalware.org","threadId":"14747","inReplyTo":"18a8b4aaf4cc4bc720bd673166d4a7722ed79556.1217372486.git.vmiklos@frugalware.org","subject":"[PATCH 3/4] Add a new test for using a custom merge strategy","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-07-29T23:17:00Z","receivedAt":"2008-07-29T23:17:00Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"Testing is done by creating a simple git-merge-theirs strategy which is\nthe opposite of ours. Using this in real merges is not recommended but\nit's perfect for our testing needs.\n\nSigned-off-by: Miklos Vajna <vmiklos@frugalware.org>\n---\n t/t7606-merge-custom.sh |   46 ++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 46 insertions(+), 0 deletions(-)\n create mode 100755 t/t7606-merge-custom.sh\n\ndiff --git a/t/t7606-merge-custom.sh b/t/t7606-merge-custom.sh\nnew file mode 100755\nindex 0000000..13e8ff5\n--- /dev/null\n+++ b/t/t7606-merge-custom.sh\n@@ -0,0 +1,46 @@\n+#!/bin/sh\n+\n+test_description='git-merge\n+\n+Testing a custom strategy.'\n+\n+. ./test-lib.sh\n+\n+cat > git-merge-theirs << EOF\n+#!/bin/sh\n+eval git read-tree --reset -u \\\\\\$\\$#\n+EOF\n+chmod +x git-merge-theirs\n+PATH=.:$PATH\n+export PATH\n+\n+test_expect_success 'setup' '\n+\techo c0 > c0.c &&\n+\tgit add c0.c &&\n+\tgit commit -m c0 &&\n+\tgit tag c0 &&\n+\techo c1 > c1.c &&\n+\tgit add c1.c &&\n+\tgit commit -m c1 &&\n+\tgit tag c1 &&\n+\tgit reset --hard c0 &&\n+\techo c2 > c2.c &&\n+\tgit add c2.c &&\n+\tgit commit -m c2 &&\n+\tgit tag c2\n+'\n+\n+test_expect_success 'merge c2 with a custom strategy' '\n+\tgit reset --hard c1 &&\n+\tgit merge -s theirs c2 &&\n+\ttest \"$(git rev-parse c1)\" != \"$(git rev-parse HEAD)\" &&\n+\ttest \"$(git rev-parse c1)\" = \"$(git rev-parse HEAD^1)\" &&\n+\ttest \"$(git rev-parse c2)\" = \"$(git rev-parse HEAD^2)\" &&\n+\ttest \"$(git rev-parse c2^{tree})\" = \"$(git rev-parse HEAD^{tree})\" &&\n+\tgit diff --exit-code &&\n+\ttest -f c0.c &&\n+\ttest ! -f c1.c &&\n+\ttest -f c2.c\n+'\n+\n+test_done\n-- \n1.6.0.rc0.14.g95f8.dirty\n"},{"id":"85537","messageId":"f8e5afe347c194013ffd104b196a19a37d762f9b.1217372486.git.vmiklos@frugalware.org","threadId":"14747","inReplyTo":"ee13073cd83ff4a3cffb926b21cc54583e67f869.1217372486.git.vmiklos@frugalware.org","subject":"[PATCH 4/4] Add a second testcase for handling invalid strategies in git-merge","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-07-29T23:17:01Z","receivedAt":"2008-07-29T23:17:01Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"This one tests '-s index' which is interesting because git-merge-index\nis an existing git command but it is not a valid strategy.\n\nSigned-off-by: Miklos Vajna <vmiklos@frugalware.org>\n---\n t/t7600-merge.sh |    4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 5eeb6c2..0329aee 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -230,6 +230,10 @@ test_expect_success 'test option parsing' '\n \ttest_must_fail git merge\n '\n \n+test_expect_success 'reject non-strategy with a git-merge-foo name' '\n+\ttest_must_fail git merge -s index c1\n+'\n+\n test_expect_success 'merge c0 with c1' '\n \tgit reset --hard c0 &&\n \tgit merge c1 &&\n-- \n1.6.0.rc0.14.g95f8.dirty\n"},{"id":"85542","messageId":"7vhca89s02.fsf@gitster.siamese.dyndns.org","threadId":"14747","inReplyTo":"f8e5afe347c194013ffd104b196a19a37d762f9b.1217372486.git.vmiklos@frugalware.org","subject":"Re: [PATCH 4/4] Add a second testcase for handling invalid strategies in git-merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-29T23:42:53Z","receivedAt":"2008-07-29T23:42:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miklos Vajna <vmiklos@frugalware.org> writes:\n\n> This one tests '-s index' which is interesting because git-merge-index\n> is an existing git command but it is not a valid strategy.\n>\n> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>\n> ---\n>  t/t7600-merge.sh |    4 ++++\n>  1 files changed, 4 insertions(+), 0 deletions(-)\n>\n> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\n> index 5eeb6c2..0329aee 100755\n> --- a/t/t7600-merge.sh\n> +++ b/t/t7600-merge.sh\n> @@ -230,6 +230,10 @@ test_expect_success 'test option parsing' '\n>  \ttest_must_fail git merge\n>  '\n>  \n> +test_expect_success 'reject non-strategy with a git-merge-foo name' '\n> +\ttest_must_fail git merge -s index c1\n> +'\n> +\n\nTrue, but with the old code that might blindly have executed\ngit-merge-index the test would also have failed, and you would want to\ntell two cases apart, wouldn't you?\n"},{"id":"85543","messageId":"7vabg09rye.fsf@gitster.siamese.dyndns.org","threadId":"14747","inReplyTo":"ee13073cd83ff4a3cffb926b21cc54583e67f869.1217372486.git.vmiklos@frugalware.org","subject":"Re: [PATCH 3/4] Add a new test for using a custom merge strategy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-29T23:43:53Z","receivedAt":"2008-07-29T23:43:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miklos Vajna <vmiklos@frugalware.org> writes:\n\n> +cat > git-merge-theirs << EOF\n> +#!/bin/sh\n> +eval git read-tree --reset -u \\\\\\$\\$#\n> +EOF\n\nThis should be $SHELL_PATH, instead of hardcoded /bin/sh, to be easy on\npeople on Solaris and other systems.\n\nOther than that, the patch is good and there is no need to resend it.\nThanks.\n\nBy the way, this eval shows why \"theirs\" cannot be a symmetric operation\nof \"ours\".  You are taking the last remote HEAD even when you are merging\nmore than one remote into the current branch at once.  \"ours\" can be\nsensibly defined for an octopus, but \"theirs\" has this \"which theirs\"\nproblem ;-)\n"},{"id":"85586","messageId":"7vljzj99j3.fsf@gitster.siamese.dyndns.org","threadId":"14747","inReplyTo":"a2d2bc675801bb03e3035ec0102eb27f08f27f1b.1217372486.git.vmiklos@frugalware.org","subject":"Re: [PATCH 1/4] builtin-help: make some internal functions available to other builtins","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-30T06:21:52Z","receivedAt":"2008-07-30T06:21:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks; all four patches queued.\n"},{"id":"85653","messageId":"20080730172532.GF32057@genesis.frugalware.org","threadId":"14747","inReplyTo":"7vabg09rye.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 3/4] Add a new test for using a custom merge strategy","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-07-30T17:25:32Z","receivedAt":"2008-07-30T17:25:32Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Tue, Jul 29, 2008 at 04:43:53PM -0700, Junio C Hamano <gitster@pobox.com> wrote:\n> By the way, this eval shows why \"theirs\" cannot be a symmetric operation\n> of \"ours\".  You are taking the last remote HEAD even when you are merging\n> more than one remote into the current branch at once.  \"ours\" can be\n> sensibly defined for an octopus, but \"theirs\" has this \"which theirs\"\n> problem ;-)\n\nOh, well, sure. But _if_ it turns out there is a demand for that kind of\ngit-merge-theirs, then I suppose we could still give up if we are given\ntwo or more remotes, just like merge-resolve and others do.\n"},{"id":"85654","messageId":"20080730172702.GG32057@genesis.frugalware.org","threadId":"14747","inReplyTo":"7vhca89s02.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 4/4] Add a second testcase for handling invalid strategies in git-merge","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-07-30T17:27:02Z","receivedAt":"2008-07-30T17:27:02Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Tue, Jul 29, 2008 at 04:42:53PM -0700, Junio C Hamano <gitster@pobox.com> wrote:\n> True, but with the old code that might blindly have executed\n> git-merge-index the test would also have failed, and you would want to\n> tell two cases apart, wouldn't you?\n\nHmm, I wonder what is the right approach to test it. Should I exit with\na different error code with the strategy is invalid?\n"}]}