Re: [PATCH v3 18/24] merge-recursive: consolidate unnecessary fields in merge_options
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 16, 2019, 22:14 UTC
- Message-ID
- <xmqqftm0zrjs.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20190815214053.16594-19-newren@gmail.com>
Elijah Newren <newren@gmail.com> writes:
Show 6 quoted lines
> static inline int merge_detect_rename(struct merge_options *opt)
> {
> - return opt->merge_detect_rename >= 0 ? opt->merge_detect_rename :
> - opt->diff_detect_rename >= 0 ? opt->diff_detect_rename : 1;
> + return (opt->detect_renames != -1) ? opt->detect_renames : 1;
> }Every time I see "is it not negative?" (or more generally "is it in this range?") converted to "is it not this exact value?", it makes me feel uneasy.
> - opts.rename_limit = opt->merge_rename_limit >= 0 ? opt->merge_rename_limit : > - opt->diff_rename_limit >= 0 ? opt->diff_rename_limit : > - 1000; > + opts.rename_limit = (opt->rename_limit != -1) ? opt->rename_limit : 1000;
Likewise. I have no objection to merging two rename-limit to a single field (and two detect-renames to a single field).
Show 8 quoted lines
> @@ -3732,14 +3729,14 @@ static void merge_recursive_config(struct merge_options *opt)
> {
> char *value = NULL;
> git_config_get_int("merge.verbosity", &opt->verbosity);
> - git_config_get_int("diff.renamelimit", &opt->diff_rename_limit);
> - git_config_get_int("merge.renamelimit", &opt->merge_rename_limit);
> + git_config_get_int("diff.renamelimit", &opt->rename_limit);
> + git_config_get_int("merge.renamelimit", &opt->rename_limit);Hmph. If merge.renameLimit is there, that would overwrite whatever we get by reading from diff.renameLimit, so the two fields with runtime precedence order can easily be replaced by these two calls.
Nice.
If you have "[diff] renamelimit = -2" in your $GIT_DIR/config, would we change behaviour due to the earlier conversion that has nothing to do with the theme of this step (i.e. consolidate two variables into one)?
Show 51 quoted lines
> @@ -3765,11 +3762,9 @@ void init_merge_options(struct merge_options *opt,
> opt->repo = repo;
> opt->verbosity = 2;
> opt->buffer_output = 1;
> - opt->diff_rename_limit = -1;
> - opt->merge_rename_limit = -1;
> + opt->rename_limit = -1;
> opt->renormalize = 0;
> - opt->diff_detect_rename = -1;
> - opt->merge_detect_rename = -1;
> + opt->detect_renames = -1;
> opt->detect_directory_renames = MERGE_DIRECTORY_RENAMES_CONFLICT;
> merge_recursive_config(opt);
> merge_verbosity = getenv("GIT_MERGE_VERBOSITY");
> @@ -3821,16 +3816,16 @@ int parse_merge_opt(struct merge_options *opt, const char *s)
> else if (!strcmp(s, "no-renormalize"))
> opt->renormalize = 0;
> else if (!strcmp(s, "no-renames"))
> - opt->merge_detect_rename = 0;
> + opt->detect_renames = 0;
> else if (!strcmp(s, "find-renames")) {
> - opt->merge_detect_rename = 1;
> + opt->detect_renames = 1;
> opt->rename_score = 0;
> }
> else if (skip_prefix(s, "find-renames=", &arg) ||
> skip_prefix(s, "rename-threshold=", &arg)) {
> if ((opt->rename_score = parse_rename_score(&arg)) == -1 || *arg != 0)
> return -1;
> - opt->merge_detect_rename = 1;
> + opt->detect_renames = 1;
> }
> /*
> * Please update $__git_merge_strategy_options in
> diff --git a/merge-recursive.h b/merge-recursive.h
> index 0fdae904dd..f4bdfbc897 100644
> --- a/merge-recursive.h
> +++ b/merge-recursive.h
> @@ -27,10 +27,8 @@ struct merge_options {
> MERGE_DIRECTORY_RENAMES_CONFLICT = 1,
> MERGE_DIRECTORY_RENAMES_TRUE = 2
> } detect_directory_renames;
> - int diff_detect_rename;
> - int merge_detect_rename;
> - int diff_rename_limit;
> - int merge_rename_limit;
> + int detect_renames;
> + int rename_limit;
> int rename_score;
> int needed_rename_limit;
> int show_rename_progress;