{"thread":{"id":"43018","subject":"[PATCH] merge: use string_list_split() in add_strategies()","startedAt":"2016-08-05T21:01:53Z","lastAt":"2016-08-10T20:54:43Z","messageCount":6,"participants":["René Scharfe","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"293236","messageId":"57A4FEAF.3040208@web.de","threadId":"43018","inReplyTo":null,"subject":"[PATCH] merge: use string_list_split() in add_strategies()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-08-05T21:01:35Z","receivedAt":"2016-08-05T21:01:53Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Call string_list_split() for cutting a space separated list into pieces\ninstead of reimplementing it based on struct strategy.  The attr member\nof struct strategy was not used split_merge_strategies(); it was a pure\nstring operation.  Also be nice and clean up once we're done splitting;\nthe old code didn't bother freeing any of the allocated memory.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n builtin/merge.c | 44 ++++++++++----------------------------------\n 1 file changed, 10 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 19b3bc2..a95a801 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -30,6 +30,7 @@\n #include \"fmt-merge-msg.h\"\n #include \"gpg-interface.h\"\n #include \"sequencer.h\"\n+#include \"string-list.h\"\n \n #define DEFAULT_TWOHEAD (1<<0)\n #define DEFAULT_OCTOPUS (1<<1)\n@@ -703,42 +704,17 @@ static int count_unmerged_entries(void)\n \treturn ret;\n }\n \n-static void split_merge_strategies(const char *string, struct strategy **list,\n-\t\t\t\t   int *nr, int *alloc)\n-{\n-\tchar *p, *q, *buf;\n-\n-\tif (!string)\n-\t\treturn;\n-\n-\tbuf = xstrdup(string);\n-\tq = buf;\n-\tfor (;;) {\n-\t\tp = strchr(q, ' ');\n-\t\tif (!p) {\n-\t\t\tALLOC_GROW(*list, *nr + 1, *alloc);\n-\t\t\t(*list)[(*nr)++].name = xstrdup(q);\n-\t\t\tfree(buf);\n-\t\t\treturn;\n-\t\t} else {\n-\t\t\t*p = '\\0';\n-\t\t\tALLOC_GROW(*list, *nr + 1, *alloc);\n-\t\t\t(*list)[(*nr)++].name = xstrdup(q);\n-\t\t\tq = ++p;\n-\t\t}\n-\t}\n-}\n-\n static void add_strategies(const char *string, unsigned attr)\n {\n-\tstruct strategy *list = NULL;\n-\tint list_alloc = 0, list_nr = 0, i;\n-\n-\tmemset(&list, 0, sizeof(list));\n-\tsplit_merge_strategies(string, &list, &list_nr, &list_alloc);\n-\tif (list) {\n-\t\tfor (i = 0; i < list_nr; i++)\n-\t\t\tappend_strategy(get_strategy(list[i].name));\n+\tint i;\n+\n+\tif (string) {\n+\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\t\tstruct string_list_item *item;\n+\t\tstring_list_split(&list, string, ' ', -1);\n+\t\tfor_each_string_list_item(item, &list)\n+\t\t\tappend_strategy(get_strategy(item->string));\n+\t\tstring_list_clear(&list, 0);\n \t\treturn;\n \t}\n \tfor (i = 0; i < ARRAY_SIZE(all_strategy); i++)\n-- \n2.9.2\n\n"},{"id":"293342","messageId":"alpine.DEB.2.20.1608081034250.5786@virtualbox","threadId":"43018","inReplyTo":"57A4FEAF.3040208@web.de","subject":"Re: [PATCH] merge: use string_list_split() in add_strategies()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-08T08:39:08Z","receivedAt":"2016-08-08T08:39:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Fri, 5 Aug 2016, René Scharfe wrote:\n\n>  static void add_strategies(const char *string, unsigned attr)\n>  {\n> -\tstruct strategy *list = NULL;\n> -\tint list_alloc = 0, list_nr = 0, i;\n> -\n> -\tmemset(&list, 0, sizeof(list));\n> -\tsplit_merge_strategies(string, &list, &list_nr, &list_alloc);\n> -\tif (list) {\n> -\t\tfor (i = 0; i < list_nr; i++)\n> -\t\t\tappend_strategy(get_strategy(list[i].name));\n> +\tint i;\n> +\n> +\tif (string) {\n> +\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n> +\t\tstruct string_list_item *item;\n> +\t\tstring_list_split(&list, string, ' ', -1);\n> +\t\tfor_each_string_list_item(item, &list)\n> +\t\t\tappend_strategy(get_strategy(item->string));\n> +\t\tstring_list_clear(&list, 0);\n>  \t\treturn;\n>  \t}\n\nA nice code reduction!\n\nI wonder, however, if we could somhow turn things around by introducing\nsomething like\n\n\tsplit_and_do_for_each(item_p, length, string, delimiter)\n\t\t... <do something with item_p and length> ...\n\nthat both string_list_split() *and* add_strategies() could use? We would\nthen be able to avoid allocating the list and duplicating the items in the\nlatter case.\n\nCiao,\nDscho"},{"id":"293371","messageId":"xmqqeg5zf8mh.fsf@gitster.mtv.corp.google.com","threadId":"43018","inReplyTo":"alpine.DEB.2.20.1608081034250.5786@virtualbox","subject":"Re: [PATCH] merge: use string_list_split() in add_strategies()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-08T17:55:34Z","receivedAt":"2016-08-08T17:55:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> I wonder, however, if we could somhow turn things around by introducing\n> something like\n>\n> \tsplit_and_do_for_each(item_p, length, string, delimiter)\n> \t\t... <do something with item_p and length> ...\n>\n> that both string_list_split() *and* add_strategies() could use? We would\n> then be able to avoid allocating the list and duplicating the items in the\n> latter case.\n\nI do think such a feature may be useful if we often work on pieces\nof a string delimited by a delimiter, but if the caller does not see\nthe split result, then the function with \"split\" is probably\nmisnamed.\n\nI however suspect the variant of this where \"delimiter\" can just be\na single byte would not be so useful.\n\nIf the input comes from the end user, we certainly would want to\nallow \"word1 word2\\tword3 \" as input (i.e. squishing repeated\ndelimiters into one without introducing an \"empty\" element, allowing\nmore than one delimiter characters like SP and HT, and ignoring\nleading or trailing runs of delimiter characters).\n\nIf the input is generated internally, perhaps we should rethink the\ninterface between the function that wants to do the for-each-word\nand its caller; if the caller wants to pass multiple things to the\ncallee, it should be able to do so without first having to stuff\nthese multiple things into a single string, only to force the callee\nto use this helper to split them out into individual pieces.\n\n"},{"id":"293388","messageId":"xmqqfuqfdnqr.fsf@gitster.mtv.corp.google.com","threadId":"43018","inReplyTo":"xmqqeg5zf8mh.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] merge: use string_list_split() in add_strategies()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-08T20:11:56Z","receivedAt":"2016-08-08T20:12:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> If the input comes from the end user, we certainly would want to\n> allow \"word1 word2\\tword3 \" as input (i.e. squishing repeated\n\nAny intelligent reader may have guessed already, but before I\nstupidly told Emacs to refill the paragraph, the above example had\ntwo SPs between word1 and word2.  Sorry for being sloppy.\n\n> delimiters into one without introducing an \"empty\" element, allowing\n> more than one delimiter characters like SP and HT, and ignoring\n> leading or trailing runs of delimiter characters).\n>\n> If the input is generated internally, perhaps we should rethink the\n> interface between the function that wants to do the for-each-word\n> and its caller; if the caller wants to pass multiple things to the\n> callee, it should be able to do so without first having to stuff\n> these multiple things into a single string, only to force the callee\n> to use this helper to split them out into individual pieces.\n"},{"id":"293608","messageId":"alpine.DEB.2.20.1608101431300.4924@virtualbox","threadId":"43018","inReplyTo":"xmqqeg5zf8mh.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] merge: use string_list_split() in add_strategies()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-10T12:31:34Z","receivedAt":"2016-08-10T19:21:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 8 Aug 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > I wonder, however, if we could somhow turn things around by\n> > introducing something like\n> >\n> > \tsplit_and_do_for_each(item_p, length, string, delimiter)\n> > \t\t... <do something with item_p and length> ...\n> >\n> > that both string_list_split() *and* add_strategies() could use? We\n> > would then be able to avoid allocating the list and duplicating the\n> > items in the latter case.\n> \n> I do think such a feature may be useful if we often work on pieces of a\n> string delimited by a delimiter, but if the caller does not see the\n> split result, then the function with \"split\" is probably misnamed.\n> \n> I however suspect the variant of this where \"delimiter\" can just be a\n> single byte would not be so useful.\n> \n> If the input comes from the end user, we certainly would want to allow\n> \"word1  word2\\tword3 \" as input (i.e. squishing repeated delimiters into\n> one without introducing an \"empty\" element, allowing more than one\n> delimiter characters like SP and HT, and ignoring leading or trailing\n> runs of delimiter characters).\n> \n> If the input is generated internally, perhaps we should rethink the\n> interface between the function that wants to do the for-each-word and\n> its caller; if the caller wants to pass multiple things to the callee,\n> it should be able to do so without first having to stuff these multiple\n> things into a single string, only to force the callee to use this helper\n> to split them out into individual pieces.\n\nAll true, but I guess this type of complexity would really complexify\nRené's patch too much, so I am comfortable with the patch as-is.\n\nCiao,\nDscho"},{"id":"293645","messageId":"xmqqfuqc7fe1.fsf@gitster.mtv.corp.google.com","threadId":"43018","inReplyTo":"alpine.DEB.2.20.1608101431300.4924@virtualbox","subject":"Re: [PATCH] merge: use string_list_split() in add_strategies()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-10T16:33:26Z","receivedAt":"2016-08-10T20:54:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> All true, but I guess this type of complexity would really complexify\n> René's patch too much, so I am comfortable with the patch as-is.\n\nYeah, good that we reached the same conclusion, as my point was that\nfor_each_word() would not be all that useful.\n"}]}