Re: [PATCH 09/22] sequencer: completely revamp the "todo" script parsing

>> Hmmm... commit_list is, as defined in commit.h, a linked list.
> 
> That is the most prominent reason why the rest is not a mindless
> conversion from commit_list to todo_list.
> 
> And we need todo_list as an array, because we need to be able to peek, or
> even move, backwards from the current command.
> 
>> Here todo_list uses growable array implementation of list.  Which
>> is I guess better on current CPU architecture, with slow memory,
>> limited-size caches, and adjacency prefetching.
> 
> That is not the reason that an array is used here. The array allows us
> much more flexibility.

>>> +struct todo_item *append_todo(struct todo_list *todo_list)
>>
>> Errr... I don't quite understand the name of this function.
>> What are you appending here to the todo_list?
> 
> A new item.
> 
>> Compare string_list_append() and string_list_append_nodup(),
>> where the second parameter is item to append.
> 
> Yes, that is correct. In the case of a todo_item, things are a lot more
> complicated, though. Some of the values have to be determined tediously
> (such as the offset and length of the oneline after the "pick <oid>"
> command). I just put those values directly into the newly allocated item,
> is all.

>>> +	ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);
>>> +	return todo_list->items + todo_list->nr++;
>>>  }
>>>  
>>> -static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)
>>> +static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
>>
>> Why the change of return type?  
> 
> Because it makes no sense to return a commit here because not all commands
> are about commits (think rebase -i's `exec`). It makes tons of sense to
> return an error condition, though.

>> Why now struct todo_item is first when struct replay_opts was last?
> 
> Those play very, very different roles.
> 
> The opts parameter used to provide parse_insn_line() with enough
> information to complain loudly when the overall command was not identical
> to the parsed command.
> 
> The item parameter is a receptacle for the parsed data. It will contain
> the pointer to the commit that was previously returned, if any. But it
> will also contain much more information, such as the command, the oneline,
> the offset in the buffer, etc etc
> 
> So "opts" was an "in" parameter while "item" is an "out" one. Apples and
> oranges.

>>> -	end_of_object_name = bol + strcspn(bol, " \t\n");
>>> +	end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
>>
>> Why is this cast needed?
> 
> Because bol is a "const char *" and we need to put "NUL" temporarily to
> *end_of_object_name:

> 
>>>  	saved = *end_of_object_name;
>>>  	*end_of_object_name = '\0';
>>>  	status = get_sha1(bol, commit_sha1);
>>>  	*end_of_object_name = saved;
> 
> Technically, this would have made a fine excuse to teach get_sha1() a mode
> where it expects a length parameter instead of relying on a NUL-terminated
> string.
> 
> Practically, such fine excuses cost me months in this rebase--helper
> project already, and I need to protect my time better.

> 
>>> -	/*
>>> -	 * Verify that the action matches up with the one in
>>> -	 * opts; we don't support arbitrary instructions
>>> -	 */
>>> -	if (action != opts->action) {
>>> -		if (action == REPLAY_REVERT)
>>> -		      error((opts->action == REPLAY_REVERT)
>>> -			    ? _("Cannot revert during another revert.")
>>
>> Errr... could the above ever happen?  Namely
>>
>>   action != opts->action && action == REPLAY_REVERT && opts->action == REPLAY_REVERT
>>
>> Surely not.
> 
> Your reply pointed to the very circumstance when this may happen: `git
> cherry-pick --continue` after an interrupted `git revert`.

>>> -			    : _("Cannot revert during a cherry-pick."));
>>> -		else
>>> -		      error((opts->action == REPLAY_REVERT)
>>> -			    ? _("Cannot cherry-pick during a revert.")
>>> -			    : _("Cannot cherry-pick during another cherry-pick."));
>>> -		return NULL;
>>> -	}
>>
>> Anyway, while it is / would be a good idea to prevent starting any
>> sequencer-based command (cherry-pick, revert, soon rebase -i) when
>> other command is in progress (cherry-pick, revert, soon rebase -i).
>> That is, if cherry-pick / revert waits for user action, you cannot
>> run another cherry-pick or revert.
>>
>> Which I guess the above code was not about...
> 
> It was about that, though.

> 
> It went about it in a pretty round-about way: opts->action comes from the
> name of the command ("was I called as `git revert` or `git cherry-pick`?")
> and action comes from the todo script, which was assumed to be written by
> a previous run of the sequencer, using the then-current value of
> opts->action.

> 
> So it wrote that command into *every single line* of the todo script, *for
> the sole purpose* of verifying that it was the same action when running
> via --continue.
> 
> As I said earlier, I would not complain at all if an interrupted `git
> revert` could be continued via `git cherry-pick --continue`.
> 
> If that is not desirable, I can reintroduce that overzealous check, but
> that will have to wait until after v2.10.0. And it would require an
> argument that convinces me.

> 
>>> +		item = append_todo(todo_list);
>>
>> A better name, in my personal option, would be
>>
>>   +		item = todo_list_next(todo_list);
>>
>> Or todo_next(todo_list).
> 
> That sounds more like a function that performs the next command in the
> todo_list.
> 
> While I agree that naming is hard, I still think that `append_todo()` with
> the todo_list as single parameter and returning a todo_item is pretty much
> self-explanatory: it appends a new item to the todo_list and returns a
> pointer to it.

>>> -	if (strbuf_read(&buf, fd, 0) < 0) {
>>> +	if (strbuf_read(&todo_list->buf, fd, 0) < 0) {
>>>  		close(fd);
>>> -		strbuf_release(&buf);
>>
>> A question: when is todo_list->buf released?
> 
> Why, I am glad you asked! It is released in todo_list_release(), called at
> the end e.g. of sequencer_continue().

>>> -static int walk_revs_populate_todo(struct commit_list **todo_list,
>>> +static int walk_revs_populate_todo(struct todo_list *todo_list,
>>>  				struct replay_opts *opts)
>>>  {
>>> +	enum todo_command command = opts->action == REPLAY_PICK ?
>>> +		TODO_PICK : TODO_REVERT;
>>>  	struct commit *commit;
>>> -	struct commit_list **next;
>>>  
>>>  	if (prepare_revs(opts))
>>>  		return -1;
>>>  
>>> -	next = todo_list;
>>> -	while ((commit = get_revision(opts->revs)))
>>> -		next = commit_list_append(commit, next);
>>> +	while ((commit = get_revision(opts->revs))) {
>>> +		struct todo_item *item = append_todo(todo_list);
>>> +		const char *commit_buffer = get_commit_buffer(commit, NULL);
>>
>> I see that you are creating todo file contents while walking revision list,
>> something that was left for later in current / previous implementation
>> of the sequencer...
> 
> Not really. This function was always about generating a todo_list. It just
> did not format it yet.
> 
> With the change of keeping the original formatting of the todo script
> instead of re-formatting it in save_todo(), this function now has to
> format the todo_list itself.

>>> -static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)
>>> +static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
>>>  {
>>>  	static struct lock_file todo_lock;
>>> -	struct strbuf buf = STRBUF_INIT;
>>> -	int fd;
>>> +	const char *todo_path = get_todo_path(opts);
>>> +	int next = todo_list->current, offset, fd;
>>
>> The "next = todo_list->current" looks a bit strange.
> 
> Depending whether we need rebase -i processing or revert/cherry-pick's
> slightly different one, the "current" position points to the next one
> already...
> 
>> Also, we do not change todo_list->current, we use it in one place, so it
>> can be used directly without help of temporary / helper variable.  But
>> that is just my personal opinion.
> 
> No, it has nothing to do with opinion. It prepares the code to keep it
> readable even when REPLAY_INTERACTIVE_REBASE is introduced.

>> and this should be done in one of earlier patches, isn't it?
> 
> No. I deliberately skipped save_todo() from "future-proofing" as I planned
> to rewrite it anyway. There is no point in future-proofing something you
> are going to toss in a minute.
> 

>>> -	if (commit_lock_file(&todo_lock) < 0) {
>>> -		strbuf_release(&buf);
>>> -		return error(_("Error wrapping up %s."), git_path_todo_file());
>>> -	}
>>> -	strbuf_release(&buf);
>>> +	if (commit_lock_file(&todo_lock) < 0)
>>> +		return error(_("Error wrapping up %s."), todo_path);
>>
>> Note: this is unrelated change, but we usually put paths in quotes, like this
>>
>>   +		return error(_("Error wrapping up '%s'."), todo_path);
>>
>> (in this and earlier error message), so that paths containing spaces show
>> correctly and readably to the user.  Though this possibly is not a problem
>> for this path.
> 
> Right.
> 
>> Also, how user is to understand "wrapping up"?
> 
> The same as before: the removed lines already had the error message,
> missing the quotes, too.
> 
> Don't get me wrong: I am a big fan of consistency, and I wish that Git's
> source code had more of it. So I would love to see a patch series that
> makes all error messages consistently reporting paths enclosed in single
> quotes.
> 
> I am also a big fan of the separation of concerns, though. And this patch
> series' concern is consistency *with the existing code*.
> 
> So I won't change the error message that I inherited at this point.

>>>  static int single_pick(struct commit *cmit, struct replay_opts *opts)
>>>  {
>>>  	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
>>> -	return do_pick_commit(cmit, opts);
>>> +	return do_pick_commit(opts->action == REPLAY_PICK ?
>>> +		TODO_PICK : TODO_REVERT, cmit, opts);
>>
>> The ternary conditional operator here translates one enum to other enum,
>> isn't it?
> 
> Well, almost. Please note that the enum will receive a new value in the
> sequencer-i patch series. And there is no equivalent todo_command for
> REPLAY_INTERACTIVE_REBASE.