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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

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

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