fix migration bug and make fsck warn

* migrate: Fix bug in migration between eg SHA256 and SHA256E,
  that caused the extension to be included in SHA256 keys,
  and omitted from SHA256E keys.
  (Bug introduced in version 6.20170214)
* migrate: Check for above bug when migrating from SHA256 to SHA256
  (and same for SHA1 to SHA1 etc), and remove the extension that should
  not be in the SHA256 key.
* fsck: Detect and warn when keys need an upgrade, either to fix up
  from the above migrate bug, or to add missing size information
  (a long ago transition), or because of a few other past key related
  bugs.

This commit was sponsored by Henrik Riomar on Patreon.

Backend/Hash.hs

@@ -150,26 +150,41 @@ validInExtension c
 	| c == '.' = True
 	| otherwise = False
 
-{- Upgrade keys that have the \ prefix on their sha due to a bug, or
- - that contain non-alphanumeric characters in their extension. -}
+{- Upgrade keys that have the \ prefix on their hash due to a bug, or
+ - that contain non-alphanumeric characters in their extension.
+ -
+ - Also, for a while migrate from eg SHA256E to SHA256 resulted in a SHA256
+ - key that contained an extension inside its keyName. Upgrade those
+ - keys, removing the extension.
+ -}
 needsUpgrade :: Key -> Bool
-needsUpgrade key = "\\" `isPrefixOf` keyHash key ||
-	any (not . validInExtension) (takeExtensions $ keyName key)
+needsUpgrade key = or
+	[ "\\" `isPrefixOf` keyHash key
+	, any (not . validInExtension) (takeExtensions $ keyName key)
+	, not (hasExt (keyVariety key)) && keyHash key /= keyName key
+	]
 
 trivialMigrate :: Key -> Backend -> AssociatedFile -> Maybe Key
 trivialMigrate oldkey newbackend afile
 	{- Fast migration from hashE to hash backend. -}
-	| migratable && hasExt newvariety = Just $ oldkey
+	| migratable && hasExt oldvariety = Just $ oldkey
 		{ keyName = keyHash oldkey
 		, keyVariety = newvariety
 		}
 	{- Fast migration from hash to hashE backend. -}
-	| migratable && hasExt oldvariety = case afile of
+	| migratable && hasExt newvariety = case afile of
 		AssociatedFile Nothing -> Nothing
 		AssociatedFile (Just file) -> Just $ oldkey
 			{ keyName = keyHash oldkey ++ selectExtension file
 			, keyVariety = newvariety
 			}
+	{- Upgrade to fix bad previous migration that created a
+	 - non-extension preserving key, with an extension
+	 - in its keyName. -}
+	| newvariety == oldvariety && not (hasExt oldvariety) &&
+		keyHash oldkey /= keyName oldkey = Just $ oldkey
+			{ keyName = keyHash oldkey
+			}
 	| otherwise = Nothing
   where
 	migratable = oldvariety /= newvariety 

CHANGELOG

@@ -10,6 +10,17 @@ git-annex (6.20180510) UNRELEASED; urgency=medium
   * copy: --force no longer does anything.
   * Prevent haskell http-client from decompressing gzip files, so downloads
     of such files works the same as it used to with wget and curl.
+  * migrate: Fix bug in migration between eg SHA256 and SHA256E, 
+    that caused the extension to be included in SHA256 keys,
+    and omitted from SHA256E keys.
+    (Bug introduced in version 6.20170214)
+  * migrate: Check for above bug when migrating from SHA256 to SHA256
+    (and same for SHA1 to SHA1 etc), and remove the extension that should
+    not be in the SHA256 key.
+  * fsck: Detect and warn when keys need an upgrade, either to fix up
+    from the above migrate bug, or to add missing size information
+    (a long ago transition), or because of a few other past key related
+    bugs.
 
  -- Joey Hess <id@joeyh.name>  Mon, 14 May 2018 13:42:41 -0400
 

Command/Fsck.hs

@@ -129,6 +129,7 @@ perform key file backend numcopies = do
 		, verifyWorkTree key file
 		, checkKeySize key keystatus ai
 		, checkBackend backend key keystatus afile
+		, checkKeyUpgrade backend key ai afile
 		, checkKeyNumCopies key afile numcopies
 		]
   where
@@ -409,6 +410,31 @@ checkKeySizeOr bad key file ai = case keySize key of
 			, msg
 			]
 
+{- Check for keys that are upgradable.
+ -
+ - Warns and suggests the user migrate, but does not migrate itself,
+ - because migration can cause more disk space to be used, and makes
+ - worktree changes that need to be committed.
+ -}
+checkKeyUpgrade :: Backend -> Key -> ActionItem -> AssociatedFile -> Annex Bool
+checkKeyUpgrade backend key ai (AssociatedFile (Just file)) =
+	case Types.Backend.canUpgradeKey backend of
+		Just a | a key -> do
+			warning $ concat
+				[ actionItemDesc ai key
+				, ": Can be upgraded to an improved key format. "
+				, "You can do so by running: git annex migrate --backend="
+				, formatKeyVariety (keyVariety key) ++ " "
+				, file
+				]
+			return True
+		_ -> return True
+checkKeyUpgrade _ _ _ (AssociatedFile Nothing) =
+	-- Don't suggest migrating without a filename, because
+	-- while possible to do, there is no actual benefit from
+	-- doing that in this situation.
+	return True
+
 {- Runs the backend specific check on a key's content object.
  -
  - When a file is unlocked, it may be a hard link to the object,

doc/bugs/migrating_to_non-E_backend_still_adds_extensions.mdwn

@@ -46,3 +46,4 @@ lrwxrwxrwx 1 grawity grawity 201 May 22 08:17 EN_EXCH2003_ENT.ISO -> ../../../.g
 ### Have you had any luck using git-annex before? (Sometimes we get tired of reading bug reports all day and a lil' positive end note does wonders)
 
 
+> [[fixed|done]] --[[Joey]]

doc/bugs/migrating_to_non-E_backend_still_adds_extensions/comment_2_f9b73dc8344b5ca98257a0024e097535._comment

@@ -0,0 +1,27 @@
+[[!comment format=mdwn
+ username="joey"
+ subject="""comment 2"""
+ date="2018-05-23T17:18:35Z"
+ content="""
+The bug was that it got the test backwards for whether 
+it was supposed to be adding or removing the extension!
+
+Bug was introduced in [[!commit 9c4650358ca85a298b747bb897dbf4f8f891fa22]]
+over a year ago.
+
+The bogus SHA256 key with an extension tacked on at the end
+passes fsck because the code happens to always strip extensions from
+hashes, even if the key type is not supposed to include an extension.
+
+Fixed the bug. But this leaves the potential for these badly formed
+SHA256 keys with an extension on the end being in a repository and
+needing to keep code working for them. (The SHA256E keys without an
+extension that also result from the bug are technically not badly formed.)
+
+So, I also made migrate fix those badly formed keys. You have to specify
+--backend=SHA256, and then it will migrate the badly formed SHA256 key to
+a correctly formed SHA256 key.
+
+Also, git-annex fsck will now warn when it detects a key needing such a
+migration.
+"""]]