On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb <li...@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.
The patch itself looks okay.
> Signed-off-by: Moritz Neeb <li...@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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
