Re: [PATCH 13/22] sequencer: remember the onelines when parsing the todo file

Hi Junio,

On Wed, 31 Aug 2016, Junio C Hamano wrote:

Jakub Narębski [off-list ref] writes:

quoted

quoted

quoted

quoted

@@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts *opts)
 struct todo_item {
 	enum todo_command command;
 	struct commit *commit;
+	const char *arg;
+	int arg_len;

quoted

I am not sure what the "commit" field of type "struct commit *" is
for.  It is not needed until it is the commit's turn to be picked or
reverted; if we end up stopping in the middle, parsing the commit
object for later steps will end up being wasted effort.

From what I understand this was what sequencer did before this
series, so it is not a regression (I think; the commit parsing
was in different function, but I think at the same place in
the callchain).

Yes, I agree with you and I didn't mean "this is a new bug" at all.
It just is an indication that further refactoring after this step is
needed, and it is likely to involve removal or modification of this
field.

Not so. After you review the sequencer-i patches, you will see that it
makes tons of sense to have that "commit" field: "pick" is by far the most
common case, and it would not make sense at all to validate the todo
script, only to throw away the information we got, only to re-gain it
later in the sequencer.

Read: I strongly disagree with your cursory verdict that the "commit"
field should go.

Ciao,
Dscho