Richard W.M. Jones
2022-Feb-12 18:18 UTC
[Libguestfs] [libguestfs-common PATCH 3/3] add common ("standard") option -w / --wrap
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).> > 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.g30247aa5d201With 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. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
Laszlo Ersek
2022-Feb-14 11:10 UTC
[Libguestfs] [libguestfs-common PATCH 3/3] add common ("standard") option -w / --wrap
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. >
Laszlo Ersek
2022-Feb-14 17:57 UTC
[Libguestfs] [libguestfs-common PATCH 3/3] add common ("standard") option -w / --wrap
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). > >> >> 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. > > Thanks, > > Rich. >I've pushed this series as commit range 5b5fac3e0b10..41126802097f, with the short option "-w" dropped. I'll follow up with patches for the dependent projects (libguestfs, guestfs-tools, virt-v2v). Thanks! Laszlo