Web lists-archives.com

Re: [GSoC][PATCH v3 10/13] rebase--interactive: rewrite complete_action() in C




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