From: Alban Gruin <alban.gruin@gmail.com>
Date: Thu, 09 Aug 2018 17:00:27 GMT
Subject: Re: [GSoC][PATCH v5 11/20] rebase -i: rewrite complete_action() in C
Message-ID: <df96eadb-8c97-2c21-3ab9-6ffbaa61160c@gmail.com>
In-Reply-To: <f2698204-c376-0b0c-b151-22b45d482b7c@talktalk.net>

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