| Message ID | patch-v4-4.9-aae1d5c12a9-20230202T131155Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f6f348a6d5d585fb3eda326a20bf2bab9e60ef51 |
| Headers | show |
| Series | config API: make "multi" safe, fix numerous segfaults | expand |
Æ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.
diff --git a/versioncmp.c b/versioncmp.c index 069ee94a4d7..323f5d35ea8 100644 --- a/versioncmp.c +++ b/versioncmp.c @@ -160,15 +160,18 @@ int versioncmp(const char *s1, const char *s2) } 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; + + 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; + initialized = 1; - prereleases = git_config_get_value_multi("versionsort.suffix"); - deprecated_prereleases = git_config_get_value_multi("versionsort.prereleasesuffix"); - if (prereleases) { - if (deprecated_prereleases) - warning("ignoring versionsort.prereleasesuffix because versionsort.suffix is set"); - } else - prereleases = deprecated_prereleases; } if (prereleases && swap_prereleases(s1, s2, (const char *) p1 - s1 - 1, &diff))
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, and refactor the nested if/else to avoid indenting it, and the existing bracing style issue. This all helps with the subsequent commit, where we'll need to start checking different git_config_get_value_multi() return value. See c026557a373 (versioncmp: generalize version sort suffix reordering, 2016-12-08) for the original implementation of most of this. Moving the "initialized = 1" assignment allows us to move some of this to the variable declarations in the subsequent commit. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- versioncmp.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)