On Mon, Feb 29, 2016 at 2:26 PM, Moritz Neeb <lists@xxxxxxxxxxxxx> wrote: > On 02/29/2016 07:19 PM, Eric Sunshine wrote: >> If you do elect to keep things the way they are, then (as mentioned in >> my v2 review) it would be helpful for the above paragraph to explain >> that strbuf_split() leave the "terminator" on the split elements, thus >> clarifying why the rtrim() of split[0] is still needed. > > Yes I would rather leave it like it is. I have the feeling it is > unmotivated to remove the rtrim of split[1] in the patch 5/7, because it > is directly related to the strbuf_getline_lf() replacement. Thats's what > I was trying to explain in the 2nd paragraph of the commit message. > > First I was following your review, but then I had to add a paragraph in > patch 5/7 that says something like "because the effect of the previous > patch is that there is not a CR anymore, we can now safely remove > rtrim() split[1]." > > You're right, maybe I should add a comment about why I left rtrim() of > split[0] to make it more obvious. I thought that would get clear by > looking at the context, i.e. patch 5/7, where it is explained (by you, > thanks for that), that strbuf_split leave this space. Is the assumption, > that those two patches are most times viewed in context wrong? I was more concerned about someone reading patch 4/7 in isolation and not consulting 5/7 (which might happen during a "blame" session, but it's a very minor point, not worth a re-roll if you and Junio are happy with the series as is. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html