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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Mon, 27 Nov 2017, 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.

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