From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Mon, 01 Aug 2011 17:48:14 GMT
Subject: Re: [PATCH 17/18] revert: Introduce --continue to continue the operation
Message-ID: <CALkWK0mrx+3jdCQD9xya3AWMsPpSiZzEz+Z9XVxZNzw3UdKMVw@mail.gmail.com>
In-Reply-To: <201107311751.52965.chriscool@tuxfamily.org>

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