From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 16 May 2013 19:29:24 GMT
Subject: Re: [PATCH v10 09/14] git-clean: use a git-add-interactive compatible UI
Message-ID: <7vy5be4tbf.fsf@alter.siamese.dyndns.org>
In-Reply-To: <9d73d532fb2d8372b971d607b9ddaa1af2b84ffd.1368696028.git.worldhello.net@gmail.com>

Jiang Xin <worldhello.net@gmail.com> writes:

> +static void print_highlight_menu_stuff(struct menu_stuff *stuff, int **chosen)
> +{
> +	struct string_list menu_list = STRING_LIST_INIT_DUP;
> +	struct strbuf menu = STRBUF_INIT;
> +	int i;
> +
> +	if (MENU_STUFF_TYPE_MENU_ITEM == stuff->type) {
> +	...
> +	} else if (MENU_STUFF_TYPE_STRING_LIST == stuff->type) {
> +	...
> +	}

This is better to write as:

	switch (stuff->type) {
	default:
        	die("programming error");
        case MENU_STUFF_TYPE_MENU_ITEM:
        	...
                break;
	case MENU_STUFF_TYPE_STRING_LIST:
        	...
		break;
	}

Besides, there is no good reason to write an equality comparison
between constant and variable in that order (people call it a "Yoda
condition"); do "var == const" if you must.

Also please fix this one:

> +		for_each_string_list_item(item, (struct string_list *)stuff->stuff) {
> +			if ((*chosen)[i] < 0)
> +				(*chosen)[i] = 0;
> +			strbuf_addf(&menu, "%s%2d: %s", (*chosen)[i] ? "*" : " ", ++i, item->string);

Because the evaluation order of function arguments are not defined
(not left to right; these are comma-expressions),

	(*chosen)[i] ? "*" : " "

may use the original value of "i", or value after increment the
evaluation of

	++i

left in "i".