Bug 29936

Summary: Should never show manpage unless requested
Product: PolicyKit Reporter: David Zeuthen (not reading bugmail) <zeuthen>
Component: daemonAssignee: David Zeuthen (not reading bugmail) <zeuthen>
Status: RESOLVED MOVED QA Contact: David Zeuthen (not reading bugmail) <zeuthen>
Severity: normal    
Priority: medium CC: anders.jonsson
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: 0001-Don-t-spawn-man-for-help.patch
0002-Set-locale-before-processing-command-line-options.patch
0003-Fix-package-version-bug-report-address-mixing.patch
0004-Add-bug-reporting-address-and-home-page-to-help-outp.patch
0005-Refuse-unrecognized-command-line-operands.patch
0006-Exit-pkaction-with-status-0-on-success.patch
0001-Don-t-spawn-man-for-help.patch
0004-Add-bug-reporting-address-and-home-page-to-help-outp.patch
0001-Fix-text-in-pkcheck-help.patch
0001-Set-locale-before-processing-command-line-options.patch

Description David Zeuthen (not reading bugmail) 2010-09-01 06:49:03 UTC
... since this may upset scripts / programs using the utilities. See rhbz 628862 for more discussion.
Comment 1 Miloslav Trmac 2013-04-19 23:30:05 UTC
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.
Comment 2 Miloslav Trmac 2013-04-19 23:33:55 UTC
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.
Comment 3 Miloslav Trmac 2013-04-19 23:34:15 UTC
Created attachment 78260 [details] [review]
0003-Fix-package-version-bug-report-address-mixing.patch
Comment 4 Miloslav Trmac 2013-04-19 23:34:32 UTC
Created attachment 78261 [details] [review]
0004-Add-bug-reporting-address-and-home-page-to-help-outp.patch
Comment 5 Miloslav Trmac 2013-04-19 23:34:59 UTC
Created attachment 78262 [details] [review]
0005-Refuse-unrecognized-command-line-operands.patch

Might break some (incorrect) users.
Comment 6 Miloslav Trmac 2013-04-19 23:35:37 UTC
Created attachment 78263 [details] [review]
0006-Exit-pkaction-with-status-0-on-success.patch

(Not strictly related to the rest of the series.)
Comment 7 Colin Walters 2013-05-06 18:08:36 UTC
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
Comment 8 Miloslav Trmac 2013-05-06 18:59:20 UTC
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.
Comment 9 Miloslav Trmac 2013-05-06 18:59:56 UTC
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.
Comment 10 Miloslav Trmac 2013-05-06 19:04:34 UTC
(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 11 Colin Walters 2013-05-06 19:26:26 UTC
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 12 Colin Walters 2013-05-06 19:29:27 UTC
Comment on attachment 78262 [details] [review]
0005-Refuse-unrecognized-command-line-operands.patch

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

Looks correct.
Comment 13 Colin Walters 2013-05-06 19:29:55 UTC
Comment on attachment 78263 [details] [review]
0006-Exit-pkaction-with-status-0-on-success.patch

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

Yes.
Comment 14 Colin Walters 2013-05-06 19:32:27 UTC
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.
Comment 15 Miloslav Trmac 2013-05-13 15:51:41 UTC
Thanks for the review; I have pushed all but 0002, and opened bug 64551 about the internationalization to eventually discuss separately.
Comment 16 Miloslav Trmac 2014-08-27 16:43:37 UTC
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.
Comment 17 Anders Jonsson 2015-10-21 20:43:09 UTC
(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?
Comment 18 Miloslav Trmac 2015-10-22 14:33:17 UTC
(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?
Comment 19 Miloslav Trmac 2017-06-21 20:17:58 UTC
Created attachment 132120 [details] [review]
0001-Set-locale-before-processing-command-line-options.patch
Comment 20 GitLab Migration User 2018-08-20 21:36:38 UTC
-- 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.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.