{"thread":{"id":"41462","subject":"[PATCH v2 2/6] bisect: read bisect paths with strbuf_getline()","startedAt":"2016-02-22T01:00:43Z","lastAt":"2016-02-22T19:40:53Z","messageCount":14,"participants":["Moritz Neeb","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":6},"messages":[{"id":"278816","messageId":"56CA5DBB.8040006@moritzneeb.de","threadId":"41462","inReplyTo":"56ACF82B.2030005@moritzneeb.de","subject":"[PATCH v2 0/6] replacing strbuf_getline_lf() by strbuf_getline() on trimmed input","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-22T01:00:43Z","receivedAt":"2016-02-22T01:00:43Z","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 CRLR-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\"'. \nChanges since v1:\n\n* adapting the behaviour of sq_(de)quote to be a one-to-one transformation\n* removing some unneccesary trimming calls in:\n * wt-status.c\n * builting/notes.c\n * builtin/clean.c\n * bisect.c\n\n-Moritz\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/284104\n\nMoritz Neeb (6):\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: read copied notes 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 | 12 +++---------\n builtin/notes.c | 3 +--\n quote.c | 2 ++\n remote.c | 2 +-\n wt-status.c | 3 +--\n 6 files changed, 10 insertions(+), 17 deletions(-)\n\n-- \n2.7.1.345.gc14003e\n"},{"id":"278810","messageId":"56CA6118.2060407@moritzneeb.de","threadId":"41462","inReplyTo":"56CA5DBB.8040006@moritzneeb.de","subject":"[PATCH v2 1/6] quote: remove leading space in sq_dequote_step","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-22T01:15:04Z","receivedAt":"2016-02-22T01:15:04Z","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 +\tif (*src == ' ')\n+\t\tsrc++;\n \tif (*src != '\\'')\n \t\treturn NULL;\n \tfor (;;) {\n-- \n2.7.1.345.gc14003e\n"},{"id":"278809","messageId":"56CA613C.1080106@moritzneeb.de","threadId":"41462","inReplyTo":"56CA5DBB.8040006@moritzneeb.de","subject":"[PATCH v2 2/6] bisect: read bisect paths with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-22T01:15:40Z","receivedAt":"2016-02-22T01:15:40Z","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 -\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 -- 2.7.1.345.gc14003e\n"},{"id":"278814","messageId":"56CA6160.7010908@moritzneeb.de","threadId":"41462","inReplyTo":"56CA5DBB.8040006@moritzneeb.de","subject":"[PATCH v2 4/6] notes: read copied notes with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-22T01:16:16Z","receivedAt":"2016-02-22T01:16:16Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"The notes are copied from stdin. They should only contain SHA1s... Not\nspaces. CR could be there, because the file/the data from stdin could\nhave been written via an editor that adds them.\n\nThe notes that are copied from stdin are trimmed with strbuf_rtrim() after\nsplitting by ' '. 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 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 -\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.7.1.345.gc14003e\n"},{"id":"278817","messageId":"56CA61B2.2020904@moritzneeb.de","threadId":"41462","inReplyTo":"56CA5DBB.8040006@moritzneeb.de","subject":"[PATCH v2 6/6] wt-status: read rebase todolist with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-22T01:17:38Z","receivedAt":"2016-02-22T01:17:38Z","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.7.1.345.gc14003e\n"},{"id":"278811","messageId":"56CA6264.1040400@moritzneeb.de","threadId":"41462","inReplyTo":"56CA5DBB.8040006@moritzneeb.de","subject":"[PATCH v2 3/6] clean: read user input with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-22T01:20:36Z","receivedAt":"2016-02-22T01:20:36Z","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\nBefore the user input was trimmed to remove the CR. This would be now\nredundant. Another effect of the trimming was that some (accidentally)\ntyped spaces were filtered. But here we want to be consistent with similar UIs\nlike interactive adding, which only accepts space-less input.\n\nFor the case of filtering by patterns the input is still trimmed in an\nuntouched codepath after it is split up into multiple patterns.\nThis is considered as desirable, because of two reasons:\nFirst this fitering is not part of similar UIs and it is way more likely\nto accidentally type a space in this way of interacting.\n\nSigned-off-by: Moritz Neeb \n---\nWhen playing around with the interactive git clean I noticed that it is\nnot possible to have a pattern actually containing a space (i.e. escaping it).\nNot sure how relevant this is, because I have no feeling how good the support\nand demand for smoothly handling space-containing files in git is.\n\n builtin/clean.c | 12 +++---------\n 1 file changed, 3 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 7b08237..01cc2ff 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -570,9 +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 -\t\tif (strbuf_getline_lf(&choice, stdin) != EOF) {\n-\t\t\tstrbuf_trim(&choice);\n-\t\t} else {\n+\t\tif (strbuf_getline(&choice, stdin) == EOF) {\n \t\t\teof = 1;\n \t\t\tbreak;\n \t\t}\n@@ -652,9 +650,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\t\tstrbuf_trim(&confirm);\n-\t\telse\n+\t\tif (strbuf_getline(&confirm, stdin) == EOF)\n \t\t\tputchar('\\n');\n \t\t/* quit filter_by_pattern mode if press ENTER or Ctrl-D */\n@@ -750,9 +746,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\t\tstrbuf_trim(&confirm);\n-\t\t\t} else {\n+\t\t\tif (strbuf_getline(&confirm, stdin) == EOF) {\n \t\t\t\tputchar('\\n');\n \t\t\t\teof = 1;\n \t\t\t}\n-- \n2.7.1.345.gc14003e\n"},{"id":"278813","messageId":"56CA62D3.7060808@moritzneeb.de","threadId":"41462","inReplyTo":"56CA5DBB.8040006@moritzneeb.de","subject":"[PATCH v2 5/6] remote: read $GIT_DIR/branches/* with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-22T01:22:27Z","receivedAt":"2016-02-22T01:22:27Z","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---\nTo be honest, I did not yet fully understand the purpose of this branches/ file.\nWhat I'd expect is that it is some intermediary file while fetching?\nOr is it edited directly by the user and thus it's necessary to strip spaces\nthat could be added accidentally?\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 -\tstrbuf_getline_lf(&buf, f);\n+\tstrbuf_getline(&buf, f);\n \tfclose(f);\n \tstrbuf_trim(&buf);\n \tif (!buf.len) {\n-- \n2.7.1.345.gc14003e\n"},{"id":"278818","messageId":"CAPig+cSi-4R-a=HVmpCWAZ3kr=yQtJ9GdT-JZ4hJ2kmqg-edVA@mail.gmail.com","threadId":"41462","inReplyTo":"56CA6264.1040400@moritzneeb.de","subject":"Re: [PATCH v2 3/6] clean: read user input with strbuf_getline()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-22T02:27:29Z","receivedAt":"2016-02-22T02:27:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 21, 2016 at 8:20 PM, 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> Before the user input was trimmed to remove the CR. This would be now\n> redundant. Another effect of the trimming was that some (accidentally)\n> typed spaces were filtered. But here we want to be consistent with similar UIs\n> like interactive adding, which only accepts space-less input.\n\nI don't at all insist upon it, but this behavior change feels somewhat\nlike it ought to be in its own commit. I'm also not convinced that\nmaking this consistent with the less forgiving behavior of\n\"interactive adding\" is desirable (rather the reverse: that that case\nshould be more flexible). However, I wasn't following the discussion\nwith Junio closely, and perhaps missed you two agreeing that this is\npreferable.\n\n> For the case of filtering by patterns the input is still trimmed in an\n> untouched codepath after it is split up into multiple patterns.\n> This is considered as desirable, because of two reasons:\n\ns/, because of/ for/\n\n> First this fitering is not part of similar UIs and it is way more likely\n> to accidentally type a space in this way of interacting.\n>\n> Signed-off-by: Moritz Neeb \n> ---\n> diff --git a/builtin/clean.c b/builtin/clean.c\n> @@ -570,9 +570,7 @@ static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff)\n> clean_get_color(CLEAN_COLOR_RESET));\n> }\n> - if (strbuf_getline_lf(&choice, stdin) != EOF) {\n> - strbuf_trim(&choice);\n> - } else {\n> + if (strbuf_getline(&choice, stdin) == EOF) {\n> eof = 1;\n> break;\n> }\n> @@ -652,9 +650,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> - strbuf_trim(&confirm);\n> - else\n> + if (strbuf_getline(&confirm, stdin) == EOF)\n> putchar('\\n');\n> /* quit filter_by_pattern mode if press ENTER or Ctrl-D */\n> @@ -750,9 +746,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> - strbuf_trim(&confirm);\n> - } else {\n> + if (strbuf_getline(&confirm, stdin) == EOF) {\n> putchar('\\n');\n> eof = 1;\n> }\n> --\n> 2.7.1.345.gc14003e\n"},{"id":"278819","messageId":"CAPig+cReRiwHBJiatWJ=Gc+k+dtcMhdwFn4K57yHAjE3d_fzwQ@mail.gmail.com","threadId":"41462","inReplyTo":"56CA6160.7010908@moritzneeb.de","subject":"Re: [PATCH v2 4/6] notes: read copied notes with strbuf_getline()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-22T02:41:44Z","receivedAt":"2016-02-22T02:41:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 21, 2016 at 8:16 PM, Moritz Neeb wrote:\n> The notes are copied from stdin. They should only contain SHA1s... Not\n> spaces. CR could be there, because the file/the data from stdin could\n> have been written via an editor that adds them.\n>\n> The notes that are copied from stdin are trimmed with strbuf_rtrim() after\n> splitting by ' '. There is thus no logic expecting CR, so strbuf_getline_lf()\n> can be replaced by its CRLF counterpart.\n>\n> Signed-off-by: Moritz Neeb \n> ---\n> diff --git a/builtin/notes.c 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> - 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\nGiven the commit message, I understand that this rtrim is effectively\nredundant, thus can be dropped, however, I'm not sure that doing so\nimproves the code since the reader now has to think extra hard to\nunderstand the asymmetry of only trimming split[0] (and that\nunderstanding may require blaming this code in order to consult the\ncommit message).\n\nA deeper issue not touched upon by the commit message (but which\nshould be) is that that strbuf_split() leaves the \"terminator\" (space,\nin this case) on the component strings, and that is why split[0] must\nbe rtrim'd. Rather than dropping only one of the rtrim's, a cleaner\napproach might be to convert the code to use string_list_split() which\ndoesn't have the \"odd\" behavior of leaving the terminator on the split\nstrings, in which case both rtrim's could be retired. This, of course,\nwould be done as a separate preparatory patch.\n\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.7.1.345.gc14003e\n"},{"id":"278840","messageId":"56CABB8B.2070502@moritzneeb.de","threadId":"41462","inReplyTo":"CAPig+cSi-4R-a=HVmpCWAZ3kr=yQtJ9GdT-JZ4hJ2kmqg-edVA@mail.gmail.com","subject":"Re: [PATCH v2 3/6] clean: read user input with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-22T07:40:59Z","receivedAt":"2016-02-22T07:40:59Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"On 02/22/2016 03:27 AM, Eric Sunshine wrote:\n> On Sun, Feb 21, 2016 at 8:20 PM, 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>> Before the user input was trimmed to remove the CR. This would be now\n>> redundant. Another effect of the trimming was that some (accidentally)\n>> typed spaces were filtered. But here we want to be consistent with similar UIs\n>> like interactive adding, which only accepts space-less input.\n> \n> I don't at all insist upon it, but this behavior change feels somewhat\n> like it ought to be in its own commit.\n\nYou're right, two commits would be nicer. I was also thinking about\nsplitting up the three codepaths, but I decided all of the\nclean-interaction belongs together.\n\n> I'm also not convinced that\n> making this consistent with the less forgiving behavior of\n> \"interactive adding\" is desirable (rather the reverse: that that case\n> should be more flexible). However, I wasn't following the discussion\n> with Junio closely, and perhaps missed you two agreeing that this is\n> preferable.\n\nTo summarize the discussion with Junio: We were not directly talking\nabout that. Two aspects from the whole discussion were that I should\ndecide something and justify a stance (which I did) and that it's also\nbeneficial to think aloud (which I forgot).\n\nIn fact, I was surprised, that interactive adding is that strict. I\nshould've added that to the discussion. I am at the moment sometimes\nunsure whether I find things weird because git standards are different\nthan what I'd expect or because things really should be changed. So I\nwent for the former and decided to go for consistency with the base. I'd\nexpect, from my own behaviour, interactive adding is used by far more\nthan interactive cleaning, which might be an argument to adapt the latter.\n\nBut now that we're discussing this, I don't really see a benefit from\nthe user perspective, it's more code cleanup.\n"},{"id":"278895","messageId":"xmqqa8ms7eap.fsf@gitster.mtv.corp.google.com","threadId":"41462","inReplyTo":"56CA62D3.7060808@moritzneeb.de","subject":"Re: [PATCH v2 5/6] remote: read $GIT_DIR/branches/* with strbuf_getline()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-22T19:09:18Z","receivedAt":"2016-02-22T19:09:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Moritz Neeb writes:\n\n> The line read from the branch file is directly trimmed after reading with\n> strbuf_trim(). There is thus no logic expecting CR, so strbuf_getline_lf()\n> can be replaced by its CRLF counterpart.\n>\n> Signed-off-by: Moritz Neeb \n> ---\n> To be honest, I did not yet fully understand the purpose of this branches/ file.\n> What I'd expect is that it is some intermediary file while fetching?\n> Or is it edited directly by the user and thus it's necessary to strip spaces\n> that could be added accidentally?\n\n[Documentation/gitrepository-layout.txt]\n\nbranches::\n\tA slightly deprecated way to store shorthands to be used\n\tto specify a URL to 'git fetch', 'git pull' and 'git push'.\n\tA file can be stored as `branches/` and then\n\t'name' can be given to these commands in place of\n\t'repository' argument. See the REMOTES section in\n\tlinkgit:git-fetch[1] for details. This mechanism is legacy\n\tand not likely to be found in modern repositories. This\n\tdirectory is ignored if $GIT_COMMON_DIR is set and\n\t\"$GIT_COMMON_DIR/branches\" will be used instead.\n"},{"id":"278897","messageId":"xmqq60xg7dfu.fsf@gitster.mtv.corp.google.com","threadId":"41462","inReplyTo":"CAPig+cReRiwHBJiatWJ=Gc+k+dtcMhdwFn4K57yHAjE3d_fzwQ@mail.gmail.com","subject":"Re: [PATCH v2 4/6] notes: read copied notes with strbuf_getline()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-22T19:27:49Z","receivedAt":"2016-02-22T19:27:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> A deeper issue not touched upon by the commit message (but which\n> should be) is that that strbuf_split() leaves the \"terminator\" (space,\n> in this case) on the component strings, and that is why split[0] must\n> be rtrim'd. Rather than dropping only one of the rtrim's, a cleaner\n> approach might be to convert the code to use string_list_split() which\n> doesn't have the \"odd\" behavior of leaving the terminator on the split\n> strings, in which case both rtrim's could be retired.\n> This, of course,\n> would be done as a separate preparatory patch.\n\nYeah, this is a good point to raise.\n\nThanks.\n"},{"id":"278898","messageId":"xmqqy4ac5yq5.fsf@gitster.mtv.corp.google.com","threadId":"41462","inReplyTo":"56CA61B2.2020904@moritzneeb.de","subject":"Re: [PATCH v2 6/6] wt-status: read rebase todolist with strbuf_getline()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-22T19:30:58Z","receivedAt":"2016-02-22T19:30:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Moritz Neeb writes:\n\n> diff --git a/wt-status.c b/wt-status.c\n> index 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\nNot related to the substance of the patch series at all, but all\nexcept for this patch in the series seem to be corrupt in that the\nvery first line that is removed in each patch has an extra space\nbefore the '-' deletion sign. It is a very curious symptom. Please\ndouble check the way you send out patch e-mails (e.g. send them\nfirst only to yourself and then try to apply them with \"git am\").\n\nThanks.\n"},{"id":"278899","messageId":"xmqqtwl05y9m.fsf@gitster.mtv.corp.google.com","threadId":"41462","inReplyTo":"CAPig+cSi-4R-a=HVmpCWAZ3kr=yQtJ9GdT-JZ4hJ2kmqg-edVA@mail.gmail.com","subject":"Re: [PATCH v2 3/6] clean: read user input with strbuf_getline()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-22T19:40:53Z","receivedAt":"2016-02-22T19:40:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Sun, Feb 21, 2016 at 8:20 PM, 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>> Before the user input was trimmed to remove the CR. This would be now\n>> redundant. Another effect of the trimming was that some (accidentally)\n>> typed spaces were filtered. But here we want to be consistent with similar UIs\n>> like interactive adding, which only accepts space-less input.\n>\n> I don't at all insist upon it, but this behavior change feels somewhat\n> like it ought to be in its own commit. I'm also not convinced that\n> making this consistent with the less forgiving behavior of\n> \"interactive adding\" is desirable (rather the reverse: that that case\n> should be more flexible). However, I wasn't following the discussion\n> with Junio closely, and perhaps missed you two agreeing that this is\n> preferable.\n\nThere was no such discussion ;-)\n\nI am not 100% sure if we want to be lenient in reading \"yes, please\nremove this one\", but if we already are loose, I tend to agree that\nthere is not much point tightening it, especially with a clean-up\ntopic like this one.\n\nThanks.\n"}]}