Hi Daniel, Daniel Barkalow writes: >> 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. >> 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. >> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html