[PATCH] grep: correct help string for --exclude-standard

[PATCH] grep: correct help string for --exclude-standard

[Nguyễn Thái Ngọc Duy]

The current help string is about --no-exclude-standard. But "git grep -h"
would show --exclude-standard instead. Flip the string. See 0a93fb8
(grep: teach --untracked and --exclude-standard options - 2011-09-27)
for more info about these options.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/grep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 4063882..e77f7cf 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -641,7 +641,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "untracked", &untracked,
 			N_("search in both tracked and untracked files")),
 		OPT_SET_INT(0, "exclude-standard", &opt_exclude,
-			    N_("search also in ignored files"), 1),
+			    N_("ignore files specified via '.gitignore'"), 1),
 		OPT_GROUP(""),
 		OPT_BOOL('v', "invert-match", &opt.invert,
 			N_("show non-matching lines")),
-- 
2.3.0.rc1.137.g477eb31

Re: [PATCH] grep: correct help string for --exclude-standard

[Jeff King]

On Fri, Feb 27, 2015 at 09:01:58PM +0700, Nguyễn Thái Ngọc Duy wrote:

> The current help string is about --no-exclude-standard. But "git grep -h"
> would show --exclude-standard instead. Flip the string. See 0a93fb8
> (grep: teach --untracked and --exclude-standard options - 2011-09-27)
> for more info about these options.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/grep.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 4063882..e77f7cf 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -641,7 +641,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  		OPT_BOOL(0, "untracked", &untracked,
>  			N_("search in both tracked and untracked files")),
>  		OPT_SET_INT(0, "exclude-standard", &opt_exclude,
> -			    N_("search also in ignored files"), 1),
> +			    N_("ignore files specified via '.gitignore'"), 1),

Hmm. If the default is "--exclude-standard", then what expect people to
use is "--no-exclude-standard". Would it make more sense to list that in
the "-h" output? Sadly I think to do that you have to manually specify
"--no-exclude-standard" with OPT_NONEG, and then manually specify
"--exclude-standard" in addition with OPT_HIDDEN.

It might be nice if parseopt had a PARSE_OPT_NEGHELP option or something
to show the "--no-" form. Something like:

diff --git a/builtin/grep.c b/builtin/grep.c
index 9262b73..c03c3e7 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -640,8 +640,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			 N_("find in contents not managed by git"), 1),
 		OPT_BOOL(0, "untracked", &untracked,
 			N_("search in both tracked and untracked files")),
-		OPT_SET_INT(0, "exclude-standard", &opt_exclude,
-			    N_("search also in ignored files"), 1),
+		{ OPTION_SET_INT, 0, "exclude-standard", &opt_exclude,
+		  NULL, N_("search also in ignored files"),
+		  PARSE_OPT_NOARG | PARSE_OPT_NEGHELP,
+		  NULL, 1 },
 		OPT_GROUP(""),
 		OPT_BOOL('v', "invert-match", &opt.invert,
 			N_("show non-matching lines")),
diff --git a/parse-options.c b/parse-options.c
index 80106c0..0ba7dc4 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -599,8 +599,12 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 		}
 		if (opts->long_name && opts->short_name)
 			pos += fprintf(outfile, ", ");
-		if (opts->long_name)
-			pos += fprintf(outfile, "--%s", opts->long_name);
+		if (opts->long_name) {
+			int neg = opts->flags & PARSE_OPT_NEGHELP;
+			pos += fprintf(outfile, "--%s%s",
+				       neg ? "no-" : "",
+				       opts->long_name);
+		}
 		if (opts->type == OPTION_NUMBER)
 			pos += utf8_fprintf(outfile, _("-NUM"));
 
diff --git a/parse-options.h b/parse-options.h
index 7940bc7..e688c32 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -37,6 +37,7 @@ enum parse_opt_option_flags {
 	PARSE_OPT_LASTARG_DEFAULT = 16,
 	PARSE_OPT_NODASH = 32,
 	PARSE_OPT_LITERAL_ARGHELP = 64,
+	PARSE_OPT_NEGHELP = 128,
 	PARSE_OPT_SHELL_EVAL = 256
 };
 

