Re: [PATCH 1/9] string-list: add string_list_{pop, last} functions

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

Stefan Beller <sbeller@google.com> writes:

> 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.
>
>  - _pop usually has a friend _push. This role is taken by
>     string_list_append already, as they are not symmetrical
>     in our code base: _append returns the pointer, such that
>     adding a util is easy, but _pop doesn't return such a pointer.

This third-item somehow is indented one place too deeply.

"our pop() does not return a pointer, so we won't introduce push()
that does return a pointer"?  Am I reading the third item right?

pop() and push() usually go together, but they are opposite
operations, they are expected to behave oppositely, and they are
expected to be interfaced differently.  pop() that does not return a
pointer is no better or no worse match to push() that returns a
pointer, no?

Lack of something_push(), when something_pop() exists, would always
be surprising to anybody new to the "something" API, and no amount
of mumbling would justify it, I would think, but this third item
sounds like it is trying to make a lame excuse when there is no need
to.

Wouldn't it be sufficient to say "there is no string_list_push();
string_list_append() can be used in its place" and stop there?

> 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);
>     }

"free_util" as the second argument to this sample call is missing.

>  string-list.c | 14 ++++++++++++++
>  string-list.h | 11 +++++++++++
>  2 files changed, 25 insertions(+)

> diff --git a/string-list.c b/string-list.c
> index 771c4550980..04db2b537c0 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--;
> +}
> +
>  int string_list_has_string(const struct string_list *list, const char *string)
>  {
>  	int exact_match;
> diff --git a/string-list.h b/string-list.h
> index ff8f6094a33..ce2528bbe15 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);
> +
> +static inline struct string_list_item *string_list_last(struct string_list *list)

We may want to warn users that pop(), append(), etc. shouldn't be
done while using the returned value from this function in an in-code
comment or docstring.

> +{
> +	return &list->items[list->nr - 1];
> +}
> +

Other than that, nicely done.

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