On Fri, Aug 16, 2019 at 3:14 PM Junio C Hamano <[email protected]> wrote: > > Elijah Newren <[email protected]> writes: > > > 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). > > > @@ -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)?
At the end of this series, the "merge-recursive: add sanity checks for relevant merge_options" commit adds some assertions that would fail if someone passed such a value, regardless of whether this patch was included or not. (Are we worried about people having such a config value and should we support it? It goes against the documented values, but I guess someone could have set it anyway even if it seems odd to set a value that says, "give me whatever the default is.") If we tried with this specific commit, though, then: diffcore-rename.c checks for rename_limit <= 0 and sets the value to 32767 in that case, so it'd have the effect of extending the default merge-related rename limit. As far as detect_rename, diff.c treats that as a boolean in most places (e.g. "if (options->detect_rename)"), and specifically compares against DIFF_DETECT_COPY in places where copy detection are relevant. So, detect_rename would behave the same. All of that said, I'm happy to restore the is-not-negative checks.
