On Sat, 13 Aug 2011, Ramkumar Ramachandra wrote: > Hi again, > > Jonathan Nieder writes: > > Another thought. I wonder if it's possible to leave > > sequencer_parse_args() private to builtin/revert.c, making the split > > a little more logical: > > Yes, I'd like this too. However, there are two new issues: > revert_or_cherry_pick_usage and action_name. The former has two > callsites: one in prepare_revs (in sequencer.c) and another in > parse_args (in builtin/revert.c). Unfortunately, action_name is even > more complicated to get rid of: the information from it is used all > over the place. One thing to consider is that sequencer.c will be used by all sorts of different builtins, which are each implementing instructions given differently by the user. Just because a message makes sense as output from revert.c doesn't mean it will make sense from (for example) bisect*. > Attempting to attack the problems one by one: > 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. -Daniel *This .sig left intentionally blank*