Re: [PATCH 2/8] builtin-merge.c: call exclude_cmds() correctly.

On Thu, Nov 26, 2009 at 12:36 AM, Junio C Hamano [off-list ref] wrote:

"Avery Pennarun" [off-list ref] writes:

quoted

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