Re: [PATCH v2 7/9] rebase -i: skip unnecessary picks using the rebase--helper

From: Jeff King <peff@peff.net>
Date: Apr 26, 2017, 10:55 UTC
Message-ID: <20170426105503.wewlbefrsdmjqnob@sigill.intra.peff.net>
In-Reply-To: <ecab086a62e96388f4dfc7e36a679821fdc7c8c3.1493128210.git.johannes.schindelin@gmx.de>

On Tue, Apr 25, 2017 at 03:52:10PM +0200, Johannes Schindelin wrote:

Show 11 quoted lines

> 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:

Show 12 quoted lines

> +	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()).

Show 8 quoted lines

> +
> +		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