On Thu, Nov 26, 2009 at 12:36 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > "Avery Pennarun" <apenwarr@xxxxxxxxx> writes: > >> We need to call exclude_cmds() after the loop, not during the loop, because >> excluding a command from the array can change the indexes of objects in the >> array. The result is that, depending on file ordering, some commands >> weren't excluded as they should have been. > > As an independent bugfix, I would prefer this to be made against 'maint' > and not as a part of this series. > > How did you notice it? Can you make a test case out of your experience of > noticing this bug in the first place, by the way (I am suspecting that you > saw some breakage and chased it in the debugger)? The story behind this one is a bit silly, but since you asked: I forgot to add recursive-ours and recursive-theirs to the list of known merge strategies, but was surprised to find that my test for recursive-theirs passed, while recursive-ours didn't. Investigating further, I found that the printed list of "found" strategies included recursive-theirs but not recursive-ours. Turns out that this is because when recursive-ours was being (correctly) removed, that slot in the array was being filled by recursive-theirs, and then immediately i++, which meant that recursive-theirs was never checked for exclusion as it should have been. Of course, after fixing this bug *both* tests were broken, but the correct thing to do was add both strategies to the list, which hides the effect of this bugfix. Since the bug is actually that *too many* strategies are listed instead of too few, it's pretty minor and I doubt it needs to go into maint. Also, I don't know of a way to test it that would be reliable. And I doubt this particular bug will recur anyway. (If it were too *few* strategies listed, I'm guessing it would be caught by any number of other tests.) Suggestions welcome. Thanks, Avery -- 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