[PATCH 0/8] grep: simplify & delete code by changing obscure cfg variable behavior

I'd like to use the grep API for more things in some upcoming
optimization patches, and to do that I need to use grep_init(),
grep_config() etc.

These APIs have a very unusual API, and we've got quite a bit of code
in grep.c to support an obscure edge case in how "grep.extendedRegexp"
behaves when clashing with a "grep.patternType" variable present in
the same config space.

This series is an opinionated change of that behavior, and resulting
large deletion of code.

The series starts out by deleting some unused code in grep.c, and
moving a bit of related builtin/grep.c-specific code to that file, and
out of grep.c.

Ævar Arnfjörð Bjarmason (8):
  grep.h: remove unused "regex_t regexp" from grep_opt
  git.c & grep.c: assert that "prefix" is NULL or non-zero string
  grep: remove unused "prefix_length" member
  grep.c: move "prefix" out of "struct grep_opt"
  log tests: check if grep_config() is called by "log"-like cmds
  grep API: call grep_config() after grep_init()
  grep: simplify config parsing, change grep.<rx config> interaction
  grep: make "extendedRegexp=true" the same as "patternType=extended"

 Documentation/config/grep.txt |   4 +-
 Documentation/git-grep.txt    |   4 +-
 builtin/grep.c                |  31 +++++----
 builtin/log.c                 |  13 +++-
 git.c                         |   4 +-
 grep.c                        | 118 ++++------------------------------
 grep.h                        |  34 ++++++----
 revision.c                    |   4 +-
 t/t4202-log.sh                |  16 +++++
 t/t7810-grep.sh               |   4 +-
 10 files changed, 89 insertions(+), 143 deletions(-)

-- 
2.34.0.rc1.741.gab7bfd97031

This "regex_t" in grep_opt has not been used since
f9b9faf6f8a (builtin-grep: allow more than one patterns., 2006-05-02),
we still use a "regex_t" for compiling regexes, but that's in the
"grep_pat" struct".

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 grep.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/grep.h b/grep.h
index 3e8815c347b..95cccb670f9 100644
--- a/grep.h
+++ b/grep.h

@@ -136,7 +136,6 @@ struct grep_opt {
 
 	const char *prefix;
 	int prefix_length;
-	regex_t regexp;
 	int linenum;
 	int columnnum;
 	int invert;

-- 
2.34.0.rc1.741.gab7bfd97031

The "prefix" we get from setup.c is either going to be NULL or a
string of length >0, never "". So let's drop the "prefix && *prefix"
check in grep.c added in 0d042fecf2f (git-grep: show pathnames
relative to the current directory, 2006-08-11).

As seen in code in revision.c that was added in cd676a51367 (diff
--relative: output paths as relative to the current subdirectory,
2008-02-12) we have existing code that does away with this assertion.

This makes it easier to reason about a subsequent change to the
"prefix_length" code in grep.c in a subsequent commit, and since we're
going to the trouble of doing that let's leave behind an assert() to
promise this to any future callers.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 git.c  | 4 ++--
 grep.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/git.c b/git.c
index 5ff21be21f3..aa4f0d77c4b 100644
--- a/git.c
+++ b/git.c

@@ -420,9 +420,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 {
 	int status, help;
 	struct stat st;
-	const char *prefix;
+	const char *prefix = NULL;
 
-	prefix = NULL;
 	help = argc == 2 && !strcmp(argv[1], "-h");
 	if (!help) {
 		if (p->option & RUN_SETUP)

@@ -431,6 +430,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 			int nongit_ok;
 			prefix = setup_git_directory_gently(&nongit_ok);
 		}
+		assert(!prefix || (prefix && *prefix));
 		precompose_argv_prefix(argc, argv, NULL);
 		if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&
 		    !(p->option & DELAY_PAGER_CONFIG))

diff --git a/grep.c b/grep.c
index f6e113e9f0f..88ebc504630 100644
--- a/grep.c
+++ b/grep.c

@@ -145,7 +145,7 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
 
 	opt->repo = repo;
 	opt->prefix = prefix;
-	opt->prefix_length = (prefix && *prefix) ? strlen(prefix) : 0;
+	opt->prefix_length = prefix ? strlen(prefix) : 0;
 	opt->pattern_tail = &opt->pattern_list;
 	opt->header_tail = &opt->header_list;
 }

-- 
2.34.0.rc1.741.gab7bfd97031

Remove the "prefix_length" member, which we compute with a strlen() on
the "prefix" argument to grep_init(), but whose strlen() hasn't been
used since 493b7a08d80 (grep: accept relative paths outside current
working directory, 2009-09-05).

When this code was added in 0d042fecf2f (git-grep: show pathnames
relative to the current directory, 2006-08-11) we used the length, but
since 493b7a08d80 we haven't used it for anything except a boolean
check that we could have done on the "prefix" member itself.

