On Tuesday, 3 January 2017 11:18:53 CET Cédric Bosdonnat wrote:
Getting checksum involves the same code than verifying them. Create a get_checksum function and use it in verify_checksum. ---
The idea is good, although there are few issues: - you need to rebase the patch on master, since the code changed and this patch will not apply - I'd call the function as 'calculate_checksum', as fits more what it does - I don't like much the usage of csum_t as parameter, since it's a bit misleading to pass a checksum string which will be ignored; one idea to avoid duplicating code with no need to refactor much could be still use your approach internally of using a csum_t, constructing that from the string of a checksum type -- something like: let rec calculate_checksum csum_type filename = do_calculate_checksum (of_string csum_type "") and do_calculate_checksum csum filename = (* rest of your code *) calculate_checksum would be the public API (exported), while do_calculate_checksum the private implementation used by calculate_checksum and verify_checksum. Also, of_string already has the string -> csum_t mapping, including raising an error on unknown type.
diff --git a/mllib/checksums.ml b/mllib/checksums.ml index dfa8c3ae7..3efc764b9 100644 --- a/mllib/checksums.ml +++ b/mllib/checksums.ml @@ -45,23 +45,21 @@ let of_string csum_type csum_value = | "sha512" -> SHA512 csum_value | _ -> invalid_arg csum_type
-let verify_checksum csum filename = - let prog, csum_ref = - match csum with - | SHA1 c -> "sha1sum", c - | SHA256 c -> "sha256sum", c - | SHA512 c -> "sha512sum", c - in - +let get_checksum csum filename = + let prog = (string_of_csum_t csum) ^ "sum" in
I'd leave the way prog is set as it is, as it is more explicit, and will be easier to adapt in the future when I get to make this code work on non-GNU platforms (e.g. FreeBSD). Thanks, -- Pino Toscano