Re: [GSoC 2011] Git Sequencer

[Ramkumar Ramachandra <artagnon@gmail.com>]

Hi Christian,

Christian Couder writes:
> On Sunday 03 April 2011 19:20:56 Ramkumar Ramachandra wrote:
> > I'd like to re-apply this year as a student because I really want to
> > see (among other things), a sequencer in git.git.  Also, since I
> > worked on areas related to fast-import and remote helpers last year, I
> > thought I should work on something completely orthogonal this year.
> > 
> > I now have a draft of my proposal ready, and I'd really appreciate
> > feedback.  Also, could someone mentor me?
> Yeah! I would be happy to mentor you (or co-mentor you if someone else want to 
> be involved)!

Awesome! Thanks :)

> > == The Objective ==
> > 
> > To write git-sequencer, a new builtin command, and implement existing
> > commands on top of that.  This should give the commands more
> > functionality, improve their error handling, and make them faster.
> 
> You should first talk about extending git cherry-pick with --continue, --abort 
> and --skip, because it can be very valuable already if done properly with many 
> tests and if it's merged of course.
> 
> > The project can only be considered successful if all (or most) of the
> > code written gets merged into upstream.
> 
> Yeah, just say "most of the code". It is definitely good enough.

Okay, here's the new objective:

Extend 'git cherry-pick' with '--continue', '--abort', and '--skip'
features.  This will ultimately be used to write git-sequencer, a new
builtin command.  The sequencer will provide a uniform interface over
which existing commands like 'rebase', 'rebase -i' and 'am' can be
re-implented.  This should give the commands more functionality,
improve their error handling, and make them faster.  The project can
only be considered successful if most of the code written gets merged
into upstream.

> > 1.1. Factor out all calls to 'die' with 'return error' so so that we
> > can pause the entire process when a commit doesn't apply
> > automatically.
> > 
> > 1.2. Create and populate TODO and DONE files, similar to the one that
> > 'git-rebase--interactive.sh' creates.  For now, it should simply give
> > us information about why a 'cherry-pick' failed.  Use these files with
> > 'git-rebase--interactive.sh' to resume.
> 
> I am not sure it's a good thing to use 'git-rebase--interactive.sh' to resume 
> the cherry-pick. The parsing code already exists, is not very big, is in C and 
> has been reviewed and tested, so I think it's better to use.

Okay, noted.

> > 1.3. Port selective tests from the current 't3404' to make sure that
> > TODO and DONE are populated correctly; "stop on conflicting pick" is a
> > good candidate.
> > 
> > 1.4. Decouple the 'revert' functionality from the 'cherry-pick'
> > functionality in 'revert.c'.  Implement '--abort' for 'cherry-pick'
> > and port "abort" test from 't3404'.
> 
> I am not sure decoupling revert and cherry-pick functionnalities is really 
> needed, or I don't know what you mean exactly.

What I was thinking when I wrote that: we should have a 'revert.c'
independent of 'cherry-pick.c' after the sequencer is implemented (see
2).  By "decouple" I meant: move code around so we don't have to use
the enum { REVERT, CHERRY_PICK }.

> > 1.5. Implement parsing the TODO and DONE files into suitable data
> > structures.  Derive inspiration from the code written in 2008 to do
> > this.
> 
> Yeah, you can use this patch:
> 
> http://article.gmane.org/gmane.comp.version-control.git/162198

Yeah, this is exactly the patch I was referring to.  I just forgot to
include the link :p

> > 1.6. Implement '--continue' and '--skip', and write suitable tests.
> > Merge into upstream.
> 
> Great!
> 
> > 2. Factor out the 'cherry-pick' code from 'revert.c' into a new
> > 'builtin/sequencer.c', and expose a simple cherry-picking API in
> > 'sequencer.h'.
> 
> Yeah!
> 
> > 3. Implement a fresh 'cherry-pick.c' as a simple API call to the
> > sequencer, and make sure that all the existing tests pass.  After
> > this, cherry-pick will not be a builtin command anymore*.  Merge into
> > upstream.
> 
> Yeah, it is closely linked with the previous point. So maybe this can be 2.2 
> and the previous one can be 2.1. And this way we know that at the end of 1) 
> and at the end of 2) everything should be merged upstream.

Okay.

> > 4. Extend the sequncer to parse commands like 'execute', 'reword',
> > 'squash', and 'fixup' that are specific to interactive rebasing.
> > Carefully implement the functionality for each of these keywords in a
> > step-wise manner, making sure that it inter-operates with 'rebase -i'
> > seamlessly.
> 
> Great!
> 
> > 5. Port the entire testsuite from 'rebase -i', and rewrite
> > 'git-rebase.sh', 'git-rebase--interactive.sh' to call the sequncer.
> > The script should essentially build to a list of TODO instructions and
> > pass it to the 'git-sequencer' binary.  Merge into upstream.
> 
> Great! But as it is closely llinked with the previous point, maybe these 2 
> points should be 3.1 and 3.2.

Okay.

> > 6. [Optional] Lib'ify the sequncer: modify the sequencer API to
> > include rebase-related functionality.  Write 'rebase.c' as a bunch of
> > API calls to the sequencer.  Make sure that the existing tests pass.
> > Merge into upstream.
> > 
> > 7. [Optional] Re-implement 'git-am.sh' as a thin wrapper over the
> > sequncer: 'am.c'.  Bulk of this should be mbox parsing code.  Make sure
> > that all existing tests pass.  Merge into upstream.

I'll append Daniel's single-step-interrupt idea here, once I
understand how to implement it fully.

> > == Timeline ===
> > 
> > Deriving from last year's experience, I've decided not to present a
> > tight timeline.  Instead, I simply have an outline: Get the extended
> > cherry-pick functionality merged before mid-term evaluations, and the
> > rest before the final evaluations.
> 
> I agree, but still you could perhaps state something like this:
> 
> - before mid june:
> 	some patch series for everything in 1) should have been sent to the list
> - before midterm evaluation:
> 	everything in 1) should be merged upstream
> 	some patch series for everything in 2) and 3) (or 2.1 and 2.2 if you use 
> the numbering I suggest) should have been sent to the list
> - before the end of July:
> 	everything in 2) and 3) should be merged upstream
> 	some patch series for everything in 4) and 5) (or 3.1 and 3.2 if you use 
> the numbering I suggest) should have been sent to the list
> - before final evaluation:
> 	everything should be merged
> 
> I think it is better to have more details like the above because this way we 
> can realize early that there is not a lot of time after the midterm 
> evaluation.

Great suggestion! I'll include this in the next iteration of my
proposal.

Thanks for the detailed review and the suggestions.

-- Ram