external: nice error message for keys with spaces in their name

External special remotes will refuse to operate on keys with spaces in
their names. That has never worked correctly due to the design of the
external special remote protocol. Display an error message suggesting
migration.

Not super happy with this, but it's a pragmatic solution. Better than
complicating the external special remote interface and all external special
remotes.

Note that I only made it use SafeKey in Request, not Response. git-annex
does not construct a Response, so that would not add any safety. And
presumably, if git-annex avoids feeding any such keys to an external
special remote, it will never have a reason to make a Response using such a
key. If it did, it would result in a protocol error anyway.

There's still a Serializeable instance for Key; it's used by P2P.Protocol.
There, the Key is always in the final position, so it's ok if it contains
spaces.

Note that the protocol documentation has been fixed to say that the File
may contain spaces. One way that can happen, even though the Key can't,
is when using direct mode, and the work tree filename contains spaces.
When sending such a file to the external special remote the worktree
filename is used.

This commit was sponsored by Thom May on Patreon.

5 files changed, 62 insertions, 11 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 91a1e3488..bb3a73a1c 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -23,6 +23,14 @@ git-annex (6.20170521) UNRELEASED; urgency=medium
     external special remote protocol.
   * migrate: WORM keys containing spaces will be migrated to not contain
     spaces anymore.
+  * External special remotes will refuse to operate on keys with spaces in
+    their names. That has never worked correctly due to the design of the
+    external special remote protocol. Display an error message suggesting
+    migration.
+  * Fix incorrect external special remote documentation, which said that
+    the filename parameter to the TRANSFER command could not contain
+    spaces. It can in fact contain spaces. Special remotes implementors
+    that relied on that may need to fix bugs in their special remotes.
 
  -- Joey Hess <id@joeyh.name>  Sat, 17 Jun 2017 13:02:24 -0400
 
diff --git a/Remote/External.hs b/Remote/External.hs
index 0ac381b8c..32b95e9bb 100644
--- a/Remote/External.hs
+++ b/Remote/External.hs
@@ -134,7 +134,7 @@ externalSetup _ mu _ c gc = do
 
 store :: External -> Storer
 store external = fileStorer $ \k f p ->
-	handleRequest external (TRANSFER Upload k f) (Just p) $ \resp ->
+	handleRequestKey external (\sk -> TRANSFER Upload sk f) k (Just p) $ \resp ->
 		case resp of
 			TRANSFER_SUCCESS Upload k' | k == k' ->
 				Just $ return True
@@ -146,7 +146,7 @@ store external = fileStorer $ \k f p ->
 
 retrieve :: External -> Retriever
 retrieve external = fileRetriever $ \d k p -> 
-	handleRequest external (TRANSFER Download k d) (Just p) $ \resp ->
+	handleRequestKey external (\sk -> TRANSFER Download sk d) k (Just p) $ \resp ->
 		case resp of
 			TRANSFER_SUCCESS Download k'
 				| k == k' -> Just $ return ()
@@ -156,7 +156,7 @@ retrieve external = fileRetriever $ \d k p ->
 
 remove :: External -> Remover
 remove external k = safely $ 
-	handleRequest external (REMOVE k) Nothing $ \resp ->
+	handleRequestKey external REMOVE k Nothing $ \resp ->
 		case resp of
 			REMOVE_SUCCESS k'
 				| k == k' -> Just $ return True
@@ -169,7 +169,7 @@ remove external k = safely $
 checkKey :: External -> CheckPresent
 checkKey external k = either giveup id <$> go
   where
-	go = handleRequest external (CHECKPRESENT k) Nothing $ \resp ->
+	go = handleRequestKey external CHECKPRESENT k Nothing $ \resp ->
 		case resp of
 			CHECKPRESENT_SUCCESS k'
 				| k' == k -> Just $ return $ Right True
@@ -180,7 +180,7 @@ checkKey external k = either giveup id <$> go
 			_ -> Nothing
 
 whereis :: External -> Key -> Annex [String]
-whereis external k = handleRequest external (WHEREIS k) Nothing $ \resp -> case resp of
+whereis external k = handleRequestKey external WHEREIS k Nothing $ \resp -> case resp of
 	WHEREIS_SUCCESS s -> Just $ return [s]
 	WHEREIS_FAILURE -> Just $ return []
 	UNSUPPORTED_REQUEST -> Just $ return []
@@ -212,6 +212,11 @@ handleRequest external req mp responsehandler =
 	withExternalState external $ \st -> 
 		handleRequest' st external req mp responsehandler
 
+handleRequestKey :: External -> (SafeKey -> Request) -> Key -> Maybe MeterUpdate -> (Response -> Maybe (Annex a)) -> Annex a
+handleRequestKey external mkreq k mp responsehandler = case mkSafeKey k of
+	Right sk -> handleRequest external (mkreq sk) mp responsehandler
+	Left e -> giveup e
+
 handleRequest' :: ExternalState -> External -> Request -> Maybe MeterUpdate -> (Response -> Maybe (Annex a)) -> Annex a
 handleRequest' st external req mp responsehandler
 	| needsPREPARE req = do
