I noticed that with the (more or less) recent series from Marc-André the
output of qemu-img amend -f qcow2 -o help changed to this:

$ ./qemu-img amend -f qcow2 -o help
Creation options for 'qcow2':
qcow2-create-opts.backing_file=str - File name of a base image
qcow2-create-opts.backing_fmt=str - Image format of the base image
qcow2-create-opts.cluster_size=size - qcow2 cluster size
qcow2-create-opts.compat=str - Compatibility level (0.10 or 1.1)
[...]

The types are a nice addition, but I didn't like having the list name
printed in every single line (in fact, the list name does not make any
sense here at all, because there already is a caption which reads
"Creation options for 'qcow2'"), and I did not like the use of '=' for
types.

In general, I don't like the robot-y appearance, which is even worse in
things like -device virtio-blk,help, which gives you this (among
other lines):

> virtio-blk-pci.iothread=link<iothread>

Sadly, there isn't much we can do about the "link<iothread>", so this
series doesn't improve on that point.

What this series does do, however, is it changes these lists not to
print the list name on every single line, but only as a caption (and for
option lists, this caption is option, because the caller may want to
print a custom caption that is more expressive -- as is the case for
qemu-img amend -o help).

Consequentially, all list items are indented by two spaces to make clear
they belong to the caption.  I can already see that some people might
disagree on having this indentation, but I like it, so I have it in this
series.

Furthermore, types are now enclosed by angle brackets, and the alignment
we originally had for descriptions is restored (although now after 24
instead of 16 characters, because every option name is now accompanied
by indentation and a type).


Thus, after this series, the amend output looks like this:

$ ./qemu-img amend -f qcow2 -o help
Creation options for 'qcow2':
  backing_file=<str>     - File name of a base image
  backing_fmt=<str>      - Image format of the base image
  cluster_size=<size>    - qcow2 cluster size
  compat=<str>           - Compatibility level (0.10 or 1.1)
[...]


virtio-blk's list presents itself like so:

$ x86_64-softmmu/qemu-system-x86_64 -device virtio-blk,help
virtio-blk-pci options:
  iothread=<link<iothread>>
  request-merging=<bool> - on/off
  secs=<uint32>
[...]


And now we even print something when there are no options:

$ x86_64-softmmu/qemu-system-x86_64 -object can-bus,help
There are no options for can-bus.

(Before this series, there just is no output.)


As a side effect, patch 1 fixes iotest 082.


v2:
- Patch 1:
  - Abandon the "$name: $type" formatting in favor of "$name=<$type>"
    [Marc-André, at least the "abandon" part]
  - Restore description alignment [Kevin]
  - Do the alignment when generating each line's GString instead of when
    printing them.  This results in less lines modified and allows the
    compiler to optimize the printf("%s\n", x) to puts(x).
- Patch 3:
  - Same changes as above, with the addition of also separating the
    description with " - " instead of enclosing it in parentheses (to
    match the other places)
  - Also, we never did align the descriptions here, so this is not
    "restore" but "introduce description alignment".
- Patch 4:
  - Same as patch 1, but again with the catch of s/restore/introduce/.


git-backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/5:[0963] [FC] 'option: Make option help nicer to read'
002/5:[----] [--] 'chardev: Indent list of chardevs'
003/5:[0008] [FC] 'qdev-monitor: Make device options help nicer'
004/5:[0007] [FC] 'object: Make option help nicer to read'
005/5:[----] [--] 'fw_cfg: Drop newline in @file description'


Max Reitz (5):
  option: Make option help nicer to read
  chardev: Indent list of chardevs
  qdev-monitor: Make device options help nicer
  object: Make option help nicer to read
  fw_cfg: Drop newline in @file description

 include/qemu/option.h      |   2 +-
 chardev/char.c             |   2 +-
 qdev-monitor.c             |  13 +-
 qemu-img.c                 |   4 +-
 util/qemu-option.c         |  32 +-
 vl.c                       |  15 +-
 tests/qemu-iotests/082.out | 956 ++++++++++++++++++-------------------
 7 files changed, 530 insertions(+), 494 deletions(-)

-- 
2.17.1

Ping.  I don't quite want the help output to change in 3.1 only to
change it to something else in 3.2.

(And at some point into freeze we have to consider just merging one of
the many simple 082 reference output "fixes", which I really don't want,
because I still consider the test "broken for a reason"(tm).)

Max


