Re: [RFC PATCH 00/11] Sequencer Foundations

Hi Daniel,

Daniel Barkalow writes:

quoted

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

quoted

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.

quoted

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?

quoted

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