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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 28, 2016, 06:56 UTC
Message-ID: <CAPig+cT2GQ7mr0i649JRkJA7xGzXLEmy0RD31u537==sU1mtqQ@mail.gmail.com>
In-Reply-To: <56D28207.6080600@moritzneeb.de>

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.

The patch itself looks okay.

Show 47 quoted lines

> 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