On Fri, Aug 16, 2019 at 3:14 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Elijah Newren <newren@xxxxxxxxx> 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.
