[Libguestfs] [Bug 1046905] New: RFE: add argument to virt-sysprep to disable individual default operations

[Pino Toscano]

On Friday 10 January 2014 10:09:19 Richard W.M. Jones wrote:
> On Thu, Jan 09, 2014 at 03:45:54PM +0000, Richard W.M. Jones wrote:
> > On Thu, Jan 09, 2014 at 04:21:10PM +0100, Pino Toscano wrote:
> > > +  and set_operations op_string =
> > > +    let currentopset =
> > > +      match (!operations) with
> > 
> > No need for parentheses around !operations.

Fixed.

> > > +        let n = ref op_name in
> > > +        let remove = string_prefix op_name "-" in
> > > +        if remove then
> > > +          n := String.sub op_name 1 (String.length op_name - 1);
> > > +        match !n with
> > 
> > This can be written a bit more naturally as ...
> > 
> >   let n, remove =
> >     if string_prefix op_name "-" then
> >       String.sub op_name 1 (String.length op_name-1), true
> >     else
> >       op_name, false in
> >   match n with
> >   ...

This is nice even without the way below.

> An even more natural way to write this is:

Maybe for skilled OCaml programmers ;) not (yet) for me.

>   let op =
>     if string_prefix op_name "-" then
>       `Remove (String.sub op_name 1 (String.length op_name-1))
>     else
>       `Add op_name
>    match op with
> 
>    | `Add "" | `Remove "" -> (* error *)
>    | `Add "defaults" -> Sysprep_operation.add_defaults_to_set opset
>    | `Remove "defaults" -> Sysprep_operation.remove_defaults_from_set
>    | opset etc
> 
> The type of op will be [ `Add of string | `Remove of string ].
> Actually you never need to write that type out, as OCaml will infer it
> (and check it).  It will only appear in error messages if you get the
> code wrong.

Interesting way, made use of it, thanks.

> > +          let f = if remove then
> > Sysprep_operation.remove_defaults_from_set else
> > Sysprep_operation.add_defaults_to_set in
> > +          f opset
> 
> You can actually write:
> 
>   (if remove then Sysprep_operation.remove_defaults_from_set
>    else Sysprep_operation.add_defaults_to_set) opset
> 
> I don't know which way you think is clearer, but I would avoid the >80
> character lines.

Yes, I knew about that, just thought it was clearer to split the actual 
call on an own line, making the argument a bit less "hidden" by the 
whole instruction.

> > +    "--operations", Arg.String set_operations, " " ^
> > s_"Enable/disable specific operations";
> 
> I'd also add an alias "--operation" (it would just do the same thing).

Added.

> > +let add_list_to_opset l s =
> > +  List.fold_left (
> > +    fun acc elem ->
> > +      OperationSet.add elem acc
> > +  ) s l
> > +
> > +let remove_list_from_opset l set =
> > +  OperationSet.fold (
> > +    fun elem opset ->
> > +      if List.exists (
> > +        fun li ->
> > +          li.name = elem.name
> > +      ) l then
> > +        opset
> > +      else
> > +        OperationSet.add elem opset
> > +  ) set empty_set
> 
> OCaml Set has subset, intersection, difference operations if you need
> them.  See /usr/lib64/ocaml/set.mli

True; I went with manual folding to avoid build new temporary sets when 
needed, but I guess doing this could be more natural.

-- 
Pino Toscano