Re: [PATCH v7 7/8] sequencer.c: define get_config_from_cleanup

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

Denton Liu <liu.denton@gmail.com> writes:

> +struct cleanup_config_mapping {
> +    const char *config_value;
> +    enum commit_msg_cleanup_mode editor_cleanup;
> +    enum commit_msg_cleanup_mode no_editor_cleanup;
> +};

Is this code using 4-space indent?  Please don't.  Also, I found
that Eric's comment on naming gave a good suggestion.

Is the cleanup_config_mapping[] array we see below supposed to be
constant, or does it allow further runtime configuration?  I am
assuming the former (i.e. when the user says "default", then
editor_cleanup will always become CLEANUP_ALL and no_editor_cleanup
will always become CLEANUP_SPACE), in which case, I wonder if we can
be more explicit about constness of the table.

> +/* note that we assume that cleanup_config_mapping[0] contains the default settings */

That sounds as if it is bad to make that assumption.  Be more
positive and direct to clearly tell future programmers what rule
they need to honor, e.g.

	/* the 0th element of this array must be the "default" */

> +struct cleanup_config_mapping cleanup_config_mappings[] = {

Do not give a plural name to an array.  Access to an element in a
array "type thing[]" can be written thing[4] and can be more
naturally read as "the fourth thing" (you do not say "the fourth
things") that way.

An exception is when you very often pass around the array as a whole
as one unit of datum across callchains.  

> +	struct cleanup_config_mapping *default_mapping = &cleanup_config_mappings[0];
> +	struct cleanup_config_mapping *current_mapping;
> +
> +	if (!cleanup_arg) {
> +		return use_editor ? default_mapping->editor_cleanup :
> +				    default_mapping->no_editor_cleanup;
> +	}

No need for extra {}.

> +
> +	for (current_mapping = cleanup_config_mappings; current_mapping->config_value; current_mapping++) {
> +		if (!strcmp(cleanup_arg, current_mapping->config_value)) {
> +			return use_editor ? current_mapping->editor_cleanup :
> +					    current_mapping->no_editor_cleanup;
> +		}
> +	}

Ditto.  In addition, perhaps split the for (...) like so:

	for (current_mapping = cleanup_config_mappings;
             current_mapping->config_value;
             current_mapping++)
		if (...)
			return ...;

> +const char *get_config_from_cleanup(enum commit_msg_cleanup_mode cleanup_mode)

static???

Is this really getting "config" from "cleanup"?  It rather smells
backwards i.e. grabbing the clean-up settings from the config system
to me.

> +{
> +	struct cleanup_config_mapping *current_mapping;
> +
> +	for (current_mapping = &cleanup_config_mappings[1]; current_mapping->config_value; current_mapping++) {
> +		if (cleanup_mode == current_mapping->editor_cleanup) {
> +			return current_mapping->config_value;
> +		}
> +	}
> +
> +	BUG(_("invalid cleanup_mode provided"));

Is that a bug (i.e. programming error) or a bad configuration file?
I think you meant the former, but then I do not think we want _()
around the message.  Instead, however, we may want to show the
cleanup_mode that was provided, possibly with the available values
in the .editor_cleanup field of cleanup_config_mapping[] entries.