On 08/25/2015 01:00 PM, Pavel Březina wrote:
Hi Pavel, I have 3 formal comments to coding style. Now I am running the tests. I will send you mail with results soon. Petr
0001-sss_override-document-debug-options.patch
From f181b0a94863f082abaf074a0940e83fbf1c89b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?=pbrezina@redhat.com Date: Tue, 25 Aug 2015 12:58:45 +0200 Subject: [PATCH] sss_override: document --debug options
Resolves: https://fedorahosted.org/sssd/ticket/2758
src/man/sss_override.8.xml | 18 +++++++++++++++++- src/tools/common/sss_tools.c | 25 +++++++++++++++++++++---- 2 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/src/man/sss_override.8.xml b/src/man/sss_override.8.xml index d289f5b7dfa7fbd328831b4c71d45b4c555225cf..3db8cbe05322ddf662faaa4810cd3bf6b25f8883 100644 --- a/src/man/sss_override.8.xml +++ b/src/man/sss_override.8.xml @@ -38,7 +38,7 @@ all local overrides are lost. </para> </refsect1>
^^^^ There are 4 trailing white spaces.
<refsect1 id='commands'> <title>AVAILABLE COMMANDS</title> <para>@@ -189,6 +189,22 @@ </varlistentry> </variablelist> </refsect1>
^^^^ There are 4 trailing white spaces too.
<refsect1 id='options'>
<title>COMMON OPTIONS</title><para>Those options are available with all commands.</para><variablelist remap='IP'><varlistentry><term><option>-d</option>,<option>--debug</option><replaceable>LEVEL</replaceable></term><xi:include xmlns:xi="http://www.w3.org/2001/XInclude" href="include/debug_levels.xml" /></varlistentry></variablelist></refsect1>
<xi:include xmlns:xi="http://www.w3.org/2001/XInclude" href="include/seealso.xml" />
diff --git a/src/tools/common/sss_tools.c b/src/tools/common/sss_tools.c index 6bbce3a25ddddc0b23ebc108a917a38e94981b65..3e732a3411494262ea34a1e5c332e86f5128e771 100644 --- a/src/tools/common/sss_tools.c +++ b/src/tools/common/sss_tools.c @@ -36,6 +36,13 @@ struct sss_cmdline { const char **argv; };
+static void sss_tool_print_common_opts(void) +{
- fprintf(stderr, _("Common options:\n"));
- fprintf(stderr, " -d, --debug=INT %s\n",
_("Enable debug at level"));
I'm not sure, but is it habbit to indent to '('? I mean:
|| + fprintf(stderr, " -d, --debug=INT %s\n", || + _("Enable debug at level"));
+}
- static void sss_tool_common_opts(struct sss_tool_ctx *tool_ctx, int *argc, const char **argv) {
@@ -201,6 +208,9 @@ int sss_tool_usage(const char *tool_name, fprintf(stderr, "* %s\n", commands[i].command); }
- fprintf(stderr, _("\n"));
- sss_tool_print_common_opts();
return EXIT_FAILURE;@@ -237,6 +247,13 @@ int sss_tool_route(int argc, const char **argv, return sss_tool_usage(argv[0], commands); }
+static void sss_tool_popt_print_help(poptContext pc) +{
- poptPrintHelp(pc, stderr, 0);
- fprintf(stderr, "\n");
- sss_tool_print_common_opts();
+}
- int sss_tool_popt_ex(struct sss_cmdline *cmdline, struct poptOption *options, enum sss_tool_opt require_option,
@@ -286,7 +303,7 @@ int sss_tool_popt_ex(struct sss_cmdline *cmdline, } else { fprintf(stderr, _("Invalid option %s: %s\n\n"), poptBadOption(pc, 0), poptStrerror(ret));
poptPrintHelp(pc, stderr, 0);
sss_tool_popt_print_help(pc); ret = EXIT_FAILURE; goto done; }@@ -297,7 +314,7 @@ int sss_tool_popt_ex(struct sss_cmdline *cmdline, *_fopt = poptGetArg(pc); if (*_fopt == NULL) { fprintf(stderr, _("Missing option: %s\n\n"), fopt_help);
poptPrintHelp(pc, stderr, 0);
sss_tool_popt_print_help(pc); ret = EXIT_FAILURE; goto done; }@@ -305,7 +322,7 @@ int sss_tool_popt_ex(struct sss_cmdline *cmdline, /* No more arguments expected. If something follows it is an error. */ if (poptGetArg(pc)) { fprintf(stderr, _("Only one free argument is expected!\n\n"));
poptPrintHelp(pc, stderr, 0);
sss_tool_popt_print_help(pc); ret = EXIT_FAILURE; goto done; }@@ -315,7 +332,7 @@ int sss_tool_popt_ex(struct sss_cmdline *cmdline, if (require_option == SSS_TOOL_OPT_REQUIRED && ((_fopt != NULL && cmdline->argc < 2) || cmdline->argc < 1)) { fprintf(stderr, _("At least one option is required!\n\n"));
poptPrintHelp(pc, stderr, 0);
sss_tool_popt_print_help(pc); ret = EXIT_FAILURE; goto done; }-- 2.1.0
On 08/25/2015 01:32 PM, Petr Cech wrote:
On 08/25/2015 01:00 PM, Pavel Březina wrote:
Hi Pavel, I have 3 formal comments to coding style. Now I am running the tests. I will send you mail with results soon. Petr
0001-sss_override-document-debug-options.patch
From f181b0a94863f082abaf074a0940e83fbf1c89b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?=pbrezina@redhat.com Date: Tue, 25 Aug 2015 12:58:45 +0200 Subject: [PATCH] sss_override: document --debug options
Resolves: https://fedorahosted.org/sssd/ticket/2758
src/man/sss_override.8.xml | 18 +++++++++++++++++- src/tools/common/sss_tools.c | 25 +++++++++++++++++++++---- 2 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/src/man/sss_override.8.xml b/src/man/sss_override.8.xml index d289f5b7dfa7fbd328831b4c71d45b4c555225cf..3db8cbe05322ddf662faaa4810cd3bf6b25f8883 100644 --- a/src/man/sss_override.8.xml +++ b/src/man/sss_override.8.xml @@ -38,7 +38,7 @@ all local overrides are lost. </para> </refsect1>
^^^^There are 4 trailing white spaces.
Fixed.
<refsect1 id='commands'> <title>AVAILABLE COMMANDS</title> <para>@@ -189,6 +189,22 @@ </varlistentry> </variablelist> </refsect1>
^^^^ There are 4 trailing white spaces too.
Fixed.
<refsect1 id='options'>
<title>COMMON OPTIONS</title><para>Those options are available with all commands.</para><variablelist remap='IP'><varlistentry><term><option>-d</option>,<option>--debug</option><replaceable>LEVEL</replaceable></term><xi:include xmlns:xi="http://www.w3.org/2001/XInclude" href="include/debug_levels.xml" /></varlistentry></variablelist></refsect1>
<xi:include xmlns:xi="http://www.w3.org/2001/XInclude" href="include/seealso.xml" />
diff --git a/src/tools/common/sss_tools.c b/src/tools/common/sss_tools.c index 6bbce3a25ddddc0b23ebc108a917a38e94981b65..3e732a3411494262ea34a1e5c332e86f5128e771 100644 --- a/src/tools/common/sss_tools.c +++ b/src/tools/common/sss_tools.c @@ -36,6 +36,13 @@ struct sss_cmdline { const char **argv; };
+static void sss_tool_print_common_opts(void) +{
- fprintf(stderr, _("Common options:\n"));
- fprintf(stderr, " -d, --debug=INT %s\n",
_("Enable debug at level"));I'm not sure, but is it habbit to indent to '('? I mean:
|| + fprintf(stderr, " -d, --debug=INT %s\n", || + _("Enable debug at level"));
I tend to indent to format specifier if possible, so format and data remains together.
+}
- static void sss_tool_common_opts(struct sss_tool_ctx *tool_ctx, int *argc, const char **argv) {
@@ -201,6 +208,9 @@ int sss_tool_usage(const char *tool_name, fprintf(stderr, "* %s\n", commands[i].command); }
- fprintf(stderr, _("\n"));
- sss_tool_print_common_opts();
return EXIT_FAILURE;@@ -237,6 +247,13 @@ int sss_tool_route(int argc, const char **argv, return sss_tool_usage(argv[0], commands); }
+static void sss_tool_popt_print_help(poptContext pc) +{
- poptPrintHelp(pc, stderr, 0);
- fprintf(stderr, "\n");
- sss_tool_print_common_opts();
+}
- int sss_tool_popt_ex(struct sss_cmdline *cmdline, struct poptOption *options, enum sss_tool_opt require_option,
@@ -286,7 +303,7 @@ int sss_tool_popt_ex(struct sss_cmdline *cmdline, } else { fprintf(stderr, _("Invalid option %s: %s\n\n"), poptBadOption(pc, 0), poptStrerror(ret));
poptPrintHelp(pc, stderr, 0);
sss_tool_popt_print_help(pc); ret = EXIT_FAILURE; goto done; }@@ -297,7 +314,7 @@ int sss_tool_popt_ex(struct sss_cmdline *cmdline, *_fopt = poptGetArg(pc); if (*_fopt == NULL) { fprintf(stderr, _("Missing option: %s\n\n"), fopt_help);
poptPrintHelp(pc, stderr, 0);
sss_tool_popt_print_help(pc); ret = EXIT_FAILURE; goto done; }@@ -305,7 +322,7 @@ int sss_tool_popt_ex(struct sss_cmdline *cmdline, /* No more arguments expected. If something follows it is an error. */ if (poptGetArg(pc)) { fprintf(stderr, _("Only one free argument is expected!\n\n"));
poptPrintHelp(pc, stderr, 0);
sss_tool_popt_print_help(pc); ret = EXIT_FAILURE; goto done; }@@ -315,7 +332,7 @@ int sss_tool_popt_ex(struct sss_cmdline *cmdline, if (require_option == SSS_TOOL_OPT_REQUIRED && ((_fopt != NULL && cmdline->argc < 2) || cmdline->argc < 1)) { fprintf(stderr, _("At least one option is required!\n\n"));
poptPrintHelp(pc, stderr, 0);
sss_tool_popt_print_help(pc); ret = EXIT_FAILURE; goto done; }-- 2.1.0
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 08/25/2015 01:36 PM, Pavel Březina wrote:
+static void sss_tool_print_common_opts(void) +{
- fprintf(stderr, _("Common options:\n"));
- fprintf(stderr, " -d, --debug=INT %s\n",
_("Enable debug at level"));I'm not sure, but is it habbit to indent to '('? I mean:
|| + fprintf(stderr, " -d, --debug=INT %s\n", || + _("Enable debug at level"));
I tend to indent to format specifier if possible, so format and data remains together.
OK.
I was install it, everything is right. http://sssd-ci.duckdns.org/logs/job/23/76/summary.html
ACK
Petr
On (25/08/15 13:00), Pavel Březina wrote:
From f181b0a94863f082abaf074a0940e83fbf1c89b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Tue, 25 Aug 2015 12:58:45 +0200 Subject: [PATCH] sss_override: document --debug options
Resolves: https://fedorahosted.org/sssd/ticket/2758
src/man/sss_override.8.xml | 18 +++++++++++++++++- src/tools/common/sss_tools.c | 25 +++++++++++++++++++++---- 2 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/src/man/sss_override.8.xml b/src/man/sss_override.8.xml index d289f5b7dfa7fbd328831b4c71d45b4c555225cf..3db8cbe05322ddf662faaa4810cd3bf6b25f8883 100644 --- a/src/man/sss_override.8.xml +++ b/src/man/sss_override.8.xml @@ -38,7 +38,7 @@ all local overrides are lost. </para> </refsect1>
<refsect1 id='commands'> <title>AVAILABLE COMMANDS</title> <para>
@@ -189,6 +189,22 @@ </varlistentry> </variablelist> </refsect1>
<refsect1 id='options'>
<title>COMMON OPTIONS</title><para>Those options are available with all commands.</para><variablelist remap='IP'><varlistentry><term><option>-d</option>,<option>--debug</option><replaceable>LEVEL</replaceable></term><xi:include xmlns:xi="http://www.w3.org/2001/XInclude" href="include/debug_levels.xml" /></varlistentry></variablelist></refsect1>
<xi:include xmlns:xi="http://www.w3.org/2001/XInclude" href="include/seealso.xml" />
diff --git a/src/tools/common/sss_tools.c b/src/tools/common/sss_tools.c index 6bbce3a25ddddc0b23ebc108a917a38e94981b65..3e732a3411494262ea34a1e5c332e86f5128e771 100644 --- a/src/tools/common/sss_tools.c +++ b/src/tools/common/sss_tools.c @@ -36,6 +36,13 @@ struct sss_cmdline { const char **argv; };
+static void sss_tool_print_common_opts(void) +{
- fprintf(stderr, _("Common options:\n"));
- fprintf(stderr, " -d, --debug=INT %s\n",
_("Enable debug at level"));
NACK This does not correspond with reality. The sort version does not work.
It would be good to test own patches :-)
[root@host ~]# sss_override -d=9 user-add Usage: sss_override COMMAND COMMAND-ARGS
Available commands: * user-add * user-del * group-add * group-del
[root@host ~]# sss_override --debug=2 user-add Missing option: Specify name of modified object.
Usage: sss_override user-add NAME [OPTIONS...] -n, --name=STRING Override name -u, --uid=INT Override uid (non-zero value) -g, --gid=INT Override gid (non-zero value) -h, --home=STRING Override home directory -s, --shell=STRING Override shell -c, --gecos=STRING Override gecos
Help options: -?, --help Show this help message --usage Display brief usage message (Thu Aug 27 12:32:49:273331 2015) [sssd] [parse_cmdline] (0x0020): Unable to parse command arguments (Thu Aug 27 12:32:49:273461 2015) [sssd] [override_user_add] (0x0020): Unable to parse command line.
As you can see long version works.
I would be also curious why you need to provide hacks for printing argument description for autohelp. There is a much more elegant way how to do it with libpopt. (small hint POPT_ARGFLAG_DOC_HIDDEN in sss_tool_common_opts)
LS
On 08/27/2015 12:49 PM, Lukas Slebodnik wrote:
On (25/08/15 13:00), Pavel Březina wrote:
From f181b0a94863f082abaf074a0940e83fbf1c89b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Tue, 25 Aug 2015 12:58:45 +0200 Subject: [PATCH] sss_override: document --debug options
Resolves: https://fedorahosted.org/sssd/ticket/2758
src/man/sss_override.8.xml | 18 +++++++++++++++++- src/tools/common/sss_tools.c | 25 +++++++++++++++++++++---- 2 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/src/man/sss_override.8.xml b/src/man/sss_override.8.xml index d289f5b7dfa7fbd328831b4c71d45b4c555225cf..3db8cbe05322ddf662faaa4810cd3bf6b25f8883 100644 --- a/src/man/sss_override.8.xml +++ b/src/man/sss_override.8.xml @@ -38,7 +38,7 @@ all local overrides are lost. </para> </refsect1>
<refsect1 id='commands'> <title>AVAILABLE COMMANDS</title> <para>
@@ -189,6 +189,22 @@ </varlistentry> </variablelist> </refsect1>
<refsect1 id='options'>
<title>COMMON OPTIONS</title><para>Those options are available with all commands.</para><variablelist remap='IP'><varlistentry><term><option>-d</option>,<option>--debug</option><replaceable>LEVEL</replaceable></term><xi:include xmlns:xi="http://www.w3.org/2001/XInclude" href="include/debug_levels.xml" /></varlistentry></variablelist></refsect1>
<xi:include xmlns:xi="http://www.w3.org/2001/XInclude" href="include/seealso.xml" />
diff --git a/src/tools/common/sss_tools.c b/src/tools/common/sss_tools.c index 6bbce3a25ddddc0b23ebc108a917a38e94981b65..3e732a3411494262ea34a1e5c332e86f5128e771 100644 --- a/src/tools/common/sss_tools.c +++ b/src/tools/common/sss_tools.c @@ -36,6 +36,13 @@ struct sss_cmdline { const char **argv; };
+static void sss_tool_print_common_opts(void) +{
- fprintf(stderr, _("Common options:\n"));
- fprintf(stderr, " -d, --debug=INT %s\n",
_("Enable debug at level"));NACK This does not correspond with reality. The sort version does not work.
It would be good to test own patches :-)
[root@host ~]# sss_override -d=9 user-add Usage: sss_override COMMAND COMMAND-ARGS
Available commands: * user-add * user-del * group-add * group-del[root@host ~]# sss_override --debug=2 user-add Missing option: Specify name of modified object.
Usage: sss_override user-add NAME [OPTIONS...] -n, --name=STRING Override name -u, --uid=INT Override uid (non-zero value) -g, --gid=INT Override gid (non-zero value) -h, --home=STRING Override home directory -s, --shell=STRING Override shell -c, --gecos=STRING Override gecos
Help options: -?, --help Show this help message --usage Display brief usage message (Thu Aug 27 12:32:49:273331 2015) [sssd] [parse_cmdline] (0x0020): Unable to parse command arguments (Thu Aug 27 12:32:49:273461 2015) [sssd] [override_user_add] (0x0020): Unable to parse command line.
As you can see long version works.
Thanks for this catch.
I would be also curious why you need to provide hacks for printing argument description for autohelp. There is a much more elegant way how to do it with libpopt. (small hint POPT_ARGFLAG_DOC_HIDDEN in sss_tool_common_opts)
Your curiosity is remarkable, though I have no idea why are you hinting this flag.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (28/08/15 12:13), Pavel Březina wrote:
On 08/27/2015 12:49 PM, Lukas Slebodnik wrote:
On (25/08/15 13:00), Pavel Březina wrote:
From f181b0a94863f082abaf074a0940e83fbf1c89b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Tue, 25 Aug 2015 12:58:45 +0200 Subject: [PATCH] sss_override: document --debug options
Resolves: https://fedorahosted.org/sssd/ticket/2758
src/man/sss_override.8.xml | 18 +++++++++++++++++- src/tools/common/sss_tools.c | 25 +++++++++++++++++++++---- 2 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/src/man/sss_override.8.xml b/src/man/sss_override.8.xml index d289f5b7dfa7fbd328831b4c71d45b4c555225cf..3db8cbe05322ddf662faaa4810cd3bf6b25f8883 100644 --- a/src/man/sss_override.8.xml +++ b/src/man/sss_override.8.xml @@ -38,7 +38,7 @@ all local overrides are lost. </para> </refsect1>
<refsect1 id='commands'> <title>AVAILABLE COMMANDS</title> <para>
@@ -189,6 +189,22 @@ </varlistentry> </variablelist> </refsect1>
<refsect1 id='options'>
<title>COMMON OPTIONS</title><para>Those options are available with all commands.</para><variablelist remap='IP'><varlistentry><term><option>-d</option>,<option>--debug</option><replaceable>LEVEL</replaceable></term><xi:include xmlns:xi="http://www.w3.org/2001/XInclude" href="include/debug_levels.xml" /></varlistentry></variablelist></refsect1>
<xi:include xmlns:xi="http://www.w3.org/2001/XInclude" href="include/seealso.xml" />
diff --git a/src/tools/common/sss_tools.c b/src/tools/common/sss_tools.c index 6bbce3a25ddddc0b23ebc108a917a38e94981b65..3e732a3411494262ea34a1e5c332e86f5128e771 100644 --- a/src/tools/common/sss_tools.c +++ b/src/tools/common/sss_tools.c @@ -36,6 +36,13 @@ struct sss_cmdline { const char **argv; };
+static void sss_tool_print_common_opts(void) +{
- fprintf(stderr, _("Common options:\n"));
- fprintf(stderr, " -d, --debug=INT %s\n",
_("Enable debug at level"));NACK This does not correspond with reality. The sort version does not work.
It would be good to test own patches :-)
[root@host ~]# sss_override -d=9 user-add Usage: sss_override COMMAND COMMAND-ARGS
Available commands: * user-add * user-del * group-add * group-del[root@host ~]# sss_override --debug=2 user-add Missing option: Specify name of modified object.
Usage: sss_override user-add NAME [OPTIONS...] -n, --name=STRING Override name -u, --uid=INT Override uid (non-zero value) -g, --gid=INT Override gid (non-zero value) -h, --home=STRING Override home directory -s, --shell=STRING Override shell -c, --gecos=STRING Override gecos
Help options: -?, --help Show this help message --usage Display brief usage message (Thu Aug 27 12:32:49:273331 2015) [sssd] [parse_cmdline] (0x0020): Unable to parse command arguments (Thu Aug 27 12:32:49:273461 2015) [sssd] [override_user_add] (0x0020): Unable to parse command line.
As you can see long version works.
Thanks for this catch.
I would be also curious why you need to provide hacks for printing argument description for autohelp. There is a much more elegant way how to do it with libpopt. (small hint POPT_ARGFLAG_DOC_HIDDEN in sss_tool_common_opts)
Your curiosity is remarkable, though I have no idea why are you hinting this flag.
because I cannot see a reason why we should write tool tip with fprintf if there is a way with libpopt.
It's not a NACK but I do not like your solution so I will not ACK it either. You need to find someone else.
LS
On 08/28/2015 12:21 PM, Lukas Slebodnik wrote:
On (28/08/15 12:13), Pavel Březina wrote:
On 08/27/2015 12:49 PM, Lukas Slebodnik wrote:
On (25/08/15 13:00), Pavel Březina wrote:
From f181b0a94863f082abaf074a0940e83fbf1c89b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Tue, 25 Aug 2015 12:58:45 +0200 Subject: [PATCH] sss_override: document --debug options
Resolves: https://fedorahosted.org/sssd/ticket/2758
src/man/sss_override.8.xml | 18 +++++++++++++++++- src/tools/common/sss_tools.c | 25 +++++++++++++++++++++---- 2 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/src/man/sss_override.8.xml b/src/man/sss_override.8.xml index d289f5b7dfa7fbd328831b4c71d45b4c555225cf..3db8cbe05322ddf662faaa4810cd3bf6b25f8883 100644 --- a/src/man/sss_override.8.xml +++ b/src/man/sss_override.8.xml @@ -38,7 +38,7 @@ all local overrides are lost. </para> </refsect1>
<refsect1 id='commands'> <title>AVAILABLE COMMANDS</title> <para>
@@ -189,6 +189,22 @@ </varlistentry> </variablelist> </refsect1>
<refsect1 id='options'>
<title>COMMON OPTIONS</title><para>Those options are available with all commands.</para><variablelist remap='IP'><varlistentry><term><option>-d</option>,<option>--debug</option><replaceable>LEVEL</replaceable></term><xi:include xmlns:xi="http://www.w3.org/2001/XInclude" href="include/debug_levels.xml" /></varlistentry></variablelist></refsect1>
<xi:include xmlns:xi="http://www.w3.org/2001/XInclude" href="include/seealso.xml" />
diff --git a/src/tools/common/sss_tools.c b/src/tools/common/sss_tools.c index 6bbce3a25ddddc0b23ebc108a917a38e94981b65..3e732a3411494262ea34a1e5c332e86f5128e771 100644 --- a/src/tools/common/sss_tools.c +++ b/src/tools/common/sss_tools.c @@ -36,6 +36,13 @@ struct sss_cmdline { const char **argv; };
+static void sss_tool_print_common_opts(void) +{
- fprintf(stderr, _("Common options:\n"));
- fprintf(stderr, " -d, --debug=INT %s\n",
_("Enable debug at level"));NACK This does not correspond with reality. The sort version does not work.
It would be good to test own patches :-)
[root@host ~]# sss_override -d=9 user-add Usage: sss_override COMMAND COMMAND-ARGS
Available commands: * user-add * user-del * group-add * group-del[root@host ~]# sss_override --debug=2 user-add Missing option: Specify name of modified object.
Usage: sss_override user-add NAME [OPTIONS...] -n, --name=STRING Override name -u, --uid=INT Override uid (non-zero value) -g, --gid=INT Override gid (non-zero value) -h, --home=STRING Override home directory -s, --shell=STRING Override shell -c, --gecos=STRING Override gecos
Help options: -?, --help Show this help message --usage Display brief usage message (Thu Aug 27 12:32:49:273331 2015) [sssd] [parse_cmdline] (0x0020): Unable to parse command arguments (Thu Aug 27 12:32:49:273461 2015) [sssd] [override_user_add] (0x0020): Unable to parse command line.
As you can see long version works.
Thanks for this catch.
I would be also curious why you need to provide hacks for printing argument description for autohelp. There is a much more elegant way how to do it with libpopt. (small hint POPT_ARGFLAG_DOC_HIDDEN in sss_tool_common_opts)
Your curiosity is remarkable, though I have no idea why are you hinting this flag.
because I cannot see a reason why we should write tool tip with fprintf if there is a way with libpopt.
Because there is a way, but not an straight-forward and elegant way. Since there is only one option and the list is not likely to be extended, there is no problem with doing it this way.
It's not a NACK but I do not like your solution so I will not ACK it either. You need to find someone else.
I'm completely fine with this.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 08/28/2015 12:25 PM, Pavel Březina wrote:
I would be also curious why you need to provide hacks for printing argument description for autohelp. There is a much more elegant way how to do it with libpopt. (small hint POPT_ARGFLAG_DOC_HIDDEN in sss_tool_common_opts)
Your curiosity is remarkable, though I have no idea why are you hinting this flag.
because I cannot see a reason why we should write tool tip with fprintf if there is a way with libpopt.
Because there is a way, but not an straight-forward and elegant way. Since there is only one option and the list is not likely to be extended, there is no problem with doing it this way.
It's not a NACK but I do not like your solution so I will not ACK it either. You need to find someone else.
I'm completely fine with this.
LS
I am doing code review for fixed patch now. Patch looks good to me and it works.
I discussed the problem above (about fprintf vs. libpopt) with both offline. The libpopt solution starts here [1] and I agree it would not be straightforward. So I am inclined to the solution presented in the patch.
Regards
Petr
[1] src/tools/sss_override.c:1412
PS: I am waiting for CI tests.
On 08/28/2015 03:13 PM, Petr Cech wrote:
I am doing code review for fixed patch now. Patch looks good to me and it works.
I discussed the problem above (about fprintf vs. libpopt) with both offline. The libpopt solution starts here [1] and I agree it would not be straightforward. So I am inclined to the solution presented in the patch.
Regards
Petr
[1] src/tools/sss_override.c:1412
PS: I am waiting for CI tests.
CI: http://sssd-ci.duckdns.org/logs/job/23/84/summary.html Failing is not connected to this patch.
=> ACK
Petr
On Fri, Aug 28, 2015 at 03:24:21PM +0200, Petr Cech wrote:
On 08/28/2015 03:13 PM, Petr Cech wrote:
I am doing code review for fixed patch now. Patch looks good to me and it works.
I discussed the problem above (about fprintf vs. libpopt) with both offline. The libpopt solution starts here [1] and I agree it would not be straightforward. So I am inclined to the solution presented in the patch.
Regards
Petr
[1] src/tools/sss_override.c:1412
PS: I am waiting for CI tests.
CI: http://sssd-ci.duckdns.org/logs/job/23/84/summary.html Failing is not connected to this patch.
=> ACK
Petr
* master: 5e2ffb69dcdd157ea422c6aec256111653e4206b
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Březina -
Petr Cech