check mincopies is satisfied even when numcopies is known to be satisfied

I had been assuming that numcopies would be a larger or at most equal to
mincopies, so no need to check both. But users get confused and use configs
that don't really make sense, so make sure to handle mincopies being larger
than numcopies.

Also add something to the mincopies man page to discourage this
misconfiguration.

This commit was sponsored by Denis Dzyubenko on Patreon.

Annex/Drop.hs

@@ -69,7 +69,7 @@ handleDropsFrom locs rs reason fromhere key afile si preverified runner = do
 			else do
 				l <- mapM getFileNumMinCopies fs
 				return (maximum $ map fst l, maximum $ map snd l)
-		return (NumCopies (length have), numcopies, mincopies, S.fromList untrusted)
+		return (length have, numcopies, mincopies, S.fromList untrusted)
 
 	{- Check that we have enough copies still to drop the content.
 	 - When the remote being dropped from is untrusted, it was not
@@ -80,13 +80,14 @@ handleDropsFrom locs rs reason fromhere key afile si preverified runner = do
 	 - avoids doing extra work to do that check later in cases where it
 	 - will surely fail.
 	 -}
-	checkcopies (have, numcopies, _mincopies, _untrusted) Nothing = have > numcopies
-	checkcopies (have, numcopies, _mincopies, untrusted) (Just u)
-		| S.member u untrusted = have >= numcopies
-		| otherwise = have > numcopies
+	checkcopies (have, numcopies, mincopies, _untrusted) Nothing =
+		NumCopies have > numcopies && MinCopies have > mincopies
+	checkcopies (have, numcopies, mincopies, untrusted) (Just u)
+		| S.member u untrusted = NumCopies have >= numcopies && MinCopies have >= mincopies
+		| otherwise = NumCopies have > numcopies && MinCopies have > mincopies
 	
 	decrcopies (have, numcopies, mincopies, untrusted) Nothing =
-		(NumCopies (fromNumCopies have - 1), numcopies, mincopies, untrusted)
+		(have - 1, numcopies, mincopies, untrusted)
 	decrcopies v@(_have, _numcopies, _mincopies, untrusted) (Just u)
 		| S.member u untrusted = v
 		| otherwise = decrcopies v Nothing
@@ -122,7 +123,7 @@ handleDropsFrom locs rs reason fromhere key afile si preverified runner = do
 						AssociatedFile Nothing -> serializeKey key
 						AssociatedFile (Just af) -> fromRawFilePath af
 					, "(from " ++ maybe "here" show u ++ ")"
-					, "(copies now " ++ show (fromNumCopies have - 1) ++ ")"
+					, "(copies now " ++ show (have - 1) ++ ")"
 					, ": " ++ reason
 					]
 				return $ decrcopies n u

CHANGELOG

@@ -21,6 +21,10 @@ git-annex (8.20210331) UNRELEASED; urgency=medium
     adjusted branch checked out, and the origin remote is not named "origin".    
   * Fix some bugs that made git-annex not see recently recorded status
     information when configured with annex.alwayscommit=false.
+  * When mincopies is set to a larger value than numcopies, make sure that
+    mincopies is satisfied. Before, it assumed a sane configuration would
+    have numcopies larger or equal to mincopies. It's still a good idea
+    not to configure git-annex this way.
   * Fix build with persistent-2.12.0.1
 
  -- Joey Hess <id@joeyh.name>  Thu, 01 Apr 2021 12:17:26 -0400

Types/NumCopies.hs

@@ -145,9 +145,9 @@ isSafeDrop :: NumCopies -> MinCopies -> [VerifiedCopy] -> Maybe ContentRemovalLo
  - dropped from the local repo. That lock will prevent other git repos
  - that are concurrently dropping from using the local copy as a VerifiedCopy.
  - So, no additional locking is needed; all we need is verifications
- - of any kind of N other copies of the content. -}
-isSafeDrop (NumCopies n) _ l (Just (ContentRemovalLock _)) = 
-	length (deDupVerifiedCopies l) >= n
+ - of any kind of enough other copies of the content. -}
+isSafeDrop (NumCopies n) (MinCopies m) l (Just (ContentRemovalLock _)) = 
+	length (deDupVerifiedCopies l) >= max n m
 {- Dropping from a remote repo.
  -
  - To guarantee MinCopies is never violated, at least that many LockedCopy

doc/bugs/Drop_doesn__39__t_respect_mincopies.mdwn

@@ -18,3 +18,5 @@ whereis testmincopies (2 copies)
    	c8144467-348b-476d-8464-5dfe98580f0b -- patmaddox@istudo.local:~/Desktop/annex [istudo]
 ok
 ```
+
+> [[fixed|done]] --[[Joey]]

doc/bugs/Drop_doesn__39__t_respect_mincopies/comment_1_a8dd71c6645d6d1c64815a6ea20be760._comment

@@ -0,0 +1,25 @@
+[[!comment format=mdwn
+ username="joey"
+ subject="""comment 1"""
+ date="2021-04-27T17:06:45Z"
+ content="""
+Please, when filing a bug report, especially one that has the potential
+to be a major data loss bug, provide enough information to reproduce the bug
+report. This includes a version number, and ideally showing how to create a
+new repository and configure it, from scratch, so that the bug occurs.
+
+I suspect that what is happening here is, you don't have numcopies set to a
+number >= mincopies. And there's code that only checks numcopies, with the
+assumption that if it makes sure numcopies is satisfied, mincopies must be
+satisfied since it's a smaller or equal number.
+
+Of course, that's just a guess, but it was the first way I was able to
+reproduce a behavior like you show.
+
+I guess that's a bug, but it's not a major data loss bug since numcopies
+defaults to 1 it will preserve at least that many. Also, it's not a very
+wise configuration because mincopies is a new feature and if an older
+version of git-annex, that does not support it, is used with this
+repository, it will not know about mincopies and will only enforce
+numcopies.
+"""]]

doc/git-annex-mincopies.mdwn

@@ -25,6 +25,12 @@ repositories, git-annex may violate the numcopies setting.
 In these unusual situations, git-annex ensures that the number of copies
 never goes below mincopies.
 
+It is a good idea to not only rely on only setting mincopies. Set
+numcopies as well, to a larger number, and keep mincopies at the
+bare minimum you're comfortable with. Setting mincopies to a large
+number, rather than setting numcopies will in some cases prevent
+droping content in entirely safe situations.
+
 # SEE ALSO
 
 [[git-annex]](1)