Re: [RFC PATCH 00/11] Sequencer Foundations

[Daniel Barkalow <barkalow@iabervon.org>]

On Sun, 10 Apr 2011, Ramkumar Ramachandra wrote:

> Hi,
> 
> I've started working on building a sequencer for Git.  While the
> outline is described in [1], I'd like some additional clarifications.
> A big thanks to Christian's series [2] for the valuable roadmap.
> 
> Please note that 10/11 is not related to this series, but seems to be
> a minor nit that's required to make all existing tests pass.

That looks like an actual bug that only doesn't matter currently because 
the function is never called with enough junk on the stack.

> 0. Is the general flow alright?

I suspect it would be easier to review some of this with certain things 
squashed together; one patch that changes all of the variable references 
to what you want them to be is easier to understand than one that moves 
statics to function arguments, one that moves statics to struct fields, 
etc. Likewise, when you're converting some of the die() calls to error(), 
it's easier to review the patch if all of the die() calls that aren't 
changed in that patch don't get changed later in the series.

> 1. Is it okay to use this kind of adaptive error handling (calling
> 'die' in some places and returning error in other places), or should
> it be more uniform?

I think it should be systematic but not necessarily uniform. You should be 
able to give a guideline as to how to decide which to use (and you should 
probably actually give the guideline, so future developers make consistent 
choices). I think of "die" as being ideally for situations where the 
program can't understand what has happened well enough to know what to do 
about it.

> 2. In 11/11, I've used cmd_revert and cmd_rerere.  This is highly
> inelegant, mainly because of the command-line argument parsing
> overhead.  Re-implementing it using more low-level functions doesn't
> seem to be the way to go either: for example, 'reset --hard' has some
> additional logic of writing HEAD and ORIG_HEAD, which I don't want to
> duplicate.  Should I work on reworking parts of 'rerere.c' and
> 'revert.c', or is there some other way?

(ITYM cmd_reset here)

I think rerere.c should get a rerere_clear(). I think it would make sense 
to implement the reset locally; the abort ought to be undoing exactly 
those things that you did, and I'm not actually sure the ORIG_HEAD is 
entirely appropriate. You ought to be able to use cleanup functions that 
correspond to the functions you used to make the mess in the first place.

> 3. From the format of the TODO and DONE files, one more thing should
> be clear- I'm trying to stick to a slight variation of the 'rebase -i'
> format.  This part will go into the sequencer.  Then I'll use a
> cherry-pick specific file to keep the command-line options.  Yes, I'm
> trying to work on Daniel's idea [3] from the very start.  Is this a
> good idea?

I certainly like it. :)

> Thanks for reading.

You're welcome. :)

	-Daniel
*This .sig left intentionally blank*