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

[Moritz Neeb <lists@moritzneeb.de>]

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:
>> This patch changes, how the lines are split, when reading them from
>> stdin to copy the notes. The advantage of string_list_split() over
>> strbuf_split() is that it removes the terminator, making trimming
>> of the left part unneccesary.
> 
> Here's an alternate commit message:
> 
>     strbuf_split() has the unfortunate behavior of leaving the
>     separator character on the end of the split components, thus
>     placing the burden of manually removing the separator on the
>     caller. It's also heavyweight in that each split component is a
>     full-on strbuf. We need neither feature of strbuf_split() so
>     let's use string_list_split() instead since it removes the
>     separator character and returns an array of simple NUL-terminated
>     strings.
> 
>> 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.

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

> 
> The patch itself looks okay.
> 
>> Signed-off-by: Moritz Neeb <lists@moritzneeb.de>
>> ---
>> diff --git a/builtin/notes.c b/builtin/notes.c
>> @@ -292,18 +292,18 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
>>
>>         while (strbuf_getline_lf(&buf, stdin) != EOF) {
>>                 unsigned char from_obj[20], to_obj[20];
>> -               struct strbuf **split;
>> +               struct string_list split = STRING_LIST_INIT_DUP;
>>                 int err;
>>
>> -               split = strbuf_split(&buf, ' ');
>> -               if (!split[0] || !split[1])
>> +               strbuf_rtrim(&buf);
>> +               string_list_split(&split, buf.buf, ' ', -1);
>> +
>> +               if (split.nr != 2)
>>                         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))
>> -                       die(_("Failed to resolve '%s' as a valid ref."), split[1]->buf);
>> +               if (get_sha1(split.items[0].string, from_obj))
>> +                       die(_("Failed to resolve '%s' as a valid ref."), split.items[0].string);
>> +               if (get_sha1(split.items[1].string, to_obj))
>> +                       die(_("Failed to resolve '%s' as a valid ref."), split.items[1].string);
>>
>>                 if (rewrite_cmd)
>>                         err = copy_note_for_rewrite(c, from_obj, to_obj);
>> @@ -313,11 +313,11 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
>>
>>                 if (err) {
>>                         error(_("Failed to copy notes from '%s' to '%s'"),
>> -                             split[0]->buf, split[1]->buf);
>> +                             split.items[0].string, split.items[1].string);
>>                         ret = 1;
>>                 }
>>
>> -               strbuf_list_free(split);
>> +               string_list_clear(&split, 0);
>>         }
>>
>>         if (!rewrite_cmd) {
>> --
>> 2.4.3