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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 10, 2022, 17:03 UTC
Message-ID: <xmqq8rvn34mf.fsf@gitster.g>
In-Reply-To: <YdtVrT4gBvnXfNr6@flurp.local>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 19 quoted lines

> 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.