Re: [PATCH 13/22] sequencer: remember the onelines when parsing the todo file
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Sep 1, 2016, 13:14 UTC
- Message-ID
- <alpine.DEB.2.20.1609011512200.129229@virtualbox>
- In-Reply-To
- <xmqqzinszpww.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Wed, 31 Aug 2016, Junio C Hamano wrote:
Show 23 quoted lines
> Jakub Narębski <jnareb@gmail.com> writes:
>
> >>>> @@ -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;
> >
> >> 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