Re: [RFC PATCH 00/11] Sequencer Foundations

[Ramkumar Ramachandra <artagnon@gmail.com>]

Hi Daniel,

Daniel Barkalow writes:
> > 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.

Fixing this bug was one is one of the hidden motives of the patch I
send out just afterward [1].

> > 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.

Agreed -- I'll post a reworked series shortly.  Jonathan was finding
it hard to follow as well.

> > 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.

Jonathan concurs.  Where should I document this?

> > 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)

Yeah, sorry.

> 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.

Good suggestion -- rerere_clear is done [2]; now I have to figure out
if it makes sense to write a library for reset.

-- Ram

[1]: http://article.gmane.org/gmane.comp.version-control.git/171267
[2]: http://article.gmane.org/gmane.comp.version-control.git/171314