Re: [PATCH/RFC/GSoC 12/17] rebase-todo: introduce rebase_todo_item

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 16, 2016, 15:55 UTC
Message-ID: <alpine.DEB.2.20.1603161647560.4690@virtualbox>
In-Reply-To: <CACRoPnRH1D=8k5uuUahh1MJOAXsWkhY0fWev2AQhJm5+WWk5rQ@mail.gmail.com>

Hi Paul,

On Wed, 16 Mar 2016, Paul Tan wrote:

Show 17 quoted lines

> On Mon, Mar 14, 2016 at 9:43 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
> > On Sat, Mar 12, 2016 at 11:46 AM, Paul Tan <pyokagan@gmail.com> wrote:
> >> In an interactive rebase, commands are read and executed from a todo
> >> list (.git/rebase-merge/git-rebase-todo) to perform the rebase.
> >>
> >> In the upcoming re-implementation of git-rebase -i in C, it is useful
> >> to be able to parse each command into a data structure which can then
> >> be operated on. Implement rebase_todo_item for this.
> >
> > sequencer.{c,h} already has some code to parse and create todo lists
> > for cherry-picking or reverting multiple commits, so I am wondering if
> > it would be possible to share some code?
> 
> AFAIK, sequencer.c as it is in master parses the todo list
> destructively and does not keep the associated action for each commit
> and the "rest" string.

This is a *serious* mistake in the implementation of the sequencer, I agree.

Therefore it is a good idea to fix that mistake instead of leaving it in place.

And that is exactly what I did:

	https://github.com/dscho/git/commit/b9b5b7351

Please note that the commit is marked as a "TODO" because it has to reintroduce a stupidly strict behavior (the sequencer expects the commands in the todo script to agree with the overall action, i.e. if the action is REPLAY_REVERT, the todo script can only contain "revert" commands, if the action is REPLAY_PICK, the todo list can only contain "pick" commands). Of course this restriction is totally arbitrary and even unwanted, so I will lift it after this commit. Or maybe I'll just lift it before this commit. Yeah, I'll do that instead.

> As I said in another thread, originally I wanted to keep the scope
> simple, and just do the rewrite of rebase from C to shell, and let any
> further libifications and optimizations come after, so I didn't want
> to touch sequencer for now.

We know, however, how leaving technical debt for later will just make sure that technical debt accumulates...

And while I have a *tremendous* respect for what Karthik did (and continues to do, even long after his GSoC project ended!), I do not want to ask any future GSoC student to clean up the mess that we produce right now... So let's just not add more technical debt than we absolutely have to.

Ciao, Dscho