-
Notifications
You must be signed in to change notification settings - Fork 291
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
src/ostree: Fix split string containing multiple dots '.' #1696
Conversation
src/ostree/ot-builtin-config.c
Outdated
@@ -43,8 +43,16 @@ split_key_string (const char *k, | |||
char **out_value, | |||
GError **error) | |||
{ | |||
const char *quotes_dot = strstr(k, "\"."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, how about instead of this, we use strrchr
instead strchr
below so that we find the last period in the string? That way, we have a single path for either kind of section names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Umm, so config file can have something like below:
[remote "org.mozilla.FirefoxRepo"]
gpg-verify=true
gpg-verify-summary=true
url=https://firefox-flatpak.mojefedora.cz/repo/
xa.title=Unofficial Firefox Flatpak Repository
xa.title-is-set=true
In some cases using strrchr won't work appropriately. For example, we want to fetch value of xa.title, in this case split_key_string() will have value of k as something like remote "org.mozilla.FirefoxRepo".xa.title . Using strrchr will give as match for title instead of xa.title .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So looking at https://gitlab.gnome.org/GNOME/glib/blob/83a4cab12c2d00dbfe6013d071cff2da310109a4/glib/gkeyfile.c#L4119, it seems like group names can have almost any characters, except square brackets. If that's the case, then it seems like even ".
is not strong enough?
Here's a strawman: we support an alternative [grouname].keyname
syntax. This will allow us to completely disambiguate them. E.g. one would write ostree config get '[remote "foo.bar.baz"]'.xa.title
. What do you think?
(Note for remotes specifically, there's an open PR (#1166) that will make it more natural to change remote configs from the cmdline.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! looking for [grouname].keyname sounds good to me, will update PR with changes. Does this change require updating ostree documentation anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just the man page for the command (it's in man/ostree-config.xml
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a strawman: we support an alternative [grouname].keyname syntax.
Seems sane to me, though maybe also:
ostree config get --group 'remote "foo.bar.baz"' 'xa.title'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the --group idea, will update the PR with changes.
thanks for the update sinny.. is this ready for review? |
Ah yes, this is ready for review. Latest changes implement --group option in ostree config to pass Group Name followed by Key . |
src/ostree/ot-builtin-config.c
Outdated
@@ -85,18 +88,32 @@ ostree_builtin_config (int argc, char **argv, OstreeCommandInvocation *invocatio | |||
|
|||
if (!strcmp (op, "set")) | |||
{ | |||
if (argc < 4) | |||
if (opt_group && argc < 5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be argc < 3
? Oh wait, I see. OK, I think we want the --group
flag to take an argument. Rather than G_OPTION_ARG_NONE
, use G_OPTION_ARG_STRING
. Then the opt_group
will itself contain the group variable. Does that make sense?
Doing this should simplify a lot of the processing, particularly if we combine the argc
validation with parsing, something like:
if (opt_group)
{
if (argc < 3)
{
g_set_error (...)
goto out;
}
section = g_strdup (opt_group);
key = g_strdup (argv[2]);
value = g_strdup (argv[3]);
}
else
{
if (argc < 3)
{
g_set_error (...)
goto out;
}
section_key = argv[2];
value = argv[3];
if (!split_key_string (section_key, §ion, &key, error))
goto out;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Colin for reviewing it!
I agree that using G_OPTION_ARG_STRING makes things simpler. I have updated PR with required changes.
Fetching value from a repo config using 'ostree config get SECTIONNAME.KEYNAME' didn't work in some cases like when having dots in Group Name entry. As per Desktop entry file specification, Group Name may contain all ASCII characters except for [ and ] and control characters. Link - https://specifications.freedesktop.org/desktop-entry-spec/desktop-entry-spec-1.1.html Having --group option will help user to clearly specify Group Name and get desired result. It also adds test for ostree config get|set and bash completion for --group option Fixes ostreedev#1565
Thanks! |
☀️ Test successful - status-atomicjenkins |
❤️ - nice work @sinnykumari - thanks @cgwalters for reviewing |
Thanks @dustymabe @jlebon and @cgwalters for the help! |
This commit fixes fetching of remote repo name containing
multiple dots in config file using ostree config set|get
option.
It also adds test for ostree config get|set
Fixes #1565