Re: [PATCH 3/5] rebase -i: add exec commands via the rebase--helper
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 27, 2017, 05:14 UTC
- Message-ID
- <xmqq609we20v.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20171127045514.25647-4-liambeguin@gmail.com>
Liam Beguin <liambeguin@gmail.com> writes:
Show 10 quoted lines
> 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.
Show 10 quoted lines
> +
> + 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?
Show 18 quoted lines
> + 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.