Hi again,
Christian Couder writes:
> On Thursday 28 July 2011 18:52:30 Ramkumar Ramachandra wrote:
>> +static void read_populate_todo(struct commit_list **todo_list,
>> + struct replay_opts *opts)
>> +{
>> + const char *todo_file = git_path(SEQ_TODO_FILE);
>> + struct strbuf buf = STRBUF_INIT;
>> + struct commit_list **next;
>> + struct commit *commit;
>> + char *p;
>> + int fd;
>> +
>> + fd = open(todo_file, O_RDONLY);
>> + if (fd < 0) {
>> + strbuf_release(&buf);
>
> We don't need to release buf here.
Good catch! Fixed.
>> + die_errno(_("Could not open %s."), todo_file);
>> + }
>> + if (strbuf_read(&buf, fd, 0) < buf.len) {
>
> The other places in the code are using "strbuf_read(...) < 0" to detect an
> error.
Ah, I hadn't noticed. See, strbuf_reset can return in two ways:
1. strbuf.c:313 reads "return -1", and this is clearly an error.
2. strbuf.c:322 reads "return sb->len - oldlen", and I thought I
should catch short reads using this. Then again, malformed
instruction sheets are reported anyway during the parse anyway.
Fixed for consistency with other code.
>> + close(fd);
>> + strbuf_release(&buf);
>> + die(_("Could not read %s."), todo_file);
>> + }
>> + close(fd);
>> +
>> + next = todo_list;
>> + for (p = buf.buf; *p; p = strchr(p, '\n') + 1) {
>
> This relies on a "\n" at the end of the last line...
Yes, that was intentional. Every editor I know of inserts a '\n' at
the end of the last line, and I've seen some applications warn/ break
when the last line is not terminated with '\n'. I suppose another
reason to not change this is consistency -- we don't have to handle
the last line using a special case. So, I personally don't like it,
but I'll add a special case if you insist.
Thanks.
-- Ram
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html