threads / patch / 55047

patchgrep/log: remove hidden --debug and --grep-debug options

Subject: [PATCH] grep/log: remove hidden --debug and --grep-debug options

## tl;dr

3 messages between Jan 25, 2021 and Jan 26, 2021. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Ævar Arnfjörð Bjarmason· Jan 25, 2021, 23:36 UTC · lore

Remove the hidden "grep --debug" and "log --grep-debug" options added in 17bf35a3c7b (grep: teach --debug option to dump the parse tree, 2012-09-13).

At the time these options seem to have been intended to go along with a documentation discussion and to help the author of relevant tests to perform ad-hoc debugging on them[1].

Reasons to want this gone:
 1. They were never documented, and the only (rather trivial) use of
    them in our own codebase for testing is something I removed back
    in e01b4dab01e (grep: change non-ASCII -i test to stop using
    --debug, 2017-05-20).
 2. Googling around doesn't show any in-the-wild uses I could dig up,
    and on the Git ML the only mentions after the original discussion
    seem to have been when they came up in unrelated diff contexts, or
    that test commit of mine.
 3. An exception to that is c581e4a7499 (grep: under --debug, show
    whether PCRE JIT is enabled, 2019-08-18) where we added the
    ability to dump out when PCREv2 has the JIT in effect.
    The combination of that and my earlier b65abcafc7a (grep: use PCRE
    v2 for optimized fixed-string search, 2019-07-01) means Git prints
    this out in its most common in-the-wild configuration:
        $ git log  --grep-debug --grep=foo --grep=bar --grep=baz --all-match
        pcre2_jit_on=1
        pcre2_jit_on=1
        pcre2_jit_on=1
        [all-match]
        (or
         pattern_body<body>foo
         (or
          pattern_body<body>bar
          pattern_body<body>baz
         )
        )
        $ git grep --debug \( -e foo --and -e bar \) --or -e baz
        pcre2_jit_on=1
        pcre2_jit_on=1
        pcre2_jit_on=1
        (or
         (and
          patternfoo
          patternbar
         )
         patternbaz
        )

I.e. for each pattern we're considering for the and/or/--all-match etc. debugging we'll now diligently spew out another identical line saying whether the PCREv2 JIT is on or not.

I think that nobody's complained about that rather glaringly obviously bad output says something about how much this is used, i.e. it's not.

The need for this debugging aid for the composed grep/log patterns seems to have passed, and the desire to dump the JIT config seems to have been another one-off around the time we had JIT-related issues on the PCREv2 codepath. That the original author of this debugging facility seemingly hasn't noticed the bad output since then[2] is probably some indicator.

1. https://lore.kernel.org/git/cover.1347615361.git.git@drmicha.warpmail.net/
2. https://lore.kernel.org/git/xmqqk1b8x0ac.fsf@gitster-ct.c.googlers.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c |   5 ---
 grep.c         | 101 +------------------------------------------------
 grep.h         |   1 -
 revision.c     |   2 -
 4 files changed, 2 insertions(+), 107 deletions(-)
Show changes to 4 files +2 −107

builtin/grep.c, grep.c, grep.h, revision.c

diff --git a/builtin/grep.c b/builtin/grep.c
index ca259af4416..55d06c95130 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -216,8 +216,6 @@ static void start_threads(struct grep_opt *opt)
 		int err;
 		struct grep_opt *o = grep_opt_dup(opt);
 		o->output = strbuf_out;
-		if (i)
-			o->debug = 0;
 		compile_grep_patterns(o);
 		err = pthread_create(&threads[i], NULL, run, o);
 
@@ -936,9 +934,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			   N_("indicate hit with exit status without output")),
 		OPT_BOOL(0, "all-match", &opt.all_match,
 			N_("show only matches from files that match all patterns")),
-		OPT_SET_INT_F(0, "debug", &opt.debug,
-			      N_("show parse tree for grep expression"),
-			      1, PARSE_OPT_HIDDEN),
 		OPT_GROUP(""),
 		{ OPTION_STRING, 'O', "open-files-in-pager", &show_in_pager,
 			N_("pager"), N_("show matching files in the pager"),
diff --git a/grep.c b/grep.c
index efeb6dc58db..21f0ee03be9 100644
--- a/grep.c
+++ b/grep.c
@@ -400,8 +400,6 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)
 
 #if defined(PCRE_CONFIG_JIT) && !defined(NO_LIBPCRE1_JIT)
 	pcre_config(PCRE_CONFIG_JIT, &p->pcre1_jit_on);
