On Wed, Feb 23, 2022 at 4:17 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes: > >> + if (opts->type != OPTION_GROUP && opts->help && > >> + !(starts_with(opts->help, "HEAD") || > >> + starts_with(opts->help, "GPG") || > >> + starts_with(opts->help, "DEPRECATED") || > >> + starts_with(opts->help, "SHA1")) && > >> + (opts->help[0] >= 65 && opts->help[0] <= 90)) > > > > This list of hardcoded exceptions may become a maintenance burden. I > > can figure out why OPTION_GROUP is treated specially here, but why use > > magic numbers 65 and 90 rather than a more obvious function like > > isupper()? > > > > Perhaps instead of hardcoding an exception list and magic numbers, we > > can use a simple heuristic instead. For instance, if the first two > > characters of the help string are uppercase, then assume it is an > > acronym (i.e. "GPG") or special name (i.e. "HEAD"), thus allowed. > > Maybe something like this: > > > > if (opts->type != OPTION_GROUP && opts->help && > > opts->help[0] && isupper(opts->help[0]) && > > !(opts->help[1] && isupper(opts->help[1]))) > > Much better than what was posted, but such a heuristic deserves some > in-code comment to check why we see the first two. Yes, I had the same thought as soon as I walked away from the computer and was going to post a follow-up email to say as much but got distracted by other things and never got around to it. Thanks for filling in the gap.
