On 02/28/2016 07:56 AM, Eric Sunshine wrote: > On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb <lists@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> >> --- >> 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 -- 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