On 19.10.18 18:49, Max Reitz wrote:
> I noticed that with the (more or less) recent series from Marc-André the
> output of qemu-img amend -f qcow2 -o help changed to this:
> 
> $ ./qemu-img amend -f qcow2 -o help
> Creation options for 'qcow2':
> qcow2-create-opts.backing_file=str - File name of a base image
> qcow2-create-opts.backing_fmt=str - Image format of the base image
> qcow2-create-opts.cluster_size=size - qcow2 cluster size
> qcow2-create-opts.compat=str - Compatibility level (0.10 or 1.1)
> [...]
> 
> The types are a nice addition, but I didn't like having the list name
> printed in every single line (in fact, the list name does not make any
> sense here at all, because there already is a caption which reads
> "Creation options for 'qcow2'"), and I did not like the use of '=' for
> types.
> 
> In general, I don't like the robot-y appearance, which is even worse in
> things like -device virtio-blk,help, which gives you this (among
> other lines):
> 
>> virtio-blk-pci.iothread=link<iothread>
> 
> Sadly, there isn't much we can do about the "link<iothread>", so this
> series doesn't improve on that point.
> 
> What this series does do, however, is it changes these lists not to
> print the list name on every single line, but only as a caption (and for
> option lists, this caption is option, because the caller may want to
> print a custom caption that is more expressive -- as is the case for
> qemu-img amend -o help).
> 
> Consequentially, all list items are indented by two spaces to make clear
> they belong to the caption.  I can already see that some people might
> disagree on having this indentation, but I like it, so I have it in this
> series.
> 
> Furthermore, types are now enclosed by angle brackets, and the alignment
> we originally had for descriptions is restored (although now after 24
> instead of 16 characters, because every option name is now accompanied
> by indentation and a type).
> 
> 
> Thus, after this series, the amend output looks like this:
> 
> $ ./qemu-img amend -f qcow2 -o help
> Creation options for 'qcow2':
>   backing_file=<str>     - File name of a base image
>   backing_fmt=<str>      - Image format of the base image
>   cluster_size=<size>    - qcow2 cluster size
>   compat=<str>           - Compatibility level (0.10 or 1.1)
> [...]
> 
> 
> virtio-blk's list presents itself like so:
> 
> $ x86_64-softmmu/qemu-system-x86_64 -device virtio-blk,help
> virtio-blk-pci options:
>   iothread=<link<iothread>>
>   request-merging=<bool> - on/off
>   secs=<uint32>
> [...]
> 
> 
> And now we even print something when there are no options:
> 
> $ x86_64-softmmu/qemu-system-x86_64 -object can-bus,help
> There are no options for can-bus.
> 
> (Before this series, there just is no output.)
> 
> 
> As a side effect, patch 1 fixes iotest 082.
> 
> 
> v2:
> - Patch 1:
>   - Abandon the "$name: $type" formatting in favor of "$name=<$type>"
>     [Marc-André, at least the "abandon" part]
>   - Restore description alignment [Kevin]
>   - Do the alignment when generating each line's GString instead of when
>     printing them.  This results in less lines modified and allows the
>     compiler to optimize the printf("%s\n", x) to puts(x).
> - Patch 3:
>   - Same changes as above, with the addition of also separating the
>     description with " - " instead of enclosing it in parentheses (to
>     match the other places)
>   - Also, we never did align the descriptions here, so this is not
>     "restore" but "introduce description alignment".
> - Patch 4:
>   - Same as patch 1, but again with the catch of s/restore/introduce/.
> 
> 
> git-backport-diff against v1:
> 
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> 
> 001/5:[0963] [FC] 'option: Make option help nicer to read'
> 002/5:[----] [--] 'chardev: Indent list of chardevs'
> 003/5:[0008] [FC] 'qdev-monitor: Make device options help nicer'
> 004/5:[0007] [FC] 'object: Make option help nicer to read'
> 005/5:[----] [--] 'fw_cfg: Drop newline in @file description'
> 
> 
> Max Reitz (5):
>   option: Make option help nicer to read
>   chardev: Indent list of chardevs
>   qdev-monitor: Make device options help nicer
>   object: Make option help nicer to read
>   fw_cfg: Drop newline in @file description
> 
>  include/qemu/option.h      |   2 +-
>  chardev/char.c             |   2 +-
>  qdev-monitor.c             |  13 +-
>  qemu-img.c                 |   4 +-
>  util/qemu-option.c         |  32 +-
>  vl.c                       |  15 +-
>  tests/qemu-iotests/082.out | 956 ++++++++++++++++++-------------------
>  7 files changed, 530 insertions(+), 494 deletions(-)
> 

