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