Re: [RFC PATCH 00/11] Sequencer Foundations

Hi Jonathan,

Jonathan Nieder writes:

Ramkumar Ramachandra wrote:

quoted

0. Is the general flow alright?

Not sure --- I don't have the big picture.  Could you give a quick
summary of the flow in the cover letter ("patch 1 does such-and-such,
so patch 2 can do such-and-such, leading to...") to the next revision,
and quick explanations of the purpose (i.e., why we should want each
change) in the individual change descriptions?

Agreed.  I'll ask again after the next re-roll.

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?

'die' gets used in two ways (well, one way really):

 - to say "there is no sane way to recover from this failure".  For
   example, xmalloc dies if even after trying to free up memory,
   malloc still could not satisfy the request.

 - to say "so far we've been too lazy to implement recovery from
   this failure".  Or "while we *could* recover from this failure, no
   one has needed it, so let's not --- that code would just bitrot."

Therefore a mixture of 'die' and 'return error' seems inevitable.  The
dangerous mixtures to avoid are those likely to trip up callers (e.g.,
if all code paths 'return error' except one edge case).

My thoughts precisely.  I was worried about what would happen in
future when Git is completely lib'ified, but I suppose it makes little
sense to think about that now.

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?

See "git log --grep=libify" for examples.  Isn't rerere already
libified?  Ah, you need "rerere clear" --- I think a rerere_clear
function alongside rerere_forget et al would make sense.

Done in [1].

More generally, it should be feasible to expose a nice, simple API for
any functionality you need (with params struct if necessary, etc).
That's how many of the current APIs (revision walking, diffcore, ...)
came about.

I see.  Okay, I'll see if it makes sense to craft an API for rebase.

quoted

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?

This is still bouncing in my head.  I think I like it --- is the idea
that some day you could put commands like

	am topic.mbox

in your insn sheet, or do nested rebases with a --force-nested option?
That does sound useful.  How would one request "skip to the next
operation in the outer rebase" on the command line?  This is starting
to feel like a debugger.

I'll respond to this later in the thread, since Daniel has already
said something.  I just need help with crafting a nice instruction
sheet now -- please join the discussion at [2].

quoted

4. I have a feeling that I've broken translation strings.  Is there a
README, plus a bunch of tests I can run to make sure that I've not
broken anything?

If you put "GETTEXT_POISON = YesPlease" in your config.mak, the
translations will be translated to gibberish when the GIT_GETTEXT_POISON
envvar is set, so you can use the test suite as a sanity check.
"make pot" can be used to get the translation template that
translators will see.

As for a readme explaining how to use _, N_, and Q_, yes, I think that
would be useful.  I think Ævar's series has something like that (but
targetted at translators) later on; it might make sense to prod him or
me for a simpler explanation can be merged immediately.  Until then,
there is "git log gettext.h".

Thanks!  Someone should really document this whole translations thing.

-- Ram

[1]: http://article.gmane.org/gmane.comp.version-control.git/171314
[2]: http://thread.gmane.org/gmane.comp.version-control.git/171255/focus=171307