Re: [RFC PATCH 00/11] Sequencer Foundations

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 10, 2011, 19:47 UTC
Message-ID: <20110410194739.GC28163@elie>
In-Reply-To: <1302448317-32387-1-git-send-email-artagnon@gmail.com>

Hi again,

Ramkumar Ramachandra wrote:

> I've started working on building a sequencer for Git.

Happy to see.

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

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

Show 7 quoted lines

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

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.

Show 6 quoted lines

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

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

Regards, Jonathan