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

Hi Daniel,

Daniel Barkalow writes:

quoted

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.

Yes :) I really liked the "revert: Propagate errors upwards from
do_pick_commit" patch.  I hope we improve the error handling in other
parts of Git soon.

quoted

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.

Excellent suggestion- I want to create a differentiation between the
sequencer "pick" action and the builtin "cherry-pick" operation.  I
mentioned earlier that I wanted the sequencer to be more than a fast
git-shell.  It'll be interesting to see the kinds of composite
"actions" we invent later; especially ones that only make sense in the
context of sequencing commits.

quoted

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

Hm, I was actually thinking of a much less ambitious helper that would
kick in when the heuristic correlation between two files is above a
certain threshold.  Your idea is better!  It would be totally awesome
if we could modify the diffing algorithm to work between files,
although I can't even imagine where to start.  Oh, and I think we have
to go far beyond the traditional in-file conflict markers to resolve
conflicts.  Pretty insane challenge :D  I'm definitely not experienced
enough to take this on now, although I hope to be good enough someday.

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

Thanks for the interesting sidenote.

-- Ram