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

[liam Beguin <liambeguin@gmail.com>]

Hi Junio,

On 27/11/17 12:14 AM, Junio C Hamano wrote:
> 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.
> 

I agree that moving 'first = 1' just above the for() loop makes it
more obvious. I'm not quite fond of how this is implemented, I just
'translated' the shell code and was hoping on maybe a few comments
on how to improve 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?

Thanks for pointing this out! I'll update. And as Johannes pointed out,
I've copied this from surrounding functions, I'll add a preparatory path
to update those too.

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

Will do.
Thanks, 

Liam