Le 09/08/2018 à 16:22, Phillip Wood a écrit :
>> +int complete_action(struct replay_opts *opts, unsigned flags,
>> + const char *shortrevisions, const char *onto_name,
>> + const char *onto, const char *orig_head, const char *cmd,
>> + unsigned autosquash)
>> +{
>> + const char *shortonto, *todo_file = rebase_path_todo();
>> + struct todo_list todo_list = TODO_LIST_INIT;
>> + struct strbuf *buf = &(todo_list.buf);
>> + struct object_id oid;
>> + struct stat st;
>> +
>> + get_oid(onto, &oid);
>
> Is onto guaranteed to exist? If not the return value of get_oid() needs
> to be checked. Otherwise a comment or an assertion might be useful here.
>
Yes, either the user defines it manually (with --onto=), or it is
automatically determinated by git-rebase.sh.
>> + shortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV);
>> +
>> + if (!lstat(todo_file, &st) && st.st_size == 0 &&
>> + write_message("noop\n", 5, todo_file, 0))
>> + return error_errno(_("could not write '%s'"), todo_file);
>> +
>> + if (autosquash && rearrange_squash())
>> + return 1;
>
> git functions normally return -1 for an error as the 'return error(...)'
> does above, is there a reason for a different return value here?
>
No, I will fix this.
>> +
>> + if (cmd && *cmd)
>> + sequencer_add_exec_commands(cmd); > +
>> + if (strbuf_read_file(buf, todo_file, 0) < 0)
>> + return error_errno(_("could not read '%s'."), todo_file);
>> +
>> + if (parse_insn_buffer(buf->buf, &todo_list)) {
>
> I was worried when I saw this because buf is an alias of todo_list.buf
> and I thought passing the same buffer in twice would end badly. However
> parse_insn_buffer() has a confusing signature, it expects the caller to
> have filled todo_list.buf with the buffer to be parsed and to pass a
> pointer to the same buffer. I think this should be cleaned up at some
> point but it is outside the scope of this series.
>
>> + todo_list_release(&todo_list);
>> + return error(_("unusable todo list: '%s'"), todo_file);
>> + }
>> +
>> + if (count_commands(&todo_list) == 0) {
>> + apply_autostash(opts);
>> + sequencer_remove_state(opts);
>> + todo_list_release(&todo_list);
>> +
>> + fputs("Nothing to do\n", stderr);
>
> The capital 'N' is a faithful copy of the current message in rebase.sh.
> However it might be worth changing it to 'n' to match the normal style
> of git error messages starting with a lowercase letter.
>
>> + return 1;
>
> It might be better to do 'return error(...)' instead
>
This will require a test change – not a big deal, of course. It’s
perhaps a good idea to mark this string for translation, too.
>> + }
>> +
>> + strbuf_addch(buf, '\n');
>> + strbuf_commented_addf(buf, Q_("Rebase %s onto %s (%d command)",
>> + "Rebase %s onto %s (%d commands)",
>> + count_commands(&todo_list)),
>> + shortrevisions, shortonto,
>> count_commands(&todo_list));
>> + append_todo_help(0, flags & TODO_LIST_KEEP_EMPTY, buf);
>> +
>> + if (write_message(buf->buf, buf->len, todo_file, 0)) {
>> + todo_list_release(&todo_list);
>> + return error_errno(_("could not write '%s'"), todo_file);
>> + }
>> +
>> + copy_file(rebase_path_todo_backup(), todo_file, 0666);
>> + transform_todos(flags | TODO_LIST_SHORTEN_IDS);
>
> Both of the above lines can fail, so the return values need checking I
> think.
>
>> +
>> + strbuf_reset(buf);
>> +
>> + if (launch_sequence_editor(todo_file, buf, NULL)) {
>> + apply_autostash(opts);
>> + sequencer_remove_state(opts);
>> + todo_list_release(&todo_list);
>> +
>> + return error(_("could not execute editor"));
>
> I think launch_sequence_editor() will have printed an error message
> already, so this is unnecessary.
>
And the error would be more specific, true.
Thanks!
Alban
