Re: [PATCH 17/18] revert: Introduce --continue to continue the operation
- From
Ramkumar Ramachandra <artagnon@gmail.com>
- Date
- Aug 1, 2011, 17:48 UTC
- Message-ID
- <CALkWK0mrx+3jdCQD9xya3AWMsPpSiZzEz+Z9XVxZNzw3UdKMVw@mail.gmail.com>
- In-Reply-To
- <201107311751.52965.chriscool@tuxfamily.org>
Hi again,
Christian Couder writes:
Show 16 quoted lines
> 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.
Show 6 quoted lines
>> + 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.
Show 10 quoted lines
>> + 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