[GitHub] davisp commented on a change in pull request #1470: Fix calculation of external size for attachments

[GitBox <gi...@apache.org>]

davisp commented on a change in pull request #1470: Fix calculation of external size for attachments
URL: https://github.com/apache/couchdb/pull/1470#discussion_r233066088
 
 

 ##########
 File path: src/couch/src/couch_db.erl
 ##########
 @@ -914,7 +914,18 @@ prep_and_validate_update(Db, #doc{id=Id,revs={RevStart, Revs}}=Doc,
         % new doc, and we have existing revs.
         % reuse existing deleted doc
         if OldFullDocInfo#full_doc_info.deleted orelse AllowConflict ->
-            {validate_doc_update(Db, Doc, fun() -> nil end), Doc};
+            case couch_doc:has_stubs(Doc) of
+                true ->
+                    #full_doc_info{rev_tree=RevTree} = OldFullDocInfo,
+                    Leafs = couch_key_tree:get_all_leafs(RevTree),
+                    [{Leaf, DiskRevs} | _] = lists:reverse(Leafs),
 
 Review comment:
   I can't convince myself that this is correct. Is there some sort of guarantee that we're getting on the order of leaves returned by `get_all_leafs/1`? If not it seems like we need be looking at adding a sort based on `{Depth, Rev}` for the returned leaves. Though that's also assuming that we're only allowing the un-deletion of the latest rev. 
   
   The test here would be when the deleted doc has multiple deleted conflicts of which only some have the required missing attachments. Given that the new doc comes in without a revision it seems like we'd need to pick a deterministic one? Though that confuses me on when a different deleted leaf revision would satisfy the attachment conditions...

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services