[v3,18/24] merge-recursive: consolidate unnecessary fields in merge_options
diff mbox series

Message ID 20190815214053.16594-19-newren@gmail.com
State New
Headers show
Series
  • Clean up merge API
Related show

Commit Message

Elijah Newren Aug. 15, 2019, 9:40 p.m. UTC
We provided users with the ability to state whether they wanted rename
detection, and to put a limit on how much CPU would be spent.  Both of
these fields had multiple configuration parameters for setting them,
with one being a fallback and the other being an override.  However,
instead of implementing the logic for how to combine the multiple
source locations into the appropriate setting at config loading time,
we loaded and tracked both values and then made the code combine them
every time it wanted to check the overall value.  This had a few
minor drawbacks:
  * it seems more complicated than necessary
  * it runs the risk of people using the independent settings in the
    future and breaking the intent of how the options are used
    together
  * it makes merge_options more complicated than necessary for other
    potential users of the API

Fix these problems by moving the logic for combining the pairs of
options into a single value; make it apply at time-of-config-loading
instead of each-time-of-use.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c | 27 +++++++++++----------------
 merge-recursive.h |  6 ++----
 2 files changed, 13 insertions(+), 20 deletions(-)

Comments

Junio C Hamano Aug. 16, 2019, 10:14 p.m. UTC | #1
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)?

> @@ -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;
Elijah Newren Aug. 16, 2019, 10:59 p.m. UTC | #2
On Fri, Aug 16, 2019 at 3:14 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.
Junio C Hamano Aug. 16, 2019, 11:24 p.m. UTC | #3
Elijah Newren <newren@gmail.com> writes:

> 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.")

I am somewhat worried about changing the rule on the users without
telling them (and without having a good reason to enforce a tighter
version retroactively).

If we are formally forbidding "[diff] renameLimit = -1" and other
out-of-bound values, (1) assert() will most often turn into noop in
non-debug builds, so the last step of this series may not be such a
good sanity check anyway, (2) "if (condition) BUG();" may be better,
but that's for catching programming errors that made the condition
hold and configuration the end-user had should not trigger it.

So if we want to really catch bogus end-user-supplied values (and we
really do---but the definition of "bogus" may be debatable), the
place to do so is not the "sanity-check at the end", but where we
call get_config_int() and friends.

Patch
diff mbox series

diff --git a/merge-recursive.c b/merge-recursive.c
index e401114b8f..b846acf931 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -385,8 +385,7 @@  static int add_cacheinfo(struct merge_options *opt,
 
 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;
 }
 
 static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree)
@@ -1883,9 +1882,7 @@  static struct diff_queue_struct *get_diffpairs(struct merge_options *opt,
 	 */
 	if (opts.detect_rename > DIFF_DETECT_RENAME)
 		opts.detect_rename = DIFF_DETECT_RENAME;
-	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;
 	opts.rename_score = opt->rename_score;
 	opts.show_rename_progress = opt->show_rename_progress;
 	opts.output_format = DIFF_FORMAT_NO_OUTPUT;
@@ -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);
 	if (!git_config_get_string("diff.renames", &value)) {
-		opt->diff_detect_rename = git_config_rename("diff.renames", value);
+		opt->detect_renames = git_config_rename("diff.renames", value);
 		free(value);
 	}
 	if (!git_config_get_string("merge.renames", &value)) {
-		opt->merge_detect_rename = git_config_rename("merge.renames", value);
+		opt->detect_renames = git_config_rename("merge.renames", value);
 		free(value);
 	}
 	if (!git_config_get_string("merge.directoryrenames", &value)) {
@@ -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;