[Libguestfs] [libguestfs-common PATCH 3/3] add common ("standard") option -w / --wrap

Mon Feb 14 11:10:56 UTC 2022

On 02/12/22 19:18, Richard W.M. Jones wrote:
> On Fri, Feb 11, 2022 at 04:32:25PM +0100, Laszlo Ersek wrote:
>> Similarly to how users can can force ANSI colorization with "--colors"
>> even when stdout / stderr are redirected to a non-tty, allow them to force
>> wrapping with "--wrap".
>>
>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1820221
>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>> ---
>>
>> Notes:
>>     - This patch will require documentation updates for the following
>>       guestfs-tools utilities (those that already document "--colours"):
>>     
>>         builder/virt-builder-repository.pod
>>         builder/virt-builder.pod
>>         customize/virt-customize.pod
>>         dib/virt-dib.pod
>>         get-kernel/virt-get-kernel.pod
>>         resize/virt-resize.pod
>>         sparsify/virt-sparsify.pod
>>         sysprep/virt-sysprep.pod
>>     
>>       Virt-v2v as well:
>>     
>>         docs/virt-v2v.pod
>>     
>>       This is evident e.g. from "test-virt-customize-docs.sh" failing in
>>       guestfs-tools, and "test-v2v-docs.sh" failing in virt-v2v.
>>     
>>       If this patch is accepted, I intend to post such guestfs-tools and
>>       virt-v2v patches that atomically bump the submodule reference and
>>       update the documentation.
>>     
>>     - Some utilities in libguestfs already use the short "-w" option
>>       (guestfish, guestmount, virt-rescue); however, those utilities do not
>>       document "--colours", so they already don't make a TTY-based
>>       distinction, and will not see the new short "-w" option.
> 
> Yes, this is awkward.
> 
> Do we have to have the short version?  I'd really like to not burn
> through useful-looking single letter options such as -w -- even if not
> currently used in all tools -- for a pretty obscure feature that about
> 0.0001% of people are going to care about.  And as you say guestfish
> establishes a clear precedence for this option meaning "writable".
> 
>>     - Easily testable e.g. with virt-resize:
>>     
>>       $ guestfish -N fs:ext4 exit
>>       $ truncate -s 2G test1.resized.img
>>       $ virt-resize    --expand /dev/sda1 test1.img test1.resized.img
>>       $ virt-resize -w --expand /dev/sda1 test1.img test1.resized.img
>>       $ virt-resize    --expand /dev/sda1 test1.img test1.resized.img > log1
>>       $ virt-resize -w --expand /dev/sda1 test1.img test1.resized.img > log2
>>       $ cat log1
>>       $ cat log2
>>
>>  mlstdutils/std_utils.mli | 6 ++++--
>>  mlstdutils/std_utils.ml  | 8 ++++++--
>>  mltools/tools_utils.ml   | 3 ++-
>>  mltools/test-getopt.sh   | 2 ++
>>  4 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/mlstdutils/std_utils.mli b/mlstdutils/std_utils.mli
>> index 534caa80d6a7..87e923e24d29 100644
>> --- a/mlstdutils/std_utils.mli
>> +++ b/mlstdutils/std_utils.mli
>> @@ -376,8 +376,10 @@ val set_trace : unit -> unit
>>  val trace : unit -> bool
>>  val set_verbose : unit -> unit
>>  val verbose : unit -> bool
>> -(** Stores the colours ([--colours]), quiet ([--quiet]), trace ([-x]) and
>> -    verbose ([-v]) flags in global variables. *)
>> +val set_wrap : unit -> unit
>> +val wrap : unit -> bool
>> +(** Stores the colours ([--colours]), quiet ([--quiet]), trace ([-x]), verbose
>> +    ([-v]) and wrap ([-w]) flags in global variables. *)
>>  
>>  val with_open_in : string -> (in_channel -> 'a) -> 'a
>>  (** [with_open_in filename f] calls function [f] with [filename]
>> diff --git a/mlstdutils/std_utils.ml b/mlstdutils/std_utils.ml
>> index 58ac058c1b3a..52f44760c490 100644
>> --- a/mlstdutils/std_utils.ml
>> +++ b/mlstdutils/std_utils.ml
>> @@ -580,8 +580,8 @@ let which executable =
>>  (* Program name. *)
>>  let prog = Filename.basename Sys.executable_name
>>  
>> -(* Stores the colours (--colours), quiet (--quiet), trace (-x) and
>> - * verbose (-v) flags in a global variable.
>> +(* Stores the colours (--colours), quiet (--quiet), trace (-x), verbose (-v)
>> + * and wrap (-w) flags in a global variable.
>>   *)
>>  let colours = ref false
>>  let set_colours () = colours := true
>> @@ -599,6 +599,10 @@ let verbose = ref false
>>  let set_verbose () = verbose := true
>>  let verbose () = !verbose
>>  
>> +let wrap = ref false
>> +let set_wrap () = wrap := true
>> +let wrap () = !wrap
>> +
>>  let with_open_in filename f =
>>    let chan = open_in filename in
>>    protect ~f:(fun () -> f chan) ~finally:(fun () -> close_in chan)
>> diff --git a/mltools/tools_utils.ml b/mltools/tools_utils.ml
>> index 4c5188988e03..eb8da290eec6 100644
>> --- a/mltools/tools_utils.ml
>> +++ b/mltools/tools_utils.ml
>> @@ -126,7 +126,7 @@ let message fs =
>>  type wrap_break_t = WrapEOS | WrapSpace | WrapNL
>>  
>>  let rec wrap ?(chan = stdout) ?(indent = 0) str =
>> -  if istty chan then
>> +  if Std_utils.wrap () || istty chan then
>>      let len = String.length str in
>>      _wrap chan indent 0 0 len str
>>    else (
>> @@ -388,6 +388,7 @@ let create_standard_options argspec ?anon_fun ?(key_opts = false)
>>    add_argspec ([ S 'q'; L"quiet" ],   Getopt.Unit set_quiet,    s_"Don’t print progress messages");
>>    add_argspec ([ L"color"; L"colors";
>>                   L"colour"; L"colours" ], Getopt.Unit set_colours, s_"Use ANSI colour sequences even if not tty");
>> +  add_argspec ([ S 'w'; L"wrap" ],    Getopt.Unit set_wrap,     s_"Wrap log messages even if not tty");
> 
> ... So what I'd do is drop the -w version (keep the --wrap version of course).

Yep, I'll just drop "-w"; "--color" (and friends) are also long-only
options.

> 
>>  
>>    if key_opts then (
>>      let parse_key_selector arg =
>> diff --git a/mltools/test-getopt.sh b/mltools/test-getopt.sh
>> index d9919a9a915a..1d7bb7b1dc4f 100755
>> --- a/mltools/test-getopt.sh
>> +++ b/mltools/test-getopt.sh
>> @@ -68,6 +68,7 @@ $t --short-options | grep '^-is'
>>  $t --short-options | grep '^-t'
>>  $t --short-options | grep '^-V'
>>  $t --short-options | grep '^-v'
>> +$t --short-options | grep '^-w'
>>  $t --short-options | grep '^-x'
>>  
>>  # --long-options
>> @@ -89,6 +90,7 @@ $t --long-options | grep '^--ii'
>>  $t --long-options | grep '^--is'
>>  $t --long-options | grep '^--version'
>>  $t --long-options | grep '^--verbose'
>> +$t --long-options | grep '^--wrap'
>>  
>>  # -a/--add parameter.
>>  $t | grep '^adds = \[\]'
>> -- 
>> 2.19.1.3.g30247aa5d201
> 
> With the appropriate change to drop -w in various places in this patch:
> 
> Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
> 
> You might as well push the docs & submodule updates in the other tools
> without review.

Thank you!
Laszlo

> 
> Thanks,
> 
> Rich.
> 