Hi Junio,
On Mon, 27 Nov 2017, Junio C Hamano wrote:
> Liam Beguin <liambeguin@xxxxxxxxx> 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.
How about a "yes, and" instead? As in:
To further improve this patch, let's use the name
sequencer_add_exec_commands() for this function because it is defined
globally now.
> > + 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.
Funny, I would have assumed it the other way round: since "first" always
has to be initialized with 1, I would have been surprised to see an
explicit assignment much later than it is declared.
> > + 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?
Ah, this is one of the downsides of patch-based review. If it was reviewed
in context, you would have easily spotted that Liam was merely
copy-editing my code that is still around.
And indeed, I had missed that function when I started to write the
rebase--helper patches.
> > + 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.
While it won't matter in practice, this would be "more correct" to do,
yes.
Ciao,
Dscho
