Re: [PATCH 5/5] wt-status: read rebase todolist with strbuf_getline()

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 1, 2016, 21:39 UTC
Message-ID: <xmqq8u34un3d.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <56ACFB7B.6050800@moritzneeb.de>

Moritz Neeb <lists@moritzneeb.de> writes:

Show 20 quoted lines

>     In read_rebase_todolist() every line is, after reading, checked
>     for a comment_line_char. After that it is trimmed via strbuf_trim().
>     Assuming we do never expect a CR as comment_line_char,
>     strbuf_getline_lf() can be safely replaced by its CRLF counterpart.
>
> Signed-off-by: Moritz Neeb <lists@moritzneeb.de>
> ---
>
> Notes:
>     Looking at the code in read_rebase_todolist(), the
>     lines are read, checked for a comment_line_char and then trimmed. I
>     wonder why the input is not trimmed before checking for this character?
>     Is it safe to replace strbuf_getline_lf() by strbuf_getline() anyway?
>         The only case I can imagine that could lead to unexpected behaviour then
>     would be when someone sets the comment_line_char to CR. How likely is that?
>         Why is the trim _after_ checking for the comment char anyway? Should
>     something like "   # foobar" not be considered as comment?
>         I decided to roll out the patch because I considered it not as a risk to be
>     taken seriously, that the comment line char will be '\r'.
>         Meta-question: Should something of this discussion be put into the commit?

Yes to the meta question. Add something like this as the second paragraph of the log message, but do *not* change the patch text.

	The current code checks if the line begins with a comment
	line char (typically '#') before trimming, which is
	consistent with the way how commands like 'git commit'
	prepares commented lines in that this does not treat a line
	that begins with whitespaces and '#' as commented out.  We
	however made a mistake in the parser of "git rebase -i" long
	time ago to allow such a line to be treated as a comment,
	and made an exception with 1db168ee (rebase-i: loosen
	over-eager check_bad_cmd check, 2015-10-01).  It probably is
	a good idea to match that exception by swapping the order of
	comment check and trimming in this codepath as well.

Show 17 quoted lines

>
>  wt-status.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index ab4f80d..f053b2f 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1076,7 +1076,7 @@ static void read_rebase_todolist(const char *fname, struct string_list *lines)
>  	if (!f)
>  		die_errno("Could not open file %s for reading",
>  			  git_path("%s", fname));
> -	while (!strbuf_getline_lf(&line, f)) {
> +	while (!strbuf_getline(&line, f)) {
>  		if (line.len && line.buf[0] == comment_line_char)
>  			continue;
>  		strbuf_trim(&line);