From 5b2e530360891ddf1c2f375d01c9ef9b41240e82 Mon Sep 17 00:00:00 2001
From: Derrick Stolee <dstolee@microsoft.com>
Date: Fri, 31 May 2024 09:45:16 -0400
Subject: [PATCH] fixup! scalar: add the `cache-server` command

A user reported that a use of 'scalar cache-server --set ...' on Linux
resulted in a failure with message

  munmap_chunk(): invalid pointer
  Aborted (core dumped)

It turns out that the cmd_cache_server() method has several invalid
mechanisms around command parsing. Specifically in this case, it is
attempting to free() the strings that are loaded from command-line
options. These strings are not actually owned by the process and should
not be freed. In fact, they should be stored as 'const' pointers to make
the compiler complain about freeing them.

There are a few other things that also seemed odd that I took the
liberty to fix while here. For one, the "--list" option used a
nonstandard version of OPT_STRING() only so it could set "(default)" as
the value when no string is provided. However, this does not work as
expected, and "--list" always requires a value. Make that explicit.

Add tests that check some of the critical paths of this command,
including the "--list" command querying the gvfs/config endpoint of the
remote URL. This works even without using "scalar clone --gvfs-protocol"
because it is only setting Git config as an end-result.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 scalar.c          | 27 +++++++++------------------
 t/t9210-scalar.sh | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 18 deletions(-)

diff --git a/scalar.c b/scalar.c
index 975f36ea73..369ed03b3b 100644
--- a/scalar.c
+++ b/scalar.c
@@ -1328,21 +1328,19 @@ static int cmd_version(int argc, const char **argv)
 static int cmd_cache_server(int argc, const char **argv)
 {
 	int get = 0;
-	char *set = NULL, *list = NULL;
-	const char *default_remote = "(default)";
+	const char *set = NULL, *list = NULL;
 	struct option options[] = {
-		OPT_BOOL(0, "get", &get,
-			 N_("get the configured cache-server URL")),
+		OPT_CMDMODE(0, "get", &get,
+			    N_("get the configured cache-server URL"), 1),
 		OPT_STRING(0, "set", &set, N_("URL"),
-			    N_("configure the cache-server to use")),
-		{ OPTION_STRING, 0, "list", &list, N_("remote"),
-		  N_("list the possible cache-server URLs"),
-		  PARSE_OPT_OPTARG, NULL, (intptr_t) default_remote },
+			   N_("configure the cache-server to use")),
+		OPT_STRING(0, "list", &list, N_("remote"),
+			   N_("list the possible cache-server URLs")),
 		OPT_END(),
 	};
 	const char * const usage[] = {
-		N_("scalar cache_server "
-		   "[--get | --set <url> | --list [<remote>]] [<enlistment>]"),
+		N_("scalar cache-server "
+		   "[--get | --set <url> | --list <remote>] [<enlistment>]"),
 		NULL
 	};
 	int res = 0;
@@ -1359,31 +1357,24 @@ static int cmd_cache_server(int argc, const char **argv)
 	if (list) {
 		const char *name = list, *url = list;
 
-		if (list == default_remote)
-			list = NULL;
-
-		if (!list || !strchr(list, '/')) {
+		if (!strchr(list, '/')) {
 			struct remote *remote;
 
 			/* Look up remote */
 			remote = remote_get(list);
 			if (!remote) {
 				error("no such remote: '%s'", name);
-				free(list);
 				return 1;
 			}
 			if (!remote->url) {
-				free(list);
 				return error(_("remote '%s' has no URLs"),
 					     name);
 			}
 			url = remote->url[0];
 		}
 		res = supports_gvfs_protocol(url, NULL);
-		free(list);
 	} else if (set) {
 		res = set_config("gvfs.cache-server=%s", set);
-		free(set);
 	} else {
 		char *url = NULL;
 
diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh
index 495d2e4255..720956130e 100755
--- a/t/t9210-scalar.sh
+++ b/t/t9210-scalar.sh
@@ -466,4 +466,38 @@ test_expect_success '`scalar delete` with existing repo' '
 	test_path_is_missing existing
 '
 
+test_expect_success 'scalar cache-server basics' '
+	repo=with-cache-server &&
+	git init $repo &&
+	scalar cache-server --get $repo >out &&
+	cat >expect <<-EOF &&
+	Using cache server: (undefined)
+	EOF
+	test_cmp expect out &&
+
+	scalar cache-server --set http://fake-server/url $repo &&
+	test_cmp_config -C $repo http://fake-server/url gvfs.cache-server &&
+	scalar delete $repo &&
+	test_path_is_missing $repo
+'
+
+test_expect_success 'scalar cache-server list URL' '
+	repo=with-real-gvfs &&
+	git init $repo &&
+	git -C $repo remote add origin http://$HOST_PORT/ &&
+	scalar cache-server --list origin $repo >out &&
+
+	cat >expect <<-EOF &&
+	#0: http://$HOST_PORT/servertype/cache
+	EOF
+
+	test_cmp expect out &&
+
+	test_must_fail scalar -C $repo cache-server --list 2>err &&
+	grep "requires a value" err &&
+
+	scalar delete $repo &&
+	test_path_is_missing $repo
+'
+
 test_done