Re: [PATCH v2 2/4] stripspace: Use parse-options for command-line parsing

[Tobias Klauser]

On 2015-10-17 at 23:24:13 +0200, Junio C Hamano  wrote:
> Tobias Klauser  writes:
> 
> > On 2015-10-16 at 19:29:35 +0200, Junio C Hamano  wrote:
> >> Junio C Hamano  writes:
> >> 
> >> >> -   if (mode == INVAL)
> >> >> -   usage(usage_msg);
> >> >
> >> > When given "git stripspace -s blorg", we used to set mode to INVAL
> >> > and then showed the correct usage.  But we no longer have a check
> >> > that corresponds to the old INVAL thing, do we?  Perhaps check argc
> >> > to detect presence of an otherwise ignored non-option argument
> >> > immediately after parse_options() returns?
> >> 
> >> Perhaps like this.
> >
> > Thanks. I'll fold it into v3.
> 
> Before starting v3, please fetch from me and check what is queued on
> 'pu'.  It may turn out that the fix-ups I did while queuing this
> round is sufficient, in which case you can just say that instead ;-)

Now that patches 3 and 4 will be dropped and no changes being necessary
for patches 1 and 2 (except for your changes that I see are already
folded into 'pu'), do you want me to submit a v3 of the series? Or is it
enough if I ask you to drop patches 3 (stripspace: implement
--count-lines option) and 4 (rebase -i: use "stripspace --count-lines"
when counting todo items)?

Thanks
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] stripspace: Use parse-options for command-line parsing

[Junio C Hamano]

Tobias Klauser  writes:

> On 2015-10-17 at 23:24:13 +0200, Junio C Hamano  wrote:
>> Before starting v3, please fetch from me and check what is queued on
>> 'pu'.  It may turn out that the fix-ups I did while queuing this
>> round is sufficient, in which case you can just say that instead ;-)
>
> Now that patches 3 and 4 will be dropped and no changes being necessary
> for patches 1 and 2 (except for your changes that I see are already
> folded into 'pu'), do you want me to submit a v3 of the series? Or is it
> enough if I ask you to drop patches 3 (stripspace: implement
> --count-lines option) and 4 (rebase -i: use "stripspace --count-lines"
> when counting todo items)?

Yes, the latter is sufficient and preferrable (less work for both of
us, without losing anything to help other people to review and
discuss who may want to chime in).

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] stripspace: Use parse-options for command-line parsing

[Tobias Klauser]

On 2015-10-16 at 19:07:34 +0200, Junio C Hamano  wrote:
> Tobias Klauser  writes:
> 
> > Use parse-options to parse command-line options instead of a
> > hand-crafted implementation.
> >
> > This is a preparatory patch to simplify the introduction of the
> > --count-lines option in a follow-up patch.
> 
> The second paragraph is probably of much lessor importance than one
> thing you forgot to mention: the users can now use a unique prefix
> of the option and say "stripspace --comment".

I didn't even know about that feature, but now that you've mentioned it
I will certainly make use of it more in the future :) And of course,
I'll adjust the commit message accordingly for v3.

