On Tue, Apr 25, 2017 at 03:52:10PM +0200, Johannes Schindelin wrote:
> diff --git a/sequencer.c b/sequencer.c
> index 3a935fa4cbc..bbbc98c9116 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2616,3 +2616,93 @@ int check_todo_list(void)
>
> return res;
> }
> +
> +/* skip picking commits whose parents are unchanged */
> +int skip_unnecessary_picks(void)
Coverity warns of some descriptor leaks in this function (and in
rearrange_squash). I think you get those emails, so I won't repeat the
details here. But I while looking at them I did notice something it
didn't mention:
> + if (i > 0) {
> + int offset = i < todo_list.nr ?
> + todo_list.items[i].offset_in_buf : todo_list.buf.len;
> + const char *done_path = rebase_path_done();
> +
> + fd = open(done_path, O_CREAT | O_WRONLY | O_APPEND, 0666);
> + if (write_in_full(fd, todo_list.buf.buf, offset) < 0) {
> + todo_list_release(&todo_list);
> + return error_errno(_("could not write to '%s'"),
> + done_path);
> + }
> + close(fd);
This should probably check the result of open(). I know write_in_full()
will fail if fd is -1, but we'd rather show the user the errno from
open(), not EBADF.
Technically the free() calls from todo_list_release() can also munge
errno before you print it. You might want to just call error_errno()
first, then do the cleanup (including the missing close()).
> +
> + fd = open(rebase_path_todo(), O_WRONLY, 0666);
> + if (write_in_full(fd, todo_list.buf.buf + offset,
> + todo_list.buf.len - offset) < 0) {
> + todo_list_release(&todo_list);
> + return error_errno(_("could not write to '%s'"),
> + rebase_path_todo());
> + }
Ditto here.
-Peff
