Re: [PATCH v3 4/7] notes copy --stdin: split lines with string_list_split()

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 28, 2016, 16:02 UTC
Message-ID: <CAPig+cSOjdCkMAKEJ+7o=-cGsLgZWG5mnVmkWLSbyedg=mY5Lw@mail.gmail.com>
In-Reply-To: <56D2A60A.4000306@moritzneeb.de>

On Sun, Feb 28, 2016 at 2:47 AM, Moritz Neeb <lists@moritzneeb.de> wrote:

Show 16 quoted lines

> On 02/28/2016 07:56 AM, Eric Sunshine wrote:
>> On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb <lists@moritzneeb.de> wrote:
>>> The strbuf is now rtrimmed before splitting. This is still required
>>> to remove potential CRs. In the next step this will then be done
>>> implicitly by strbuf_readline(). Thus, this is a preparatory refactoring,
>>> towards a trim-free codepath.
>>
>> I would actually swap patches 4 and 5 so that strbuf_getline() is done
>> first (without removing any of the rtrim's) and string_list_split()
>> second. That way, you don't have to add that extra rtrim in one patch
>> and immediately remove it in the next. And, as a bonus, you can drop
>> the above paragraph altogether.
>
> Yeah, I also was thinking about that, should've pointed that out.
> I was just following your "guiding" in v2 [1], that's why I did it this way,
> because I thought it is somehow expected to be a prepraratory change.

Indeed, I meant to add a footnote acknowledging that and saying something along the lines of: "Despite suggesting this as a preparatory change in my v2 review, having now seen actually seen it, it makes more sense as a follow-on change."

> Ok, when switching 4 and 5, I could call it something like "post cleanup/refactoring"
> instead.
>
> [1] http://article.gmane.org/gmane.comp.version-control.git/286868