GNOME Bugzilla – Bug 793597
gdbus-tool: Make --dest optional for emit again
Last modified: 2018-02-21 14:57:22 UTC
We noticed in Ubuntu that a test was failing because `gdbus emit' gained a new mandatory `--dest' parameter. I took a look at this and I think it was an unintended consequence of the fix for bug #788594. Here's a patch to make it optional again. I tried to make/keep the completion as robust as possible, so added some guards for cases which can be entered now that couldn't be before - for example don't try to complete --object-path with no --dest and --signal without both --object-path and --dest. It's a bit twisty though, so review appreciated.
Created attachment 368563 [details] [review] gdbus-tool: Make --dest optional for emit again Commit faf94409083f40ed096565b4f948852323bad697 made the bash completion more robust, but in doing so it made the optional --dest argument to `gdbus emit' mandatory by mistake. Remove the error case when --dest is not specified. To keep the completion working, we shuffle the cases around. --dest should be offered up for completion after --session/--system/--address have been supplied, so we can offer up potential arguments. Additionally, if --dest isn't specified then we can't complete --object-path or --signal, so guard these completions accordingly.
Review of attachment 368563 [details] [review]: ::: gio/gdbus-tool.c @@ +585,3 @@ o = g_option_context_new (NULL); + if (request_completion) + g_option_context_set_ignore_unknown_options (o, TRUE); Other commands have this - it's so that we parse --session/--system even when tab completing a half-finished command like --obj... If we don't get a bus due to an option parsing failure then there's an early-exit that will prevent us reaching the later completion code. Arguably this could be a separate commit and I'll split it out if you like.
Review of attachment 368563 [details] [review]: Thanks. Splitting out the commits would be good. ::: gio/gdbus-tool.c @@ +585,3 @@ o = g_option_context_new (NULL); + if (request_completion) + g_option_context_set_ignore_unknown_options (o, TRUE); > Arguably this could be a separate commit and I'll split it out if you like. Yeah, that would be good. @@ +656,3 @@ if (request_completion && g_strcmp0 ("--dest", completion_prev) == 0) { print_names (c, g_str_has_prefix (opt_emit_dest, ":")); If (opt_emit_dest == NULL) here, bad things will happen. @@ +716,3 @@ if (opt_emit_signal == NULL) { + /* don't keep repeatedly completing --signal */ Looks like this should be split out into a separate commit too, since it’s not to do with the --dest handling.
Created attachment 368707 [details] [review] gdbus-tool: Ignore unknown options for the 'emit' subcommand when completing When completing, we parse the options that the user has typed so far. Up until now we've been doing this without ignoring unknown options. This leads to broken completions when the user has typed an incomplete parameter. For example, when doing the following: $ gdbus emit --session --obj<tab> We expect --object-path to be completed, but it is currently not. What happens is that we fail to parse the options, therefore don't act on --session and so don't connect to the session bus, then we early-exit because we need to know which bus to operate on for later completions. Instead we can ignore the half-completed --obj, parse --session, get connected to the bus and then move on to the later completion code.
Created attachment 368708 [details] [review] gdbus-tool: Don't repeatedly complete --signal In this situation: $ gdbus emit --session --object-path /org/foo/bar --sig<tab><tab><tab> We will currently insert --signal three times. We should only do that once.
Created attachment 368709 [details] [review] gdbus-tool: Make --dest optional for emit again Commit faf94409083f40ed096565b4f948852323bad697 made the bash completion more robust, but in doing so it made the optional --dest argument to `gdbus emit' mandatory by mistake. Remove the error case when --dest is not specified. To keep the completion working, we shuffle the cases around. --dest should be offered up for completion after --session/--system/--address have been supplied, so we can complete its argument. Additionally, if --dest isn't specified then we can't complete --object-path or --signal, so guard these completions accordingly.
I fat fingered and attached the last two patches in the wrong order so you need to 'i' when applying and swap them around, but this should address the comments, thanks for the review.
Review of attachment 368707 [details] [review]: Thanks. I suspect this also needs to be done for the other handle_*() methods. Fancy submitting a patch for that too?
Review of attachment 368708 [details] [review]: Good, thanks.
Review of attachment 368709 [details] [review]: OK.
All pushed, thanks. The emit changes did not make it into glib-2-54, so these patches do not need to be backported. Attachment 368707 [details] pushed as 1717a8c - gdbus-tool: Ignore unknown options for the 'emit' subcommand when completing Attachment 368708 [details] pushed as e2d9884 - gdbus-tool: Don't repeatedly complete --signal Attachment 368709 [details] pushed as 2a27170 - gdbus-tool: Make --dest optional for emit again
(In reply to Philip Withnall from comment #8) > Review of attachment 368707 [details] [review] [review]: > > Thanks. I suspect this also needs to be done for the other handle_*() > methods. Fancy submitting a patch for that too? Actually, I’ll do that now, while I’m here.
Created attachment 368717 [details] [review] gdbus-tool: Factor out common GOptionContext construction In doing so, ensure that g_option_context_set_ignore_unknown_options() is always called if completion is being done. Signed-off-by: Philip Withnall <withnall@endlessm.com>
Review of attachment 368717 [details] [review]: Thanks for committing. One question on your diff, but otherwise LGTM although IANAmaintainer. ::: gio/gdbus-tool.c @@ +150,3 @@ + g_option_context_add_main_entries (o, entries, GETTEXT_PACKAGE); + + return g_steal_pointer (&o); Why the g_steal_pointer?
(In reply to Iain Lane from comment #14) > Review of attachment 368717 [details] [review] [review]: > > Thanks for committing. > > One question on your diff, but otherwise LGTM although IANAmaintainer. Your review is enough for this, thanks. > ::: gio/gdbus-tool.c > @@ +150,3 @@ > + g_option_context_add_main_entries (o, entries, GETTEXT_PACKAGE); > + > + return g_steal_pointer (&o); > > Why the g_steal_pointer? It makes no functional difference here, but it’s a coding style which I’ve been moving towards for a while. It makes the ownership transfer of the memory more explicit to anyone reading the code. In more complex situations, it makes it much harder to introduce memory errors (typically, use-after-free) when refactoring.
Ta, pushed to master. Attachment 368717 [details] pushed as 0cf523e - gdbus-tool: Factor out common GOptionContext construction