Re: [PATCH v2 6/8] grep API: call grep_config() after grep_init()

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 12, 2021, 17:32 UTC
Message-ID: <xmqq7ddds2id.fsf@gitster.g>
In-Reply-To: <patch-v2-6.8-917944f79a5-20211110T013632Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 14 quoted lines

> 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.

Show 10 quoted lines

> 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 ...

Show 9 quoted lines

>  	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.

Show 9 quoted lines

>  	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.

Show 9 quoted lines

> 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?

Show 26 quoted lines

>  	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.