Bug 29936

Created attachment 78258 [details] [review]
0001-Don-t-spawn-man-for-help.patch

I think the overall cleanest way to do this is to migrate to GOptionContext, which gives us --help almost for free (and overall more functionality in the same number of lines).  This series does that.

Created attachment 78259 [details] [review]
0002-Set-locale-before-processing-command-line-options.patch

This is split mainly to highlight that right now polkit has almost no i18n.  

This patch set starts marking more strings for translation; if the lack of i18n is intentional, let's discuss that.

Created attachment 78260 [details] [review]
0003-Fix-package-version-bug-report-address-mixing.patch

Created attachment 78261 [details] [review]
0004-Add-bug-reporting-address-and-home-page-to-help-outp.patch

Created attachment 78262 [details] [review]
0005-Refuse-unrecognized-command-line-operands.patch

Might break some (incorrect) users.

Created attachment 78263 [details] [review]
0006-Exit-pkaction-with-status-0-on-success.patch

(Not strictly related to the rest of the series.)

Comment on attachment 78258 [details] [review]
0001-Don-t-spawn-man-for-help.patch

Review of attachment 78258 [details] [review]:
-----------------------------------------------------------------

This patch is a nice cleanup.  Looking at what systemd does, it hardcodes the --help output, and will auto-spawn a pager.  I guess we could try to adapt that code if we wanted to match, but that can be for a later patch.

Just a few minor nits, otherwise looks good:

::: src/programs/pkaction.c
@@ +94,5 @@
>    gboolean opt_verbose;
> +  GOptionEntry options[] =
> +    {
> +      {
> +	"action-id", 'a', 0, G_OPTION_ARG_STRING, &action_id,

"opt_action_id" for consistency?

::: src/programs/pkttyagent.c
@@ +36,5 @@
>    gboolean opt_fallback = FALSE;
> +  gchar *opt_process = NULL;
> +  gchar *opt_system_bus_name = NULL;
> +  gint notify_fd = -1;
> +  GOptionEntry opt_options[] =

Nitpick: opt_options seems overly redundant.  How about just "options" like elsewhere?

@@ +43,5 @@
> +	"fallback", 0, 0, G_OPTION_ARG_NONE, &opt_fallback,
> +	N_("Don't replace existing agent if any"), NULL
> +      },
> +      {
> +	"notify-fd", 0, 0, G_OPTION_ARG_INT, &notify_fd,

opt_notify_fd

Created attachment 78941 [details] [review]
0001-Don-t-spawn-man-for-help.patch

(In reply to comment #7)
Thanks for the review.  All fixed - I went for a minimal patch, but the cleanup is well worth it.

Created attachment 78942 [details] [review]
0004-Add-bug-reporting-address-and-home-page-to-help-outp.patch

No change, just rebased to apply on top of the updated 0001.

(In reply to comment #7)
> This patch is a nice cleanup.  Looking at what systemd does, it hardcodes
> the --help output, and will auto-spawn a pager.  I guess we could try to
> adapt that code if we wanted to match, but that can be for a later patch.

GNU coding standards, the most pervasive HIG for CLI available, are opposed to auto-paging on TTYs (info standards 'Program Behavior' 'User Interfaces').  Yes, git and systemd have decided to ignore them...

Comment on attachment 78260 [details] [review]
0003-Fix-package-version-bug-report-address-mixing.patch

Review of attachment 78260 [details] [review]:
-----------------------------------------------------------------

Minor comment, patch looks good in current form as well.

::: configure.ac
@@ +1,5 @@
>  dnl Process this file with autoconf to produce a configure script.
>  
>  AC_PREREQ(2.59c)
> +AC_INIT([polkit], [0.111], [http://lists.freedesktop.org/mailman/listinfo/polkit-devel])
> +AM_INIT_AUTOMAKE([])

Possibly add some bits like:

[foreign 1.11]

I always use foreign since not doing so complains about ChangeLog and other nonsense.

Comment on attachment 78262 [details] [review]
0005-Refuse-unrecognized-command-line-operands.patch

Review of attachment 78262 [details] [review]:
-----------------------------------------------------------------

Looks correct.

Comment on attachment 78263 [details] [review]
0006-Exit-pkaction-with-status-0-on-success.patch

Review of attachment 78263 [details] [review]:
-----------------------------------------------------------------

Yes.

Comment on attachment 78942 [details] [review]
0004-Add-bug-reporting-address-and-home-page-to-help-outp.patch

Review of attachment 78942 [details] [review]:
-----------------------------------------------------------------

Sure.

Thanks for the review; I have pushed all but 0002, and opened bug 64551 about the internationalization to eventually discuss separately.

Created attachment 105344 [details] [review]
0001-Fix-text-in-pkcheck-help.patch

Actually, the text for (pkcheck --help) is overwhelmingly wrong.  Sorry, I have no idea how I could be so careless.  This should fix it.

(In reply to Miloslav Trmac from comment #16)
> Created attachment 105344 [details] [review] [review]
> 0001-Fix-text-in-pkcheck-help.patch
> 
> Actually, the text for (pkcheck --help) is overwhelmingly wrong.  Sorry, I
> have no idea how I could be so careless.  This should fix it.

Ping.

That patch looks right to me, and the man page agrees. Maybe time to push it?

(In reply to Anders Jonsson from comment #17)
> (In reply to Miloslav Trmac from comment #16)
> > Created attachment 105344 [details] [review] [review] [review]
> > 0001-Fix-text-in-pkcheck-help.patch
> > 
> > Actually, the text for (pkcheck --help) is overwhelmingly wrong.  Sorry, I
> > have no idea how I could be so careless.  This should fix it.
> 
> Ping.
> 
> That patch looks right to me, and the man page agrees. Maybe time to push it?

Colin, can I get an ACK, please?

Created attachment 132120 [details] [review]
0001-Set-locale-before-processing-command-line-options.patch

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/polkit/polkit/issues/38.