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

[Avery Pennarun <apenwarr@gmail.com>]

On Thu, Nov 26, 2009 at 12:36 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Avery Pennarun" <apenwarr@gmail.com> 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