Re: [PATCH 3/5] rebase -i: add exec commands via the rebase--helper
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Nov 27, 2017, 21:41 UTC
- Message-ID
- <alpine.DEB.2.21.1.1711272230490.6482@virtualbox>
- In-Reply-To
- <xmqq609we20v.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 27 Nov 2017, Junio C Hamano wrote:
Show 15 quoted lines
> 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.
Show 12 quoted lines
> > + 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.
Show 11 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?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.
Show 21 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.While it won't matter in practice, this would be "more correct" to do, yes.
Ciao, Dscho