On Thu, May 19, 2016 at 11:08 AM, Junio C Hamano <gits...@pobox.com> wrote: > Stefan Beller <sbel...@google.com> writes: > >> Signed-off-by: Stefan Beller <sbel...@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"?
That is true. I missed that though when reading the documentation and read the source code to be clear. > > 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... So, please drop? I do not feel strongly about this patch. I basically wrote to for myself after I consulted the source. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
