[
https://issues.apache.org/jira/browse/COUCHDB-687?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12974230#action_12974230
]
Filipe Manana commented on COUCHDB-687:
---------------------------------------
Once again thanks Juuso.
My comments:
1) Except maybe for Window's notepad, most text editors allow you define a 80
characters right margin - this helps you identify which lines exceed the limit.
I still see several in the patch. I know there's existing code which exceeds
the 80 characters rule, but don't consider it as an example to follow;
2) Don't forget to test the md5 attribute remains the same after database
compaction;
3) Avoid changing necessary lines like the following for e.g.:
- ] ++
+ ]++
Or the removal of blank lines like in line 316 of the diff;
4) Try to make the 'case' indentation style the same like in the surrounding
code. E.g.:
+ IdentityMd5 = case HexIdentityMd5 of
+ undefined -> undefined;
+ _ -> couch_util:hex_to_bin(HexIdentityMd5)
+ end,
Should be:
+ IdentityMd5 = case HexIdentityMd5 of
+ undefined ->
+ undefined;
+ _ ->
+ couch_util:hex_to_bin(HexIdentityMd5)
+ end,
5) As for the hex_to_bin function, it's better to allow the parameter to be a
list as well, instead of assuming it's always a binary:
+hex_to_bin(B) when is_binary(B) ->
+ hex_to_bin(?b2l(B));
+hex_to_bin(B) when is_list(B) ->
+ hex_to_bin(B, []);
+
+hex_to_bin([], Acc) ->
+ ?l2b(lists:reverse(Acc));
+hex_to_bin([X,Y|T], Acc) ->
+ {ok, [V], []} = io_lib:fread("~16u", [X,Y]),
+ hex_to_bin(T, [V | Acc]).
Also note that it's not following the 4 spaces indentation convention we follow
in CouchDB.
6) Here for e.g.:
+ OutIdentityMd5 = case IdentityMd5 of
+ undefined -> couch_util:md5(Bin);
+ _ -> IdentityMd5
+ end,
I see inconsistent mix of spaces and tabs. Most editors allow you to
automatically replace tabs with N spaces.
7) Here:
+ OutIdentityMd5 = case {InIdentityMd5, Enc, NewEnc} of
+ {undefined,gzip,_} ->
+ % gzipped attachment without incoming value should not use anything
+ undefined;
+ {_,identity,_} ->
+ % new attachment should use calculated md5, unless attachment is
+ % compressed, updated attachment should use calculated md5 insted of
+ % existing one
+ IdentityMd5;
+ {_,gzip,_} ->
+ % compressed attachment sent through replication api should use
+ % incoming md5 when it's defined
+ InIdentityMd5
+ end,
Why the 3 tuple element? In all the case clauses you don't care about the third
element, so just drop it.
Everything else seems fine.
regards
> Add md5 hash to _attachments properties for documents
> -----------------------------------------------------
>
> Key: COUCHDB-687
> URL: https://issues.apache.org/jira/browse/COUCHDB-687
> Project: CouchDB
> Issue Type: Improvement
> Environment: CouchDB
> Reporter: mikeal
> Assignee: Filipe Manana
> Attachments: couchdb-md5-in-attachment-COUCHDB-687-v2.patch,
> couchdb-md5-in-attachment-COUCHDB-687.patch, md5.patch
>
>
> The current attachment information looks like this:
> GET /dbname/docid
> "_attachments": {
> "jquery-1.4.1.min.js": {
> "content_type": "text/javascript"
> "revpos": 138
> "length": 70844
> "stub": true
> }
> }
> If a client wanted to sync local files as attachments with a document it
> would not currently be able to do so without keeping a local store of the
> revpos. If this information included an md5 hash of the attachment clients
> could compare it against a hash of the local file to see if they match.
> -Mikeal
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
