couchdb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Filipe Manana (JIRA)" <j...@apache.org>
Subject [jira] Commented: (COUCHDB-687) Add md5 hash to _attachments properties for documents
Date Wed, 22 Dec 2010 15:03:17 GMT

    [ 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.


Mime
View raw message