From: Junio C Hamano Date: Mon, 04 Jan 2016 19:50:33 GMT Subject: Re: [PATCH v2 03/17] update-index: read --index-info with strbuf_getline_crlf() Message-ID: In-Reply-To: Johannes Schindelin writes: >> diff --git a/builtin/update-index.c b/builtin/update-index.c >> index 7431938..a7a9a7e 100644 >> --- a/builtin/update-index.c >> +++ b/builtin/update-index.c >> @@ -473,7 +473,9 @@ static void read_index_info(int line_termination) >> struct strbuf buf = STRBUF_INIT; >> struct strbuf uq = STRBUF_INIT; >> >> - while (strbuf_getline(&buf, stdin, line_termination) != EOF) { >> + while ((line_termination >> + ? strbuf_getline_crlf(&buf, stdin) >> + : strbuf_getline(&buf, stdin, '\0')) != EOF) { >> char *ptr, *tab; > > This is a problematic change because it does not safeguard for future > introduction of a line_termination value other than LF or NUL. I believe > the safest would be to change read_index_info() to take a `nul_delimited` > parameter instead of the `line_termination` parameter first, and then > introduce that change to use strbuf_getline_crlf if !nul_delimited. When I introduced line_termination long time ago, I wrote these codepaths anticipating that there might be a value other than NUL and LF that could be useful, but no useful caller that uses other useful value has emerged, so I agree that the interface was too broad and general for its own good. I agree with your comments for all the changes in this series that switch between getline and getline_delim('\0') based on the value of line_termination; its generality is useless and we should declare "the line termination is either NUL or LF, no other choices"; using nul_delimited = 0/1 is a very good way to express it clearly. Thanks.