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

diff --git a/sequencer.c b/sequencer.c
index 06759d4..3398774 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -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;

Why 'arg', and not 'oneline', or 'subject'?
I'm not saying it is bad name.

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.

No, it won't be wasted effort, as we *validate* the todo script this way.
And since we may very well need the info later (most rebases do not fail
in the middle), we store it, too.

Also, when the sequencer becomes one sequencer to rule them all, the
command set may contain something that does not even mention any
commit at all (think: exec).

Right, in which case the "commit" field will have the value... *drum
roll*... NULL!

So I am not sure if we want a parsed commit there (I would not
object if we kept the texual object name read from the file,
though).  The "one sequencer to rule them all" may even have to say
"now give name ':1' to the result of the previous operation" in one
step and in another later step have an instruction "merge ':1'".
When that happens, you cannot even pre-populate the commit object
when the sequencer reads the file, as the commit has not yet been
created at that point.

These considerations are pretty hypothetical. I would even place a bet
that we will *never* have ":1" as names, not if I have anything to say...
;-)

What is not so hypothetical is that after parsing the todo_list, we will
have to act on the information contained therein. For example we will have
to cherry-pick some of the indicated commits (requiring a struct commit *
for use in do_pick_commit()). Another example: we may need to determine
the oneline for use in fixup!/squash! reordering.

So: keeping *that* aspect of the previous todo_list parsing, i.e. store a
pointer to the already-parsed commit, is the right thing to do.

Ciao,
Dscho