Elijah Newren <new...@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.
