From: Eric Sunshine Date: Sun, 28 Feb 2016 16:02:38 GMT Subject: Re: [PATCH v3 4/7] notes copy --stdin: split lines with string_list_split() Message-ID: In-Reply-To: <56D2A60A.4000306@moritzneeb.de> On Sun, Feb 28, 2016 at 2:47 AM, Moritz Neeb wrote: > On 02/28/2016 07:56 AM, Eric Sunshine wrote: >> On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb 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