Re: [PATCH v2 4/6] notes: read copied notes with strbuf_getline()
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Feb 22, 2016, 02:41 UTC
- Message-ID
- <CAPig+cReRiwHBJiatWJ=Gc+k+dtcMhdwFn4K57yHAjE3d_fzwQ@mail.gmail.com>
- In-Reply-To
- <56CA6160.7010908@moritzneeb.de>
On Sun, Feb 21, 2016 at 8:16 PM, Moritz Neeb <lists@moritzneeb.de> wrote:
Show 24 quoted lines
> The notes are copied from stdin. They should only contain SHA1s... Not
> spaces. CR could be there, because the file/the data from stdin could
> have been written via an editor that adds them.
>
> The notes that are copied from stdin are trimmed with strbuf_rtrim() after
> splitting by ' '. There is thus no logic expecting CR, so strbuf_getline_lf()
> can be replaced by its CRLF counterpart.
>
> Signed-off-by: Moritz Neeb <lists@moritzneeb.de>
> ---
> diff --git 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]);Given the commit message, I understand that this rtrim is effectively redundant, thus can be dropped, however, I'm not sure that doing so improves the code since the reader now has to think extra hard to understand the asymmetry of only trimming split[0] (and that understanding may require blaming this code in order to consult the commit message).
A deeper issue not touched upon by the commit message (but which should be) is that that strbuf_split() leaves the "terminator" (space, in this case) on the component strings, and that is why split[0] must be rtrim'd. Rather than dropping only one of the rtrim's, a cleaner approach might be to convert the code to use string_list_split() which doesn't have the "odd" behavior of leaving the terminator on the split strings, in which case both rtrim's could be retired. This, of course, would be done as a separate preparatory patch.
Show 5 quoted lines
> 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.7.1.345.gc14003e