[2/5] *: relax git_configset_get_value_multi result

Commit Message

Derrick Stolee Sept. 27, 2022, 2:08 p.m. UTC

From: Derrick Stolee <[e-mail]>

We will change the config API to return an empty list instead of a NULL
list when there are no values in a multi-valued key. Update checks that
look for NULL and have them instead check for NULL or empty lists.

There are other checks that look for NULL, but then do an operation that
would be a no-op on an empty list. These are not modified here but will
be modified after we change the behavior of the config API.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 builtin/submodule--helper.c | 10 ++++++++--
 submodule.c                 |  2 +-
 t/helper/test-config.c      |  4 ++--
 versioncmp.c                |  8 ++++----
 4 files changed, 15 insertions(+), 9 deletions(-)

Comments

Taylor Blau Sept. 28, 2022, 3:58 p.m. UTC | #1

On Tue, Sep 27, 2022 at 02:08:28PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <[e-mail]>

Should the patch message be:

  *: relax git_config_get_value_multi() result

instead of referring to git_configset_get_value_multi?

> ---
>  builtin/submodule--helper.c | 10 ++++++++--
>  submodule.c                 |  2 +-
>  t/helper/test-config.c      |  4 ++--
>  versioncmp.c                |  8 ++++----
>  4 files changed, 15 insertions(+), 9 deletions(-)

All looks good here. I did a git grep for git_config_get_value_multi()
to make sure all of those spots were covered here. No comments from me,
other than a little thinking aloud below on a couple of the conversions.

> @@ -552,7 +553,9 @@ static int module_init(int argc, const char **argv, const char *prefix)
>  	 * If there are no path args and submodule.active is set then,
>  	 * by default, only initialize 'active' modules.
>  	 */
> -	if (!argc && git_config_get_value_multi("submodule.active"))
> +	if (!argc &&
> +	    (active_modules = git_config_get_value_multi("submodule.active")) &&
> +	    active_modules->nr)

Yuck ;-). I was going to suggest that the addition of a helper function
something like:

    static int any_configured(const char *key)
    {
      const struct string_list *vals = git_configset_get_value_multi(key);
      return vals && vals->nr;
    }

would be worthwhile for this and the below conversion, but the change at
the end of this series makes it a moot point. So the temporary eye-sore
is just fine.

> diff --git a/t/helper/test-config.c b/t/helper/test-config.c
> index 4ba9eb65606..62644dd71d7 100644
> --- a/t/helper/test-config.c
> +++ b/t/helper/test-config.c
> @@ -96,7 +96,7 @@ int cmd__config(int argc, const char **argv)
>  		}
>  	} else if (argc == 3 && !strcmp(argv[1], "get_value_multi")) {
>  		strptr = git_config_get_value_multi(argv[2]);
> -		if (strptr) {
> +		if (strptr && strptr->nr) {
>  			for (i = 0; i < strptr->nr; i++) {
>  				v = strptr->items[i].string;
>  				if (!v)

Good catch. I was thinking that this whole "if" statement could have
been dropped, but the goto that is hidden by the context meant that this
*would* have been a behavior change otherwise. So the conversion here is
appropriate.

Thanks,
Taylor

Patch

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 0b4acb442b2..1ddc08b076c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -531,6 +531,7 @@  static int module_init(int argc, const char **argv, const char *prefix)
 	struct init_cb info = INIT_CB_INIT;
 	struct pathspec pathspec = { 0 };
 	struct module_list list = MODULE_LIST_INIT;
+	const struct string_list *active_modules;
 	int quiet = 0;
 	struct option module_init_options[] = {
 		OPT__QUIET(&quiet, N_("suppress output for initializing a submodule")),
@@ -552,7 +553,9 @@  static int module_init(int argc, const char **argv, const char *prefix)
 	 * If there are no path args and submodule.active is set then,
 	 * by default, only initialize 'active' modules.
 	 */
-	if (!argc && git_config_get_value_multi("submodule.active"))
+	if (!argc &&
+	    (active_modules = git_config_get_value_multi("submodule.active")) &&
+	    active_modules->nr)
 		module_list_active(&list);
 
 	info.prefix = prefix;
@@ -2706,6 +2709,7 @@  static int module_update(int argc, const char **argv, const char *prefix)
 		opt.warn_if_uninitialized = 1;
 
 	if (opt.init) {
+		const struct string_list *active_modules;
 		struct module_list list = MODULE_LIST_INIT;
 		struct init_cb info = INIT_CB_INIT;
 
@@ -2720,7 +2724,9 @@  static int module_update(int argc, const char **argv, const char *prefix)
 		 * If there are no path args and submodule.active is set then,
 		 * by default, only initialize 'active' modules.
 		 */
-		if (!argc && git_config_get_value_multi("submodule.active"))
+		if (!argc &&
+		    (active_modules = git_config_get_value_multi("submodule.active")) &&
+		    active_modules->nr)
 			module_list_active(&list);
 
 		info.prefix = opt.prefix;
diff --git a/submodule.c b/submodule.c
index bf7a2c79183..06230961c80 100644
--- a/submodule.c
+++ b/submodule.c
@@ -275,7 +275,7 @@  int is_tree_submodule_active(struct repository *repo,
 
 	/* submodule.active is set */
 	sl = repo_config_get_value_multi(repo, "submodule.active");
-	if (sl) {
+	if (sl && sl->nr) {
 		struct pathspec ps;
 		struct strvec args = STRVEC_INIT;
 		const struct string_list_item *item;
diff --git a/t/helper/test-config.c b/t/helper/test-config.c
index 4ba9eb65606..62644dd71d7 100644
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
@@ -96,7 +96,7 @@  int cmd__config(int argc, const char **argv)
 		}
 	} else if (argc == 3 && !strcmp(argv[1], "get_value_multi")) {
 		strptr = git_config_get_value_multi(argv[2]);
-		if (strptr) {
+		if (strptr && strptr->nr) {
 			for (i = 0; i < strptr->nr; i++) {
 				v = strptr->items[i].string;
 				if (!v)
@@ -160,7 +160,7 @@  int cmd__config(int argc, const char **argv)
 			}
 		}
 		strptr = git_configset_get_value_multi(&cs, argv[2]);
-		if (strptr) {
+		if (strptr && strptr->nr) {
 			for (i = 0; i < strptr->nr; i++) {
 				v = strptr->items[i].string;
 				if (!v)
diff --git a/versioncmp.c b/versioncmp.c
index 069ee94a4d7..8772f869042 100644
--- a/versioncmp.c
+++ b/versioncmp.c
@@ -164,14 +164,14 @@  int versioncmp(const char *s1, const char *s2)
 		initialized = 1;
 		prereleases = git_config_get_value_multi("versionsort.suffix");
 		deprecated_prereleases = git_config_get_value_multi("versionsort.prereleasesuffix");
-		if (prereleases) {
-			if (deprecated_prereleases)
+		if (prereleases && prereleases->nr) {
+			if (deprecated_prereleases && deprecated_prereleases->nr)
 				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))
+	if (prereleases && prereleases->nr &&
+	    swap_prereleases(s1, s2, (const char *) p1 - s1 - 1, &diff))
 		return diff;
 
 	state = result_type[state * 3 + (((c2 == '0') + (isdigit (c2) != 0)))];