Re: [GSOC PATCH 1/2] environment: remove the global variable 'merge_log_config'

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

Ayush Chandekar <ayu.chandekar@gmail.com> writes:

> The global variable 'merge_log_config', set via the "merge.log" or
> "merge.summary" settings, is only used in 'cmd_fmt_merge_msg()' and
> 'cmd_merge()' to adjust the 'shortlog_len' variable.
>
> Remove 'merge_log_config' and introduce a function
> 'adjust_shortlog_len()' in fmt-merge-msg.c to handle the 'shortlog_len'
> variable.
>
> This change is part of an ongoing effort to eliminate global variables,
> improve modularity and help libify the codebase.

And the downsides of this change are...?

One obvious behaviour change I can see can happen when you have an
invalid value set to merge.summary and run the command with command
line override with the "--log" option.  In the current code, the
config callback barfs when it notices an invalid merge.summary
setting, even though it won't be used because the valid value given
via the "--log" option would override it.  In the updated code,
adjust_shortlog_len() would short-circuit and does not even bother
reading from the configuration, so the user will not be notified of
a broken configuration.

It is not immediately obvious if this particular behaviour change is
a regression or an improvement, but it probably deserves to be noted
somewhere to help future developers what our thinking was.

> @@ -26,14 +26,7 @@ static struct string_list suppress_dest_patterns = STRING_LIST_INIT_DUP;
>  int fmt_merge_msg_config(const char *key, const char *value,
>  			 const struct config_context *ctx, void *cb)
>  {
> -	if (!strcmp(key, "merge.log") || !strcmp(key, "merge.summary")) {
> -		int is_bool;
> -		merge_log_config = git_config_bool_or_int(key, value, ctx->kvi, &is_bool);
> -		if (!is_bool && merge_log_config < 0)
> -			return error("%s: negative length %s", key, value);
> -		if (is_bool && merge_log_config)
> -			merge_log_config = DEFAULT_MERGE_LOG_LEN;
> -	} else if (!strcmp(key, "merge.branchdesc")) {
> +	if (!strcmp(key, "merge.branchdesc")) {
>  		use_branch_desc = git_config_bool(key, value);
>  	} else if (!strcmp(key, "merge.suppressdest")) {
>  		if (!value)
> @@ -645,6 +638,27 @@ static void find_merge_parents(struct merge_parents *result,
>  	result->nr = j;
>  }
>  
> +void adjust_shortlog_len(struct repository *r, int *shortlog_len)
> +{
> +	const char *keys[] = { "merge.log", "merge.summary", NULL};
> +	
> +	if (*shortlog_len >= 0)
> +		return;
> +
> +	for (const char **key = keys; *key; ++key) {
> +		int is_bool, value;
> +		if (!repo_config_get_bool_or_int(r, *key, &is_bool, &value)) {
> +			if (!is_bool && value < 0) {
> +				error("%s: negative length %d", *key, value);
> +				return;
> +			}
> +			*shortlog_len = (is_bool && value) ? DEFAULT_MERGE_LOG_LEN : value;
> +			return;
> +		}
> +	}
> +
> +	*shortlog_len = 0;
> +}
>  
>  int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
>  		  struct fmt_merge_msg_opts *opts)
> diff --git a/fmt-merge-msg.h b/fmt-merge-msg.h
> index 73ca3e4465..f54f00d26f 100644
> --- a/fmt-merge-msg.h
> +++ b/fmt-merge-msg.h
> @@ -2,6 +2,7 @@
>  #define FMT_MERGE_MSG_H
>  
>  #include "strbuf.h"
> +#include "repository.h"
>  
>  #define DEFAULT_MERGE_LOG_LEN 20
>  
> @@ -12,9 +13,9 @@ struct fmt_merge_msg_opts {
>  	const char *into_name;
>  };
>  
> -extern int merge_log_config;
>  int fmt_merge_msg_config(const char *key, const char *value,
>  			 const struct config_context *ctx, void *cb);
> +void adjust_shortlog_len(struct repository *r, int *shortlog_len);
>  int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
>  		  struct fmt_merge_msg_opts *);