cmd/gitannex: Permit remotes with options

It looks like commit 2a1e28f5f5683eaba22d67a0e253c95b934f8c41 did not
fix the errors in the integration tests that I hoped it would. Upon
further inspection, I noticed that I forgot that remotes can have
options just like backends.

This should fix some of the failing integration tests. For context:
https://github.com/rclone/rclone/pull/7987#issuecomment-2688580667

Specifically, I believe that TestGitAnnexFstestBackendCases/HandlesInit
should no longer fail on the Azure backend with "INITREMOTE-FAILURE
remote does not exist: TestAzureBlob,directory_markers:".

Issue #7984

cmd/gitannex/gitannex.go

@@ -274,48 +274,64 @@ func (s *server) handleInitRemote() error {
 		return fmt.Errorf("failed to get configs: %w", err)
 	}
 
-	// Explicitly check whether [server.configRcloneRemoteName] names a remote.
+	if err := validateRemoteName(s.configRcloneRemoteName); err != nil {
-	//
+		s.sendMsg(fmt.Sprintf("INITREMOTE-FAILURE %s", err))
-	// - We do not permit file paths in the remote name; that's what
+		return fmt.Errorf("failed to init remote: %w", err)
-	//   [s.configPrefix] is for. If we simply checked whether [cache.Get]
-	//   returns [fs.ErrorNotFoundInConfigFile], we would incorrectly identify
-	//   file names as valid remote names.
-	//
-	// - In order to support remotes defined by environment variables, we must
-	//   use [config.GetRemoteNames] instead of [config.FileSections].
-	trimmedName := strings.TrimSuffix(s.configRcloneRemoteName, ":")
-	if slices.Contains(config.GetRemoteNames(), trimmedName) {
-		s.sendMsg("INITREMOTE-SUCCESS")
-		return nil
 	}
 
-	// Otherwise, check whether [server.configRcloneRemoteName] is actually a
+	s.sendMsg("INITREMOTE-SUCCESS")
-	// backend string such as ":local:". These are not remote names, per se, but
+	return nil
-	// they are permitted for compatibility with [fstest]. We could guard this
+}
-	// behavior behind [testing.Testing] to prevent users from specifying
+
-	// backend strings, but there's no obvious harm in permitting it.
+// validateRemoteName validates the "rcloneremotename" config that we receive
-	maybeBackend := strings.HasPrefix(s.configRcloneRemoteName, ":")
+// from git-annex. It returns nil iff `value` is valid. Otherwise, it returns a
-	if !maybeBackend {
+// descriptive error suitable for sending back to git-annex via stdout.
-		s.sendMsg("INITREMOTE-FAILURE remote does not exist: " + s.configRcloneRemoteName)
+//
-		return fmt.Errorf("remote does not exist: %s", s.configRcloneRemoteName)
+// The value is only valid when:
+//  1. It is the exact name of an existing remote.
+//  2. It is an fspath string that names an existing remote or a backend. The
+//     string may include options, but it must not include a path. (That's what
+//     the "rcloneprefix" config is for.)
+//
+// While backends are not remote names, per se, they are permitted for
+// compatibility with [fstest]. We could guard this behavior behind
+// [testing.Testing] to prevent users from specifying backend strings, but
+// there's no obvious harm in permitting it.
+func validateRemoteName(value string) error {
+	remoteNames := config.GetRemoteNames()
+	// Check whether `value` is an exact match for an existing remote.
+	//
+	// If we checked whether [cache.Get] returns [fs.ErrorNotFoundInConfigFile],
+	// we would incorrectly identify file names as valid remote names. We also
+	// avoid [config.FileSections] because it will miss remotes that are defined
+	// by environment variables.
+	if slices.Contains(remoteNames, value) {
+		return nil
 	}
-	parsed, err := fspath.Parse(s.configRcloneRemoteName)
+	parsed, err := fspath.Parse(value)
 	if err != nil {
-		s.sendMsg("INITREMOTE-FAILURE remote could not be parsed as a backend: " + s.configRcloneRemoteName)
+		return fmt.Errorf("remote could not be parsed: %s", value)
-		return fmt.Errorf("remote could not be parsed as a backend: %s", s.configRcloneRemoteName)
 	}
 	if parsed.Path != "" {
-		s.sendMsg("INITREMOTE-FAILURE backend must not have a path: " + s.configRcloneRemoteName)
+		return fmt.Errorf("remote does not exist or incorrectly contains a path: %s", value)
-		return fmt.Errorf("backend must not have a path: %s", s.configRcloneRemoteName)
 	}
-	// Strip the leading colon and options before searching for the backend,
+	// Now that we've established `value` is an fspath string that does not
-	// i.e. search for "local" instead of ":local,description=hello:/tmp/foo".
+	// include a path component, we only need to check whether it names an
+	// existing remote or backend.
+	if slices.Contains(remoteNames, parsed.Name) {
+		return nil
+	}
+	maybeBackend := strings.HasPrefix(value, ":")
+	if !maybeBackend {
+		return fmt.Errorf("remote does not exist: %s", value)
+	}
+	// Strip the leading colon before searching for the backend. For instance,
+	// search for "local" instead of ":local". Note that `parsed.Name` already
+	// omits any config options baked into the string.
 	trimmedBackendName := strings.TrimPrefix(parsed.Name, ":")
 	if _, err = fs.Find(trimmedBackendName); err != nil {
-		s.sendMsg("INITREMOTE-FAILURE backend does not exist: " + trimmedBackendName)
 		return fmt.Errorf("backend does not exist: %s", trimmedBackendName)
 	}
-	s.sendMsg("INITREMOTE-SUCCESS")
 	return nil
 }
 

cmd/gitannex/gitannex_test.go

@@ -536,11 +536,11 @@ var fstestTestCases = []testCase{
 			require.True(t, h.server.configsDone)
 
 			h.requireWriteLine("INITREMOTE")
-			h.requireReadLineExact("INITREMOTE-FAILURE remote does not exist: thisRemoteDoesNotExist")
+			h.requireReadLineExact("INITREMOTE-FAILURE remote does not exist or incorrectly contains a path: thisRemoteDoesNotExist")
 
 			require.NoError(t, h.mockStdinW.Close())
 		},
-		expectedError: "remote does not exist: thisRemoteDoesNotExist",
+		expectedError: "remote does not exist or incorrectly contains a path: thisRemoteDoesNotExist",
 	},
 	{
 		label: "HandlesPrepareWithPathAsRemote",
@@ -567,13 +567,13 @@ var fstestTestCases = []testCase{
 			h.requireWriteLine("INITREMOTE")
 
 			require.Regexp(t,
-				regexp.MustCompile("^INITREMOTE-FAILURE remote does not exist: "),
+				regexp.MustCompile("^INITREMOTE-FAILURE remote does not exist or incorrectly contains a path: "),
 				h.requireReadLine(),
 			)
 
 			require.NoError(t, h.mockStdinW.Close())
 		},
-		expectedError: "remote does not exist:",
+		expectedError: "remote does not exist or incorrectly contains a path:",
 	},
 	{
 		label: "HandlesPrepareWithNonexistentBackendAsRemote",
@@ -640,11 +640,11 @@ var fstestTestCases = []testCase{
 			require.True(t, h.server.configsDone)
 
 			h.requireWriteLine("INITREMOTE")
-			h.requireReadLineExact("INITREMOTE-FAILURE remote could not be parsed as a backend: :local")
+			h.requireReadLineExact("INITREMOTE-FAILURE remote could not be parsed: :local")
 
 			require.NoError(t, h.mockStdinW.Close())
 		},
-		expectedError: "remote could not be parsed as a backend:",
+		expectedError: "remote could not be parsed:",
 	},
 	{
 		label: "HandlesPrepareWithBackendContainingOptionsAsRemote",
@@ -687,14 +687,38 @@ var fstestTestCases = []testCase{
 			require.True(t, h.server.configsDone)
 
 			h.requireWriteLine("INITREMOTE")
-			require.Regexp(t,
+			h.requireReadLineExact("INITREMOTE-FAILURE remote does not exist or incorrectly contains a path: :local,description=banana:/bad/path")
-				regexp.MustCompile("^INITREMOTE-FAILURE backend must not have a path: "),
+
-				h.requireReadLine(),
+			require.NoError(t, h.mockStdinW.Close())
-			)
+		},
+		expectedError: "remote does not exist or incorrectly contains a path:",
+	},
+	{
+		label: "HandlesPrepareWithRemoteContainingOptions",
+		testProtocolFunc: func(t *testing.T, h *testState) {
+			const envVar = "RCLONE_CONFIG_fake_remote_TYPE"
+			require.NoError(t, os.Setenv(envVar, "memory"))
+			t.Cleanup(func() { require.NoError(t, os.Unsetenv(envVar)) })
+
+			h.requireReadLineExact("VERSION 1")
+			h.requireWriteLine("PREPARE")
+			h.requireReadLineExact("GETCONFIG rcloneremotename")
+			h.requireWriteLine("VALUE fake_remote,banana=yes:")
+			h.requireReadLineExact("GETCONFIG rcloneprefix")
+			h.requireWriteLine("VALUE /foo")
+			h.requireReadLineExact("GETCONFIG rclonelayout")
+			h.requireWriteLine("VALUE foo")
+			h.requireReadLineExact("PREPARE-SUCCESS")
+
+			require.Equal(t, "fake_remote,banana=yes:", h.server.configRcloneRemoteName)
+			require.Equal(t, "/foo", h.server.configPrefix)
+			require.True(t, h.server.configsDone)
+
+			h.requireWriteLine("INITREMOTE")
+			h.requireReadLineExact("INITREMOTE-SUCCESS")
 
 			require.NoError(t, h.mockStdinW.Close())
 		},
-		expectedError: "backend must not have a path:",
 	},
 	{
 		label: "HandlesPrepareWithSynonyms",