Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various CLI improvements #2811

Merged
merged 8 commits into from
Feb 9, 2023
Merged

Various CLI improvements #2811

merged 8 commits into from
Feb 9, 2023

Conversation

dbnicholson
Copy link
Member

Often I want to quickly inspect local and remote repos to see what's currently in them. While it's often better to write something that uses the API to get the precise desired information, the CLI is very handy for quickly assessing the situation. This adds some options to various CLI builtins to expose or improve that information.

In all cases documentation was missing from the manual and the bash
completion was incorrectly assigning it as a boolean option.
The output is much more readable sorted. I can't think of any reason
you'd want it unsorted (which is essentially dentry order).
Allow printing the revision along with the ref. This is very convenient
for looping over the refs in a shell as well as for quickly seeing which
refs are pointed to the same commit.
The only other way to get the remote ref revision from the CLI is to
scrape the output of `ostree remote summary` or pull the commit. The
revision is already there in the summary's ref map, so might as well add
an option to show it.
While `--print-metadata-key` is very useful, it's not that helpful if
you don't know what the keys are.
Like with commit metadata, it's useful to list and print metadata keys
are in a summary file. This adds helpers to do that.
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

src/ostree/ot-builtin-refs.c Show resolved Hide resolved
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the well split-out patches!

Restarted CI. Will let @cgwalters have a look and merge.

g_autoptr(GVariant) metadata = NULL;
g_autoptr(GVariant) value = NULL;

summary = g_variant_new_from_bytes (OSTREE_SUMMARY_GVARIANT_FORMAT,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all fine as is but personally I've been trying to convert the whole codebase to declare-and-initialize because it's simply shorter, also matches how e.g. Rust works, and works better with goto out and not having implicitly nullable pointers.

I imagine you work on other codebases that still use C89 separation between declaration and init, but hopefully we can agree to keep this one using declare-and-initialize.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair. I split it for 2 reasons.

One is that I personally think sometimes it gets pretty busy with the initializer on the same line with an autoptr declaration. The other is that I have had gcc whine about using using uninitialized variables before when autoptr variables aren't explicitly initialized to NULL. Which seems like a compiler bug but there is some logic to it in certain scenarios.

Anyways, I'll be sure to do the full initialization next time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other is that I have had gcc whine about using using uninitialized variables before when autoptr variables aren't explicitly initialized to NULL.

I'm talking about declare-and-initialize, so there's no point at which it's set to NULL.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope I'm remembering this right, but I am talking about declare-and-initialize of a g_autoptr. I believe what happened is that the compiler decided that the initialization code could be run later, but then the __attribute__((cleanup(func))) code would warn that it was potentially using an uninitialized variable. As as said, likely a gcc bug since the presence of the cleanup attribute means it shouldn't split the initializer. I've only seen it a few times, though.

@cgwalters
Copy link
Member

Well, something clearly broke WRT our space checks, but it's also not related to this PR
/override continuous-integration/jenkins/pr-merge

@openshift-ci
Copy link

openshift-ci bot commented Feb 9, 2023

@cgwalters: Overrode contexts on behalf of cgwalters: continuous-integration/jenkins/pr-merge

In response to this:

Well, something clearly broke WRT our space checks, but it's also not related to this PR
/override continuous-integration/jenkins/pr-merge

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.

@cgwalters cgwalters merged commit cff0a17 into ostreedev:main Feb 9, 2023
@dbnicholson dbnicholson deleted the cli-polish branch February 13, 2023 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants