On Sun, Feb 21, 2016 at 8:16 PM, Moritz Neeb <li...@moritzneeb.de> wrote:
> 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 <li...@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.
> 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
--
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