-	if (opt->debug)
-		fprintf(stderr, "pcre1_jit_on=%d\n", p->pcre1_jit_on);
 
 	if (p->pcre1_jit_on)
 		study_options = PCRE_STUDY_JIT_COMPILE;
@@ -508,8 +506,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 	}
 
 	pcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on);
-	if (opt->debug)
-		fprintf(stderr, "pcre2_jit_on=%d\n", p->pcre2_jit_on);
 	if (p->pcre2_jit_on) {
 		jitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);
 		if (jitret)
@@ -535,9 +531,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 			BUG("pcre2_pattern_info() failed: %d", patinforet);
 		if (jitsizearg == 0) {
 			p->pcre2_jit_on = 0;
-			if (opt->debug)
-				fprintf(stderr, "pcre2_jit_on=%d: (*NO_JIT) in regex\n",
-					p->pcre2_jit_on);
 			return;
 		}
 	}
@@ -616,8 +609,6 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)
 	if (opt->ignore_case)
 		regflags |= REG_ICASE;
 	err = regcomp(&p->regexp, sb.buf, regflags);
-	if (opt->debug)
-		fprintf(stderr, "fixed %s\n", sb.buf);
 	strbuf_release(&sb);
 	if (err) {
 		char errbuf[1024];
@@ -812,87 +803,6 @@ static struct grep_expr *compile_pattern_expr(struct grep_pat **list)
 	return compile_pattern_or(list);
 }
 
