Re: [PATCH 02/11] string-list.h: add string_list_{pop, last} functions

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> A string list can be used as a stack, but should we?

verb missing.  "We can use a string list as ..., but should we?" is
readable.  "A string list can be ..., but should it be?" also is.

> A later patch shows
> how useful this will be.

Instead of all of the above, how about being more direct, i.e. e.g.

	Add a few functions to allow a string-list to be used as a
	stack:

	- string_list_last() lets a caller peek the string_list_item
          at the end of the string list.  The caller needs to be
          aware that it is borrowing a pointer, which can become
          invalid if/when the string_list is resized.

	- string_list_pop() removes the string_list_item at the end
          of the string list.

	You can use them in this pattern:

		while (list.nr) {
			struct string_list_item *item = string_list_last(&list);

			work_on(item);
			string_list_pop(&list);
		}

Then the readers would immediately have enough information to judge
if the new API is useful without blindly trusting what you promise.

Two things to note in the above suggested rewrite are:

 - It is curious that _pop() is not accompanied by _push().  It
   probably is a good idea to add the third bullet point that says
   "existing string_list_append() can be used in place of the
   missing string_list_push()."

 - As it already lays out the memory ownership issue, it probably
   makes the description we see below unnecessary (besides, the code
   snippet in there were buggy in at least two counts).

> In an earlier iteration of this patch it was suggested to return the last
> ...
> Also provide the function to access the last item in the list.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  string-list.c | 14 ++++++++++++++
>  string-list.h | 11 +++++++++++
>  2 files changed, 25 insertions(+)
>
> diff --git a/string-list.c b/string-list.c
> index 1ebbe1f56ea..21559f222a7 100644
> --- a/string-list.c
> +++ b/string-list.c
> @@ -80,6 +80,20 @@ void string_list_remove(struct string_list *list, const char *string,
>  	}
>  }
>  
> +void string_list_pop(struct string_list *list, int free_util)
> +{
> +	if (list->nr == 0)
> +		BUG("tried to remove an item from empty string list");
> +
> +	if (list->strdup_strings)
> +		free(list->items[list->nr - 1].string);
> +
> +	if (free_util)
> +		free(list->items[list->nr - 1].util);
> +
> +	list->nr--;
> +}

Conceptually, this allows string_list_clear() to be implemented in
terms of this function, i.e.

	string_list_clear(struct string_list *list, int free_util)
	{
		while (list->nr)
			string_list_pop(list, free_util);
		free(list->items);
		list->items = NULL;
		list->nr = list->alloc = 0;
	}

It is unfortunate that string_list has become such a kitchen-sink in
that a string list whose util field is used in such a way that
string_list_clear_func() needs to be used to clear the list cannot
be used as a stack.  Ideally each of these "features" (like
"sorted/unsorted", "owning/borrowing util", "owning/borrowing
string", etc.)  ought to be orthogonal, but they are not.

> diff --git a/string-list.h b/string-list.h
> index 5b22560cf19..d7cdf38e57a 100644
> --- a/string-list.h
> +++ b/string-list.h
> @@ -191,6 +191,17 @@ extern void string_list_remove(struct string_list *list, const char *string,
>   */
>  struct string_list_item *string_list_lookup(struct string_list *list, const char *string);
>  
> +/**
> + * Removes the last item from the list.
> + * The caller must ensure that the list is not empty.
> + */
> +void string_list_pop(struct string_list *list, int free_util);
> +
> +inline struct string_list_item *string_list_last(struct string_list *list)
> +{
> +	return &list->items[list->nr - 1];
> +}

inline functions declared in a header file must be marked "static
inline", I think.

>  /*
>   * Remove all but the first of consecutive entries with the same
>   * string value.  If free_util is true, call free() on the util