Though it is annoying that we have to give up the nice OPT_SET_INT macro
to specify an extra flag.

-Peff

Re: [PATCH] grep: correct help string for --exclude-standard

[Duy Nguyen]

On Wed, Mar 4, 2015 at 5:16 PM, Jeff King <peff@peff.net> wrote:
> Hmm. If the default is "--exclude-standard", then what expect people to
> use is "--no-exclude-standard". Would it make more sense to list that in
> the "-h" output?

I thought about it and actually edited git-grep man page to clarify
the default, only to find out this.  When --untracked is used,
--exclude-standard is the default. When --no-index is used,
--no-exclude-standard is the default. Can't have it both ways. This is
already mentioned with the subtle phrase "only useful with...".

> It might be nice if parseopt had a PARSE_OPT_NEGHELP option or something
> to show the "--no-" form.

Regardless, yes it would be nice to have something like this. I think
there are places that can make use of this.
-- 
Duy

Re: [PATCH] grep: correct help string for --exclude-standard

[Jeff King]

On Wed, Mar 04, 2015 at 06:13:32PM +0700, Duy Nguyen wrote:

> On Wed, Mar 4, 2015 at 5:16 PM, Jeff King <peff@peff.net> wrote:
> > Hmm. If the default is "--exclude-standard", then what expect people to
> > use is "--no-exclude-standard". Would it make more sense to list that in
> > the "-h" output?
> 
> I thought about it and actually edited git-grep man page to clarify
> the default, only to find out this.  When --untracked is used,
> --exclude-standard is the default. When --no-index is used,
> --no-exclude-standard is the default. Can't have it both ways. This is
> already mentioned with the subtle phrase "only useful with...".

Yuck. :)

I agree your patch is the right thing to do, then.

> > It might be nice if parseopt had a PARSE_OPT_NEGHELP option or something
> > to show the "--no-" form.
> 
> Regardless, yes it would be nice to have something like this. I think
> there are places that can make use of this.

Grepping around, it looks like the best form would be an OPT_NEGBOOL
that acts like a boolean but negates the truth value, and advertises the
negative form. We have a lot of:

  OPT_BOOL('n', "no-checkout", &option_no_checkout,
           N_("don't create a checkout"))

where countermanding an earlier "--no-checkout" has to be spelled as
"--no-no-checkout", rather than "--checkout". If we could write:

  OPT_NEGBOOL('n', "checkout", ...)

that would be nicer. But the short option is a bit weird. We'd want:

  -n: option_no_checkout=true
  --checkout: option_no_checkout=false
  --no-checkout: option_no_checkout=true

That is, we flip the sense of the long option, but the short option
still yields "true". I think that would be useful, but it sure is weird
to explain.

-Peff

Re: [PATCH] grep: correct help string for --exclude-standard

[Duy Nguyen]

On Wed, Mar 4, 2015 at 6:26 PM, Jeff King <peff@peff.net> wrote:
>> > It might be nice if parseopt had a PARSE_OPT_NEGHELP option or something
>> > to show the "--no-" form.
>>
>> Regardless, yes it would be nice to have something like this. I think
>> there are places that can make use of this.
>
> Grepping around, it looks like the best form would be an OPT_NEGBOOL
> that acts like a boolean but negates the truth value, and advertises the
> negative form. We have a lot of:
>
>   OPT_BOOL('n', "no-checkout", &option_no_checkout,
>            N_("don't create a checkout"))
>
> where countermanding an earlier "--no-checkout" has to be spelled as
> "--no-no-checkout", rather than "--checkout". If we could write:
>
>   OPT_NEGBOOL('n', "checkout", ...)
>
> that would be nicer. But the short option is a bit weird. We'd want:
>
>   -n: option_no_checkout=true
>   --checkout: option_no_checkout=false
>   --no-checkout: option_no_checkout=true
>
> That is, we flip the sense of the long option, but the short option
> still yields "true". I think that would be useful, but it sure is weird
> to explain.

Yeah it looks confusing.. How about leaving that first arg as the
short option "checkout" and move 'n' elsewhere? Something like this

OPT_NEGBOOL(0, "checkout", 'n', ....)
-- 
Duy