Re: [PATCH 5/5] wt-status: read rebase todolist with strbuf_getline()
From: Junio C Hamano <hidden>
Date: 2016-06-15 23:08:03
Moritz Neeb [off-list ref] writes:
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 <redacted>
---
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.
quoted hunk
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);