Re: [PATCH v4 4/7] notes copy --stdin: read lines with strbuf_getline()
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Feb 29, 2016, 18:19 UTC
- Message-ID
- <CAPig+cSptQr21QMOJmxT4RPVR3r3zkEQ2TkTU8RoaJfo7=KChw@mail.gmail.com>
- In-Reply-To
- <56D40314.7040608@moritzneeb.de>
On Mon, Feb 29, 2016 at 3:36 AM, Moritz Neeb <lists@moritzneeb.de> wrote:
Show 7 quoted lines
> The format of a line that is expected when copying notes via stdin > is "sha1 sha1". As this is text-only, strbuf_getline() should be used > instead of strbuf_getline_lf(), as documentation of this fact. > > When reading with strbuf_getline() the trimming of split[1] can be > removed. It was necessary before to remove potential CRs inserted > through a dos editor.
s/dos/DOS/
This may not be worth a re-roll, but the suggestion of my v3 review was to keep both rtrim's in this patch and then remove them in the next patch when converting to string_list_split(). A benefit of doing so is that you can then drop the above paragraph altogether, and both patches become simpler (description and content), thus easier to review.
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.
Show 28 quoted lines
> Signed-off-by: Moritz Neeb <lists@moritzneeb.de>
> ---
> 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