Re: [PATCH v4 4/9] versioncmp.c: refactor config reading next commit

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Refactor the reading of the versionSort.suffix and
> versionSort.prereleaseSuffix configuration variables to stay within
> the bounds of our CodingGuidelines when it comes to line length, and
> to avoid repeating ourselves.
>
> Let's also split out the names of the config variables into variables
> of our own, so we don't have to repeat ourselves, 

You do not have to repeat "we don't have to repeat" by mentioning it
twice in two paragraphs.

> Moving the "initialized = 1" assignment allows us to move some of this
> to the variable declarations in the subsequent commit.

Unclear until looking at these subsequent steps; let's see what
happens next ;-).

>  	if (!initialized) {
> -		const struct string_list *deprecated_prereleases;
> +		const char *const newk = "versionsort.suffix";
> +		const char *const oldk = "versionsort.prereleasesuffix";
> +		const struct string_list *oldl;

With s/oldl/deprecated_prereleases/ the damage would even be
smaller.  It's not like a more descriptive name in this small scope
hurts line length or readability, is it?

> +		prereleases = git_config_get_value_multi(newk);
> +		oldl = git_config_get_value_multi(oldk);

> +		if (prereleases && oldl)
> +			warning("ignoring %s because %s is set", oldk, newk);
> +		else if (!prereleases)
> +			prereleases = oldl;

This makes it more clear than the original what is going on, even
though they are equivalent.  If we have both, we ignore the
fallback, and if we don't have what we need, we replace it with the
fallback, which could be NULL in which case we end up not having
any.

It is a very nice added bonus that we ended up with a shallow
nesting.