Re: [PATCH v4 4/7] notes copy --stdin: read lines with strbuf_getline()

From: Moritz Neeb <lists@moritzneeb.de>
Date: Feb 29, 2016, 19:26 UTC
Message-ID: <56D49B7A.2060601@moritzneeb.de>
In-Reply-To: <CAPig+cSptQr21QMOJmxT4RPVR3r3zkEQ2TkTU8RoaJfo7=KChw@mail.gmail.com>

On 02/29/2016 07:19 PM, Eric Sunshine wrote:

Show 5 quoted lines

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

Thanks, Moritz

Show 28 quoted lines

>> Signed-off-by: Moritz Neeb <lists@moritzneeb.de>
>> ---
>>  builtin/notes.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/builtin/notes.c b/builtin/notes.c
>> index ed6f222..706ec11 100644
>> --- a/builtin/notes.c
>> +++ b/builtin/notes.c
>> @@ -290,7 +290,7 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
>>                 t = &default_notes_tree;
>>         }
>>
>> -       while (strbuf_getline_lf(&buf, stdin) != EOF) {
>> +       while (strbuf_getline(&buf, stdin) != EOF) {
>>                 unsigned char from_obj[20], to_obj[20];
>>                 struct strbuf **split;
>>                 int err;
>> @@ -299,7 +299,6 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
>>                 if (!split[0] || !split[1])
>>                         die(_("Malformed input line: '%s'."), buf.buf);
>>                 strbuf_rtrim(split[0]);
>> -               strbuf_rtrim(split[1]);
>>                 if (get_sha1(split[0]->buf, from_obj))
>>                         die(_("Failed to resolve '%s' as a valid ref."), split[0]->buf);
>>                 if (get_sha1(split[1]->buf, to_obj))
>> --
>> 2.4.3