Ayush Chandekar <ayu.chandekar@xxxxxxxxx> 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 *);
