Re: [PATCHv8 1/5] string list: improve comment

From: Junio C Hamano <gitster@pobox.com>
Date: May 19, 2016, 18:08 UTC
Message-ID: <xmqq60u9kjjh.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160519010935.27856-2-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 15 quoted lines

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  string-list.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/string-list.h b/string-list.h
> index d3809a1..465a1f0 100644
> --- a/string-list.h
> +++ b/string-list.h
> @@ -106,7 +106,7 @@ void unsorted_string_list_delete_item(struct string_list *list, int i, int free_
>   * 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
> - * appended to list.
> + * appended to list. The list may be non-empty already.

I personally find that the original comment is clear enough, though.

When somebody says "resulting elements of the split are appended to the list" without saying either:

  a. "the list MUST be empty in the beginning", or
  b. "the list will be emptied first before the split result are appended",

wouldn't it be natural to take it as "you can append them to any list, be it empty or not, and they are _appended_, not replaced"?

So while this is not incorrect per-se, I am not sure if it adds much value. If somebody needs this additional clarification, because the original did not say a. above, she would certainly need more clarification because the original did not say b. above, either.

"The list may be non-empty already", but she would keep wondering if the existing contents would be discarded before the result gets appended to it.

You may say "No, there won't be such a confusion, because we say 'append'; empty and then append is 'replace'". But then the same logic would say "There cannot be a requirement for the list to be empty in the first place, because we say 'append'".

So...