Re: [PATCH 3/5] rebase -i: add exec commands via the rebase--helper

[Junio C Hamano <gitster@pobox.com>]

Liam Beguin <liambeguin@gmail.com> writes:

> diff --git a/sequencer.c b/sequencer.c
> index fa94ed652d2c..810b7850748e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2492,6 +2492,52 @@ int sequencer_make_script(int keep_empty, FILE *out,
>  	return 0;
>  }
>  
> +int add_exec_commands(const char *command)
> +{

As the name of a public function, it does not feel that this hints
it strongly enough that it is from and a part of sequencer.c API.

> +	const char *todo_file = rebase_path_todo();
> +	struct todo_list todo_list = TODO_LIST_INIT;
> +	int fd, res, i, first = 1;
> +	FILE *out;

Having had to scan backwards while trying to see what the loop that
uses this variable is doing and if it gets affected by things that
happened before we entered the loop, I'd rather not to see 'first'
initialized here, left unused for quite some time until the loop is
entered.  It would make it a lot easier to follow if it is declared
and left uninitilized here, and set to 1 immediately before the
for() loop that uses it.

> +
> +	strbuf_reset(&todo_list.buf);
> +	fd = open(todo_file, O_RDONLY);
> +	if (fd < 0)
> +		return error_errno(_("could not open '%s'"), todo_file);
> +	if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
> +		close(fd);
> +		return error(_("could not read '%s'."), todo_file);
> +	}
> +	close(fd);

Is this strbuf_read_file() written in longhand?

> +	res = parse_insn_buffer(todo_list.buf.buf, &todo_list);
> +	if (res) {
> +		todo_list_release(&todo_list);
> +		return error(_("unusable todo list: '%s'"), todo_file);
> +	}
> +
> +	out = fopen(todo_file, "w");
> +	if (!out) {
> +		todo_list_release(&todo_list);
> +		return error(_("unable to open '%s' for writing"), todo_file);
> +	}
> +	for (i = 0; i < todo_list.nr; i++) {
> +		struct todo_item *item = todo_list.items + i;
> +		int bol = item->offset_in_buf;
> +		const char *p = todo_list.buf.buf + bol;
> +		int eol = i + 1 < todo_list.nr ?
> +			todo_list.items[i + 1].offset_in_buf :
> +			todo_list.buf.len;

Should bol and eol be of type size_t instead?  The values that get
assigned to them from other structures are.