diff --git a/Remote/External/Types.hs b/Remote/External/Types.hs
index ef8724ee7..cda934220 100644
--- a/Remote/External/Types.hs
+++ b/Remote/External/Types.hs
@@ -18,6 +18,8 @@ module Remote.External.Types (
 	Proto.Sendable(..),
 	Proto.Receivable(..),
 	Request(..),
+	SafeKey,
+	mkSafeKey,
 	needsPREPARE,
 	Response(..),
 	RemoteRequest(..),
@@ -36,11 +38,13 @@ import Types.Transfer (Direction(..))
 import Config.Cost (Cost)
 import Types.Remote (RemoteConfig)
 import Types.Availability (Availability(..))
+import Types.Key
 import Utility.Url (URLString)
 import qualified Utility.SimpleProtocol as Proto
 
 import Control.Concurrent.STM
 import Network.URI
+import Data.Char
 
 data External = External
 	{ externalType :: ExternalType
@@ -77,6 +81,29 @@ type PID = Int
 
 data PrepareStatus = Unprepared | Prepared | FailedPrepare ErrorMsg
 
+-- The protocol does not support keys with spaces in their names;
+-- SafeKey can only be constructed for keys that are safe to use with the
+-- protocol.
+newtype SafeKey = SafeKey Key
+	deriving (Show)
+
+mkSafeKey :: Key -> Either String SafeKey
+mkSafeKey k 
+	| any isSpace (keyName k) = Left $ concat
+		[ "Sorry, this file cannot be stored on an external special remote because its key's name contains a space. "
+		, "To avoid this problem, you can run: git-annex migrate --backend="
+		, formatKeyVariety (keyVariety k)
+		, " and pass it the name of the file"
+		]
+	| otherwise = Right (SafeKey k)
+
+fromSafeKey :: SafeKey -> Key
+fromSafeKey (SafeKey k) = k
+
+instance Proto.Serializable SafeKey where
+	serialize = Proto.serialize . fromSafeKey
+	deserialize = fmap SafeKey . Proto.deserialize
+
 -- Messages that can be sent to the external remote to request it do something.
 data Request 
 	= PREPARE 
@@ -85,10 +112,10 @@ data Request
 	| GETAVAILABILITY
 	| CLAIMURL URLString
 	| CHECKURL URLString
-	| TRANSFER Direction Key FilePath
-	| CHECKPRESENT Key
-	| REMOVE Key
-	| WHEREIS Key
+	| TRANSFER Direction SafeKey FilePath
+	| CHECKPRESENT SafeKey
+	| REMOVE SafeKey
+	| WHEREIS SafeKey
 	deriving (Show)
 
 -- Does PREPARE need to have been sent before this request?
diff --git a/doc/bugs/external_special_remote_protocol_broken_by_key_with_spaces.mdwn b/doc/bugs/external_special_remote_protocol_broken_by_key_with_spaces.mdwn
index a5e45b108..4180678c9 100644
--- a/doc/bugs/external_special_remote_protocol_broken_by_key_with_spaces.mdwn
+++ b/doc/bugs/external_special_remote_protocol_broken_by_key_with_spaces.mdwn
@@ -3,3 +3,14 @@ external special remote protocol, which uses whitespace to separate the key
 parameter from subsequent parameters.
 
 I think that this only causes problems for WORM keys. --[[Joey]]
+
+> Ok, went with my last approach, rather than complicating all special
+> remotes due to this problem, we'll deprecate WORM keys with spaces in
+> their name, and provide a migratipon path.
+> 
+> The error message looks like this:
+
+> > Sorry, this file cannot be stored on an external special remote because its key's name contains a space. To avoid this problem, you can run: git-annex migrate --backend=WORM
+
+> This is fixed as well as is feasible, so while that kind of sucks,
+> calling it [[done]]. --[[Joey]] 
diff --git a/doc/design/external_special_remote_protocol.mdwn b/doc/design/external_special_remote_protocol.mdwn
index 8cb7ed3b6..87a838bd4 100644
--- a/doc/design/external_special_remote_protocol.mdwn
+++ b/doc/design/external_special_remote_protocol.mdwn
@@ -106,8 +106,8 @@ The following requests *must* all be supported by the special remote.
 * `TRANSFER STORE|RETRIEVE Key File`  
   Requests the transfer of a key. For STORE, the File is the file to upload;
   for RETRIEVE the File is where to store the download.  
-  Note that the File should not influence the filename used on the remote.
-  The filename will not contain any whitespace.  
+  Note that the File should not influence the filename used on the remote.  
+  Note that in some cases, the File may contain whitespace.  
   Note that it's important that, while a Key is being stored, CHECKPRESENT
   not indicate it's present until all the data has been transferred.  
 * `CHECKPRESENT Key`