Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> 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.
OK, so we did this because we wanted to be able to
1. call grep_config() only once to populate the template;
2. call grep_init() more than once, and match the grep_opt to
what the config wanted, without having to call grep_config()
once per grep_init() invocation.
3. each invocation of grep_init() in 2. may be followed by
parse_options() to further tweak grep_opt.
And now we instead have to do
1. call grep_init()
2. call grep_config()
3. parse_options() to tweak
for each instance of grep_opt, which is much more common.
OK.
> 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;
This used to tweak the "default template", which we no longer use,
so can go? And in its place ...
> 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);
> +
... each command in the "log" family tweaks the grep_opt used for
real from the configuration.
> 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);
> +
Ditto. OK, the new pattern makes sense.
> diff --git a/grep.h b/grep.h
> index 62deadb885f..30a7dfd3294 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -171,12 +171,34 @@ struct grep_opt {
> int show_hunk_mark;
> int file_break;
> int heading;
> + void *caller_priv;
This is unrelated and unexplained change, isn't it?
> void *priv;
>
> void (*output)(struct grep_opt *opt, const void *data, size_t size);
> 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, \
> +}
Other than the mysterious caller_priv bit, the change makes sense to
me.
Thanks.
