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?
Thanks,
Moritz
>> Signed-off-by: Moritz Neeb <lists@xxxxxxxxxxxxx>
>> ---
>> 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
--
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