Hi Junio,
Le 11/07/2018 à 00:33, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@xxxxxxxxx> writes:
>> - complete_action
>> + exec git rebase--helper --complete-action "$shortrevisions" "$onto_name" \
>> + "$shortonto" "$orig_head" "$cmd" $allow_empty_message \
>> + ${autosquash:+--autosquash} ${keep_empty:+--keep-empty} \
>> + ${verbose:+--verbose} ${force_rebase:+--no-ff}
>
> The $allow_empty_message and later options are all dashed ones.
> git-rebase.sh gives us either empty or --allow-empty-message in the
> variable for $allow_empty_message, and if you follow suit to prepare
> all the other variables that way, the ugly ${frotz+=--frotz} dance
> will all become unnecessary here.
>
Good idea.
>> +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, unsigned keep_empty)
>> +{
>> + 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);
>> + 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);
>
> Wait a minute... thinking back to the early "age-old ommission"
> topic, can the todo file be a non-empty file that does not have any
> command (e.g. just a single blank line, or full of comments and
> nothing else)? The original wouldn't have added "noop" and then the
> first "do we have anything to do?" check would still have been
> necessary, which would mean that ff74126c's not removing the first
> check was actually not a bug but was a cautious and sensible thing
> to do, and removal of that exact check by this commit becomes a
> regression. So,... can the todo file be a non-empty file that does
> not have any command in it at this point?
>
Hum, yes, if the commits are empty, and if rebase was invoked without
the `--keep-empty` flag. In this case, it would die with the message
“Nothing to do”, instead of dropping the commit altogether.
I will add a test case in the next iteration.
>> + if (autosquash && rearrange_squash())
>> + return 0;
>
> The original, when rearrange-squash failed, exited with failure,
> like so:
>
> - test -z "$autosquash" || git rebase--helper --rearrange-squash || exit
>
> Now this function returns success when rearrange_squash fails.
> Is that intended?
>
Yes, it is. I just saw in the man page that `exit` returns the last
exit status.
>> + if (cmd && *cmd)
>
> Shouldn't it be a BUG (programming error) if cmd == NULL? I thought
> the caller of complete_action() in this patch insisted that it got
> argc == 6 or something, so *cmd might be NUL but cmd would never be
> NULL if I understand your code correctly. IOW, shouldn't the line
> be more like:
>
> if (!cmd)
> BUG("...");
> if (*cmd)
>
> ?
>
I don’t know, it’s not really problematic if cmd is NULL. And I think
that when interactive rebase will be a builtin, it will be possible for
cmd to be NULL.
>
>> + 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)) {
>
> Nice that we have parse* function. We do not have to work with
> stripspace plus "wc -l" ;-).
>
>> + todo_list_release(&todo_list);
>> + return error(_("unusable todo list: '%s'"), todo_file);
>
> When the users see this error message, is it easy for them to
> diagnose what went wrong (e.g. has parse_insn_buffer() already
> issued some message to help pinpoint which insn in the todo list is
> misspelt, for example)?
>
Yes, parse_insn_buffer() will say which line caused the error. But at
this point, the user should not have changed the todo-list, so this
error should never appear.
Perhaps this is a good use case of BUG()?
>> + }
>> +
>> + strbuf_addch(buf, '\n');
>> + strbuf_commented_addf(buf, Q_("Rebase %s onto %s (%d command)",
>> + "Rebase %s onto %s (%d commands)",
>> + todo_list.nr),
>> + shortrevisions, shortonto, todo_list.nr);
>> + append_todo_help(0, 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);
>> +
>
> It is a bit sad that we are mostly working on the byte array
> buf->buf (or external file touched by various helper functions we
> call), even though we have a perfectly usable parsed representation
> in todo_list structure in all of the above and the rest of this
> function.
>
> It might be better to grab todo_list.nr into a local simple integer
> variable immediately after parse_insn_buffer() returns and then call
> todo_list_release() on todo_list, as the parsed todo-list is only
> used for two purposes (i.e. detecting format error by seeing if
> parser returns success, and seeing how many insns are on the todo
> list by checking todo_list.nr field) and nothing else. By releasing
> the otherwise unused todo_list early, you do not have to sprinkle
> various error return codepaths with calls to todo_list_release().
>
> That incidentally would manage too high expectation from readers of
> the code as well ;-).
>
Unfortunately, this strbuf is part of the todo_list, and it will be
freed if I call todo_list_release().
:/
Cheers,
Alban
