Subject: Re: [PATCH v3 18/24] merge-recursive: consolidate unnecessary fields in merge_options

From: Junio C Hamano <[email-masked]>
Date: Fri, 16 Aug 2019 16:24:32 -0700

Elijah Newren <[email-masked]> 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.