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

From: Avery Pennarun <apenwarr@gmail.com>
Date: Nov 26, 2009, 22:00 UTC
Message-ID: <32541b130911261400t6b1b439em6305c4e1bfe135f8@mail.gmail.com>
In-Reply-To: <7vpr75hmpq.fsf@alter.siamese.dyndns.org>

On Thu, Nov 26, 2009 at 12:36 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

> "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