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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sep 1, 2016, 09:37 UTC
Message-ID: <alpine.DEB.2.20.1609011052260.129229@virtualbox>
In-Reply-To: <xmqq8tvc21re.fsf@gitster.mtv.corp.google.com>

Hi Junio,

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

Show 20 quoted lines

> Jakub Narębski <jnareb@gmail.com> writes:
> 
> >> 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!

Show 8 quoted lines

> 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