{"thread":{"id":"31481","subject":"[PATCH 0/4] Add some string_list-related functions","startedAt":"2012-09-09T05:53:06Z","lastAt":"2012-09-11T01:01:25Z","messageCount":23,"participants":["Michael Haggerty","Junio C Hamano","Jeff King","Andreas Ericsson"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"198597","messageId":"1347169990-9279-1-git-send-email-mhagger@alum.mit.edu","threadId":"31481","inReplyTo":null,"subject":"[PATCH 0/4] Add some string_list-related functions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-09T05:53:06Z","receivedAt":"2012-09-09T05:53:06Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This patch series adds a few functions to the string_list API. They\nwill be used in two upcoming patch series. Unfortunately, both of the\nseries (which are otherwise logically independent) need the same\nfunction; therefore, I am submitting these string-list enhancements as\na separate series on which the other two can depend.\n\nThis patch series applies to current master.\n\nMichael Haggerty (4):\n Add a new function, string_list_split_in_place()\n Add a new function, filter_string_list()\n Add a new function, string_list_remove_duplicates()\n Add a function string_list_longest_prefix()\n\n .gitignore | 1 +\n Documentation/technical/api-string-list.txt | 32 ++++++++++\n Makefile | 1 +\n string-list.c | 77 ++++++++++++++++++++++++\n string-list.h | 41 +++++++++++++\n t/t0063-string-list.sh | 93 +++++++++++++++++++++++++++++\n test-string-list.c | 47 +++++++++++++++\n 7 files changed, 292 insertions(+)\n create mode 100755 t/t0063-string-list.sh\n create mode 100644 test-string-list.c\n\n-- \n1.7.11.3\n"},{"id":"198599","messageId":"1347169990-9279-2-git-send-email-mhagger@alum.mit.edu","threadId":"31481","inReplyTo":"1347169990-9279-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/4] Add a new function, string_list_split_in_place()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-09T05:53:07Z","receivedAt":"2012-09-09T05:53:07Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Split a string into a string_list on a separator character.\n\nThis is similar to the strbuf_split_*() functions except that it works\nwith the more powerful string_list interface. If strdup_strings is\nfalse, it reuses the memory from the input string (thereby needing no\nstring memory allocations, though of course allocations are still\nneeded for the string_list_items array).\n\nSigned-off-by: Michael Haggerty \n---\n\nIn the tests, I use here documents to specify the expected output. Is\nthis OK? (It is certainly convenient.)\n\n .gitignore | 1 +\n Documentation/technical/api-string-list.txt | 12 ++++++\n Makefile | 1 +\n string-list.c | 23 +++++++++++\n string-list.h | 19 +++++++++\n t/t0063-string-list.sh | 63 +++++++++++++++++++++++++++++\n test-string-list.c | 25 ++++++++++++\n 7 files changed, 144 insertions(+)\n create mode 100755 t/t0063-string-list.sh\n create mode 100644 test-string-list.c\n\ndiff --git a/.gitignore b/.gitignore\nindex bb5c91e..0ca7df8 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -193,6 +193,7 @@\n /test-run-command\n /test-sha1\n /test-sigchain\n+/test-string-list\n /test-subprocess\n /test-svn-fe\n /common-cmds.h\ndiff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt\nindex 5a0c14f..3b959a2 100644\n--- a/Documentation/technical/api-string-list.txt\n+++ b/Documentation/technical/api-string-list.txt\n@@ -124,6 +124,18 @@ counterpart for sorted lists, which performs a binary search.\n \tis set. The third parameter controls if the `util` pointer of the\n \titems should be freed or not.\n \n+`string_list_split_in_place`::\n+\n+\tSplit string into substrings on character delim and append the\n+\tsubstrings to a string_list. The delimiter characters in\n+\tstring are overwritten with NULs in the process. If maxsplit\n+\tis a positive integer, then split at most maxsplit times. If\n+\tlist.strdup_strings is not set, then the new string_list_items\n+\tpoint into string, which therefore must not be modified or\n+\tfreed while the string_list is in use. Return the number of\n+\tsubstrings appended to the list.\n+\n+\n Data structures\n ---------------\n \ndiff --git a/Makefile b/Makefile\nindex 66e8216..ebbb381 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -501,6 +501,7 @@ TEST_PROGRAMS_NEED_X += test-run-command\n TEST_PROGRAMS_NEED_X += test-scrap-cache-tree\n TEST_PROGRAMS_NEED_X += test-sha1\n TEST_PROGRAMS_NEED_X += test-sigchain\n+TEST_PROGRAMS_NEED_X += test-string-list\n TEST_PROGRAMS_NEED_X += test-subprocess\n TEST_PROGRAMS_NEED_X += test-svn-fe\n \ndiff --git a/string-list.c b/string-list.c\nindex d9810ab..110449c 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -194,3 +194,26 @@ void unsorted_string_list_delete_item(struct string_list *list, int i, int free_\n \tlist->items[i] = list->items[list->nr-1];\n \tlist->nr--;\n }\n+\n+int string_list_split_in_place(struct string_list *list, char *string,\n+\t\t\t int delim, int maxsplit)\n+{\n+\tint count = 0;\n+\tchar *p = string, *end;\n+\tfor (;;) {\n+\t\tcount++;\n+\t\tif (maxsplit > 0 && count > maxsplit) {\n+\t\t\tstring_list_append(list, p);\n+\t\t\treturn count;\n+\t\t}\n+\t\tend = strchr(p, delim);\n+\t\tif (end) {\n+\t\t\t*end = '\\0';\n+\t\t\tstring_list_append(list, p);\n+\t\t\tp = end + 1;\n+\t\t} else {\n+\t\t\tstring_list_append(list, p);\n+\t\t\treturn count;\n+\t\t}\n+\t}\n+}\ndiff --git a/string-list.h b/string-list.h\nindex 0684cb7..7e51d03 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -45,4 +45,23 @@ int unsorted_string_list_has_string(struct string_list *list, const char *string\n struct string_list_item *unsorted_string_list_lookup(struct string_list *list,\n \t\t\t\t\t\t const char *string);\n void unsorted_string_list_delete_item(struct string_list *list, int i, int free_util);\n+\n+/*\n+ * Split string into substrings on character delim and append the\n+ * substrings to list. The delimiter characters in string are\n+ * overwritten with NULs in the process. If maxsplit is a positive\n+ * integer, then split at most maxsplit times. If list.strdup_strings\n+ * is not set, then the new string_list_items point into string, which\n+ * therefore must not be modified or freed while the string_list\n+ * is in use. Return the number of substrings appended to list.\n+ *\n+ * Examples:\n+ * string_list_split_in_place(l, \"foo:bar:baz\", ':', -1) -> [\"foo\", \"bar\", \"baz\"]\n+ * string_list_split_in_place(l, \"foo:bar:baz\", ':', 1) -> [\"foo\", \"bar:baz\"]\n+ * string_list_split_in_place(l, \"foo:bar:\", ':', -1) -> [\"foo\", \"bar\", \"\"]\n+ * string_list_split_in_place(l, \"\", ':', -1) -> [\"\"]\n+ * string_list_split_in_place(l, \":\", ':', -1) -> [\"\", \"\"]\n+ */\n+int string_list_split_in_place(struct string_list *list, char *string,\n+\t\t\t int delim, int maxsplit);\n #endif /* STRING_LIST_H */\ndiff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh\nnew file mode 100755\nindex 0000000..0eede83\n--- /dev/null\n+++ b/t/t0063-string-list.sh\n@@ -0,0 +1,63 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2012 Michael Haggerty\n+#\n+\n+test_description='Test string list functionality'\n+\n+. ./test-lib.sh\n+\n+string_list_split_in_place() {\n+\tcat >split-expected &&\n+\ttest_expect_success \"split $1 $2 $3\" \"\n+\t\ttest-string-list split_in_place '$1' '$2' '$3' >split-actual &&\n+\t\ttest_cmp split-expected split-actual\n+\t\"\n+}\n+\n+string_list_split_in_place \"foo:bar:baz\" \":\" \"-1\" <\n---\n Documentation/technical/api-string-list.txt | 8 ++++++++\n string-list.c | 17 +++++++++++++++++\n string-list.h | 9 +++++++++\n 3 files changed, 34 insertions(+)\n\ndiff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt\nindex 3b959a2..15b8072 100644\n--- a/Documentation/technical/api-string-list.txt\n+++ b/Documentation/technical/api-string-list.txt\n@@ -60,6 +60,14 @@ Functions\n \n * General ones (works with sorted and unsorted lists as well)\n \n+`filter_string_list`::\n+\n+\tApply a function to each item in a list, retaining only the\n+\titems for which the function returns true. If free_util is\n+\ttrue, call free() on the util members of any items that have\n+\tto be deleted. Preserve the order of the items that are\n+\tretained.\n+\n `print_string_list`::\n \n \tDump a string_list to stdout, useful mainly for debugging purposes. It\ndiff --git a/string-list.c b/string-list.c\nindex 110449c..72610ce 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -102,6 +102,23 @@ int for_each_string_list(struct string_list *list,\n \treturn ret;\n }\n \n+void filter_string_list(struct string_list *list, int free_util,\n+\t\t\tstring_list_each_func_t fn, void *cb_data)\n+{\n+\tint src, dst = 0;\n+\tfor (src = 0; src < list->nr; src++) {\n+\t\tif (fn(&list->items[src], cb_data)) {\n+\t\t\tlist->items[dst++] = list->items[src];\n+\t\t} else {\n+\t\t\tif (list->strdup_strings)\n+\t\t\t\tfree(list->items[src].string);\n+\t\t\tif (free_util)\n+\t\t\t\tfree(list->items[src].util);\n+\t\t}\n+\t}\n+\tlist->nr = dst;\n+}\n+\n void string_list_clear(struct string_list *list, int free_util)\n {\n \tif (list->items) {\ndiff --git a/string-list.h b/string-list.h\nindex 7e51d03..84996aa 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -29,6 +29,15 @@ int for_each_string_list(struct string_list *list,\n #define for_each_string_list_item(item,list) \\\n \tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n \n+/*\n+ * Apply fn to each item in list, retaining only the ones for which\n+ * the function returns true. If free_util is true, call free() on\n+ * the util members of any items that have to be deleted. Preserve\n+ * the order of the items that are retained.\n+ */\n+void filter_string_list(struct string_list *list, int free_util,\n+\t\t\tstring_list_each_func_t fn, void *cb_data);\n+\n /* Use these functions only on sorted lists: */\n int string_list_has_string(const struct string_list *list, const char *string);\n int string_list_find_insert_index(const struct string_list *list, const char *string,\n-- \n1.7.11.3\n"},{"id":"198600","messageId":"1347169990-9279-4-git-send-email-mhagger@alum.mit.edu","threadId":"31481","inReplyTo":"1347169990-9279-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 3/4] Add a new function, string_list_remove_duplicates()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-09T05:53:09Z","receivedAt":"2012-09-09T05:53:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty \n---\n Documentation/technical/api-string-list.txt | 4 ++++\n string-list.c | 17 +++++++++++++++++\n string-list.h | 5 +++++\n 3 files changed, 26 insertions(+)\n\ndiff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt\nindex 15b8072..9206f8f 100644\n--- a/Documentation/technical/api-string-list.txt\n+++ b/Documentation/technical/api-string-list.txt\n@@ -104,6 +104,10 @@ write `string_list_insert(...)->util = ...;`.\n \tLook up a given string in the string_list, returning the containing\n \tstring_list_item. If the string is not found, NULL is returned.\n \n+`string_list_remove_duplicates`::\n+\n+\tRemove all but the first entry that has a given string value.\n+\n * Functions for unsorted lists only\n \n `string_list_append`::\ndiff --git a/string-list.c b/string-list.c\nindex 72610ce..bfef6cf 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -92,6 +92,23 @@ struct string_list_item *string_list_lookup(struct string_list *list, const char\n \treturn list->items + i;\n }\n \n+void string_list_remove_duplicates(struct string_list *list, int free_util)\n+{\n+\tif (list->nr > 1) {\n+\t\tint src, dst;\n+\t\tfor (src = dst = 1; src < list->nr; src++) {\n+\t\t\tif (!strcmp(list->items[dst - 1].string, list->items[src].string)) {\n+\t\t\t\tif (list->strdup_strings)\n+\t\t\t\t\tfree(list->items[src].string);\n+\t\t\t\tif (free_util)\n+\t\t\t\t\tfree(list->items[src].util);\n+\t\t\t} else\n+\t\t\t\tlist->items[dst++] = list->items[src];\n+\t\t}\n+\t\tlist->nr = dst;\n+\t}\n+}\n+\n int for_each_string_list(struct string_list *list,\n \t\t\t string_list_each_func_t fn, void *cb_data)\n {\ndiff --git a/string-list.h b/string-list.h\nindex 84996aa..c4dc659 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -38,6 +38,7 @@ int for_each_string_list(struct string_list *list,\n void filter_string_list(struct string_list *list, int free_util,\n \t\t\tstring_list_each_func_t fn, void *cb_data);\n \n+\n /* Use these functions only on sorted lists: */\n int string_list_has_string(const struct string_list *list, const char *string);\n int string_list_find_insert_index(const struct string_list *list, const char *string,\n@@ -47,6 +48,10 @@ struct string_list_item *string_list_insert_at_index(struct string_list *list,\n \t\t\t\t\t\t int insert_at, const char *string);\n struct string_list_item *string_list_lookup(struct string_list *list, const char *string);\n \n+/* Remove all but the first entry that has a given string value. */\n+void string_list_remove_duplicates(struct string_list *list, int free_util);\n+\n+\n /* Use these functions only on unsorted lists: */\n struct string_list_item *string_list_append(struct string_list *list, const char *string);\n void sort_string_list(struct string_list *list);\n-- \n1.7.11.3\n"},{"id":"198601","messageId":"1347169990-9279-5-git-send-email-mhagger@alum.mit.edu","threadId":"31481","inReplyTo":"1347169990-9279-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 4/4] Add a function string_list_longest_prefix()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-09T05:53:10Z","receivedAt":"2012-09-09T05:53:10Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty \n---\n Documentation/technical/api-string-list.txt | 8 ++++++++\n string-list.c | 20 +++++++++++++++++++\n string-list.h | 8 ++++++++\n t/t0063-string-list.sh | 30 +++++++++++++++++++++++++++++\n test-string-list.c | 22 +++++++++++++++++++++\n 5 files changed, 88 insertions(+)\n\ndiff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt\nindex 9206f8f..291ac4c 100644\n--- a/Documentation/technical/api-string-list.txt\n+++ b/Documentation/technical/api-string-list.txt\n@@ -68,6 +68,14 @@ Functions\n \tto be deleted. Preserve the order of the items that are\n \tretained.\n \n+`string_list_longest_prefix`::\n+\n+\tReturn the longest string within a string_list that is a\n+\tprefix (in the sense of prefixcmp()) of the specified string,\n+\tor NULL if no such prefix exists. This function does not\n+\trequire the string_list to be sorted (it does a linear\n+\tsearch).\n+\n `print_string_list`::\n \n \tDump a string_list to stdout, useful mainly for debugging purposes. It\ndiff --git a/string-list.c b/string-list.c\nindex bfef6cf..043f6c4 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -136,6 +136,26 @@ void filter_string_list(struct string_list *list, int free_util,\n \tlist->nr = dst;\n }\n \n+char *string_list_longest_prefix(const struct string_list *prefixes,\n+\t\t\t\t const char *string)\n+{\n+\tint i, max_len = -1;\n+\tchar *retval = NULL;\n+\n+\tfor (i = 0; i < prefixes->nr; i++) {\n+\t\tchar *prefix = prefixes->items[i].string;\n+\t\tif (!prefixcmp(string, prefix)) {\n+\t\t\tint len = strlen(prefix);\n+\t\t\tif (len > max_len) {\n+\t\t\t\tretval = prefix;\n+\t\t\t\tmax_len = len;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+\treturn retval;\n+}\n+\n void string_list_clear(struct string_list *list, int free_util)\n {\n \tif (list->items) {\ndiff --git a/string-list.h b/string-list.h\nindex c4dc659..680916c 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -38,6 +38,14 @@ int for_each_string_list(struct string_list *list,\n void filter_string_list(struct string_list *list, int free_util,\n \t\t\tstring_list_each_func_t fn, void *cb_data);\n \n+/*\n+ * Return the longest string in prefixes that is a prefix (in the\n+ * sense of prefixcmp()) of string, or NULL if no such prefix exists.\n+ * This function does not require the string_list to be sorted (it\n+ * does a linear search).\n+ */\n+char *string_list_longest_prefix(const struct string_list *prefixes, const char *string);\n+\n \n /* Use these functions only on sorted lists: */\n int string_list_has_string(const struct string_list *list, const char *string);\ndiff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh\nindex 0eede83..fa96eba 100755\n--- a/t/t0063-string-list.sh\n+++ b/t/t0063-string-list.sh\n@@ -15,6 +15,14 @@ string_list_split_in_place() {\n \t\"\n }\n \n+longest_prefix() {\n+\ttest \"$(test-string-list longest_prefix \"$1\" \"$2\")\" = \"$3\"\n+}\n+\n+no_longest_prefix() {\n+\ttest_must_fail test-string-list longest_prefix \"$1\" \"$2\"\n+}\n+\n string_list_split_in_place \"foo:bar:baz\" \":\" \"-1\" <|- */\n+\t\tstruct string_list prefixes = STRING_LIST_INIT_NODUP;\n+\t\tint retval;\n+\t\tchar *prefix_string = xstrdup(argv[2]);\n+\t\tchar *string = argv[3];\n+\t\tchar *match;\n+\n+\t\tif (strcmp(prefix_string, \"-\"))\n+\t\t\tstring_list_split_in_place(&prefixes, prefix_string, ':', -1);\n+\t\tmatch = string_list_longest_prefix(&prefixes, string);\n+\t\tif (match) {\n+\t\t\tprintf(\"%s\\n\", match);\n+\t\t\tretval = 0;\n+\t\t}\n+\t\telse\n+\t\t\tretval = 1;\n+\t\tstring_list_clear(&prefixes, 0);\n+\t\tfree(prefix_string);\n+\t\treturn retval;\n+\t}\n+\n \tfprintf(stderr, \"%s: unknown function name: %s\\n\", argv[0],\n \t\targv[1] ? argv[1] : \"(there was none)\");\n \treturn 1;\n-- \n1.7.11.3\n"},{"id":"198621","messageId":"7voblfsfmd.fsf@alter.siamese.dyndns.org","threadId":"31481","inReplyTo":"1347169990-9279-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 1/4] Add a new function, string_list_split_in_place()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-09T09:35:38Z","receivedAt":"2012-09-09T09:35:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Split a string into a string_list on a separator character.\n>\n> This is similar to the strbuf_split_*() functions except that it works\n> with the more powerful string_list interface. If strdup_strings is\n> false, it reuses the memory from the input string (thereby needing no\n> string memory allocations, though of course allocations are still\n> needed for the string_list_items array).\n>\n> Signed-off-by: Michael Haggerty \n> ---\n>\n> In the tests, I use here documents to specify the expected output. Is\n> this OK? (It is certainly convenient.)\n\nI offhand do not have an objection to that style, but it looked\nfunny to see the helper test function \"string_list_split_in_place\"\nnamed without \"test_\" prefix. Maybe it's just me.\n\n> diff --git a/.gitignore b/.gitignore\n> index bb5c91e..0ca7df8 100644\n> --- a/.gitignore\n> +++ b/.gitignore\n> @@ -193,6 +193,7 @@\n> /test-run-command\n> /test-sha1\n> /test-sigchain\n> +/test-string-list\n> /test-subprocess\n> /test-svn-fe\n> /common-cmds.h\n> diff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt\n> index 5a0c14f..3b959a2 100644\n> --- a/Documentation/technical/api-string-list.txt\n> +++ b/Documentation/technical/api-string-list.txt\n> @@ -124,6 +124,18 @@ counterpart for sorted lists, which performs a binary search.\n> \tis set. The third parameter controls if the `util` pointer of the\n> \titems should be freed or not.\n> \n> +`string_list_split_in_place`::\n> +\n> +\tSplit string into substrings on character delim and append the\n> +\tsubstrings to a string_list. The delimiter characters in\n> +\tstring are overwritten with NULs in the process. If maxsplit\n> +\tis a positive integer, then split at most maxsplit times. If\n\nSo passing 0 is the natural way to say \"pay attention to all the\ndelimiters\", which matches my intuition.\n\n> +\tlist.strdup_strings is not set, then the new string_list_items\n> +\tpoint into string, which therefore must not be modified or\n> +\tfreed while the string_list is in use. Return the number of\n> +\tsubstrings appended to the list.\n\nI am not sure about this strdup_strings business; it smells somewhat\nfishy from the API design point of view.\n\nIf you are not mucking with the input string and not splitting in\nplace, it would not be possible to do this without strdup_strings,\nbut if you are doing the in-place splitting, is there any reason for\nthe caller to ask for strdup_strings?\n\nIn such a case, the reason the caller cannot promise that the input\nstring will not go away to the string_list (hence it has to ask to\nmake a copy) is because it does not own the string in the first\nplace, and in such a case, I would imagine it cannot allow the\ndelimiters in the string to be overwritten with NULs.\n\nI would sort-of-kind-of understand a function \"string_list_split\"\nthat bases its decision to do an in-place splitting or not on the\nstrdup_strings flag in the string list that was passed in. But it\nwould make the use of the function a bit limited (e.g. you cannot\nsanely mix and match tokens from different kind of strings).\n\n> diff --git a/string-list.c b/string-list.c\n> index d9810ab..110449c 100644\n> --- a/string-list.c\n> +++ b/string-list.c\n> @@ -194,3 +194,26 @@ void unsorted_string_list_delete_item(struct string_list *list, int i, int free_\n> \tlist->items[i] = list->items[list->nr-1];\n> \tlist->nr--;\n> }\n> +\n> +int string_list_split_in_place(struct string_list *list, char *string,\n> +\t\t\t int delim, int maxsplit)\n> +{\n> +\tint count = 0;\n> +\tchar *p = string, *end;\n\nBlank line here.\n\n> +\tfor (;;) {\n> +\t\tcount++;\n> +\t\tif (maxsplit > 0 && count > maxsplit) {\n> +\t\t\tstring_list_append(list, p);\n> +\t\t\treturn count;\n> +\t\t}\n> +\t\tend = strchr(p, delim);\n> +\t\tif (end) {\n> +\t\t\t*end = '\\0';\n> +\t\t\tstring_list_append(list, p);\n> +\t\t\tp = end + 1;\n> +\t\t} else {\n> +\t\t\tstring_list_append(list, p);\n> +\t\t\treturn count;\n> +\t\t}\n> +\t}\n> +}\n> diff --git a/string-list.h b/string-list.h\n> index 0684cb7..7e51d03 100644\n> --- a/string-list.h\n> +++ b/string-list.h\n> @@ -45,4 +45,23 @@ int unsorted_string_list_has_string(struct string_list *list, const char *string\n> struct string_list_item *unsorted_string_list_lookup(struct string_list *list,\n> \t\t\t\t\t\t const char *string);\n> void unsorted_string_list_delete_item(struct string_list *list, int i, int free_util);\n> +\n> +/*\n> + * Split string into substrings on character delim and append the\n> + * substrings to list. The delimiter characters in string are\n> + * overwritten with NULs in the process. If maxsplit is a positive\n> + * integer, then split at most maxsplit times. If list.strdup_strings\n> + * is not set, then the new string_list_items point into string, which\n> + * therefore must not be modified or freed while the string_list\n> + * is in use. Return the number of substrings appended to list.\n> + *\n> + * Examples:\n> + * string_list_split_in_place(l, \"foo:bar:baz\", ':', -1) -> [\"foo\", \"bar\", \"baz\"]\n> + * string_list_split_in_place(l, \"foo:bar:baz\", ':', 1) -> [\"foo\", \"bar:baz\"]\n> + * string_list_split_in_place(l, \"foo:bar:\", ':', -1) -> [\"foo\", \"bar\", \"\"]\n\nI would find it more natural to see a sentinel value against\n\"positive\" to be 0, not -1. \"-1\" gives an impression as if \"-2\"\nmight do something different from \"-1\", but Zero is a lot more\nspecial.\n\n> diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh\n> new file mode 100755\n> index 0000000..0eede83\n> --- /dev/null\n> +++ b/t/t0063-string-list.sh\n> @@ -0,0 +1,63 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (c) 2012 Michael Haggerty\n> +#\n> +\n> +test_description='Test string list functionality'\n> +\n> +. ./test-lib.sh\n> +\n> +string_list_split_in_place() {\n\nSP before ()?\n\n> +\tcat >split-expected &&\n> +\ttest_expect_success \"split $1 $2 $3\" \"\n\n\"split '$1' at '$2', max $3\", or something?\n\n> +\t\ttest-string-list split_in_place '$1' '$2' '$3' >split-actual &&\n> +\t\ttest_cmp split-expected split-actual\n> +\t\"\n> +}\n\nWhat is it buying us to use \"split-\" before expected and actual to\nmake things \"unusual\" from many other tests?\n\n> +string_list_split_in_place \"foo:bar:baz\" \":\" \"-1\" < +3\n> +[0]: \"foo\"\n> +[1]: \"bar\"\n> +[2]: \"baz\"\n> +EOF\n> +\n> +string_list_split_in_place \"foo:bar:baz\" \":\" \"0\" < +3\n> +[0]: \"foo\"\n> +[1]: \"bar\"\n> +[2]: \"baz\"\n> +EOF\n> +\n> +string_list_split_in_place \"foo:bar:baz\" \":\" \"1\" < +2\n> +[0]: \"foo\"\n> +[1]: \"bar:baz\"\n> +EOF\n> +\n> +string_list_split_in_place \"foo:bar:baz\" \":\" \"2\" < +3\n> +[0]: \"foo\"\n> +[1]: \"bar\"\n> +[2]: \"baz\"\n> +EOF\n> +\n> +string_list_split_in_place \"foo:bar:\" \":\" \"-1\" < +3\n> +[0]: \"foo\"\n> +[1]: \"bar\"\n> +[2]: \"\"\n> +EOF\n> +\n> +string_list_split_in_place \"\" \":\" \"-1\" < +1\n> +[0]: \"\"\n> +EOF\n> +\n> +string_list_split_in_place \":\" \":\" \"-1\" < +2\n> +[0]: \"\"\n> +[1]: \"\"\n> +EOF\n> +\n> +test_done\n> diff --git a/test-string-list.c b/test-string-list.c\n> new file mode 100644\n> index 0000000..f08d3cc\n> --- /dev/null\n> +++ b/test-string-list.c\n> @@ -0,0 +1,25 @@\n> +#include \"cache.h\"\n> +#include \"string-list.h\"\n> +\n> +int main(int argc, char **argv)\n> +{\n> +\tif ((argc == 4 || argc == 5) && !strcmp(argv[1], \"split_in_place\")) {\n> +\t\tstruct string_list list = STRING_LIST_INIT_NODUP;\n> +\t\tint i;\n> +\t\tchar *s = xstrdup(argv[2]);\n> +\t\tint delim = *argv[3];\n> +\t\tint maxsplit = (argc == 5) ? atoi(argv[4]) : -1;\n\nLikewise on \"-1\".\n\n> +\t\ti = string_list_split_in_place(&list, s, delim, maxsplit);\n> +\t\tprintf(\"%d\\n\", i);\n> +\t\tfor (i = 0; i < list.nr; i++)\n> +\t\t\tprintf(\"[%d]: \\\"%s\\\"\\n\", i, list.items[i].string);\n> +\t\tstring_list_clear(&list, 0);\n> +\t\tfree(s);\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tfprintf(stderr, \"%s: unknown function name: %s\\n\", argv[0],\n> +\t\targv[1] ? argv[1] : \"(there was none)\");\n> +\treturn 1;\n> +}\n"},{"id":"198622","messageId":"7vk3w3sfe9.fsf@alter.siamese.dyndns.org","threadId":"31481","inReplyTo":"1347169990-9279-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 2/4] Add a new function, filter_string_list()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-09T09:40:30Z","receivedAt":"2012-09-09T09:40:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Signed-off-by: Michael Haggerty \n> ---\n> Documentation/technical/api-string-list.txt | 8 ++++++++\n> string-list.c | 17 +++++++++++++++++\n> string-list.h | 9 +++++++++\n> 3 files changed, 34 insertions(+)\n>\n> diff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt\n> index 3b959a2..15b8072 100644\n> --- a/Documentation/technical/api-string-list.txt\n> +++ b/Documentation/technical/api-string-list.txt\n> @@ -60,6 +60,14 @@ Functions\n> \n> * General ones (works with sorted and unsorted lists as well)\n> \n> +`filter_string_list`::\n> +\n> +\tApply a function to each item in a list, retaining only the\n> +\titems for which the function returns true. If free_util is\n> +\ttrue, call free() on the util members of any items that have\n> +\tto be deleted. Preserve the order of the items that are\n> +\tretained.\n\nIn other words, this can safely be used on both sorted and unsorted\nstring list. Good.\n\n> `print_string_list`::\n> \n> \tDump a string_list to stdout, useful mainly for debugging purposes. It\n> diff --git a/string-list.c b/string-list.c\n> index 110449c..72610ce 100644\n> --- a/string-list.c\n> +++ b/string-list.c\n> @@ -102,6 +102,23 @@ int for_each_string_list(struct string_list *list,\n> \treturn ret;\n> }\n> \n> +void filter_string_list(struct string_list *list, int free_util,\n> +\t\t\tstring_list_each_func_t fn, void *cb_data)\n> +{\n> +\tint src, dst = 0;\n> +\tfor (src = 0; src < list->nr; src++) {\n> +\t\tif (fn(&list->items[src], cb_data)) {\n> +\t\t\tlist->items[dst++] = list->items[src];\n> +\t\t} else {\n> +\t\t\tif (list->strdup_strings)\n> +\t\t\t\tfree(list->items[src].string);\n> +\t\t\tif (free_util)\n> +\t\t\t\tfree(list->items[src].util);\n> +\t\t}\n> +\t}\n> +\tlist->nr = dst;\n> +}\n> +\n> void string_list_clear(struct string_list *list, int free_util)\n> {\n> \tif (list->items) {\n> diff --git a/string-list.h b/string-list.h\n> index 7e51d03..84996aa 100644\n> --- a/string-list.h\n> +++ b/string-list.h\n> @@ -29,6 +29,15 @@ int for_each_string_list(struct string_list *list,\n> #define for_each_string_list_item(item,list) \\\n> \tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n> \n> +/*\n> + * Apply fn to each item in list, retaining only the ones for which\n> + * the function returns true. If free_util is true, call free() on\n> + * the util members of any items that have to be deleted. Preserve\n> + * the order of the items that are retained.\n> + */\n> +void filter_string_list(struct string_list *list, int free_util,\n> +\t\t\tstring_list_each_func_t fn, void *cb_data);\n> +\n> /* Use these functions only on sorted lists: */\n> int string_list_has_string(const struct string_list *list, const char *string);\n> int string_list_find_insert_index(const struct string_list *list, const char *string,\n\nHaving seen that the previous patch introduced a new test helper for\nunit testing (which is a very good idea) and dedicated a new test\nnumber, I would have expected to see a new test for filtering\nhere.\n"},{"id":"198623","messageId":"7vfw6rsf64.fsf@alter.siamese.dyndns.org","threadId":"31481","inReplyTo":"1347169990-9279-4-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 3/4] Add a new function, string_list_remove_duplicates()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-09T09:45:23Z","receivedAt":"2012-09-09T09:45:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Signed-off-by: Michael Haggerty \n> ---\n> Documentation/technical/api-string-list.txt | 4 ++++\n> string-list.c | 17 +++++++++++++++++\n> string-list.h | 5 +++++\n> 3 files changed, 26 insertions(+)\n>\n> diff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt\n> index 15b8072..9206f8f 100644\n> --- a/Documentation/technical/api-string-list.txt\n> +++ b/Documentation/technical/api-string-list.txt\n> @@ -104,6 +104,10 @@ write `string_list_insert(...)->util = ...;`.\n> \tLook up a given string in the string_list, returning the containing\n> \tstring_list_item. If the string is not found, NULL is returned.\n> \n> +`string_list_remove_duplicates`::\n> +\n> +\tRemove all but the first entry that has a given string value.\n\nUnlike the previous patch, free_util is not documented?\n\nIt is kind of shame that the string list must be sorted for this\nfunction to work, but I guess you do not have a need for a version\nthat works on both sorted and unsorted (yet). We can introduce a\nvariant with \"unsorted_\" prefix later when it becomes necessary, so\nOK.\n\n> * Functions for unsorted lists only\n> \n> `string_list_append`::\n> diff --git a/string-list.c b/string-list.c\n> index 72610ce..bfef6cf 100644\n> --- a/string-list.c\n> +++ b/string-list.c\n> @@ -92,6 +92,23 @@ struct string_list_item *string_list_lookup(struct string_list *list, const char\n> \treturn list->items + i;\n> }\n> \n> +void string_list_remove_duplicates(struct string_list *list, int free_util)\n> +{\n> +\tif (list->nr > 1) {\n> +\t\tint src, dst;\n> +\t\tfor (src = dst = 1; src < list->nr; src++) {\n> +\t\t\tif (!strcmp(list->items[dst - 1].string, list->items[src].string)) {\n> +\t\t\t\tif (list->strdup_strings)\n> +\t\t\t\t\tfree(list->items[src].string);\n> +\t\t\t\tif (free_util)\n> +\t\t\t\t\tfree(list->items[src].util);\n> +\t\t\t} else\n> +\t\t\t\tlist->items[dst++] = list->items[src];\n> +\t\t}\n> +\t\tlist->nr = dst;\n> +\t}\n> +}\n> +\n> int for_each_string_list(struct string_list *list,\n> \t\t\t string_list_each_func_t fn, void *cb_data)\n> {\n> diff --git a/string-list.h b/string-list.h\n> index 84996aa..c4dc659 100644\n> --- a/string-list.h\n> +++ b/string-list.h\n> @@ -38,6 +38,7 @@ int for_each_string_list(struct string_list *list,\n> void filter_string_list(struct string_list *list, int free_util,\n> \t\t\tstring_list_each_func_t fn, void *cb_data);\n> \n> +\n> /* Use these functions only on sorted lists: */\n> int string_list_has_string(const struct string_list *list, const char *string);\n> int string_list_find_insert_index(const struct string_list *list, const char *string,\n> @@ -47,6 +48,10 @@ struct string_list_item *string_list_insert_at_index(struct string_list *list,\n> \t\t\t\t\t\t int insert_at, const char *string);\n> struct string_list_item *string_list_lookup(struct string_list *list, const char *string);\n> \n> +/* Remove all but the first entry that has a given string value. */\n> +void string_list_remove_duplicates(struct string_list *list, int free_util);\n> +\n> +\n> /* Use these functions only on unsorted lists: */\n> struct string_list_item *string_list_append(struct string_list *list, const char *string);\n> void sort_string_list(struct string_list *list);\n"},{"id":"198624","messageId":"7vbohfser4.fsf@alter.siamese.dyndns.org","threadId":"31481","inReplyTo":"1347169990-9279-5-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 4/4] Add a function string_list_longest_prefix()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-09T09:54:23Z","receivedAt":"2012-09-09T09:54:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Signed-off-by: Michael Haggerty \n> ---\n> Documentation/technical/api-string-list.txt | 8 ++++++++\n> string-list.c | 20 +++++++++++++++++++\n> string-list.h | 8 ++++++++\n> t/t0063-string-list.sh | 30 +++++++++++++++++++++++++++++\n> test-string-list.c | 22 +++++++++++++++++++++\n> 5 files changed, 88 insertions(+)\n>\n> diff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt\n> index 9206f8f..291ac4c 100644\n> --- a/Documentation/technical/api-string-list.txt\n> +++ b/Documentation/technical/api-string-list.txt\n> @@ -68,6 +68,14 @@ Functions\n> \tto be deleted. Preserve the order of the items that are\n> \tretained.\n> \n> +`string_list_longest_prefix`::\n> +\n> +\tReturn the longest string within a string_list that is a\n> +\tprefix (in the sense of prefixcmp()) of the specified string,\n> +\tor NULL if no such prefix exists. This function does not\n> +\trequire the string_list to be sorted (it does a linear\n> +\tsearch).\n> +\n> `print_string_list`::\n\nThis may feel like outside the scope of this series, but since this\nseries will be the main culprit for adding many new functions to\nthis API in the recent history...\n\n - We may want to name things a bit more consistently so that people\n can tell which ones can be called on any string list, which ones\n are sorted list only, and which ones are unsorted one only.\n\n In addition, the last category _may_ need a bit more thought.\n Calling unsorted_string_list_lookup() on an already sorted list\n is not a crime---it is just a stupid thing to do.\n\n - Why are these new functions described at the top, not appended at\n the bottom? I would have expected either an alphabetical, or a\n more generic ones first (i.e. print and clear are a lot \"easier\"\n ones compared to filter and prefix that are very much more\n specialized).\n\n> diff --git a/string-list.c b/string-list.c\n> index bfef6cf..043f6c4 100644\n> --- a/string-list.c\n> +++ b/string-list.c\n> @@ -136,6 +136,26 @@ void filter_string_list(struct string_list *list, int free_util,\n> \tlist->nr = dst;\n> }\n> \n> +char *string_list_longest_prefix(const struct string_list *prefixes,\n> +\t\t\t\t const char *string)\n> +{\n> +\tint i, max_len = -1;\n> +\tchar *retval = NULL;\n> +\n> +\tfor (i = 0; i < prefixes->nr; i++) {\n> +\t\tchar *prefix = prefixes->items[i].string;\n> +\t\tif (!prefixcmp(string, prefix)) {\n> +\t\t\tint len = strlen(prefix);\n> +\t\t\tif (len > max_len) {\n> +\t\t\t\tretval = prefix;\n> +\t\t\t\tmax_len = len;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\treturn retval;\n> +}\n> +\n> void string_list_clear(struct string_list *list, int free_util)\n> {\n> \tif (list->items) {\n> diff --git a/string-list.h b/string-list.h\n> index c4dc659..680916c 100644\n> --- a/string-list.h\n> +++ b/string-list.h\n> @@ -38,6 +38,14 @@ int for_each_string_list(struct string_list *list,\n> void filter_string_list(struct string_list *list, int free_util,\n> \t\t\tstring_list_each_func_t fn, void *cb_data);\n> \n> +/*\n> + * Return the longest string in prefixes that is a prefix (in the\n> + * sense of prefixcmp()) of string, or NULL if no such prefix exists.\n> + * This function does not require the string_list to be sorted (it\n> + * does a linear search).\n> + */\n> +char *string_list_longest_prefix(const struct string_list *prefixes, const char *string);\n> +\n> \n> /* Use these functions only on sorted lists: */\n> int string_list_has_string(const struct string_list *list, const char *string);\n> diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh\n> index 0eede83..fa96eba 100755\n> --- a/t/t0063-string-list.sh\n> +++ b/t/t0063-string-list.sh\n> @@ -15,6 +15,14 @@ string_list_split_in_place() {\n> \t\"\n> }\n> \n> +longest_prefix() {\n> +\ttest \"$(test-string-list longest_prefix \"$1\" \"$2\")\" = \"$3\"\n> +}\n> +\n> +no_longest_prefix() {\n> +\ttest_must_fail test-string-list longest_prefix \"$1\" \"$2\"\n> +}\n> +\n> string_list_split_in_place \"foo:bar:baz\" \":\" \"-1\" < 3\n> [0]: \"foo\"\n> @@ -60,4 +68,26 @@ string_list_split_in_place \":\" \":\" \"-1\" < [1]: \"\"\n> EOF\n> \n> +test_expect_success \"test longest_prefix\" '\n> +\tno_longest_prefix - '' &&\n> +\tno_longest_prefix - x &&\n> +\tlongest_prefix \"\" x \"\" &&\n> +\tlongest_prefix x x x &&\n> +\tlongest_prefix \"\" foo \"\" &&\n> +\tlongest_prefix : foo \"\" &&\n> +\tlongest_prefix f foo f &&\n> +\tlongest_prefix foo foobar foo &&\n> +\tlongest_prefix foo foo foo &&\n> +\tno_longest_prefix bar foo &&\n> +\tno_longest_prefix bar:bar foo &&\n> +\tno_longest_prefix foobar foo &&\n> +\tlongest_prefix foo:bar foo foo &&\n> +\tlongest_prefix foo:bar bar bar &&\n> +\tlongest_prefix foo::bar foo foo &&\n> +\tlongest_prefix foo:foobar foo foo &&\n> +\tlongest_prefix foobar:foo foo foo &&\n> +\tlongest_prefix foo: bar \"\" &&\n> +\tlongest_prefix :foo bar \"\"\n> +'\n> +\n> test_done\n> diff --git a/test-string-list.c b/test-string-list.c\n> index f08d3cc..c7e71f2 100644\n> --- a/test-string-list.c\n> +++ b/test-string-list.c\n> @@ -19,6 +19,28 @@ int main(int argc, char **argv)\n> \t\treturn 0;\n> \t}\n> \n> +\tif (argc == 4 && !strcmp(argv[1], \"longest_prefix\")) {\n> +\t\t/* arguments: |- */\n> +\t\tstruct string_list prefixes = STRING_LIST_INIT_NODUP;\n> +\t\tint retval;\n> +\t\tchar *prefix_string = xstrdup(argv[2]);\n> +\t\tchar *string = argv[3];\n> +\t\tchar *match;\n> +\n> +\t\tif (strcmp(prefix_string, \"-\"))\n> +\t\t\tstring_list_split_in_place(&prefixes, prefix_string, ':', -1);\n> +\t\tmatch = string_list_longest_prefix(&prefixes, string);\n> +\t\tif (match) {\n> +\t\t\tprintf(\"%s\\n\", match);\n> +\t\t\tretval = 0;\n> +\t\t}\n> +\t\telse\n> +\t\t\tretval = 1;\n> +\t\tstring_list_clear(&prefixes, 0);\n> +\t\tfree(prefix_string);\n> +\t\treturn retval;\n> +\t}\n> +\n> \tfprintf(stderr, \"%s: unknown function name: %s\\n\", argv[0],\n> \t\targv[1] ? argv[1] : \"(there was none)\");\n> \treturn 1;\n"},{"id":"198657","messageId":"504D7082.9020903@alum.mit.edu","threadId":"31481","inReplyTo":"7voblfsfmd.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/4] Add a new function, string_list_split_in_place()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-10T04:45:54Z","receivedAt":"2012-09-10T04:45:54Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/09/2012 11:35 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> Split a string into a string_list on a separator character.\n>>\n>> This is similar to the strbuf_split_*() functions except that it works\n>> with the more powerful string_list interface. If strdup_strings is\n>> false, it reuses the memory from the input string (thereby needing no\n>> string memory allocations, though of course allocations are still\n>> needed for the string_list_items array).\n>>\n>> Signed-off-by: Michael Haggerty \n>> ---\n>>\n>> In the tests, I use here documents to specify the expected output. Is\n>> this OK? (It is certainly convenient.)\n> \n> I offhand do not have an objection to that style, but it looked\n> funny to see the helper test function \"string_list_split_in_place\"\n> named without \"test_\" prefix. Maybe it's just me.\n\nI renamed the test function to \"test_split_in_place\". (The\n\"string_list\" part can be dispensed with in a test called\nt0063-string-list.sh, I think.)\n\n>> diff --git a/.gitignore b/.gitignore\n>> index bb5c91e..0ca7df8 100644\n>> --- a/.gitignore\n>> +++ b/.gitignore\n>> @@ -193,6 +193,7 @@\n>> /test-run-command\n>> /test-sha1\n>> /test-sigchain\n>> +/test-string-list\n>> /test-subprocess\n>> /test-svn-fe\n>> /common-cmds.h\n>> diff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt\n>> index 5a0c14f..3b959a2 100644\n>> --- a/Documentation/technical/api-string-list.txt\n>> +++ b/Documentation/technical/api-string-list.txt\n>> @@ -124,6 +124,18 @@ counterpart for sorted lists, which performs a binary search.\n>> \tis set. The third parameter controls if the `util` pointer of the\n>> \titems should be freed or not.\n>> \n>> +`string_list_split_in_place`::\n>> +\n>> +\tSplit string into substrings on character delim and append the\n>> +\tsubstrings to a string_list. The delimiter characters in\n>> +\tstring are overwritten with NULs in the process. If maxsplit\n>> +\tis a positive integer, then split at most maxsplit times. If\n> \n> So passing 0 is the natural way to say \"pay attention to all the\n> delimiters\", which matches my intuition.\n\nSee below.\n\n>> +\tlist.strdup_strings is not set, then the new string_list_items\n>> +\tpoint into string, which therefore must not be modified or\n>> +\tfreed while the string_list is in use. Return the number of\n>> +\tsubstrings appended to the list.\n> \n> I am not sure about this strdup_strings business; it smells somewhat\n> fishy from the API design point of view.\n> \n> If you are not mucking with the input string and not splitting in\n> place, it would not be possible to do this without strdup_strings,\n> but if you are doing the in-place splitting, is there any reason for\n> the caller to ask for strdup_strings?\n>\n> In such a case, the reason the caller cannot promise that the input\n> string will not go away to the string_list (hence it has to ask to\n> make a copy) is because it does not own the string in the first\n> place, and in such a case, I would imagine it cannot allow the\n> delimiters in the string to be overwritten with NULs.\n\nOn one level it is immaterial whether this is a sensible usage; the\ncaller *can* set strdup_strings on any string_list and pass that\nstring_list into the function. In that case the function *has* to\nstrdup the strings because when strdup_strings is set, the string_list\nfunctions are allowed to free() a string whenever they want.\n\nBut there are situations when this usage is also convenient; namely when\nthe lifetime of the string being split is shorter than the lifetime of\nthe string_list. Consider something like\n\n struct string_list *split_file_into_words(FILE *f)\n {\n char buf[1024];\n struct string_list *list = new string list;\n list->strdup_strings = 1;\n while (not EOF) {\n read_line_into_buf();\n string_list_split_in_place(list, buf, ' ', -1);\n }\n return list;\n }\n\nConsider also that the string_list passed to\nstring_list_split_in_place() might already have contents from elsewhere.\n Since a string_list has a single allocation policy covering all of its\nentries, it might be more convenient for the caller to allow the split\nstrings to be copied than to change the allocation policy of the\npre-inserted strings.\n\n> I would sort-of-kind-of understand a function \"string_list_split\"\n> that bases its decision to do an in-place splitting or not on the\n> strdup_strings flag in the string list that was passed in. But it\n> would make the use of the function a bit limited (e.g. you cannot\n> sanely mix and match tokens from different kind of strings).\n\nHere is my thinking:\n\nI want to have a very low overhead way to use string_list, without a\nmemory allocation per string (so that people don't have an excuse to\navoid the API). Thus the \"in-place\" option.\n\nIt would be easy (and have no run-time cost) to change the split\nfunction to *not* modify the input string if strdup_strings is set on\nthe string_list. But (1) I don't think it is a good idea for the\nhandling of the string argument to depend on an option buried in another\nparameters, and (2) the string argument could not be declared \"const\",\nso many situations when this variant were useful would require const to\nbe cast away.\n\nSo if/when such a need arises, I think it would be better to invent a\nnew string_list_split() variant that *never* overwrites its input string\n(though this function could *only* work correctly if strdup_strings is set).\n\n>> diff --git a/string-list.c b/string-list.c\n>> index d9810ab..110449c 100644\n>> --- a/string-list.c\n>> +++ b/string-list.c\n>> @@ -194,3 +194,26 @@ void unsorted_string_list_delete_item(struct string_list *list, int i, int free_\n>> \tlist->items[i] = list->items[list->nr-1];\n>> \tlist->nr--;\n>> }\n>> +\n>> +int string_list_split_in_place(struct string_list *list, char *string,\n>> +\t\t\t int delim, int maxsplit)\n>> +{\n>> +\tint count = 0;\n>> +\tchar *p = string, *end;\n> \n> Blank line here.\n\nThanks.\n\n>> +\tfor (;;) {\n>> +\t\tcount++;\n>> +\t\tif (maxsplit > 0 && count > maxsplit) {\n>> +\t\t\tstring_list_append(list, p);\n>> +\t\t\treturn count;\n>> +\t\t}\n>> +\t\tend = strchr(p, delim);\n>> +\t\tif (end) {\n>> +\t\t\t*end = '\\0';\n>> +\t\t\tstring_list_append(list, p);\n>> +\t\t\tp = end + 1;\n>> +\t\t} else {\n>> +\t\t\tstring_list_append(list, p);\n>> +\t\t\treturn count;\n>> +\t\t}\n>> +\t}\n>> +}\n>> diff --git a/string-list.h b/string-list.h\n>> index 0684cb7..7e51d03 100644\n>> --- a/string-list.h\n>> +++ b/string-list.h\n>> @@ -45,4 +45,23 @@ int unsorted_string_list_has_string(struct string_list *list, const char *string\n>> struct string_list_item *unsorted_string_list_lookup(struct string_list *list,\n>> \t\t\t\t\t\t const char *string);\n>> void unsorted_string_list_delete_item(struct string_list *list, int i, int free_util);\n>> +\n>> +/*\n>> + * Split string into substrings on character delim and append the\n>> + * substrings to list. The delimiter characters in string are\n>> + * overwritten with NULs in the process. If maxsplit is a positive\n>> + * integer, then split at most maxsplit times. If list.strdup_strings\n>> + * is not set, then the new string_list_items point into string, which\n>> + * therefore must not be modified or freed while the string_list\n>> + * is in use. Return the number of substrings appended to list.\n>> + *\n>> + * Examples:\n>> + * string_list_split_in_place(l, \"foo:bar:baz\", ':', -1) -> [\"foo\", \"bar\", \"baz\"]\n>> + * string_list_split_in_place(l, \"foo:bar:baz\", ':', 1) -> [\"foo\", \"bar:baz\"]\n>> + * string_list_split_in_place(l, \"foo:bar:\", ':', -1) -> [\"foo\", \"bar\", \"\"]\n> \n> I would find it more natural to see a sentinel value against\n> \"positive\" to be 0, not -1. \"-1\" gives an impression as if \"-2\"\n> might do something different from \"-1\", but Zero is a lot more\n> special.\n\nYou have raised a good point and I think there is a flaw in the API, but\nI'm not sure I agree with you what the flaw is...\n\nThe \"maxsplit\" argument limits the number of times the string should be\nsplit. I.e., if maxsplit is set, then the output will have at most\n(maxsplit + 1) strings.\n\nWhen I used \"-1\" as the special value for \"unlimited\", I was\nsubconsciously thinking that \"0\" is an imaginable edge case; it could\nmean \"don't split string at all\"; i.e., return the whole input as the\nsingle string in the output string_list. This would be a kindof silly\nusage, but might perhaps remove the need to handle 0 specially at a\ncaller if \"maxsplit\" is derived from another source.\n\nBut of course my definition of string_list_split_in_place() *doesn't*\ntreat 0 this way. I think *that* was my mistake. So I propose to\nchange the meaning of maxsplit to\n\n If maxsplit is a non-negative integer, then split at most maxsplit\n times.\n\nand to continue using -1 as the \"special\" value for unlimited splits.\nAre you OK with that?\n\n>> diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh\n>> new file mode 100755\n>> index 0000000..0eede83\n>> --- /dev/null\n>> +++ b/t/t0063-string-list.sh\n>> @@ -0,0 +1,63 @@\n>> +#!/bin/sh\n>> +#\n>> +# Copyright (c) 2012 Michael Haggerty\n>> +#\n>> +\n>> +test_description='Test string list functionality'\n>> +\n>> +. ./test-lib.sh\n>> +\n>> +string_list_split_in_place() {\n> \n> SP before ()?\n\nThanks.\n\n>> +\tcat >split-expected &&\n>> +\ttest_expect_success \"split $1 $2 $3\" \"\n> \n> \"split '$1' at '$2', max $3\", or something?\n\nGood idea.\n\n>> +\t\ttest-string-list split_in_place '$1' '$2' '$3' >split-actual &&\n>> +\t\ttest_cmp split-expected split-actual\n>> +\t\"\n>> +}\n> \n> What is it buying us to use \"split-\" before expected and actual to\n> make things \"unusual\" from many other tests?\n\nNothing, I guess. Changed.\n\n>> +string_list_split_in_place \"foo:bar:baz\" \":\" \"-1\" < \n> Likewise about \"-1\" (I think your test helper does not even require\n> \"-1\" to be spelled out here).\n\nSince we're testing the C API I think it's best that the tests be\nexplicit about the value to be passed to the function.\n\nTherefore I guess I will change the test program to *require* the\nmaxsplit option, since testing is its only purpose.\n\n>> +3\n>> +[0]: \"foo\"\n>> +[1]: \"bar\"\n>> +[2]: \"baz\"\n>> +EOF\n>> +\n>> +string_list_split_in_place \"foo:bar:baz\" \":\" \"0\" <> +3\n>> +[0]: \"foo\"\n>> +[1]: \"bar\"\n>> +[2]: \"baz\"\n>> +EOF\n>> +\n>> +string_list_split_in_place \"foo:bar:baz\" \":\" \"1\" <> +2\n>> +[0]: \"foo\"\n>> +[1]: \"bar:baz\"\n>> +EOF\n>> +\n>> +string_list_split_in_place \"foo:bar:baz\" \":\" \"2\" <> +3\n>> +[0]: \"foo\"\n>> +[1]: \"bar\"\n>> +[2]: \"baz\"\n>> +EOF\n>> +\n>> +string_list_split_in_place \"foo:bar:\" \":\" \"-1\" <> +3\n>> +[0]: \"foo\"\n>> +[1]: \"bar\"\n>> +[2]: \"\"\n>> +EOF\n>> +\n>> +string_list_split_in_place \"\" \":\" \"-1\" <> +1\n>> +[0]: \"\"\n>> +EOF\n>> +\n>> +string_list_split_in_place \":\" \":\" \"-1\" <> +2\n>> +[0]: \"\"\n>> +[1]: \"\"\n>> +EOF\n>> +\n>> +test_done\n>> diff --git a/test-string-list.c b/test-string-list.c\n>> new file mode 100644\n>> index 0000000..f08d3cc\n>> --- /dev/null\n>> +++ b/test-string-list.c\n>> @@ -0,0 +1,25 @@\n>> +#include \"cache.h\"\n>> +#include \"string-list.h\"\n>> +\n>> +int main(int argc, char **argv)\n>> +{\n>> +\tif ((argc == 4 || argc == 5) && !strcmp(argv[1], \"split_in_place\")) {\n>> +\t\tstruct string_list list = STRING_LIST_INIT_NODUP;\n>> +\t\tint i;\n>> +\t\tchar *s = xstrdup(argv[2]);\n>> +\t\tint delim = *argv[3];\n>> +\t\tint maxsplit = (argc == 5) ? atoi(argv[4]) : -1;\n> \n> Likewise on \"-1\".\n> \n>> +\t\ti = string_list_split_in_place(&list, s, delim, maxsplit);\n>> +\t\tprintf(\"%d\\n\", i);\n>> +\t\tfor (i = 0; i < list.nr; i++)\n>> +\t\t\tprintf(\"[%d]: \\\"%s\\\"\\n\", i, list.items[i].string);\n>> +\t\tstring_list_clear(&list, 0);\n>> +\t\tfree(s);\n>> +\t\treturn 0;\n>> +\t}\n>> +\n>> +\tfprintf(stderr, \"%s: unknown function name: %s\\n\", argv[0],\n>> +\t\targv[1] ? argv[1] : \"(there was none)\");\n>> +\treturn 1;\n>> +}\n\nThanks for your helpful feedback! I will send a revised patch series as\nsoon as I can.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"198658","messageId":"7vhar6pgxs.fsf@alter.siamese.dyndns.org","threadId":"31481","inReplyTo":"504D7082.9020903@alum.mit.edu","subject":"Re: [PATCH 1/4] Add a new function, string_list_split_in_place()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-10T05:47:43Z","receivedAt":"2012-09-10T05:47:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> ... Consider something like\n>\n> struct string_list *split_file_into_words(FILE *f)\n> {\n> char buf[1024];\n> struct string_list *list = new string list;\n> list->strdup_strings = 1;\n> while (not EOF) {\n> read_line_into_buf();\n> string_list_split_in_place(list, buf, ' ', -1);\n> }\n> return list;\n> }\n\nThat is a prime example to argue that string_list_split() would make\nmore sense, no? The caller does _not_ mind if the function mucks\nwith buf, but the resulting list is not allowed to point into buf.\n\nIn such a case, the caller shouldn't have to _care_ if it wants to\nallow buf to be mucked with; it is already asking that the resulting\nlist _not_ point into buf by setting strdup_strings (by the way,\nthat is part of the function input, so think of it like various *opt\nvariables passed into functions to tweak their behaviour). If the\nimplementation can do so without sacrificing performance (and in\nthis case, as you said, it can), it should take \"const char *buf\".\n\nThe above caller shouldn't have to choose between sl_split() and\nsl_split_in_place(), in other words.\n\nSo it appears to me that sl_split_in_place(), if implemented, should\nbe kept as a special case for performance-minded callers that have\nfull control of the lifetime rules of the variables they use, can\nset strdup_strings to false, and can let buf modified in place, and\ncan accept list that point into buf.\n\n>>> + * Examples:\n>>> + * string_list_split_in_place(l, \"foo:bar:baz\", ':', -1) -> [\"foo\", \"bar\", \"baz\"]\n>>> + * string_list_split_in_place(l, \"foo:bar:baz\", ':', 1) -> [\"foo\", \"bar:baz\"]\n>>> + * string_list_split_in_place(l, \"foo:bar:\", ':', -1) -> [\"foo\", \"bar\", \"\"]\n>> \n>> I would find it more natural to see a sentinel value against\n>> \"positive\" to be 0, not -1. \"-1\" gives an impression as if \"-2\"\n>> might do something different from \"-1\", but Zero is a lot more\n>> special.\n>\n> You have raised a good point and I think there is a flaw in the API, but\n> I'm not sure I agree with you what the flaw is...\n>\n> The \"maxsplit\" argument limits the number of times the string should be\n> split. I.e., if maxsplit is set, then the output will have at most\n> (maxsplit + 1) strings.\n\nSo \"do not split, just give me the whole thing\" would be maxsplit == 0\nto split into (maxsplit+1) == 1 string. I think we are in agreement\nthat your \"-1\" does not make any sense, and your documentation that\nsaid \"positive\" is the saner thing to do, no?\n"},{"id":"198662","messageId":"504DABB5.7090401@alum.mit.edu","threadId":"31481","inReplyTo":"7vk3w3sfe9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/4] Add a new function, filter_string_list()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-10T08:58:29Z","receivedAt":"2012-09-10T08:58:29Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/09/2012 11:40 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> Signed-off-by: Michael Haggerty \n>> ---\n>> Documentation/technical/api-string-list.txt | 8 ++++++++\n>> string-list.c | 17 +++++++++++++++++\n>> string-list.h | 9 +++++++++\n>> 3 files changed, 34 insertions(+)\n>>\n>> diff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt\n>> index 3b959a2..15b8072 100644\n>> --- a/Documentation/technical/api-string-list.txt\n>> +++ b/Documentation/technical/api-string-list.txt\n>> @@ -60,6 +60,14 @@ Functions\n>> \n>> * General ones (works with sorted and unsorted lists as well)\n>> \n>> +`filter_string_list`::\n>> +\n>> +\tApply a function to each item in a list, retaining only the\n>> +\titems for which the function returns true. If free_util is\n>> +\ttrue, call free() on the util members of any items that have\n>> +\tto be deleted. Preserve the order of the items that are\n>> +\tretained.\n> \n> In other words, this can safely be used on both sorted and unsorted\n> string list. Good.\n\nPreserving order (while retaining performance) is the main reason for\nthis function. Otherwise, unsorted_string_list_delete_item() could be\nused in a loop.\n\n>> `print_string_list`::\n>> \n>> \tDump a string_list to stdout, useful mainly for debugging purposes. It\n>> diff --git a/string-list.c b/string-list.c\n>> index 110449c..72610ce 100644\n>> --- a/string-list.c\n>> +++ b/string-list.c\n>> @@ -102,6 +102,23 @@ int for_each_string_list(struct string_list *list,\n>> \treturn ret;\n>> }\n>> \n>> +void filter_string_list(struct string_list *list, int free_util,\n>> +\t\t\tstring_list_each_func_t fn, void *cb_data)\n>> +{\n>> +\tint src, dst = 0;\n>> +\tfor (src = 0; src < list->nr; src++) {\n>> +\t\tif (fn(&list->items[src], cb_data)) {\n>> +\t\t\tlist->items[dst++] = list->items[src];\n>> +\t\t} else {\n>> +\t\t\tif (list->strdup_strings)\n>> +\t\t\t\tfree(list->items[src].string);\n>> +\t\t\tif (free_util)\n>> +\t\t\t\tfree(list->items[src].util);\n>> +\t\t}\n>> +\t}\n>> +\tlist->nr = dst;\n>> +}\n>> +\n>> void string_list_clear(struct string_list *list, int free_util)\n>> {\n>> \tif (list->items) {\n>> diff --git a/string-list.h b/string-list.h\n>> index 7e51d03..84996aa 100644\n>> --- a/string-list.h\n>> +++ b/string-list.h\n>> @@ -29,6 +29,15 @@ int for_each_string_list(struct string_list *list,\n>> #define for_each_string_list_item(item,list) \\\n>> \tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n>> \n>> +/*\n>> + * Apply fn to each item in list, retaining only the ones for which\n>> + * the function returns true. If free_util is true, call free() on\n>> + * the util members of any items that have to be deleted. Preserve\n>> + * the order of the items that are retained.\n>> + */\n>> +void filter_string_list(struct string_list *list, int free_util,\n>> +\t\t\tstring_list_each_func_t fn, void *cb_data);\n>> +\n>> /* Use these functions only on sorted lists: */\n>> int string_list_has_string(const struct string_list *list, const char *string);\n>> int string_list_find_insert_index(const struct string_list *list, const char *string,\n> \n> Having seen that the previous patch introduced a new test helper for\n> unit testing (which is a very good idea) and dedicated a new test\n> number, I would have expected to see a new test for filtering\n> here.\n\nI thought that the code was too trivial to warrant a test, especially\nconsidering that the memory handling aspect of the function can't be\ntested very well. But you've correctly shamed me into adding tests for\nthis and also for patch 3/4, string_list_remove_duplicates().\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"198663","messageId":"504DAF9A.4030202@alum.mit.edu","threadId":"31481","inReplyTo":"7vfw6rsf64.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/4] Add a new function, string_list_remove_duplicates()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-10T09:15:06Z","receivedAt":"2012-09-10T09:15:06Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/09/2012 11:45 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> Signed-off-by: Michael Haggerty \n>> ---\n>> Documentation/technical/api-string-list.txt | 4 ++++\n>> string-list.c | 17 +++++++++++++++++\n>> string-list.h | 5 +++++\n>> 3 files changed, 26 insertions(+)\n>>\n>> diff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt\n>> index 15b8072..9206f8f 100644\n>> --- a/Documentation/technical/api-string-list.txt\n>> +++ b/Documentation/technical/api-string-list.txt\n>> @@ -104,6 +104,10 @@ write `string_list_insert(...)->util = ...;`.\n>> \tLook up a given string in the string_list, returning the containing\n>> \tstring_list_item. If the string is not found, NULL is returned.\n>> \n>> +`string_list_remove_duplicates`::\n>> +\n>> +\tRemove all but the first entry that has a given string value.\n> \n> Unlike the previous patch, free_util is not documented?\n\nFixed.\n\n> It is kind of shame that the string list must be sorted for this\n> function to work, but I guess you do not have a need for a version\n> that works on both sorted and unsorted (yet). We can introduce a\n> variant with \"unsorted_\" prefix later when it becomes necessary, so\n> OK.\n\nNot only that; for an unsorted list it is not quite as obvious what a\ncaller would want. Often lists are used as a poor man's set, in which\ncase the caller would probably not mind sorting the list anyway. There\nare two operations that one might conceive of for unsorted lists: (1)\nremove duplicates while preserving the order of the remaining entries,\nor (2) remove duplicates while not worrying about the order of the\nremaining entries. (Admittedly the first is not much more expensive\nthan the second.) These are more complicated to program, require\ntemporary space, and are of less obvious utility than removing\nduplicates from a sorted list.\n\n>> * Functions for unsorted lists only\n>> \n>> `string_list_append`::\n>> diff --git a/string-list.c b/string-list.c\n>> index 72610ce..bfef6cf 100644\n>> --- a/string-list.c\n>> +++ b/string-list.c\n>> @@ -92,6 +92,23 @@ struct string_list_item *string_list_lookup(struct string_list *list, const char\n>> \treturn list->items + i;\n>> }\n>> \n>> +void string_list_remove_duplicates(struct string_list *list, int free_util)\n>> +{\n>> +\tif (list->nr > 1) {\n>> +\t\tint src, dst;\n>> +\t\tfor (src = dst = 1; src < list->nr; src++) {\n>> +\t\t\tif (!strcmp(list->items[dst - 1].string, list->items[src].string)) {\n>> +\t\t\t\tif (list->strdup_strings)\n>> +\t\t\t\t\tfree(list->items[src].string);\n>> +\t\t\t\tif (free_util)\n>> +\t\t\t\t\tfree(list->items[src].util);\n>> +\t\t\t} else\n>> +\t\t\t\tlist->items[dst++] = list->items[src];\n>> +\t\t}\n>> +\t\tlist->nr = dst;\n>> +\t}\n>> +}\n>> +\n>> int for_each_string_list(struct string_list *list,\n>> \t\t\t string_list_each_func_t fn, void *cb_data)\n>> {\n>> diff --git a/string-list.h b/string-list.h\n>> index 84996aa..c4dc659 100644\n>> --- a/string-list.h\n>> +++ b/string-list.h\n>> @@ -38,6 +38,7 @@ int for_each_string_list(struct string_list *list,\n>> void filter_string_list(struct string_list *list, int free_util,\n>> \t\t\tstring_list_each_func_t fn, void *cb_data);\n>> \n>> +\n>> /* Use these functions only on sorted lists: */\n>> int string_list_has_string(const struct string_list *list, const char *string);\n>> int string_list_find_insert_index(const struct string_list *list, const char *string,\n>> @@ -47,6 +48,10 @@ struct string_list_item *string_list_insert_at_index(struct string_list *list,\n>> \t\t\t\t\t\t int insert_at, const char *string);\n>> struct string_list_item *string_list_lookup(struct string_list *list, const char *string);\n>> \n>> +/* Remove all but the first entry that has a given string value. */\n>> +void string_list_remove_duplicates(struct string_list *list, int free_util);\n>> +\n>> +\n>> /* Use these functions only on unsorted lists: */\n>> struct string_list_item *string_list_append(struct string_list *list, const char *string);\n>> void sort_string_list(struct string_list *list);\n\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"198664","messageId":"504DBA62.3080001@alum.mit.edu","threadId":"31481","inReplyTo":"7vbohfser4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/4] Add a function string_list_longest_prefix()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-10T10:01:06Z","receivedAt":"2012-09-10T10:01:06Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/09/2012 11:54 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> [...]\n>> diff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt\n>> index 9206f8f..291ac4c 100644\n>> --- a/Documentation/technical/api-string-list.txt\n>> +++ b/Documentation/technical/api-string-list.txt\n>> @@ -68,6 +68,14 @@ Functions\n>> \tto be deleted. Preserve the order of the items that are\n>> \tretained.\n>> \n>> +`string_list_longest_prefix`::\n>> +\n>> +\tReturn the longest string within a string_list that is a\n>> +\tprefix (in the sense of prefixcmp()) of the specified string,\n>> +\tor NULL if no such prefix exists. This function does not\n>> +\trequire the string_list to be sorted (it does a linear\n>> +\tsearch).\n>> +\n>> `print_string_list`::\n> \n> This may feel like outside the scope of this series, but since this\n> series will be the main culprit for adding many new functions to\n> this API in the recent history...\n> \n> - We may want to name things a bit more consistently so that people\n> can tell which ones can be called on any string list, which ones\n> are sorted list only, and which ones are unsorted one only.\n> \n> In addition, the last category _may_ need a bit more thought.\n> Calling unsorted_string_list_lookup() on an already sorted list\n> is not a crime---it is just a stupid thing to do.\n\nYes, this could be clearer. Though I'm skeptical that a naming\nconvention can capture all of the variation without being too cumbersome.\n\nAnother idea: in string-list.h, one could name parameters \"sorted_list\"\nwhen they must be sorted as a precondition of the function.\n\nBut before getting too hung up on finery, the effort might be better\ninvested adding documentation for functions that are totally lacking it,\nlike\n\n string_list_clear_func()\n for_each_string_list()\n for_each_string_list_item()\n string_list_find_insert_index()\n string_list_insert_at_index()\n\nWhile we're on the subject, it seems to me that documenting APIs like\nthese in separate files under Documentation/technical rather than in the\nheader files themselves\n\n- makes the documentation for a particular function harder to find,\n\n- makes it easier for the documentation to get out of sync with the\nactual collection of functions (e.g., the 5 undocumented functions\nlisted above).\n\n- makes it awkward for the documentation to refer to particular function\nparameters by name.\n\nWhile it is nice to have a high-level prose description of an API, I am\noften frustrated by the lack of \"docstrings\" in the header file where a\nfunction is declared. The high-level description of an API could be put\nat the top of the header file.\n\nAlso, better documentation in header files could enable the automatic\ngeneration of API docs (e.g., via doxygen).\n\nIs there some reason for the current documentation policy or is it\nhistorical and just needs somebody to put in the work to change it?\n\n> - Why are these new functions described at the top, not appended at\n> the bottom? I would have expected either an alphabetical, or a\n> more generic ones first (i.e. print and clear are a lot \"easier\"\n> ones compared to filter and prefix that are very much more\n> specialized).\n\nThe order seemed logical to me at the time (given the constraint that\nfunctions are grouped by sorted/unsorted/don't-care):\nprint_string_list() is only useful for debugging, so it seemed to belong\nbelow the \"production\" functions. string_list_clear() was already below\nprint_string_list() (which I guessed was because it is logically used\nlast in the life of a string_list) so I left it at the end of its\nsection. My preference would probably have been to move\nprint_string_list() below string_list_clear(), but somebody else made\nthe opposite choice so I decided to respect it.\n\nThat being said, I don't have anything against a different order.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"198667","messageId":"504DD3A5.8000201@alum.mit.edu","threadId":"31481","inReplyTo":"7vhar6pgxs.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/4] Add a new function, string_list_split_in_place()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-10T11:48:53Z","receivedAt":"2012-09-10T11:48:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/10/2012 07:47 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> ... Consider something like\n>>\n>> struct string_list *split_file_into_words(FILE *f)\n>> {\n>> char buf[1024];\n>> struct string_list *list = new string list;\n>> list->strdup_strings = 1;\n>> while (not EOF) {\n>> read_line_into_buf();\n>> string_list_split_in_place(list, buf, ' ', -1);\n>> }\n>> return list;\n>> }\n> \n> That is a prime example to argue that string_list_split() would make\n> more sense, no? The caller does _not_ mind if the function mucks\n> with buf, but the resulting list is not allowed to point into buf.\n> \n> In such a case, the caller shouldn't have to _care_ if it wants to\n> allow buf to be mucked with; it is already asking that the resulting\n> list _not_ point into buf by setting strdup_strings (by the way,\n> that is part of the function input, so think of it like various *opt\n> variables passed into functions to tweak their behaviour). If the\n> implementation can do so without sacrificing performance (and in\n> this case, as you said, it can), it should take \"const char *buf\".\n\nYou're right, I was thinking that a caller of\nstring_list_split_in_place() could choose to remain ignorant of whether\nstrdup_strings is set, but this is incorrect because it needs to know\nwhether to do its own memory management of the strings that it passes\ninto string_list_append().\n\n> So it appears to me that sl_split_in_place(), if implemented, should\n> be kept as a special case for performance-minded callers that have\n> full control of the lifetime rules of the variables they use, can\n> set strdup_strings to false, and can let buf modified in place, and\n> can accept list that point into buf.\n\nOK, so the bottom line would be to have two versions of the function.\nOne takes a (const char *) and *requires* strdup_strings to be set on\nits input list:\n\nint string_list_split(struct string_list *list, const char *string,\n\t\t int delim, int maxsplit)\n{\n\tassert(list->strdup_strings);\n\t...\n}\n\nThe other takes a (char *) and modifies it in-place, and maybe even\nrequires strdup_strings to be false on its input list:\n\nint string_list_split_in_place(struct string_list *list, char *string,\n\t\t\t int delim, int maxsplit)\n{\n\t/* not an error per se but a strong suggestion of one: */\n\tassert(!list->strdup_strings);\n\t...\n}\n\n(The latter (modulo assert) is the one that I have implemented, but it\nmight not be needed immediately.) Do you agree?\n\n>>>> + * Examples:\n>>>> + * string_list_split_in_place(l, \"foo:bar:baz\", ':', -1) -> [\"foo\", \"bar\", \"baz\"]\n>>>> + * string_list_split_in_place(l, \"foo:bar:baz\", ':', 1) -> [\"foo\", \"bar:baz\"]\n>>>> + * string_list_split_in_place(l, \"foo:bar:\", ':', -1) -> [\"foo\", \"bar\", \"\"]\n>>>\n>>> I would find it more natural to see a sentinel value against\n>>> \"positive\" to be 0, not -1. \"-1\" gives an impression as if \"-2\"\n>>> might do something different from \"-1\", but Zero is a lot more\n>>> special.\n>>\n>> You have raised a good point and I think there is a flaw in the API, but\n>> I'm not sure I agree with you what the flaw is...\n>>\n>> The \"maxsplit\" argument limits the number of times the string should be\n>> split. I.e., if maxsplit is set, then the output will have at most\n>> (maxsplit + 1) strings.\n> \n> So \"do not split, just give me the whole thing\" would be maxsplit == 0\n> to split into (maxsplit+1) == 1 string. I think we are in agreement\n> that your \"-1\" does not make any sense, and your documentation that\n> said \"positive\" is the saner thing to do, no?\n\nNo. I think that my handling of maxsplit=0 was incorrect but that we\nshould continue using -1 as the special value.\n\nI see maxsplit=0 as a legitimate degenerate case meaning \"split into 1\nstring\". Granted, it would only be useful in specialized situations\n[1]. Moreover, \"-1\" makes a much more obvious special value than \"0\";\nsomebody reading code with maxsplit=-1 knows immediately that this is a\nspecial value, whereas the handling of maxsplit=0 isn't quite so\nblindingly obvious unless the reader knows the outcome of our current\ndiscussion :-)\n\nTherefore I still prefer treating only negative values of maxsplit to\nmean \"unlimited\" and fixing maxsplit=0 as described. But if you insist\non the other convention, let me know and I will change it.\n\nMichael\n\n[1] A case I can think of would be parsing a format like\n\n NUMPARENTS [PARENT...] SUMMARY\n\nwhere \"string_list_split(list, rest_of_line, ' ', numparents)\" does the\nright thing even if numparents==0.\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"198679","messageId":"7vd31tq2qf.fsf@alter.siamese.dyndns.org","threadId":"31481","inReplyTo":"504DD3A5.8000201@alum.mit.edu","subject":"Re: [PATCH 1/4] Add a new function, string_list_split_in_place()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-10T16:09:12Z","receivedAt":"2012-09-10T16:09:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> OK, so the bottom line would be to have two versions of the function.\n> One takes a (const char *) and *requires* strdup_strings to be set on\n> its input list:\n>\n> int string_list_split(struct string_list *list, const char *string,\n> \t\t int delim, int maxsplit)\n> {\n> \tassert(list->strdup_strings);\n> \t...\n> }\n>\n> The other takes a (char *) and modifies it in-place, and maybe even\n> requires strdup_strings to be false on its input list:\n>\n> int string_list_split_in_place(struct string_list *list, char *string,\n> \t\t\t int delim, int maxsplit)\n> {\n> \t/* not an error per se but a strong suggestion of one: */\n> \tassert(!list->strdup_strings);\n> \t...\n> }\n>\n> (The latter (modulo assert) is the one that I have implemented, but it\n> might not be needed immediately.) Do you agree?\n\nOK; I do not offhand know which one you immediately needed, but I\nthink that is a sensible way to structure the API.\n\n> [1] A case I can think of would be parsing a format like\n>\n> NUMPARENTS [PARENT...] SUMMARY\n>\n> where \"string_list_split(list, rest_of_line, ' ', numparents)\" does the\n> right thing even if numparents==0.\n\nOK.\n"},{"id":"198686","messageId":"7v1ui9q21a.fsf@alter.siamese.dyndns.org","threadId":"31481","inReplyTo":"504DBA62.3080001@alum.mit.edu","subject":"Re: [PATCH 4/4] Add a function string_list_longest_prefix()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-10T16:24:17Z","receivedAt":"2012-09-10T16:24:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Another idea: in string-list.h, one could name parameters \"sorted_list\"\n> when they must be sorted as a precondition of the function.\n\nThat sounds like a very sensible thing to do.\n\n> But before getting too hung up on finery, the effort might be better\n> invested adding documentation for functions that are totally lacking it,\n> like\n>\n> string_list_clear_func()\n> for_each_string_list()\n> for_each_string_list_item()\n> string_list_find_insert_index()\n> string_list_insert_at_index()\n>\n> While we're on the subject, it seems to me that documenting APIs like\n> these in separate files under Documentation/technical rather than in the\n> header files themselves\n>\n> - makes the documentation for a particular function harder to find,\n>\n> - makes it easier for the documentation to get out of sync with the\n> actual collection of functions (e.g., the 5 undocumented functions\n> listed above).\n>\n> - makes it awkward for the documentation to refer to particular function\n> parameters by name.\n>\n> While it is nice to have a high-level prose description of an API, I am\n> often frustrated by the lack of \"docstrings\" in the header file where a\n> function is declared. The high-level description of an API could be put\n> at the top of the header file.\n>\n> Also, better documentation in header files could enable the automatic\n> generation of API docs (e.g., via doxygen).\n\nYeah, perhaps you may want to look into doing an automated\ngeneration of Documentation/technical/api-*.txt files out of the\nheaders.\n"},{"id":"198691","messageId":"20120910163310.GE9435@sigill.intra.peff.net","threadId":"31481","inReplyTo":"7v1ui9q21a.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/4] Add a function string_list_longest_prefix()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-10T16:33:10Z","receivedAt":"2012-09-10T16:33:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 10, 2012 at 09:24:17AM -0700, Junio C Hamano wrote:\n\n> > While we're on the subject, it seems to me that documenting APIs like\n> > these in separate files under Documentation/technical rather than in the\n> > header files themselves\n> >\n> > - makes the documentation for a particular function harder to find,\n> >\n> > - makes it easier for the documentation to get out of sync with the\n> > actual collection of functions (e.g., the 5 undocumented functions\n> > listed above).\n> >\n> > - makes it awkward for the documentation to refer to particular function\n> > parameters by name.\n> >\n> > While it is nice to have a high-level prose description of an API, I am\n> > often frustrated by the lack of \"docstrings\" in the header file where a\n> > function is declared. The high-level description of an API could be put\n> > at the top of the header file.\n> >\n> > Also, better documentation in header files could enable the automatic\n> > generation of API docs (e.g., via doxygen).\n> \n> Yeah, perhaps you may want to look into doing an automated\n> generation of Documentation/technical/api-*.txt files out of the\n> headers.\n\nI was just documenting something in technical/api-* the other day, and\nhad the same feeling. I'd be very happy if we moved to some kind of\nliterate-programming system. I have no idea which ones are good or bad,\nthough. I have used doxygen, but all I remember is it being painfully\nbaroque. I'd much rather have something simple and lightweight, with an\neasy markup format. For example, this:\n\n http://tomdoc.org/\n\nLooks much nicer to me than most doxygen I've seen. But again, it's been\na while, so maybe doxygen is nicer than I remember.\n\n-Peff\n"},{"id":"198703","messageId":"504E27D7.8010505@op5.se","threadId":"31481","inReplyTo":"20120910163310.GE9435@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] Add a function string_list_longest_prefix()","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2012-09-10T17:48:07Z","receivedAt":"2012-09-10T17:48:07Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"On 09/10/2012 06:33 PM, Jeff King wrote:\n> On Mon, Sep 10, 2012 at 09:24:17AM -0700, Junio C Hamano wrote:\n> \n>>> While we're on the subject, it seems to me that documenting APIs like\n>>> these in separate files under Documentation/technical rather than in the\n>>> header files themselves\n>>>\n>>> - makes the documentation for a particular function harder to find,\n>>>\n>>> - makes it easier for the documentation to get out of sync with the\n>>> actual collection of functions (e.g., the 5 undocumented functions\n>>> listed above).\n>>>\n>>> - makes it awkward for the documentation to refer to particular function\n>>> parameters by name.\n>>>\n>>> While it is nice to have a high-level prose description of an API, I am\n>>> often frustrated by the lack of \"docstrings\" in the header file where a\n>>> function is declared. The high-level description of an API could be put\n>>> at the top of the header file.\n>>>\n>>> Also, better documentation in header files could enable the automatic\n>>> generation of API docs (e.g., via doxygen).\n>>\n>> Yeah, perhaps you may want to look into doing an automated\n>> generation of Documentation/technical/api-*.txt files out of the\n>> headers.\n> \n> I was just documenting something in technical/api-* the other day, and\n> had the same feeling. I'd be very happy if we moved to some kind of\n> literate-programming system. I have no idea which ones are good or bad,\n> though. I have used doxygen, but all I remember is it being painfully\n> baroque. I'd much rather have something simple and lightweight, with an\n> easy markup format. For example, this:\n> \n> http://tomdoc.org/\n> \n> Looks much nicer to me than most doxygen I've seen. But again, it's been\n> a while, so maybe doxygen is nicer than I remember.\n> \n\nDoxygen has a the very nifty feature of being able to generate\ncallgraphs though. We use it extensively at $dayjob, so if you need a\nhand building something sensible out of git's headers, I'd be happy\nto help.\n\nlibgit2 uses doxygen btw, and has done since the start. If we ever\nmerge the two, it would be neat to use the same.\n\n-- \nAndreas Ericsson andreas.ericsson@op5.se\nOP5 AB www.op5.se\nTel: +46 8-230225 Fax: +46 8-230231\n\nConsidering the successes of the wars on alcohol, poverty, drugs and\nterror, I think we should give some serious thought to declaring war\non peace.\n"},{"id":"198710","messageId":"504E3DA8.7040906@alum.mit.edu","threadId":"31481","inReplyTo":"504E27D7.8010505@op5.se","subject":"Using doxygen (or something similar) to generate API docs [was [PATCH 4/4] Add a function string_list_longest_prefix()]","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-10T19:21:12Z","receivedAt":"2012-09-10T19:21:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"I'm renaming this thread so that the bikeshedding can get over ASAP.\n\nOn 09/10/2012 07:48 PM, Andreas Ericsson wrote:\n> On 09/10/2012 06:33 PM, Jeff King wrote:\n>> On Mon, Sep 10, 2012 at 09:24:17AM -0700, Junio C Hamano wrote:\n>>> Michael Haggerty writes:\n>>>> Also, better documentation in header files could enable the automatic\n>>>> generation of API docs (e.g., via doxygen).\n>>>\n>>> Yeah, perhaps you may want to look into doing an automated\n>>> generation of Documentation/technical/api-*.txt files out of the\n>>> headers.\n>>\n>> I was just documenting something in technical/api-* the other day, and\n>> had the same feeling. I'd be very happy if we moved to some kind of\n>> literate-programming system. I have no idea which ones are good or bad,\n>> though. I have used doxygen, but all I remember is it being painfully\n>> baroque. I'd much rather have something simple and lightweight, with an\n>> easy markup format. For example, this:\n>>\n>> http://tomdoc.org/\n>>\n>> Looks much nicer to me than most doxygen I've seen. But again, it's been\n>> a while, so maybe doxygen is nicer than I remember.\n\nI don't have a personal preference for what system is used. I mentioned\ndoxygen only because it seems to be a well-known example.\n\n>From a glance at the URL you mentioned, it looks like TomDoc is only\napplicable to Ruby code.\n\n> Doxygen has a the very nifty feature of being able to generate\n> callgraphs though. We use it extensively at $dayjob, so if you need a\n> hand building something sensible out of git's headers, I'd be happy\n> to help.\n\nMy plate is full. If you are able to work on this, it would be awesome.\n As far as I'm concerned, you are the new literate documentation czar :-)\n\nMost importantly, having a convenient system of converting header\ncomments into documentation would hopefully motivate other people to add\nbetter header comments in the first place, and motivate reviewers to\ninsist on them. It's shocking (to me) how few functions are documented,\nand how often I have to read masses of C code to figure out what a\nfunction is for, its pre- and post-conditions, its memory policy, etc.\nOften I find myself having to read functions three layers down the call\ntree to figure out the behavior of the top-layer function. I try to\ndocument things as I go, but it's only a drop in the bucket.\n\n> libgit2 uses doxygen btw, and has done since the start. If we ever\n> merge the two, it would be neat to use the same.\n\nThat would be a nice bonus.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"198745","messageId":"20120910215633.GB1537@sigill.intra.peff.net","threadId":"31481","inReplyTo":"504E3DA8.7040906@alum.mit.edu","subject":"Re: Using doxygen (or something similar) to generate API docs [was [PATCH 4/4] Add a function string_list_longest_prefix()]","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-10T21:56:33Z","receivedAt":"2012-09-10T21:56:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 10, 2012 at 09:21:12PM +0200, Michael Haggerty wrote:\n\n> I'm renaming this thread so that the bikeshedding can get over ASAP.\n\nThanks. :)\n\n> >> http://tomdoc.org/\n> >>\n> >> Looks much nicer to me than most doxygen I've seen. But again, it's been\n> >> a while, so maybe doxygen is nicer than I remember.\n> \n> I don't have a personal preference for what system is used. I mentioned\n> doxygen only because it seems to be a well-known example.\n> \n> From a glance at the URL you mentioned, it looks like TomDoc is only\n> applicable to Ruby code.\n\nYeah, sorry, I should have been more clear; tomdoc is not an option\nbecause it doesn't do C. But what I like about it is the more\nnatural markup syntax. I was wondering if there were other similar\nsolutions. Looks like \"NaturalDocs\" is one:\n\n http://www.naturaldocs.org/documenting.html\n\nOn the other hand, doxygen is well-known among open source folks, which\ncounts for something. And from what I've read, recent versions support\nMarkdown, but I'm not sure of the details. So maybe it is a lot better\nthan I remember.\n\n> > Doxygen has a the very nifty feature of being able to generate\n> > callgraphs though. We use it extensively at $dayjob, so if you need a\n> > hand building something sensible out of git's headers, I'd be happy\n> > to help.\n\nIt has been over a decade since I seriously used doxygen for anything,\nand then it was a medium-sized project. So take my opinion with a grain\nof salt. But I remember the callgraph feature being one of those things\nthat _sounded_ really cool, but in practice was not all that useful.\n\n> My plate is full. If you are able to work on this, it would be awesome.\n> As far as I'm concerned, you are the new literate documentation czar :-)\n\nLucky me? :)\n\nI think I'll leave it for the moment, and next time I start to add some\napi-level documentation I'll take a look at doxygen-ating them and see\nhow I like it. And I'd invite anyone else to do the same (in doxygen, or\nwhatever system you like -- the best way to evaluate a tool like this is\nto see how your real work would look).\n\n-Peff\n"},{"id":"198749","messageId":"504E651A.4030401@alum.mit.edu","threadId":"31481","inReplyTo":"20120910215633.GB1537@sigill.intra.peff.net","subject":"Re: Using doxygen (or something similar) to generate API docs [was [PATCH 4/4] Add a function string_list_longest_prefix()]","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-10T22:09:30Z","receivedAt":"2012-09-10T22:09:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/10/2012 11:56 PM, Jeff King wrote:\n> On Mon, Sep 10, 2012 at 09:21:12PM +0200, Michael Haggerty wrote:\n>> My plate is full. If you are able to work on this, it would be awesome.\n>> As far as I'm concerned, you are the new literate documentation czar :-)\n> \n> Lucky me? :)\n\nI was nominating Andreas, who rashly volunteered to help. But don't\nfeel left out; there's enough work to go around :-)\n\n> I think I'll leave it for the moment, and next time I start to add some\n> api-level documentation I'll take a look at doxygen-ating them and see\n> how I like it. And I'd invite anyone else to do the same (in doxygen, or\n> whatever system you like -- the best way to evaluate a tool like this is\n> to see how your real work would look).\n\nI agree with that. A very good start would be to mark up a single API\nand build the docs (by hand if need be) using a proposed tool. This\nwill let people get a feel for (1) what the markup has to look like and\n(2) what they get out of it.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"198754","messageId":"504E8D65.9030000@op5.se","threadId":"31481","inReplyTo":"20120910215633.GB1537@sigill.intra.peff.net","subject":"Re: Using doxygen (or something similar) to generate API docs [was [PATCH 4/4] Add a function string_list_longest_prefix()]","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2012-09-11T01:01:25Z","receivedAt":"2012-09-11T01:01:25Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"On 09/10/2012 11:56 PM, Jeff King wrote:\n> On Mon, Sep 10, 2012 at 09:21:12PM +0200, Michael Haggerty wrote:\n> \n>> I'm renaming this thread so that the bikeshedding can get over ASAP.\n> \n> Thanks. :)\n> \n>>>> http://tomdoc.org/\n>>>>\n>>>> Looks much nicer to me than most doxygen I've seen. But again, it's been\n>>>> a while, so maybe doxygen is nicer than I remember.\n>>\n>> I don't have a personal preference for what system is used. I mentioned\n>> doxygen only because it seems to be a well-known example.\n>>\n>> From a glance at the URL you mentioned, it looks like TomDoc is only\n>> applicable to Ruby code.\n> \n> Yeah, sorry, I should have been more clear; tomdoc is not an option\n> because it doesn't do C. But what I like about it is the more\n> natural markup syntax. I was wondering if there were other similar\n> solutions. Looks like \"NaturalDocs\" is one:\n> \n> http://www.naturaldocs.org/documenting.html\n> \n> On the other hand, doxygen is well-known among open source folks, which\n> counts for something. And from what I've read, recent versions support\n> Markdown, but I'm not sure of the details. So maybe it is a lot better\n> than I remember.\n> \n\nMarkdown is supported, yes. There aren't really any details to it.\nI don't particularly like markdown, but my colleagues tend to use\nit for howto's and whatnot and it can be mixed with other doxygen\nstyles without problem.\n\n\n>>> Doxygen has a the very nifty feature of being able to generate\n>>> callgraphs though. We use it extensively at $dayjob, so if you need a\n>>> hand building something sensible out of git's headers, I'd be happy\n>>> to help.\n> \n> It has been over a decade since I seriously used doxygen for anything,\n> and then it was a medium-sized project. So take my opinion with a grain\n> of salt. But I remember the callgraph feature being one of those things\n> that _sounded_ really cool, but in practice was not all that useful.\n> \n\nIt's like all tools; Once you're used to it, it's immensely useful. I\ntend to prefer using it to find either code in dire need of refactoring\n(where the graph is too large), or engines and exit points. For those\npurposes, it's pretty hard to beat a good callgraph.\n\n>> My plate is full. If you are able to work on this, it would be awesome.\n>> As far as I'm concerned, you are the new literate documentation czar :-)\n> \n> Lucky me? :)\n> \n\nI think he was talking to me, but since you seem to have volunteered... ;)\n\n> I think I'll leave it for the moment, and next time I start to add some\n> api-level documentation I'll take a look at doxygen-ating them and see\n> how I like it. And I'd invite anyone else to do the same (in doxygen, or\n> whatever system you like -- the best way to evaluate a tool like this is\n> to see how your real work would look).\n> \n\nThat's one of the problems. People follow what's already there, and there\nare no comments there now so there won't be any added in the future :-/\n\n-- \nAndreas Ericsson andreas.ericsson@op5.se\nOP5 AB www.op5.se\nTel: +46 8-230225 Fax: +46 8-230231\n\nConsidering the successes of the wars on alcohol, poverty, drugs and\nterror, I think we should give some serious thought to declaring war\non peace.\n"}]}