[PATCH 0/4] Assorted comment fixes

[PATCH 0/4] Assorted comment fixes

[Han-Wen Nienhuys]

I followed Peff's advice for string-list.h comments.

Han-Wen Nienhuys (4):
  Fix typo in submodule.h
  Clarify return value ownership of real_path and read_gitfile_gently
  Document submodule_to_gitdir
  Move documentation of string_list into string-list.h

 Documentation/technical/api-string-list.txt | 209 ----------------------------
 abspath.c                                   |   3 +
 setup.c                                     |   3 +-
 string-list.h                               | 187 +++++++++++++++++++++----
 submodule.c                                 |   3 +
 submodule.h                                 |   2 +-
 6 files changed, 168 insertions(+), 239 deletions(-)
 delete mode 100644 Documentation/technical/api-string-list.txt

--
2.14.1.821.g8fa685d3b7-goog

[PATCH 1/4] Fix typo in submodule.h

[Han-Wen Nienhuys]

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 submodule.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/submodule.h b/submodule.h
index 6b52133c8..f0da0277a 100644
--- a/submodule.h
+++ b/submodule.h
@@ -120,7 +120,7 @@ extern int submodule_move_head(const char *path,
 
 /*
  * Prepare the "env_array" parameter of a "struct child_process" for executing
- * a submodule by clearing any repo-specific envirionment variables, but
+ * a submodule by clearing any repo-specific environment variables, but
  * retaining any config in the environment.
  */
 extern void prepare_submodule_repo_env(struct argv_array *out);
-- 
2.14.1.821.g8fa685d3b7-goog

[PATCH 2/4] Clarify return value ownership of real_path and read_gitfile_gently

[Han-Wen Nienhuys]

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 abspath.c | 3 +++
 setup.c   | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/abspath.c b/abspath.c
index 708aff8d4..792a2d533 100644
--- a/abspath.c
+++ b/abspath.c
@@ -202,6 +202,9 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
 	return retval;
 }
 
