{"thread":{"id":"41291","subject":"[PATCH 0/5] Replacing strbuf_getline_lf() by strbuf_getline() on trimmed input","startedAt":"2016-01-30T17:51:39Z","lastAt":"2016-02-22T00:07:34Z","messageCount":14,"participants":["Moritz Neeb","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"277089","messageId":"56ACF82B.2030005@moritzneeb.de","threadId":"41291","inReplyTo":null,"subject":"[PATCH 0/5] Replacing strbuf_getline_lf() by strbuf_getline() on trimmed input","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-01-30T17:51:39Z","receivedAt":"2016-01-30T17:51:39Z","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 trimmed before doing anything that\ncould possibly expect a CR character. As the codepath after trimming can not\nin any way expect a CR at the end of the line, because trim dropped it before\nthe change, this can be safely done.\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\"'. \nApart from a general review, I would be happy about comments how I can improve\nthis (and future) patches, as this is my first one. Especially I am not sure\nabout how many arguments from the discussion on the mailing list should be put\ninto the commits.\n\n-Moritz\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/284104\n\n\nMoritz Neeb (5):\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        | 2 +-\n builtin/clean.c | 6 +++---\n builtin/notes.c | 2 +-\n remote.c        | 2 +-\n wt-status.c     | 2 +-\n 5 files changed, 7 insertions(+), 7 deletions(-)\n\n-- \n2.4.3\n"},{"id":"277091","messageId":"56ACFB03.3020000@moritzneeb.de","threadId":"41291","inReplyTo":"56ACF82B.2030005@moritzneeb.de","subject":"[PATCH 1/5] bisect: read bisect paths with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-01-30T18:03:47Z","receivedAt":"2016-01-30T18:03:47Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"    The lines read from BISECT_NAMES are trimmed with strbuf_trim()\n    immediately. There is thus no logic expecting CR, so\n    strbuf_getline_lf() can be replaced by its CRLF counterpart.\n\nSigned-off-by: Moritz Neeb <lists@moritzneeb.de>\n---\n bisect.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 06ec54e..bf7c885 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -440,7 +440,7 @@ 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+\twhile (strbuf_getline(&str, fp) != EOF) {\n \t\tstrbuf_trim(&str);\n \t\tif (sq_dequote_to_argv_array(str.buf, array))\n \t\t\tdie(\"Badly quoted content in file '%s': %s\",\n-- \n2.4.3\n"},{"id":"277093","messageId":"56ACFB3D.6010309@moritzneeb.de","threadId":"41291","inReplyTo":"56ACF82B.2030005@moritzneeb.de","subject":"[PATCH 2/5] clean: read user input with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-01-30T18:04:45Z","receivedAt":"2016-01-30T18:04:45Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"    The input that is read is trimmed with strbuf_trim() immediately.\n    There is thus no logic expecting CR, so strbuf_getline_lf() can be\n    replaced by its CRLF counterpart.\n\nSigned-off-by: Moritz Neeb <lists@moritzneeb.de>\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 -\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":"277090","messageId":"56ACFB53.7070400@moritzneeb.de","threadId":"41291","inReplyTo":"56ACF82B.2030005@moritzneeb.de","subject":"[PATCH 3/5] notes: read copied notes with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-01-30T18:05:07Z","receivedAt":"2016-01-30T18:05:07Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"    The notes that are copied from stdin are trimmed with strbuf_rtrim()\n    after splitting by ' '. There is thus no logic expecting CR, so\n    strbuf_getline_lf() can be replaced by its CRLF counterpart.\n\nSigned-off-by: Moritz Neeb <lists@moritzneeb.de>\n---\n builtin/notes.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex ed6f222..bbd7544 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-- \n2.4.3\n"},{"id":"277094","messageId":"56ACFB6D.4040804@moritzneeb.de","threadId":"41291","inReplyTo":"56ACF82B.2030005@moritzneeb.de","subject":"[PATCH 4/5] remote: read $GIT_DIR/branches/* with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-01-30T18:05:33Z","receivedAt":"2016-01-30T18:05:33Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"    The line read from the branch file is directly trimmed after\n    reading with strbuf_trim(). There is thus no logic expecting CR,\n    so strbuf_getline_lf() can be replaced by its CRLF counterpart.\n\nSigned-off-by: Moritz Neeb <lists@moritzneeb.de>\n---\n remote.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/remote.c b/remote.c\nindex 7d61dab..78f97f0 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.4.3\n"},{"id":"277092","messageId":"56ACFB7B.6050800@moritzneeb.de","threadId":"41291","inReplyTo":"56ACF82B.2030005@moritzneeb.de","subject":"[PATCH 5/5] wt-status: read rebase todolist with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-01-30T18:05:47Z","receivedAt":"2016-01-30T18:05:47Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"    In read_rebase_todolist() every line is, after reading, checked\n    for a comment_line_char. After that it is trimmed via strbuf_trim().\n    Assuming we do never expect a CR as comment_line_char,\n    strbuf_getline_lf() can be safely replaced by its CRLF counterpart.\n\nSigned-off-by: Moritz Neeb <lists@moritzneeb.de>\n---\n\nNotes:\n    Looking at the code in read_rebase_todolist(), the\n    lines are read, checked for a comment_line_char and then trimmed. I\n    wonder why the input is not trimmed before checking for this character?\n    Is it safe to replace strbuf_getline_lf() by strbuf_getline() anyway?\n        The only case I can imagine that could lead to unexpected behaviour then\n    would be when someone sets the comment_line_char to CR. How likely is that?\n        Why is the trim _after_ checking for the comment char anyway? Should\n    something like \"   # foobar\" not be considered as comment?\n        I decided to roll out the patch because I considered it not as a risk to be\n    taken seriously, that the comment line char will be '\\r'.\n        Meta-question: Should something of this discussion be put into the commit?\n\n wt-status.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex ab4f80d..f053b2f 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1076,7 +1076,7 @@ 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-- \n2.4.3\n"},{"id":"277184","messageId":"xmqqlh74unja.fsf@gitster.mtv.corp.google.com","threadId":"41291","inReplyTo":"56ACFB03.3020000@moritzneeb.de","subject":"Re: [PATCH 1/5] bisect: read bisect paths with strbuf_getline()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-01T21:30:01Z","receivedAt":"2016-02-01T21:30:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Moritz Neeb <lists@moritzneeb.de> writes:\n\n>     The lines read from BISECT_NAMES are trimmed with strbuf_trim()\n>     immediately. There is thus no logic expecting CR, so\n>     strbuf_getline_lf() can be replaced by its CRLF counterpart.\n\nWe do not indent the whole log message.\n\nYou would also want to think about the necessity of strbuf_trim()\nhere.  Now strbuf_getline() would trim the trailing CR, would we\nstill need to call strbuf_trim() here?  The code will break if you\njust remove the call, but on the other hand, you will realize that\nthe trimming done by calling it is excessive and unnecessary, once\nyou inspect the code and learn who writes the file being read here\nand how.\n\n> Signed-off-by: Moritz Neeb <lists@moritzneeb.de>\n> ---\n>  bisect.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/bisect.c b/bisect.c\n> index 06ec54e..bf7c885 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -440,7 +440,7 @@ 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> +\twhile (strbuf_getline(&str, fp) != EOF) {\n>  \t\tstrbuf_trim(&str);\n>  \t\tif (sq_dequote_to_argv_array(str.buf, array))\n>  \t\t\tdie(\"Badly quoted content in file '%s': %s\",\n"},{"id":"277185","messageId":"xmqqh9hsunhw.fsf@gitster.mtv.corp.google.com","threadId":"41291","inReplyTo":"56ACFB3D.6010309@moritzneeb.de","subject":"Re: [PATCH 2/5] clean: read user input with strbuf_getline()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-01T21:30:51Z","receivedAt":"2016-02-01T21:30:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The same comment (including \"think if this trim is still necessary\")\nas 1/5 applies here.\n"},{"id":"277186","messageId":"xmqqd1sgunci.fsf@gitster.mtv.corp.google.com","threadId":"41291","inReplyTo":"56ACFB53.7070400@moritzneeb.de","subject":"Re: [PATCH 3/5] notes: read copied notes with strbuf_getline()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-01T21:34:05Z","receivedAt":"2016-02-01T21:34:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Same comment as 1/5 and 2/5 applies.  You need to think if\nstrbuf_rtim(split[1]) is necessary here.\n"},{"id":"277187","messageId":"xmqq8u34un3d.fsf@gitster.mtv.corp.google.com","threadId":"41291","inReplyTo":"56ACFB7B.6050800@moritzneeb.de","subject":"Re: [PATCH 5/5] wt-status: read rebase todolist with strbuf_getline()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-01T21:39:34Z","receivedAt":"2016-02-01T21:39:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Moritz Neeb <lists@moritzneeb.de> writes:\n\n>     In read_rebase_todolist() every line is, after reading, checked\n>     for a comment_line_char. After that it is trimmed via strbuf_trim().\n>     Assuming we do never expect a CR as comment_line_char,\n>     strbuf_getline_lf() can be safely replaced by its CRLF counterpart.\n>\n> Signed-off-by: Moritz Neeb <lists@moritzneeb.de>\n> ---\n>\n> Notes:\n>     Looking at the code in read_rebase_todolist(), the\n>     lines are read, checked for a comment_line_char and then trimmed. I\n>     wonder why the input is not trimmed before checking for this character?\n>     Is it safe to replace strbuf_getline_lf() by strbuf_getline() anyway?\n>         The only case I can imagine that could lead to unexpected behaviour then\n>     would be when someone sets the comment_line_char to CR. How likely is that?\n>         Why is the trim _after_ checking for the comment char anyway? Should\n>     something like \"   # foobar\" not be considered as comment?\n>         I decided to roll out the patch because I considered it not as a risk to be\n>     taken seriously, that the comment line char will be '\\r'.\n>         Meta-question: Should something of this discussion be put into the commit?\n\nYes to the meta question.  Add something like this as the second\nparagraph of the log message, but do *not* change the patch text.\n\n\tThe current code checks if the line begins with a comment\n\tline char (typically '#') before trimming, which is\n\tconsistent with the way how commands like 'git commit'\n\tprepares commented lines in that this does not treat a line\n\tthat begins with whitespaces and '#' as commented out.  We\n\thowever made a mistake in the parser of \"git rebase -i\" long\n\ttime ago to allow such a line to be treated as a comment,\n\tand made an exception with 1db168ee (rebase-i: loosen\n\tover-eager check_bad_cmd check, 2015-10-01).  It probably is\n\ta good idea to match that exception by swapping the order of\n\tcomment check and trimming in this codepath as well.\n\n>\n>  wt-status.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/wt-status.c b/wt-status.c\n> index ab4f80d..f053b2f 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -1076,7 +1076,7 @@ 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"},{"id":"278120","messageId":"56C0EB3F.90202@moritzneeb.de","threadId":"41291","inReplyTo":"xmqqlh74unja.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/5] bisect: read bisect paths with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-14T21:01:51Z","receivedAt":"2016-02-14T21:01:51Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"On 02/01/2016 10:30 PM, Junio C Hamano wrote:\n> Moritz Neeb <lists@moritzneeb.de> writes:\n> \n>>     The lines read from BISECT_NAMES are trimmed with strbuf_trim()\n>>     immediately. There is thus no logic expecting CR, so\n>>     strbuf_getline_lf() can be replaced by its CRLF counterpart.\n> \n> We do not indent the whole log message.\n\nThanks for spotting, I will change that in the next iteration.\n \n> You would also want to think about the necessity of strbuf_trim()\n> here.  Now strbuf_getline() would trim the trailing CR, would we\n> still need to call strbuf_trim() here?  The code will break if you\n> just remove the call, but on the other hand, you will realize that\n> the trimming done by calling it is excessive and unnecessary, once\n> you inspect the code and learn who writes the file being read here\n> and how.\n\nI am not sure what you mean by excessive: How much can I assume that\nthe input is like expected? The files we are talking about are supposed\nto be read and written by git only. But could be modified in theory with\nan editor, right? Then things could break, right? This question maybe holds\ntrue for the other patches as well, I still have to look into them.\n\nAssuming that things are just like expected, what has to be done in\nread_bisect_paths before dequoting is to remove the leading space,\nwhich is added by 'git rev-parse --sq-quote \"$@\"' in git-bisect.sh.\n\nI am just not sure, where to remove it:\n* in sq_quote_argv? - see some analysis below.\n* in read_bisect_paths via strbuf_ltrim instead of strbuf_trim,\n  is that what you meant?\n* in sq_dequote_argv_array? - see below as well.\n\nI tried to understand, why the space is added by sq_quote_argv() in quotes.c\nin the first place. It is there since the beginning in 7cf6720. I have the\nfeeling that the space is redundant for bisect, because the only other codepath\nwhere BISECT_NAMES is read is in bisect_visualize(), where the necessary space\nto separate the arguments is added before '$(...)':\n\neval '\"$@\"' --bisect -- $(cat \"$GIT_DIR/BISECT_NAMES\")\n\nOther places where sq_quote_argv() and sq_dequote_to_argv_array() are called\nas well are builtin/am.c and trace.c.\n\nIn builtin/am.c when reading 'apply-opt', the data is trimmed in read_state_file,\nso the space in the beginning is superfluous.\n\nLooking at trace.c it gets clear that this code is dependent on this space,\notherwise the trace output will look like \"trace: built-in: git'status'\"\nwhich is obviously bogus. It would maybe make more sense to add this space\ndirectly there than to trim it in all the other places. I realized that this\ncode came up in 7cf6720 as well which explains why things are like they are\n(and why I'm CC'ing Christian Couder).\n\nAnother option, at the moment my favourite because I think it's the least invasive,\nwould be to \"trim\" this one space directly in sq_dequote_step(). See the patch below.\nThis would not affect the tracing functionality, because there is no dequoting used.\n\n-- >8 --\nSubject: [PATCH] quote: remove leading space in sq_dequote_step\n\nBecause 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\nSigned-off-by: Moritz Neeb <lists@moritzneeb.de>\n---\nquote.c | 2 ++\n1 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)\nchar *src = arg;\nchar c;\n\n+ if (*src == ' ')\n+ src++;\nif (*src != '\\'')\nreturn NULL;\nfor (;;) {\n-- \n2.4.3\n"},{"id":"278129","messageId":"xmqqtwlar2bd.fsf@gitster.mtv.corp.google.com","threadId":"41291","inReplyTo":"56C0EB3F.90202@moritzneeb.de","subject":"Re: [PATCH 1/5] bisect: read bisect paths with strbuf_getline()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-15T05:05:10Z","receivedAt":"2016-02-15T05:05:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Moritz Neeb <lists@moritzneeb.de> writes:\n\n>> You would also want to think about the necessity of strbuf_trim()\n>> here.  Now strbuf_getline() would trim the trailing CR, would we\n>> still need to call strbuf_trim() here?  The code will break if you\n>> just remove the call, but on the other hand, you will realize that\n>> the trimming done by calling it is excessive and unnecessary, once\n>> you inspect the code and learn who writes the file being read here\n>> and how.\n>\n> I am not sure what you mean by excessive: How much can I assume that\n> the input is like expected? The files we are talking about are supposed\n> to be read and written by git only. But could be modified in theory with\n> an editor, right? Then things could break, right? This question maybe holds\n> true for the other patches as well, I still have to look into them.\n\nThese are all good questions you as a Git contributor to be asking\nyourself, and I really like the fact that you are thinking aloud\nhere.\n\nAs to bisect pathspec, while I do not think it is sensible to change\nit in the middle of bisection, I do not think it would cause the\nbisect command to produce an incorrect result if you did so.  You\nwould be changing the definition of what is the \"middle point\" of\nthe history on the fly by changing the pathspec, hence you may end\nup making the bisection suboptimal, but it shouldn't affect the\ncorrectness of the bisection.\n\nSo I tend to agree with you that it is a good idea to be prepared to\nparse what the end user may have futzed with the editor, as long as\nthe user did not break the syntax of the quoted string.\n\nYour answer to the question quoted at the top of this message could\nbe a summary of the above reasoning, with a conclusion: \"we would\nwant to tolerate trailing (or leading) whitespaces the user might\nadd with his editor; even though we use strbuf_getline() to remove\nthe CR at the end of line, we still need to strbuf_trim() the line\nwe read\".\n\n\tSide note: of course, you can instead say \"while it is\n\ttechnically possible, we have never advertised this file to\n\tbe editable by user, or encouraged them to do so\" and\n\ttighten the parsing to assume only what we write out,\n\terroring out when we see any attempt to edit it.\n\nEither way, the most important thing is to record the reasoning\nbehind the design choice you made clearly in the proposed commit log\nmessage.\n\nThanks.\n"},{"id":"278812","messageId":"56CA4CE6.9070802@moritzneeb.de","threadId":"41291","inReplyTo":"xmqqtwlar2bd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/5] bisect: read bisect paths with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-21T23:48:54Z","receivedAt":"2016-02-21T23:48:54Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"On 02/15/2016 06:05 AM, Junio C Hamano wrote:\n> Moritz Neeb <lists@moritzneeb.de> writes:\n> \n>>> You would also want to think about the necessity of strbuf_trim()\n>>> here.  Now strbuf_getline() would trim the trailing CR, would we\n>>> still need to call strbuf_trim() here?  The code will break if you\n>>> just remove the call, but on the other hand, you will realize that\n>>> the trimming done by calling it is excessive and unnecessary, once\n>>> you inspect the code and learn who writes the file being read here\n>>> and how.\n>>\n>> I am not sure what you mean by excessive: How much can I assume that\n>> the input is like expected? The files we are talking about are supposed\n>> to be read and written by git only. But could be modified in theory with\n>> an editor, right? Then things could break, right? This question maybe holds\n>> true for the other patches as well, I still have to look into them.\n> \n> These are all good questions you as a Git contributor to be asking\n> yourself, and I really like the fact that you are thinking aloud\n> here.\n\nThanks for this feedback I will try to continue this thinking aloud\nwhenever appropriate.\n\nSorry by the way for only answering every week - I currently only have\ntime to work on git during the weekend. What is (assuming familiarity\nwith the codebase) the expected \"cooking\"-time of a smaller patch? I'm\nsure the answer is \"depends\", but maybe you can give away some experiences.\n"},{"id":"278815","messageId":"56CA5146.7050008@moritzneeb.de","threadId":"41291","inReplyTo":"xmqqtwlar2bd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/5] bisect: read bisect paths with strbuf_getline()","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-22T00:07:34Z","receivedAt":"2016-02-22T00:07:34Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"On 02/15/2016 06:05 AM, Junio C Hamano wrote:\n> As to bisect pathspec, while I do not think it is sensible to change\n> it in the middle of bisection, I do not think it would cause the\n> bisect command to produce an incorrect result if you did so.  You\n> would be changing the definition of what is the \"middle point\" of\n> the history on the fly by changing the pathspec, hence you may end\n> up making the bisection suboptimal, but it shouldn't affect the\n> correctness of the bisection.\n\nYep I also think it would not affect the correctness. To reformulate\nyour explanation: changing the pathspec does not change the good and bad\n\"frontiers\". And those should be still correct under the assumption\nyou're still looking for the same bug/regression.\n\nAs to the question if it is sensible, I also don't really think it could\nbe helpful. The only way I would expect it to be useful, it so \"refine\"\nthe search by adding specific files to the filepath. But then it's\npossible to end up in a dead-end if the commits left are not touching\nthe specified files with:\n\n\tNo testable commit found.\n\tMaybe you started with bad path parameters?\n\nI have also seen other messed up messages, like \"-1 commits left\" while\nplaying around.\n\n> So I tend to agree with you that it is a good idea to be prepared to\n> parse what the end user may have futzed with the editor, as long as\n> the user did not break the syntax of the quoted string.\n\nNow I am not convinced anymore, because I think if it potentially leads\nto troubles, it should not be made easier to get oneself into them.\n\n> \tSide note: of course, you can instead say \"while it is\n> \ttechnically possible, we have never advertised this file to\n> \tbe editable by user, or encouraged them to do so\" and\n> \ttighten the parsing to assume only what we write out,\n> \terroring out when we see any attempt to edit it.\n\nI will include something like that in the patch v2 that is to follow soon.\n"}]}