> > +enum stripspace_mode {
> > +   STRIP_DEFAULT = 0,
> > +   STRIP_COMMENTS,
> > +   COMMENT_LINES
> > +};
> >  
> >  int cmd_stripspace(int argc, const char **argv, const char *prefix)
> >  {
> > struct strbuf buf = STRBUF_INIT;
> > -   int strip_comments = 0;
> > -   enum { INVAL = 0, STRIP_SPACE = 1, COMMENT_LINES = 2 } mode = 
> > STRIP_SPACE;
> > -
> > -   if (argc == 2) {
> > -   if (!strcmp(argv[1], "-s") ||
> > -   !strcmp(argv[1], "--strip-comments")) {
> > -   strip_comments = 1;
> > -   } else if (!strcmp(argv[1], "-c") ||
> > -  !strcmp(argv[1], "--comment-lines")) {
> > -   mode = COMMENT_LINES;
> > -   } else {
> > -   mode = INVAL;
> > -   }
> > -   } else if (argc > 1) {
> > -   mode = INVAL;
> > -   }
> > -
> > -   if (mode == INVAL)
> > -   usage(usage_msg);
> 
> When given "git stripspace -s blorg", we used to set mode to INVAL
> and then showed the correct usage.  But we no longer have a check
> that corresponds to the old INVAL thing, do we?  Perhaps check argc
> to detect presence of an otherwise ignored non-option argument
> immediately after parse_options() returns?

Agreed, we should check it. I'll go with the implementation you
suggested in the follow-up message.

> > -   if (strip_comments || mode == COMMENT_LINES)
> > +   enum stripspace_mode mode = STRIP_DEFAULT;
> > +
> > +   const struct option options[] = {
> > +   OPT_CMDMODE('s', "strip-comments", ,
> > +   N_("skip and remove all lines starting with comment 
> > character"),
> > +   STRIP_COMMENTS),
> > +   OPT_CMDMODE('c', "comment-lines", ,
> > +   N_("prepend comment character and blank to each 
> > line"),
> > +   COMMENT_LINES),
> > +   OPT_END()
> > +   };
> > +
> > +   argc = parse_options(argc, argv, prefix, options, stripspace_usage,
> > +PARSE_OPT_KEEP_DASHDASH);
> 
> What is the point of keep-dashdash here?

Likewise, it shouldn't be there as in your follow-up patch.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] stripspace: Use parse-options for command-line parsing

[Tobias Klauser]

On 2015-10-16 at 19:29:35 +0200, Junio C Hamano  wrote:
> Junio C Hamano  writes:
> 
> >> -  if (mode == INVAL)
> >> -  usage(usage_msg);
> >
> > When given "git stripspace -s blorg", we used to set mode to INVAL
> > and then showed the correct usage.  But we no longer have a check
> > that corresponds to the old INVAL thing, do we?  Perhaps check argc
> > to detect presence of an otherwise ignored non-option argument
> > immediately after parse_options() returns?
> 
> Perhaps like this.

Thanks. I'll fold it into v3.

> diff --git a/builtin/stripspace.c b/builtin/stripspace.c
> index ac1ab3d..a8b7a93 100644
> --- a/builtin/stripspace.c
> +++ b/builtin/stripspace.c
> @@ -40,8 +40,9 @@ int cmd_stripspace(int argc, const char **argv, const char 
> *prefix)
>   OPT_END()
>   };
>  
> - argc = parse_options(argc, argv, prefix, options, stripspace_usage,
> -  PARSE_OPT_KEEP_DASHDASH);
> + argc = parse_options(argc, argv, prefix, options, stripspace_usage, 0);
> + if (argc)
> + usage_with_options(stripspace_usage, options);
>  
>   if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
>   git_config(git_default_config, NULL);
> 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] stripspace: Use parse-options for command-line parsing

[Junio C Hamano]

Tobias Klauser  writes:

> On 2015-10-16 at 19:29:35 +0200, Junio C Hamano  wrote:
>> Junio C Hamano  writes:
>> 
>> >> - if (mode == INVAL)
>> >> - usage(usage_msg);
>> >
>> > When given "git stripspace -s blorg", we used to set mode to INVAL
>> > and then showed the correct usage.  But we no longer have a check
>> > that corresponds to the old INVAL thing, do we?  Perhaps check argc
>> > to detect presence of an otherwise ignored non-option argument
>> > immediately after parse_options() returns?
>> 
>> Perhaps like this.
>
> Thanks. I'll fold it into v3.

Before starting v3, please fetch from me and check what is queued on
'pu'.  It may turn out that the fix-ups I did while queuing this
round is sufficient, in which case you can just say that instead ;-)

Thanks.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] stripspace: Use parse-options for command-line parsing

[Junio C Hamano]

Tobias Klauser  writes:

> Use parse-options to parse command-line options instead of a
> hand-crafted implementation.
>
> This is a preparatory patch to simplify the introduction of the
> --count-lines option in a follow-up patch.

The second paragraph is probably of much lessor importance than one
thing you forgot to mention: the users can now use a unique prefix
of the option and say "stripspace --comment".

