Re: [PATCH] branch: optionally setup branch.*.merge from upstream local branches

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 20, 2008, 00:48 UTC
Message-ID: <7v4pc4fo6y.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20080220001339.GA16574@steel.home>

Alex Riesen <raa.lkml@gmail.com> writes:

Show 10 quoted lines

> Well, it could also mean that there is no rules yet, and you can
> do the next sane thing of your choice.
>
>> enum color_branch {
>> 	COLOR_BRANCH_RESET = 0,
>> 	COLOR_BRANCH_PLAIN = 1,
>> 	COLOR_BRANCH_REMOTE = 2,
>> 	COLOR_BRANCH_LOCAL = 3,
>> 	COLOR_BRANCH_CURRENT = 4,
>> };

This enum is used as an index into branch_colors[] array. Of course, by omitting everything you will get the default "start with 0, incrementing by 1" which will be the right assignment anyway. But we would want to leave a clue for people who would want to touch this later that individual values have some meaning, more than just that they have to be distinct.

Show 5 quoted lines

>> enum {
>> 	TAGS_UNSET = 0,
>> 	TAGS_DEFAULT = 1,
>> 	TAGS_SET = 2
>> };

This one can be made unspecified or even shuffled, because nobody does:

	int function_that_acts_on_tag_setting(int tag) {
                if (!tag) {
                        ... do something ...
                }
                if (TAGS_DEFAULT <= tag) {
                        ... do something else ...
                }
                return some_array[tag];
	}

So there is no reason to spell out any of the values.

>> enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;

Likewise.

Show 5 quoted lines

>> enum exist_status {
>> 	index_nonexistent = 0,
>> 	index_directory,
>> 	index_gitdir,
>> };

Likewise, modulo that making "nonexistent" explicitly to 0 is a very sensible thing whoever wrote that code has done. This is used as a type of directory_exists_in_index() function, and callers can say:

       	if (!directory_exists_in_index(dirname)) {
        	... ah, there is no such directory ...
	} else {
        	... something exists, but I do not care what kind ...
	}

So spelling out that "nonexistent MUST BE 0" (even though C language will give value 0 to it anyway) is a good convention.

Show 6 quoted lines

>> enum CAPABILITY {
>> 	NOLOGIN = 0,
>> 	UIDPLUS,
>> 	LITERALPLUS,
>> 	NAMESPACE,
>> };

This seems to be meant to match the order in the corresponding cap_list[] array, so this cannot be reshuffled (iow, it is similar to color_branch).

If you do not need to have any specific value assigned nor order among the enum tokens, (i.e. somebody later can add a new enum anywhere in the decl or even reshuffle the existing ones without breaking your code), it is a good idea to hint that fact by not having any "= value" in the decl. If you do rely on specific one being zero (see "exist_status" example above), it is better to spell it out that it has to be zero even if it is the first entry to hint that fact. Of course, if you need a set of values that are not sequential, you would need to spell out each and every one of them.