+/* Resolve `path` into an absolute, cleaned-up path. The return value
+ * comes from a global shared buffer and should not be freed.
+ */
 const char *real_path(const char *path)
 {
 	static struct strbuf realpath = STRBUF_INIT;
diff --git a/setup.c b/setup.c
index 6d8380acd..31853724c 100644
--- a/setup.c
+++ b/setup.c
@@ -541,7 +541,8 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
 
 /*
  * Try to read the location of the git directory from the .git file,
- * return path to git directory if found.
+ * return path to git directory if found. The return value comes from
+ * a shared pool and should not be freed.
  *
  * On failure, if return_error_code is not NULL, return_error_code
  * will be set to an error code and NULL will be returned. If
-- 
2.14.1.821.g8fa685d3b7-goog

[PATCH 3/4] Document submodule_to_gitdir

[Han-Wen Nienhuys]

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 submodule.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/submodule.c b/submodule.c
index b12600fc7..b66c23f5d 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1997,6 +1997,9 @@ const char *get_superproject_working_tree(void)
 	return ret;
 }
 
+/* Put the gitdir for a submodule (given relative to the main repository worktree)
+ * into `buf`, or return -1 on error.
+ */
 int submodule_to_gitdir(struct strbuf *buf, const char *submodule)
 {
 	const struct submodule *sub;
-- 
2.14.1.821.g8fa685d3b7-goog

[PATCH 4/4] Move documentation of string_list into string-list.h

[Han-Wen Nienhuys]

This mirrors commit bdfdaa4978dd92992737e662f25adc01d32d0774 which did
the same for strbuf.h:

* API documentation uses /** */ to set it apart from other comments.

* Function names were stripped from the comments.

* Ordering of the header was adjusted to follow the one from the text
  file.

* Edited some existing comments to follow the new standard.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 Documentation/technical/api-string-list.txt | 209 ----------------------------
 string-list.h                               | 187 +++++++++++++++++++++----
 2 files changed, 159 insertions(+), 237 deletions(-)
 delete mode 100644 Documentation/technical/api-string-list.txt

diff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt
deleted file mode 100644
index c08402b12..000000000
--- a/Documentation/technical/api-string-list.txt
+++ /dev/null
@@ -1,209 +0,0 @@
-string-list API
-===============
-
-The string_list API offers a data structure and functions to handle
-sorted and unsorted string lists.  A "sorted" list is one whose
-entries are sorted by string value in `strcmp()` order.
-
-The 'string_list' struct used to be called 'path_list', but was renamed
-because it is not specific to paths.
-
-The caller:
-
-. Allocates and clears a `struct string_list` variable.
-
-. Initializes the members. You might want to set the flag `strdup_strings`
-  if the strings should be strdup()ed. For example, this is necessary
-  when you add something like git_path("..."), since that function returns
-  a static buffer that will change with the next call to git_path().
-+
-If you need something advanced, you can manually malloc() the `items`
-member (you need this if you add things later) and you should set the
-`nr` and `alloc` members in that case, too.
-
-. Adds new items to the list, using `string_list_append`,
-  `string_list_append_nodup`, `string_list_insert`,
-  `string_list_split`, and/or `string_list_split_in_place`.
-
-. Can check if a string is in the list using `string_list_has_string` or
-  `unsorted_string_list_has_string` and get it from the list using
-  `string_list_lookup` for sorted lists.
-
-. Can sort an unsorted list using `string_list_sort`.
-
-. Can remove duplicate items from a sorted list using
-  `string_list_remove_duplicates`.
-
-. Can remove individual items of an unsorted list using
-  `unsorted_string_list_delete_item`.
-
-. Can remove items not matching a criterion from a sorted or unsorted
-  list using `filter_string_list`, or remove empty strings using
-  `string_list_remove_empty_items`.
-
-. Finally it should free the list using `string_list_clear`.
-
-Example:
-
-----
-struct string_list list = STRING_LIST_INIT_NODUP;
-int i;
-
-string_list_append(&list, "foo");
-string_list_append(&list, "bar");
-for (i = 0; i < list.nr; i++)
-	printf("%s\n", list.items[i].string)
-----
-
-NOTE: It is more efficient to build an unsorted list and sort it
-afterwards, instead of building a sorted list (`O(n log n)` instead of
-`O(n^2)`).
-+
-However, if you use the list to check if a certain string was added
-already, you should not do that (using unsorted_string_list_has_string()),
-because the complexity would be quadratic again (but with a worse factor).
-
-Functions
----------
-
-* General ones (works with sorted and unsorted lists as well)
-
-`string_list_init`::
-
-	Initialize the members of the string_list, set `strdup_strings`
-	member according to the value of the second parameter.
-
-`filter_string_list`::
-
-	Apply a function to each item in a list, retaining only the
-	items for which the function returns true.  If free_util is
-	true, call free() on the util members of any items that have
-	to be deleted.  Preserve the order of the items that are
-	retained.
-
-`string_list_remove_empty_items`::
-
-	Remove any empty strings from the list.  If free_util is true,
-	call free() on the util members of any items that have to be
-	deleted.  Preserve the order of the items that are retained.
-
-`print_string_list`::
-
-	Dump a string_list to stdout, useful mainly for debugging purposes. It
-	can take an optional header argument and it writes out the
-	string-pointer pairs of the string_list, each one in its own line.
-
-`string_list_clear`::
-
-	Free a string_list. The `string` pointer of the items will be freed in
-	case the `strdup_strings` member of the string_list is set. The second
-	parameter controls if the `util` pointer of the items should be freed
-	or not.
-
-* Functions for sorted lists only
-
-`string_list_has_string`::
-
-	Determine if the string_list has a given string or not.
-
-`string_list_insert`::
-
-	Insert a new element to the string_list. The returned pointer can be
-	handy if you want to write something to the `util` pointer of the
-	string_list_item containing the just added string. If the given
-	string already exists the insertion will be skipped and the
-	pointer to the existing item returned.
-+
-Since this function uses xrealloc() (which die()s if it fails) if the
-list needs to grow, it is safe not to check the pointer. I.e. you may
-write `string_list_insert(...)->util = ...;`.
-
-`string_list_lookup`::
-
-	Look up a given string in the string_list, returning the containing
-	string_list_item. If the string is not found, NULL is returned.
-
-`string_list_remove_duplicates`::
-
-	Remove all but the first of consecutive entries that have the
-	same string value.  If free_util is true, call free() on the
-	util members of any items that have to be deleted.
-
-* Functions for unsorted lists only
-
-`string_list_append`::
-
-	Append a new string to the end of the string_list.  If
-	`strdup_string` is set, then the string argument is copied;
-	otherwise the new `string_list_entry` refers to the input
-	string.
-
-`string_list_append_nodup`::
-
-	Append a new string to the end of the string_list.  The new
-	`string_list_entry` always refers to the input string, even if
-	`strdup_string` is set.  This function can be used to hand
-	ownership of a malloc()ed string to a `string_list` that has
-	`strdup_string` set.
-
-`string_list_sort`::
-
-	Sort the list's entries by string value in `strcmp()` order.
-
-`unsorted_string_list_has_string`::
-
-	It's like `string_list_has_string()` but for unsorted lists.
-
-`unsorted_string_list_lookup`::
-
-	It's like `string_list_lookup()` but for unsorted lists.
-+
-The above two functions need to look through all items, as opposed to their
-counterpart for sorted lists, which performs a binary search.
-
-`unsorted_string_list_delete_item`::
-
-	Remove an item from a string_list. The `string` pointer of the items
-	will be freed in case the `strdup_strings` member of the string_list
-	is set. The third parameter controls if the `util` pointer of the
-	items should be freed or not.
-
-`string_list_split`::
-`string_list_split_in_place`::
-
-	Split a string into substrings on a delimiter character and
-	append the substrings to a `string_list`.  If `maxsplit` is
-	non-negative, then split at most `maxsplit` times.  Return the
-	number of substrings appended to the list.
-+
-`string_list_split` requires a `string_list` that has `strdup_strings`
-set to true; it leaves the input string untouched and makes copies of
-the substrings in newly-allocated memory.
-`string_list_split_in_place` requires a `string_list` that has
-`strdup_strings` set to false; it splits the input string in place,
-overwriting the delimiter characters with NULs and creating new
-string_list_items that point into the original string (the original
-string must therefore not be modified or freed while the `string_list`
-is in use).
-
-
-Data structures
----------------
-
-* `struct string_list_item`
-
-Represents an item of the list. The `string` member is a pointer to the
-string, and you may use the `util` member for any purpose, if you want.
-
-* `struct string_list`
-
-Represents the list itself.
-
-. The array of items are available via the `items` member.
-. The `nr` member contains the number of items stored in the list.
-. The `alloc` member is used to avoid reallocating at every insertion.
-  You should not tamper with it.
-. Setting the `strdup_strings` member to 1 will strdup() the strings
-  before adding them, see above.
-. The `compare_strings_fn` member is used to specify a custom compare
-  function, otherwise `strcmp()` is used as the default function.
diff --git a/string-list.h b/string-list.h
index 79ae567cb..e63ee4854 100644
--- a/string-list.h
+++ b/string-list.h
@@ -1,6 +1,72 @@
 #ifndef STRING_LIST_H
 #define STRING_LIST_H
 
+/**
+ * The string_list API offers a data structure and functions to handle
+ * sorted and unsorted arrays of strings.  A "sorted" list is one whose
+ * entries are sorted by string value in `strcmp()` order.
+ *
+ * The 'string_list' struct used to be called 'path_list', but was renamed
+ * because it is not specific to paths.
+ *
+ * The caller:
+ *
+ * . Allocates and clears a `struct string_list` variable.
+ *
+ * . Initializes the members. You might want to set the flag `strdup_strings`
+ *   if the strings should be strdup()ed. For example, this is necessary
+ *   when you add something like git_path("..."), since that function returns
+ *   a static buffer that will change with the next call to git_path().
+ *
+ * If you need something advanced, you can manually malloc() the `items`
+ * member (you need this if you add things later) and you should set the
+ * `nr` and `alloc` members in that case, too.
+ *
+ * . Adds new items to the list, using `string_list_append`,
+ *   `string_list_append_nodup`, `string_list_insert`,
+ *   `string_list_split`, and/or `string_list_split_in_place`.
+ *
+ * . Can check if a string is in the list using `string_list_has_string` or
+ *   `unsorted_string_list_has_string` and get it from the list using
+ *   `string_list_lookup` for sorted lists.
+ *
+ * . Can sort an unsorted list using `string_list_sort`.
+ *
+ * . Can remove duplicate items from a sorted list using
+ *   `string_list_remove_duplicates`.
+ *
+ * . Can remove individual items of an unsorted list using
+ *   `unsorted_string_list_delete_item`.
+ *
+ * . Can remove items not matching a criterion from a sorted or unsorted
+ *   list using `filter_string_list`, or remove empty strings using
+ *   `string_list_remove_empty_items`.
+ *
+ * . Finally it should free the list using `string_list_clear`.
+ *
+ * Example:
+ *
+ *     struct string_list list = STRING_LIST_INIT_NODUP;
+ *     int i;
+ *
+ *     string_list_append(&list, "foo");
+ *     string_list_append(&list, "bar");
+ *     for (i = 0; i < list.nr; i++)
+ *             printf("%s\n", list.items[i].string)
+ *
+ * NOTE: It is more efficient to build an unsorted list and sort it
+ * afterwards, instead of building a sorted list (`O(n log n)` instead of
+ * `O(n^2)`).
+ *
+ * However, if you use the list to check if a certain string was added
+ * already, you should not do that (using unsorted_string_list_has_string()),
+ * because the complexity would be quadratic again (but with a worse factor).
+ */
+
+/**
+ * Represents an item of the list. The `string` member is a pointer to the
+ * string, and you may use the `util` member for any purpose, if you want.
+ */
 struct string_list_item {
 	char *string;
 	void *util;
@@ -8,6 +74,18 @@ struct string_list_item {
 
 typedef int (*compare_strings_fn)(const char *, const char *);
 
+/**
+ * Represents the list itself.
+ *
+ * . The array of items are available via the `items` member.
+ * . The `nr` member contains the number of items stored in the list.
+ * . The `alloc` member is used to avoid reallocating at every insertion.
+ *   You should not tamper with it.
+ * . Setting the `strdup_strings` member to 1 will strdup() the strings
+ *   before adding them, see above.
+ * . The `compare_strings_fn` member is used to specify a custom compare
+ *   function, otherwise `strcmp()` is used as the default function.
+ */
 struct string_list {
 	struct string_list_item *items;
 	unsigned int nr, alloc;
@@ -18,35 +96,61 @@ struct string_list {
 #define STRING_LIST_INIT_NODUP { NULL, 0, 0, 0, NULL }
 #define STRING_LIST_INIT_DUP   { NULL, 0, 0, 1, NULL }
 
+/* General functions which work with both sorted and unsorted lists. */
+
+/**
+ * Initialize the members of the string_list, set `strdup_strings`
+ * member according to the value of the second parameter.
+ */
 void string_list_init(struct string_list *list, int strdup_strings);
 
+/**
+ * Apply want to each item in list, retaining only the ones for which
+ * the function returns true.  If free_util is true, call free() on
+ * the util members of any items that have to be deleted.  Preserve
+ * the order of the items that are retained.
+ */
+void filter_string_list(struct string_list *list, int free_util,
+			string_list_each_func_t want, void *cb_data);
+
+/**
+ * Dump a string_list to stdout, useful mainly for debugging
+ * purposes. It can take an optional header argument and it writes out
+ * the string-pointer pairs of the string_list, each one in its own
+ * line.
+ */
 void print_string_list(const struct string_list *p, const char *text);
+
+/**
+ * Free a string_list. The `string` pointer of the items will be freed
+ * in case the `strdup_strings` member of the string_list is set. The
+ * second parameter controls if the `util` pointer of the items should
+ * be freed or not.
+ */
 void string_list_clear(struct string_list *list, int free_util);
 
-/* Use this function to call a custom clear function on each util pointer */
-/* The string associated with the util pointer is passed as the second argument */
+/**
+ * Callback type for `string_list_clear_func`.  The string associated
+ * with the util pointer is passed as the second argument
+ */
 typedef void (*string_list_clear_func_t)(void *p, const char *str);
+
+/** Call a custom clear function on each util pointer */
 void string_list_clear_func(struct string_list *list, string_list_clear_func_t clearfunc);
 
-/* Use this function or the macro below to iterate over each item */
 typedef int (*string_list_each_func_t)(struct string_list_item *, void *);
+
+/** Iterate over each item. */
 int for_each_string_list(struct string_list *list,
 			 string_list_each_func_t, void *cb_data);
+
+/** Iterate over each item, as a macro. */
 #define for_each_string_list_item(item,list)            \
 	for (item = (list)->items;                      \
 	     item && item < (list)->items + (list)->nr; \
 	     ++item)
 
-/*
- * Apply want to each item in list, retaining only the ones for which
- * the function returns true.  If free_util is true, call free() on
- * the util members of any items that have to be deleted.  Preserve
- * the order of the items that are retained.
- */
-void filter_string_list(struct string_list *list, int free_util,
-			string_list_each_func_t want, void *cb_data);
-
-/*
+/**
  * Remove any empty strings from the list.  If free_util is true, call
  * free() on the util members of any items that have to be deleted.
  * Preserve the order of the items that are retained.
@@ -54,25 +158,34 @@ void filter_string_list(struct string_list *list, int free_util,
 void string_list_remove_empty_items(struct string_list *list, int free_util);
 
 /* Use these functions only on sorted lists: */
+
+/** Determine if the string_list has a given string or not. */
 int string_list_has_string(const struct string_list *list, const char *string);
 int string_list_find_insert_index(const struct string_list *list, const char *string,
 				  int negative_existing_index);
-/*
- * Inserts the given string into the sorted list.
- * If the string already exists, the list is not altered.
- * Returns the string_list_item, the string is part of.
+
+/**
+ * Insert a new element to the string_list. The returned pointer can
+ * be handy if you want to write something to the `util` pointer of
+ * the string_list_item containing the just added string. If the given
+ * string already exists the insertion will be skipped and the pointer
+ * to the existing item returned.
+ *
+ * Since this function uses xrealloc() (which die()s if it fails) if the
+ * list needs to grow, it is safe not to check the pointer. I.e. you may
+ * write `string_list_insert(...)->util = ...;`.
  */
 struct string_list_item *string_list_insert(struct string_list *list, const char *string);
 
-/*
- * Removes the given string from the sorted list.
- * If the string doesn't exist, the list is not altered.
+/**
+ * Remove the given string from the sorted list.  If the string
+ * doesn't exist, the list is not altered.
  */
 extern void string_list_remove(struct string_list *list, const char *string,
 			       int free_util);
 
-/*
- * Checks if the given string is part of a sorted list. If it is part of the list,
+/**
+ * Check if the given string is part of a sorted list. If it is part of the list,
  * return the coresponding string_list_item, NULL otherwise.
  */
 struct string_list_item *string_list_lookup(struct string_list *list, const char *string);
@@ -87,14 +200,14 @@ void string_list_remove_duplicates(struct string_list *sorted_list, int free_uti
 
 /* Use these functions only on unsorted lists: */
 
-/*
+/**
  * Add string to the end of list.  If list->strdup_string is set, then
  * string is copied; otherwise the new string_list_entry refers to the
  * input string.
  */
 struct string_list_item *string_list_append(struct string_list *list, const char *string);
 
-/*
+/**
  * Like string_list_append(), except string is never copied.  When
  * list->strdup_strings is set, this function can be used to hand
  * ownership of a malloc()ed string to list without making an extra
@@ -102,16 +215,34 @@ struct string_list_item *string_list_append(struct string_list *list, const char
  */
 struct string_list_item *string_list_append_nodup(struct string_list *list, char *string);
 
+/**
+ * Sort the list's entries by string value in `strcmp()` order.
+ */
 void string_list_sort(struct string_list *list);
+
+/**
+ * Like `string_list_has_string()` but for unsorted lists. Linear in
+ * size of the list.
+ */
 int unsorted_string_list_has_string(struct string_list *list, const char *string);
+
+/**
+ * Like `string_list_lookup()` but for unsorted lists. Linear in size
+ * of the list.
+ */
 struct string_list_item *unsorted_string_list_lookup(struct string_list *list,
 						     const char *string);
-
+/**
+ * Remove an item from a string_list. The `string` pointer of the
+ * items will be freed in case the `strdup_strings` member of the
+ * string_list is set. The third parameter controls if the `util`
+ * pointer of the items should be freed or not.
+ */
 void unsorted_string_list_delete_item(struct string_list *list, int i, int free_util);
 
-/*
- * Split string into substrings on character delim and append the
- * substrings to list.  The input string is not modified.
+/**
+ * Split string into substrings on character `delim` and append the
+ * substrings to `list`.  The input string is not modified.
  * list->strdup_strings must be set, as new memory needs to be
  * allocated to hold the substrings.  If maxsplit is non-negative,
  * then split at most maxsplit times.  Return the number of substrings
-- 
2.14.1.821.g8fa685d3b7-goog

Re: [PATCH 2/4] Clarify return value ownership of real_path and read_gitfile_gently

[Brandon Williams]

On 09/25, Han-Wen Nienhuys wrote:
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
>  abspath.c | 3 +++
>  setup.c   | 3 ++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/abspath.c b/abspath.c
> index 708aff8d4..792a2d533 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -202,6 +202,9 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
>  	return retval;
>  }
>  
> +/* Resolve `path` into an absolute, cleaned-up path. The return value
> + * comes from a global shared buffer and should not be freed.
> + */

nit: Our comment style requires the opening '/*' which starts a comment
block to be on its own line.

>  const char *real_path(const char *path)
>  {
>  	static struct strbuf realpath = STRBUF_INIT;
> diff --git a/setup.c b/setup.c
> index 6d8380acd..31853724c 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -541,7 +541,8 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
>  
>  /*
>   * Try to read the location of the git directory from the .git file,
> - * return path to git directory if found.
> + * return path to git directory if found. The return value comes from
> + * a shared pool and should not be freed.
>   *
>   * On failure, if return_error_code is not NULL, return_error_code
>   * will be set to an error code and NULL will be returned. If
> -- 
> 2.14.1.821.g8fa685d3b7-goog
> 

-- 
Brandon Williams

Re: [PATCH 3/4] Document submodule_to_gitdir

[Brandon Williams]

On 09/25, Han-Wen Nienhuys wrote:
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
>  submodule.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/submodule.c b/submodule.c
> index b12600fc7..b66c23f5d 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1997,6 +1997,9 @@ const char *get_superproject_working_tree(void)
>  	return ret;
>  }
>  
> +/* Put the gitdir for a submodule (given relative to the main repository worktree)
> + * into `buf`, or return -1 on error.
> + */

Same nit as in patch [2/4]

>  int submodule_to_gitdir(struct strbuf *buf, const char *submodule)
>  {
>  	const struct submodule *sub;
> -- 
> 2.14.1.821.g8fa685d3b7-goog
> 

-- 
Brandon Williams

Re: [PATCH 4/4] Move documentation of string_list into string-list.h

[Brandon Williams]

On 09/25, Han-Wen Nienhuys wrote:
> This mirrors commit bdfdaa4978dd92992737e662f25adc01d32d0774 which did

Not really important but most times we reference another commit from a
commit msg we include the one line summary like:
    'bdfdaa497 (strbuf.h: integrate api-strbuf.txt documentation,
    2015-01-16)'

> the same for strbuf.h:
> 
> * API documentation uses /** */ to set it apart from other comments.
> 
> * Function names were stripped from the comments.
> 
> * Ordering of the header was adjusted to follow the one from the text
>   file.
> 
> * Edited some existing comments to follow the new standard.
> 
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
>  Documentation/technical/api-string-list.txt | 209 ----------------------------
>  string-list.h                               | 187 +++++++++++++++++++++----
>  2 files changed, 159 insertions(+), 237 deletions(-)
>  delete mode 100644 Documentation/technical/api-string-list.txt
> 
> diff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt
> deleted file mode 100644
> index c08402b12..000000000
> --- a/Documentation/technical/api-string-list.txt
> +++ /dev/null
> @@ -1,209 +0,0 @@
> -string-list API
> -===============
> -
> -The string_list API offers a data structure and functions to handle
> -sorted and unsorted string lists.  A "sorted" list is one whose
> -entries are sorted by string value in `strcmp()` order.
> -
> -The 'string_list' struct used to be called 'path_list', but was renamed
> -because it is not specific to paths.
> -
> -The caller:
> -
> -. Allocates and clears a `struct string_list` variable.
> -
> -. Initializes the members. You might want to set the flag `strdup_strings`
> -  if the strings should be strdup()ed. For example, this is necessary
> -  when you add something like git_path("..."), since that function returns
> -  a static buffer that will change with the next call to git_path().
> -+
> -If you need something advanced, you can manually malloc() the `items`
> -member (you need this if you add things later) and you should set the
> -`nr` and `alloc` members in that case, too.
> -
> -. Adds new items to the list, using `string_list_append`,
> -  `string_list_append_nodup`, `string_list_insert`,
> -  `string_list_split`, and/or `string_list_split_in_place`.
> -
> -. Can check if a string is in the list using `string_list_has_string` or
> -  `unsorted_string_list_has_string` and get it from the list using
> -  `string_list_lookup` for sorted lists.
> -
> -. Can sort an unsorted list using `string_list_sort`.
> -
> -. Can remove duplicate items from a sorted list using
> -  `string_list_remove_duplicates`.
> -
> -. Can remove individual items of an unsorted list using
> -  `unsorted_string_list_delete_item`.
> -
> -. Can remove items not matching a criterion from a sorted or unsorted
> -  list using `filter_string_list`, or remove empty strings using
> -  `string_list_remove_empty_items`.
> -
> -. Finally it should free the list using `string_list_clear`.
> -
> -Example:
> -
> -----
> -struct string_list list = STRING_LIST_INIT_NODUP;
> -int i;
> -
> -string_list_append(&list, "foo");
> -string_list_append(&list, "bar");
> -for (i = 0; i < list.nr; i++)
> -	printf("%s\n", list.items[i].string)
> -----
> -
> -NOTE: It is more efficient to build an unsorted list and sort it
> -afterwards, instead of building a sorted list (`O(n log n)` instead of
> -`O(n^2)`).
> -+
> -However, if you use the list to check if a certain string was added
> -already, you should not do that (using unsorted_string_list_has_string()),
> -because the complexity would be quadratic again (but with a worse factor).
> -
> -Functions
> ----------
> -
> -* General ones (works with sorted and unsorted lists as well)
> -
> -`string_list_init`::
> -
> -	Initialize the members of the string_list, set `strdup_strings`
> -	member according to the value of the second parameter.
> -
> -`filter_string_list`::
> -
> -	Apply a function to each item in a list, retaining only the
> -	items for which the function returns true.  If free_util is
> -	true, call free() on the util members of any items that have
> -	to be deleted.  Preserve the order of the items that are
> -	retained.
> -
> -`string_list_remove_empty_items`::
> -
> -	Remove any empty strings from the list.  If free_util is true,
> -	call free() on the util members of any items that have to be
> -	deleted.  Preserve the order of the items that are retained.
> -
> -`print_string_list`::
> -
> -	Dump a string_list to stdout, useful mainly for debugging purposes. It
> -	can take an optional header argument and it writes out the
> -	string-pointer pairs of the string_list, each one in its own line.
> -
> -`string_list_clear`::
> -
> -	Free a string_list. The `string` pointer of the items will be freed in
> -	case the `strdup_strings` member of the string_list is set. The second
> -	parameter controls if the `util` pointer of the items should be freed
> -	or not.
> -
> -* Functions for sorted lists only
> -
> -`string_list_has_string`::
> -
> -	Determine if the string_list has a given string or not.
> -
> -`string_list_insert`::
> -
> -	Insert a new element to the string_list. The returned pointer can be
> -	handy if you want to write something to the `util` pointer of the
> -	string_list_item containing the just added string. If the given
> -	string already exists the insertion will be skipped and the
> -	pointer to the existing item returned.
> -+
> -Since this function uses xrealloc() (which die()s if it fails) if the
> -list needs to grow, it is safe not to check the pointer. I.e. you may
> -write `string_list_insert(...)->util = ...;`.
> -
> -`string_list_lookup`::
> -
> -	Look up a given string in the string_list, returning the containing
> -	string_list_item. If the string is not found, NULL is returned.
> -
> -`string_list_remove_duplicates`::
> -
> -	Remove all but the first of consecutive entries that have the
> -	same string value.  If free_util is true, call free() on the
> -	util members of any items that have to be deleted.
> -
> -* Functions for unsorted lists only
> -
> -`string_list_append`::
> -
> -	Append a new string to the end of the string_list.  If
> -	`strdup_string` is set, then the string argument is copied;
> -	otherwise the new `string_list_entry` refers to the input
> -	string.
> -
> -`string_list_append_nodup`::
> -
> -	Append a new string to the end of the string_list.  The new
> -	`string_list_entry` always refers to the input string, even if
> -	`strdup_string` is set.  This function can be used to hand
> -	ownership of a malloc()ed string to a `string_list` that has
> -	`strdup_string` set.
> -
> -`string_list_sort`::
> -
> -	Sort the list's entries by string value in `strcmp()` order.
> -
> -`unsorted_string_list_has_string`::
> -
> -	It's like `string_list_has_string()` but for unsorted lists.
> -
> -`unsorted_string_list_lookup`::
> -
> -	It's like `string_list_lookup()` but for unsorted lists.
> -+
> -The above two functions need to look through all items, as opposed to their
> -counterpart for sorted lists, which performs a binary search.
> -
> -`unsorted_string_list_delete_item`::
> -
> -	Remove an item from a string_list. The `string` pointer of the items
> -	will be freed in case the `strdup_strings` member of the string_list
> -	is set. The third parameter controls if the `util` pointer of the
> -	items should be freed or not.
> -
> -`string_list_split`::
> -`string_list_split_in_place`::
> -
> -	Split a string into substrings on a delimiter character and
> -	append the substrings to a `string_list`.  If `maxsplit` is
> -	non-negative, then split at most `maxsplit` times.  Return the
> -	number of substrings appended to the list.
> -+
> -`string_list_split` requires a `string_list` that has `strdup_strings`
> -set to true; it leaves the input string untouched and makes copies of
> -the substrings in newly-allocated memory.
> -`string_list_split_in_place` requires a `string_list` that has
> -`strdup_strings` set to false; it splits the input string in place,
> -overwriting the delimiter characters with NULs and creating new
> -string_list_items that point into the original string (the original
> -string must therefore not be modified or freed while the `string_list`
> -is in use).
> -
> -
> -Data structures
> ----------------
> -
> -* `struct string_list_item`
> -
> -Represents an item of the list. The `string` member is a pointer to the
> -string, and you may use the `util` member for any purpose, if you want.
> -
> -* `struct string_list`
> -
> -Represents the list itself.
> -
> -. The array of items are available via the `items` member.
> -. The `nr` member contains the number of items stored in the list.
> -. The `alloc` member is used to avoid reallocating at every insertion.
> -  You should not tamper with it.
> -. Setting the `strdup_strings` member to 1 will strdup() the strings
> -  before adding them, see above.
> -. The `compare_strings_fn` member is used to specify a custom compare
> -  function, otherwise `strcmp()` is used as the default function.
> diff --git a/string-list.h b/string-list.h
> index 79ae567cb..e63ee4854 100644
> --- a/string-list.h
> +++ b/string-list.h
> @@ -1,6 +1,72 @@
>  #ifndef STRING_LIST_H
>  #define STRING_LIST_H
>  
> +/**
> + * The string_list API offers a data structure and functions to handle
> + * sorted and unsorted arrays of strings.  A "sorted" list is one whose
> + * entries are sorted by string value in `strcmp()` order.
> + *
> + * The 'string_list' struct used to be called 'path_list', but was renamed
> + * because it is not specific to paths.

This line probably doesn't need to be included in the docs anymore.  I
know this was pretty much a code move so this could be removed in a
follow up patch later.

> + *
> + * The caller:
> + *
> + * . Allocates and clears a `struct string_list` variable.
> + *
> + * . Initializes the members. You might want to set the flag `strdup_strings`
> + *   if the strings should be strdup()ed. For example, this is necessary
> + *   when you add something like git_path("..."), since that function returns
> + *   a static buffer that will change with the next call to git_path().
> + *
> + * If you need something advanced, you can manually malloc() the `items`
> + * member (you need this if you add things later) and you should set the
> + * `nr` and `alloc` members in that case, too.
> + *
> + * . Adds new items to the list, using `string_list_append`,
> + *   `string_list_append_nodup`, `string_list_insert`,
> + *   `string_list_split`, and/or `string_list_split_in_place`.
> + *
> + * . Can check if a string is in the list using `string_list_has_string` or
> + *   `unsorted_string_list_has_string` and get it from the list using
> + *   `string_list_lookup` for sorted lists.
> + *
> + * . Can sort an unsorted list using `string_list_sort`.
> + *
> + * . Can remove duplicate items from a sorted list using
> + *   `string_list_remove_duplicates`.
> + *
> + * . Can remove individual items of an unsorted list using
> + *   `unsorted_string_list_delete_item`.
> + *
> + * . Can remove items not matching a criterion from a sorted or unsorted
> + *   list using `filter_string_list`, or remove empty strings using
> + *   `string_list_remove_empty_items`.
> + *
> + * . Finally it should free the list using `string_list_clear`.
> + *
> + * Example:
> + *
> + *     struct string_list list = STRING_LIST_INIT_NODUP;
> + *     int i;
> + *
> + *     string_list_append(&list, "foo");
> + *     string_list_append(&list, "bar");
> + *     for (i = 0; i < list.nr; i++)
> + *             printf("%s\n", list.items[i].string)
> + *
> + * NOTE: It is more efficient to build an unsorted list and sort it
> + * afterwards, instead of building a sorted list (`O(n log n)` instead of
> + * `O(n^2)`).
> + *
> + * However, if you use the list to check if a certain string was added
> + * already, you should not do that (using unsorted_string_list_has_string()),
> + * because the complexity would be quadratic again (but with a worse factor).
> + */
> +
> +/**
> + * Represents an item of the list. The `string` member is a pointer to the
> + * string, and you may use the `util` member for any purpose, if you want.
> + */
>  struct string_list_item {
>  	char *string;
>  	void *util;
> @@ -8,6 +74,18 @@ struct string_list_item {
>  
>  typedef int (*compare_strings_fn)(const char *, const char *);
>  
> +/**
> + * Represents the list itself.
> + *
> + * . The array of items are available via the `items` member.
> + * . The `nr` member contains the number of items stored in the list.
> + * . The `alloc` member is used to avoid reallocating at every insertion.
> + *   You should not tamper with it.
> + * . Setting the `strdup_strings` member to 1 will strdup() the strings
> + *   before adding them, see above.
> + * . The `compare_strings_fn` member is used to specify a custom compare
> + *   function, otherwise `strcmp()` is used as the default function.
> + */
>  struct string_list {
>  	struct string_list_item *items;
>  	unsigned int nr, alloc;
> @@ -18,35 +96,61 @@ struct string_list {
>  #define STRING_LIST_INIT_NODUP { NULL, 0, 0, 0, NULL }
>  #define STRING_LIST_INIT_DUP   { NULL, 0, 0, 1, NULL }
>  
> +/* General functions which work with both sorted and unsorted lists. */
> +
> +/**
> + * Initialize the members of the string_list, set `strdup_strings`
> + * member according to the value of the second parameter.
> + */
>  void string_list_init(struct string_list *list, int strdup_strings);
>  
> +/**
> + * Apply want to each item in list, retaining only the ones for which

Maybe enclose 'want' in some form of quotes to distinguish that it is a
variable.

> + * the function returns true.  If free_util is true, call free() on
> + * the util members of any items that have to be deleted.  Preserve
> + * the order of the items that are retained.
> + */
> +void filter_string_list(struct string_list *list, int free_util,
> +			string_list_each_func_t want, void *cb_data);
> +
> +/**
> + * Dump a string_list to stdout, useful mainly for debugging
> + * purposes. It can take an optional header argument and it writes out
> + * the string-pointer pairs of the string_list, each one in its own
> + * line.
> + */
>  void print_string_list(const struct string_list *p, const char *text);
> +
> +/**
> + * Free a string_list. The `string` pointer of the items will be freed
> + * in case the `strdup_strings` member of the string_list is set. The
> + * second parameter controls if the `util` pointer of the items should
> + * be freed or not.
> + */
>  void string_list_clear(struct string_list *list, int free_util);
>  
> -/* Use this function to call a custom clear function on each util pointer */
> -/* The string associated with the util pointer is passed as the second argument */
> +/**
> + * Callback type for `string_list_clear_func`.  The string associated
> + * with the util pointer is passed as the second argument
> + */
>  typedef void (*string_list_clear_func_t)(void *p, const char *str);
> +
> +/** Call a custom clear function on each util pointer */
>  void string_list_clear_func(struct string_list *list, string_list_clear_func_t clearfunc);
>  
> -/* Use this function or the macro below to iterate over each item */

Maybe include a comment stating this is the callback type for
'for_each_string_list'

>  typedef int (*string_list_each_func_t)(struct string_list_item *, void *);
> +
> +/** Iterate over each item. */
>  int for_each_string_list(struct string_list *list,
>  			 string_list_each_func_t, void *cb_data);
> +
> +/** Iterate over each item, as a macro. */
>  #define for_each_string_list_item(item,list)            \
>  	for (item = (list)->items;                      \
>  	     item && item < (list)->items + (list)->nr; \
>  	     ++item)
>  
> -/*
> - * Apply want to each item in list, retaining only the ones for which
> - * the function returns true.  If free_util is true, call free() on
> - * the util members of any items that have to be deleted.  Preserve
> - * the order of the items that are retained.
> - */
> -void filter_string_list(struct string_list *list, int free_util,
> -			string_list_each_func_t want, void *cb_data);
> -
> -/*
> +/**
>   * Remove any empty strings from the list.  If free_util is true, call
>   * free() on the util members of any items that have to be deleted.
>   * Preserve the order of the items that are retained.
> @@ -54,25 +158,34 @@ void filter_string_list(struct string_list *list, int free_util,
>  void string_list_remove_empty_items(struct string_list *list, int free_util);
>  
>  /* Use these functions only on sorted lists: */
> +
> +/** Determine if the string_list has a given string or not. */
>  int string_list_has_string(const struct string_list *list, const char *string);
>  int string_list_find_insert_index(const struct string_list *list, const char *string,
>  				  int negative_existing_index);
> -/*
> - * Inserts the given string into the sorted list.
> - * If the string already exists, the list is not altered.
> - * Returns the string_list_item, the string is part of.
> +
> +/**
> + * Insert a new element to the string_list. The returned pointer can
> + * be handy if you want to write something to the `util` pointer of
> + * the string_list_item containing the just added string. If the given
> + * string already exists the insertion will be skipped and the pointer
> + * to the existing item returned.
> + *
> + * Since this function uses xrealloc() (which die()s if it fails) if the
> + * list needs to grow, it is safe not to check the pointer. I.e. you may
> + * write `string_list_insert(...)->util = ...;`.
>   */
>  struct string_list_item *string_list_insert(struct string_list *list, const char *string);
>  
> -/*
> - * Removes the given string from the sorted list.
> - * If the string doesn't exist, the list is not altered.
> +/**
> + * Remove the given string from the sorted list.  If the string
> + * doesn't exist, the list is not altered.
>   */
>  extern void string_list_remove(struct string_list *list, const char *string,
>  			       int free_util);
>  
> -/*
> - * Checks if the given string is part of a sorted list. If it is part of the list,
> +/**
> + * Check if the given string is part of a sorted list. If it is part of the list,
>   * return the coresponding string_list_item, NULL otherwise.
>   */
>  struct string_list_item *string_list_lookup(struct string_list *list, const char *string);
> @@ -87,14 +200,14 @@ void string_list_remove_duplicates(struct string_list *sorted_list, int free_uti
>  
>  /* Use these functions only on unsorted lists: */
>  
> -/*
> +/**
>   * Add string to the end of list.  If list->strdup_string is set, then
>   * string is copied; otherwise the new string_list_entry refers to the
>   * input string.
>   */
>  struct string_list_item *string_list_append(struct string_list *list, const char *string);
>  
> -/*
> +/**
>   * Like string_list_append(), except string is never copied.  When
>   * list->strdup_strings is set, this function can be used to hand
>   * ownership of a malloc()ed string to list without making an extra
> @@ -102,16 +215,34 @@ struct string_list_item *string_list_append(struct string_list *list, const char
>   */
>  struct string_list_item *string_list_append_nodup(struct string_list *list, char *string);
>  
> +/**
> + * Sort the list's entries by string value in `strcmp()` order.
> + */
>  void string_list_sort(struct string_list *list);
> +
> +/**
> + * Like `string_list_has_string()` but for unsorted lists. Linear in
> + * size of the list.
> + */
>  int unsorted_string_list_has_string(struct string_list *list, const char *string);
> +
> +/**
> + * Like `string_list_lookup()` but for unsorted lists. Linear in size
> + * of the list.
> + */
>  struct string_list_item *unsorted_string_list_lookup(struct string_list *list,
>  						     const char *string);
> -
> +/**
> + * Remove an item from a string_list. The `string` pointer of the
> + * items will be freed in case the `strdup_strings` member of the
> + * string_list is set. The third parameter controls if the `util`
> + * pointer of the items should be freed or not.
> + */
>  void unsorted_string_list_delete_item(struct string_list *list, int i, int free_util);
>  
> -/*
> - * Split string into substrings on character delim and append the
> - * substrings to list.  The input string is not modified.
> +/**
> + * Split string into substrings on character `delim` and append the
> + * substrings to `list`.  The input string is not modified.
>   * list->strdup_strings must be set, as new memory needs to be
>   * allocated to hold the substrings.  If maxsplit is non-negative,
>   * then split at most maxsplit times.  Return the number of substrings
> -- 
> 2.14.1.821.g8fa685d3b7-goog
> 

-- 
Brandon Williams

Re: [PATCH 4/4] Move documentation of string_list into string-list.h

[Stefan Beller]

On Mon, Sep 25, 2017 at 10:40 AM, Brandon Williams <bmwill@google.com> wrote:
> On 09/25, Han-Wen Nienhuys wrote:
>> This mirrors commit bdfdaa4978dd92992737e662f25adc01d32d0774 which did
>
> Not really important but most times we reference another commit from a
> commit msg we include the one line summary like:
>     'bdfdaa497 (strbuf.h: integrate api-strbuf.txt documentation,
>     2015-01-16)'

I have

    alias.gcs=show --date=short -s --pretty='format:%h (%s, %ad)'

in the config file for that.

Thanks for converting documentation into header files!

Stefan

Re: [PATCH 2/4] Clarify return value ownership of real_path and read_gitfile_gently

[Junio C Hamano]

Han-Wen Nienhuys <hanwen@google.com> writes:

>>Subject: Re: [PATCH 2/4] Clarify return value ownership of real_path and read_gitfile_gently

We try to make "shortlog --no-merges" output consistently say what
area the change is about, followed by a colon, followed by a short
description.

> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
>  abspath.c | 3 +++
>  setup.c   | 3 ++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/abspath.c b/abspath.c
> index 708aff8d4..792a2d533 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -202,6 +202,9 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
>  	return retval;
>  }
>  
> +/* Resolve `path` into an absolute, cleaned-up path. The return value
> + * comes from a global shared buffer and should not be freed.
> + */
>  const char *real_path(const char *path)
>  {
>  	static struct strbuf realpath = STRBUF_INIT;

The part about "what it does" is a good thing to have here, but I do
not think the second sentence adds much if it stays here (if the
comment were in a header file far from implementation, then it is a
different matter).  Besides, "should not be freed" is not the only
important thing---it is already implied by the function returning
"const char *" (i.e. the caller/receiver does not own it).  It will
stay valid only until the next call, so the caller needs to xstrdup()
it if it wants to keep the value.  That is equally important.

But quite honestly, that pattern appears very often in our codebase
(all users of get_pathname() share the same characteristics) that
these details (i.e. do not free, expect the buffer to be recycled)
probaly is not worth it.  Mentioning that the returned value is in a
shared buffer for short-term use would be sufficient.

> diff --git a/setup.c b/setup.c
> index 6d8380acd..31853724c 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -541,7 +541,8 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
>  
>  /*
>   * Try to read the location of the git directory from the .git file,
> - * return path to git directory if found.
> + * return path to git directory if found. The return value comes from
> + * a shared pool and should not be freed.

OK, the returned value comes from the return value of real_path(),
so the early half of the new warning is worth having.  "should not
be freed" is both extraneous (for those who are already aware of the
common pattern in our codebase) and insufficient (for those who are
not yet).

Thanks.

>   *
>   * On failure, if return_error_code is not NULL, return_error_code
>   * will be set to an error code and NULL will be returned. If

Re: [PATCH 4/4] Move documentation of string_list into string-list.h

[Junio C Hamano]

Han-Wen Nienhuys <hanwen@google.com> writes:

> This mirrors commit bdfdaa4978dd92992737e662f25adc01d32d0774 which did
> the same for strbuf.h:
>
> * API documentation uses /** */ to set it apart from other comments.
>
> * Function names were stripped from the comments.
>
> * Ordering of the header was adjusted to follow the one from the text
>   file.
>
> * Edited some existing comments to follow the new standard.
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---

Thanks.  I am not sure if you can safely reorder the contents of the
header files in general, but I trust that you made sure that this
does not introduce problems (e.g. referrals before definition).

Also, I am not sure what "the new standard" refers to.  Have we
established a new standard somewhere?  Did we have discussion on it?
Puzzled.

Thanks.


>  Documentation/technical/api-string-list.txt | 209 ----------------------------
>  string-list.h                               | 187 +++++++++++++++++++++----
>  2 files changed, 159 insertions(+), 237 deletions(-)
>  delete mode 100644 Documentation/technical/api-string-list.txt
>
> diff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt
> deleted file mode 100644
> index c08402b12..000000000
> --- a/Documentation/technical/api-string-list.txt
> +++ /dev/null
> @@ -1,209 +0,0 @@
> -string-list API
> -===============
> -
> -The string_list API offers a data structure and functions to handle
> -sorted and unsorted string lists.  A "sorted" list is one whose
> -entries are sorted by string value in `strcmp()` order.
> -
> -The 'string_list' struct used to be called 'path_list', but was renamed
> -because it is not specific to paths.
> -
> -The caller:
> -
> -. Allocates and clears a `struct string_list` variable.
> -
> -. Initializes the members. You might want to set the flag `strdup_strings`
> -  if the strings should be strdup()ed. For example, this is necessary
> -  when you add something like git_path("..."), since that function returns
> -  a static buffer that will change with the next call to git_path().
> -+
> -If you need something advanced, you can manually malloc() the `items`
> -member (you need this if you add things later) and you should set the
> -`nr` and `alloc` members in that case, too.
> -
> -. Adds new items to the list, using `string_list_append`,
> -  `string_list_append_nodup`, `string_list_insert`,
> -  `string_list_split`, and/or `string_list_split_in_place`.
> -
> -. Can check if a string is in the list using `string_list_has_string` or
> -  `unsorted_string_list_has_string` and get it from the list using
> -  `string_list_lookup` for sorted lists.
> -
> -. Can sort an unsorted list using `string_list_sort`.
> -
> -. Can remove duplicate items from a sorted list using
> -  `string_list_remove_duplicates`.
> -
> -. Can remove individual items of an unsorted list using
> -  `unsorted_string_list_delete_item`.
> -
> -. Can remove items not matching a criterion from a sorted or unsorted
> -  list using `filter_string_list`, or remove empty strings using
> -  `string_list_remove_empty_items`.
> -
> -. Finally it should free the list using `string_list_clear`.
> -
> -Example:
> -
> -----
> -struct string_list list = STRING_LIST_INIT_NODUP;
> -int i;
> -
> -string_list_append(&list, "foo");
> -string_list_append(&list, "bar");
> -for (i = 0; i < list.nr; i++)
> -	printf("%s\n", list.items[i].string)
> -----
> -
> -NOTE: It is more efficient to build an unsorted list and sort it
> -afterwards, instead of building a sorted list (`O(n log n)` instead of
> -`O(n^2)`).
> -+
> -However, if you use the list to check if a certain string was added
> -already, you should not do that (using unsorted_string_list_has_string()),
> -because the complexity would be quadratic again (but with a worse factor).
> -
> -Functions
> ----------
> -
> -* General ones (works with sorted and unsorted lists as well)
> -
> -`string_list_init`::
> -
> -	Initialize the members of the string_list, set `strdup_strings`
> -	member according to the value of the second parameter.
> -
> -`filter_string_list`::
> -
> -	Apply a function to each item in a list, retaining only the
> -	items for which the function returns true.  If free_util is
> -	true, call free() on the util members of any items that have
> -	to be deleted.  Preserve the order of the items that are
> -	retained.
> -
> -`string_list_remove_empty_items`::
> -
> -	Remove any empty strings from the list.  If free_util is true,
> -	call free() on the util members of any items that have to be
> -	deleted.  Preserve the order of the items that are retained.
> -
> -`print_string_list`::
> -
> -	Dump a string_list to stdout, useful mainly for debugging purposes. It
> -	can take an optional header argument and it writes out the
> -	string-pointer pairs of the string_list, each one in its own line.
> -
> -`string_list_clear`::
> -
> -	Free a string_list. The `string` pointer of the items will be freed in
> -	case the `strdup_strings` member of the string_list is set. The second
> -	parameter controls if the `util` pointer of the items should be freed
> -	or not.
> -
> -* Functions for sorted lists only
> -
> -`string_list_has_string`::
> -
> -	Determine if the string_list has a given string or not.
> -
> -`string_list_insert`::
> -
> -	Insert a new element to the string_list. The returned pointer can be
> -	handy if you want to write something to the `util` pointer of the
> -	string_list_item containing the just added string. If the given
> -	string already exists the insertion will be skipped and the
> -	pointer to the existing item returned.
> -+
> -Since this function uses xrealloc() (which die()s if it fails) if the
> -list needs to grow, it is safe not to check the pointer. I.e. you may
> -write `string_list_insert(...)->util = ...;`.
> -
> -`string_list_lookup`::
> -
> -	Look up a given string in the string_list, returning the containing
> -	string_list_item. If the string is not found, NULL is returned.
> -
> -`string_list_remove_duplicates`::
> -
> -	Remove all but the first of consecutive entries that have the
> -	same string value.  If free_util is true, call free() on the
> -	util members of any items that have to be deleted.
> -
> -* Functions for unsorted lists only
> -
> -`string_list_append`::
> -
> -	Append a new string to the end of the string_list.  If
> -	`strdup_string` is set, then the string argument is copied;
> -	otherwise the new `string_list_entry` refers to the input
> -	string.
> -
> -`string_list_append_nodup`::
> -
> -	Append a new string to the end of the string_list.  The new
> -	`string_list_entry` always refers to the input string, even if
> -	`strdup_string` is set.  This function can be used to hand
> -	ownership of a malloc()ed string to a `string_list` that has
> -	`strdup_string` set.
> -
> -`string_list_sort`::
> -
> -	Sort the list's entries by string value in `strcmp()` order.
> -
> -`unsorted_string_list_has_string`::
> -
> -	It's like `string_list_has_string()` but for unsorted lists.
> -
> -`unsorted_string_list_lookup`::
> -
> -	It's like `string_list_lookup()` but for unsorted lists.
> -+
> -The above two functions need to look through all items, as opposed to their
> -counterpart for sorted lists, which performs a binary search.
> -
> -`unsorted_string_list_delete_item`::
> -
> -	Remove an item from a string_list. The `string` pointer of the items
> -	will be freed in case the `strdup_strings` member of the string_list
> -	is set. The third parameter controls if the `util` pointer of the
> -	items should be freed or not.
> -
> -`string_list_split`::
> -`string_list_split_in_place`::
> -
> -	Split a string into substrings on a delimiter character and
> -	append the substrings to a `string_list`.  If `maxsplit` is
> -	non-negative, then split at most `maxsplit` times.  Return the
> -	number of substrings appended to the list.
> -+
> -`string_list_split` requires a `string_list` that has `strdup_strings`
> -set to true; it leaves the input string untouched and makes copies of
> -the substrings in newly-allocated memory.
> -`string_list_split_in_place` requires a `string_list` that has
> -`strdup_strings` set to false; it splits the input string in place,
> -overwriting the delimiter characters with NULs and creating new
> -string_list_items that point into the original string (the original
> -string must therefore not be modified or freed while the `string_list`
> -is in use).
> -
> -
> -Data structures
> ----------------
> -
> -* `struct string_list_item`
> -
> -Represents an item of the list. The `string` member is a pointer to the
> -string, and you may use the `util` member for any purpose, if you want.
> -
> -* `struct string_list`
> -
> -Represents the list itself.
> -
> -. The array of items are available via the `items` member.
> -. The `nr` member contains the number of items stored in the list.
> -. The `alloc` member is used to avoid reallocating at every insertion.
> -  You should not tamper with it.
> -. Setting the `strdup_strings` member to 1 will strdup() the strings
> -  before adding them, see above.
> -. The `compare_strings_fn` member is used to specify a custom compare
> -  function, otherwise `strcmp()` is used as the default function.
> diff --git a/string-list.h b/string-list.h
> index 79ae567cb..e63ee4854 100644
> --- a/string-list.h
> +++ b/string-list.h
> @@ -1,6 +1,72 @@
>  #ifndef STRING_LIST_H
>  #define STRING_LIST_H
>  
> +/**
> + * The string_list API offers a data structure and functions to handle
> + * sorted and unsorted arrays of strings.  A "sorted" list is one whose
> + * entries are sorted by string value in `strcmp()` order.
> + *
> + * The 'string_list' struct used to be called 'path_list', but was renamed
> + * because it is not specific to paths.
> + *
> + * The caller:
> + *
> + * . Allocates and clears a `struct string_list` variable.
> + *
> + * . Initializes the members. You might want to set the flag `strdup_strings`
> + *   if the strings should be strdup()ed. For example, this is necessary
> + *   when you add something like git_path("..."), since that function returns
> + *   a static buffer that will change with the next call to git_path().
> + *
> + * If you need something advanced, you can manually malloc() the `items`
> + * member (you need this if you add things later) and you should set the
> + * `nr` and `alloc` members in that case, too.
> + *
> + * . Adds new items to the list, using `string_list_append`,
> + *   `string_list_append_nodup`, `string_list_insert`,
> + *   `string_list_split`, and/or `string_list_split_in_place`.
> + *
> + * . Can check if a string is in the list using `string_list_has_string` or
> + *   `unsorted_string_list_has_string` and get it from the list using
> + *   `string_list_lookup` for sorted lists.
> + *
> + * . Can sort an unsorted list using `string_list_sort`.
> + *
> + * . Can remove duplicate items from a sorted list using
> + *   `string_list_remove_duplicates`.
> + *
> + * . Can remove individual items of an unsorted list using
> + *   `unsorted_string_list_delete_item`.
> + *
> + * . Can remove items not matching a criterion from a sorted or unsorted
> + *   list using `filter_string_list`, or remove empty strings using
> + *   `string_list_remove_empty_items`.
> + *
> + * . Finally it should free the list using `string_list_clear`.
> + *
> + * Example:
> + *
> + *     struct string_list list = STRING_LIST_INIT_NODUP;
> + *     int i;
> + *
> + *     string_list_append(&list, "foo");
> + *     string_list_append(&list, "bar");
> + *     for (i = 0; i < list.nr; i++)
> + *             printf("%s\n", list.items[i].string)
> + *
> + * NOTE: It is more efficient to build an unsorted list and sort it
> + * afterwards, instead of building a sorted list (`O(n log n)` instead of
> + * `O(n^2)`).
> + *
> + * However, if you use the list to check if a certain string was added
> + * already, you should not do that (using unsorted_string_list_has_string()),
> + * because the complexity would be quadratic again (but with a worse factor).
> + */
> +
> +/**
> + * Represents an item of the list. The `string` member is a pointer to the
> + * string, and you may use the `util` member for any purpose, if you want.
> + */
>  struct string_list_item {
>  	char *string;
>  	void *util;
> @@ -8,6 +74,18 @@ struct string_list_item {
>  
>  typedef int (*compare_strings_fn)(const char *, const char *);
>  
> +/**
> + * Represents the list itself.
> + *
> + * . The array of items are available via the `items` member.
> + * . The `nr` member contains the number of items stored in the list.
> + * . The `alloc` member is used to avoid reallocating at every insertion.
> + *   You should not tamper with it.
> + * . Setting the `strdup_strings` member to 1 will strdup() the strings
> + *   before adding them, see above.
> + * . The `compare_strings_fn` member is used to specify a custom compare
> + *   function, otherwise `strcmp()` is used as the default function.
> + */
>  struct string_list {
>  	struct string_list_item *items;
>  	unsigned int nr, alloc;
> @@ -18,35 +96,61 @@ struct string_list {
>  #define STRING_LIST_INIT_NODUP { NULL, 0, 0, 0, NULL }
>  #define STRING_LIST_INIT_DUP   { NULL, 0, 0, 1, NULL }
>  
> +/* General functions which work with both sorted and unsorted lists. */
> +
> +/**
> + * Initialize the members of the string_list, set `strdup_strings`
> + * member according to the value of the second parameter.
> + */
>  void string_list_init(struct string_list *list, int strdup_strings);
>  
> +/**
> + * Apply want to each item in list, retaining only the ones for which
> + * the function returns true.  If free_util is true, call free() on
> + * the util members of any items that have to be deleted.  Preserve
> + * the order of the items that are retained.
> + */
> +void filter_string_list(struct string_list *list, int free_util,
> +			string_list_each_func_t want, void *cb_data);
> +
> +/**
> + * Dump a string_list to stdout, useful mainly for debugging
> + * purposes. It can take an optional header argument and it writes out
> + * the string-pointer pairs of the string_list, each one in its own
> + * line.
> + */
>  void print_string_list(const struct string_list *p, const char *text);
> +
> +/**
> + * Free a string_list. The `string` pointer of the items will be freed
> + * in case the `strdup_strings` member of the string_list is set. The
> + * second parameter controls if the `util` pointer of the items should
> + * be freed or not.
> + */
>  void string_list_clear(struct string_list *list, int free_util);
>  
> -/* Use this function to call a custom clear function on each util pointer */
> -/* The string associated with the util pointer is passed as the second argument */
> +/**
> + * Callback type for `string_list_clear_func`.  The string associated
> + * with the util pointer is passed as the second argument
> + */
>  typedef void (*string_list_clear_func_t)(void *p, const char *str);
> +
> +/** Call a custom clear function on each util pointer */
>  void string_list_clear_func(struct string_list *list, string_list_clear_func_t clearfunc);
>  
> -/* Use this function or the macro below to iterate over each item */
>  typedef int (*string_list_each_func_t)(struct string_list_item *, void *);
> +
> +/** Iterate over each item. */
>  int for_each_string_list(struct string_list *list,
>  			 string_list_each_func_t, void *cb_data);
> +
> +/** Iterate over each item, as a macro. */
>  #define for_each_string_list_item(item,list)            \
>  	for (item = (list)->items;                      \
>  	     item && item < (list)->items + (list)->nr; \
>  	     ++item)
>  
> -/*
> - * Apply want to each item in list, retaining only the ones for which
> - * the function returns true.  If free_util is true, call free() on
> - * the util members of any items that have to be deleted.  Preserve
> - * the order of the items that are retained.
> - */
> -void filter_string_list(struct string_list *list, int free_util,
> -			string_list_each_func_t want, void *cb_data);
> -
> -/*
> +/**
>   * Remove any empty strings from the list.  If free_util is true, call
>   * free() on the util members of any items that have to be deleted.
>   * Preserve the order of the items that are retained.
> @@ -54,25 +158,34 @@ void filter_string_list(struct string_list *list, int free_util,
>  void string_list_remove_empty_items(struct string_list *list, int free_util);
>  
>  /* Use these functions only on sorted lists: */
> +
> +/** Determine if the string_list has a given string or not. */
>  int string_list_has_string(const struct string_list *list, const char *string);
>  int string_list_find_insert_index(const struct string_list *list, const char *string,
>  				  int negative_existing_index);
> -/*
> - * Inserts the given string into the sorted list.
> - * If the string already exists, the list is not altered.
> - * Returns the string_list_item, the string is part of.
> +
> +/**
> + * Insert a new element to the string_list. The returned pointer can
> + * be handy if you want to write something to the `util` pointer of
> + * the string_list_item containing the just added string. If the given
> + * string already exists the insertion will be skipped and the pointer
> + * to the existing item returned.
> + *
> + * Since this function uses xrealloc() (which die()s if it fails) if the
> + * list needs to grow, it is safe not to check the pointer. I.e. you may
> + * write `string_list_insert(...)->util = ...;`.
>   */
>  struct string_list_item *string_list_insert(struct string_list *list, const char *string);
>  
> -/*
> - * Removes the given string from the sorted list.
> - * If the string doesn't exist, the list is not altered.
> +/**
> + * Remove the given string from the sorted list.  If the string
> + * doesn't exist, the list is not altered.
>   */
>  extern void string_list_remove(struct string_list *list, const char *string,
>  			       int free_util);
>  
> -/*
> - * Checks if the given string is part of a sorted list. If it is part of the list,
> +/**
> + * Check if the given string is part of a sorted list. If it is part of the list,
>   * return the coresponding string_list_item, NULL otherwise.
>   */
>  struct string_list_item *string_list_lookup(struct string_list *list, const char *string);
> @@ -87,14 +200,14 @@ void string_list_remove_duplicates(struct string_list *sorted_list, int free_uti
>  
>  /* Use these functions only on unsorted lists: */
>  
> -/*
> +/**
>   * Add string to the end of list.  If list->strdup_string is set, then
>   * string is copied; otherwise the new string_list_entry refers to the
>   * input string.
>   */
>  struct string_list_item *string_list_append(struct string_list *list, const char *string);
>  
> -/*
> +/**
>   * Like string_list_append(), except string is never copied.  When
>   * list->strdup_strings is set, this function can be used to hand
>   * ownership of a malloc()ed string to list without making an extra
> @@ -102,16 +215,34 @@ struct string_list_item *string_list_append(struct string_list *list, const char
>   */
>  struct string_list_item *string_list_append_nodup(struct string_list *list, char *string);
>  
> +/**
> + * Sort the list's entries by string value in `strcmp()` order.
> + */
>  void string_list_sort(struct string_list *list);
> +
> +/**
> + * Like `string_list_has_string()` but for unsorted lists. Linear in
> + * size of the list.
> + */
>  int unsorted_string_list_has_string(struct string_list *list, const char *string);
> +
> +/**
> + * Like `string_list_lookup()` but for unsorted lists. Linear in size
> + * of the list.
> + */
>  struct string_list_item *unsorted_string_list_lookup(struct string_list *list,
>  						     const char *string);
> -
> +/**
> + * Remove an item from a string_list. The `string` pointer of the
> + * items will be freed in case the `strdup_strings` member of the
> + * string_list is set. The third parameter controls if the `util`
> + * pointer of the items should be freed or not.
> + */
>  void unsorted_string_list_delete_item(struct string_list *list, int i, int free_util);
>  
> -/*
> - * Split string into substrings on character delim and append the
> - * substrings to list.  The input string is not modified.
> +/**
> + * Split string into substrings on character `delim` and append the
> + * substrings to `list`.  The input string is not modified.
>   * list->strdup_strings must be set, as new memory needs to be
>   * allocated to hold the substrings.  If maxsplit is non-negative,
>   * then split at most maxsplit times.  Return the number of substrings

Re: [PATCH 4/4] Move documentation of string_list into string-list.h

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>
> Thanks.  I am not sure if you can safely reorder the contents of the
> header files in general, but I trust that you made sure that this
> does not introduce problems (e.g. referrals before definition).

Alas, this time it seems my trust was grossly misplaced.  Discarding
this patch and redoing the integration cycle for the day.

Re: [PATCH 4/4] Move documentation of string_list into string-list.h

[Han-Wen Nienhuys]

On Tue, Sep 26, 2017 at 7:22 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>
>> Thanks.  I am not sure if you can safely reorder the contents of the
>> header files in general, but I trust that you made sure that this
>> does not introduce problems (e.g. referrals before definition).
>
> Alas, this time it seems my trust was grossly misplaced.  Discarding
> this patch and redoing the integration cycle for the day.

Oops, my bad.

I resent the remaining patches. They do compile now :)

--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Re: [PATCH 4/4] Move documentation of string_list into string-list.h

[Andrey Rybak]

On 25.09.2017 20:40, Brandon Williams wrote:
> On 09/25, Han-Wen Nienhuys wrote:
>> This mirrors commit bdfdaa4978dd92992737e662f25adc01d32d0774 which did
> 
> Not really important but most times we reference another commit from a
> commit msg we include the one line summary like:
>     'bdfdaa497 (strbuf.h: integrate api-strbuf.txt documentation,
>     2015-01-16)'
> 

Also available from the context menu in gitk as "Copy commit summary".

--
Best regards, Andrey Rybak