Skip to contentSkip to conversation

merge: use string_list_split() in add_strategies()

PATCH In review René Scharfe 3 participants 6 messages Aug 5, 2016 – Aug 10, 2016

lore
Share & tools
lore

René Scharfe

author
#0 lore raw

PATCH 1/1 merge: use string_list_split() in add_strategies()

Call string_list_split() for cutting a space separated list into pieces instead of reimplementing it based on struct strategy. The attr member of struct strategy was not used split_merge_strategies(); it was a pure string operation. Also be nice and clean up once we're done splitting; the old code didn't bother freeing any of the allocated memory.

Signed-off-by Rene Scharfe <[email protected]>
---
 builtin/merge.c | 44 ++++++++++----------------------------------
 1 file changed, 10 insertions(+), 34 deletions(-)
builtin/merge.c +10 -34
index 19b3bc2..a95a801 100644
@@ -30,6 +30,7 @@
 #include "fmt-merge-msg.h"
 #include "gpg-interface.h"
 #include "sequencer.h"
+#include "string-list.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)

@@ -703,42 +704,17 @@ static int count_unmerged_entries(void)
 	return ret;
 }
 
-static void split_merge_strategies(const char *string, struct strategy **list,
-				   int *nr, int *alloc)
-{
-	char *p, *q, *buf;
-
-	if (!string)
-		return;
-
-	buf = xstrdup(string);
-	q = buf;
-	for (;;) {
-		p = strchr(q, ' ');
-		if (!p) {
-			ALLOC_GROW(*list, *nr + 1, *alloc);
-			(*list)[(*nr)++].name = xstrdup(q);
-			free(buf);
-			return;
-		} else {
-			*p = '\0';
-			ALLOC_GROW(*list, *nr + 1, *alloc);
-			(*list)[(*nr)++].name = xstrdup(q);
-			q = ++p;
-		}
-	}
-}
-
 static void add_strategies(const char *string, unsigned attr)
 {
-	struct strategy *list = NULL;
-	int list_alloc = 0, list_nr = 0, i;
-
-	memset(&list, 0, sizeof(list));
-	split_merge_strategies(string, &list, &list_nr, &list_alloc);
-	if (list) {
-		for (i = 0; i < list_nr; i++)
-			append_strategy(get_strategy(list[i].name));
+	int i;
+
+	if (string) {
+		struct string_list list = STRING_LIST_INIT_DUP;
+		struct string_list_item *item;
+		string_list_split(&list, string, ' ', -1);
+		for_each_string_list_item(item, &list)
+			append_strategy(get_strategy(item->string));
+		string_list_clear(&list, 0);
 		return;
 	}
 	for (i = 0; i < ARRAY_SIZE(all_strategy); i++)
Signature · 1 line
2.9.2
#1 lore raw

Hi René,

Quoted 11 lines · René Scharfe

static void add_strategies(const char *string, unsigned attr) {

builtin/merge.c +1 -8
-	struct strategy *list = NULL;
-	int list_alloc = 0, list_nr = 0, i;
-
-	memset(&list, 0, sizeof(list));
-	split_merge_strategies(string, &list, &list_nr, &list_alloc);
-	if (list) {
-		for (i = 0; i < list_nr; i++)
-			append_strategy(get_strategy(list[i].name));
+	int i;

builtin/merge.c static int count_unmerged_entries(void) · line 719

Quoted 10 lines
builtin/merge.c +8 -0
+
+	if (string) {
+		struct string_list list = STRING_LIST_INIT_DUP;
+		struct string_list_item *item;
+		string_list_split(&list, string, ' ', -1);
+		for_each_string_list_item(item, &list)
+			append_strategy(get_strategy(item->string));
+		string_list_clear(&list, 0);
 		return;
 	}

A nice code reduction!

I wonder, however, if we could somhow turn things around by introducing something like

split_and_do_for_each(item_p, length, string, delimiter)
... <do something with item_p and length> ...

that both string_list_split() *and* add_strategies() could use? We would then be able to avoid allocating the list and duplicating the items in the latter case.

Ciao,
Dscho

#2 lore raw
Quoted 9 lines · Johannes Schindelin

I wonder, however, if we could somhow turn things around by introducing something like

split_and_do_for_each(item_p, length, string, delimiter)
... <do something with item_p and length> ...

that both string_list_split() *and* add_strategies() could use? We would then be able to avoid allocating the list and duplicating the items in the latter case.

I do think such a feature may be useful if we often work on pieces of a string delimited by a delimiter, but if the caller does not see the split result, then the function with "split" is probably misnamed.

I however suspect the variant of this where "delimiter" can just be a single byte would not be so useful.

If the input comes from the end user, we certainly would want to allow "word1 word2\tword3 " as input (i.e. squishing repeated delimiters into one without introducing an "empty" element, allowing more than one delimiter characters like SP and HT, and ignoring leading or trailing runs of delimiter characters).

If the input is generated internally, perhaps we should rethink the interface between the function that wants to do the for-each-word and its caller; if the caller wants to pass multiple things to the callee, it should be able to do so without first having to stuff these multiple things into a single string, only to force the callee to use this helper to split them out into individual pieces.

#3 lore raw
Quoted 2 lines · Junio C Hamano

If the input comes from the end user, we certainly would want to allow "word1 word2\tword3 " as input (i.e. squishing repeated

Any intelligent reader may have guessed already, but before I stupidly told Emacs to refill the paragraph, the above example had two SPs between word1 and word2. Sorry for being sloppy.

Quoted 10 lines

delimiters into one without introducing an "empty" element, allowing more than one delimiter characters like SP and HT, and ignoring leading or trailing runs of delimiter characters).

If the input is generated internally, perhaps we should rethink the interface between the function that wants to do the for-each-word and its caller; if the caller wants to pass multiple things to the callee, it should be able to do so without first having to stuff these multiple things into a single string, only to force the callee to use this helper to split them out into individual pieces.

#4 lore raw
In reply to Junio C Hamano

Hi Junio,

Quoted 25 lines · Junio C Hamano
Quoted 9 lines · Johannes Schindelin

I wonder, however, if we could somhow turn things around by introducing something like

split_and_do_for_each(item_p, length, string, delimiter)
... <do something with item_p and length> ...

that both string_list_split() *and* add_strategies() could use? We would then be able to avoid allocating the list and duplicating the items in the latter case.

I do think such a feature may be useful if we often work on pieces of a string delimited by a delimiter, but if the caller does not see the split result, then the function with "split" is probably misnamed.

I however suspect the variant of this where "delimiter" can just be a single byte would not be so useful.

If the input comes from the end user, we certainly would want to allow "word1 word2\tword3 " as input (i.e. squishing repeated delimiters into one without introducing an "empty" element, allowing more than one delimiter characters like SP and HT, and ignoring leading or trailing runs of delimiter characters).

Quoted 6 lines

If the input is generated internally, perhaps we should rethink the interface between the function that wants to do the for-each-word and its caller; if the caller wants to pass multiple things to the callee, it should be able to do so without first having to stuff these multiple things into a single string, only to force the callee to use this helper to split them out into individual pieces.

All true, but I guess this type of complexity would really complexify René's patch too much, so I am comfortable with the patch as-is.

Ciao,
Dscho

#5 lore raw
Quoted 2 lines · Johannes Schindelin

All true, but I guess this type of complexity would really complexify René's patch too much, so I am comfortable with the patch as-is.

Yeah, good that we reached the same conclusion, as my point was that for_each_word() would not be all that useful.