Re: [Libguestfs] [PATCH v12 3/3] New tool: virt-builder-repository

On Mon, Nov 13, 2017 at 02:56:12PM +0100, Cédric Bosdonnat wrote: This is still a bit difficult to understand. How about this explicit loop? let checksums_get_sha512 = function | None -> None | Some csums -> let rec loop = function | [] -> None | Checksums.SHA512 csum :: _ -> Some csum | _ :: rest -> loop rest in loop csums It doesn't really matter for this, but there is a nice way to write a generic "memoize" higher-order function: https://stackoverflow.com/questions/14454981/memoization-in-ocaml (You wouldn't need the recursive variant here) Don't need parens around function application. You don't need %! here because the ‘info’ function calls ‘print_newline’ from stdlib and ‘print_newline’ calls ‘flush stdout’: https://github.com/libguestfs/libguestfs/blob/a88385add653c4fc2592639d72b... https://github.com/ocaml/ocaml/blob/c5fe6932b2151d0e4426072b4df3510318bc4... I read the code of run_command but I don't think it closes outfd, and so outfd would be leaked here. Also it'd be nice to use with_openfile, if it was upstream. https://www.redhat.com/archives/libguestfs/2017-November/msg00028.html Pino ^ x 2 ? Don't need parens. You don't need parens here. If you define Index.string_of_arch in the earlier then you can remove the definition of string_of_arch here as well. Don't need parens. Since commit c7651744da45 you can now write this as: let { format; size } = get_disk_image_info filepath in Seems quite a long way of writing: let ask ?default ?values message = printf "%s" message; (match default with | None -> () | Some x -> printf " [%s] " x); (match values with | None -> () | Some x -> printf (f_"Choose ... etc ..)); let value = read_line () in ... I don't think you need to flush the channel because stdout is line buffered, and read_line will flush stdout anyway. It's not wrong to use Str, but you might want to use PCRE instead (see common/mlpcre) since Perl regexps are more familiar for most people. This could be written as: let re_valid_id = PCRE.compile ~anchored:true "[-a-zA-Z0-9_.]+" ... if not (PCRE.matches re_valid_id id) then ( etc. Note that Str regexps are anchored by default but PCRE regexps are not. It seems as if the ‘ask’ function returns ‘default’ if the user just hits return already. Use open_guestfs from Tools_utils instead of this line. You can just write this with the same meaning: | Some (_, { Index.printable_name }) -> [etc] This can be written more concisely as: (id, { Index.printable_name; osinfo; file_uri = Filename.basename out_path; arch; signature_uri = None; checksums = Some [checksum]; revision; [etc] This can be written as: let _, { Index.checksums } = file_entry in You don't need parens here. You can now write this as: let _, { Index.checksums } = and: fun (_, { Index.file_uri }) -> How about: List.filter ( fun file -> is_supported_format (cmdline.repo // file) && is_new file ) files Be nice to use with_open_out here, if that patch was upstream. Pino ^ ? You can write: fun (id, { Index.arch; Index.file_uri }) -> let { Index.file_uri } = entry in We normally keep the index files even if we have a signed version. Not sure which is better actually. Maybe we should delete them. The test should ‘rm’ any temporary files it created after a successful run (but not on error paths). s/version/revision/ Looks good in general. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW