Re: [PATCH 5/6] sequencer: Expose API to cherry-picking machinery

>> 1. Make prepare_revs and walk_revs_populate_todo return errors to be
>> handled by callers.  This is a fairly small patch that can come before
>> the big "code moving patch".
>
> This makes sense. If you type "git log --stat=foo", you don't get a
> diff usage message, even though it's an error parsing options that were
> originally part of diff.

>> 2. Duplicate action_name in both files.  I don't think it's too
>> serious, and we can fix this later.
>
> This is actually probably even a good idea, because the two functions
> might actually want to give different results. Maybe revert.c will end up
> doing different sequencer operations depending on whether the commit is a
> merge, but if revert.c has to give an error, it would call it the same
> thing either way, because the difference doesn't matter at the level of
> detail the revert.c works at; on the other hand, sequencer.c would want to
> distinguish the cases so that it is explaining exactly what it's trying to
> do in this step because it matters to how the issue would be resolved.
>
> Of course, at the point where you move the code, you only have one piece
> of code that you're starting from, so they'll be the same. But you might
> want to name them differently.

>> It has been enormously annoying to work with this "code moving patch":
>> everytime I make some changes to the earlier patches, I have to
>> recreate this one by hand; rebase offers no help whatsoever.  After
>> throwing away code based on this patch several times, I learnt my
>> lesson and restricted my series to avoid building on this patch.  I
>> consider this a very serious glitch and I'm interested in fixing it.
>> Thoughts?
>
> It's a hard problem, although likely worthwhile to solve. But only when
> you're not doing anything else, because it's complicated and will take you
> far afield. Essentially, what you need to do is implement a diff algorithm
> that can detect reorganization (or copying) of sections; this isn't
> something you can represent in unified diff output, but that's okay
> because you're not going to output it. You merge two of these results and
> apply the result to the base, which gives you a file (potentially with
> conflicts, which is another interesting issue because you have to
> represent and explain them somehow).

> *: bisect could be using sequencer in order to handle the situation where
> the user has said "commit A is good, commit B is bad, commit C breaks my
> system in a way that's unrelated"; the system should then be able to check
> out a maybe-bad commit and revert C from it, but it would be doing this in
> response to an instruction from the user: "give me something to test
> next", and would have to present errors differently.