Hi Liam,
could I ask for a favor? I'd like the oneline to start with
rebase -i -x: ...
(this would help future me to realize what this commit touches already
from the concise graph output I favor).
On Sun, 26 Nov 2017, Liam Beguin wrote:
> Recent work on `git-rebase--interactive` aim to convert shell code to C.
> Even if this is most likely not a big performance enhacement, let's
> convert it too since a comming change to abbreviate command names requires
> it to be updated.
Since Junio did not comment on the commit message: could you replace
`aim` by `aims`, `enhacement` by `enhancement` and `comming` by `coming`?
> @@ -36,6 +37,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
> N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
> OPT_CMDMODE(0, "rearrange-squash", &command,
> N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
> + OPT_CMDMODE(0, "add-exec", &command,
> + N_("insert exec commands in todo list"), ADD_EXEC),
Maybe `add-exec-commands`? I know it is longer to type, but these options do
not need to be typed interactively and the longer name would be consistent
with the function name.
> 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;
> }
>
As the code in add_exec_commands() may appear convoluted (why not simply
append the command after any pick?), the original comment would be really
nice here:
/*
* Add commands after pick and (series of) squash/fixup commands
* in the todo list.
*/
> +int add_exec_commands(const char *command)
> +{
> + const char *todo_file = rebase_path_todo();
> + struct todo_list todo_list = TODO_LIST_INIT;
> + int fd, res, i, first = 1;
> + FILE *out;
> +
> + strbuf_reset(&todo_list.buf);
The todo_list.buf has been initialized already (via TODO_LIST_INIT), no
need to reset it again.
> + 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);
As Junio pointed out so gently: there is a helper function that does this
all very conveniently for us:
if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
return error_errno(_("could not read '%s'"), todo_file);
And as I realized looking at the surrounding code: you probably just
inherited my inelegant code by copy-editing from another function in
sequencer.c. Should you decide to add a preparatory patch to your patch
series that converts these other callers, or even refactors all that code
that reads the git-rebase-todo file and then parses it, I would be quite
happy... :-) (although I would understand if you deemed this outside the
purpose of your patch series).
> + 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);
> + }
The variable `res` is not really used here. Let's just put the
parse_insn_buffer() call inside the if ().
> + 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;
This smells like another copy-edited snippet that originated from my
brain, and I am not at all proud by the complexity I used there.
The function should also check for errors during writing. So how about
something like this instead?
struct strbuf *buf = &todo_list.buf;
size_t offset = 0, command_len = strlen(command);
int first = 1, i;
struct todo_item *item;
...
/* insert <command> before every pick except the first one */
for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++)
if (item->command == TODO_PICK) {
if (first)
first = 0;
else {
strbuf_splice(buf,
item->offset_in_buf + offset, 0,
command, command_len);
offset += command_len;
}
}
/* append a final <command> */
strbuf_complete_list(buf);
strbuf_add(buf, command, command_len);
i = write_message(buf->buf, buf->len, todo_file, 0);
todo_list_release(&todo_list);
return i;
Ciao,
Dscho