Am 19.10.2018 um 18:49 hat Max Reitz geschrieben:
> I noticed that with the (more or less) recent series from Marc-André the
> output of qemu-img amend -f qcow2 -o help changed to this:
> 
> $ ./qemu-img amend -f qcow2 -o help
> Creation options for 'qcow2':
> qcow2-create-opts.backing_file=str - File name of a base image
> qcow2-create-opts.backing_fmt=str - Image format of the base image
> qcow2-create-opts.cluster_size=size - qcow2 cluster size
> qcow2-create-opts.compat=str - Compatibility level (0.10 or 1.1)
> [...]
> 
> The types are a nice addition, but I didn't like having the list name
> printed in every single line (in fact, the list name does not make any
> sense here at all, because there already is a caption which reads
> "Creation options for 'qcow2'"), and I did not like the use of '=' for
> types.
> 
> In general, I don't like the robot-y appearance, which is even worse in
> things like -device virtio-blk,help, which gives you this (among
> other lines):
> 
> > virtio-blk-pci.iothread=link<iothread>
> 
> Sadly, there isn't much we can do about the "link<iothread>", so this
> series doesn't improve on that point.
> 
> What this series does do, however, is it changes these lists not to
> print the list name on every single line, but only as a caption (and for
> option lists, this caption is option, because the caller may want to
> print a custom caption that is more expressive -- as is the case for
> qemu-img amend -o help).
> 
> Consequentially, all list items are indented by two spaces to make clear
> they belong to the caption.  I can already see that some people might
> disagree on having this indentation, but I like it, so I have it in this
> series.
> 
> Furthermore, types are now enclosed by angle brackets, and the alignment
> we originally had for descriptions is restored (although now after 24
> instead of 16 characters, because every option name is now accompanied
> by indentation and a type).
> 
> 
> Thus, after this series, the amend output looks like this:
> 
> $ ./qemu-img amend -f qcow2 -o help
> Creation options for 'qcow2':
>   backing_file=<str>     - File name of a base image
>   backing_fmt=<str>      - Image format of the base image
>   cluster_size=<size>    - qcow2 cluster size
>   compat=<str>           - Compatibility level (0.10 or 1.1)
> [...]
> 
> 
> virtio-blk's list presents itself like so:
> 
> $ x86_64-softmmu/qemu-system-x86_64 -device virtio-blk,help
> virtio-blk-pci options:
>   iothread=<link<iothread>>
>   request-merging=<bool> - on/off
>   secs=<uint32>
> [...]
> 
> 
> And now we even print something when there are no options:
> 
> $ x86_64-softmmu/qemu-system-x86_64 -object can-bus,help
> There are no options for can-bus.
> 
> (Before this series, there just is no output.)
> 
> 
> As a side effect, patch 1 fixes iotest 082.

Thanks, applied to the block branch.

Kevin

On 05.11.18 15:18, Kevin Wolf wrote:
> Am 19.10.2018 um 18:49 hat Max Reitz geschrieben:
>> I noticed that with the (more or less) recent series from Marc-André the
>> output of qemu-img amend -f qcow2 -o help changed to this:
>>
>> $ ./qemu-img amend -f qcow2 -o help
>> Creation options for 'qcow2':
>> qcow2-create-opts.backing_file=str - File name of a base image
>> qcow2-create-opts.backing_fmt=str - Image format of the base image
>> qcow2-create-opts.cluster_size=size - qcow2 cluster size
>> qcow2-create-opts.compat=str - Compatibility level (0.10 or 1.1)
>> [...]
>>
>> The types are a nice addition, but I didn't like having the list name
>> printed in every single line (in fact, the list name does not make any
>> sense here at all, because there already is a caption which reads
>> "Creation options for 'qcow2'"), and I did not like the use of '=' for
>> types.
>>
>> In general, I don't like the robot-y appearance, which is even worse in
>> things like -device virtio-blk,help, which gives you this (among
>> other lines):
>>
>>> virtio-blk-pci.iothread=link<iothread>
>>
>> Sadly, there isn't much we can do about the "link<iothread>", so this
>> series doesn't improve on that point.
>>
>> What this series does do, however, is it changes these lists not to
>> print the list name on every single line, but only as a caption (and for
>> option lists, this caption is option, because the caller may want to
>> print a custom caption that is more expressive -- as is the case for
>> qemu-img amend -o help).
>>
>> Consequentially, all list items are indented by two spaces to make clear
>> they belong to the caption.  I can already see that some people might
>> disagree on having this indentation, but I like it, so I have it in this
>> series.
>>
>> Furthermore, types are now enclosed by angle brackets, and the alignment
>> we originally had for descriptions is restored (although now after 24
>> instead of 16 characters, because every option name is now accompanied
>> by indentation and a type).
>>
>>
>> Thus, after this series, the amend output looks like this:
>>
>> $ ./qemu-img amend -f qcow2 -o help
>> Creation options for 'qcow2':
>>   backing_file=<str>     - File name of a base image
>>   backing_fmt=<str>      - Image format of the base image
>>   cluster_size=<size>    - qcow2 cluster size
>>   compat=<str>           - Compatibility level (0.10 or 1.1)
>> [...]
>>
>>
>> virtio-blk's list presents itself like so:
>>
>> $ x86_64-softmmu/qemu-system-x86_64 -device virtio-blk,help
>> virtio-blk-pci options:
>>   iothread=<link<iothread>>
>>   request-merging=<bool> - on/off
>>   secs=<uint32>
>> [...]
>>
>>
>> And now we even print something when there are no options:
>>
>> $ x86_64-softmmu/qemu-system-x86_64 -object can-bus,help
>> There are no options for can-bus.
>>
>> (Before this series, there just is no output.)
>>
>>
>> As a side effect, patch 1 fixes iotest 082.
> 
> Thanks, applied to the block branch.

Thank you both for the quick pong. :-)

Max