Re: [PATCH v3 18/24] merge-recursive: consolidate unnecessary fields in merge_options
- From
Elijah Newren <newren@gmail.com>
- Date
- Aug 16, 2019, 22:59 UTC
- Message-ID
- <CABPp-BF6LPszUcKEwFGPr2DuFRZGU00uM-ky5YhKWw+het2QRA@mail.gmail.com>
- In-Reply-To
- <xmqqftm0zrjs.fsf@gitster-ct.c.googlers.com>
On Fri, Aug 16, 2019 at 3:14 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 41 quoted lines
>
> Elijah Newren <newren@gmail.com> 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.