> +enum stripspace_mode {
> + STRIP_DEFAULT = 0,
> + STRIP_COMMENTS,
> + COMMENT_LINES
> +};
>  
>  int cmd_stripspace(int argc, const char **argv, const char *prefix)
>  {
>   struct strbuf buf = STRBUF_INIT;
> - int strip_comments = 0;
> - enum { INVAL = 0, STRIP_SPACE = 1, COMMENT_LINES = 2 } mode = 
> STRIP_SPACE;
> -
> - if (argc == 2) {
> - if (!strcmp(argv[1], "-s") ||
> - !strcmp(argv[1], "--strip-comments")) {
> - strip_comments = 1;
> - } else if (!strcmp(argv[1], "-c") ||
> -!strcmp(argv[1], "--comment-lines")) {
> - mode = COMMENT_LINES;
> - } else {
> - mode = INVAL;
> - }
> - } else if (argc > 1) {
> - mode = INVAL;
> - }
> -
> - if (mode == INVAL)
> - usage(usage_msg);

When given "git stripspace -s blorg", we used to set mode to INVAL
and then showed the correct usage.  But we no longer have a check
that corresponds to the old INVAL thing, do we?  Perhaps check argc
to detect presence of an otherwise ignored non-option argument
immediately after parse_options() returns?

> - if (strip_comments || mode == COMMENT_LINES)
> + enum stripspace_mode mode = STRIP_DEFAULT;
> +
> + const struct option options[] = {
> + OPT_CMDMODE('s', "strip-comments", ,
> + N_("skip and remove all lines starting with comment 
> character"),
> + STRIP_COMMENTS),
> + OPT_CMDMODE('c', "comment-lines", ,
> + N_("prepend comment character and blank to each 
> line"),
> + COMMENT_LINES),
> + OPT_END()
> + };
> +
> + argc = parse_options(argc, argv, prefix, options, stripspace_usage,
> +  PARSE_OPT_KEEP_DASHDASH);

What is the point of keep-dashdash here?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] stripspace: Use parse-options for command-line parsing

[Junio C Hamano]

Junio C Hamano  writes:

>> -if (mode == INVAL)
>> -usage(usage_msg);
>
> When given "git stripspace -s blorg", we used to set mode to INVAL
> and then showed the correct usage.  But we no longer have a check
> that corresponds to the old INVAL thing, do we?  Perhaps check argc
> to detect presence of an otherwise ignored non-option argument
> immediately after parse_options() returns?

Perhaps like this.

diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index ac1ab3d..a8b7a93 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -40,8 +40,9 @@ int cmd_stripspace(int argc, const char **argv, const char 
*prefix)
OPT_END()
};
 
-   argc = parse_options(argc, argv, prefix, options, stripspace_usage,
-PARSE_OPT_KEEP_DASHDASH);
+   argc = parse_options(argc, argv, prefix, options, stripspace_usage, 0);
+   if (argc)
+   usage_with_options(stripspace_usage, options);
 
if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
git_config(git_default_config, NULL);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/4] stripspace: Use parse-options for command-line parsing

[Tobias Klauser]

Use parse-options to parse command-line options instead of a
hand-crafted implementation.

This is a preparatory patch to simplify the introduction of the
--count-lines option in a follow-up patch.

Signed-off-by: Tobias Klauser 
---
 builtin/stripspace.c | 56 
 1 file changed, 30 insertions(+), 26 deletions(-)

diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index f677093..ac1ab3d 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -1,5 +1,6 @@
 #include "builtin.h"
 #include "cache.h"
+#include "parse-options.h"
 #include "strbuf.h"
 
 static void comment_lines(struct strbuf *buf)
@@ -12,41 +13,44 @@ static void comment_lines(struct strbuf *buf)
free(msg);
 }
 
-static const char *usage_msg = "\n"
-"  git stripspace [-s | --strip-comments] < input\n"
-"  git stripspace [-c | --comment-lines] < input";
+static const char * const stripspace_usage[] = {
+   N_("git stripspace [-s | --strip-comments] < input"),
+   N_("git stripspace [-c | --comment-lines] < input"),
+   NULL
+};
+
+enum stripspace_mode {
+   STRIP_DEFAULT = 0,
+   STRIP_COMMENTS,
+   COMMENT_LINES
+};
 
 int cmd_stripspace(int argc, const char **argv, const char *prefix)
 {
struct strbuf buf = STRBUF_INIT;
-   int strip_comments = 0;
-   enum { INVAL = 0, STRIP_SPACE = 1, COMMENT_LINES = 2 } mode = 
STRIP_SPACE;
-
-   if (argc == 2) {
-   if (!strcmp(argv[1], "-s") ||
-   !strcmp(argv[1], "--strip-comments")) {
-   strip_comments = 1;
-   } else if (!strcmp(argv[1], "-c") ||
-  !strcmp(argv[1], "--comment-lines")) {
-   mode = COMMENT_LINES;
-   } else {
-   mode = INVAL;
-   }
-   } else if (argc > 1) {
-   mode = INVAL;
-   }
-
-   if (mode == INVAL)
-   usage(usage_msg);
-
-   if (strip_comments || mode == COMMENT_LINES)
+   enum stripspace_mode mode = STRIP_DEFAULT;
+
+   const struct option options[] = {
+   OPT_CMDMODE('s', "strip-comments", ,
+   N_("skip and remove all lines starting with comment 
character"),
+   STRIP_COMMENTS),
+   OPT_CMDMODE('c', "comment-lines", ,
+   N_("prepend comment character and blank to each 
line"),
+   COMMENT_LINES),
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options, stripspace_usage,
+PARSE_OPT_KEEP_DASHDASH);
+
+   if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
git_config(git_default_config, NULL);
 
if (strbuf_read(, 0, 1024) < 0)
die_errno("could not read the input");
 
-   if (mode == STRIP_SPACE)
-   strbuf_stripspace(, strip_comments);
+   if (mode == STRIP_DEFAULT || mode == STRIP_COMMENTS)
+   strbuf_stripspace(, mode == STRIP_COMMENTS);
else
comment_lines();
 
-- 
2.6.1.148.g7927db1


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html