Before a preceding commit we also used to guard the strlen() with
"prefix && *prefix", but as that commit notes the RHS of that && chain
was also redundant.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/grep.c | 4 ++--
 grep.c         | 1 -
 grep.h         | 1 -
 3 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 9e34a820ad4..bd4d2107351 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -315,7 +315,7 @@ static void grep_source_name(struct grep_opt *opt, const char *filename,
 	strbuf_reset(out);
 
 	if (opt->null_following_name) {
-		if (opt->relative && opt->prefix_length) {
+		if (opt->relative && opt->prefix) {
 			struct strbuf rel_buf = STRBUF_INIT;
 			const char *rel_name =
 				relative_path(filename + tree_name_len,

@@ -332,7 +332,7 @@ static void grep_source_name(struct grep_opt *opt, const char *filename,
 		return;
 	}
 
-	if (opt->relative && opt->prefix_length)
+	if (opt->relative && opt->prefix)
 		quote_path(filename + tree_name_len, opt->prefix, out, 0);
 	else
 		quote_c_style(filename + tree_name_len, out, NULL, 0);

diff --git a/grep.c b/grep.c
index 88ebc504630..755afb5f96d 100644
--- a/grep.c
+++ b/grep.c

@@ -145,7 +145,6 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
 
 	opt->repo = repo;
 	opt->prefix = prefix;
-	opt->prefix_length = prefix ? strlen(prefix) : 0;
 	opt->pattern_tail = &opt->pattern_list;
 	opt->header_tail = &opt->header_list;
 }

diff --git a/grep.h b/grep.h
index 95cccb670f9..467d775b5a9 100644
--- a/grep.h
+++ b/grep.h

@@ -135,7 +135,6 @@ struct grep_opt {
 	struct repository *repo;
 
 	const char *prefix;
-	int prefix_length;
 	int linenum;
 	int columnnum;
 	int invert;

-- 
2.34.0.rc1.741.gab7bfd97031

The "struct grep_opt" is a mixture of things that would be needed by
all callers of the grep.c API, and quite a few things that only the
builtin/grep.c needs.

Since we got rid of "prefix_length" in the previous commit, let's move
the "prefix" variable over to "builtin/grep.c" where it's used. To do
this let's create a "struct grep_cmd_opt", which we'll have a pointer
to in a new "caller_priv" member in "struct grep_opt" (the existing
"priv" is used by the top-level "grep.c" itself).

We might eventually need to have grep_opt_dup() learn about this new
member, but since the prefix can be a "const char *const" (i.e. we
never change it in any way) let's leave that aside for now, in any
case "builtin/grep.c" is the only user of grep_opt_dup(), even though
it lives in the top-level "grep.c".

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/grep.c | 17 ++++++++++++-----
 grep.c         |  3 +--
 grep.h         |  4 ++--
 revision.c     |  2 +-
 4 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index bd4d2107351..960c7aac123 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -37,6 +37,10 @@ static int num_threads;
 
 static pthread_t *threads;
 
+struct grep_cmd_opt {
+	const char *const prefix;
+};
+
 /* We use one producer thread and THREADS consumer
  * threads. The producer adds struct work_items to 'todo' and the
  * consumers pick work items from the same array.

@@ -312,14 +316,15 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)
 static void grep_source_name(struct grep_opt *opt, const char *filename,
 			     int tree_name_len, struct strbuf *out)
 {
+	struct grep_cmd_opt *opt_cmd = opt->caller_priv;
 	strbuf_reset(out);
 
 	if (opt->null_following_name) {
-		if (opt->relative && opt->prefix) {
+		if (opt->relative && opt_cmd->prefix) {
 			struct strbuf rel_buf = STRBUF_INIT;
 			const char *rel_name =
 				relative_path(filename + tree_name_len,
-					      opt->prefix, &rel_buf);
+					      opt_cmd->prefix, &rel_buf);
 
 			if (tree_name_len)
 				strbuf_add(out, filename, tree_name_len);

@@ -332,8 +337,8 @@ static void grep_source_name(struct grep_opt *opt, const char *filename,
 		return;
 	}
 
-	if (opt->relative && opt->prefix)
-		quote_path(filename + tree_name_len, opt->prefix, out, 0);
+	if (opt->relative && opt_cmd->prefix)
+		quote_path(filename + tree_name_len, opt_cmd->prefix, out, 0);
 	else
 		quote_c_style(filename + tree_name_len, out, NULL, 0);
 

@@ -837,6 +842,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	int external_grep_allowed__ignored;
 	const char *show_in_pager = NULL, *default_pager = "dummy";
 	struct grep_opt opt;
+	struct grep_cmd_opt opt_cmd = { .prefix = prefix };
 	struct object_array list = OBJECT_ARRAY_INIT;
 	struct pathspec pathspec;
 	struct string_list path_list = STRING_LIST_INIT_DUP;

@@ -964,7 +970,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	};
 
 	git_config(grep_cmd_config, NULL);
-	grep_init(&opt, the_repository, prefix);
+	grep_init(&opt, the_repository);
+	opt.caller_priv = &opt_cmd;
 
 	/*
 	 * If there is no -- then the paths must exist in the working

diff --git a/grep.c b/grep.c
index 755afb5f96d..c9065254aeb 100644
--- a/grep.c
+++ b/grep.c

@@ -139,12 +139,11 @@ int grep_config(const char *var, const char *value, void *cb)
  * default values from the template we read the configuration
  * information in an earlier call to git_config(grep_config).
  */
-void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
+void grep_init(struct grep_opt *opt, struct repository *repo)
 {
 	*opt = grep_defaults;
 
 	opt->repo = repo;
-	opt->prefix = prefix;
 	opt->pattern_tail = &opt->pattern_list;
 	opt->header_tail = &opt->header_list;
 }

diff --git a/grep.h b/grep.h
index 467d775b5a9..6b923d8599c 100644
--- a/grep.h
+++ b/grep.h

@@ -134,7 +134,6 @@ struct grep_opt {
 	 */
 	struct repository *repo;
 
-	const char *prefix;
 	int linenum;
 	int columnnum;
 	int invert;

@@ -172,6 +171,7 @@ struct grep_opt {
 	int show_hunk_mark;
 	int file_break;
 	int heading;
+	void *caller_priv;
 	void *priv;
 
 	void (*output)(struct grep_opt *opt, const void *data, size_t size);

@@ -179,7 +179,7 @@ struct grep_opt {
 };
 
 int grep_config(const char *var, const char *value, void *);
-void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);
+void grep_init(struct grep_opt *, struct repository *repo);
 void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);
 
 void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);

diff --git a/revision.c b/revision.c
index ab7c1358042..9f9b0d2429e 100644
--- a/revision.c
+++ b/revision.c

@@ -1833,7 +1833,7 @@ void repo_init_revisions(struct repository *r,
 	revs->commit_format = CMIT_FMT_DEFAULT;
 	revs->expand_tabs_in_log_default = 8;
 
-	grep_init(&revs->grep_filter, revs->repo, prefix);
+	grep_init(&revs->grep_filter, revs->repo);
 	revs->grep_filter.status_only = 1;
 
 	repo_diff_setup(revs->repo, &revs->diffopt);

-- 
2.34.0.rc1.741.gab7bfd97031

Extend the tests added in my 9df46763ef1 (log: add exhaustive tests
for pattern style options & config, 2017-05-20) to check not only
whether "git log" handles "grep.patternType", but also "git show"
etc.

It's sufficient to check whether a PCRE regex matches for the purposes
of this test, we otherwise assume that it's running the same code as
"git log", whose behavior is tested more exhaustively by test added in
9df46763ef1e.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 t/t4202-log.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 7884e3d46b3..a114c49ef27 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh

@@ -449,6 +449,22 @@ test_expect_success !FAIL_PREREQS 'log with various grep.patternType configurati
 	)
 '
 
+for cmd in show whatchanged reflog format-patch
+do
+	myarg=
+	if test "$cmd" = "format-patch"
+	then
+		myarg="HEAD~.."
+	fi
+
+	test_expect_success PCRE "$cmd: understands grep.patternType=perl, like 'log'" '
+		git -c grep.patternType=fixed -C pattern-type $cmd --grep="1(?=\|2)" $myarg >actual &&
+		test_must_be_empty actual &&
+		git -c grep.patternType=perl -C pattern-type $cmd --grep="1(?=\|2)" $myarg >actual &&
+		test_file_not_empty actual
+	'
+done
+
 test_expect_success 'log --author' '
 	cat >expect <<-\EOF &&
 	Author: <BOLD;RED>A U<RESET> Thor <author@example.com>

-- 
2.34.0.rc1.741.gab7bfd97031

The grep_init() function used the odd pattern of initializing the
passed-in "struct grep_opt" with a statically defined "grep_defaults"
struct, which would be modified in-place when we invoked
grep_config().

So we effectively (b) initialized config, (a) then defaults, (c)
followed by user options. Usually those are ordered as "a", "b" and
"c" instead.

As the comments being removed here show the previous behavior needed
to be carefully explained as we'd potentially share the populated
configuration among different instances of grep_init(). In practice we
didn't do that, but now that it can't be a concern anymore let's
remove those comments.

See 6ba9bb76e02 (grep: copy struct in one fell swoop, 2020-11-29) and
7687a0541e0 (grep: move the configuration parsing logic to grep.[ch],
2012-10-09) for the commits that added the comments.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/grep.c |  4 ++--
 builtin/log.c  | 13 +++++++++++--
 grep.c         | 39 +++------------------------------------
 grep.h         | 21 +++++++++++++++++++++
 4 files changed, 37 insertions(+), 40 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 960c7aac123..7f95f44e948 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -288,7 +288,7 @@ static int wait_all(void)
 static int grep_cmd_config(const char *var, const char *value, void *cb)
 {
 	int st = grep_config(var, value, cb);
-	if (git_color_default_config(var, value, cb) < 0)
+	if (git_color_default_config(var, value, NULL) < 0)
 		st = -1;
 
 	if (!strcmp(var, "grep.threads")) {

@@ -969,8 +969,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	git_config(grep_cmd_config, NULL);
 	grep_init(&opt, the_repository);
+	git_config(grep_cmd_config, &opt);
 	opt.caller_priv = &opt_cmd;
 
 	/*

diff --git a/builtin/log.c b/builtin/log.c
index f75d87e8d7f..bfddacdfa6c 100644
--- a/builtin/log.c
+++ b/builtin/log.c

@@ -505,8 +505,6 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	if (grep_config(var, value, cb) < 0)
-		return -1;
 	if (git_gpg_config(var, value, cb) < 0)
 		return -1;
 	return git_diff_ui_config(var, value, cb);

@@ -521,6 +519,8 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix)
 	git_config(git_log_config, NULL);
 
 	repo_init_revisions(the_repository, &rev, prefix);
+	git_config(grep_config, &rev.grep_filter);
+
 	rev.diff = 1;
 	rev.simplify_history = 0;
 	memset(&opt, 0, sizeof(opt));

@@ -635,6 +635,8 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 
 	memset(&match_all, 0, sizeof(match_all));
 	repo_init_revisions(the_repository, &rev, prefix);
+	git_config(grep_config, &rev.grep_filter);
+
 	rev.diff = 1;
 	rev.always_show_header = 1;
 	rev.no_walk = 1;

@@ -718,6 +720,8 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)
 
 	repo_init_revisions(the_repository, &rev, prefix);
 	init_reflog_walk(&rev.reflog_info);
+	git_config(grep_config, &rev.grep_filter);
+
 	rev.verbose_header = 1;
 	memset(&opt, 0, sizeof(opt));
 	opt.def = "HEAD";

@@ -751,6 +755,8 @@ int cmd_log(int argc, const char **argv, const char *prefix)
 	git_config(git_log_config, NULL);
 
 	repo_init_revisions(the_repository, &rev, prefix);
+	git_config(grep_config, &rev.grep_filter);
+
 	rev.always_show_header = 1;
 	memset(&opt, 0, sizeof(opt));
 	opt.def = "HEAD";

@@ -1833,10 +1839,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	extra_hdr.strdup_strings = 1;
 	extra_to.strdup_strings = 1;
 	extra_cc.strdup_strings = 1;
+
 	init_log_defaults();
 	init_display_notes(&notes_opt);
 	git_config(git_format_config, NULL);
 	repo_init_revisions(the_repository, &rev, prefix);
+	git_config(grep_config, &rev.grep_filter);
+
 	rev.show_notes = show_notes;
 	memcpy(&rev.notes_opt, &notes_opt, sizeof(notes_opt));
 	rev.commit_format = CMIT_FMT_EMAIL;

diff --git a/grep.c b/grep.c
index c9065254aeb..fb3f63c63ef 100644
--- a/grep.c
+++ b/grep.c

@@ -19,27 +19,6 @@ static void std_output(struct grep_opt *opt, const void *buf, size_t size)
 	fwrite(buf, size, 1, stdout);
 }
 
-static struct grep_opt grep_defaults = {
-	.relative = 1,
-	.pathname = 1,
-	.max_depth = -1,
-	.pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED,
-	.colors = {
-		[GREP_COLOR_CONTEXT] = "",
-		[GREP_COLOR_FILENAME] = "",
-		[GREP_COLOR_FUNCTION] = "",
-		[GREP_COLOR_LINENO] = "",
-		[GREP_COLOR_COLUMNNO] = "",
-		[GREP_COLOR_MATCH_CONTEXT] = GIT_COLOR_BOLD_RED,
-		[GREP_COLOR_MATCH_SELECTED] = GIT_COLOR_BOLD_RED,
-		[GREP_COLOR_SELECTED] = "",
-		[GREP_COLOR_SEP] = GIT_COLOR_CYAN,
-	},
-	.only_matching = 0,
-	.color = -1,
-	.output = std_output,
-};
-
 static const char *color_grep_slots[] = {
 	[GREP_COLOR_CONTEXT]	    = "context",
 	[GREP_COLOR_FILENAME]	    = "filename",

@@ -75,20 +54,12 @@ define_list_config_array_extra(color_grep_slots, {"match"});
  */
 int grep_config(const char *var, const char *value, void *cb)
 {
-	struct grep_opt *opt = &grep_defaults;
+	struct grep_opt *opt = cb;
 	const char *slot;
 
 	if (userdiff_config(var, value) < 0)
 		return -1;
 
-	/*
-	 * The instance of grep_opt that we set up here is copied by
-	 * grep_init() to be used by each individual invocation.
-	 * When populating a new field of this structure here, be
-	 * sure to think about ownership -- e.g., you might need to
-	 * override the shallow copy in grep_init() with a deep copy.
-	 */
-
 	if (!strcmp(var, "grep.extendedregexp")) {
 		opt->extended_regexp_option = git_config_bool(var, value);
 		return 0;

@@ -134,14 +105,10 @@ int grep_config(const char *var, const char *value, void *cb)
 	return 0;
 }
 
-/*
- * Initialize one instance of grep_opt and copy the
- * default values from the template we read the configuration
- * information in an earlier call to git_config(grep_config).
- */
 void grep_init(struct grep_opt *opt, struct repository *repo)
 {
-	*opt = grep_defaults;
+	struct grep_opt blank = GREP_OPT_INIT;
+	memcpy(opt, &blank, sizeof(*opt));
 
 	opt->repo = repo;
 	opt->pattern_tail = &opt->pattern_list;

diff --git a/grep.h b/grep.h
index 6b923d8599c..30a7dfd3294 100644
--- a/grep.h
+++ b/grep.h

@@ -178,6 +178,27 @@ struct grep_opt {
 	void *output_priv;
 };
 
+#define GREP_OPT_INIT { \
+	.relative = 1, \
+	.pathname = 1, \
+	.max_depth = -1, \
+	.pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED, \
+	.colors = { \
+		[GREP_COLOR_CONTEXT] = "", \
+		[GREP_COLOR_FILENAME] = "", \
+		[GREP_COLOR_FUNCTION] = "", \
+		[GREP_COLOR_LINENO] = "", \
+		[GREP_COLOR_COLUMNNO] = "", \
+		[GREP_COLOR_MATCH_CONTEXT] = GIT_COLOR_BOLD_RED, \
+		[GREP_COLOR_MATCH_SELECTED] = GIT_COLOR_BOLD_RED, \
+		[GREP_COLOR_SELECTED] = "", \
+		[GREP_COLOR_SEP] = GIT_COLOR_CYAN, \
+	}, \
+	.only_matching = 0, \
+	.color = -1, \
+	.output = std_output, \
+}
+
 int grep_config(const char *var, const char *value, void *);
 void grep_init(struct grep_opt *, struct repository *repo);
 void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);

-- 
2.34.0.rc1.741.gab7bfd97031

Change the interaction between "grep.patternType=default" and
"grep.extendedRegexp=true" to make setting "grep.extendedRegexp=true"
synonymous with setting "grep.patternType=extended".

This changes our existing config parsing behavior as detailed below,
but in a way that's consistent with how we parse other
configuration.

Pedantically speaking we're probably breaking past promises here, but
I doubt that this will impact anyone in practice. The reduction in
complexity and resulting consistency with other default config
behavior is worth it.

When "grep.patternType" was introduced in 84befcd0a4a (grep: add a
grep.patternType configuration setting, 2012-08-03) we made two
seemingly contradictory promises:

 1. You can set "grep.patternType", and "[setting it to] 'default'
    will return to the default matching behavior".

 2. Support the existing "grep.extendedRegexp" option, but ignore it
    when the new "grep.patternType" is set, *except* "when the
    `grep.patternType` option is set. to a value other than 'default'".

I think that 84befcd0a4a probably didn't intend this behavior, but
instead ended up conflating our internal "unspecified" state with a
user's explicit desire to set the configuration back to the
default.

I.e. a user would correctly expect this to keep working:

    # ERE grep
    git -c grep.extendedRegexp=true grep <pattern>

And likewise for "grep.patternType=default" to take precedence over
the disfavored "grep.extendedRegexp" option, i.e. the usual "last set
wins" semantics.

    # BRE grep
    git -c grep.extendedRegexp=true -c grep.patternType=basic grep <pattern>

But probably not for this to ignore the new "grep.patternType" option
entirely, say if /etc/gitconfig was still setting
"grep.extendedRegexp", but "~/.gitconfig" used the new
"grep.patternType" (and wanted to use the "default" value):

    # Was ERE, now BRE
    git -c grep.extendedRegexp=true grep.patternType=default grep <pattern>

I think that in practice nobody or almost nobody is going to be
relying on this obscure interaction, and as shown here it makes the
config parsing much simpler. We no longer have to carry a complex
state machine in "grep_commit_pattern_type()" and
"grep_set_pattern_type_option()".

We can also do away with the "int fixed" and "int pcre2" members in
favor of using "pattern_type_option" directly in "grep.c", as well as
dropping the "pattern_type_arg" variable in "builtin/grep.c" in favor
of using the "pattern_type_option" member directly.

See my 07a3d411739 (grep: remove regflags from the public grep_opt
API, 2017-06-29) for addition of the two comments being removed here,
i.e. the complexity noted in that commit is now going away.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 Documentation/config/grep.txt |  3 +-
 Documentation/git-grep.txt    |  3 +-
 builtin/grep.c                | 10 ++---
 grep.c                        | 71 +++++------------------------------
 grep.h                        |  6 +--
 revision.c                    |  2 -
 t/t7810-grep.sh               |  2 +-
 7 files changed, 17 insertions(+), 80 deletions(-)

diff --git a/Documentation/config/grep.txt b/Documentation/config/grep.txt
index 44abe45a7ca..2669b1757d3 100644
--- a/Documentation/config/grep.txt
+++ b/Documentation/config/grep.txt

@@ -12,8 +12,7 @@ grep.patternType::
 
 grep.extendedRegexp::
 	If set to true, enable `--extended-regexp` option by default. This
-	option is ignored when the `grep.patternType` option is set to a value
-	other than 'default'.
+	option is ignored when the `grep.patternType` option is set.
 
 grep.threads::
 	Number of grep worker threads to use.

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 3d393fbac1b..078dfeadf50 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt

@@ -348,8 +348,7 @@ grep.patternType::
 
 grep.extendedRegexp::
 	If set to true, enable `--extended-regexp` option by default. This
-	option is ignored when the `grep.patternType` option is set to a value
-	other than 'default'.
+	option is ignored when the `grep.patternType` option is set.
 
 grep.threads::
 	Number of grep worker threads to use. If unset (or set to 0), Git will

diff --git a/builtin/grep.c b/builtin/grep.c
index 7f95f44e948..a4964baf9c0 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -849,7 +849,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	int i;
 	int dummy;
 	int use_index = 1;
-	int pattern_type_arg = GREP_PATTERN_TYPE_UNSPECIFIED;
 	int allow_revs;
 
 	struct option options[] = {

@@ -883,16 +882,16 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			N_("descend at most <depth> levels"), PARSE_OPT_NONEG,
 			NULL, 1 },
 		OPT_GROUP(""),
-		OPT_SET_INT('E', "extended-regexp", &pattern_type_arg,
+		OPT_SET_INT('E', "extended-regexp", &opt.pattern_type_option,
 			    N_("use extended POSIX regular expressions"),
 			    GREP_PATTERN_TYPE_ERE),
-		OPT_SET_INT('G', "basic-regexp", &pattern_type_arg,
+		OPT_SET_INT('G', "basic-regexp", &opt.pattern_type_option,
 			    N_("use basic POSIX regular expressions (default)"),
 			    GREP_PATTERN_TYPE_BRE),
-		OPT_SET_INT('F', "fixed-strings", &pattern_type_arg,
+		OPT_SET_INT('F', "fixed-strings", &opt.pattern_type_option,
 			    N_("interpret patterns as fixed strings"),
 			    GREP_PATTERN_TYPE_FIXED),
-		OPT_SET_INT('P', "perl-regexp", &pattern_type_arg,
+		OPT_SET_INT('P', "perl-regexp", &opt.pattern_type_option,
 			    N_("use Perl-compatible regular expressions"),
 			    GREP_PATTERN_TYPE_PCRE),
 		OPT_GROUP(""),

@@ -986,7 +985,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options, grep_usage,
 			     PARSE_OPT_KEEP_DASHDASH |
 			     PARSE_OPT_STOP_AT_NON_OPTION);
-	grep_commit_pattern_type(pattern_type_arg, &opt);
 
 	if (use_index && !startup_info->have_repository) {
 		int fallback = 0;

diff --git a/grep.c b/grep.c
index fb3f63c63ef..dda8e536fe3 100644
--- a/grep.c
+++ b/grep.c

@@ -60,8 +60,10 @@ int grep_config(const char *var, const char *value, void *cb)
 	if (userdiff_config(var, value) < 0)
 		return -1;
 
-	if (!strcmp(var, "grep.extendedregexp")) {
-		opt->extended_regexp_option = git_config_bool(var, value);
+	if (opt->pattern_type_option == GREP_PATTERN_TYPE_UNSPECIFIED &&
+	    !strcmp(var, "grep.extendedregexp") &&
+	    git_config_bool(var, value)) {
+		opt->pattern_type_option = GREP_PATTERN_TYPE_ERE;
 		return 0;
 	}
 

@@ -115,62 +117,6 @@ void grep_init(struct grep_opt *opt, struct repository *repo)
 	opt->header_tail = &opt->header_list;
 }
 
-static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)
-{
-	/*
-	 * When committing to the pattern type by setting the relevant
-	 * fields in grep_opt it's generally not necessary to zero out
-	 * the fields we're not choosing, since they won't have been
-	 * set by anything. The extended_regexp_option field is the
-	 * only exception to this.
-	 *
-	 * This is because in the process of parsing grep.patternType
-	 * & grep.extendedRegexp we set opt->pattern_type_option and
-	 * opt->extended_regexp_option, respectively. We then
-	 * internally use opt->extended_regexp_option to see if we're
-	 * compiling an ERE. It must be unset if that's not actually
-	 * the case.
-	 */
-	if (pattern_type != GREP_PATTERN_TYPE_ERE &&
-	    opt->extended_regexp_option)
-		opt->extended_regexp_option = 0;
-
-	switch (pattern_type) {
-	case GREP_PATTERN_TYPE_UNSPECIFIED:
-		/* fall through */
-
-	case GREP_PATTERN_TYPE_BRE:
-		break;
-
-	case GREP_PATTERN_TYPE_ERE:
-		opt->extended_regexp_option = 1;
-		break;
-
-	case GREP_PATTERN_TYPE_FIXED:
-		opt->fixed = 1;
-		break;
-
-	case GREP_PATTERN_TYPE_PCRE:
-		opt->pcre2 = 1;
-		break;
-	}
-}
-
-void grep_commit_pattern_type(enum grep_pattern_type pattern_type, struct grep_opt *opt)
-{
-	if (pattern_type != GREP_PATTERN_TYPE_UNSPECIFIED)
-		grep_set_pattern_type_option(pattern_type, opt);
-	else if (opt->pattern_type_option != GREP_PATTERN_TYPE_UNSPECIFIED)
-		grep_set_pattern_type_option(opt->pattern_type_option, opt);
-	else if (opt->extended_regexp_option)
-		/*
-		 * This branch *must* happen after setting from the
-		 * opt->pattern_type_option above, we don't want
-		 * grep.extendedRegexp to override grep.patternType!
-		 */
-		grep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, opt);
-}
-
 static struct grep_pat *create_grep_pat(const char *pat, size_t patlen,
 					const char *origin, int no,
 					enum grep_pat_token t,

@@ -492,9 +438,10 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
 
 	p->word_regexp = opt->word_regexp;
 	p->ignore_case = opt->ignore_case;
-	p->fixed = opt->fixed;
+	p->fixed = opt->pattern_type_option == GREP_PATTERN_TYPE_FIXED;
 
-	if (memchr(p->pattern, 0, p->patternlen) && !opt->pcre2)
+	if (opt->pattern_type_option != GREP_PATTERN_TYPE_PCRE &&
+	    memchr(p->pattern, 0, p->patternlen))
 		die(_("given pattern contains NULL byte (via -f <file>). This is only supported with -P under PCRE v2"));
 
 	p->is_fixed = is_fixed(p->pattern, p->patternlen);

@@ -545,14 +492,14 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
 		return;
 	}
 
-	if (opt->pcre2) {
+	if (opt->pattern_type_option == GREP_PATTERN_TYPE_PCRE) {
 		compile_pcre2_pattern(p, opt);
 		return;
 	}
 
 	if (p->ignore_case)
 		regflags |= REG_ICASE;
-	if (opt->extended_regexp_option)
+	if (opt->pattern_type_option == GREP_PATTERN_TYPE_ERE)
 		regflags |= REG_EXTENDED;
 	err = regcomp(&p->regexp, p->pattern, regflags);
 	if (err) {

diff --git a/grep.h b/grep.h
index 30a7dfd3294..e4e548aed90 100644
--- a/grep.h
+++ b/grep.h

@@ -143,7 +143,6 @@ struct grep_opt {
 	int unmatch_name_only;
 	int count;
 	int word_regexp;
-	int fixed;
 	int all_match;
 #define GREP_BINARY_DEFAULT	0
 #define GREP_BINARY_NOMATCH	1

@@ -152,7 +151,6 @@ struct grep_opt {
 	int allow_textconv;
 	int extended;
 	int use_reflog_filter;
-	int pcre2;
 	int relative;
 	int pathname;
 	int null_following_name;

@@ -161,8 +159,7 @@ struct grep_opt {
 	int max_depth;
 	int funcname;
 	int funcbody;
-	int extended_regexp_option;
-	int pattern_type_option;
+	enum grep_pattern_type pattern_type_option;
 	int ignore_locale;
 	char colors[NR_GREP_COLORS][COLOR_MAXLEN];
 	unsigned pre_context;

@@ -201,7 +198,6 @@ struct grep_opt {
 
 int grep_config(const char *var, const char *value, void *);
 void grep_init(struct grep_opt *, struct repository *repo);
-void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);
 
 void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);
 void append_grep_pattern(struct grep_opt *opt, const char *pat, const char *origin, int no, enum grep_pat_token t);

diff --git a/revision.c b/revision.c
index 9f9b0d2429e..ed29d245c89 100644
--- a/revision.c
+++ b/revision.c

@@ -2864,8 +2864,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 
 	diff_setup_done(&revs->diffopt);
 
-	grep_commit_pattern_type(GREP_PATTERN_TYPE_UNSPECIFIED,
-				 &revs->grep_filter);
 	if (!is_encoding_utf8(get_log_output_encoding()))
 		revs->grep_filter.ignore_locale = 1;
 	compile_grep_patterns(&revs->grep_filter);

diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 6b6423a07c3..a59a9726357 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh

@@ -443,7 +443,7 @@ do
 	'
 
 	test_expect_success "grep $L with grep.extendedRegexp=true and grep.patternType=default" '
-		echo "${HC}ab:abc" >expected &&
+		echo "${HC}ab:a+bc" >expected &&
 		git \
 			-c grep.extendedRegexp=true \
 			-c grep.patternType=default \

-- 
2.34.0.rc1.741.gab7bfd97031

In the preceding commit we changed how a "grep.patternType=default"
set after "grep.extendedRegexp=true" would be handled so that the last
set would win, but a "grep.extendedRegexp=true" would only be used if
"grep.patternType" was set to a value other than "default".

Thus a user who had old config and set "grep.extendedRegexp=true" in
their ~/.gitconfig expecting ERE behavior would be opted-in to say
"perl" regexes if a system "/etc/gitconfig" started setting
"grep.patternType=perl".

These funny semantics of only paying attention to a set if another key
is not set to a given value aren't how we treat other config keys, so
let's do away with this caveat for consistency.

The new semantics are simple, a "grep.extendedRegexp=true" is an exact
synonym for specifying "grep.patternType=extended" in the
config. We'll keep ignoring ""grep.extendedRegexp=false", although
arguably we could treat it as a "grep.patternType=basic".

As argued in the preceding commit I think this behavior came about
because we were conflating the state of our code's own internal
"default" value with what we found in explicit user config. See
84befcd0a4a (grep: add a grep.patternType configuration setting,
2012-08-03) for that past behavior.

Let's further change the documentation to note that
"grep.extendedRegexp" is a deprecated synonym, perhaps we'll be able
to remove it at some point in the future and do away with this
special-case entirely.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 Documentation/config/grep.txt | 3 +--
 Documentation/git-grep.txt    | 3 +--
 grep.c                        | 8 +++-----
 grep.h                        | 5 +----
 t/t7810-grep.sh               | 2 +-
 5 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/Documentation/config/grep.txt b/Documentation/config/grep.txt
index 2669b1757d3..9868ea8a061 100644
--- a/Documentation/config/grep.txt
+++ b/Documentation/config/grep.txt

@@ -11,8 +11,7 @@ grep.patternType::
 	value 'default' will return to the default matching behavior.
 
 grep.extendedRegexp::
-	If set to true, enable `--extended-regexp` option by default. This
-	option is ignored when the `grep.patternType` option is set.
+	Deprecated synonym for 'grep.patternType=extended`.
 
 grep.threads::
 	Number of grep worker threads to use.

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 078dfeadf50..211ba6801b0 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt

@@ -347,8 +347,7 @@ grep.patternType::
 	value 'default' will return to the default matching behavior.
 
 grep.extendedRegexp::
-	If set to true, enable `--extended-regexp` option by default. This
-	option is ignored when the `grep.patternType` option is set.
+	Deprecated synonym for 'grep.patternType=extended`.
 
 grep.threads::
 	Number of grep worker threads to use. If unset (or set to 0), Git will

diff --git a/grep.c b/grep.c
index dda8e536fe3..ef8746d85f0 100644
--- a/grep.c
+++ b/grep.c

@@ -33,9 +33,8 @@ static const char *color_grep_slots[] = {
 
 static int parse_pattern_type_arg(const char *opt, const char *arg)
 {
-	if (!strcmp(arg, "default"))
-		return GREP_PATTERN_TYPE_UNSPECIFIED;
-	else if (!strcmp(arg, "basic"))
+	if (!strcmp(arg, "basic") ||
+	    !strcmp(arg, "default"))
 		return GREP_PATTERN_TYPE_BRE;
 	else if (!strcmp(arg, "extended"))
 		return GREP_PATTERN_TYPE_ERE;

@@ -60,8 +59,7 @@ int grep_config(const char *var, const char *value, void *cb)
 	if (userdiff_config(var, value) < 0)
 		return -1;
 
-	if (opt->pattern_type_option == GREP_PATTERN_TYPE_UNSPECIFIED &&
-	    !strcmp(var, "grep.extendedregexp") &&
+	if (!strcmp(var, "grep.extendedregexp") &&
 	    git_config_bool(var, value)) {
 		opt->pattern_type_option = GREP_PATTERN_TYPE_ERE;
 		return 0;

diff --git a/grep.h b/grep.h
index e4e548aed90..55b23d045e0 100644
--- a/grep.h
+++ b/grep.h

@@ -94,8 +94,7 @@ enum grep_expr_node {
 };
 
 enum grep_pattern_type {
-	GREP_PATTERN_TYPE_UNSPECIFIED = 0,
-	GREP_PATTERN_TYPE_BRE,
+	GREP_PATTERN_TYPE_BRE = 0,
 	GREP_PATTERN_TYPE_ERE,
 	GREP_PATTERN_TYPE_FIXED,
 	GREP_PATTERN_TYPE_PCRE

@@ -179,7 +178,6 @@ struct grep_opt {
 	.relative = 1, \
 	.pathname = 1, \
 	.max_depth = -1, \
-	.pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED, \
 	.colors = { \
 		[GREP_COLOR_CONTEXT] = "", \
 		[GREP_COLOR_FILENAME] = "", \

@@ -191,7 +189,6 @@ struct grep_opt {
 		[GREP_COLOR_SELECTED] = "", \
 		[GREP_COLOR_SEP] = GIT_COLOR_CYAN, \
 	}, \
-	.only_matching = 0, \
 	.color = -1, \
 	.output = std_output, \
 }

diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index a59a9726357..afca938a4d0 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh

@@ -461,7 +461,7 @@ do
 	'
 
 	test_expect_success "grep $L with grep.patternType=basic and grep.extendedRegexp=true" '
-		echo "${HC}ab:a+bc" >expected &&
+		echo "${HC}ab:abc" >expected &&
 		git \
 			-c grep.patternType=basic \
 			-c grep.extendedRegexp=true \

-- 
2.34.0.rc1.741.gab7bfd97031

On Sat, Nov 06, 2021 at 10:10:48PM +0100, Ævar Arnfjörð Bjarmason wrote:

Small nit, but the check to `prefix` (in `prefix && *prefix`) is
redundant with the left-hand side of the or.

Looking around, ls-tree's initialization includes a conditional of the
form:

    if (prefix && *prefix)
      chomp_prefix = strlen(prefix);

So that could be cleaned up too. But honestly, the pre-image of this
patch (and the spot in ls-tree) doesn't make a lot of sense to me to
begin with.

Even if prefix were the empty string, calling strlen() on it will just
give us zero. So there is no difference between assigning `str && *str ?
strlen(str) : 0` and `str ? strlen(str) : 0`.

So I am confused why this needs hardening with an assertion when it
seems like the checks before calling strlen() were overly restrictive to
begin with. In other words: why not only include this hunk (either in
this patch, or squashed into another patch later on)?

Thanks,
Taylor

On Sat, Nov 06, 2021 at 10:10:49PM +0100, Ævar Arnfjörð Bjarmason wrote:

Remove the "prefix_length" member, which we compute with a strlen() on
the "prefix" argument to grep_init(), but whose strlen() hasn't been
used since 493b7a08d80 (grep: accept relative paths outside current
working directory, 2009-09-05).

OK, so now we *are* relying on the assumption that prefix is either NULL
or a non-empty string.

I assume that the last patch was along the lines of "let's clean up this
redundant check before calling strlen()" and "prepare to not call
strlen() at all and just check the string itself for NULL". To be
honest, I imagine that it would have been much easier to review if these
two had been squashed into one, since I was a little surprised to see
the line I had just been commenting on in the previous patch removed.

Perhaps I should have looked a little further in the series before
commenting there, but I think it would have been even easier for
reviewers to see these two patches together.

When this code was added in 0d042fecf2f (git-grep: show pathnames
relative to the current directory, 2006-08-11) we used the length, but
since 493b7a08d80 we haven't used it for anything except a boolean
check that we could have done on the "prefix" member itself.

Before a preceding commit we also used to guard the strlen() with
"prefix && *prefix", but as that commit notes the RHS of that && chain
was also redundant.

Everything in this patch looks fine to me, assuming that prefix is
indeed always NULL or non-empty (which I haven't verified myself).

Thanks,
Taylor

On Sat, Nov 06, 2021 at 10:10:50PM +0100, Ævar Arnfjörð Bjarmason wrote:

The "struct grep_opt" is a mixture of things that would be needed by
all callers of the grep.c API, and quite a few things that only the
builtin/grep.c needs.

Since we got rid of "prefix_length" in the previous commit, let's move
the "prefix" variable over to "builtin/grep.c" where it's used. To do
this let's create a "struct grep_cmd_opt", which we'll have a pointer
to in a new "caller_priv" member in "struct grep_opt" (the existing
"priv" is used by the top-level "grep.c" itself).

I'm definitely in favor of removing specialized, caller-specific bits
from an internal API. But I'm not sure why grep.c needs to keep track o
this new "caller_priv" field at all.

Among the uses of `prefix` in builtin/grep.c, I see grep_source_name,
and the call to run_pager(). Would it be more straightforward to pass
down prefix from cmd_grep down to its use in grep_source_name?

There are quite a few intermediary functions that we go through to get
from cmd_grep() down to grep_source_name(). For instance, we could reach
it through:

    cmd_grep -> grep_tree -> grep_submodule -> grep_cache -> grep_oid ->
    grep_source_name

But passing prefix from cmd_grep down to grep_source_name without
relying on the internals of grep.c seems like a good direction to me. I
could even buy that (ab)using a static variable in builtin/grep.c to
keep track of a constant prefix value would save you some plumbing
(though I'd rather see the usage spelled out more explicitly).

All of that is to say that I share your motivation for this patch and
think that the direction is good, but I would have preferred to do it
without the caller_priv variable (unless there is something that I am
missing here).

Thanks,
Taylor

On Sat, Nov 06, 2021 at 10:10:52PM +0100, Ævar Arnfjörð Bjarmason wrote:

The grep_init() function used the odd pattern of initializing the
passed-in "struct grep_opt" with a statically defined "grep_defaults"
struct, which would be modified in-place when we invoked
grep_config().

So we effectively (b) initialized config, (a) then defaults, (c)
followed by user options. Usually those are ordered as "a", "b" and
"c" instead.

Do we risk changing any user-visible behavior here? Based on my reading
of grep.c before and after this patch, I think the answer is "no", but I
wasn't sure if you had done a similar analysis.

In any case, I think the "bring your own structure" instead of getting
one copied around is much easier to reason about. Even if we weren't
accidentally stomping on ownership of the struct before, not having to
reason about it is a nice benefit.

As the comments being removed here show the previous behavior needed
to be carefully explained as we'd potentially share the populated
configuration among different instances of grep_init(). In practice we
didn't do that, but now that it can't be a concern anymore let's
remove those comments.

Makes sense, I agree.

This doesn't appear strictly related to the rest of your changes, but
only serves to prevent the caller-provided data from being sent down to
git_color_default_config().

It didn't matter before because (a) the caller doesn't specify any data
to begin with, and git_color_default_config() (or the functions that it
calls) don't do anything with the extra pointer. Now cmd_grep() is going
to start passing around a pointer to a struct grep_opt.

But git_color_default_config() still doesn't do anything with the
pointer it receives, and passing that pointer around is standard
practice among config.c code. So I don't think that this hunk is
strictly necessary, and it's somewhat different than the pattern
established within config.c.

I wouldn't be sad to see this hunk dropped (and in fact have a slight
preference leaning this way), but I don't mind keeping it around,
either.

I'm nit-picking, but creating a throwaway struct for the convenience of
using designated initialization (at the cost of having to memcpy an
entire struct around) seems like overkill.

Especially since we're just going to write into the other fields of the
the target struct anyway, I'd probably rather have seen everything
written out explicitly without the throwaway or memcpy.

Thanks,
Taylor

On Sat, Nov 06, 2021 at 10:10:53PM +0100, Ævar Arnfjörð Bjarmason wrote:

I.e. a user would correctly expect this to keep working:

    # ERE grep
    git -c grep.extendedRegexp=true grep <pattern>

And likewise for "grep.patternType=default" to take precedence over
the disfavored "grep.extendedRegexp" option, i.e. the usual "last set
wins" semantics.

    # BRE grep
    git -c grep.extendedRegexp=true -c grep.patternType=basic grep <pattern>

But probably not for this to ignore the new "grep.patternType" option
entirely, say if /etc/gitconfig was still setting
"grep.extendedRegexp", but "~/.gitconfig" used the new
"grep.patternType" (and wanted to use the "default" value):

    # Was ERE, now BRE
    git -c grep.extendedRegexp=true grep.patternType=default grep <pattern>

OK, so this is the case that we'd be "breaking". And I think that the
new behavior you're outlining here (where a higher-precedence
grep.patternType=default overrides a lower-precedence
grep.extendedRegexp=true, resulting in using BRE over ERE) makes more
sense.

At least, it makes more sense if your expectation of "default" is "the
default matching behavior", not "fallthrough to grep.extendedRegexp".

In any case, I am sensitive to breaking existing user workflows, but
this seems so obscure to me that I have a hard time expecting that
m(any?) users will even notice this at all.

The situation I'm most concerned about is having grep.extendedRegexp set
in, say, /etc/gitconfig and grep.patternType=default set at a
higher-precedence level.

---
 Documentation/config/grep.txt |  3 +-
 Documentation/git-grep.txt    |  3 +-

Not the fault of your patch, but these two are annoyingly (and subtly)
different from one another. Could we clean this up and put everything in
Documentation/config/grep.txt (and then include that in the
CONFIGURATION section of Documentation/git-grep.txt)?

Makes sense, and matches your description.

And here's our "as long as we haven't set the pattern type already via
grep.patternType, allow grep.extendedRegexp to set it". But the same
"only set when unspecified" condition *isn't* in place for
grep.patternType, which is what makes us prefer values from that
configuration over the other. Makes sense.

Everything else looks good here, too.

Thanks,
Taylor

On Mon, Nov 08 2021, Taylor Blau wrote:

On Sat, Nov 06, 2021 at 10:10:53PM +0100, Ævar Arnfjörð Bjarmason wrote:

quoted

I.e. a user would correctly expect this to keep working:

    # ERE grep
    git -c grep.extendedRegexp=true grep <pattern>

And likewise for "grep.patternType=default" to take precedence over
the disfavored "grep.extendedRegexp" option, i.e. the usual "last set
wins" semantics.

    # BRE grep
    git -c grep.extendedRegexp=true -c grep.patternType=basic grep <pattern>

But probably not for this to ignore the new "grep.patternType" option
entirely, say if /etc/gitconfig was still setting
"grep.extendedRegexp", but "~/.gitconfig" used the new
"grep.patternType" (and wanted to use the "default" value):

    # Was ERE, now BRE
    git -c grep.extendedRegexp=true grep.patternType=default grep <pattern>

OK, so this is the case that we'd be "breaking". And I think that the
new behavior you're outlining here (where a higher-precedence
grep.patternType=default overrides a lower-precedence
grep.extendedRegexp=true, resulting in using BRE over ERE) makes more
sense.

At least, it makes more sense if your expectation of "default" is "the
default matching behavior", not "fallthrough to grep.extendedRegexp".

In any case, I am sensitive to breaking existing user workflows, but
this seems so obscure to me that I have a hard time expecting that
m(any?) users will even notice this at all.

The situation I'm most concerned about is having grep.extendedRegexp set
in, say, /etc/gitconfig and grep.patternType=default set at a
higher-precedence level.

*nod*, but the only user who'd end up with that is someone who's trying
to override grep.extendedRegexp but failing to do it, so this would
help.

Or someone who'd read the docs, understood that we promised that would
do nothing, and inserted that just to test us, but that seems unlikely
:)

Or, I suppose someone who's entirely confused, and will continue being
even more confused now that behavior changes on a git upgrade from ERE
to BRE.

I'm hoping the last two paragraphs describe no-one & that this is safe
to do.

quoted

---
 Documentation/config/grep.txt |  3 +-
 Documentation/git-grep.txt    |  3 +-

Not the fault of your patch, but these two are annoyingly (and subtly)
different from one another. Could we clean this up and put everything in
Documentation/config/grep.txt (and then include that in the
CONFIGURATION section of Documentation/git-grep.txt)?

I've got a large series to do that for all of these, but opted to skip
that particular digression here (even for just grep.txt it's a bit
distracting).

On Mon, Nov 08 2021, Taylor Blau wrote:

On Sat, Nov 06, 2021 at 10:10:52PM +0100, Ævar Arnfjörð Bjarmason wrote:

quoted

The grep_init() function used the odd pattern of initializing the
passed-in "struct grep_opt" with a statically defined "grep_defaults"
struct, which would be modified in-place when we invoked
grep_config().

So we effectively (b) initialized config, (a) then defaults, (c)
followed by user options. Usually those are ordered as "a", "b" and
"c" instead.

Do we risk changing any user-visible behavior here? Based on my reading
of grep.c before and after this patch, I think the answer is "no", but I
wasn't sure if you had done a similar analysis.

In any case, I think the "bring your own structure" instead of getting
one copied around is much easier to reason about. Even if we weren't
accidentally stomping on ownership of the struct before, not having to
reason about it is a nice benefit.

I don't think we're changing any behavior except the one noted in this
series.

We only set a few config variables, so I thought that was fairly easy to
trace...

[...]

quoted

diff --git a/builtin/grep.c b/builtin/grep.c
index 960c7aac123..7f95f44e948 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -288,7 +288,7 @@ static int wait_all(void)
 static int grep_cmd_config(const char *var, const char *value, void *cb)
 {
 	int st = grep_config(var, value, cb);
-	if (git_color_default_config(var, value, cb) < 0)
+	if (git_color_default_config(var, value, NULL) < 0)

This doesn't appear strictly related to the rest of your changes, but
only serves to prevent the caller-provided data from being sent down to
git_color_default_config().

It didn't matter before because (a) the caller doesn't specify any data
to begin with, and git_color_default_config() (or the functions that it
calls) don't do anything with the extra pointer. Now cmd_grep() is going
to start passing around a pointer to a struct grep_opt.

But git_color_default_config() still doesn't do anything with the
pointer it receives, and passing that pointer around is standard
practice among config.c code. So I don't think that this hunk is
strictly necessary, and it's somewhat different than the pattern
established within config.c.

I wouldn't be sad to see this hunk dropped (and in fact have a slight
preference leaning this way), but I don't mind keeping it around,
either.

Will either split it up or drop it.

[...]

quoted

-/*
- * Initialize one instance of grep_opt and copy the
- * default values from the template we read the configuration
- * information in an earlier call to git_config(grep_config).
- */
 void grep_init(struct grep_opt *opt, struct repository *repo)
 {
-	*opt = grep_defaults;
+	struct grep_opt blank = GREP_OPT_INIT;
+	memcpy(opt, &blank, sizeof(*opt));

I'm nit-picking, but creating a throwaway struct for the convenience of
using designated initialization (at the cost of having to memcpy an
entire struct around) seems like overkill.

Especially since we're just going to write into the other fields of the
the target struct anyway, I'd probably rather have seen everything
written out explicitly without the throwaway or memcpy.

It's a widely used pattern in the codebase at this point, see
5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT
macro, 2021-07-01) (mine, but I stole it from Jeff King).

As his linked-to compiler test shows the memcpy() is optimized away, so
modern compilers will treat these idioms the same way.

There was a suggestions somewhere that we should prorably move to that
"*<x> = <y>" or whatever it was briefer C99 (I think) syntax across the
board, it would be less verbose. But I haven't tested if it's as widely
supported, so I've just been sticking with that blank/memcpy() pattern
for "do init in terms of macro".

On Mon, Nov 08 2021, Taylor Blau wrote:

On Sat, Nov 06, 2021 at 10:10:50PM +0100, Ævar Arnfjörð Bjarmason wrote:

quoted

The "struct grep_opt" is a mixture of things that would be needed by
all callers of the grep.c API, and quite a few things that only the
builtin/grep.c needs.

Since we got rid of "prefix_length" in the previous commit, let's move
the "prefix" variable over to "builtin/grep.c" where it's used. To do
this let's create a "struct grep_cmd_opt", which we'll have a pointer
to in a new "caller_priv" member in "struct grep_opt" (the existing
"priv" is used by the top-level "grep.c" itself).

I'm definitely in favor of removing specialized, caller-specific bits
from an internal API. But I'm not sure why grep.c needs to keep track o
this new "caller_priv" field at all.

Among the uses of `prefix` in builtin/grep.c, I see grep_source_name,
and the call to run_pager(). Would it be more straightforward to pass
down prefix from cmd_grep down to its use in grep_source_name?

There are quite a few intermediary functions that we go through to get
from cmd_grep() down to grep_source_name(). For instance, we could reach
it through:

    cmd_grep -> grep_tree -> grep_submodule -> grep_cache -> grep_oid ->
    grep_source_name

But passing prefix from cmd_grep down to grep_source_name without
relying on the internals of grep.c seems like a good direction to me. I
could even buy that (ab)using a static variable in builtin/grep.c to
keep track of a constant prefix value would save you some plumbing
(though I'd rather see the usage spelled out more explicitly).

All of that is to say that I share your motivation for this patch and
think that the direction is good, but I would have preferred to do it
without the caller_priv variable (unless there is something that I am
missing here).

Yes, that would make much more sense. I just had tunnel vision while
writing this, evidently. Yeah, just either passing it or using a static
variable in grep.c would make more sense.

Arguably we could even have it be read only (const char *const) and have
a global "the_prefix" or something, but it's probably not widely used
enough for that to make sense.

On Tue, Nov 09, 2021 at 03:01:12AM +0100, Ævar Arnfjörð Bjarmason wrote:

Or someone who'd read the docs, understood that we promised that would
do nothing, and inserted that just to test us, but that seems unlikely
:)

Or, I suppose someone who's entirely confused, and will continue being
even more confused now that behavior changes on a git upgrade from ERE
to BRE.

I'm hoping the last two paragraphs describe no-one & that this is safe
to do.

Yeah, I agree that it seems very unlikely that anybody would actually be
affected here, so I'm comfortable with the change.

quoted

quoted

---
 Documentation/config/grep.txt |  3 +-
 Documentation/git-grep.txt    |  3 +-

Not the fault of your patch, but these two are annoyingly (and subtly)
different from one another. Could we clean this up and put everything in
Documentation/config/grep.txt (and then include that in the
CONFIGURATION section of Documentation/git-grep.txt)?

I've got a large series to do that for all of these, but opted to skip
that particular digression here (even for just grep.txt it's a bit
distracting).

I think that you could include a preparatory clean-up in this series to
make the Documentation/config/grep.txt included by
Documentation/git-grep.txt without being too distracting. But if punting
on it now works better for you, I don't think either makes a reviewer's
job substantially different.

Thanks,
Taylor

On Tue, Nov 09, 2021 at 03:06:22AM +0100, Ævar Arnfjörð Bjarmason wrote:

quoted

quoted

-/*
- * Initialize one instance of grep_opt and copy the
- * default values from the template we read the configuration
- * information in an earlier call to git_config(grep_config).
- */
 void grep_init(struct grep_opt *opt, struct repository *repo)
 {
-	*opt = grep_defaults;
+	struct grep_opt blank = GREP_OPT_INIT;
+	memcpy(opt, &blank, sizeof(*opt));

I'm nit-picking, but creating a throwaway struct for the convenience of
using designated initialization (at the cost of having to memcpy an
entire struct around) seems like overkill.

Especially since we're just going to write into the other fields of the
the target struct anyway, I'd probably rather have seen everything
written out explicitly without the throwaway or memcpy.

It's a widely used pattern in the codebase at this point, see
5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT
macro, 2021-07-01) (mine, but I stole it from Jeff King).

As his linked-to compiler test shows the memcpy() is optimized away, so
modern compilers will treat these idioms the same way.

There was a suggestions somewhere that we should prorably move to that
"*<x> = <y>" or whatever it was briefer C99 (I think) syntax across the
board, it would be less verbose. But I haven't tested if it's as widely
supported, so I've just been sticking with that blank/memcpy() pattern
for "do init in terms of macro".

I do at least prefer memcpy() over *<x> = <y> when x and y are
structures. But I wasn't aware that this was common in our codebase.
Anyway, my suggestion was only along the lines of "you're already
writing individual fields below, so why not just do that throughout
instead of memcpy()-ing some of them via a macro which expands to a
designated initializer?"

But this is a cosmetic point, so whatever you feel fits in most with the
surrounding style (so long as the pattern we're propagating isn't
terrible, which is the case here) then I'm OK with it.

Thanks,
Taylor

On Tue, Nov 09, 2021 at 03:10:31AM +0100, Ævar Arnfjörð Bjarmason wrote:

quoted

All of that is to say that I share your motivation for this patch and
think that the direction is good, but I would have preferred to do it
without the caller_priv variable (unless there is something that I am
missing here).

Yes, that would make much more sense. I just had tunnel vision while
writing this, evidently. Yeah, just either passing it or using a static
variable in grep.c would make more sense.

Arguably we could even have it be read only (const char *const) and have
a global "the_prefix" or something, but it's probably not widely used
enough for that to make sense.

Either of the first two seem fine to me, but introducing `the_prefix`
seems both over-specified, and too big a step for this series alone.

Thanks,
Taylor

This series changes the behavior of an obscure interaction between
"grep.extendedRegexp=true" and "grep.patternType=default". See
7-8/8. Along the way we can delete a lot of code that was needed to
support the previous behavior.

For v1 and a more extensive summary see [1]. Thanks a lot Taylor for
the detailed review on v1!

Hopefully this v1 addresses all the feedback on one way or another,
it's still 8 patches, but much of the early part of v1 is squashed
together & re-done as suggested.

Then there's a mid-series de-duplication of the grep config
documentation. I ended up keeping the change to not needlessly pass
"cb" around in grep_cmd_config(), but that's now also in its own
patch.

1. https://lore.kernel.org/git/cover-0.8-00000000000-20211106T210711Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (8):
  grep.h: remove unused "regex_t regexp" from grep_opt
  built-ins: trust the "prefix" from run_builtin()
  log tests: check if grep_config() is called by "log"-like cmds
  grep docs: de-duplicate configuration sections
  grep.c: don't pass along NULL callback value
  grep API: call grep_config() after grep_init()
  grep: simplify config parsing, change grep.<rx config> interaction
  grep: make "extendedRegexp=true" the same as "patternType=extended"

 Documentation/config/grep.txt |  11 ++--
 Documentation/git-grep.txt    |  30 +--------
 builtin/grep.c                |  27 ++++----
 builtin/log.c                 |  13 +++-
 builtin/ls-tree.c             |   9 ++-
 git.c                         |   4 +-
 grep.c                        | 118 ++++------------------------------
 grep.h                        |  35 ++++++----
 revision.c                    |   4 +-
 t/t4202-log.sh                |  16 +++++
 t/t7810-grep.sh               |   4 +-
 11 files changed, 97 insertions(+), 174 deletions(-)

Range-diff against v1:
1:  412b8b65266 = 1:  1435db727ef grep.h: remove unused "regex_t regexp" from grep_opt
2:  244715e3497 < -:  ----------- git.c & grep.c: assert that "prefix" is NULL or non-zero string
3:  3338cc95b81 < -:  ----------- grep: remove unused "prefix_length" member
4:  78298657d69 < -:  ----------- grep.c: move "prefix" out of "struct grep_opt"
-:  ----------- > 2:  63cf2fe266d built-ins: trust the "prefix" from run_builtin()
5:  ba9be0b9283 = 3:  41e38ebb32c log tests: check if grep_config() is called by "log"-like cmds
-:  ----------- > 4:  efe95397d72 grep docs: de-duplicate configuration sections
-:  ----------- > 5:  d0f0ac6c7ae grep.c: don't pass along NULL callback value
6:  933ac853bca ! 6:  917944f79a5 grep API: call grep_config() after grep_init()
    @@ Commit message
         didn't do that, but now that it can't be a concern anymore let's
         remove those comments.
     
    +    This does not change the behavior of any of the configuration
    +    variables or options. That would have been the case if we didn't move
    +    around the grep_config() call in "builtin/log.c". But now that we call
    +    "grep_config" after "git_log_config" and "git_format_config" we'll
    +    need to pass in the already initialized "struct grep_opt *".
    +
         See 6ba9bb76e02 (grep: copy struct in one fell swoop, 2020-11-29) and
         7687a0541e0 (grep: move the configuration parsing logic to grep.[ch],
         2012-10-09) for the commits that added the comments.
     
    +    The memcpy() pattern here will be optimized away and follows the
    +    convention of other *_init() functions. See 5726a6b4012 (*.c *_init():
    +    define in terms of corresponding *_INIT macro, 2021-07-01).
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
     
      ## builtin/grep.c ##
     @@ builtin/grep.c: static int wait_all(void)
    + 
      static int grep_cmd_config(const char *var, const char *value, void *cb)
      {
    - 	int st = grep_config(var, value, cb);
    --	if (git_color_default_config(var, value, cb) < 0)
    -+	if (git_color_default_config(var, value, NULL) < 0)
    +-	int st = grep_config(var, value, NULL);
    ++	int st = grep_config(var, value, cb);
    + 	if (git_color_default_config(var, value, NULL) < 0)
      		st = -1;
      
    - 	if (!strcmp(var, "grep.threads")) {
     @@ builtin/grep.c: int cmd_grep(int argc, const char **argv, const char *prefix)
    - 		OPT_END()
      	};
    + 	grep_prefix = prefix;
      
     -	git_config(grep_cmd_config, NULL);
      	grep_init(&opt, the_repository);
     +	git_config(grep_cmd_config, &opt);
    - 	opt.caller_priv = &opt_cmd;
      
      	/*
    + 	 * If there is no -- then the paths must exist in the working
     
      ## builtin/log.c ##
     @@ builtin/log.c: static int git_log_config(const char *var, const char *value, void *cb)
    @@ grep.c: int grep_config(const char *var, const char *value, void *cb)
     
      ## grep.h ##
     @@ grep.h: struct grep_opt {
    + 	int show_hunk_mark;
    + 	int file_break;
    + 	int heading;
    ++	void *caller_priv;
    + 	void *priv;
    + 
    + 	void (*output)(struct grep_opt *opt, const void *data, size_t size);
      	void *output_priv;
      };
      
7:  677a8f8520f ! 7:  140a7416223 grep: simplify config parsing, change grep.<rx config> interaction
    @@ Commit message
         but in a way that's consistent with how we parse other
         configuration.
     
    -    Pedantically speaking we're probably breaking past promises here, but
    -    I doubt that this will impact anyone in practice. The reduction in
    -    complexity and resulting consistency with other default config
    -    behavior is worth it.
    +    We are breaking past promises here, but I doubt that this will impact
    +    anyone in practice. The reduction in complexity and resulting
    +    consistency with other default config behavior is worth it.
     
         When "grep.patternType" was introduced in 84befcd0a4a (grep: add a
         grep.patternType configuration setting, 2012-08-03) we made two
    @@ Commit message
             # BRE grep
             git -c grep.extendedRegexp=true -c grep.patternType=basic grep <pattern>
     
    -    But probably not for this to ignore the new "grep.patternType" option
    -    entirely, say if /etc/gitconfig was still setting
    +    But probably not for this to ignore the favored "grep.patternType"
    +    option entirely, say if /etc/gitconfig was still setting
         "grep.extendedRegexp", but "~/.gitconfig" used the new
         "grep.patternType" (and wanted to use the "default" value):
     
    @@ Documentation/config/grep.txt: grep.patternType::
      	If set to true, enable `--extended-regexp` option by default. This
     -	option is ignored when the `grep.patternType` option is set to a value
     -	other than 'default'.
    -+	option is ignored when the `grep.patternType` option is set.
    - 
    - grep.threads::
    - 	Number of grep worker threads to use.
    -
    - ## Documentation/git-grep.txt ##
    -@@ Documentation/git-grep.txt: grep.patternType::
    - 
    - grep.extendedRegexp::
    - 	If set to true, enable `--extended-regexp` option by default. This
    --	option is ignored when the `grep.patternType` option is set to a value
    --	other than 'default'.
     +	option is ignored when the `grep.patternType` option is set.
      
      grep.threads::
8:  dadd5dff77a ! 8:  cc904d93b26 grep: make "extendedRegexp=true" the same as "patternType=extended"
    @@ Documentation/config/grep.txt: grep.patternType::
      grep.extendedRegexp::
     -	If set to true, enable `--extended-regexp` option by default. This
     -	option is ignored when the `grep.patternType` option is set.
    -+	Deprecated synonym for 'grep.patternType=extended`.
    - 
    - grep.threads::
    - 	Number of grep worker threads to use.
    -
    - ## Documentation/git-grep.txt ##
    -@@ Documentation/git-grep.txt: grep.patternType::
    - 	value 'default' will return to the default matching behavior.
    - 
    - grep.extendedRegexp::
    --	If set to true, enable `--extended-regexp` option by default. This
    --	option is ignored when the `grep.patternType` option is set.
     +	Deprecated synonym for 'grep.patternType=extended`.
      
      grep.threads::
    @@ grep.h: struct grep_opt {
      	.colors = { \
      		[GREP_COLOR_CONTEXT] = "", \
      		[GREP_COLOR_FILENAME] = "", \
    -@@ grep.h: struct grep_opt {
    - 		[GREP_COLOR_SELECTED] = "", \
    - 		[GREP_COLOR_SEP] = GIT_COLOR_CYAN, \
    - 	}, \
    --	.only_matching = 0, \
    - 	.color = -1, \
    - 	.output = std_output, \
    - }
     
      ## t/t7810-grep.sh ##
     @@ t/t7810-grep.sh: do
-- 
2.34.0.rc1.741.gab7bfd97031

This "regex_t" in grep_opt has not been used since
f9b9faf6f8a (builtin-grep: allow more than one patterns., 2006-05-02),
we still use a "regex_t" for compiling regexes, but that's in the
"grep_pat" struct".

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 grep.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/grep.h b/grep.h
index 3e8815c347b..95cccb670f9 100644
--- a/grep.h
+++ b/grep.h

@@ -136,7 +136,6 @@ struct grep_opt {
 
 	const char *prefix;
 	int prefix_length;
-	regex_t regexp;
 	int linenum;
 	int columnnum;
 	int invert;