-static void indent(int in)
-{
-	while (in-- > 0)
-		fputc(' ', stderr);
-}
-
-static void dump_grep_pat(struct grep_pat *p)
-{
-	switch (p->token) {
-	case GREP_AND: fprintf(stderr, "*and*"); break;
-	case GREP_OPEN_PAREN: fprintf(stderr, "*(*"); break;
-	case GREP_CLOSE_PAREN: fprintf(stderr, "*)*"); break;
-	case GREP_NOT: fprintf(stderr, "*not*"); break;
-	case GREP_OR: fprintf(stderr, "*or*"); break;
-
-	case GREP_PATTERN: fprintf(stderr, "pattern"); break;
-	case GREP_PATTERN_HEAD: fprintf(stderr, "pattern_head"); break;
-	case GREP_PATTERN_BODY: fprintf(stderr, "pattern_body"); break;
-	}
-
-	switch (p->token) {
-	default: break;
-	case GREP_PATTERN_HEAD:
-		fprintf(stderr, "<head %d>", p->field); break;
-	case GREP_PATTERN_BODY:
-		fprintf(stderr, "<body>"); break;
-	}
-	switch (p->token) {
-	default: break;
-	case GREP_PATTERN_HEAD:
-	case GREP_PATTERN_BODY:
-	case GREP_PATTERN:
-		fprintf(stderr, "%.*s", (int)p->patternlen, p->pattern);
-		break;
-	}
-	fputc('\n', stderr);
-}
-
-static void dump_grep_expression_1(struct grep_expr *x, int in)
-{
-	indent(in);
-	switch (x->node) {
-	case GREP_NODE_TRUE:
-		fprintf(stderr, "true\n");
-		break;
-	case GREP_NODE_ATOM:
-		dump_grep_pat(x->u.atom);
-		break;
-	case GREP_NODE_NOT:
-		fprintf(stderr, "(not\n");
-		dump_grep_expression_1(x->u.unary, in+1);
-		indent(in);
-		fprintf(stderr, ")\n");
-		break;
-	case GREP_NODE_AND:
-		fprintf(stderr, "(and\n");
-		dump_grep_expression_1(x->u.binary.left, in+1);
-		dump_grep_expression_1(x->u.binary.right, in+1);
-		indent(in);
-		fprintf(stderr, ")\n");
-		break;
-	case GREP_NODE_OR:
-		fprintf(stderr, "(or\n");
-		dump_grep_expression_1(x->u.binary.left, in+1);
-		dump_grep_expression_1(x->u.binary.right, in+1);
-		indent(in);
-		fprintf(stderr, ")\n");
-		break;
-	}
-}
-
-static void dump_grep_expression(struct grep_opt *opt)
-{
-	struct grep_expr *x = opt->pattern_expression;
-
-	if (opt->all_match)
-		fprintf(stderr, "[all-match]\n");
-	dump_grep_expression_1(x, 0);
-	fflush(NULL);
-}
-
 static struct grep_expr *grep_true_expr(void)
 {
 	struct grep_expr *z = xcalloc(1, sizeof(*z));
@@ -973,7 +883,7 @@ static struct grep_expr *grep_splice_or(struct grep_expr *x, struct grep_expr *y
 	return z;
 }
 
-static void compile_grep_patterns_real(struct grep_opt *opt)
+void compile_grep_patterns(struct grep_opt *opt)
 {
 	struct grep_pat *p;
 	struct grep_expr *header_expr = prep_header_patterns(opt);
@@ -993,7 +903,7 @@ static void compile_grep_patterns_real(struct grep_opt *opt)
 
 	if (opt->all_match || header_expr)
 		opt->extended = 1;
-	else if (!opt->extended && !opt->debug)
+	else if (!opt->extended)
 		return;
 
 	p = opt->pattern_list;
@@ -1016,13 +926,6 @@ static void compile_grep_patterns_real(struct grep_opt *opt)
 	opt->all_match = 1;
 }
 
-void compile_grep_patterns(struct grep_opt *opt)
-{
-	compile_grep_patterns_real(opt);
-	if (opt->debug)
-		dump_grep_expression(opt);
-}
-
 static void free_pattern_expr(struct grep_expr *x)
 {
 	switch (x->node) {
diff --git a/grep.h b/grep.h
index b5c4e223a8f..5248c6ef7ea 100644
--- a/grep.h
+++ b/grep.h
@@ -136,7 +136,6 @@ struct grep_opt {
 	int word_regexp;
 	int fixed;
 	int all_match;
-	int debug;
 #define GREP_BINARY_DEFAULT	0
 #define GREP_BINARY_NOMATCH	1
 #define GREP_BINARY_TEXT	2
diff --git a/revision.c b/revision.c
index 1bb590ece78..657e5502532 100644
--- a/revision.c
+++ b/revision.c
@@ -2489,8 +2489,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if ((argcount = parse_long_opt("grep", argv, &optarg))) {
 		add_message_grep(revs, optarg);
 		return argcount;
-	} else if (!strcmp(arg, "--grep-debug")) {
-		revs->grep_filter.debug = 1;
 	} else if (!strcmp(arg, "--basic-regexp")) {
 		revs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_BRE;
 	} else if (!strcmp(arg, "--extended-regexp") || !strcmp(arg, "-E")) {
-- 
2.29.2.222.g5d2a92d10f8
Junio C Hamano· Jan 26, 2021, 19:35 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] grep/log: remove hidden --debug and --grep-debug options

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 19 quoted lines
> Remove the hidden "grep --debug" and "log --grep-debug" options added
> in 17bf35a3c7b (grep: teach --debug option to dump the parse tree,
> 2012-09-13).
>
> At the time these options seem to have been intended to go along with
> a documentation discussion and to help the author of relevant tests to
> perform ad-hoc debugging on them[1].
>
> Reasons to want this gone:
>
>  1. They were never documented, and the only (rather trivial) use of
>     them in our own codebase for testing is something I removed back
>     in e01b4dab01e (grep: change non-ASCII -i test to stop using
>     --debug, 2017-05-20).
>
>  2. Googling around doesn't show any in-the-wild uses I could dig up,
>     and on the Git ML the only mentions after the original discussion
>     seem to have been when they came up in unrelated diff contexts, or
>     that test commit of mine.

I do not expect anybody uses it; just get rid of it and we'd be fine.

Show 17 quoted lines
> The need for this debugging aid for the composed grep/log patterns
> seems to have passed, and the desire to dump the JIT config seems to
> have been another one-off around the time we had JIT-related issues on
> the PCREv2 codepath. That the original author of this debugging
> facility seemingly hasn't noticed the bad output since then[2] is
> probably some indicator.
>
> 1. https://lore.kernel.org/git/cover.1347615361.git.git@drmicha.warpmail.net/
> 2. https://lore.kernel.org/git/xmqqk1b8x0ac.fsf@gitster-ct.c.googlers.com/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/grep.c |   5 ---
>  grep.c         | 101 +------------------------------------------------
>  grep.h         |   1 -
>  revision.c     |   2 -
>  4 files changed, 2 insertions(+), 107 deletions(-)
Thanks.

Loss of a member in the struct is a sure way to ensure that all codepaths are covered or we would quickly see that compilers barf. The changes look good.

Show 209 quoted lines
> diff --git a/builtin/grep.c b/builtin/grep.c
> index ca259af4416..55d06c95130 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -216,8 +216,6 @@ static void start_threads(struct grep_opt *opt)
>  		int err;
>  		struct grep_opt *o = grep_opt_dup(opt);
>  		o->output = strbuf_out;
> -		if (i)
> -			o->debug = 0;
>  		compile_grep_patterns(o);
>  		err = pthread_create(&threads[i], NULL, run, o);
>  
> @@ -936,9 +934,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  			   N_("indicate hit with exit status without output")),
>  		OPT_BOOL(0, "all-match", &opt.all_match,
>  			N_("show only matches from files that match all patterns")),
> -		OPT_SET_INT_F(0, "debug", &opt.debug,
> -			      N_("show parse tree for grep expression"),
> -			      1, PARSE_OPT_HIDDEN),
>  		OPT_GROUP(""),
>  		{ OPTION_STRING, 'O', "open-files-in-pager", &show_in_pager,
>  			N_("pager"), N_("show matching files in the pager"),
> diff --git a/grep.c b/grep.c
> index efeb6dc58db..21f0ee03be9 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -400,8 +400,6 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)
>  
>  #if defined(PCRE_CONFIG_JIT) && !defined(NO_LIBPCRE1_JIT)
>  	pcre_config(PCRE_CONFIG_JIT, &p->pcre1_jit_on);
> -	if (opt->debug)
> -		fprintf(stderr, "pcre1_jit_on=%d\n", p->pcre1_jit_on);
>  
>  	if (p->pcre1_jit_on)
>  		study_options = PCRE_STUDY_JIT_COMPILE;
> @@ -508,8 +506,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
>  	}
>  
>  	pcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on);
> -	if (opt->debug)
> -		fprintf(stderr, "pcre2_jit_on=%d\n", p->pcre2_jit_on);
>  	if (p->pcre2_jit_on) {
>  		jitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);
>  		if (jitret)
> @@ -535,9 +531,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
>  			BUG("pcre2_pattern_info() failed: %d", patinforet);
>  		if (jitsizearg == 0) {
>  			p->pcre2_jit_on = 0;
> -			if (opt->debug)
> -				fprintf(stderr, "pcre2_jit_on=%d: (*NO_JIT) in regex\n",
> -					p->pcre2_jit_on);
>  			return;
>  		}
>  	}
> @@ -616,8 +609,6 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)
>  	if (opt->ignore_case)
>  		regflags |= REG_ICASE;
>  	err = regcomp(&p->regexp, sb.buf, regflags);
> -	if (opt->debug)
> -		fprintf(stderr, "fixed %s\n", sb.buf);
>  	strbuf_release(&sb);
>  	if (err) {
>  		char errbuf[1024];
> @@ -812,87 +803,6 @@ static struct grep_expr *compile_pattern_expr(struct grep_pat **list)
>  	return compile_pattern_or(list);
>  }
>  
> -static void indent(int in)
> -{
> -	while (in-- > 0)
> -		fputc(' ', stderr);
> -}
> -
> -static void dump_grep_pat(struct grep_pat *p)
> -{
> -	switch (p->token) {
> -	case GREP_AND: fprintf(stderr, "*and*"); break;
> -	case GREP_OPEN_PAREN: fprintf(stderr, "*(*"); break;
> -	case GREP_CLOSE_PAREN: fprintf(stderr, "*)*"); break;
> -	case GREP_NOT: fprintf(stderr, "*not*"); break;
> -	case GREP_OR: fprintf(stderr, "*or*"); break;
> -
> -	case GREP_PATTERN: fprintf(stderr, "pattern"); break;
> -	case GREP_PATTERN_HEAD: fprintf(stderr, "pattern_head"); break;
> -	case GREP_PATTERN_BODY: fprintf(stderr, "pattern_body"); break;
> -	}
> -
> -	switch (p->token) {
> -	default: break;
> -	case GREP_PATTERN_HEAD:
> -		fprintf(stderr, "<head %d>", p->field); break;
> -	case GREP_PATTERN_BODY:
> -		fprintf(stderr, "<body>"); break;
> -	}
> -	switch (p->token) {
> -	default: break;
> -	case GREP_PATTERN_HEAD:
> -	case GREP_PATTERN_BODY:
> -	case GREP_PATTERN:
> -		fprintf(stderr, "%.*s", (int)p->patternlen, p->pattern);
> -		break;
> -	}
> -	fputc('\n', stderr);
> -}
> -
> -static void dump_grep_expression_1(struct grep_expr *x, int in)
> -{
> -	indent(in);
> -	switch (x->node) {
> -	case GREP_NODE_TRUE:
> -		fprintf(stderr, "true\n");
> -		break;
> -	case GREP_NODE_ATOM:
> -		dump_grep_pat(x->u.atom);
> -		break;
> -	case GREP_NODE_NOT:
> -		fprintf(stderr, "(not\n");
> -		dump_grep_expression_1(x->u.unary, in+1);
> -		indent(in);
> -		fprintf(stderr, ")\n");
> -		break;
> -	case GREP_NODE_AND:
> -		fprintf(stderr, "(and\n");
> -		dump_grep_expression_1(x->u.binary.left, in+1);
> -		dump_grep_expression_1(x->u.binary.right, in+1);
> -		indent(in);
> -		fprintf(stderr, ")\n");
> -		break;
> -	case GREP_NODE_OR:
> -		fprintf(stderr, "(or\n");
> -		dump_grep_expression_1(x->u.binary.left, in+1);
> -		dump_grep_expression_1(x->u.binary.right, in+1);
> -		indent(in);
> -		fprintf(stderr, ")\n");
> -		break;
> -	}
> -}
> -
> -static void dump_grep_expression(struct grep_opt *opt)
> -{
> -	struct grep_expr *x = opt->pattern_expression;
> -
> -	if (opt->all_match)
> -		fprintf(stderr, "[all-match]\n");
> -	dump_grep_expression_1(x, 0);
> -	fflush(NULL);
> -}
> -
>  static struct grep_expr *grep_true_expr(void)
>  {
>  	struct grep_expr *z = xcalloc(1, sizeof(*z));
> @@ -973,7 +883,7 @@ static struct grep_expr *grep_splice_or(struct grep_expr *x, struct grep_expr *y
>  	return z;
>  }
>  
> -static void compile_grep_patterns_real(struct grep_opt *opt)
> +void compile_grep_patterns(struct grep_opt *opt)
>  {
>  	struct grep_pat *p;
>  	struct grep_expr *header_expr = prep_header_patterns(opt);
> @@ -993,7 +903,7 @@ static void compile_grep_patterns_real(struct grep_opt *opt)
>  
>  	if (opt->all_match || header_expr)
>  		opt->extended = 1;
> -	else if (!opt->extended && !opt->debug)
> +	else if (!opt->extended)
>  		return;
>  
>  	p = opt->pattern_list;
> @@ -1016,13 +926,6 @@ static void compile_grep_patterns_real(struct grep_opt *opt)
>  	opt->all_match = 1;
>  }
>  
> -void compile_grep_patterns(struct grep_opt *opt)
> -{
> -	compile_grep_patterns_real(opt);
> -	if (opt->debug)
> -		dump_grep_expression(opt);
> -}
> -
>  static void free_pattern_expr(struct grep_expr *x)
>  {
>  	switch (x->node) {
> diff --git a/grep.h b/grep.h
> index b5c4e223a8f..5248c6ef7ea 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -136,7 +136,6 @@ struct grep_opt {
>  	int word_regexp;
>  	int fixed;
>  	int all_match;
> -	int debug;
>  #define GREP_BINARY_DEFAULT	0
>  #define GREP_BINARY_NOMATCH	1
>  #define GREP_BINARY_TEXT	2
> diff --git a/revision.c b/revision.c
> index 1bb590ece78..657e5502532 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2489,8 +2489,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>  	} else if ((argcount = parse_long_opt("grep", argv, &optarg))) {
>  		add_message_grep(revs, optarg);
>  		return argcount;
> -	} else if (!strcmp(arg, "--grep-debug")) {
> -		revs->grep_filter.debug = 1;
>  	} else if (!strcmp(arg, "--basic-regexp")) {
>  		revs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_BRE;
>  	} else if (!strcmp(arg, "--extended-regexp") || !strcmp(arg, "-E")) {
Michael J Gruber· Jan 26, 2021, 20:27 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] grep/log: remove hidden --debug and --grep-debug options

Ævar Arnfjörð Bjarmason venit, vidit, dixit 2021-01-26 00:36:51:
Show 7 quoted lines
> Remove the hidden "grep --debug" and "log --grep-debug" options added
> in 17bf35a3c7b (grep: teach --debug option to dump the parse tree,
> 2012-09-13).
> 
> At the time these options seem to have been intended to go along with
> a documentation discussion and to help the author of relevant tests to
> perform ad-hoc debugging on them[1].
I'm not actively working on these tests right now, but thanks for asking ;)
Show 60 quoted lines
> 
> Reasons to want this gone:
> 
>  1. They were never documented, and the only (rather trivial) use of
>     them in our own codebase for testing is something I removed back
>     in e01b4dab01e (grep: change non-ASCII -i test to stop using
>     --debug, 2017-05-20).
> 
>  2. Googling around doesn't show any in-the-wild uses I could dig up,
>     and on the Git ML the only mentions after the original discussion
>     seem to have been when they came up in unrelated diff contexts, or
>     that test commit of mine.
> 
>  3. An exception to that is c581e4a7499 (grep: under --debug, show
>     whether PCRE JIT is enabled, 2019-08-18) where we added the
>     ability to dump out when PCREv2 has the JIT in effect.
> 
>     The combination of that and my earlier b65abcafc7a (grep: use PCRE
>     v2 for optimized fixed-string search, 2019-07-01) means Git prints
>     this out in its most common in-the-wild configuration:
> 
>         $ git log  --grep-debug --grep=foo --grep=bar --grep=baz --all-match
>         pcre2_jit_on=1
>         pcre2_jit_on=1
>         pcre2_jit_on=1
>         [all-match]
>         (or
>          pattern_body<body>foo
>          (or
>           pattern_body<body>bar
>           pattern_body<body>baz
>          )
>         )
> 
>         $ git grep --debug \( -e foo --and -e bar \) --or -e baz
>         pcre2_jit_on=1
>         pcre2_jit_on=1
>         pcre2_jit_on=1
>         (or
>          (and
>           patternfoo
>           patternbar
>          )
>          patternbaz
>         )
> 
> I.e. for each pattern we're considering for the and/or/--all-match
> etc. debugging we'll now diligently spew out another identical line
> saying whether the PCREv2 JIT is on or not.
> 
> I think that nobody's complained about that rather glaringly obviously
> bad output says something about how much this is used, i.e. it's
> not.
> 
> The need for this debugging aid for the composed grep/log patterns
> seems to have passed, and the desire to dump the JIT config seems to
> have been another one-off around the time we had JIT-related issues on
> the PCREv2 codepath. That the original author of this debugging
> facility seemingly hasn't noticed the bad output since then[2] is
> probably some indicator.
I agree completely with this analysis.

Cheers Michael

Show 223 quoted lines
> 1. https://lore.kernel.org/git/cover.1347615361.git.git@drmicha.warpmail.net/
> 2. https://lore.kernel.org/git/xmqqk1b8x0ac.fsf@gitster-ct.c.googlers.com/
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/grep.c |   5 ---
>  grep.c         | 101 +------------------------------------------------
>  grep.h         |   1 -
>  revision.c     |   2 -
>  4 files changed, 2 insertions(+), 107 deletions(-)
> 
> diff --git a/builtin/grep.c b/builtin/grep.c
> index ca259af4416..55d06c95130 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -216,8 +216,6 @@ static void start_threads(struct grep_opt *opt)
>                 int err;
>                 struct grep_opt *o = grep_opt_dup(opt);
>                 o->output = strbuf_out;
> -               if (i)
> -                       o->debug = 0;
>                 compile_grep_patterns(o);
>                 err = pthread_create(&threads[i], NULL, run, o);
>  
> @@ -936,9 +934,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>                            N_("indicate hit with exit status without output")),
>                 OPT_BOOL(0, "all-match", &opt.all_match,
>                         N_("show only matches from files that match all patterns")),
> -               OPT_SET_INT_F(0, "debug", &opt.debug,
> -                             N_("show parse tree for grep expression"),
> -                             1, PARSE_OPT_HIDDEN),
>                 OPT_GROUP(""),
>                 { OPTION_STRING, 'O', "open-files-in-pager", &show_in_pager,
>                         N_("pager"), N_("show matching files in the pager"),
> diff --git a/grep.c b/grep.c
> index efeb6dc58db..21f0ee03be9 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -400,8 +400,6 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)
>  
>  #if defined(PCRE_CONFIG_JIT) && !defined(NO_LIBPCRE1_JIT)
>         pcre_config(PCRE_CONFIG_JIT, &p->pcre1_jit_on);
> -       if (opt->debug)
> -               fprintf(stderr, "pcre1_jit_on=%d\n", p->pcre1_jit_on);
>  
>         if (p->pcre1_jit_on)
>                 study_options = PCRE_STUDY_JIT_COMPILE;
> @@ -508,8 +506,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
>         }
>  
>         pcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on);
> -       if (opt->debug)
> -               fprintf(stderr, "pcre2_jit_on=%d\n", p->pcre2_jit_on);
>         if (p->pcre2_jit_on) {
>                 jitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);
>                 if (jitret)
> @@ -535,9 +531,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
>                         BUG("pcre2_pattern_info() failed: %d", patinforet);
>                 if (jitsizearg == 0) {
>                         p->pcre2_jit_on = 0;
> -                       if (opt->debug)
> -                               fprintf(stderr, "pcre2_jit_on=%d: (*NO_JIT) in regex\n",
> -                                       p->pcre2_jit_on);
>                         return;
>                 }
>         }
> @@ -616,8 +609,6 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)
>         if (opt->ignore_case)
>                 regflags |= REG_ICASE;
>         err = regcomp(&p->regexp, sb.buf, regflags);
> -       if (opt->debug)
> -               fprintf(stderr, "fixed %s\n", sb.buf);
>         strbuf_release(&sb);
>         if (err) {
>                 char errbuf[1024];
> @@ -812,87 +803,6 @@ static struct grep_expr *compile_pattern_expr(struct grep_pat **list)
>         return compile_pattern_or(list);
>  }
>  
> -static void indent(int in)
> -{
> -       while (in-- > 0)
> -               fputc(' ', stderr);
> -}
> -
> -static void dump_grep_pat(struct grep_pat *p)
> -{
> -       switch (p->token) {
> -       case GREP_AND: fprintf(stderr, "*and*"); break;
> -       case GREP_OPEN_PAREN: fprintf(stderr, "*(*"); break;
> -       case GREP_CLOSE_PAREN: fprintf(stderr, "*)*"); break;
> -       case GREP_NOT: fprintf(stderr, "*not*"); break;
> -       case GREP_OR: fprintf(stderr, "*or*"); break;
> -
> -       case GREP_PATTERN: fprintf(stderr, "pattern"); break;
> -       case GREP_PATTERN_HEAD: fprintf(stderr, "pattern_head"); break;
> -       case GREP_PATTERN_BODY: fprintf(stderr, "pattern_body"); break;
> -       }
> -
> -       switch (p->token) {
> -       default: break;
> -       case GREP_PATTERN_HEAD:
> -               fprintf(stderr, "<head %d>", p->field); break;
> -       case GREP_PATTERN_BODY:
> -               fprintf(stderr, "<body>"); break;
> -       }
> -       switch (p->token) {
> -       default: break;
> -       case GREP_PATTERN_HEAD:
> -       case GREP_PATTERN_BODY:
> -       case GREP_PATTERN:
> -               fprintf(stderr, "%.*s", (int)p->patternlen, p->pattern);
> -               break;
> -       }
> -       fputc('\n', stderr);
> -}
> -
> -static void dump_grep_expression_1(struct grep_expr *x, int in)
> -{
> -       indent(in);
> -       switch (x->node) {
> -       case GREP_NODE_TRUE:
> -               fprintf(stderr, "true\n");
> -               break;
> -       case GREP_NODE_ATOM:
> -               dump_grep_pat(x->u.atom);
> -               break;
> -       case GREP_NODE_NOT:
> -               fprintf(stderr, "(not\n");
> -               dump_grep_expression_1(x->u.unary, in+1);
> -               indent(in);
> -               fprintf(stderr, ")\n");
> -               break;
> -       case GREP_NODE_AND:
> -               fprintf(stderr, "(and\n");
> -               dump_grep_expression_1(x->u.binary.left, in+1);
> -               dump_grep_expression_1(x->u.binary.right, in+1);
> -               indent(in);
> -               fprintf(stderr, ")\n");
> -               break;
> -       case GREP_NODE_OR:
> -               fprintf(stderr, "(or\n");
> -               dump_grep_expression_1(x->u.binary.left, in+1);
> -               dump_grep_expression_1(x->u.binary.right, in+1);
> -               indent(in);
> -               fprintf(stderr, ")\n");
> -               break;
> -       }
> -}
> -
> -static void dump_grep_expression(struct grep_opt *opt)
> -{
> -       struct grep_expr *x = opt->pattern_expression;
> -
> -       if (opt->all_match)
> -               fprintf(stderr, "[all-match]\n");
> -       dump_grep_expression_1(x, 0);
> -       fflush(NULL);
> -}
> -
>  static struct grep_expr *grep_true_expr(void)
>  {
>         struct grep_expr *z = xcalloc(1, sizeof(*z));
> @@ -973,7 +883,7 @@ static struct grep_expr *grep_splice_or(struct grep_expr *x, struct grep_expr *y
>         return z;
>  }
>  
> -static void compile_grep_patterns_real(struct grep_opt *opt)
> +void compile_grep_patterns(struct grep_opt *opt)
>  {
>         struct grep_pat *p;
>         struct grep_expr *header_expr = prep_header_patterns(opt);
> @@ -993,7 +903,7 @@ static void compile_grep_patterns_real(struct grep_opt *opt)
>  
>         if (opt->all_match || header_expr)
>                 opt->extended = 1;
> -       else if (!opt->extended && !opt->debug)
> +       else if (!opt->extended)
>                 return;
>  
>         p = opt->pattern_list;
> @@ -1016,13 +926,6 @@ static void compile_grep_patterns_real(struct grep_opt *opt)
>         opt->all_match = 1;
>  }
>  
> -void compile_grep_patterns(struct grep_opt *opt)
> -{
> -       compile_grep_patterns_real(opt);
> -       if (opt->debug)
> -               dump_grep_expression(opt);
> -}
> -
>  static void free_pattern_expr(struct grep_expr *x)
>  {
>         switch (x->node) {
> diff --git a/grep.h b/grep.h
> index b5c4e223a8f..5248c6ef7ea 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -136,7 +136,6 @@ struct grep_opt {
>         int word_regexp;
>         int fixed;
>         int all_match;
> -       int debug;
>  #define GREP_BINARY_DEFAULT    0
>  #define GREP_BINARY_NOMATCH    1
>  #define GREP_BINARY_TEXT       2
> diff --git a/revision.c b/revision.c
> index 1bb590ece78..657e5502532 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2489,8 +2489,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>         } else if ((argcount = parse_long_opt("grep", argv, &optarg))) {
>                 add_message_grep(revs, optarg);
>                 return argcount;
> -       } else if (!strcmp(arg, "--grep-debug")) {
> -               revs->grep_filter.debug = 1;
>         } else if (!strcmp(arg, "--basic-regexp")) {
>                 revs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_BRE;
>         } else if (!strcmp(arg, "--extended-regexp") || !strcmp(arg, "-E")) {
> -- 
> 2.29.2.222.g5d2a92d10f8
>

← back to recent threads