Re: [PATCH v3 0/7] replacing strbuf_getline_lf() by strbuf_getline()

[Moritz Neeb <lists@moritzneeb.de>]

On 02/28/2016 07:30 AM, Eric Sunshine wrote:
> On Sun, Feb 28, 2016 at 12:07 AM, Moritz Neeb <lists@moritzneeb.de> wrote:
>> This series deals with strbuf_getline_lf() in certain codepaths:
>> Those, where the input that is read, is/was trimmed before doing anything that
>> could possibly expect a CR character. Those places can be assumed to be "text"
>> input, where a CR never would be a meaningful control character.
>> [...]
>>
>> Changes since v2:
>>
>> * Line splitting in notes_copy_from_stdin() is changed to string_list_split as
>>   suggested by Eric Sunshine.
>> * The behavior change in interactive cleaning from patch v2 is undone.
>> * Some of the previous patches were broken because of some unexpected
>>   whitespace. This should be fixed now.
> 
> In the future, as an aid to reviewers, please include an interdiff
> since the previous version, as well a link to the previous round[1].
> It's also very helpful to say which patches have changed (and which
> have not).
> 
> Thanks.
> 
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/285118/focus=286865
> 

Maybe not too late for other reviewers, here comes the interdiff (this assumes the non-broken version 2):

diff --git a/builtin/clean.c b/builtin/clean.c
index 18b6056..5b17a31 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -570,7 +570,9 @@ static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff)
 			       clean_get_color(CLEAN_COLOR_RESET));
 		}
 
-		if (strbuf_getline(&choice, stdin) == EOF) {
+		if (strbuf_getline(&choice, stdin) != EOF) {
+			strbuf_trim(&choice);
+		} else {
 			eof = 1;
 			break;
 		}
@@ -650,7 +652,9 @@ static int filter_by_patterns_cmd(void)
 		clean_print_color(CLEAN_COLOR_PROMPT);
 		printf(_("Input ignore patterns>> "));
 		clean_print_color(CLEAN_COLOR_RESET);
-		if (strbuf_getline(&confirm, stdin) == EOF)
+		if (strbuf_getline(&confirm, stdin) != EOF)
+			strbuf_trim(&confirm);
+		else
 			putchar('\n');
 
 		/* quit filter_by_pattern mode if press ENTER or Ctrl-D */
@@ -746,7 +750,9 @@ static int ask_each_cmd(void)
 			qname = quote_path_relative(item->string, NULL, &buf);
 			/* TRANSLATORS: Make sure to keep [y/N] as is */
 			printf(_("Remove %s [y/N]? "), qname);
-			if (strbuf_getline(&confirm, stdin) == EOF) {
+			if (strbuf_getline(&confirm, stdin) != EOF) {
+				strbuf_trim(&confirm);
+			} else {
 				putchar('\n');
 				eof = 1;
 			}
diff --git a/builtin/notes.c b/builtin/notes.c
index 706ec11..660c0b7 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -292,17 +292,17 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 
 	while (strbuf_getline(&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])
+		string_list_split(&split, buf.buf, ' ', -1);
+
+		if (split.nr != 2)
 			die(_("Malformed input line: '%s'."), buf.buf);
-		strbuf_rtrim(split[0]);
-		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);
@@ -312,11 +312,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) {