[packagekit] [PATCH 0/2] gnome-packagekit option parsing updates

[James Bowes]

On 10/11/07, Richard Hughes <hughsient at gmail.com> wrote:
> On Wed, 2007-10-10 at 23:15 -0400, James Bowes wrote:
> > diff --git a/src/pk-application-main.c b/src/pk-application-main.c
> > index f2224c7..c55d553 100644
> > --- a/src/pk-application-main.c
> > +++ b/src/pk-application-main.c
> > @@ -66,13 +66,16 @@ main (int argc, char *argv[])
> >  {
> >         GMainLoop *loop;
> >         gboolean verbose = FALSE;
> > +       gboolean program_version = FALSE;
> >         PkApplication *application = NULL;
> >         GOptionContext *context;
> >
> >         const GOptionEntry options[] = {
> >                 { "verbose", '\0', 0, G_OPTION_ARG_NONE, &verbose,
> >                   "Show extra debugging information", NULL },
> > -               { NULL}
> > +               { "version", '\0', 0, G_OPTION_ARG_NONE,
> > &program_version,
> > +                 "Show the program version and exit", NULL },
> > +       { NULL}
>
> I think you need to use two tabs before {NULL}, not one.

Oops.

>
> > +       if (program_version) {
> > +               g_print (VERSION "\n");
> > +               return 0;
> > +       }
>
> Can you please do "if (program_version == TRUE) {" - and then we know
> that it's boolean, and it's very obvious what it's doing when it's
> reviewed. It's very easy to miss a "!". Could you fix up the patch to do
> this for all the new code please. Thanks.
>
> > diff --git a/src/pk-install-file.c b/src/pk-install-file.c
> > index 6bc0475..9790db0 100644
> > --- a/src/pk-install-file.c
> > +++ b/src/pk-install-file.c
> > @@ -53,6 +53,16 @@ main (int argc, char *argv[])
> >  {
> >         GOptionContext *context;
> >         gboolean ret;
> > +       gboolean verbose = FALSE;
> > +       gboolean program_version = FALSE;
> > +
> > +       const GOptionEntry options[] = {
> > +               { "verbose", '\0', 0, G_OPTION_ARG_NONE, &verbose,
> > +                 "Show extra debugging information", NULL },
> > +               { "version", '\0', 0, G_OPTION_ARG_NONE,
> > &program_version,
> > +                 "Show the program version and exit", NULL },
> > +               { NULL}
> > +       };
>
> Are you sure this works? pk-install-file takes the first argument
> argument as the thing to install (e.g.
> (pk-install-file /home/ania/Desktop/tomtom-1.23.rpm") and so I think
> this is going to break if we do "pk-install-file
> --verbose /home/ania/Desktop/tomtom-1.23.rpm" - you might need to fix my
> braindead argc/argv stuff lower down.

GOption modifies argc and argv as it goes, so yes, it still works.

>
> > diff --git a/src/pk-install-package.c b/src/pk-install-package.c
> > index ab71d28..12f67ee 100644
> > --- a/src/pk-install-package.c
> > +++ b/src/pk-install-package.c
> > @@ -100,6 +100,16 @@ main (int argc, char *argv[])
> >  {
> >         GOptionContext *context;
> >         gboolean ret;
> > +       gboolean verbose = FALSE;
> > +       gboolean program_version = FALSE;
> > +
> > +       const GOptionEntry options[] = {
> > +               { "verbose", '\0', 0, G_OPTION_ARG_NONE, &verbose,
> > +                 "Show extra debugging information", NULL },
> > +               { "version", '\0', 0, G_OPTION_ARG_NONE,
> > &program_version,
> > +                 "Show the program version and exit", NULL },
> > +               { NULL}
> > +       };
>
> Same problem with pk-install-package...
>
> Other than that, the patches look good. Thanks,

I've attached an updated patch fixing addressing the points you
brought up. Thanks for the feedback!

-James
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-version-option-for-commands.patch
Type: text/x-patch
Size: 9020 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/packagekit/attachments/20071011/7ef98df2/attachment-0004.bin>