{"thread":{"id":"41558","subject":"[PATCH v3 6/7] remote: read $GIT_DIR/branches/* with strbuf_getline()","startedAt":"2016-02-28T05:07:30Z","lastAt":"2016-03-09T20:28:45Z","messageCount":38,"participants":["Moritz Neeb","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":7},"messages":[{"id":"279688","messageId":"56D28092.9090209@moritzneeb.de","threadId":"41558","inReplyTo":"56CA5DBB.8040006@moritzneeb.de","subject":"[PATCH v3 0/7] replacing strbuf_getline_lf() by strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-28T05:07:30Z","receivedAt":"2016-02-28T05:07:30Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"This series deals with strbuf_getline_lf() in certain codepaths:\nThose, where the input that is read, is/was trimmed before doing anything that\ncould possibly expect a CR character. Those places can be assumed to be \"text\"\ninput, where a CR never would be a meaningful control character.\n\nThe purpose of this series is to document these places to have this property,\nby using strbuf_getline() instead of strbuf_getline_lf(). Also in some codepaths,\nthe CR could be a leftover of an editor and is thus removed.\n\nEvery codepath was examined, if after the change it is still necessary to have\ntrimming or if the additional CRLF-removal suffices.\n\nThe series is an idea out of [1], where Junio proposed to replace the calls\nto strbuf_getline_lf() because it 'would [be] a good way to document them as\ndealing with \"text\"'. \n\nChanges since v2:\n\n* Line splitting in notes_copy_from_stdin() is changed to string_list_split as\n suggested by Eric Sunshine.\n* The behavior change in interactive cleaning from patch v2 is undone.\n* Some of the previous patches were broken because of some unexpected\n whitespace. This should be fixed now.\n\n-Moritz\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/284104\n\nMoritz Neeb (7):\n quote: remove leading space in sq_dequote_step\n bisect: read bisect paths with strbuf_getline()\n clean: read user input with strbuf_getline()\n notes copy --stdin: split lines with string_list_split()\n notes copy --stdin: read lines with strbuf_getline()\n remote: read $GIT_DIR/branches/* with strbuf_getline()\n wt-status: read rebase todolist with strbuf_getline()\n\n bisect.c | 5 ++---\n builtin/clean.c | 6 +++---\n builtin/notes.c | 23 +++++++++++------------\n quote.c | 2 ++\n remote.c | 2 +-\n wt-status.c | 3 +--\n 6 files changed, 20 insertions(+), 21 deletions(-)\n\n-- \n2.7.1.346.gc0ef946\n"},{"id":"279689","messageId":"56D281F7.4010605@moritzneeb.de","threadId":"41558","inReplyTo":"56D28092.9090209@moritzneeb.de","subject":"[PATCH v3 1/7] quote: remove leading space in sq_dequote_step","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-28T05:13:27Z","receivedAt":"2016-02-28T05:13:27Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"Because sq_quote_argv adds a leading space (which is expected in trace.c),\nsq_dequote_step should remove this space again, such that the operations\nof quoting and dequoting are inverse of each other.\n\nThis patch is preparing the way to remove some excessive trimming\noperation in bisect in the following commit.\n\nSigned-off-by: Moritz Neeb \n---\n quote.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/quote.c b/quote.c\nindex fe884d2..2714f27 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -63,6 +63,8 @@ static char *sq_dequote_step(char *arg, char **next)\n \tchar *src = arg;\n \tchar c;\n \n+\tif (*src == ' ')\n+\t\tsrc++;\n \tif (*src != '\\'')\n \t\treturn NULL;\n \tfor (;;) {\n-- \n2.4.3\n"},{"id":"279687","messageId":"56D281FD.1070707@moritzneeb.de","threadId":"41558","inReplyTo":"56D28092.9090209@moritzneeb.de","subject":"[PATCH v3 2/7] bisect: read bisect paths with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-28T05:13:33Z","receivedAt":"2016-02-28T05:13:33Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"The file BISECT_NAMES is written by \"git rev-parse --sq-quote\" via\nsq_quote_argv() when starting a bisection. It can contain pathspecs\nto narrow down the search. When reading it back, it should be expected that\nsq_dequote_to_argv_array() is able to parse this file. In fact, the\nprevious commit ensures this.\n\nAs the content is of type \"text\", that means there is no logic expecting\nCR, strbuf_getline_lf() will be replaced by strbuf_getline().\n\nApart from whitespace added and removed in quote.c, no more whitespaces\nare expexted. While it is technically possible, we have never advertised\nthis file to be editable by user, or encouraged them to do so, thus\nthe call to strbuf_trim() turns obsolete in various ways.\n\nFor the case that this file is modified nonetheless, in an invalid way\nsuch that dequoting fails, the error message is broadened to both cases:\nbad quoting and unexpected whitespace.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Moritz Neeb \n---\n bisect.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 06ec54e..e2df02f 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -440,10 +440,9 @@ static void read_bisect_paths(struct argv_array *array)\n \tif (!fp)\n \t\tdie_errno(\"Could not open file '%s'\", filename);\n \n-\twhile (strbuf_getline_lf(&str, fp) != EOF) {\n-\t\tstrbuf_trim(&str);\n+\twhile (strbuf_getline(&str, fp) != EOF) {\n \t\tif (sq_dequote_to_argv_array(str.buf, array))\n-\t\t\tdie(\"Badly quoted content in file '%s': %s\",\n+\t\t\tdie(\"Badly quoted content or unexpected whitespace in file '%s': %s\",\n \t\t\t filename, str.buf);\n \t}\n \n-- \n2.4.3\n"},{"id":"279691","messageId":"56D28203.7040502@moritzneeb.de","threadId":"41558","inReplyTo":"56D28092.9090209@moritzneeb.de","subject":"[PATCH v3 3/7] clean: read user input with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-28T05:13:39Z","receivedAt":"2016-02-28T05:13:39Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"The inputs that are read are all answers that are given by the user\nwhen interacting with git on the commandline. As these answers are\nnot supposed to contain a meaningful CR it is safe to\nreplace strbuf_getline_lf() can be replaced by strbuf_getline().\n\nIn the subsequent codepath, the input is trimmed. This leads to\naccepting user input with spaces, e.g. \" y \", as a valid answer in\nthe interactive cleaning process.\n\nAlthough trimming would not be required anymore to remove a potential CR,\nwe don't want to change the existing behavior with this patch.\nThus, the trimming is kept in place.\n\nSigned-off-by: Moritz Neeb \n---\n builtin/clean.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 7b08237..956283d 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -570,7 +570,7 @@ static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff)\n \t\t\t clean_get_color(CLEAN_COLOR_RESET));\n \t\t}\n \n-\t\tif (strbuf_getline_lf(&choice, stdin) != EOF) {\n+\t\tif (strbuf_getline(&choice, stdin) != EOF) {\n \t\t\tstrbuf_trim(&choice);\n \t\t} else {\n \t\t\teof = 1;\n@@ -652,7 +652,7 @@ static int filter_by_patterns_cmd(void)\n \t\tclean_print_color(CLEAN_COLOR_PROMPT);\n \t\tprintf(_(\"Input ignore patterns>> \"));\n \t\tclean_print_color(CLEAN_COLOR_RESET);\n-\t\tif (strbuf_getline_lf(&confirm, stdin) != EOF)\n+\t\tif (strbuf_getline(&confirm, stdin) != EOF)\n \t\t\tstrbuf_trim(&confirm);\n \t\telse\n \t\t\tputchar('\\n');\n@@ -750,7 +750,7 @@ static int ask_each_cmd(void)\n \t\t\tqname = quote_path_relative(item->string, NULL, &buf);\n \t\t\t/* TRANSLATORS: Make sure to keep [y/N] as is */\n \t\t\tprintf(_(\"Remove %s [y/N]? \"), qname);\n-\t\t\tif (strbuf_getline_lf(&confirm, stdin) != EOF) {\n+\t\t\tif (strbuf_getline(&confirm, stdin) != EOF) {\n \t\t\t\tstrbuf_trim(&confirm);\n \t\t\t} else {\n \t\t\t\tputchar('\\n');\n-- \n2.4.3\n"},{"id":"279685","messageId":"56D28207.6080600@moritzneeb.de","threadId":"41558","inReplyTo":"56D28092.9090209@moritzneeb.de","subject":"[PATCH v3 4/7] notes copy --stdin: split lines with string_list_split()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-28T05:13:43Z","receivedAt":"2016-02-28T05:13:43Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"This patch changes, how the lines are split, when reading them from\nstdin to copy the notes. The advantage of string_list_split() over\nstrbuf_split() is that it removes the terminator, making trimming\nof the left part unneccesary.\n\nThe strbuf is now rtrimmed before splitting. This is still required\nto remove potential CRs. In the next step this will then be done\nimplicitly by strbuf_readline(). Thus, this is a preparatory refactoring,\ntowards a trim-free codepath.\n\nSigned-off-by: Moritz Neeb \n---\n builtin/notes.c | 22 +++++++++++-----------\n 1 file changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex ed6f222..22909c7 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -292,18 +292,18 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n \n \twhile (strbuf_getline_lf(&buf, stdin) != EOF) {\n \t\tunsigned char from_obj[20], to_obj[20];\n-\t\tstruct strbuf **split;\n+\t\tstruct string_list split = STRING_LIST_INIT_DUP;\n \t\tint err;\n \n-\t\tsplit = strbuf_split(&buf, ' ');\n-\t\tif (!split[0] || !split[1])\n+\t\tstrbuf_rtrim(&buf);\n+\t\tstring_list_split(&split, buf.buf, ' ', -1);\n+\n+\t\tif (split.nr != 2)\n \t\t\tdie(_(\"Malformed input line: '%s'.\"), buf.buf);\n-\t\tstrbuf_rtrim(split[0]);\n-\t\tstrbuf_rtrim(split[1]);\n-\t\tif (get_sha1(split[0]->buf, from_obj))\n-\t\t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), split[0]->buf);\n-\t\tif (get_sha1(split[1]->buf, to_obj))\n-\t\t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), split[1]->buf);\n+\t\tif (get_sha1(split.items[0].string, from_obj))\n+\t\t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), split.items[0].string);\n+\t\tif (get_sha1(split.items[1].string, to_obj))\n+\t\t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), split.items[1].string);\n \n \t\tif (rewrite_cmd)\n \t\t\terr = copy_note_for_rewrite(c, from_obj, to_obj);\n@@ -313,11 +313,11 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n \n \t\tif (err) {\n \t\t\terror(_(\"Failed to copy notes from '%s' to '%s'\"),\n-\t\t\t split[0]->buf, split[1]->buf);\n+\t\t\t split.items[0].string, split.items[1].string);\n \t\t\tret = 1;\n \t\t}\n \n-\t\tstrbuf_list_free(split);\n+\t\tstring_list_clear(&split, 0);\n \t}\n \n \tif (!rewrite_cmd) {\n-- \n2.4.3\n"},{"id":"279690","messageId":"56D28211.5060101@moritzneeb.de","threadId":"41558","inReplyTo":"56D28092.9090209@moritzneeb.de","subject":"[PATCH v3 5/7] notes copy --stdin: read lines with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-28T05:13:53Z","receivedAt":"2016-02-28T05:13:53Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"The format of a line that is expected when copying notes via stdin\nis \"sha1 sha1\". As this is text-only, strbuf_getline() can be used\ninstead of strbuf_getline_lf().\n\nWhen reading with strbuf_getline() the trimming can be removed.\nIt was necessary before to remove potential CRs inserted through\na dos editor.\n\nSigned-off-by: Moritz Neeb \n---\n builtin/notes.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 22909c7..660c0b7 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -290,12 +290,11 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n \t\tt = &default_notes_tree;\n \t}\n \n-\twhile (strbuf_getline_lf(&buf, stdin) != EOF) {\n+\twhile (strbuf_getline(&buf, stdin) != EOF) {\n \t\tunsigned char from_obj[20], to_obj[20];\n \t\tstruct string_list split = STRING_LIST_INIT_DUP;\n \t\tint err;\n \n-\t\tstrbuf_rtrim(&buf);\n \t\tstring_list_split(&split, buf.buf, ' ', -1);\n \n \t\tif (split.nr != 2)\n-- \n2.4.3\n"},{"id":"279684","messageId":"56D2821F.4010806@moritzneeb.de","threadId":"41558","inReplyTo":"56D28092.9090209@moritzneeb.de","subject":"[PATCH v3 6/7] remote: read $GIT_DIR/branches/* with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-28T05:14:07Z","receivedAt":"2016-02-28T05:14:07Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"The line read from the branch file is directly trimmed after reading with\nstrbuf_trim(). There is thus no logic expecting CR, so strbuf_getline_lf()\ncan be replaced by its CRLF counterpart.\n\nSigned-off-by: Moritz Neeb \n---\n remote.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/remote.c b/remote.c\nindex 02e698a..aaff6aa 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -281,7 +281,7 @@ static void read_branches_file(struct remote *remote)\n \tif (!f)\n \t\treturn;\n \n-\tstrbuf_getline_lf(&buf, f);\n+\tstrbuf_getline(&buf, f);\n \tfclose(f);\n \tstrbuf_trim(&buf);\n \tif (!buf.len) {\n-- \n2.4.3\n"},{"id":"279686","messageId":"56D28224.9010005@moritzneeb.de","threadId":"41558","inReplyTo":"56D28092.9090209@moritzneeb.de","subject":"[PATCH v3 7/7] wt-status: read rebase todolist with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-28T05:14:12Z","receivedAt":"2016-02-28T05:14:12Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"In read_rebase_todolist() the files $GIT_DIR/rebase-merge/done and\n$GIT_DIR/rebase-merge/git-rebase-todo are read to collect status\ninformation.\n\nThe access to this file should always happen via git rebase, e.g. via\n\"git rebase -i\" or \"git rebase --edit-todo\". We can assume, that this\ninterface handles the preprocessing of whitespaces, especially CRLFs\ncorrectly. Thus in this codepath we can remove the call to strbuf_trim().\n\nFor documenting the input as expecting \"text\" input, strbuf_getline_lf()\nis still replaced by strbuf_getline().\n\nSigned-off-by: Moritz Neeb \n---\n wt-status.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex ab4f80d..8047cf2 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1076,10 +1076,9 @@ static void read_rebase_todolist(const char *fname, struct string_list *lines)\n \tif (!f)\n \t\tdie_errno(\"Could not open file %s for reading\",\n \t\t\t git_path(\"%s\", fname));\n-\twhile (!strbuf_getline_lf(&line, f)) {\n+\twhile (!strbuf_getline(&line, f)) {\n \t\tif (line.len && line.buf[0] == comment_line_char)\n \t\t\tcontinue;\n-\t\tstrbuf_trim(&line);\n \t\tif (!line.len)\n \t\t\tcontinue;\n \t\tabbrev_sha1_in_line(&line);\n-- \n2.4.3\n"},{"id":"279692","messageId":"CAPig+cSv=7zixz_BK=f0MhQnTTB-agB5=4aSrFE5vAJtOgbuGg@mail.gmail.com","threadId":"41558","inReplyTo":"56D28092.9090209@moritzneeb.de","subject":"Re: [PATCH v3 0/7] replacing strbuf_getline_lf() by strbuf_getline()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-28T06:30:01Z","receivedAt":"2016-02-28T06:30:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 28, 2016 at 12:07 AM, Moritz Neeb wrote:\n> This series deals with strbuf_getline_lf() in certain codepaths:\n> Those, where the input that is read, is/was trimmed before doing anything that\n> could possibly expect a CR character. Those places can be assumed to be \"text\"\n> input, where a CR never would be a meaningful control character.\n> [...]\n>\n> Changes since v2:\n>\n> * Line splitting in notes_copy_from_stdin() is changed to string_list_split as\n> suggested by Eric Sunshine.\n> * The behavior change in interactive cleaning from patch v2 is undone.\n> * Some of the previous patches were broken because of some unexpected\n> whitespace. This should be fixed now.\n\nIn the future, as an aid to reviewers, please include an interdiff\nsince the previous version, as well a link to the previous round[1].\nIt's also very helpful to say which patches have changed (and which\nhave not).\n\nThanks.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/285118/focus=286865\n"},{"id":"279693","messageId":"CAPig+cRMX1DF7ffEKR4fWGY9wZjpKsaOaf4C1YdWipUGh1+8AA@mail.gmail.com","threadId":"41558","inReplyTo":"56D281FD.1070707@moritzneeb.de","subject":"Re: [PATCH v3 2/7] bisect: read bisect paths with strbuf_getline()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-28T06:33:16Z","receivedAt":"2016-02-28T06:33:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb wrote:\n> The file BISECT_NAMES is written by \"git rev-parse --sq-quote\" via\n> sq_quote_argv() when starting a bisection. It can contain pathspecs\n> to narrow down the search. When reading it back, it should be expected that\n> sq_dequote_to_argv_array() is able to parse this file. In fact, the\n> previous commit ensures this.\n>\n> As the content is of type \"text\", that means there is no logic expecting\n> CR, strbuf_getline_lf() will be replaced by strbuf_getline().\n>\n> Apart from whitespace added and removed in quote.c, no more whitespaces\n> are expexted. While it is technically possible, we have never advertised\n\ns/expexted/expected/\n\n> this file to be editable by user, or encouraged them to do so, thus\n> the call to strbuf_trim() turns obsolete in various ways.\n\nNot sure what \"various ways\" you mean. Perhaps say instead that (as a\nconsequence of \"not advertised or encouraged\") you're tightening the\nparsing of this file by removing strbuf_trim().\n\n> For the case that this file is modified nonetheless, in an invalid way\n> such that dequoting fails, the error message is broadened to both cases:\n> bad quoting and unexpected whitespace.\n>\n> Helped-by: Junio C Hamano \n> Signed-off-by: Moritz Neeb \n> ---\n> diff --git a/bisect.c b/bisect.c\n> @@ -440,10 +440,9 @@ static void read_bisect_paths(struct argv_array *array)\n> if (!fp)\n> die_errno(\"Could not open file '%s'\", filename);\n>\n> - while (strbuf_getline_lf(&str, fp) != EOF) {\n> - strbuf_trim(&str);\n> + while (strbuf_getline(&str, fp) != EOF) {\n> if (sq_dequote_to_argv_array(str.buf, array))\n> - die(\"Badly quoted content in file '%s': %s\",\n> + die(\"Badly quoted content or unexpected whitespace in file '%s': %s\",\n> filename, str.buf);\n> }\n>\n> --\n> 2.4.3\n"},{"id":"279694","messageId":"CAPig+cSkV0KMG7gONabPxgQkzWgPU+1YGTMb4SDmpg1QrHxhSA@mail.gmail.com","threadId":"41558","inReplyTo":"56D28203.7040502@moritzneeb.de","subject":"Re: [PATCH v3 3/7] clean: read user input with strbuf_getline()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-28T06:36:05Z","receivedAt":"2016-02-28T06:36:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb wrote:\n> The inputs that are read are all answers that are given by the user\n> when interacting with git on the commandline. As these answers are\n> not supposed to contain a meaningful CR it is safe to\n> replace strbuf_getline_lf() can be replaced by strbuf_getline().\n\nGrammo: \"it is safe to replace ... can be replaced by ...\"\n\n> In the subsequent codepath, the input is trimmed. This leads to\n\nHow about?\n\n After being read, the input is trimmed.\n\n> accepting user input with spaces, e.g. \" y \", as a valid answer in\n> the interactive cleaning process.\n>\n> Although trimming would not be required anymore to remove a potential CR,\n> we don't want to change the existing behavior with this patch.\n> Thus, the trimming is kept in place.\n>\n> Signed-off-by: Moritz Neeb \n> ---\n> diff --git a/builtin/clean.c b/builtin/clean.c\n> @@ -570,7 +570,7 @@ static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff)\n> clean_get_color(CLEAN_COLOR_RESET));\n> }\n>\n> - if (strbuf_getline_lf(&choice, stdin) != EOF) {\n> + if (strbuf_getline(&choice, stdin) != EOF) {\n> strbuf_trim(&choice);\n> } else {\n> eof = 1;\n> @@ -652,7 +652,7 @@ static int filter_by_patterns_cmd(void)\n> clean_print_color(CLEAN_COLOR_PROMPT);\n> printf(_(\"Input ignore patterns>> \"));\n> clean_print_color(CLEAN_COLOR_RESET);\n> - if (strbuf_getline_lf(&confirm, stdin) != EOF)\n> + if (strbuf_getline(&confirm, stdin) != EOF)\n> strbuf_trim(&confirm);\n> else\n> putchar('\\n');\n> @@ -750,7 +750,7 @@ static int ask_each_cmd(void)\n> qname = quote_path_relative(item->string, NULL, &buf);\n> /* TRANSLATORS: Make sure to keep [y/N] as is */\n> printf(_(\"Remove %s [y/N]? \"), qname);\n> - if (strbuf_getline_lf(&confirm, stdin) != EOF) {\n> + if (strbuf_getline(&confirm, stdin) != EOF) {\n> strbuf_trim(&confirm);\n> } else {\n> putchar('\\n');\n> --\n> 2.4.3\n"},{"id":"279695","messageId":"CAPig+cT2GQ7mr0i649JRkJA7xGzXLEmy0RD31u537==sU1mtqQ@mail.gmail.com","threadId":"41558","inReplyTo":"56D28207.6080600@moritzneeb.de","subject":"Re: [PATCH v3 4/7] notes copy --stdin: split lines with string_list_split()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-28T06:56:51Z","receivedAt":"2016-02-28T06:56:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb wrote:\n> This patch changes, how the lines are split, when reading them from\n> stdin to copy the notes. The advantage of string_list_split() over\n> strbuf_split() is that it removes the terminator, making trimming\n> of the left part unneccesary.\n\nHere's an alternate commit message:\n\n strbuf_split() has the unfortunate behavior of leaving the\n separator character on the end of the split components, thus\n placing the burden of manually removing the separator on the\n caller. It's also heavyweight in that each split component is a\n full-on strbuf. We need neither feature of strbuf_split() so\n let's use string_list_split() instead since it removes the\n separator character and returns an array of simple NUL-terminated\n strings.\n\n> The strbuf is now rtrimmed before splitting. This is still required\n> to remove potential CRs. In the next step this will then be done\n> implicitly by strbuf_readline(). Thus, this is a preparatory refactoring,\n> towards a trim-free codepath.\n\nI would actually swap patches 4 and 5 so that strbuf_getline() is done\nfirst (without removing any of the rtrim's) and string_list_split()\nsecond. That way, you don't have to add that extra rtrim in one patch\nand immediately remove it in the next. And, as a bonus, you can drop\nthe above paragraph altogether.\n\nThe patch itself looks okay.\n\n> Signed-off-by: Moritz Neeb \n> ---\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> @@ -292,18 +292,18 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n>\n> while (strbuf_getline_lf(&buf, stdin) != EOF) {\n> unsigned char from_obj[20], to_obj[20];\n> - struct strbuf **split;\n> + struct string_list split = STRING_LIST_INIT_DUP;\n> int err;\n>\n> - split = strbuf_split(&buf, ' ');\n> - if (!split[0] || !split[1])\n> + strbuf_rtrim(&buf);\n> + string_list_split(&split, buf.buf, ' ', -1);\n> +\n> + if (split.nr != 2)\n> die(_(\"Malformed input line: '%s'.\"), buf.buf);\n> - strbuf_rtrim(split[0]);\n> - strbuf_rtrim(split[1]);\n> - if (get_sha1(split[0]->buf, from_obj))\n> - die(_(\"Failed to resolve '%s' as a valid ref.\"), split[0]->buf);\n> - if (get_sha1(split[1]->buf, to_obj))\n> - die(_(\"Failed to resolve '%s' as a valid ref.\"), split[1]->buf);\n> + if (get_sha1(split.items[0].string, from_obj))\n> + die(_(\"Failed to resolve '%s' as a valid ref.\"), split.items[0].string);\n> + if (get_sha1(split.items[1].string, to_obj))\n> + die(_(\"Failed to resolve '%s' as a valid ref.\"), split.items[1].string);\n>\n> if (rewrite_cmd)\n> err = copy_note_for_rewrite(c, from_obj, to_obj);\n> @@ -313,11 +313,11 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n>\n> if (err) {\n> error(_(\"Failed to copy notes from '%s' to '%s'\"),\n> - split[0]->buf, split[1]->buf);\n> + split.items[0].string, split.items[1].string);\n> ret = 1;\n> }\n>\n> - strbuf_list_free(split);\n> + string_list_clear(&split, 0);\n> }\n>\n> if (!rewrite_cmd) {\n> --\n> 2.4.3\n"},{"id":"279696","messageId":"56D29FB3.3030707@moritzneeb.de","threadId":"41558","inReplyTo":"CAPig+cSv=7zixz_BK=f0MhQnTTB-agB5=4aSrFE5vAJtOgbuGg@mail.gmail.com","subject":"Re: [PATCH v3 0/7] replacing strbuf_getline_lf() by strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-28T07:20:19Z","receivedAt":"2016-02-28T07:20:19Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"On 02/28/2016 07:30 AM, Eric Sunshine wrote:\n> On Sun, Feb 28, 2016 at 12:07 AM, Moritz Neeb wrote:\n>> This series deals with strbuf_getline_lf() in certain codepaths:\n>> Those, where the input that is read, is/was trimmed before doing anything that\n>> could possibly expect a CR character. Those places can be assumed to be \"text\"\n>> input, where a CR never would be a meaningful control character.\n>> [...]\n>>\n>> Changes since v2:\n>>\n>> * Line splitting in notes_copy_from_stdin() is changed to string_list_split as\n>> suggested by Eric Sunshine.\n>> * The behavior change in interactive cleaning from patch v2 is undone.\n>> * Some of the previous patches were broken because of some unexpected\n>> whitespace. This should be fixed now.\n> \n> In the future, as an aid to reviewers, please include an interdiff\n> since the previous version, as well a link to the previous round[1].\n> It's also very helpful to say which patches have changed (and which\n> have not).\n> \n> Thanks.\n> \n> [1]: http://thread.gmane.org/gmane.comp.version-control.git/285118/focus=286865\n> \n\nMaybe not too late for other reviewers, here comes the interdiff (this assumes the non-broken version 2):\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 18b6056..5b17a31 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -570,7 +570,9 @@ static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff)\n \t\t\t clean_get_color(CLEAN_COLOR_RESET));\n \t\t}\n \n-\t\tif (strbuf_getline(&choice, stdin) == EOF) {\n+\t\tif (strbuf_getline(&choice, stdin) != EOF) {\n+\t\t\tstrbuf_trim(&choice);\n+\t\t} else {\n \t\t\teof = 1;\n \t\t\tbreak;\n \t\t}\n@@ -650,7 +652,9 @@ static int filter_by_patterns_cmd(void)\n \t\tclean_print_color(CLEAN_COLOR_PROMPT);\n \t\tprintf(_(\"Input ignore patterns>> \"));\n \t\tclean_print_color(CLEAN_COLOR_RESET);\n-\t\tif (strbuf_getline(&confirm, stdin) == EOF)\n+\t\tif (strbuf_getline(&confirm, stdin) != EOF)\n+\t\t\tstrbuf_trim(&confirm);\n+\t\telse\n \t\t\tputchar('\\n');\n \n \t\t/* quit filter_by_pattern mode if press ENTER or Ctrl-D */\n@@ -746,7 +750,9 @@ static int ask_each_cmd(void)\n \t\t\tqname = quote_path_relative(item->string, NULL, &buf);\n \t\t\t/* TRANSLATORS: Make sure to keep [y/N] as is */\n \t\t\tprintf(_(\"Remove %s [y/N]? \"), qname);\n-\t\t\tif (strbuf_getline(&confirm, stdin) == EOF) {\n+\t\t\tif (strbuf_getline(&confirm, stdin) != EOF) {\n+\t\t\t\tstrbuf_trim(&confirm);\n+\t\t\t} else {\n \t\t\t\tputchar('\\n');\n \t\t\t\teof = 1;\n \t\t\t}\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 706ec11..660c0b7 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -292,17 +292,17 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n \n \twhile (strbuf_getline(&buf, stdin) != EOF) {\n \t\tunsigned char from_obj[20], to_obj[20];\n-\t\tstruct strbuf **split;\n+\t\tstruct string_list split = STRING_LIST_INIT_DUP;\n \t\tint err;\n \n-\t\tsplit = strbuf_split(&buf, ' ');\n-\t\tif (!split[0] || !split[1])\n+\t\tstring_list_split(&split, buf.buf, ' ', -1);\n+\n+\t\tif (split.nr != 2)\n \t\t\tdie(_(\"Malformed input line: '%s'.\"), buf.buf);\n-\t\tstrbuf_rtrim(split[0]);\n-\t\tif (get_sha1(split[0]->buf, from_obj))\n-\t\t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), split[0]->buf);\n-\t\tif (get_sha1(split[1]->buf, to_obj))\n-\t\t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), split[1]->buf);\n+\t\tif (get_sha1(split.items[0].string, from_obj))\n+\t\t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), split.items[0].string);\n+\t\tif (get_sha1(split.items[1].string, to_obj))\n+\t\t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), split.items[1].string);\n \n \t\tif (rewrite_cmd)\n \t\t\terr = copy_note_for_rewrite(c, from_obj, to_obj);\n@@ -312,11 +312,11 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n \n \t\tif (err) {\n \t\t\terror(_(\"Failed to copy notes from '%s' to '%s'\"),\n-\t\t\t split[0]->buf, split[1]->buf);\n+\t\t\t split.items[0].string, split.items[1].string);\n \t\t\tret = 1;\n \t\t}\n \n-\t\tstrbuf_list_free(split);\n+\t\tstring_list_clear(&split, 0);\n \t}\n \n \tif (!rewrite_cmd) {\n"},{"id":"279697","messageId":"56D2A1FC.4020008@moritzneeb.de","threadId":"41558","inReplyTo":"CAPig+cRMX1DF7ffEKR4fWGY9wZjpKsaOaf4C1YdWipUGh1+8AA@mail.gmail.com","subject":"Re: [PATCH v3 2/7] bisect: read bisect paths with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-28T07:30:04Z","receivedAt":"2016-02-28T07:30:04Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"On 02/28/2016 07:33 AM, Eric Sunshine wrote:\n> On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb wrote:\n>> The file BISECT_NAMES is written by \"git rev-parse --sq-quote\" via\n>> sq_quote_argv() when starting a bisection. It can contain pathspecs\n>> to narrow down the search. When reading it back, it should be expected that\n>> sq_dequote_to_argv_array() is able to parse this file. In fact, the\n>> previous commit ensures this.\n>>\n>> As the content is of type \"text\", that means there is no logic expecting\n>> CR, strbuf_getline_lf() will be replaced by strbuf_getline().\n>>\n>> Apart from whitespace added and removed in quote.c, no more whitespaces\n>> are expexted. While it is technically possible, we have never advertised\n> \n> s/expexted/expected/\n> \n>> this file to be editable by user, or encouraged them to do so, thus\n>> the call to strbuf_trim() turns obsolete in various ways.\n> \n> Not sure what \"various ways\" you mean. Perhaps say instead that (as a\n> consequence of \"not advertised or encouraged\") you're tightening the\n> parsing of this file by removing strbuf_trim().\n\nYeah that doesn't make sense. What I meant \"it's neither needed for CR-removal,\nnor for removal of other potential whitespace\". I will rewrite the paragraph to:\n\nApart from whitespace added and removed in quote.c, no other whitespaces\nare expected. While it is technically possible, we have never advertised\nthis file to be editable by user, or encouraged them to do so. As a\nconsequence, the parsing of BISECT_NAMES is tightened by removing\nstrbuf_trim().\n\n> \n>> For the case that this file is modified nonetheless, in an invalid way\n>> such that dequoting fails, the error message is broadened to both cases:\n>> bad quoting and unexpected whitespace.\n>>\n>> Helped-by: Junio C Hamano \n>> Signed-off-by: Moritz Neeb \n>> ---\n>> diff --git a/bisect.c b/bisect.c\n>> @@ -440,10 +440,9 @@ static void read_bisect_paths(struct argv_array *array)\n>> if (!fp)\n>> die_errno(\"Could not open file '%s'\", filename);\n>>\n>> - while (strbuf_getline_lf(&str, fp) != EOF) {\n>> - strbuf_trim(&str);\n>> + while (strbuf_getline(&str, fp) != EOF) {\n>> if (sq_dequote_to_argv_array(str.buf, array))\n>> - die(\"Badly quoted content in file '%s': %s\",\n>> + die(\"Badly quoted content or unexpected whitespace in file '%s': %s\",\n>> filename, str.buf);\n>> }\n>>\n>> --\n>> 2.4.3\n"},{"id":"279698","messageId":"56D2A390.9010804@moritzneeb.de","threadId":"41558","inReplyTo":"CAPig+cSkV0KMG7gONabPxgQkzWgPU+1YGTMb4SDmpg1QrHxhSA@mail.gmail.com","subject":"Re: [PATCH v3 3/7] clean: read user input with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-28T07:36:48Z","receivedAt":"2016-02-28T07:36:48Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"On 02/28/2016 07:36 AM, Eric Sunshine wrote:\n> On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb wrote:\n>> The inputs that are read are all answers that are given by the user\n>> when interacting with git on the commandline. As these answers are\n>> not supposed to contain a meaningful CR it is safe to\n>> replace strbuf_getline_lf() can be replaced by strbuf_getline().\n> \n> Grammo: \"it is safe to replace ... can be replaced by ...\"\n> \n\nI dropped the second duplication.\n\n>> In the subsequent codepath, the input is trimmed. This leads to\n> \n> How about?\n> \n> After being read, the input is trimmed.\n\nYep, sounds better.\n\nThanks.\n\n> \n>> accepting user input with spaces, e.g. \" y \", as a valid answer in\n>> the interactive cleaning process.\n>>\n>> Although trimming would not be required anymore to remove a potential CR,\n>> we don't want to change the existing behavior with this patch.\n>> Thus, the trimming is kept in place.\n>>\n>> Signed-off-by: Moritz Neeb \n>> ---\n>> diff --git a/builtin/clean.c b/builtin/clean.c\n>> @@ -570,7 +570,7 @@ static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff)\n>> clean_get_color(CLEAN_COLOR_RESET));\n>> }\n>>\n>> - if (strbuf_getline_lf(&choice, stdin) != EOF) {\n>> + if (strbuf_getline(&choice, stdin) != EOF) {\n>> strbuf_trim(&choice);\n>> } else {\n>> eof = 1;\n>> @@ -652,7 +652,7 @@ static int filter_by_patterns_cmd(void)\n>> clean_print_color(CLEAN_COLOR_PROMPT);\n>> printf(_(\"Input ignore patterns>> \"));\n>> clean_print_color(CLEAN_COLOR_RESET);\n>> - if (strbuf_getline_lf(&confirm, stdin) != EOF)\n>> + if (strbuf_getline(&confirm, stdin) != EOF)\n>> strbuf_trim(&confirm);\n>> else\n>> putchar('\\n');\n>> @@ -750,7 +750,7 @@ static int ask_each_cmd(void)\n>> qname = quote_path_relative(item->string, NULL, &buf);\n>> /* TRANSLATORS: Make sure to keep [y/N] as is */\n>> printf(_(\"Remove %s [y/N]? \"), qname);\n>> - if (strbuf_getline_lf(&confirm, stdin) != EOF) {\n>> + if (strbuf_getline(&confirm, stdin) != EOF) {\n>> strbuf_trim(&confirm);\n>> } else {\n>> putchar('\\n');\n>> --\n>> 2.4.3\n"},{"id":"279699","messageId":"56D2A60A.4000306@moritzneeb.de","threadId":"41558","inReplyTo":"CAPig+cT2GQ7mr0i649JRkJA7xGzXLEmy0RD31u537==sU1mtqQ@mail.gmail.com","subject":"Re: [PATCH v3 4/7] notes copy --stdin: split lines with string_list_split()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-28T07:47:22Z","receivedAt":"2016-02-28T07:47:22Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"On 02/28/2016 07:56 AM, Eric Sunshine wrote:\n> On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb wrote:\n>> This patch changes, how the lines are split, when reading them from\n>> stdin to copy the notes. The advantage of string_list_split() over\n>> strbuf_split() is that it removes the terminator, making trimming\n>> of the left part unneccesary.\n> \n> Here's an alternate commit message:\n> \n> strbuf_split() has the unfortunate behavior of leaving the\n> separator character on the end of the split components, thus\n> placing the burden of manually removing the separator on the\n> caller. It's also heavyweight in that each split component is a\n> full-on strbuf. We need neither feature of strbuf_split() so\n> let's use string_list_split() instead since it removes the\n> separator character and returns an array of simple NUL-terminated\n> strings.\n> \n>> The strbuf is now rtrimmed before splitting. This is still required\n>> to remove potential CRs. In the next step this will then be done\n>> implicitly by strbuf_readline(). Thus, this is a preparatory refactoring,\n>> towards a trim-free codepath.\n> \n> I would actually swap patches 4 and 5 so that strbuf_getline() is done\n> first (without removing any of the rtrim's) and string_list_split()\n> second. That way, you don't have to add that extra rtrim in one patch\n> and immediately remove it in the next. And, as a bonus, you can drop\n> the above paragraph altogether.\n\nYeah, I also was thinking about that, should've pointed that out.\nI was just following your \"guiding\" in v2 [1], that's why I did it this way,\nbecause I thought it is somehow expected to be a prepraratory change.\n\nOk, when switching 4 and 5, I could call it something like \"post cleanup/refactoring\"\ninstead.\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/286868\n\n> \n> The patch itself looks okay.\n> \n>> Signed-off-by: Moritz Neeb \n>> ---\n>> diff --git a/builtin/notes.c b/builtin/notes.c\n>> @@ -292,18 +292,18 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n>>\n>> while (strbuf_getline_lf(&buf, stdin) != EOF) {\n>> unsigned char from_obj[20], to_obj[20];\n>> - struct strbuf **split;\n>> + struct string_list split = STRING_LIST_INIT_DUP;\n>> int err;\n>>\n>> - split = strbuf_split(&buf, ' ');\n>> - if (!split[0] || !split[1])\n>> + strbuf_rtrim(&buf);\n>> + string_list_split(&split, buf.buf, ' ', -1);\n>> +\n>> + if (split.nr != 2)\n>> die(_(\"Malformed input line: '%s'.\"), buf.buf);\n>> - strbuf_rtrim(split[0]);\n>> - strbuf_rtrim(split[1]);\n>> - if (get_sha1(split[0]->buf, from_obj))\n>> - die(_(\"Failed to resolve '%s' as a valid ref.\"), split[0]->buf);\n>> - if (get_sha1(split[1]->buf, to_obj))\n>> - die(_(\"Failed to resolve '%s' as a valid ref.\"), split[1]->buf);\n>> + if (get_sha1(split.items[0].string, from_obj))\n>> + die(_(\"Failed to resolve '%s' as a valid ref.\"), split.items[0].string);\n>> + if (get_sha1(split.items[1].string, to_obj))\n>> + die(_(\"Failed to resolve '%s' as a valid ref.\"), split.items[1].string);\n>>\n>> if (rewrite_cmd)\n>> err = copy_note_for_rewrite(c, from_obj, to_obj);\n>> @@ -313,11 +313,11 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n>>\n>> if (err) {\n>> error(_(\"Failed to copy notes from '%s' to '%s'\"),\n>> - split[0]->buf, split[1]->buf);\n>> + split.items[0].string, split.items[1].string);\n>> ret = 1;\n>> }\n>>\n>> - strbuf_list_free(split);\n>> + string_list_clear(&split, 0);\n>> }\n>>\n>> if (!rewrite_cmd) {\n>> --\n>> 2.4.3\n"},{"id":"279701","messageId":"56D2A9D3.4050303@moritzneeb.de","threadId":"41558","inReplyTo":"56D28092.9090209@moritzneeb.de","subject":"Re: [PATCH v3 0/7] replacing strbuf_getline_lf() by strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-28T08:03:31Z","receivedAt":"2016-02-28T08:03:31Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"On 02/28/2016 06:07 AM, Moritz Neeb wrote:\n> Changes since v2:\n> \n> * Line splitting in notes_copy_from_stdin() is changed to string_list_split as\n> suggested by Eric Sunshine.\n> * The behavior change in interactive cleaning from patch v2 is undone.\n> * Some of the previous patches were broken because of some unexpected\n> whitespace. This should be fixed now.\n> \n\nWhat I could not yet develop, is a feeling on \"how often\" to send out new versions of patches.\nFor a small patch like this it feels a bit weird to send out version after version, but maybe\nthat is a good practice? Now that I have implemented some (mainly textual) improvements that\ncame up through the review by Eric, would I rather send out a v4 or wait for comments by other\nreviewers?\n\nThanks\n"},{"id":"279716","messageId":"CAPig+cSOjdCkMAKEJ+7o=-cGsLgZWG5mnVmkWLSbyedg=mY5Lw@mail.gmail.com","threadId":"41558","inReplyTo":"56D2A60A.4000306@moritzneeb.de","subject":"Re: [PATCH v3 4/7] notes copy --stdin: split lines with string_list_split()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-28T16:02:38Z","receivedAt":"2016-02-28T16:02:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 28, 2016 at 2:47 AM, Moritz Neeb wrote:\n> On 02/28/2016 07:56 AM, Eric Sunshine wrote:\n>> On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb wrote:\n>>> The strbuf is now rtrimmed before splitting. This is still required\n>>> to remove potential CRs. In the next step this will then be done\n>>> implicitly by strbuf_readline(). Thus, this is a preparatory refactoring,\n>>> towards a trim-free codepath.\n>>\n>> I would actually swap patches 4 and 5 so that strbuf_getline() is done\n>> first (without removing any of the rtrim's) and string_list_split()\n>> second. That way, you don't have to add that extra rtrim in one patch\n>> and immediately remove it in the next. And, as a bonus, you can drop\n>> the above paragraph altogether.\n>\n> Yeah, I also was thinking about that, should've pointed that out.\n> I was just following your \"guiding\" in v2 [1], that's why I did it this way,\n> because I thought it is somehow expected to be a prepraratory change.\n\nIndeed, I meant to add a footnote acknowledging that and saying\nsomething along the lines of: \"Despite suggesting this as a\npreparatory change in my v2 review, having now seen actually seen it,\nit makes more sense as a follow-on change.\"\n\n> Ok, when switching 4 and 5, I could call it something like \"post cleanup/refactoring\"\n> instead.\n>\n> [1] http://article.gmane.org/gmane.comp.version-control.git/286868\n"},{"id":"279745","messageId":"56D401C2.8020100@moritzneeb.de","threadId":"41558","inReplyTo":"56D28092.9090209@moritzneeb.de","subject":"[PATCH v4 0/7] replacing strbuf_getline_lf() by strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-29T08:30:58Z","receivedAt":"2016-02-29T08:30:58Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"Although I was not sure [4], I decided to roll out v4, in the hope that the next\nreviewers will profit by the more polished commit messages and order.\n\nThis series deals with strbuf_getline_lf() in certain codepaths:\nThose, where the input that is read, is/was trimmed before doing anything that\ncould possibly expect a CR character. Those places can be assumed to be \"text\"\ninput, where a CR never would be a meaningful control character.\n\nThe purpose of this series is to document these places to have this property,\nby using strbuf_getline() instead of strbuf_getline_lf(). Also in some codepaths,\nthe CR could be a leftover of an editor and is thus removed.\n\nEvery codepath was examined, if after the change it is still necessary to have\ntrimming or if the additional CRLF-removal suffices.\n\nThe series is an idea out of [1], where Junio proposed to replace the calls\nto strbuf_getline_lf() because it 'would [be] a good way to document them as\ndealing with \"text\"'. \n\nChanges since v3 [3] (the changes to single patches are indicated below):\n\n * Commit messages refined\n * Order of patch 4 and 5 in v2 was switched.\n\nThe interdiff only removes an empty line (I noticed, when changing the order of\ncommits, that the splitting operation had no newline before this whole series,\nso I left it that way):\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 660c0b7..715fade 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -296,7 +296,6 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n int err;\n \n string_list_split(&split, buf.buf, ' ', -1);\n-\n if (split.nr != 2)\n die(_(\"Malformed input line: '%s'.\"), buf.buf);\n if (get_sha1(split.items[0].string, from_obj))\n\n-Moritz\n\n[1], idea: http://thread.gmane.org/gmane.comp.version-control.git/284104\n[2], v2: http://thread.gmane.org/gmane.comp.version-control.git/285118/focus=286865\n[3], v3: http://thread.gmane.org/gmane.comp.version-control.git/285118/focus=287747\n[4] http://thread.gmane.org/gmane.comp.version-control.git/285118/focus=287760\n\nMoritz Neeb (7):\n quote: remove leading space in sq_dequote_step -- as in v2\n bisect: read bisect paths with strbuf_getline() -- refined commit message\n clean: read user input with strbuf_getline() -- simplified commit message\n notes copy --stdin: read lines with strbuf_getline() -- switched with below\n notes copy --stdin: split lines with string_list_split() -- switched with above\n remote: read $GIT_DIR/branches/* with strbuf_getline() -- as in v3\n wt-status: read rebase todolist with strbuf_getline() -- as in v2\n\n bisect.c | 5 ++---\n builtin/clean.c | 6 +++---\n builtin/notes.c | 22 ++++++++++------------\n quote.c | 2 ++\n remote.c | 2 +-\n wt-status.c | 3 +--\n 6 files changed, 19 insertions(+), 21 deletions(-)\n\n-- \n2.4.3\n"},{"id":"279749","messageId":"56D40301.8020007@moritzneeb.de","threadId":"41558","inReplyTo":"56D401C2.8020100@moritzneeb.de","subject":"[PATCH v4 1/7] quote: remove leading space in sq_dequote_step","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-29T08:36:17Z","receivedAt":"2016-02-29T08:36:17Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"Because sq_quote_argv adds a leading space (which is expected in trace.c),\nsq_dequote_step should remove this space again, such that the operations\nof quoting and dequoting are inverse of each other.\n\nThis patch is preparing the way to remove some excessive trimming\noperation in bisect in the following commit.\n\nSigned-off-by: Moritz Neeb \n---\n quote.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/quote.c b/quote.c\nindex fe884d2..2714f27 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -63,6 +63,8 @@ static char *sq_dequote_step(char *arg, char **next)\n \tchar *src = arg;\n \tchar c;\n \n+\tif (*src == ' ')\n+\t\tsrc++;\n \tif (*src != '\\'')\n \t\treturn NULL;\n \tfor (;;) {\n-- \n2.4.3\n"},{"id":"279746","messageId":"56D40304.30205@moritzneeb.de","threadId":"41558","inReplyTo":"56D401C2.8020100@moritzneeb.de","subject":"[PATCH v4 2/7] bisect: read bisect paths with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-29T08:36:20Z","receivedAt":"2016-02-29T08:36:20Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"The file BISECT_NAMES is written by \"git rev-parse --sq-quote\" via\nsq_quote_argv() when starting a bisection. It can contain pathspecs\nto narrow down the search. When reading it back, it should be expected that\nsq_dequote_to_argv_array() is able to parse this file. In fact, the\nprevious commit ensures this.\n\nAs the content is of type \"text\", that means there is no logic expecting\nCR, strbuf_getline_lf() will be replaced by strbuf_getline().\n\nApart from whitespace added and removed in quote.c, no other whitespaces\nare expected. While it is technically possible, we have never advertised\nthis file to be editable by user, or encouraged them to do so. As a\nconsequence, the parsing of BISECT_NAMES is tightened by removing\nstrbuf_trim().\n\nFor the case that this file is modified nonetheless, in an invalid way\nsuch that dequoting fails, the error message is broadened to both cases:\nbad quoting and unexpected whitespace.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Moritz Neeb \n---\n bisect.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 7996c29..f63aa10 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -440,10 +440,9 @@ static void read_bisect_paths(struct argv_array *array)\n \tif (!fp)\n \t\tdie_errno(\"Could not open file '%s'\", filename);\n \n-\twhile (strbuf_getline_lf(&str, fp) != EOF) {\n-\t\tstrbuf_trim(&str);\n+\twhile (strbuf_getline(&str, fp) != EOF) {\n \t\tif (sq_dequote_to_argv_array(str.buf, array))\n-\t\t\tdie(\"Badly quoted content in file '%s': %s\",\n+\t\t\tdie(\"Badly quoted content or unexpected whitespace in file '%s': %s\",\n \t\t\t filename, str.buf);\n \t}\n \n-- \n2.4.3\n"},{"id":"279747","messageId":"56D40307.4080903@moritzneeb.de","threadId":"41558","inReplyTo":"56D401C2.8020100@moritzneeb.de","subject":"[PATCH v4 3/7] clean: read user input with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-29T08:36:23Z","receivedAt":"2016-02-29T08:36:23Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"The inputs that are read are all answers that are given by the user\nwhen interacting with git on the commandline. As these answers are\nnot supposed to contain a meaningful CR it is safe to\nreplace strbuf_getline_lf() by strbuf_getline().\n\nAfter being read, the input is trimmed. This leads to accepting user\ninput with spaces, e.g. \" y \", as a valid answer in the interactive\ncleaning process.\n\nAlthough trimming would not be required anymore to remove a potential CR,\nwe don't want to change the existing behavior with this patch.\nThus, the trimming is kept in place.\n\nSigned-off-by: Moritz Neeb \n---\n builtin/clean.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 0371010..5b17a31 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -570,7 +570,7 @@ static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff)\n \t\t\t clean_get_color(CLEAN_COLOR_RESET));\n \t\t}\n \n-\t\tif (strbuf_getline_lf(&choice, stdin) != EOF) {\n+\t\tif (strbuf_getline(&choice, stdin) != EOF) {\n \t\t\tstrbuf_trim(&choice);\n \t\t} else {\n \t\t\teof = 1;\n@@ -652,7 +652,7 @@ static int filter_by_patterns_cmd(void)\n \t\tclean_print_color(CLEAN_COLOR_PROMPT);\n \t\tprintf(_(\"Input ignore patterns>> \"));\n \t\tclean_print_color(CLEAN_COLOR_RESET);\n-\t\tif (strbuf_getline_lf(&confirm, stdin) != EOF)\n+\t\tif (strbuf_getline(&confirm, stdin) != EOF)\n \t\t\tstrbuf_trim(&confirm);\n \t\telse\n \t\t\tputchar('\\n');\n@@ -750,7 +750,7 @@ static int ask_each_cmd(void)\n \t\t\tqname = quote_path_relative(item->string, NULL, &buf);\n \t\t\t/* TRANSLATORS: Make sure to keep [y/N] as is */\n \t\t\tprintf(_(\"Remove %s [y/N]? \"), qname);\n-\t\t\tif (strbuf_getline_lf(&confirm, stdin) != EOF) {\n+\t\t\tif (strbuf_getline(&confirm, stdin) != EOF) {\n \t\t\t\tstrbuf_trim(&confirm);\n \t\t\t} else {\n \t\t\t\tputchar('\\n');\n-- \n2.4.3\n"},{"id":"279752","messageId":"56D4030B.8000303@moritzneeb.de","threadId":"41558","inReplyTo":"56D401C2.8020100@moritzneeb.de","subject":"[PATCH v4 5/7] notes copy --stdin: split lines with string_list_split()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-29T08:36:27Z","receivedAt":"2016-02-29T08:36:27Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"strbuf_split() has the unfortunate behavior of leaving the\nseparator character on the end of the split components, thus\nplacing the burden of manually removing the separator on the\ncaller. It's also heavyweight in that each split component is a\nfull-on strbuf. We need neither feature of strbuf_split() so\nlet's use string_list_split() instead since it removes the\nseparator character and returns an array of simple NUL-terminated\nstrings.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Moritz Neeb \n---\n builtin/notes.c | 19 +++++++++----------\n 1 file changed, 9 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 706ec11..715fade 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -292,17 +292,16 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n \n \twhile (strbuf_getline(&buf, stdin) != EOF) {\n \t\tunsigned char from_obj[20], to_obj[20];\n-\t\tstruct strbuf **split;\n+\t\tstruct string_list split = STRING_LIST_INIT_DUP;\n \t\tint err;\n \n-\t\tsplit = strbuf_split(&buf, ' ');\n-\t\tif (!split[0] || !split[1])\n+\t\tstring_list_split(&split, buf.buf, ' ', -1);\n+\t\tif (split.nr != 2)\n \t\t\tdie(_(\"Malformed input line: '%s'.\"), buf.buf);\n-\t\tstrbuf_rtrim(split[0]);\n-\t\tif (get_sha1(split[0]->buf, from_obj))\n-\t\t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), split[0]->buf);\n-\t\tif (get_sha1(split[1]->buf, to_obj))\n-\t\t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), split[1]->buf);\n+\t\tif (get_sha1(split.items[0].string, from_obj))\n+\t\t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), split.items[0].string);\n+\t\tif (get_sha1(split.items[1].string, to_obj))\n+\t\t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), split.items[1].string);\n \n \t\tif (rewrite_cmd)\n \t\t\terr = copy_note_for_rewrite(c, from_obj, to_obj);\n@@ -312,11 +311,11 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n \n \t\tif (err) {\n \t\t\terror(_(\"Failed to copy notes from '%s' to '%s'\"),\n-\t\t\t split[0]->buf, split[1]->buf);\n+\t\t\t split.items[0].string, split.items[1].string);\n \t\t\tret = 1;\n \t\t}\n \n-\t\tstrbuf_list_free(split);\n+\t\tstring_list_clear(&split, 0);\n \t}\n \n \tif (!rewrite_cmd) {\n-- \n2.4.3\n"},{"id":"279750","messageId":"56D4030E.4030301@moritzneeb.de","threadId":"41558","inReplyTo":"56D401C2.8020100@moritzneeb.de","subject":"[PATCH v4 6/7] remote: read $GIT_DIR/branches/* with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-29T08:36:30Z","receivedAt":"2016-02-29T08:36:30Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"The line read from the branch file is directly trimmed after reading with\nstrbuf_trim(). There is thus no logic expecting CR, so strbuf_getline_lf()\ncan be replaced by its CRLF counterpart.\n\nSigned-off-by: Moritz Neeb \n---\n remote.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/remote.c b/remote.c\nindex fc02698..77e011a 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -281,7 +281,7 @@ static void read_branches_file(struct remote *remote)\n \tif (!f)\n \t\treturn;\n \n-\tstrbuf_getline_lf(&buf, f);\n+\tstrbuf_getline(&buf, f);\n \tfclose(f);\n \tstrbuf_trim(&buf);\n \tif (!buf.len) {\n-- \n2.4.3\n"},{"id":"279751","messageId":"56D40311.9080603@moritzneeb.de","threadId":"41558","inReplyTo":"56D401C2.8020100@moritzneeb.de","subject":"[PATCH v4 7/7] wt-status: read rebase todolist with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-29T08:36:33Z","receivedAt":"2016-02-29T08:36:33Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"In read_rebase_todolist() the files $GIT_DIR/rebase-merge/done and\n$GIT_DIR/rebase-merge/git-rebase-todo are read to collect status\ninformation.\n\nThe access to this file should always happen via git rebase, e.g. via\n\"git rebase -i\" or \"git rebase --edit-todo\". We can assume, that this\ninterface handles the preprocessing of whitespaces, especially CRLFs\ncorrectly. Thus in this codepath we can remove the call to strbuf_trim().\n\nFor documenting the input as expecting \"text\" input, strbuf_getline_lf()\nis still replaced by strbuf_getline().\n\nSigned-off-by: Moritz Neeb \n---\n wt-status.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex ab4f80d..8047cf2 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1076,10 +1076,9 @@ static void read_rebase_todolist(const char *fname, struct string_list *lines)\n \tif (!f)\n \t\tdie_errno(\"Could not open file %s for reading\",\n \t\t\t git_path(\"%s\", fname));\n-\twhile (!strbuf_getline_lf(&line, f)) {\n+\twhile (!strbuf_getline(&line, f)) {\n \t\tif (line.len && line.buf[0] == comment_line_char)\n \t\t\tcontinue;\n-\t\tstrbuf_trim(&line);\n \t\tif (!line.len)\n \t\t\tcontinue;\n \t\tabbrev_sha1_in_line(&line);\n-- \n2.4.3\n"},{"id":"279748","messageId":"56D40314.7040608@moritzneeb.de","threadId":"41558","inReplyTo":"56D401C2.8020100@moritzneeb.de","subject":"[PATCH v4 4/7] notes copy --stdin: read lines with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-29T08:36:36Z","receivedAt":"2016-02-29T08:36:36Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"The format of a line that is expected when copying notes via stdin\nis \"sha1 sha1\". As this is text-only, strbuf_getline() should be used\ninstead of strbuf_getline_lf(), as documentation of this fact.\n\nWhen reading with strbuf_getline() the trimming of split[1] can be\nremoved. It was necessary before to remove potential CRs inserted\nthrough a dos editor.\n\nSigned-off-by: Moritz Neeb \n---\n builtin/notes.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex ed6f222..706ec11 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -290,7 +290,7 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n \t\tt = &default_notes_tree;\n \t}\n \n-\twhile (strbuf_getline_lf(&buf, stdin) != EOF) {\n+\twhile (strbuf_getline(&buf, stdin) != EOF) {\n \t\tunsigned char from_obj[20], to_obj[20];\n \t\tstruct strbuf **split;\n \t\tint err;\n@@ -299,7 +299,6 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n \t\tif (!split[0] || !split[1])\n \t\t\tdie(_(\"Malformed input line: '%s'.\"), buf.buf);\n \t\tstrbuf_rtrim(split[0]);\n-\t\tstrbuf_rtrim(split[1]);\n \t\tif (get_sha1(split[0]->buf, from_obj))\n \t\t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), split[0]->buf);\n \t\tif (get_sha1(split[1]->buf, to_obj))\n-- \n2.4.3\n"},{"id":"279795","messageId":"CAPig+cSptQr21QMOJmxT4RPVR3r3zkEQ2TkTU8RoaJfo7=KChw@mail.gmail.com","threadId":"41558","inReplyTo":"56D40314.7040608@moritzneeb.de","subject":"Re: [PATCH v4 4/7] notes copy --stdin: read lines with strbuf_getline()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-29T18:19:08Z","receivedAt":"2016-02-29T18:19:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 29, 2016 at 3:36 AM, Moritz Neeb wrote:\n> The format of a line that is expected when copying notes via stdin\n> is \"sha1 sha1\". As this is text-only, strbuf_getline() should be used\n> instead of strbuf_getline_lf(), as documentation of this fact.\n>\n> When reading with strbuf_getline() the trimming of split[1] can be\n> removed. It was necessary before to remove potential CRs inserted\n> through a dos editor.\n\ns/dos/DOS/\n\nThis may not be worth a re-roll, but the suggestion of my v3 review\nwas to keep both rtrim's in this patch and then remove them in the\nnext patch when converting to string_list_split(). A benefit of doing\nso is that you can then drop the above paragraph altogether, and both\npatches become simpler (description and content), thus easier to\nreview.\n\nIf you do elect to keep things the way they are, then (as mentioned in\nmy v2 review) it would be helpful for the above paragraph to explain\nthat strbuf_split() leave the \"terminator\" on the split elements, thus\nclarifying why the rtrim() of split[0] is still needed.\n\n> Signed-off-by: Moritz Neeb \n> ---\n> builtin/notes.c | 3 +--\n> 1 file changed, 1 insertion(+), 2 deletions(-)\n>\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index ed6f222..706ec11 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -290,7 +290,7 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n> t = &default_notes_tree;\n> }\n>\n> - while (strbuf_getline_lf(&buf, stdin) != EOF) {\n> + while (strbuf_getline(&buf, stdin) != EOF) {\n> unsigned char from_obj[20], to_obj[20];\n> struct strbuf **split;\n> int err;\n> @@ -299,7 +299,6 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n> if (!split[0] || !split[1])\n> die(_(\"Malformed input line: '%s'.\"), buf.buf);\n> strbuf_rtrim(split[0]);\n> - strbuf_rtrim(split[1]);\n> if (get_sha1(split[0]->buf, from_obj))\n> die(_(\"Failed to resolve '%s' as a valid ref.\"), split[0]->buf);\n> if (get_sha1(split[1]->buf, to_obj))\n> --\n> 2.4.3\n"},{"id":"279799","messageId":"CAPig+cT4+bKS53E6bmLNn=xWkXK2Tx8N9bDEqGCNwNy-qjMOUg@mail.gmail.com","threadId":"41558","inReplyTo":"56D401C2.8020100@moritzneeb.de","subject":"Re: [PATCH v4 0/7] replacing strbuf_getline_lf() by strbuf_getline()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-29T18:26:48Z","receivedAt":"2016-02-29T18:26:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 29, 2016 at 3:30 AM, Moritz Neeb wrote:\n> Although I was not sure [4], I decided to roll out v4, in the hope that the next\n> reviewers will profit by the more polished commit messages and order.\n>\n> Changes since v3 [3] (the changes to single patches are indicated below):\n>\n> * Commit messages refined\n> * Order of patch 4 and 5 in v2 was switched.\n\nThanks. With the exception of my commentary on patch 4/7, I think v4\naddresses all my v3 review comments.\n"},{"id":"279811","messageId":"xmqqy4a39w8b.fsf@gitster.mtv.corp.google.com","threadId":"41558","inReplyTo":"56D40301.8020007@moritzneeb.de","subject":"Re: [PATCH v4 1/7] quote: remove leading space in sq_dequote_step","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-29T19:01:40Z","receivedAt":"2016-02-29T19:01:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Moritz Neeb writes:\n\n> Because sq_quote_argv adds a leading space (which is expected in trace.c),\n> sq_dequote_step should remove this space again, such that the operations\n> of quoting and dequoting are inverse of each other.\n>\n> This patch is preparing the way to remove some excessive trimming\n> operation in bisect in the following commit.\n>\n> Signed-off-by: Moritz Neeb \n> ---\n> quote.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/quote.c b/quote.c\n> index fe884d2..2714f27 100644\n> --- a/quote.c\n> +++ b/quote.c\n> @@ -63,6 +63,8 @@ static char *sq_dequote_step(char *arg, char **next)\n> \tchar *src = arg;\n> \tchar c;\n> \n> +\tif (*src == ' ')\n> +\t\tsrc++;\n> \tif (*src != '\\'')\n> \t\treturn NULL;\n> \tfor (;;) {\n\nIf we look at this \"for (;;)\" loop, we notice that (1) it accepts as\nmany spaces as there are between two quoted strings, and (2) it does\nnot limit it to SP but uses isspace().\n\nI wonder if you would instead want\n\n\twhile (isspace(*src))\n \tsrc++;\n\nto be consistent?\n"},{"id":"279828","messageId":"56D49B7A.2060601@moritzneeb.de","threadId":"41558","inReplyTo":"CAPig+cSptQr21QMOJmxT4RPVR3r3zkEQ2TkTU8RoaJfo7=KChw@mail.gmail.com","subject":"Re: [PATCH v4 4/7] notes copy --stdin: read lines with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-29T19:26:50Z","receivedAt":"2016-02-29T19:26:50Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"On 02/29/2016 07:19 PM, Eric Sunshine wrote:\n> If you do elect to keep things the way they are, then (as mentioned in\n> my v2 review) it would be helpful for the above paragraph to explain\n> that strbuf_split() leave the \"terminator\" on the split elements, thus\n> clarifying why the rtrim() of split[0] is still needed.\n> \n\nYes I would rather leave it like it is. I have the feeling it is\nunmotivated to remove the rtrim of split[1] in the patch 5/7, because it\nis directly related to the strbuf_getline_lf() replacement. Thats's what\nI was trying to explain in the 2nd paragraph of the commit message.\n\nFirst I was following your review, but then I had to add a paragraph in\npatch 5/7 that says something like \"because the effect of the previous\npatch is that there is not a CR anymore, we can now safely remove\nrtrim() split[1].\"\n\nYou're right, maybe I should add a comment about why I left rtrim() of\nsplit[0] to make it more obvious. I thought that would get clear by\nlooking at the context, i.e. patch 5/7, where it is explained (by you,\nthanks for that), that strbuf_split leave this space. Is the assumption,\nthat those two patches are most times viewed in context wrong?\n\nThanks,\nMoritz\n\n\n>> Signed-off-by: Moritz Neeb \n>> ---\n>> builtin/notes.c | 3 +--\n>> 1 file changed, 1 insertion(+), 2 deletions(-)\n>>\n>> diff --git a/builtin/notes.c b/builtin/notes.c\n>> index ed6f222..706ec11 100644\n>> --- a/builtin/notes.c\n>> +++ b/builtin/notes.c\n>> @@ -290,7 +290,7 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n>> t = &default_notes_tree;\n>> }\n>>\n>> - while (strbuf_getline_lf(&buf, stdin) != EOF) {\n>> + while (strbuf_getline(&buf, stdin) != EOF) {\n>> unsigned char from_obj[20], to_obj[20];\n>> struct strbuf **split;\n>> int err;\n>> @@ -299,7 +299,6 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n>> if (!split[0] || !split[1])\n>> die(_(\"Malformed input line: '%s'.\"), buf.buf);\n>> strbuf_rtrim(split[0]);\n>> - strbuf_rtrim(split[1]);\n>> if (get_sha1(split[0]->buf, from_obj))\n>> die(_(\"Failed to resolve '%s' as a valid ref.\"), split[0]->buf);\n>> if (get_sha1(split[1]->buf, to_obj))\n>> --\n>> 2.4.3\n"},{"id":"279835","messageId":"CAPig+cQwhPvpGmiOa-KJzeDsEJZVp9KJMd2Oj_RjDTq6aEtWXA@mail.gmail.com","threadId":"41558","inReplyTo":"56D49B7A.2060601@moritzneeb.de","subject":"Re: [PATCH v4 4/7] notes copy --stdin: read lines with strbuf_getline()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-29T19:48:18Z","receivedAt":"2016-02-29T19:48:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 29, 2016 at 2:26 PM, Moritz Neeb wrote:\n> On 02/29/2016 07:19 PM, Eric Sunshine wrote:\n>> If you do elect to keep things the way they are, then (as mentioned in\n>> my v2 review) it would be helpful for the above paragraph to explain\n>> that strbuf_split() leave the \"terminator\" on the split elements, thus\n>> clarifying why the rtrim() of split[0] is still needed.\n>\n> Yes I would rather leave it like it is. I have the feeling it is\n> unmotivated to remove the rtrim of split[1] in the patch 5/7, because it\n> is directly related to the strbuf_getline_lf() replacement. Thats's what\n> I was trying to explain in the 2nd paragraph of the commit message.\n>\n> First I was following your review, but then I had to add a paragraph in\n> patch 5/7 that says something like \"because the effect of the previous\n> patch is that there is not a CR anymore, we can now safely remove\n> rtrim() split[1].\"\n>\n> You're right, maybe I should add a comment about why I left rtrim() of\n> split[0] to make it more obvious. I thought that would get clear by\n> looking at the context, i.e. patch 5/7, where it is explained (by you,\n> thanks for that), that strbuf_split leave this space. Is the assumption,\n> that those two patches are most times viewed in context wrong?\n\nI was more concerned about someone reading patch 4/7 in isolation and\nnot consulting 5/7 (which might happen during a \"blame\" session, but\nit's a very minor point, not worth a re-roll if you and Junio are\nhappy with the series as is.\n"},{"id":"279851","messageId":"56D4BC14.90301@moritzneeb.de","threadId":"41558","inReplyTo":"xmqqy4a39w8b.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 1/7] quote: remove leading space in sq_dequote_step","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-29T21:45:56Z","receivedAt":"2016-02-29T21:45:56Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"On 02/29/2016 08:01 PM, Junio C Hamano wrote:\n> Moritz Neeb writes:\n> \n>> Because sq_quote_argv adds a leading space (which is expected in trace.c),\n>> sq_dequote_step should remove this space again, such that the operations\n>> of quoting and dequoting are inverse of each other.\n>>\n>> This patch is preparing the way to remove some excessive trimming\n>> operation in bisect in the following commit.\n>>\n>> Signed-off-by: Moritz Neeb \n>> ---\n>> quote.c | 2 ++\n>> 1 file changed, 2 insertions(+)\n>>\n>> diff --git a/quote.c b/quote.c\n>> index fe884d2..2714f27 100644\n>> --- a/quote.c\n>> +++ b/quote.c\n>> @@ -63,6 +63,8 @@ static char *sq_dequote_step(char *arg, char **next)\n>> \tchar *src = arg;\n>> \tchar c;\n>> \n>> +\tif (*src == ' ')\n>> +\t\tsrc++;\n>> \tif (*src != '\\'')\n>> \t\treturn NULL;\n>> \tfor (;;) {\n> \n> If we look at this \"for (;;)\" loop, we notice that (1) it accepts as\n> many spaces as there are between two quoted strings, and (2) it does\n> not limit it to SP but uses isspace().\n> \n> I wonder if you would instead want\n> \n> \twhile (isspace(*src))\n> \tsrc++;\n> \n> to be consistent?\n> \n\nMy intention was to explicitly remove the space added by\nstrbuf_addch(dst, ' ') in sq_quote_argv().\n\nI think it would not make sense to remove more spaces, because for\nfor sq_dequote() it is defined:\n\n\tThis unwraps what sq_quote() produces in place, but returns\n\tNULL if the input does not look like what sq_quote would have\n\tproduced.\n\nI understand that this counts also for the sq_dequote_array*() family.\n\nThanks\n"},{"id":"279852","messageId":"56D4BCB6.90209@moritzneeb.de","threadId":"41558","inReplyTo":"56D4BC14.90301@moritzneeb.de","subject":"Re: [PATCH v4 1/7] quote: remove leading space in sq_dequote_step","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-29T21:48:38Z","receivedAt":"2016-02-29T21:48:38Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"On 02/29/2016 10:45 PM, Moritz Neeb wrote:\n> On 02/29/2016 08:01 PM, Junio C Hamano wrote:\n>> Moritz Neeb writes:\n>>\n>>> Because sq_quote_argv adds a leading space (which is expected in trace.c),\n>>> sq_dequote_step should remove this space again, such that the operations\n>>> of quoting and dequoting are inverse of each other.\n>>>\n>>> This patch is preparing the way to remove some excessive trimming\n>>> operation in bisect in the following commit.\n>>>\n>>> Signed-off-by: Moritz Neeb \n>>> ---\n>>> quote.c | 2 ++\n>>> 1 file changed, 2 insertions(+)\n>>>\n>>> diff --git a/quote.c b/quote.c\n>>> index fe884d2..2714f27 100644\n>>> --- a/quote.c\n>>> +++ b/quote.c\n>>> @@ -63,6 +63,8 @@ static char *sq_dequote_step(char *arg, char **next)\n>>> \tchar *src = arg;\n>>> \tchar c;\n>>> \n>>> +\tif (*src == ' ')\n>>> +\t\tsrc++;\n>>> \tif (*src != '\\'')\n>>> \t\treturn NULL;\n>>> \tfor (;;) {\n>>\n>> If we look at this \"for (;;)\" loop, we notice that (1) it accepts as\n>> many spaces as there are between two quoted strings, and (2) it does\n>> not limit it to SP but uses isspace().\n>>\n>> I wonder if you would instead want\n>>\n>> \twhile (isspace(*src))\n>> \tsrc++;\n>>\n>> to be consistent?\n>>\n> \n> My intention was to explicitly remove the space added by\n> strbuf_addch(dst, ' ') in sq_quote_argv().\n> \n> I think it would not make sense to remove more spaces, because for\n> for sq_dequote() it is defined:\n> \n> \tThis unwraps what sq_quote() produces in place, but returns\n> \tNULL if the input does not look like what sq_quote would have\n> \tproduced.\n> \n> I understand that this counts also for the sq_dequote_array*() family.\n\ns/array/to_argv/\n\n> \n> Thanks\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"280398","messageId":"56DF6D67.9040103@moritzneeb.de","threadId":"41558","inReplyTo":"56D401C2.8020100@moritzneeb.de","subject":"Re: [PATCH v4 0/7] replacing strbuf_getline_lf() by strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-03-09T00:25:11Z","receivedAt":"2016-03-09T00:25:11Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"Hi,\n\nhow to deal with patches during the v2.8.0 rc freeze? Will they wait on\nthe mailing list until the feature release cycle is finished?\n\nOr if it's me who should act on this series, because it got below the\nradar during the rc freeze?\n\nTo my knowledge there's only minor points that have to be discussed:\n\nOn 02/29/2016 09:30 AM, Moritz Neeb wrote:\n> \n> Moritz Neeb (7):\n> quote: remove leading space in sq_dequote_step -- as in v2\n\nin patch 1/7: How many spaces should be removed, cf.:\n\n\thttp://thread.gmane.org/gmane.comp.version-control.git/285118/focus=287911\n\n> bisect: read bisect paths with strbuf_getline() -- refined commit message\n> clean: read user input with strbuf_getline() -- simplified commit message\n> notes copy --stdin: read lines with strbuf_getline() -- switched with below\n> notes copy --stdin: split lines with string_list_split() -- switched with above\n\nin patches 4/7 and 5/7: Which commit should remove the trimming of\n\"split[0]\", cf.:\n\n\thttp://thread.gmane.org/gmane.comp.version-control.git/285118/focus=287894\n\n> remote: read $GIT_DIR/branches/* with strbuf_getline() -- as in v3\n> wt-status: read rebase todolist with strbuf_getline() -- as in v2\n> \n> bisect.c | 5 ++---\n> builtin/clean.c | 6 +++---\n> builtin/notes.c | 22 ++++++++++------------\n> quote.c | 2 ++\n> remote.c | 2 +-\n> wt-status.c | 3 +--\n> 6 files changed, 19 insertions(+), 21 deletions(-)\n> \n"},{"id":"280399","messageId":"xmqq37s0jxgg.fsf@gitster.mtv.corp.google.com","threadId":"41558","inReplyTo":"56DF6D67.9040103@moritzneeb.de","subject":"Re: [PATCH v4 0/7] replacing strbuf_getline_lf() by strbuf_getline()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-09T00:39:59Z","receivedAt":"2016-03-09T00:39:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Moritz Neeb writes:\n\n> how to deal with patches during the v2.8.0 rc freeze? Will they wait on\n> the mailing list until the feature release cycle is finished?\n\nBecause people are expected to stop getting distracted by new\nfeatures and no-op clean-up changes and instead to focus on helping\nfind and fix regressions that have been introduced since v2.7.x\nseries during the pre-release period, you may not get review\ncomments unless your patches are really important.\n\nTo participate in regression hunting or not is your choice. In any\ncase, you'd likely be re-sending a reroll after a release concludes\nthis cycle in order to get sufficient reviews and Ack's, as people\nmay have expired the last round of patches from you from their\nmailboxes and their brain by then. And then we go from there.\n\nThanks.\n"},{"id":"280402","messageId":"56DF78BD.2030506@moritzneeb.de","threadId":"41558","inReplyTo":"xmqq37s0jxgg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 0/7] replacing strbuf_getline_lf() by strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-03-09T01:13:33Z","receivedAt":"2016-03-09T01:13:33Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"\n\nOn 03/09/2016 01:39 AM, Junio C Hamano wrote:\n> Moritz Neeb writes:\n> \n>> how to deal with patches during the v2.8.0 rc freeze? Will they wait on\n>> the mailing list until the feature release cycle is finished?\n> \n> Because people are expected to stop getting distracted by new\n> features and no-op clean-up changes and instead to focus on helping\n> find and fix regressions that have been introduced since v2.7.x\n> series during the pre-release period, you may not get review\n> comments unless your patches are really important.\n> \n\nOk, I was not sure, sorry for the noise generated. Will resend post-release.\n\n> To participate in regression hunting or not is your choice.\n\nSay, I would like to participate. This might be a very naive question,\nbut: What is the \"workflow\" in regression hunting?\n\nThere is this known \"git status\"-regression, where you seem to be at\nleast close to a decision:\n\n\thttp://thread.gmane.org/gmane.comp.version-control.git/288228/focus=288444\n\nWhat I can imagine could lead towards finding regessions, though maybe a\nbit aimless: Go through the list of changes/patches that are supposed to\nbe included in v2.8.0 and confirm they are working as expected. This\nwould be like a post-review.\n"},{"id":"280404","messageId":"CAPig+cRGH=ae2jm4YbwTUpNPx7ZgiZkMf=xKCPP=Ewsr9xJu+g@mail.gmail.com","threadId":"41558","inReplyTo":"56DF6D67.9040103@moritzneeb.de","subject":"Re: [PATCH v4 0/7] replacing strbuf_getline_lf() by strbuf_getline()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-09T01:17:08Z","receivedAt":"2016-03-09T01:17:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 8, 2016 at 7:25 PM, Moritz Neeb wrote:\n> how to deal with patches during the v2.8.0 rc freeze? Will they wait on\n> the mailing list until the feature release cycle is finished?\n>\n> Or if it's me who should act on this series, because it got below the\n> radar during the rc freeze?\n>\n> To my knowledge there's only minor points that have to be discussed:\n>\n> in patches 4/7 and 5/7: Which commit should remove the trimming of\n> \"split[0]\", cf.:\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/285118/focus=287894\n\nThe reason I brought the issue up in review is that it wasn't clear if\ndropping the rtrims over the course of two patches was intentional or\njust an oversight (given that the review recommendation from the\nprevious round had suggested removing both in the same patch). From\nyour response, it is clear that it was intentional and, as mentioned\nin the cited messages, while I do have an opinion on the matter, it's\nsuch a minor point that it's not worth a lot of back and forth, and\nthe patch can move forward if you're happy with it the way it is.\n"},{"id":"280507","messageId":"xmqqr3fjgzuq.fsf@gitster.mtv.corp.google.com","threadId":"41558","inReplyTo":"56DF78BD.2030506@moritzneeb.de","subject":"Re: [PATCH v4 0/7] replacing strbuf_getline_lf() by strbuf_getline()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-09T20:28:45Z","receivedAt":"2016-03-09T20:28:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Moritz Neeb writes:\n\n> What I can imagine could lead towards finding regessions, though\n> maybe a bit aimless: Go through the list of changes/patches that\n> are supposed to be included in v2.8.0 and confirm they are working\n> as expected. This would be like a post-review.\n\nThat would be one way for a very dedicated contributor.\n\nNormal use of Git in your everyday workflow, noticing any unexpected\nbehaviour and digging into it would be what I had in mind, though\n;-)\n"}]}