Re: [PATCH v3] gpg-interface: trim CR from ssh-keygen

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

Eric Sunshine <sunshine@sunshineco.com> writes:

> of the existing string-splitting functions. For instance, something
> like this:
>
>     struct strbuf **line, **to_free;
>     line = to_free = strbuf_split(&ssh_principals_out, '\n');
>     for (; *line; line++) {
>         strbuf_trim_trailing_newline(*line);
>         if (!(*line)->len)
>             continue;
>         principal = (*line)->buf;
>
> keeping in mind that strbuf_trim_trailing_newline() takes care of
> CR/LF, and with appropriate cleanup at the end of the loop:
>
>         strbuf_list_free(to_free);
>
> (and removal of `FREE_AND_NULL(principal)` which is no longer needed).
>
> Something similar can be done with string_list_split(), as well.

Unless you are writing an interactive text editor, an array of
lines, each of which can individually be manupulated cheaply when
inserting or deleting a span of chars, is a way too ugly and overly
expensive data structure to keep your data in the long haul.  In
short, strbuf_split() was a mistaken piece of API that does not
belong to this project ;-)

The cycles spent by crypto before getting to this point in the code
is expensive enough that the extra cycles to separately scan to
split them into lines and another scan from the end of the each line
to trim may not matter, so I'd stop at saying "I'd rather not to see
the above code" instead of my usual "Please don't", from performance
perspective in this case.

But from code cleanliness perspective, well, let me just say that
this is not Python or Java but a C project.