Sort and indent packages/package groups in rpm-ostree status

[Mershl]

Created this PR as a first step towards fixing #1613.

Fixed issues:

• By using the existing print_packages() for RemovedBasePackages and ReplacedBasePackages multiple package groups are no longer started in the same line if the next package/package group would not fit into the same starting line.
• print_packages() creates a packages_sorted array, but never actually sorts the package list.
• RemovedBasePackages and ReplacedBasePackages do no longer use the comma separator. Package groups are now either nice and short (in which case it's okay to print two in the same line), or new package groups start in a new line - which obsoletes the comma separator.

Open points:

• I removed the rpmostreecxx::maybe_shell_quote (pkg) call in print_packages() as it introduced quotes for package groups (which have spaces in it). I've not found usecases where the call is necessary. This absolutly needs a review.
• Up for discussion: Separate every RemovedBasePackages and ReplacedBasePackages package group with a newline?
• Up for discussion: See the extreme-cases below. The PR's implementation ensures that these extreme-cases will always be printed in their own line - thus the user can resize the terminal and eventually see it in a single line. Implementing an artificial line break in print_packages() would result in a nicely indented block (all lines starting behind the colon of the first column), but also hinder the layout on resize.
• initramfs::test::test_initramfs_overlay currently fails. I'll look into that tomorrow.

The following screenshots show two extreme cases with mesa and gnome-shell overrides where a single package group exceeds a single line. With that being said, it's much nicer than before, especially in the non-extreme cases and it formats nicely when the user grows the terminal horizontally.

rpm-ostree-2021.12-2.fc35.x86_64

rpm-ostree-main + sort_and_indent_rpmostree_status

[openshift-ci]

Hi @Mershl. Thanks for your PR.

I'm waiting for a coreos member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[lucab]

/ok-to-test

[cgwalters]

Hi, thank you for the patch!

Can you squash this into one commit, and follow this for commit message style: https://chris.beams.io/posts/git-commit/

I removed the rpmostreecxx::maybe_shell_quote (pkg) call in print_packages() as it introduced quotes for package groups (which have spaces in it). I've not found usecases where the call is necessary. This absolutly needs a review.

I think the rationale there is it allows copy-pasting the output back into a bash command.
I don't personally have a strong opinion on this. The logic here is...old. Dates to at least 4c2fab8 and maybe 051492b

Now, if removing it seems important, that seems like exactly the kind of thing that's best done as a separate commit.

[cgwalters]

This call deletes all the data, right? So I'm a bit confused...why are we keeping around str here at all?

[Mershl]

Consistency with the current code base. Implementations around this allocate a buffer outside of the loop, then in the loop fill, use the buffer and erase it for the next loop. Proposal: Move autoptr of the buffer into the loop scope to make it's intention clear and avoid misuse from outer scope + rename str to buf (seen this in other places of the codebase). What do you think?

[Mershl]

I'll try to clean up the string buffer handling a little bit with my next commit - the handling for ReplacedBasePackages is not "trivial". One has to see that the positive if branch ends with a truncate to size 0. I'm not 100% confident with the existing implementation of the loop containing let's just use str as a scratchpad to avoid excessive mallocs; the buf needs to be stretched anyway for the final output. I'll leave it mostly untounched for now.

[cgwalters]

follow this for commit message style: https://chris.beams.io/posts/git-commit/

The logic here is...old. Dates to at least 4c2fab8 and maybe 051492b

To combine these two things - the process of reviewing this PR involved digging up and looking at 4 year old changes. In the future, it might be someone besides you or me that needs to find this commit and understand more about why the changes were made.

Now you did include a lot of good stuff in the PR description but I don't want to totally rely on Github being around forever. Perhaps just to start:

status: Ensure lines are wrapped consistently

Rework how we print RemovedBasePackages and ReplacedBasePackages to consistently
line wrap.

or so?

[Mershl]

Squashed commits.
Split removal of maybe_shell_quote into separate commit.
Added body message to commits
Avoid existing string buffer reusage. This in turn avoids the necessary erase at the end of a loop.
-> Ready for Review.

Up for discussion: Separate every RemovedBasePackages and ReplacedBasePackages package group with a newline?

Unrelated to this PR's changes: https://github.com/coreos/rpm-ostree/blob/main/src/app/rpmostree-builtin-status.cxx#L871
This string buffer's scope can be reduced to avoid confusion/misuse. The later GLNX_HASH_TABLE_FOREACH_KV was reusing it before, under the assumption that the previous users last iteration ended with a truncation to 0 (which in my tests always happens). I've introduced a separate local string buffer for the FOREACH_KV to make the code more readable and less error prone.

[jlebon]

Hmm, instead of passing to print_packages an array of grouped packages, another option is to for it to be completely broken up, e.g. ["foo", "foo-ext", "1.2-3", "->", "1.2-4,"], etc... Then there are many more wrapping points available when outputting. Do you want to try that out? I think it'd result in a nicer output. (And yeah, we really should rename that function to just e.g. print_array or so.)

[cgwalters]

Hmm. I think on the other hand it wouldn't look as good to wrap at -> in most cases. I can see that being necessary for small terminals and/or very long package names as an alternative strategy. This problem space isn't new of course, it's basically the same problem as hyphenation.

I think this PR is enough of an improvement to merge as is - we can always circle back and do more improvements later.

Thanks for your work on this!