Revision fea00266488c85ba94e735d5885d991298e8f9d7

[Etan Reisner]

On Fri, Aug 24, 2007 at 02:19:45AM -0400, jeff2 at soc.pidgin.im wrote:
> Add an assertion in purple_blist_add_{chat,buddy} to return if a buddy was
> added to the buddy list in a group that is not in the buddy list. This
> improper usage previously caused duplicate groups to be shown in the
> buddy list, which are then dropped after restarting Pidgin.
>
> This change may incur a performance hit on every buddy added to the buddy
> list with a non-null group. If this performance is noticeably worse, an
> alternate assertion (which is less readable) can be used as #2752.
>
> Closes #2752.

> ============================================================
> --- libpurple/blist.c	8baced85c0fded9721bee0d5b841a56da23e2f74
> +++ libpurple/blist.c	53ea0c69ae6b3c9b7fb6fa7c9e7f2b9ca16e2b5c
> @@ -1190,6 +1190,9 @@ void purple_blist_add_chat(PurpleChat *c
>  			group = purple_group_new(_("Chats"));
>  			purple_blist_add_group(group,
>  					purple_blist_get_last_sibling(purplebuddylist->root));
> +		} else {
> +			/* Fail if tried to add buddy to a group that isn't on the blist. #2752. */
> +			g_return_if_fail(purple_find_group(group->name));
>  		}
>  	} else {
>  		group = (PurpleGroup*)node->parent;
> @@ -1284,6 +1287,10 @@ void purple_blist_add_buddy(PurpleBuddy
>  		g = (PurpleGroup *)((PurpleBlistNode *)c)->parent;
>  	} else {
>  		if (group) {
> +			/*  Fail if trying to add buddy to a group that is not on the buddy list.
> +			 *  Fix for #2752. */
> +			g_return_if_fail(purple_find_group(group->name));
> +
>  			g = group;
>  		} else {
>  			g = purple_group_new(_("Buddies"));

This patch looks suspicious to me. To begin with I'm not at all sure I
understand the bug that it is trying to fix, nor how this is a correct fix
for it.

It seems to me that the bug is about PurpleGroup:s that have been created
with purple_group_new being used in the purple_blist_add_{chat,buddy}
functions before having been added to the buddy list with
purple_blist_add_group, is that correct? Assuming that is the problem
wouldn't it have been just as effective (and significantly more friendly)
to just *add* the group in add_{chat,buddy} if it isn't in the buddy list
already?

Even if that isn't a more reasonable solution I think the bug is in fact
in pidgin and not in libpurple in that it displays a group more than once
(when that group isn't added correctly at least), no?

Ignoring all of the above, even if the current 'fix' is the right one
(which as I've said I doubt since, if for no other reason, we silently
create a 'Buddies' and 'Chats' group if no group is given), having left
the g = group line is wrong.

	-Etan

P.S. I'm off to the beach for my wife's last weekend of freedom and won't
be back until Sunday night some time, so I won't be around to discuss this
until then. Feel free to pretend that any email Ethan writes I would have
written with a slightly style. =)